MGMT-22553: Add missing fallbacks for network configs in VIP validation #8704
Conversation
WalkthroughUpdates VIP validation to make update-time network fields conditional: existing cluster networks are preserved when update params omit them; VIP validation gains a flag controlling whether MachineNetworks from params are considered. Tests added for partial network updates in dual‑stack scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
Comment |
openshift-ci
bot
added
the
size/M
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @LiorSoffer. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
![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
🧹 Nitpick comments (1)
internal/cluster/validations/validation_test.go (1)
1064-1107: Expand test coverage for partial network updates.The test only covers updating ClusterNetworks in isolation. Consider adding test cases for:
- ServiceNetworks-only updates
- MachineNetworks-only updates
- Updating combinations of network types
- Verifying that non-updated network types retain their original values (not just that validation passes)
This ensures the fallback mechanism works correctly for all network configuration scenarios.
Example additional test cases
It("preserves ClusterNetworks and ServiceNetworks when only updating MachineNetworks", func() { params := &models.V2ClusterUpdateParams{ MachineNetworks: []*models.MachineNetwork{ {Cidr: "192.168.2.0/24"}, {Cidr: "2001:db8:1::/64"}, }, } err := ValidateClusterUpdateVIPAddresses(true, &cluster, params, nil) Expect(err).ShouldNot(HaveOccurred()) }) It("preserves MachineNetworks and ClusterNetworks when only updating ServiceNetworks", func() { params := &models.V2ClusterUpdateParams{ ServiceNetworks: []*models.ServiceNetwork{ {Cidr: "172.31.0.0/16"}, {Cidr: "fd03::/112"}, }, } err := ValidateClusterUpdateVIPAddresses(true, &cluster, params, nil) Expect(err).ShouldNot(HaveOccurred()) })
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
internal/cluster/validations/validation_test.gointernal/cluster/validations/validations.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
internal/cluster/validations/validations.gointernal/cluster/validations/validation_test.go
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8704 +/- ##
==========================================
+ Coverage 43.51% 43.61% +0.10%
==========================================
Files 411 411
Lines 71264 71276 +12
==========================================
+ Hits 31007 31088 +81
+ Misses 37498 37385 -113
- Partials 2759 2803 +44
🚀 New features to boost your workflow:
|
LiorSoffer
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@LiorSoffer: This pull request references MGMT-22553 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 "4.22.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. |
Signed-off-by: Lior Soffer <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LiorSoffer The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/cluster/validations/validations.go (1)
229-230: Minor: Trailing whitespace on line 230.The line has trailing whitespace after the closing parenthesis.
- return validateVIPAddresses(ipV6Supported, targetConfiguration, params.MachineNetworks != nil) + return validateVIPAddresses(ipV6Supported, targetConfiguration, params.MachineNetworks != nil)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
internal/cluster/validations/validation_test.gointernal/cluster/validations/validations.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cluster/validations/validation_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
internal/cluster/validations/validations.go
🧬 Code graph analysis (1)
internal/cluster/validations/validations.go (1)
models/cluster.go (1)
Cluster(24-284)
🔇 Additional comments (4)
internal/cluster/validations/validations.go (4)
314-332: Sound approach for preserving existing network configurations on partial updates.The pattern of initializing from cluster values and only overriding when
len(params.X) > 0correctly handles both nil (omitted) and empty slice cases, ensuring partial updates (e.g., updating onlyClusterNetworks) don't unintentionally clear other network fields. This addresses the concern from the previous review about empty slice handling for network params.
349-353: Good documentation of the validation deferral rationale.The comment clearly explains why VIP-in-network validation is conditional: in saas mode, MachineNetworks may be auto-calculated from VIPs later, with the actual validation occurring in
updateNonDhcpNetworkParams.
563-566: Correct conditional gating for VIP-in-network validation.The dual condition (
len > 0 && machineNetworksInParams) correctly ensures validation only runs when MachineNetworks are both explicitly provided and non-empty, allowing auto-calculation paths to defer this check.
582-583: Clear documentation for dual-stack constraint.Good clarification that dual-stack clusters cannot rely on auto-calculated MachineNetworks and require explicit configuration.
|
@LiorSoffer: The following tests 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
https://issues.redhat.com/browse/MGMT-22553