Merge pull request #53558 from nikhita/cr-strategic-merge-patch

Automatic merge from submit-queue (batch tested with PRs 54800, 53898, 54812, 54921, 53558). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Fix error for strategic merge patch of custom resources

Fixes #50037.

We need the go struct tags `patchMergeKey` and `patchStrategy` for fields that support a strategic merge patch. For native resources, we can easily figure out these tags since we know the fields.

Because custom resources are decoded as Unstructured and because we're missing the metadata about how to handle each field in a strategic merge patch, we can't find the go struct tags. Hence, we can't easily  do a strategic merge for custom resources.

So we should fail fast and return an error.

**Release note**:

```release-note
NONE
```

/cc @sttts @deads2k @ncdc

staging/src/k8s.io/apimachinery/pkg/util/mergepatch/errors.go

@@ -29,6 +29,7 @@ var (
 	ErrBadPatchFormatForRetainKeys          = errors.New("invalid patch format of retainKeys")
 	ErrBadPatchFormatForSetElementOrderList = errors.New("invalid patch format of setElementOrder list")
 	ErrPatchContentNotMatchRetainKeys       = errors.New("patch content doesn't match retainKeys list")
+	ErrUnsupportedStrategicMergePatchFormat = errors.New("strategic merge patch format is not supported")
 )
 
 func ErrNoMergeKey(m map[string]interface{}, k string) error {

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/BUILD

@@ -25,6 +25,7 @@ go_library(
     srcs = ["patch.go"],
     importpath = "k8s.io/apimachinery/pkg/util/strategicpatch",
     deps = [
+        "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/json:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/mergepatch:go_default_library",
         "//vendor/k8s.io/apimachinery/third_party/forked/golang/json:go_default_library",

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

@@ -22,6 +22,7 @@ import (
 	"sort"
 	"strings"
 
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/util/json"
 	"k8s.io/apimachinery/pkg/util/mergepatch"
 	forkedjson "k8s.io/apimachinery/third_party/forked/golang/json"
@@ -828,7 +829,7 @@ func handleUnmarshal(j []byte) (map[string]interface{}, error) {
 	return m, nil
 }
 
-// StrategicMergePatch applies a strategic merge patch. The original and patch documents
+// StrategicMergeMapPatch applies a strategic merge patch. The original and patch documents
 // must be JSONMap. A patch can be created from an original and modified document by
 // calling CreateTwoWayMergeMapPatch.
 // Warning: the original and patch JSONMap objects are mutated by this function and should not be reused.
@@ -837,6 +838,17 @@ func StrategicMergeMapPatch(original, patch JSONMap, dataStruct interface{}) (JS
 	if err != nil {
 		return nil, err
 	}
+
+	// We need the go struct tags `patchMergeKey` and `patchStrategy` for fields that support a strategic merge patch.
+	// For native resources, we can easily figure out these tags since we know the fields.
+
+	// Because custom resources are decoded as Unstructured and because we're missing the metadata about how to handle
+	// each field in a strategic merge patch, we can't find the go struct tags. Hence, we can't easily  do a strategic merge
+	// for custom resources. So we should fail fast and return an error.
+	if _, ok := dataStruct.(*unstructured.Unstructured); ok {
+		return nil, mergepatch.ErrUnsupportedStrategicMergePatchFormat
+	}
+
 	mergeOptions := MergeOptions{
 		MergeParallelList:    true,
 		IgnoreUnmatchedNulls: true,
@@ -1532,7 +1544,7 @@ func findMapInSliceBasedOnKeyValue(m []interface{}, key string, value interface{
 	for k, v := range m {
 		typedV, ok := v.(map[string]interface{})
 		if !ok {
-			return nil, 0, false, fmt.Errorf("value for key %v is not a map.", k)
+			return nil, 0, false, fmt.Errorf("value for key %v is not a map", k)
 		}
 
 		valueToMatch, ok := typedV[key]

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/BUILD

@@ -21,6 +21,7 @@ go_test(
         "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/serializer:go_default_library",

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

@@ -444,7 +444,7 @@ func applyPatchToObject(
 ) error {
 	patchedObjMap, err := strategicpatch.StrategicMergeMapPatch(originalMap, patchMap, versionedObj)
 	if err != nil {
-		return err
+		return interpretPatchError(err)
 	}
 
 	// Rather than serialize the patched map to JSON, then decode it to an object, we go directly from a map to an object
@@ -460,7 +460,7 @@ func applyPatchToObject(
 // interpretPatchError interprets the error type and returns an error with appropriate HTTP code.
 func interpretPatchError(err error) error {
 	switch err {
-	case mergepatch.ErrBadJSONDoc, mergepatch.ErrBadPatchFormatForPrimitiveList, mergepatch.ErrBadPatchFormatForRetainKeys, mergepatch.ErrBadPatchFormatForSetElementOrderList:
+	case mergepatch.ErrBadJSONDoc, mergepatch.ErrBadPatchFormatForPrimitiveList, mergepatch.ErrBadPatchFormatForRetainKeys, mergepatch.ErrBadPatchFormatForSetElementOrderList, mergepatch.ErrUnsupportedStrategicMergePatchFormat:
 		return errors.NewBadRequest(err.Error())
 	case mergepatch.ErrNoListOfLists, mergepatch.ErrPatchContentNotMatchRetainKeys:
 		return errors.NewGenericServerResponse(http.StatusUnprocessableEntity, "", schema.GroupResource{}, "", err.Error(), 0, false)

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go

@@ -29,6 +29,7 @@ import (
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	apierrors "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"
 	"k8s.io/apimachinery/pkg/runtime/serializer"
@@ -113,7 +114,39 @@ func TestPatchInvalid(t *testing.T) {
 
 	actual := &testPatchType{}
 	err := strategicPatchObject(codec, defaulter, original, []byte(patch), actual, &testPatchType{})
-	if apierrors.IsBadRequest(err) == false {
+	if !apierrors.IsBadRequest(err) {
+		t.Errorf("expected HTTP status: BadRequest, got: %#v", apierrors.ReasonForError(err))
+	}
+	if err.Error() != expectedError {
+		t.Errorf("expected %#v, got %#v", expectedError, err.Error())
+	}
+}
+
+func TestPatchCustomResource(t *testing.T) {
+	testGV := schema.GroupVersion{Group: "mygroup.example.com", Version: "v1beta1"}
+	scheme.AddKnownTypes(testGV, &unstructured.Unstructured{})
+	codec := codecs.LegacyCodec(testGV)
+	defaulter := runtime.ObjectDefaulter(scheme)
+
+	original := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"apiVersion": "mygroup.example.com/v1beta1",
+			"kind":       "Noxu",
+			"metadata": map[string]interface{}{
+				"namespace": "Namespaced",
+				"name":      "foo",
+			},
+			"spec": map[string]interface{}{
+				"num": "10",
+			},
+		},
+	}
+	patch := `{"spec":{"num":"20"}}`
+	expectedError := "strategic merge patch format is not supported"
+
+	actual := &unstructured.Unstructured{}
+	err := strategicPatchObject(codec, defaulter, original, []byte(patch), actual, &unstructured.Unstructured{})
+	if !apierrors.IsBadRequest(err) {
 		t.Errorf("expected HTTP status: BadRequest, got: %#v", apierrors.ReasonForError(err))
 	}
 	if err.Error() != expectedError {