Skip to content

Return remote API errors as JSON #22880

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
vdemeester merged 1 commit into from
Jun 8, 2016
Merged

Return remote API errors as JSON #22880

vdemeester merged 1 commit into from
Jun 8, 2016

Conversation

@bfirsh
Copy link
Contributor

@bfirsh bfirsh commented May 21, 2016 •
edited
Loading

- What I did

The Remote API returns JSON responses on success and plain text responses on failure. There have been complaints that this is unexpected. It is also impossible to represent this using a Swagger API definition. It has also been mentioned in #5893.

This pull request makes the response body of API errors JSON in the form {"message": ...}. API versions <1.24 will continue to return plain text errors.

It depends upon docker-archive-public/docker.engine-api#233 but this PR is ready to review besides vendoring that.

- How to verify it

$ curl localhost:2375/fdshjkfdsfd
{"message":"page not found"}
$ curl localhost:2375/v1.24/fdshjkfdsfd
{"message":"page not found"}
$ curl localhost:2375/v1.23/fdshjkfdsfd
page not found
$ curl -X POST --data "{}" -H "Content-Type: application/json" localhost:2375/containers/create
{"message":"Config cannot be empty in order to create a container"}
$ curl -X POST --data "{}" -H "Content-Type: application/json" localhost:2375/v1.23/containers/create
Config cannot be empty in order to create a container

- Description for the changelog

Remote API errors are now returned as JSON.

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

404

@bfirsh bfirsh mentioned this pull request May 21, 2016
Merged
@bfirsh bfirsh force-pushed the remote-api-json-errors branch from 0f28216 to 49cbb77 Compare May 21, 2016 12:30
@thaJeztah
Copy link
Member

thaJeztah commented May 21, 2016

Also see the discussion here; #22104 /cc @justincormack @Silex

@Silex
Copy link

Silex commented May 22, 2016

I think it's fine, but should probably be part of a big release with big warnings in order to give time to adapt for tools used to the old format.

@thaJeztah
Copy link
Member

thaJeztah commented May 22, 2016

@Silex yes, there should be a warning in the changelog; of course systems can still pin to an older version of the API

@AkihiroSuda
Copy link
Member

AkihiroSuda commented May 23, 2016

+1 (non-binding), but I have a question: why do you need "code" verbosely in HTTP body, even though it is included in the HTTP header?

@bfirsh
Copy link
Contributor Author

bfirsh commented May 23, 2016

@AkihiroSuda It's a fairly common pattern putting the response code in REST API error responses. It's useful in cases where the client doesn't make it easy to get at the response code (e.g. curl) and stuff like that.

Agreed though that theoretically it's redundant, but I guess it's just a convenience thing. I don't have strong feelings about this, so happy to remove if others do have strong feelings!

@thaJeztah
Copy link
Member

thaJeztah commented May 23, 2016

I think I like the error in the response, but wondering if there's a standard JSON response structure for errors; if there is, would be nice to use that. Otherwise, I'm +1 to changing

@cpuguy83
Copy link
Member

cpuguy83 commented May 23, 2016

I don't think we should be conflating errors with wire format.
It's also not incredibly helpful to have an error message with a status code and a message.
I think we need to look at this at a grander scale.

Currently registry uses the errcode package which does indeed marshal the errors to json, but also provides a good way to determine what the error actual is and makes RPC significantly simpler.

@bfirsh
Copy link
Contributor Author

bfirsh commented May 23, 2016 •
edited
Loading

Closest thing to a standard: http://jsonapi.org/format/#errors

At the very least, we should be returning a JSON response with the message that usually gets returned as a plain text string. That could almost be considered a bug and is causing users problems. We can work our way towards a more sophisticated system with proper codes and multiple errors at some point in the future I suppose.

@cpuguy83
Copy link
Member

cpuguy83 commented May 23, 2016

@bfirsh But who are we breaking by not changing this now? Who will benefit from getting a json object with a message if we do change it now?

My biggest concern:
What will we have to change to make this useful for RPC?
I think we should spend a little extra time making this something we can use as soon as it's merged...Maybe the CLI would consume the errors and handle properly rather than the current string inspection it currently does.

@justincormack
Copy link
Contributor

justincormack commented May 23, 2016

@cpuguy83 the main benefit right now is uniformity, and consistency of always getting json. In future it makes it easier to add additional more structured error info, eg you could get a stack trace in debug mode or file + line number information about where the error came from, and we could wrap the errors from runc and containerd better.

@justincormack
Copy link
Contributor

justincormack commented May 24, 2016

The janky test failure should go away if you rebase

@bfirsh bfirsh force-pushed the remote-api-json-errors branch from 49cbb77 to bd9eec0 Compare May 25, 2016 18:06
@bfirsh
Copy link
Contributor Author

bfirsh commented May 25, 2016

@cpuguy83 Pretty much what @justincormack said. It's also makes it harder to write clients, and it is impossible to specify with a Swagger API specification.

I have updated this PR to use ErrorResponse from engine-api to represent errors instead of the jsonmessage.APIError. This also removes the code key to make this as uncontroversial as possible. It has dubious value, and message is the only thing that's essential to make this change. If we decide upon a standard way to represent errors, we can start adding more fields.

@bfirsh
Copy link
Contributor Author

bfirsh commented May 26, 2016

Some clarification about backwards compatibility: even if users aren't fixing to a particular version of the API, it is unlikely to affect them. If it is being consumed by a bit of code, they are probably using pattern matching to find the error, and the pattern will still be in the response. If it is being consumed by a human, worst case they still see the error with a bit of JSON around it.

@bfirsh bfirsh force-pushed the remote-api-json-errors branch from bd9eec0 to 67db6e4 Compare May 27, 2016 01:13
@icecrime icecrime added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jun 1, 2016
@thaJeztah
Copy link
Member

thaJeztah commented Jun 2, 2016

The status code has been removed in the engine-api change, and we're only returning a message for now (we can always add more if needed); docker-archive-public/docker.engine-api#233

I'm moving this to code review; design LGTM

@thaJeztah thaJeztah added this to the 1.12.0 milestone Jun 2, 2016
@bfirsh bfirsh force-pushed the remote-api-json-errors branch from 67db6e4 to 5614b8c Compare June 2, 2016 21:04
@bfirsh
Copy link
Contributor Author

bfirsh commented Jun 2, 2016

docker-archive-public/docker.engine-api#233 has been merged so properly vendored it now.

@bfirsh bfirsh force-pushed the remote-api-json-errors branch from 5614b8c to 31b03b4 Compare June 3, 2016 20:47
@thaJeztah thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Jun 6, 2016
vdemeester
vdemeester reviewed Jun 7, 2016
View reviewed changes
integration-cli/docker_api_attach_test.go Outdated
Copy link
Member

@vdemeester vdemeester Jun 7, 2016

Choose a reason for hiding this comment

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

That's a nit and not required for the PR (so it's more a quick note). We might want to add a helper for that (checkError(…)) 👼

Copy link
Contributor Author

@bfirsh bfirsh Jun 7, 2016

Choose a reason for hiding this comment

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

Agreed. Wasn't sure whether this was a good idea because a lot of other stuff in tests like this is explicit. Also, it might actually be less neat because we probably want a function that gets the message from the error then the assertion to be in the test, which means an extra line to check for errors when unmarshalling. I guess it could also be a go-checker checker.

Aaanyway. I will add a helper in a follow-up PR if I find time. :)

Copy link
Contributor Author

@bfirsh bfirsh Jun 8, 2016

Choose a reason for hiding this comment

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

okok I'm just making excuses. Added a getErrorMessage helper. ✨

@vdemeester
Copy link
Member

vdemeester commented Jun 7, 2016

gccgo is 💚.
code LGTM 🐹

@thaJeztah
Copy link
Member

thaJeztah commented Jun 7, 2016

LGTM

@thaJeztah
Copy link
Member

thaJeztah commented Jun 7, 2016

@vdemeester anything on the docs?

@vdemeester
Copy link
Member

vdemeester commented Jun 7, 2016

Docs LGTM 🐹
But needs a rebase 🙀

@bfirsh bfirsh force-pushed the remote-api-json-errors branch from 31b03b4 to 686fbf1 Compare June 7, 2016 22:28
@bfirsh
Copy link
Contributor Author

bfirsh commented Jun 7, 2016

rebased!

@bfirsh
Return remote API errors as JSON
322e2a7 
Signed-off-by: Ben Firshman <ben@firshman.co.uk>
@bfirsh bfirsh force-pushed the remote-api-json-errors branch from 686fbf1 to 322e2a7 Compare June 8, 2016 01:46
@bfirsh
Copy link
Contributor Author

bfirsh commented Jun 8, 2016 •
edited
Loading

Fixed some broken tests from the rebase and added getErrorMessage helper.

@vdemeester vdemeester merged commit a4422e6 into moby:master Jun 8, 2016
@thaJeztah thaJeztah mentioned this pull request Aug 9, 2016
Closed
@johnflavin johnflavin mentioned this pull request Sep 28, 2016
Closed
@thaJeztah thaJeztah mentioned this pull request Jan 22, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

impact/api impact/changelog status/4-merge

Projects

None yet

Milestone

1.12.0

Development

Successfully merging this pull request may close these issues.

9 participants

@bfirsh @thaJeztah @Silex @AkihiroSuda @cpuguy83 @justincormack @vdemeester @icecrime @GordonTheTurtle