Merge pull request #117568 from alexzielenski/apiserver/smp/merge-with-empty

Fix Strategic Merge merging leaving patch directives in objects when field doesn't exist

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

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go

@@ -778,17 +778,21 @@ func TestCustomStrategicMergePatch(t *testing.T) {
 		return
 	}
 
-	for _, schema := range schemas {
 	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)
 				}
 			}
+		})
+	}
 }
 
 // These are test cases for StrategicMergePatch, to assert that applying  a patch
@@ -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,6 +6182,8 @@ func TestStrategicMergePatch(t *testing.T) {
 	}
 
 	for _, schema := range schemas {
+		t.Run(schema.Name(), func(t *testing.T) {
+
 			testStrategicMergePatchWithCustomArguments(t, "bad original",
 				"<THIS IS NOT JSON>", "{}", schema, mergepatch.ErrBadJSONDoc)
 			testStrategicMergePatchWithCustomArguments(t, "bad patch",
@@ -6142,15 +6192,24 @@ func TestStrategicMergePatch(t *testing.T) {
 				"{}", "{}", 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 {
+				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,9 +6952,10 @@ func TestUnknownField(t *testing.T) {
 	}
 
 	for _, k := range sets.StringKeySet(testcases).List() {
+		t.Run(k, func(t *testing.T) {
 			tc := testcases[k]
 			for _, schema := range schemas {
-			func() {
+				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 {
@@ -6921,9 +6981,9 @@ func TestUnknownField(t *testing.T) {
 						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() {
+				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 {
@@ -6947,7 +7007,8 @@ func TestUnknownField(t *testing.T) {
 						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
 					}
-			}()
+				})
-		}
+			}
+		})
 	}
 }

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)
+}