return 400 status when invalid json patch passed to apiserver #68346

CaoShuFeng · 2018-09-06T10:00:44Z

related to : #68202

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

kube-apiserver would return 400 Bad Request when it couldn't decode a json patch.
kube-apiserver would return 422 Unprocessable Entity when a json patch couldn't be applied to one object.

felixfbecker

It's important to distinguish between an invalid patch and a failed test operation.

felixfbecker · 2018-09-06T10:21:06Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

+		}
+		patchedJS, err := patchObj.Apply(versionedJS)
+		if err != nil {
+			return nil, errors.NewBadRequest(err.Error())


I assume that this is the branch that gets hit when a test operation fails. Is there a way to check for that error specifically and send a 409 Conflict? A failed test operation is very different from an "invalid" patch (e.g. wrong paths etc) and it needs to be possible to handle it specifically on the client. A failed test operation means a failed precondition and the client likely just wants to update the local state from the server, regenerate the patch and try again. If the patch is invalid it would not try again.

Agree that 400 is not good here.

fedebongio · 2018-09-06T20:24:11Z

/assign @caesarxuchao

caesarxuchao · 2018-09-07T20:57:19Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

@@ -284,9 +284,13 @@ func (p *jsonPatcher) applyJSPatch(versionedJS []byte) (patchedJS []byte, retErr
 	case types.JSONPatchType:
 		patchObj, err := jsonpatch.DecodePatch(p.patchJS)
 		if err != nil {
-			return nil, err
+			return nil, errors.NewBadRequest(err.Error())


Can you use the interpretPatchError(err) like other places in this file?

interpretPatchError can only deal with mergepatch errors

Got it.

Can you remove the interpretPatchError at this line:

kubernetes/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

Line 268 in d472a54

return nil, interpretPatchError(err)

? Because it does nothing.

Could you rename the interpretPatchError function to interpretStrategicMergePatchError, since that's what it handles.

I agree that we should fix upstream to make json-patch return typed errors. Then we can make interpretPatchError interprets json-patch errors.

caesarxuchao · 2018-09-07T20:57:46Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

+		}
+		patchedJS, err := patchObj.Apply(versionedJS)
+		if err != nil {
+			return nil, errors.NewGenericServerResponse(http.StatusUnprocessableEntity, "", schema.GroupResource{}, "", err.Error(), 0, false)


caesarxuchao · 2018-09-07T21:03:44Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go

@@ -326,6 +327,9 @@ func (tc *patchTestCase) Run(t *testing.T) {
 		patch := []byte{}
 		switch patchType {
 		case types.StrategicMergePatchType:
+			if len(tc.jsonPatch) > 0 {
+				continue
+			}


Are these lines for debugging only? If so please remove.

caesarxuchao · 2018-09-07T21:09:42Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go

+	tc.startingPod.UID = uid
+	tc.startingPod.ResourceVersion = "2"
+	tc.startingPod.APIVersion = examplev1.SchemeGroupVersion.String()
+	tc.updatePod = tc.startingPod


Maybe you don't need updatePod at all? It's weird to have updatePod: &example.Pod{} and then tc.updatePod = tc.startingPod.

Actually why not using setTcPod like other test cases?

caesarxuchao · 2018-09-07T21:14:08Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go

+	tc.startingPod.UID = uid
+	tc.startingPod.ResourceVersion = "2"
+	tc.startingPod.APIVersion = examplev1.SchemeGroupVersion.String()
+	tc.updatePod = tc.startingPod


I would have made a utility function to return a pod pointer to avoid duplicate code.

felixfbecker · 2018-09-10T08:42:44Z

My comment above got collapsed by GitHub, but this still does not fix the linked issue. It is an improvement, but #68202 is specifically about failed test operations which should return a distinct status code (409 Conflict).

CaoShuFeng · 2018-09-10T08:52:15Z

My comment above got collapsed by GitHub, but this still does not fix the linked issue. It is an improvement, but #68202 is specifically about failed test operations which should return a distinct status code (409 Conflict).

I see. Maybe we should fix this in upstream first.
json-patch just return fmt.Errorf for all kinds of errors. So we cant't distinguish the error should be a 409 or 422 here.

https://github.com/evanphx/json-patch/blob/master/patch.go#L536

felixfbecker · 2018-09-10T08:56:04Z

@CaoShuFeng got it. In that case please remove the "Fix: #68202" from the PR description so the issue doesn't get closed. Thanks for the improvements in this PR!

CaoShuFeng · 2018-09-10T08:59:24Z

. In that case please remove the "Fix: #68202" from the PR description so the issue doesn't get closed.

Done.

caesarxuchao · 2018-09-10T18:44:05Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go

@@ -604,6 +618,52 @@ func TestPatchResourceWithRacingVersionConflict(t *testing.T) {
 	tc.Run(t)
 }

+func TestPatchResourceWithInvalidJSONPatch(t *testing.T) {


It seems you need to change the patchTestCase framework a bit to make it work for your two test cases.

Instead of making that framework more complicated, how about making a focused test for applyJSPatch? You don't need to fake too much to make it work.

And you can move the tests to right below TestPatchInvalid. (And you can rename TestPatchInvalid to TestStrategicMergePatchInvalid to make is clear).

What do you think?

caesarxuchao

lgtm except for one nit.

caesarxuchao · 2018-09-11T17:43:10Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go

+		versionedJS, err := runtime.Encode(codec, pod)
+		if err != nil {
+			t.Errorf("%s: unexpected error: %v", test.name, err)
+			return


use Fatalf?

Done.

replace return with continue, just as other places do.

caesarxuchao · 2018-09-18T01:14:56Z

/lgtm

CaoShuFeng · 2018-09-18T01:20:43Z

/assign @sttts
for approve
Thanks.

k8s-ci-robot · 2018-09-18T01:20:44Z

@CaoShuFeng: GitHub didn't allow me to assign the following users: for, approve.

Note that only kubernetes members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign @sttts for approve
Thanks.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

caesarxuchao · 2018-09-19T16:59:21Z

/assign @lavalamp

for approval and possibly apply milestone.

lavalamp · 2018-09-19T17:23:25Z

/lgtm
/approve

k8s-ci-robot · 2018-09-19T17:23:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, CaoShuFeng, lavalamp

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/apiserver/OWNERS~~ [lavalamp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

lavalamp · 2018-09-19T17:24:33Z

Please add a release note explaining the status code change.

lavalamp · 2018-09-19T17:25:03Z

(I think this isn't urgent and can wait until master opens back up for merges.)

CaoShuFeng · 2018-09-20T02:31:02Z

Please add a release note explaining the status code change.

Done.

k8s-ci-robot requested review from caesarxuchao and tallclair September 6, 2018 10:00

CaoShuFeng mentioned this pull request Sep 6, 2018

Failed test operation in JSON Patch should return 4xx, not 500 Internal Server Error #68202

Closed

felixfbecker reviewed Sep 6, 2018

View reviewed changes

CaoShuFeng force-pushed the 400_500 branch from fb22171 to 887d2b6 Compare September 6, 2018 13:11

k8s-ci-robot assigned caesarxuchao Sep 6, 2018

caesarxuchao reviewed Sep 7, 2018

View reviewed changes

CaoShuFeng force-pushed the 400_500 branch from 887d2b6 to c8bb50a Compare September 10, 2018 08:30

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 10, 2018

caesarxuchao reviewed Sep 10, 2018

View reviewed changes

CaoShuFeng force-pushed the 400_500 branch from c8bb50a to b3a2cf4 Compare September 11, 2018 06:15

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 11, 2018

CaoShuFeng force-pushed the 400_500 branch from b3a2cf4 to 72c4fa4 Compare September 11, 2018 06:24

caesarxuchao reviewed Sep 11, 2018

View reviewed changes

return 400 status when invalid json patch passed to apiserver

1248f56

CaoShuFeng force-pushed the 400_500 branch from 72c4fa4 to 1248f56 Compare September 12, 2018 02:07

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 18, 2018

k8s-ci-robot assigned sttts Sep 18, 2018

nikhita mentioned this pull request Sep 19, 2018

apiserver: return 4xx for invalid json patch #68838

Closed

k8s-ci-robot assigned lavalamp Sep 19, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 19, 2018

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 20, 2018

k8s-ci-robot merged commit 48e93c7 into kubernetes:master Sep 25, 2018

jennybuckley mentioned this pull request Jan 5, 2019

API server should return 4xx for invalid path in JSON Patch, not 500 Internal Server Error #68224

Closed

aanm mentioned this pull request Mar 1, 2019

Automated cherry pick of #68346: return 400 status when invalid json patch passed to apiserver #74824

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

return 400 status when invalid json patch passed to apiserver #68346

return 400 status when invalid json patch passed to apiserver #68346

CaoShuFeng commented Sep 6, 2018 •

edited

felixfbecker left a comment

felixfbecker Sep 6, 2018

CaoShuFeng Sep 6, 2018

fedebongio commented Sep 6, 2018

caesarxuchao Sep 7, 2018

CaoShuFeng Sep 10, 2018

caesarxuchao Sep 10, 2018

CaoShuFeng Sep 11, 2018

caesarxuchao Sep 7, 2018

caesarxuchao Sep 7, 2018

CaoShuFeng Sep 10, 2018

caesarxuchao Sep 7, 2018

caesarxuchao Sep 7, 2018

CaoShuFeng Sep 10, 2018

caesarxuchao Sep 7, 2018

CaoShuFeng Sep 10, 2018

felixfbecker commented Sep 10, 2018

CaoShuFeng commented Sep 10, 2018 •

edited

felixfbecker commented Sep 10, 2018

CaoShuFeng commented Sep 10, 2018

caesarxuchao Sep 10, 2018

CaoShuFeng Sep 11, 2018

caesarxuchao left a comment

caesarxuchao Sep 11, 2018

CaoShuFeng Sep 12, 2018

caesarxuchao commented Sep 18, 2018

CaoShuFeng commented Sep 18, 2018 •

edited

k8s-ci-robot commented Sep 18, 2018

caesarxuchao commented Sep 19, 2018

lavalamp commented Sep 19, 2018

k8s-ci-robot commented Sep 19, 2018

lavalamp commented Sep 19, 2018

lavalamp commented Sep 19, 2018

CaoShuFeng commented Sep 20, 2018

return 400 status when invalid json patch passed to apiserver #68346

return 400 status when invalid json patch passed to apiserver #68346

Conversation

CaoShuFeng commented Sep 6, 2018 • edited

felixfbecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedebongio commented Sep 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfbecker commented Sep 10, 2018

CaoShuFeng commented Sep 10, 2018 • edited

felixfbecker commented Sep 10, 2018

CaoShuFeng commented Sep 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Sep 18, 2018

CaoShuFeng commented Sep 18, 2018 • edited

k8s-ci-robot commented Sep 18, 2018

caesarxuchao commented Sep 19, 2018

lavalamp commented Sep 19, 2018

k8s-ci-robot commented Sep 19, 2018

lavalamp commented Sep 19, 2018

lavalamp commented Sep 19, 2018

CaoShuFeng commented Sep 20, 2018

CaoShuFeng commented Sep 6, 2018 •

edited

CaoShuFeng commented Sep 10, 2018 •

edited

CaoShuFeng commented Sep 18, 2018 •

edited