From c48b37af547e60daa3a7e1e8b9b779b193731806 Mon Sep 17 00:00:00 2001 From: Oleg Zaytsev Date: Thu, 1 Dec 2022 15:46:22 +0100 Subject: [PATCH] Add default "time" param to parsed form (if any) (#3599) * Add default "time" param to parsed form (if any) If the form was already parsed by a downstream middleware, we need to add it to that form too, otherwise it won't be available to upstream by just adding it to the url query. Signed-off-by: Oleg Zaytsev * Check that form doesn't have "time" Signed-off-by: Oleg Zaytsev Signed-off-by: Oleg Zaytsev --- pkg/frontend/querymiddleware/roundtrip.go | 15 +++- .../querymiddleware/roundtrip_test.go | 81 +++++++++++++++++-- pkg/frontend/transport/handler_test.go | 59 ++++++++++++-- 3 files changed, 137 insertions(+), 18 deletions(-) diff --git a/pkg/frontend/querymiddleware/roundtrip.go b/pkg/frontend/querymiddleware/roundtrip.go index a1bc9334f94..37e55b8abf7 100644 --- a/pkg/frontend/querymiddleware/roundtrip.go +++ b/pkg/frontend/querymiddleware/roundtrip.go @@ -251,7 +251,6 @@ func newQueryTripperware( queryrange := newLimitedParallelismRoundTripper(next, codec, limits, queryRangeMiddleware...) instant := defaultInstantQueryParamsRoundTripper( newLimitedParallelismRoundTripper(next, codec, limits, queryInstantMiddleware...), - time.Now, ) return RoundTripFunc(func(r *http.Request) (*http.Response, error) { switch { @@ -308,12 +307,20 @@ func isInstantQuery(path string) bool { return strings.HasSuffix(path, instantQueryPathSuffix) } -func defaultInstantQueryParamsRoundTripper(next http.RoundTripper, now func() time.Time) http.RoundTripper { +func defaultInstantQueryParamsRoundTripper(next http.RoundTripper) http.RoundTripper { return RoundTripFunc(func(r *http.Request) (*http.Response, error) { - if isInstantQuery(r.URL.Path) && !r.URL.Query().Has("time") { + if isInstantQuery(r.URL.Path) && !r.Form.Has("time") && !r.URL.Query().Has("time") { + nowUnixStr := strconv.FormatInt(time.Now().Unix(), 10) + q := r.URL.Query() - q.Add("time", strconv.FormatInt(time.Now().Unix(), 10)) + q.Add("time", nowUnixStr) r.URL.RawQuery = q.Encode() + + // If form was already parsed, add this param to the form too. + // (The form doesn't have "time", otherwise we'd not be here) + if r.Form != nil { + r.Form.Set("time", nowUnixStr) + } } return next.RoundTrip(r) }) diff --git a/pkg/frontend/querymiddleware/roundtrip_test.go b/pkg/frontend/querymiddleware/roundtrip_test.go index 4468acf429c..c0247178d43 100644 --- a/pkg/frontend/querymiddleware/roundtrip_test.go +++ b/pkg/frontend/querymiddleware/roundtrip_test.go @@ -128,7 +128,6 @@ func TestInstantTripperware(t *testing.T) { ) require.NoError(t, err) - ts := time.Date(2021, 1, 2, 3, 4, 5, 0, time.UTC) rt := RoundTripFunc(func(r *http.Request) (*http.Response, error) { // We will provide a sample exactly for the requested time, // this way we'll also be able to tell which time was requested. @@ -153,11 +152,14 @@ func TestInstantTripperware(t *testing.T) { }) }) - queryClient, err := api.NewClient(api.Config{Address: "http://localhost", RoundTripper: tw(rt)}) - require.NoError(t, err) - api := v1.NewAPI(queryClient) + tripper := tw(rt) + + t.Run("specific time happy case", func(t *testing.T) { + queryClient, err := api.NewClient(api.Config{Address: "http://localhost", RoundTripper: tripper}) + require.NoError(t, err) + api := v1.NewAPI(queryClient) - t.Run("happy case roundtrip", func(t *testing.T) { + ts := time.Date(2021, 1, 2, 3, 4, 5, 0, time.UTC) res, _, err := api.Query(ctx, `sum(increase(we_dont_care_about_this[1h])) by (foo)`, ts) require.NoError(t, err) require.Equal(t, model.Vector{ @@ -165,8 +167,32 @@ func TestInstantTripperware(t *testing.T) { }, res) }) - t.Run("default time param", func(t *testing.T) { - res, _, err := api.Query(ctx, `sum(increase(we_dont_care_about_this[1h])) by (foo)`, time.Now()) + t.Run("specific time param with form being already parsed", func(t *testing.T) { + ts := time.Date(2021, 1, 2, 3, 4, 5, 0, time.UTC) + + formParserRoundTripper := RoundTripFunc(func(r *http.Request) (*http.Response, error) { + assert.NoError(t, r.ParseForm()) + return tripper.RoundTrip(r) + }) + queryClient, err := api.NewClient(api.Config{Address: "http://localhost", RoundTripper: formParserRoundTripper}) + require.NoError(t, err) + api := v1.NewAPI(queryClient) + + res, _, err := api.Query(ctx, `sum(increase(we_dont_care_about_this[1h])) by (foo)`, ts) + require.NoError(t, err) + require.IsType(t, model.Vector{}, res) + require.NotEmpty(t, res.(model.Vector)) + + resultTime := res.(model.Vector)[0].Timestamp.Time() + require.Equal(t, ts.Unix(), resultTime.Unix()) + }) + + t.Run("default time param happy case", func(t *testing.T) { + queryClient, err := api.NewClient(api.Config{Address: "http://localhost", RoundTripper: tripper}) + require.NoError(t, err) + api := v1.NewAPI(queryClient) + + res, _, err := api.Query(ctx, `sum(increase(we_dont_care_about_this[1h])) by (foo)`, time.Time{}) require.NoError(t, err) require.IsType(t, model.Vector{}, res) require.NotEmpty(t, res.(model.Vector)) @@ -174,6 +200,47 @@ func TestInstantTripperware(t *testing.T) { resultTime := res.(model.Vector)[0].Timestamp.Time() require.InDelta(t, time.Now().Unix(), resultTime.Unix(), 1) }) + + t.Run("default time param with form being already parsed", func(t *testing.T) { + formParserRoundTripper := RoundTripFunc(func(r *http.Request) (*http.Response, error) { + assert.NoError(t, r.ParseForm()) + return tripper.RoundTrip(r) + }) + queryClient, err := api.NewClient(api.Config{Address: "http://localhost", RoundTripper: formParserRoundTripper}) + require.NoError(t, err) + api := v1.NewAPI(queryClient) + + res, _, err := api.Query(ctx, `sum(increase(we_dont_care_about_this[1h])) by (foo)`, time.Time{}) + require.NoError(t, err) + require.IsType(t, model.Vector{}, res) + require.NotEmpty(t, res.(model.Vector)) + + resultTime := res.(model.Vector)[0].Timestamp.Time() + require.InDelta(t, time.Now().Unix(), resultTime.Unix(), 1) + }) + + t.Run("post form time param takes precedence over query time param ", func(t *testing.T) { + postFormTimeParam := time.Date(2021, 1, 2, 3, 4, 5, 0, time.UTC) + + addQueryTimeParam := RoundTripFunc(func(r *http.Request) (*http.Response, error) { + query := r.URL.Query() + // Set query's "time" param to something wrong, so that it would fail to be parsed if it's used. + query.Set("time", "query-time-param-should-not-be-used") + r.URL.RawQuery = query.Encode() + return tripper.RoundTrip(r) + }) + queryClient, err := api.NewClient(api.Config{Address: "http://localhost", RoundTripper: addQueryTimeParam}) + require.NoError(t, err) + api := v1.NewAPI(queryClient) + + res, _, err := api.Query(ctx, `sum(increase(we_dont_care_about_this[1h])) by (foo)`, postFormTimeParam) + require.NoError(t, err) + require.IsType(t, model.Vector{}, res) + require.NotEmpty(t, res.(model.Vector)) + + resultTime := res.(model.Vector)[0].Timestamp.Time() + require.Equal(t, postFormTimeParam.Unix(), resultTime.Unix()) + }) } func TestTripperware_Metrics(t *testing.T) { diff --git a/pkg/frontend/transport/handler_test.go b/pkg/frontend/transport/handler_test.go index ae92e6c3878..f74f6639bbf 100644 --- a/pkg/frontend/transport/handler_test.go +++ b/pkg/frontend/transport/handler_test.go @@ -10,6 +10,7 @@ import ( "io" "net/http" "net/http/httptest" + "net/url" "strings" "testing" @@ -52,21 +53,67 @@ func TestHandler_ServeHTTP(t *testing.T) { for _, tt := range []struct { name string cfg HandlerConfig + request func() *http.Request + expectedParams url.Values expectedMetrics int }{ { - name: "test handler with stats enabled", - cfg: HandlerConfig{QueryStatsEnabled: true}, + name: "handler with stats enabled, POST request with params", + cfg: HandlerConfig{QueryStatsEnabled: true, MaxBodySize: 1024}, + request: func() *http.Request { + form := url.Values{ + "query": []string{"some_metric"}, + "time": []string{"42"}, + } + r := httptest.NewRequest("POST", "/api/v1/query", strings.NewReader(form.Encode())) + r.Header.Add("Content-Type", "application/x-www-form-urlencoded") + return r + }, + expectedParams: url.Values{ + "query": []string{"some_metric"}, + "time": []string{"42"}, + }, expectedMetrics: 4, }, { - name: "test handler with stats disabled", - cfg: HandlerConfig{QueryStatsEnabled: false}, + name: "handler with stats enabled, GET request with params", + cfg: HandlerConfig{QueryStatsEnabled: true}, + request: func() *http.Request { + return httptest.NewRequest("GET", "/api/v1/query?query=some_metric&time=42", nil) + }, + expectedParams: url.Values{ + "query": []string{"some_metric"}, + "time": []string{"42"}, + }, + expectedMetrics: 4, + }, + { + name: "handler with stats enabled, GET request without params", + cfg: HandlerConfig{QueryStatsEnabled: true}, + request: func() *http.Request { + return httptest.NewRequest("GET", "/api/v1/query", nil) + }, + expectedParams: url.Values{}, + expectedMetrics: 4, + }, + { + name: "handler with stats disabled, GET request with params", + cfg: HandlerConfig{QueryStatsEnabled: false}, + request: func() *http.Request { + return httptest.NewRequest("GET", "/api/v1/query?query=some_metric&time=42", nil) + }, + expectedParams: url.Values{ + "query": []string{"some_metric"}, + "time": []string{"42"}, + }, expectedMetrics: 0, }, } { t.Run(tt.name, func(t *testing.T) { roundTripper := roundTripperFunc(func(req *http.Request) (*http.Response, error) { + assert.NoError(t, req.ParseForm()) + assert.Equal(t, tt.expectedParams, req.Form) + return &http.Response{ StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader("{}")), @@ -76,9 +123,7 @@ func TestHandler_ServeHTTP(t *testing.T) { reg := prometheus.NewPedanticRegistry() handler := NewHandler(tt.cfg, roundTripper, log.NewNopLogger(), reg) - ctx := user.InjectOrgID(context.Background(), "12345") - req := httptest.NewRequest("GET", "/", nil) - req = req.WithContext(ctx) + req := tt.request().WithContext(user.InjectOrgID(context.Background(), "12345")) resp := httptest.NewRecorder() handler.ServeHTTP(resp, req)