From 8318aa2d5d65e471c67f26097617711118e72a55 Mon Sep 17 00:00:00 2001 From: Vadym Martsynovskyy Date: Wed, 7 Aug 2019 08:13:10 -0700 Subject: [PATCH] Check for duplicate label names in remote read (#5829) * Check for duplicate label names in remote read Also add test to confirm that #5731 is fixed * Use subtests in TestValidateLabelsAndMetricName * Really check that expectedErr matches err Signed-off-by: Vadym Martsynovskyy --- storage/remote/codec.go | 8 +++- storage/remote/codec_test.go | 91 +++++++++++++++++++++++++++++++----- 2 files changed, 86 insertions(+), 13 deletions(-) diff --git a/storage/remote/codec.go b/storage/remote/codec.go index 283f9aeb8..dc4ee2809 100644 --- a/storage/remote/codec.go +++ b/storage/remote/codec.go @@ -277,9 +277,10 @@ func (c *concreteSeriesIterator) Err() error { return nil } -// validateLabelsAndMetricName validates the label names/values and metric names returned from remote read. +// validateLabelsAndMetricName validates the label names/values and metric names returned from remote read, +// also making sure that there are no labels with duplicate names func validateLabelsAndMetricName(ls labels.Labels) error { - for _, l := range ls { + for i, l := range ls { if l.Name == labels.MetricName && !model.IsValidMetricName(model.LabelValue(l.Value)) { return errors.Errorf("invalid metric name: %v", l.Value) } @@ -289,6 +290,9 @@ func validateLabelsAndMetricName(ls labels.Labels) error { if !model.LabelValue(l.Value).IsValid() { return errors.Errorf("invalid label value: %v", l.Value) } + if i > 0 && l.Name == ls[i-1].Name { + return errors.Errorf("duplicate label with name: %v", l.Name) + } } return nil } diff --git a/storage/remote/codec_test.go b/storage/remote/codec_test.go index b875065a2..ebc1ec8ea 100644 --- a/storage/remote/codec_test.go +++ b/storage/remote/codec_test.go @@ -14,6 +14,7 @@ package remote import ( + "fmt" "testing" "github.com/stretchr/testify/require" @@ -21,6 +22,7 @@ import ( "github.com/prometheus/prometheus/pkg/labels" "github.com/prometheus/prometheus/prompb" "github.com/prometheus/prometheus/storage" + "github.com/prometheus/prometheus/util/testutil" ) func TestValidateLabelsAndMetricName(t *testing.T) { @@ -28,6 +30,7 @@ func TestValidateLabelsAndMetricName(t *testing.T) { input labels.Labels expectedErr string shouldPass bool + description string }{ { input: labels.FromStrings( @@ -36,6 +39,7 @@ func TestValidateLabelsAndMetricName(t *testing.T) { ), expectedErr: "", shouldPass: true, + description: "regular labels", }, { input: labels.FromStrings( @@ -44,57 +48,96 @@ func TestValidateLabelsAndMetricName(t *testing.T) { ), expectedErr: "", shouldPass: true, + description: "label name with _", }, { input: labels.FromStrings( "__name__", "name", "@labelName", "labelValue", ), - expectedErr: "Invalid label name: @labelName", + expectedErr: "invalid label name: @labelName", shouldPass: false, + description: "label name with @", }, { input: labels.FromStrings( "__name__", "name", "123labelName", "labelValue", ), - expectedErr: "Invalid label name: 123labelName", + expectedErr: "invalid label name: 123labelName", shouldPass: false, + description: "label name starts with numbers", }, { input: labels.FromStrings( "__name__", "name", "", "labelValue", ), - expectedErr: "Invalid label name: ", + expectedErr: "invalid label name: ", shouldPass: false, + description: "label name is empty string", }, { input: labels.FromStrings( "__name__", "name", "labelName", string([]byte{0xff}), ), - expectedErr: "Invalid label value: " + string([]byte{0xff}), + expectedErr: "invalid label value: " + string([]byte{0xff}), shouldPass: false, + description: "label value is an invalid UTF-8 value", }, { input: labels.FromStrings( "__name__", "@invalid_name", ), - expectedErr: "Invalid metric name: @invalid_name", + expectedErr: "invalid metric name: @invalid_name", shouldPass: false, + description: "metric name starts with @", + }, + { + input: labels.FromStrings( + "__name__", "name1", + "__name__", "name2", + ), + expectedErr: "duplicate label with name: __name__", + shouldPass: false, + description: "duplicate label names", + }, + { + input: labels.FromStrings( + "label1", "name", + "label2", "name", + ), + expectedErr: "", + shouldPass: true, + description: "duplicate label values", + }, + { + input: labels.FromStrings( + "", "name", + "label2", "name", + ), + expectedErr: "invalid label name: ", + shouldPass: false, + description: "don't report as duplicate label name", }, } for _, test := range tests { - err := validateLabelsAndMetricName(test.input) - if test.shouldPass != (err == nil) { - if test.shouldPass { - t.Fatalf("Test should pass, got unexpected error: %v", err) + t.Run(test.description, func(t *testing.T) { + err := validateLabelsAndMetricName(test.input) + if err == nil { + if !test.shouldPass { + t.Fatalf("Test should fail, but passed instead.") + } } else { - t.Fatalf("Test should fail, unexpected error, got: %v, expected: %v", err, test.expectedErr) + if test.shouldPass { + t.Fatalf("Test should pass, got unexpected error: %v", err) + } else if err.Error() != test.expectedErr { + t.Fatalf("Test should fail with: %s got unexpected error instead: %v", test.expectedErr, err) + } } - } + }) } } @@ -145,3 +188,29 @@ func TestConcreteSeriesClonesLabels(t *testing.T) { gotLabels = cs.Labels() require.Equal(t, lbls, gotLabels) } + +func TestFromQueryResultWithDuplicates(t *testing.T) { + ts1 := prompb.TimeSeries{ + Labels: []prompb.Label{ + prompb.Label{Name: "foo", Value: "bar"}, + prompb.Label{Name: "foo", Value: "def"}, + }, + Samples: []prompb.Sample{ + prompb.Sample{Value: 0.0, Timestamp: 0}, + }, + } + + res := prompb.QueryResult{ + Timeseries: []*prompb.TimeSeries{ + &ts1, + }, + } + + series := FromQueryResult(&res) + + errSeries, isErrSeriesSet := series.(errSeriesSet) + + testutil.Assert(t, isErrSeriesSet, "Expected resulting series to be an errSeriesSet") + errMessage := errSeries.Err().Error() + testutil.Assert(t, errMessage == "duplicate label with name: foo", fmt.Sprintf("Expected error to be from duplicate label, but got: %s", errMessage)) +}