Preserve info about which addresses are local in AddrInfo

[matheus23]

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:

iroh/iroh-base/src/node_addr.rs

Lines 77 to 84 in e9075f3

	/// Addressing information to connect to a peer.
	#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, Default)]
	pub struct AddrInfo {
	/// The peer's home relay url.
	pub relay_url: Option<RelayUrl>,
	/// Socket addresses where the peer might be reached directly.
	pub direct_addresses: BTreeSet<SocketAddr>,
	}

---

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:

iroh/iroh-net/src/config.rs

Lines 10 to 34 in e9075f3

	// magicsock endpoint". this time it means "an IP address on which our local magicsock
	// endpoint is listening". Name this better.
	/// An endpoint IPPort and an associated type.
	#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
	pub struct Endpoint {
	/// The address of the endpoint.
	pub addr: SocketAddr,
	/// The kind of endpoint.
	pub typ: EndpointType,
	}
	/// Type of endpoint.
	#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)]
	pub enum EndpointType {
	/// Endpoint kind has not been determined yet.
	Unknown,
	/// Endpoint is bound to a local address.
	Local,
	/// Endpoint has a publicly reachable address found via STUN.
	Stun,
	/// Endpoint uses a port mapping in the router.
	Portmapped,
	/// Hard NAT: STUN'ed IPv4 address + local fixed port.
	Stun4LocalPort,
	}

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

	/// Publishes our address to a discovery service, if configured.
	///
	/// Called whenever our addresses or home relay node changes.
	fn publish_my_addr(&self) {
	if let Some(ref discovery) = self.discovery {
	let eps = self.endpoints.read();
	let relay_url = self.my_relay();
	let direct_addresses = eps.iter().map(|ep| ep.addr).collect();
	let info = AddrInfo {
	relay_url,
	direct_addresses,
	};
	discovery.publish(&info);
	}
	}

(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

[flub]

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.

[matheus23]

That's a good point. Does there exist an issue for that/should that have an issue?

[flub]

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: ***@***.***>

[divagant-martian]

private address ranges are defined in IETF RFC 1918, are the rust functions for this not enough?

[matheus23]

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).

[ramfox]

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.

[matheus23]

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>).

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.

[dignifiedquire]

we have these helpers already implemented here: https://github.com/n0-computer/iroh/blob/main/iroh-net/src/net/ip.rs

[flub]

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>).

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.