From 3820b584b51af4bfcc1e8f9b6c0819c412499e23 Mon Sep 17 00:00:00 2001 From: Sally MacFarlane Date: Thu, 12 May 2022 07:40:50 +1000 Subject: [PATCH] added tests to cater for IP and DNS resolution (#3818) Signed-off-by: Sally MacFarlane --- ...lowlistWithDnsPersistorAcceptanceTest.java | 72 +++++++++++++------ .../permissioning/AllowlistPersistor.java | 11 +++ .../permissioning/AllowlistPersistorTest.java | 26 +++++++ 3 files changed, 88 insertions(+), 21 deletions(-) diff --git a/acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/permissioning/AllowlistWithDnsPersistorAcceptanceTest.java b/acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/permissioning/AllowlistWithDnsPersistorAcceptanceTest.java index dc4b71de0d..65fd928b5e 100644 --- a/acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/permissioning/AllowlistWithDnsPersistorAcceptanceTest.java +++ b/acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/permissioning/AllowlistWithDnsPersistorAcceptanceTest.java @@ -14,10 +14,15 @@ */ package org.hyperledger.besu.tests.acceptance.permissioning; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.hyperledger.besu.ethereum.permissioning.AllowlistPersistor.ALLOWLIST_TYPE; +import org.hyperledger.besu.ethereum.p2p.peers.EnodeURLImpl; +import org.hyperledger.besu.ethereum.p2p.peers.ImmutableEnodeDnsConfiguration; +import org.hyperledger.besu.plugin.data.EnodeURL; import org.hyperledger.besu.tests.acceptance.dsl.AcceptanceTestBase; import org.hyperledger.besu.tests.acceptance.dsl.account.Account; +import org.hyperledger.besu.tests.acceptance.dsl.condition.Condition; import org.hyperledger.besu.tests.acceptance.dsl.node.Node; import java.net.InetAddress; @@ -27,14 +32,17 @@ import java.util.ArrayList; import java.util.Collections; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; public class AllowlistWithDnsPersistorAcceptanceTest extends AcceptanceTestBase { - private String ENODE_ONE; - private String ENODE_TWO; - private String ENODE_THREE; + public static final String ENODE_PREFIX = + "enode://6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0@"; + public static final String PORT_SUFFIX = ":4567"; + + private String ENODE_LOCALHOST_DNS; + private String ENODE_LOCALHOST_IP; + private String ENODE_TWO_IP; private Node node; private Account senderA; @@ -42,17 +50,13 @@ public class AllowlistWithDnsPersistorAcceptanceTest extends AcceptanceTestBase @Before public void setUp() throws Exception { - ENODE_ONE = - "enode://6f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0@" - + InetAddress.getLocalHost().getHostName() - + ":4567"; - ENODE_TWO = - "enode://5f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0@192.168.0.10:4567"; - ENODE_THREE = - "enode://4f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0@192.168.0.11:4567"; + ENODE_LOCALHOST_DNS = ENODE_PREFIX + InetAddress.getLocalHost().getHostName() + PORT_SUFFIX; + ENODE_LOCALHOST_IP = ENODE_PREFIX + "127.0.0.1" + PORT_SUFFIX; + ENODE_TWO_IP = + "enode://5f8a80d14311c39f35f516fa664deaaaa13e85b2f7493f37f6144d86991ec012937307647bd3b9a82abe2974e1407241d54947bbb39763a4cac9f77166ad92a0@192.168.0.10:1234"; senderA = accounts.getPrimaryBenefactor(); - tempFile = Files.createTempFile("test", "test"); + tempFile = Files.createTempFile("test", "perm-dns-test"); this.node = permissionedNodeBuilder @@ -67,22 +71,48 @@ public class AllowlistWithDnsPersistorAcceptanceTest extends AcceptanceTestBase cluster.start(this.node); } - @Ignore("test is failing in CI") @Test - public void manipulatedNodesAllowlistWithHostnameShouldWorkWhenDnsEnabled() { + public void addingEnodeWithIp_andThenAddingSameEnodeWithHostname_shouldThrow() { - node.verify(perm.addNodesToAllowlist(ENODE_ONE, ENODE_TWO)); + node.verify(perm.addNodesToAllowlist(ENODE_LOCALHOST_IP)); node.verify( perm.expectPermissioningAllowlistFileKeyValue( - ALLOWLIST_TYPE.NODES, tempFile, ENODE_ONE, ENODE_TWO)); + ALLOWLIST_TYPE.NODES, tempFile, ENODE_LOCALHOST_DNS)); + + // expect an exception whe adding using hostname, since this node is already added with IP + final Condition condition = perm.addNodesToAllowlist(ENODE_LOCALHOST_DNS); + assertThatThrownBy(() -> node.verify(condition)).isInstanceOf(RuntimeException.class); + } - node.verify(perm.removeNodesFromAllowlist(ENODE_ONE)); + @Test + public void addingEnodeWithHostNameShouldWorkWhenDnsEnabled() { + + node.verify(perm.addNodesToAllowlist(ENODE_LOCALHOST_DNS)); + + // This should just work since there is no IP address to resolve to a host name. + // With DNS enabled, the ENODE with the DNS hostname in it should remain as is. node.verify( - perm.expectPermissioningAllowlistFileKeyValue(ALLOWLIST_TYPE.NODES, tempFile, ENODE_TWO)); + perm.expectPermissioningAllowlistFileKeyValue( + ALLOWLIST_TYPE.NODES, tempFile, ENODE_LOCALHOST_DNS)); + } + + @Test + public void manipulatedNodesAllowlistWithHostnameShouldWorkWhenDnsEnabled() { + + node.verify(perm.addNodesToAllowlist(ENODE_LOCALHOST_DNS, ENODE_TWO_IP)); + // use DNS config to resolve the Enode with IP. It either resolves to a hostname or remain as is + final EnodeURL enodeURL0 = + EnodeURLImpl.fromString( + ENODE_TWO_IP, + ImmutableEnodeDnsConfiguration.builder().dnsEnabled(true).updateEnabled(true).build()); + final String enode2ResolvedToDns = enodeURL0.toString(); + node.verify( + perm.expectPermissioningAllowlistFileKeyValue( + ALLOWLIST_TYPE.NODES, tempFile, ENODE_LOCALHOST_DNS, enode2ResolvedToDns)); - node.verify(perm.addNodesToAllowlist(ENODE_ONE, ENODE_THREE)); + node.verify(perm.removeNodesFromAllowlist(ENODE_LOCALHOST_DNS)); node.verify( perm.expectPermissioningAllowlistFileKeyValue( - ALLOWLIST_TYPE.NODES, tempFile, ENODE_TWO, ENODE_ONE, ENODE_THREE)); + ALLOWLIST_TYPE.NODES, tempFile, enode2ResolvedToDns)); } } diff --git a/ethereum/permissioning/src/main/java/org/hyperledger/besu/ethereum/permissioning/AllowlistPersistor.java b/ethereum/permissioning/src/main/java/org/hyperledger/besu/ethereum/permissioning/AllowlistPersistor.java index 5c98a3ca98..2c0f9ffd59 100644 --- a/ethereum/permissioning/src/main/java/org/hyperledger/besu/ethereum/permissioning/AllowlistPersistor.java +++ b/ethereum/permissioning/src/main/java/org/hyperledger/besu/ethereum/permissioning/AllowlistPersistor.java @@ -14,6 +14,8 @@ */ package org.hyperledger.besu.ethereum.permissioning; +import static org.hyperledger.besu.util.Slf4jLambdaHelper.debugLambda; + import java.io.File; import java.io.IOException; import java.nio.file.Files; @@ -31,8 +33,11 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Charsets; import org.apache.tuweni.toml.Toml; import org.apache.tuweni.toml.TomlParseResult; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class AllowlistPersistor { + private static final Logger LOG = LoggerFactory.getLogger(AllowlistPersistor.class); private final File configurationFile; @@ -70,6 +75,12 @@ public class AllowlistPersistor { boolean listsMatch = existingValues.containsAll(checkLists); if (!listsMatch) { + debugLambda( + LOG, + "\n LISTS DO NOT MATCH configFile::", + existingValues::toString, + configurationFilePath::toString); + debugLambda(LOG, "\nLISTS DO NOT MATCH in-memory ::", checkLists::toString); throw new AllowlistFileSyncException(); } return listsMatch; diff --git a/ethereum/permissioning/src/test/java/org/hyperledger/besu/ethereum/permissioning/AllowlistPersistorTest.java b/ethereum/permissioning/src/test/java/org/hyperledger/besu/ethereum/permissioning/AllowlistPersistorTest.java index 27b2f92032..25cb91c2aa 100644 --- a/ethereum/permissioning/src/test/java/org/hyperledger/besu/ethereum/permissioning/AllowlistPersistorTest.java +++ b/ethereum/permissioning/src/test/java/org/hyperledger/besu/ethereum/permissioning/AllowlistPersistorTest.java @@ -15,6 +15,8 @@ package org.hyperledger.besu.ethereum.permissioning; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.hyperledger.besu.ethereum.permissioning.AllowlistPersistor.verifyConfigFileMatchesState; import org.hyperledger.besu.ethereum.permissioning.AllowlistPersistor.ALLOWLIST_TYPE; @@ -110,6 +112,30 @@ public class AllowlistPersistorTest { assertThat(hasKeyAndExactLineContent(key, expectedValue)).isTrue(); } + @Test + public void compareContentsWhenEqual_shouldMatch() throws Exception { + final ALLOWLIST_TYPE key = ALLOWLIST_TYPE.ACCOUNTS; + final List newValue = Lists.newArrayList("account5", "account6", "account4"); + + allowlistPersistor.updateConfig(key, newValue); + + assertThat(verifyConfigFileMatchesState(key, newValue, tempFile.toPath())).isTrue(); + } + + @Test + public void compareContentsWhenDifferent_shouldThrow() throws Exception { + final ALLOWLIST_TYPE key = ALLOWLIST_TYPE.ACCOUNTS; + final List newValue = Lists.newArrayList("account5", "account6", "account4"); + + allowlistPersistor.updateConfig(key, newValue); + + assertThatThrownBy( + () -> + verifyConfigFileMatchesState( + key, Lists.newArrayList("not-the-same"), tempFile.toPath())) + .isInstanceOf(AllowlistFileSyncException.class); + } + @After public void tearDown() { tempFile.delete();