Harden CredentialRotation against stale CRs and partial retries

- Self-heal per-namespace user/my.cnf Secrets during active rotation by
  re-applying pending passwords in Rotated/Discarding/Discarded phases.
  Fall back to current passwords during the brief Discarded→Completed
  window where pending keys have been promoted.
- Treat CR Get errors strictly: only NotFound/NoMatch are "no active
  rotation"; transient errors abort reconciliation instead of falling
  back to current passwords.
- Refuse to adopt or act on a stale CredentialRotation (ownerRef UID
  does not match the live cluster). Apply this consistently in the
  CredentialRotation reconciler, ClusterManager, MySQLClusterReconciler,
  and the kubectl-moco CLI.
- Validate CR delete: forbid mid-rotation deletes, but always allow
  garbage collection (owner NotFound, owner Terminating, stale UID).
- Make DISCARD OLD PASSWORD per-user idempotent via HasDualPassword,
  matching the existing RETAIN behavior; required because MySQL rejects
  DISCARD when no retained password remains.
- Reject discard while replicas=0 in handleStartDiscard and emit a
  RotationBlocked Warning Event when handleRotatingPhase is stalled by
  replicas=0.
- Replace spec.discardOldPassword (bool) with spec.discardGeneration
  (int64) to eliminate edge-trigger semantics and the webhook
  transition contortions it required (false->true reject, forced reset
  on rotation bump). discardGeneration is monotonic and must satisfy
  0 <= discardGeneration <= rotationGeneration; bumping it to match
  rotationGeneration triggers discard. Adds status.observedDiscardGeneration
  to mirror the existing rotation pattern, and updates the webhook to
  enforce monotonicity and the upper-bound invariant (phase guards
  remain: rotation bump requires \"\"/Completed, discard bump requires
  Rotated). kubectl-moco credential discard now patches
  discardGeneration to match rotationGeneration instead of toggling a
  bool.
- Update the design doc to match the implementation (MySQLClusterReconciler
  code, DISCARD idempotency, stale-CR handling, MySQLCluster deletion,
  validation webhook, discardGeneration semantics, GitOps-vs-CLI
  guidance).
- Add unit and envtest coverage for the new paths.

Signed-off-by: shunki-fujita <shunki-fujita@cybozu.co.jp>

Author: shunki-fujita

api/v1beta2/credentialrotation_types.go

@@ -13,11 +13,13 @@ type CredentialRotationSpec struct {
 	// +optional
 	RotationGeneration int64 `json:"rotationGeneration,omitempty"`
 
-	// DiscardOldPassword triggers the discard phase.
-	// Can only be set to true when Phase is Rotated.
-	// Must be reset to false when incrementing rotationGeneration.
+	// DiscardGeneration is a monotonically increasing counter that triggers
+	// the discard phase. Must satisfy 0 <= discardGeneration <= rotationGeneration.
+	// Bumping this value (typically to match rotationGeneration) signals the
+	// controller to discard the retained old password from the previous
+	// rotation. The bump is only honored when Phase is Rotated.
 	// +optional
-	DiscardOldPassword bool `json:"discardOldPassword,omitempty"`
+	DiscardGeneration int64 `json:"discardGeneration,omitempty"`
 }
 
 // CredentialRotationStatus defines the observed state of CredentialRotation.
@@ -34,6 +36,11 @@ type CredentialRotationStatus struct {
 	// that completed successfully.
 	// +optional
 	ObservedRotationGeneration int64 `json:"observedRotationGeneration,omitempty"`
+
+	// ObservedDiscardGeneration is the last discardGeneration
+	// that completed successfully.
+	// +optional
+	ObservedDiscardGeneration int64 `json:"observedDiscardGeneration,omitempty"`
 }
 
 // RotationPhase represents the phase of a credential rotation.
@@ -52,8 +59,10 @@ const (
 // +kubebuilder:subresource:status
 // +kubebuilder:storageversion
 // +kubebuilder:printcolumn:name="Phase",type="string",JSONPath=".status.phase"
-// +kubebuilder:printcolumn:name="Generation",type="integer",JSONPath=".spec.rotationGeneration"
-// +kubebuilder:printcolumn:name="Observed",type="integer",JSONPath=".status.observedRotationGeneration"
+// +kubebuilder:printcolumn:name="RotationGen",type="integer",JSONPath=".spec.rotationGeneration"
+// +kubebuilder:printcolumn:name="ObservedRotation",type="integer",JSONPath=".status.observedRotationGeneration"
+// +kubebuilder:printcolumn:name="DiscardGen",type="integer",JSONPath=".spec.discardGeneration"
+// +kubebuilder:printcolumn:name="ObservedDiscard",type="integer",JSONPath=".status.observedDiscardGeneration"
 // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"
 
 // CredentialRotation is the Schema for the credentialrotations API

api/v1beta2/credentialrotation_webhook.go

@@ -2,6 +2,7 @@ package v1beta2
 
 import (
 	"context"
+	"fmt"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -22,7 +23,7 @@ type credentialRotationAdmission struct {
 	client client.Reader
 }
 
-//+kubebuilder:webhook:path=/validate-moco-cybozu-com-v1beta2-credentialrotation,mutating=false,failurePolicy=fail,sideEffects=None,matchPolicy=Equivalent,groups=moco.cybozu.com,resources=credentialrotations,verbs=create;update,versions=v1beta2,name=vcredentialrotation.kb.io,admissionReviewVersions=v1
+//+kubebuilder:webhook:path=/validate-moco-cybozu-com-v1beta2-credentialrotation,mutating=false,failurePolicy=fail,sideEffects=None,matchPolicy=Equivalent,groups=moco.cybozu.com,resources=credentialrotations,verbs=create;update;delete,versions=v1beta2,name=vcredentialrotation.kb.io,admissionReviewVersions=v1
 
 var _ admission.Validator[*CredentialRotation] = &credentialRotationAdmission{}
 
@@ -55,10 +56,14 @@ func (a *credentialRotationAdmission) ValidateCreate(ctx context.Context, cr *Cr
 			cr.Spec.RotationGeneration, "must be > 0"))
 	}
 
-	// discardOldPassword must be false
-	if cr.Spec.DiscardOldPassword {
-		errs = append(errs, field.Invalid(field.NewPath("spec", "discardOldPassword"),
-			cr.Spec.DiscardOldPassword, "must be false on creation"))
+	// discardGeneration must be in [0, rotationGeneration]
+	if cr.Spec.DiscardGeneration < 0 {
+		errs = append(errs, field.Invalid(field.NewPath("spec", "discardGeneration"),
+			cr.Spec.DiscardGeneration, "must be >= 0"))
+	}
+	if cr.Spec.DiscardGeneration > cr.Spec.RotationGeneration {
+		errs = append(errs, field.Invalid(field.NewPath("spec", "discardGeneration"),
+			cr.Spec.DiscardGeneration, "must be <= rotationGeneration"))
 	}
 
 	if len(errs) > 0 {
@@ -76,23 +81,28 @@ func (a *credentialRotationAdmission) ValidateUpdate(ctx context.Context, oldCR,
 		errs = append(errs, field.Invalid(field.NewPath("spec", "rotationGeneration"),
 			newCR.Spec.RotationGeneration, "must be >= previous value (monotonically increasing)"))
 	}
+	// discardGeneration must be monotonically increasing
+	if newCR.Spec.DiscardGeneration < oldCR.Spec.DiscardGeneration {
+		errs = append(errs, field.Invalid(field.NewPath("spec", "discardGeneration"),
+			newCR.Spec.DiscardGeneration, "must be >= previous value (monotonically increasing)"))
+	}
+	// discardGeneration must not exceed rotationGeneration
+	if newCR.Spec.DiscardGeneration > newCR.Spec.RotationGeneration {
+		errs = append(errs, field.Invalid(field.NewPath("spec", "discardGeneration"),
+			newCR.Spec.DiscardGeneration, "must be <= rotationGeneration"))
+	}
 
-	generationChanged := newCR.Spec.RotationGeneration > oldCR.Spec.RotationGeneration
+	rotationIncreased := newCR.Spec.RotationGeneration > oldCR.Spec.RotationGeneration
+	discardIncreased := newCR.Spec.DiscardGeneration > oldCR.Spec.DiscardGeneration
 
-	if generationChanged {
+	if rotationIncreased {
 		// rotationGeneration can only increase when Phase is "" or Completed
 		phase := oldCR.Status.Phase
 		if phase != "" && phase != RotationPhaseCompleted {
 			errs = append(errs, field.Forbidden(field.NewPath("spec", "rotationGeneration"),
 				"can only increment rotationGeneration when phase is empty or Completed"))
 		}
 
-		// When rotationGeneration increases, discardOldPassword must be false
-		if newCR.Spec.DiscardOldPassword {
-			errs = append(errs, field.Invalid(field.NewPath("spec", "discardOldPassword"),
-				newCR.Spec.DiscardOldPassword, "must be false when incrementing rotationGeneration"))
-		}
-
 		// Check MySQLCluster replicas > 0
 		cluster := &MySQLCluster{}
 		if err := a.client.Get(ctx, types.NamespacedName{
@@ -109,19 +119,14 @@ func (a *credentialRotationAdmission) ValidateUpdate(ctx context.Context, oldCR,
 			errs = append(errs, field.Invalid(field.NewPath("metadata", "name"), newCR.Name,
 				"target MySQLCluster must have replicas > 0"))
 		}
-	} else {
-		// When rotationGeneration is unchanged, discardOldPassword can only go false→true
-		if oldCR.Spec.DiscardOldPassword && !newCR.Spec.DiscardOldPassword {
-			errs = append(errs, field.Forbidden(field.NewPath("spec", "discardOldPassword"),
-				"cannot change from true to false without incrementing rotationGeneration"))
-		}
+	}
 
-		// discardOldPassword=true requires Phase==Rotated
-		if newCR.Spec.DiscardOldPassword && !oldCR.Spec.DiscardOldPassword {
-			if oldCR.Status.Phase != RotationPhaseRotated {
-				errs = append(errs, field.Forbidden(field.NewPath("spec", "discardOldPassword"),
-					"can only set to true when phase is Rotated"))
-			}
+	if discardIncreased && !rotationIncreased {
+		// discardGeneration can only increase when Phase is Rotated
+		// (during the same rotation cycle in which discard is being requested).
+		if oldCR.Status.Phase != RotationPhaseRotated {
+			errs = append(errs, field.Forbidden(field.NewPath("spec", "discardGeneration"),
+				"can only increment discardGeneration when phase is Rotated"))
 		}
 	}
 
@@ -132,6 +137,63 @@ func (a *credentialRotationAdmission) ValidateUpdate(ctx context.Context, oldCR,
 	return nil, nil
 }
 
-func (a *credentialRotationAdmission) ValidateDelete(ctx context.Context, _ *CredentialRotation) (admission.Warnings, error) {
-	return nil, nil
+// hasStaleClusterOwnerRef reports whether cr carries a MySQLCluster owner
+// reference that points at a UID different from cluster.UID, with no matching
+// reference. That signals the CR is left over from a deleted cluster that has
+// since been recreated under the same name.
+func hasStaleClusterOwnerRef(cr *CredentialRotation, cluster *MySQLCluster) bool {
+	hasStale := false
+	for _, ref := range cr.OwnerReferences {
+		if ref.Kind != "MySQLCluster" {
+			continue
+		}
+		if ref.UID == cluster.UID {
+			return false
+		}
+		hasStale = true
+	}
+	return hasStale
+}
+
+func (a *credentialRotationAdmission) ValidateDelete(ctx context.Context, cr *CredentialRotation) (admission.Warnings, error) {
+	// Always allow deletion when the rotation is idle/completed.
+	switch cr.Status.Phase {
+	case "", RotationPhaseCompleted:
+		return nil, nil
+	}
+
+	// Allow garbage-collection deletes: if the owning MySQLCluster is gone,
+	// the CR has nothing to act on and must be reclaimable. Blocking GC here
+	// would orphan the CR and poison a future cluster recreated with the
+	// same name (it would inherit a stale in-progress rotation).
+	cluster := &MySQLCluster{}
+	err := a.client.Get(ctx, types.NamespacedName{Namespace: cr.Namespace, Name: cr.Name}, cluster)
+	if apierrors.IsNotFound(err) {
+		return nil, nil
+	}
+	if err != nil {
+		return nil, apierrors.NewInternalError(err)
+	}
+	// The cluster is being torn down. Owner references use blockOwnerDeletion,
+	// so Kubernetes GC must be allowed to remove this CR; otherwise the
+	// MySQLCluster would be stuck in Terminating until the rotation finishes.
+	if cluster.DeletionTimestamp != nil {
+		return nil, nil
+	}
+	// The cluster lookup matches by name only; if the live cluster has a
+	// different UID than the CR's owner, the original cluster has been deleted
+	// and replaced. The CR is stale and must be deletable so it does not
+	// poison the new cluster with leftover rotation state.
+	if hasStaleClusterOwnerRef(cr, cluster) {
+		return nil, nil
+	}
+
+	// Otherwise forbid: a deletion mid-rotation abandons the workflow,
+	// leaving pending/dual passwords on instances with no automatic recovery.
+	errs := field.ErrorList{
+		field.Forbidden(field.NewPath("status", "phase"),
+			fmt.Sprintf("cannot delete CredentialRotation while phase is %q; wait for the rotation to reach Completed", cr.Status.Phase)),
+	}
+	return nil, apierrors.NewInvalid(
+		schema.GroupKind{Group: GroupVersion.Group, Kind: "CredentialRotation"}, cr.Name, errs)
 }