validate label value (#9185)

* validate label value Signed-off-by: Mauro Stettler <[email protected]> * fix test Signed-off-by: Mauro Stettler <[email protected]> * add docs Signed-off-by: Mauro Stettler <[email protected]> * update CHANGELOG Signed-off-by: Mauro Stettler <[email protected]> * use writer request skip label validation flag Signed-off-by: Mauro Stettler <[email protected]> * add test with skipLabelValidation enabled Signed-off-by: Mauro Stettler <[email protected]> * do not rename header Signed-off-by: Mauro Stettler <[email protected]> --------- Signed-off-by: Mauro Stettler <[email protected]>
grafana · Oct 7, 2024 · 320ca98 · 320ca98
1 parent 59759c3
commit 320ca98
Show file tree

Hide file tree

Showing 18 changed files with 303 additions and 259 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -33,6 +33,7 @@
 * [ENHANCEMENT] Ingester: improve performance of reading the WAL. #9508
 * [ENHANCEMENT] Query-scheduler: improve the errors and traces emitted by query-schedulers when communicating with queriers. #9519
 * [ENHANCEMENT] Compactor: uploaded blocks cannot be bigger than max configured compactor time range, and cannot cross the boundary for given time range. #9524
+* [ENHANCEMENT] The distributor now validates that received label values only contain allowed characters. #9185
 * [BUGFIX] Fix issue where functions such as `rate()` over native histograms could return incorrect values if a float stale marker was present in the selected range. #9508
 * [BUGFIX] Fix issue where negation of native histograms (eg. `-some_native_histogram_series`) did nothing. #9508
 * [BUGFIX] Fix issue where `metric might not be a counter, name does not end in _total/_sum/_count/_bucket` annotation would be emitted even if `rate` or `increase` did not have enough samples to compute a result. #9508

diff --git a/docs/sources/mimir/manage/mimir-runbooks/_index.md b/docs/sources/mimir/manage/mimir-runbooks/_index.md
@@ -1745,6 +1745,15 @@ A label name name can only contain characters as defined by Prometheus’ [Metri
 Invalid series are skipped during the ingestion, and valid series within the same request are ingested.
 {{< /admonition >}}
 
+### err-mimir-label-value-invalid
+
+This non-critical error occurs when Mimir receives a write request that contains a series with a label that has an invalid value.
+A label value can only contain unicode characters as defined by Prometheus’ [Metric names and labels](https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels).
+
+{{< admonition type="note" >}}
+Invalid series are skipped during the ingestion, and valid series within the same request are ingested.
+{{< /admonition >}}
+
 ### err-mimir-label-name-too-long
 
 This non-critical error occurs when Mimir receives a write request that contains a series with a label name whose length exceeds the configured limit.

diff --git a/pkg/distributor/distributor.go b/pkg/distributor/distributor.go
@@ -198,9 +198,9 @@ type Config struct {
 	// for testing and for extending the ingester by adding calls to the client
 	IngesterClientFactory ring_client.PoolFactory `yaml:"-"`
 
-	// when true the distributor does not validate the label name, Mimir doesn't directly use
+	// when true the distributor does not validate the label name and value, Mimir doesn't directly use
 	// this (and should never use it) but this feature is used by other projects built on top of it
-	SkipLabelNameValidation bool `yaml:"-"`
+	SkipLabelValidation bool `yaml:"-"`
 
 	// This config is dynamically injected because it is defined in the querier config.
 	ShuffleShardingLookbackPeriod              time.Duration `yaml:"-"`
@@ -713,8 +713,8 @@ func (d *Distributor) checkSample(ctx context.Context, userID, cluster, replica
 // May alter timeseries data in-place.
 // The returned error may retain the series labels.
 // It uses the passed nowt time to observe the delay of sample timestamps.
-func (d *Distributor) validateSeries(nowt time.Time, ts *mimirpb.PreallocTimeseries, userID, group string, skipLabelNameValidation bool, minExemplarTS, maxExemplarTS int64) error {
-	if err := validateLabels(d.sampleValidationMetrics, d.limits, userID, group, ts.Labels, skipLabelNameValidation); err != nil {
+func (d *Distributor) validateSeries(nowt time.Time, ts *mimirpb.PreallocTimeseries, userID, group string, skipLabelValidation bool, minExemplarTS, maxExemplarTS int64) error {
+	if err := validateLabels(d.sampleValidationMetrics, d.limits, userID, group, ts.Labels, skipLabelValidation); err != nil {
 		return err
 	}
 
@@ -1050,9 +1050,9 @@ func (d *Distributor) prePushValidationMiddleware(next PushFunc) PushFunc {
 
 			d.labelsHistogram.Observe(float64(len(ts.Labels)))
 
-			skipLabelNameValidation := d.cfg.SkipLabelNameValidation || req.GetSkipLabelNameValidation()
+			skipLabelValidation := d.cfg.SkipLabelValidation || req.GetSkipLabelValidation()
 			// Note that validateSeries may drop some data in ts.
-			validationErr := d.validateSeries(now, &req.Timeseries[tsIdx], userID, group, skipLabelNameValidation, minExemplarTS, maxExemplarTS)
+			validationErr := d.validateSeries(now, &req.Timeseries[tsIdx], userID, group, skipLabelValidation, minExemplarTS, maxExemplarTS)
 
 			// Errors in validation are considered non-fatal, as one series in a request may contain
 			// invalid data but all the remaining series could be perfectly valid.

diff --git a/pkg/distributor/distributor_test.go b/pkg/distributor/distributor_test.go
@@ -1271,11 +1271,11 @@ func TestDistributor_Push_LabelNameValidation(t *testing.T) {
 				numDistributors:  1,
 				shuffleShardSize: 0,
 				configure: func(config *Config) {
-					config.SkipLabelNameValidation = tc.skipLabelNameValidationCfg
+					config.SkipLabelValidation = tc.skipLabelNameValidationCfg
 				},
 			})
 			req := mockWriteRequest(tc.inputLabels, 42, 100000)
-			req.SkipLabelNameValidation = tc.skipLabelNameValidationReq
+			req.SkipLabelValidation = tc.skipLabelNameValidationReq
 			_, err := ds[0].Push(ctx, req)
 			if tc.errExpected {
 				fromError, _ := grpcutil.ErrorToStatus(err)

diff --git a/pkg/distributor/otel_test.go b/pkg/distributor/otel_test.go
@@ -446,7 +446,7 @@ func TestHandler_otlpDroppedMetricsPanic(t *testing.T) {
 		request, err := pushReq.WriteRequest()
 		assert.NoError(t, err)
 		assert.Len(t, request.Timeseries, 3)
-		assert.False(t, request.SkipLabelNameValidation)
+		assert.False(t, request.SkipLabelValidation)
 		pushReq.CleanUp()
 		return nil
 	}, nil, nil, log.NewNopLogger(), true)
@@ -493,7 +493,7 @@ func TestHandler_otlpDroppedMetricsPanic2(t *testing.T) {
 		t.Cleanup(pushReq.CleanUp)
 		require.NoError(t, err)
 		assert.Len(t, request.Timeseries, 1)
-		assert.False(t, request.SkipLabelNameValidation)
+		assert.False(t, request.SkipLabelValidation)
 		return nil
 	}, nil, nil, log.NewNopLogger(), true)
 	handler.ServeHTTP(resp, req)
@@ -519,7 +519,7 @@ func TestHandler_otlpDroppedMetricsPanic2(t *testing.T) {
 		t.Cleanup(pushReq.CleanUp)
 		require.NoError(t, err)
 		assert.Len(t, request.Timeseries, 9) // 6 buckets (including +Inf) + 2 sum/count + 2 from the first case
-		assert.False(t, request.SkipLabelNameValidation)
+		assert.False(t, request.SkipLabelValidation)
 		return nil
 	}, nil, nil, log.NewNopLogger(), true)
 	handler.ServeHTTP(resp, req)

diff --git a/pkg/distributor/push.go b/pkg/distributor/push.go
@@ -172,9 +172,9 @@ func handler(
 			}
 
 			if allowSkipLabelNameValidation {
-				req.SkipLabelNameValidation = req.SkipLabelNameValidation && r.Header.Get(SkipLabelNameValidationHeader) == "true"
+				req.SkipLabelValidation = req.SkipLabelValidation && r.Header.Get(SkipLabelNameValidationHeader) == "true"
 			} else {
-				req.SkipLabelNameValidation = false
+				req.SkipLabelValidation = false
 			}
 
 			cleanup := func() {

diff --git a/pkg/distributor/push_test.go b/pkg/distributor/push_test.go
@@ -144,7 +144,7 @@ func TestHandler_EnsureSkipLabelNameValidationBehaviour(t *testing.T) {
 		expectedStatusCode                        int
 	}{
 		{
-			name:                         "config flag set to false means SkipLabelNameValidation is false",
+			name:                         "config flag set to false means SkipLabelValidation is false",
 			allowSkipLabelNameValidation: false,
 			req:                          createRequest(t, createMimirWriteRequestProtobufWithNonSupportedLabelNames(t, false)),
 			verifyReqHandler: func(_ context.Context, pushReq *Request) error {
@@ -154,15 +154,15 @@ func TestHandler_EnsureSkipLabelNameValidationBehaviour(t *testing.T) {
 				assert.Equal(t, "a-label", request.Timeseries[0].Labels[0].Name)
 				assert.Equal(t, "value", request.Timeseries[0].Labels[0].Value)
 				assert.Equal(t, mimirpb.RULE, request.Source)
-				assert.False(t, request.SkipLabelNameValidation)
+				assert.False(t, request.SkipLabelValidation)
 				pushReq.CleanUp()
 				return nil
 			},
 			includeAllowSkiplabelNameValidationHeader: true,
 			expectedStatusCode:                        http.StatusOK,
 		},
 		{
-			name:                         "config flag set to false means SkipLabelNameValidation is always false even if write requests sets it to true",
+			name:                         "config flag set to false means SkipLabelValidation is always false even if write requests sets it to true",
 			allowSkipLabelNameValidation: false,
 			req:                          createRequest(t, createMimirWriteRequestProtobufWithNonSupportedLabelNames(t, true)),
 			verifyReqHandler: func(_ context.Context, pushReq *Request) error {
@@ -173,14 +173,14 @@ func TestHandler_EnsureSkipLabelNameValidationBehaviour(t *testing.T) {
 				assert.Equal(t, "a-label", request.Timeseries[0].Labels[0].Name)
 				assert.Equal(t, "value", request.Timeseries[0].Labels[0].Value)
 				assert.Equal(t, mimirpb.RULE, request.Source)
-				assert.False(t, request.SkipLabelNameValidation)
+				assert.False(t, request.SkipLabelValidation)
 				return nil
 			},
 			includeAllowSkiplabelNameValidationHeader: true,
 			expectedStatusCode:                        http.StatusOK,
 		},
 		{
-			name:                         "config flag set to true but write request set to false means SkipLabelNameValidation is false",
+			name:                         "config flag set to true but write request set to false means SkipLabelValidation is false",
 			allowSkipLabelNameValidation: true,
 			req:                          createRequest(t, createMimirWriteRequestProtobufWithNonSupportedLabelNames(t, false)),
 			verifyReqHandler: func(_ context.Context, pushReq *Request) error {
@@ -190,14 +190,14 @@ func TestHandler_EnsureSkipLabelNameValidationBehaviour(t *testing.T) {
 				assert.Equal(t, "a-label", request.Timeseries[0].Labels[0].Name)
 				assert.Equal(t, "value", request.Timeseries[0].Labels[0].Value)
 				assert.Equal(t, mimirpb.RULE, request.Source)
-				assert.False(t, request.SkipLabelNameValidation)
+				assert.False(t, request.SkipLabelValidation)
 				pushReq.CleanUp()
 				return nil
 			},
 			expectedStatusCode: http.StatusOK,
 		},
 		{
-			name:                         "config flag set to true and write request set to true means SkipLabelNameValidation is true",
+			name:                         "config flag set to true and write request set to true means SkipLabelValidation is true",
 			allowSkipLabelNameValidation: true,
 			req:                          createRequest(t, createMimirWriteRequestProtobufWithNonSupportedLabelNames(t, true)),
 			verifyReqHandler: func(_ context.Context, pushReq *Request) error {
@@ -207,14 +207,14 @@ func TestHandler_EnsureSkipLabelNameValidationBehaviour(t *testing.T) {
 				assert.Equal(t, "a-label", request.Timeseries[0].Labels[0].Name)
 				assert.Equal(t, "value", request.Timeseries[0].Labels[0].Value)
 				assert.Equal(t, mimirpb.RULE, request.Source)
-				assert.True(t, request.SkipLabelNameValidation)
+				assert.True(t, request.SkipLabelValidation)
 				pushReq.CleanUp()
 				return nil
 			},
 			expectedStatusCode: http.StatusOK,
 		},
 		{
-			name:                         "config flag set to true and write request set to true but header not sent means SkipLabelNameValidation is false",
+			name:                         "config flag set to true and write request set to true but header not sent means SkipLabelValidation is false",
 			allowSkipLabelNameValidation: true,
 			req:                          createRequest(t, createMimirWriteRequestProtobufWithNonSupportedLabelNames(t, true)),
 			verifyReqHandler: func(_ context.Context, pushReq *Request) error {
@@ -224,7 +224,7 @@ func TestHandler_EnsureSkipLabelNameValidationBehaviour(t *testing.T) {
 				assert.Equal(t, "a-label", request.Timeseries[0].Labels[0].Name)
 				assert.Equal(t, "value", request.Timeseries[0].Labels[0].Value)
 				assert.Equal(t, mimirpb.RULE, request.Source)
-				assert.False(t, request.SkipLabelNameValidation)
+				assert.False(t, request.SkipLabelValidation)
 				pushReq.CleanUp()
 				return nil
 			},
@@ -390,7 +390,7 @@ func verifyWritePushFunc(t *testing.T, expectSource mimirpb.WriteRequest_SourceE
 		require.Equal(t, "__name__", request.Timeseries[0].Labels[0].Name)
 		require.Equal(t, "foo", request.Timeseries[0].Labels[0].Value)
 		require.Equal(t, expectSource, request.Source)
-		require.False(t, request.SkipLabelNameValidation)
+		require.False(t, request.SkipLabelValidation)
 		return nil
 	}
 }
@@ -456,9 +456,9 @@ func createMimirWriteRequestProtobuf(t *testing.T, skipLabelNameValidation bool)
 		},
 	}
 	input := mimirpb.WriteRequest{
-		Timeseries:              []mimirpb.PreallocTimeseries{ts},
-		Source:                  mimirpb.RULE,
-		SkipLabelNameValidation: skipLabelNameValidation,
+		Timeseries:          []mimirpb.PreallocTimeseries{ts},
+		Source:              mimirpb.RULE,
+		SkipLabelValidation: skipLabelNameValidation,
 	}
 	inoutBytes, err := input.Marshal()
 	require.NoError(t, err)
@@ -478,9 +478,9 @@ func createMimirWriteRequestProtobufWithNonSupportedLabelNames(t *testing.T, ski
 		},
 	}
 	input := mimirpb.WriteRequest{
-		Timeseries:              []mimirpb.PreallocTimeseries{ts},
-		Source:                  mimirpb.RULE,
-		SkipLabelNameValidation: skipLabelNameValidation,
+		Timeseries:          []mimirpb.PreallocTimeseries{ts},
+		Source:              mimirpb.RULE,
+		SkipLabelValidation: skipLabelNameValidation,
 	}
 	inoutBytes, err := input.Marshal()
 	require.NoError(t, err)

diff --git a/pkg/distributor/validate.go b/pkg/distributor/validate.go
@@ -34,6 +34,7 @@ var (
 	reasonInvalidMetricName            = globalerror.InvalidMetricName.LabelValue()
 	reasonMaxLabelNamesPerSeries       = globalerror.MaxLabelNamesPerSeries.LabelValue()
 	reasonInvalidLabel                 = globalerror.SeriesInvalidLabel.LabelValue()
+	reasonInvalidLabelValue            = globalerror.SeriesInvalidLabelValue.LabelValue()
 	reasonLabelNameTooLong             = globalerror.SeriesLabelNameTooLong.LabelValue()
 	reasonLabelValueTooLong            = globalerror.SeriesLabelValueTooLong.LabelValue()
 	reasonMaxNativeHistogramBuckets    = globalerror.MaxNativeHistogramBuckets.LabelValue()
@@ -70,9 +71,10 @@ var (
 		"received a series whose label value length exceeds the limit, label: '%s', value: '%.200s' (truncated) series: '%.200s'",
 		validation.MaxLabelValueLengthFlag,
 	)
-	invalidLabelMsgFormat   = globalerror.SeriesInvalidLabel.Message("received a series with an invalid label: '%.200s' series: '%.200s'")
-	duplicateLabelMsgFormat = globalerror.SeriesWithDuplicateLabelNames.Message("received a series with duplicate label name, label: '%.200s' series: '%.200s'")
-	tooManyLabelsMsgFormat  = globalerror.MaxLabelNamesPerSeries.MessageWithPerTenantLimitConfig(
+	invalidLabelMsgFormat      = globalerror.SeriesInvalidLabel.Message("received a series with an invalid label: '%.200s' series: '%.200s'")
+	invalidLabelValueMsgFormat = globalerror.SeriesInvalidLabelValue.Message("received a series with invalid value in label '%.200s': '%.200s' series: '%.200s'")
+	duplicateLabelMsgFormat    = globalerror.SeriesWithDuplicateLabelNames.Message("received a series with duplicate label name, label: '%.200s' series: '%.200s'")
+	tooManyLabelsMsgFormat     = globalerror.MaxLabelNamesPerSeries.MessageWithPerTenantLimitConfig(
 		"received a series whose number of labels exceeds the limit (actual: %d, limit: %d) series: '%.200s%s'",
 		validation.MaxLabelNamesPerSeriesFlag,
 	)
@@ -125,6 +127,7 @@ type sampleValidationMetrics struct {
 	invalidMetricName            *prometheus.CounterVec
 	maxLabelNamesPerSeries       *prometheus.CounterVec
 	invalidLabel                 *prometheus.CounterVec
+	invalidLabelValue            *prometheus.CounterVec
 	labelNameTooLong             *prometheus.CounterVec
 	labelValueTooLong            *prometheus.CounterVec
 	maxNativeHistogramBuckets    *prometheus.CounterVec
@@ -140,6 +143,7 @@ func (m *sampleValidationMetrics) deleteUserMetrics(userID string) {
 	m.invalidMetricName.DeletePartialMatch(filter)
 	m.maxLabelNamesPerSeries.DeletePartialMatch(filter)
 	m.invalidLabel.DeletePartialMatch(filter)
+	m.invalidLabelValue.DeletePartialMatch(filter)
 	m.labelNameTooLong.DeletePartialMatch(filter)
 	m.labelValueTooLong.DeletePartialMatch(filter)
 	m.maxNativeHistogramBuckets.DeletePartialMatch(filter)
@@ -154,6 +158,7 @@ func (m *sampleValidationMetrics) deleteUserMetricsForGroup(userID, group string
 	m.invalidMetricName.DeleteLabelValues(userID, group)
 	m.maxLabelNamesPerSeries.DeleteLabelValues(userID, group)
 	m.invalidLabel.DeleteLabelValues(userID, group)
+	m.invalidLabelValue.DeleteLabelValues(userID, group)
 	m.labelNameTooLong.DeleteLabelValues(userID, group)
 	m.labelValueTooLong.DeleteLabelValues(userID, group)
 	m.maxNativeHistogramBuckets.DeleteLabelValues(userID, group)
@@ -169,6 +174,7 @@ func newSampleValidationMetrics(r prometheus.Registerer) *sampleValidationMetric
 		invalidMetricName:            validation.DiscardedSamplesCounter(r, reasonInvalidMetricName),
 		maxLabelNamesPerSeries:       validation.DiscardedSamplesCounter(r, reasonMaxLabelNamesPerSeries),
 		invalidLabel:                 validation.DiscardedSamplesCounter(r, reasonInvalidLabel),
+		invalidLabelValue:            validation.DiscardedSamplesCounter(r, reasonInvalidLabelValue),
 		labelNameTooLong:             validation.DiscardedSamplesCounter(r, reasonLabelNameTooLong),
 		labelValueTooLong:            validation.DiscardedSamplesCounter(r, reasonLabelValueTooLong),
 		maxNativeHistogramBuckets:    validation.DiscardedSamplesCounter(r, reasonMaxNativeHistogramBuckets),
@@ -368,7 +374,7 @@ func removeNonASCIIChars(in string) (out string) {
 
 // validateLabels returns an err if the labels are invalid.
 // The returned error may retain the provided series labels.
-func validateLabels(m *sampleValidationMetrics, cfg labelValidationConfig, userID, group string, ls []mimirpb.LabelAdapter, skipLabelNameValidation bool) error {
+func validateLabels(m *sampleValidationMetrics, cfg labelValidationConfig, userID, group string, ls []mimirpb.LabelAdapter, skipLabelValidation bool) error {
 	unsafeMetricName, err := extract.UnsafeMetricNameFromLabelAdapters(ls)
 	if err != nil {
 		m.missingMetricName.WithLabelValues(userID, group).Inc()
@@ -391,12 +397,15 @@ func validateLabels(m *sampleValidationMetrics, cfg labelValidationConfig, userI
 	maxLabelValueLength := cfg.MaxLabelValueLength(userID)
 	lastLabelName := ""
 	for _, l := range ls {
-		if !skipLabelNameValidation && !model.LabelName(l.Name).IsValid() {
+		if !skipLabelValidation && !model.LabelName(l.Name).IsValid() {
 			m.invalidLabel.WithLabelValues(userID, group).Inc()
 			return fmt.Errorf(invalidLabelMsgFormat, l.Name, mimirpb.FromLabelAdaptersToString(ls))
 		} else if len(l.Name) > maxLabelNameLength {
 			m.labelNameTooLong.WithLabelValues(userID, group).Inc()
 			return fmt.Errorf(labelNameTooLongMsgFormat, l.Name, mimirpb.FromLabelAdaptersToString(ls))
+		} else if !skipLabelValidation && !model.LabelValue(l.Value).IsValid() {
+			m.invalidLabelValue.WithLabelValues(userID, group).Inc()
+			return fmt.Errorf(invalidLabelValueMsgFormat, l.Name, l.Value, mimirpb.FromLabelAdaptersToString(ls))
 		} else if len(l.Value) > maxLabelValueLength {
 			m.labelValueTooLong.WithLabelValues(userID, group).Inc()
 			return fmt.Errorf(labelValueTooLongMsgFormat, l.Name, l.Value, mimirpb.FromLabelAdaptersToString(ls))