Skip to content

Commit 25d1d31

Browse files
committed
refactor(rust): read the proxy environment through hyper-util's matcher
`hyper_util` ships `client::proxy::matcher::Matcher` behind its `client-proxy` feature: the `*_PROXY` convention including `ALL_PROXY`, `NO_PROXY` matched the way curl matches it, and credentials taken out of a proxy URL. This crate had written all of that again, so `resolve_proxy`, `system_proxy`, `read_env_first` and `no_proxy_matches` are gone, and `system` now means what it means for any hyper-based client. Only `system` goes through the matcher. It answers `None` for a target with no host or a scheme other than `http`/`https`, so routing an explicitly named proxy through it would silently skip the proxy the caller configured; that path dials its URI directly, as it did before. The tunnel stays ours. `hyper_util`'s reads the `CONNECT` response as if recvd.starts_with(b"HTTP/1.1 200") || recvd.starts_with(b"HTTP/1.0 200") else if recvd.starts_with(b"HTTP/1.1 407") ... else return Err(TunnelError::TunnelUnsuccessful) so a status line split across reads is rejected although the tunnel is good, and `HTTP/1.0 407` loses its meaning. Ours reads to the blank line before parsing. `basic_auth` now returns the whole header value, scheme included, so that it and `Intercept::basic_auth` are interchangeable at the call site. Getting that wrong sent `Basic Basic <b64>` and was caught by the authentication tests. Measured, from `packages/rust`: $ cargo test -p armonik-transport --all-features 70 passing $ cargo clippy --workspace --all-features --all-targets --no-deps -- -Dwarnings $ cargo clippy --workspace --all-features --no-deps \ -- -Dwarnings -Dunused-crate-dependencies $ RUSTDOCFLAGS="-Dwarnings" cargo doc --workspace --no-deps $ cargo build --workspace --locked $ cargo build -p armonik-transport $ cargo fmt --all --check all silent proxy.rs goes from 730 to 611 lines. Prompted by a maintainer asking whether hyper-util already did this.
1 parent 8f1c836 commit 25d1d31

5 files changed

Lines changed: 193 additions & 196 deletions

File tree

packages/rust/Cargo.lock

Lines changed: 9 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/rust/armonik-transport/Cargo.toml

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,9 @@ tonic = { workspace = true, features = ["channel", "codegen"] }
2020
snafu.workspace = true
2121
tracing.workspace = true
2222
hyper = { workspace = true, features = ["client", "http1", "http2"] }
23-
hyper-util = { workspace = true, features = ["client", "http1"] }
23+
# `client-proxy` for the proxy matcher: the `*_PROXY` convention, `NO_PROXY` on curl's rules, and
24+
# taking credentials out of a proxy URL, none of which is worth writing again here.
25+
hyper-util = { workspace = true, features = ["client", "client-proxy", "http1"] }
2426
hyper-rustls = { workspace = true, features = [
2527
"http1",
2628
"http2",

packages/rust/armonik-transport/src/config.rs

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,12 @@ pub enum ProxySource {
1515
/// The default, so that adding proxy support changes nothing for a client that never asked for it.
1616
#[default]
1717
Disabled,
18-
/// Read the proxy from `HTTPS_PROXY`, `HTTP_PROXY` and `NO_PROXY`, in either case.
18+
/// Read the proxy from the environment, on `hyper_util`'s rules: `ALL_PROXY`, `HTTPS_PROXY`,
19+
/// `HTTP_PROXY` and `NO_PROXY`, in either case, with `NO_PROXY` matched as curl matches it.
20+
///
21+
/// Read when `connect` builds the channel, not when this is built and not again afterwards: a
22+
/// variable changed in between is the one that counts, and a channel that reconnects keeps the
23+
/// values it started with. Every other option is read in [`ClientConfigArgs::from_env`].
1924
System,
2025
/// Use this specific proxy.
2126
Explicit(Uri),
@@ -213,8 +218,8 @@ pub struct ClientConfigArgs {
213218
pub user_agent: String,
214219
/// HTTP proxy to reach the endpoint through.
215220
///
216-
/// Empty for a direct connection, `none` to disable proxying explicitly, `system` to read
217-
/// `HTTPS_PROXY`/`HTTP_PROXY`/`NO_PROXY` from the environment, otherwise the proxy URL.
221+
/// Empty for a direct connection, `none` to disable proxying explicitly, `system` to read the
222+
/// environment (see [`ProxySource::System`]), otherwise the proxy URL.
218223
#[cfg_attr(feature = "serde", serde(default))]
219224
pub proxy: String,
220225
/// Username for proxy authentication.

0 commit comments

Comments
 (0)