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 <vmartsynovskyy@gmail.com>

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
 }

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 {
+		t.Run(test.description, func(t *testing.T) {
 			err := validateLabelsAndMetricName(test.input)
-		if test.shouldPass != (err == nil) {
+			if err == nil {
+				if !test.shouldPass {
+					t.Fatalf("Test should fail, but passed instead.")
+				}
+			} else {
 				if test.shouldPass {
 					t.Fatalf("Test should pass, got unexpected error: %v", err)
-			} else {
-				t.Fatalf("Test should fail, unexpected error, got: %v, expected: %v", err, test.expectedErr)
+				} 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))
+}