From e0a2168ecbf8b4e43f932a32fa55cd55215123cc Mon Sep 17 00:00:00 2001 From: Nikhita Raghunath Date: Tue, 24 Oct 2017 17:26:03 +0530 Subject: [PATCH] apiserver: return 4xx for invalid patch Add interpretPatchError to return appropriate http code (400 or 422) according to the error type. We add this function in apiserver because we don't want to mention the http code in apimachinery. The apimachinery code is also used in kubectl. The client should not return a server error. Add a test to validate the http error code and error message. --- .../apimachinery/pkg/api/errors/errors.go | 33 ++++++++++--------- .../pkg/api/errors/errors_test.go | 4 +-- .../pkg/util/strategicpatch/patch.go | 2 +- .../apiserver/pkg/endpoints/handlers/patch.go | 28 +++++++++++----- .../pkg/endpoints/handlers/rest_test.go | 23 +++++++++++++ 5 files changed, 63 insertions(+), 27 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go b/staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go index d5503fac5d9..9960600be33 100644 --- a/staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go +++ b/staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go @@ -405,84 +405,84 @@ func NewGenericServerResponse(code int, verb string, qualifiedResource schema.Gr // IsNotFound returns true if the specified error was created by NewNotFound. func IsNotFound(err error) bool { - return reasonForError(err) == metav1.StatusReasonNotFound + return ReasonForError(err) == metav1.StatusReasonNotFound } // IsAlreadyExists determines if the err is an error which indicates that a specified resource already exists. func IsAlreadyExists(err error) bool { - return reasonForError(err) == metav1.StatusReasonAlreadyExists + return ReasonForError(err) == metav1.StatusReasonAlreadyExists } // IsConflict determines if the err is an error which indicates the provided update conflicts. func IsConflict(err error) bool { - return reasonForError(err) == metav1.StatusReasonConflict + return ReasonForError(err) == metav1.StatusReasonConflict } // IsInvalid determines if the err is an error which indicates the provided resource is not valid. func IsInvalid(err error) bool { - return reasonForError(err) == metav1.StatusReasonInvalid + return ReasonForError(err) == metav1.StatusReasonInvalid } // IsGone is true if the error indicates the requested resource is no longer available. func IsGone(err error) bool { - return reasonForError(err) == metav1.StatusReasonGone + return ReasonForError(err) == metav1.StatusReasonGone } // IsResourceExpired is true if the error indicates the resource has expired and the current action is // no longer possible. func IsResourceExpired(err error) bool { - return reasonForError(err) == metav1.StatusReasonExpired + return ReasonForError(err) == metav1.StatusReasonExpired } // IsMethodNotSupported determines if the err is an error which indicates the provided action could not // be performed because it is not supported by the server. func IsMethodNotSupported(err error) bool { - return reasonForError(err) == metav1.StatusReasonMethodNotAllowed + return ReasonForError(err) == metav1.StatusReasonMethodNotAllowed } // IsServiceUnavailable is true if the error indicates the underlying service is no longer available. func IsServiceUnavailable(err error) bool { - return reasonForError(err) == metav1.StatusReasonServiceUnavailable + return ReasonForError(err) == metav1.StatusReasonServiceUnavailable } // IsBadRequest determines if err is an error which indicates that the request is invalid. func IsBadRequest(err error) bool { - return reasonForError(err) == metav1.StatusReasonBadRequest + return ReasonForError(err) == metav1.StatusReasonBadRequest } // IsUnauthorized determines if err is an error which indicates that the request is unauthorized and // requires authentication by the user. func IsUnauthorized(err error) bool { - return reasonForError(err) == metav1.StatusReasonUnauthorized + return ReasonForError(err) == metav1.StatusReasonUnauthorized } // IsForbidden determines if err is an error which indicates that the request is forbidden and cannot // be completed as requested. func IsForbidden(err error) bool { - return reasonForError(err) == metav1.StatusReasonForbidden + return ReasonForError(err) == metav1.StatusReasonForbidden } // IsTimeout determines if err is an error which indicates that request times out due to long // processing. func IsTimeout(err error) bool { - return reasonForError(err) == metav1.StatusReasonTimeout + return ReasonForError(err) == metav1.StatusReasonTimeout } // IsServerTimeout determines if err is an error which indicates that the request needs to be retried // by the client. func IsServerTimeout(err error) bool { - return reasonForError(err) == metav1.StatusReasonServerTimeout + return ReasonForError(err) == metav1.StatusReasonServerTimeout } // IsInternalError determines if err is an error which indicates an internal server error. func IsInternalError(err error) bool { - return reasonForError(err) == metav1.StatusReasonInternalError + return ReasonForError(err) == metav1.StatusReasonInternalError } // IsTooManyRequests determines if err is an error which indicates that there are too many requests // that the server cannot handle. func IsTooManyRequests(err error) bool { - if reasonForError(err) == metav1.StatusReasonTooManyRequests { + if ReasonForError(err) == metav1.StatusReasonTooManyRequests { return true } switch t := err.(type) { @@ -536,7 +536,8 @@ func SuggestsClientDelay(err error) (int, bool) { return 0, false } -func reasonForError(err error) metav1.StatusReason { +// ReasonForError returns the HTTP status for a particular error. +func ReasonForError(err error) metav1.StatusReason { switch t := err.(type) { case APIStatus: return t.Status().Reason diff --git a/staging/src/k8s.io/apimachinery/pkg/api/errors/errors_test.go b/staging/src/k8s.io/apimachinery/pkg/api/errors/errors_test.go index afc26fd31c4..303a9d3f48f 100644 --- a/staging/src/k8s.io/apimachinery/pkg/api/errors/errors_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/api/errors/errors_test.go @@ -188,8 +188,8 @@ func TestNewInvalid(t *testing.T) { } } -func Test_reasonForError(t *testing.T) { - if e, a := metav1.StatusReasonUnknown, reasonForError(nil); e != a { +func TestReasonForError(t *testing.T) { + if e, a := metav1.StatusReasonUnknown, ReasonForError(nil); e != a { t.Errorf("unexpected reason type: %#v", a) } } 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 01014f9d36a..11588abdd58 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go @@ -1552,7 +1552,7 @@ func sortMergeListsByName(mapJSON []byte, dataStruct interface{}) ([]byte, error var m map[string]interface{} err := json.Unmarshal(mapJSON, &m) if err != nil { - return nil, err + return nil, mergepatch.ErrBadJSONDoc } newM, err := sortMergeListsByNameMap(m, reflect.TypeOf(dataStruct)) 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 b5d2192e30e..588cd7917d5 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go @@ -186,7 +186,7 @@ func patchResource( case types.JSONPatchType, types.MergePatchType: originalJS, patchedJS, err := patchObjectJSON(patchType, codec, currentObject, patchJS, objToUpdate, versionedObj) if err != nil { - return nil, err + return nil, interpretPatchError(err) } originalObjJS, originalPatchedObjJS = originalJS, patchedJS @@ -198,13 +198,13 @@ func patchResource( // Compute once originalPatchBytes, err = strategicpatch.CreateTwoWayMergePatch(originalObjJS, originalPatchedObjJS, versionedObj) if err != nil { - return nil, err + return nil, interpretPatchError(err) } } // Return a fresh map every time originalPatchMap := make(map[string]interface{}) if err := json.Unmarshal(originalPatchBytes, &originalPatchMap); err != nil { - return nil, err + return nil, errors.NewBadRequest(err.Error()) } return originalPatchMap, nil } @@ -242,7 +242,7 @@ func patchResource( getOriginalPatchMap = func() (map[string]interface{}, error) { patchMap := make(map[string]interface{}) if err := json.Unmarshal(patchJS, &patchMap); err != nil { - return nil, err + return nil, errors.NewBadRequest(err.Error()) } return patchMap, nil } @@ -277,7 +277,7 @@ func patchResource( var err error currentPatchMap, err = strategicpatch.CreateTwoWayMergeMapPatch(originalObjMap, currentObjMap, versionedObj) if err != nil { - return nil, err + return nil, interpretPatchError(err) } } else { // Compute current patch. @@ -287,11 +287,11 @@ func patchResource( } currentPatch, err := strategicpatch.CreateTwoWayMergePatch(originalObjJS, currentObjJS, versionedObj) if err != nil { - return nil, err + return nil, interpretPatchError(err) } currentPatchMap = make(map[string]interface{}) if err := json.Unmarshal(currentPatch, ¤tPatchMap); err != nil { - return nil, err + return nil, errors.NewBadRequest(err.Error()) } } @@ -422,7 +422,7 @@ func strategicPatchObject( patchMap := make(map[string]interface{}) if err := json.Unmarshal(patchJS, &patchMap); err != nil { - return err + return errors.NewBadRequest(err.Error()) } if err := applyPatchToObject(codec, defaulter, originalObjMap, patchMap, objToUpdate, versionedObj); err != nil { @@ -456,3 +456,15 @@ func applyPatchToObject( return nil } + +// 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: + return errors.NewBadRequest(err.Error()) + case mergepatch.ErrNoListOfLists, mergepatch.ErrPatchContentNotMatchRetainKeys: + return errors.NewGenericServerResponse(http.StatusUnprocessableEntity, "", schema.GroupResource{}, "", err.Error(), 0, false) + default: + return err + } +} 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 a99fdaf6fed..5e6ca117a27 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 @@ -98,6 +98,29 @@ func TestPatchAnonymousField(t *testing.T) { } } +func TestPatchInvalid(t *testing.T) { + testGV := schema.GroupVersion{Group: "", Version: "v"} + scheme.AddKnownTypes(testGV, &testPatchType{}) + codec := codecs.LegacyCodec(testGV) + defaulter := runtime.ObjectDefaulter(scheme) + + original := &testPatchType{ + TypeMeta: metav1.TypeMeta{Kind: "testPatchType", APIVersion: "v"}, + TestPatchSubType: TestPatchSubType{StringField: "my-value"}, + } + patch := `barbaz` + expectedError := "invalid character 'b' looking for beginning of value" + + actual := &testPatchType{} + err := strategicPatchObject(codec, defaulter, original, []byte(patch), actual, &testPatchType{}) + if apierrors.IsBadRequest(err) == false { + t.Errorf("expected HTTP status: BadRequest, got: %#v", apierrors.ReasonForError(err)) + } + if err.Error() != expectedError { + t.Errorf("expected %#v, got %#v", expectedError, err.Error()) + } +} + type testPatcher struct { t *testing.T