From 07be95a0015c682faaa0f10dd25a51a411e99ab2 Mon Sep 17 00:00:00 2001 From: Andrej Ocenas Date: Wed, 15 Jun 2022 16:46:21 +0200 Subject: [PATCH] Prometheus: Fix body not being included in resource calls if they are POST (#50833) * Pass on request body * Fix GETs and add tests * Fix err handling * Add comments * Close response body * Fix lint maybe * Fix test --- pkg/tsdb/prometheus/client/client.go | 46 +++++++++---- pkg/tsdb/prometheus/client/client_test.go | 83 +++++++++++++++++++++++ pkg/tsdb/prometheus/resource/resource.go | 8 +-- 3 files changed, 115 insertions(+), 22 deletions(-) create mode 100644 pkg/tsdb/prometheus/client/client_test.go diff --git a/pkg/tsdb/prometheus/client/client.go b/pkg/tsdb/prometheus/client/client.go index 500125eb1ed..f6df3de7191 100644 --- a/pkg/tsdb/prometheus/client/client.go +++ b/pkg/tsdb/prometheus/client/client.go @@ -1,8 +1,8 @@ package client import ( + "bytes" "context" - "io/ioutil" "net/http" "net/url" "path" @@ -10,6 +10,7 @@ import ( "strings" "time" + "github.com/grafana/grafana-plugin-sdk-go/backend" "github.com/grafana/grafana/pkg/tsdb/prometheus/models" ) @@ -42,7 +43,7 @@ func (c *Client) QueryRange(ctx context.Context, q *models.Query) (*http.Respons qs.Set("end", formatTime(tr.End)) qs.Set("step", strconv.FormatFloat(tr.Step.Seconds(), 'f', -1, 64)) - return c.fetch(ctx, c.method, u, qs) + return c.fetch(ctx, c.method, u, qs, nil) } func (c *Client) QueryInstant(ctx context.Context, q *models.Query) (*http.Response, error) { @@ -60,7 +61,7 @@ func (c *Client) QueryInstant(ctx context.Context, q *models.Query) (*http.Respo qs.Set("time", formatTime(tr.End)) } - return c.fetch(ctx, c.method, u, qs) + return c.fetch(ctx, c.method, u, qs, nil) } func (c *Client) QueryExemplars(ctx context.Context, q *models.Query) (*http.Response, error) { @@ -77,36 +78,51 @@ func (c *Client) QueryExemplars(ctx context.Context, q *models.Query) (*http.Res qs.Set("start", formatTime(tr.Start)) qs.Set("end", formatTime(tr.End)) - return c.fetch(ctx, c.method, u, qs) + return c.fetch(ctx, c.method, u, qs, nil) } -func (c *Client) QueryResource(ctx context.Context, method string, p string, qs url.Values) (*http.Response, error) { - u, err := url.ParseRequestURI(c.baseUrl) +type FetchReq struct { + Method string + Url *url.URL + QueryString url.Values +} + +func (c *Client) QueryResource(ctx context.Context, req *backend.CallResourceRequest) (*http.Response, error) { + // The way URL is represented in CallResourceRequest and what we need for the fetch function is different + // so here we have to do a bit of parsing, so we can then compose it with the base url in correct way. + baseUrlParsed, err := url.ParseRequestURI(c.baseUrl) + if err != nil { + return nil, err + } + reqUrlParsed, err := url.Parse(req.URL) if err != nil { return nil, err } - u.Path = path.Join(u.Path, p) + baseUrlParsed.Path = path.Join(baseUrlParsed.Path, req.Path) + baseUrlParsed.RawQuery = reqUrlParsed.RawQuery - return c.fetch(ctx, method, u, qs) + return c.fetch(ctx, req.Method, baseUrlParsed, nil, req.Body) } -func (c *Client) fetch(ctx context.Context, method string, u *url.URL, qs url.Values) (*http.Response, error) { - if strings.ToUpper(method) == http.MethodGet { +func (c *Client) fetch(ctx context.Context, method string, u *url.URL, qs url.Values, body []byte) (*http.Response, error) { + // The qs arg seems to be used in some callers of this method, but you can already pass them in the URL object + if strings.ToUpper(method) == http.MethodGet && qs != nil { u.RawQuery = qs.Encode() } - - r, err := http.NewRequestWithContext(ctx, method, u.String(), nil) + bodyReader := bytes.NewReader(body) + request, err := http.NewRequestWithContext(ctx, method, u.String(), bodyReader) if err != nil { return nil, err } + // This may not be true but right now we don't have more information here and seems like we send just this type + // of encoding right now if it is a POST if strings.ToUpper(method) == http.MethodPost { - r.Body = ioutil.NopCloser(strings.NewReader(qs.Encode())) - r.Header.Set("Content-Type", "application/x-www-form-urlencoded") + request.Header.Set("Content-Type", "application/x-www-form-urlencoded") } - return c.doer.Do(r) + return c.doer.Do(request) } func formatTime(t time.Time) string { diff --git a/pkg/tsdb/prometheus/client/client_test.go b/pkg/tsdb/prometheus/client/client_test.go new file mode 100644 index 00000000000..cb2b2bd8e33 --- /dev/null +++ b/pkg/tsdb/prometheus/client/client_test.go @@ -0,0 +1,83 @@ +package client + +import ( + "context" + "io/ioutil" + "net/http" + "testing" + + "github.com/grafana/grafana-plugin-sdk-go/backend" + "github.com/grafana/grafana/pkg/cmd/grafana-cli/logger" + "github.com/stretchr/testify/require" +) + +type MockDoer struct { + Req *http.Request +} + +func (doer *MockDoer) Do(req *http.Request) (*http.Response, error) { + doer.Req = req + return &http.Response{ + StatusCode: http.StatusOK, + Status: "200 OK", + }, nil +} + +func TestClient(t *testing.T) { + t.Run("QueryResource", func(t *testing.T) { + doer := &MockDoer{} + // The method here does not really matter for resource calls + client := NewClient(doer, http.MethodGet, "http://localhost:9090") + + t.Run("sends correct POST request", func(t *testing.T) { + req := &backend.CallResourceRequest{ + PluginContext: backend.PluginContext{}, + Path: "/api/v1/series", + Method: http.MethodPost, + URL: "/api/v1/series", + Headers: nil, + Body: []byte("match%5B%5D: ALERTS\nstart: 1655271408\nend: 1655293008"), + } + res, err := client.QueryResource(context.Background(), req) + defer func() { + if res != nil && res.Body != nil { + if err := res.Body.Close(); err != nil { + logger.Warn("Error", "err", err) + } + } + }() + require.NoError(t, err) + require.NotNil(t, doer.Req) + require.Equal(t, http.MethodPost, doer.Req.Method) + body, err := ioutil.ReadAll(doer.Req.Body) + require.NoError(t, err) + require.Equal(t, []byte("match%5B%5D: ALERTS\nstart: 1655271408\nend: 1655293008"), body) + require.Equal(t, "http://localhost:9090/api/v1/series", doer.Req.URL.String()) + }) + + t.Run("sends correct GET request", func(t *testing.T) { + req := &backend.CallResourceRequest{ + PluginContext: backend.PluginContext{}, + Path: "/api/v1/series", + Method: http.MethodGet, + URL: "api/v1/series?match%5B%5D=ALERTS&start=1655272558&end=1655294158", + Headers: nil, + } + res, err := client.QueryResource(context.Background(), req) + defer func() { + if res != nil && res.Body != nil { + if err := res.Body.Close(); err != nil { + logger.Warn("Error", "err", err) + } + } + }() + require.NoError(t, err) + require.NotNil(t, doer.Req) + require.Equal(t, http.MethodGet, doer.Req.Method) + body, err := ioutil.ReadAll(doer.Req.Body) + require.NoError(t, err) + require.Equal(t, []byte{}, body) + require.Equal(t, "http://localhost:9090/api/v1/series?match%5B%5D=ALERTS&start=1655272558&end=1655294158", doer.Req.URL.String()) + }) + }) +} diff --git a/pkg/tsdb/prometheus/resource/resource.go b/pkg/tsdb/prometheus/resource/resource.go index 44babce41c5..c6479f68185 100644 --- a/pkg/tsdb/prometheus/resource/resource.go +++ b/pkg/tsdb/prometheus/resource/resource.go @@ -8,7 +8,6 @@ import ( "io" "io/ioutil" "net/http" - "net/url" "strings" "github.com/grafana/grafana-plugin-sdk-go/backend" @@ -133,12 +132,7 @@ func (r *Resource) Execute(ctx context.Context, req *backend.CallResourceRequest func (r *Resource) fetch(ctx context.Context, client *client.Client, req *backend.CallResourceRequest) (int, []byte, error) { r.log.Debug("Sending resource query", "URL", req.URL) - u, err := url.Parse(req.URL) - if err != nil { - return 500, nil, err - } - - resp, err := client.QueryResource(ctx, req.Method, u.Path, u.Query()) + resp, err := client.QueryResource(ctx, req) if err != nil { statusCode := 500 if resp != nil {