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.
- Update the design doc to match the implementation (MySQLClusterReconciler
  code, DISCARD idempotency, stale-CR handling, MySQLCluster deletion,
  validation webhook).
- 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_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{}
 
@@ -132,6 +133,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)
 }

api/v1beta2/credentialrotation_webhook_test.go

@@ -226,4 +226,162 @@ var _ = Describe("CredentialRotation Webhook", func() {
 			Expect(err).To(HaveOccurred())
 		})
 	})
+
+	Context("ValidateDelete", func() {
+		It("should allow deletion when phase is empty", func() {
+			cluster := makeMySQLCluster()
+			err := k8sClient.Create(ctx, cluster)
+			Expect(err).NotTo(HaveOccurred())
+
+			cr := makeCredentialRotation("test", 1)
+			err = k8sClient.Create(ctx, cr)
+			Expect(err).NotTo(HaveOccurred())
+
+			err = k8sClient.Delete(ctx, cr)
+			Expect(err).NotTo(HaveOccurred())
+		})
+
+		It("should allow deletion when phase is Completed", func() {
+			cluster := makeMySQLCluster()
+			err := k8sClient.Create(ctx, cluster)
+			Expect(err).NotTo(HaveOccurred())
+
+			cr := makeCredentialRotation("test", 1)
+			err = k8sClient.Create(ctx, cr)
+			Expect(err).NotTo(HaveOccurred())
+
+			cr.Status.Phase = mocov1beta2.RotationPhaseCompleted
+			err = k8sClient.Status().Update(ctx, cr)
+			Expect(err).NotTo(HaveOccurred())
+
+			err = k8sClient.Delete(ctx, cr)
+			Expect(err).NotTo(HaveOccurred())
+		})
+
+		It("should allow deletion when the owning MySQLCluster is gone (GC)", func() {
+			cluster := makeMySQLCluster()
+			err := k8sClient.Create(ctx, cluster)
+			Expect(err).NotTo(HaveOccurred())
+
+			cr := makeCredentialRotation("test", 1)
+			err = k8sClient.Create(ctx, cr)
+			Expect(err).NotTo(HaveOccurred())
+
+			cr.Status.Phase = mocov1beta2.RotationPhaseRotating
+			err = k8sClient.Status().Update(ctx, cr)
+			Expect(err).NotTo(HaveOccurred())
+
+			// Delete the MySQLCluster first to simulate GC ordering after owner removal.
+			err = k8sClient.Delete(ctx, cluster)
+			Expect(err).NotTo(HaveOccurred())
+			Eventually(func() bool {
+				c := &mocov1beta2.MySQLCluster{}
+				return k8sClient.Get(ctx, client.ObjectKeyFromObject(cluster), c) != nil
+			}).Should(BeTrue())
+
+			err = k8sClient.Delete(ctx, cr)
+			Expect(err).NotTo(HaveOccurred())
+		})
+
+		It("should allow deletion when the owning cluster is terminating (GC unblock)", func() {
+			cluster := makeMySQLCluster()
+			cluster.Finalizers = []string{"moco.cybozu.com/test-block-delete"}
+			err := k8sClient.Create(ctx, cluster)
+			Expect(err).NotTo(HaveOccurred())
+
+			cr := makeCredentialRotation("test", 1)
+			err = k8sClient.Create(ctx, cr)
+			Expect(err).NotTo(HaveOccurred())
+
+			cr.Status.Phase = mocov1beta2.RotationPhaseRotating
+			err = k8sClient.Status().Update(ctx, cr)
+			Expect(err).NotTo(HaveOccurred())
+
+			// Delete the cluster — finalizer keeps it in Terminating state.
+			err = k8sClient.Delete(ctx, cluster)
+			Expect(err).NotTo(HaveOccurred())
+			Eventually(func() bool {
+				c := &mocov1beta2.MySQLCluster{}
+				if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(cluster), c); err != nil {
+					return false
+				}
+				return c.DeletionTimestamp != nil
+			}).Should(BeTrue())
+
+			// GC delete of the CR must be allowed so the cluster can finish terminating.
+			err = k8sClient.Delete(ctx, cr)
+			Expect(err).NotTo(HaveOccurred())
+
+			// Clean up: remove the finalizer so the cluster can be deleted.
+			Eventually(func() error {
+				c := &mocov1beta2.MySQLCluster{}
+				if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(cluster), c); err != nil {
+					return err
+				}
+				c.Finalizers = nil
+				return k8sClient.Update(ctx, c)
+			}).Should(Succeed())
+		})
+
+		It("should allow deletion when ownerReference points to a recreated cluster (different UID)", func() {
+			cluster := makeMySQLCluster()
+			err := k8sClient.Create(ctx, cluster)
+			Expect(err).NotTo(HaveOccurred())
+
+			cr := makeCredentialRotation("test", 1)
+			// Stale ownerRef pointing at a UID that no live cluster has.
+			cr.OwnerReferences = []metav1.OwnerReference{{
+				APIVersion: mocov1beta2.GroupVersion.String(),
+				Kind:       "MySQLCluster",
+				Name:       cluster.Name,
+				UID:        "stale-uid",
+				Controller: new(true),
+			}}
+			err = k8sClient.Create(ctx, cr)
+			Expect(err).NotTo(HaveOccurred())
+
+			cr.Status.Phase = mocov1beta2.RotationPhaseRotating
+			err = k8sClient.Status().Update(ctx, cr)
+			Expect(err).NotTo(HaveOccurred())
+
+			err = k8sClient.Delete(ctx, cr)
+			Expect(err).NotTo(HaveOccurred())
+		})
+
+		It("should reject deletion when phase is active", func() {
+			for _, phase := range []mocov1beta2.RotationPhase{
+				mocov1beta2.RotationPhaseRotating,
+				mocov1beta2.RotationPhaseRetained,
+				mocov1beta2.RotationPhaseRotated,
+				mocov1beta2.RotationPhaseDiscarding,
+				mocov1beta2.RotationPhaseDiscarded,
+			} {
+				cluster := makeMySQLCluster()
+				err := k8sClient.Create(ctx, cluster)
+				Expect(err).NotTo(HaveOccurred())
+
+				cr := makeCredentialRotation("test", 1)
+				err = k8sClient.Create(ctx, cr)
+				Expect(err).NotTo(HaveOccurred())
+
+				cr.Status.Phase = phase
+				err = k8sClient.Status().Update(ctx, cr)
+				Expect(err).NotTo(HaveOccurred())
+
+				err = k8sClient.Delete(ctx, cr)
+				Expect(err).To(HaveOccurred(), "phase=%s should reject delete", phase)
+
+				// Force-cleanup via status reset for next iteration.
+				err = k8sClient.Get(ctx, client.ObjectKeyFromObject(cr), cr)
+				Expect(err).NotTo(HaveOccurred())
+				cr.Status.Phase = mocov1beta2.RotationPhaseCompleted
+				err = k8sClient.Status().Update(ctx, cr)
+				Expect(err).NotTo(HaveOccurred())
+				err = k8sClient.Delete(ctx, cr)
+				Expect(err).NotTo(HaveOccurred())
+				err = deleteMySQLCluster()
+				Expect(err).NotTo(HaveOccurred())
+			}
+		})
+	})
 })

charts/moco/templates/generated/crds/moco_crds.yaml

@@ -2761,7 +2761,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.19.0
+    controller-gen.kubebuilder.io/version: v0.20.1
     helm.sh/resource-policy: keep
   labels:
     app.kubernetes.io/managed-by: '{{ .Release.Service }}'

charts/moco/templates/generated/generated.yaml

@@ -524,6 +524,7 @@ webhooks:
         operations:
           - CREATE
           - UPDATE
+          - DELETE
         resources:
           - credentialrotations
     sideEffects: None

clustering/manager_test.go

@@ -1129,6 +1129,12 @@ var _ = Describe("manager", func() {
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      "test",
 				Namespace: "test",
+				OwnerReferences: []metav1.OwnerReference{{
+					APIVersion: mocov1beta2.GroupVersion.String(),
+					Kind:       "MySQLCluster",
+					Name:       cluster.Name,
+					UID:        cluster.UID,
+				}},
 			},
 			Spec: mocov1beta2.CredentialRotationSpec{
 				RotationGeneration: 1,
@@ -1195,11 +1201,25 @@ var _ = Describe("manager", func() {
 		// Set user auth plugin to something different to trigger migration.
 		of.setUserAuthPlugin("mysql_native_password")
 
+		// Mimic post-RETAIN state: every user holds a retained (dual) password
+		// on every instance. DISCARD OLD PASSWORD is now gated on this state.
+		for i := range 3 {
+			for _, user := range constants.MocoUsers {
+				of.setDualPassword(cluster.PodHostname(i), user, true)
+			}
+		}
+
 		// Create a CredentialRotation CR in Discarding phase.
 		cr := &mocov1beta2.CredentialRotation{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      "test",
 				Namespace: "test",
+				OwnerReferences: []metav1.OwnerReference{{
+					APIVersion: mocov1beta2.GroupVersion.String(),
+					Kind:       "MySQLCluster",
+					Name:       cluster.Name,
+					UID:        cluster.UID,
+				}},
 			},
 			Spec: mocov1beta2.CredentialRotationSpec{
 				RotationGeneration: 1,
@@ -1275,6 +1295,12 @@ var _ = Describe("manager", func() {
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      "test",
 				Namespace: "test",
+				OwnerReferences: []metav1.OwnerReference{{
+					APIVersion: mocov1beta2.GroupVersion.String(),
+					Kind:       "MySQLCluster",
+					Name:       cluster.Name,
+					UID:        cluster.UID,
+				}},
 			},
 			Spec: mocov1beta2.CredentialRotationSpec{
 				RotationGeneration: 1,
@@ -1300,4 +1326,77 @@ var _ = Describe("manager", func() {
 		Expect(users).To(HaveLen(len(constants.MocoUsers) - 1))
 		Expect(users).NotTo(ContainElement(constants.AdminUser))
 	})
+
+	It("should skip DISCARD for users without retained password (partial retry)", func() {
+		testSetupResources(ctx, 1, "")
+
+		cm := NewClusterManager(1*time.Second, mgr, of, af, stdr.New(nil), "test")
+		defer cm.StopAll()
+
+		cluster, err := testGetCluster(ctx)
+		Expect(err).NotTo(HaveOccurred())
+		cm.Update(client.ObjectKeyFromObject(cluster), "test")
+
+		Eventually(func(g Gomega) {
+			cluster, err = testGetCluster(ctx)
+			g.Expect(err).NotTo(HaveOccurred())
+			condHealthy, err := testGetCondition(cluster, mocov1beta2.ConditionHealthy)
+			g.Expect(err).NotTo(HaveOccurred())
+			g.Expect(condHealthy.Status).To(Equal(metav1.ConditionTrue))
+		}).Should(Succeed())
+
+		controllerSecret := mysqlPassword.ToSecret()
+		controllerSecret.Namespace = "test"
+		controllerSecret.Name = cluster.ControllerSecretName()
+		rotationID := "test-rotation-discard-idempotent"
+		_, err = password.SetPendingPasswords(controllerSecret, rotationID)
+		Expect(err).NotTo(HaveOccurred())
+		err = k8sClient.Create(ctx, controllerSecret)
+		Expect(err).NotTo(HaveOccurred())
+
+		// Simulate a partial-progress retry: AdminUser already had DISCARD
+		// applied (no retained password remains), while the rest still hold a
+		// retained password from the prior RETAIN phase.
+		for _, user := range constants.MocoUsers {
+			if user == constants.AdminUser {
+				continue
+			}
+			of.setDualPassword(cluster.PodHostname(0), user, true)
+		}
+
+		cr := &mocov1beta2.CredentialRotation{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test",
+				Namespace: "test",
+				OwnerReferences: []metav1.OwnerReference{{
+					APIVersion: mocov1beta2.GroupVersion.String(),
+					Kind:       "MySQLCluster",
+					Name:       cluster.Name,
+					UID:        cluster.UID,
+				}},
+			},
+			Spec: mocov1beta2.CredentialRotationSpec{
+				RotationGeneration: 1,
+				DiscardOldPassword: true,
+			},
+		}
+		err = k8sClient.Create(ctx, cr)
+		Expect(err).NotTo(HaveOccurred())
+		cr.Status.Phase = mocov1beta2.RotationPhaseDiscarding
+		cr.Status.RotationID = rotationID
+		err = k8sClient.Status().Update(ctx, cr)
+		Expect(err).NotTo(HaveOccurred())
+
+		Eventually(func(g Gomega) {
+			cr := &mocov1beta2.CredentialRotation{}
+			err := k8sClient.Get(ctx, client.ObjectKey{Namespace: "test", Name: "test"}, cr)
+			g.Expect(err).NotTo(HaveOccurred())
+			g.Expect(cr.Status.Phase).To(Equal(mocov1beta2.RotationPhaseDiscarded))
+		}).Should(Succeed())
+
+		// AdminUser should have been skipped; others should have been discarded.
+		users := of.getDiscardedUsers(cluster.PodHostname(0))
+		Expect(users).To(HaveLen(len(constants.MocoUsers) - 1))
+		Expect(users).NotTo(ContainElement(constants.AdminUser))
+	})
 })