Drop annotation toggle; use plain Get+Update for re-encoding

The annotation toggle was a workaround for a misdiagnosed problem.
Earlier in this branch we observed empty SSAs being elided and
generalised it to "all unmodified writes elide" — and added a
toolhive.stacklok.dev/storage-version-migrated-at annotation toggle
to force a real diff per write. A second-opinion investigation
pointed out the elision check happens AFTER storage encoding: the
apiserver re-encodes the request body at the current storage version,
then byte-compares against etcd. For the actual migration scenario
(CR stamped at an older apiVersion in etcd, storage version now newer)
the encoded apiVersion differs, the comparison fails, and the write
proceeds. Verified empirically: a CR stored at v1alpha1 with storage
flipped to v1beta1 has plain Get+Update bump RV from 202 to 204; a CR
already at v1beta1 has plain Get+Update elide cleanly. Both behaviours
are correct — the elision is exactly the no-migration-needed case.

Changes:
  - restoreOne: plain Get + Update of the unmodified live object. No
    annotation, no timestamp, no MigrationTimestampAnnotation constant.
  - Controller docstring rewritten to describe the actual mechanism
    (encode-then-compare elision) and cite ksvm#65 for posterity.
  - HappyPath envtest loosened: per-CR resourceVersion bump assertions
    are dropped (already-clean CRs correctly elide; checking RV bumps
    on them was an artefact of the annotation-induced writes). The
    storedVersions convergence assertion remains as the contract test.
  - New cross-version envtest "re-encodes CRs that are stored at a
    prior storage version" exercises the real migration scenario:
    install CRD with v1alpha1 storage, create CR, flip storage to
    v1beta1, run reconciler, assert storedVersions trims and the CR's
    RV bumped (proving the apiserver actually rewrote etcd).
  - User docs updated to drop annotation references and describe the
    real mechanism. Reference implementation citation switched from
    cluster-api/crdmigrator to kube-storage-version-migrator (the
    actual upstream SIG tool we share semantics with).
  - FallsBackWhenNoStatusSubresource test removed: the SSA path it
    exercised no longer exists. Plain Update on a CRD without /status
    is just a normal Update; covered by the happy-path spec.

8/8 envtest cases pass. Lint clean. No RBAC drift (verbs already
correct).

Refs PR #5011 review by @jhrozek.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: ChrisJBurns

cmd/thv-operator/controllers/storageversionmigrator_controller.go

@@ -79,20 +79,20 @@ var errMigrationRetriedDueToConflicts = errors.New(
 // StorageVersionMigratorReconciler reconciles CustomResourceDefinition objects
 // in the toolhive.stacklok.dev group that carry the opt-in
 // AutoMigrateLabel=AutoMigrateValue. For each such CRD it re-stores every CR
-// at the current storage version by doing a Get + Update on the live object
-// while toggling the MigrationTimestampAnnotation to force a real content
-// diff. The annotation toggle is required because the apiserver elides no-op
-// writes (an Update with bytes equal to what's already stored does not bump
-// resourceVersion and does not re-encode) — without a real diff the migration
-// would be a hollow operation. After all CRs have been re-stored it patches
+// at the current storage version by doing a Get + Update on the live object.
+// The Update is a full PUT of the unmodified object; the apiserver re-encodes
+// the request body at the current storage version, then compares the
+// resulting bytes to what's in etcd. When the CR was originally stored at a
+// different version (the actual migration scenario) the bytes carry a
+// different apiVersion stamp than etcd's record, the comparison fails, and
+// the write proceeds — re-encoding the object at the current storage
+// version. When the CR is already at the current storage version, the bytes
+// match and the apiserver harmlessly elides the write — there was nothing to
+// migrate. After all CRs have been processed it patches
 // CRD.status.storedVersions down to [<currentStorageVersion>] so a future
 // release can drop deprecated versions from spec.versions without orphaning
-// etcd objects.
-//
-// Webhook interaction: the Update goes through the main resource, so
-// validating/mutating admission webhooks on the kind DO see this write.
-// Webhooks that need to ignore migration-only writes should branch on the
-// presence of MigrationTimestampAnnotation in the diff.
+// etcd objects. See https://github.com/kubernetes-sigs/kube-storage-version-migrator/issues/65
+// for the upstream maintainers' explanation of this mechanism.
 //
 // Enabled by default. Opt out operator-wide via
 // operator.features.storageVersionMigrator (ENABLE_STORAGE_VERSION_MIGRATOR=false)
@@ -323,29 +323,18 @@ func (r *StorageVersionMigratorReconciler) restoreCRs(
 	return kerrors.NewAggregate(errs)
 }
 
-// MigrationTimestampAnnotation is set on each CR by the migrator to force a
-// real content diff on Update. The apiserver's storage layer elides no-op
-// writes (an Update with bytes equal to what's already in etcd does not
-// re-encode and does not bump resourceVersion), so a Get + Update on the live
-// object is not enough on its own — there must be at least one byte of
-// difference. Mutating this annotation guarantees the apiserver re-writes the
-// object, which is exactly the storage-version re-encode we need. The value
-// is the RFC3339Nano timestamp of the most recent successful migration.
-//
-// This matches the upstream kube-storage-version-migrator's approach (which
-// also uses an annotation toggle) and is part of the public contract: do not
-// remove or rename this constant across releases without a migration plan.
-const MigrationTimestampAnnotation = "toolhive.stacklok.dev/storage-version-migrated-at"
-
-// restoreOne issues a Get + Update on the live CR, mutating the migration
-// timestamp annotation to force a real content diff so the apiserver actually
-// re-encodes the object at the current storage version. The Update goes
-// through the main resource (not /status), so any validating/mutating
-// admission webhooks on the kind WILL see this write. CRDs that need to avoid
-// webhook side effects from this controller should configure their webhooks
-// to ignore writes that change only this annotation. Returns the live object
-// after the update so the caller can record its post-update resourceVersion
-// in the cache.
+// restoreOne issues a plain Get + Update on the live CR. The apiserver
+// re-encodes the request body at the current storage version and compares
+// it to etcd's record; when the CR was originally stored at a different
+// apiVersion the bytes differ, the write proceeds, and etcd is re-encoded
+// at the current storage version. When the CR is already at the current
+// storage version the bytes match and the apiserver harmlessly elides the
+// write — there was nothing to migrate. The Update goes through the main
+// resource, so validating/mutating admission webhooks on the kind see this
+// request as part of normal admission flow; only requests that actually
+// persist produce downstream state changes. Returns the live object after
+// the update so the caller can record its post-update resourceVersion in
+// the cache.
 func (r *StorageVersionMigratorReconciler) restoreOne(
 	ctx context.Context,
 	gvk schema.GroupVersionKind,
@@ -357,12 +346,6 @@ func (r *StorageVersionMigratorReconciler) restoreOne(
 		// IsNotFound is propagated to the caller, which handles it.
 		return nil, err
 	}
-	annotations := live.GetAnnotations()
-	if annotations == nil {
-		annotations = map[string]string{}
-	}
-	annotations[MigrationTimestampAnnotation] = time.Now().UTC().Format(time.RFC3339Nano)
-	live.SetAnnotations(annotations)
 	if err := r.Update(ctx, live); err != nil {
 		return nil, err
 	}

cmd/thv-operator/test-integration/storageversionmigrator/controller_test.go

@@ -174,12 +174,15 @@ func createCRs(gvk schema.GroupVersionKind, basename string, count int) []*unstr
 
 // Note on "did the re-store actually fire" verification:
 //
-// The controller now does a Get + Update on /status (or the main resource as a
-// fallback). An unconditional Update always writes the object back to etcd, so
-// the object's resourceVersion bumps on every successful re-store — that's the
-// observable proof a re-encode happened. The HappyPath test snapshots each
-// CR's resourceVersion before reconcile and asserts it has increased after
-// reconcile completes.
+// The controller does a plain Get + Update on each CR. When the CR is already
+// at the current storage version the apiserver freshly re-encodes the request
+// body, sees it matches etcd byte-for-byte, and elides the write — that's
+// correct behaviour, not a controller bug, and it means the per-CR RV does
+// not bump for an already-clean CR. The dedicated cross-version test
+// ("re-encodes CRs that are stored at a prior storage version") proves the
+// migration mechanism actually works for objects stored at older versions:
+// it stores a CR at v1alpha1, flips storage to v1beta1, and asserts the CR's
+// RV bumps after reconcile.
 //
 // The pagination test additionally verifies the continue-token loop via a
 // list-call counter, and the partial-failure test asserts storedVersions is
@@ -245,7 +248,7 @@ var _ = Describe("StorageVersionMigrator", func() {
 			Expect(getStoredVersions(spec.Name)).To(Equal([]string{"v1beta1"}))
 		})
 
-		It("migrates storedVersions from [v1alpha1,v1beta1] to [v1beta1] and re-stores all CRs", func() {
+		It("migrates storedVersions from [v1alpha1,v1beta1] to [v1beta1]", func() {
 			suf := uniqueSuffix()
 			spec := crdSpec{
 				Name:              "happies" + suf + "." + toolhiveGroup,
@@ -264,39 +267,147 @@ var _ = Describe("StorageVersionMigrator", func() {
 			installCRD(spec)
 			DeferCleanup(func() { deleteCRD(spec.Name) })
 
-			crs := createCRs(
+			// The CRs created here are already stored at v1beta1 (the
+			// current storage version), so the apiserver will elide each
+			// per-CR Update — its freshly-encoded body matches etcd
+			// byte-for-byte. That's correct apiserver behaviour, not a
+			// controller bug, so this test does NOT assert per-CR
+			// resourceVersion bumps. The cross-version test below proves
+			// the migration mechanism actually re-encodes objects that were
+			// stored at an older apiVersion.
+			createCRs(
 				schema.GroupVersionKind{Group: spec.Group, Version: "v1beta1", Kind: spec.Kind},
 				"obj-"+suf, 3,
 			)
 
-			// Snapshot pre-reconcile resourceVersions. The controller does
-			// Get + Update on each CR, so a successful re-store must bump RV.
-			// Asserting the bump is the empirical proof the re-encode happened
-			// (an empty SSA would not have bumped RV — that was the bug).
-			preRVs := make(map[string]string, len(crs))
-			for _, cr := range crs {
-				live := &unstructured.Unstructured{}
-				live.SetGroupVersionKind(cr.GroupVersionKind())
-				Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cr), live)).To(Succeed())
-				preRVs[cr.GetName()] = live.GetResourceVersion()
-			}
-
 			setStoredVersions(spec.Name, []string{"v1alpha1", "v1beta1"})
 
 			_, err := reconcile(newReconciler(), spec.Name)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(getStoredVersions(spec.Name)).To(Equal([]string{"v1beta1"}))
+		})
 
-			// Verify every CR still exists AND its RV bumped, proving the
-			// /status Update actually wrote to etcd.
-			for _, cr := range crs {
-				live := &unstructured.Unstructured{}
-				live.SetGroupVersionKind(cr.GroupVersionKind())
-				Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cr), live)).To(Succeed())
-				Expect(live.GetResourceVersion()).NotTo(Equal(preRVs[cr.GetName()]),
-					"CR %s/%s resourceVersion did not bump — re-store did not write to etcd",
-					cr.GetNamespace(), cr.GetName())
+		// Load-bearing proof of the migration mechanism: a CR stored at
+		// v1alpha1, after the storage version has flipped to v1beta1, must
+		// have its resourceVersion bumped by reconcile — that's the
+		// observable evidence the apiserver actually re-encoded the etcd
+		// document. See the upstream confirmation at
+		// https://github.com/kubernetes-sigs/kube-storage-version-migrator/issues/65.
+		It("re-encodes CRs that are stored at a prior storage version", func() {
+			suf := uniqueSuffix()
+			crdName := "crossvers" + suf + "." + toolhiveGroup
+			kind := "CrossVer" + suf
+			plural := "crossvers" + suf
+
+			versionSchema := func() *apiextensionsv1.CustomResourceValidation {
+				return &apiextensionsv1.CustomResourceValidation{
+					OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
+						Type: "object",
+						Properties: map[string]apiextensionsv1.JSONSchemaProps{
+							"spec":   {Type: "object", XPreserveUnknownFields: ptrBool(true)},
+							"status": {Type: "object", XPreserveUnknownFields: ptrBool(true)},
+						},
+					},
+				}
 			}
+
+			// Step 1: install CRD with v1alpha1 as the storage version so
+			// CRs created next are written to etcd as v1alpha1 bytes.
+			crd := &apiextensionsv1.CustomResourceDefinition{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:   crdName,
+					Labels: map[string]string{migrateLabel: migrateValue},
+				},
+				Spec: apiextensionsv1.CustomResourceDefinitionSpec{
+					Group: toolhiveGroup,
+					Names: apiextensionsv1.CustomResourceDefinitionNames{
+						Kind:     kind,
+						ListKind: kind + "List",
+						Plural:   plural,
+						Singular: "crossver" + suf,
+					},
+					Scope: apiextensionsv1.NamespaceScoped,
+					Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
+						{Name: "v1alpha1", Served: true, Storage: true, Schema: versionSchema()},
+						{Name: "v1beta1", Served: true, Storage: false, Schema: versionSchema()},
+					},
+				},
+			}
+			Expect(k8sClient.Create(ctx, crd)).To(Succeed())
+			DeferCleanup(func() { deleteCRD(crdName) })
+
+			Eventually(func() bool {
+				live := &apiextensionsv1.CustomResourceDefinition{}
+				if err := k8sClient.Get(ctx, types.NamespacedName{Name: crdName}, live); err != nil {
+					return false
+				}
+				for _, c := range live.Status.Conditions {
+					if c.Type == apiextensionsv1.Established && c.Status == apiextensionsv1.ConditionTrue {
+						return true
+					}
+				}
+				return false
+			}, time.Second*10, time.Millisecond*200).Should(BeTrue())
+
+			// Step 2: create one CR — etcd writes apiVersion: v1alpha1 bytes.
+			cr := createCRs(
+				schema.GroupVersionKind{Group: toolhiveGroup, Version: "v1alpha1", Kind: kind},
+				"obj-"+suf, 1,
+			)[0]
+
+			// Step 3: flip storage to v1beta1.
+			Eventually(func() error {
+				live := &apiextensionsv1.CustomResourceDefinition{}
+				if err := k8sClient.Get(ctx, types.NamespacedName{Name: crdName}, live); err != nil {
+					return err
+				}
+				orig := live.DeepCopy()
+				for i := range live.Spec.Versions {
+					live.Spec.Versions[i].Storage = (live.Spec.Versions[i].Name == "v1beta1")
+				}
+				return k8sClient.Patch(ctx, live, client.MergeFrom(orig))
+			}, time.Second*10, time.Millisecond*200).Should(Succeed())
+
+			// Confirm the storage flip settled before proceeding.
+			Eventually(func() bool {
+				live := &apiextensionsv1.CustomResourceDefinition{}
+				if err := k8sClient.Get(ctx, types.NamespacedName{Name: crdName}, live); err != nil {
+					return false
+				}
+				for _, v := range live.Spec.Versions {
+					if v.Name == "v1beta1" && v.Storage {
+						return true
+					}
+				}
+				return false
+			}, time.Second*10, time.Millisecond*200).Should(BeTrue())
+
+			// Step 4: storedVersions reflects the historical v1alpha1 entry.
+			setStoredVersions(crdName, []string{"v1alpha1", "v1beta1"})
+
+			// Step 5: snapshot RV before reconcile.
+			preLive := &unstructured.Unstructured{}
+			preLive.SetGroupVersionKind(schema.GroupVersionKind{
+				Group: toolhiveGroup, Version: "v1beta1", Kind: kind,
+			})
+			Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cr), preLive)).To(Succeed())
+			preRV := preLive.GetResourceVersion()
+
+			// Step 6: reconcile.
+			_, err := reconcile(newReconciler(), crdName)
+			Expect(err).NotTo(HaveOccurred())
+
+			// Step 7: storedVersions trimmed.
+			Expect(getStoredVersions(crdName)).To(Equal([]string{"v1beta1"}))
+
+			// Step 8: empirical proof — RV bumped because the cross-version
+			// Update actually wrote etcd.
+			postLive := &unstructured.Unstructured{}
+			postLive.SetGroupVersionKind(preLive.GroupVersionKind())
+			Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cr), postLive)).To(Succeed())
+			Expect(postLive.GetResourceVersion()).NotTo(Equal(preRV),
+				"CR %s/%s resourceVersion did not bump (pre=%s post=%s) — cross-version re-store did not write to etcd",
+				cr.GetNamespace(), cr.GetName(), preRV, postLive.GetResourceVersion())
 		})
 
 		It("skips CRDs in foreign API groups", func() {
@@ -353,42 +464,6 @@ var _ = Describe("StorageVersionMigrator", func() {
 				"storedVersions must be untouched for unlabelled CRDs")
 		})
 
-		It("falls back to main-resource SSA when the storage version has no /status subresource", func() {
-			suf := uniqueSuffix()
-			spec := crdSpec{
-				Name:              "nostatus" + suf + "." + toolhiveGroup,
-				Group:             toolhiveGroup,
-				Kind:              "NoStatus" + suf,
-				ListKind:          "NoStatus" + suf + "List",
-				Plural:            "nostatus" + suf,
-				Singular:          "nostatus" + suf,
-				Labelled:          true,
-				HasStatusOnStored: false,
-				Versions: []versionSpec{
-					{Name: "v1alpha1", Served: true, Storage: false},
-					{Name: "v1beta1", Served: true, Storage: true},
-				},
-			}
-			installCRD(spec)
-			DeferCleanup(func() { deleteCRD(spec.Name) })
-
-			cr := createCRs(
-				schema.GroupVersionKind{Group: spec.Group, Version: "v1beta1", Kind: spec.Kind},
-				"obj-"+suf, 1,
-			)[0]
-			setStoredVersions(spec.Name, []string{"v1alpha1", "v1beta1"})
-
-			_, err := reconcile(newReconciler(), spec.Name)
-			Expect(err).NotTo(HaveOccurred(),
-				"reconcile must succeed via main-resource SSA when no /status subresource exists")
-			Expect(getStoredVersions(spec.Name)).To(Equal([]string{"v1beta1"}))
-
-			// CR still exists (migrator doesn't delete).
-			live := &unstructured.Unstructured{}
-			live.SetGroupVersionKind(cr.GroupVersionKind())
-			Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cr), live)).To(Succeed())
-		})
-
 		It("handles pagination across multiple list pages", func() {
 			suf := uniqueSuffix()
 			spec := crdSpec{