From 616175b1ce32e43366799b6747e76883d0ec59e7 Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Fri, 12 May 2023 16:52:09 -0700 Subject: [PATCH] lib/promutils: properly return error when incorrect Prometheus label names are passed to NewLabelsFromString() Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4284 See also https://github.com/VictoriaMetrics/VictoriaMetrics/pull/4304 --- docs/CHANGELOG.md | 1 + lib/promutils/labels_test.go | 49 +++++++++++++++++++++++ lib/protoparser/prometheus/parser.go | 3 ++ lib/protoparser/prometheus/parser_test.go | 5 +++ 4 files changed, 58 insertions(+) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index ab00f4685..73a166e66 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -61,6 +61,7 @@ The following tip changes can be tested by building VictoriaMetrics components f * BUGFIX: change the max allowed value for `-memory.allowedPercent` from 100 to 200. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4171). * BUGFIX: properly limit the number of [OpenTSDB HTTP](https://docs.victoriametrics.com/#sending-opentsdb-data-via-http-apiput-requests) concurrent requests specified via `-maxConcurrentInserts` command-line flag. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4204). Thanks to @zouxiang1993 for [the fix](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/4208). * BUGFIX: do not ignore trailing empty field in CSV lines when [importing data in CSV format](https://docs.victoriametrics.com/#how-to-import-csv-data). See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4048). +* BUGFIX: disallow `"` chars when parsing Prometheus label names, since they aren't allowed by [Prometheus text exposition format](https://github.com/prometheus/docs/blob/main/content/docs/instrumenting/exposition_formats.md#text-format-example). Previously this could result in silent incorrect parsing of incorrect Prometheus labels such as `foo{"bar"="baz"}` or `{foo:"bar",baz="aaa"}`. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4284). * BUGFIX: [VictoriaMetrics cluster](https://docs.victoriametrics.com/Cluster-VictoriaMetrics.html): prevent from possible panic when the number of vmstorage nodes increases when [automatic vmstorage discovery](https://docs.victoriametrics.com/Cluster-VictoriaMetrics.html#automatic-vmstorage-discovery) is enabled. * BUGFIX: [MetricsQL](https://docs.victoriametrics.com/MetricsQL.html): fix a panic when the duration in the query contains uppercase `M` suffix. Such a suffix isn't allowed to use in durations, since it clashes with `a million` suffix, e.g. it isn't clear whether `rate(metric[5M])` means rate over 5 minutes, 5 months or 5 million seconds. See [this](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3589) and [this](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4120) issues. * BUGFIX: [vmagent](https://docs.victoriametrics.com/vmagent.html): properly handle the `vm_promscrape_config_last_reload_successful` metric after config reload. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4260). diff --git a/lib/promutils/labels_test.go b/lib/promutils/labels_test.go index 18bdfee4f..7d1cc8ff9 100644 --- a/lib/promutils/labels_test.go +++ b/lib/promutils/labels_test.go @@ -194,3 +194,52 @@ func TestLabels_Set(t *testing.T) { f(`http_request_total{a="b"}`, `a`, `c`, `{__name__="http_request_total",a="c"}`) f(`http_request_total{a="b"}`, `ip`, `127.0.0.1`, `{__name__="http_request_total",a="b",ip="127.0.0.1"}`) } + +func TestNewLabelsFromStringSuccess(t *testing.T) { + f := func(s, resultExpected string) { + t.Helper() + labels, err := NewLabelsFromString(s) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + result := labels.String() + if result != resultExpected { + t.Fatalf("unexpected result;\ngot\n%s\nwant\n%s", result, resultExpected) + } + } + + f("{}", "{}") + f("foo", `{__name__="foo"}`) + f(`foo{bar="baz"}`, `{__name__="foo",bar="baz"}`) + f(`foo {bar="baz", a="b"}`, `{__name__="foo",bar="baz",a="b"}`) + f(`{foo="bar", baz="a"}`, `{foo="bar",baz="a"}`) + f(`{__name__="aaa"}`, `{__name__="aaa"}`) + f(`{__name__="abc",de="fg"}`, `{__name__="abc",de="fg"}`) +} + +func TestNewLabelsFromStringFailure(t *testing.T) { + f := func(s string) { + t.Helper() + labels, err := NewLabelsFromString(s) + if labels != nil { + t.Fatalf("unexpected non-nil labels: %s", labels) + } + if err == nil { + t.Fatalf("expecting non-nil error") + } + } + + f("") + f("foo bar") + f(`foo{`) + f(`foo{bar`) + f(`foo{bar=`) + f(`foo{bar="`) + f(`foo{bar="baz`) + f(`foo{bar="baz"`) + f(`foo{bar="baz",`) + f(`foo{"bar"="baz"}`) + f(`{"bar":"baz"}`) + f(`{bar:"baz"}`) + f(`{bar=~"baz"}`) +} diff --git a/lib/protoparser/prometheus/parser.go b/lib/protoparser/prometheus/parser.go index 8b6411a50..923737f12 100644 --- a/lib/protoparser/prometheus/parser.go +++ b/lib/protoparser/prometheus/parser.go @@ -249,6 +249,9 @@ func unmarshalTags(dst []Tag, s string, noEscapes bool) (string, []Tag, error) { return s, dst, fmt.Errorf("missing value for tag %q", s) } key := skipTrailingWhitespace(s[:n]) + if strings.IndexByte(key, '"') >= 0 { + return s, dst, fmt.Errorf("tag key %q cannot contain double quotes", key) + } s = skipLeadingWhitespace(s[n+1:]) if len(s) == 0 || s[0] != '"' { return s, dst, fmt.Errorf("expecting quoted value for tag %q; got %q", key, s) diff --git a/lib/protoparser/prometheus/parser_test.go b/lib/protoparser/prometheus/parser_test.go index 27ca2fd63..39116c937 100644 --- a/lib/protoparser/prometheus/parser_test.go +++ b/lib/protoparser/prometheus/parser_test.go @@ -216,6 +216,11 @@ func TestRowsUnmarshalFailure(t *testing.T) { f(`a {foo ="bar" , `) f(`a {foo ="bar" , baz } 2`) + // invalid tags - see https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4284 + f(`a{"__name__":"upsd_time_left_ns","host":"myhost", status_OB="true"} 12`) + f(`a{host:"myhost"} 12`) + f(`a{host:"myhost",foo="bar"} 12`) + // empty metric name f(`{foo="bar"}`)