n0-computer
Preserve info about which addresses are local in `AddrInfo`
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. The swarm_discovery trait also expects you to only provide a single port & one IPv4 and one IPv6 address, so it would be useful to
This is the struct you get when implementing the Discovery::publish trait function:
https://github.com/n0-computer/iroh/blob/e9075f3b93038a74a4f11c545992ac4ba39590d0/iroh-base/src/node_addr.rs#L77-L84
The actual values for direct_addresses actually come from Endpoint that's derived from the netcheck::Report and other things in magicsock::Actor::store_endpoints_update, and this is what Endpoint looks like:
https://github.com/n0-computer/iroh/blob/e9075f3b93038a74a4f11c545992ac4ba39590d0/iroh-net/src/config.rs#L10-L34
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:
https://github.com/n0-computer/iroh/blob/e9075f3b93038a74a4f11c545992ac4ba39590d0/iroh-net/src/magicsock.rs#L1174-L1188
(we only map out ep.addr, and throw away ep.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
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
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 https://github.com/n0-computer/iroh/issues/2364#issuecomment-2172607111, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABD2IALDEYYU2DZ7NN7ZHDZH2MBBAVCNFSM6AAAAABJNOOTESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZSGYYDOMJRGE. You are receiving this because you commented.Message ID: @.***>
That's probably a good idea.
I guess there could be cases where the Endpoint is bound to public IP addresses, but I'm not sure how useful mdns is in such cases (or whether one should then use it).
The Rust APIs for is_private for IPv4 work just fine, but the is_unicast method for IPv6Addr is currently only in nightly.
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 swarm-discovery API to let us publish a Vec<SocketAddr> instead of the current (u16, Vec<IpAddr>).
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.
we should really just ask to change the
swarm-discoveryAPI to let us publish aVec<SocketAddr>instead of the current(u16, Vec<IpAddr>).
This is a limitation of how mdns works, due to how the port and ip addrs are represented. Because mDNS is just DNS packets and DNS packets on their own usually don't list ports at all, a single port is added as a "SRV" record (I think it stands for "service"?) and is assumed to be the same port for all IP addresses in the DNS packet.
You can see how service_discovery creates this packet.
we have these helpers already implemented here: https://github.com/n0-computer/iroh/blob/main/iroh-net/src/net/ip.rs
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
swarm-discoveryAPI to let us publish aVec<SocketAddr>instead of the current(u16, Vec<IpAddr>).
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.