From f2f92cba4c090f02756cb67248491542ac868a58 Mon Sep 17 00:00:00 2001 From: TehilaTheStudent Date: Sun, 15 Feb 2026 22:06:18 +0200 Subject: [PATCH] fix ssa patch by applying all managed fields at once Signed-off-by: TehilaTheStudent --- pkg/reconciler/managed/api.go | 46 ++++++- pkg/reconciler/managed/api_test.go | 192 +++++++++++++++++++++++++++++ 2 files changed, 237 insertions(+), 1 deletion(-) diff --git a/pkg/reconciler/managed/api.go b/pkg/reconciler/managed/api.go index 7bf6a1514..b2f3c577e 100644 --- a/pkg/reconciler/managed/api.go +++ b/pkg/reconciler/managed/api.go @@ -25,11 +25,14 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" corev1 "k8s.io/api/core/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/managedfields" "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/structured-merge-diff/v6/typed" "github.com/crossplane/crossplane-runtime/v2/pkg/errors" "github.com/crossplane/crossplane-runtime/v2/pkg/meta" @@ -53,6 +56,8 @@ const ( errUpdateManagedStatus = "cannot update managed resource status" errResolveReferences = "cannot resolve references" errUpdateCriticalAnnotations = "cannot update critical annotations" + errMarshalManagedFields = "cannot marshal previously managed fields" + errMergePatches = "cannot merge patches" ) // NameAsExternalName writes the name of the managed resource to @@ -228,8 +233,47 @@ func prepareJSONMerge(existing, resolved runtime.Object) ([]byte, error) { } patch, err := jsonpatch.CreateMergePatch(eBuff, rBuff) + if err != nil { + return nil, errors.Wrap(err, errPreparePatch) + } + + // Merge with previously managed fields to maintain ownership. + return mergeWithManagedFields(patch, resolved, fieldOwnerAPISimpleRefResolver) +} + +// mergeWithManagedFields merges the patch with fields previously managed by the +// specified field manager. This ensures that SSA patches include all fields the +// manager owns, preventing the API server from interpreting missing fields as +// intentional deletions which would cause an infinite reconciliation loop. +func mergeWithManagedFields(patch []byte, obj runtime.Object, fieldManager string) ([]byte, error) { + // ExtractInto requires an unstructured representation of the object. + u, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj) + if err != nil { + return patch, nil //nolint:nilerr // Conversion errors are non-fatal; return the original patch. + } + + managed := &unstructured.Unstructured{} + if err := managedfields.ExtractInto(&unstructured.Unstructured{Object: u}, typed.DeducedParseableType, fieldManager, managed, ""); err != nil { + // ExtractInto can fail for edge cases like managedFields references fields that don't exist on the observed object. + // See https://github.com/crossplane-contrib/provider-kubernetes/issues/420 + return patch, nil //nolint:nilerr // Extraction errors are non-fatal; return the original patch. + } + + if len(managed.Object) == 0 { + return patch, nil + } + + b, err := json.Marshal(managed.Object) + if err != nil { + return nil, errors.Wrap(err, errMarshalManagedFields) + } + + merged, err := jsonpatch.MergePatch(b, patch) + if err != nil { + return nil, errors.Wrap(err, errMergePatches) + } - return patch, errors.Wrap(err, errPreparePatch) + return merged, nil } // ResolveReferences of the supplied managed resource by calling its diff --git a/pkg/reconciler/managed/api_test.go b/pkg/reconciler/managed/api_test.go index 16467f446..2a57aed92 100644 --- a/pkg/reconciler/managed/api_test.go +++ b/pkg/reconciler/managed/api_test.go @@ -23,6 +23,7 @@ import ( "github.com/google/go-cmp/cmp" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" @@ -503,6 +504,39 @@ func TestPrepareJSONMerge(t *testing.T) { err error } + newManagedFieldsObj := func() *unstructured.Unstructured { + u := &unstructured.Unstructured{} + u.SetGroupVersionKind(schema.GroupVersionKind{ + Group: "example.com", + Version: "v1", + Kind: "TestResource", + }) + u.SetName("test-resource") + _ = unstructured.SetNestedField(u.Object, "new-ref-id", "spec", "forProvider", "someRefId") + u.SetManagedFields([]metav1.ManagedFieldsEntry{ + { + Manager: fieldOwnerAPISimpleRefResolver, + Operation: metav1.ManagedFieldsOperationApply, + FieldsType: "FieldsV1", + FieldsV1: &metav1.FieldsV1{ + Raw: []byte(`{"f:spec":{"f:forProvider":{"f:otherRefId":{}}}}`), + }, + }, + }) + return u + } + + withoutManagedFields := func() *unstructured.Unstructured { + u := &unstructured.Unstructured{} + u.SetGroupVersionKind(schema.GroupVersionKind{ + Group: "example.com", + Version: "v1", + Kind: "TestResource", + }) + u.SetName("test-resource") + return u + } + cases := map[string]struct { reason string args args @@ -522,6 +556,43 @@ func TestPrepareJSONMerge(t *testing.T) { patch: `{"name":"resolved"}`, }, }, + "PatchWithNoChanges": { + reason: "Should return empty patch when existing and resolved are the same.", + args: args{ + existing: &fake.LegacyManaged{ + ObjectMeta: metav1.ObjectMeta{ + Name: "same", + }, + }, + resolved: &fake.LegacyManaged{ + ObjectMeta: metav1.ObjectMeta{ + Name: "same", + }, + }, + }, + want: want{ + patch: `{}`, + }, + }, + "PatchWithManagedFieldsMerged": { + // Verifies that managed fields referencing fields not in the current patch + // are still included in the final patch to maintain SSA field ownership. + // Note: The expected patch includes managedFields metadata because existing + // has none while resolved does - this is a test artifact. In production, + // both objects would have identical managedFields so the diff wouldn't include them. + reason: "Should merge previously managed fields into the patch to maintain SSA field ownership.", + args: args{ + existing: withoutManagedFields(), + resolved: newManagedFieldsObj(), + }, + want: want{ + patch: `{"apiVersion":"example.com/v1","kind":"TestResource",` + + `"metadata":{"managedFields":[{"fieldsType":"FieldsV1",` + + `"fieldsV1":{"f:spec":{"f:forProvider":{"f:otherRefId":{}}}},` + + `"manager":"managed.crossplane.io/api-simple-reference-resolver",` + + `"operation":"Apply"}]},"spec":{"forProvider":{"someRefId":"new-ref-id"}}}`, + }, + }, } for name, tc := range cases { @@ -538,6 +609,127 @@ func TestPrepareJSONMerge(t *testing.T) { } } +func TestMergeWithManagedFields(t *testing.T) { + type args struct { + patch []byte + obj runtime.Object + fieldManager string + } + + type want struct { + patch string + err error + } + + withManagedFields := func() *unstructured.Unstructured { + u := &unstructured.Unstructured{} + u.SetGroupVersionKind(schema.GroupVersionKind{ + Group: "example.com", + Version: "v1", + Kind: "TestResource", + }) + u.SetName("test-resource") + _ = unstructured.SetNestedField(u.Object, "old-ref-value", "spec", "forProvider", "someRefId") + u.SetManagedFields([]metav1.ManagedFieldsEntry{ + { + Manager: fieldOwnerAPISimpleRefResolver, + Operation: metav1.ManagedFieldsOperationApply, + FieldsType: "FieldsV1", + FieldsV1: &metav1.FieldsV1{ + Raw: []byte(`{"f:spec":{"f:forProvider":{"f:someRefId":{}}}}`), + }, + }, + }) + return u + } + + withDifferentManager := func() *unstructured.Unstructured { + u := &unstructured.Unstructured{} + u.SetGroupVersionKind(schema.GroupVersionKind{ + Group: "example.com", + Version: "v1", + Kind: "TestResource", + }) + u.SetName("test-resource") + _ = unstructured.SetNestedField(u.Object, "other-ref-value", "spec", "forProvider", "otherRefId") + u.SetManagedFields([]metav1.ManagedFieldsEntry{ + { + Manager: "different-manager", + Operation: metav1.ManagedFieldsOperationApply, + FieldsType: "FieldsV1", + FieldsV1: &metav1.FieldsV1{ + Raw: []byte(`{"f:spec":{"f:forProvider":{"f:otherRefId":{}}}}`), + }, + }, + }) + return u + } + + cases := map[string]struct { + reason string + args args + want want + }{ + "NoManagedFields": { + reason: "Should return the original patch when there are no managed fields.", + args: args{ + patch: []byte(`{"metadata":{"name":"resolved"}}`), + obj: &fake.LegacyManaged{}, + fieldManager: fieldOwnerAPISimpleRefResolver, + }, + want: want{ + patch: `{"metadata":{"name":"resolved"}}`, + }, + }, + "EmptyPatch": { + reason: "Should return empty patch when patch is empty and no managed fields.", + args: args{ + patch: []byte(`{}`), + obj: &fake.LegacyManaged{}, + fieldManager: fieldOwnerAPISimpleRefResolver, + }, + want: want{ + patch: `{}`, + }, + }, + "WithManagedFields": { + reason: "Should merge previously managed fields into the patch to maintain field ownership.", + args: args{ + patch: []byte(`{"spec":{"forProvider":{"newRefId":"new-value"}}}`), + obj: withManagedFields(), + fieldManager: fieldOwnerAPISimpleRefResolver, + }, + want: want{ + patch: `{"apiVersion":"example.com/v1","kind":"TestResource","spec":{"forProvider":{"newRefId":"new-value","someRefId":"old-ref-value"}}}`, + }, + }, + "DifferentFieldManager": { + reason: "Should not include managed fields from a different field manager.", + args: args{ + patch: []byte(`{"metadata":{"name":"resolved"}}`), + obj: withDifferentManager(), + fieldManager: fieldOwnerAPISimpleRefResolver, + }, + want: want{ + patch: `{"metadata":{"name":"resolved"}}`, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + patch, err := mergeWithManagedFields(tc.args.patch, tc.args.obj, tc.args.fieldManager) + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Errorf("\n%s\nmergeWithManagedFields(...): -wantErr, +gotErr:\n%s", tc.reason, diff) + } + + if diff := cmp.Diff(tc.want.patch, string(patch)); diff != "" { + t.Errorf("\n%s\nmergeWithManagedFields(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +} + func TestRetryingCriticalAnnotationUpdater(t *testing.T) { errBoom := errors.New("boom")