feat(rust): reach the endpoint through an HTTP CONNECT proxy - #696
Closed
wkirschenmann wants to merge 12 commits into
Closed
feat(rust): reach the endpoint through an HTTP CONNECT proxy#696wkirschenmann wants to merge 12 commits into
wkirschenmann wants to merge 12 commits into
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
wkirschenmann
force-pushed
the
wk/feat/rust-proxy
branch
from
August 1, 2026 13:44
158a134 to
8f1c836
Compare
wkirschenmann
force-pushed
the
wk/fix/rust-apply-timeout-and-rate-limit
branch
from
August 1, 2026 15:19
1d2ebea to
8a89cd9
Compare
wkirschenmann
force-pushed
the
wk/feat/rust-proxy
branch
from
August 1, 2026 15:19
25d1d31 to
43f90b1
Compare
wkirschenmann
marked this pull request as ready for review
August 1, 2026 16:12
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43f90b13c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
wkirschenmann
force-pushed
the
wk/fix/rust-apply-timeout-and-rate-limit
branch
from
August 1, 2026 16:22
8a89cd9 to
6ff580b
Compare
wkirschenmann
force-pushed
the
wk/feat/rust-proxy
branch
2 times, most recently
from
August 1, 2026 17:48
f452722 to
6008eb8
Compare
Base automatically changed from
wk/fix/rust-apply-timeout-and-rate-limit
to
main
August 1, 2026 18:40
Contributor
|
It should be implemented using https://docs.rs/hyper-util/latest/hyper_util/client/legacy/connect/proxy/struct.Tunnel.html |
wkirschenmann
force-pushed
the
wk/feat/rust-proxy
branch
from
August 2, 2026 00:13
5ae495d to
d1701ad
Compare
Contributor
Author
|
Tried, and kept ours for two defects in 0.1.20, the latest published version. A status line arriving split across reads falls into the |
Contributor
Author
|
Added as a test rather than a paragraph, in 4ce1829: it drives your |
Contributor
|
I would go in the other direction: The defects you observe are not blocking, so I would go with hyper-utils proxy, contribute the fixes, and be aware that we have those defects until upstream is fixed. |
wkirschenmann
marked this pull request as draft
August 2, 2026 10:09
wkirschenmann
force-pushed
the
wk/feat/rust-proxy
branch
from
August 2, 2026 15:03
4ce1829 to
f264853
Compare
wkirschenmann
added a commit
that referenced
this pull request
Aug 2, 2026
…written here A maintainer asked, on #696, to use `hyper_util::client::legacy::connect::proxy::Tunnel` for the handshake instead of the hand-written one, accepting its known defects as non-blocking rather than carrying ~150 lines this crate does not have to own. Followed: `tunnel()`, `status_code()` and `target_authority()` are gone, and `ProxyConnector::call` builds a `Tunnel::new(proxy_uri, self.inner.clone())` per call, which is why `S: Clone` joins the bound. `Tunnel` has no timeout of its own, so the existing 30s `tokio::time::timeout` around the handshake stays. `Tunnel`'s error type, `pub enum TunnelError`, sits in a private `mod tunnel` of `hyper-util` and has no path outside that crate: nothing here can match its variants. `translate` is generic over `impl Error + Send + Sync` instead, and recovers two of the old, more actionable messages by testing `error.to_string()` for hyper-util's own fixed wording. Documented as brittle in its own comment: each case is pinned by an integration test, so a wording change upstream breaks the test loudly rather than silently losing the hint. Delegating the handshake reveals four cases `Tunnel` gets wrong, none of them this crate's to fix, each pinned by a `known_issue_*` test so a `hyper-util` release that fixes one turns that test red: - only an exact `200` opens the tunnel, not any `2xx` as RFC 9110 asks; - a status line split across two reads is rejected, though the connection is fine; - an `HTTP/1.0 407` is not recognised as a request for credentials, only `HTTP/1.1 407` is; - a target naming no port is dialled on `443` regardless of scheme, not `80` for `http`. The third and fourth were found while writing this change, not asked for by the maintainer; both are listed in the README's new "Known issues" section alongside the two under discussion. The fourth test needed its own minimal proxy stub: reusing the existing fake proxy meant really dialling the documentation-space test target, which neither refuses nor answers, so the test spent 21s on a genuine TCP connect timeout before it could assert anything. The replacement records the `CONNECT` line and closes without dialling, and runs in 30ms. `tests/upstream_tunnel.rs`, a canary added earlier that drives `Tunnel` directly, is reworded: it no longer argues for keeping this crate's own tunnel, since there no longer is one, only for keeping the README's "Known issues" and the sibling test in tests/proxy.rs in sync with reality. cargo test -p armonik-transport --all-features: 73 passed, 0 failed. clippy --workspace --all-features --all-targets -Dwarnings, cargo fmt --all --check, RUSTDOCFLAGS=-Dwarnings cargo doc: clean.
Contributor
Author
done. |
wkirschenmann
marked this pull request as ready for review
August 2, 2026 16:21
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc62f6e0cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
7 tasks
`GrpcClient__Proxy`, `GrpcClient__ProxyUsername` and `GrpcClient__ProxyPassword`
are part of ArmoniK's client configuration and did nothing here, so a client
behind a corporate proxy could not reach a cluster at all.
A `ProxyConnector` sits between the TCP connector and the TLS one and opens a
`CONNECT` tunnel, so TLS stays end to end with the real server and the proxy only
forwards opaque bytes. Terminating TLS at the proxy would defeat the point of a
transport that exists to control the TLS stack.
Reading the environment is `hyper_util`'s: `client::proxy::matcher::Matcher`
already implements the `*_PROXY` convention including `ALL_PROXY`, matches
`NO_PROXY` the way curl does, and takes credentials out of a proxy URL. Only
`system` goes through it, because it answers `None` for a target with no host or
a scheme other than `http`/`https`, which would silently skip an explicitly named
proxy.
The tunnel is 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.
Measured, from `packages/rust`:
$ cargo test -p armonik-transport --all-features
69 passing, 11 of them driving a real client through a real CONNECT proxy
$ # with the proxy configuration prevented from reaching the connector
7 of the 8 tunnel tests fail; the eighth asserts the absence of proxying
$ 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 -p armonik-transport
$ cargo fmt --all --check
all silent
The split is on the last `@` because a password may contain a slash. Why it is not on the authority is not the reader's problem.
…nused half
`Revealed` and its serialisation in clear are gone. They existed so a caller could
write a configuration out and read it back, and no such caller exists: nothing in
the repository enables the `serde` feature, the FFI layer passes typed fields
across a C ABI, and the configuration comes from environment variables. The only
round-trip was the test written for it.
What is left is complete. `Debug` and `Serialize` redact; `Deserialize` refuses the
redaction marker, so reading back a dump fails where it can be understood rather
than as an unexplained rejection later.
`Deref` is gone too, which made the type match its own documentation: `&*password`
handed the value out with no call to anything. Emptiness, the only thing the call
sites wanted, has its own method. The accessor is `expose_secret`, named in full so
that a call site reads as the deliberate act it is.
`rustls` guards a private key the same way, verified in rustls-pki-types 1.15.1:
no `Deref`, no `AsRef`, a named `secret_*_der()` accessor, and a `Debug` that
elides.
The comments this branch adds were revised as a whole rather than only where the
code changed: 200 lines audited, the four longest blocks cut.
$ cargo test -p armonik-transport --all-features
69 passing
…each
Two defects reported by the automated reviewer.
A compliant proxy may answer `CONNECT` with any 2xx, which RFC 9110 says switches
the connection to tunnel mode. Requiring exactly 200 reported such a proxy as
having refused.
An `https` proxy URL was accepted and then reached in the clear: the handshake is
written onto whatever the inner connector opens, and the TLS layer above applies
to the target rather than to the proxy, so a proxy expecting TLS would see
gibberish and the failure would read as an unreachable proxy. It is refused now,
at both points where the scheme is known: while parsing an explicit
`GrpcClient__Proxy`, and before dialling one that came from the environment.
Measured, from `packages/rust`, against the unfixed code as well:
$ cargo test -p armonik-transport --all-features
72 passing
$ # with the range narrowed back to 200 and the scheme check removed
test a_success_other_than_200_still_opens_the_tunnel ... FAILED
test an_https_proxy_from_the_environment_is_refused_before_dialling ... FAILED
test result: FAILED. 11 passed; 2 failed
…agraph
This crate tunnels `CONNECT` itself because `hyper_util`'s `Tunnel` gets two cases wrong: a
status line arriving split across reads falls into its catch-all refusal, and only `200` opens
the tunnel where RFC 9110 says any 2xx does. Both are on hyper-util's main branch today, not
only in the published 0.1.20.
Written down, that justification goes stale silently. As a test it expires by itself: the day
a release fixes either, this goes red and says what to delete. `tunnel_through` is also the
shape the replacement would take, so the work is half done when the notice arrives.
Measured against `Tunnel`, with an unsplit 200 as the control:
plain 200: ACCEPTED, as it should be
201: REFUSED with `tunnel error: unsuccessful`
split: REFUSED with `tunnel error: unsuccessful`
The firing path was checked by making the 201 case answer 200:
hyper-util now opens the tunnel on a 2xx other than 200. Good news: check the
split-status-line case too, and if that is fixed as well, delete this crate's `tunnel` and
use `hyper_util`'s. See #699.
Nagle is off on the fixture's socket and the halves are 50ms apart, so a coalesced read cannot
make the split case pass for the wrong reason; the message names that possibility anyway,
since a false "upstream fixed it" is the one failure worth guarding against.
`client-legacy` joins the declared features. This crate names `HttpConnector`, which lives
behind it, and until now the feature was only on because `hyper-rustls` turns it on.
`Cargo.lock` is unchanged: nothing new enters the graph.
cargo test -p armonik-transport --all-features: 75 passed, 0 failed, and the new target five
times over. clippy --all-targets --all-features -Dwarnings, cargo fmt --all --check, cargo
build --workspace --locked --all-features: clean.
…written here A maintainer asked, on #696, to use `hyper_util::client::legacy::connect::proxy::Tunnel` for the handshake instead of the hand-written one, accepting its known defects as non-blocking rather than carrying ~150 lines this crate does not have to own. Followed: `tunnel()`, `status_code()` and `target_authority()` are gone, and `ProxyConnector::call` builds a `Tunnel::new(proxy_uri, self.inner.clone())` per call, which is why `S: Clone` joins the bound. `Tunnel` has no timeout of its own, so the existing 30s `tokio::time::timeout` around the handshake stays. `Tunnel`'s error type, `pub enum TunnelError`, sits in a private `mod tunnel` of `hyper-util` and has no path outside that crate: nothing here can match its variants. `translate` is generic over `impl Error + Send + Sync` instead, and recovers two of the old, more actionable messages by testing `error.to_string()` for hyper-util's own fixed wording. Documented as brittle in its own comment: each case is pinned by an integration test, so a wording change upstream breaks the test loudly rather than silently losing the hint. Delegating the handshake reveals four cases `Tunnel` gets wrong, none of them this crate's to fix, each pinned by a `known_issue_*` test so a `hyper-util` release that fixes one turns that test red: - only an exact `200` opens the tunnel, not any `2xx` as RFC 9110 asks; - a status line split across two reads is rejected, though the connection is fine; - an `HTTP/1.0 407` is not recognised as a request for credentials, only `HTTP/1.1 407` is; - a target naming no port is dialled on `443` regardless of scheme, not `80` for `http`. The third and fourth were found while writing this change, not asked for by the maintainer; both are listed in the README's new "Known issues" section alongside the two under discussion. The fourth test needed its own minimal proxy stub: reusing the existing fake proxy meant really dialling the documentation-space test target, which neither refuses nor answers, so the test spent 21s on a genuine TCP connect timeout before it could assert anything. The replacement records the `CONNECT` line and closes without dialling, and runs in 30ms. `tests/upstream_tunnel.rs`, a canary added earlier that drives `Tunnel` directly, is reworded: it no longer argues for keeping this crate's own tunnel, since there no longer is one, only for keeping the README's "Known issues" and the sibling test in tests/proxy.rs in sync with reality. cargo test -p armonik-transport --all-features: 73 passed, 0 failed. clippy --workspace --all-features --all-targets -Dwarnings, cargo fmt --all --check, RUSTDOCFLAGS=-Dwarnings cargo doc: clean.
`ProxyConnector` used the dedicated username/password as a whole pair, falling back to the credentials the matcher intercepted from `HTTP_PROXY`/`HTTPS_PROXY` only when both were empty. Setting only one of `GrpcClient__ProxyUsername`/`ProxyPassword` therefore sent that field alone, paired with an empty one, discarding whatever the intercepted URL carried for the other - `HTTP_PROXY=http://url-user:old@proxy` with only `ProxyPassword=new` sent `Basic :new` rather than `Basic url-user:new`. The matcher only exposes an already base64-encoded `Basic` header for an `http` proxy (`raw_auth` is for other schemes, e.g. `socks5h`), so it is decoded back into a username and password to merge each independently with the dedicated option, the same rule `ClientConfig::from_config_args` already applies to an explicit `proxy` URL. cargo test -p armonik-transport --test proxy --all-features: 16 passed, 0 failed. cargo fmt --all --check and cargo clippy --all-features --all-targets -Dwarnings: clean.
A hand-rolled String leaves the password sitting in memory for the allocator to reuse; secrecy zeroizes it on drop. Secret's own API is unchanged, PartialEq/Eq/Hash are now written by hand since SecretString does not derive them.
config.rs and proxy.rs each hand-wrote the same "a dedicated username or password must not discard the URL's other half" merge, with the same rule explained twice in near-identical comments. One helper now backs both call sites.
The 2xx-that-is-not-200 tripwire looped over 201 and 204, asserting the identical fact twice: both exercise the same "not literally 200" check in hyper_util's Tunnel. Kept 201 alone. Also shares the request-target parsing duplicated across two tests.
wkirschenmann
force-pushed
the
wk/feat/rust-proxy
branch
from
August 4, 2026 15:29
f94d362 to
3eaacab
Compare
…cent-encoding crate percent_decode hand-rolled the same %XX walk this crate already depends on transitively through another crate's tree. Declaring percent-encoding directly adds no new crate to the build, and percent_decode_str behaves identically for a malformed escape: left exactly as written.
…wire hyper_util_still_refuses_a_2xx_that_is_not_200 (tests/upstream_tunnel.rs) called Tunnel directly, with none of this crate's own code involved, to pin the same known defect known_issue_a_success_other_than_200_does_not_open_the_tunnel (tests/proxy.rs) already pins through the full stack - config, connect, ProxyConnector, translate(). Both go red the day hyper-util fixes it; the proxy.rs one is strictly more informative, since it also proves this crate's own error translation still reacts correctly. Kept that one, dropped the raw duplicate. The other two tests in upstream_tunnel.rs stay: the control, and the split-status-line case, which has no equivalent in tests/proxy.rs. Updated the file's module doc and the remaining test's panic message, which both referenced the removed test. Corrected the README's "Known issues" intro, which claimed every pinning test is named known_issue_*: the upstream_tunnel.rs ones never were.
wkirschenmann
force-pushed
the
wk/feat/rust-proxy
branch
from
August 6, 2026 00:02
6279f8f to
5b2d2ec
Compare
|
wkirschenmann
marked this pull request as draft
August 6, 2026 07:38
Contributor
Author
|
Re-cut as a stacked series, one reviewable concern per PR:
The series carries this branch's content with the review findings fixed in place. The dependent |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GrpcClient__Proxy,GrpcClient__ProxyUsernameandGrpcClient__ProxyPasswordare part of ArmoniK'sclient configuration and did nothing here. A client behind a corporate proxy could not reach a cluster at
all.
Description
A
ProxyConnectorbetween the TCP connector and the TLS one. It opens an HTTPCONNECTtunnel, so TLS,mutual TLS included, is negotiated end to end with the real server and the proxy only forwards opaque
bytes. Terminating TLS at the proxy would defeat the point of a transport that exists to control the TLS
stack. The handshake goes out in the clear, so the proxy's own URL has to be
http: anything else isrefused while parsing, or before dialling when it came from the environment.
Reading the environment is
hyper_util's, not ours:client::proxy::matcher::Matcher, behind itsclient-proxyfeature, already implements the*_PROXYconvention includingALL_PROXY, matchesNO_PROXYthe way curl does, and takes credentials out of a proxy URL, sosystemmeans exactly what itmeans for any hyper-based client. An explicitly named proxy does not go through it, since it answers
Nonefor a target with no host or an unexpected scheme and would silently skip the proxy the callerasked for.
The tunnel itself is deliberately ours.
hyper_util's rejects aCONNECTresponse whose status linearrives split across reads, which is legal and more likely with a slow proxy; ours reads to the blank line
before parsing, and treats any 2xx as the tunnel being open, as RFC 9110 requires. #699 tracks deleting it
once hyperium/hyper-util#300 is released.
Credentials written into a proxy URL are honoured, and the dedicated options win field by field, so
setting only the password keeps the username the URL carried; the same rule is shared between the
explicit-proxy and system-proxy paths rather than written twice. A password is held in a
Secret,redacted by
Debugand by serialisation and readable only throughexpose_secret, with noDereforAsRefto let it out silently, and stored internally in asecrecy::SecretStringso it is zeroized ondrop.
rustlsguards a private key the same way.Testing
cargo test -p armonik-transport --all-featuresgives 74 tests, all passing. Sixteen are integrationtests driving a real client through a real
CONNECTproxy to a real gRPC server over loopback sockets:the tunnel, credentials demanded and presented, credentials in the URL, credentials missing and wrong, an
unreachable proxy, proxying disabled, a success other than 200, an
httpsproxy refused before dialling,systemtaking the proxy from the environment, andNO_PROXYboth bypassing insystemmode anddeliberately not applying to an explicit proxy. The proxy is a few dozen lines in the test file rather
than an external binary, so it runs wherever CI does.
Each fix was checked against the unfixed code rather than assumed: with the proxy configuration prevented
from reaching the connector, seven of the eight tunnel tests fail, and with the status range narrowed back
to 200 and the scheme check removed, the two tests covering them fail while the rest pass. The commit
messages carry the runs.
Impact
No behaviour change for anyone who sets none of the three options: the default is a direct connection, and
the connector delegates straight through when proxying is off.
ClientConfigArgs::proxy_passwordis aSecretrather than aString, so a caller assigning it writes.into(). The field is new here, so nothing existing breaks.Three new direct dependencies:
base64for theBasicheader, already in the tree throughtonic;hyper-util'sclient-proxyfeature, which bringsipnetandpercent-encoding; andsecrecy, forSecret's zeroize-on-drop storage.