Conversation
github-actions
Bot
added
the
documentation
| log.V(1).Info("Deleting User", "User", user.Name) | ||
| if user.Status.EffectiveBMCSecretRef != nil { | ||
| log.V(1).Info("Removing effective BMCSecret reference from User", "User", user.Name) | ||
| if err := r.removeEffectiveSecret(ctx, log, user); err != nil { |
There was a problem hiding this comment.
why is this necessary? wont it get deleted as the resource creating it is anyway deleted?
There was a problem hiding this comment.
if we want to delete it, we should delete the BMC Secret ref from Spec which we created, along with the Ref in status
There was a problem hiding this comment.
I forgot to comment out the delete function call, since we did not decide if the controller should also remove users on CRD deletion.
It kind of makes sense to do it, but what if that user was the metal-operators admin user?! maybe an immutable flag is needed for those kind of users
Nuckal777
left a comment
Nuckal777
left a comment
There was a problem hiding this comment.
Thanks, PR still needs some cleanup though.
| } | ||
|
|
||
| // Vendor-specific constraints map. | ||
| var manufacturerPasswordConfigs = map[Manufacturer]ManufacturerPasswordConfig{ |
There was a problem hiding this comment.
for future improvement. this needs to be changeable (in config map, or a user input spec) as this changes based on some settings and preference.
There was a problem hiding this comment.
creating an issue for that
There was a problem hiding this comment.
improvement for next PR: we need to move these to our mock server setup.
|
In order to move this PR forward, can you please rebase this PR @stefanhipfel? |
WalkthroughAdds a cluster-scoped BMCUser CRD, its controller and tests, account-management APIs in BMC implementations (Redfish and local/mock), a BMCSecret validating webhook, CRD/RBAC/samples, docs, and supporting mock data for credential lifecycle and rotation. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Request
participant K8s as Kubernetes API
participant Ctrl as BMCUserReconciler
participant BMC as BMC (Redfish/Local)
participant Secret as BMCSecret
User->>K8s: Create BMCUser
K8s->>Ctrl: Reconcile(BMCUser)
Ctrl->>K8s: Get BMCUser, referenced BMC and Secret
Ctrl->>Ctrl: Ensure finalizer, decide create/rotate credentials
Ctrl->>BMC: CreateOrUpdateAccount(userName, role, password)
BMC-->>Ctrl: Account result (ID, expiration)
Ctrl->>K8s: Create immutable credentials Secret
K8s-->>Ctrl: Secret created
Ctrl->>K8s: Patch BMCUser status (ID, lastRotation, passwordExpiration)
K8s-->>Ctrl: Status updated
sequenceDiagram
participant User as User
participant K8s as Kubernetes API
participant Webhook as Admission Webhook
participant Val as BMCSecretCustomValidator
User->>K8s: Update BMCSecret (Immutable: true)
K8s->>Webhook: ValidateUpdate
Webhook->>Val: ValidateUpdate(oldObj, newObj)
Val->>Val: Compare Data/StringData (DeepEqual)
alt Data changed and Immutable == true
Val-->>Webhook: Return error (deny)
Webhook-->>K8s: Reject update
else
Val-->>Webhook: Allow update
Webhook-->>K8s: Accept update
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labelsenhancement 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bmc/redfish_local.go (1)
30-53: Avoid leaking the underlying Redfish client on auth failure; also guard against nilAccounts.
IfNewRedfishBMCClientsucceeds and the credentials don’t match, you return 401 but never log out/close the created client. Also,UnitTestMockUps.Accountsbeing nil will panic on map access.Possible fix
bmc, err := NewRedfishBMCClient(ctx, options) if err != nil { return nil, err } +if UnitTestMockUps.Accounts == nil { + UnitTestMockUps.Accounts = map[string]*redfish.ManagerAccount{} +} if acc, ok := UnitTestMockUps.Accounts[options.Username]; ok { if acc.Password == options.Password { // authenticated return &RedfishLocalBMC{RedfishBMC: bmc}, nil } } +// avoid leaking a successfully created client +bmc.Logout() return nil, &gofishCommon.Error{ HTTPReturnedStatusCode: 401, }
🤖 Fix all issues with AI agents
In @api/v1alpha1/bmcuser_types.go:
- Around line 11-24: Add kubebuilder validation markers and omitempty json tags
on BMCUserSpec: add +kubebuilder:validation:Pattern and/or
+kubebuilder:validation:MinLength/MaxLength above UserName; add either
+kubebuilder:validation:Enum (or +kubebuilder:validation:Pattern) above RoleID
to restrict valid role IDs; add a minimum constraint above RotationPeriod (e.g.,
+kubebuilder:validation:Minimum=<seconds> or an appropriate duration validation)
to prevent zero/negative durations; and add `omitempty` to the json tags for
Enabled and BMCControllerUser so false values are not always serialized. Apply
these markers to the BMCUserSpec struct fields UserName, RoleID, RotationPeriod,
Enabled, and BMCControllerUser.
- Around line 26-32: The PasswordExpiration field on BMCUserStatus is currently
a string which prevents type-safe time handling; change its type from string to
*metav1.Time (update the struct definition of BMCUserStatus and any
constructors/serializers/deserializers that reference PasswordExpiration),
update imports to ensure metav1 is available, and update controller/comparison
logic that reads/writes PasswordExpiration to use metav1.Time operations; if you
must keep string for vendor reasons, add a clear comment on
BMCUserStatus.PasswordExpiration explaining the format constraints and add
parsing/validation functions instead.
In @bmc/redfish_local.go:
- Around line 55-62: UnitTestMockUps.Accounts access is not concurrency-safe and
some ops are inefficient/inconsistent: protect reads/writes with a sync.RWMutex
on UnitTestMockUps (use RLock/RUnlock for reads in GetAccounts and RLock when
checking in CreateOrUpdateAccount, Lock/Unlock for mutations), change
CreateOrUpdateAccount to look up the account directly using if a, ok :=
UnitTestMockUps.Accounts[userName] instead of scanning all values, and make
DeleteAccount either honor the passed id (validate that id matches the account
being deleted) or explicitly document/validate that id is ignored in local mock
mode and delete by userName under the same mutex protection.
In @bmc/redfish.go:
- Around line 797-803: The GetAccountService method on RedfishBMC should return
nil when AccountService() fails to follow Go conventions; update the error
branch in RedfishBMC.GetAccountService so that when
r.client.GetService().AccountService() returns an error you return (nil,
fmt.Errorf("failed to get account service: %w", err)) instead of returning the
potentially non-nil service value.
- Around line 740-773: In CreateOrUpdateAccount, fix the incorrect error message
and credential usage: update the fmt.Errorf message returned after
service.CreateAccount(...) to say "failed to create account" instead of "failed
to update account", and confirm ChangePassword is called with the current/old
password for authorization (ensure using the existing session/admin credential
rather than r.options.Password if that is not the current password) in the
CreateOrUpdateAccount function where ChangePassword(user password,
currentPassword) is invoked.
In @config/samples/metal_v1alpha1_bmcuser.yaml:
- Around line 13-16: The bmcSecretRef in this sample uses the wrong resource
name; change the value of bmcSecretRef.name from "bmc-secret-sample" to the
actual resource name "bmcsecret-sample" so it matches the BMCSecret resource
(referenced symbol: bmcSecretRef.name -> bmcsecret-sample) and the sample can be
applied successfully.
In @docs/api-reference/api.md:
- Around line 597-656: The API docs for BMCUser/BMCUserSpec/BMCUserStatus lack
field descriptions; update the tables in the BMCUser section to add clear,
user-facing descriptions for each field: for BMCUserSpec.userName describe
expected format, allowed characters, length limits and uniqueness; roleID list
valid roles and examples (e.g., Administrator, Operator, ReadOnly); description
explain purpose; rotationPeriod explain rotation behavior, valid duration
format/ranges and default policy; bmcSecretRef state when to supply vs
auto-generate, secret key/format requirements; bmcRef indicate required/optional
and operator behavior when unset; enabled explain effect of toggling the user.
For BMCUserStatus, document effectiveBMCSecretRef and how it differs from
spec.bmcSecretRef, lastRotation semantics (timestamp format, UTC),
passwordExpiration exact format and timezone handling (or change to Time if
appropriate), and id format/uniqueness; include example values for userName and
roleID to aid users. Ensure these descriptions appear next to the corresponding
table rows for BMCUserSpec and BMCUserStatus in the existing docs block.
In @internal/controller/bmcuser_controller_test.go:
- Around line 120-135: The test is deleting the effectiveSecret explicitly and
using broad ShouldNot(Succeed()) checks which are flaky because the secret may
be garbage-collected; remove the explicit call to k8sClient.Delete(ctx,
effectiveSecret) and instead assert deletion by polling the API: use
Eventually(Get(effectiveSecret)).Should(Satisfy(apierrors.IsNotFound)); likewise
replace any Eventually(Get(...)).ShouldNot(Succeed()) occurrences (e.g. for user
or other resources) with
Eventually(Get(resource)).Should(Satisfy(apierrors.IsNotFound)) so ownership/GC
races are handled consistently (apply same change to the other affected block at
lines 284-328).
In @internal/controller/bmcuser_controller.go:
- Around line 216-227: handleUpdatedSecretRef currently updates the BMC account
but does not update user.Status.EffectiveBMCSecretRef, which causes repeated
reconciles; after a successful CreateOrUpdateAccount call in
handleUpdatedSecretRef, set user.Status.EffectiveBMCSecretRef =
user.Spec.BMCSecretRef and persist the status (use r.Status().Update(ctx, user)
or equivalent) and handle/return any error from that status update so the status
change is saved and the reconcile loop no longer retriggers the same update.
- Around line 356-372: The bmcConnectionTest currently discards the BMC client
returned by bmcutils.CreateBMCClient and returns without closing it; change the
call to capture the client (e.g., bmcClient, err :=
bmcutils.CreateBMCClient(...)) and ensure you call bmcClient.Logout(ctx) when
the connection test succeeds (either via defer immediately after a successful
CreateBMCClient or by calling Logout before returning true/false), handling any
logout error (log it) but still returning the original success result; keep the
existing 401/403 handling and error returns intact.
- Around line 374-378: In BMCUserReconciler.delete, when user.Spec.BMCRef is nil
the function returns without removing the finalizer; change this so that before
returning you remove the BMCUser finalizer from user.Finalizers (use
controllerutil.RemoveFinalizer or equivalent with the same finalizer identifier
used elsewhere), call r.Client.Update(ctx, user) to persist the change, log the
outcome, and handle/update any update error so the resource can finish deleting;
keep this logic inside the delete method (referencing BMCUserReconciler.delete,
user.Spec.BMCRef, controllerutil.RemoveFinalizer / user.Finalizers and
r.Client.Update).
- Around line 394-397: The current code logs "Removed finalizer for User" even
when PatchEnsureNoFinalizer returned an error; update the control flow around
clientutils.PatchEnsureNoFinalizer(ctx, r.Client, user, BMCUserFinalizer) so you
only call log.Info("Removed finalizer for User", "User", user.Name) when
modified is true and err is nil, and log the error via log.Error(err, "failed to
remove finalizer for User", "User", user.Name) (or return the error) when err !=
nil; keep the same return signature but ensure the success log is not emitted on
error.
In @internal/webhook/v1alpha1/bmcsecret_webhook_test.go:
- Around line 26-36: The test BMCSecret fixture has an invalid Namespace value
and uses awkward slice-index pointer hacks for booleans; set a real namespace
string (e.g., "test-namespace") on the BMCSecret.ObjectMeta.Namespace and
replace the &[]bool{true}[0] pattern by declaring a bool variable (e.g.,
immutableTrue := true) and using &immutableTrue for BMCSecret.Immutable; also
create immutableFalse := false and use &immutableFalse where the test currently
uses &[]bool{false}[0].
🧹 Nitpick comments (14)
bmc/mock/server/server.go (1)
160-175: Minor inconsistency in log output and potential mock behavior consideration.
- The log statement on line 161 is missing the
"address", s.addrfield that other handlers include (see lines 92, 247).- Returning 404 for resources that exist in the embedded filesystem but not in overrides may be acceptable for mock purposes, but verify this aligns with expected test scenarios where DELETE is called on resources created via POST.
🔧 Suggested fix for log consistency
func (s *MockServer) handleRedfishDELETE(w http.ResponseWriter, r *http.Request) { - s.log.Info("Received request", "method", r.Method, "path", r.URL.Path) + s.log.Info("Received request", "method", r.Method, "path", r.URL.Path, "address", s.addr)bmc/redfish.go (1)
1092-1105: Type mismatch and potential double-percent in vendor config.
Line 1102: The key
"default"is a string literal, but the map type ismap[Manufacturer]ManufacturerPasswordConfig. SinceManufactureris a string type alias, this works but is inconsistent. Consider defining a constant likeManufacturerDefault Manufacturer = "default".Line 1094: The Dell special chars contain
%%which results in two%characters in the string. If only a single%is intended, change to%.🔧 Suggested improvements
+const ManufacturerDefault Manufacturer = "default" + var manufacturerPasswordConfigs = map[Manufacturer]ManufacturerPasswordConfig{ ManufacturerDell: { - SpecialChars: "!#$%%&()*.?-@[]^_`{}|~+=", + SpecialChars: "!#$%&()*.?-@[]^_`{}|~+=", }, // ... - "default": { + ManufacturerDefault: { SpecialChars: "!@#$%&*()_-+=[]{}/?~|", }, }And update line 1111:
- config = manufacturerPasswordConfigs["default"] + config = manufacturerPasswordConfigs[ManufacturerDefault]config/samples/metal_v1alpha1_bmcuser.yaml (1)
18-18: Verify the field name casing is intentional.The
BMCControllerUserfield uses PascalCase, which is inconsistent with other fields in the spec that use camelCase (e.g.,userName,roleID,bmcSecretRef). Please confirm whether this naming is:
- Intentional (perhaps to emphasize it's a special system field), or
- Should be changed to
bmcControllerUserfor consistency with Kubernetes naming conventions.bmc/mockup.go (1)
6-9: Consider updating gofish to v0.20.1.The code currently uses
github.com/stmcginnis/gofishv0.20.0. Version v0.20.1 is available and no known security vulnerabilities were found in either version. While the update is optional, keeping dependencies current is a good practice.internal/webhook/v1alpha1/bmcsecret_webhook_test.go (1)
44-46: Remove empty AfterEach or implement teardown logic.The
AfterEachblock contains only a TODO comment with no implementation. If no teardown logic is needed beyond theDeferCleanupinBeforeEach(line 39), remove this empty block entirely.cmd/manager/main.go (1)
416-430: Use consistent controller name in logs (“BMCUser” vs “User”).
Right now the setup error logs"controller", "User", while the reconciler isBMCUserReconcilerand the CRD kind isBMCUser—this makes grepping logs harder.Proposed tweak
- setupLog.Error(err, "unable to create controller", "controller", "User") + setupLog.Error(err, "unable to create controller", "controller", "BMCUser")internal/controller/bmcuser_controller_test.go (2)
66-77: AfterEach cleanup is brittle (may hang / fail when server or objects don’t exist).
Eventually(Get(server)).Should(Succeed())can hang if the server never got created (e.g., earlier failure). Also, deletes forbmcSecret/bmccan fail if already deleted.
123-125: Make password/RoleID assertions less fragile (exact length + inconsistent casing).
Expect(password).To(HaveLen(30))will break if the generator changes but still respects max length; consider asserting<= 30(and maybe a minimum / complexity expectation if needed).- RoleID strings vary (
ReadOnlyvsReadonly)—if the controller validates known roles, this can cause inconsistent test outcomes. Prefer a shared constant/enum if available.Also applies to: 137-241
internal/webhook/v1alpha1/bmcsecret_webhook.go (2)
26-34: Either wireClient: mgr.GetClient()or remove the unused field.
Right nowBMCSecretCustomValidator.Clientis never set and never used, which is confusing for future extensions.Proposed wiring
func SetupBMCSecretWebhookWithManager(mgr ctrl.Manager) error { return ctrl.NewWebhookManagedBy(mgr).For(&metalv1alpha1.BMCSecret{}). - WithValidator(&BMCSecretCustomValidator{}). + WithValidator(&BMCSecretCustomValidator{Client: mgr.GetClient()}). Complete() }
52-73: Immutability checks: watch out for nil-vs-empty map diffs; consider structured field errors.
reflect.DeepEqualwill treatniland{}as different; if admission decoding/defaulting ever changes representation, this could block “no-op” updates. Also, returningfmt.Errorf(...)works but loses field context; a structured invalid error is nicer for UX.internal/bmcutils/bmcutils.go (1)
147-188: Replace commented code with a TODO referencing the correct type, and consider a stricter options API.The commented admin-user block references
metalv1alpha1.User, but the codebase only definesmetalv1alpha1.BMCUser. Replace the large commented block with a concise TODO pointing to the intended feature and tracking issue, using the correct type name to prevent future confusion.The
opts ...BMCClientOptionsvariadic pattern (int-based enum) works but is prone to misuse as the codebase scales. Consider migrating to functional options or a small struct for better type safety.config/crd/bases/metal.ironcore.dev_bmcusers.yaml (1)
84-86: Addformat: date-timetopasswordExpirationfield.
status.passwordExpiration(line 118–119) is a timestamp but lacks the OpenAPI format declaration.status.lastRotation(line 115–117) correctly usesformat: date-time;passwordExpirationshould match.Optionally, add pattern validation to
spec.rotationPeriod(line 84–86) to enforce duration format, e.g.pattern: "^([0-9]+(ns|us|µs|ms|s|m|h)?)+$". Note: durations do not use an OpenAPI format value, onlytype: string.internal/controller/bmcuser_controller.go (2)
239-250: UseCreateinstead ofCreateOrPatchfor secrets withGenerateName.Since the secret uses
GenerateName(line 233) and is marked immutable, it will always be a new creation—never a patch. UsingCreateOrPatchis misleading here and adds unnecessary complexity. Additionally, consider usingptr.To(true)fromk8s.io/utils/ptrfor the boolean pointer.♻️ Proposed refactor
+ "k8s.io/utils/ptr" ... secret := &metalv1alpha1.BMCSecret{ ObjectMeta: metav1.ObjectMeta{ GenerateName: user.Name + "-bmcsecret-", }, Data: map[string][]byte{ metalv1alpha1.BMCSecretUsernameKeyName: []byte(user.Spec.UserName), metalv1alpha1.BMCSecretPasswordKeyName: []byte(password), }, - Immutable: &[]bool{true}[0], // Make the secret immutable + Immutable: ptr.To(true), } - op, err := controllerutil.CreateOrPatch(ctx, r.Client, secret, func() error { - if err := controllerutil.SetControllerReference(user, secret, r.Scheme); err != nil { - return fmt.Errorf("failed to set controller reference for BMCSecret: %w", err) - } - return nil - }) + if err := controllerutil.SetControllerReference(user, secret, r.Scheme); err != nil { + return fmt.Errorf("failed to set controller reference for BMCSecret: %w", err) + } + if err := r.Create(ctx, secret); err != nil { + return fmt.Errorf("failed to create BMCSecret: %w", err) + } - if err != nil { - return fmt.Errorf("failed to create or patch BMCSecret: %w", err) - } - log.V(1).Info("BMCSecret created or patched", "BMCSecret", secret.Name, "Operation", op) + log.V(1).Info("BMCSecret created", "BMCSecret", secret.Name)
333-340: Redundant nil check forEffectiveBMCSecretRef.The nil case is already handled and returns at lines 325-331, so the check at line 334 is always true here.
♻️ Minor cleanup
effSecret := &metalv1alpha1.BMCSecret{} - if user.Status.EffectiveBMCSecretRef != nil { - if err := r.Get(ctx, client.ObjectKey{ - Name: user.Status.EffectiveBMCSecretRef.Name, - }, effSecret); err != nil { - return fmt.Errorf("failed to get effective BMCSecret %s: %w", user.Status.EffectiveBMCSecretRef.Name, err) - } + if err := r.Get(ctx, client.ObjectKey{ + Name: user.Status.EffectiveBMCSecretRef.Name, + }, effSecret); err != nil { + return fmt.Errorf("failed to get effective BMCSecret %s: %w", user.Status.EffectiveBMCSecretRef.Name, err) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
dist/chart/templates/crd/metal.ironcore.dev_bmcusers.yamlis excluded by!**/dist/**dist/chart/templates/crd/metal.ironcore.dev_users.yamlis excluded by!**/dist/**dist/chart/templates/rbac/bmcuser_admin_role.yamlis excluded by!**/dist/**dist/chart/templates/rbac/bmcuser_editor_role.yamlis excluded by!**/dist/**dist/chart/templates/rbac/bmcuser_viewer_role.yamlis excluded by!**/dist/**dist/chart/templates/rbac/role.yamlis excluded by!**/dist/**
📒 Files selected for processing (31)
PROJECTapi/v1alpha1/bmc_types.goapi/v1alpha1/bmcuser_types.goapi/v1alpha1/constants.goapi/v1alpha1/zz_generated.deepcopy.gobmc/bmc.gobmc/mock/server/data/AccountService/index.jsonbmc/mock/server/server.gobmc/mockup.gobmc/redfish.gobmc/redfish_local.gocmd/manager/main.goconfig/certmanager/issuer.yamlconfig/crd/bases/metal.ironcore.dev_bmcusers.yamlconfig/crd/kustomization.yamlconfig/rbac/bmcuser_admin_role.yamlconfig/rbac/bmcuser_editor_role.yamlconfig/rbac/bmcuser_viewer_role.yamlconfig/rbac/kustomization.yamlconfig/rbac/role.yamlconfig/samples/kustomization.yamlconfig/samples/metal_v1alpha1_bmcuser.yamldocs/api-reference/api.mdinternal/bmcutils/bmcutils.gointernal/controller/bmcuser_controller.gointernal/controller/bmcuser_controller_test.gointernal/controller/suite_test.gointernal/controller/test_helper.gointernal/webhook/v1alpha1/bmcsecret_webhook.gointernal/webhook/v1alpha1/bmcsecret_webhook_test.gointernal/webhook/v1alpha1/webhook_suite_test.go
🧰 Additional context used
🧬 Code graph analysis (12)
internal/controller/suite_test.go (2)
internal/controller/bmcuser_controller.go (1)
BMCUserReconciler(31-36)bmc/redfish.go (1)
Options(43-53)
internal/webhook/v1alpha1/webhook_suite_test.go (1)
internal/webhook/v1alpha1/bmcsecret_webhook.go (1)
SetupBMCSecretWebhookWithManager(26-30)
api/v1alpha1/bmcuser_types.go (1)
api/v1alpha1/groupversion_info.go (1)
SchemeBuilder(19-19)
internal/controller/test_helper.go (1)
api/v1alpha1/bmcuser_types.go (1)
BMCUserList(50-54)
bmc/redfish_local.go (3)
bmc/mockup.go (1)
UnitTestMockUps(193-193)bmc/redfish.go (1)
RedfishBMC(56-59)bmc/bmc.go (1)
Entity(137-142)
internal/bmcutils/bmcutils.go (1)
api/v1alpha1/bmc_types.go (1)
Protocol(127-138)
internal/webhook/v1alpha1/bmcsecret_webhook.go (1)
api/v1alpha1/bmcsecret_types.go (1)
BMCSecret(24-57)
internal/webhook/v1alpha1/bmcsecret_webhook_test.go (2)
api/v1alpha1/bmcsecret_types.go (1)
BMCSecret(24-57)internal/webhook/v1alpha1/bmcsecret_webhook.go (1)
BMCSecretCustomValidator(32-34)
api/v1alpha1/zz_generated.deepcopy.go (1)
api/v1alpha1/bmcuser_types.go (4)
BMCUser(39-45)BMCUserList(50-54)BMCUserSpec(12-24)BMCUserStatus(27-32)
cmd/manager/main.go (2)
internal/controller/bmcuser_controller.go (1)
BMCUserReconciler(31-36)bmc/redfish.go (1)
Options(43-53)
internal/controller/bmcuser_controller.go (7)
bmc/redfish.go (1)
Options(43-53)api/v1alpha1/bmcuser_types.go (1)
BMCUser(39-45)api/v1alpha1/bmc_types.go (2)
BMC(257-263)Protocol(127-138)internal/bmcutils/bmcutils.go (4)
GetBMCCredentialsFromSecret(49-60)GetBMCClientFromBMC(147-188)GetBMCAddressForBMC(93-109)CreateBMCClient(190-229)api/v1alpha1/constants.go (2)
OperationAnnotation(21-21)OperationAnnotationRotateCredentials(45-45)bmc/bmc.go (1)
Manufacturer(19-19)api/v1alpha1/bmcsecret_types.go (2)
BMCSecretUsernameKeyName(13-13)BMCSecretPasswordKeyName(15-15)
bmc/redfish.go (1)
bmc/bmc.go (4)
Manufacturer(19-19)ManufacturerDell(22-22)ManufacturerHPE(24-24)ManufacturerLenovo(23-23)
🪛 Checkov (3.2.334)
config/rbac/bmcuser_admin_role.yaml
[medium] 8-27: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
🔇 Additional comments (33)
bmc/mock/server/server.go (1)
84-85: LGTM!The DELETE method routing follows the established pattern for other HTTP methods in the handler.
bmc/mock/server/data/AccountService/index.json (1)
12-12: LGTM!Adding
MaxPasswordLength: 30provides the constraint needed for password generation validation in the mock AccountService. This aligns with the broader BMCUser controller changes.PROJECT (2)
32-34: LGTM!Adding webhook validation support for
BMCSecretenables credential validation during create/update operations, which is important for the BMCUser credential management workflow.
148-155: LGTM!The
BMCUserresource is correctly registered as a cluster-scoped resource with controller support, following the pattern of other resources in this project.api/v1alpha1/constants.go (2)
44-45: LGTM!The new
OperationAnnotationRotateCredentialsconstant follows the established naming convention and provides a clear mechanism for triggering credential rotation via annotations.
49-61: Old constant names already removed or never existed in codebase.The old names
GracefulShutdownServerPowerandGracefulShutdownBMCdo not appear anywhere in the repository. The new constantsGracefulRestartServerPowerandGracefulRestartBMCare actively used and consistently referenced across multiple controller files (biossettings_controller.go,biosversion_controller.go,bmcsettings_controller.go) and in theAnnotationToRedfishMapping. Since this is a v1alpha1 API, breaking changes are expected and acceptable per standard API versioning practices.api/v1alpha1/zz_generated.deepcopy.go (1)
837-948: LGTM!This is auto-generated code by
controller-genfor the newBMCUsertypes. The deepcopy implementations correctly handle all pointer fields (RotationPeriod,BMCSecretRef,BMCRef,EffectiveBMCSecretRef,LastRotation) as expected.bmc/redfish.go (5)
84-89: LGTM!Enabling
ReuseConnections: trueimproves performance by reusing HTTP connections, which is beneficial for the multiple API calls made during account management operations.
775-795: LGTM!The dual verification of both
userNameandidbefore deletion is a good safety measure to prevent accidental deletion of the wrong account.
805-815: LGTM!Clean wrapper method following standard patterns.
1107-1161: LGTM - Secure password generation implementation.The password generation logic correctly:
- Uses
crypto/randfor cryptographic randomness- Ensures minimum complexity by including one character from each required category
- Shuffles the result to avoid predictable character positions
- Validates minimum length requirements
1163-1181: LGTM!Helper functions
randomCharandshuffleRunescorrectly usecrypto/randfor secure random number generation with Fisher-Yates shuffle algorithm.api/v1alpha1/bmc_types.go (1)
23-30: Verify usage of the new PasswordPolicy type.The
PasswordPolicytype and its constants are well-defined with clear documentation. However, ensure this type is referenced in theBMCUserSpecor related API types to be useful in the API surface.#!/bin/bash # Verify PasswordPolicy type is used in the API types rg -n "PasswordPolicy" --type=go -C 2bmc/mockup.go (1)
6-9: LGTM!The imports for gofish common and redfish packages are correctly added to support the ManagerAccount functionality.
config/crd/kustomization.yaml (2)
19-19: LGTM!The BMCUser CRD base resource is correctly added to the kustomization resources list.
41-41: The patch filecainjection_in_accounts.yamldoes not exist.The commented line references a patch that has no implementation in the
config/crd/patches/directory. Since this line is currently commented out, it poses no runtime risk. However, if this patch is intended to be used, it either needs to be created or the reference should be removed.Note: The original concern about naming inconsistency between the resource (
bmcusers) and patch (accounts) is not the issue—both patch files are missing from the directory.Likely an incorrect or invalid review comment.
internal/controller/suite_test.go (2)
131-131: LGTM!Adding the Logger to the test manager configuration improves observability during test execution.
264-273: LGTM!The BMCUserReconciler setup is properly configured for the test environment with appropriate polling intervals and options consistent with other reconcilers in the suite.
config/samples/kustomization.yaml (1)
17-17: LGTM!The BMCUser sample manifest is correctly added to the kustomization resources.
config/samples/metal_v1alpha1_bmcuser.yaml (1)
1-12: LGTM!The sample manifest structure and field values are well-formed and provide a good example for BMCUser resource creation. The rotation period of 720 hours (30 days) is a reasonable default.
Also applies to: 17-19
internal/webhook/v1alpha1/webhook_suite_test.go (1)
120-122: LGTM! BMCSecret webhook setup follows the established pattern.The webhook registration is correctly positioned and includes proper error handling, consistent with other webhook setups in the test suite.
config/rbac/kustomization.yaml (1)
25-27: LGTM! BMCUser RBAC roles added correctly.The three RBAC role files (admin, editor, viewer) follow the standard scaffolding pattern used for other CRDs in this project.
Note: The AI summary indicates that bmcversionset_* roles are removed, but they remain present at lines 28-30. Only the bmcuser_* roles are being added in this change.
internal/controller/test_helper.go (1)
25-27: LGTM! BMCUser cleanup check follows the established pattern.The cleanup verification for BMCUserList is correctly implemented and consistent with the other resource cleanup checks in
EnsureCleanState.config/rbac/bmcuser_viewer_role.yaml (1)
1-29: LGTM! BMCUser viewer role is properly configured.The ClusterRole grants appropriate read-only permissions (get, list, watch) for monitoring and visibility purposes. The role follows Kubernetes RBAC best practices and is consistent with other viewer roles in the project.
config/rbac/bmcuser_editor_role.yaml (1)
1-33: LGTM! BMCUser editor role is properly configured.The ClusterRole grants appropriate permissions for managing BMCUser resources. The role correctly provides full CRUD operations on bmcusers while limiting status access to read-only, which aligns with Kubernetes controller best practices where status is typically managed by controllers.
config/rbac/role.yaml (1)
50-50: LGTM! BMCUser RBAC permissions are properly configured.The additions of
bmcusers,bmcusers/finalizers, andbmcusers/statusare consistent with the existing pattern for other CRDs in this ClusterRole. All necessary permissions for the BMCUser controller are in place.Also applies to: 77-77, 97-97
config/rbac/bmcuser_admin_role.yaml (1)
1-27: Verify wildcard verb usage is intentional for admin role.The ClusterRole uses wildcard (
*) verbs for thebmcusersresource (line 21). While the comments indicate this is intended for cluster admins who can delegate permissions, consider whether a more restrictive set of verbs would be appropriate:
- Create, get, list, watch, update, patch, delete are typical for admin roles
- Wildcard includes future verbs that may be added to the API
The pattern is consistent with Kubernetes admin role conventions, but explicitly listing verbs provides better auditability and security boundaries.
Based on learnings from static analysis (Checkov CKV_K8S_49), wildcard usage in RBAC should be minimized.
🔒 Consider explicit verb listing
- apiGroups: - metal.ironcore.dev resources: - bmcusers verbs: - - '*' + - create + - delete + - get + - list + - patch + - update + - watchapi/v1alpha1/bmcuser_types.go (1)
34-58: LGTM! BMCUser type definitions follow Kubernetes conventions.The BMCUser and BMCUserList types are properly structured with:
- Correct kubebuilder markers for CRD generation (root object, status subresource, cluster scope)
- Standard TypeMeta, ObjectMeta, Spec, and Status composition
- Proper scheme registration in init()
The structure aligns with Kubernetes API conventions and other CRDs in this project.
bmc/bmc.go (1)
118-129: All BMC implementations are complete; no breaking changes to resolve. DeleteAccount semantics are clear from usage patterns in the codebase.All three BMC implementations—RedfishBMC, RedfishLocalBMC, and RedfishKubeBMC—have been updated with the new account management methods. RedfishKubeBMC inherits all methods from RedfishBMC via struct embedding, so it needs no explicit implementations. The DeleteAccount method signature is unambiguous:
userNameis the user's login name andidis the unique account identifier returned by the BMC, as shown by usage inbmcuser_controller.gowhere it's called withuser.Spec.UserNameanduser.Status.ID.internal/controller/bmcuser_controller.go (4)
26-36: LGTM!The reconciler struct and finalizer constant follow standard controller-runtime patterns.
42-58: LGTM!The entry point follows the standard controller pattern with proper deletion timestamp handling and
client.IgnoreNotFoundusage.
402-409: LGTM!The controller setup correctly watches BMCUser resources and owns BMCSecret resources for proper event triggering.
163-168: Nil pointer dereference whenforceRotationis true butRotationPeriodis nil.If
forceRotationis true (due to annotation or expired password) butuser.Spec.RotationPeriodis nil, accessinguser.Spec.RotationPeriod.Durationon line 163 will cause a panic.🐛 Proposed fix
- if user.Status.LastRotation != nil && user.Status.LastRotation.Add(user.Spec.RotationPeriod.Duration).After(metav1.Now().Time) && !forceRotation { + if user.Spec.RotationPeriod != nil && user.Status.LastRotation != nil && user.Status.LastRotation.Add(user.Spec.RotationPeriod.Duration).After(metav1.Now().Time) && !forceRotation { log.V(1).Info("BMC user password rotation is not needed yet", "User", user.Name) return ctrl.Result{ Requeue: true, RequeueAfter: user.Spec.RotationPeriod.Duration, }, nil }Likely an incorrect or invalid review comment.
| // BMCUserSpec defines the desired state of BMCUser. | ||
| type BMCUserSpec struct { | ||
| UserName string `json:"userName"` | ||
| RoleID string `json:"roleID"` | ||
| Description string `json:"description,omitempty"` | ||
| RotationPeriod *metav1.Duration `json:"rotationPeriod,omitempty"` | ||
| // if not set, the operator will generate a secure password based on BMC manufacturer requirements. | ||
| BMCSecretRef *v1.LocalObjectReference `json:"bmcSecretRef,omitempty"` | ||
| BMCRef *v1.LocalObjectReference `json:"bmcRef,omitempty"` | ||
| Enabled bool `json:"enabled"` | ||
| // set if the user should be used by the BMC controller to access the system. | ||
| // +kubebuilder:default=false | ||
| BMCControllerUser bool `json:"bmcControllerUser"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add validation markers for BMCUserSpec fields.
The BMCUserSpec lacks validation constraints that would help prevent invalid configurations:
- UserName (line 13): No pattern, length constraints, or format validation
- RoleID (line 14): No validation for valid role IDs (consider enum or pattern)
- RotationPeriod (line 16): No minimum duration validation
- Enabled (line 20): Missing
omitemptytag -falsewill always be serialized - BMCControllerUser (line 23): Missing
omitemptytag -falsewill always be serialized
🔧 Proposed validation additions
type BMCUserSpec struct {
- UserName string `json:"userName"`
- RoleID string `json:"roleID"`
+ // +kubebuilder:validation:MinLength=1
+ // +kubebuilder:validation:MaxLength=64
+ // +kubebuilder:validation:Pattern=`^[a-zA-Z0-9_-]+$`
+ UserName string `json:"userName"`
+ // +kubebuilder:validation:MinLength=1
+ RoleID string `json:"roleID"`
Description string `json:"description,omitempty"`
+ // +kubebuilder:validation:Minimum=1
RotationPeriod *metav1.Duration `json:"rotationPeriod,omitempty"`
// if not set, the operator will generate a secure password based on BMC manufacturer requirements.
BMCSecretRef *v1.LocalObjectReference `json:"bmcSecretRef,omitempty"`
BMCRef *v1.LocalObjectReference `json:"bmcRef,omitempty"`
- Enabled bool `json:"enabled"`
+ Enabled bool `json:"enabled,omitempty"`
// set if the user should be used by the BMC controller to access the system.
// +kubebuilder:default=false
- BMCControllerUser bool `json:"bmcControllerUser"`
+ BMCControllerUser bool `json:"bmcControllerUser,omitempty"`
}Validation patterns should be confirmed against BMC vendor requirements. If RoleID has a fixed set of values, use an enum validation instead.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // BMCUserSpec defines the desired state of BMCUser. | |
| type BMCUserSpec struct { | |
| UserName string `json:"userName"` | |
| RoleID string `json:"roleID"` | |
| Description string `json:"description,omitempty"` | |
| RotationPeriod *metav1.Duration `json:"rotationPeriod,omitempty"` | |
| // if not set, the operator will generate a secure password based on BMC manufacturer requirements. | |
| BMCSecretRef *v1.LocalObjectReference `json:"bmcSecretRef,omitempty"` | |
| BMCRef *v1.LocalObjectReference `json:"bmcRef,omitempty"` | |
| Enabled bool `json:"enabled"` | |
| // set if the user should be used by the BMC controller to access the system. | |
| // +kubebuilder:default=false | |
| BMCControllerUser bool `json:"bmcControllerUser"` | |
| } | |
| // BMCUserSpec defines the desired state of BMCUser. | |
| type BMCUserSpec struct { | |
| // +kubebuilder:validation:MinLength=1 | |
| // +kubebuilder:validation:MaxLength=64 | |
| // +kubebuilder:validation:Pattern=`^[a-zA-Z0-9_-]+$` | |
| UserName string `json:"userName"` | |
| // +kubebuilder:validation:MinLength=1 | |
| RoleID string `json:"roleID"` | |
| Description string `json:"description,omitempty"` | |
| // +kubebuilder:validation:Minimum=1 | |
| RotationPeriod *metav1.Duration `json:"rotationPeriod,omitempty"` | |
| // if not set, the operator will generate a secure password based on BMC manufacturer requirements. | |
| BMCSecretRef *v1.LocalObjectReference `json:"bmcSecretRef,omitempty"` | |
| BMCRef *v1.LocalObjectReference `json:"bmcRef,omitempty"` | |
| Enabled bool `json:"enabled,omitempty"` | |
| // set if the user should be used by the BMC controller to access the system. | |
| // +kubebuilder:default=false | |
| BMCControllerUser bool `json:"bmcControllerUser,omitempty"` | |
| } |
🤖 Prompt for AI Agents
In @api/v1alpha1/bmcuser_types.go around lines 11 - 24, Add kubebuilder
validation markers and omitempty json tags on BMCUserSpec: add
+kubebuilder:validation:Pattern and/or
+kubebuilder:validation:MinLength/MaxLength above UserName; add either
+kubebuilder:validation:Enum (or +kubebuilder:validation:Pattern) above RoleID
to restrict valid role IDs; add a minimum constraint above RotationPeriod (e.g.,
+kubebuilder:validation:Minimum=<seconds> or an appropriate duration validation)
to prevent zero/negative durations; and add `omitempty` to the json tags for
Enabled and BMCControllerUser so false values are not always serialized. Apply
these markers to the BMCUserSpec struct fields UserName, RoleID, RotationPeriod,
Enabled, and BMCControllerUser.
| // GetAccounts retrieves all user accounts from the BMC. | ||
| func (r *RedfishLocalBMC) GetAccounts(ctx context.Context) ([]*redfish.ManagerAccount, error) { | ||
| accounts := make([]*redfish.ManagerAccount, 0, len(UnitTestMockUps.Accounts)) | ||
| for _, a := range UnitTestMockUps.Accounts { | ||
| accounts = append(accounts, a) | ||
| } | ||
| return accounts, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file to see the actual implementation of all three methods
head -100 bmc/redfish_local.go | tail -50Repository: ironcore-dev/metal-operator
Length of output: 1565
🏁 Script executed:
# Also check the full file structure around the specified lines
wc -l bmc/redfish_local.goRepository: ironcore-dev/metal-operator
Length of output: 95
🏁 Script executed:
# Let's look at the specific lines mentioned
sed -n '55,96p' bmc/redfish_local.goRepository: ironcore-dev/metal-operator
Length of output: 1342
🏁 Script executed:
# Check for UnitTestMockUps.Accounts usage patterns
rg -n --type=go 'UnitTestMockUps\.Accounts' -C2Repository: ironcore-dev/metal-operator
Length of output: 2913
🏁 Script executed:
# Check for concurrent test patterns
rg -n --type=go 'Parallel\(|GOMAXPROCS' -C2Repository: ironcore-dev/metal-operator
Length of output: 53
🏁 Script executed:
# Check if tests use -race flag in CI configuration
fd -e yml -e yaml | xargs rg -l 'race|go.*test' || echo "No matches"Repository: ironcore-dev/metal-operator
Length of output: 163
🏁 Script executed:
# Also check the UnitTestMockUps definition to understand the data structure
rg -n 'UnitTestMockUps.*{|var.*UnitTestMockUps|type.*UnitTestMockUps' -A 20 --type=goRepository: ironcore-dev/metal-operator
Length of output: 232
🏁 Script executed:
# Search for UnitTestMockUps definition
rg -n 'UnitTestMockUps\s*=' --type=go -A 5Repository: ironcore-dev/metal-operator
Length of output: 844
🏁 Script executed:
# Search for type definition
rg -n 'type.*Mock|var.*Mock' --type=go | head -20Repository: ironcore-dev/metal-operator
Length of output: 224
🏁 Script executed:
# Check for goroutines in tests
rg -n 'go\s+' bmc/ --type=go -C2Repository: ironcore-dev/metal-operator
Length of output: 4866
🏁 Script executed:
# Check CI configuration files for race flag
find . -name '.github' -o -name 'Makefile' -o -name '*.yml' -o -name '*.yaml' | head -20Repository: ironcore-dev/metal-operator
Length of output: 718
🏁 Script executed:
# Check the RedfishMockUps struct definition
sed -n '12,100p' bmc/mockup.goRepository: ironcore-dev/metal-operator
Length of output: 2291
🏁 Script executed:
# Check if there's a Makefile with test commands
grep -n 'test\|race' Makefile | head -20Repository: ironcore-dev/metal-operator
Length of output: 1429
🏁 Script executed:
# Check CI workflow for test flags
cat .github/workflows/test.yml | grep -A 5 -B 5 'race\|go test'Repository: ironcore-dev/metal-operator
Length of output: 53
🏁 Script executed:
# Let's verify the concurrent goroutines access the Accounts field specifically
rg -n 'BIOSUpgradeTaskIndex|PendingBMCSetting|BMCUpgradeTaskIndex' bmc/redfish_local.go -B 5 -A 5Repository: ironcore-dev/metal-operator
Length of output: 3983
🏁 Script executed:
# Check if these goroutines run during tests
rg -n 'UpdateFirmwareImage|ResetManager|UpgradeBMCFirmware' bmc/redfish_local.goRepository: ironcore-dev/metal-operator
Length of output: 254
🏁 Script executed:
# Final verification: check if any tests actually call these three account methods concurrently
rg -n 'GetAccounts|CreateOrUpdateAccount|DeleteAccount' internal/controller/ --type=go -B 2 -A 2Repository: ironcore-dev/metal-operator
Length of output: 3175
Mock account storage isn't concurrency-safe and has efficiency issues.
The three account methods (GetAccounts, CreateOrUpdateAccount, DeleteAccount) mutate/read the shared global UnitTestMockUps.Accounts map without synchronization, making them race-prone if tests or concurrent operations access them.
Additionally:
CreateOrUpdateAccountscans all map values unnecessarily; use direct lookup byuserNamekey instead:if a, ok := UnitTestMockUps.Accounts[userName]DeleteAccountaccepts anidparameter but ignores it, using onlyuserNamefor deletion. This creates API inconsistency since the controller passesuser.Status.ID(line 391 in bmcuser_controller.go); consider validating or documenting thatidis unused in local mock mode.
🤖 Prompt for AI Agents
In @bmc/redfish_local.go around lines 55 - 62, UnitTestMockUps.Accounts access
is not concurrency-safe and some ops are inefficient/inconsistent: protect
reads/writes with a sync.RWMutex on UnitTestMockUps (use RLock/RUnlock for reads
in GetAccounts and RLock when checking in CreateOrUpdateAccount, Lock/Unlock for
mutations), change CreateOrUpdateAccount to look up the account directly using
if a, ok := UnitTestMockUps.Accounts[userName] instead of scanning all values,
and make DeleteAccount either honor the passed id (validate that id matches the
account being deleted) or explicitly document/validate that id is ignored in
local mock mode and delete by userName under the same mutex protection.
| func (r *BMCUserReconciler) handleUpdatedSecretRef(ctx context.Context, log logr.Logger, user *metalv1alpha1.BMCUser, bmcSecret *metalv1alpha1.BMCSecret, bmcClient bmc.BMC) error { | ||
| log.Info("BMCSecret credentials have changed, updating BMC user", "User", user.Name) | ||
| _, password, err := bmcutils.GetBMCCredentialsFromSecret(bmcSecret) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get credentials from BMCSecret: %w", err) | ||
| } | ||
| // Update the BMC account with the new password | ||
| if err := bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, password, r.Insecure); err != nil { | ||
| return fmt.Errorf("failed to create or update BMC account with new password: %w", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
EffectiveBMCSecretRef not updated after handling secret reference change.
After successfully updating the BMC account with the new password, user.Status.EffectiveBMCSecretRef is not updated to match user.Spec.BMCSecretRef. This causes the condition at line 112 to trigger on every subsequent reconcile, repeatedly calling CreateOrUpdateAccount.
🐛 Proposed fix
func (r *BMCUserReconciler) handleUpdatedSecretRef(ctx context.Context, log logr.Logger, user *metalv1alpha1.BMCUser, bmcSecret *metalv1alpha1.BMCSecret, bmcClient bmc.BMC) error {
log.Info("BMCSecret credentials have changed, updating BMC user", "User", user.Name)
_, password, err := bmcutils.GetBMCCredentialsFromSecret(bmcSecret)
if err != nil {
return fmt.Errorf("failed to get credentials from BMCSecret: %w", err)
}
// Update the BMC account with the new password
if err := bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, password, r.Insecure); err != nil {
return fmt.Errorf("failed to create or update BMC account with new password: %w", err)
}
+ // Update the effective secret reference to match the new secret
+ if err := r.setEffectiveSecretRef(ctx, log, user, bmcSecret); err != nil {
+ return fmt.Errorf("failed to update effective BMCSecretRef: %w", err)
+ }
return nil
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @internal/controller/bmcuser_controller.go around lines 216 - 227,
handleUpdatedSecretRef currently updates the BMC account but does not update
user.Status.EffectiveBMCSecretRef, which causes repeated reconciles; after a
successful CreateOrUpdateAccount call in handleUpdatedSecretRef, set
user.Status.EffectiveBMCSecretRef = user.Spec.BMCSecretRef and persist the
status (use r.Status().Update(ctx, user) or equivalent) and handle/return any
error from that status update so the status change is saved and the reconcile
loop no longer retriggers the same update.
There was a problem hiding this comment.
updating the EffectiveBMCSecretRef, happens only at the beginning of a reconcile loop, when password was changed and when the new user/password have been tested.
There was a problem hiding this comment.
@stefanhipfel, thank you for the clarification! I understand now—the updateEffectiveSecret method at the beginning of the reconcile loop is responsible for testing the new credentials and updating EffectiveBMCSecretRef after validation, while handleUpdatedSecretRef focuses solely on updating the BMC account itself. This separation of concerns makes sense, and the reconcile loop naturally coordinates the status update and BMC account update across iterations. I appreciate you explaining the design.
🧠 Learnings used
Learnt from: Nuckal777
Repo: ironcore-dev/metal-operator PR: 471
File: internal/controller/bmcsettingsset_controller.go:233-236
Timestamp: 2026-01-14T19:27:23.322Z
Learning: In the ironcore-dev/metal-operator repository, BMCSettings.Spec.BMCRef is marked with +required kubebuilder marker, ensuring API-level validation that prevents nil values; defensive nil checks are not needed when iterating BMCSettings from List operations.
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Flag non-idempotent reconcile logic, Spec mutations during reconciliation, and custom patterns duplicating standard Kubernetes behavior
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Reconcile logic must be idempotent and safe to re-run at any time without side effects
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Prevent hot loops caused by status-only changes triggering reconciliation
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Reconciliation must be based on current cluster state, not assumptions; handle NotFound errors correctly and gracefully manage partial failures
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Use `client.Client`, `controllerutil.CreateOrUpdate`, and OwnerReferences from controller-runtime rather than direct API calls
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @internal/webhook/v1alpha1/bmcsecret_webhook_test.go:
- Line 38: The test descriptions use incorrect article "an" before the
initialism BMC; update the string literals in the test cases (e.g., the
By("Creating an BMCSecret") occurrences in bmcsecret_webhook_test.go, including
the other occurrence at the same file) to use "a" instead (By("Creating a
BMCSecret")) so the grammar matches the consonant sound of "BMC".
- Around line 27-31: The Namespace field on the BMCSecret ObjectMeta is set to
the literal string "ns.Name" which is invalid; update the metav1.ObjectMeta for
BMCSecret to use a valid namespace by either removing the quotes to reference
the test namespace variable (e.g., use ns.Name) or replace it with a valid
namespace string like "test-namespace" so metav1.ObjectMeta.Namespace is a
proper Kubernetes name.
🧹 Nitpick comments (7)
internal/webhook/v1alpha1/bmcsecret_webhook_test.go (3)
36-36: Useptr.To(true)for consistency.Line 56 already uses
ptr.To(false)from the importedk8s.io/utils/ptrpackage. Use the same approach here for consistency and readability.♻️ Proposed fix
- Immutable: &[]bool{true}[0], + Immutable: ptr.To(true),
17-19: Variable naming should follow Go conventions.Go convention uses camelCase for variable names.
BMCSecretlooks like a type name rather than a variable. Consider renaming tobmcSecretto distinguish it from the typemetalv1alpha1.BMCSecret.♻️ Proposed fix
var ( - BMCSecret *metalv1alpha1.BMCSecret + bmcSecret *metalv1alpha1.BMCSecret validator BMCSecretCustomValidator )Then update all references throughout the test accordingly (
BMCSecretUpdated→bmcSecretUpdated,BMCSecretMutable→bmcSecretMutable).
26-26: Redundant nil check on struct value.
validatoris a struct value (not a pointer), so it can never be nil. This assertion will always pass and doesn't provide meaningful validation.♻️ Proposed fix
validator = BMCSecretCustomValidator{ Client: k8sClient, } - Expect(validator).NotTo(BeNil(), "Expected validator to be initialized") + Expect(validator.Client).NotTo(BeNil(), "Expected validator client to be initialized")Alternatively, remove the check entirely if client initialization is guaranteed by the test suite.
docs/api-reference/api.md (1)
627-656: Missing field descriptions in BMCUserSpec and BMCUserStatus documentation.The documentation tables for
BMCUserSpec(lines 629-636) andBMCUserStatus(lines 651-655) have empty description columns for most fields. This is inconsistent with other types documented in this file and reduces API discoverability.Consider adding descriptions for fields like
userName,roleID,enabled,lastRotation,passwordExpiration, andidto match the documentation quality of other types in this API reference.internal/controller/bmcuser_controller_test.go (3)
126-134: Clarify or fix effective secret deletion test logic.The test verifies owner references are set (lines 110-118), implying the effective secret should be garbage collected when the BMCUser is deleted. However, line 133 explicitly deletes the
effectiveSecretafter the user is deleted, which contradicts testing the GC behavior.If the intent is to verify cascading deletion works, wait for the secret to be garbage collected instead of manually deleting it. If manual cleanup is intended (e.g., background deletion isn't synchronous in envtest), add a comment explaining this.
Suggested approach to verify GC behavior
Expect(k8sClient.Delete(ctx, user)).To(Succeed()) By("Ensuring that the User resource has been deleted") Eventually(Get(user)).Should(Satisfy(apierrors.IsNotFound)) - By("Ensuring that the effective BMCSecret has been deleted") - Expect(k8sClient.Delete(ctx, effectiveSecret)).To(Succeed()) + By("Ensuring that the effective BMCSecret has been garbage collected") + // Owner reference with BlockOwnerDeletion should trigger cascading deletion Eventually(Get(effectiveSecret)).Should(Satisfy(apierrors.IsNotFound))
319-328: Use consistent assertion pattern for deletion verification.Line 321 uses
Eventually(Get(user)).ShouldNot(Succeed())while line 131 uses the more preciseEventually(Get(user)).Should(Satisfy(apierrors.IsNotFound)). The latter pattern explicitly verifies the expected error type rather than just any failure.Suggested fix
By("Ensuring that the User resource has been deleted") - Eventually(Get(user)).ShouldNot(Succeed()) + Eventually(Get(user)).Should(Satisfy(apierrors.IsNotFound))
273-279: Clarify password rotation assertion.Line 279 asserts the password is not
"bar", but"bar"doesn't appear in the test setup - the initial secret has password"adminpass"(line 35). This assertion may be a remnant from a previous test version or checking against a mock default.Consider either:
- Asserting the password differs from
"adminpass"(the actual initial password)- Adding a comment explaining what
"bar"represents
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
dist/chart/templates/crd/metal.ironcore.dev_bmcusers.yamlis excluded by!**/dist/**
📒 Files selected for processing (8)
api/v1alpha1/bmcuser_types.goapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/metal.ironcore.dev_bmcusers.yamldocs/api-reference/api.mdinternal/controller/bmcuser_controller.gointernal/controller/bmcuser_controller_test.gointernal/webhook/v1alpha1/bmcsecret_webhook.gointernal/webhook/v1alpha1/bmcsecret_webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/webhook/v1alpha1/bmcsecret_webhook.go
- api/v1alpha1/bmcuser_types.go
- internal/controller/bmcuser_controller.go
- config/crd/bases/metal.ironcore.dev_bmcusers.yaml
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Code must follow standard Go formatting and idioms; use clear, explicit error handling with minimal global state
Use small, focused functions; introduce interfaces only when justified
Files:
internal/controller/bmcuser_controller_test.goapi/v1alpha1/zz_generated.deepcopy.gointernal/webhook/v1alpha1/bmcsecret_webhook_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Tests must be deterministic and not rely on timing assumptions
Files:
internal/controller/bmcuser_controller_test.gointernal/webhook/v1alpha1/bmcsecret_webhook_test.go
**/api/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Implement defaulting and validation via webhooks (preferred) or OpenAPI schema annotations
Files:
api/v1alpha1/zz_generated.deepcopy.go
🧠 Learnings (10)
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller_test.go : Use Envtest-based tests for controller behavior and API interactions
Applied to files:
internal/controller/bmcuser_controller_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller_test.go : Write unit tests for reconcile logic and pure functions
Applied to files:
internal/controller/bmcuser_controller_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_test.go : Use fake clients appropriately in tests
Applied to files:
internal/controller/bmcuser_controller_test.gointernal/webhook/v1alpha1/bmcsecret_webhook_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*.go : Include relevant identifiers in logs; emit events for meaningful user-facing state changes
Applied to files:
internal/controller/bmcuser_controller_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Use `client.Client`, `controllerutil.CreateOrUpdate`, and OwnerReferences from controller-runtime rather than direct API calls
Applied to files:
internal/controller/bmcuser_controller_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/*_test.go : Tests must be deterministic and not rely on timing assumptions
Applied to files:
internal/controller/bmcuser_controller_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Reconciliation must be based on current cluster state, not assumptions; handle NotFound errors correctly and gracefully manage partial failures
Applied to files:
internal/controller/bmcuser_controller_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Flag non-idempotent reconcile logic, Spec mutations during reconciliation, and custom patterns duplicating standard Kubernetes behavior
Applied to files:
internal/controller/bmcuser_controller_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/api/**/*_types.go : Clearly mark and document deprecated fields in API types
Applied to files:
api/v1alpha1/zz_generated.deepcopy.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/api/**/*.go : Implement defaulting and validation via webhooks (preferred) or OpenAPI schema annotations
Applied to files:
internal/webhook/v1alpha1/bmcsecret_webhook_test.go
🧬 Code graph analysis (1)
internal/webhook/v1alpha1/bmcsecret_webhook_test.go (2)
api/v1alpha1/bmcsecret_types.go (1)
BMCSecret(24-57)internal/webhook/v1alpha1/bmcsecret_webhook.go (1)
BMCSecretCustomValidator(32-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run tests
- GitHub Check: Run linter
- GitHub Check: Run test chart
- GitHub Check: Run test e2e
🔇 Additional comments (4)
internal/webhook/v1alpha1/bmcsecret_webhook_test.go (1)
45-62: Test coverage looks appropriate for validating webhook behavior.The two test cases correctly cover both branches of the immutability validation logic:
- Deny mutation when
Immutable=true- Allow mutation when
Immutable=falseThe use of
ValidateUpdatedirectly on the validator is a good approach for unit testing webhook logic.internal/controller/bmcuser_controller_test.go (2)
243-282: Potential timing sensitivity in rotation test.This test uses a
1 * time.Secondrotation period (line 257-259) and relies on the controller to detect expiration and perform rotation. While the 4-second Eventually timeout (line 268) provides margin, this pattern can be flaky under CI load where reconciliation may be delayed.Consider using the annotation-driven rotation approach (tested separately at line 330) as the primary rotation mechanism test, or ensure the controller triggers immediate reconciliation when rotation period expires rather than relying on wall-clock time.
Based on learnings, tests must be deterministic and not rely on timing assumptions.
21-77: LGTM on test structure and setup.The test setup follows good practices:
- Uses Ginkgo/Gomega with envtest appropriately per learnings
- BeforeEach/AfterEach properly manage test fixtures
- Uses Eventually with Object/Get patterns for async assertions
The overall test coverage for BMCUser lifecycle (creation, multi-user scenarios, rotation, deletion, annotation-driven rotation) is comprehensive.
api/v1alpha1/zz_generated.deepcopy.go (1)
837-952: Auto-generated deepcopy implementations look correct.The
controller-gengenerated deepcopy functions forBMCUser,BMCUserList,BMCUserSpec, andBMCUserStatusfollow the established patterns in this file and correctly handle:
- Pointer fields with nil checks and explicit copying
metav1.Timepointers using.DeepCopy()- Slice items with element-wise
DeepCopyIntoAs this is auto-generated code, ensure it stays in sync with the source types in
bmcuser_types.goby runningmake generatewhen type definitions change.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @internal/controller/bmcuser_controller.go:
- Around line 165-175: The code can panic when forceRotation is true but
user.Spec.RotationPeriod is nil because the second if accesses
user.Spec.RotationPeriod.Duration; fix by guarding that access: ensure you check
user.Spec.RotationPeriod != nil before any use of .Duration (either move the
existing nil check to run unconditionally or change the second condition to
start with user.Spec.RotationPeriod != nil && user.Status.LastRotation != nil &&
...), and if RotationPeriod is nil decide and document the intended behavior for
forceRotation (e.g., skip rotation or handle as immediate rotation) so
RequeueAfter is only set when Duration is valid.
- Around line 185-196: The rotation is non-atomic because createSecret(...) runs
before bmcClient.CreateOrUpdateAccount(...), leaving Spec.BMCSecretRef pointing
at creds that may not match the BMC on failure; generate the newPassword first,
call bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID,
newPassword, r.Insecure) before calling r.createSecret(ctx, log, user,
newPassword), and only patch user.Status.LastRotation after both account update
and secret creation succeed; if you prefer to keep the current order instead
implement a rollback: if createSecret succeeds but CreateOrUpdateAccount fails,
delete the newly created secret (or restore the previous secret reference) to
avoid leaving the cluster pointing at invalid credentials.
- Around line 335-351: bmcConnectionTest creates a BMC client via
CreateBMCClient but never closes it; change the call to capture the returned
client (e.g., client, err := bmcutils.CreateBMCClient(...)), and after a
successful creation immediately defer closing or logging out the client (call
the client's Close/Logout method) so the connection/session is released; ensure
the defer runs regardless of subsequent error handling (including the 401/403
path) and log any logout error using the reconciler logger.
🧹 Nitpick comments (4)
internal/controller/bmcuser_controller.go (4)
257-261: Spec mutation during reconciliation.The
createSecretfunction modifiesuser.Spec.BMCSecretRefvia a patch operation. Mutating the Spec within a reconcile loop is generally discouraged as it can cause reconciliation loops and makes the controller less predictable. Consider having the user provide theBMCSecretRefexternally, or if auto-generation is intentional, document this behavior clearly and ensure the reconciliation handles the re-queue properly.Based on learnings, spec mutations during reconciliation should be flagged.
131-136: Consider surfacing password expiration parsing failures.When parsing fails, the error is logged but the reconciliation continues successfully. If the BMC consistently returns an unparseable format, this issue will be silently hidden. Consider emitting a Kubernetes event or setting a condition on the status to make this visible to operators.
Based on learnings, events should be emitted for meaningful user-facing state changes.
245-245: Consider usingptr.To(true)for cleaner pointer creation.The expression
&[]bool{true}[0]works but is non-idiomatic Go. Thek8s.io/utils/ptrpackage provides a cleaner helper.♻️ Suggested improvement
+import "k8s.io/utils/ptr" + secret := &metalv1alpha1.BMCSecret{ ObjectMeta: metav1.ObjectMeta{ GenerateName: user.Name + "-bmcsecret-", }, Data: map[string][]byte{ metalv1alpha1.BMCSecretUsernameKeyName: []byte(user.Spec.UserName), metalv1alpha1.BMCSecretPasswordKeyName: []byte(password), }, - Immutable: &[]bool{true}[0], // Make the secret immutable + Immutable: ptr.To(true), }
381-388: Consider watching BMC resources for state changes.The controller watches BMCUser and owned BMCSecret resources, but doesn't watch BMC resources. If a referenced BMC transitions to
Enabledstate after the BMCUser is created, the controller won't be notified to retry. Consider adding a watch on BMC resources with an appropriate filter.♻️ Suggested enhancement
+import ( + "sigs.k8s.io/controller-runtime/pkg/handler" +) + func (r *BMCUserReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&metalv1alpha1.BMCUser{}). Owns(&metalv1alpha1.BMCSecret{}). + Watches( + &metalv1alpha1.BMC{}, + handler.EnqueueRequestsFromMapFunc(r.findBMCUsersForBMC), + ). Named("bmcuser"). Complete(r) } + +func (r *BMCUserReconciler) findBMCUsersForBMC(ctx context.Context, obj client.Object) []ctrl.Request { + // Implementation to find BMCUsers referencing this BMC +}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/controller/bmcuser_controller.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Code must follow standard Go formatting and idioms; use clear, explicit error handling with minimal global state
Use small, focused functions; introduce interfaces only when justified
Files:
internal/controller/bmcuser_controller.go
🧠 Learnings (4)
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Flag non-idempotent reconcile logic, Spec mutations during reconciliation, and custom patterns duplicating standard Kubernetes behavior
Applied to files:
internal/controller/bmcuser_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Use `client.Client`, `controllerutil.CreateOrUpdate`, and OwnerReferences from controller-runtime rather than direct API calls
Applied to files:
internal/controller/bmcuser_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Prevent hot loops caused by status-only changes triggering reconciliation
Applied to files:
internal/controller/bmcuser_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*.go : Include relevant identifiers in logs; emit events for meaningful user-facing state changes
Applied to files:
internal/controller/bmcuser_controller.go
🧬 Code graph analysis (1)
internal/controller/bmcuser_controller.go (6)
api/v1alpha1/bmcuser_types.go (1)
BMCUser(39-45)api/v1alpha1/bmc_types.go (2)
BMC(257-263)Protocol(127-138)internal/webhook/v1alpha1/bmcsecret_webhook_test.go (1)
BMCSecret(18-18)internal/bmcutils/bmcutils.go (4)
GetBMCCredentialsFromSecret(49-60)GetBMCClientFromBMC(147-188)GetProtocolScheme(39-47)CreateBMCClient(190-229)api/v1alpha1/constants.go (2)
OperationAnnotation(21-21)OperationAnnotationRotateCredentials(45-45)api/v1alpha1/bmcsecret_types.go (2)
BMCSecretUsernameKeyName(13-13)BMCSecretPasswordKeyName(15-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run test chart
- GitHub Check: Run linter
- GitHub Check: Run tests
- GitHub Check: Run test e2e
🔇 Additional comments (2)
internal/controller/bmcuser_controller.go (2)
26-36: LGTM!The reconciler struct follows the standard controller-runtime pattern, and the RBAC markers correctly declare permissions for BMCUser resources and their status/finalizers.
42-58: LGTM!The reconciliation entry point follows idiomatic controller-runtime patterns with proper error handling and deletion timestamp checking.
| if err := r.createSecret(ctx, log, user, newPassword); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to create BMCSecret: %w", err) | ||
| } | ||
| if err := bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, newPassword, r.Insecure); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to create or update BMC account with new password: %w", err) | ||
| } | ||
| // Update the last rotation time | ||
| userBase := user.DeepCopy() | ||
| user.Status.LastRotation = &metav1.Time{Time: metav1.Now().Time} | ||
| if err := r.Status().Patch(ctx, user, client.MergeFrom(userBase)); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to patch User status with last rotation time: %w", err) | ||
| } |
There was a problem hiding this comment.
Non-atomic credential rotation may leave inconsistent state.
The sequence creates a new secret first (line 185), then updates the BMC account (line 188). If CreateOrUpdateAccount fails, the Spec.BMCSecretRef already points to a secret with credentials that don't match the BMC. Subsequent reconciliation attempts will fail to authenticate.
Consider reversing the order: update the BMC account first (you can generate the password before calling createSecret), then create the secret only on success. Alternatively, implement a rollback mechanism.
🛠️ Suggested approach
log.Info("Rotating BMC user password", "User", user.Name)
accountService, err := bmcClient.GetAccountService(ctx)
if err != nil {
return ctrl.Result{}, fmt.Errorf("failed to get account service: %w", err)
}
newPassword, err := bmc.GenerateSecurePassword(bmc.Manufacturer(bmcObj.Status.Manufacturer), accountService.MaxPasswordLength)
if err != nil {
return ctrl.Result{}, fmt.Errorf("failed to generate new password for BMC user %s: %w", user.Name, err)
}
- if err := r.createSecret(ctx, log, user, newPassword); err != nil {
- return ctrl.Result{}, fmt.Errorf("failed to create BMCSecret: %w", err)
- }
+ // Update BMC account first to ensure credentials are valid before persisting
if err := bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, newPassword, r.Insecure); err != nil {
return ctrl.Result{}, fmt.Errorf("failed to create or update BMC account with new password: %w", err)
}
+ if err := r.createSecret(ctx, log, user, newPassword); err != nil {
+ return ctrl.Result{}, fmt.Errorf("failed to create BMCSecret: %w", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := r.createSecret(ctx, log, user, newPassword); err != nil { | |
| return ctrl.Result{}, fmt.Errorf("failed to create BMCSecret: %w", err) | |
| } | |
| if err := bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, newPassword, r.Insecure); err != nil { | |
| return ctrl.Result{}, fmt.Errorf("failed to create or update BMC account with new password: %w", err) | |
| } | |
| // Update the last rotation time | |
| userBase := user.DeepCopy() | |
| user.Status.LastRotation = &metav1.Time{Time: metav1.Now().Time} | |
| if err := r.Status().Patch(ctx, user, client.MergeFrom(userBase)); err != nil { | |
| return ctrl.Result{}, fmt.Errorf("failed to patch User status with last rotation time: %w", err) | |
| } | |
| // Update BMC account first to ensure credentials are valid before persisting | |
| if err := bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, newPassword, r.Insecure); err != nil { | |
| return ctrl.Result{}, fmt.Errorf("failed to create or update BMC account with new password: %w", err) | |
| } | |
| if err := r.createSecret(ctx, log, user, newPassword); err != nil { | |
| return ctrl.Result{}, fmt.Errorf("failed to create BMCSecret: %w", err) | |
| } | |
| // Update the last rotation time | |
| userBase := user.DeepCopy() | |
| user.Status.LastRotation = &metav1.Time{Time: metav1.Now().Time} | |
| if err := r.Status().Patch(ctx, user, client.MergeFrom(userBase)); err != nil { | |
| return ctrl.Result{}, fmt.Errorf("failed to patch User status with last rotation time: %w", err) | |
| } |
🤖 Prompt for AI Agents
In @internal/controller/bmcuser_controller.go around lines 185 - 196, The
rotation is non-atomic because createSecret(...) runs before
bmcClient.CreateOrUpdateAccount(...), leaving Spec.BMCSecretRef pointing at
creds that may not match the BMC on failure; generate the newPassword first,
call bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID,
newPassword, r.Insecure) before calling r.createSecret(ctx, log, user,
newPassword), and only patch user.Status.LastRotation after both account update
and secret creation succeed; if you prefer to keep the current order instead
implement a rollback: if createSecret succeeds but CreateOrUpdateAccount fails,
delete the newly created secret (or restore the previous secret reference) to
avoid leaving the cluster pointing at invalid credentials.
There was a problem hiding this comment.
The other way round is more critical. If we update the user password on the BMC successfully, but then fail to create the k8s Secret, we will loose the password and possibly the access to our BMC
There was a problem hiding this comment.
@stefanhipfel You make an excellent point! You're absolutely right that losing the password entirely is a more critical failure mode than having a temporary mismatch. If the BMC is updated successfully but the secret creation fails, the randomly generated password would be lost forever, potentially locking out access to that BMC account.
The current order (create secret → update BMC) is safer in that regard because the password is persisted before we touch the BMC.
However, to handle the failure case where the secret is created but the BMC update fails, you might want to consider adding some safeguards:
-
Preserve the old secret: Ensure the old secret (referenced by
Status.EffectiveBMCSecretRef) isn't deleted until the BMC update succeeds. That way, if the BMC update fails, the next reconciliation can still authenticate with the old credentials. -
Status conditions: Add a status condition to track the rotation state (e.g.,
RotationInProgress,RotationFailed) so operators can detect and remediate stuck rotations. -
Exponential backoff: The controller-runtime will already retry on errors, but you might want to add explicit logic to handle transient BMC update failures gracefully.
The current implementation is reasonable given the trade-offs. The most important thing is that you've clearly thought through the failure modes!
🧠 Learnings used
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Prevent hot loops caused by status-only changes triggering reconciliation
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Flag non-idempotent reconcile logic, Spec mutations during reconciliation, and custom patterns duplicating standard Kubernetes behavior
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Reconcile logic must be idempotent and safe to re-run at any time without side effects
afritzler
left a comment
afritzler
left a comment
There was a problem hiding this comment.
Thanks a lot @stefanhipfel for the updates. A few more comments from my side here.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @internal/controller/bmcuser_controller.go:
- Around line 234-262: The createSecret function currently mutates
user.Spec.BMCSecretRef (in BMCUserReconciler.createSecret) which is a spec
anti-pattern; instead, after creating/patching the BMCSecret, do not modify
user.Spec — create a deep copy of the original user, set
user.Status.EffectiveBMCSecretRef = &v1.LocalObjectReference{Name: secret.Name},
and call r.Status().Patch(ctx, user, client.MergeFrom(userBase)) (or r.Patch on
status) to persist the status change; remove the lines that assign to
Spec.BMCSecretRef and update any reconcile logic to prefer
user.Status.EffectiveBMCSecretRef when user.Spec.BMCSecretRef is nil.
- Around line 334-350: The bmcConnectionTest function calls
bmcutils.CreateBMCClient but doesn't close the returned client on success;
capture the returned client (e.g., client or bc) from CreateBMCClient and ensure
you call its appropriate cleanup method (Logout/Close/Destroy) before returning
(use defer immediately after verifying client != nil) so the Redfish session is
terminated; update bmcConnectionTest to defer the client's logout/close after
CreateBMCClient succeeds and ensure nil-checks to avoid panics.
- Around line 86-97: The code may dereference user.Spec.BMCSecretRef.Name after
calling handleMissingBMCSecretRef, which can still leave BMCSecretRef nil; after
handleMissingBMCSecretRef returns successfully, re-fetch the User resource
(using r.Get) or add a defensive check that user.Spec.BMCSecretRef != nil before
accessing .Name; update the code paths around
handleMissingBMCSecretRef/createSecret to either refresh the local user object
or return an explicit error if the ref is still nil, then safely proceed to
populate bmcSecret via r.Get using user.Spec.BMCSecretRef.Name.
- Around line 352-378: The delete function may return without removing the
BMCUserFinalizer causing stuck resources; ensure you always attempt to remove
the finalizer (using clientutils.PatchEnsureNoFinalizer(ctx, r.Client, user,
BMCUserFinalizer)) before every early return path: when user.Spec.BMCRef == nil,
and when the referenced BMC lookup returns NotFound (the client.IgnoreNotFound
branch), and also when getBMCClient fails if appropriate; consolidate the
finalizer removal call so it runs once before returning on error/early-exit,
handle its (modified, err) results correctly (log removal only when modified is
true, and return err if err != nil), and remove or relocate the unreachable log
statement after the finalizer-removal logic so logs reflect actual execution.
- Around line 59-118: The CreateOrUpdateAccount calls are passing r.Insecure for
the account "enabled" flag; replace that with the BMCUser spec value
user.Spec.Enabled. Update every invocation of CreateOrUpdateAccount (referenced
by the method name CreateOrUpdateAccount) from the controller: inside reconcile
and in the helper flows that reconcile triggers (e.g.,
handleMissingBMCSecretRef, handleUpdatedSecretRef, handleRotatingPassword) so
each call uses user.Spec.Enabled instead of r.Insecure. Ensure the user
parameter (user) is in scope where you change the call or pass it through to the
helper so user.Spec.Enabled can be used.
🧹 Nitpick comments (8)
internal/webhook/v1alpha1/bmcsecret_webhook_test.go (6)
17-17: Use lowercase for local variable name.Go convention uses lowercase names for local variables.
BMCSecretshould bebmcSecretto avoid confusion with the type name.♻️ Suggested change
- BMCSecret *metalv1alpha1.BMCSecret + bmcSecret *metalv1alpha1.BMCSecretThen update all references throughout the test accordingly.
25-25: Remove meaningless nil check.
validatoris a struct value, not a pointer. Struct values cannot be nil in Go, making this assertion always pass and provide no value.♻️ Suggested fix
- Expect(validator).NotTo(BeNil(), "Expected validator to be initialized")
35-35: Useptr.To(true)for consistency.The
ptr.To()utility fromk8s.io/utils/ptris already imported and used on line 53. Using it here improves readability and maintains consistency.♻️ Suggested fix
- Immutable: &[]bool{true}[0], + Immutable: ptr.To(true),
37-37: Minor grammar fix."an BMCSecret" should be "a BMCSecret" (BMC starts with a consonant sound).
♻️ Suggested fix
- By("Creating an BMCSecret") + By("Creating a BMCSecret")
43-48: Test logic is correct; minor grammar fix needed.The test correctly validates that updates are denied when
Immutableis true. Grammar: "an BMCSecret" → "a BMCSecret" on line 44.♻️ Suggested fix
- By("Updating an BMCSecret with Immutable set to True") + By("Updating a BMCSecret with Immutable set to True")
50-59: Test logic is correct; minor grammar fix needed.The test properly updates
Immutableto false via the API before validating that data changes are allowed. Grammar: "an BMCSecret" → "a BMCSecret" on line 51.♻️ Suggested fix
- By("Updating an BMCSecret with Immutable set to False") + By("Updating a BMCSecret with Immutable set to False")api/v1alpha1/bmcuser_types.go (1)
12-34: Consider adding validation markers and review theEnabledfield default behavior.A few observations on the spec definition:
Enabledfield (line 30): The boolean defaults tofalse(Go zero value), meaning newly created BMCUser resources will have disabled accounts unless explicitly set. If the typical use case expects enabled accounts, consider using a pointer (*bool) or adding+kubebuilder:default=true.Missing validation markers: Consider adding
+kubebuilder:validation:Requiredfor mandatory fields likeuserNameandroleID, and+kubebuilder:validation:MinLength=1to prevent empty strings.
BMCControllerUsercomment (line 31): The comment should start with the field name per Go conventions: "BMCControllerUser indicates if the user should be used by...".Suggested improvements
// BMCUserSpec defines the desired state of BMCUser. type BMCUserSpec struct { // Username of the BMC user. + // +kubebuilder:validation:Required + // +kubebuilder:validation:MinLength=1 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". + // +kubebuilder:validation:Required + // +kubebuilder:validation:MinLength=1 RoleID string `json:"roleID"` // Description is an optional description for the BMC user. Description string `json:"description,omitempty"` // RotationPeriod defines how often the password should be rotated. // if not set, the password will not be rotated. RotationPeriod *metav1.Duration `json:"rotationPeriod,omitempty"` // BMCSecretRef references the BMCSecret containing the credentials for this user. // If not set, the operator will generate a secure password based on BMC manufacturer requirements. BMCSecretRef *v1.LocalObjectReference `json:"bmcSecretRef,omitempty"` // BMCRef references the BMC this user should be created on. BMCRef *v1.LocalObjectReference `json:"bmcRef,omitempty"` // Enabled indicates whether the user account is enabled. + // +kubebuilder:default=true Enabled bool `json:"enabled"` - // set if the user should be used by the BMC controller to access the system. + // BMCControllerUser indicates if this user should be used by the BMC controller to access the system. // +kubebuilder:default=false BMCControllerUser bool `json:"bmcControllerUser"` }internal/controller/bmcuser_controller.go (1)
334-350: Consider renaming function for clarity on return value semantics.The function
bmcConnectionTestreturns(bool, error)wheretrueindicates invalid credentials. This is counter-intuitive—typically a "test" returningtrueimplies success. The callers at lines 297-304 and 320-330 useinvalidCredentialsvariable name, which helps, but the function signature is still confusing.Consider renaming to
areCredentialsInvalidorcheckCredentialsInvalidto make the boolean semantics explicit.-func (r *BMCUserReconciler) bmcConnectionTest(ctx context.Context, secret *metalv1alpha1.BMCSecret, bmcObj *metalv1alpha1.BMC) (bool, error) { +func (r *BMCUserReconciler) areCredentialsInvalid(ctx context.Context, secret *metalv1alpha1.BMCSecret, bmcObj *metalv1alpha1.BMC) (bool, error) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
dist/chart/templates/crd/metal.ironcore.dev_bmcusers.yamlis excluded by!**/dist/**
📒 Files selected for processing (5)
api/v1alpha1/bmcuser_types.goconfig/crd/bases/metal.ironcore.dev_bmcusers.yamldocs/api-reference/api.mdinternal/controller/bmcuser_controller.gointernal/webhook/v1alpha1/bmcsecret_webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/api-reference/api.md
- config/crd/bases/metal.ironcore.dev_bmcusers.yaml
🧰 Additional context used
📓 Path-based instructions (4)
**/api/**/*_types.go
📄 CodeRabbit inference engine (AGENTS.md)
**/api/**/*_types.go: API type fields must follow Kubernetes naming conventions: camelCase in JSON, PascalCase in Go structs
Usemetav1.Time,metav1.Condition, andresource.Quantityfor standard Kubernetes types in API definitions
Spec fields must be declarative and user-facing; Status fields must be controller-owned
Conditions in Status must follow Kubernetes conventions with stable Type, correct Status, Reason, and Message fields
No mutable fields in Spec that belong in Status; maintain clear separation between desired state and observed state
Do not make breaking changes to existing API fields without versioning; ensure additive-only changes for existing API versions
Clearly mark and document deprecated fields in API types
Files:
api/v1alpha1/bmcuser_types.go
**/api/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Implement defaulting and validation via webhooks (preferred) or OpenAPI schema annotations
Files:
api/v1alpha1/bmcuser_types.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Code must follow standard Go formatting and idioms; use clear, explicit error handling with minimal global state
Use small, focused functions; introduce interfaces only when justified
Files:
api/v1alpha1/bmcuser_types.gointernal/webhook/v1alpha1/bmcsecret_webhook_test.gointernal/controller/bmcuser_controller.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Tests must be deterministic and not rely on timing assumptions
Files:
internal/webhook/v1alpha1/bmcsecret_webhook_test.go
🧠 Learnings (11)
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/api/**/*_types.go : Spec fields must be declarative and user-facing; Status fields must be controller-owned
Applied to files:
api/v1alpha1/bmcuser_types.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/api/**/*_types.go : Use `metav1.Time`, `metav1.Condition`, and `resource.Quantity` for standard Kubernetes types in API definitions
Applied to files:
api/v1alpha1/bmcuser_types.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/api/**/*_types.go : No mutable fields in Spec that belong in Status; maintain clear separation between desired state and observed state
Applied to files:
api/v1alpha1/bmcuser_types.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/api/**/*.go : Implement defaulting and validation via webhooks (preferred) or OpenAPI schema annotations
Applied to files:
api/v1alpha1/bmcuser_types.gointernal/webhook/v1alpha1/bmcsecret_webhook_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/api/**/*_types.go : Conditions in Status must follow Kubernetes conventions with stable Type, correct Status, Reason, and Message fields
Applied to files:
api/v1alpha1/bmcuser_types.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/*_test.go : Tests must be deterministic and not rely on timing assumptions
Applied to files:
internal/webhook/v1alpha1/bmcsecret_webhook_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_test.go : Use fake clients appropriately in tests
Applied to files:
internal/webhook/v1alpha1/bmcsecret_webhook_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Flag non-idempotent reconcile logic, Spec mutations during reconciliation, and custom patterns duplicating standard Kubernetes behavior
Applied to files:
internal/controller/bmcuser_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Use `client.Client`, `controllerutil.CreateOrUpdate`, and OwnerReferences from controller-runtime rather than direct API calls
Applied to files:
internal/controller/bmcuser_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Prevent hot loops caused by status-only changes triggering reconciliation
Applied to files:
internal/controller/bmcuser_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*.go : Include relevant identifiers in logs; emit events for meaningful user-facing state changes
Applied to files:
internal/controller/bmcuser_controller.go
🧬 Code graph analysis (3)
api/v1alpha1/bmcuser_types.go (1)
api/v1alpha1/groupversion_info.go (1)
SchemeBuilder(19-19)
internal/webhook/v1alpha1/bmcsecret_webhook_test.go (2)
api/v1alpha1/bmcsecret_types.go (1)
BMCSecret(24-57)internal/webhook/v1alpha1/bmcsecret_webhook.go (1)
BMCSecretCustomValidator(32-34)
internal/controller/bmcuser_controller.go (8)
bmc/redfish.go (2)
Options(43-53)GenerateSecurePassword(1108-1161)api/v1alpha1/bmcuser_types.go (1)
BMCUser(54-60)api/v1alpha1/bmc_types.go (2)
BMC(257-263)Protocol(127-138)internal/webhook/v1alpha1/bmcsecret_webhook_test.go (1)
BMCSecret(17-17)internal/bmcutils/bmcutils.go (5)
GetBMCCredentialsFromSecret(49-60)GetBMCClientFromBMC(147-188)GetProtocolScheme(39-47)GetBMCAddressForBMC(93-109)CreateBMCClient(190-229)api/v1alpha1/constants.go (2)
OperationAnnotation(21-21)OperationAnnotationRotateCredentials(45-45)bmc/bmc.go (2)
Manufacturer(19-19)Manager(301-312)api/v1alpha1/bmcsecret_types.go (2)
BMCSecretUsernameKeyName(13-13)BMCSecretPasswordKeyName(15-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run test e2e
- GitHub Check: Run test chart
- GitHub Check: Run tests
🔇 Additional comments (5)
internal/webhook/v1alpha1/bmcsecret_webhook_test.go (1)
1-13: LGTM!License header and imports are appropriate for a Ginkgo/Gomega webhook test.
api/v1alpha1/bmcuser_types.go (2)
36-47: LGTM - Status struct follows proper conventions.The status fields correctly use
metav1.Timefor timestamps andv1.LocalObjectReferencefor secret references. All fields are appropriately marked as optional withomitempty. The separation between Spec (user-desired state) and Status (controller-observed state) is correct.
49-73: LGTM - CRD type definitions and registration are correct.The kubebuilder markers are properly configured for a cluster-scoped resource with status subresource. The scheme registration follows the standard pattern.
internal/controller/bmcuser_controller.go (2)
120-144: LGTM - Status patching follows correct patterns.The status patching uses
client.MergeFromcorrectly. Error handling for password expiration parsing is reasonable—logging the error and continuing rather than failing the entire reconciliation for an optional field.
380-387: LGTM - Controller setup is correct.The controller setup properly watches
BMCUserresources and ownsBMCSecretresources, which is appropriate since the controller creates BMCSecrets with owner references. The named controller "bmcuser" follows conventions.
| if user.Status.EffectiveBMCSecretRef != nil && user.Spec.BMCSecretRef.Name != user.Status.EffectiveBMCSecretRef.Name { | ||
| log.Info("BMCSecret reference has changed, updating BMC account", "User", user.Name) | ||
| if err := r.handleUpdatedSecretRef(ctx, log, user, bmcSecret, bmcClient); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to handle updated BMCSecret reference: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing status update after secret reference change may cause repeated updates.
After handleUpdatedSecretRef updates the BMC account password, Status.EffectiveBMCSecretRef is not updated to match Spec.BMCSecretRef. On subsequent reconciliations, the condition at line 111 will continue to detect a mismatch, potentially causing repeated BMC account updates until updateEffectiveSecret (line 70) updates the status under specific conditions.
Consider updating Status.EffectiveBMCSecretRef in handleUpdatedSecretRef after successfully updating the BMC account:
Suggested fix
func (r *BMCUserReconciler) handleUpdatedSecretRef(ctx context.Context, log logr.Logger, user *metalv1alpha1.BMCUser, bmcSecret *metalv1alpha1.BMCSecret, bmcClient bmc.BMC) error {
log.Info("BMCSecret credentials have changed, updating BMC user", "User", user.Name)
_, password, err := bmcutils.GetBMCCredentialsFromSecret(bmcSecret)
if err != nil {
return fmt.Errorf("failed to get credentials from BMCSecret: %w", err)
}
// Update the BMC account with the new password
if err := bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, password, r.Insecure); err != nil {
return fmt.Errorf("failed to create or update BMC account with new password: %w", err)
}
+ // Update the effective secret reference to match the new spec
+ if err := r.setEffectiveSecretRef(ctx, log, user, bmcSecret); err != nil {
+ return fmt.Errorf("failed to update effective BMCSecretRef: %w", err)
+ }
return nil
}Also applies to: 221-232
| func (r *BMCUserReconciler) createSecret(ctx context.Context, log logr.Logger, user *metalv1alpha1.BMCUser, password string) error { | ||
| log.Info("Creating BMCSecret for User", "User", user.Name) | ||
| secret := &metalv1alpha1.BMCSecret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| GenerateName: user.Name + "-bmcsecret-", | ||
| }, | ||
| Data: map[string][]byte{ | ||
| metalv1alpha1.BMCSecretUsernameKeyName: []byte(user.Spec.UserName), | ||
| metalv1alpha1.BMCSecretPasswordKeyName: []byte(password), | ||
| }, | ||
| Immutable: &[]bool{true}[0], // Make the secret immutable | ||
| } | ||
| op, err := controllerutil.CreateOrPatch(ctx, r.Client, secret, func() error { | ||
| if err := controllerutil.SetControllerReference(user, secret, r.Scheme); err != nil { | ||
| return fmt.Errorf("failed to set controller reference for BMCSecret: %w", err) | ||
| } | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create or patch BMCSecret: %w", err) | ||
| } | ||
| log.V(1).Info("BMCSecret created or patched", "BMCSecret", secret.Name, "Operation", op) | ||
| userBase := user.DeepCopy() | ||
| user.Spec.BMCSecretRef = &v1.LocalObjectReference{Name: secret.Name} | ||
| if err := r.Patch(ctx, user, client.MergeFrom(userBase)); err != nil { | ||
| return fmt.Errorf("failed to patch User status with effective BMCSecretRef: %w", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Spec mutation anti-pattern: Controller should not modify Spec.BMCSecretRef.
The controller modifies user.Spec.BMCSecretRef at lines 256-259. This violates the Kubernetes convention that Spec fields are user-owned (declarative desired state) while Status fields are controller-owned (observed state). Based on coding guidelines, "Spec fields must be declarative and user-facing; Status fields must be controller-owned."
The BMCUserStatus.EffectiveBMCSecretRef field already exists for this purpose. Instead of patching Spec, the controller should:
- Generate the secret
- Set
Status.EffectiveBMCSecretRefto reference the generated secret - Use
Status.EffectiveBMCSecretRefthroughout reconciliation whenSpec.BMCSecretRefis nil
This also improves idempotency—currently, if reconciliation is interrupted after the Spec patch but before the BMC account creation, the next reconciliation may behave unexpectedly.
Suggested refactor
func (r *BMCUserReconciler) createSecret(ctx context.Context, log logr.Logger, user *metalv1alpha1.BMCUser, password string) error {
log.Info("Creating BMCSecret for User", "User", user.Name)
secret := &metalv1alpha1.BMCSecret{
ObjectMeta: metav1.ObjectMeta{
GenerateName: user.Name + "-bmcsecret-",
},
Data: map[string][]byte{
metalv1alpha1.BMCSecretUsernameKeyName: []byte(user.Spec.UserName),
metalv1alpha1.BMCSecretPasswordKeyName: []byte(password),
},
Immutable: &[]bool{true}[0],
}
- op, err := controllerutil.CreateOrPatch(ctx, r.Client, secret, func() error {
+ if err := controllerutil.SetControllerReference(user, secret, r.Scheme); err != nil {
+ return fmt.Errorf("failed to set controller reference for BMCSecret: %w", err)
+ }
+ if err := r.Create(ctx, secret); err != nil {
+ return fmt.Errorf("failed to create BMCSecret: %w", err)
+ }
+ log.V(1).Info("BMCSecret created", "BMCSecret", secret.Name)
+ // Update Status, not Spec
+ if err := r.setEffectiveSecretRef(ctx, log, user, secret); err != nil {
+ return fmt.Errorf("failed to set effective BMCSecretRef in status: %w", err)
+ }
- if err := controllerutil.SetControllerReference(user, secret, r.Scheme); err != nil {
- return fmt.Errorf("failed to set controller reference for BMCSecret: %w", err)
- }
- return nil
- })
- if err != nil {
- return fmt.Errorf("failed to create or patch BMCSecret: %w", err)
- }
- log.V(1).Info("BMCSecret created or patched", "BMCSecret", secret.Name, "Operation", op)
- userBase := user.DeepCopy()
- user.Spec.BMCSecretRef = &v1.LocalObjectReference{Name: secret.Name}
- if err := r.Patch(ctx, user, client.MergeFrom(userBase)); err != nil {
- return fmt.Errorf("failed to patch User status with effective BMCSecretRef: %w", err)
- }
return nil
}Then update the reconcile logic to use Status.EffectiveBMCSecretRef when Spec.BMCSecretRef is nil.
🤖 Prompt for AI Agents
In @internal/controller/bmcuser_controller.go around lines 234 - 262, The
createSecret function currently mutates user.Spec.BMCSecretRef (in
BMCUserReconciler.createSecret) which is a spec anti-pattern; instead, after
creating/patching the BMCSecret, do not modify user.Spec — create a deep copy of
the original user, set user.Status.EffectiveBMCSecretRef =
&v1.LocalObjectReference{Name: secret.Name}, and call r.Status().Patch(ctx,
user, client.MergeFrom(userBase)) (or r.Patch on status) to persist the status
change; remove the lines that assign to Spec.BMCSecretRef and update any
reconcile logic to prefer user.Status.EffectiveBMCSecretRef when
user.Spec.BMCSecretRef is nil.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@bmc/redfish.go`:
- Around line 788-794: The Delete call currently discards the *http.Response and
leaks its Body; update the block that calls r.client.Delete(a.ODataID) (inside
the account deletion check for a.UserName==userName && a.ID==id) to capture the
response (resp, err := r.client.Delete(...)), ensure the body is closed on all
paths (defer resp.Body.Close() after a non-nil resp), and on error ensure any
non-nil resp.Body is closed before returning; optionally drain the body
(io.Copy(io.Discard, resp.Body)) before closing to allow connection reuse.
In `@internal/controller/suite_test.go`:
- Around line 266-275: Add a ManagerNamespace string field to the
BMCUserReconciler struct and ensure createSecret uses it when constructing
BMCSecret objects (set the Secret.ObjectMeta.Namespace = r.ManagerNamespace); in
the test instantiate BMCUserReconciler with ManagerNamespace set (e.g.,
k8sManager.GetNamespace() or the same namespace used by other reconcilers) so
SetupWithManager uses the correct namespace; mirror the pattern used by
BMCSettingsReconciler/BIOSVersionReconciler/BMCReconciler and ensure any
test-created secrets specify that namespace.
♻️ Duplicate comments (2)
bmc/redfish.go (2)
771-774: Incorrect error message for account creation.Line 773 says "failed to update account" but this code path handles
CreateAccount. This was flagged in a previous review.🐛 Fix error message
_, err = service.CreateAccount(userName, password, role) if err != nil { - return fmt.Errorf("failed to update account: %w", err) + return fmt.Errorf("failed to create account: %w", err) }
800-806: Returnnilon error for consistent Go error handling.When
AccountService()fails, returning(service, error)may provide a nil or invalid service to callers. This was flagged in a previous review.🔧 Suggested fix
func (r *RedfishBMC) GetAccountService(ctx context.Context) (*redfish.AccountService, error) { service, err := r.client.GetService().AccountService() if err != nil { - return service, fmt.Errorf("failed to get account service: %w", err) + return nil, fmt.Errorf("failed to get account service: %w", err) } return service, nil }
🧹 Nitpick comments (2)
PROJECT (1)
148-155: LGTM - BMCUser resource scaffolding.The BMCUser resource is correctly configured as a cluster-scoped resource (no
namespaced: true), consistent with other BMC-related resources in the project.Consider whether a validation webhook should be added for BMCUser in a follow-up, similar to other resources like BMCSettings and BMCVersion, to enforce constraints on fields like
roleIDoruserName.bmc/redfish.go (1)
1095-1108: Double%character and type inconsistency.Two issues:
Line 1097:
%%in a regular Go string literal is literally two%characters. If you intended a single%, use just%.Line 1105: Using the string literal
"default"as a map key is inconsistent with theManufacturertype. Consider defining a constant likeManufacturerDefault Manufacturer = "default"for type safety.♻️ Proposed fix
var manufacturerPasswordConfigs = map[Manufacturer]ManufacturerPasswordConfig{ ManufacturerDell: { - SpecialChars: "!#$%%&()*.?-@[]^_`{}|~+=", + SpecialChars: "!#$%&()*.?-@[]^_`{}|~+=", }, ManufacturerHPE: { SpecialChars: "~`!@#$%^&*()_-+={[}]|.?/", }, ManufacturerLenovo: { SpecialChars: ";@!$%-+=[]{}|/?~_", }, - "default": { + ManufacturerDefault: { SpecialChars: "!@#$%&*()_-+=[]{}/?~|", }, }Add to
bmc/bmc.go:ManufacturerDefault Manufacturer = "default"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
PROJECTapi/v1alpha1/zz_generated.deepcopy.gobmc/redfish.gocmd/manager/main.goconfig/crd/kustomization.yamlconfig/rbac/kustomization.yamlconfig/samples/kustomization.yamldocs/api-reference/api.mdinternal/controller/suite_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- config/samples/kustomization.yaml
- config/rbac/kustomization.yaml
- cmd/manager/main.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Code must follow standard Go formatting and idioms; use clear, explicit error handling with minimal global state
Use small, focused functions; introduce interfaces only when justified
Files:
bmc/redfish.goapi/v1alpha1/zz_generated.deepcopy.gointernal/controller/suite_test.go
**/api/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Implement defaulting and validation via webhooks (preferred) or OpenAPI schema annotations
Files:
api/v1alpha1/zz_generated.deepcopy.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Tests must be deterministic and not rely on timing assumptions
Files:
internal/controller/suite_test.go
🧠 Learnings (10)
📚 Learning: 2026-01-14T19:27:09.972Z
Learnt from: Nuckal777
Repo: ironcore-dev/metal-operator PR: 471
File: internal/controller/bmcsettingsset_controller.go:233-236
Timestamp: 2026-01-14T19:27:09.972Z
Learning: In the ironcore-dev/metal-operator repository, BMCSettings.Spec.BMCRef is marked with +required kubebuilder marker, ensuring API-level validation that prevents nil values; defensive nil checks are not needed when iterating BMCSettings from List operations.
Applied to files:
docs/api-reference/api.mdconfig/crd/kustomization.yamlPROJECT
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/api/**/*_types.go : Clearly mark and document deprecated fields in API types
Applied to files:
api/v1alpha1/zz_generated.deepcopy.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller_test.go : Write unit tests for reconcile logic and pure functions
Applied to files:
internal/controller/suite_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*.go : Include relevant identifiers in logs; emit events for meaningful user-facing state changes
Applied to files:
internal/controller/suite_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller_test.go : Use Envtest-based tests for controller behavior and API interactions
Applied to files:
internal/controller/suite_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Flag non-idempotent reconcile logic, Spec mutations during reconciliation, and custom patterns duplicating standard Kubernetes behavior
Applied to files:
internal/controller/suite_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*.go : Use structured logging with controller-runtime logger; avoid excessive log noise in hot paths
Applied to files:
internal/controller/suite_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Prevent hot loops caused by status-only changes triggering reconciliation
Applied to files:
internal/controller/suite_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Avoid silent failures; ensure logging includes sufficient context (namespaced name, resource identifiers)
Applied to files:
internal/controller/suite_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/api/**/*.go : Implement defaulting and validation via webhooks (preferred) or OpenAPI schema annotations
Applied to files:
PROJECT
🧬 Code graph analysis (3)
bmc/redfish.go (1)
bmc/bmc.go (4)
Manufacturer(19-19)ManufacturerDell(22-22)ManufacturerHPE(24-24)ManufacturerLenovo(23-23)
api/v1alpha1/zz_generated.deepcopy.go (1)
api/v1alpha1/bmcuser_types.go (4)
BMCUser(54-60)BMCUserList(65-69)BMCUserSpec(12-34)BMCUserStatus(37-47)
internal/controller/suite_test.go (2)
internal/controller/bmcuser_controller.go (1)
BMCUserReconciler(31-36)bmc/redfish.go (1)
Options(43-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run tests
- GitHub Check: Run test e2e
- GitHub Check: Run test chart
- GitHub Check: Run linter
🔇 Additional comments (11)
config/crd/kustomization.yaml (2)
19-19: LGTM!The BMCUser CRD base resource is correctly added to the kustomization resources list, aligning with the new BMCUser API type introduced in this PR.
42-42: Clarify the commented CA injection patch naming.The commented patch references
cainjection_in_accounts.yaml, but there's no corresponding "accounts" CRD in the resources list. Should this becainjection_in_bmcusers.yamlto match the new BMCUser CRD, or is this an unrelated placeholder?internal/controller/suite_test.go (1)
133-133: LGTM!Adding the
GinkgoLogrlogger to the manager improves test observability and debugging capabilities.api/v1alpha1/zz_generated.deepcopy.go (1)
944-1059: LGTM - Auto-generated code.The deepcopy implementations for
BMCUser,BMCUserList,BMCUserSpec, andBMCUserStatusare correctly generated bycontroller-genand properly handle all pointer fields with appropriate nil checks and deep copy semantics.docs/api-reference/api.md (2)
22-22: LGTM!The
BMCUserresource is correctly added to the Resource Types list, maintaining alphabetical ordering within the BMC-prefixed resources.
674-733: LGTM - API documentation now includes field descriptions.The BMCUser API documentation comprehensively covers:
BMCUserSpec: userName, roleID (with Redfish role examples), description, rotationPeriod, bmcSecretRef, bmcRef, enabled, and bmcControllerUser fieldsBMCUserStatus: effectiveBMCSecretRef, lastRotation, passwordExpiration, and id fieldsAll fields now have meaningful descriptions that explain their purpose and behavior.
PROJECT (1)
32-34: LGTM!The BMCSecret webhook configuration is correctly added, enabling validation webhooks for BMCSecret resources. This aligns with the webhook implementation in
internal/webhook/v1alpha1/bmcsecret_webhook.go.bmc/redfish.go (4)
84-89: LGTM!Enabling
ReuseConnections: trueis a sensible optimization for reducing connection overhead when making multiple Redfish API calls.
808-818: LGTM!The implementation correctly retrieves accounts with proper error handling.
1110-1164: Solid password generation implementation.The algorithm correctly ensures complexity by mandating one character from each category (lowercase, uppercase, digit, special), then fills the remainder and shuffles using crypto-secure randomness. The length validation at line 1127 correctly rejects lengths below the minimum required for complexity.
Note: Line 1114 uses the same string literal
"default"issue mentioned above.
1166-1184: LGTM!Both helper functions correctly use
crypto/randfor cryptographically secure randomness. The Fisher-Yates shuffle implementation is correct and unbiased.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| Expect((&BMCUserReconciler{ | ||
| Client: k8sManager.GetClient(), | ||
| Scheme: k8sManager.GetScheme(), | ||
| Insecure: true, | ||
| BMCOptions: bmc.Options{ | ||
| PowerPollingInterval: 50 * time.Millisecond, | ||
| PowerPollingTimeout: 200 * time.Millisecond, | ||
| BasicAuth: true, | ||
| }, | ||
| }).SetupWithManager(k8sManager)).To(Succeed()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check BMCUserReconciler struct definition
echo "=== BMCUserReconciler struct definition ==="
ast-grep --pattern $'type BMCUserReconciler struct {
$$$
}'
# Check if ManagerNamespace is mentioned in bmcuser_controller.go
echo -e "\n=== ManagerNamespace usage in bmcuser_controller.go ==="
rg -n "ManagerNamespace" internal/controller/bmcuser_controller.go || echo "No ManagerNamespace found"
# Check other reconcilers setup in suite_test.go
echo -e "\n=== Other reconcilers setup in suite_test.go ==="
rg -n "Reconciler\{" internal/controller/suite_test.go -A 8 | head -80Repository: ironcore-dev/metal-operator
Length of output: 3708
🏁 Script executed:
#!/bin/bash
# Check BMCUserReconciler's Reconcile method
echo "=== BMCUserReconciler Reconcile method ==="
rg -n "func.*Reconcile" internal/controller/bmcuser_controller.go -A 30
# Check if BMCSecret is accessed
echo -e "\n=== BMCSecret access in bmcuser_controller.go ==="
rg -n "BMCSecret\|Secret" internal/controller/bmcuser_controller.go -B 2 -A 2
# Check other reconcilers' Reconcile to see how they use ManagerNamespace
echo -e "\n=== How ManagerNamespace is used in another reconciler ==="
rg -n "ManagerNamespace" internal/controller/bmcsettings_controller.go -B 3 -A 3Repository: ironcore-dev/metal-operator
Length of output: 14488
Add ManagerNamespace field to BMCUserReconciler struct and use it when creating/accessing namespaced resources.
The BMCUserReconciler is missing the ManagerNamespace field present in other reconcilers (BMCSettingsReconciler, BIOSVersionReconciler, BMCReconciler). More critically, the createSecret method creates BMCSecret objects without specifying a namespace, causing them to default to the "default" namespace instead of the manager's namespace. This mirrors how BMCSettingsReconciler uses ManagerNamespace when creating ServerMaintenance objects. The test setup should include this field for consistency and correctness.
🤖 Prompt for AI Agents
In `@internal/controller/suite_test.go` around lines 266 - 275, Add a
ManagerNamespace string field to the BMCUserReconciler struct and ensure
createSecret uses it when constructing BMCSecret objects (set the
Secret.ObjectMeta.Namespace = r.ManagerNamespace); in the test instantiate
BMCUserReconciler with ManagerNamespace set (e.g., k8sManager.GetNamespace() or
the same namespace used by other reconcilers) so SetupWithManager uses the
correct namespace; mirror the pattern used by
BMCSettingsReconciler/BIOSVersionReconciler/BMCReconciler and ensure any
test-created secrets specify that namespace.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@bmc/redfish.go`:
- Around line 1101-1114: The Dell special-character string in
manufacturerPasswordConfigs for ManufacturerDell contains an accidental double
percent ("% %") — update the SpecialChars value in the ManufacturerDell entry to
use a single '%' (i.e., change "$%%&" to "$%&"). Also replace the literal
"default" map key with a typed constant (e.g., define ManufacturerDefault of
type Manufacturer and use it as the key) so the default entry matches the
Manufacturer type used by other keys; ensure ManufacturerDefault is declared
before manufacturerPasswordConfigs.
- Around line 791-800: The Delete call may return a non-nil resp together with
err so the current code leaks resp.Body on the error path; update the block
around r.client.Delete(a.ODataID) to check if resp != nil immediately after the
call and close resp.Body (or defer closing) before returning the error, i.e.,
ensure resp.Body is closed both when err != nil and when returning nil;
reference r.client.Delete, resp, resp.Body and the existing deferred close to
implement this change.
| resp, err := r.client.Delete(a.ODataID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer func(Body io.ReadCloser) { | ||
| if err = Body.Close(); err != nil { | ||
| log.Error(err, "failed to close response body") | ||
| } | ||
| }(resp.Body) | ||
| return nil |
There was a problem hiding this comment.
Potential resource leak if Delete returns both response and error.
Some HTTP clients return a non-nil response alongside an error (e.g., to allow reading error details from the body). If r.client.Delete behaves this way, the response body won't be closed on the error path at line 793.
🐛 Proposed fix
if a.UserName == userName && a.ID == id {
resp, err := r.client.Delete(a.ODataID)
+ if resp != nil {
+ defer func(Body io.ReadCloser) {
+ if closeErr := Body.Close(); closeErr != nil {
+ log.Error(closeErr, "failed to close response body")
+ }
+ }(resp.Body)
+ }
if err != nil {
return err
}
- defer func(Body io.ReadCloser) {
- if err = Body.Close(); err != nil {
- log.Error(err, "failed to close response body")
- }
- }(resp.Body)
return nil
}🤖 Prompt for AI Agents
In `@bmc/redfish.go` around lines 791 - 800, The Delete call may return a non-nil
resp together with err so the current code leaks resp.Body on the error path;
update the block around r.client.Delete(a.ODataID) to check if resp != nil
immediately after the call and close resp.Body (or defer closing) before
returning the error, i.e., ensure resp.Body is closed both when err != nil and
when returning nil; reference r.client.Delete, resp, resp.Body and the existing
deferred close to implement this change.
| var manufacturerPasswordConfigs = map[Manufacturer]ManufacturerPasswordConfig{ | ||
| ManufacturerDell: { | ||
| SpecialChars: "!#$%%&()*.?-@[]^_`{}|~+=", | ||
| }, | ||
| ManufacturerHPE: { | ||
| SpecialChars: "~`!@#$%^&*()_-+={[}]|.?/", | ||
| }, | ||
| ManufacturerLenovo: { | ||
| SpecialChars: ";@!$%-+=[]{}|/?~_", | ||
| }, | ||
| "default": { | ||
| SpecialChars: "!@#$%&*()_-+=[]{}/?~|", | ||
| }, | ||
| } |
There was a problem hiding this comment.
Double % in Dell special characters appears unintentional.
Line 1103 has $%%& which includes two consecutive % characters. In Go strings, % doesn't require escaping, so this likely should be a single %.
🐛 Proposed fix
var manufacturerPasswordConfigs = map[Manufacturer]ManufacturerPasswordConfig{
ManufacturerDell: {
- SpecialChars: "!#$%%&()*.?-@[]^_`{}|~+=",
+ SpecialChars: "!#$%&()*.?-@[]^_`{}|~+=",
},Additionally, consider defining a ManufacturerDefault constant for the "default" key to maintain type consistency with other Manufacturer values.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var manufacturerPasswordConfigs = map[Manufacturer]ManufacturerPasswordConfig{ | |
| ManufacturerDell: { | |
| SpecialChars: "!#$%%&()*.?-@[]^_`{}|~+=", | |
| }, | |
| ManufacturerHPE: { | |
| SpecialChars: "~`!@#$%^&*()_-+={[}]|.?/", | |
| }, | |
| ManufacturerLenovo: { | |
| SpecialChars: ";@!$%-+=[]{}|/?~_", | |
| }, | |
| "default": { | |
| SpecialChars: "!@#$%&*()_-+=[]{}/?~|", | |
| }, | |
| } | |
| var manufacturerPasswordConfigs = map[Manufacturer]ManufacturerPasswordConfig{ | |
| ManufacturerDell: { | |
| SpecialChars: "!#$%&()*.?-@[]^_`{}|~+=", | |
| }, | |
| ManufacturerHPE: { | |
| SpecialChars: "~`!@#$%^&*()_-+={[}]|.?/", | |
| }, | |
| ManufacturerLenovo: { | |
| SpecialChars: ";@!$%-+=[]{}|/?~_", | |
| }, | |
| "default": { | |
| SpecialChars: "!@#$%&*()_-+=[]{}/?~|", | |
| }, | |
| } |
🤖 Prompt for AI Agents
In `@bmc/redfish.go` around lines 1101 - 1114, The Dell special-character string
in manufacturerPasswordConfigs for ManufacturerDell contains an accidental
double percent ("% %") — update the SpecialChars value in the ManufacturerDell
entry to use a single '%' (i.e., change "$%%&" to "$%&"). Also replace the
literal "default" map key with a typed constant (e.g., define
ManufacturerDefault of type Manufacturer and use it as the key) so the default
entry matches the Manufacturer type used by other keys; ensure
ManufacturerDefault is declared before manufacturerPasswordConfigs.
stefanhipfel
force-pushed
the
accounts
branch
from
fcda415 to
5d8038d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@config/crd/kustomization.yaml`:
- Line 42: The kustomization patch entry references a non-existent file
"cainjection_in_accounts.yaml"; update the patch path in
config/crd/kustomization.yaml so the commented line (currently "#- path:
patches/cainjection_in_accounts.yaml") instead references the correct BMCUsers
patch file "patches/cainjection_in_bmcusers.yaml" to match the BMCUsers resource
declared earlier (the resource at line 19) and follow the existing
cainjection_in_<resource>.yaml naming pattern.
In `@config/rbac/bmcuser_admin_role.yaml`:
- Around line 16-21: Replace the wildcard verbs entry under the RBAC rule for
apiGroups: metal.ironcore.dev and resources: bmcusers by enumerating explicit
actions to follow least-privilege; update the verbs list (the verbs field) to
include only create, delete, get, list, patch, update, and watch (and add any
other specific verbs you actually need) instead of '*' so policy scanners and
auditors accept the bmcusers rule.
In `@internal/webhook/v1alpha1/bmcsecret_webhook.go`:
- Around line 61-68: The immutability check only runs when the new object has
Immutable=true, allowing clients to unset Immutable and mutate Data/StringData;
update the logic in the webhook (referencing bmcsecret.Immutable,
oldSecret.Immutable, bmcsecret.Data, oldSecret.Data, bmcsecret.StringData,
oldSecret.StringData) to enforce immutability if the old resource was immutable
or the new one is set immutable — i.e., treat immutability as sticky by checking
if (oldSecret.Immutable != nil && *oldSecret.Immutable) || (bmcsecret.Immutable
!= nil && *bmcsecret.Immutable) before comparing Data/StringData and
additionally reject attempts to change Immutable from true to false (return an
error when oldSecret.Immutable is true and bmcsecret.Immutable is nil or false).
In `@REUSE.toml`:
- Line 26: The REUSE.toml annotation block currently lists "test/data/*.json"
but omits "test/data/*.yaml", leaving test/data/dns_record_template.yaml without
SPDX metadata; fix by either (A) adding the pattern "test/data/*.yaml" back into
the REUSE.toml annotation block so all YAML in test/data is covered, or (B)
adding SPDX headers to the YAML file itself (e.g., SPDX-FileCopyrightText and
SPDX-License-Identifier lines at the top of test/data/dns_record_template.yaml)
so the file has explicit licensing information.
♻️ Duplicate comments (12)
bmc/redfish_local.go (2)
68-76: Use direct map lookup instead of iteration.The map is keyed by
userName, so a direct lookupUnitTestMockUps.Accounts[userName]is more efficient than iterating over all values.♻️ Suggested refactor
func (r *RedfishLocalBMC) CreateOrUpdateAccount( ctx context.Context, userName, role, password string, enabled bool, ) error { - for _, a := range UnitTestMockUps.Accounts { - if a.UserName == userName { - a.RoleID = role - a.UserName = userName - a.Enabled = enabled - a.Password = password - return nil - } - } + if a, ok := UnitTestMockUps.Accounts[userName]; ok { + a.RoleID = role + a.Enabled = enabled + a.Password = password + return nil + } newAccount := redfish.ManagerAccount{
90-96: Theidparameter is unused.The
DeleteAccountmethod accepts anidparameter (passed asuser.Status.IDfrom the controller) but only usesuserNamefor deletion. Consider either validating thatidmatches the account being deleted, or adding a comment documenting thatidis intentionally ignored in the local mock implementation.api/v1alpha1/bmcuser_types.go (1)
11-28: Add basic validation/defaulting for BMCUserSpec fields (UserName, RoleID, RotationPeriod, refs).Consider OpenAPI/kubebuilder validation markers or a validating webhook to prevent invalid CRs (e.g., empty usernames/roles or zero/negative rotation periods).
As per coding guidelines, add validation/defaulting for API types.bmc/redfish.go (2)
1132-1144: Dell special chars include a double “%”; consider a typed default key.The Dell string has
%%(likely unintended), and"default"is a magic string for aManufacturer-typed map key.
804-812: Close response bodies even on the error path from DeleteAccount.If
r.client.Deletereturns a non‑nil response alongside an error, the body is leaked. Close it before returning.🐛 Suggested fix
resp, err := r.client.Delete(a.ODataID) - if err != nil { - return err - } + if resp != nil { + defer func(Body io.ReadCloser) { + if err = Body.Close(); err != nil { + log.Error(err, "failed to close response body") + } + }(resp.Body) + } + if err != nil { + return err + } - defer func(Body io.ReadCloser) { - if err = Body.Close(); err != nil { - log.Error(err, "failed to close response body") - } - }(resp.Body) return nilinternal/webhook/v1alpha1/bmcsecret_webhook_test.go (1)
35-45: Use ptr.To(true) and fix “an BMCSecret” grammar.Prefer
ptr.To(true)over&[]bool{true}[0], and update “an BMCSecret” → “a BMCSecret.”🔧 Suggested tweak
- Immutable: &[]bool{true}[0], + Immutable: ptr.To(true), } - By("Creating an BMCSecret") + By("Creating a BMCSecret")cmd/manager/main.go (1)
550-563: Use “BMCUser” as the controller name in the setup error log.This log label is inconsistent with other controllers and the reconciler name.
🔧 Suggested fix
- setupLog.Error(err, "unable to create controller", "controller", "User") + setupLog.Error(err, "unable to create controller", "controller", "BMCUser")internal/controller/bmcuser_controller_test.go (1)
126-135: Avoid explicit deletion of GC-owned effective secrets in tests.These secrets are owner-referenced by the BMCUser, so explicit deletes can race with GC and intermittently fail. Prefer asserting deletion via
apierrors.IsNotFound, and avoidShouldNot(Succeed())which can mask real errors.🧪 Suggested pattern (apply similarly to the other blocks)
- Expect(k8sClient.Delete(ctx, effectiveSecret)).To(Succeed()) - Eventually(Get(effectiveSecret)).Should(Satisfy(apierrors.IsNotFound)) + Eventually(Get(effectiveSecret)).Should(Satisfy(apierrors.IsNotFound))- Eventually(Get(user)).ShouldNot(Succeed()) + Eventually(Get(user)).Should(Satisfy(apierrors.IsNotFound))Also applies to: 235-241, 280-282, 320-328, 388-395
internal/controller/bmcuser_controller.go (4)
106-118: Respectspec.enabledwhen creating/updating BMC accounts.All
CreateOrUpdateAccountcalls hardcodetrue, so disabled users can’t be represented. Useuser.Spec.Enabled(or its default) consistently.✅ Suggested change
- if err = bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, password, true); err != nil { + if err = bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, password, user.Spec.Enabled); err != nil { return ctrl.Result{}, fmt.Errorf("failed to create or update BMC account with new password: %w", err) }- if err := bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, newPassword, true); err != nil { + if err := bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, newPassword, user.Spec.Enabled); err != nil { return ctrl.Result{}, fmt.Errorf("failed to create or update BMC account with new password: %w", err) }- if err := bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, newPassword, true); err != nil { + if err := bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, newPassword, user.Spec.Enabled); err != nil { return fmt.Errorf("failed to create or update BMC account with new password: %w", err) }- if err := bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, password, true); err != nil { + if err := bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, password, user.Spec.Enabled); err != nil { return fmt.Errorf("failed to create or update BMC account with new password: %w", err) }Also applies to: 186-200, 224-227, 238-241
267-271: Don’t mutatespec.bmcSecretRefin the controller.This is a user-owned field; controllers should record observed state in
status(e.g.,EffectiveBMCSecretRef) and update reconcile logic to prefer status whenspec.bmcSecretRefis nil. Based on learnings, Spec fields must remain user-owned while Status is controller-owned.
345-359: Close the BMC client after the connection test.
CreateBMCClientlikely opens a session; without a logout/close, repeated checks can leak sessions.🔧 Suggested fix
- _, err = bmcutils.CreateBMCClient(ctx, r.Client, protocolScheme, bmcObj.Spec.Protocol.Name, address, bmcObj.Spec.Protocol.Port, secret, r.BMCOptions) + bmcClient, err := bmcutils.CreateBMCClient(ctx, r.Client, protocolScheme, bmcObj.Spec.Protocol.Name, address, bmcObj.Spec.Protocol.Port, secret, r.BMCOptions) if err != nil { if httpErr, ok := err.(*common.Error); ok { if httpErr.HTTPReturnedStatusCode == 401 || httpErr.HTTPReturnedStatusCode == 403 { return true, nil } } return false, fmt.Errorf("failed to create BMC client: %w", err) } + defer bmcClient.Logout() return false, nil
245-266:CreateOrPatchwithGenerateNamewill fail.
controllerutil.CreateOrPatchrequires a stablemetadata.nameto fetch and patch the object. SinceGenerateNameleavesNameempty until the API server assigns it, the initial GET will fail. User.Createinstead for generated names.🛠️ Suggested fix
- op, err := controllerutil.CreateOrPatch(ctx, r.Client, secret, func() error { - if err := controllerutil.SetControllerReference(user, secret, r.Scheme); err != nil { - return fmt.Errorf("failed to set controller reference for BMCSecret: %w", err) - } - return nil - }) - if err != nil { - return fmt.Errorf("failed to create or patch BMCSecret: %w", err) - } - log.V(1).Info("BMCSecret created or patched", "BMCSecret", secret.Name, "Operation", op) + if err := controllerutil.SetControllerReference(user, secret, r.Scheme); err != nil { + return fmt.Errorf("failed to set controller reference for BMCSecret: %w", err) + } + if err := r.Create(ctx, secret); err != nil { + return fmt.Errorf("failed to create BMCSecret: %w", err) + } + log.V(1).Info("BMCSecret created", "BMCSecret", secret.Name)
🧹 Nitpick comments (1)
internal/controller/bmcuser_controller_test.go (1)
243-282: Make the rotation-period test deterministic.The 1s rotation window with a 4s
Eventuallytimeout is wall‑clock dependent and can be flaky under load. Consider faking time or settingStatus.LastRotationto a past timestamp and triggering reconcile immediately (or reuse the annotation-based rotation path) to avoid timing assumptions. As per coding guidelines, tests must be deterministic and avoid timing assumptions.
| - apiGroups: | ||
| - metal.ironcore.dev | ||
| resources: | ||
| - bmcusers | ||
| verbs: | ||
| - '*' |
There was a problem hiding this comment.
Avoid wildcard verbs to maintain least‑privilege RBAC.
Using '*' on bmcusers broadens permissions unnecessarily and will keep tripping policy scanners. Prefer explicit verbs (create, delete, get, list, patch, update, watch) unless there’s a concrete need for wildcard.
🔧 Suggested change
rules:
- apiGroups:
- metal.ironcore.dev
resources:
- bmcusers
verbs:
- - '*'
+ - create
+ - delete
+ - get
+ - list
+ - patch
+ - update
+ - watch📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - apiGroups: | |
| - metal.ironcore.dev | |
| resources: | |
| - bmcusers | |
| verbs: | |
| - '*' | |
| - apiGroups: | |
| - metal.ironcore.dev | |
| resources: | |
| - bmcusers | |
| verbs: | |
| - create | |
| - delete | |
| - get | |
| - list | |
| - patch | |
| - update | |
| - watch |
🤖 Prompt for AI Agents
In `@config/rbac/bmcuser_admin_role.yaml` around lines 16 - 21, Replace the
wildcard verbs entry under the RBAC rule for apiGroups: metal.ironcore.dev and
resources: bmcusers by enumerating explicit actions to follow least-privilege;
update the verbs list (the verbs field) to include only create, delete, get,
list, patch, update, and watch (and add any other specific verbs you actually
need) instead of '*' so policy scanners and auditors accept the bmcusers rule.
| if bmcsecret.Immutable != nil && *bmcsecret.Immutable { | ||
| if !reflect.DeepEqual(bmcsecret.Data, oldSecret.Data) { | ||
| return nil, fmt.Errorf("data field is immutable and cannot be updated") | ||
| } | ||
| if !reflect.DeepEqual(bmcsecret.StringData, oldSecret.StringData) { | ||
| return nil, fmt.Errorf("stringData field is immutable and cannot be updated") | ||
| } | ||
| } |
There was a problem hiding this comment.
Immutability can be bypassed if Immutable is unset on update.
The check only runs when the new object has Immutable=true, so a client can flip it to false and mutate Data/StringData. If immutability should be sticky, gate on the old value too (and optionally reject unsetting).
🔒 Suggested guard
- if bmcsecret.Immutable != nil && *bmcsecret.Immutable {
+ if (oldSecret.Immutable != nil && *oldSecret.Immutable) || (bmcsecret.Immutable != nil && *bmcsecret.Immutable) {
+ if oldSecret.Immutable != nil && *oldSecret.Immutable && (bmcsecret.Immutable == nil || !*bmcsecret.Immutable) {
+ return nil, fmt.Errorf("immutable field cannot be unset")
+ }
if !reflect.DeepEqual(bmcsecret.Data, oldSecret.Data) {
return nil, fmt.Errorf("data field is immutable and cannot be updated")
}
if !reflect.DeepEqual(bmcsecret.StringData, oldSecret.StringData) {
return nil, fmt.Errorf("stringData field is immutable and cannot be updated")
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if bmcsecret.Immutable != nil && *bmcsecret.Immutable { | |
| if !reflect.DeepEqual(bmcsecret.Data, oldSecret.Data) { | |
| return nil, fmt.Errorf("data field is immutable and cannot be updated") | |
| } | |
| if !reflect.DeepEqual(bmcsecret.StringData, oldSecret.StringData) { | |
| return nil, fmt.Errorf("stringData field is immutable and cannot be updated") | |
| } | |
| } | |
| if (oldSecret.Immutable != nil && *oldSecret.Immutable) || (bmcsecret.Immutable != nil && *bmcsecret.Immutable) { | |
| if oldSecret.Immutable != nil && *oldSecret.Immutable && (bmcsecret.Immutable == nil || !*bmcsecret.Immutable) { | |
| return nil, fmt.Errorf("immutable field cannot be unset") | |
| } | |
| if !reflect.DeepEqual(bmcsecret.Data, oldSecret.Data) { | |
| return nil, fmt.Errorf("data field is immutable and cannot be updated") | |
| } | |
| if !reflect.DeepEqual(bmcsecret.StringData, oldSecret.StringData) { | |
| return nil, fmt.Errorf("stringData field is immutable and cannot be updated") | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@internal/webhook/v1alpha1/bmcsecret_webhook.go` around lines 61 - 68, The
immutability check only runs when the new object has Immutable=true, allowing
clients to unset Immutable and mutate Data/StringData; update the logic in the
webhook (referencing bmcsecret.Immutable, oldSecret.Immutable, bmcsecret.Data,
oldSecret.Data, bmcsecret.StringData, oldSecret.StringData) to enforce
immutability if the old resource was immutable or the new one is set immutable —
i.e., treat immutability as sticky by checking if (oldSecret.Immutable != nil &&
*oldSecret.Immutable) || (bmcsecret.Immutable != nil && *bmcsecret.Immutable)
before comparing Data/StringData and additionally reject attempts to change
Immutable from true to false (return an error when oldSecret.Immutable is true
and bmcsecret.Immutable is nil or false).
| "internal/cmd/visualizer/index.html", | ||
| "test/data/*.json", | ||
| "test/data/*.yaml" | ||
| "test/data/*.json" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, examine REUSE.toml to understand the context
echo "=== REUSE.toml content ==="
cat REUSE.toml
echo ""
echo "=== YAML/YML files in test/data/ ==="
fd -e yaml -e yml . test/data/ 2>/dev/null || echo "No YAML files found"
echo ""
echo "=== Check SPDX headers in found YAML files ==="
for f in $(fd -e yaml -e yml . test/data/ 2>/dev/null); do
echo "--- $f ---"
head -10 "$f"
doneRepository: ironcore-dev/metal-operator
Length of output: 1674
Add SPDX licensing for test/data/*.yaml files.
The test/data/*.yaml pattern was removed from the annotation block, but test/data/dns_record_template.yaml exists without SPDX headers. Either add the pattern back to the first annotation block or add SPDX headers directly to the YAML file.
🤖 Prompt for AI Agents
In `@REUSE.toml` at line 26, The REUSE.toml annotation block currently lists
"test/data/*.json" but omits "test/data/*.yaml", leaving
test/data/dns_record_template.yaml without SPDX metadata; fix by either (A)
adding the pattern "test/data/*.yaml" back into the REUSE.toml annotation block
so all YAML in test/data is covered, or (B) adding SPDX headers to the YAML file
itself (e.g., SPDX-FileCopyrightText and SPDX-License-Identifier lines at the
top of test/data/dns_record_template.yaml) so the file has explicit licensing
information.
There was a problem hiding this comment.
I think the yaml exclusion should be added back again.
There was a problem hiding this comment.
@afritzler, understood! Adding "test/data/*.yaml" back to the REUSE.toml annotation block will ensure that all YAML files in test/data/ are properly covered by SPDX licensing metadata.
Would you like me to plan the code change to restore this line, or will you handle it as part of your updates to this PR?
There was a problem hiding this comment.
it is already. this is outdated
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/controller/bmcuser_controller.go`:
- Around line 379-384: The log "Successfully deleted BMC account and removed
finalizer for User" is misleading because the finalizer is removed in
reconcileExists, not here; update the log in the block that calls
bmcClient.DeleteAccount (the code that returns fmt.Errorf on failure) to only
state successful account deletion (e.g., "Successfully deleted BMC account for
User") and remove any mention of finalizer removal, keeping references to
user.Name and the bmcClient.DeleteAccount call to locate the change.
In `@internal/webhook/v1alpha1/bmcsecret_webhook.go`:
- Around line 61-68: The immutability check is using the new object
(bmcsecret.Immutable) allowing a client to flip Immutable=false and change data
in one update; change the logic to inspect the existing object
(oldSecret.Immutable) instead and, if oldSecret.Immutable is non-nil and true,
reject any update that modifies Immutable, Data, or StringData. Update the check
in the validation function (the block referencing bmcsecret.Immutable,
bmcsecret.Data, bmcsecret.StringData and oldSecret) to use oldSecret.Immutable
and return an error if the incoming bmcsecret attempts to change Immutable from
true or alter Data/StringData when oldSecret.Immutable was true.
♻️ Duplicate comments (12)
api/v1alpha1/bmcuser_types.go (1)
12-29: Add validation markers to BMCUserSpec and consider marking BMCRef as required.The BMCUserSpec lacks validation markers for fields like
UserNameandRoleID, which should be non-empty. Additionally,BMCRefis currently optional (pointer type) but is required for the controller to function—similar toBMCSettings.BMCRef, which is marked as+required.To align with the codebase patterns, add:
+kubebuilder:validation:Requiredand+kubebuilder:validation:MinLength=1onUserNameandRoleID+kubebuilder:validation:RequiredonBMCRefAlternatively, implement these validations via a webhook (preferred per coding guidelines). Currently, there is no
bmcuser_webhook.gounlike similar types (BMCSettings, BMCSecret).internal/webhook/v1alpha1/bmcsecret_webhook_test.go (2)
35-35: Useptr.To(true)for consistency.Line 53 correctly uses
ptr.To(false), but this line uses the awkward&[]bool{true}[0]pattern.♻️ Suggested fix
- Immutable: &[]bool{true}[0], + Immutable: ptr.To(true),
37-37: Grammar: use "a" before "BMCSecret"."BMC" is pronounced with a consonant sound, so it should be "a BMCSecret" not "an BMCSecret". Same applies to lines 44 and 51.
📝 Suggested fix
- By("Creating an BMCSecret") + By("Creating a BMCSecret")Also update lines 44 and 51 similarly.
cmd/manager/main.go (1)
550-563: Fix controller name in setup error log.
The reconciler isBMCUserReconciler, but the log says"User", which is inconsistent with other controller names.🔧 Proposed fix
}).SetupWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create controller", "controller", "User") + setupLog.Error(err, "unable to create controller", "controller", "BMCUser") os.Exit(1) }bmc/redfish_local.go (1)
55-95: Guard mock account map access and honoridparameter.
UnitTestMockUps.Accountsis accessed/mutated without synchronization, andDeleteAccountignores theidargument whileCreateOrUpdateAccountscans all entries. Consider adding a mutex toUnitTestMockUps, using direct lookup byuserName, and validatingidwhen provided.internal/controller/bmcuser_controller_test.go (2)
127-134: Avoid explicit deletion of effective secret; assert GC instead.
Deleting the effective secret directly can be flaky if GC already removed it. Prefer asserting NotFound.🔧 Suggested adjustment
- Expect(k8sClient.Delete(ctx, effectiveSecret)).To(Succeed()) - Eventually(Get(effectiveSecret)).Should(Satisfy(apierrors.IsNotFound)) + Eventually(Get(effectiveSecret)).Should(Satisfy(apierrors.IsNotFound))
317-328: Use explicit NotFound checks instead ofShouldNot(Succeed()).
ShouldNot(Succeed())can hide real errors. Useapierrors.IsNotFoundand avoid explicit secret deletes in GC scenarios.🔧 Suggested adjustment
- Eventually(Get(user)).ShouldNot(Succeed()) + Eventually(Get(user)).Should(Satisfy(apierrors.IsNotFound)) - Expect(k8sClient.Delete(ctx, effectiveSecret)).To(Succeed()) + Eventually(Get(effectiveSecret)).Should(Satisfy(apierrors.IsNotFound))bmc/redfish.go (2)
804-812: Close delete response body even whenDeletereturns an error.
IfDeletereturns a non‑nil response alongside an error, the current code leaksresp.Body.🔧 Proposed fix
- resp, err := r.client.Delete(a.ODataID) - if err != nil { - return err - } - defer func(Body io.ReadCloser) { + resp, err := r.client.Delete(a.ODataID) + if resp != nil { + defer func(Body io.ReadCloser) { if err = Body.Close(); err != nil { log.Error(err, "failed to close response body") } - }(resp.Body) + }(resp.Body) + } + if err != nil { + return err + } return nil
1132-1144: Fix Dell special‑char typo and use a typed default key.
The Dell character set contains%%and the"default"key isn’t typed asManufacturer.🔧 Proposed fix
var manufacturerPasswordConfigs = map[Manufacturer]ManufacturerPasswordConfig{ ManufacturerDell: { - SpecialChars: "!#$%%&()*.?-@[]^_`{}|~+=", + SpecialChars: "!#$%&()*.?-@[]^_`{}|~+=", }, @@ - "default": { + Manufacturer("default"): { SpecialChars: "!@#$%&*()_-+=[]{}/?~|", }, }internal/controller/bmcuser_controller.go (3)
106-114: Useuser.Spec.Enabledfor account enablement.
The enabled flag is hard‑coded totrue, which ignores the user’s desired state. Apply the same change to allCreateOrUpdateAccountcalls in this file (Lines 112, 198, 225, 239).🔧 Proposed fix (pattern)
- if err = bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, password, true); err != nil { + if err = bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, password, user.Spec.Enabled); err != nil { return ctrl.Result{}, fmt.Errorf("failed to create or update BMC account with new password: %w", err) }
245-271: Do not mutate Spec when creating the secret.
createSecretpatchesSpec.BMCSecretRef, which should be user‑owned. Use status (EffectiveBMCSecretRef) instead.🔧 Suggested direction
- userBase := user.DeepCopy() - user.Spec.BMCSecretRef = &v1.LocalObjectReference{Name: secret.Name} - if err := r.Patch(ctx, user, client.MergeFrom(userBase)); err != nil { - return fmt.Errorf("failed to patch User status with effective BMCSecretRef: %w", err) - } + if err := r.setEffectiveSecretRef(ctx, log, user, secret); err != nil { + return fmt.Errorf("failed to patch User status with effective BMCSecretRef: %w", err) + }As per coding guidelines, spec fields must remain user‑owned; controller state belongs in status.
345-358: Close the BMC client after connection test.
bmcConnectionTestcreates a client but never logs out, which can leak sessions.🔧 Proposed fix
- _, err = bmcutils.CreateBMCClient(ctx, r.Client, protocolScheme, bmcObj.Spec.Protocol.Name, address, bmcObj.Spec.Protocol.Port, secret, r.BMCOptions) + bmcClient, err := bmcutils.CreateBMCClient(ctx, r.Client, protocolScheme, bmcObj.Spec.Protocol.Name, address, bmcObj.Spec.Protocol.Port, secret, r.BMCOptions) if err != nil { if httpErr, ok := err.(*common.Error); ok { if httpErr.HTTPReturnedStatusCode == 401 || httpErr.HTTPReturnedStatusCode == 403 { return true, nil } } return false, fmt.Errorf("failed to create BMC client: %w", err) } + defer bmcClient.Logout() return false, nil
🧹 Nitpick comments (5)
config/rbac/bmcuser_admin_role.yaml (1)
15-27: Wildcard verb usage is appropriate for admin role, but status permissions may be incomplete.The static analysis hint (CKV_K8S_49) flags the wildcard verb usage, but this is intentional for an admin role designed to delegate permissions to other users.
However, consider whether the
bmcusers/statussubresource should also have broader permissions (e.g.,patch,update) for consistency with the admin role's purpose. Currently, admins can do everything on the main resource but onlygetthe status.🔧 Suggested enhancement for status permissions
- apiGroups: - metal.ironcore.dev resources: - bmcusers/status verbs: - get + - patch + - updateapi/v1alpha1/bmcuser_types.go (1)
44-55: Consider whetherSpecshould be required.The
Specfield usesomitempty(line 53), which means an empty spec is valid. If the BMCUser resource requires a spec to be meaningful, consider removingomitemptyor adding validation to ensure required fields are present.🔧 Optional: Make spec required
- Spec BMCUserSpec `json:"spec,omitempty"` + Spec BMCUserSpec `json:"spec"`internal/webhook/v1alpha1/bmcsecret_webhook.go (1)
32-34: UnusedClientfield.The
Clientfield is declared but never used in any of the validation methods. Either remove it or wire it up if it's intended for future use.♻️ Suggested fix
type BMCSecretCustomValidator struct { - Client client.Client }bmc/mockup.go (1)
114-146: Good test fixture for account management.The mock accounts provide varied roles (
ReadOnly,Administrator) for comprehensive test coverage of account management operations.Minor nit: Line 129 has an inconsistent blank line between
Entityblock andUserNamethat doesn't appear in other account entries.♻️ Optional: remove inconsistent blank line
"admin": { Entity: common.Entity{ ID: "1", }, - UserName: "admin",internal/controller/bmcuser_controller_test.go (1)
243-272: Make rotation test deterministic (avoid real time waits).
The test relies on wall-clock delays (1speriod +Eventually("4s")), which can be flaky under load. Consider injecting a fake clock or triggering rotation via annotation to remove timing dependence.As per coding guidelines, tests must be deterministic and not rely on timing assumptions.
| log.Info("Deleting BMC account for User", "User", user.Name) | ||
| if err := bmcClient.DeleteAccount(ctx, user.Spec.UserName, user.Status.ID); err != nil { | ||
| return fmt.Errorf("failed to delete BMC account: %w", err) | ||
| } | ||
| log.Info("Successfully deleted BMC account and removed finalizer for User", "User", user.Name) | ||
| return nil |
There was a problem hiding this comment.
Log message is misleading about finalizer removal.
This log claims the finalizer was removed here, but removal happens in reconcileExists. Consider updating the message to only mention account deletion.
🔧 Proposed fix
- log.Info("Successfully deleted BMC account and removed finalizer for User", "User", user.Name)
+ log.Info("Successfully deleted BMC account for User", "User", user.Name)🤖 Prompt for AI Agents
In `@internal/controller/bmcuser_controller.go` around lines 379 - 384, The log
"Successfully deleted BMC account and removed finalizer for User" is misleading
because the finalizer is removed in reconcileExists, not here; update the log in
the block that calls bmcClient.DeleteAccount (the code that returns fmt.Errorf
on failure) to only state successful account deletion (e.g., "Successfully
deleted BMC account for User") and remove any mention of finalizer removal,
keeping references to user.Name and the bmcClient.DeleteAccount call to locate
the change.
| if bmcsecret.Immutable != nil && *bmcsecret.Immutable { | ||
| if !reflect.DeepEqual(bmcsecret.Data, oldSecret.Data) { | ||
| return nil, fmt.Errorf("data field is immutable and cannot be updated") | ||
| } | ||
| if !reflect.DeepEqual(bmcsecret.StringData, oldSecret.StringData) { | ||
| return nil, fmt.Errorf("stringData field is immutable and cannot be updated") | ||
| } | ||
| } |
There was a problem hiding this comment.
Immutability check uses wrong object — bypass possible.
The check uses bmcsecret.Immutable (new object) instead of oldSecret.Immutable. A user can bypass immutability by setting Immutable=false and changing Data in the same update request.
Following Kubernetes Secret semantics, once Immutable is true, the field itself and the data should be locked.
🐛 Proposed fix
- if bmcsecret.Immutable != nil && *bmcsecret.Immutable {
+ if oldSecret.Immutable != nil && *oldSecret.Immutable {
+ // Once immutable, the Immutable field itself cannot be changed
+ if bmcsecret.Immutable == nil || !*bmcsecret.Immutable {
+ return nil, fmt.Errorf("immutable field cannot be changed once set to true")
+ }
if !reflect.DeepEqual(bmcsecret.Data, oldSecret.Data) {
return nil, fmt.Errorf("data field is immutable and cannot be updated")
}
if !reflect.DeepEqual(bmcsecret.StringData, oldSecret.StringData) {
return nil, fmt.Errorf("stringData field is immutable and cannot be updated")
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if bmcsecret.Immutable != nil && *bmcsecret.Immutable { | |
| if !reflect.DeepEqual(bmcsecret.Data, oldSecret.Data) { | |
| return nil, fmt.Errorf("data field is immutable and cannot be updated") | |
| } | |
| if !reflect.DeepEqual(bmcsecret.StringData, oldSecret.StringData) { | |
| return nil, fmt.Errorf("stringData field is immutable and cannot be updated") | |
| } | |
| } | |
| if oldSecret.Immutable != nil && *oldSecret.Immutable { | |
| // Once immutable, the Immutable field itself cannot be changed | |
| if bmcsecret.Immutable == nil || !*bmcsecret.Immutable { | |
| return nil, fmt.Errorf("immutable field cannot be changed once set to true") | |
| } | |
| if !reflect.DeepEqual(bmcsecret.Data, oldSecret.Data) { | |
| return nil, fmt.Errorf("data field is immutable and cannot be updated") | |
| } | |
| if !reflect.DeepEqual(bmcsecret.StringData, oldSecret.StringData) { | |
| return nil, fmt.Errorf("stringData field is immutable and cannot be updated") | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@internal/webhook/v1alpha1/bmcsecret_webhook.go` around lines 61 - 68, The
immutability check is using the new object (bmcsecret.Immutable) allowing a
client to flip Immutable=false and change data in one update; change the logic
to inspect the existing object (oldSecret.Immutable) instead and, if
oldSecret.Immutable is non-nil and true, reject any update that modifies
Immutable, Data, or StringData. Update the check in the validation function (the
block referencing bmcsecret.Immutable, bmcsecret.Data, bmcsecret.StringData and
oldSecret) to use oldSecret.Immutable and return an error if the incoming
bmcsecret attempts to change Immutable from true or alter Data/StringData when
oldSecret.Immutable was true.
There was a problem hiding this comment.
@stefanhipfel that is actually a good point here. Shouldn't we check the immutability of the old object instead the new one?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
afritzler
left a comment
afritzler
left a comment
There was a problem hiding this comment.
Just some small nitpicks from my side.
| if modified, err := clientutils.PatchEnsureNoFinalizer(ctx, r.Client, user, BMCUserFinalizer); err != nil || modified { | ||
| return ctrl.Result{}, err | ||
| } |
There was a problem hiding this comment.
Can we move this into the delete() method.
| return ctrl.Result{}, nil | ||
| } | ||
|
|
||
| func (r *BMCUserReconciler) handleMissingBMCSecretRef(ctx context.Context, log logr.Logger, bmcClient bmc.BMC, user *metalv1alpha1.BMCUser, bmcObj *metalv1alpha1.BMC) error { |
There was a problem hiding this comment.
Can we call this method ensureBMCSecretForUser()
| log.Info("Creating BMCSecret for User", "User", user.Name) | ||
| secret := &metalv1alpha1.BMCSecret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| GenerateName: user.Name + "-bmcsecret-", |
There was a problem hiding this comment.
Can we drop the -bmcsecret- as the kind already is clear on what this object is?
| metalv1alpha1.BMCSecretUsernameKeyName: []byte(user.Spec.UserName), | ||
| metalv1alpha1.BMCSecretPasswordKeyName: []byte(password), | ||
| }, | ||
| Immutable: &[]bool{true}[0], // Make the secret immutable |
There was a problem hiding this comment.
Can't we use the ptr package here?
| return nil | ||
| } | ||
|
|
||
| func (r *BMCUserReconciler) createSecret(ctx context.Context, log logr.Logger, user *metalv1alpha1.BMCUser, password string) error { |
There was a problem hiding this comment.
I would actually split this method into:
- Creating the
BMCSecretfor aUserand returning the secret object -> hence renaming it intocreateBMCSecretForUser - And update the
BMCUserseffective secret in the status outside e.g. in theensureBMCSecretForUsermethod
There was a problem hiding this comment.
setting the effective secret is done at a later point, after everything was done without errors
There was a problem hiding this comment.
it would be a setBMCSecretRef function, which just sets the new bmcSecretRef
afritzler
left a comment
afritzler
left a comment
There was a problem hiding this comment.
We also need to address the RBAC issues here.
Nuckal777
left a comment
Nuckal777
left a comment
There was a problem hiding this comment.
Good to go from my side. I annotated some stuff for cleanup.
| defer func(Body io.ReadCloser) { | ||
| if err = Body.Close(); err != nil { | ||
| log.Error(err, "failed to close response body") | ||
| } | ||
| }(resp.Body) |
There was a problem hiding this comment.
Deferring here seems superfluous. The body could closed directly, instead.
| return fmt.Errorf("account %s not found", userName) | ||
| } | ||
|
|
||
| func (r *RedfishBMC) GetAccountService(ctx context.Context) (*redfish.AccountService, error) { |
There was a problem hiding this comment.
true. We got unused ctx in many more functions as well in redfish
There was a problem hiding this comment.
wondering why linter did not report it
| return service, nil | ||
| } | ||
|
|
||
| func (r *RedfishBMC) GetAccounts(ctx context.Context) ([]*redfish.ManagerAccount, error) { |
| validator = BMCSecretCustomValidator{ | ||
| Client: k8sClient, | ||
| } | ||
| Expect(validator).NotTo(BeNil(), "Expected validator to be initialized") |
There was a problem hiding this comment.
Can be deleted, validator is initialized 3 lines above and cannot be nil, because it's a struct.
afritzler
changed the title
BMCUser types and controller
afritzler
changed the title
BMCUser types and controllerBMCUser type and controller
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/controller/bmcuser_controller.go`:
- Around line 186-192: The code passes accountService.MaxPasswordLength directly
into bmc.GenerateSecurePassword which can be 0/unspecified and causes failures;
update the reconciliation logic (around GetAccountService() usage and where
newPassword is generated) to defensively compute an effective length: use
accountService.MinPasswordLength if present and >=4, otherwise fall back to a
sane default (e.g., 16), and cap/validate the chosen length to satisfy
bmc.GenerateSecurePassword's requirement (length >= 4) before calling
bmc.GenerateSecurePassword(bmc.Manufacturer(bmcObj.Status.Manufacturer),
effectiveLength); ensure the same fix is applied to the other occurrence around
lines 216-223.
| accountService, err := bmcClient.GetAccountService() | ||
| if err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to get account service: %w", err) | ||
| } | ||
| newPassword, err := bmc.GenerateSecurePassword(bmc.Manufacturer(bmcObj.Status.Manufacturer), accountService.MaxPasswordLength) | ||
| if err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to generate new password for BMC user %s: %w", user.Name, err) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "*.go" -path "*/bmcuser*" | head -20Repository: ironcore-dev/metal-operator
Length of output: 195
🏁 Script executed:
ls -la internal/controller/ | head -20Repository: ironcore-dev/metal-operator
Length of output: 1534
🏁 Script executed:
cat -n internal/controller/bmcuser_controller.go | head -100Repository: ironcore-dev/metal-operator
Length of output: 4414
🏁 Script executed:
cat -n internal/controller/bmcuser_controller.go | sed -n '180,230p'Repository: ironcore-dev/metal-operator
Length of output: 2904
🏁 Script executed:
grep -n "GenerateSecurePassword" internal/controller/bmcuser_controller.goRepository: ironcore-dev/metal-operator
Length of output: 334
🏁 Script executed:
grep -r "GenerateSecurePassword" bmc/ --include="*.go" -A 5Repository: ironcore-dev/metal-operator
Length of output: 491
🏁 Script executed:
cat -n bmc/redfish.go | grep -A 30 "func GenerateSecurePassword"Repository: ironcore-dev/metal-operator
Length of output: 1527
🏁 Script executed:
cat -n bmc/redfish.go | sed -n '1,100p'Repository: ironcore-dev/metal-operator
Length of output: 3754
🏁 Script executed:
cat -n bmc/redfish.go | sed -n '1160,1200p'Repository: ironcore-dev/metal-operator
Length of output: 1446
🏁 Script executed:
grep -n "GetAccountService" bmc/ --include="*.go" -A 10Repository: ironcore-dev/metal-operator
Length of output: 97
🏁 Script executed:
grep -r "MaxPasswordLength" . --include="*.go"Repository: ironcore-dev/metal-operator
Length of output: 414
🏁 Script executed:
grep -rn "GetAccountService" bmc/ --include="*.go" -A 10Repository: ironcore-dev/metal-operator
Length of output: 1158
🏁 Script executed:
cat -n bmc/redfish.go | grep -A 20 "func.*GetAccountService"Repository: ironcore-dev/metal-operator
Length of output: 929
🏁 Script executed:
grep -rn "MinPasswordLength" . --include="*.go"Repository: ironcore-dev/metal-operator
Length of output: 53
🏁 Script executed:
grep -n "accountService\." internal/controller/bmcuser_controller.goRepository: ironcore-dev/metal-operator
Length of output: 334
🏁 Script executed:
cat -n internal/controller/bmcuser_controller_test.go | head -100Repository: ironcore-dev/metal-operator
Length of output: 3751
🏁 Script executed:
grep -n "MaxPasswordLength" internal/controller/bmcuser_controller_test.goRepository: ironcore-dev/metal-operator
Length of output: 53
🏁 Script executed:
grep -n "accountService\|AccountService" internal/controller/bmcuser_controller_test.go | head -20Repository: ironcore-dev/metal-operator
Length of output: 53
🏁 Script executed:
go mod graph | grep gofishRepository: ironcore-dev/metal-operator
Length of output: 309
🏁 Script executed:
cat -n go.mod | grep gofishRepository: ironcore-dev/metal-operator
Length of output: 115
🏁 Script executed:
find . -name "go.mod" -exec cat {} \; | grep -A 2 -B 2 gofishRepository: ironcore-dev/metal-operator
Length of output: 234
🏁 Script executed:
cat -n internal/controller/bmcuser_controller_test.go | sed -n '79,200p'Repository: ironcore-dev/metal-operator
Length of output: 5263
🏁 Script executed:
grep -rn "mock\|Mock" internal/controller/bmcuser_controller_test.go | head -20Repository: ironcore-dev/metal-operator
Length of output: 390
🏁 Script executed:
grep -n "GenerateSecurePassword" bmc/redfish_test.goRepository: ironcore-dev/metal-operator
Length of output: 123
🏁 Script executed:
grep -rn "UnitTestMockUps\|MockUps" bmc/ --include="*.go" | head -30Repository: ironcore-dev/metal-operator
Length of output: 2266
🏁 Script executed:
grep -rn "default.*password\|password.*default" bmc/ --include="*.go" | head -20Repository: ironcore-dev/metal-operator
Length of output: 53
🏁 Script executed:
cat -n bmc/redfish.go | sed -n '1140,1165p'Repository: ironcore-dev/metal-operator
Length of output: 1323
🏁 Script executed:
cat -n bmc/mockup.go | sed -n '1,100p'Repository: ironcore-dev/metal-operator
Length of output: 3301
🏁 Script executed:
grep -rn "AccountService\|MaxPasswordLength" bmc/mockup.goRepository: ironcore-dev/metal-operator
Length of output: 53
🏁 Script executed:
cat -n internal/controller/bmcuser_controller.go | sed -n '150,180p'Repository: ironcore-dev/metal-operator
Length of output: 1812
🏁 Script executed:
python3 << 'EOF'
import subprocess
import json
# Search for any tests or documentation about MaxPasswordLength being zero
result = subprocess.run(
["rg", "-n", "MaxPasswordLength|MinPasswordLength", "--type", "go"],
capture_output=True,
text=True,
cwd="."
)
print("=== MaxPasswordLength/MinPasswordLength references ===")
print(result.stdout)
print(result.stderr)
EOFRepository: ironcore-dev/metal-operator
Length of output: 157
🏁 Script executed:
grep -rn "if.*length\|if.*MaxPassword\|if.*0" bmc/redfish.go | grep -i "password\|length" | head -20Repository: ironcore-dev/metal-operator
Length of output: 230
🏁 Script executed:
cat -n bmc/mockup.go | sed -n '100,200p'Repository: ironcore-dev/metal-operator
Length of output: 2842
🏁 Script executed:
grep -rn "GetAccountService\|AccountService" bmc/ --include="*.go" -B 2 -A 8 | head -60Repository: ironcore-dev/metal-operator
Length of output: 2709
🏁 Script executed:
cat -n internal/controller/bmcuser_controller.go | sed -n '185,195p'Repository: ironcore-dev/metal-operator
Length of output: 698
🏁 Script executed:
grep -rn "MaxPasswordLength.*0\|if.*MaxPassword\|fallback" . --include="*.go"Repository: ironcore-dev/metal-operator
Length of output: 207
🏁 Script executed:
grep -rn "type AccountService\|MaxPasswordLength\|MinPasswordLength" --include="*.go" | grep -v test | head -20Repository: ironcore-dev/metal-operator
Length of output: 418
🏁 Script executed:
cat -n bmc/redfish.go | sed -n '1145,1165p'Repository: ironcore-dev/metal-operator
Length of output: 1226
🏁 Script executed:
find . -name "*.go" -exec grep -l "gofish" {} \; | head -5Repository: ironcore-dev/metal-operator
Length of output: 833
🏁 Script executed:
web_search
Redfish AccountService MaxPasswordLength specification - can it be zero or unspecified?Repository: ironcore-dev/metal-operator
Length of output: 167
🏁 Script executed:
grep -rn "length.*4\|mustInclude" bmc/redfish.goRepository: ironcore-dev/metal-operator
Length of output: 404
🏁 Script executed:
cat -n internal/controller/bmcuser_controller.go | sed -n '214,235p'Repository: ironcore-dev/metal-operator
Length of output: 1518
🏁 Script executed:
grep -n "accountService\." internal/controller/bmcuser_controller.goRepository: ironcore-dev/metal-operator
Length of output: 334
🏁 Script executed:
grep -rn "0.*password\|password.*0" bmc/ --include="*.go" | head -15Repository: ironcore-dev/metal-operator
Length of output: 53
🏁 Script executed:
cat -n bmc/redfish.go | sed -n '1160,1165p'Repository: ironcore-dev/metal-operator
Length of output: 399
🌐 Web query:
Redfish AccountService MaxPasswordLength specification optional field zero value
💡 Result:
In the DMTF Redfish specification, AccountService/MaxPasswordLength is simply “the maximum password length that the implementation allows” for local accounts (it “does not apply to accounts from external account providers”). It is not defined as having any special meaning when the value is 0. [1]
In the AccountService schema, MaxPasswordLength has a minimum of 0 (so 0 is syntactically valid), but the schema’s long description still describes it only as “the maximum password length” and does not say “0 means unlimited/unknown/unsupported”. [2]
So, if you see:
- Property omitted: treat as not reported / not supported / unknown (implementation-dependent).
- Property present with value
0: the Redfish spec does not define a standard interpretation; you cannot assume it means “no maximum” unless the vendor’s Redfish documentation explicitly says so. [1][2][3]
Sources:
[1] DMTF Redfish Data Model Specification (DSP0268) – MaxPasswordLength description (redfish.dmtf.org)
[2] DMTF AccountService schema (OData CSDL) – MaxPasswordLength min=0 + long description (gbmc.googlesource.com)
[3] DMTF Redfish Resource & Schema Guide (DSP2046) – MaxPasswordLength property listing (redfish.dmtf.org)
Citations:
- 1: https://redfish.dmtf.org/schemas/v1/DSP0268_2024.1.html?utm_source=openai
- 2: https://gbmc.googlesource.com/gbmcweb/%2B/refs/heads/master/static/redfish/v1/schema/AccountService_v1.xml
- 3: https://redfish.dmtf.org/schemas/v1/DSP2046_2025.3.html?utm_source=openai
Add defensive handling for zero or unspecified MaxPasswordLength.
Per the Redfish specification, MaxPasswordLength can be 0 or omitted (implementation-dependent). The current code at lines 190 and 220 passes this value directly to GenerateSecurePassword, which requires length ≥ 4 and fails otherwise, causing reconciliation to loop indefinitely. Introduce a fallback to a sane default (e.g., 16) or use MinPasswordLength when available.
🔧 Example fallback
- newPassword, err := bmc.GenerateSecurePassword(bmc.Manufacturer(bmcObj.Status.Manufacturer), accountService.MaxPasswordLength)
+ length := accountService.MaxPasswordLength
+ if length <= 0 {
+ length = 16
+ // If MinPasswordLength exists and is larger, prefer it.
+ // if accountService.MinPasswordLength > length { length = accountService.MinPasswordLength }
+ }
+ newPassword, err := bmc.GenerateSecurePassword(bmc.Manufacturer(bmcObj.Status.Manufacturer), length)Also applies to: 216-223
🤖 Prompt for AI Agents
In `@internal/controller/bmcuser_controller.go` around lines 186 - 192, The code
passes accountService.MaxPasswordLength directly into bmc.GenerateSecurePassword
which can be 0/unspecified and causes failures; update the reconciliation logic
(around GetAccountService() usage and where newPassword is generated) to
defensively compute an effective length: use accountService.MinPasswordLength if
present and >=4, otherwise fall back to a sane default (e.g., 16), and
cap/validate the chosen length to satisfy bmc.GenerateSecurePassword's
requirement (length >= 4) before calling
bmc.GenerateSecurePassword(bmc.Manufacturer(bmcObj.Status.Manufacturer),
effectiveLength); ensure the same fix is applied to the other occurrence around
lines 216-223.
5 participants
Proposed Changes
Implement User controller which handles the creation of user accounts in a bmc
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.