api: return HTTP status on non-JSON API error bodies - #2062
Conversation
When Prometheus API error statuses (400/422) arrive with a non-JSON body from a proxy, prefer a status-based client error over a JSON unmarshal failure so callers see the real HTTP code. Signed-off-by: dpacgdm <dpac.gdm@gmail.com>
gnanirahulnutakki
left a comment
gnanirahulnutakki
left a comment
There was a problem hiding this comment.
I reproduced one remaining status-before-decode gap: syntactically valid non-envelope JSON on HTTP 400/422 can still return a nil error. Details and a regression case are inline. The existing api/prometheus/v1 package test suite passes on the PR branch.
| if jsonErr := json.Unmarshal(body, &result); jsonErr != nil { | ||
| // Non-2xx API error codes (400/422) with a non-JSON body (e.g. HTML from a | ||
| // proxy) should surface the HTTP status instead of a confusing unmarshal error. | ||
| if code/100 != 2 { |
There was a problem hiding this comment.
This still allows a non-2xx response to return a nil error when the body is valid JSON but not a Prometheus API envelope. For HTTP 400 with {"message":"proxy error"}, json.Unmarshal succeeds, leaves result.Status empty, and neither status check below sets err. I reproduced this with a TestAPIClientDo case; it failed with expected error, but got none.
Could the 400/422 path also fall back to errorTypeAndMsgFor when the decoded body is not a valid API error response, with this valid-but-non-envelope JSON case added as a regression test? That would preserve issue #763's requirement that the status code is never ignored.
2 participants
Summary
apiClientImpl.Donow returns a status-based error with the body asDetailinstead of a confusing JSON unmarshal failure.Addresses #763 (status-before-unmarshal for proxy/non-JSON error bodies). Body close/exhaust is already handled by the lower HTTP client and is out of scope here.
Test plan
go test ./api/prometheus/v1/ -run TestAPIClientDogo test ./api/prometheus/v1/ -run TestAPIs