diff --git a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go index 341a092f86d..920c113bbd7 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go @@ -1182,7 +1182,13 @@ func mergePatchIntoOriginal(original, patch map[string]interface{}, schema Looku merged = originalFieldValue case !foundOriginal && foundPatch: // list was added - merged = patchFieldValue + v, keep := removeDirectives(patchFieldValue) + if !keep { + // Shouldn't be possible since patchFieldValue is a slice + continue + } + + merged = v.([]interface{}) case foundOriginal && foundPatch: merged, err = mergeSliceHandler(originalList, patchList, subschema, patchStrategy, patchMeta.GetPatchMergeKey(), false, mergeOptions) @@ -1270,6 +1276,42 @@ func partitionMapsByPresentInList(original, partitionBy []interface{}, mergeKey return patch, serverOnly, nil } +// Removes directives from an object and returns value to use instead and whether +// or not the field/index should even be kept +// May modify input +func removeDirectives(obj interface{}) (interface{}, bool) { + if obj == nil { + return obj, true + } else if typedV, ok := obj.(map[string]interface{}); ok { + if _, hasDirective := typedV[directiveMarker]; hasDirective { + return nil, false + } + + for k, v := range typedV { + var keep bool + typedV[k], keep = removeDirectives(v) + if !keep { + delete(typedV, k) + } + } + return typedV, true + } else if typedV, ok := obj.([]interface{}); ok { + var res []interface{} + if typedV != nil { + // Make sure res is non-nil if patch is non-nil + res = []interface{}{} + } + for _, v := range typedV { + if newV, keep := removeDirectives(v); keep { + res = append(res, newV) + } + } + return res, true + } else { + return obj, true + } +} + // Merge fields from a patch map into the original map. Note: This may modify // both the original map and the patch because getting a deep copy of a map in // golang is highly non-trivial. @@ -1333,7 +1375,10 @@ func mergeMap(original, patch map[string]interface{}, schema LookupPatchMeta, me if mergeOptions.IgnoreUnmatchedNulls { discardNullValuesFromPatch(patchV) } - original[k] = patchV + original[k], ok = removeDirectives(patchV) + if !ok { + delete(original, k) + } } continue } @@ -1345,7 +1390,10 @@ func mergeMap(original, patch map[string]interface{}, schema LookupPatchMeta, me if mergeOptions.IgnoreUnmatchedNulls { discardNullValuesFromPatch(patchV) } - original[k] = patchV + original[k], ok = removeDirectives(patchV) + if !ok { + delete(original, k) + } } continue } @@ -1372,7 +1420,11 @@ func mergeMap(original, patch map[string]interface{}, schema LookupPatchMeta, me } original[k], err = mergeSliceHandler(original[k], patchV, subschema, patchStrategy, patchMeta.GetPatchMergeKey(), isDeleteList, mergeOptions) default: - original[k] = patchV + original[k], ok = removeDirectives(patchV) + if !ok { + // if patchV itself is a directive, then don't keep it + delete(original, k) + } } if err != nil { return nil, err diff --git a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go index 864cbdf5edf..80b8c30ab86 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go @@ -778,16 +778,20 @@ func TestCustomStrategicMergePatch(t *testing.T) { return } - for _, schema := range schemas { - for _, c := range tc.TestCases { - original, expectedTwoWayPatch, _, expectedResult := twoWayTestCaseToJSONOrFail(t, c, schema) - testPatchApplication(t, original, expectedTwoWayPatch, expectedResult, c.Description, "", schema) - } + for _, c := range tc.TestCases { + t.Run(c.Description, func(t *testing.T) { + for _, schema := range schemas { + t.Run(schema.Name(), func(t *testing.T) { + original, expectedTwoWayPatch, _, expectedResult := twoWayTestCaseToJSONOrFail(t, c, schema) + testPatchApplication(t, original, expectedTwoWayPatch, expectedResult, c.Description, "", schema) + }) - for _, c := range customStrategicMergePatchRawTestCases { - original, expectedTwoWayPatch, _, expectedResult := twoWayRawTestCaseToJSONOrFail(t, c) - testPatchApplication(t, original, expectedTwoWayPatch, expectedResult, c.Description, c.ExpectedError, schema) - } + for _, c := range customStrategicMergePatchRawTestCases { + original, expectedTwoWayPatch, _, expectedResult := twoWayRawTestCaseToJSONOrFail(t, c) + testPatchApplication(t, original, expectedTwoWayPatch, expectedResult, c.Description, c.ExpectedError, schema) + } + } + }) } } @@ -1248,6 +1252,50 @@ testCases: `) var strategicMergePatchRawTestCases = []StrategicMergePatchRawTestCase{ + { + Description: "nested patch merge with empty list", + StrategicMergePatchRawTestCaseData: StrategicMergePatchRawTestCaseData{ + Original: []byte(` +name: hi +`), + Current: []byte(` +name: hi +mergingList: +- name: hello2 +`), + Modified: []byte(` +name: hi +mergingList: +- name: hello +- $patch: delete + name: doesntexist +`), + TwoWay: []byte(` +mergingList: +- name: hello +- $patch: delete + name: doesntexist +`), + ThreeWay: []byte(` +$setElementOrder/mergingList: +- name: hello +- name: doesntexist +mergingList: +- name: hello +`), + TwoWayResult: []byte(` +name: hi +mergingList: +- name: hello +`), + Result: []byte(` +name: hi +mergingList: +- name: hello +- name: hello2 +`), + }, + }, { Description: "delete items in lists of scalars", StrategicMergePatchRawTestCaseData: StrategicMergePatchRawTestCaseData{ @@ -6134,23 +6182,34 @@ func TestStrategicMergePatch(t *testing.T) { } for _, schema := range schemas { - testStrategicMergePatchWithCustomArguments(t, "bad original", - "", "{}", schema, mergepatch.ErrBadJSONDoc) - testStrategicMergePatchWithCustomArguments(t, "bad patch", - "{}", "", schema, mergepatch.ErrBadJSONDoc) - testStrategicMergePatchWithCustomArguments(t, "nil struct", - "{}", "{}", nil, mergepatch.ErrBadArgKind(struct{}{}, nil)) + t.Run(schema.Name(), func(t *testing.T) { - for _, c := range tc.TestCases { - testTwoWayPatch(t, c, schema) - testThreeWayPatch(t, c, schema) - } + testStrategicMergePatchWithCustomArguments(t, "bad original", + "", "{}", schema, mergepatch.ErrBadJSONDoc) + testStrategicMergePatchWithCustomArguments(t, "bad patch", + "{}", "", schema, mergepatch.ErrBadJSONDoc) + testStrategicMergePatchWithCustomArguments(t, "nil struct", + "{}", "{}", nil, mergepatch.ErrBadArgKind(struct{}{}, nil)) + + for _, c := range tc.TestCases { + t.Run(c.Description+"/TwoWay", func(t *testing.T) { + testTwoWayPatch(t, c, schema) + }) + t.Run(c.Description+"/ThreeWay", func(t *testing.T) { + testThreeWayPatch(t, c, schema) + }) + } + }) // run multiple times to exercise different map traversal orders for i := 0; i < 10; i++ { for _, c := range strategicMergePatchRawTestCases { - testTwoWayPatchForRawTestCase(t, c, schema) - testThreeWayPatchForRawTestCase(t, c, schema) + t.Run(c.Description+"/TwoWay", func(t *testing.T) { + testTwoWayPatchForRawTestCase(t, c, schema) + }) + t.Run(c.Description+"/ThreeWay", func(t *testing.T) { + testThreeWayPatchForRawTestCase(t, c, schema) + }) } } } @@ -6893,61 +6952,63 @@ func TestUnknownField(t *testing.T) { } for _, k := range sets.StringKeySet(testcases).List() { - tc := testcases[k] - for _, schema := range schemas { - func() { - twoWay, err := CreateTwoWayMergePatchUsingLookupPatchMeta([]byte(tc.Original), []byte(tc.Modified), schema) - if err != nil { - if len(tc.ExpectedTwoWayErr) == 0 { - t.Errorf("using %s in testcase %s: error making two-way patch: %v", getSchemaType(schema), k, err) + t.Run(k, func(t *testing.T) { + tc := testcases[k] + for _, schema := range schemas { + t.Run(schema.Name()+"/TwoWay", func(t *testing.T) { + twoWay, err := CreateTwoWayMergePatchUsingLookupPatchMeta([]byte(tc.Original), []byte(tc.Modified), schema) + if err != nil { + if len(tc.ExpectedTwoWayErr) == 0 { + t.Errorf("using %s in testcase %s: error making two-way patch: %v", getSchemaType(schema), k, err) + } + if !strings.Contains(err.Error(), tc.ExpectedTwoWayErr) { + t.Errorf("using %s in testcase %s: expected error making two-way patch to contain '%s', got %s", getSchemaType(schema), k, tc.ExpectedTwoWayErr, err) + } + return } - if !strings.Contains(err.Error(), tc.ExpectedTwoWayErr) { - t.Errorf("using %s in testcase %s: expected error making two-way patch to contain '%s', got %s", getSchemaType(schema), k, tc.ExpectedTwoWayErr, err) + + if string(twoWay) != tc.ExpectedTwoWay { + t.Errorf("using %s in testcase %s: expected two-way patch:\n\t%s\ngot\n\t%s", getSchemaType(schema), k, string(tc.ExpectedTwoWay), string(twoWay)) + return } - return - } - if string(twoWay) != tc.ExpectedTwoWay { - t.Errorf("using %s in testcase %s: expected two-way patch:\n\t%s\ngot\n\t%s", getSchemaType(schema), k, string(tc.ExpectedTwoWay), string(twoWay)) - return - } - - twoWayResult, err := StrategicMergePatchUsingLookupPatchMeta([]byte(tc.Original), twoWay, schema) - if err != nil { - t.Errorf("using %s in testcase %s: error applying two-way patch: %v", getSchemaType(schema), k, err) - return - } - if string(twoWayResult) != tc.ExpectedTwoWayResult { - t.Errorf("using %s in testcase %s: expected two-way result:\n\t%s\ngot\n\t%s", getSchemaType(schema), k, string(tc.ExpectedTwoWayResult), string(twoWayResult)) - return - } - }() - - func() { - threeWay, err := CreateThreeWayMergePatch([]byte(tc.Original), []byte(tc.Modified), []byte(tc.Current), schema, false) - if err != nil { - if len(tc.ExpectedThreeWayErr) == 0 { - t.Errorf("using %s in testcase %s: error making three-way patch: %v", getSchemaType(schema), k, err) - } else if !strings.Contains(err.Error(), tc.ExpectedThreeWayErr) { - t.Errorf("using %s in testcase %s: expected error making three-way patch to contain '%s', got %s", getSchemaType(schema), k, tc.ExpectedThreeWayErr, err) + twoWayResult, err := StrategicMergePatchUsingLookupPatchMeta([]byte(tc.Original), twoWay, schema) + if err != nil { + t.Errorf("using %s in testcase %s: error applying two-way patch: %v", getSchemaType(schema), k, err) + return } - return - } + if string(twoWayResult) != tc.ExpectedTwoWayResult { + t.Errorf("using %s in testcase %s: expected two-way result:\n\t%s\ngot\n\t%s", getSchemaType(schema), k, string(tc.ExpectedTwoWayResult), string(twoWayResult)) + return + } + }) - if string(threeWay) != tc.ExpectedThreeWay { - t.Errorf("using %s in testcase %s: expected three-way patch:\n\t%s\ngot\n\t%s", getSchemaType(schema), k, string(tc.ExpectedThreeWay), string(threeWay)) - return - } + t.Run(schema.Name()+"/ThreeWay", func(t *testing.T) { + threeWay, err := CreateThreeWayMergePatch([]byte(tc.Original), []byte(tc.Modified), []byte(tc.Current), schema, false) + if err != nil { + if len(tc.ExpectedThreeWayErr) == 0 { + t.Errorf("using %s in testcase %s: error making three-way patch: %v", getSchemaType(schema), k, err) + } else if !strings.Contains(err.Error(), tc.ExpectedThreeWayErr) { + t.Errorf("using %s in testcase %s: expected error making three-way patch to contain '%s', got %s", getSchemaType(schema), k, tc.ExpectedThreeWayErr, err) + } + return + } - threeWayResult, err := StrategicMergePatch([]byte(tc.Current), threeWay, schema) - if err != nil { - t.Errorf("using %s in testcase %s: error applying three-way patch: %v", getSchemaType(schema), k, err) - return - } else if string(threeWayResult) != tc.ExpectedThreeWayResult { - t.Errorf("using %s in testcase %s: expected three-way result:\n\t%s\ngot\n\t%s", getSchemaType(schema), k, string(tc.ExpectedThreeWayResult), string(threeWayResult)) - return - } - }() - } + if string(threeWay) != tc.ExpectedThreeWay { + t.Errorf("using %s in testcase %s: expected three-way patch:\n\t%s\ngot\n\t%s", getSchemaType(schema), k, string(tc.ExpectedThreeWay), string(threeWay)) + return + } + + threeWayResult, err := StrategicMergePatch([]byte(tc.Current), threeWay, schema) + if err != nil { + t.Errorf("using %s in testcase %s: error applying three-way patch: %v", getSchemaType(schema), k, err) + return + } else if string(threeWayResult) != tc.ExpectedThreeWayResult { + t.Errorf("using %s in testcase %s: expected three-way result:\n\t%s\ngot\n\t%s", getSchemaType(schema), k, string(tc.ExpectedThreeWayResult), string(threeWayResult)) + return + } + }) + } + }) } } diff --git a/test/integration/apiserver/patch_test.go b/test/integration/apiserver/patch_test.go index 7a1c09d8285..db4077e66aa 100644 --- a/test/integration/apiserver/patch_test.go +++ b/test/integration/apiserver/patch_test.go @@ -24,8 +24,10 @@ import ( "testing" "github.com/google/uuid" + "github.com/stretchr/testify/require" - v1 "k8s.io/api/core/v1" + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -56,7 +58,7 @@ func TestPatchConflicts(t *testing.T) { UID: uid, }) } - secret := &v1.Secret{ + secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "test", OwnerReferences: ownerRefs, @@ -136,3 +138,74 @@ func findOwnerRefByUID(ownerRefs []metav1.OwnerReference, uid types.UID) bool { } return false } + +// Shows that a strategic merge patch with a nested patch which is merged +// with an empty slice is handled property +// https://github.com/kubernetes/kubernetes/issues/117470 +func TestNestedStrategicMergePatchWithEmpty(t *testing.T) { + clientSet, _, tearDownFn := setup(t) + defer tearDownFn() + + url := "https://foo.com" + se := admissionregistrationv1.SideEffectClassNone + + _, err := clientSet. + AdmissionregistrationV1(). + ValidatingWebhookConfigurations(). + Create( + context.TODO(), + &admissionregistrationv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "base-validation", + }, + Webhooks: []admissionregistrationv1.ValidatingWebhook{ + { + AdmissionReviewVersions: []string{"v1"}, + ClientConfig: admissionregistrationv1.WebhookClientConfig{URL: &url}, + Name: "foo.bar.com", + SideEffects: &se, + }, + }, + }, + metav1.CreateOptions{ + FieldManager: "kubectl-client-side-apply", + FieldValidation: metav1.FieldValidationStrict, + }, + ) + require.NoError(t, err) + + _, err = clientSet. + AdmissionregistrationV1(). + ValidatingWebhookConfigurations(). + Patch( + context.TODO(), + "base-validation", + types.StrategicMergePatchType, + []byte(` + { + "webhooks": null + } +`), + metav1.PatchOptions{ + FieldManager: "kubectl-edit", + FieldValidation: metav1.FieldValidationStrict, + }, + ) + require.NoError(t, err) + + // Try to apply a patch to the webhook + _, err = clientSet. + AdmissionregistrationV1(). + ValidatingWebhookConfigurations(). + Patch( + context.TODO(), + "base-validation", + types.StrategicMergePatchType, + []byte(`{"$setElementOrder/webhooks":[{"name":"new.foo.com"}],"metadata":{"annotations":{"kubectl.kubernetes.io/last-applied-configuration":"{\"apiVersion\":\"admissionregistration.k8s.io/v1\",\"kind\":\"ValidatingWebhookConfiguration\",\"metadata\":{\"annotations\":{},\"name\":\"base-validation\"},\"webhooks\":[{\"admissionReviewVersions\":[\"v1\"],\"clientConfig\":{\"url\":\"https://foo.com\"},\"name\":\"new.foo.com\",\"sideEffects\":\"None\"}]}\n"}},"webhooks":[{"admissionReviewVersions":["v1"],"clientConfig":{"url":"https://foo.com"},"name":"new.foo.com","sideEffects":"None"},{"$patch":"delete","name":"foo.bar.com"}]}`), + metav1.PatchOptions{ + FieldManager: "kubectl-client-side-apply", + FieldValidation: metav1.FieldValidationStrict, + }, + ) + require.NoError(t, err) +}