fix: disable auth before backuping

Author: androndo

cmd/etcd-migrate/main.go

@@ -28,9 +28,9 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/kubernetes"
-	"k8s.io/client-go/rest"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
 	_ "k8s.io/client-go/plugin/pkg/client/auth" // Import all auth providers
+	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -186,24 +186,18 @@ func runMigration(ctx context.Context, cfg *Config, stdin io.Reader, stdout io.W
 		return fmt.Errorf("aborted")
 	}
 
-	// Backup phase: every to-be-adopted cluster is snapshotted before any
-	// mutation; a failed backup excludes that cluster from the apply.
+	var backup func() error
 	if cfg.backupConfigured() {
-		if err := runBackups(ctx, cfg, restCfg, kube, ctrlClient, plans, d, stdout); err != nil {
-			return err
-		}
+		backup = func() error { return runBackups(ctx, cfg, restCfg, kube, ctrlClient, plans, d, stdout) }
 	} else {
 		fmt.Fprintln(stdout, "! pre-adoption backup skipped (--skip-backup)")
 	}
 
-	// Auth phase: the legacy NoPassword root cannot match a credentials
-	// Secret, so auth is switched off on the live etcd; the new operator
-	// re-enables it with the referenced Secret once it takes over.
-	if err := disableAuthForAdoptions(ctx, restCfg, kube, plans, d, facts, stdout); err != nil {
-		return err
-	}
-
-	stats, err := applyPlans(ctx, ctrlClient, dyn, plans, stdout)
+	stats, err := runMutationPhases(
+		func() error { return disableAuthForAdoptions(ctx, restCfg, kube, plans, d, facts, stdout) },
+		backup,
+		func() (applyStats, error) { return applyPlans(ctx, ctrlClient, dyn, plans, stdout) },
+	)
 	if err != nil {
 		return err
 	}
@@ -217,6 +211,33 @@ func runMigration(ctx context.Context, cfg *Config, stdin io.Reader, stdout io.W
 	return errorIfPlanFailed(plans)
 }
 
+// runMutationPhases runs the post-confirmation adoption phases in the order
+// their inter-phase contracts REQUIRE, and returns the apply stats:
+//
+//  1. authDisable — switch auth off on every auth-enabled legacy etcd.
+//  2. backup      — snapshot each to-be-adopted cluster (nil ⇒ --skip-backup).
+//  3. apply       — re-own the data plane and create the new CRs.
+//
+// Auth-disable MUST precede backup: the snapshot Job dials etcd anonymously
+// (cert-only, no user), and etcd gates the Maintenance Snapshot RPC behind
+// auth when it is enabled — so for an auth-enabled cluster (the Cozystack/
+// Kamaji case) the backup can only succeed once auth is off. Running them in
+// the reverse order silently flips exactly those clusters to ActionError and
+// excludes them from adoption. A cluster whose auth-disable fails is itself
+// flipped to ActionError and then skipped by the backup and apply phases, so
+// an unprotected cluster is never adopted.
+func runMutationPhases(authDisable func() error, backup func() error, apply func() (applyStats, error)) (applyStats, error) {
+	if err := authDisable(); err != nil {
+		return applyStats{}, err
+	}
+	if backup != nil {
+		if err := backup(); err != nil {
+			return applyStats{}, err
+		}
+	}
+	return apply()
+}
+
 // mustNamespace/mustName split a pre-validated namespace/name ref.
 func mustNamespace(ref string) string { ns, _, _ := splitRef(ref); return ns }
 func mustName(ref string) string      { _, n, _ := splitRef(ref); return n }

cmd/etcd-migrate/main_test.go

@@ -447,6 +447,73 @@ func TestApplyAdoption(t *testing.T) {
 	}
 }
 
+// TestRunMutationPhases_AuthDisableBeforeBackup pins the inter-phase contract
+// that the snapshot Job depends on: auth-disable MUST run before the backup,
+// because the Job dials etcd anonymously and etcd gates the Maintenance
+// Snapshot RPC behind auth. The historical bug ran backup first, which made
+// the safety backup fail for exactly the auth-enabled clusters the tool
+// targets. This test fails if the order regresses.
+func TestRunMutationPhases_AuthDisableBeforeBackup(t *testing.T) {
+	var order []string
+	authDisable := func() error { order = append(order, "auth"); return nil }
+	backup := func() error { order = append(order, "backup"); return nil }
+	apply := func() (applyStats, error) { order = append(order, "apply"); return applyStats{Adopted: 1}, nil }
+
+	stats, err := runMutationPhases(authDisable, backup, apply)
+	if err != nil {
+		t.Fatalf("runMutationPhases: %v", err)
+	}
+	if stats.Adopted != 1 {
+		t.Errorf("stats not propagated from apply: %+v", stats)
+	}
+	want := []string{"auth", "backup", "apply"}
+	if len(order) != len(want) {
+		t.Fatalf("phase order = %v, want %v", order, want)
+	}
+	for i := range want {
+		if order[i] != want[i] {
+			t.Fatalf("phase order = %v, want %v (auth-disable must precede backup)", order, want)
+		}
+	}
+}
+
+// TestRunMutationPhases_AuthFailureSkipsBackupAndApply: when auth-disable
+// errors, neither backup nor apply runs and the error propagates — an
+// unprotected, still-auth'd cluster must never reach the mutation phases.
+func TestRunMutationPhases_AuthFailureSkipsBackupAndApply(t *testing.T) {
+	wantErr := fmt.Errorf("auth boom")
+	var ran []string
+	_, err := runMutationPhases(
+		func() error { ran = append(ran, "auth"); return wantErr },
+		func() error { ran = append(ran, "backup"); return nil },
+		func() (applyStats, error) { ran = append(ran, "apply"); return applyStats{}, nil },
+	)
+	if err == nil {
+		t.Fatal("expected auth-disable error to propagate")
+	}
+	if len(ran) != 1 || ran[0] != "auth" {
+		t.Errorf("after auth-disable failure, ran = %v; want only [auth]", ran)
+	}
+}
+
+// TestRunMutationPhases_NilBackupSkips: --skip-backup (nil backup fn) still
+// runs auth-disable then apply, in that order.
+func TestRunMutationPhases_NilBackupSkips(t *testing.T) {
+	var order []string
+	_, err := runMutationPhases(
+		func() error { order = append(order, "auth"); return nil },
+		nil,
+		func() (applyStats, error) { order = append(order, "apply"); return applyStats{}, nil },
+	)
+	if err != nil {
+		t.Fatalf("runMutationPhases: %v", err)
+	}
+	want := []string{"auth", "apply"}
+	if len(order) != len(want) || order[0] != want[0] || order[1] != want[1] {
+		t.Errorf("phase order with nil backup = %v, want %v", order, want)
+	}
+}
+
 func assertControllerOwner(t *testing.T, refs []metav1.OwnerReference, kind, name string) {
 	t.Helper()
 	for _, o := range refs {

docs/migration.md

@@ -54,9 +54,11 @@ Per cluster, the tool:
 1. **Inspects** the live etcd (read-only, over a port-forward with the legacy
    operator's client certificate): member list, cluster ID, auth status.
    Runs in dry-run too, so the printed plan shows the real IDs.
-2. **Backs up** the cluster (see below) — before anything is mutated.
-3. **Disables legacy auth** if enabled (the legacy NoPassword root can never
-   match a credentials Secret; the new operator re-enables auth itself).
+2. **Disables legacy auth** if enabled (the legacy NoPassword root can never
+   match a credentials Secret; the new operator re-enables auth itself). This
+   runs **before** the backup on purpose: the snapshot Job dials etcd
+   anonymously, and etcd rejects the Maintenance Snapshot RPC while auth is on.
+3. **Backs up** the cluster (see below) — before anything is mutated.
 4. **Creates the new CRs with prefilled status**: the `EtcdCluster` gets
    `status.clusterID`/`clusterToken`/`observed` (so the operator's bootstrap
    branch never fires against a cluster that already exists), and one
@@ -148,7 +150,9 @@ merge the CA into the referenced Secret **before** starting the new operator
 
 Adoption rewires ownership of live storage, so the tool snapshots every
 cluster to the `--backup-s3-*`/`--backup-pvc-claim` destination **before any
-mutation** (a one-off Job running the operator image's snapshot agent —
+ownership/data-plane mutation** — the only step that precedes it is the
+auth-disable above, which the snapshot Job's anonymous dial depends on (a
+one-off Job running the operator image's snapshot agent —
 `--agent-image` overrides; by default the image is read from the new
 controller Deployment's spec, which works at replicas=0). Nothing is restored
 from the artifact — the data never moves — it exists purely for disaster