Fix CodeRabbit AI review findings

Resolves all 7 CodeRabbit AI issues from PR review:

1. Fix controller requeue bug that could delay expiration past status.ExpiresAt
   - Change warning period requeue to min(5min, timeUntilExpiration)

2. Refactor webhook validation to close validation gaps
   - Extract validateBMCUserExpirationSpec helper for DRY code
   - Add ttlChanged and expiresAtChanged helpers for nil-safe comparisons
   - Apply full validation on updates (future dates, TTL ranges)
   - Warn on all expiration field changes (removals, switches, modifications)
   - Add comprehensive test coverage (13 new test cases, 346 lines)

3. Add missing CRD list markers for proper API server merge behavior
   - Add +listType=map and +listMapKey=type to Conditions field

4. Update sample YAML timestamps from 2026 to 2099 to prevent validation failures

5. Update documentation example timestamps from 2026 to 2099

6. Clarify ExpiresAt field documentation for permanence condition

7. Fix test suite consistency by reusing shared accessor variable

Additional improvement:
- Reduce TTL maximum from 10 years to 1 week for more reasonable temporary access

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

Author: stefanhipfel

api/v1alpha1/bmcuser_types.go

@@ -43,7 +43,7 @@ type BMCUserSpec struct {
 
 	// ExpiresAt specifies an absolute timestamp when this user should be deleted.
 	// This is useful for users that need to expire at a specific time.
-	// If not set along with TTL, the user is permanent.
+	// If neither ExpiresAt nor TTL is set, the user is permanent (no automatic deletion).
 	// Mutually exclusive with TTL - only one should be set.
 	// +optional
 	ExpiresAt *metav1.Time `json:"expiresAt,omitempty"`
@@ -73,6 +73,8 @@ type BMCUserStatus struct {
 	// Conditions represents the latest available observations of the BMCUser's current state.
 	// +patchStrategy=merge
 	// +patchMergeKey=type
+	// +listType=map
+	// +listMapKey=type
 	// +optional
 	Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`
 }

config/crd/bases/metal.ironcore.dev_bmcusers.yaml

@@ -102,7 +102,7 @@ spec:
                 description: |-
                   ExpiresAt specifies an absolute timestamp when this user should be deleted.
                   This is useful for users that need to expire at a specific time.
-                  If not set along with TTL, the user is permanent.
+                  If neither ExpiresAt nor TTL is set, the user is permanent (no automatic deletion).
                   Mutually exclusive with TTL - only one should be set.
                 format: date-time
                 type: string
@@ -194,6 +194,9 @@ spec:
                   - type
                   type: object
                 type: array
+                x-kubernetes-list-map-keys:
+                - type
+                x-kubernetes-list-type: map
               effectiveBMCSecretRef:
                 description: |-
                   EffectiveBMCSecretRef references the BMCSecret currently used for this user.

config/samples/metal_v1alpha1_bmcuser_temporary.yaml

@@ -36,7 +36,8 @@ spec:
   description: "User for maintenance window"
 
   # Absolute expiration at specific time (end of maintenance window)
-  expiresAt: "2026-04-11T02:00:00Z"
+  # Replace with future date before applying
+  expiresAt: "2099-04-11T02:00:00Z"
 
   bmcRef:
     name: bmc-sample

docs/usage/bmcuser-ttl.md

@@ -37,7 +37,7 @@ metadata:
 spec:
   userName: maintenance
   roleID: "Operator"
-  expiresAt: "2026-04-11T02:00:00Z"  # User expires at this time
+  expiresAt: "2099-04-11T02:00:00Z"  # User expires at this time
   bmcRef:
     name: my-bmc
 ```
@@ -103,11 +103,11 @@ spec:
 apiVersion: metal.ironcore.dev/v1alpha1
 kind: BMCUser
 metadata:
-  name: maintenance-2026-04-10
+  name: maintenance-2099-04-10
 spec:
   userName: maintenance-tech
   roleID: "Operator"
-  expiresAt: "2026-04-11T06:00:00Z"
+  expiresAt: "2099-04-11T06:00:00Z"
   bmcRef:
     name: datacenter-bmc
 ```

internal/controller/bmcuser_controller.go

@@ -642,8 +642,8 @@ func (r *BMCUserReconciler) coordinateRequeue(user *metalv1alpha1.BMCUser, rotat
 		// Not in warning period yet - requeue when warning starts
 		expirationRequeue = timeUntilExpiration - warningThreshold
 	} else {
-		// In warning period - check every 5 minutes for expiration
-		expirationRequeue = 5 * time.Minute
+		// In warning period - check frequently, but not past expiration
+		expirationRequeue = min(5*time.Minute, timeUntilExpiration)
 	}
 
 	// Choose the earlier of rotation or expiration requeue

internal/controller/suite_test.go

@@ -324,7 +324,7 @@ func SetupTest(redfishMockServers []netip.AddrPort) *corev1.Namespace {
 			Scheme:             k8sManager.GetScheme(),
 			DefaultProtocol:    metalv1alpha1.HTTPProtocolScheme,
 			SkipCertValidation: true,
-			Conditions:         conditionutils.NewAccessor(conditionutils.AccessorOptions{}),
+			Conditions:         accessor,
 			BMCOptions: bmc.Options{
 				PowerPollingInterval: 50 * time.Millisecond,
 				PowerPollingTimeout:  200 * time.Millisecond,

internal/webhook/v1alpha1/bmcuser_webhook.go

@@ -35,33 +35,60 @@ type BMCUserCustomValidator struct {
 	Client client.Client
 }
 
-// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type BMCUser.
-func (v *BMCUserCustomValidator) ValidateCreate(_ context.Context, obj *metalv1alpha1.BMCUser) (admission.Warnings, error) {
-	bmcuserlog.Info("Validation for BMCUser upon creation", "name", obj.GetName())
-
+// validateBMCUserExpirationSpec validates TTL and ExpiresAt fields.
+// Returns error if validation fails.
+func validateBMCUserExpirationSpec(spec *metalv1alpha1.BMCUserSpec) error {
 	// Validate TTL and ExpiresAt are mutually exclusive
-	if obj.Spec.TTL != nil && obj.Spec.ExpiresAt != nil {
-		return nil, fmt.Errorf("spec.ttl and spec.expiresAt are mutually exclusive")
+	if spec.TTL != nil && spec.ExpiresAt != nil {
+		return fmt.Errorf("spec.ttl and spec.expiresAt are mutually exclusive")
 	}
 
 	// Validate ExpiresAt is in the future
-	if obj.Spec.ExpiresAt != nil {
-		if obj.Spec.ExpiresAt.Before(&metav1.Time{Time: time.Now()}) {
-			return nil, fmt.Errorf("spec.expiresAt must be in the future")
+	if spec.ExpiresAt != nil {
+		if spec.ExpiresAt.Before(&metav1.Time{Time: time.Now()}) {
+			return fmt.Errorf("spec.expiresAt must be in the future")
 		}
 	}
 
-	// Validate TTL is reasonable (> 0, < 10 years)
-	if obj.Spec.TTL != nil {
-		if obj.Spec.TTL.Duration <= 0 {
-			return nil, fmt.Errorf("spec.ttl must be positive")
+	// Validate TTL is reasonable (> 0, <= 1 week)
+	if spec.TTL != nil {
+		if spec.TTL.Duration <= 0 {
+			return fmt.Errorf("spec.ttl must be positive")
 		}
-		if obj.Spec.TTL.Duration > 87600*time.Hour { // 10 years
-			return nil, fmt.Errorf("spec.ttl exceeds maximum of 10 years")
+		if spec.TTL.Duration > 168*time.Hour { // 1 week
+			return fmt.Errorf("spec.ttl exceeds maximum of 1 week")
 		}
 	}
 
-	return nil, nil
+	return nil
+}
+
+// ttlChanged returns true if TTL field changed between old and new objects.
+func ttlChanged(oldTTL, newTTL *metav1.Duration) bool {
+	if oldTTL == nil && newTTL == nil {
+		return false
+	}
+	if oldTTL == nil || newTTL == nil {
+		return true // one is nil, other isn't
+	}
+	return oldTTL.Duration != newTTL.Duration
+}
+
+// expiresAtChanged returns true if ExpiresAt field changed between old and new objects.
+func expiresAtChanged(oldTime, newTime *metav1.Time) bool {
+	if oldTime == nil && newTime == nil {
+		return false
+	}
+	if oldTime == nil || newTime == nil {
+		return true // one is nil, other isn't
+	}
+	return !oldTime.Equal(newTime)
+}
+
+// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type BMCUser.
+func (v *BMCUserCustomValidator) ValidateCreate(_ context.Context, obj *metalv1alpha1.BMCUser) (admission.Warnings, error) {
+	bmcuserlog.Info("Validation for BMCUser upon creation", "name", obj.GetName())
+	return nil, validateBMCUserExpirationSpec(&obj.Spec)
 }
 
 // ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type BMCUser.
@@ -70,24 +97,20 @@ func (v *BMCUserCustomValidator) ValidateUpdate(_ context.Context, oldObj, newOb
 
 	var warnings admission.Warnings
 
-	// Validate TTL and ExpiresAt are mutually exclusive
-	if newObj.Spec.TTL != nil && newObj.Spec.ExpiresAt != nil {
-		return warnings, fmt.Errorf("spec.ttl and spec.expiresAt are mutually exclusive")
+	// Apply same validation as create
+	if err := validateBMCUserExpirationSpec(&newObj.Spec); err != nil {
+		return warnings, err
 	}
 
 	// Warn if TTL/ExpiresAt changed after expiration was calculated
 	if oldObj.Status.ExpiresAt != nil {
-		if oldObj.Spec.TTL != nil && newObj.Spec.TTL != nil {
-			if oldObj.Spec.TTL.Duration != newObj.Spec.TTL.Duration {
-				warnings = append(warnings,
-					"TTL was changed but expiration time is already set and will not be recalculated")
-			}
+		if ttlChanged(oldObj.Spec.TTL, newObj.Spec.TTL) {
+			warnings = append(warnings,
+				"TTL was changed but expiration time is already set and will not be recalculated")
 		}
-		if oldObj.Spec.ExpiresAt != nil && newObj.Spec.ExpiresAt != nil {
-			if !oldObj.Spec.ExpiresAt.Equal(newObj.Spec.ExpiresAt) {
-				warnings = append(warnings,
-					"ExpiresAt was changed but expiration time is already calculated and will not be updated")
-			}
+		if expiresAtChanged(oldObj.Spec.ExpiresAt, newObj.Spec.ExpiresAt) {
+			warnings = append(warnings,
+				"ExpiresAt was changed but expiration time is already calculated and will not be updated")
 		}
 	}