From f3adc0c3fb91e67e96d80e4f23f9b7a169846714 Mon Sep 17 00:00:00 2001 From: garyschulte Date: Mon, 15 Nov 2021 14:42:39 -0800 Subject: [PATCH] Feature/keep peers on dns fail (#3057) * add behavior to not drop peers on failed lookup, add test coverage * add dns discovery server override cli flag Signed-off-by: garyschulte --- .../options/unstable/NetworkingOptions.java | 18 ++++- .../cli/options/NetworkingOptionsTest.java | 25 +++++++ .../p2p/config/NetworkingConfiguration.java | 12 +++ .../p2p/network/DefaultP2PNetwork.java | 72 +++++++++++------- .../p2p/network/DefaultP2PNetworkTest.java | 74 ++++++++++++++++--- 5 files changed, 164 insertions(+), 37 deletions(-) diff --git a/besu/src/main/java/org/hyperledger/besu/cli/options/unstable/NetworkingOptions.java b/besu/src/main/java/org/hyperledger/besu/cli/options/unstable/NetworkingOptions.java index 7e586f1fb8..ac6a973bb4 100644 --- a/besu/src/main/java/org/hyperledger/besu/cli/options/unstable/NetworkingOptions.java +++ b/besu/src/main/java/org/hyperledger/besu/cli/options/unstable/NetworkingOptions.java @@ -20,6 +20,7 @@ import org.hyperledger.besu.ethereum.p2p.config.NetworkingConfiguration; import java.util.Arrays; import java.util.List; +import java.util.Optional; import picocli.CommandLine; @@ -28,6 +29,7 @@ public class NetworkingOptions implements CLIOptions { "--Xp2p-initiate-connections-frequency"; private final String CHECK_MAINTAINED_CONNECTIONS_FREQUENCY_FLAG = "--Xp2p-check-maintained-connections-frequency"; + private final String DNS_DISCOVERY_SERVER_OVERRIDE_FLAG = "--Xp2p-dns-discovery-server"; @CommandLine.Option( names = INITIATE_CONNECTIONS_FREQUENCY_FLAG, @@ -49,6 +51,14 @@ public class NetworkingOptions implements CLIOptions { private int checkMaintainedConnectionsFrequencySec = NetworkingConfiguration.DEFAULT_CHECK_MAINTAINED_CONNECTIONS_FREQUENCY_SEC; + @CommandLine.Option( + names = DNS_DISCOVERY_SERVER_OVERRIDE_FLAG, + hidden = true, + defaultValue = "", + description = + "DNS server host to use for doing DNS Discovery of peers, rather than the machine's configured DNS server") + private String dnsDiscoveryServerOverride = null; + private NetworkingOptions() {} public static NetworkingOptions create() { @@ -61,6 +71,8 @@ public class NetworkingOptions implements CLIOptions { networkingConfig.getCheckMaintainedConnectionsFrequencySec(); cliOptions.initiateConnectionsFrequencySec = networkingConfig.getInitiateConnectionsFrequencySec(); + cliOptions.dnsDiscoveryServerOverride = + networkingConfig.getDnsDiscoveryServerOverride().orElse(""); return cliOptions; } @@ -69,6 +81,8 @@ public class NetworkingOptions implements CLIOptions { NetworkingConfiguration config = NetworkingConfiguration.create(); config.setCheckMaintainedConnectionsFrequency(checkMaintainedConnectionsFrequencySec); config.setInitiateConnectionsFrequency(initiateConnectionsFrequencySec); + config.setDnsDiscoveryServerOverride( + Optional.of(dnsDiscoveryServerOverride).filter(z -> !z.isBlank()).orElse(null)); return config; } @@ -78,6 +92,8 @@ public class NetworkingOptions implements CLIOptions { CHECK_MAINTAINED_CONNECTIONS_FREQUENCY_FLAG, OptionParser.format(checkMaintainedConnectionsFrequencySec), INITIATE_CONNECTIONS_FREQUENCY_FLAG, - OptionParser.format(initiateConnectionsFrequencySec)); + OptionParser.format(initiateConnectionsFrequencySec), + DNS_DISCOVERY_SERVER_OVERRIDE_FLAG, + dnsDiscoveryServerOverride); } } diff --git a/besu/src/test/java/org/hyperledger/besu/cli/options/NetworkingOptionsTest.java b/besu/src/test/java/org/hyperledger/besu/cli/options/NetworkingOptionsTest.java index 8b58776b8c..d97087f4d4 100644 --- a/besu/src/test/java/org/hyperledger/besu/cli/options/NetworkingOptionsTest.java +++ b/besu/src/test/java/org/hyperledger/besu/cli/options/NetworkingOptionsTest.java @@ -72,6 +72,31 @@ public class NetworkingOptionsTest assertThat(commandOutput.toString()).isEmpty(); } + @Test + public void checkDnsServerOverrideFlag_isSet() { + final TestBesuCommand cmd = parseCommand("--Xp2p-dns-discovery-server", "localhost"); + + final NetworkingOptions options = cmd.getNetworkingOptions(); + final NetworkingConfiguration networkingConfig = options.toDomainObject(); + assertThat(networkingConfig.getDnsDiscoveryServerOverride()).isPresent(); + assertThat(networkingConfig.getDnsDiscoveryServerOverride().get()).isEqualTo("localhost"); + + assertThat(commandErrorOutput.toString()).isEmpty(); + assertThat(commandOutput.toString()).isEmpty(); + } + + @Test + public void checkDnsServerOverrideFlag_isNotSet() { + final TestBesuCommand cmd = parseCommand(); + + final NetworkingOptions options = cmd.getNetworkingOptions(); + final NetworkingConfiguration networkingConfig = options.toDomainObject(); + assertThat(networkingConfig.getDnsDiscoveryServerOverride()).isEmpty(); + + assertThat(commandErrorOutput.toString()).isEmpty(); + assertThat(commandOutput.toString()).isEmpty(); + } + @Override NetworkingConfiguration createDefaultDomainObject() { return NetworkingConfiguration.create(); diff --git a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/config/NetworkingConfiguration.java b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/config/NetworkingConfiguration.java index 383c3f0c1f..a5207b7aef 100644 --- a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/config/NetworkingConfiguration.java +++ b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/config/NetworkingConfiguration.java @@ -17,6 +17,7 @@ package org.hyperledger.besu.ethereum.p2p.config; import static com.google.common.base.Preconditions.checkArgument; import java.util.Objects; +import java.util.Optional; public class NetworkingConfiguration { public static final int DEFAULT_INITIATE_CONNECTIONS_FREQUENCY_SEC = 30; @@ -27,6 +28,7 @@ public class NetworkingConfiguration { private int initiateConnectionsFrequencySec = DEFAULT_INITIATE_CONNECTIONS_FREQUENCY_SEC; private int checkMaintainedConnectionsFrequencySec = DEFAULT_CHECK_MAINTAINED_CONNECTIONS_FREQUENCY_SEC; + private String dnsDiscoveryServerOverride = null; public static NetworkingConfiguration create() { return new NetworkingConfiguration(); @@ -65,6 +67,16 @@ public class NetworkingConfiguration { return checkMaintainedConnectionsFrequencySec; } + public NetworkingConfiguration setDnsDiscoveryServerOverride( + final String dnsDiscoveryServerOverride) { + this.dnsDiscoveryServerOverride = dnsDiscoveryServerOverride; + return this; + } + + public Optional getDnsDiscoveryServerOverride() { + return Optional.ofNullable(dnsDiscoveryServerOverride); + } + public NetworkingConfiguration setCheckMaintainedConnectionsFrequency( final int checkMaintainedConnectionsFrequency) { checkArgument(checkMaintainedConnectionsFrequency > 0); diff --git a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java index d528372db4..28a9123fea 100644 --- a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java +++ b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetwork.java @@ -77,6 +77,7 @@ import org.apache.logging.log4j.Logger; import org.apache.tuweni.bytes.Bytes; import org.apache.tuweni.devp2p.EthereumNodeRecord; import org.apache.tuweni.discovery.DNSDaemon; +import org.apache.tuweni.discovery.DNSDaemonListener; /** * The peer network service (defunct PeerNetworkingService) is the entrypoint to the peer-to-peer @@ -143,10 +144,10 @@ public class DefaultP2PNetwork implements P2PNetwork { private final AtomicBoolean stopped = new AtomicBoolean(false); private final CountDownLatch shutdownLatch = new CountDownLatch(2); private final Duration shutdownTimeout = Duration.ofMinutes(1); - - private final AtomicReference> dnsPeers = new AtomicReference<>(); private DNSDaemon dnsDaemon; + @VisibleForTesting final AtomicReference> dnsPeers = new AtomicReference<>(); + /** * Creates a peer networking service for production purposes. * @@ -203,29 +204,27 @@ public class DefaultP2PNetwork implements P2PNetwork { final String address = config.getDiscovery().getAdvertisedHost(); final int configuredDiscoveryPort = config.getDiscovery().getBindPort(); final int configuredRlpxPort = config.getRlpx().getBindPort(); - if (config.getDiscovery().getDNSDiscoveryURL() != null) { - LOG.info("Starting DNS discovery with URL {}", config.getDiscovery().getDNSDiscoveryURL()); - dnsDaemon = - new DNSDaemon( - config.getDiscovery().getDNSDiscoveryURL(), - (seq, records) -> { - List peers = new ArrayList<>(); - for (EthereumNodeRecord enr : records) { - EnodeURL enodeURL = - EnodeURLImpl.builder() - .ipAddress(enr.ip()) - .nodeId(enr.publicKey().bytes()) - .discoveryPort(Optional.ofNullable(enr.udp())) - .listeningPort(Optional.ofNullable(enr.tcp())) - .build(); - DiscoveryPeer peer = DiscoveryPeer.fromEnode(enodeURL); - peers.add(peer); - rlpxAgent.connect(peer); - } - dnsPeers.set(peers); - }); - } - getDnsDaemon().ifPresent(DNSDaemon::start); + + Optional.ofNullable(config.getDiscovery().getDNSDiscoveryURL()) + .ifPresent( + disco -> { + LOG.info("Starting DNS discovery with URL {}", disco); + config + .getDnsDiscoveryServerOverride() + .ifPresent( + dnsServer -> + LOG.info( + "Starting DNS discovery with DNS Server override {}", dnsServer)); + + dnsDaemon = + new DNSDaemon( + disco, + createDaemonListener(), + 0L, + 60000L, + config.getDnsDiscoveryServerOverride().orElse(null)); + dnsDaemon.start(); + }); final int listeningPort = rlpxAgent.start().join(); final int discoveryPort = @@ -325,6 +324,29 @@ public class DefaultP2PNetwork implements P2PNetwork { return Optional.ofNullable(dnsDaemon); } + @VisibleForTesting + DNSDaemonListener createDaemonListener() { + return (seq, records) -> { + List peers = new ArrayList<>(); + for (EthereumNodeRecord enr : records) { + EnodeURL enodeURL = + EnodeURLImpl.builder() + .ipAddress(enr.ip()) + .nodeId(enr.publicKey().bytes()) + .discoveryPort(Optional.ofNullable(enr.udp())) + .listeningPort(Optional.ofNullable(enr.tcp())) + .build(); + DiscoveryPeer peer = DiscoveryPeer.fromEnode(enodeURL); + peers.add(peer); + rlpxAgent.connect(peer); + } + // only replace dnsPeers if the lookup was successful: + if (!peers.isEmpty()) { + dnsPeers.set(peers); + } + }; + } + @VisibleForTesting void checkMaintainedConnectionPeers() { if (!localNode.isReady()) { diff --git a/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetworkTest.java b/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetworkTest.java index f1f0133511..d7039cf11f 100644 --- a/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetworkTest.java +++ b/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/network/DefaultP2PNetworkTest.java @@ -18,6 +18,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; @@ -52,6 +53,7 @@ import org.hyperledger.besu.nat.NatService; import org.hyperledger.besu.nat.core.domain.NetworkProtocol; import org.hyperledger.besu.nat.upnp.UpnpNatManager; +import java.net.InetAddress; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -61,6 +63,10 @@ import java.util.stream.Collectors; import java.util.stream.Stream; import org.apache.tuweni.bytes.Bytes; +import org.apache.tuweni.bytes.Bytes32; +import org.apache.tuweni.crypto.SECP256K1; +import org.apache.tuweni.devp2p.EthereumNodeRecord; +import org.apache.tuweni.discovery.DNSDaemonListener; import org.assertj.core.api.Assertions; import org.junit.Before; import org.junit.Test; @@ -72,8 +78,11 @@ import org.mockito.junit.MockitoJUnitRunner; @RunWith(MockitoJUnitRunner.StrictStubs.class) public final class DefaultP2PNetworkTest { - final MaintainedPeers maintainedPeers = new MaintainedPeers(); + final SECP256K1.SecretKey mockKey = + SECP256K1.SecretKey.fromBytes( + Bytes32.fromHexString( + "0x8f2a55949038a9610f50fb23b5883af3b4ecb3c3bb792cbcefbd1542c692be63")); @Mock PeerDiscoveryAgent discoveryAgent; @Mock RlpxAgent rlpxAgent; @@ -334,18 +343,15 @@ public final class DefaultP2PNetworkTest { } @Test - public void shouldNotStartDnsDiscoveryWhenDNSURLIsNotConfigured() { - // spy on DefaultP2PNetwork - DefaultP2PNetwork testClass = spy(network()); - + public void shouldNotStartDnsDiscoveryWhenDnsURLIsNotConfigured() { + DefaultP2PNetwork testClass = network(); testClass.start(); - // ensure we called getDnsDaemon during start, and that it is NOT present: - verify(testClass, times(1)).getDnsDaemon(); + // ensure DnsDaemon is NOT present: assertThat(testClass.getDnsDaemon()).isNotPresent(); } @Test - public void shouldStartDnsDiscoveryWhenDNSURLIsNotConfigured() { + public void shouldStartDnsDiscoveryWhenDnsURLIsConfigured() { // create a discovery config with a dns config DiscoveryConfiguration disco = DiscoveryConfiguration.create().setDnsDiscoveryURL("enrtree://mock@localhost"); @@ -355,12 +361,58 @@ public final class DefaultP2PNetworkTest { when(spy(config).getDiscovery()).thenReturn(disco).getMock(); // spy on DefaultP2PNetwork - DefaultP2PNetwork testClass = spy((DefaultP2PNetwork) builder().config(dnsConfig).build()); + DefaultP2PNetwork testClass = (DefaultP2PNetwork) builder().config(dnsConfig).build(); + + testClass.start(); + assertThat(testClass.getDnsDaemon()).isPresent(); + } + + @Test + public void shouldUseDnsServerOverrideIfPresent() { + // create a discovery config with a dns config + DiscoveryConfiguration disco = + DiscoveryConfiguration.create().setDnsDiscoveryURL("enrtree://mock@localhost"); + + // spy on config to return dns discovery config: + NetworkingConfiguration dnsConfig = spy(config); + doReturn(disco).when(dnsConfig).getDiscovery(); + doReturn(Optional.of("localhost")).when(dnsConfig).getDnsDiscoveryServerOverride(); + DefaultP2PNetwork testClass = (DefaultP2PNetwork) builder().config(dnsConfig).build(); testClass.start(); - // ensure we called getDnsDaemon during start, and that it is present: - verify(testClass, times(1)).getDnsDaemon(); + + // ensure we used the dns server override config when building DNSDaemon: assertThat(testClass.getDnsDaemon()).isPresent(); + verify(dnsConfig, times(2)).getDnsDiscoveryServerOverride(); + } + + @Test + public void shouldNotDropDnsHostsOnEmptyLookup() { + DefaultP2PNetwork network = network(); + DNSDaemonListener listenerUnderTest = network.createDaemonListener(); + + // assert no entries prior to lookup + assertThat(network.dnsPeers.get()).isNull(); + + // simulate successful lookup of 1 peer + listenerUnderTest.newRecords( + 1, + List.of( + EthereumNodeRecord.create( + SECP256K1.KeyPair.fromSecretKey(mockKey), + 1L, + null, + null, + InetAddress.getLoopbackAddress(), + 30303, + 30303))); + assertThat(network.dnsPeers.get()).isNotEmpty(); + assertThat(network.dnsPeers.get().size()).isEqualTo(1); + + // simulate failed lookup empty list + listenerUnderTest.newRecords(2, Collections.emptyList()); + assertThat(network.dnsPeers.get()).isNotEmpty(); + assertThat(network.dnsPeers.get().size()).isEqualTo(1); } private DefaultP2PNetwork network() {