From 79349c93bddcc1125a9d6ea4528c6d63b172f083 Mon Sep 17 00:00:00 2001 From: Nikhita Raghunath Date: Sat, 7 Oct 2017 17:29:19 +0530 Subject: [PATCH] Fix error for strategic merge patch of custom resources 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. --- .../pkg/util/mergepatch/errors.go | 1 + .../pkg/util/strategicpatch/BUILD | 1 + .../pkg/util/strategicpatch/patch.go | 16 +++++++-- .../apiserver/pkg/endpoints/handlers/BUILD | 1 + .../apiserver/pkg/endpoints/handlers/patch.go | 4 +-- .../pkg/endpoints/handlers/rest_test.go | 35 ++++++++++++++++++- 6 files changed, 53 insertions(+), 5 deletions(-) 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 {