From 36cf85fc1e6f1ebe7f41a3b5028843ba4dd167f1 Mon Sep 17 00:00:00 2001 From: bwplotka Date: Mon, 27 Jan 2025 09:49:50 +0000 Subject: [PATCH] Addressed comments. Signed-off-by: bwplotka --- model/relabel/relabel.go | 19 ++++-- model/relabel/relabel_test.go | 118 +++++++++++++++++----------------- model/rulefmt/rulefmt_test.go | 2 +- 3 files changed, 72 insertions(+), 67 deletions(-) diff --git a/model/relabel/relabel.go b/model/relabel/relabel.go index 1425141248..1373484426 100644 --- a/model/relabel/relabel.go +++ b/model/relabel/relabel.go @@ -29,8 +29,8 @@ import ( ) var ( + // relabelTargetLegacy allows targeting labels with legacy Prometheus character set, plus ${} variables for dynamic characters from source the metrics. relabelTargetLegacy = regexp.MustCompile(`^(?:(?:[a-zA-Z_]|\$(?:\{\w+\}|\w+))+\w*)+$`) - relabelTargetUTF8 = regexp.MustCompile(`^(?:(?:[^\${}]|\$(?:\{[^\${}]+\}|[^\${}]+))+[^\${}]*)+$`) // All UTF-8 chars, but ${}. DefaultRelabelConfig = Config{ Action: Replace, @@ -129,11 +129,18 @@ func (c *Config) Validate() error { return fmt.Errorf("%q is invalid 'target_label' for %s action", c.TargetLabel, c.Action) } - relabelTargetRe := relabelTargetUTF8 - if model.NameValidationScheme == model.LegacyValidation { - relabelTargetRe = relabelTargetLegacy + isValidLabelNameWithRegexVarFn := func(value string) bool { + // UTF-8 allows ${} characters, so standard validation allow $variables by default. + // TODO(bwplotka): Relabelling users cannot put $ and ${<...>} characters in metric names or values. + // Design escaping mechanism to allow that, once valid use case appears. + return model.LabelName(value).IsValid() } - if c.Action == Replace && varInRegexTemplate(c.TargetLabel) && !relabelTargetRe.MatchString(c.TargetLabel) { + if model.NameValidationScheme == model.LegacyValidation { + isValidLabelNameWithRegexVarFn = func(value string) bool { + return relabelTargetLegacy.MatchString(value) + } + } + if c.Action == Replace && varInRegexTemplate(c.TargetLabel) && !isValidLabelNameWithRegexVarFn(c.TargetLabel) { return fmt.Errorf("%q is invalid 'target_label' for %s action", c.TargetLabel, c.Action) } if (c.Action == Lowercase || c.Action == Uppercase || c.Action == KeepEqual || c.Action == DropEqual) && !model.LabelName(c.TargetLabel).IsValid() { @@ -142,7 +149,7 @@ func (c *Config) Validate() error { if (c.Action == Lowercase || c.Action == Uppercase || c.Action == KeepEqual || c.Action == DropEqual) && c.Replacement != DefaultRelabelConfig.Replacement { return fmt.Errorf("'replacement' can not be set for %s action", c.Action) } - if c.Action == LabelMap && !relabelTargetRe.MatchString(c.Replacement) { + if c.Action == LabelMap && !isValidLabelNameWithRegexVarFn(c.Replacement) { return fmt.Errorf("%q is invalid 'replacement' for %s action", c.Replacement, c.Action) } if c.Action == HashMod && !model.LabelName(c.TargetLabel).IsValid() { diff --git a/model/relabel/relabel_test.go b/model/relabel/relabel_test.go index c5064540f7..a8026f275e 100644 --- a/model/relabel/relabel_test.go +++ b/model/relabel/relabel_test.go @@ -651,7 +651,7 @@ func TestRelabel(t *testing.T) { Regex: MustNewRegexp("line1.*line2"), TargetLabel: "utf-8.label${1}", Separator: ";", - Replacement: `match${1}`, + Replacement: "match${1}", Action: Replace, }, }, @@ -662,6 +662,38 @@ func TestRelabel(t *testing.T) { "utf-8.label": "match", }), }, + { // Replace targetLabel with UTF-8 characters and various $var styles. + input: labels.FromMap(map[string]string{ + "a": "line1\nline2", + "b": "bar", + "c": "baz", + }), + relabel: []*Config{ + { + SourceLabels: model.LabelNames{"a"}, + Regex: MustNewRegexp("(line1).*(?line2)"), + TargetLabel: "label1_${1}", + Separator: ";", + Replacement: "val_${second}", + Action: Replace, + }, + { + SourceLabels: model.LabelNames{"a"}, + Regex: MustNewRegexp("(line1).*(?line2)"), + TargetLabel: "label2_$1", + Separator: ";", + Replacement: "val_$second", + Action: Replace, + }, + }, + output: labels.FromMap(map[string]string{ + "a": "line1\nline2", + "b": "bar", + "c": "baz", + "label1_line1": "val_line2", + "label2_line1": "val_line2", + }), + }, } for _, test := range tests { @@ -758,65 +790,31 @@ func TestRelabelValidate(t *testing.T) { } } -func TestTargetLabelValidity(t *testing.T) { - t.Run("legacy", func(t *testing.T) { - for _, test := range []struct { - str string - valid bool - }{ - {"-label", false}, - {"label", true}, - {"label${1}", true}, - {"${1}label", true}, - {"${1}", true}, - {"${1}label", true}, - {"${", false}, - {"$", false}, - {"${}", false}, - {"foo${", false}, - {"$1", true}, - {"asd$2asd", true}, - {"-foo${1}bar-", false}, - {"bar.foo${1}bar", false}, - {"_${1}_", true}, - {"foo${bar}foo", true}, - } { - t.Run(test.str, func(t *testing.T) { - require.Equal(t, test.valid, relabelTargetLegacy.Match([]byte(test.str)), - "Expected %q to be %v", test.str, test.valid) - }) - } - }) - t.Run("utf-8", func(t *testing.T) { - for _, test := range []struct { - str string - valid bool - }{ - {"-label", true}, - {"label", true}, - {"label${1}", true}, - {"${1}label", true}, - {"${1}", true}, - {"${1}label", true}, - {"$1", true}, - {"asd$2asd", true}, - {"-foo${1}bar-", true}, - {"bar.foo${1}bar", true}, - {"_${1}_", true}, - {"foo${bar}foo", true}, - - // Those can be ambiguous. Currently, we assume user error. - {"${", false}, - {"$", false}, - {"${}", false}, - {"foo${", false}, - } { - t.Run(test.str, func(t *testing.T) { - require.Equal(t, test.valid, relabelTargetUTF8.Match([]byte(test.str)), - "Expected %q to be %v", test.str, test.valid) - }) - } - }) +func TestTargetLabelLegacyValidity(t *testing.T) { + for _, test := range []struct { + str string + valid bool + }{ + {"-label", false}, + {"label", true}, + {"label${1}", true}, + {"${1}label", true}, + {"${1}", true}, + {"${1}label", true}, + {"${", false}, + {"$", false}, + {"${}", false}, + {"foo${", false}, + {"$1", true}, + {"asd$2asd", true}, + {"-foo${1}bar-", false}, + {"bar.foo${1}bar", false}, + {"_${1}_", true}, + {"foo${bar}foo", true}, + } { + require.Equal(t, test.valid, relabelTargetLegacy.Match([]byte(test.str)), + "Expected %q to be %v", test.str, test.valid) + } } func BenchmarkRelabel(b *testing.B) { diff --git a/model/rulefmt/rulefmt_test.go b/model/rulefmt/rulefmt_test.go index f24b02f7b3..9e191820c5 100644 --- a/model/rulefmt/rulefmt_test.go +++ b/model/rulefmt/rulefmt_test.go @@ -60,7 +60,7 @@ func TestParseFileFailure(t *testing.T) { }, { filename: "invalid_record_name.bad.yaml", - errMsg: "potential issue in the recording rule name; should it be in expr?: strawberry{flavor=\"sweet\"}", + errMsg: "braces present in the recording rule name; should it be in expr?: strawberry{flavor=\"sweet\"}", }, { filename: "bad_field.bad.yaml",