added tests to cater for IP and DNS resolution (#3818)

Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>

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));
   }
 }

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;

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<String> 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<String> 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();