feat(iroh)!: allow multiple IP transports, including filtering by interface#3692
feat(iroh)!: allow multiple IP transports, including filtering by interface#3692dignifiedquire merged 33 commits intomainfrom
Conversation
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3692/docs/iroh/ Last updated: 2026-01-08T19:59:29Z |
6726f3c to
479652f
Compare
479652f to
232524f
Compare
|
We could remove the |
well currently they are treated differently internally as an explicit fallback, thats why the difference in configuration exists |
matheus23
left a comment
There was a problem hiding this comment.
I'm not super convinced this is the right API 🤔
Looking at your example:
let endpoint = Endpoint::builder()
.bind_addr_v4_default("127.0.0.1", 1234)
.bind_addr_v4("192.168.1.2/24".parse()?, 1234) // bind to prefix_len of 24
.bind()
.await?It looks like the configuration for an endpoint that will only send traffic in the local network and localhost.
So let's say quinn produces a datagram that's meant to be sent to 192.168.1.101:1234.
Iroh will then first check this against your 192.168.1.2/24 and find a match. However, it turns out that interface is actually busy, so it continues.
Then it'll see the "default" 127.0.0.1 interface and try to send on that. But obviously it can't send outside localhost, so ideally that should just fail sending, but I think it won't? I'm not actually sure what will happen.
Also another thing about the default socket: What if someone wants to bind a dual-stack socket with [::]? I think that should match against an IPv4 address, but IIUC currently it doesn't?
Minor note: I'm not sure what should happen if you bind multiple sockets to the same port, but different IP addrs. What happens in these cases? I'm not sure.
We don't support dual socket, not currently, and not in this setup |
flub
left a comment
There was a problem hiding this comment.
hitting the "submit review" button to get the routing comments out
iroh/src/endpoint/bind.rs
Outdated
| /// | ||
| /// Defaults to `0`. | ||
| /// | ||
| /// Must not be larger than `32` |
There was a problem hiding this comment.
This isn't used for IPv6 addresses? There they go up to /64 or /128 even.
Maybe you don't even really need a limit, if the netmask is larger then the address it is the equivalent of the netmask of all ones after all.
There was a problem hiding this comment.
fixed already
iroh/src/magicsock/transports/ip.rs
Outdated
| /// IP transport configuration | ||
| #[derive(Debug, Copy, Clone, PartialEq, Eq)] | ||
| pub enum Config { | ||
| /// Default IPv4 binding |
There was a problem hiding this comment.
Needs to elaborate what "default" means here. I assume it means that this will be the default route for initiated outgoing packets that don't match the subnet of any other bind.
I do think splitting V4 anv V4Default here is logically a bit wrong. Because each bound socket bound on a specific local interface does have the subnet of that interface, regardless of whether it is used as default if no subnets match or not. So I'd rather have an is_default in the fields of V4 & V6.
There was a problem hiding this comment.
So I'd rather have an is_default in the fields of V4 & V6.
they store different information at this point, which is why they are different enum variants
There was a problem hiding this comment.
I think what @flub is suggesting is removing Config::V*Default cases, and adding an is_default: bool.
And then representing e.g. Config::V4Default { ip_addr: "0.0.0.0".parse()?, port: 1234, is_required: true } as
Config::V4 { ip_net: "0.0.0.0/0".parse()?, port: 1234, is_required: true, is_default: true }.
So, in short, representing the old default case as having prefix len 0.
iroh/src/magicsock/transports/ip.rs
Outdated
| /// General IPv4 binding | ||
| V4 { | ||
| /// The IP address to bind on | ||
| ip_addr: Ipv4Net, |
There was a problem hiding this comment.
nit, call this field ip_net or even net. This also shows up in the functions where you use this where it's suddenly confusing to read ip_addr.add() or ip_addr.contains(an_actual_ip_addr).
| let socket = bind_with_fallback(bind_addr)?; | ||
| Ok(Self::new(bind_addr, Arc::new(socket), metrics.clone())) | ||
| pub(crate) fn bind(config: Config, metrics: Arc<MagicsockMetrics>) -> io::Result<Self> { | ||
| let socket = bind_with_fallback(config.into())?; |
There was a problem hiding this comment.
I find this very surprising that someone passed in a Config with a specific port but it will now potentially ignore this port. For folks who configure things accurately this is bad behaviour.
There was a problem hiding this comment.
this has been the behaviour for a long time, so changing this should be a separate discussion imho
There was a problem hiding this comment.
It has been the default for a long time and when users notice it has been really surprising to them!
The API we're adding is explicitly to give more control over what is exactly bound. I think this should also be exposed in a BindOpts field so that you get control over it. And by default this should be set to not fallback as otherwise this will be very surprising to users.
If a user does not manually call any Builder::bind_addr* functions then setting that bind option to use the fallback on the two IP transports added in that case is reasonable.
There was a problem hiding this comment.
will do this change in a follow up
| } | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
IIUC if we are CPU bound and can not keep up we'll prefer v4_default, then v4, then v6_default and then v6. I guess that's fine for now, but could we leave a TODO or some comment that this bias exists?
iroh/src/endpoint.rs
Outdated
| /// | ||
| /// Only a single interface can be the default, per IP family, so this errors out if one is already set. | ||
| /// | ||
| /// If you want to remove the default transports, make sure to call `clear_ip` first. |
There was a problem hiding this comment.
How empty is a Buidler::empty_builder? I think right now it still includes these default binds, which honestly is a bit confusing.
Also, having to call clear_ip is quite the footgun. I think it would be more logical from the user's view if we always created a builder without any pre-configured binds. And only if the user did not call any bind_addr* method before calling Builder::bind then add the IPv4 UNSPECIFIED and optional IPv6 UNSPECIFIED binds.
There was a problem hiding this comment.
And only if the user did not call any bind_addr* method before calling Builder::bind then add the IPv4 UNSPECIFIED and optional IPv6 UNSPECIFIED binds.
I tried that, that doesn't work, as we have a test and use case for not having any IP transport already
There was a problem hiding this comment.
Also, having to call clear_ip is quite the footgun
we have it for other things like discovery, and it really is needed to make sure for example things from presets are removed
There was a problem hiding this comment.
How empty is a Buidler::empty_builder? I think right now it still includes these default binds, which honestly is a bit confusing.
it is as empty as we can make it, while still being able to do something when starting it
There was a problem hiding this comment.
we could make it fully empty, and add the default binds using the n0 preset
There was a problem hiding this comment.
Consider that today you can call clear_relay_transport and then you can not add a relay transport back (you can only add a relay transport by using Builder::empty(RelayMode)) I would propose to rename clear_relay_transport and clear_ip_transport into disable_*_transport and then this is solved.
matheus23
left a comment
There was a problem hiding this comment.
Yay routing tables and interfaces.
The "default_ipv{4,6}" I commented on can be ignored. "Default" here is an overloaded term. It's fine to keep it as is (it's not public API, we can improve it later)
## Description The new user-defined IP transports added #3692 don't actually work because they are filtered out in `Transports::bind`. Here's a fix, and a test that tests the currently implemented behavior. The test fails without this fix. ## Breaking Changes <!-- Optional, if there are any breaking changes document them, including how to migrate older code. --> ## Notes & open questions <!-- Any notes, remarks or open questions you have to make about the PR. --> ## Change checklist <!-- Remove any that are not relevant. --> - [ ] Self-review. - [ ] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant. - [ ] Tests if relevant. - [ ] All breaking changes documented. - [ ] List all breaking changes in the above "Breaking Changes" section. - [ ] Open an issue or PR on any number0 repos that are affected by this breaking change. Give guidance on how the updates should be handled or do the actual updates themselves. The major ones are: - [ ] [`quic-rpc`](https://github.com/n0-computer/quic-rpc) - [ ] [`iroh-gossip`](https://github.com/n0-computer/iroh-gossip) - [ ] [`iroh-blobs`](https://github.com/n0-computer/iroh-blobs) - [ ] [`dumbpipe`](https://github.com/n0-computer/dumbpipe) - [ ] [`sendme`](https://github.com/n0-computer/sendme)
Description
This allows for multiple interfaces to be bound, and be actually used.
You can now use this by passing
The selection of the interface is done internally by first looking at all specific bindings, and then fallbing back to the
defaultversion for this family.Closes #3218
Breaking Changes
irohEndpoint::bind_addr_v4Endpoint::bind_addr_v6Endpoint::bind_addrEndpoint::bind_addr_with_optsendpoint::BindOptsendpoint::ToSocketAddr