Skip to content

MGMT-21985: Enable primary IPv6 clusters#658

Merged
openshift-merge-bot[bot] merged 1 commit intorh-ecosystem-edge:mainfrom
danmanor:primary-ipv6
Oct 27, 2025
Merged

MGMT-21985: Enable primary IPv6 clusters#658
openshift-merge-bot[bot] merged 1 commit intorh-ecosystem-edge:mainfrom
danmanor:primary-ipv6

Conversation

@danmanor
Copy link
Copy Markdown
Member

@danmanor danmanor commented Oct 22, 2025

Background:
There are two variants of dual-stack clusters:
  * IPv4 primary, IPv6 secondary
  * IPv6 primary, IPv4 secondary

Only the first variant was supported until now, This PR enables the second variant as well by removing all assumptions about the order of which the ips are provided / detected.

Summary by CodeRabbit

  • Refactor
    • Strengthened validation for dual-stack IP operations, ensuring counts align before changes are applied.
    • Unified handling of node/internal IPs to support arbitrary IP pairs rather than fixed per-type paths.
    • Improved logging and per-pair error context to make IP renaming steps clearer and easier to diagnose.

@openshift-ci-robot
Copy link
Copy Markdown
Collaborator

openshift-ci-robot commented Oct 22, 2025

@danmanor: This pull request references MGMT-21985 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set.

Details

In response to this:

Remove the "IPv4 first" assumption

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot requested review from omertuc and tsorya October 22, 2025 11:27
@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Oct 22, 2025

Hi @danmanor. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Oct 22, 2025

Walkthrough

Refactors dual-stack IP renaming to validate equal counts of original and new IPs, iterate generically over (original_ip, new_ip) pairs for directory and etcd resource fixes, and consolidate InternalIP extraction into a single collection with unified logging. No public signatures changed.

Changes

Cohort / File(s) Summary
IP Rename Refactoring
src/ocp_postprocess/ip_rename.rs
Validate that counts of original and new dual-stack IPs match; add info log before applying dual-stack changes; replace hard-coded IPv4/IPv6 replacement logic with a generic loop over (original_ip, new_ip) pairs; add per-pair logging and contextualized error messages; internal-only changes, no public API changes.
IP Extraction Consolidation
src/ocp_postprocess/ip_rename/etcd_rename.rs
Remove is_ipv6 helper and per-type parsing; collect all InternalIP values into one unified vector; simplify extraction loop and consolidate logging to report total count and combined InternalIP list; preserve validation and function boundaries, no signature changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant IPRename as ip_rename.rs
  participant EtcdExtract as etcd_rename.rs
  participant DirFix
  participant EtcdFix

  Note over Caller,IPRename: Start dual-stack IP rename flow
  Caller->>IPRename: call rename_all_dual_stack(original_ips, new_ips)
  IPRename->>IPRename: validate len(original_ips) == len(new_ips)
  IPRename->>EtcdExtract: (optional) extract original InternalIP list
  EtcdExtract-->>IPRename: return unified_internal_ips

  alt counts match
    IPRename->>IPRename: log "applying dual-stack IP changes"
    loop for each (orig, new)
      IPRename->>DirFix: fix_dir_resources_dual_stack(orig, new)
      DirFix-->>IPRename: ok / error (with context)
      IPRename->>EtcdFix: fix_etcd_resources_dual_stack(orig, new)
      EtcdFix-->>IPRename: ok / error (with context)
    end
    IPRename-->>Caller: success
  else mismatch
    IPRename-->>Caller: error (count mismatch)
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Areas to focus on:

  • src/ocp_postprocess/ip_rename.rs: correctness of per-pair iteration, logging, and error contexts.
  • src/ocp_postprocess/ip_rename/etcd_rename.rs: ensure unified extraction preserves ordering/semantics expected by callers.
  • Any implicit assumptions about IPv4/IPv6 ordering in callers or downstream consumers.

Possibly related PRs

Suggested labels

lgtm

Suggested reviewers

  • omertuc
  • tsorya

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "MGMT-21985: Enable primary IPv6 clusters" directly aligns with the PR's main objective: removing the "IPv4 first" assumption to enable support for dual-stack clusters where IPv6 is the primary address. The file-level changes refactor the IP handling logic to be ordering-agnostic by replacing hard-coded IPv4/IPv6 replacements with generic loops and unified IP collection, which is precisely what enables this capability. The title is concise, clear, specific, and avoids vague terminology; a teammate scanning git history would immediately understand that this PR enables IPv6-primary cluster support rather than treating IPv6 as secondary.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci-robot
Copy link
Copy Markdown
Collaborator

openshift-ci-robot commented Oct 22, 2025

@danmanor: This pull request references MGMT-21985 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set.

Details

In response to this:

Remove the "IPv4 first" assumption

Summary by CodeRabbit

  • Refactor
  • Improved IP address handling logic with generalized pair iteration and enhanced validation checks for configuration consistency.
  • Simplified address collection by consolidating separate tracking into unified result handling.

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link
Copy Markdown
Collaborator

openshift-ci-robot commented Oct 22, 2025

@danmanor: This pull request references MGMT-21985 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set.

Details

In response to this:

Remove the "IPv4 first" assumption when:

  1. Fetching original IPs.
  2. setting the new ones.

Summary by CodeRabbit

  • Refactor
  • Improved IP address handling logic with generalized pair iteration and enhanced validation checks for configuration consistency.
  • Simplified address collection by consolidating separate tracking into unified result handling.

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 openshift-eng/jira-lifecycle-plugin repository.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/ocp_postprocess/ip_rename/etcd_rename.rs (3)

63-66: Kubernetes kind casing bug breaks lookups.

"Configmap" should be "ConfigMap". This likely prevents fetching the resource.

Apply:

-    let k8s_resource_location = K8sResourceLocation::new(Some("openshift-apiserver"), "Configmap", "config", "v1");
+    let k8s_resource_location = K8sResourceLocation::new(Some("openshift-apiserver"), "ConfigMap", "config", "v1");

387-392: Minor: typo in context string.

"aut.openshift.io" → "auth.openshift.io".

-                                .context("aut.openshift.io/certificate-hostnames annotation not a string")?
+                                .context("auth.openshift.io/certificate-hostnames annotation not a string")?

1-606: Fix Kubernetes API resource kind casing from "Configmap" to "ConfigMap" across codebase.

Kubernetes API convention requires "ConfigMap" (PascalCase). Found 11 instances of incorrect "Configmap" (lowercase 'm'):

  • src/ocp_postprocess/ip_rename/etcd_rename.rs:64
  • src/ocp_postprocess/cluster_domain_rename.rs:134, 141, 191, 199
  • src/ocp_postprocess/cluster_domain_rename/etcd_rename.rs:333, 452, 576, 624, 675, 1065
🧹 Nitpick comments (5)
src/ocp_postprocess/ip_rename/etcd_rename.rs (3)

39-48: Order-dependent pairing risk.

Collecting all InternalIP values relies on upstream consumers passing new IPs in the same order. If order differs (e.g., IPv6 first), replacements may cross-wire.

  • Either document and enforce ordering contract, or validate per-pair IP family match upstream (see suggestion in ip_rename.rs comment).

Also applies to: 57-58


86-91: Centralize IPv6 bracket/URL construction to avoid drift.

Multiple spots build https://host:port with ad‑hoc checks (contains(':') vs parse::()). Consolidate into one helper to keep behavior consistent.

Example helper:

fn url_host(ip: &str) -> std::borrow::Cow<'_, str> {
    if ip.parse::<Ipv6Addr>().is_ok() {
        format!("[{ip}]").into()
    } else {
        std::borrow::Cow::Borrowed(ip)
    }
}

Usage:

let expected_original_url = format!("https://{}:2379", url_host(original_ip));
let new_member_url       = format!("https://{}:2380", url_host(ip));

Also applies to: 121-127, 132-136, 331-338, 482-506, 569-583


96-99: Optional: write YAML to reduce churn.

You're parsing YAML but serializing JSON back into the "config.yaml" string. Consider YAML to minimize diffs.

-            serde_json::Value::String(serde_json::to_string(&config).context("serializing config.yaml")?),
+            serde_json::Value::String(serde_yaml::to_string(&config).context("serializing config.yaml")?),
src/ocp_postprocess/ip_rename.rs (2)

75-87: Validate IP family per pair to prevent cross-family swaps.

Add a check ensuring original/new IPs in each pair are same family (IPv4↔IPv4 or IPv6↔IPv6).

Apply within this function:

 async fn fix_dir_resources_dual_stack(original_ips: &[String], ips: &[String], dir: &Path) -> Result<()> {
@@
-    for (idx, (original_ip, new_ip)) in original_ips.iter().zip(ips.iter()).enumerate() {
+    for (idx, (original_ip, new_ip)) in original_ips.iter().zip(ips.iter()).enumerate() {
+        let orig_v6 = original_ip.parse::<std::net::Ipv6Addr>().is_ok();
+        let new_v6  = new_ip.parse::<std::net::Ipv6Addr>().is_ok();
+        ensure!(
+            orig_v6 == new_v6,
+            "IP pair {idx} family mismatch: original {} ({}), new {} ({})",
+            original_ip,
+            if orig_v6 { "IPv6" } else { "IPv4" },
+            new_ip,
+            if new_v6 { "IPv6" } else { "IPv4" }
+        );
         filesystem_rename::fix_filesystem_ip(original_ip, new_ip, dir)
             .await
             .context(format!("fix filesystem IP replacement pair {} in {:?}", idx, dir))?;
     }

You can factor this check into a small helper if reused.


186-203: Mirror the family check and clarify logging.

  • Add the same per-pair family validation before applying etcd changes.
  • Make the header log reflect actual pair count.
-    log::info!("Applying dual-stack IP changes");
+    log::info!("Applying IP changes for {} pair(s)", original_ips.len());
@@
-    for (idx, (original_ip, new_ip)) in original_ips.iter().zip(new_ips.iter()).enumerate() {
+    for (idx, (original_ip, new_ip)) in original_ips.iter().zip(new_ips.iter()).enumerate() {
+        let orig_v6 = original_ip.parse::<std::net::Ipv6Addr>().is_ok();
+        let new_v6  = new_ip.parse::<std::net::Ipv6Addr>().is_ok();
+        ensure!(
+            orig_v6 == new_v6,
+            "IP pair {idx} family mismatch: original {} ({}), new {} ({})",
+            original_ip,
+            if orig_v6 { "IPv6" } else { "IPv4" },
+            new_ip,
+            if new_v6 { "IPv6" } else { "IPv4" }
+        );
         log::info!("Applying replacements pair {}: {} → {}", idx, original_ip, new_ip);
         fix_etcd_resources_for_ip_pair(etcd_client, original_ip, new_ip)
             .await
             .context(format!("applying etcd resource fixes for pair {}", idx))?;
     }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9d60bef and eadaa75.

📒 Files selected for processing (2)
  • src/ocp_postprocess/ip_rename.rs (2 hunks)
  • src/ocp_postprocess/ip_rename/etcd_rename.rs (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/ocp_postprocess/ip_rename.rs (1)
src/ocp_postprocess/ip_rename/filesystem_rename.rs (1)
  • fix_filesystem_ip (9-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build

@omertuc
Copy link
Copy Markdown
Member

omertuc commented Oct 22, 2025

/ok-to-test

@omertuc
Copy link
Copy Markdown
Member

omertuc commented Oct 22, 2025

Can you please provide some background on what's going on here and why this is needed? The way I understand it, there are two variants of dual-stack clusters:

  • IPv4 primary, IPv6 secondary
  • IPv6 primary, IPv4 secondary

Is this correct?

In that case, our existing assumption was that only clusters of the former variant exist, and this commit makes it so that we also support clusters of the latter variant?

What happens if a user tries to upgrade a cluster of the latter variant using a seed of the former variant or vice versa? Will they get an error?

Since this is essentially a property of clusters that is non-flexible for IBX, can we also have lifecycle agent inspect the seed image labels to know which variant the seed has and give an explicit error in case it's the wrong variant?

Or maybe I'm way off here. I would appreciate some context

@danmanor
Copy link
Copy Markdown
Member Author

danmanor commented Oct 22, 2025

@omertuc

Is this correct?

Yes exactly

In that case, our existing assumption was that only clusters of the former variant exist, and this commit makes it so that we also support clusters of the latter variant?

exactly

What happens if a user tries to upgrade a cluster of the latter variant using a seed of the former variant or vice versa? Will they get an error?

There are no seed of the latter variant because there are no clusters of this variant as it was blocked by the installer for SNO (none platform more accurate). Anyway, the seed's variant have to match the cluster's otherwise it won't work.

Since this is essentially a property of clusters that is non-flexible for IBX, can we also have lifecycle agent inspect the seed image labels to know which variant the seed has and give an explicit error in case it's the wrong variant?

Yes this is a good idea, I am following up with LCA PR

@omertuc
Copy link
Copy Markdown
Member

omertuc commented Oct 23, 2025

OK great. Please edit the commit message to include this background and information

@openshift-ci-robot
Copy link
Copy Markdown
Collaborator

openshift-ci-robot commented Oct 23, 2025

@danmanor: This pull request references MGMT-21985 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set.

Details

In response to this:

Remove the "IPv4 first" assumption when:

  1. Fetching original IPs.
  2. setting the new ones.

Summary by CodeRabbit

  • Refactor
  • Generalized dual-stack IP processing to iterate over paired original/new addresses with per-pair validation, logging, and error context.
  • Consolidated IP collection into a single unified result list, simplifying handling of single- vs multi-address cases and standardizing status messages.

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link
Copy Markdown
Collaborator

openshift-ci-robot commented Oct 23, 2025

@danmanor: This pull request references MGMT-21985 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set.

Details

In response to this:

Background:
There are two variants of dual-stack clusters:
* IPv4 primary, IPv6 secondary
* IPv6 primary, IPv4 secondary

Only the first variant was supported until now, This PR enables the second variant as well by removing all assumptions about the order of which the ips are provided / detected.

Summary by CodeRabbit

  • Refactor
  • Generalized dual-stack IP processing to iterate over paired original/new addresses with per-pair validation, logging, and error context.
  • Consolidated IP collection into a single unified result list, simplifying handling of single- vs multi-address cases and standardizing status messages.

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 openshift-eng/jira-lifecycle-plugin repository.

@danmanor danmanor requested a review from omertuc October 23, 2025 12:24
@danmanor
Copy link
Copy Markdown
Member Author

@omertuc Done, thank you

@openshift-ci-robot
Copy link
Copy Markdown
Collaborator

openshift-ci-robot commented Oct 23, 2025

@danmanor: This pull request references MGMT-21985 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set.

Details

In response to this:

Background:
There are two variants of dual-stack clusters:
* IPv4 primary, IPv6 secondary
* IPv6 primary, IPv4 secondary

Only the first variant was supported until now, This PR enables the second variant as well by removing all assumptions about the order of which the ips are provided / detected.

Summary by CodeRabbit

  • Refactor
  • Generalized dual-stack IP processing to iterate over paired original/new addresses with per-pair validation, logging, and contextual errors; unified IP collection into a single list and standardized messaging for single- vs multi-address cases.
  • Chores
  • Simplified example configuration to present dual-stack addresses as a flat list.

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 openshift-eng/jira-lifecycle-plugin repository.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/ocp_postprocess/ip_rename.rs (2)

28-36: Good centralization; add a family-alignment guard to prevent v4↔v6 cross-wiring.

Pairing currently relies on order. Add a check that each pair is IPv4↔IPv4 or IPv6↔IPv6 to avoid accidental cross-family replacements when input order varies. Also consider including the pair count in the log line.

Apply:

 use anyhow::{ensure, Context, Result};
+use std::net::{Ipv4Addr, Ipv6Addr};
@@
     ensure!(
         original_ips.len() == ips.len(),
         "original IPs count ({}) must equal new IPs count ({})",
         original_ips.len(),
         ips.len()
     );
 
-    log::info!("Applying dual-stack IP changes");
+    // Ensure each pair preserves IP family (IPv4↔IPv4 or IPv6↔IPv6).
+    let families_ok = original_ips
+        .iter()
+        .zip(ips.iter())
+        .all(|(o, n)| (o.parse::<Ipv4Addr>().is_ok() && n.parse::<Ipv4Addr>().is_ok())
+            || (o.parse::<Ipv6Addr>().is_ok() && n.parse::<Ipv6Addr>().is_ok()));
+    ensure!(families_ok, "original/new IP family mismatch: ensure pairs are v4↔v4 or v6↔v6");
+
+    log::info!("Applying dual-stack IP changes ({} pairs)", original_ips.len());

85-89: Improve error context for filesystem replacements (include IPs; 1-based index).

Helps triage which pair failed in which dir without relying on external logs. Keep at error path only to avoid extra info logs.

-    for (idx, (original_ip, new_ip)) in original_ips.iter().zip(ips.iter()).enumerate() {
+    for (idx, (original_ip, new_ip)) in original_ips.iter().zip(ips.iter()).enumerate() {
         filesystem_rename::fix_filesystem_ip(original_ip, new_ip, dir)
             .await
-            .context(format!("fix filesystem IP replacement pair {} in {:?}", idx, dir))?;
+            .context(format!(
+                "fix filesystem IP replacement pair {} ({} → {}) in {:?}",
+                idx + 1,
+                original_ip,
+                new_ip,
+                dir
+            ))?;
     }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2b9df51 and 58255a3.

📒 Files selected for processing (3)
  • hack/dummy_config.yaml (1 hunks)
  • src/ocp_postprocess/ip_rename.rs (3 hunks)
  • src/ocp_postprocess/ip_rename/etcd_rename.rs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • hack/dummy_config.yaml
  • src/ocp_postprocess/ip_rename/etcd_rename.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/ocp_postprocess/ip_rename.rs (2)
src/ocp_postprocess/ip_rename/etcd_rename.rs (1)
  • original_ip (569-569)
src/ocp_postprocess/ip_rename/filesystem_rename.rs (1)
  • fix_filesystem_ip (9-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build

Comment on lines +189 to +194
for (idx, (original_ip, new_ip)) in original_ips.iter().zip(new_ips.iter()).enumerate() {
log::info!("Applying replacements pair {}: {} → {}", idx, original_ip, new_ip);
fix_etcd_resources_for_ip_pair(etcd_client, original_ip, new_ip)
.await
.context(format!("applying etcd resource fixes for pair {}", idx))?;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Avoid logging IPs at info level; downgrade to debug.

IPs can be sensitive operational data and are noisy at info. Keep the message but at debug.

-        log::info!("Applying replacements pair {}: {} → {}", idx, original_ip, new_ip);
+        log::debug!("Applying replacements pair {}: {} → {}", idx, original_ip, new_ip);
📝 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.

Suggested change
for (idx, (original_ip, new_ip)) in original_ips.iter().zip(new_ips.iter()).enumerate() {
log::info!("Applying replacements pair {}: {} → {}", idx, original_ip, new_ip);
fix_etcd_resources_for_ip_pair(etcd_client, original_ip, new_ip)
.await
.context(format!("applying etcd resource fixes for pair {}", idx))?;
}
for (idx, (original_ip, new_ip)) in original_ips.iter().zip(new_ips.iter()).enumerate() {
log::debug!("Applying replacements pair {}: {} → {}", idx, original_ip, new_ip);
fix_etcd_resources_for_ip_pair(etcd_client, original_ip, new_ip)
.await
.context(format!("applying etcd resource fixes for pair {}", idx))?;
}
🤖 Prompt for AI Agents
In src/ocp_postprocess/ip_rename.rs around lines 189 to 194, the code logs IP
addresses at info level; change the log call from info to debug so the same
replacement message is retained but emitted at debug level
(log::debug!("Applying replacements pair {}: {} → {}", idx, original_ip,
new_ip);), leaving the rest of the loop and error handling unchanged.

@openshift-ci-robot
Copy link
Copy Markdown
Collaborator

openshift-ci-robot commented Oct 23, 2025

@danmanor: This pull request references MGMT-21985 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set.

Details

In response to this:

Background:
There are two variants of dual-stack clusters:
* IPv4 primary, IPv6 secondary
* IPv6 primary, IPv4 secondary

Only the first variant was supported until now, This PR enables the second variant as well by removing all assumptions about the order of which the ips are provided / detected.

Summary by CodeRabbit

  • Refactor
  • Enhanced validation and error handling for IP address operations
  • Improved logging messages for IP renaming processes with better contextual information

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 openshift-eng/jira-lifecycle-plugin repository.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
src/ocp_postprocess/ip_rename.rs (1)

189-194: Downgrade IP-containing log to debug (duplicate of prior feedback).

Avoid emitting IPs at info; use debug to reduce noise and limit sensitive data exposure.

-        log::info!("Applying replacements pair {}: {} → {}", idx, original_ip, new_ip);
+        log::debug!("Applying replacements pair {}: {} → {}", idx, original_ip, new_ip);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 58255a3 and 401c52a.

📒 Files selected for processing (2)
  • src/ocp_postprocess/ip_rename.rs (3 hunks)
  • src/ocp_postprocess/ip_rename/etcd_rename.rs (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • src/ocp_postprocess/ip_rename/etcd_rename.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/ocp_postprocess/ip_rename.rs (2)
src/ocp_postprocess/ip_rename/etcd_rename.rs (1)
  • original_ip (569-569)
src/ocp_postprocess/ip_rename/filesystem_rename.rs (1)
  • fix_filesystem_ip (9-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🔇 Additional comments (1)
src/ocp_postprocess/ip_rename.rs (1)

85-89: LGTM: unified per-pair filesystem updates.

Iterating pairs with zip + enumerate and passing context is clean and maintainable.

@omertuc
Copy link
Copy Markdown
Member

omertuc commented Oct 27, 2025

/lgtm

@omertuc
Copy link
Copy Markdown
Member

omertuc commented Oct 27, 2025

/approve

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Oct 27, 2025

@danmanor: danmanor unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:.

Details

In response to this:

/override ci/prow/e2e-aws-ovn-single-node-recert-parallel

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.

@openshift-ci-robot
Copy link
Copy Markdown
Collaborator

openshift-ci-robot commented Oct 27, 2025

@danmanor: This pull request references MGMT-21985 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set.

Details

In response to this:

Background:
There are two variants of dual-stack clusters:
* IPv4 primary, IPv6 secondary
* IPv6 primary, IPv4 secondary

Only the first variant was supported until now, This PR enables the second variant as well by removing all assumptions about the order of which the ips are provided / detected.

Summary by CodeRabbit

  • Refactor
  • Strengthened validation for dual-stack IP operations, ensuring counts align before changes are applied.
  • Unified handling of node/internal IPs to support arbitrary IP pairs rather than fixed per-type paths.
  • Improved logging and per-pair error context to make IP renaming steps clearer and easier to diagnose.

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 openshift-eng/jira-lifecycle-plugin repository.

@danmanor
Copy link
Copy Markdown
Member Author

/override ci/prow/e2e-aws-ovn-single-node-recert-parallel

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Oct 27, 2025

@danmanor: danmanor unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:.

Details

In response to this:

/override ci/prow/e2e-aws-ovn-single-node-recert-parallel

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.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
src/ocp_postprocess/ip_rename.rs (1)

28-35: Add IP family validation to prevent silent IPv4↔IPv6 swaps.

The length check is necessary but insufficient. Pairing IPs solely by position risks silent family mismatches if the caller provides IPs in different orders. A previous review flagged this as critical, but the validation is still missing.

Apply this diff after the length check:

     ensure!(
         original_ips.len() == ips.len(),
         "original IPs count ({}) must equal new IPs count ({})",
         original_ips.len(),
         ips.len()
     );
+    // Validate IP family alignment to prevent IPv4<->IPv6 swaps
+    for (idx, (orig, new)) in original_ips.iter().zip(ips.iter()).enumerate() {
+        let orig_parsed: std::net::IpAddr = orig.parse()
+            .context(format!("parsing original IP at pair {}: {}", idx, orig))?;
+        let new_parsed: std::net::IpAddr = new.parse()
+            .context(format!("parsing new IP at pair {}: {}", idx, new))?;
+        ensure!(
+            orig_parsed.is_ipv4() == new_parsed.is_ipv4(),
+            "IP family mismatch at pair {}: original={} (IPv{}) new={} (IPv{})",
+            idx, orig, if orig_parsed.is_ipv4() { "4" } else { "6" },
+            new, if new_parsed.is_ipv4() { "4" } else { "6" }
+        );
+    }
 
     log::info!("Applying dual-stack IP changes");

Based on past review comments.

🧹 Nitpick comments (1)
src/ocp_postprocess/ip_rename.rs (1)

189-194: Consider debug-level logging for IP addresses; iteration logic is sound.

The generic iteration correctly processes arbitrary dual-stack pairs with proper error context. However, a previous review recommended downgrading IP logging from info to debug, which remains unaddressed.

If you want to reduce noise and avoid logging potentially sensitive IPs at info level, apply:

-        log::info!("Applying replacements pair {}: {} → {}", idx, original_ip, new_ip);
+        log::debug!("Applying replacements pair {}: {} → {}", idx, original_ip, new_ip);

Based on past review comments.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 401c52a and aabc845.

📒 Files selected for processing (2)
  • src/ocp_postprocess/ip_rename.rs (3 hunks)
  • src/ocp_postprocess/ip_rename/etcd_rename.rs (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/ocp_postprocess/ip_rename.rs (2)
src/ocp_postprocess/ip_rename/etcd_rename.rs (1)
  • original_ip (569-569)
src/ocp_postprocess/ip_rename/filesystem_rename.rs (1)
  • fix_filesystem_ip (9-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Red Hat Konflux / recert-4-21-on-pull-request
  • GitHub Check: build
🔇 Additional comments (3)
src/ocp_postprocess/ip_rename.rs (1)

85-89: LGTM! Generic iteration correctly handles arbitrary dual-stack orderings.

The refactor from hard-coded IP handling to generic paired iteration achieves the PR objective of removing IPv4-first assumptions. Error context includes the pair index for clear debugging.

src/ocp_postprocess/ip_rename/etcd_rename.rs (2)

9-9: LGTM! Unified InternalIP collection removes ordering assumptions.

The simplification from separate IPv4/IPv6 tracking to a single result vector directly supports the PR's goal of enabling arbitrary dual-stack orderings. The Ipv6Addr import is retained for formatting checks elsewhere (lines 569, 578), which is appropriate.

Also applies to: 39-39, 47-47


57-57: LGTM! Logging now reflects unified IP collection.

The updated log message correctly shows the total count and all IPs in a single message, consistent with the order-agnostic refactor.

@danmanor
Copy link
Copy Markdown
Member Author

/override ci/prow/e2e-aws-ovn-single-node-recert-parallel

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Oct 27, 2025

@danmanor: danmanor unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:.

Details

In response to this:

/override ci/prow/e2e-aws-ovn-single-node-recert-parallel

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.

@danmanor
Copy link
Copy Markdown
Member Author

/override ci/prow/e2e-aws-ovn-single-node-recert-parallel

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Oct 27, 2025

@danmanor: danmanor unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:.

Details

In response to this:

/override ci/prow/e2e-aws-ovn-single-node-recert-parallel

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.

@danmanor
Copy link
Copy Markdown
Member Author

/override ci/prow/e2e-aws-ovn-single-node-recert-serial

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Oct 27, 2025

@danmanor: danmanor unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:.

Details

In response to this:

/override ci/prow/e2e-aws-ovn-single-node-recert-serial

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.

@danmanor
Copy link
Copy Markdown
Member Author

/override ci/prow/e2e-aws-ovn-single-node-recert-serial

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Oct 27, 2025

@danmanor: Overrode contexts on behalf of danmanor: ci/prow/e2e-aws-ovn-single-node-recert-serial

Details

In response to this:

/override ci/prow/e2e-aws-ovn-single-node-recert-serial

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.

@danmanor
Copy link
Copy Markdown
Member Author

/override ci/prow/e2e-aws-ovn-single-node-recert-parallel

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Oct 27, 2025

@danmanor: Overrode contexts on behalf of danmanor: ci/prow/e2e-aws-ovn-single-node-recert-parallel

Details

In response to this:

/override ci/prow/e2e-aws-ovn-single-node-recert-parallel

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
Copy link
Copy Markdown
Member

omertuc commented Oct 27, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 27, 2025
@danmanor
Copy link
Copy Markdown
Member Author

/override ci/prow/e2e-aws-ovn-single-node-recert-parallel

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Oct 27, 2025

@danmanor: Overrode contexts on behalf of danmanor: ci/prow/e2e-aws-ovn-single-node-recert-parallel

Details

In response to this:

/override ci/prow/e2e-aws-ovn-single-node-recert-parallel

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.

@danmanor
Copy link
Copy Markdown
Member Author

/override ci/prow/e2e-aws-ovn-single-node-recert-serial

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Oct 27, 2025

@danmanor: Overrode contexts on behalf of danmanor: ci/prow/e2e-aws-ovn-single-node-recert-serial

Details

In response to this:

/override ci/prow/e2e-aws-ovn-single-node-recert-serial

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.

@openshift-merge-bot openshift-merge-bot bot merged commit 8d9fe53 into rh-ecosystem-edge:main Oct 27, 2025
18 checks passed
@danmanor
Copy link
Copy Markdown
Member Author

/cherry-pick release-4.20

@openshift-cherrypick-robot
Copy link
Copy Markdown

@danmanor: only rh-ecosystem-edge org members may request cherry picks. If you are already part of the org, make sure to change your membership to public. Otherwise you can still do the cherry-pick manually.

Details

In response to this:

/cherry-pick release-4.20

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.

@danmanor
Copy link
Copy Markdown
Member Author

/cherry-pick release-4.20

@openshift-cherrypick-robot
Copy link
Copy Markdown

@danmanor: only rh-ecosystem-edge org members may request cherry picks. If you are already part of the org, make sure to change your membership to public. Otherwise you can still do the cherry-pick manually.

Details

In response to this:

/cherry-pick release-4.20

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.

@danmanor
Copy link
Copy Markdown
Member Author

/cherry-pick release-4.20

@openshift-cherrypick-robot
Copy link
Copy Markdown

@danmanor: new pull request created: #667

Details

In response to this:

/cherry-pick release-4.20

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants