Repository navigation
Conversation
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/4400/docs/iroh/ Last updated: 2026-10-28T09:24:07Z |
0db5c01 to
119035b
Compare
|
Would this help with #4386 ? |
I have not looked at this PR but it depends on how it is done: The point of multiple QAD connections is that you can detect EIM from EDM NATs. So you need at least two to be able to tell. So the question is how many QAD connections is this keeping open at the same time? Do you gain anything from keeping only one open if you are on an EIM NAT? Probably not because when you change network you want to check again. |
|
Another consideration with this is that just keeping a connection open will not immediately give you a new QAD address when you move. The client needs to send something to the server before it can detect a new address. So this likely needs the ability to send a PING when iroh detects a network change. At this point there is nothing in terms of RTT that is between the streaming and non-streaming versions. Because QAD already sends the OBSERVED_ADDRESS at 0.5 RTT time. It could be however that it is less work to keep open an existing connection compared to always starting a new one. I suspect that would be the primary gain from this. (again, i've not looked at the PR itself yet) |
13fcd0f to
f3f0e15
Compare
f3f0e15 to
9df0665
Compare
…0-computer#4463) ## Description Closes n0-computer#4111. `Endpoint::Builder::proxy_url()` and `Builder::proxy_from_env()` were only ever honoured by the relay client. The value was cloned into `RelayActorConfig` and stopped there — `net_report` never received it. As a result both HTTP(S) based probes connected directly, ignoring the proxy the user explicitly configured: - the HTTPS latency probe (`run_https_probe`) - the captive portal check (`check_captive_portal`) ### User-visible failure On a network where direct egress is blocked and all traffic must go through a proxy, every HTTPS probe fails. The QAD probes fail too (no UDP/QUIC egress), so the resulting report has empty `relay_latency` and `preferred_relay: None`. The endpoint never selects a home relay and the node stays undialable over the relay — even though the relay client itself was perfectly capable of connecting through the proxy. The reporter's logs on n0-computer#4111 show exactly this. ## Fix Thread the proxy URL from `socket::Options` through `net_report::Options` and `reportgen::SocketState` down to both `reqwest::ClientBuilder` call sites — the same path the DNS resolver already travels, so the probes' egress configuration now lives in one place. A few deliberate choices: - **`SocketState`, not a new `Client` field.** `dns_resolver` is already carried there, and `reportgen::Client::new` already needs `#[allow(clippy::too_many_arguments)]`; this leaves that signature unchanged. - **A bad proxy URL is reported, not ignored.** `reqwest::Proxy::all` is fallible, and its error is mapped onto the existing `CreateReqwestClient` variant on both probe error enums (no new variants). The probe fails loudly rather than silently falling back to a direct connection — quietly probing past the proxy the user asked for would be worse than failing. This matches the relay path, where a bad proxy surfaces as a `DialError` at dial time. - **No new dependency or feature.** HTTP `CONNECT` proxying is core reqwest; only SOCKS sits behind the `socks` feature, which this does not need. - **`wasm_browser` is untouched.** Every added line is inside a `#[cfg(not(wasm_browser))]` item or block, and the browser `Options` variant is unchanged. Browsers have no `SocketState` to carry a proxy on and defer to the user agent's own proxy settings anyway. The comment above `run_https_probe` pointing at (now-closed) n0-computer#2901 describes this exact gap. This closes the proxy half of it; the probes still don't share the relay client's connection establishment, so the comment is left in place. ## Breaking Changes None. No public API changes — `Builder::proxy_url` already existed, it just wasn't wired up. All new fields and methods are `pub(crate)` / `pub(super)`. ## Testing New test `net_report::reportgen::tests::test_measure_https_latency_via_proxy` stands up a fake HTTP proxy on an ephemeral localhost port, points an HTTPS probe at a real test relay, and asserts the proxy received a `CONNECT` for the relay's authority — and that the probe does not succeed when the proxy refuses to tunnel. Verified red first. Without the fix: ``` probe must not succeed through a proxy that refuses to tunnel, got Ok(HttpsProbeReport { relay: RelayUrl("https://127.0.0.1:58448/"), latency: 4.539584ms }) ``` with the relay's own log showing `connection opened from 127.0.0.1:58452` — the probe dialling direct, which is the bug in one line. - `cargo test -p iroh --lib`: 112 → 113 passing, 0 failures before and after - `cargo clippy --workspace --all-targets --lib --bins --tests --benches --examples` in all three CI configurations (default, `--all-features`, `--no-default-features`): clean - `cargo make format-check`: clean - `cargo doc -p iroh --all-features --no-deps --document-private-items`: clean Not run: the `wasm32-unknown-unknown` target build, as that target isn't installed locally. Every added line is behind `#[cfg(not(wasm_browser))]` and I audited each one, but worth a CI confirmation. ## Notes and open questions - `iroh/src/address_lookup/pkarr.rs` builds its `reqwest` clients the same way and has the same gap — pkarr HTTP traffic also bypasses a configured proxy. Left out to keep this PR focused; happy to open a follow-up issue or fold it in here if you'd prefer. - **Collision note:** n0-computer#4400 (streaming netreport) rewrites this module and splits out `https.rs` / `captive_portal.rs`. It's currently a draft with conflicts. This fix is small and self-contained, so it should land cleanly on `main` first and relocate into that refactor with minimal effort — the proxy just needs to follow whichever struct inherits `SocketState`'s role. ## Change checklist - [x] Self-review. - [x] Documentation updates following the style guide, if relevant. - [x] Tests if relevant. - [x] All breaking changes documented. --------- Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Floris Bruynooghe <flub@n0.computer>
…0-computer#4463) ## Description Closes n0-computer#4111. `Endpoint::Builder::proxy_url()` and `Builder::proxy_from_env()` were only ever honoured by the relay client. The value was cloned into `RelayActorConfig` and stopped there — `net_report` never received it. As a result both HTTP(S) based probes connected directly, ignoring the proxy the user explicitly configured: - the HTTPS latency probe (`run_https_probe`) - the captive portal check (`check_captive_portal`) ### User-visible failure On a network where direct egress is blocked and all traffic must go through a proxy, every HTTPS probe fails. The QAD probes fail too (no UDP/QUIC egress), so the resulting report has empty `relay_latency` and `preferred_relay: None`. The endpoint never selects a home relay and the node stays undialable over the relay — even though the relay client itself was perfectly capable of connecting through the proxy. The reporter's logs on n0-computer#4111 show exactly this. ## Fix Thread the proxy URL from `socket::Options` through `net_report::Options` and `reportgen::SocketState` down to both `reqwest::ClientBuilder` call sites — the same path the DNS resolver already travels, so the probes' egress configuration now lives in one place. A few deliberate choices: - **`SocketState`, not a new `Client` field.** `dns_resolver` is already carried there, and `reportgen::Client::new` already needs `#[allow(clippy::too_many_arguments)]`; this leaves that signature unchanged. - **A bad proxy URL is reported, not ignored.** `reqwest::Proxy::all` is fallible, and its error is mapped onto the existing `CreateReqwestClient` variant on both probe error enums (no new variants). The probe fails loudly rather than silently falling back to a direct connection — quietly probing past the proxy the user asked for would be worse than failing. This matches the relay path, where a bad proxy surfaces as a `DialError` at dial time. - **No new dependency or feature.** HTTP `CONNECT` proxying is core reqwest; only SOCKS sits behind the `socks` feature, which this does not need. - **`wasm_browser` is untouched.** Every added line is inside a `#[cfg(not(wasm_browser))]` item or block, and the browser `Options` variant is unchanged. Browsers have no `SocketState` to carry a proxy on and defer to the user agent's own proxy settings anyway. The comment above `run_https_probe` pointing at (now-closed) n0-computer#2901 describes this exact gap. This closes the proxy half of it; the probes still don't share the relay client's connection establishment, so the comment is left in place. ## Breaking Changes None. No public API changes — `Builder::proxy_url` already existed, it just wasn't wired up. All new fields and methods are `pub(crate)` / `pub(super)`. ## Testing New test `net_report::reportgen::tests::test_measure_https_latency_via_proxy` stands up a fake HTTP proxy on an ephemeral localhost port, points an HTTPS probe at a real test relay, and asserts the proxy received a `CONNECT` for the relay's authority — and that the probe does not succeed when the proxy refuses to tunnel. Verified red first. Without the fix: ``` probe must not succeed through a proxy that refuses to tunnel, got Ok(HttpsProbeReport { relay: RelayUrl("https://127.0.0.1:58448/"), latency: 4.539584ms }) ``` with the relay's own log showing `connection opened from 127.0.0.1:58452` — the probe dialling direct, which is the bug in one line. - `cargo test -p iroh --lib`: 112 → 113 passing, 0 failures before and after - `cargo clippy --workspace --all-targets --lib --bins --tests --benches --examples` in all three CI configurations (default, `--all-features`, `--no-default-features`): clean - `cargo make format-check`: clean - `cargo doc -p iroh --all-features --no-deps --document-private-items`: clean Not run: the `wasm32-unknown-unknown` target build, as that target isn't installed locally. Every added line is behind `#[cfg(not(wasm_browser))]` and I audited each one, but worth a CI confirmation. ## Notes and open questions - `iroh/src/address_lookup/pkarr.rs` builds its `reqwest` clients the same way and has the same gap — pkarr HTTP traffic also bypasses a configured proxy. Left out to keep this PR focused; happy to open a follow-up issue or fold it in here if you'd prefer. - **Collision note:** n0-computer#4400 (streaming netreport) rewrites this module and splits out `https.rs` / `captive_portal.rs`. It's currently a draft with conflicts. This fix is small and self-contained, so it should land cleanly on `main` first and relocate into that refactor with minimal effort — the proxy just needs to follow whichever struct inherits `SocketState`'s role. ## Change checklist - [x] Self-review. - [x] Documentation updates following the style guide, if relevant. - [x] Tests if relevant. - [x] All breaking changes documented. --------- Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Floris Bruynooghe <flub@n0.computer>
Move the probe-execution code into dedicated submodules: QUIC address discovery (connections, probes, relay-address resolution) into `qad`, HTTPS latency measurement into `https`, and captive portal detection into `captive_portal`. The reportgen actor keeps orchestrating; it now calls into these modules. This is a pure code move with no behavioral change. Item bodies are unchanged; only module paths and item visibility are adjusted. It prepares for the streaming net_report rewrite so that change stays focused on behavior rather than file reorganization.
Run probes and interface monitoring in a self-driving actor. Publish address, home-relay, and network-change outputs through a client that owns task lifetime and update cursors. Prepare early subscriptions through a consuming builder. Keep relay failover, network readiness, address composition, and portmapper lifecycle changes together with their regression coverage. Use contextual probe errors and share HTTP probe client setup.
Exercise network readiness, probe outcomes, retained addresses, relay failover, and debug report publication.
Cover home relay failover, local address updates without relays, and NAT mapping changes. Add support for labs with multiple relays and enable netreport observations in the Patchbay task.
9df0665 to
bae422d
Compare
Description
(early draft, not yet fully reviewed)
motivation: make net report streaming so that we are informed about changing direct addrs immediately. but also keep the cycle-based model to have checkpoints where to potentially switch home relays. do this via a single long-running net report actor instead of a per-round actor managed in the socket state. also a preparation for reusing qad conns for h3 or the HTTPS probe TLS streams for the actual relay conns (this is not implemented yet).
best to look at commit-by-commit:
1 - only moving items around as preparation for the larger diff that follows. no other changes.
2 - the actual impl
3 - new patchbay tests
Breaking Changes
None
Notes & open questions
Change checklist
proposed change and wrote an as clear and concise description as
they could.
intented effect.