Merge pull request #54477 from nikhita/invalid-patch-code

Automatic merge from submit-queue. 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>.

apiserver: return 4xx for invalid patch

Fixes #54423 

Currently, an invalid patch returns 500. The apiserver should return a 400 (`BadRequest`) or 422 (`Unprocessable Entity`).

**Release note**:

```release-note
NONE
```

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