fix ssa patch by applying all managed fields at once #928
+237
−1
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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"}}}`, | ||
| }, | ||
| }, | ||
|
Comment on lines
+577
to
+595
Contributor
There was a problem hiding this comment.
The The test is therefore exercising the fallback path, not "previously managed fields merged into the patch." Could we rename it (e.g., ✏️ Suggested rename- "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.",
+ "PatchFallsBackWhenExtractIntoFails": {
+ // Verifies that when ExtractInto fails because a managed-fields entry references
+ // a spec field that is absent from the object (the edge case in provider-kubernetes#420),
+ // prepareJSONMerge falls back to returning the raw diff patch unchanged.
+ // The managedFields block in the expected output is a diff artifact (existing has none,
+ // resolved does) and is not produced by the merge logic itself.
+ reason: "Should fall back to the raw diff patch when ExtractInto fails due to a managed field referencing an absent spec field.",🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| 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") | ||
|
|
||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silently swallowing the conversion error re-introduces the original bug.
Thanks for the clear
nolintannotation! I have a question about the intent here though: ifToUnstructuredfails, the function returns the unmerged patch — which is exactly the broken behavior this PR is fixing (omitted fields → API server deletes them → infinite loop). While a failure here seems unlikely (sincejson.Marshal(resolved)already succeeded a few lines earlier), silently falling back to the buggy path feels risky.Could we propagate the error instead, so that a reconciliation retry can surface what went wrong? That way, if this ever does fail, operators get a clear signal rather than a mysterious infinite loop.
Suggested fix
🤖 Prompt for AI Agents