-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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.
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
[API] Handle plain text errors in client #8816
Conversation
Consul agent returns errors in plain text. Yet when this happens the api client still tries to decode them in JSON, failing, and hidding the real error message. In the case the Content-Type is text/plain, and the response code is an error one (>= 400), we return the response body as an error instead.
Hello, could you have a look at this? This PR fixes issues with haproxy-connect, and will be useful to other useful of this lib as well :
Thanks :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ShimmerGlass, thank you for the PR! Sorry it has taken us some time to respond.
This fix sounds good to me. I've left one small suggestion on the implementation which I think will make the errors easier to work with.
It would also be great to add a test for this change. I think https://golang.org/pkg/net/http/httptest/#Server can be used to setup a server with a handler which will return 4xx error codes. That should make it easy to test Client.query
.
return qm, err | ||
} | ||
|
||
return qm, fmt.Errorf("unexpected response: %s (%d)", string(body), resp.StatusCode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we expect the caller to look at the error code, I think we should expose the resp.StatusCode
as a field on an error type. Something like this:
type StatusError {
err error
Code int
}
func (e *StatusError) Error() string {
return fmt.Sprintf("unexpected response (%d): %v", e.Code, e.err)
}
That would allow callers to use https://pkg.go.dev/errors#As to get the exact error code out, without relying on string comparison (which can be error prone).
The StatusError
could be used here as:
return qm, &StatusError{Code: resp.StatusCode, err: errors.New(string(body))}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coincidentally, there was just a post on the forums asking for exactly this: https://discuss.hashicorp.com/t/golang-consul-api-pulling-out-the-statuscode-from-deeply-nested-responses/18037
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After digging a bit more, I found there are some API calls that handle this better already. For example, anything that uses requireOk
: https://github.com/hashicorp/consul/blob/master/api/api.go#L1007-L1019
Maybe this query
function should use requireOk
, and we can change requireOk
to use this new error type?
Thank you again for this PR! It seems this PR is not actively being worked on, so I am going to close it. This seems like a good thing to improve, so if anyone is still interested in making this change, please do open another PR and we will do our best to get it reviewed promptly. We have discussed improvements to our HTTP API, so at some point we may also look to return JSON responses for errors, which may also improve this situation. |
I created #10865 to track this bug. |
Consul agent returns errors in plain text. Yet when this happens the
api client still tries to decode them in JSON, failing, and hidding the
real error message.
In the case the Content-Type is text/plain, and the response code is an
error one (>= 400), we return the response body as an error instead.