From 12e5c26e1b5676fb6e348909832e1a907c37df47 Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Thu, 3 Dec 2020 11:29:20 -0500 Subject: [PATCH] fix index out of bound bug when comparing ZLabelSets (#3520) * fix index out of bound bug when comparing ZLabelSets Signed-off-by: Ben Ye * fix param parsing error message Signed-off-by: Ben Ye * address comment feedbacks Signed-off-by: Ben Ye --- pkg/queryfrontend/labels_codec.go | 8 ++-- pkg/queryfrontend/queryrange_codec.go | 10 ++--- pkg/store/labelpb/label.go | 5 ++- pkg/store/labelpb/label_test.go | 57 +++++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 11 deletions(-) diff --git a/pkg/queryfrontend/labels_codec.go b/pkg/queryfrontend/labels_codec.go index 246a8592bc..d096fe48e2 100644 --- a/pkg/queryfrontend/labels_codec.go +++ b/pkg/queryfrontend/labels_codec.go @@ -84,7 +84,7 @@ func (c labelsCodec) MergeResponse(responses ...queryrange.Response) (queryrange Data: lbls, }, nil case *ThanosSeriesResponse: - seriesData := make([]labelpb.ZLabelSet, 0) + seriesData := make(labelpb.ZLabelSets, 0) uniqueSeries := make(map[string]struct{}) for _, res := range responses { @@ -283,7 +283,7 @@ func (c labelsCodec) parseLabelsRequest(r *http.Request, op string) (queryrange. return nil, err } - result.StoreMatchers, err = parseMatchersParam(r.Form[queryv1.StoreMatcherParam]) + result.StoreMatchers, err = parseMatchersParam(r.Form, queryv1.StoreMatcherParam) if err != nil { return nil, err } @@ -317,7 +317,7 @@ func (c labelsCodec) parseSeriesRequest(r *http.Request) (queryrange.Request, er return nil, err } - result.Matchers, err = parseMatchersParam(r.Form[queryv1.MatcherParam]) + result.Matchers, err = parseMatchersParam(r.Form, queryv1.MatcherParam) if err != nil { return nil, err } @@ -336,7 +336,7 @@ func (c labelsCodec) parseSeriesRequest(r *http.Request) (queryrange.Request, er result.ReplicaLabels = r.Form[queryv1.ReplicaLabelsParam] } - result.StoreMatchers, err = parseMatchersParam(r.Form[queryv1.StoreMatcherParam]) + result.StoreMatchers, err = parseMatchersParam(r.Form, queryv1.StoreMatcherParam) if err != nil { return nil, err } diff --git a/pkg/queryfrontend/queryrange_codec.go b/pkg/queryfrontend/queryrange_codec.go index 004fb522d5..3206c28be8 100644 --- a/pkg/queryfrontend/queryrange_codec.go +++ b/pkg/queryfrontend/queryrange_codec.go @@ -111,7 +111,7 @@ func (c queryRangeCodec) DecodeRequest(_ context.Context, r *http.Request) (quer result.ReplicaLabels = r.Form[queryv1.ReplicaLabelsParam] } - result.StoreMatchers, err = parseMatchersParam(r.Form[queryv1.StoreMatcherParam]) + result.StoreMatchers, err = parseMatchersParam(r.Form, queryv1.StoreMatcherParam) if err != nil { return nil, err } @@ -221,12 +221,12 @@ func parsePartialResponseParam(s string, defaultEnablePartialResponse bool) (boo return defaultEnablePartialResponse, nil } -func parseMatchersParam(ss []string) ([][]*labels.Matcher, error) { - matchers := make([][]*labels.Matcher, 0, len(ss)) - for _, s := range ss { +func parseMatchersParam(ss url.Values, matcherParam string) ([][]*labels.Matcher, error) { + matchers := make([][]*labels.Matcher, 0, len(ss[matcherParam])) + for _, s := range ss[matcherParam] { ms, err := parser.ParseMetricSelector(s) if err != nil { - return nil, httpgrpc.Errorf(http.StatusBadRequest, errCannotParse, queryv1.StoreMatcherParam) + return nil, httpgrpc.Errorf(http.StatusBadRequest, errCannotParse, matcherParam) } matchers = append(matchers, ms) } diff --git a/pkg/store/labelpb/label.go b/pkg/store/labelpb/label.go index 36c5bfc139..ea0a7fa49b 100644 --- a/pkg/store/labelpb/label.go +++ b/pkg/store/labelpb/label.go @@ -307,7 +307,8 @@ func (z ZLabelSets) Less(i, j int) bool { l := 0 r := 0 var result int - for l < z[i].Size() && r < z[j].Size() { + lenI, lenJ := len(z[i].Labels), len(z[j].Labels) + for l < lenI && r < lenJ { result = z[i].Labels[l].Compare(z[j].Labels[r]) if result == 0 { l++ @@ -317,5 +318,5 @@ func (z ZLabelSets) Less(i, j int) bool { return result < 0 } - return l == z[i].Size() + return l == lenI } diff --git a/pkg/store/labelpb/label_test.go b/pkg/store/labelpb/label_test.go index ab8543657b..d8d258e8e1 100644 --- a/pkg/store/labelpb/label_test.go +++ b/pkg/store/labelpb/label_test.go @@ -129,6 +129,34 @@ func TestSortZLabelSets(t *testing.T) { }), ), }, + { + Labels: ZLabelsFromPromLabels( + labels.FromMap(map[string]string{ + "__name__": "grpc_client_handled_total", + "cluster": "test", + "grpc_code": "OK", + "aa": "1", + "bb": "2", + "cc": "3", + "dd": "4", + "ee": "5", + }), + ), + }, + { + Labels: ZLabelsFromPromLabels( + labels.FromMap(map[string]string{ + "__name__": "grpc_client_handled_total", + "cluster": "test", + "grpc_code": "OK", + "aa": "1", + "bb": "2", + "cc": "3", + "dd": "4", + "ee": "5", + }), + ), + }, { Labels: ZLabelsFromPromLabels( labels.FromMap(map[string]string{ @@ -188,6 +216,35 @@ func TestSortZLabelSets(t *testing.T) { }), ), }, + { + Labels: ZLabelsFromPromLabels( + labels.FromMap(map[string]string{ + "__name__": "grpc_client_handled_total", + "cluster": "test", + "grpc_code": "OK", + "aa": "1", + "bb": "2", + "cc": "3", + "dd": "4", + "ee": "5", + }), + ), + }, + // This label set is the same as the previous one, which should correctly return 0 in Less() function. + { + Labels: ZLabelsFromPromLabels( + labels.FromMap(map[string]string{ + "cluster": "test", + "__name__": "grpc_client_handled_total", + "grpc_code": "OK", + "aa": "1", + "bb": "2", + "cc": "3", + "dd": "4", + "ee": "5", + }), + ), + }, } sort.Sort(list)