Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DNS support for static nodes and discovery #885

Merged
merged 30 commits into from
Nov 29, 2019
Merged

DNS support for static nodes and discovery #885

merged 30 commits into from
Nov 29, 2019

Conversation

prd-fox
Copy link
Contributor

@prd-fox prd-fox commented Nov 19, 2019

Adds full DNS support for Quorum.

This includes:

use of DNS hostnames in the static-nodes.json file, so that we can connect to our static nodes even when the IP address of the DNS name has changed, meaning less updating of the static node definition file. This is an improvement to previous behaviour which resolved the DNS hostname once at startup and then only tracked the IP address

Add a switch to enable DNS changes in raft to ensure default behaviour is backwards-compatible with older versions.
Add ability to deserialise old raft address information
Remove IP and undecoded elements from output via raft API.

Allow P2P layer to use the FQDN

Add hostname flag to specify the DNS hostname to use during discovery for this node
Add hostname of this node to ping packets, so that the receiver adds the DNS name to their node bucket

Add Quorum specific UDP classes and types, which are a modified version of discv4

Make bootnode listen for Quorum Discovery messages

Remove double-decode of rlp elements by creating the RLP stream outside.
Add forward-compatibility by adding a tail of undecoded RLP elements that are ignored.

Add document for command line arguments, link to upstream arguments and
including DNS specific changes.
move docs to correct place and update doc index
Fix mehod comments and Quorum location comments
Move new "hostname" flag to be under Quorum options instead of
networking options.
Take upstream change for hostname parsing found at
ethereum/go-ethereum#18524
Keep raft CLI arg name in line with other raft args by keeping as all
one word
The node will not start if the provided hostname flag does not resolve
to an IP address, to prevent a user accidentally mistyping their
intended address.
Krish1979
Krish1979 previously approved these changes Nov 19, 2019
@jpmsam jpmsam requested review from trung and jbhurat November 19, 2019 21:24
Copy link
Contributor

@trung trung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial quick feedbacks

mkdocs.yml Outdated Show resolved Hide resolved
mkdocs.yml Show resolved Hide resolved
p2p/shared_udp_connection.go Outdated Show resolved Hide resolved
p2p/discover/udp_quorum.go Outdated Show resolved Hide resolved
p2p/discover/udp_quorum.go Outdated Show resolved Hide resolved
Copy link
Contributor

@libby libby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial feedback =)

p2p/enode/urlv4.go Show resolved Hide resolved
p2p/enode/urlv4.go Outdated Show resolved Hide resolved
p2p/enode/urlv4_test.go Show resolved Hide resolved
p2p/server.go Outdated Show resolved Hide resolved
p2p/discover/udp.go Outdated Show resolved Hide resolved
raft/handler.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jbhurat jbhurat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that there is a lot of duplication between udp.go and udp_quorum.go is it possible to re-evaluate udp.go and udp_quorum.go to see there is no code duplication

@prd-fox
Copy link
Contributor Author

prd-fox commented Nov 22, 2019

I have removed the discovery section of the PR, so discovery will only use IP addresses as before. I have resolved comments relating to those portions.

time. This allows us to continuously try to connect to static nodes
without restarting.
hostname does not resolve to a valid IP, so the output will not match
the input as the current test setup expects. The output will be caught
in other part of the systems and handled appropriately.
keep resolving the hostname instead of just at startup.

Ran goimports.
trung
trung previously approved these changes Nov 25, 2019
Copy link
Contributor

@trung trung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jbhurat
jbhurat previously approved these changes Nov 26, 2019
Copy link
Contributor

@jbhurat jbhurat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

…old"

vs "new", which will not having meaning in the future.
@prd-fox prd-fox dismissed stale reviews from jbhurat and trung via 2e925bf November 27, 2019 14:47
@jpmsam jpmsam requested a review from libby November 27, 2019 20:27
@jpmsam jpmsam merged commit e3089e3 into Consensys:master Nov 29, 2019
@prd-fox prd-fox deleted the dns-support branch November 29, 2019 14:43
@jimthematrix
Copy link
Contributor

thank you Quorum team for making this enhancement!

libby added a commit to libby/qubernetes that referenced this pull request Aug 20, 2020
* Use k8s service hostname for quorum peers: permissioned-nodes.json and static-nodes.json.

* Use k8s service hostname in tessera config for peer discovery.

* The hostnames are obtained via the quorum services which are known at the time of generation and do not change.
This removes the need to do sed and manipulation on the config files after they are deployed in order to set the IPs
(which are not known until after k8s resources have been deployed). This also helps with automatically updating the
config files when new nodes are deployed or removed, thus avoiding restarting the nodes.

* Enabled by this PR: Consensys/quorum#885.
libby added a commit to Consensys/qubernetes that referenced this pull request Aug 20, 2020
* Switch peers from using variable IPs to hostnames.

* Use k8s service hostname for quorum peers: permissioned-nodes.json and static-nodes.json.

* Use k8s service hostname in tessera config for peer discovery.

* The hostnames are obtained via the quorum services which are known at the time of generation and do not change.
This removes the need to do sed and manipulation on the config files after they are deployed in order to set the IPs
(which are not known until after k8s resources have been deployed). This also helps with automatically updating the
config files when new nodes are deployed or removed, thus avoiding restarting the nodes.

* Enabled by this PR: Consensys/quorum#885.

* Remove support / testing for quorum <= 2.3.0

* hostname not supported pre 2.3.0, if there is demand to support them, we can include support, but for now removing support as these version should be phased out.

* add sleep in quorum version tests after running initial public contract to allow block number to increase.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants