feat(etcd): support TLS via OpenSSL#49
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdd optional PEM-based TLS config for etcd (ca_file, cert_file, key_file, insecure_skip_verify), enable Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Provider as EtcdConfigProvider
participant OpenSSL as OpenSslClientConfig
participant Etcd as etcd-server
Caller->>Provider: connect_client(config)
Provider->>Provider: parse hosts, detect "https://" vs "http://"
alt HTTPS detected
Provider->>OpenSSL: build TLS config (load ca_file, cert_file, key_file, set insecure_skip_verify)
OpenSSL-->>Provider: OpenSSL TLS config
Provider->>Etcd: connect using with_openssl_tls(...)
else HTTP only
Provider->>Etcd: connect without TLS
end
Etcd-->>Provider: respond (status)
Provider-->>Caller: return client or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/etcd.rs`:
- Around line 23-28: Add triple-slash doc comments for the public EtcdTlsConfig
struct and its fields (EtcdTlsConfig, ca_pem, cert_pem, key_pem) describing each
field's purpose and expected format (e.g., PEM-encoded CA, client cert, client
key), and add a /// doc comment on the public Config::tls field explaining that
it holds optional TLS configuration for connecting to etcd; ensure comments
follow repo style and cover that fields are optional and when they should be
provided.
- Around line 101-113: The code currently enables TLS when any host in
config.host is "https://" and silently ignores partial mTLS settings; update the
logic in the etcd client setup (the block that computes use_tls and builds
OpenSslClientConfig) so that TLS is enabled only if all hosts use the "https://"
scheme (fail with an error if host list contains mixed "http://" and
"https://"), and validate config.tls so that if either cert_pem or key_pem is
present without the other you return/fail immediately with a clear configuration
error; continue to apply ca_pem when present and only call
opts.with_openssl_tls(tls_cfg) after these validations succeed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e10613f6-cb8f-430f-a5ee-a599b4da565e
📒 Files selected for processing (2)
Cargo.tomlsrc/config/etcd.rs
- Regenerate Cargo.lock to include new OpenSSL TLS dependencies introduced by switching etcd-client to features = ["tls-openssl"] - Add 9 unit tests covering EtcdTlsConfig defaults, deserialization, and TLS host detection logic
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/config/etcd.rs (2)
23-28:⚠️ Potential issue | 🟡 MinorAdd
///docs for the new public TLS config items.Line 23 and Line 37 introduce public API without Rust doc comments. Please document
EtcdTlsConfig, its fields, andConfig::tls.Suggested patch
#[derive(Clone, Debug, Default, Deserialize)] +/// TLS material for connecting to etcd over HTTPS. pub struct EtcdTlsConfig { + /// Optional PEM-encoded CA certificate used to validate the etcd server certificate. pub ca_pem: Option<String>, + /// Optional PEM-encoded client certificate for mTLS authentication. pub cert_pem: Option<String>, + /// Optional PEM-encoded private key matching `cert_pem`. pub key_pem: Option<String>, } @@ pub struct Config { @@ + /// Optional TLS settings applied when etcd endpoints use `https://`. pub tls: Option<EtcdTlsConfig>, }As per coding guidelines:
Use /// for doc comments on public items in Rust.Also applies to: 37-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/etcd.rs` around lines 23 - 28, Public API items lack Rust doc comments: add /// documentation for the EtcdTlsConfig struct and each public field (EtcdTlsConfig, ca_pem, cert_pem, key_pem) and for Config::tls; describe purpose, expected formats/encodings (e.g., PEM), optionality, and any security or usage notes (e.g., when each field is required), keeping the docs concise and using /// on the struct and each field as per Rust guidelines.
93-113:⚠️ Potential issue | 🟠 MajorHarden TLS setup: reject mixed host schemes and incomplete mTLS keypairs.
Line 101 enables TLS if any endpoint is HTTPS, and Lines 108-110 ignore partial
cert_pem/key_pem. Both should fail fast to avoid ambiguous/broken runtime behavior.Suggested patch
- async fn connect_client(config: &Config) -> Result<etcd_client::Client, etcd_client::Error> { + async fn connect_client(config: &Config) -> Result<etcd_client::Client> { let mut opts = etcd_client::ConnectOptions::default() .with_connect_timeout(Duration::from_secs(config.timeout as u64)); @@ - let use_tls = config.host.iter().any(|h| h.starts_with("https://")); + let has_https = config.host.iter().any(|h| h.starts_with("https://")); + let has_http = config.host.iter().any(|h| h.starts_with("http://")); + if has_http && has_https { + return Err(anyhow!( + "etcd hosts must use a single scheme (all http:// or all https://)" + )); + } + let use_tls = has_https; if use_tls { let mut tls_cfg = etcd_client::OpenSslClientConfig::default(); if let Some(t) = &config.tls { if let Some(ca_pem) = &t.ca_pem { tls_cfg = tls_cfg.ca_cert_pem(ca_pem.as_bytes()); } - if let (Some(cert_pem), Some(key_pem)) = (&t.cert_pem, &t.key_pem) { - tls_cfg = tls_cfg.client_cert_pem_and_key(cert_pem.as_bytes(), key_pem.as_bytes()); - } + match (&t.cert_pem, &t.key_pem) { + (Some(cert_pem), Some(key_pem)) => { + tls_cfg = + tls_cfg.client_cert_pem_and_key(cert_pem.as_bytes(), key_pem.as_bytes()); + } + (None, None) => {} + _ => return Err(anyhow!("both tls.cert_pem and tls.key_pem must be set together")), + } } opts = opts.with_openssl_tls(tls_cfg); } @@ - client.status().await.map(|_| Ok(client))? + client.status().await?; + Ok(client) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/etcd.rs` around lines 93 - 113, The connect_client function currently enables TLS if any host starts with "https://" and silently ignores partial mTLS keypairs; change it to validate up-front: compute whether all hosts are HTTPS or all are non-HTTPS and return an error if schemes are mixed (do not proceed when config.host contains both "http://" and "https://"), and when config.tls is present require both cert_pem and key_pem together (return an error if only one is set). Update the use_tls decision and the TLS block in connect_client to perform these checks and return a descriptive Err from connect_client instead of silently continuing; reference connect_client, config.host, use_tls, config.tls, cert_pem and key_pem to locate the code to change.
🧹 Nitpick comments (1)
src/config/etcd.rs (1)
423-518: Use repository-preferred assertion macros in tests.The new tests use standard
assert_eq!/assert!; this repo preferspretty_assertions::assert_eqandassert_matches::assert_matchesfor test diagnostics.Suggested patch (pattern)
#[cfg(test)] mod tests { use super::*; + use assert_matches::assert_matches; + use pretty_assertions::assert_eq; @@ - assert!(tls.ca_pem.is_none()); + assert_matches!(tls.ca_pem, None); @@ - assert_eq!(cfg.host, vec!["http://127.0.0.1:2379"]); + assert_eq!(cfg.host, vec!["http://127.0.0.1:2379"]);As per coding guidelines:
Use pretty_assertions::assert_eq and assert_matches::assert_matches for better test output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/etcd.rs` around lines 423 - 518, Replace standard assert_eq!/assert! in the tests (e.g. test_etcd_tls_config_default, test_config_default_no_tls, test_use_tls_detected_from_https_host, test_use_tls_not_detected_for_http_host, test_use_tls_mixed_hosts_any_https_triggers_tls, test_etcd_tls_config_deserialization, test_etcd_tls_config_partial_deserialization, test_config_deserialization_with_tls, test_config_deserialization_without_tls) with the repository-preferred macros: use pretty_assertions::assert_eq for equality checks on EtcdTlsConfig and Config values and use assert_matches::assert_matches where pattern matching assertions are needed; add the necessary use imports at the top of the test module so EtcdTlsConfig::default(), Config::default(), serde_json::from_str(...) assertions use assert_eq and assert_matches consistently across all tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/config/etcd.rs`:
- Around line 23-28: Public API items lack Rust doc comments: add ///
documentation for the EtcdTlsConfig struct and each public field (EtcdTlsConfig,
ca_pem, cert_pem, key_pem) and for Config::tls; describe purpose, expected
formats/encodings (e.g., PEM), optionality, and any security or usage notes
(e.g., when each field is required), keeping the docs concise and using /// on
the struct and each field as per Rust guidelines.
- Around line 93-113: The connect_client function currently enables TLS if any
host starts with "https://" and silently ignores partial mTLS keypairs; change
it to validate up-front: compute whether all hosts are HTTPS or all are
non-HTTPS and return an error if schemes are mixed (do not proceed when
config.host contains both "http://" and "https://"), and when config.tls is
present require both cert_pem and key_pem together (return an error if only one
is set). Update the use_tls decision and the TLS block in connect_client to
perform these checks and return a descriptive Err from connect_client instead of
silently continuing; reference connect_client, config.host, use_tls, config.tls,
cert_pem and key_pem to locate the code to change.
---
Nitpick comments:
In `@src/config/etcd.rs`:
- Around line 423-518: Replace standard assert_eq!/assert! in the tests (e.g.
test_etcd_tls_config_default, test_config_default_no_tls,
test_use_tls_detected_from_https_host, test_use_tls_not_detected_for_http_host,
test_use_tls_mixed_hosts_any_https_triggers_tls,
test_etcd_tls_config_deserialization,
test_etcd_tls_config_partial_deserialization,
test_config_deserialization_with_tls, test_config_deserialization_without_tls)
with the repository-preferred macros: use pretty_assertions::assert_eq for
equality checks on EtcdTlsConfig and Config values and use
assert_matches::assert_matches where pattern matching assertions are needed; add
the necessary use imports at the top of the test module so
EtcdTlsConfig::default(), Config::default(), serde_json::from_str(...)
assertions use assert_eq and assert_matches consistently across all tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7e07efa-717a-430a-90c4-5b6728332e51
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
src/config/etcd.rs
- Add doc comments to EtcdTlsConfig fields and Config::tls - Reject mixed http/https host schemes with a clear error message - Reject partial mTLS config (cert_pem without key_pem, or vice versa) - Update connect_client return type to anyhow::Result for consistency - Add unit tests for the new validation paths
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/config/etcd.rs (1)
466-498: Prefer behavior tests over predicate-reimplementation tests.The host-scheme tests in this block re-implement
starts_withchecks and don’t validateconnect_clientbehavior. Consider replacing them with outcome-based tests only (success/failure paths), which are less brittle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/etcd.rs` around lines 466 - 498, The tests re-check host schemes with starts_with instead of exercising the real logic; replace the three predicate tests (test_tls_detected_from_https_host, test_tls_not_detected_for_http_host, test_mixed_http_https_hosts_detected) with outcome-driven tests that construct Config instances (using Config::default() and Config { host: vec![...] }) and call the connect_client function (or the public API that uses it) to assert the actual behavior: successful connection for all-http hosts, successful connection for all-https hosts (or TLS-enabled path), and a rejected/error result for mixed http+https hosts; assert on the returned Result/Err or error kind rather than re-implementing starts_with checks on cfg.host.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/etcd.rs`:
- Around line 447-604: The tests in this module should use the
repository-standard assertion macros: add imports for
pretty_assertions::assert_eq and assert_matches::assert_matches at the top of
the tests module, then replace plain assert_eq! calls in tests like
test_etcd_tls_config_default, test_config_default_no_tls,
test_etcd_tls_config_deserialization,
test_etcd_tls_config_partial_deserialization,
test_config_deserialization_with_tls, and
test_config_deserialization_without_tls with pretty_assertions::assert_eq; also
replace the match-based error checks in async tests
test_connect_client_rejects_mixed_schemes,
test_connect_client_rejects_partial_mtls_cert_only, and
test_connect_client_rejects_partial_mtls_key_only with assert_matches!(Err(e), e
if e.to_string().contains("...")) (or equivalent assert_matches patterns) so the
tests use assert_matches! for error assertions and pretty_assertions::assert_eq
for equality.
---
Nitpick comments:
In `@src/config/etcd.rs`:
- Around line 466-498: The tests re-check host schemes with starts_with instead
of exercising the real logic; replace the three predicate tests
(test_tls_detected_from_https_host, test_tls_not_detected_for_http_host,
test_mixed_http_https_hosts_detected) with outcome-driven tests that construct
Config instances (using Config::default() and Config { host: vec![...] }) and
call the connect_client function (or the public API that uses it) to assert the
actual behavior: successful connection for all-http hosts, successful connection
for all-https hosts (or TLS-enabled path), and a rejected/error result for mixed
http+https hosts; assert on the returned Result/Err or error kind rather than
re-implementing starts_with checks on cfg.host.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Replace standard assert_eq! and manual match blocks with project-standard assertion macros per coding guidelines.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/config/etcd.rs (1)
469-501: These scheme-detection tests are tautological; prefer behavior-level coverage.These tests re-implement
starts_withchecks instead of assertingconnect_clientoutcomes. Consider consolidating into parameterized cases that verify accept/reject behavior directly.As per coding guidelines:
Use #[test] for synchronous unit tests, #[tokio::test] for async tests, and #[rstest] for parameterized tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/etcd.rs` around lines 469 - 501, The tests currently probe host scheme detection by re-implementing starts_with checks on Config.host; instead, update them to exercise the actual behavior by calling connect_client (or the function that validates/creates the client) and asserting whether the call succeeds or returns an error for each scenario. Replace the three tautological tests with a parameterized #[rstest] (or multiple #[tokio::test]/#[test] as appropriate) that supplies hosts like "https://...", "http://...", and a mixed list and then asserts connect_client returns Ok for all-https, Ok or Err per your intended policy for all-http, and Err for mixed; reference the Config struct and connect_client function in the test so the tests validate behavior rather than re-checking starts_with.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/etcd.rs`:
- Around line 108-114: The current validation only checks for mixed http/https
but allows endpoints like "127.0.0.1:2379"; update the validation so every entry
in config.host is required to start with either "http://" or "https://",
returning an error if any host does not match those prefixes before deciding TLS
mode; modify the logic around config.host, has_http and has_https to first
assert all hosts have an explicit scheme (fail fast on non-HTTP(S) endpoints),
then proceed with the existing mixed-scheme check.
---
Nitpick comments:
In `@src/config/etcd.rs`:
- Around line 469-501: The tests currently probe host scheme detection by
re-implementing starts_with checks on Config.host; instead, update them to
exercise the actual behavior by calling connect_client (or the function that
validates/creates the client) and asserting whether the call succeeds or returns
an error for each scenario. Replace the three tautological tests with a
parameterized #[rstest] (or multiple #[tokio::test]/#[test] as appropriate) that
supplies hosts like "https://...", "http://...", and a mixed list and then
asserts connect_client returns Ok for all-https, Ok or Err per your intended
policy for all-http, and Err for mixed; reference the Config struct and
connect_client function in the test so the tests validate behavior rather than
re-checking starts_with.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…erify - Remove inline PEM string fields (ca_pem, cert_pem, key_pem); keep only file-based fields (ca_file, cert_file, key_file) - Replace resolve_pem helper with simpler read_pem (file path only) - Add insecure_skip_verify field (default false) to skip TLS certificate validation; useful for development environments with IP/hostname mismatches - Update config.yaml to use insecure_skip_verify: true for current test env - Update tests to match simplified struct and helper
…t, reject invalid schemes - Move cert/key pair presence check before read_pem calls so the error message is always 'cert and key must be set together' regardless of whether the files exist (fixes two flaky CI test failures) - Implement custom Debug for EtcdTlsConfig and Config that redacts key_file and password fields to prevent sensitive material leaking into logs - Reject etcd host entries that lack a http:// or https:// scheme with a clear error message - Add test_connect_client_rejects_missing_scheme for the new validation
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/config/etcd.rs (1)
130-136:⚠️ Potential issue | 🟠 MajorReject endpoints without explicit
http://orhttps://scheme.The validation only rejects explicit mixed schemes, but a host like
127.0.0.1:2379(no scheme) bypasses this check. When combined with anhttps://endpoint, this creates an inconsistent TLS state.Suggested fix
- let has_https = config.host.iter().any(|h| h.starts_with("https://")); - let has_http = config.host.iter().any(|h| h.starts_with("http://")); + let mut has_https = false; + let mut has_http = false; + for h in &config.host { + if h.starts_with("https://") { + has_https = true; + } else if h.starts_with("http://") { + has_http = true; + } else { + return Err(anyhow!( + "etcd host must start with http:// or https://: {}", + h + )); + } + } if has_http && has_https { return Err(anyhow!( "etcd hosts must use a single scheme (all http:// or all https://)" )); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/etcd.rs` around lines 130 - 136, The host-scheme validation in etcd.rs currently only checks for mixed http/https via has_http and has_https but allows entries without any scheme (e.g., "127.0.0.1:2379"), which can create inconsistent TLS state; update the validation around config.host to require every host string to start with either "http://" or "https://" and return an error if any host lacks a scheme, and continue to reject mixed schemes as before (i.e., compute a flag like has_scheme per-host or check for !(starts_with("http://") || starts_with("https://")) and surface a clear anyhow! error mentioning config.host and the offending entry), referencing the existing variables has_http / has_https and config.host to locate and replace the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/config/etcd.rs`:
- Around line 130-136: The host-scheme validation in etcd.rs currently only
checks for mixed http/https via has_http and has_https but allows entries
without any scheme (e.g., "127.0.0.1:2379"), which can create inconsistent TLS
state; update the validation around config.host to require every host string to
start with either "http://" or "https://" and return an error if any host lacks
a scheme, and continue to reject mixed schemes as before (i.e., compute a flag
like has_scheme per-host or check for !(starts_with("http://") ||
starts_with("https://")) and surface a clear anyhow! error mentioning
config.host and the offending entry), referencing the existing variables
has_http / has_https and config.host to locate and replace the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22b3b8f7-863b-4f98-906d-cfcd350ad5a9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
config.yamlsrc/config/etcd.rs
✅ Files skipped from review due to trivial changes (1)
- config.yaml
Add ca_pem, cert_pem, key_pem fields to EtcdTlsConfig so that TLS credentials can be supplied as inline strings (e.g. from env vars) rather than only from files on disk. File-based fields take precedence when both are supplied for the same credential. This is useful for environments where certificates are injected via environment variables rather than mounted as files. - Update connect_client to resolve each credential from file or inline PEM - Update validation to cover cross-source cert/key pair mismatches - Add tests for inline PEM deserialization and partial pair rejection
bzp2010
reviewed
Apr 20, 2026
…g redaction - Add EtcdConfigError enum (thiserror) with MixedSchemes, MissingScheme, PartialMtlsKeypair variants for precise, matchable error types - Extract host scheme and mTLS keypair validation into validate_connect_config() returning Result<bool, EtcdConfigError> instead of inline anyhow errors - Fix EtcdTlsConfig Debug: key_file path is no longer redacted; only PEM content strings (ca_pem, cert_pem, key_pem) are redacted - Add 5 direct unit tests for validate_connect_config covering each error variant and the happy paths (http/https)
Remove ca_pem, cert_pem, key_pem fields from EtcdTlsConfig and all associated code paths, tests, and documentation references. TLS certificate material is now exclusively supplied via file paths (ca_file, cert_file, key_file).
…ile-based certs Add EtcdTlsCertConfig enum with Strings and Files variants to unify cert/key material from two sources. Methods ca_pem()/cert_pem()/key_pem() abstract file I/O vs. inline strings behind a single interface. EtcdTlsConfig deserializes from a flat config structure via a custom Deserialize impl (intermediate EtcdTlsConfigRaw struct) that rejects mixed inline+file configs at parse time. Convenience methods on EtcdTlsConfig delegate to the inner cert config so callers avoid Option boilerplate. Deref<Target=Option<EtcdTlsCertConfig>> is also implemented for direct pattern matching on the cert variant.
bzp2010
reviewed
Apr 20, 2026
| insecure_skip_verify: bool, | ||
| } | ||
|
|
||
| impl<'de> Deserialize<'de> for EtcdTlsConfig { |
Collaborator
There was a problem hiding this comment.
https://serde.rs/attr-flatten.html
Why not use serde's native flattening? A custom parser would require writing a lot of boilerplate code. In most cases, this is unnecessary.
| }) | ||
| } | ||
|
|
||
| async fn connect_client(config: &Config) -> Result<etcd_client::Client, etcd_client::Error> { |
Collaborator
There was a problem hiding this comment.
Return to the original pattern.
The validate_connect_config should not be included within connect_client, because connect_client is an internal-only function with a fairly clear semantic: "establish a connection."
Configuration errors can be checked for and thrown during EtcdConfigProvider::new -> Result<()>; there is no need to enter the connect_client function.
Comment on lines
+220
to
+221
| #[error("etcd hosts must use a single scheme (all http:// or all https://)")] | ||
| MixedSchemes, |
Collaborator
There was a problem hiding this comment.
Is this a requirement for etcd-client?
Contributor
Author
There was a problem hiding this comment.
No, client.status() is not a requirement of etcd-client. It was added as an early connection health-check, but calling the Maintenance service during connect_client is unnecessary and caused 'unknown service etcdserverpb.Maintenance' errors when the server does not expose that gRPC service. Removed in this PR.
Co-authored-by: Zeping Bai <zeping@api7.ai>
Co-authored-by: Zeping Bai <zeping@api7.ai>
- Replace EtcdTlsConfigRaw + custom Deserialize with PemFiles/PemStrings helper structs using #[serde(deny_unknown_fields)] and #[serde(untagged)] on EtcdTlsCertConfig, eliminating manual deserialization boilerplate - Change EtcdTlsCertConfig from struct variants to tuple variants; add Default impl (-> Files with all None) so EtcdTlsConfig.cert is non-Optional - Remove EtcdTlsConfig wrapper pem methods; callers use Deref to EtcdTlsCertConfig - Change validate_connect_config return type to Result<(), EtcdConfigError> and move the call to EtcdConfigProvider::new() so config errors fail fast without entering retry logic - Remove client.status().await? from connect_client - Update tests accordingly; remove redundant connect_client validation tests
…nfigRaw serde Keep validate_connect_config in new() and client.status() removal, but restore the original struct-variant enum and custom Deserialize approach to avoid the pub(crate) interface visibility issue with untagged enum tuple variants.
…tagged
Use #[serde(untagged, deny_unknown_fields)] on EtcdTlsCertConfig and
#[serde(flatten)] on EtcdTlsConfig.cert to eliminate the EtcdTlsConfigRaw
intermediate struct and the hand-written Deserialize implementation.
The untagged enum tries Files first, so file-path fields take precedence
when deserializing. Mixed inputs (e.g. ca_file + cert) are rejected
because deny_unknown_fields causes both variants to fail.
EtcdTlsConfig.cert is now EtcdTlsCertConfig (non-Optional) with a Default
of Files { all None }; pem accessors and validate_connect_config are
updated accordingly.
bzp2010
approved these changes
Apr 20, 2026
Collaborator
bzp2010
left a comment
bzp2010
left a comment
There was a problem hiding this comment.
I approve this change in principle. I will address the remaining issues in next pull request.
bzp2010
reviewed
Apr 20, 2026
bzp2010
reviewed
Apr 20, 2026
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.
Why
The original implementation used
etcd-client'stlsfeature, which is backed byrustls. This has several drawbacks:ring/aws-lc-rsas additional cryptographic dependencies, adding compilation overhead and FFI risk.The project already links against OpenSSL (used by
reqwestvianative-tlsand byaxum-server), so reusing it for etcd TLS avoids pulling in a second cryptographic stack.What changed
etcd-clientfromfeatures = ["tls"]tofeatures = ["tls-openssl"].EtcdTlsCertConfigenum with two variants:Strings { ca, cert, key }— inline PEM-encoded content (e.g. from environment variables or secrets managers).Files { ca_file, cert_file, key_file }— paths to PEM files on disk.EtcdTlsConfigholds an optionalcert: Option<EtcdTlsCertConfig>andinsecure_skip_verify(defaultfalse). It deserializes from a flat config block; mixing inline strings and file paths in the same block is rejected at parse time.ca_pem(),cert_pem(),key_pem()on both types abstract over the two sources so call sites are source-agnostic.https://scheme.http:///https://host lists and hosts without a valid scheme are rejected with a clear typed error (EtcdConfigError).certandkey(orcert_fileandkey_file) must be provided together for mTLS; specifying only one is an error.insecure_skip_verify: truedisables all certificate validation (hostname, CA); intended for development/testing only.Debugimplementations redact all inline PEM content andpasswordfields to prevent sensitive material leaking into logs. File paths are shown as-is.validate_connect_config()returning a typedEtcdConfigError, making error handling precise and matchable.Configuration
Two forms are supported and are mutually exclusive within the same
tlsblock.File paths:
Inline PEM strings (suitable for container environments where secrets are injected as environment variables):
tlsis optional. When omitted, the system's default CA store is used for server certificate verification.