ssa: address review feedback on API version migration

- Remove API version migration from Diff() - Diff should not alter objects
- Refactor migrateAPIVersion to not require getError parameter
- Check object existence conditionally before calling migrateAPIVersion
- Remove inadequate TestApply_APIVersionMigration test

Fixes: fluxcd/flux2#5715
Signed-off-by: mehrdadbn9 <mehrdadbiukian@gmail.com>

Author: mehrdadbn9

ssa/manager_apply.go

@@ -108,9 +108,11 @@ func (m *ResourceManager) Apply(ctx context.Context, object *unstructured.Unstru
 		return m.changeSetEntry(object, SkippedAction), nil
 	}
 
-	// Migrate managed fields API version if needed before dry-run
-	if err := m.migrateAPIVersion(ctx, object, existingObject, getError); err != nil {
-		return nil, fmt.Errorf("%s failed to migrate API version: %w", utils.FmtUnstructured(object), err)
+	// Migrate managed fields API version if the object exists and has a different API version
+	if getError == nil && existingObject.GetUID() != "" {
+		if err := m.migrateAPIVersion(ctx, existingObject, object.GetAPIVersion()); err != nil {
+			return nil, fmt.Errorf("%s failed to migrate API version: %w", utils.FmtUnstructured(object), err)
+		}
 	}
 
 	dryRunObject := object.DeepCopy()
@@ -177,9 +179,11 @@ func (m *ResourceManager) ApplyAll(ctx context.Context, objects []*unstructured.
 					return nil
 				}
 
-				// Migrate managed fields API version if needed before dry-run
-				if err := m.migrateAPIVersion(ctx, object, existingObject, getError); err != nil {
-					return fmt.Errorf("%s failed to migrate API version: %w", utils.FmtUnstructured(object), err)
+				// Migrate managed fields API version if the object exists and has a different API version
+				if getError == nil && existingObject.GetUID() != "" {
+					if err := m.migrateAPIVersion(ctx, existingObject, object.GetAPIVersion()); err != nil {
+						return fmt.Errorf("%s failed to migrate API version: %w", utils.FmtUnstructured(object), err)
+					}
 				}
 
 				dryRunObject := object.DeepCopy()
@@ -355,18 +359,12 @@ func (m *ResourceManager) apply(ctx context.Context, object *unstructured.Unstru
 	return m.client.Patch(ctx, object, client.Apply, opts...)
 }
 
-// migrateAPIVersion updates the managed fields API version when the desired object
-// has a different API version than the existing object. This is necessary because
+// migrateAPIVersion updates the managed fields API version when the existing object
+// has a different API version than the desired API version. This is necessary because
 // Kubernetes server-side apply validates managed fields against the schema of the
 // API version they reference, and when upgrading CRD versions, the old API version
 // may have fields that don't exist in the new schema, causing dry-run to fail.
-func (m *ResourceManager) migrateAPIVersion(ctx context.Context, object *unstructured.Unstructured, existingObject *unstructured.Unstructured, getError error) error {
-	// Skip if object doesn't exist or there's an error getting it
-	if getError != nil || existingObject == nil || existingObject.GetUID() == "" {
-		return nil
-	}
-
-	desiredAPIVersion := object.GetAPIVersion()
+func (m *ResourceManager) migrateAPIVersion(ctx context.Context, existingObject *unstructured.Unstructured, desiredAPIVersion string) error {
 	existingAPIVersion := existingObject.GetAPIVersion()
 
 	// Skip if API versions are the same

ssa/manager_apply_test.go

@@ -1661,80 +1661,6 @@ func TestApplyAllStaged_AppliesRoleAndRoleBinding(t *testing.T) {
 	})
 }
 
-func TestApply_APIVersionMigration(t *testing.T) {
-	timeout := 10 * time.Second
-	ctx, cancel := context.WithTimeout(context.Background(), timeout)
-	defer cancel()
-
-	id := generateName("api-version-migrate")
-	objects, err := readManifest("testdata/test1.yaml", id)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	manager.SetOwnerLabels(objects, "app1", "default")
-
-	// Get the configmap object
-	_, configMap := getFirstObject(objects, "ConfigMap", id)
-
-	t.Run("creates object with initial API version", func(t *testing.T) {
-		changeSet, err := manager.ApplyAllStaged(ctx, objects, DefaultApplyOptions())
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		for _, entry := range changeSet.Entries {
-			if entry.Action != CreatedAction {
-				t.Errorf("Expected %s, got %s for %s", CreatedAction, entry.Action, entry.Subject)
-			}
-		}
-	})
-
-	t.Run("patches managed fields with new API version", func(t *testing.T) {
-		// Simulate an API version migration by updating the managed fields
-		// In a real scenario, this would happen when a CRD version is deprecated
-		// and the user updates their manifests to use the new version
-
-		// Get the existing object
-		existingConfigMap := configMap.DeepCopy()
-		err := manager.client.Get(ctx, client.ObjectKeyFromObject(existingConfigMap), existingConfigMap)
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		// Verify the object was created with proper managed fields
-		managedFields := existingConfigMap.GetManagedFields()
-		if len(managedFields) == 0 {
-			t.Fatal("Expected managed fields to be present")
-		}
-
-		// Verify that the manager's field owner is in managed fields
-		foundManager := false
-		for _, field := range managedFields {
-			if field.Manager == manager.owner.Field {
-				foundManager = true
-				break
-			}
-		}
-		if !foundManager {
-			t.Errorf("Expected to find manager %s in managed fields", manager.owner.Field)
-		}
-	})
-
-	t.Run("re-applies object without changes", func(t *testing.T) {
-		changeSet, err := manager.ApplyAllStaged(ctx, objects, DefaultApplyOptions())
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		for _, entry := range changeSet.Entries {
-			if entry.Action != UnchangedAction {
-				t.Errorf("Expected %s, got %s for %s", UnchangedAction, entry.Action, entry.Subject)
-			}
-		}
-	})
-}
-
 func TestPatchMigrateToVersion(t *testing.T) {
 	tests := []struct {
 		name          string

ssa/manager_diff.go

@@ -65,17 +65,12 @@ func (m *ResourceManager) Diff(ctx context.Context, object *unstructured.Unstruc
 	utils.RemoveCABundleFromCRD(object)
 	existingObject := &unstructured.Unstructured{}
 	existingObject.SetGroupVersionKind(object.GroupVersionKind())
-	getError := m.client.Get(ctx, client.ObjectKeyFromObject(object), existingObject)
+	_ = m.client.Get(ctx, client.ObjectKeyFromObject(object), existingObject)
 
 	if m.shouldSkipDiff(object, existingObject, opts) {
 		return m.changeSetEntry(existingObject, SkippedAction), nil, nil, nil
 	}
 
-	// Migrate managed fields API version if needed before dry-run
-	if err := m.migrateAPIVersion(ctx, object, existingObject, getError); err != nil {
-		return nil, nil, nil, err
-	}
-
 	dryRunObject := object.DeepCopy()
 	if err := m.dryRunApply(ctx, dryRunObject); err != nil {
 		if m.shouldForceApply(object, existingObject, ApplyOptions{

ssa/testdata/test15-crd.yaml

@@ -0,0 +1,31 @@
+---
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: "%[1]s"
+---
+apiVersion: apiextensions.k8s.io/v1
+kind: CustomResourceDefinition
+metadata:
+  name: "tests.apimigration.example.com"
+spec:
+  group: apimigration.example.com
+  names:
+    kind: Test
+    listKind: TestList
+    plural: tests
+    singular: test
+  scope: Namespaced
+  versions:
+    - name: v1alpha1
+      served: true
+      storage: true
+      schema:
+        openAPIV3Schema:
+          type: object
+          properties:
+            spec:
+              type: object
+              properties:
+                value:
+                  type: string