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: Check status code before marshalling and returning response #763
Comments
It looks like the underlying implementation of |
Ah good point, @simonpasquier thanks for this. Yea, let's check how we can fix second AC. There are two options:
|
@bwplotka it seems the client already does the second option. What interesting is that the client closes the body twice. The response can be ignored if the go http client returns an error. The body is parsed in a different goroutine, but the result is awaited for anyway even if the context was cancelled. Also, it seems it is possible to have a nil context in the Do method. Which means ctx.Done() and ctx.Err() may lead to nil dereference. Currently, in case of an error the body is returned only in DoGetFallback. The documentation for this method says nothing about the body not being nil in case of error. So, if the body could be discarded, and this ticket was about refactoring the client I would suggest smth like:
|
Hey, Sorry for the delayed answer but I believe we should actually get a more systematic approach here. With @Hangzhi we are working on adding OpenAPI declaration for Thanos and Prometheus APIs and we can then host auto-generated client implementation (: |
I think this is a bug that we don't check status code before we start to do things in our client APIs code against Prometheus. Related discussion: #755 (comment)
The problem is that users can access the API through various proxies, so we can easily get 404, 403, and other 4xx or 5xx code without proper return type. This will cause very confusing unmarshalling error without even mentioning the response code. I think this is quite a serious problem and something that potential blocks Thanos to switch to those APIs.
AC:
Help wanted, I can guide someone through.
cc @beorn7 @lilic
The text was updated successfully, but these errors were encountered: