choose rpc code to determine status code by allencloud · Pull Request #32122 · moby/moby

allencloud · 2017-03-27T05:15:27Z

Signed-off-by: allencloud [email protected]

Since docker daemon use gRPC to communicate with swarmkit, so when docker daemon accepts http request and transfers to swarmkit side, the at last, swarmkit will return a gRPC code to docker daemon. For the user's request, we definitely need to return a response with an HTTP status code. This PR hopes to determine the http status code from the gRPC code.

this fixes #31909

- What I did

choose rpc code to determine status code
make secret operation use the correct status code. currently I only change the secret side.
add two test cases for issue Secrets API: Name conflict returns 500, should be 409 #31909
update docker-py to the version 2.3.0 in Dockerfiles

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

vdemeester

/cc @thaJeztah @stevvooe @aaronlehmann

cpuguy83 · 2017-03-27T13:19:38Z

I'd like to not propagate this usage.
At the very least we can catch grpc errors in the API package and produce the correct error code from there.

Thanks for your feedback. Like you mentioned, I tried to move that part into api/server/httputils/errors.go. PTAL
@cpuguy83

stevvooe · 2017-03-30T21:47:24Z

This is a mouthful.

newHTTPErrorFromGRPC

stevvooe · 2017-03-30T21:48:32Z

This is incorrect. 408 is only used when the client doesn't send a complete request.

A 504 would work here.

stevvooe · 2017-03-30T21:49:06Z

Conflict is a specific http error. This should be a 400 or some other error. There is already an open issue about the incorrect usage of 409. @justincormack

stevvooe · 2017-03-30T21:49:42Z

This is a specific http error that doesn't map to the same thing as the GRPC error code.

thaJeztah · 2017-04-10T14:31:52Z

typo s/non-exsiting/non-existing/ :joy:

thaJeztah · 2017-04-10T14:32:31Z

Looks like some tests need updating

cpuguy83

Thanks for moving this, seems like a better place to handle it, and in line with the changes requested in #32144

cpuguy83 · 2017-04-10T14:38:12Z

Can we move this up to top of the default case and instead pass the error directly rather than checking the error string?

Then we can gate the horrid string checking pattern currently above this with something like:

if statusCode == http.StatusInternalServerError { // horrible string parser }

Then we can gate the horrid string checking pattern currently above this with something like:

Sorry, actually I cannot follow this one.

Basically, move this above this stuff:

{"not found", http.StatusNotFound}, {"cannot find", http.StatusNotFound}, {"no such", http.StatusNotFound}, {"bad parameter", http.StatusBadRequest}, {"no command", http.StatusBadRequest}, {"conflict", http.StatusConflict}, {"impossible", http.StatusNotAcceptable}, {"wrong login/password", http.StatusUnauthorized}, {"unauthorized", http.StatusUnauthorized}, {"hasn't been activated", http.StatusForbidden}, {"this node", http.StatusServiceUnavailable}, {"needs to be unlocked", http.StatusServiceUnavailable}, {"certificates have expired", http.StatusServiceUnavailable},

And only check that stuff if the parsed statusCode from the grpc check is 500.

cpuguy83 · 2017-04-10T14:38:45Z

Why not implemented? Wouldn't this be StatusForbidden?

cpuguy83 · 2017-04-10T14:39:56Z

This does not seem like the correct code.
This is used when the server is acting as a proxy. In this case something just went really wrong. I think this would just be a 500.

While the engine may technically be acting as a proxy to containerd/swarm, I don't think this is something that should propagate up to the user.

What do you propose instead?

Yeah, in failed test:

11:17:28 docker_api_swarm_test.go:342: 11:17:28 c.Assert(status, checker.Equals, http.StatusInternalServerError, check.Commentf("deadline exceeded", string(out))) 11:17:28 ... obtained int = 504 11:17:28 ... expected int = 500 11:17:28 ... deadline exceeded%!(EXTRA string={"message":"rpc error: code = 4 desc = context deadline exceeded"} 11:17:28 ) 11:17:28 11:17:28 [d0f11ccd4b298] exiting daemon

Original code is 500

cpuguy83 · 2017-04-10T14:40:49Z

This should be conflict, I think

400 works too, just not 500.

well, technically I think 400 is for the HTTP server and shouldn't be used by us, but we do :)

cpuguy83 · 2017-04-10T14:49:13Z

We probably shouldn't make up new status code usages here. The description of this status does not seem like anything we'd ever require.

OutOfRange should just be mapped to 400 Bad Request.

cpuguy83 · 2017-04-10T14:50:33Z

Is swarm or containerd returning this ever? If so, is the HTTP code documented in the API?

cpuguy83 · 2017-04-10T14:50:43Z

Can move this to the default case

cpuguy83 · 2017-04-10T14:51:11Z

I don't think this would ever happen?

Yeah, updated. PTAL

allencloud · 2017-04-13T07:43:14Z

Yes, some tests need updating. @thaJeztah
I think POST /secrets/{id}/update introduces a status code of 400, which is included in PR #32464

And POST /nodes/{id}/update adds a case whose status code is originally 500, now to be 400.

I think I still need to update docs/api/version-history.md, WDYT?

allencloud · 2017-04-14T02:14:00Z

I am afraid such kind of change may influence docker-py, here is some evidence which docekr-py test failed:

10:32:09 =================================== FAILURES ===================================
10:32:09 _______________________ SwarmTest.test_remove_main_node ________________________
10:32:09 /docker-py/tests/integration/api_swarm_test.py:171: in test_remove_main_node
10:32:09     assert e.value.response.status_code == 500
10:32:09 E   AssertionError: assert 400 == 500
10:32:09 E    +  where 400 = <Response [400]>.status_code
10:32:09 E    +    where <Response [400]> = APIError(HTTPError('400 Client Error: Bad Request',),).response
10:32:09 E    +      where APIError(HTTPError('400 Client Error: Bad Request',),) = <ExceptionInfo APIError tblen=6>.value
10:32:09 ======== 1 failed, 205 passed, 31 skipped, 2 xfailed in 408.65 seconds =========
10:32:09 ---> Making bundle: .integration-daemon-stop (in bundles/17.06.0-dev/test-docker-py)

Any input for this, or what should I do for this? @shin-

thaJeztah · 2017-05-23T23:37:34Z

docker/docker-py#1621 was merged; can you try updating this PR?

allencloud · 2017-05-24T03:02:48Z

Thanks for your feedback, @thaJeztah
Actually this pr has been updated yesterday.

The status code test error in docker-py never happens. While another error in docker-py occurs:

04:31:10 =================================== FAILURES ===================================
04:31:10 ______________________ PruneImagesTest.test_prune_images _______________________
04:31:10 /docker-py/tests/integration/api_image_test.py:306: in test_prune_images
04:31:10     result = self.client.prune_images()
04:31:10 /docker-py/docker/utils/decorators.py:35: in wrapper
04:31:10     return f(self, *args, **kwargs)
04:31:10 /docker-py/docker/api/image.py:300: in prune_images
04:31:10     return self._result(self._post(url, params=params), True)
04:31:10 /docker-py/docker/utils/decorators.py:47: in inner
04:31:10     return f(self, *args, **kwargs)
04:31:10 /docker-py/docker/api/client.py:179: in _post
04:31:10     return self.post(url, **self._set_request_timeout(kwargs))
04:31:10 /usr/lib/python2.7/dist-packages/requests/sessions.py:500: in post
04:31:10     return self.request('POST', url, data=data, json=json, **kwargs)
04:31:10 /usr/lib/python2.7/dist-packages/requests/sessions.py:457: in request
04:31:10     resp = self.send(prep, **send_kwargs)
04:31:10 /usr/lib/python2.7/dist-packages/requests/sessions.py:569: in send
04:31:10     r = adapter.send(request, **kwargs)
04:31:10 /usr/lib/python2.7/dist-packages/requests/adapters.py:407: in send
04:31:10     raise ConnectionError(err, request=request)
04:31:10 E   ConnectionError: ('Connection aborted.', BadStatusLine("''",))
04:31:10 ========= 1 failed, 241 passed, 7 skipped, 3 xfailed in 377.00 seconds =========

I am still searching the error's root cause.

alfred-landrum · 2017-05-25T16:06:15Z

@allencloud - I have a PR that is also blocked on the test_prune_images failure, which seems to be unrelated to the content of my PR. I haven't been able to reproduce it locally, but will share anything I find.
#33005

allencloud · 2017-05-27T10:48:02Z

Thanks @alfred-landrum
I think it is good for us to see all green. And Is there anything more I could do to meet your change request. @cpuguy83

In addition, I think we still to add more status code change in version_history.md, right?
While I still do not know it will be included in which version. 1.30?
Currently I updated the 1.30 API changes in version_history.md. @thaJeztah

allencloud · 2017-05-31T17:14:28Z

Any update? ☀️

cpuguy83 · 2017-05-31T18:18:52Z

This string check is probably more expensive, and certainly more fragile, than just checking statusCodeFromGRPCError in all cases.

If we move this to the top of the default case, in many cases we won't have to mess around with this string handling.
so...

default: statusCode = statusCodeFromGRPCError(err) if statusCode == http.StatusInternalServerError { // nasty string comparison stuff }

Thanks for your feedback. @cpuguy83 I think what you said is reasonable. Thanks a lot.
While I think CI fails according to accident. Can you re-trigger that? @thaJeztah

Signed-off-by: allencloud <[email protected]>

allencloud · 2017-06-08T13:33:09Z

Does the change I added seem OK for you, @cpuguy83 ?
And feel free to tell me if I could do anything else? ☀️

thaJeztah · 2017-06-08T13:51:39Z

+* `DELETE /secrets/(name)` now returns status code 404 instead of 500 when the secret does not exist.
+* `POST /secrets/create` now returns status code 409 instead of 500 when creating an already existing secret.
+* `POST /secrets/(name)/update` now returns status code 400 instead of 500 when updating a secret's content which is not the labels.
+* `POST /nodes/(name)/update` now returns status code 400 instead of 500 when demoting last node fails.


Probably we need to update the swagger.yml as well?

I think there is no need to update swagger.yml.

cpuguy83 · 2017-06-08T13:54:06Z

@allencloud perfect!

LK4D4 · 2017-06-08T21:08:23Z

@cpuguy83 LGTY? feel free to merge if yes

cpuguy83 · 2017-06-08T21:11:01Z

Just needs the changes requested by @thaJeztah

allencloud · 2017-06-09T02:23:49Z

I think there is no need to update swagger.yml, since in swagger.yml no change will be involved.
For example,
DELETE /secrets/(name) already has code 404 in swagger.yml, but in old version of docker, like docker-17.05, it has no code of 404, but 404 is still in swagger.yml;
POST /secrets/create already has code 409;
POST /secrets/(name)/update already has two code 400 and 500, but one case changing from 500 to 400;
POST /nodes/(name)/update already has two code 400 and 500, but one case changing from 500 to 400;

So there will be no change for swagger.yml.
@thaJeztah @cpuguy83

thaJeztah · 2017-06-09T12:30:31Z

So there will be no change for swagger.yml.

Even better, thanks for checking 👍

cpuguy83

LGTM

thaJeztah

yes, LGTM as well 😅

GordonTheTurtle added the status/0-triage label Mar 27, 2017

vdemeester added status/1-design-review and removed status/0-triage labels Mar 27, 2017

vdemeester reviewed Mar 27, 2017

View reviewed changes

cpuguy83 requested changes Mar 27, 2017

View reviewed changes

allencloud force-pushed the choose-rpc-code-to-determine-status-code branch from a1ecc6f to 946ce45 Compare March 28, 2017 02:12

stevvooe reviewed Mar 30, 2017

View reviewed changes

Comment thread daemon/cluster/utils.go Outdated

Copy link
Copy Markdown

Contributor

stevvooe Mar 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a mouthful.

newHTTPErrorFromGRPC

stevvooe reviewed Mar 30, 2017

View reviewed changes

allencloud force-pushed the choose-rpc-code-to-determine-status-code branch 2 times, most recently from cd2e853 to d2d607a Compare April 1, 2017 03:41

allencloud force-pushed the choose-rpc-code-to-determine-status-code branch 4 times, most recently from 6263cdc to 6b4de0f Compare April 10, 2017 09:13

thaJeztah reviewed Apr 10, 2017

View reviewed changes

cpuguy83 requested changes Apr 10, 2017

View reviewed changes

GordonTheTurtle assigned LK4D4 Apr 11, 2017

allencloud force-pushed the choose-rpc-code-to-determine-status-code branch 7 times, most recently from f29dbe1 to 02b47ad Compare April 13, 2017 07:38

allencloud force-pushed the choose-rpc-code-to-determine-status-code branch from 02b47ad to b0936f9 Compare April 14, 2017 02:11

allencloud force-pushed the choose-rpc-code-to-determine-status-code branch 2 times, most recently from 4101773 to 30a8589 Compare May 23, 2017 02:14

alfred-landrum mentioned this pull request May 26, 2017

test_prune_images failing with recent docker-py #33407

Closed

allencloud force-pushed the choose-rpc-code-to-determine-status-code branch from 30a8589 to 56fae16 Compare May 27, 2017 05:28

allencloud force-pushed the choose-rpc-code-to-determine-status-code branch 2 times, most recently from dffd995 to 3b5d5c4 Compare May 29, 2017 04:31

cpuguy83 reviewed May 31, 2017

View reviewed changes

allencloud force-pushed the choose-rpc-code-to-determine-status-code branch 2 times, most recently from 93429fc to 224dec0 Compare June 5, 2017 08:32

choose rpc code to determine status code

f257f77

Signed-off-by: allencloud <[email protected]>

allencloud force-pushed the choose-rpc-code-to-determine-status-code branch from 224dec0 to f257f77 Compare June 6, 2017 02:09

thaJeztah added the rebuild/windowsRS1 label Jun 6, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Jun 6, 2017

thaJeztah reviewed Jun 8, 2017

View reviewed changes

cpuguy83 approved these changes Jun 9, 2017

View reviewed changes

thaJeztah approved these changes Jun 9, 2017

View reviewed changes

thaJeztah merged commit 27ca921 into moby:master Jun 9, 2017

allencloud deleted the choose-rpc-code-to-determine-status-code branch June 9, 2017 13:22

thaJeztah mentioned this pull request Jul 13, 2021

errdefs: remove dependency on grpc, containerd, and distribution #42624

Closed

Conversation

allencloud commented Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 10, 2017

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allencloud commented Apr 13, 2017

Uh oh!

allencloud commented Apr 14, 2017

Uh oh!

thaJeztah commented May 23, 2017

Uh oh!

allencloud commented May 24, 2017

Uh oh!

alfred-landrum commented May 25, 2017

Uh oh!

allencloud commented May 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allencloud commented May 31, 2017

allencloud commented Mar 27, 2017 •

edited

Loading

allencloud commented May 27, 2017 •

edited

Loading