refactor(etcd): refine config struct and parser#53
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:
📝 WalkthroughWalkthroughDeserializer now accepts inline PEM or file paths, reads files when provided, parses PEM into OpenSSL objects (CA chain Changes
Sequence Diagram(s)sequenceDiagram
participant UserConfig as "User config (JSON/TOML)"
participant Deserializer as "EtcdTlsConfig::Deserialize"
participant FS as "File system (optional)"
participant OpenSSL as "OpenSSL PEM parser"
participant EtcdClient as "Etcd TLS setup"
UserConfig->>Deserializer: provide `ca`/`cert`/`key` or `*_file` fields
Deserializer->>FS: read file paths if `*_file` present
Deserializer->>OpenSSL: parse PEM into X509 chain(s) and PKey
OpenSSL-->>Deserializer: return parsed objects
Deserializer-->>EtcdClient: produce `EtcdTlsConfig` with parsed certs/keys
EtcdClient->>OpenSSL: load CA certs into store, set client cert/key
EtcdClient-->>UserConfig: TLS-ready etcd connection config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config/etcd.rs (1)
161-176:⚠️ Potential issue | 🟡 MinorReject an empty host list during validation.
host: []currently passes validation and only fails later in the retry loop, which makes a bad config look like a transient connectivity problem.Small fix
pub enum EtcdConfigError { + #[error("etcd host list cannot be empty")] + EmptyHosts, + #[error("etcd hosts must use a single scheme (all http:// or all https://)")] MixedSchemes,fn validate_connect_config(config: &Config) -> Result<(), EtcdConfigError> { + if config.host.is_empty() { + return Err(EtcdConfigError::EmptyHosts); + } + let has_https = config.host.iter().any(|h| h.starts_with("https://")); let has_http = config.host.iter().any(|h| h.starts_with("http://"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/etcd.rs` around lines 161 - 176, validate_connect_config currently allows an empty config.host which should be rejected early; add a check at the start of validate_connect_config (before checking schemes) that returns a new EtcdConfigError variant for an empty host list when config.host.is_empty() is true, and if that variant does not exist add EtcdConfigError::EmptyHostList to the EtcdConfigError enum (or use a clearly named existing variant) so callers get a configuration error instead of a connectivity retry loop; keep the rest of the scheme checks (has_http/has_https and MissingScheme) unchanged.
🤖 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 80-109: Replace the current use of serde_json::Value in
EtcdTlsConfig::deserialize with a small strongly-typed intermediate struct
(e.g., EtcdTlsRaw { insecure_skip_verify: Option<bool>, ca: Option<String>,
ca_file: Option<String>, cert: Option<String>, cert_file: Option<String>, key:
Option<String>, key_file: Option<String> }) and deserialize into that to force
type validation; then map fields from that struct into EtcdTlsConfig (set
insecure_skip_verify from the Option<bool> or error on wrong type), and when
constructing ca_cert and client_cert use the existing helpers (Self::extract_pem
logic should be adapted to accept either the raw inline string or file path from
the typed struct, then call Self::parse_x509_chain("ca"/"cert", &pem) and
PKey::private_key_from_pem for the key), returning de::Error::custom on
missing/invalid combinations—apply the same pattern for the client_cert branch
used with EtcdTlsCertConfig, Self::parse_x509_chain, and
PKey::private_key_from_pem so type errors like `"insecure_skip_verify": "true"`
fail deserialization instead of silently defaulting.
---
Outside diff comments:
In `@src/config/etcd.rs`:
- Around line 161-176: validate_connect_config currently allows an empty
config.host which should be rejected early; add a check at the start of
validate_connect_config (before checking schemes) that returns a new
EtcdConfigError variant for an empty host list when config.host.is_empty() is
true, and if that variant does not exist add EtcdConfigError::EmptyHostList to
the EtcdConfigError enum (or use a clearly named existing variant) so callers
get a configuration error instead of a connectivity retry loop; keep the rest of
the scheme checks (has_http/has_https and MissingScheme) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/config/etcd.rs (1)
80-86:⚠️ Potential issue | 🟠 MajorFail fast on invalid TLS field types during deserialization.
as_bool().unwrap_or(false)andas_str()-only extraction silently coerce wrong JSON types into defaults/missing values. Example:"insecure_skip_verify": "true"is treated asfalseinstead of erroring.Suggested fix (typed raw struct)
+#[derive(Deserialize)] +struct RawEtcdTlsConfig { + insecure_skip_verify: Option<bool>, + ca: Option<String>, + ca_file: Option<String>, + cert: Option<String>, + cert_file: Option<String>, + key: Option<String>, + key_file: Option<String>, +} + impl<'de> Deserialize<'de> for EtcdTlsConfig { fn deserialize<D>(deserializer: D) -> std::result::Result<Self, D::Error> where D: Deserializer<'de>, { - let mut config = EtcdTlsConfig::default(); - let value = serde_json::Value::deserialize(deserializer)?; + let raw = RawEtcdTlsConfig::deserialize(deserializer)?; + let mut config = EtcdTlsConfig::default(); - config.insecure_skip_verify = value - .get("insecure_skip_verify") - .and_then(|v| v.as_bool()) - .unwrap_or(false); + config.insecure_skip_verify = raw.insecure_skip_verify.unwrap_or(false); // map remaining fields from typed options...In serde_json, what does `serde_json::Value::as_bool()` return when the JSON value is a string, e.g. `"true"`?Also applies to: 95-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/etcd.rs` around lines 80 - 86, The deserializer for EtcdTlsConfig is silently accepting wrong JSON types (e.g., "insecure_skip_verify": "true") via as_bool()/as_str() and unwrap_or, so change it to validate types and fail fast: when deserializing inside the function that builds EtcdTlsConfig (the block using EtcdTlsConfig::default and Value::deserialize(deserializer)), replace the as_bool().unwrap_or(false) and any as_str() extractions with explicit checks that the JSON value exists and is the expected type (matching Value::get(...).and_then(...) with pattern matching) and return a serde::de::Error::custom on type mismatch; alternatively parse into a strongly-typed intermediate struct via serde_json::from_value and map errors to serde::de errors — apply the same fix for all TLS fields handled in the same deserializer (the subsequent fields around the current block).
🧹 Nitpick comments (1)
src/config/etcd.rs (1)
127-138: Reject ambiguouslabel+label_filecombinations explicitly.If both inline PEM and file path are provided for the same field, current logic silently prefers inline content. Returning an explicit error makes config intent unambiguous and easier to debug.
Suggested direction
-fn extract_pem(label: &str, value: &serde_json::Value) -> Option<Result<String>> { - if let Some(s) = value.get(label).and_then(|v| v.as_str()) { - Some(Ok(s.to_string())) - } else { - value - .get(format!("{label}_file")) - .and_then(|v| v.as_str()) - .map(|path| { - std::fs::read_to_string(path) - .with_context(|| format!("failed to read {label} file \"{path}\"")) - }) - } +fn extract_pem(label: &str, value: &serde_json::Value) -> Result<Option<String>> { + let inline = value.get(label).and_then(|v| v.as_str()).map(str::to_owned); + let file = value + .get(format!("{label}_file")) + .and_then(|v| v.as_str()) + .map(str::to_owned); + + match (inline, file) { + (Some(_), Some(_)) => Err(anyhow!("provide either {label} or {label}_file, not both")), + (Some(pem), None) => Ok(Some(pem)), + (None, Some(path)) => Ok(Some( + std::fs::read_to_string(&path) + .with_context(|| format!("failed to read {label} file \"{path}\""))?, + )), + (None, None) => Ok(None), + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/etcd.rs` around lines 127 - 138, In extract_pem, explicitly detect when both inline and "{label}_file" are provided and return Some(Err(...)) instead of silently preferring the inline value; inside the function (refer to extract_pem and the `label` variable) check value.get(label).and_then(|v| v.as_str()) and value.get(format!("{label}_file")).and_then(|v| v.as_str()) — if both are Some, return Some(Err(anyhow::anyhow!("both {label} and {label}_file provided, please supply only one"))) so the caller sees an explicit error; otherwise preserve the existing behavior for single sources.
🤖 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 80-86: The deserializer for EtcdTlsConfig is silently accepting
wrong JSON types (e.g., "insecure_skip_verify": "true") via as_bool()/as_str()
and unwrap_or, so change it to validate types and fail fast: when deserializing
inside the function that builds EtcdTlsConfig (the block using
EtcdTlsConfig::default and Value::deserialize(deserializer)), replace the
as_bool().unwrap_or(false) and any as_str() extractions with explicit checks
that the JSON value exists and is the expected type (matching
Value::get(...).and_then(...) with pattern matching) and return a
serde::de::Error::custom on type mismatch; alternatively parse into a
strongly-typed intermediate struct via serde_json::from_value and map errors to
serde::de errors — apply the same fix for all TLS fields handled in the same
deserializer (the subsequent fields around the current block).
---
Nitpick comments:
In `@src/config/etcd.rs`:
- Around line 127-138: In extract_pem, explicitly detect when both inline and
"{label}_file" are provided and return Some(Err(...)) instead of silently
preferring the inline value; inside the function (refer to extract_pem and the
`label` variable) check value.get(label).and_then(|v| v.as_str()) and
value.get(format!("{label}_file")).and_then(|v| v.as_str()) — if both are Some,
return Some(Err(anyhow::anyhow!("both {label} and {label}_file provided, please
supply only one"))) so the caller sees an explicit error; otherwise preserve the
existing behavior for single sources.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/config/etcd.rs (1)
35-43: Consider returning references instead of cloning in accessors.The
certs()andkey()methods clone their contents on every call. While this works, returning references (&[X509]and&PKey<Private>) would be more idiomatic and efficient. The current callers inconnect_clientcan clone where needed.♻️ Suggested refactor
- /// Return the parsed client certificate chain. - pub fn certs(&self) -> Vec<X509> { - self.certs.clone() + /// Return the parsed client certificate chain. + pub fn certs(&self) -> &[X509] { + &self.certs } - /// Return the parsed client private key. - pub fn key(&self) -> PKey<pkey::Private> { - self.key.clone() + /// Return the parsed client private key. + pub fn key(&self) -> &PKey<pkey::Private> { + &self.key }Then update the usage in
connect_client:- let cert = client_cert.certs(); - let key = client_cert.key(); + let cert = client_cert.certs().to_vec(); + let key = client_cert.key().clone();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/etcd.rs` around lines 35 - 43, Change the accessors to return references instead of cloning: modify certs() to return &[X509] (or &Vec<X509>) and key() to return &PKey<pkey::Private>, and update all call sites (notably connect_client) to clone only when they actually need owned values; keep the existing function names (certs, key) and types (X509, PKey<pkey::Private>) so you can locate them easily and replace the cloning at call sites with explicit .clone() where ownership is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/config/etcd.rs`:
- Around line 35-43: Change the accessors to return references instead of
cloning: modify certs() to return &[X509] (or &Vec<X509>) and key() to return
&PKey<pkey::Private>, and update all call sites (notably connect_client) to
clone only when they actually need owned values; keep the existing function
names (certs, key) and types (X509, PKey<pkey::Private>) so you can locate them
easily and replace the cloning at call sites with explicit .clone() where
ownership is required.
bzp2010
changed the title
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
1 participant
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.
Summary by CodeRabbit
Refactor
Bug Fixes
Tests