add dns support #1247

matkt · 2020-07-22T15:50:08Z

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

PR description

This PR add the support to DNS. By default Besu refuses the use of a DNS but it is possible to use it by adding the following flag --Xdns-enabled=true. Adding this flag will resolve the hostname when starting besu and then it won't change

If there is a need for a more dynamic update (eg for permissioning) add also this flag --Xdns-update-enabled = true ( this will query the DNS every time. So you must trust the DNS on which you are looking for the IP)

Test performed

Local Clique Permissioning network (valid/invalid hostname)
Azure IBT2 Permissioning network (init network, restart pod)
Sync Goerli with static-nodes and without permissioning (dns disabled)
Provide Static-nodes with or without hostname (dns enabled/disabled)
Provide bootnodes with or without hostname (dns enabled/disabled)

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

sambacha · 2020-07-24T18:16:41Z

Can this DNS option also check CAA policies down the line, or perhaps verify TLS certificate is authorized based on said CAA policy?
Obviously not now, but just trying to see if it's only for ennode usage, 👍

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

matkt · 2020-08-21T08:28:46Z

Can this DNS option also check CAA policies down the line, or perhaps verify TLS certificate is authorized based on said CAA policy?
Obviously not now, but just trying to see if it's only for ennode usage, 👍

yes I think it might be interesting to explore this idea. this could be an additional parameter

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

CjHare · 2020-08-24T05:08:35Z

besu/src/test/java/org/hyperledger/besu/util/LocalPermissioningConfigurationValidatorTest.java

@@ -123,4 +136,89 @@ public void nodeAllowlistCheckShouldIgnoreDiscoveryPortParam() throws Exception
          "Exception not expected. Validation of nodes in allowlist should ignore the optional discovery port param.");
    }
  }
+
+  @Test
+  public void nodeAllowlistCheckShouldWorkWithHostnameIfDndEnabled() throws Exception {


Is there a typo in this test name? (Dnd instead of Dns)

Good catch . Changed

CjHare

Code and test LGTM.

As it's been just over a month since this PR was raised, any reason for it not being reviewed or preventing merging?

CjHare · 2020-08-24T05:18:41Z

...dger/besu/tests/acceptance/dsl/node/configuration/permissioning/PermissionedNodeBuilder.java

@@ -209,12 +210,15 @@ private LocalPermissioningConfiguration localConfigPermissioningConfiguration()
        localConfigNodesPermissioningFile = createTemporaryPermissionsFile();
      }

-      List<String> nodesAsListOfStrings =
+      final List<String> nodesAsListOfStrings =


nit; these local variables could be have more succinct names (as we know they are list of strings from typing).

Maybe something like localPermittedNodes and localPermittedEnodeUrls?

it's changed 👍

CjHare · 2020-08-24T05:22:14Z

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

+  public void dnsEnabledOptionIsParsedCorrectly() {
+    TestBesuCommand besuCommand = parseCommand("--Xdns-enabled", "true");
+
+    assertThat(besuCommand.getEnodeDnsConfiguration().dnsEnabled()).isEqualTo(true);


nit; isTrue() and isFalse() can be nicer to read then isEqualTo(true) and isEqualTo(false)

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

matkt · 2020-08-25T12:33:09Z

Code and test LGTM.

As it's been just over a month since this PR was raised, any reason for it not being reviewed or preventing merging?

No It's just that I was working on something else 😄 . I also wanted to take the time to validate my implementation on Azure etc. Thanks for your review

CjHare

Looks good and once you've verified the Azure deployment you mention, feel free to merge 👍

shemnon · 2020-08-25T14:33:28Z

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

-  }
-
-  private List<EnodeURL> bootNodes = null;
+  private final List<String> bootnodes = null;


Should this be camelCased? bootNodes? It was before and other places in the edited code retain the camel casing. Whatever it chosen it should be consistent.

shemnon · 2020-08-25T14:38:18Z

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/peers/EnodeURL.java

  }

  public InetAddress getIp() {
+    this.ip =


Is doing a DNS lookup every time the design intent? If not perhaps ip should be a supplier and this logic gets put in a Suppliers.memoize call? If it is the design intent it's work a comment so a later refactoring doesn't undo it.

No it's a voluntary choice to DNS lookup every time. Because a cache could give a bad IP before it is invalidated again. If the "dns-update-enabled" flag is false it will do it only once.
we could add a cache but I'm afraid that this will prevent nodes from communicating each other while waiting for the next cache update

I just added a comment 👍

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

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

shemnon · 2020-08-25T16:13:31Z

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/peers/EnodeURL.java

+  /**
+   * Get IP of the EnodeURL
+   *
+   * <p>If "dns" and "dns-update" are enabled -> DNS lookup every time to have the IP up to date and


Suggested change

* <p>If "dns" and "dns-update" are enabled -> DNS lookup every time to have the IP up to date and

* <p>If "dns" and "dns-update" are enabled -> DNS lookup every time to have the IP up to date and

shemnon · 2020-08-25T16:14:15Z

ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/peers/EnodeURL.java

+   * <p>If "dns" and "dns-update" are enabled -> DNS lookup every time to have the IP up to date and
+   * not to rely on an invalid cache
+   *
+   * <p>If the "dns" is enabled but "dns-update" is disabled -> IP is retrieved only one time and


Suggested change

* <p>If the "dns" is enabled but "dns-update" is disabled -> IP is retrieved only one time and

* <p>If the "dns" is enabled but "dns-update" is disabled -> IP is retrieved only one time and

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

matkt added 3 commits July 22, 2020 17:41

add dns support

c8f8832

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

update error message

f39f528

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

fix tests

f943765

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

MadelineMurray mentioned this pull request Jul 23, 2020

Doc DNS support hyperledger/besu-docs#450

Closed

matkt added 2 commits August 17, 2020 18:31

clean code

ffbff03

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

update hostname detection

1adca54

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

matkt force-pushed the add-dns-support branch from a4e6536 to 1adca54 Compare August 17, 2020 16:37

matkt added 3 commits August 19, 2020 22:27

spotlessApply

634e7e9

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

Merge branch 'upstream/master' into add-dns-support

ddbaa0a

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

add tests

d60de83

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

matkt added 2 commits August 21, 2020 10:44

update CHANGELOG.md

3910077

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

Merge branch 'master' into add-dns-support

0434f60

matkt marked this pull request as ready for review August 21, 2020 08:57

matkt requested review from lucassaldanha and joshuafernandes August 21, 2020 12:05

CjHare reviewed Aug 24, 2020

View reviewed changes

fix review issues and add acceptance tests

b250ae8

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

Merge branch 'master' into add-dns-support

ae203e6

CjHare approved these changes Aug 25, 2020

View reviewed changes

Merge branch 'master' into add-dns-support

73a087f

shemnon approved these changes Aug 25, 2020

View reviewed changes

matkt added 2 commits August 25, 2020 17:46

fix review issues

abf0db5

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

Merge remote-tracking branch 'origin/add-dns-support' into add-dns-su…

6e4f8c7

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

matkt force-pushed the add-dns-support branch from 2185f31 to 6e4f8c7 Compare August 25, 2020 15:48

shemnon reviewed Aug 25, 2020

View reviewed changes

matkt and others added 2 commits August 25, 2020 18:55

fix javadoc

d769a4c

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

Merge branch 'master' into add-dns-support

8768934

matkt merged commit 210c85c into hyperledger:master Aug 25, 2020

sownak mentioned this pull request Sep 1, 2020

Enable DNS support in Besu hyperledger-bevel/bevel#983

Closed

matkt deleted the add-dns-support branch September 8, 2020 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add dns support #1247

add dns support #1247

matkt commented Jul 22, 2020 •

edited

Loading

sambacha commented Jul 24, 2020

matkt commented Aug 21, 2020

CjHare Aug 24, 2020

matkt Aug 25, 2020

CjHare left a comment •

edited

Loading

CjHare Aug 24, 2020

matkt Aug 25, 2020

CjHare Aug 24, 2020

matkt Aug 25, 2020

matkt commented Aug 25, 2020 •

edited

Loading

CjHare left a comment

shemnon Aug 25, 2020

matkt Aug 25, 2020

shemnon Aug 25, 2020

matkt Aug 25, 2020

matkt Aug 25, 2020

shemnon Aug 25, 2020

shemnon Aug 25, 2020

	* <p>If "dns" and "dns-update" are enabled -> DNS lookup every time to have the IP up to date and
	* <p>If "dns" and "dns-update" are enabled -> DNS lookup every time to have the IP up to date and

	* <p>If the "dns" is enabled but "dns-update" is disabled -> IP is retrieved only one time and
	* <p>If the "dns" is enabled but "dns-update" is disabled -> IP is retrieved only one time and

add dns support #1247

add dns support #1247

Conversation

matkt commented Jul 22, 2020 • edited Loading

PR description

Test performed

sambacha commented Jul 24, 2020

matkt commented Aug 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CjHare left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matkt commented Aug 25, 2020 • edited Loading

CjHare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matkt commented Jul 22, 2020 •

edited

Loading

CjHare left a comment •

edited

Loading

matkt commented Aug 25, 2020 •

edited

Loading