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

feat(service): add wildcard address resolver #1099

Merged
merged 36 commits into from
Jun 6, 2024
Merged

Conversation

diegomrsantos
Copy link
Collaborator

@diegomrsantos diegomrsantos commented May 15, 2024

Summary

This PR adds a service that expands wildcard addresses to their respective interface addresses equivalent.
closes #1087

Context

Users of nim-libp2p often listen to wildcard addresses like 0.0.0.0 (IPv4) and [::] (IPv6) to allow the OS to bind the transports to all available network interfaces. This is useful for applications that need to be accessible on multiple networks without specifying each interface.

When listening on wildcard addresses, the OS binds to all available IP addresses, simplifying network configuration but requiring a mechanism to resolve these addresses to their specific network interfaces for communication with other peers.

The listenAddrs field contains addresses the node listens on, which may include wildcard and private addresses (not directly reachable). The addrs field contains resolved addresses that other peers can use to connect, including public-facing NAT and port-forwarded addresses.

Before this PR, nim-libp2p didn't offer a way to resolve wildcard addresses in the addrs field.

This was reported on status-im/nimbus-eth2#6060 and #1087.

Changes

  • Service created
  • Documentation added
  • Tests added

@diegomrsantos diegomrsantos self-assigned this May 15, 2024
@diegomrsantos diegomrsantos marked this pull request as ready for review May 22, 2024 12:23
@lchenut
Copy link
Collaborator

lchenut commented May 23, 2024

Is it possible to add a withWilcardAddressResolverService() function in the switch builder? To have a simple way of creating it. This can work well in addition to withServices(@[ .... ])

@diegomrsantos
Copy link
Collaborator Author

Is it possible to add a withWilcardAddressResolverService() function in the switch builder? To have a simple way of creating it. This can work well in addition to withServices(@[ .... ])

Not a big fan tbh. Maybe we should always add it to the switch.

@@ -0,0 +1,216 @@
# Nim-LibP2P
# Copyright (c) 2024 Status Research & Development GmbH
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI: We should change this to IFT. I'll sync with legal and come back.
We can change this in a follow up PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we change all nim-libp2p files?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ill get back once legal gets back to me.
I asked that question :).
In any case, we can do that in a follow up PR.

libp2p/services/wildcardresolverservice.nim Outdated Show resolved Hide resolved
@kaiserd
Copy link
Collaborator

kaiserd commented May 23, 2024

Is it possible to add a withWilcardAddressResolverService() function in the switch builder? To have a simple way of creating it. This can work well in addition to withServices(@[ .... ])

Not a big fan tbh. Maybe we should always add it to the switch.

Is there a reason to model the resolver as a service?
Is there a situation in which this service would no be desired?
Imo, the switch should offer this per default.

@diegomrsantos
Copy link
Collaborator Author

diegomrsantos commented May 23, 2024

Is it possible to add a withWilcardAddressResolverService() function in the switch builder? To have a simple way of creating it. This can work well in addition to withServices(@[ .... ])

Not a big fan tbh. Maybe we should always add it to the switch.

Is there a reason to model the resolver as a service? Is there a situation in which this service would no be desired? Imo, the switch should offer this per default.

The motivation was to make it optional. It might be a breaking change if we make it the default behavior tho.

@kaiserd
Copy link
Collaborator

kaiserd commented May 24, 2024

The motivation was to make it optional. It might be a breaking change if we make it the default behavior tho.

With our plan to keep only the public marked API as stable in versions 1.x, we could break that.

@diegomrsantos
Copy link
Collaborator Author

The motivation was to make it optional. It might be a breaking change if we make it the default behavior tho.

With our plan to keep only the public marked API as stable in versions 1.x, we could break that.

But I think keeping it as a Service is still nice cause the setup/stop is handled automatically by the Switch.

@diegomrsantos
Copy link
Collaborator Author

diegomrsantos commented May 27, 2024

The last commits that enabled the service by default were reverted as they caused strange errors when running the tests. Initially, it was nil access errors in the tor transport test. After those were fixed, random tests failed as the transports were not closed properly. This happened when running on my macOS m1. On CI the test would hang as it's possible to see in the commits.

@diegomrsantos
Copy link
Collaborator Author

I added back the wildcard service enabled by default. There were some issues in the tests that I had to fix, but it seems that the weird errors happened cause of what was explained here #1105.

Copy link
Collaborator

@kaiserd kaiserd left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.
Let's merge this and open an issue for a deeper investigation of the error described in #1105

@@ -11,6 +11,6 @@ COPY . nim-libp2p/

RUN \
cd nim-libp2p && \
nim c --skipProjCfg --skipParentCfg --NimblePath:./nimbledeps/pkgs -p:nim-libp2p -d:chronicles_log_level=WARN -d:chronicles_default_output_device=stderr --threads:off ./tests/transport-interop/main.nim
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this part of this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't know what's going on. Seems something went wrong during the reverts and cherry-picks. Sorry, I'm gonna fix it.

Copy link
Collaborator

@kaiserd kaiserd May 29, 2024

Choose a reason for hiding this comment

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

no problem; thanks for clarifying.

@diegomrsantos diegomrsantos force-pushed the anyaddr-resolver branch 2 times, most recently from 7837885 to ae9cb4d Compare May 28, 2024 18:42
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 83.47826% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 84.48%. Comparing base (02c96fc) to head (1bb1c1a).
Report is 8 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1099      +/-   ##
==========================================
- Coverage   84.53%   84.48%   -0.06%     
==========================================
  Files          91       92       +1     
  Lines       15517    15640     +123     
==========================================
+ Hits        13118    13213      +95     
- Misses       2399     2427      +28     
Files Coverage Δ
libp2p/builders.nim 94.59% <100.00%> (+0.07%) ⬆️
libp2p/multiaddress.nim 87.25% <ø> (+0.32%) ⬆️
libp2p/services/autorelayservice.nim 94.89% <100.00%> (ø)
libp2p/utility.nim 30.39% <100.00%> (+0.39%) ⬆️
libp2p/peerinfo.nim 69.23% <0.00%> (-6.70%) ⬇️
libp2p/services/wildcardresolverservice.nim 81.13% <82.52%> (ø)

... and 18 files with indirect coverage changes

Copy link
Collaborator

@kaiserd kaiserd left a comment

Choose a reason for hiding this comment

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

Let's address @lchenut comments and merge :).

Copy link
Collaborator

@lchenut lchenut left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@diegomrsantos diegomrsantos enabled auto-merge (squash) June 6, 2024 10:35
@diegomrsantos diegomrsantos merged commit bccb305 into master Jun 6, 2024
9 checks passed
@diegomrsantos diegomrsantos deleted the anyaddr-resolver branch June 6, 2024 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: done
Development

Successfully merging this pull request may close these issues.

switch.peerInfo reports with INADDR_ANY or INADDR6_ANY multiaddresses.
5 participants