Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bwplotka merged 1 commit into from
Aug 20, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion expfmt/bench_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand Down
41 changes: 32 additions & 9 deletions expfmt/decode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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() {
Expand All @@ -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 {
Expand Down
11 changes: 6 additions & 5 deletions expfmt/decode_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand Down
90 changes: 90 additions & 0 deletions expfmt/encode_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}
27 changes: 27 additions & 0 deletions expfmt/text_parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Copy link
Copy Markdown
Member Author

@ywwg ywwg Aug 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fact that this member was not reset does not bode well for the correctness of the whole parser

Copy link
Copy Markdown
Member

@bwplotka bwplotka Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It also proves it's not heavily used (:

if p.buf == nil {
p.buf = bufio.NewReader(in)
} else {
Expand Down Expand Up @@ -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.
}
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
}
Expand All @@ -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()
}
Expand All @@ -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()) {
Copy link
Copy Markdown
Member Author

@ywwg ywwg Aug 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these checks were not done at all previously

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'
Expand Down Expand Up @@ -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
}
Expand Down
Loading
Loading