Updating webhooks for maintenance plan#978
Draft
asergeant01 wants to merge 1 commit into
Draft
Conversation
Contributor
📝 WalkthroughWalkthroughDuplicate-reference validation in BIOSSettings, BIOSVersion, BMCSettings, and BMCVersion webhooks now requires both the reference name and version to match before flagging a duplicate, with early returns when the reference name is unchanged during updates. Test suites are expanded accordingly. A BMCVersion controller error-wrapping change is also included. ChangesDuplicate reference validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Validator
participant K8sAPI
Client->>Validator: ValidateUpdate(oldObj, newObj)
alt reference name unchanged
Validator-->>Client: allow (skip duplicate check)
else reference name changed
Validator->>K8sAPI: List existing objects
K8sAPI-->>Validator: existing records
Validator->>Validator: check name AND version match
Validator-->>Client: allow or deny based on duplicate match
end
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
internal/controller/bmcversion_controller.go (1)
265-267: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueFixes a real bug: previously an unconditional
fmt.Errorfwrap produced a non-nil error even whenresetBMCreturned(false, nil).The prior wrapping unconditionally applied
fmt.Errorf(..., %w, err), so wheneverresetBMClegitimately returned(false, nil)(e.g., waiting for the BMC-reset annotation to clear), the reconciler would still surface a spurious non-nil error. Returningerrdirectly correctly propagatesnilin that case while still surfacing genuine errors (whichresetBMCalready wraps with specific context internally).Minor tradeoff: the outer "failed to reset bmc %s" context (BMC name) is now dropped for genuine error paths. Consider whether that's worth preserving only in the
err != nilbranch.🤖 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/bmcversion_controller.go` around lines 265 - 267, The `resetBMC` call in `bmcversion_controller.go` should not wrap the returned error unconditionally, because that turns the legitimate `(false, nil)` wait state into a spurious non-nil error. Update the reconcile branch that calls `r.resetBMC(...)` so it returns `err` directly when `!ok || err != nil`, preserving nil when `resetBMC` is waiting and still propagating real failures from `resetBMC` with their existing context. Keep the fix localized around `resetBMC` and the reconcile return path in `bmcversion_controller.go`.
🤖 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 `@internal/webhook/v1alpha1/biossettings_webhook.go`:
- Around line 72-77: The fast path in the webhook duplicate check is too broad:
in biossettings_webhook.go, the early return around the ServerRef comparison
skips validation even when only spec.version changes. Update the logic in the
webhook validation path to also compare the version field before returning
early, so the uniqueness check still runs whenever the effective key (serverRef
+ version) may change. Use the existing update-validation flow in the webhook
methods to keep the duplicate check active for version-only updates.
- Around line 109-122: Guard the duplicate-check logic in biossettings webhook
against nil spec.serverRef before dereferencing Name. In the helper that
compares settings and bs (the block returning apierrors.NewInvalid), add an
early skip/continue when either settings.Spec.ServerRef or bs.Spec.ServerRef is
nil, and keep the existing duplicate validation only for objects that actually
set ServerRef. This avoids panics while preserving the current validation flow
in the webhook path.
In `@internal/webhook/v1alpha1/biosversion_webhook.go`:
- Around line 72-77: The early return in the BIOSVersion duplicate-check path is
too broad because it only compares ServerRef.Name and skips validation even when
spec.version changes. Update the webhook logic in the BIOSVersion validation
flow so duplicate detection still runs unless both serverRef and version are
unchanged, using the existing check around serverRef/version comparison in the
webhook code. Make sure the uniqueness check uses the combined serverRef +
version key instead of treating a matching serverRef alone as safe.
In `@internal/webhook/v1alpha1/bmcsettings_webhook.go`:
- Around line 80-85: The fast path in bmc settings validation is too broad: the
duplicate check in the webhook’s update path skips validation whenever BMCRef is
unchanged, but changing spec.version can still introduce a duplicate under the
bmcRef + version uniqueness rule. Update the logic in the webhook handler around
the BMCRef equality guard so it only bypasses the lookup when both BMCRef and
version are unchanged, and otherwise continue through the existing uniqueness
validation.
In `@internal/webhook/v1alpha1/bmcversion_webhook.go`:
- Around line 71-75: The fast-path and duplicate-check logic in the BMCVersion
webhook assumes spec.BMCRef is always set, but it is still optional and can be
nil. Update the validation path in the BMCVersion webhook handler to guard all
accesses to BMCRef.Name before the early return and inside the duplicate scan
loop, and treat a nil BMCRef as its own safe case instead of dereferencing it.
Use the webhook’s existing validation method and the BMCRef field access points
to make the nil handling consistent throughout the scan.
- Around line 71-75: The early return in the webhook duplicate check is too
broad: in BMCVersion webhook validation, `oldObj.Spec.BMCRef.Name ==
newObj.Spec.BMCRef.Name` should not skip validation when `spec.version` changes.
Update the logic in `BMCVersionWebhook` to keep running duplicate detection
unless both `Spec.BMCRef.Name` and `Spec.Version` are unchanged, so uniqueness
is enforced on the `bmcRef + version` pair.
---
Nitpick comments:
In `@internal/controller/bmcversion_controller.go`:
- Around line 265-267: The `resetBMC` call in `bmcversion_controller.go` should
not wrap the returned error unconditionally, because that turns the legitimate
`(false, nil)` wait state into a spurious non-nil error. Update the reconcile
branch that calls `r.resetBMC(...)` so it returns `err` directly when `!ok ||
err != nil`, preserving nil when `resetBMC` is waiting and still propagating
real failures from `resetBMC` with their existing context. Keep the fix
localized around `resetBMC` and the reconcile return path in
`bmcversion_controller.go`.
🪄 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: 92d9707b-c45b-4468-892a-5fe8279571e1
📒 Files selected for processing (9)
internal/controller/bmcversion_controller.gointernal/webhook/v1alpha1/biossettings_webhook.gointernal/webhook/v1alpha1/biossettings_webhook_test.gointernal/webhook/v1alpha1/biosversion_webhook.gointernal/webhook/v1alpha1/biosversion_webhook_test.gointernal/webhook/v1alpha1/bmcsettings_webhook.gointernal/webhook/v1alpha1/bmcsettings_webhook_test.gointernal/webhook/v1alpha1/bmcversion_webhook.gointernal/webhook/v1alpha1/bmcversion_webhook_test.go
asergeant01
force-pushed
the
feat/maintenance-plan-webhooks
branch
2 times, most recently
from
b9e992c to
d4cd6da
Compare
Comment on lines
+1026
to
+1028
| if settings.Status.State == metalv1alpha1.BIOSSettingsStateApplied && len(settingsDiff) == 0 { | ||
| return ctrl.Result{}, nil | ||
| } |
Contributor
There was a problem hiding this comment.
what is the logic for this?? when handleAppliedState function is called, it is already in BIOSSettingsStateApplied state. and we return ctrl.Result{}, nil anyway if settingsDiff ==0
Comment on lines
+73
to
+74
| if oldObj.Spec.ServerRef != nil && newObj.Spec.ServerRef != nil && | ||
| oldObj.Spec.ServerRef.Name == newObj.Spec.ServerRef.Name && |
Contributor
There was a problem hiding this comment.
i think ServerRef is immutable :D so this check is redundant.
this logic basically says that version needs to change everytime i update the spec??? dont seem to be right.
asergeant01
force-pushed
the
feat/maintenance-plan-webhooks
branch
from
d4cd6da to
5e60afc
Compare
Signed-off-by: Alan Sergeant <alan.sergeant@sap.com>
asergeant01
force-pushed
the
feat/maintenance-plan-webhooks
branch
from
5e60afc to
fa1a300
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR updates the validating webhooks for
BIOSSettings,BIOSVersion,BMCSettings, andBMCVersionto support the upcoming metal-maintenance-operator, which introduces aMaintenancePlanRuncontroller that creates these CRs programmatically to drive firmware andconfiguration upgrades across a fleet of servers.
Problem
The existing webhook duplicate-detection logic blocked any two CRs from targeting the same server
or BMC ref, regardless of version. This was too strict for the maintenance plan use case, where the
operator needs to create a new CR for a different target version on the same server — for example,
to upgrade BIOS firmware from
P71toP79. The old logic would reject this as a duplicate.Additionally, the webhooks were needlessly re-running the full duplicate check on every update even
when the
serverRef/bmcReffield hadn't changed, which could never introduce a new duplicate andwas wasted work.
Changes
biossettings_webhook.go,biosversion_webhook.go,bmcsettings_webhook.go,bmcversion_webhook.goDual-condition duplicate detection: a CR is now only rejected as a duplicate when both the
server/BMC ref and the target
versionmatch an existing record. Two CRs targeting the sameserver with different versions are explicitly allowed, enabling the maintenance plan operator to
stage sequential upgrades.
Early return on unchanged ref:
ValidateUpdatenow skips the duplicate check entirely whenthe
serverRef/bmcRefhas not changed between old and new object, since no new duplicate canbe introduced in that case.
bmcversion_controller.goresetBMCcall path (the inner error alreadycontains sufficient context).
Tests
All four webhook test files have been updated:
differs; allow when ref is unchanged but version changes
Context
The
metal-maintenance-operatorMaintenancePlanRuncontroller createsBIOSSettings,BIOSVersion,BMCSettings, andBMCVersionCRs on behalf of aMaintenancePlan. Each stage ina plan targets a specific
version, and multiple stages across a plan run may reference the sameserver/BMC with different versions. Without this change, the webhooks would reject those creates and
block the maintenance pipeline entirely.
Summary by CodeRabbit