[BESU-531] Implement fallback method for Nat (#692)

* Log to trace denied because of host whitelisting (#663)

When we deny a connection based on HTTP hostname log to trace the
rejected value.

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
Co-authored-by: Usman Saleem <usman@usmans.info>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>

* implement fallback for nat

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>

* fix unit test

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>

* spotlessApply

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>

* clean documentation

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>

Co-authored-by: Danno Ferrin <danno.ferrin@gmail.com>
Co-authored-by: Usman Saleem <usman@usmans.info>
Co-authored-by: Abdelhamid Bakhta <45264458+abdelhamidbakhta@users.noreply.github.com>

nat/src/main/java/org/hyperledger/besu/nat/NatService.java

@@ -33,8 +33,8 @@ public class NatService {
 
   protected static final Logger LOG = LogManager.getLogger();
 
-  private final NatMethod currentNatMethod;
-  private final Optional<NatManager> currentNatManager;
+  private NatMethod currentNatMethod;
+  private Optional<NatManager> currentNatManager;
 
   public NatService(final Optional<NatManager> natManager) {
     this.currentNatMethod = retrieveNatMethod(natManager);
@@ -87,7 +87,8 @@ public class NatService {
       try {
         getNatManager().orElseThrow().start();
       } catch (Exception e) {
-        LOG.warn("Caught exception while trying to start the manager or service.", e);
+        LOG.debug("Caught exception while trying to start the manager or service.", e);
+        disableNatManager();
       }
     } else {
       LOG.info("No NAT environment detected so no service could be started");
@@ -110,46 +111,57 @@ public class NatService {
   /**
    * Returns a {@link Optional} wrapping the advertised IP address.
    *
+   * @param fallbackValue the advertised IP address fallback value
    * @return The advertised IP address wrapped in a {@link Optional}. Empty if
    *     `isNatExternalIpUsageEnabled` is false
    */
-  public Optional<String> queryExternalIPAddress() {
+  public String queryExternalIPAddress(final String fallbackValue) {
     if (isNatEnvironment()) {
       try {
         final NatManager natManager = getNatManager().orElseThrow();
-        LOG.info(
+        LOG.debug(
             "Waiting for up to {} seconds to detect external IP address...",
             NatManager.TIMEOUT_SECONDS);
-        return Optional.of(
-            natManager.queryExternalIPAddress().get(NatManager.TIMEOUT_SECONDS, TimeUnit.SECONDS));
+        return Optional.ofNullable(
+                natManager
+                    .queryExternalIPAddress()
+                    .get(NatManager.TIMEOUT_SECONDS, TimeUnit.SECONDS))
+            .orElseThrow();
 
       } catch (Exception e) {
         LOG.warn(
-            "Caught exception while trying to query NAT external IP address (ignoring): {}", e);
+            "Caught exception while trying to query NAT external IP address (ignoring). Using the fallback value : {} ",
+            fallbackValue,
+            e);
       }
     }
-    return Optional.empty();
+    return fallbackValue;
   }
 
   /**
    * Returns a {@link Optional} wrapping the local IP address.
    *
+   * @param fallbackValue the advertised IP address fallback value
    * @return The local IP address wrapped in a {@link Optional}.
    */
-  public Optional<String> queryLocalIPAddress() throws RuntimeException {
+  public String queryLocalIPAddress(final String fallbackValue) throws RuntimeException {
     if (isNatEnvironment()) {
       try {
         final NatManager natManager = getNatManager().orElseThrow();
-        LOG.info(
-            "Waiting for up to {} seconds to detect external IP address...",
+        LOG.debug(
+            "Waiting for up to {} seconds to detect local IP address...",
             NatManager.TIMEOUT_SECONDS);
-        return Optional.of(
-            natManager.queryLocalIPAddress().get(NatManager.TIMEOUT_SECONDS, TimeUnit.SECONDS));
+        return Optional.ofNullable(
+                natManager.queryLocalIPAddress().get(NatManager.TIMEOUT_SECONDS, TimeUnit.SECONDS))
+            .orElseThrow();
       } catch (Exception e) {
-        LOG.warn("Caught exception while trying to query local IP address (ignoring): {}", e);
+        LOG.warn(
+            "Caught exception while trying to query local IP address (ignoring). Using the fallback value : {} ",
+            fallbackValue,
+            e);
       }
     }
-    return Optional.empty();
+    return fallbackValue;
   }
 
   /**
@@ -172,6 +184,13 @@ public class NatService {
     return Optional.empty();
   }
 
+  /** Disable the natManager */
+  private void disableNatManager() {
+    LOG.warn("Unable to use NAT. Disabling NAT manager");
+    currentNatMethod = NatMethod.NONE;
+    currentNatManager = Optional.empty();
+  }
+
   /**
    * Retrieve the current NatMethod.
    *

nat/src/main/java/org/hyperledger/besu/nat/core/AbstractNatManager.java

@@ -20,6 +20,7 @@ import org.hyperledger.besu.nat.NatMethod;
 import org.hyperledger.besu.nat.core.domain.NatPortMapping;
 import org.hyperledger.besu.nat.core.domain.NatServiceType;
 import org.hyperledger.besu.nat.core.domain.NetworkProtocol;
+import org.hyperledger.besu.nat.core.exception.NatInitializationException;
 
 import java.net.InetAddress;
 import java.net.UnknownHostException;
@@ -46,7 +47,7 @@ public abstract class AbstractNatManager implements NatManager {
     this.natMethod = natMethod;
   }
 
-  protected abstract void doStart();
+  protected abstract void doStart() throws NatInitializationException;
 
   protected abstract void doStop();
 
@@ -84,7 +85,7 @@ public abstract class AbstractNatManager implements NatManager {
   }
 
   @Override
-  public void start() {
+  public void start() throws NatInitializationException {
     if (started.compareAndSet(false, true)) {
       doStart();
     } else {

nat/src/main/java/org/hyperledger/besu/nat/core/NatManager.java

@@ -18,6 +18,7 @@ import org.hyperledger.besu.nat.NatMethod;
 import org.hyperledger.besu.nat.core.domain.NatPortMapping;
 import org.hyperledger.besu.nat.core.domain.NatServiceType;
 import org.hyperledger.besu.nat.core.domain.NetworkProtocol;
+import org.hyperledger.besu.nat.core.exception.NatInitializationException;
 
 import java.util.List;
 import java.util.concurrent.CompletableFuture;
@@ -37,8 +38,12 @@ public interface NatManager {
    */
   NatMethod getNatMethod();
 
-  /** Starts the manager or service. */
-  void start();
+  /**
+   * Starts the manager or service.
+   *
+   * @throws NatInitializationException if failure during the initialization
+   */
+  void start() throws NatInitializationException;
 
   /** Stops the manager or service. */
   void stop();

nat/src/main/java/org/hyperledger/besu/nat/core/exception/NatInitializationException.java

@@ -0,0 +1,26 @@
+/*
+ * Copyright ConsenSys AG.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+package org.hyperledger.besu.nat.core.exception;
+
+public class NatInitializationException extends Exception {
+
+  public NatInitializationException(final String message) {
+    super(message);
+  }
+
+  public NatInitializationException(final String message, final Throwable cause) {
+    super(message, cause);
+  }
+}

nat/src/main/java/org/hyperledger/besu/nat/kubernetes/KubernetesNatManager.java

@@ -20,6 +20,7 @@ import org.hyperledger.besu.nat.core.AbstractNatManager;
 import org.hyperledger.besu.nat.core.domain.NatPortMapping;
 import org.hyperledger.besu.nat.core.domain.NatServiceType;
 import org.hyperledger.besu.nat.core.domain.NetworkProtocol;
+import org.hyperledger.besu.nat.core.exception.NatInitializationException;
 
 import java.nio.charset.Charset;
 import java.nio.file.Files;
@@ -59,12 +60,8 @@ public class KubernetesNatManager extends AbstractNatManager {
   }
 
   @Override
-  protected void doStart() {
+  protected void doStart() throws NatInitializationException {
     LOG.info("Starting kubernetes NAT manager.");
-    update();
-  }
-
-  private void update() {
     try {
       LOG.debug("Trying to update information using Kubernetes client SDK.");
       final String kubeConfigPath =
@@ -72,7 +69,7 @@ public class KubernetesNatManager extends AbstractNatManager {
       LOG.debug(
           "Checking if Kubernetes config file is present on file system: {}.", kubeConfigPath);
       if (!Files.exists(Paths.get(kubeConfigPath))) {
-        throw new IllegalStateException("Cannot locate Kubernetes config file.");
+        throw new NatInitializationException("Cannot locate Kubernetes config file.");
       }
       // loading the out-of-cluster config, a kubeconfig from file-system
       final ApiClient client =
@@ -95,12 +92,13 @@ public class KubernetesNatManager extends AbstractNatManager {
           .ifPresent(this::updateUsingBesuService);
 
     } catch (Exception e) {
-      LOG.warn("Failed update information using Kubernetes client SDK.", e);
+      throw new NatInitializationException(
+          "Failed update information using Kubernetes client SDK.", e);
     }
   }
 
   @VisibleForTesting
-  void updateUsingBesuService(final V1Service service) {
+  void updateUsingBesuService(final V1Service service) throws RuntimeException {
     try {
       LOG.info("Found Besu service: {}", service.getMetadata().getName());
       LOG.info("Setting host IP to: {}.", service.getSpec().getClusterIP());
@@ -129,7 +127,7 @@ public class KubernetesNatManager extends AbstractNatManager {
                 }
               });
     } catch (Exception e) {
-      LOG.warn("Failed update information using pod metadata.", e);
+      throw new RuntimeException("Failed update information using pod metadata.", e);
     }
   }
 

nat/src/main/java/org/hyperledger/besu/nat/upnp/UpnpNatManager.java

@@ -22,6 +22,7 @@ import org.hyperledger.besu.nat.core.AbstractNatManager;
 import org.hyperledger.besu.nat.core.domain.NatPortMapping;
 import org.hyperledger.besu.nat.core.domain.NatServiceType;
 import org.hyperledger.besu.nat.core.domain.NetworkProtocol;
+import org.hyperledger.besu.nat.core.exception.NatInitializationException;
 
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -112,11 +113,15 @@ public class UpnpNatManager extends AbstractNatManager {
    * @throws IllegalStateException if already started.
    */
   @Override
-  public synchronized void doStart() {
+  public synchronized void doStart() throws NatInitializationException {
     LOG.info("Starting UPnP Service");
-    upnpService.startup();
-    upnpService.getRegistry().addListener(registryListener);
-    initiateExternalIpQuery();
+    try {
+      upnpService.startup();
+      upnpService.getRegistry().addListener(registryListener);
+      initiateExternalIpQuery();
+    } catch (Exception e) {
+      throw new NatInitializationException("Failed start UPnP nat service.", e);
+    }
   }
 
   /**

nat/src/test/java/org/hyperledger/besu/nat/NatServiceTest.java

@@ -16,6 +16,8 @@
 package org.hyperledger.besu.nat;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -24,6 +26,7 @@ import org.hyperledger.besu.nat.core.NatManager;
 import org.hyperledger.besu.nat.core.domain.NatPortMapping;
 import org.hyperledger.besu.nat.core.domain.NatServiceType;
 import org.hyperledger.besu.nat.core.domain.NetworkProtocol;
+import org.hyperledger.besu.nat.core.exception.NatInitializationException;
 import org.hyperledger.besu.nat.upnp.UpnpNatManager;
 
 import java.util.Optional;
@@ -95,6 +98,7 @@ public class NatServiceTest {
 
   @Test
   public void assertQueryExternalIpWorksProperlyWithUpNp() {
+    final String fallbackExternalIp = "127.0.0.1";
     final String externalIp = "127.0.0.3";
     final NatManager natManager = mock(NatManager.class);
     when(natManager.queryExternalIPAddress())
@@ -103,25 +107,28 @@ public class NatServiceTest {
 
     final NatService natService = new NatService(Optional.of(natManager));
 
-    final Optional<String> resultIp = natService.queryExternalIPAddress();
+    final String resultIp = natService.queryExternalIPAddress(fallbackExternalIp);
 
     verify(natManager).queryExternalIPAddress();
 
-    assertThat(resultIp).containsSame(externalIp);
+    assertThat(resultIp).isEqualTo(externalIp);
   }
 
   @Test
   public void assertThatQueryExternalIpWorksProperlyWithoutNat() {
 
+    final String fallbackExternalIp = "127.0.0.1";
+
     final NatService natService = new NatService(Optional.empty());
 
-    final Optional<String> resultIp = natService.queryExternalIPAddress();
+    final String resultIp = natService.queryExternalIPAddress(fallbackExternalIp);
 
-    assertThat(resultIp).isNotPresent();
+    assertThat(resultIp).isEqualTo(fallbackExternalIp);
   }
 
   @Test
   public void assertThatQueryLocalIPAddressWorksProperlyWithUpNp() {
+    final String fallbackExternalIp = "127.0.0.1";
     final String externalIp = "127.0.0.3";
     final NatManager natManager = mock(NatManager.class);
     when(natManager.queryLocalIPAddress())
@@ -130,21 +137,64 @@ public class NatServiceTest {
 
     final NatService natService = new NatService(Optional.of(natManager));
 
-    final Optional<String> resultIp = natService.queryLocalIPAddress();
+    final String resultIp = natService.queryLocalIPAddress(fallbackExternalIp);
 
     verify(natManager).queryLocalIPAddress();
 
-    assertThat(resultIp).containsSame(externalIp);
+    assertThat(resultIp).isEqualTo(externalIp);
   }
 
   @Test
   public void assertThatQueryLocalIPAddressWorksProperlyWithoutNat() {
 
+    final String fallbackValue = "1.2.3.4";
+
     final NatService natService = new NatService(Optional.empty());
 
-    final Optional<String> resultIp = natService.queryLocalIPAddress();
+    final String resultIp = natService.queryLocalIPAddress(fallbackValue);
 
-    assertThat(resultIp).isNotPresent();
+    assertThat(resultIp).isEqualTo(fallbackValue);
+  }
+
+  @Test
+  public void assertThatManagerSwitchToNoneForInvalidNatEnvironment()
+      throws NatInitializationException {
+
+    final String externalIp = "1.2.3.4";
+    final String localIp = "2.2.3.4";
+    final String fallbackExternalIp = "3.4.5.6";
+    final String fallbackLocalIp = "4.4.5.6";
+
+    final NatManager natManager = mock(NatManager.class);
+    doThrow(NatInitializationException.class).when(natManager).start();
+    when(natManager.queryExternalIPAddress())
+        .thenReturn(CompletableFuture.completedFuture(externalIp));
+    when(natManager.queryLocalIPAddress()).thenReturn(CompletableFuture.completedFuture(localIp));
+    when(natManager.getPortMapping(any(NatServiceType.class), any(NetworkProtocol.class)))
+        .thenReturn(
+            new NatPortMapping(
+                NatServiceType.DISCOVERY, NetworkProtocol.UDP, localIp, externalIp, 1111, 1111));
+    when(natManager.getNatMethod()).thenReturn(NatMethod.UPNP);
+
+    final NatService natService = new NatService(Optional.of(natManager));
+
+    assertThat(natService.getNatMethod()).isEqualTo(NatMethod.UPNP);
+    assertThat(natService.isNatEnvironment()).isTrue();
+    assertThat(natService.getNatManager()).contains(natManager);
+    assertThat(natService.getPortMapping(NatServiceType.DISCOVERY, NetworkProtocol.UDP))
+        .isPresent();
+    assertThat(natService.queryExternalIPAddress(fallbackExternalIp)).isEqualTo(externalIp);
+    assertThat(natService.queryLocalIPAddress(fallbackLocalIp)).isEqualTo(localIp);
+
+    natService.start();
+
+    assertThat(natService.getNatMethod()).isEqualTo(NatMethod.NONE);
+    assertThat(natService.isNatEnvironment()).isFalse();
+    assertThat(natService.getNatManager()).isNotPresent();
+    assertThat(natService.getPortMapping(NatServiceType.DISCOVERY, NetworkProtocol.UDP))
+        .isNotPresent();
+    assertThat(natService.queryExternalIPAddress(fallbackExternalIp)).isEqualTo(fallbackExternalIp);
+    assertThat(natService.queryLocalIPAddress(fallbackLocalIp)).isEqualTo(fallbackLocalIp);
   }
 
   @Test

nat/src/test/java/org/hyperledger/besu/nat/core/AbstractNatManagerTest.java

@@ -21,6 +21,7 @@ import static org.mockito.Mockito.verify;
 
 import org.hyperledger.besu.nat.NatMethod;
 import org.hyperledger.besu.nat.core.domain.NatPortMapping;
+import org.hyperledger.besu.nat.core.exception.NatInitializationException;
 
 import java.net.InetAddress;
 import java.net.UnknownHostException;
@@ -37,7 +38,7 @@ import org.mockito.junit.MockitoJUnitRunner;
 public class AbstractNatManagerTest {
 
   @Test
-  public void assertThatManagerIsStartedAfterStart() {
+  public void assertThatManagerIsStartedAfterStart() throws NatInitializationException {
     final AbstractNatManager natManager = buildNatManager(NatMethod.UPNP);
     assertThat(natManager.isStarted()).isFalse();
     natManager.start();
@@ -45,9 +46,10 @@ public class AbstractNatManagerTest {
   }
 
   @Test
-  public void assertThatManagerIsStoppedAfterStopped() {
+  public void assertThatManagerIsStoppedAfterStopped() throws NatInitializationException {
     final AbstractNatManager natManager = buildNatManager(NatMethod.UPNP);
     assertThat(natManager.isStarted()).isFalse();
+
     natManager.start();
     assertThat(natManager.isStarted()).isTrue();
     natManager.stop();
@@ -55,7 +57,7 @@ public class AbstractNatManagerTest {
   }
 
   @Test
-  public void assertThatDoStartIsCalledOnlyOnce() {
+  public void assertThatDoStartIsCalledOnlyOnce() throws NatInitializationException {
     final AbstractNatManager natManager = Mockito.spy(buildNatManager(NatMethod.UPNP));
     natManager.start();
     natManager.start();
@@ -64,7 +66,7 @@ public class AbstractNatManagerTest {
   }
 
   @Test
-  public void assertThatDoStopIsCalledOnlyOnce() {
+  public void assertThatDoStopIsCalledOnlyOnce() throws NatInitializationException {
     final AbstractNatManager natManager = Mockito.spy(buildNatManager(NatMethod.UPNP));
     natManager.start();
     natManager.stop();

nat/src/test/java/org/hyperledger/besu/nat/docker/DockerNatManagerTest.java

@@ -21,6 +21,7 @@ import static org.mockito.Mockito.when;
 import org.hyperledger.besu.nat.core.domain.NatPortMapping;
 import org.hyperledger.besu.nat.core.domain.NatServiceType;
 import org.hyperledger.besu.nat.core.domain.NetworkProtocol;
+import org.hyperledger.besu.nat.core.exception.NatInitializationException;
 
 import java.net.InetAddress;
 import java.net.UnknownHostException;
@@ -44,7 +45,7 @@ public final class DockerNatManagerTest {
   private DockerNatManager natManager;
 
   @Before
-  public void initialize() {
+  public void initialize() throws NatInitializationException {
     hostBasedIpDetector = mock(HostBasedIpDetector.class);
     when(hostBasedIpDetector.detectExternalIp()).thenReturn(Optional.of(detectedAdvertisedHost));
     natManager = new DockerNatManager(hostBasedIpDetector, advertisedHost, p2pPort, rpcHttpPort);

nat/src/test/java/org/hyperledger/besu/nat/manual/ManualNatManagerTest.java

@@ -20,6 +20,7 @@ import static org.assertj.core.api.Assertions.assertThat;
 import org.hyperledger.besu.nat.core.domain.NatPortMapping;
 import org.hyperledger.besu.nat.core.domain.NatServiceType;
 import org.hyperledger.besu.nat.core.domain.NetworkProtocol;
+import org.hyperledger.besu.nat.core.exception.NatInitializationException;
 
 import java.net.InetAddress;
 import java.net.UnknownHostException;
@@ -40,7 +41,7 @@ public class ManualNatManagerTest {
   private ManualNatManager natManager;
 
   @Before
-  public void initialize() {
+  public void initialize() throws NatInitializationException {
     natManager = new ManualNatManager(advertisedHost, p2pPort, rpcHttpPort);
     natManager.start();
   }

nat/src/test/java/org/hyperledger/besu/nat/upnp/UpnpNatManagerTest.java

@@ -24,6 +24,7 @@ import static org.mockito.Mockito.when;
 
 import org.hyperledger.besu.nat.core.domain.NatServiceType;
 import org.hyperledger.besu.nat.core.domain.NetworkProtocol;
+import org.hyperledger.besu.nat.core.exception.NatInitializationException;
 
 import java.net.InetAddress;
 import java.net.URI;
@@ -113,7 +114,7 @@ public final class UpnpNatManagerTest {
   }
 
   @Test
-  public void requestPortForwardThrowsWhenPortIsZero() {
+  public void requestPortForwardThrowsWhenPortIsZero() throws NatInitializationException {
     upnpManager.start();
 
     assertThatThrownBy(