Address review comments on CredentialRotation PR

- Remove SetOwnerReference in CLI (controller handles it; kubeClient
  has no Scheme() method)
- Reuse existing ROTATION_ID from source Secret in handleStartRotation
  to avoid wedging on crash recovery between Secret update and status
  update
- Fail closed in reconcileV1Secret guard: return error on transient
  CredentialRotation Get failures instead of proceeding with
  distribution
- Fix test assertion: use Equal("") instead of BeElementOf with
  duplicate values

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

Author: shunki-fujita

cmd/kubectl-moco/cmd/credential.go

@@ -11,7 +11,6 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
 var credentialConfig struct {
@@ -119,9 +118,7 @@ func credentialRotate(ctx context.Context, clusterName string) error {
 				DiscardOldPassword: false,
 			},
 		}
-		if err := controllerutil.SetOwnerReference(cluster, cr, kubeClient.Scheme()); err != nil {
-			return fmt.Errorf("failed to set ownerReference: %w", err)
-		}
+		// ownerReference is set by the controller on first reconcile.
 		if err := kubeClient.Create(ctx, cr); err != nil {
 			return fmt.Errorf("failed to create CredentialRotation: %w", err)
 		}

controllers/credentialrotation_controller.go

@@ -114,9 +114,6 @@ func (r *CredentialRotationReconciler) handleStartRotation(ctx context.Context,
 		return ctrl.Result{RequeueAfter: credRotationRequeueInterval}, nil
 	}
 
-	// Generate rotationID
-	rotationID := uuid.New().String()
-
 	// Get the source Secret
 	sourceSecret := &corev1.Secret{}
 	secretName := cluster.ControllerSecretName()
@@ -127,7 +124,14 @@ func (r *CredentialRotationReconciler) handleStartRotation(ctx context.Context,
 		return ctrl.Result{}, fmt.Errorf("failed to get source secret: %w", err)
 	}
 
-	// Generate pending passwords
+	// Reuse existing rotationID if the Secret already has complete pending
+	// passwords (crash recovery: Secret was updated but status was not).
+	rotationID := password.GetRotationID(sourceSecret)
+	if rotationID == "" {
+		rotationID = uuid.New().String()
+	}
+
+	// Generate pending passwords (idempotent if rotationID matches)
 	_, err := password.SetPendingPasswords(sourceSecret, rotationID)
 	if err != nil {
 		r.Recorder.Eventf(cr, corev1.EventTypeWarning, "RotationPendingError",
@@ -167,11 +171,20 @@ func (r *CredentialRotationReconciler) handleRetainedPhase(ctx context.Context,
 		return ctrl.Result{}, fmt.Errorf("failed to get source secret: %w", err)
 	}
 
+	// Verify pending passwords belong to this rotation cycle.
+	if hasPending, err := password.HasPendingPasswords(sourceSecret, cr.Status.RotationID); err != nil {
+		r.Recorder.Eventf(cr, corev1.EventTypeWarning, "RotationPendingError",
+			"Pending password state inconsistency: %v", err)
+		return ctrl.Result{}, fmt.Errorf("failed to verify pending passwords: %w", err)
+	} else if !hasPending {
+		r.Recorder.Eventf(cr, corev1.EventTypeWarning, "MissingRotationPending",
+			"Pending passwords not found in source secret for rotationID %s", cr.Status.RotationID)
+		return ctrl.Result{}, fmt.Errorf("pending passwords not found for rotationID %s", cr.Status.RotationID)
+	}
+
 	// Distribute pending passwords to per-namespace user Secret
 	pendingPasswd, err := password.NewMySQLPasswordFromPending(sourceSecret)
 	if err != nil {
-		r.Recorder.Eventf(cr, corev1.EventTypeWarning, "MissingRotationPending",
-			"Pending passwords not found in source secret: %v", err)
 		return ctrl.Result{}, fmt.Errorf("failed to read pending passwords: %w", err)
 	}
 

controllers/credentialrotation_controller_test.go

@@ -515,14 +515,14 @@ var _ = Describe("CredentialRotation reconciler", func() {
 		err = k8sClient.Create(ctx, cr)
 		Expect(err).NotTo(HaveOccurred())
 
-		// Phase should NOT advance past Rotating (reconciler emits warning and requeues).
+		// Phase should remain empty (reconciler refuses and requeues without advancing).
 		Consistently(func() mocov1beta2.RotationPhase {
 			cr := &mocov1beta2.CredentialRotation{}
 			if err := k8sClient.Get(ctx, client.ObjectKey{Namespace: "test", Name: "test"}, cr); err != nil {
 				return ""
 			}
 			return cr.Status.Phase
-		}).Should(BeElementOf(mocov1beta2.RotationPhase(""), mocov1beta2.RotationPhase("")))
+		}).Should(Equal(mocov1beta2.RotationPhase("")))
 	})
 
 	It("should not advance past Rotated without discardOldPassword", func() {

controllers/mysqlcluster_controller.go

@@ -348,6 +348,7 @@ func (r *MySQLClusterReconciler) reconcileV1Secret(ctx context.Context, req ctrl
 
 	// During credential rotation, the CredentialRotationReconciler owns
 	// secret distribution. Skip to prevent overwriting pending passwords.
+	// Fail closed on transient errors to avoid distributing stale passwords.
 	var cr mocov1beta2.CredentialRotation
 	if err := r.Get(ctx, client.ObjectKey{
 		Namespace: cluster.Namespace,
@@ -358,6 +359,8 @@ func (r *MySQLClusterReconciler) reconcileV1Secret(ctx context.Context, req ctrl
 			log.Info("credential rotation in progress, skipping secret distribution", "phase", phase)
 			return nil
 		}
+	} else if !apierrors.IsNotFound(err) {
+		return fmt.Errorf("failed to check CredentialRotation status: %w", err)
 	}
 
 	if err := r.reconcileUserSecret(ctx, req, cluster, secret); err != nil {

go.mod

@@ -39,6 +39,8 @@ require (
 	sigs.k8s.io/controller-runtime v0.22.0
 )
 
+require github.com/google/uuid v1.6.0
+
 require (
 	cel.dev/expr v0.24.0 // indirect
 	cloud.google.com/go v0.116.0 // indirect
@@ -97,7 +99,6 @@ require (
 	github.com/google/gnostic-models v0.7.0 // indirect
 	github.com/google/pprof v0.0.0-20250820193118-f64d9cf942d6 // indirect
 	github.com/google/s2a-go v0.1.9 // indirect
-	github.com/google/uuid v1.6.0 // indirect
 	github.com/googleapis/enterprise-certificate-proxy v0.3.4 // indirect
 	github.com/googleapis/gax-go/v2 v2.14.1 // indirect
 	github.com/gorilla/websocket v1.5.4-0.20250319132907-e064f32e3674 // indirect

pkg/password/rotation.go

@@ -63,6 +63,14 @@ var userToPendingKey = map[string]string{
 	constants.WritableUser:    WritablePasswordPendingKey,
 }
 
+// GetRotationID returns the stored ROTATION_ID from the secret, or "" if absent.
+func GetRotationID(secret *corev1.Secret) string {
+	if secret.Data == nil {
+		return ""
+	}
+	return string(secret.Data[RotationIDKey])
+}
+
 // HasPendingPasswords validates the pending state of a source secret.
 //
 // Returns: