CNTRLPLANE-3694: validate nodepool upgrade type against known values#8989
CNTRLPLANE-3694: validate nodepool upgrade type against known values#8989hypershift-jira-solve-ci[bot] wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-3694 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 "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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hypershift-jira-solve-ci[bot] 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 |
openshift-ci
Bot
added
area/hypershift-operator
📝 WalkthroughWalkthrough
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8989 +/- ##
==========================================
+ Coverage 43.78% 43.80% +0.02%
==========================================
Files 772 772
Lines 96000 96099 +99
==========================================
+ Hits 42033 42096 +63
- Misses 51055 51084 +29
- Partials 2912 2919 +7
... and 6 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| case hyperv1.UpgradeTypeInPlace: | ||
| return nil | ||
| case hyperv1.UpgradeTypeReplace: | ||
| // validated below |
There was a problem hiding this comment.
Can we not encapsulate that in a function and then add complete behavioral unit tests for it?
There was a problem hiding this comment.
Done. Extracted validateInPlaceManagement and validateReplaceManagement sub-functions and added behavioral unit tests with error message assertions for each validation path.
AI-assisted response via Claude Code
| if nodePool.Spec.Management.UpgradeType == hyperv1.UpgradeTypeInPlace { | ||
| switch nodePool.Spec.Management.UpgradeType { | ||
| case hyperv1.UpgradeTypeInPlace: | ||
| return nil |
There was a problem hiding this comment.
Shouldn't there be some validation here?
There was a problem hiding this comment.
Done. Added validateInPlaceManagement which now rejects conflicting Replace config when upgrade type is InPlace.
AI-assisted response via Claude Code
| case hyperv1.UpgradeTypeReplace: | ||
| // validated below | ||
| default: | ||
| return fmt.Errorf("unsupported upgrade type %q, supported values are %q and %q", |
There was a problem hiding this comment.
The jira ticket acceptance criteria says: If an invalid upgrade type is provided, set an appropriate NodePool condition (e.g., ValidConfiguration or similar) with a clear message indicating the unsupported value
Where is that done?
There was a problem hiding this comment.
The condition is set in conditions.go:221-243 via updateManagementEnabledCondition. When validateManagement returns an error, it sets the UpdateManagementEnabled condition to False with reason ValidationFailed and the error message as the condition message. The error is not returned to the reconciler (only logged) since it's a user input problem that reconciling won't solve.
AI-assisted response via Claude Code
|
/test address-review-comments |
|
Review agent triggered. View job |
hypershift-jira-solve-ci
Bot
force-pushed
the
fix-CNTRLPLANE-3694
branch
from
5c45e6b to
e15a163
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hypershift-operator/controllers/nodepool/nodepool_controller.go (1)
657-676: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire
MaxUnavailableandMaxSurgeforRollingUpdate.
replacedefaults only when the whole field is omitted. An explicitrollingUpdate: {}still passes this check, butcapi.golater readsMaxUnavailableandMaxSurgedirectly. Validate both inner fields here or add schema defaults/required markers.🤖 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 `@hypershift-operator/controllers/nodepool/nodepool_controller.go` around lines 657 - 676, Update validateReplaceManagement to validate both MaxUnavailable and MaxSurge within Replace.RollingUpdate when the strategy is UpgradeStrategyRollingUpdate, rejecting configurations where either field is unset; preserve the existing validation for missing Replace and unsupported strategies.
🤖 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.
Outside diff comments:
In `@hypershift-operator/controllers/nodepool/nodepool_controller.go`:
- Around line 657-676: Update validateReplaceManagement to validate both
MaxUnavailable and MaxSurge within Replace.RollingUpdate when the strategy is
UpgradeStrategyRollingUpdate, rejecting configurations where either field is
unset; preserve the existing validation for missing Replace and unsupported
strategies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 52e64e8e-9de5-4fab-82df-c71861f18b54
⛔ Files ignored due to path filters (1)
cmd/install/assets/crds/hypershift-operator/tests/nodepools.hypershift.openshift.io/stable.nodepools.management.testsuite.yamlis excluded by!cmd/install/assets/**/*.yaml
📒 Files selected for processing (2)
hypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/nodepool_controller_test.go
There was a problem hiding this comment.
Test cases are not using the unit test format, given-when-it. Please review CLAUDE.md for more details.
There was a problem hiding this comment.
Done. Renamed all test cases to use "When , it should " format per the code-formatting convention.
AI-assisted response via Claude Code
| nodePool.Spec.Management.Replace == nil { | ||
| return fmt.Errorf("this is unsupported. %q upgrade type and a strategy: %q or %q are required", | ||
| func validateReplaceManagement(nodePool *hyperv1.NodePool) error { | ||
| if nodePool.Spec.Management.Replace == nil { |
There was a problem hiding this comment.
Is this really the only validation we can do here? Did you consider what else should be checked?
There was a problem hiding this comment.
Done. Added the symmetric cross-field check: validateReplaceManagement now also rejects InPlace config when upgrade type is Replace, mirroring the existing check in validateInPlaceManagement that rejects Replace config with InPlace type. Beyond cross-field consistency, the InPlaceUpgrade struct only has the optional MaxUnavailable field (defaults to 1), and value-level validation for IntOrString is handled at the API layer. The CEL rule on NodePoolManagement (!has(self.inPlace) || self.upgradeType == 'InPlace') covers the API-level guard for InPlace config, so the backend validation here completes the picture by catching the reverse case for Replace config.
AI-assisted response via Claude Code
|
/test address-review-comments |
|
Review agent triggered. View job |
hypershift-jira-solve-ci
Bot
force-pushed
the
fix-CNTRLPLANE-3694
branch
from
e15a163 to
f9cf575
Compare
| nodePool.Spec.Management.Replace == nil { | ||
| return fmt.Errorf("this is unsupported. %q upgrade type and a strategy: %q or %q are required", | ||
| func validateInPlaceManagement(nodePool *hyperv1.NodePool) error { | ||
| if nodePool.Spec.Management.Replace != nil { |
There was a problem hiding this comment.
Is this really the only validation we can do here? Did you consider what else should be checked?
There was a problem hiding this comment.
Done. Added deeper validation in validateReplaceManagement:
- RollingUpdate + nil MaxUnavailable/MaxSurge: if
RollingUpdateis set butMaxUnavailableorMaxSurgeis nil, it now rejects — prevents nil pointers from reaching CAPI (capi.go:482-484). - OnDelete + RollingUpdate conflict: if strategy is
OnDeletebutRollingUpdateconfig is set, it now rejects the conflicting configuration.
Refactored the strategy validation into a switch to cleanly separate RollingUpdate vs OnDelete vs unknown paths.
For validateInPlaceManagement: InPlaceUpgrade has only MaxUnavailable (optional, defaults to 1 in getInPlaceMaxUnavailable at capi.go:1120-1135). The conflict check (no Replace config) is the appropriate backend validation — the downstream consumer handles nil/zero values gracefully.
AI-assisted response via Claude Code
|
/test address-review-comments |
|
Review agent triggered. View job |
hypershift-jira-solve-ci
Bot
force-pushed
the
fix-CNTRLPLANE-3694
branch
from
f9cf575 to
27c274f
Compare
| @@ -635,29 +635,53 @@ func (r *NodePoolReconciler) deleteNodePoolSecrets(ctx context.Context, nodePool | |||
| // validateManagement does additional backend validation. API validation/default should | |||
| // prevent this from ever fail. | |||
| func validateManagement(nodePool *hyperv1.NodePool) error { | |||
There was a problem hiding this comment.
@bryan-cox I don't think we need any new server side validation https://redhat.atlassian.net/browse/CNTRLPLANE-3694.
If anything we should translate this server side validation into CEL and add envtest cases
There was a problem hiding this comment.
Done. Reverted the new server-side validation (removed validateInPlaceManagement and validateReplaceManagement sub-functions) and restored validateManagement to its original behavior. The existing CEL rules already cover the InPlace validation: Enum=Replace;InPlace on UpgradeType, self == oldSelf for immutability, and !has(self.inPlace) || self.upgradeType == 'InPlace' for cross-field validation. Added envtest cases covering these existing CEL rules plus the rollingUpdate/strategy cross-field rule on ReplaceUpgrade.
AI-assisted response via Claude Code
|
/test address-review-comments |
|
Review agent triggered. View job |
Remove the TODO comment from validateManagement (the InPlace upgrade type is already validated by existing CEL rules: Enum on UpgradeType, XValidation on NodePoolManagement for inPlace/upgradeType cross-field check, and immutability on UpgradeType). Rename unit test cases to "When..., it should..." format. Add envtest cases covering the existing CEL rules for UpgradeType enum validation, immutability, inPlace cross-field validation, and rollingUpdate/strategy cross-field validation. Ref: CNTRLPLANE-3694 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
hypershift-jira-solve-ci
Bot
force-pushed
the
fix-CNTRLPLANE-3694
branch
from
27c274f to
07fd764
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hypershift-operator/controllers/nodepool/nodepool_controller_test.go (1)
191-273: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd explicit empty and
InPlacecases.This table exercises invalid and
Replaceconfigurations, but not the emptyUpgradeTypepath or the successfulInPlacebranch invalidateManagement. Add both cases so the new validation contract is protected against regressions.🤖 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 `@hypershift-operator/controllers/nodepool/nodepool_controller_test.go` around lines 191 - 273, Add table-driven cases covering an empty UpgradeType and a valid InPlace upgrade in the validation tests around validateManagement. Configure the empty UpgradeType case with the expected error result, and configure the InPlace case with valid management settings and no Replace configuration so it expects success.
🤖 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.
Outside diff comments:
In `@hypershift-operator/controllers/nodepool/nodepool_controller_test.go`:
- Around line 191-273: Add table-driven cases covering an empty UpgradeType and
a valid InPlace upgrade in the validation tests around validateManagement.
Configure the empty UpgradeType case with the expected error result, and
configure the InPlace case with valid management settings and no Replace
configuration so it expects success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c4cbb96d-e7d4-408a-84ff-c4971f9695b2
⛔ Files ignored due to path filters (1)
cmd/install/assets/crds/hypershift-operator/tests/nodepools.hypershift.openshift.io/stable.nodepools.management.testsuite.yamlis excluded by!cmd/install/assets/**/*.yaml
📒 Files selected for processing (2)
hypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/nodepool_controller_test.go
💤 Files with no reviewable changes (1)
- hypershift-operator/controllers/nodepool/nodepool_controller.go
|
/test address-review-comments |
|
Review agent triggered. View job |
|
@hypershift-jira-solve-ci[bot]: all tests passed! 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
What this PR does / why we need it:
Validates the NodePool
UpgradeTypefield against known values (Replace,InPlace) in thevalidateManagementfunction.Previously, the function skipped all validation when
UpgradeTypewasInPlaceand produced confusing error messages for unknown upgrade types. This replaces the if-chain with a switch statement that explicitly validates the upgrade type, returning a clear error for unsupported values.Additionally normalizes error messages to a consistent format and adds test coverage:
UpgradeTypeUpgradeTypeenum validation and immutability CEL rulesWhich issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-3694
Special notes for your reviewer:
Resolves the TODO that was at the former line 638 of
nodepool_controller.go.Checklist:
Always review AI generated responses prior to use.
Generated with Claude Code via openshift-developer plugin
Summary by CodeRabbit