api: Check status code before marshalling and returning response

[bwplotka]

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: https://github.com/prometheus/client_golang/pull/755#discussion_r427176238

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:

• Code like this https://github.com/prometheus/client_golang/blob/master/api/prometheus/v1/api.go#L568 should never ignore status code
• Make sure Body is always closed and exhausted (I am not sure if unmarshall gives us that)

Help wanted, I can guide someone through.

cc @beorn7 @lilic

[simonpasquier]

It looks like the underlying implementation of Do() already checks the HTTP response code. From what I can tell no caller of the Do() and DoGetFallback() functions (from the apiClient interface) use the returned *http.Response so maybe it shouldn't be returned at all and the functions should take care of closing the body?

[bwplotka]

Ah good point, @simonpasquier thanks for this.

Yea, let's check how we can fix second AC. There are two options:

1. Caller should close and exhaust as you proposed
2. We read body and close, and pass just buffered bytes. That makes sense only if we are sure that client does not stream. e.g for remote read series, that is NOT true (we recommend streaming)

[mneverov]

@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:

func (c *httpClient) Do(ctx context.Context, req *http.Request) (*http.Response, []byte, error) {
	if ctx == nil {
		ctx = context.Background()
	}
	req = req.WithContext(ctx)
	resp, err := c.client.Do(req)

	if err != nil {
		return nil, nil, err
	}

	var body []byte
	done := make(chan struct{})
	go func() {
		defer func() {
			resp.Body.Close()
		}()
		body, err = ioutil.ReadAll(resp.Body)
		close(done)
	}()

	select {
	case <-ctx.Done():
		if err == nil {
			err = ctx.Err()
		}
	case <-done:
	}

	return resp, body, err
}

[bwplotka]

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 (: