Decoder: Remove use of global name validation and add validation (#808)

This change removes the use of the global variable to determine name validation, using the provided format instead.

Also:

* adds validity checks to the text parser, which previously did not check for name validity.
* Returns error if the caller tries to create a decoder for prototext format, which is not supported.

Signed-off-by: Owen Williams <[email protected]>

Author: ywwg

expfmt/bench_test.go

@@ -26,9 +26,11 @@ import (
 
 	dto "github.com/prometheus/client_model/go"
 	"github.com/stretchr/testify/require"
+
+	"github.com/prometheus/common/model"
 )
 
-var parser TextParser
+var parser = TextParser{scheme: model.UTF8Validation}
 
 // Benchmarks to show how much penalty text format parsing actually inflicts.
 //

expfmt/decode.go

@@ -70,19 +70,34 @@ func ResponseFormat(h http.Header) Format {
 	return FmtUnknown
 }
 
-// NewDecoder returns a new decoder based on the given input format.
-// If the input format does not imply otherwise, a text format decoder is returned.
+// NewDecoder returns a new decoder based on the given input format. Metric
+// names are validated based on the provided Format -- if the format requires
+// escaping, raditional Prometheues validity checking is used. Otherwise, names
+// are checked for UTF-8 validity. Supported formats include delimited protobuf
+// and Prometheus text format.  For historical reasons, this decoder fallbacks
+// to classic text decoding for any other format. This decoder does not fully
+// support OpenMetrics although it may often succeed due to the similarities
+// between the formats. This decoder may not support the latest features of
+// Prometheus text format and is not intended for high-performance applications.
+// See: https://github.com/prometheus/common/issues/812
 func NewDecoder(r io.Reader, format Format) Decoder {
+	scheme := model.LegacyValidation
+	if format.ToEscapingScheme() == model.NoEscaping {
+		scheme = model.UTF8Validation
+	}
 	switch format.FormatType() {
 	case TypeProtoDelim:
-		return &protoDecoder{r: bufio.NewReader(r)}
+		return &protoDecoder{r: bufio.NewReader(r), s: scheme}
+	case TypeProtoText, TypeProtoCompact:
+		return &errDecoder{err: fmt.Errorf("format %s not supported for decoding", format)}
 	}
-	return &textDecoder{r: r}
+	return &textDecoder{r: r, s: scheme}
 }
 
 // protoDecoder implements the Decoder interface for protocol buffers.
 type protoDecoder struct {
 	r protodelim.Reader
+	s model.ValidationScheme
 }
 
 // Decode implements the Decoder interface.
@@ -93,8 +108,7 @@ func (d *protoDecoder) Decode(v *dto.MetricFamily) error {
 	if err := opts.UnmarshalFrom(d.r, v); err != nil {
 		return err
 	}
-	//nolint:staticcheck // model.IsValidMetricName is deprecated.
-	if !model.IsValidMetricName(model.LabelValue(v.GetName())) {
+	if !d.s.IsValidMetricName(v.GetName()) {
 		return fmt.Errorf("invalid metric name %q", v.GetName())
 	}
 	for _, m := range v.GetMetric() {
@@ -108,27 +122,36 @@ func (d *protoDecoder) Decode(v *dto.MetricFamily) error {
 			if !model.LabelValue(l.GetValue()).IsValid() {
 				return fmt.Errorf("invalid label value %q", l.GetValue())
 			}
-			//nolint:staticcheck // model.LabelName.IsValid is deprecated.
-			if !model.LabelName(l.GetName()).IsValid() {
+			if !d.s.IsValidLabelName(l.GetName()) {
 				return fmt.Errorf("invalid label name %q", l.GetName())
 			}
 		}
 	}
 	return nil
 }
 
+// errDecoder is an error-state decoder that always returns the same error.
+type errDecoder struct {
+	err error
+}
+
+func (d *errDecoder) Decode(v *dto.MetricFamily) error {
+	return d.err
+}
+
 // textDecoder implements the Decoder interface for the text protocol.
 type textDecoder struct {
 	r    io.Reader
 	fams map[string]*dto.MetricFamily
+	s    model.ValidationScheme
 	err  error
 }
 
 // Decode implements the Decoder interface.
 func (d *textDecoder) Decode(v *dto.MetricFamily) error {
 	if d.err == nil {
 		// Read all metrics in one shot.
-		var p TextParser
+		p := TextParser{scheme: d.s}
 		d.fams, d.err = p.TextToMetricFamilies(d.r)
 		// If we don't get an error, store io.EOF for the end.
 		if d.err == nil {

expfmt/decode_test.go

@@ -80,7 +80,10 @@ mf2 4
 	)
 
 	dec := &SampleDecoder{
-		Dec: &textDecoder{r: strings.NewReader(in)},
+		Dec: &textDecoder{
+			s: model.UTF8Validation,
+			r: strings.NewReader(in),
+		},
 		Opts: &DecodeOptions{
 			Timestamp: ts,
 		},
@@ -361,25 +364,23 @@ func TestProtoDecoder(t *testing.T) {
 
 	for i, scenario := range scenarios {
 		dec := &SampleDecoder{
-			Dec: &protoDecoder{r: strings.NewReader(scenario.in)},
+			Dec: &protoDecoder{r: strings.NewReader(scenario.in), s: model.LegacyValidation},
 			Opts: &DecodeOptions{
 				Timestamp: testTime,
 			},
 		}
 
 		var all model.Vector
 		for {
-			model.NameValidationScheme = model.LegacyValidation //nolint:staticcheck
 			var smpls model.Vector
 			err := dec.Decode(&smpls)
 			if err != nil && errors.Is(err, io.EOF) {
 				break
 			}
 			if scenario.legacyNameFail {
 				require.Errorf(t, err, "Expected error when decoding without UTF-8 support enabled but got none")
-				model.NameValidationScheme = model.UTF8Validation //nolint:staticcheck
 				dec = &SampleDecoder{
-					Dec: &protoDecoder{r: strings.NewReader(scenario.in)},
+					Dec: &protoDecoder{r: strings.NewReader(scenario.in), s: model.UTF8Validation},
 					Opts: &DecodeOptions{
 						Timestamp: testTime,
 					},

expfmt/encode_test.go

@@ -376,3 +376,93 @@ func TestEscapedEncode(t *testing.T) {
 		})
 	}
 }
+
+func TestDottedEncode(t *testing.T) {
+	//nolint:staticcheck
+	model.NameValidationScheme = model.UTF8Validation
+	metric := &dto.MetricFamily{
+		Name: proto.String("foo.metric"),
+		Type: dto.MetricType_COUNTER.Enum(),
+		Metric: []*dto.Metric{
+			{
+				Counter: &dto.Counter{
+					Value: proto.Float64(1.234),
+				},
+			},
+			{
+				Label: []*dto.LabelPair{
+					{
+						Name:  proto.String("dotted.label.name"),
+						Value: proto.String("my.label.value"),
+					},
+				},
+				Counter: &dto.Counter{
+					Value: proto.Float64(8),
+				},
+			},
+		},
+	}
+
+	scenarios := []struct {
+		format           Format
+		expectMetricName string
+		expectLabelName  string
+	}{
+		{
+			format:           FmtProtoDelim,
+			expectMetricName: "foo_metric",
+			expectLabelName:  "dotted_label_name",
+		},
+		{
+			format:           FmtProtoDelim.WithEscapingScheme(model.NoEscaping),
+			expectMetricName: "foo.metric",
+			expectLabelName:  "dotted.label.name",
+		},
+		{
+			format:           FmtProtoDelim.WithEscapingScheme(model.DotsEscaping),
+			expectMetricName: "foo_dot_metric",
+			expectLabelName:  "dotted_dot_label_dot_name",
+		},
+		{
+			format:           FmtText,
+			expectMetricName: "foo_metric",
+			expectLabelName:  "dotted_label_name",
+		},
+		{
+			format:           FmtText.WithEscapingScheme(model.NoEscaping),
+			expectMetricName: "foo.metric",
+			expectLabelName:  "dotted.label.name",
+		},
+		{
+			format:           FmtText.WithEscapingScheme(model.DotsEscaping),
+			expectMetricName: "foo_dot_metric",
+			expectLabelName:  "dotted_dot_label_dot_name",
+		},
+		// common library does not support proto text or open metrics parsing so we
+		// do not test those here.
+	}
+
+	for i, scenario := range scenarios {
+		out := bytes.NewBuffer(make([]byte, 0))
+		enc := NewEncoder(out, scenario.format)
+		err := enc.Encode(metric)
+		if err != nil {
+			t.Errorf("%d. error: %s", i, err)
+			continue
+		}
+
+		dec := NewDecoder(bytes.NewReader(out.Bytes()), scenario.format)
+		var gotFamily dto.MetricFamily
+		err = dec.Decode(&gotFamily)
+		if err != nil {
+			t.Errorf("%v: unexpected error during decode: %s", scenario.format, err.Error())
+		}
+		if gotFamily.GetName() != scenario.expectMetricName {
+			t.Errorf("%v: incorrect encoded metric name, want %v, got %v", scenario.format, scenario.expectMetricName, gotFamily.GetName())
+		}
+		lName := gotFamily.GetMetric()[1].Label[0].GetName()
+		if lName != scenario.expectLabelName {
+			t.Errorf("%v: incorrect encoded label name, want %v, got %v", scenario.format, scenario.expectLabelName, lName)
+		}
+	}
+}

expfmt/text_parse.go

@@ -78,6 +78,9 @@ type TextParser struct {
 	// These indicate if the metric name from the current line being parsed is inside
 	// braces and if that metric name was found respectively.
 	currentMetricIsInsideBraces, currentMetricInsideBracesIsPresent bool
+	// scheme sets the desired ValidationScheme for names. Defaults to the invalid
+	// UnsetValidation.
+	scheme model.ValidationScheme
 }
 
 // TextToMetricFamilies reads 'in' as the simple and flat text-based exchange
@@ -126,6 +129,7 @@ func (p *TextParser) TextToMetricFamilies(in io.Reader) (map[string]*dto.MetricF
 
 func (p *TextParser) reset(in io.Reader) {
 	p.metricFamiliesByName = map[string]*dto.MetricFamily{}
+	p.currentLabelPairs = nil
 	if p.buf == nil {
 		p.buf = bufio.NewReader(in)
 	} else {
@@ -216,6 +220,9 @@ func (p *TextParser) startComment() stateFn {
 		return nil
 	}
 	p.setOrCreateCurrentMF()
+	if p.err != nil {
+		return nil
+	}
 	if p.skipBlankTab(); p.err != nil {
 		return nil // Unexpected end of input.
 	}
@@ -244,6 +251,9 @@ func (p *TextParser) readingMetricName() stateFn {
 		return nil
 	}
 	p.setOrCreateCurrentMF()
+	if p.err != nil {
+		return nil
+	}
 	// Now is the time to fix the type if it hasn't happened yet.
 	if p.currentMF.Type == nil {
 		p.currentMF.Type = dto.MetricType_UNTYPED.Enum()
@@ -311,6 +321,9 @@ func (p *TextParser) startLabelName() stateFn {
 			switch p.currentByte {
 			case ',':
 				p.setOrCreateCurrentMF()
+				if p.err != nil {
+					return nil
+				}
 				if p.currentMF.Type == nil {
 					p.currentMF.Type = dto.MetricType_UNTYPED.Enum()
 				}
@@ -319,6 +332,10 @@ func (p *TextParser) startLabelName() stateFn {
 				return p.startLabelName
 			case '}':
 				p.setOrCreateCurrentMF()
+				if p.err != nil {
+					p.currentLabelPairs = nil
+					return nil
+				}
 				if p.currentMF.Type == nil {
 					p.currentMF.Type = dto.MetricType_UNTYPED.Enum()
 				}
@@ -341,6 +358,12 @@ func (p *TextParser) startLabelName() stateFn {
 	p.currentLabelPair = &dto.LabelPair{Name: proto.String(p.currentToken.String())}
 	if p.currentLabelPair.GetName() == string(model.MetricNameLabel) {
 		p.parseError(fmt.Sprintf("label name %q is reserved", model.MetricNameLabel))
+		p.currentLabelPairs = nil
+		return nil
+	}
+	if !p.scheme.IsValidLabelName(p.currentLabelPair.GetName()) {
+		p.parseError(fmt.Sprintf("invalid label name %q", p.currentLabelPair.GetName()))
+		p.currentLabelPairs = nil
 		return nil
 	}
 	// Special summary/histogram treatment. Don't add 'quantile' and 'le'
@@ -805,6 +828,10 @@ func (p *TextParser) setOrCreateCurrentMF() {
 	p.currentIsHistogramCount = false
 	p.currentIsHistogramSum = false
 	name := p.currentToken.String()
+	if !p.scheme.IsValidMetricName(name) {
+		p.parseError(fmt.Sprintf("invalid metric name %q", name))
+		return
+	}
 	if p.currentMF = p.metricFamiliesByName[name]; p.currentMF != nil {
 		return
 	}