From 71da81fe14e8750f8ae29192124f2510e7486d3a Mon Sep 17 00:00:00 2001 From: Erlan Zholdubai uulu Date: Thu, 8 Aug 2024 14:27:21 -0700 Subject: [PATCH 1/2] fix for query_rejection bug Signed-off-by: Erlan Zholdubai uulu --- CHANGELOG.md | 1 + integration/query_frontend_test.go | 40 ++++++ .../tripperware/query_attribute_matcher.go | 117 +++++++++++------- .../query_attribute_matcher_test.go | 85 +++++++++++-- 4 files changed, 185 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b97cd3b3fd..811e761792d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,7 @@ * [BUGFIX] Scheduler: Fix user queue in scheduler that was not thread-safe. #6077 * [BUGFIX] Ingester: Include out-of-order head compaction when compacting TSDB head. #6108 * [BUGFIX] Ingester: Fix `cortex_ingester_tsdb_mmap_chunks_total` metric. #6134 +* [BUGFIX] Query Frontend: Fix query rejection bug for metadata queries. #6143 ## 1.17.1 2024-05-20 diff --git a/integration/query_frontend_test.go b/integration/query_frontend_test.go index 0af72024c40..a2c81ed95ff 100644 --- a/integration/query_frontend_test.go +++ b/integration/query_frontend_test.go @@ -798,4 +798,44 @@ func TestQueryFrontendQueryRejection(t *testing.T) { require.Equal(t, http.StatusUnprocessableEntity, resp.StatusCode) require.Contains(t, string(body), tripperware.QueryRejectErrorMessage) + newRuntimeConfig = []byte(`overrides: + user-1: + query_rejection: + enabled: true + query_attributes: + - query_step_limit: + min: 12s + - api_type: "labels" + - dashboard_uid: "dash123" + panel_id: "panel321" +`) + + require.NoError(t, client.Upload(context.Background(), configFileName, bytes.NewReader(newRuntimeConfig))) + time.Sleep(2 * time.Second) + + // We expect any request for speific api to be rejected if api_type is configured for that api and no other properties provided + resp, body, err = c.LabelNamesRaw([]string{}, time.Time{}, time.Time{}, map[string]string{}) + require.NoError(t, err) + require.Equal(t, http.StatusUnprocessableEntity, resp.StatusCode) + require.Contains(t, string(body), tripperware.QueryRejectErrorMessage) + + // We expect request not to be rejected if all the provided parameters are not applicable for current API type + // There is no dashboardUID or panelId in metadata queries so if only those provided then metadata query shouldn't be rejected. + resp, body, err = c.LabelValuesRaw("cluster", []string{}, time.Time{}, time.Time{}, map[string]string{"User-Agent": "grafana-agent/v0.19.0"}) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) + require.NotContains(t, string(body), tripperware.QueryRejectErrorMessage) + + // We expect instant request not to be rejected if only query_step_limit is provided (it's not applicable to instant queries) + resp, body, err = c.QueryRaw("{instance=~\"hello.*\"}", time.Time{}, map[string]string{}) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) + require.NotContains(t, string(body), tripperware.QueryRejectErrorMessage) + + // We expect range query request to be rejected even if only query_step_limit is provided + resp, body, err = c.QueryRangeRaw(`rate(test[1m])`, now.Add(-11*time.Hour), now.Add(-8*time.Hour), 20*time.Minute, map[string]string{"X-Dashboard-Uid": "dash123", "User-Agent": "grafana"}) + require.NoError(t, err) + require.Equal(t, http.StatusUnprocessableEntity, resp.StatusCode) + require.Contains(t, string(body), tripperware.QueryRejectErrorMessage) + } diff --git a/pkg/querier/tripperware/query_attribute_matcher.go b/pkg/querier/tripperware/query_attribute_matcher.go index de92d5e2c65..f7e4cdf5d87 100644 --- a/pkg/querier/tripperware/query_attribute_matcher.go +++ b/pkg/querier/tripperware/query_attribute_matcher.go @@ -14,7 +14,7 @@ import ( "github.com/cortexproject/cortex/pkg/util/validation" ) -const QueryRejectErrorMessage = "This query has been rejected by the service operator." +const QueryRejectErrorMessage = "This query has been rejected by the service operator due to its high load on the service and impact to performance of other queries." func rejectQueryOrSetPriority(r *http.Request, now time.Time, lookbackDelta time.Duration, limits Limits, userStr string, rejectedQueriesPerTenant *prometheus.CounterVec) error { if limits == nil || !(limits.QueryPriority(userStr).Enabled || limits.QueryRejection(userStr).Enabled) { @@ -86,89 +86,120 @@ func getOperation(r *http.Request) string { } func matchAttributeForExpressionQuery(attribute validation.QueryAttribute, op string, r *http.Request, query string, now time.Time, minTime, maxTime int64) bool { - if attribute.ApiType != "" && attribute.ApiType != op { - return false + matched := false + if attribute.ApiType != "" { + matched = true + if attribute.ApiType != op { + return false + } } - if attribute.Regex != "" && attribute.Regex != ".*" && attribute.Regex != ".+" { - if attribute.CompiledRegex != nil && !attribute.CompiledRegex.MatchString(query) { + if attribute.Regex != "" { + matched = true + if attribute.Regex != ".*" && attribute.Regex != ".+" && attribute.CompiledRegex != nil && !attribute.CompiledRegex.MatchString(query) { return false } } - if !isWithinTimeAttributes(attribute.TimeWindow, now, minTime, maxTime) { - return false + if attribute.TimeWindow.Start != 0 || attribute.TimeWindow.End != 0 { + matched = true + if !isWithinTimeAttributes(attribute.TimeWindow, now, minTime, maxTime) { + return false + } } - if !isWithinTimeRangeAttribute(attribute.TimeRangeLimit, minTime, maxTime) { - return false + if attribute.TimeRangeLimit.Min != 0 || attribute.TimeRangeLimit.Max != 0 { + matched = true + if !isWithinTimeRangeAttribute(attribute.TimeRangeLimit, minTime, maxTime) { + return false + } } - if op == "query_range" && !isWithinQueryStepLimit(attribute.QueryStepLimit, r) { - return false + if op == "query_range" && (attribute.QueryStepLimit.Min != 0 || attribute.QueryStepLimit.Max != 0) { + matched = true + if !isWithinQueryStepLimit(attribute.QueryStepLimit, r) { + return false + } } - if attribute.UserAgentRegex != "" && attribute.UserAgentRegex != ".*" && attribute.CompiledUserAgentRegex != nil { - if !attribute.CompiledUserAgentRegex.MatchString(r.Header.Get("User-Agent")) { + if attribute.UserAgentRegex != "" { + matched = true + if attribute.UserAgentRegex != ".*" && attribute.CompiledUserAgentRegex != nil && !attribute.CompiledUserAgentRegex.MatchString(r.Header.Get("User-Agent")) { return false } } - if attribute.DashboardUID != "" && attribute.DashboardUID != r.Header.Get("X-Dashboard-Uid") { - return false + if attribute.DashboardUID != "" { + matched = true + if attribute.DashboardUID != r.Header.Get("X-Dashboard-Uid") { + return false + } } - if attribute.PanelID != "" && attribute.PanelID != r.Header.Get("X-Panel-Id") { - return false + if attribute.PanelID != "" { + matched = true + if attribute.PanelID != r.Header.Get("X-Panel-Id") { + return false + } } - return true + return matched } func matchAttributeForMetadataQuery(attribute validation.QueryAttribute, op string, r *http.Request, now time.Time) bool { - if attribute.ApiType != "" && attribute.ApiType != op { - return false + matched := false + if attribute.ApiType != "" { + matched = true + if attribute.ApiType != op { + return false + } } if err := r.ParseForm(); err != nil { return false } - if attribute.Regex != "" && attribute.Regex != ".*" && attribute.CompiledRegex != nil { - atLeastOneMatched := false - for _, matcher := range r.Form["match[]"] { - if attribute.CompiledRegex.MatchString(matcher) { - atLeastOneMatched = true - break + if attribute.Regex != "" { + matched = true + if attribute.Regex != ".*" && attribute.CompiledRegex != nil { + atLeastOneMatched := false + for _, matcher := range r.Form["match[]"] { + if attribute.CompiledRegex.MatchString(matcher) { + atLeastOneMatched = true + break + } + } + if !atLeastOneMatched { + return false } - } - if !atLeastOneMatched { - return false } } startTime, _ := util.ParseTime(r.FormValue("start")) endTime, _ := util.ParseTime(r.FormValue("end")) - if !isWithinTimeAttributes(attribute.TimeWindow, now, startTime, endTime) { - return false + if attribute.TimeWindow.Start != 0 || attribute.TimeWindow.End != 0 { + matched = true + if !isWithinTimeAttributes(attribute.TimeWindow, now, startTime, endTime) { + return false + } } - if !isWithinTimeRangeAttribute(attribute.TimeRangeLimit, startTime, endTime) { - return false + if attribute.TimeRangeLimit.Min != 0 || attribute.TimeRangeLimit.Max != 0 { + matched = true + if !isWithinTimeRangeAttribute(attribute.TimeRangeLimit, startTime, endTime) { + return false + } } - if attribute.UserAgentRegex != "" && attribute.UserAgentRegex != ".*" && attribute.CompiledUserAgentRegex != nil { - if !attribute.CompiledUserAgentRegex.MatchString(r.Header.Get("User-Agent")) { + if attribute.UserAgentRegex != "" { + matched = true + if attribute.UserAgentRegex != ".*" && attribute.CompiledUserAgentRegex != nil && !attribute.CompiledUserAgentRegex.MatchString(r.Header.Get("User-Agent")) { return false } } - return true + return matched } func isWithinTimeAttributes(timeWindow validation.TimeWindow, now time.Time, startTime, endTime int64) bool { - if timeWindow.Start == 0 && timeWindow.End == 0 { - return true - } - if timeWindow.Start != 0 { startTimeThreshold := now.Add(-1 * time.Duration(timeWindow.Start).Abs()).Add(-1 * time.Minute).Truncate(time.Minute).UnixMilli() if startTime == 0 || startTime < startTimeThreshold { @@ -187,9 +218,6 @@ func isWithinTimeAttributes(timeWindow validation.TimeWindow, now time.Time, sta } func isWithinTimeRangeAttribute(limit validation.TimeRangeLimit, startTime, endTime int64) bool { - if limit.Min == 0 && limit.Max == 0 { - return true - } if startTime == 0 || endTime == 0 { return false @@ -208,9 +236,6 @@ func isWithinTimeRangeAttribute(limit validation.TimeRangeLimit, startTime, endT } func isWithinQueryStepLimit(queryStepLimit validation.QueryStepLimit, r *http.Request) bool { - if queryStepLimit.Min == 0 && queryStepLimit.Max == 0 { - return true - } step, err := util.ParseDurationMs(r.FormValue("step")) if err != nil { diff --git a/pkg/querier/tripperware/query_attribute_matcher_test.go b/pkg/querier/tripperware/query_attribute_matcher_test.go index dc3c204dd9c..cbd1f949f84 100644 --- a/pkg/querier/tripperware/query_attribute_matcher_test.go +++ b/pkg/querier/tripperware/query_attribute_matcher_test.go @@ -99,6 +99,7 @@ func Test_rejectQueryOrSetPriorityShouldRejectIfMatches(t *testing.T) { type testCase struct { queryRejectionEnabled bool path string + headers http.Header expectedError error expectedPriority int64 rejectQueryAttribute validation.QueryAttribute @@ -149,10 +150,10 @@ func Test_rejectQueryOrSetPriorityShouldRejectIfMatches(t *testing.T) { }, }, - "should reject if query rejection enabled with step limit and subQuery step match": { + "should not reject if query rejection enabled with step limit and query type is not range query": { queryRejectionEnabled: true, path: "/api/v1/query?time=1536716898&query=avg_over_time%28rate%28node_cpu_seconds_total%5B1m%5D%29%5B10m%3A5s%5D%29", //avg_over_time(rate(node_cpu_seconds_total[1m])[10m:5s]) - expectedError: httpgrpc.Errorf(http.StatusUnprocessableEntity, QueryRejectErrorMessage), + expectedError: nil, rejectQueryAttribute: validation.QueryAttribute{ QueryStepLimit: validation.QueryStepLimit{ Min: model.Duration(time.Second * 5), @@ -211,12 +212,64 @@ func Test_rejectQueryOrSetPriorityShouldRejectIfMatches(t *testing.T) { CompiledRegex: regexp.MustCompile(".*sum.*"), }, }, + + "should reject if only dashboard and panelId properties provided and query has both headers matching the property": { + queryRejectionEnabled: true, + path: fmt.Sprintf("/api/v1/query?start=%d&end=%d&step=7s&query=%s", now.Add(-30*time.Minute).UnixMilli()/1000, now.Add(-20*time.Minute).UnixMilli()/1000, url.QueryEscape("count(sum(up))")), + headers: http.Header{ + "X-Dashboard-Uid": {"dashboard-uid"}, + "X-Panel-Id": {"pane"}, + }, + expectedError: httpgrpc.Errorf(http.StatusUnprocessableEntity, QueryRejectErrorMessage), + rejectQueryAttribute: validation.QueryAttribute{ + DashboardUID: "dashboard-uid", + PanelID: "pane", + }, + }, + + "should not reject if query_rejection properties only provides dashboard_uuid and panel_id but query doesn't have those header": { + queryRejectionEnabled: true, + path: fmt.Sprintf("/api/v1/series?start=%d&end=%d&step=7s&match[]=%s", now.Add(-30*time.Minute).UnixMilli()/1000, now.Add(-20*time.Minute).UnixMilli()/1000, url.QueryEscape("count(sum(up))")), + headers: http.Header{}, + expectedError: nil, + rejectQueryAttribute: validation.QueryAttribute{ + PanelID: "panel", + DashboardUID: "dash123", + }, + }, + + "should not reject if query_rejection properties only provides dashboard_uid and query doesn't have X-Dashboard-Uid header": { + queryRejectionEnabled: true, + path: fmt.Sprintf("/api/v1/series?start=%d&end=%d&step=7s&match[]=%s", now.Add(-30*time.Minute).UnixMilli()/1000, now.Add(-20*time.Minute).UnixMilli()/1000, url.QueryEscape("count(sum(up))")), + headers: http.Header{ + "X-Panel-Id": {"pane"}, + }, + expectedError: nil, + rejectQueryAttribute: validation.QueryAttribute{ + DashboardUID: "dash123", + }, + }, + + "should not reject if query_rejection properties only provides user agent regex and query doesn't have User-Agent header": { + queryRejectionEnabled: true, + path: fmt.Sprintf("/api/v1/series?start=%d&end=%d&step=7s&match[]=%s", now.Add(-30*time.Minute).UnixMilli()/1000, now.Add(-20*time.Minute).UnixMilli()/1000, url.QueryEscape("count(sum(up))")), + headers: http.Header{ + "X-Dashboard-Uid": {"dashboard-uid"}, + "X-Panel-Id": {"pane"}, + }, + expectedError: nil, + rejectQueryAttribute: validation.QueryAttribute{ + UserAgentRegex: "^goclient", + CompiledUserAgentRegex: regexp.MustCompile("^goclient"), + }, + }, } for testName, testData := range tests { t.Run(testName, func(t *testing.T) { req, err := http.NewRequest("GET", testData.path, http.NoBody) require.NoError(t, err) + req.Header = testData.headers reqStats, ctx := stats.ContextWithEmptyStats(context.Background()) req = req.WithContext(ctx) limits.queryRejection.Enabled = testData.queryRejectionEnabled @@ -257,10 +310,10 @@ func Test_matchAttributeForExpressionQueryShouldMatchRegex(t *testing.T) { query: "count(sum(up))", result: true, }, - "should hit if regex is an empty string": { + "should miss if regex is an empty string and is only provided property of query_rejection": { regex: "", query: "sum(up)", - result: true, + result: false, }, } @@ -526,15 +579,15 @@ func Test_matchAttributeForExpressionQueryHeadersShouldBeCheckedIfSet(t *testing } tests := map[string]testCase{ - "should not check any of them if attributes are empty (match)": { - expectedResult: true, + "should consider no match if no properties provided for query_attributes": { + expectedResult: false, }, - "should not check if attributes are empty even corresponding headers exist (match)": { + "should consider no match if no properties provided for query_attributes even corresponding headers exist": { headers: http.Header{ "X-Dashboard-Uid": {"dashboard-uid"}, "X-Panel-Id": {"panel-id"}, }, - expectedResult: true, + expectedResult: false, }, "should match all attributes if all set and all headers provided": { headers: http.Header{ @@ -564,14 +617,14 @@ func Test_matchAttributeForExpressionQueryHeadersShouldBeCheckedIfSet(t *testing PanelID: "panel-id", }, }, - "should not compare if values are empty (match)": { + "should ignore if query_rejection property values are empty therefore shouldn't match if only those parameters was provided": { headers: http.Header{ "X-Panel-Id": {""}, }, queryAttribute: validation.QueryAttribute{ PanelID: "", }, - expectedResult: true, + expectedResult: false, }, "should match if headers match provided attributes ": { headers: http.Header{ @@ -583,6 +636,14 @@ func Test_matchAttributeForExpressionQueryHeadersShouldBeCheckedIfSet(t *testing }, expectedResult: true, }, + "should not match if only dashboard and panel properties are specified and query missing those headers (metadata queries)": { + headers: http.Header{}, + queryAttribute: validation.QueryAttribute{ + DashboardUID: "dashboard", + PanelID: "pane", + }, + expectedResult: false, + }, } for testName, testData := range tests { @@ -620,10 +681,10 @@ func Test_matchAttributeForExpressionQueryShouldMatchUserAgentRegex(t *testing.T userAgentHeader: "grafana-agent/v0.19.0", result: true, }, - "should hit if regex is an empty string": { + "should miss if regex is an empty string and only provided property for query_attribute": { userAgentRegex: "", userAgentHeader: "grafana-agent/v0.19.0", - result: true, + result: false, }, } From 0cd3e8d1d701a5bfbfe2c92cb3ba43eb66638455 Mon Sep 17 00:00:00 2001 From: Erlan Zholdubai uulu Date: Mon, 12 Aug 2024 16:14:06 -0700 Subject: [PATCH 2/2] fix for query_rejection bug. Change error message Signed-off-by: Erlan Zholdubai uulu --- pkg/querier/tripperware/query_attribute_matcher.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/querier/tripperware/query_attribute_matcher.go b/pkg/querier/tripperware/query_attribute_matcher.go index f7e4cdf5d87..169a50553fc 100644 --- a/pkg/querier/tripperware/query_attribute_matcher.go +++ b/pkg/querier/tripperware/query_attribute_matcher.go @@ -14,7 +14,7 @@ import ( "github.com/cortexproject/cortex/pkg/util/validation" ) -const QueryRejectErrorMessage = "This query has been rejected by the service operator due to its high load on the service and impact to performance of other queries." +const QueryRejectErrorMessage = "This query does not perform well and has been rejected by the service operator." func rejectQueryOrSetPriority(r *http.Request, now time.Time, lookbackDelta time.Duration, limits Limits, userStr string, rejectedQueriesPerTenant *prometheus.CounterVec) error { if limits == nil || !(limits.QueryPriority(userStr).Enabled || limits.QueryRejection(userStr).Enabled) {