Fix etcd static pod hostname renaming for non-shell containers#1550
Fix etcd static pod hostname renaming for non-shell containers#1550omertuc wants to merge 1 commit into
Conversation
WalkthroughHostname replacement patterns are extended and the static-pod/container hostname rewrite is refactored to apply a centralized list of pattern/replacement pairs to every string argument in Changesetcd hostname rewrite changes
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ocp_postprocess/rename_utils.rs`:
- Around line 560-562: The loop that iterates "for arg in args.iter_mut()"
currently ignores non-string entries by using "let Some(arg_str) = arg.as_str()
else { continue }"; change this to fail fast by returning an Err when
arg.as_str() is None so malformed `/command` entries surface immediately. Update
the enclosing function's signature to return a Result if it doesn't already, and
replace the "continue" with an appropriate error creation (including context
like the offending arg and that it must be a string). Ensure callers handle the
propagated error instead of assuming successful renames.
🪄 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: 7d5a61b5-23a7-46ff-a0b7-72ae486a54d0
📒 Files selected for processing (1)
src/ocp_postprocess/rename_utils.rs
| for arg in args.iter_mut() { | ||
| let Some(arg_str) = arg.as_str() else { continue }; | ||
| let mut replaced = arg_str.to_string(); |
There was a problem hiding this comment.
Fail fast if a /command entry is not a string.
At Line 561, non-string args are silently skipped. That can mask malformed pod specs and make rename failures harder to debug. Prefer returning an error instead of continue.
Suggested patch
- for arg in args.iter_mut() {
- let Some(arg_str) = arg.as_str() else { continue };
+ for arg in args.iter_mut() {
+ let arg_str = arg.as_str().context("command arg not a string")?;
let mut replaced = arg_str.to_string();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for arg in args.iter_mut() { | |
| let Some(arg_str) = arg.as_str() else { continue }; | |
| let mut replaced = arg_str.to_string(); | |
| for arg in args.iter_mut() { | |
| let arg_str = arg.as_str().context("command arg not a string")?; | |
| let mut replaced = arg_str.to_string(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ocp_postprocess/rename_utils.rs` around lines 560 - 562, The loop that
iterates "for arg in args.iter_mut()" currently ignores non-string entries by
using "let Some(arg_str) = arg.as_str() else { continue }"; change this to fail
fast by returning an Err when arg.as_str() is None so malformed `/command`
entries surface immediately. Update the enclosing function's signature to return
a Result if it doesn't already, and replace the "continue" with an appropriate
error creation (including context like the offending arg and that it must be a
string). Ensure callers handle the propagated error instead of assuming
successful renames.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/ocp_postprocess/rename_utils.rs (1)
569-571:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNon-string
/commandargs are still silently skipped.The
let Some(arg_str) = arg.as_str() else { continue };at line 570 will silently bypass any malformed/commandentries instead of surfacing them. Recommend failing fast witharg.as_str().context("command arg not a string")?so unexpected pod-spec shapes are caught immediately rather than producing a partially-renamed container.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ocp_postprocess/rename_utils.rs` around lines 569 - 571, Replace the silent-skip pattern inside the args loop — change the `let Some(arg_str) = arg.as_str() else { continue };` to fail-fast by using `let arg_str = arg.as_str().context("command arg not a string")?;` (bring in anyhow::Context if not already). This ensures when iterating `args.iter_mut()` you surface malformed `/command` entries instead of continuing, and keep the rest of the logic that builds `replaced` unchanged.
🧹 Nitpick comments (1)
src/ocp_postprocess/rename_utils.rs (1)
569-586: 💤 Low valueHoist pattern interpolation out of the per-arg loop.
The two
.replace("{original_hostname}", …).replace("{original_hostname_safe}", …)calls (and the analogous ones for the replacement) only depend onoriginal_hostname/hostname, yet they are re-evaluated for every command arg × every pattern. Pre-compute the resolved(pattern, replacement)pairs once before iteratingargs, both for clarity and to avoid the redundant per-arg allocations. This also makes the new code symmetric withfix_etcd_env/fix_etcd_pod_yaml_hostname(single resolution pass).♻️ Suggested refactor
- for arg in args.iter_mut() { - let Some(arg_str) = arg.as_str() else { continue }; - let mut replaced = arg_str.to_string(); - - for (pattern, replacement) in &patterns { - let pattern = pattern - .replace("{original_hostname}", original_hostname) - .replace("{original_hostname_safe}", &env_var_safe(original_hostname)); - - let replacement = replacement - .replace("{hostname}", hostname) - .replace("{hostname_safe}", &env_var_safe(hostname)); - - replaced = replaced.replace(&pattern, &replacement); - } - - *arg = serde_json::Value::String(replaced); - } + let original_hostname_safe = env_var_safe(original_hostname); + let hostname_safe = env_var_safe(hostname); + let resolved_patterns: Vec<(String, String)> = patterns + .iter() + .map(|(pattern, replacement)| { + ( + pattern + .replace("{original_hostname}", original_hostname) + .replace("{original_hostname_safe}", &original_hostname_safe), + replacement + .replace("{hostname}", hostname) + .replace("{hostname_safe}", &hostname_safe), + ) + }) + .collect(); + + for arg in args.iter_mut() { + let Some(arg_str) = arg.as_str() else { continue }; + let mut replaced = arg_str.to_string(); + for (pattern, replacement) in &resolved_patterns { + replaced = replaced.replace(pattern, replacement); + } + *arg = serde_json::Value::String(replaced); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ocp_postprocess/rename_utils.rs` around lines 569 - 586, Precompute the interpolated (pattern, replacement) pairs once before iterating args: map the existing patterns collection by applying .replace("{original_hostname}", original_hostname).replace("{original_hostname_safe}", &env_var_safe(original_hostname)) and similarly replace "{hostname}" and "{hostname_safe}" on the replacement, producing a resolved_patterns Vec; then iterate args.iter_mut(), convert arg to str, and sequentially apply replaced = replaced.replace(&resolved_pattern, &resolved_replacement) and assign *arg = serde_json::Value::String(replaced). This moves hostname-dependent interpolation out of the per-arg loop while keeping env_var_safe, patterns, args and the existing serde_json assignment intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ocp_postprocess/rename_utils.rs`:
- Around line 427-435: Run rustfmt and commit the formatted changes: apply cargo
fmt locally, which will reformat the recently added tuples such as
"NODE_{original_hostname_safe}_ETCD_URL_HOST"/"NODE_{hostname_safe}_ETCD_URL_HOST",
"NODE_{original_hostname_safe}_ETCD_NAME"/"NODE_{hostname_safe}_ETCD_NAME", and
"NODE_{original_hostname_safe}_IP"/"NODE_{hostname_safe}_IP" in rename_utils.rs,
then stage and commit the result so CI `cargo fmt --check` passes.
---
Duplicate comments:
In `@src/ocp_postprocess/rename_utils.rs`:
- Around line 569-571: Replace the silent-skip pattern inside the args loop —
change the `let Some(arg_str) = arg.as_str() else { continue };` to fail-fast by
using `let arg_str = arg.as_str().context("command arg not a string")?;` (bring
in anyhow::Context if not already). This ensures when iterating
`args.iter_mut()` you surface malformed `/command` entries instead of
continuing, and keep the rest of the logic that builds `replaced` unchanged.
---
Nitpick comments:
In `@src/ocp_postprocess/rename_utils.rs`:
- Around line 569-586: Precompute the interpolated (pattern, replacement) pairs
once before iterating args: map the existing patterns collection by applying
.replace("{original_hostname}",
original_hostname).replace("{original_hostname_safe}",
&env_var_safe(original_hostname)) and similarly replace "{hostname}" and
"{hostname_safe}" on the replacement, producing a resolved_patterns Vec; then
iterate args.iter_mut(), convert arg to str, and sequentially apply replaced =
replaced.replace(&resolved_pattern, &resolved_replacement) and assign *arg =
serde_json::Value::String(replaced). This moves hostname-dependent interpolation
out of the per-arg loop while keeping env_var_safe, patterns, args and the
existing serde_json assignment intact.
🪄 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: b6d10128-2b9c-4e09-9bce-fdb662d6aa1f
📒 Files selected for processing (1)
src/ocp_postprocess/rename_utils.rs
Required due to openshift/cluster-etcd-operator#1606 The `etcd-ensure-env-vars` init container changed from an inline bash script to a Go binary (cluster-etcd-operator ensure-env), which caused `fix_etcd_static_pod_container` to skip renaming hostname references in command args. The function relied on finding a `"#!/bin/sh\n"` prefix to identify args containing hostname-dependent strings, and silently skipped the entire replacement block when no such prefix was found. This left stale `NODE_{old_hostname}_*` references in the container's command args while the env var definitions were correctly renamed, causing etcd to fail at startup with "environment variable `NODE_<old>_ETCD_URL_HOST` must be set". Apply hostname replacements to all command args unconditionally instead of gating on the shebang heuristic.
|
/retest |
|
/test ci/prow/baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor, omertuc The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
All PipelineRuns for this commit have already succeeded. Use |
|
/test baremetalds-sno-recert-cluster-rename |
7 similar comments
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
13 similar comments
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/test baremetalds-sno-recert-cluster-rename |
|
/retest |
|
/test ? |
|
/test ibi-e2e-flow |
|
/test ibu-e2e-flow |
|
/retest |
1 similar comment
|
/retest |
|
/override ? |
|
@omertuc: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override ci/prow/baremetalds-sno-recert-cluster-rename |
|
/override ci/prow/e2e-aws-ovn-single-node-recert-parallel |
|
/override ci/prow/ibu-e2e-flow |
|
@omertuc: Overrode contexts on behalf of omertuc: ci/prow/baremetalds-sno-recert-cluster-rename DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@omertuc: Overrode contexts on behalf of omertuc: ci/prow/e2e-aws-ovn-single-node-recert-parallel DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@omertuc: Overrode contexts on behalf of omertuc: ci/prow/ibu-e2e-flow DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Overriding cause we're looking better than #1636 |
|
@omertuc: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Required due to openshift/cluster-etcd-operator#1606
The
etcd-ensure-env-varsinit container changed from an inline bash script to a Go binary (cluster-etcd-operator ensure-env), which causedfix_etcd_static_pod_containerto skip renaming hostname references in command args. The function relied on finding a"#!/bin/sh\n"prefix to identify args containing hostname-dependent strings, and silently skipped the entire replacement block when no such prefix was found.This left stale
NODE_{old_hostname}_*references in the container's command args while the env var definitions were correctly renamed, causing etcd to fail at startup with "environment variableNODE_<old>_ETCD_URL_HOSTmust be set".Apply hostname replacements to all command args unconditionally instead of gating on the shebang heuristic.