bosagora / agora

POC Node implementation for CoinNet
https://bosagora.io
MIT License
37 stars 22 forks source link

Improve address registry UX #3215

Closed mkykadir closed 2 years ago

mkykadir commented 2 years ago

Fixes #2320 Depends on #3239

After this PR, #3240 can be considered.

This PR misses Flash configuration intentionally due to current Flash architecture. It ends up with code duplication and unnecessarily doubles memory usage for the same data (i.e. NetworkManager.addresses_to_register will be same and required for Flash).

There are three options for configuring how a node registers itself to public registry; by default all interfaces are considered as public:

  1. Default (no specific configuration from user) node.register or node.public_addresses are not configured by user thus all interfaces are registered. If an interface has 0.0.0.0 configured as its address than public IP address of the host will be used. In registry, this will result in A record for each public interface and associated URI record (including schema and port)
  2. node.register is configured by user Each public interface will be registered with value in the node.register as address of the interface. This will result in single CNAME record and associated URI records for each public interface (including schema and port)
  3. node.public_addresses is configured by user Interfaces are ignored and addresses in this field is registered as it was before in addresses_to_register method.
codecov[bot] commented 2 years ago

Codecov Report

Merging #3215 (9f09a0b) into v0.x.x (4a16c27) will decrease coverage by 0.35%. The diff coverage is 39.58%.

:exclamation: Current head 9f09a0b differs from pull request most recent head 613a348. Consider uploading reports for the commit 613a348 to get more accurate results

@@            Coverage Diff             @@
##           v0.x.x    #3215      +/-   ##
==========================================
- Coverage   87.80%   87.45%   -0.36%     
==========================================
  Files         164      165       +1     
  Lines       16923    16951      +28     
==========================================
- Hits        14860    14824      -36     
- Misses       2063     2127      +64     
Flag Coverage Δ
unittests 87.45% <39.58%> (-0.36%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
source/agora/test/Base.d 80.27% <ø> (-0.72%) :arrow_down:
source/agora/utils/InetUtils.d 75.00% <0.00%> (ø)
source/agora/network/Manager.d 78.32% <47.82%> (-0.04%) :arrow_down:
source/agora/node/Config.d 67.15% <50.00%> (-2.27%) :arrow_down:
source/agora/consensus/BlockStorage.d 68.90% <0.00%> (-10.61%) :arrow_down:
source/agora/utils/Log.d 52.20% <0.00%> (-5.04%) :arrow_down:
source/agora/consensus/protocol/Nominator.d 91.50% <0.00%> (-0.50%) :arrow_down:
source/agora/consensus/state/Ledger.d 90.26% <0.00%> (-0.34%) :arrow_down:
source/agora/node/FullNode.d 73.37% <0.00%> (-0.31%) :arrow_down:
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cf5ea6d...613a348. Read the comment docs.

Geod24 commented 2 years ago

I was thinking about it the other day, and there's a few options: 1) Having a register: {true, false} in the interface itself reduces duplication, but we could run into problems since the IP provided is likely to be a netmask (e.g. 0.0.0.0) than an actual IP address; 2) Even if we get an actual IP address OR we can detect it (we'd have to handle native as well as node in Docker), the user might want to use a hostname; 3) In addition to the above IP/hostname discrepancy, a user could also have a different port, thanks to reverse-proxying; 4) Finally, users might have addresses to register which are completely different from the interfaces setting: For example if they live behind a load balancer, or, if we go wild, if they have a message broker sitting between their public endpoint and their node;

For this reason, I was inclined to go with the following solution:

This is just a brain dump, I'll look in the PR in more details now :)

mkykadir commented 2 years ago

Here is the summary of what I've done in this PR:

  1. Added register boolean flag; interface is only registered when this is set and node is a Validator (will work on Flash nodes)
  2. Added hostname field and addresses_to_register is moved to interface. One can only be set, hostname acts as a CNAME record and addresses_to_register is for extreme scenarios
  3. If none is provided from 2, IP address is used in interface config; or public IP address is fetched when config is 0.0.0.0 or ::

It looks mostly similar to @Geod24 thoughts with different namings 😄

mkykadir commented 2 years ago

Per-interface configuration allows us to configure a CNAME for one interface and use IPs for another interface thus Payload includes CNAME and A records together; which Registry doesn't like and integration test catches that.

hewison-chris commented 2 years ago

I think you need to rebase and then update the new ngrok instructions recently merged as it mentions addresses_to_register:

mkykadir commented 2 years ago

I think you need to rebase and then update the new ngrok instructions recently merged as it mentions addresses_to_register:

Updated