MGMT-24699: Assisted Service IPv6 CIDR comparison may fail with non-normalized CIDRs#10569
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@shay23bra: This pull request references MGMT-24699 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 bug to target the "5.0.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughCIDR values are now normalized before use in Hive network entry-to-model conversions (cluster, service, machine networks) and a new ChangesCIDR Normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)Not applicable. Related Issues: Not available from provided context. Related PRs: Not available from provided context. Suggested labels: bug, network Suggested reviewers: Not available from provided context. 🐰 In burrows of code where IPs roam free, 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shay23bra 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 |
openshift-ci
Bot
added
the
approved
shay23bra
force-pushed
the
MGMT-24699-Assisted-Service-IPv6-CIDR-comparison-may-fail-with-non-normalized-CIDRs
branch
2 times, most recently
from
545e24d to
34cfcc5
Compare
PostgreSQL auto-normalizes IPv6 CIDRs on storage (e.g. fcff:0069:0001::/64 → fcff:69:1::/64), but Go string comparison treated them as different, causing infinite reconciliation loops in the controller. This is the same class of bug fixed for VIPs in MGMT-24393 but affecting CIDR comparisons.
shay23bra
force-pushed
the
MGMT-24699-Assisted-Service-IPv6-CIDR-comparison-may-fail-with-non-normalized-CIDRs
branch
from
34cfcc5 to
e31c868
Compare
openshift-ci
Bot
added
size/M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/controller/controllers/common.go (1)
324-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicated normalize-with-fallback logic.
The identical
cidr := entry...; if normalized, err := network.NormalizeCIDR(cidr); err == nil { cidr = normalized }pattern is repeated verbatim inclusterNetworksEntriesToArray,serviceNetworksEntriesToArray, andmachineNetworksEntriesToArray. Extract a small helper, e.g.normalizeCIDROrRaw(cidr string) string, and call it from all three sites.♻️ Proposed refactor
+func normalizeCIDROrRaw(cidr string) string { + if normalized, err := network.NormalizeCIDR(cidr); err == nil { + return normalized + } + return cidr +} + func clusterNetworksEntriesToArray(entries []hiveext.ClusterNetworkEntry) []*models.ClusterNetwork { return funk.Map(entries, func(entry hiveext.ClusterNetworkEntry) *models.ClusterNetwork { - cidr := entry.CIDR - if normalized, err := network.NormalizeCIDR(cidr); err == nil { - cidr = normalized - } - return &models.ClusterNetwork{Cidr: models.Subnet(cidr), HostPrefix: int64(entry.HostPrefix)} + return &models.ClusterNetwork{Cidr: models.Subnet(normalizeCIDROrRaw(entry.CIDR)), HostPrefix: int64(entry.HostPrefix)} }).([]*models.ClusterNetwork) }Apply the same pattern to
serviceNetworksEntriesToArrayandmachineNetworksEntriesToArray.🤖 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 `@internal/controller/controllers/common.go` around lines 324 - 364, Extract the repeated CIDR normalize-with-fallback logic into a shared helper, such as normalizeCIDROrRaw, and use it from clusterNetworksEntriesToArray, serviceNetworksEntriesToArray, and machineNetworksEntriesToArray. Keep the helper in the same controller/common area so the three conversion functions can call it directly, and preserve the existing behavior of returning the original CIDR when NormalizeCIDR fails.internal/network/utils_test.go (1)
473-482: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a negative IPv6 test case.
Each new test only checks that non-canonical vs canonical forms of the same IPv6 CIDR are treated as identical. Adding a companion case with two genuinely different IPv6 CIDRs (expecting
false) would guard against a regression wherecidrsEqualsilently treats all IPv6 inputs as equal (e.g., if normalization errors were mishandled).Also applies to: 580-589, 715-724
🤖 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 `@internal/network/utils_test.go` around lines 473 - 482, Add a negative IPv6 comparison test alongside the existing canonicalization cases in utils_test.go to verify cidrsEqual returns false for truly different IPv6 CIDRs, not just equivalent non-canonical/canonical forms. Update the relevant test table entries in the IPv6 test groups near the existing “IPv6 non-canonical vs canonical” cases by adding a pair of distinct CIDRs with expectedResult set to false, so the behavior of cidrsEqual is covered against over-broad normalization.
🤖 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.
Nitpick comments:
In `@internal/controller/controllers/common.go`:
- Around line 324-364: Extract the repeated CIDR normalize-with-fallback logic
into a shared helper, such as normalizeCIDROrRaw, and use it from
clusterNetworksEntriesToArray, serviceNetworksEntriesToArray, and
machineNetworksEntriesToArray. Keep the helper in the same controller/common
area so the three conversion functions can call it directly, and preserve the
existing behavior of returning the original CIDR when NormalizeCIDR fails.
In `@internal/network/utils_test.go`:
- Around line 473-482: Add a negative IPv6 comparison test alongside the
existing canonicalization cases in utils_test.go to verify cidrsEqual returns
false for truly different IPv6 CIDRs, not just equivalent
non-canonical/canonical forms. Update the relevant test table entries in the
IPv6 test groups near the existing “IPv6 non-canonical vs canonical” cases by
adding a pair of distinct CIDRs with expectedResult set to false, so the
behavior of cidrsEqual is covered against over-broad normalization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4874cf4e-fe25-45c3-a979-f0a49bd1cfc5
📒 Files selected for processing (3)
internal/controller/controllers/common.gointernal/network/utils.gointernal/network/utils_test.go
| return funk.Map(entries, func(entry hiveext.ClusterNetworkEntry) *models.ClusterNetwork { | ||
| return &models.ClusterNetwork{Cidr: models.Subnet(entry.CIDR), HostPrefix: int64(entry.HostPrefix)} | ||
| cidr := entry.CIDR | ||
| if normalized, err := network.NormalizeCIDR(cidr); err == nil { |
There was a problem hiding this comment.
Would it be easy to normalize when loading the data instead?
There was a problem hiding this comment.
The data is normalized when it's written to the DB, the problem is before it's written. The goal is to normalize everywhere it is used to align all, similar to PR #10340
There was a problem hiding this comment.
Is because user might enter canonical/non-canonical form and they won't compare between each other?
There was a problem hiding this comment.
Can we then transform them into canonical form as soon as we read them, instead of having normalize functions all over (i.e. when they get loaded into the model)?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10569 +/- ##
=======================================
Coverage 44.39% 44.39%
=======================================
Files 423 423
Lines 73543 73555 +12
=======================================
+ Hits 32649 32657 +8
- Misses 37966 37968 +2
- Partials 2928 2930 +2
🚀 New features to boost your workflow:
|
|
@shay23bra: 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. |
3 participants
Summary
fcff:0069:0001::/64→fcff:69:1::/64), but Go string comparison treated them as different, causing infinite reconciliation loopsChanges
cidrsEqual()helper ininternal/network/utils.go(same pattern as existingipsEqual())AreMachineNetworksIdentical/AreServiceNetworksIdentical/AreClusterNetworksIdenticalto use CIDR-aware comparisoncommon.go):clusterNetworksEntriesToArray,serviceNetworksEntriesToArray,machineNetworksEntriesToArrayList all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit