Fix BMCUser account creation and password rotation on Dell iDRAC (#946)

- Move BMC account creation out of ensureBMCSecretForUser into the
  Status.ID == "" branch where it belongs; the function now only
  manages the Kubernetes BMCSecret and its reference
- Include the password in the same PATCH when updating an existing
  account; Dell iDRAC returns HTTP 400 when RoleId/Enabled are
  modified without a password in the request body
- Set Status.ID and Status.LastRotation atomically in patchUserStatus
  to avoid a stale ResourceVersion conflict from a second Status patch
- Move setBMCUserSecretRef after CreateOrUpdateAccount in both
  handleRotatingPassword and ensureBMCSecretForUser so that a BMC
  failure does not leave Spec.BMCSecretRef pointing at an unverified
  secret, which would cause an infinite rotation loop
- Fix mock server to update the authentication store when Password is
  changed via PATCH, matching the behaviour of the real Redfish PATCH

Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>

Author: stefanhipfel

bmc/mock/server/server.go

@@ -493,8 +493,17 @@ func (s *MockServer) handlePatch(w http.ResponseWriter, r *http.Request) {
 	}
 
 	mergeJSON(base, update)
-	s.saveResource(filePath, base)
 
+	// Keep the authentication store in sync when Password is updated via PATCH.
+	if newPwd, ok := update["Password"].(string); ok && newPwd != "" {
+		if username, ok := base["UserName"].(string); ok && username != "" {
+			s.mu.Lock()
+			s.accounts[username] = newPwd
+			s.mu.Unlock()
+		}
+	}
+
+	s.saveResource(filePath, base)
 	w.WriteHeader(http.StatusNoContent)
 }
 

bmc/redfish.go

@@ -819,20 +819,14 @@ func (r *RedfishBaseBMC) CreateOrUpdateAccount(
 		if a.UserName == userName {
 			a.RoleID = role
 			a.Enabled = enabled
+			// Always include the password in the same PATCH. Some BMCs (e.g. Dell iDRAC) return
+			// HTTP 400 when RoleId/Enabled are modified without a password in the request body.
+			if password != "" {
+				a.Password = password
+			}
 			if err := a.Update(); err != nil {
 				return fmt.Errorf("failed to update account: %w", err)
 			}
-			// Update password using the official Redfish ChangePassword action
-			if password != "" {
-				if _, err := a.ChangePassword(password, r.options.Password); err != nil {
-					// Fallback: Use PATCH to update password
-					// This is needed for BMCs with buggy Redfish implementations
-					a.Password = password
-					if err := a.Update(); err != nil {
-						return fmt.Errorf("failed to update account password via PATCH: %w", err)
-					}
-				}
-			}
 			return nil
 		}
 	}

internal/controller/bmcuser_controller.go

@@ -79,7 +79,7 @@ func (r *BMCUserReconciler) reconcile(ctx context.Context, user *metalv1alpha1.B
 		return ctrl.Result{}, fmt.Errorf("failed to get BMC client: %w", err)
 	}
 	defer bmcClient.Logout()
-	if err = r.patchUserStatus(ctx, user, bmcClient); err != nil {
+	if err = r.patchUserStatus(ctx, user, bmcClient, metav1.Time{}); err != nil {
 		return ctrl.Result{}, fmt.Errorf("failed to update User status: %w", err)
 	}
 
@@ -103,15 +103,9 @@ func (r *BMCUserReconciler) reconcile(ctx context.Context, user *metalv1alpha1.B
 		if err = bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, password, true); err != nil {
 			return ctrl.Result{}, fmt.Errorf("failed to create or update BMC account with new password: %w", err)
 		}
-		if err = r.patchUserStatus(ctx, user, bmcClient); err != nil {
+		if err = r.patchUserStatus(ctx, user, bmcClient, metav1.Now()); err != nil {
 			return ctrl.Result{}, fmt.Errorf("failed to update User status after creating BMC account: %w", err)
 		}
-		// Set initial rotation timestamp to prevent immediate rotation after account creation
-		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 set initial rotation time: %w", err)
-		}
 	}
 	if user.Status.EffectiveBMCSecretRef != nil &&
 		user.Spec.BMCSecretRef.Name != user.Status.EffectiveBMCSecretRef.Name {
@@ -123,7 +117,7 @@ func (r *BMCUserReconciler) reconcile(ctx context.Context, user *metalv1alpha1.B
 	return r.handleRotatingPassword(ctx, user, bmcObj, bmcClient)
 }
 
-func (r *BMCUserReconciler) patchUserStatus(ctx context.Context, user *metalv1alpha1.BMCUser, bmcClient bmc.BMC) error {
+func (r *BMCUserReconciler) patchUserStatus(ctx context.Context, user *metalv1alpha1.BMCUser, bmcClient bmc.BMC, lastRotation metav1.Time) error {
 	log := ctrl.LoggerFrom(ctx)
 	accounts, err := bmcClient.GetAccounts()
 	if err != nil {
@@ -134,6 +128,9 @@ func (r *BMCUserReconciler) patchUserStatus(ctx context.Context, user *metalv1al
 			log.V(1).Info("BMC account already exists", "ID", account.ID)
 			userBase := user.DeepCopy()
 			user.Status.ID = account.ID
+			if !lastRotation.IsZero() {
+				user.Status.LastRotation = &lastRotation
+			}
 			// Only parse password expiration if it's set (empty string is valid)
 			if account.PasswordExpiration != "" {
 				exp, err := time.Parse(time.RFC3339, account.PasswordExpiration)
@@ -148,7 +145,6 @@ func (r *BMCUserReconciler) patchUserStatus(ctx context.Context, user *metalv1al
 			}
 			log.V(1).Info("Updated User status with BMC account ID", "AccountID", account.ID)
 			return nil
-
 		}
 	}
 	return nil
@@ -197,12 +193,12 @@ func (r *BMCUserReconciler) handleRotatingPassword(ctx context.Context, user *me
 	if err != nil {
 		return ctrl.Result{}, fmt.Errorf("failed to create BMCSecret: %w", err)
 	}
-	if err := r.setBMCUserSecretRef(ctx, user, secret); err != nil {
-		return ctrl.Result{}, fmt.Errorf("failed to set BMCSecret reference for User: %w", err)
-	}
 	if err := bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, newPassword, true); err != nil {
 		return ctrl.Result{}, fmt.Errorf("failed to create or update BMC account with new password: %w", err)
 	}
+	if err := r.setBMCUserSecretRef(ctx, user, secret); err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to set BMCSecret reference for User: %w", err)
+	}
 
 	// Update the last rotation time
 	userBase := user.DeepCopy()
@@ -235,11 +231,6 @@ func (r *BMCUserReconciler) ensureBMCSecretForUser(ctx context.Context, bmcClien
 	if err := r.setBMCUserSecretRef(ctx, user, secret); err != nil {
 		return fmt.Errorf("failed to set BMCSecret reference for User: %w", err)
 	}
-	log.V(1).Info("Creating BMC account with new password", "Account", user.Name)
-	if err := bmcClient.CreateOrUpdateAccount(ctx, user.Spec.UserName, user.Spec.RoleID, newPassword, true); err != nil {
-		return fmt.Errorf("failed to create or update BMC account with new password: %w", err)
-	}
-	log.V(1).Info("BMC account created with new password", "Account", user.Name)
 	return nil
 }