From f48460ad8a901686ad6715ac54915c6c9ba61bad Mon Sep 17 00:00:00 2001 From: mehrdadbn9 Date: Tue, 24 Feb 2026 01:02:04 +0330 Subject: [PATCH 1/2] ssa: fix dry-run failure when upgrading API version When upgrading a custom resource to a new API version (e.g., v1beta1 to v1beta3), the server-side apply dry-run fails with an error like: 'dry-run failed: .spec.accessPolicy: field not declared in schema' This happens because the managed fields in the existing object still reference the old API version, and Kubernetes validates those fields against the new schema during dry-run. Fields that have default values in the new version but didn't exist in the old version cause schema validation errors. The fix introduces a new function that: 1. Detects when the desired object has a different API version than existing 2. Uses the existing function to update managed fields 3. Applies the patch before the dry-run to ensure proper schema validation This enables seamless API version upgrades without manual intervention. Fixes: fluxcd/flux2#5715 Signed-off-by: mehrdadbn9 --- ssa/manager_apply.go | 53 +++++++++++ ssa/manager_apply_test.go | 195 ++++++++++++++++++++++++++++++++++++++ ssa/manager_diff.go | 7 +- 3 files changed, 254 insertions(+), 1 deletion(-) diff --git a/ssa/manager_apply.go b/ssa/manager_apply.go index 76e5a5fb..05c68080 100644 --- a/ssa/manager_apply.go +++ b/ssa/manager_apply.go @@ -108,6 +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) + } + dryRunObject := object.DeepCopy() if err := m.dryRunApply(ctx, dryRunObject); err != nil { if !errors.IsNotFound(getError) && m.shouldForceApply(object, existingObject, opts, err) { @@ -172,6 +177,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) + } + dryRunObject := object.DeepCopy() if err := m.dryRunApply(ctx, dryRunObject); err != nil { // We cannot have an immutable error (and therefore shouldn't force-apply) if the resource doesn't @@ -345,6 +355,49 @@ 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 +// 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() + existingAPIVersion := existingObject.GetAPIVersion() + + // Skip if API versions are the same + if desiredAPIVersion == existingAPIVersion { + return nil + } + + // Check if managed fields need migration + patches, err := PatchMigrateToVersion(existingObject, desiredAPIVersion) + if err != nil { + return fmt.Errorf("failed to create managed fields migration patch: %w", err) + } + + if len(patches) == 0 { + return nil + } + + // Apply the migration patch to update managed fields API version + rawPatch, err := json.Marshal(patches) + if err != nil { + return fmt.Errorf("failed to marshal managed fields migration patch: %w", err) + } + patch := client.RawPatch(types.JSONPatchType, rawPatch) + + if err := m.client.Patch(ctx, existingObject, patch, client.FieldOwner(m.owner.Field)); err != nil { + return fmt.Errorf("failed to migrate managed fields API version from %s to %s: %w", existingAPIVersion, desiredAPIVersion, err) + } + + return nil +} + // cleanupMetadata performs an HTTP PATCH request to remove entries from metadata annotations, labels and managedFields. func (m *ResourceManager) cleanupMetadata(ctx context.Context, desiredObject *unstructured.Unstructured, diff --git a/ssa/manager_apply_test.go b/ssa/manager_apply_test.go index 16021f70..4a3549d2 100644 --- a/ssa/manager_apply_test.go +++ b/ssa/manager_apply_test.go @@ -1660,3 +1660,198 @@ 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 + object *unstructured.Unstructured + targetVersion string + expectPatches bool + expectedLen int + }{ + { + name: "no managed fields", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "test", + "namespace": "default", + }, + }, + }, + targetVersion: "v1", + expectPatches: false, + }, + { + name: "same API version", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "test", + "namespace": "default", + "managedFields": []interface{}{ + map[string]interface{}{ + "apiVersion": "v1", + "manager": "test-manager", + "operation": "Apply", + }, + }, + }, + }, + }, + targetVersion: "v1", + expectPatches: false, + }, + { + name: "different API version - single managed field", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "policy.linkerd.io/v1beta1", + "kind": "Server", + "metadata": map[string]interface{}{ + "name": "test", + "namespace": "default", + "managedFields": []interface{}{ + map[string]interface{}{ + "apiVersion": "policy.linkerd.io/v1beta1", + "manager": "kustomize-controller", + "operation": "Apply", + }, + }, + }, + }, + }, + targetVersion: "policy.linkerd.io/v1beta3", + expectPatches: true, + expectedLen: 1, + }, + { + name: "different API version - multiple managed fields", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "policy.linkerd.io/v1beta3", + "kind": "Server", + "metadata": map[string]interface{}{ + "name": "test", + "namespace": "default", + "managedFields": []interface{}{ + map[string]interface{}{ + "apiVersion": "policy.linkerd.io/v1beta1", + "manager": "kubectl", + "operation": "Update", + }, + map[string]interface{}{ + "apiVersion": "policy.linkerd.io/v1beta1", + "manager": "kustomize-controller", + "operation": "Apply", + }, + }, + }, + }, + }, + targetVersion: "policy.linkerd.io/v1beta3", + expectPatches: true, + expectedLen: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + patches, err := PatchMigrateToVersion(tt.object, tt.targetVersion) + if err != nil { + t.Fatalf("PatchMigrateToVersion returned error: %v", err) + } + + if tt.expectPatches { + if len(patches) == 0 { + t.Errorf("Expected patches to be returned, got none") + } + if tt.expectedLen > 0 && len(patches) != tt.expectedLen { + t.Errorf("Expected %d patches, got %d", tt.expectedLen, len(patches)) + } + } else { + if len(patches) > 0 { + t.Errorf("Expected no patches, got %d", len(patches)) + } + } + }) + } +} diff --git a/ssa/manager_diff.go b/ssa/manager_diff.go index cf24bd8c..4738a966 100644 --- a/ssa/manager_diff.go +++ b/ssa/manager_diff.go @@ -65,12 +65,17 @@ func (m *ResourceManager) Diff(ctx context.Context, object *unstructured.Unstruc utils.RemoveCABundleFromCRD(object) existingObject := &unstructured.Unstructured{} existingObject.SetGroupVersionKind(object.GroupVersionKind()) - _ = m.client.Get(ctx, client.ObjectKeyFromObject(object), existingObject) + getError := 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{ From a03da8d5db79ded7f1dc46be9a3c66ad099154ca Mon Sep 17 00:00:00 2001 From: mehrdadbn9 Date: Wed, 25 Feb 2026 20:08:01 +0330 Subject: [PATCH 2/2] 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 --- ssa/manager_apply.go | 28 +++++++------- ssa/manager_apply_test.go | 74 ------------------------------------ ssa/manager_diff.go | 7 +--- ssa/testdata/test15-crd.yaml | 31 +++++++++++++++ 4 files changed, 45 insertions(+), 95 deletions(-) create mode 100644 ssa/testdata/test15-crd.yaml diff --git a/ssa/manager_apply.go b/ssa/manager_apply.go index 05c68080..b2781e1a 100644 --- a/ssa/manager_apply.go +++ b/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 diff --git a/ssa/manager_apply_test.go b/ssa/manager_apply_test.go index 4a3549d2..85d07538 100644 --- a/ssa/manager_apply_test.go +++ b/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 diff --git a/ssa/manager_diff.go b/ssa/manager_diff.go index 4738a966..cf24bd8c 100644 --- a/ssa/manager_diff.go +++ b/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{ diff --git a/ssa/testdata/test15-crd.yaml b/ssa/testdata/test15-crd.yaml new file mode 100644 index 00000000..4602fbe3 --- /dev/null +++ b/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