fix query priority throws 5xx when 4xx (#5704)

* fix query priority throws 5xx when 4xx Signed-off-by: Ben Ye <[email protected]> * fix lint Signed-off-by: Ben Ye <[email protected]> --------- Signed-off-by: Ben Ye <[email protected]>
cortexproject · Dec 9, 2023 · 74a75c7 · 74a75c7
1 parent 83be88f
commit 74a75c7
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 5 deletions.
diff --git a/pkg/querier/tripperware/priority.go b/pkg/querier/tripperware/priority.go
@@ -1,6 +1,7 @@
 package tripperware
 
 import (
+	"errors"
 	"net/http"
 	"strings"
 	"time"
@@ -11,6 +12,10 @@ import (
 	"github.com/cortexproject/cortex/pkg/util/validation"
 )
 
+var (
+	errParseExpr = errors.New("failed to parse expr")
+)
+
 func GetPriority(r *http.Request, userID string, limits Limits, now time.Time, lookbackDelta time.Duration) (int64, error) {
 	isQuery := strings.HasSuffix(r.URL.Path, "/query")
 	isQueryRange := strings.HasSuffix(r.URL.Path, "/query_range")
@@ -23,7 +28,9 @@ func GetPriority(r *http.Request, userID string, limits Limits, now time.Time, l
 
 	expr, err := parser.ParseExpr(query)
 	if err != nil {
-		return 0, err
+		// If query fails to be parsed, we throw a simple parse error
+		// and fail query later on querier.
+		return 0, errParseExpr
 	}
 
 	if len(queryPriority.Priorities) == 0 {

diff --git a/pkg/querier/tripperware/priority_test.go b/pkg/querier/tripperware/priority_test.go
@@ -14,7 +14,7 @@ import (
 	"github.com/cortexproject/cortex/pkg/util/validation"
 )
 
-func Test_GetPriorityShouldReturnDefaultPriorityIfNotEnabledOrEmptyQueryString(t *testing.T) {
+func Test_GetPriorityShouldReturnDefaultPriorityIfNotEnabledOrInvalidQueryString(t *testing.T) {
 	now := time.Now()
 	limits := mockLimits{queryPriority: validation.QueryPriority{
 		Priorities: []validation.PriorityDef{
@@ -33,6 +33,7 @@ func Test_GetPriorityShouldReturnDefaultPriorityIfNotEnabledOrEmptyQueryString(t
 	type testCase struct {
 		url                  string
 		queryPriorityEnabled bool
+		err                  error
 	}
 
 	tests := map[string]testCase{
@@ -43,10 +44,20 @@ func Test_GetPriorityShouldReturnDefaultPriorityIfNotEnabledOrEmptyQueryString(t
 			url:                  "/query?query=",
 			queryPriorityEnabled: true,
 		},
+		"shouldn't return error if query is invalid": {
+			url:                  "/query?query=up[4h",
+			queryPriorityEnabled: true,
+			err:                  errParseExpr,
+		},
 		"should miss if query string empty - range query": {
 			url:                  "/query_range?query=",
 			queryPriorityEnabled: true,
 		},
+		"shouldn't return error if query is invalid, range query": {
+			url:                  "/query_range?query=up[4h",
+			queryPriorityEnabled: true,
+			err:                  errParseExpr,
+		},
 		"should miss if neither instant nor range query": {
 			url:                  "/series",
 			queryPriorityEnabled: true,
@@ -58,7 +69,11 @@ func Test_GetPriorityShouldReturnDefaultPriorityIfNotEnabledOrEmptyQueryString(t
 			limits.queryPriority.Enabled = testData.queryPriorityEnabled
 			req, _ := http.NewRequest(http.MethodPost, testData.url, bytes.NewReader([]byte{}))
 			priority, err := GetPriority(req, "", limits, now, 0)
-			assert.NoError(t, err)
+			if err != nil {
+				assert.Equal(t, testData.err, err)
+			} else {
+				assert.NoError(t, err)
+			}
 			assert.Equal(t, int64(0), priority)
 		})
 	}

diff --git a/pkg/querier/tripperware/roundtrip.go b/pkg/querier/tripperware/roundtrip.go
@@ -161,8 +161,10 @@ func NewQueryTripperware(
 
 					if limits != nil && limits.QueryPriority(userStr).Enabled {
 						priority, err := GetPriority(r, userStr, limits, now, lookbackDelta)
-						if err != nil {
-							return nil, err
+						if err != nil && err == errParseExpr {
+							// If query is invalid, no need to go through tripperwares
+							// for further splitting.
+							return next.RoundTrip(r)
 						}
 						r.Header.Set(util.QueryPriorityHeaderKey, strconv.FormatInt(priority, 10))
 					}