refactor(rust): hand the transport the certificates rather than paths to them - #704
Closed
wkirschenmann wants to merge 6 commits into
Closed
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
![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: 6eebfb77cd
ℹ️ 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".
| if path.is_empty() { | ||
| return Ok(String::new()); | ||
| } | ||
| std::fs::read_to_string(&path).context(FileSnafu { name, path }) |
There was a problem hiding this comment.
When a configured certificate path points to an existing zero-byte file, this returns an empty string, which ClientConfig::from_config_args interprets as “not configured.” For an empty CA file the client therefore silently falls back to system trust roots, and if both client files are empty it silently disables mTLS; before this change, PEM parsing rejected these files. Reject empty contents when a path was supplied, or otherwise preserve the distinction between an absent variable and an empty configured file.
Useful? React with 👍 / 👎.
wkirschenmann
marked this pull request as draft
August 2, 2026 10:06
lemaitre-aneo
left a comment
lemaitre-aneo
left a comment
Contributor
There was a problem hiding this comment.
You can always access ClientConfig directly and set the certificates there. In the path you mentioned, the certificates are read during the conversion from ClientConfigArgs to ClientConfig.
If you really want to expose certificate content in ClientConfigArgs, best way would be to have both cert_pem_path and cert_pem_content, and chose which to use during the conversion.
`GrpcClient__ReusePorts` did nothing here. On Windows it sets
`SO_REUSE_UNICASTPORT`, which keeps a client that opens many connections in a
short window from exhausting the ephemeral port range. The option has to be set
before `connect`, so socket creation cannot be left to hyper: `TcpConnector`
keeps hyper's connector for every other case and only a Windows caller who asks
for it reaches the one written here.
The `setsockopt` call goes through `windows-sys`, so its signature and both
constants come from the SDK metadata. The hand-written declaration it replaces
was correct; this removes a class of future error, not a present bug.
Owning socket creation means owning what hyper does around it. Verified in
hyper-util 0.1.20: `happy_eyeballs_timeout` defaults to 300ms, so its connector
races addresses and a dual-stack host with one blackholed record still connects
quickly. `race` does the same, because without it turning the option on would
trade a 300ms connection for a `connect_timeout`-long one.
The default is `true`, matching the C# client, whose documentation defines the
option as "Enable the option SO_REUSE_UNICASTPORT upon socket opening".
The Rust exclusion for `windows-latest` is removed from `test.yml`, along with
the FIXME blaming the port exhaustion this addresses.
Measured, from `packages/rust`:
$ cargo test -p armonik-transport --all-features
76 unit, 3 concurrency, 8 proxy, 6 timeout, all passing
$ cargo build -p armonik-transport
ok
That last one is in the list because `tokio::select!` needs the `macros`
feature, which was a dev-dependency at first: the tests passed while the library
did not build.
$ 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 fmt --all --check
all silent
… a newtype `ClientConfigArgs::reuse_ports` is an `Option<bool>`, `ClientConfig::reuse_ports` a plain `bool`, and the default that is not the zero value lives in a hand-written `Default` for `ClientConfig`. The compiler rejects that impl if a field is added without one, so it cannot fall behind the struct, which is what the `ReusePorts` newtype existed to prevent. `read_env_bool_or(name, unset)` becomes `read_env_bool_opt(name)`: the caller decides. `with_optional_deadline` returns the `Result<T, Elapsed>` that `timeout_at` produces, rather than flattening it to an `Option`. `port_reuse_is_on_unless_it_is_turned_off` covers what the deleted type's tests covered: `ClientConfig::default()`, and each of the three states the argument can be in. cargo test -p armonik-transport --all-features: 89 passed, 0 failed. clippy --all-targets --all-features, and with -Dunused-crate-dependencies: clean. cargo build --workspace --locked --all-features, RUSTDOCFLAGS=-Dwarnings cargo doc: clean.
…the first
`ReusePortsConnector` raced the resolved addresses in the order the resolver gave them.
`hyper_util` partitions them by family and races the halves, so the alternate family always
starts one fallback delay in; ours reached it only after every address of the first. With
`[IPv6, IPv6, IPv4]` and IPv6 hanging, IPv4 started at 600ms, so a `connect_timeout` of 500ms
expired while a reachable address had never been tried.
`interleave_families` alternates the two, keeping the resolver's preference first. That is
what RFC 8305 asks for, it gives the same guarantee as hyper's partition-and-race, and unlike
hyper's it keeps every attempt in flight rather than walking each half in order.
Checked against the unfixed code, racing the raw order:
assertion `left == right` failed: the other family should not wait out every address of
the first
left: 600ms
right: 300ms
cargo test -p armonik-transport --all-features: 92 passed, 0 failed.
clippy --all-targets --all-features, cargo fmt --all --check, cargo build --workspace --locked
--all-features, RUSTDOCFLAGS=-Dwarnings cargo doc: clean.
…ve them `armonik-transport` read `GrpcClient__*` out of the environment, which is integration with a deployment rather than transport. It now takes the configuration it is handed and nothing else, so a caller that keeps its settings anywhere at all can use it, and the crate no longer knows it lives inside ArmoniK. `ClientConfigArgs::from_env` and `ClientConfig::from_env` become a `FromEnv` extension trait in `armonik`, together with `read_env*` and `ReadEnvError`. An extension trait because the types stay where they are and an inherent method would have to live with them. What is left of the transport's `utils` is one certificate verifier, which moves next to its only user. `ConfigError` loses its `Env` variant, and `armonik` gains `EnvConfigError` and `NewClientError`. `Client::new` returns the latter: `ConnectionError` belongs to the transport and is `#[non_exhaustive]`, so `armonik` cannot express a reading failure through it. `#[non_exhaustive]` comes off `ClientConfigArgs`. A foreign crate cannot write a literal for such a struct, only `Default` and field assignment, which would let an option added here go unread there without a word. Exhaustive, the compiler says so. Two things the move turned up. `InvalidDuration` served seven duration fields while its message named `GrpcClient__ConnectTimeout`, so a bad `Timeout` was reported against `ConnectTimeout`; the variant now carries the option. And the hidden context selectors `ConfigSnafu`, `IoSnafu`, `TlsSnafu`, `TransportSnafu` were exported for `Client::new` alone, which no longer needs them. Error messages name the field, `cert_pem` where they said `GrpcClient__CertPem`. The one thing this crate still reads from the environment is `ProxySource::System`, the `*_PROXY` convention every HTTP client obeys, which is not ArmoniK's vocabulary. cargo test --workspace --all-features --no-fail-fast -- --skip client:: : 153 passed, 0 failed. The `client::` tests want the ArmoniK mock, which CI provides and this machine does not. clippy --workspace --all-features --all-targets, and with -Dunused-crate-dependencies: clean. cargo fmt --all --check and RUSTDOCFLAGS=-Dwarnings cargo doc --workspace --all-features: clean.
`with_var` ended by removing the variable rather than restoring what it held. The Rust legs
run against a mock with `GrpcClient__Endpoint` already set, so a test that borrowed it took it
away from every `client::` test in the same binary, and all four legs went red:
called `Result::unwrap()` on an `Err` value: Config { source: Invalid { source: Uri {
source: InvalidUri(Empty), uri: "" ... } } }
It now saves the value and puts it back from a `Drop` guard, so a failing test restores too.
Restoring is not enough on its own: the `client::` tests are not serialised against this
module, so a hardcoded endpoint could be read by one of them mid-flight. The test borrows
`GrpcClient__UserAgent` instead, which nothing else depends on, and asserts on
`ClientConfigArgs::from_env`, so it needs no endpoint at all. What it dropped, args becoming a
parsed configuration, is covered in the transport's own tests.
cargo test -p armonik --all-features --lib client::env: 6 passed, 0 failed.
cargo fmt --all --check: clean.
… to them Opening a file is integration with a deployment, the same as reading an environment variable, which the previous change already moved out. `ClientConfigArgs::cert_pem`, `key_pem` and `ca_cert` now carry the PEM itself; their names already said so. `from_config_args` calls no `std::fs`, and `ConfigError::Io` goes with it. `armonik` opens the files that `GrpcClient__CertPem` and its two neighbours name, so nothing changes for a deployment. `key_pem` becomes a `Secret`, as the proxy password is: a private key now sits in a struct that derives `Debug` and `Serialize`. The compiler found the first consequence on its own, which is the point of the type: the tracing span recorded `cert_pem` and `key_pem`, harmless while they were paths. It records whether each was supplied, and nothing else. The two tests that asserted a missing file follow the behaviour to `armonik`, against `read_pem_file`, which takes the variable name so they can borrow one of their own rather than a `GrpcClient__*` the mock-backed tests depend on. In their place the transport pins what a caller who still passes a path now gets: a PEM error, for all three options, rather than this crate quietly opening whatever it points at. cargo test -p armonik-transport --all-features: 89 passed, 0 failed. cargo test -p armonik --all-features --lib client::env: 8 passed, 0 failed. The workspace, minus the `client::` tests that want the ArmoniK mock: 0 failed. clippy --workspace --all-features --all-targets -Dwarnings, cargo fmt --all --check, RUSTDOCFLAGS=-Dwarnings cargo doc --workspace --all-features: clean.
wkirschenmann
force-pushed
the
wk/refactor/rust-env-loading-to-armonik
branch
from
August 2, 2026 15:08
c6dea61 to
ebb0cad
Compare
wkirschenmann
force-pushed
the
wk/refactor/rust-certificates-as-bytes
branch
from
August 2, 2026 15:11
6eebfb7 to
e40e3cc
Compare
|
wkirschenmann
force-pushed
the
wk/refactor/rust-env-loading-to-armonik
branch
from
August 2, 2026 16:41
ebb0cad to
cb0cbd6
Compare
Contributor
Author
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.
Motivation
from_config_argscalledstd::fs::read_to_stringon the paths incert_pem,key_pemandca_cert. Opening a file is integration with a deployment, the same as reading an environmentvariable, which #701 already moved out of the transport. Knowing where material is kept belongs to the
layer that knows what a deployment looks like.
It is also what the C ABI needs: a caller converting a PKCS#12 file, or reading the Windows
certificate store, has bytes and no path, and should not have to write a private key to disk to be
understood.
Description
The three options carry the PEM itself. Their names already said
pem, so only their meaning moves.The transport calls no
std::fs, andConfigError::Iogoes with it.armonikopens the files thatGrpcClient__CertPem,GrpcClient__KeyPemandGrpcClient__CaCertname, throughread_pem_file;EnvConfigErrorgains aFilevariant naming both the variable and the path.key_pembecomes aSecret, asproxy_passwordis: a private key now sits in a struct that derivesDebugandSerialize. The compiler found the first consequence by itself, which is the point of thetype. The tracing span recorded
cert_pemandkey_pem, harmless while they were paths and not oncethey are the material. It records whether each was supplied, and nothing else.
Testing
cargo test -p armonik-transport --all-featuresgives 89 passing, andcargo test -p armonik --all-features --lib client::env8.The two tests that asserted a missing file follow the behaviour to
armonik, where they exerciseread_pem_file. It takes the variable name, so they borrow one of their own rather than aGrpcClient__*that the mock-backed tests in the same binary depend on. In their place the transportpins what a caller still passing a path now gets, for all three options: a PEM error, rather than this
crate quietly opening whatever it points at.
Impact
Nothing changes for a deployment. The variables still name files, and
armonikstill reads them.For a Rust caller building
ClientConfigArgsby hand, the three fields change meaning, andkey_pemchanges type. Passing a path is caught as an invalid certificate rather than silently read.
Additional Information
This is what lets the C ABI take its whole configuration as one JSON document, certificates included,
instead of needing a second channel for them.
Checklist