diff --git a/staging/src/k8s.io/apimachinery/pkg/util/mergepatch/errors.go b/staging/src/k8s.io/apimachinery/pkg/util/mergepatch/errors.go index ac3c1e8cfcf..16501d5afe9 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/mergepatch/errors.go +++ b/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 { diff --git a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/BUILD b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/BUILD index 45770861ed6..cde41b3042d 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/BUILD +++ b/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", 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 11588abdd58..102a1c66630 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go +++ b/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] diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/BUILD b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/BUILD index d4aff8c66d9..92b7b282322 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/BUILD +++ b/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", diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go index 588cd7917d5..7c01aafd2ad 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go +++ b/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) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go index 5e6ca117a27..68e146422f6 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go +++ b/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 {