Move p2p host validation to CLI validation (#8158)

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java

@@ -212,10 +212,8 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.math.BigInteger;
-import java.net.SocketException;
 import java.net.URI;
 import java.net.URL;
-import java.net.UnknownHostException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.attribute.GroupPrincipal;
@@ -1433,7 +1431,7 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
 
   private void validateOptions() {
     issueOptionWarnings();
-    validateP2PInterface(p2PDiscoveryOptions.p2pInterface);
+    validateP2POptions();
     validateMiningParams();
     validateNatParams();
     validateNetStatsParams();
@@ -1488,20 +1486,18 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
         commandLine, genesisConfigOptionsSupplier.get(), isMergeEnabled(), logger);
   }
 
+  private void validateP2POptions() {
+    p2PDiscoveryOptions.validate(commandLine, getNetworkInterfaceChecker());
+  }
+
   /**
-   * Validates P2P interface IP address/host name. Visible for testing.
+   * Returns a network interface checker that can be used to validate P2P options.
    *
-   * @param p2pInterface IP Address/host name
+   * @return A {@link P2PDiscoveryOptions.NetworkInterfaceChecker} that checks if a network
+   *     interface is available.
    */
-  protected void validateP2PInterface(final String p2pInterface) {
-    final String failMessage = "The provided --p2p-interface is not available: " + p2pInterface;
-    try {
-      if (!NetworkUtility.isNetworkInterfaceAvailable(p2pInterface)) {
-        throw new ParameterException(commandLine, failMessage);
-      }
-    } catch (final UnknownHostException | SocketException e) {
-      throw new ParameterException(commandLine, failMessage, e);
-    }
+  protected P2PDiscoveryOptions.NetworkInterfaceChecker getNetworkInterfaceChecker() {
+    return NetworkUtility::isNetworkInterfaceAvailable;
   }
 
   private void validateGraphQlOptions() {

besu/src/main/java/org/hyperledger/besu/cli/options/P2PDiscoveryOptions.java

@@ -25,12 +25,15 @@ import org.hyperledger.besu.util.number.Fraction;
 import org.hyperledger.besu.util.number.Percentage;
 
 import java.net.InetAddress;
+import java.net.SocketException;
+import java.net.UnknownHostException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.Optional;
 import java.util.stream.Collectors;
 
+import com.google.common.net.InetAddresses;
 import org.apache.commons.net.util.SubnetUtils;
 import org.apache.tuweni.bytes.Bytes;
 import picocli.CommandLine;
@@ -38,6 +41,22 @@ import picocli.CommandLine;
 /** Command line options for configuring P2P discovery on the node. */
 public class P2PDiscoveryOptions implements CLIOptions<P2PDiscoveryConfiguration> {
 
+  /** Functional interface for checking if a network interface is available. */
+  @FunctionalInterface
+  public interface NetworkInterfaceChecker {
+
+    /**
+     * Checks if the provided network interface is available.
+     *
+     * @param p2pInterface The network interface to check.
+     * @return True if the network interface is available, false otherwise.
+     * @throws UnknownHostException If the host is unknown.
+     * @throws SocketException If there is an error with the socket.
+     */
+    boolean isNetworkInterfaceAvailable(final String p2pInterface)
+        throws UnknownHostException, SocketException;
+  }
+
   /** Default constructor */
   public P2PDiscoveryOptions() {}
 
@@ -217,6 +236,37 @@ public class P2PDiscoveryOptions implements CLIOptions<P2PDiscoveryConfiguration
         discoveryDnsUrl);
   }
 
+  /**
+   * Validates the provided P2P discovery options.
+   *
+   * @param commandLine The command line object used for parsing and error reporting.
+   * @param networkInterfaceChecker The checker used to validate the network interface.
+   */
+  public void validate(
+      final CommandLine commandLine, final NetworkInterfaceChecker networkInterfaceChecker) {
+    validateP2PHost(commandLine);
+    validateP2PInterface(commandLine, networkInterfaceChecker);
+  }
+
+  private void validateP2PInterface(
+      final CommandLine commandLine, final NetworkInterfaceChecker networkInterfaceChecker) {
+    final String failMessage = "The provided --p2p-interface is not available: " + p2pInterface;
+    try {
+      if (!networkInterfaceChecker.isNetworkInterfaceAvailable(p2pInterface)) {
+        throw new CommandLine.ParameterException(commandLine, failMessage);
+      }
+    } catch (final UnknownHostException | SocketException e) {
+      throw new CommandLine.ParameterException(commandLine, failMessage, e);
+    }
+  }
+
+  private void validateP2PHost(final CommandLine commandLine) {
+    final String failMessage = "The provided --p2p-host is invalid: " + p2pHost;
+    if (!InetAddresses.isInetAddress(p2pHost)) {
+      throw new CommandLine.ParameterException(commandLine, failMessage);
+    }
+  }
+
   @Override
   public List<String> getCLIOptions() {
     return CommandLineUtils.getCLIOptions(this, new P2PDiscoveryOptions());

besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java

@@ -1011,113 +1011,6 @@ public class BesuCommandTest extends CommandTestAbstract {
     assertThat(commandErrorOutput.toString(UTF_8)).startsWith(expectedErrorOutputStart);
   }
 
-  @Test
-  public void p2pHostAndPortOptionsAreRespectedAndNotLeaked() {
-
-    final String host = "1.2.3.4";
-    final int port = 1234;
-    parseCommand("--p2p-host", host, "--p2p-port", String.valueOf(port));
-
-    verify(mockRunnerBuilder).p2pAdvertisedHost(stringArgumentCaptor.capture());
-    verify(mockRunnerBuilder).p2pListenPort(intArgumentCaptor.capture());
-    verify(mockRunnerBuilder).metricsConfiguration(metricsConfigArgumentCaptor.capture());
-    verify(mockRunnerBuilder).jsonRpcConfiguration(jsonRpcConfigArgumentCaptor.capture());
-    verify(mockRunnerBuilder).build();
-
-    assertThat(stringArgumentCaptor.getValue()).isEqualTo(host);
-    assertThat(intArgumentCaptor.getValue()).isEqualTo(port);
-
-    assertThat(commandOutput.toString(UTF_8)).isEmpty();
-    assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
-
-    // all other port values remain default
-    assertThat(metricsConfigArgumentCaptor.getValue().getPort()).isEqualTo(9545);
-    assertThat(metricsConfigArgumentCaptor.getValue().getPushPort()).isEqualTo(9001);
-    assertThat(jsonRpcConfigArgumentCaptor.getValue().getPort()).isEqualTo(8545);
-
-    // all other host values remain default
-    final String defaultHost = "127.0.0.1";
-    assertThat(metricsConfigArgumentCaptor.getValue().getHost()).isEqualTo(defaultHost);
-    assertThat(metricsConfigArgumentCaptor.getValue().getPushHost()).isEqualTo(defaultHost);
-    assertThat(jsonRpcConfigArgumentCaptor.getValue().getHost()).isEqualTo(defaultHost);
-  }
-
-  @Test
-  public void p2pHostAndPortOptionsAreRespectedAndNotLeakedWithMetricsEnabled() {
-
-    final String host = "1.2.3.4";
-    final int port = 1234;
-    parseCommand("--p2p-host", host, "--p2p-port", String.valueOf(port), "--metrics-enabled");
-
-    verify(mockRunnerBuilder).p2pAdvertisedHost(stringArgumentCaptor.capture());
-    verify(mockRunnerBuilder).p2pListenPort(intArgumentCaptor.capture());
-    verify(mockRunnerBuilder).metricsConfiguration(metricsConfigArgumentCaptor.capture());
-    verify(mockRunnerBuilder).jsonRpcConfiguration(jsonRpcConfigArgumentCaptor.capture());
-    verify(mockRunnerBuilder).build();
-
-    assertThat(stringArgumentCaptor.getValue()).isEqualTo(host);
-    assertThat(intArgumentCaptor.getValue()).isEqualTo(port);
-
-    assertThat(commandOutput.toString(UTF_8)).isEmpty();
-    assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
-
-    // all other port values remain default
-    assertThat(metricsConfigArgumentCaptor.getValue().getPort()).isEqualTo(9545);
-    assertThat(metricsConfigArgumentCaptor.getValue().getPushPort()).isEqualTo(9001);
-    assertThat(jsonRpcConfigArgumentCaptor.getValue().getPort()).isEqualTo(8545);
-
-    // all other host values remain default
-    final String defaultHost = "127.0.0.1";
-    assertThat(metricsConfigArgumentCaptor.getValue().getHost()).isEqualTo(defaultHost);
-    assertThat(metricsConfigArgumentCaptor.getValue().getPushHost()).isEqualTo(defaultHost);
-    assertThat(jsonRpcConfigArgumentCaptor.getValue().getHost()).isEqualTo(defaultHost);
-  }
-
-  @Test
-  public void p2pInterfaceOptionIsRespected() {
-
-    final String ip = "1.2.3.4";
-    parseCommand("--p2p-interface", ip);
-
-    assertThat(commandOutput.toString(UTF_8)).isEmpty();
-    assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
-
-    verify(mockRunnerBuilder).p2pListenInterface(stringArgumentCaptor.capture());
-    verify(mockRunnerBuilder).build();
-
-    assertThat(stringArgumentCaptor.getValue()).isEqualTo(ip);
-  }
-
-  @Test
-  public void p2pHostMayBeLocalhost() {
-
-    final String host = "localhost";
-    parseCommand("--p2p-host", host);
-
-    verify(mockRunnerBuilder).p2pAdvertisedHost(stringArgumentCaptor.capture());
-    verify(mockRunnerBuilder).build();
-
-    assertThat(stringArgumentCaptor.getValue()).isEqualTo(host);
-
-    assertThat(commandOutput.toString(UTF_8)).isEmpty();
-    assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
-  }
-
-  @Test
-  public void p2pHostMayBeIPv6() {
-
-    final String host = "2600:DB8::8545";
-    parseCommand("--p2p-host", host);
-
-    verify(mockRunnerBuilder).p2pAdvertisedHost(stringArgumentCaptor.capture());
-    verify(mockRunnerBuilder).build();
-
-    assertThat(stringArgumentCaptor.getValue()).isEqualTo(host);
-
-    assertThat(commandOutput.toString(UTF_8)).isEmpty();
-    assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
-  }
-
   @Test
   public void maxpeersOptionMustBeUsed() {
 

besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java

@@ -38,6 +38,7 @@ import org.hyperledger.besu.cli.options.EthProtocolOptions;
 import org.hyperledger.besu.cli.options.EthstatsOptions;
 import org.hyperledger.besu.cli.options.MiningOptions;
 import org.hyperledger.besu.cli.options.NetworkingOptions;
+import org.hyperledger.besu.cli.options.P2PDiscoveryOptions;
 import org.hyperledger.besu.cli.options.SynchronizerOptions;
 import org.hyperledger.besu.cli.options.TransactionPoolOptions;
 import org.hyperledger.besu.cli.options.storage.DataStorageOptions;
@@ -568,8 +569,9 @@ public abstract class CommandTestAbstract {
     }
 
     @Override
-    protected void validateP2PInterface(final String p2pInterface) {
+    protected P2PDiscoveryOptions.NetworkInterfaceChecker getNetworkInterfaceChecker() {
       // For testing, don't actually query for networking interfaces to validate this option
+      return (networkInterface) -> true;
     }
 
     @Override

besu/src/test/java/org/hyperledger/besu/cli/options/P2PDiscoveryOptionsTest.java

@@ -0,0 +1,133 @@
+/*
+ * Copyright contributors to Besu.
+ *
+ * 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.cli.options;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.verify;
+
+import org.hyperledger.besu.cli.CommandTestAbstract;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+public class P2PDiscoveryOptionsTest extends CommandTestAbstract {
+
+  @Test
+  public void p2pHostAndPortOptionsAreRespectedAndNotLeakedWithMetricsEnabled() {
+
+    final String host = "1.2.3.4";
+    final int port = 1234;
+    parseCommand("--p2p-host", host, "--p2p-port", String.valueOf(port), "--metrics-enabled");
+
+    verify(mockRunnerBuilder).p2pAdvertisedHost(stringArgumentCaptor.capture());
+    verify(mockRunnerBuilder).p2pListenPort(intArgumentCaptor.capture());
+    verify(mockRunnerBuilder).metricsConfiguration(metricsConfigArgumentCaptor.capture());
+    verify(mockRunnerBuilder).jsonRpcConfiguration(jsonRpcConfigArgumentCaptor.capture());
+    verify(mockRunnerBuilder).build();
+
+    assertThat(stringArgumentCaptor.getValue()).isEqualTo(host);
+    assertThat(intArgumentCaptor.getValue()).isEqualTo(port);
+
+    assertThat(commandOutput.toString(UTF_8)).isEmpty();
+    assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
+
+    // all other port values remain default
+    assertThat(metricsConfigArgumentCaptor.getValue().getPort()).isEqualTo(9545);
+    assertThat(metricsConfigArgumentCaptor.getValue().getPushPort()).isEqualTo(9001);
+    assertThat(jsonRpcConfigArgumentCaptor.getValue().getPort()).isEqualTo(8545);
+
+    // all other host values remain default
+    final String defaultHost = "127.0.0.1";
+    assertThat(metricsConfigArgumentCaptor.getValue().getHost()).isEqualTo(defaultHost);
+    assertThat(metricsConfigArgumentCaptor.getValue().getPushHost()).isEqualTo(defaultHost);
+    assertThat(jsonRpcConfigArgumentCaptor.getValue().getHost()).isEqualTo(defaultHost);
+  }
+
+  @Test
+  public void p2pInterfaceOptionIsRespected() {
+
+    final String ip = "1.2.3.4";
+    parseCommand("--p2p-interface", ip);
+
+    assertThat(commandOutput.toString(UTF_8)).isEmpty();
+    assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
+
+    verify(mockRunnerBuilder).p2pListenInterface(stringArgumentCaptor.capture());
+    verify(mockRunnerBuilder).build();
+
+    assertThat(stringArgumentCaptor.getValue()).isEqualTo(ip);
+  }
+
+  @ParameterizedTest
+  @ValueSource(strings = {"localhost", " localhost.localdomain", "invalid-host"})
+  public void p2pHostMustBeAnIPAddress(final String host) {
+    parseCommand("--p2p-host", host);
+
+    assertThat(commandOutput.toString(UTF_8)).isEmpty();
+    String errorMessage = "The provided --p2p-host is invalid: " + host;
+    assertThat(commandErrorOutput.toString(UTF_8)).contains(errorMessage);
+  }
+
+  @Test
+  public void p2pHostMayBeIPv6() {
+
+    final String host = "2600:DB8::8545";
+    parseCommand("--p2p-host", host);
+
+    verify(mockRunnerBuilder).p2pAdvertisedHost(stringArgumentCaptor.capture());
+    verify(mockRunnerBuilder).build();
+
+    assertThat(stringArgumentCaptor.getValue()).isEqualTo(host);
+
+    assertThat(commandOutput.toString(UTF_8)).isEmpty();
+    assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
+  }
+
+  @Test
+  public void p2pHostAndPortOptionsAreRespectedAndNotLeaked() {
+
+    final String host = "1.2.3.4";
+    final int port = 1234;
+    parseCommand("--p2p-host", host, "--p2p-port", String.valueOf(port));
+
+    verify(mockRunnerBuilder).p2pAdvertisedHost(stringArgumentCaptor.capture());
+    verify(mockRunnerBuilder).p2pListenPort(intArgumentCaptor.capture());
+    verify(mockRunnerBuilder).metricsConfiguration(metricsConfigArgumentCaptor.capture());
+    verify(mockRunnerBuilder).jsonRpcConfiguration(jsonRpcConfigArgumentCaptor.capture());
+    verify(mockRunnerBuilder).build();
+
+    assertThat(stringArgumentCaptor.getValue()).isEqualTo(host);
+    assertThat(intArgumentCaptor.getValue()).isEqualTo(port);
+
+    assertThat(commandOutput.toString(UTF_8)).isEmpty();
+    assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
+
+    // all other port values remain default
+    assertThat(metricsConfigArgumentCaptor.getValue().getPort()).isEqualTo(9545);
+    assertThat(metricsConfigArgumentCaptor.getValue().getPushPort()).isEqualTo(9001);
+    assertThat(jsonRpcConfigArgumentCaptor.getValue().getPort()).isEqualTo(8545);
+
+    // all other host values remain default
+    final String defaultHost = "127.0.0.1";
+    assertThat(metricsConfigArgumentCaptor.getValue().getHost()).isEqualTo(defaultHost);
+    assertThat(metricsConfigArgumentCaptor.getValue().getPushHost()).isEqualTo(defaultHost);
+    assertThat(jsonRpcConfigArgumentCaptor.getValue().getHost()).isEqualTo(defaultHost);
+  }
+}