feat(iroh-net): Implement the https probe #2903
Conversation
This implements the https probe. There does not need to be separate IPv4 or IPv6 HTTPS probes, because we just go with whatever the HTTPS connection works over. This is what we need to establish a relay connection after all, this also does not care. The only reason for this is to get some kind of latency to be able to chose a relay server. Luckily the https probes have been integrated in the probe plan a long time so nothing to do there. Currently this uses reqwest with it's default DNS resolution. This is not great as it is not the same DNS resolution used everywhere else. It is however the same as done in the captive portal check. This is worth at least looking at before merging this.
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2903/docs/iroh/ Last updated: 2024-11-06T16:55:50Z |
| @@ -67,7 +69,7 @@ pub async fn run_relay_server_with( | |||
| metrics_addr: None, | |||
| }; | |||
| let server = Server::spawn(config).await.unwrap(); | |||
| let url: RelayUrl = format!("https://localhost:{}", server.https_addr().unwrap().port()) | |||
| let url: RelayUrl = format!("https://{}", server.https_addr().expect("configured")) | |||
There was a problem hiding this comment.
Notice that this changes the relay URL for tests to be the IP address. This is needed to avoid the DNS lookups for localhost: depending on the host configuration this can result in IPv4 or IPv6 addresses but we want to have just IPv4 because that's what the server listens on.
| .await? | ||
| .map(|ipaddr| SocketAddr::new(ipaddr, 80)) | ||
| .collect(); | ||
| builder = builder.resolve_to_addrs(domain, &addrs); |
There was a problem hiding this comment.
Another option is to implement reqwests' Resolver trait for our resolver. But we don't own the type so that's quite a bit of changes. This keeps our changes targeted to this feature and is not really any worse.
There was a problem hiding this comment.
I really like the idea of being able to keep using reqwest and implementing our custom Resolver, perhaps allowing users to provide their own Resolver if they want to, perhaps under an unstable feature flag.
Is it feasible to lean in to reqwest even more heavily? It seems like it supports HTTP upgrades and thus we could use it in even more places.
If we manage to replace all usage of direct hyper with reqwest, we might be able to have one universal way of configuring custom DNS resolvers or HTTPS proxying.
There was a problem hiding this comment.
https://docs.rs/iroh-net/latest/iroh_net/endpoint/struct.Builder.html#method.dns_resolver is the current way to configure the DNS resolver for users. That's what this is picking up.
I don't have a lot of opinion on reqwest use, I've seen people remove it in favour of more low-level control and to reduce the dependency count. We also need to take into account #2901.
But if we can do everything with reqwest and there's benefits to it then I don't have anything against it.
flub
requested review from
dignifiedquire,
matheus23,
divagant-martian and
ramfox
| let addrs: Vec<_> = dns_resolver | ||
| .lookup_ipv4_ipv6_staggered(domain, DNS_TIMEOUT, DNS_STAGGERING_MS) | ||
| .await? | ||
| .map(|ipaddr| SocketAddr::new(ipaddr, 80)) |
There was a problem hiding this comment.
@flub I think you should probably set these ports to 0, if I'm understanding this issue correctly.
There was a problem hiding this comment.
The docs have seemingly not been updated yet. I'll look into the source if they're actually unused or not now.
There was a problem hiding this comment.
Good catch! Also, bad reqwest! #2906
There was a problem hiding this comment.
Sorry... the change was in hyper_util not in reqwest so that is why the documentation stayed outdated. seanmonstar/reqwest#2473
There was a problem hiding this comment.
In my defense, this change in hyper_util is really useful to leverage HTTPS records in custom resolvers. Even more work needed on Reqwest side to decide when to switch to HTTP2/3 according to the ALPN property in HTTPS records.
There was a problem hiding this comment.
All good. IMO it's more weird to pass in bogus info that's unused. This is what made me skeptical in the first place.
There was a problem hiding this comment.
Also ugh I didn't quite follow the data-flow in reqwest for how these domain overrides are used. It goes through some Resolver and tower traits and just gets complicated quickly. So I'm glad to see someone who knows just update the docs.
There was a problem hiding this comment.
Yeah, no worries. These things can be hard to catch, even the maintainer missed it. Thanks for following up on the docs!
And yes, that work you're doing there is certainly cool and useful!
Description
This implements the https probe.
There does not need to be separate IPv4 or IPv6 HTTPS probes, because we just go with whatever the HTTPS connection works over. This is what we need to establish a relay connection after all, this also does not care. The only reason for this is to get some kind of latency to be able to chose a relay server.
Luckily the https probes have been integrated in the probe plan a long time so nothing to do there.
Breaking Changes
The URLs served by the relay changed:
/relay/probehas moved to/ping/derp/probehas been removed.Unless you were manually using those URLs you will not notice these changes, nothing in the iroh codebase ever used the changed URLs.
Notes & open questions
Currently this uses reqwest with it's default DNS resolution. This is not great as it is not the same DNS resolution used everywhere else. It is however the same as done in the captive portal check. This is worth at least looking at before merging this.
Change checklist