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.

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

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

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

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, &currentPatchMap); 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
+	}
+}

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