-
Notifications
You must be signed in to change notification settings - Fork 171
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
Preserve info about which addresses are local in AddrInfo
#2364
Comments
DiscoveryItem
/AddrInfo
AddrInfo
One issue we have currently is that when you connect via a direct address like this we never share another way of contacting the node. If the node moves it'll lose connectivity. If a RelayUrl would be shared it would be more likely that we can keep connectivity. However that's probably not something we need to worry about here, it probably needs a more general solution anyway. |
That's a good point. Does there exist an issue for that/should that have an issue? |
I don't think there's an issue already, not sure if we need one. Maybe it makes sense.
…On Mon, 17 Jun 2024, at 10:21, Philipp Krüger wrote:
That's a good point. Does there exist an issue for that/should that have an issue?
—
Reply to this email directly, view it on GitHub <#2364 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABD2IALDEYYU2DZ7NN7ZHDZH2MBBAVCNFSM6AAAAABJNOOTESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZSGYYDOMJRGE>.
You are receiving this because you commented.Message ID: ***@***.***>
|
private address ranges are defined in IETF RFC 1918, are the rust functions for this not enough? |
That's probably a good idea. |
The Rust APIs for So having this extra bit of information is helpful for the ipv6 case, otherwise we are just adding all ipv6 addrs in. There is a larger question here that I want to surface: do we want to have the local node discovery service pass on all possible network addrs of the node, or just the local ones? If we want them to pass on all addrs, we should really just ask to change the If that's the case, then we don't necessarily need this new piece of information, although it does seem like it could be useful information to keep around. |
This is a limitation of how mdns works, due to how the port and ip addrs are represented. |
we have these helpers already implemented here: https://github.com/n0-computer/iroh/blob/main/iroh-net/src/net/ip.rs |
I think just the local addresses should be sufficient. If you can receive this message you are on the local network so only the local ones are the useful ones. If you later want to migrate to a different network location and keep the connection open it is up to the disco protocol to keep things open. With the caveat that currently it probably doesn't do that... But I'm inclined to treat that as an orthogonal issue. |
Using the private address ranges mentioned in this thread works just fine. |
When looking over @ramfox's shoulder hacking on mdns last week, I noticed it may be useful to preserve which addresses of
addr_info.direct_addresses
are our local addresses. The main reasoning being it would be unnecessary to publish public IP information to peers on the same local network. Theswarm_discovery
trait also expects you to only provide a single port & one IPv4 and one IPv6 address, so it would be useful toThis is the struct you get when implementing the
Discovery::publish
trait function:iroh/iroh-base/src/node_addr.rs
Lines 77 to 84 in e9075f3
The actual values for
direct_addresses
actually come fromEndpoint
that's derived from thenetcheck::Report
and other things inmagicsock::Actor::store_endpoints_update
, and this is whatEndpoint
looks like:iroh/iroh-net/src/config.rs
Lines 10 to 34 in e9075f3
So at this point we actually have information about which IP addrs are local and which aren't (via
EndpointType
).We throw away that info in
MagicSock::publish_my_addr
:iroh/iroh-net/src/magicsock.rs
Lines 1174 to 1188 in e9075f3
(we only map out
ep.addr
, and throw awayep.typ
)We should keep that information to implement mdns discovery services.
Why not just publish all IP addrs, public and local, on mdns?
We could do this initially, but it's weird and definitely won't work nicely with
swarm_discovery
, as that expects a single port for all addresses, and from the looks of it that's just due to how mdns responses are structured.Related issue: #2354
The text was updated successfully, but these errors were encountered: