Add BMCUserSet type and controller implementation#662
Add BMCUserSet type and controller implementation#662stefanhipfel wants to merge 8 commits intomainfrom
BMCUserSet type and controller implementation#662Conversation
github-actions
bot
added
the
documentation
WalkthroughAdds a new cluster-scoped BMCUserSet CRD, API types, deepcopy, controller, RBAC, tests, docs, and registers the controller; refactors BMCUserSpec to embed BMCUserTemplate and updates related tests and manager wiring. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Manager as Kubernetes Manager
end
rect rgba(200,255,200,0.5)
participant Reconciler as BMCUserSetReconciler
participant API as Kubernetes API
participant BMCs as BMC resources
participant BMCUsers as BMCUser resources
end
Manager->>Reconciler: Reconcile(BMCUserSet)
Reconciler->>API: Get BMCUserSet
Reconciler->>Reconciler: Check deletion / finalizer
alt Deletion in progress
Reconciler->>API: List owned BMCUsers
Reconciler->>API: Delete owned BMCUsers
Reconciler->>API: Patch BMCUserSet (remove finalizer)
else Normal reconcile
Reconciler->>API: List BMCs matching selector
Reconciler->>API: List owned BMCUsers
Reconciler->>API: Create missing BMCUsers (from template)
Reconciler->>API: Patch existing BMCUsers from template
Reconciler->>API: Delete BMCUsers for non-matching BMCs
Reconciler->>API: Update BMCUserSet status (counts)
end
Reconciler->>Manager: Return ctrl.Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@api/v1alpha1/bmcuserset_types.go`:
- Around line 35-37: The BMCUserTemplate field has a conflicting `+required`
marker and an `omitempty` JSON tag; update the struct field `BMCUserTemplate
BMCUserTemplate` to remove `omitempty` from its JSON tag (i.e., use
`json:"bmcUserTemplate"`) so the required semantics are respected, and then
ensure you add corresponding OpenAPI validation or webhook defaulting/validation
for `BMCUserTemplate` (similar to how `BMCSelector` is defined) so the API
enforces the field presence.
In `@internal/controller/bmcuserset_controller.go`:
- Around line 342-347: The loop over bmcUserSetList.Items currently returns nil
on LabelSelectorAsSelector parse errors, which aborts processing of remaining
BMCUserSets; in the for loop that iterates bmcUserSet := range
bmcUserSetList.Items (inside the reconcile/controller logic in
bmcuserset_controller), replace the `return nil` after `log.Error(err, "Failed
to parse BMCSelector", "BMCUserSet", bmcUserSet.Name)` with `continue` so the
error is logged for that BMCUserSet but the loop continues to enqueue/process
the rest.
- Around line 355-370: The loop over BMCUserSets short-circuits on error from
r.getOwnedBMCUsers by calling return nil which stops processing other sets;
change the error handling in the block where getOwnedBMCUsers is called
(function getOwnedBMCUsers, variable ownedBMCUsers, loop over bmcUserSet) to log
the error and continue processing the next bmcUserSet (i.e., replace return nil
with continue) so other BMCUserSets can still be enqueued; ensure reqs remains
in scope and behavior unchanged apart from skipping this failed set.
- Around line 305-316: The CreateOrPatch mutator for existing BMCUser objects is
missing the explicit clear of BMCSecretRef, so when
bmcUserSet.Spec.BMCUserTemplate.BMCSecretRef is removed it is not propagated;
inside the controllerutil.CreateOrPatch closure that updates bmcUser (the block
that sets UserName, RoleID, Description, RotationPeriod, etc.) explicitly set
bmcUser.Spec.BMCSecretRef = nil before the conditional that assigns it,
mirroring the pattern used for RotationPeriod and createMissingBMCUsers, so that
a nil template ref clears the existing BMCUser's BMCSecretRef.
- Around line 86-103: The current flow uses the initially fetched ownedBMCUsers
for subsequent operations but createMissingBMCUsers and deleteOrphanedBMCUsers
mutate cluster state, so re-fetch the owned BMCUsers list after those
create/delete calls and before calling patchBMCUsersFromTemplate and before
computing currentStatus; specifically, after createMissingBMCUsers and
deleteOrphanedBMCUsers complete, call the same retrieval logic you used at line
81 to refresh ownedBMCUsers, then pass the refreshed slice into
patchBMCUsersFromTemplate and use it when calling
getOwnedBMCUserSetStatus/updateStatus to ensure you neither patch deleted items
nor compute a stale status.
🧹 Nitpick comments (6)
api/v1alpha1/bmcuserset_types.go (1)
12-18: Consider adding+requiredmarkers toUserNameandRoleID.These fields are non-optional (no
omitempty, non-pointer), but without+requiredkubebuilder markers the generated CRD schema won't enforce them at admission time. A user could create a BMCUserSet with emptyuserName/roleID, potentially creating invalid BMCUser resources downstream.Proposed fix
type BMCUserTemplate struct { // Username of the BMC user. + // +required UserName string `json:"userName"` // RoleID is the ID of the role to assign to the user. // The available roles depend on the BMC implementation. // For Redfish, common role IDs are "Administrator", "Operator", "ReadOnly". + // +required RoleID string `json:"roleID"`As per coding guidelines: "Implement defaulting and validation via webhooks (preferred) or OpenAPI schema annotations".
config/crd/bases/metal.ironcore.dev_bmcusersets.yaml (1)
17-29: Printer column name "TotalBMCs" may be misleading forfullyLabeledBMCsfield.The column at Line 25 is named
TotalBMCsbut maps to.status.fullyLabeledBMCs. "TotalBMCs" implies all BMCs, whilefullyLabeledBMCssuggests only those matching the label selector. Consider renaming the printer column toFullyLabeledBMCsorMatchedBMCs(in the kubebuilder markers on the Go types) for clarity. Since this CRD is auto-generated, the fix should be applied to the+kubebuilder:printcolumnmarker inapi/v1alpha1/bmcuserset_types.go.internal/controller/bmcuserset_controller.go (2)
108-146:errors.Join(errs...)on line 145 is alwaysnilat that point.If
len(ownedBMCUsers.Items) > 0is true, the function returns at line 136. The code at line 145 is only reached when no owned BMCUsers exist, meaning the deletion loop (lines 124-128) never executes anderrsis empty. This is harmless but slightly confusing.Simplify
log.V(1).Info("BMCUserSet is deleted") - return ctrl.Result{}, errors.Join(errs...) + return ctrl.Result{}, nil
198-258: Consider extracting the template-to-spec mapping into a shared helper.The logic that maps
BMCUserTemplatefields toBMCUser.Specis duplicated betweencreateMissingBMCUsers(lines 238-248) andpatchBMCUsersFromTemplate(lines 306-315). A small helper (e.g.applyTemplateToSpec) would reduce duplication and prevent future drift — like theBMCSecretRefasymmetry already present.internal/controller/bmcuserset_controller_test.go (2)
129-141: Assertion doesn't verify fields belong to the same item.The two
ContainElementsmatchers on lines 131-134 and 135-140 are independent. The first checks that some items reference the right BMCs; the second checks that some items have the right spec fields — but they don't verify that the same item has both the correctBMCRefand the correctUserName/RoleID/etc. This works only because both BMCUsers share identical template values. Consider using a singleContainElementswith composite matchers for stronger per-item assertions.
149-153: Manual deletion is redundant withAfterEach/EnsureCleanState().Since
EnsureCleanState()is called inAfterEach, these explicit deletes are unnecessary. They also risk masking cleanup failures (e.g., if the controller's finalizer logic has a bug, the test still passes because the explicit delete kicks in beforeAfterEach).
afritzler
changed the title
BMCUserSet type and controller implementation
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/controller/bmcuserset_controller.go`:
- Around line 253-256: The code currently logs "Created BMCUser" unconditionally
even when creation failed (err != nil); update the block around the call that
appends to errs and the subsequent log so the success message is only emitted
when err == nil — i.e., after appending to errs if err != nil do not call
log.V(1).Info, otherwise in the else branch log the creation using
newBMCUser.Name, bmc.Name and opResult (or check err==nil before the log call)
to ensure the message accurately reflects success.
- Around line 330-339: syncBMCUserAnnotationsFromSet currently copies every
annotation from a BMCUserSet onto each BMCUser (additive-only), which can
propagate system annotations and critical annotations like OperationAnnotation;
change it to only synchronize a safe, explicit set or prefix of annotations and
make it mirror the set (remove annotations on the BMCUser that were removed from
the BMCUserSet). Concretely: in syncBMCUserAnnotationsFromSet, replace the blind
copy with logic that (1) defines an allowlist (e.g., allowedAnnotations map or
allowedAnnotationPrefix string) and only applies keys that match that
list/prefix, (2) removes any allowed-namespace keys present on the BMCUser that
are not present on the BMCUserSet, and (3) ensure you never copy
OperationAnnotation (reference the OperationAnnotation symbol used by
BMCUserReconciler) or kubectl/system annotation prefixes. Update the function to
compute the resulting annotations map and call bmcUser.SetAnnotations(result) so
BMCUsers mirror the filtered annotations of the BMCUserSet.
🧹 Nitpick comments (2)
internal/controller/bmcuserset_controller.go (1)
245-248: Shared pointer: template'sBMCSecretRefis assigned directly.Line 247 assigns
bmcUserSet.Spec.BMCUserTemplate.BMCSecretRefdirectly tonewBMCUser.Spec.BMCSecretRef, sharing the pointer. InpatchBMCUsersFromTemplate(line 315), a newLocalObjectReferenceis allocated instead. Be consistent — always copy to avoid aliasing surprises.Proposed fix
newBMCUser.Spec.BMCSecretRef = nil if bmcUserSet.Spec.BMCUserTemplate.BMCSecretRef != nil { - newBMCUser.Spec.BMCSecretRef = bmcUserSet.Spec.BMCUserTemplate.BMCSecretRef + newBMCUser.Spec.BMCSecretRef = &corev1.LocalObjectReference{Name: bmcUserSet.Spec.BMCUserTemplate.BMCSecretRef.Name} }internal/controller/bmcuserset_controller_test.go (1)
169-206: Manual cleanup is fragile and likely redundant withAfterEach/EnsureCleanState.If an assertion fails before line 169, this cleanup code is skipped entirely (Ginkgo aborts the
Itblock). TheAfterEachat line 27 already callsEnsureCleanState(). If that helper covers these resource types, this block is unnecessary; if it doesn't, the cleanup should be inAfterEach/DeferCleanupinstead.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
api/v1alpha1/bmcuserset_types.go (1)
12-18: Consider adding+requiredmarkers toUserNameandRoleIDfor explicit API documentation.These fields are effectively required (non-pointer, no
omitempty), so the generated CRD schema will enforce them. Adding// +requiredmarkers above each aligns with the pattern used forBMCSelectorinBMCUserSetSpecand makes the intent explicit in the source.✨ Proposed improvement
type BMCUserTemplate struct { // Username of the BMC user. + // +required UserName string `json:"userName"` // RoleID is the ID of the role to assign to the user. // The available roles depend on the BMC implementation. // For Redfish, common role IDs are "Administrator", "Operator", "ReadOnly". + // +required RoleID string `json:"roleID"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1alpha1/bmcuserset_types.go` around lines 12 - 18, The BMCUserTemplate struct's UserName and RoleID fields are effectively required but lack explicit markers; add a comment marker "// +required" immediately above the UserName and RoleID fields in the BMCUserTemplate type so the generated CRD schema and docs explicitly show these as required (mirror the pattern used for BMCSelector in BMCUserSetSpec); update the BMCUserTemplate definition to include these two "+required" markers above the UserName and RoleID field declarations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/bmcuserset_controller.go`:
- Around line 130-134: The delete loop in the reconcile (iterating
ownedBMCUsers.Items) appends NotFound errors into errs; change the Delete call
to ignore NotFound using client.IgnoreNotFound so transient external deletes
don't force an extra requeue—i.e., replace the direct r.Delete(ctx,
&ownedBMCUsers.Items[i]) error handling with
client.IgnoreNotFound(r.Delete(...)) and only append non-ignored errors to errs
(referencing ownedBMCUsers, r.Delete, and errs).
- Around line 279-294: The "Deleted orphaned BMCUser" message is logged
unconditionally even when r.Delete fails; update the loop that iterates
bmcUserList.Items (using bmcUser and bmcMap) so that you only call
log.V(1).Info("Deleted orphaned BMCUser", "BMCUser", bmcUser.Name) when
r.Delete(ctx, bmcUser) returns nil (i.e., move the log into the success branch),
continue appending errors to errs on failure, and keep returning
errors.Join(errs...) as before.
- Around line 338-355: The prefix checks in syncBMCUserAnnotationsFromSet are
wrong and allow k8s.io and kubectl.kubernetes.io keys through; replace the
manual slice checks with strings.HasPrefix and add the "strings" import,
explicitly filtering keys that start with "kubernetes.io/", "k8s.io/" or
"kubectl.kubernetes.io/"; also stop being additive-only by computing the desired
set of filtered annotations from bmcUserSet and removing any non-system
annotations on the bmcUser that are no longer present in the set (while still
preserving operator/internal keys such as metalv1alpha1.OperationAnnotation and
other system annotations), then call bmcUser.SetAnnotations with the merged
result.
---
Nitpick comments:
In `@api/v1alpha1/bmcuserset_types.go`:
- Around line 12-18: The BMCUserTemplate struct's UserName and RoleID fields are
effectively required but lack explicit markers; add a comment marker "//
+required" immediately above the UserName and RoleID fields in the
BMCUserTemplate type so the generated CRD schema and docs explicitly show these
as required (mirror the pattern used for BMCSelector in BMCUserSetSpec); update
the BMCUserTemplate definition to include these two "+required" markers above
the UserName and RoleID field declarations.
Addresses all 6 CodeRabbit review issues from PR #662: Critical fixes: - Fix annotation filter logic bug: Replace manual slice checks with strings.HasPrefix() for kubernetes.io/, k8s.io/, and kubectl.kubernetes.io/ domains. Adds proper bounds checking. - Implement bidirectional annotation sync: Refactor syncBMCUserAnnotationsFromSet() to remove annotations from BMCUser objects when deleted from BMCUserSet, preventing annotation drift. Medium severity fixes: - Fix unconditional success logging: Move "Deleted orphaned BMCUser" log inside success branch in deleteOrphanedBMCUsers(). - Add NotFound guards: Wrap delete calls with client.IgnoreNotFound() in both delete() and deleteOrphanedBMCUsers() to prevent unnecessary requeue cycles. Low priority fixes: - Add +required markers: Document UserName and RoleID as required fields in BMCUserTemplate struct. Test coverage: - Add test for annotation removal when deleted from BMCUserSet - Add test for system annotation filtering (kubernetes.io/, k8s.io/, kubectl.kubernetes.io/) All tests passing (90/90 specs), linter clean, build successful.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/controller/bmcuserset_controller.go (1)
227-260: Use deterministic naming instead of GenerateName for oversized BMCUser names.The code currently uses
GenerateNamewith an emptyNamefor oversized name combinations. While idempotency is maintained through ownership tracking (viagetOwnedBMCUsersandSetControllerReference), a truncated+hash deterministic name would be simpler and avoid relying on the keyed lookup edge case. This aligns with the idempotent reconciliation guideline: explicit, deterministic resource naming makes the reconciliation logic more obviously safe to run repeatedly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/bmcuserset_controller.go` around lines 227 - 260, The current branch that uses GenerateName for oversized newBMCUserName should be replaced with deterministic truncated+hash naming: compute a stable short hash (e.g., sha256/sha1 of newBMCUserName) and build ObjectMeta.Name = prefix + "-" + hashSuffix such that its length <= utilvalidation.DNS1123SubdomainMaxLength; use that Name when constructing the metalv1alpha1.BMCUser instead of GenerateName so controllerutil.CreateOrPatch/CreateOrPatch and controllerutil.SetControllerReference remain idempotent. Ensure you still call syncBMCUserAnnotationsFromSet(newBMCUser, bmcUserSet) and set BMCRef/BMCSecretRef/RotationPeriod as before; keep references to newBMCUserName, utilvalidation.DNS1123SubdomainMaxLength, controllerutil.CreateOrPatch, and SetControllerReference to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1alpha1/bmcuserset_types.go`:
- Around line 13-20: The UserName and RoleID struct fields (UserName, RoleID)
are marked +required but allow empty strings; update their validation tags to
reject empty values by adding kubebuilder JSON-schema validation (e.g.
+kubebuilder:validation:MinLength=1) to both fields so the CRD disallows "" at
admission time; modify the struct field tags for UserName and RoleID to include
the MinLength annotation (or equivalent webhook validation) and regenerate CRD
manifests.
- Around line 60-65: The BMCUserSet root Spec must be required: update the
BMCUserSet type by removing the omitempty from the Spec json tag (change
`json:"spec,omitempty"` to `json:"spec"`) and add the kubebuilder required
marker above the Spec field (use `+kubebuilder:validation:Required`) so the CRD
schema will reject objects missing Spec; reference the BMCUserSet struct and the
Spec field and ensure the BMCUserSetSpec type remains unchanged.
In `@internal/controller/bmcuserset_controller.go`:
- Around line 125-145: The code checks len(ownedBMCUsers.Items) using the
pre-delete list, which can wrongly keep the finalizer; after the deletion loop
in the reconcile logic, call r.getOwnedBMCUsers(ctx, bmcUserSet) again to
re-fetch current ownedBMCUsers, use that fresh list when computing currentStatus
via r.getOwnedBMCUserSetStatus and when deciding whether to return
errors.Join(errs...) or clear the finalizer (i.e., base the wait/return on the
re-fetched slice rather than the original ownedBMCUsers variable), and handle
any error from the re-list consistently (propagate or log) before updating
status with r.updateStatus.
---
Nitpick comments:
In `@internal/controller/bmcuserset_controller.go`:
- Around line 227-260: The current branch that uses GenerateName for oversized
newBMCUserName should be replaced with deterministic truncated+hash naming:
compute a stable short hash (e.g., sha256/sha1 of newBMCUserName) and build
ObjectMeta.Name = prefix + "-" + hashSuffix such that its length <=
utilvalidation.DNS1123SubdomainMaxLength; use that Name when constructing the
metalv1alpha1.BMCUser instead of GenerateName so
controllerutil.CreateOrPatch/CreateOrPatch and
controllerutil.SetControllerReference remain idempotent. Ensure you still call
syncBMCUserAnnotationsFromSet(newBMCUser, bmcUserSet) and set
BMCRef/BMCSecretRef/RotationPeriod as before; keep references to newBMCUserName,
utilvalidation.DNS1123SubdomainMaxLength, controllerutil.CreateOrPatch, and
SetControllerReference to locate the change.
🪄 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: d558be19-8595-41a1-9dc5-8a7b6b9a144b
📒 Files selected for processing (3)
api/v1alpha1/bmcuserset_types.gointernal/controller/bmcuserset_controller.gointernal/controller/bmcuserset_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/bmcuserset_controller_test.go
| // Username of the BMC user. | ||
| // +required | ||
| UserName string `json:"userName"` | ||
| // RoleID is the ID of the role to assign to the user. | ||
| // The available roles depend on the BMC implementation. | ||
| // For Redfish, common role IDs are "Administrator", "Operator", "ReadOnly". | ||
| // +required | ||
| RoleID string `json:"roleID"` |
There was a problem hiding this comment.
+required still allows empty userName and roleID.
These fields must be present, but "" still passes admission and then gets fanned out to every generated BMCUser. Add MinLength=1 (or equivalent webhook validation) so unusable templates are rejected up front.
Suggested fix
// Username of the BMC user.
// +required
+ // +kubebuilder:validation:MinLength=1
UserName string `json:"userName"`
@@
// +required
+ // +kubebuilder:validation:MinLength=1
RoleID string `json:"roleID"`Based on learnings: "When a Kubernetes controller (reconciler) errors out and does not proceed if a CRD spec field is nil/empty, that field should not be treated as optional in the API schema."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v1alpha1/bmcuserset_types.go` around lines 13 - 20, The UserName and
RoleID struct fields (UserName, RoleID) are marked +required but allow empty
strings; update their validation tags to reject empty values by adding
kubebuilder JSON-schema validation (e.g. +kubebuilder:validation:MinLength=1) to
both fields so the CRD disallows "" at admission time; modify the struct field
tags for UserName and RoleID to include the MinLength annotation (or equivalent
webhook validation) and regenerate CRD manifests.
| type BMCUserSet struct { | ||
| metav1.TypeMeta `json:",inline"` | ||
| metav1.ObjectMeta `json:"metadata,omitempty"` | ||
|
|
||
| Spec BMCUserSetSpec `json:"spec,omitempty"` | ||
| Status BMCUserSetStatus `json:"status,omitempty"` |
There was a problem hiding this comment.
Make the root spec field required.
json:"spec,omitempty" lets a BMCUserSet through with no spec at all, which bypasses the nested required fields. This controller does not have a safe zero-value behavior for that case, so admission should reject it instead.
Suggested fix
- Spec BMCUserSetSpec `json:"spec,omitempty"`
+ // +required
+ Spec BMCUserSetSpec `json:"spec"`
Status BMCUserSetStatus `json:"status,omitempty"`Based on learnings: "When a Kubernetes controller (reconciler) errors out and does not proceed if a CRD spec field is nil/empty, that field should not be treated as optional in the API schema."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v1alpha1/bmcuserset_types.go` around lines 60 - 65, The BMCUserSet root
Spec must be required: update the BMCUserSet type by removing the omitempty from
the Spec json tag (change `json:"spec,omitempty"` to `json:"spec"`) and add the
kubebuilder required marker above the Spec field (use
`+kubebuilder:validation:Required`) so the CRD schema will reject objects
missing Spec; reference the BMCUserSet struct and the Spec field and ensure the
BMCUserSetSpec type remains unchanged.
| ownedBMCUsers, err := r.getOwnedBMCUsers(ctx, bmcUserSet) | ||
| if err != nil { | ||
| log.Error(err, "Failed to list owned BMCUsers") | ||
| return ctrl.Result{}, fmt.Errorf("failed to get owned BMCUsers: %w", err) | ||
| } | ||
|
|
||
| var errs []error | ||
| for i := range ownedBMCUsers.Items { | ||
| if err := client.IgnoreNotFound(r.Delete(ctx, &ownedBMCUsers.Items[i])); err != nil { | ||
| errs = append(errs, err) | ||
| } | ||
| } | ||
|
|
||
| if len(ownedBMCUsers.Items) > 0 { | ||
| currentStatus := r.getOwnedBMCUserSetStatus(ownedBMCUsers) | ||
| if err := r.updateStatus(ctx, log, currentStatus, bmcUserSet); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to update current BMCUserSet status: %w", err) | ||
| } | ||
| log.V(1).Info("Waiting on the created BMCUsers to be deleted") | ||
| return ctrl.Result{}, errors.Join(errs...) | ||
| } |
There was a problem hiding this comment.
Re-list owned BMCUsers before deciding to keep the finalizer.
The len(ownedBMCUsers.Items) > 0 check runs against the slice captured before Lines 132-136 delete children. If one of those deletes succeeds or races into NotFound, this branch can still wait on BMCUsers that are already gone and keep the finalizer longer than necessary. Re-fetch after the loop and use the fresh list for both status and the finalizer decision.
Suggested fix
var errs []error
for i := range ownedBMCUsers.Items {
if err := client.IgnoreNotFound(r.Delete(ctx, &ownedBMCUsers.Items[i])); err != nil {
errs = append(errs, err)
}
}
+
+ ownedBMCUsers, err = r.getOwnedBMCUsers(ctx, bmcUserSet)
+ if err != nil {
+ return ctrl.Result{}, fmt.Errorf("failed to re-list owned BMCUsers: %w", err)
+ }
if len(ownedBMCUsers.Items) > 0 {As per coding guidelines: "Implement idempotent reconciliation logic - safe to run the same reconciliation multiple times without side effects"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controller/bmcuserset_controller.go` around lines 125 - 145, The
code checks len(ownedBMCUsers.Items) using the pre-delete list, which can
wrongly keep the finalizer; after the deletion loop in the reconcile logic, call
r.getOwnedBMCUsers(ctx, bmcUserSet) again to re-fetch current ownedBMCUsers, use
that fresh list when computing currentStatus via r.getOwnedBMCUserSetStatus and
when deciding whether to return errors.Join(errs...) or clear the finalizer
(i.e., base the wait/return on the re-fetched slice rather than the original
ownedBMCUsers variable), and handle any error from the re-list consistently
(propagate or log) before updating status with r.updateStatus.
3 participants
Proposed Changes
adds new BMCUserSet Controller
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Documentation