From 9ab7e3b3dec4b82870487154dce53baa3266a614 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 18 Dec 2023 14:52:42 +0000 Subject: [PATCH 1/4] relabel: refactor: extract config.Validate method And add a test for it, which fails because validation is not strong enough. Signed-off-by: Bryan Boreham --- model/relabel/relabel.go | 4 ++ model/relabel/relabel_test.go | 72 +++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/model/relabel/relabel.go b/model/relabel/relabel.go index fadf35b86..fa0d809de 100644 --- a/model/relabel/relabel.go +++ b/model/relabel/relabel.go @@ -108,6 +108,10 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error { if c.Regex.Regexp == nil { c.Regex = MustNewRegexp("") } + return c.Validate() +} + +func (c *Config) Validate() error { if c.Action == "" { return fmt.Errorf("relabel action cannot be empty") } diff --git a/model/relabel/relabel_test.go b/model/relabel/relabel_test.go index b50ff4010..fe040be3a 100644 --- a/model/relabel/relabel_test.go +++ b/model/relabel/relabel_test.go @@ -14,6 +14,7 @@ package relabel import ( + "fmt" "testing" "github.com/prometheus/common/model" @@ -575,6 +576,77 @@ func TestRelabel(t *testing.T) { } } +func TestRelabelValidate(t *testing.T) { + tests := []struct { + config Config + expected string + }{ + { + config: Config{}, + expected: `relabel action cannot be empty`, + }, + { + config: Config{ + Action: Replace, + }, + expected: `requires 'target_label' value`, + }, + { + config: Config{ + Action: Lowercase, + }, + expected: `requires 'target_label' value`, + }, + { + config: Config{ + Action: Lowercase, + Replacement: DefaultRelabelConfig.Replacement, + TargetLabel: "${3}", + }, + expected: `"${3}" is invalid 'target_label'`, + }, + { + config: Config{ + SourceLabels: model.LabelNames{"a"}, + Regex: MustNewRegexp("some-([^-]+)-([^,]+)"), + Action: Replace, + Replacement: "${1}", + TargetLabel: "${3}", + }, + }, + { + config: Config{ + SourceLabels: model.LabelNames{"a"}, + Regex: MustNewRegexp("some-([^-]+)-([^,]+)"), + Action: Replace, + Replacement: "${1}", + TargetLabel: "0${3}", + }, + expected: `"0${3}" is invalid 'target_label'`, + }, + { + config: Config{ + SourceLabels: model.LabelNames{"a"}, + Regex: MustNewRegexp("some-([^-]+)-([^,]+)"), + Action: Replace, + Replacement: "${1}", + TargetLabel: "-${3}", + }, + expected: `"-${3}" is invalid 'target_label' for replace action`, + }, + } + for i, test := range tests { + t.Run(fmt.Sprint(i), func(t *testing.T) { + err := test.config.Validate() + if test.expected == "" { + require.NoError(t, err) + } else { + require.ErrorContains(t, err, test.expected) + } + }) + } +} + func TestTargetLabelValidity(t *testing.T) { tests := []struct { str string From 2d4c367d87cdd99eac1b4786cf1621c58d43735d Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 18 Dec 2023 14:58:56 +0000 Subject: [PATCH 2/4] relabel: stricter check that target labels are valid For `Lowercase`, `KeepEqual`, etc., we do not expand a regexp, so the target label name must not contain anything like `${1}`. Also for the common case that the `Replace` target does not require any template expansion, check that the entire string passes label name validity rules. Signed-off-by: Bryan Boreham --- model/relabel/relabel.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/model/relabel/relabel.go b/model/relabel/relabel.go index fa0d809de..1947e6273 100644 --- a/model/relabel/relabel.go +++ b/model/relabel/relabel.go @@ -121,7 +121,13 @@ func (c *Config) Validate() error { if (c.Action == Replace || c.Action == HashMod || c.Action == Lowercase || c.Action == Uppercase || c.Action == KeepEqual || c.Action == DropEqual) && c.TargetLabel == "" { return fmt.Errorf("relabel configuration for %s action requires 'target_label' value", c.Action) } - if (c.Action == Replace || c.Action == Lowercase || c.Action == Uppercase || c.Action == KeepEqual || c.Action == DropEqual) && !relabelTarget.MatchString(c.TargetLabel) { + if c.Action == Replace && !strings.Contains(c.TargetLabel, "$") && !model.LabelName(c.TargetLabel).IsValid() { + return fmt.Errorf("%q is invalid 'target_label' for %s action", c.TargetLabel, c.Action) + } + if c.Action == Replace && strings.Contains(c.TargetLabel, "$") && !relabelTarget.MatchString(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() { 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) && c.Replacement != DefaultRelabelConfig.Replacement { From 000182e4b8638bfe781a583c27a1e8a366ac82bf Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 18 Dec 2023 15:03:21 +0000 Subject: [PATCH 3/4] relabel: check validity of all test cases Thought this would be a nice check on the `Validate()` function, but some of the test cases needed tweaking to pass. Signed-off-by: Bryan Boreham --- model/relabel/relabel_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/model/relabel/relabel_test.go b/model/relabel/relabel_test.go index fe040be3a..2b8fc911d 100644 --- a/model/relabel/relabel_test.go +++ b/model/relabel/relabel_test.go @@ -335,7 +335,7 @@ func TestRelabel(t *testing.T) { }, { // invalid target_labels input: labels.FromMap(map[string]string{ - "a": "some-name-value", + "a": "some-name-0", }), relabel: []*Config{ { @@ -350,18 +350,18 @@ func TestRelabel(t *testing.T) { Regex: MustNewRegexp("some-([^-]+)-([^,]+)"), Action: Replace, Replacement: "${1}", - TargetLabel: "0${3}", + TargetLabel: "${3}", }, { SourceLabels: model.LabelNames{"a"}, - Regex: MustNewRegexp("some-([^-]+)-([^,]+)"), + Regex: MustNewRegexp("some-([^-]+)(-[^,]+)"), Action: Replace, Replacement: "${1}", - TargetLabel: "-${3}", + TargetLabel: "${3}", }, }, output: labels.FromMap(map[string]string{ - "a": "some-name-value", + "a": "some-name-0", }), }, { // more complex real-life like usecase @@ -566,6 +566,7 @@ func TestRelabel(t *testing.T) { if cfg.Replacement == "" { cfg.Replacement = DefaultRelabelConfig.Replacement } + require.NoError(t, cfg.Validate()) } res, keep := Process(test.input, test.relabel...) From 0289dd61571c7a812a235ecec5bc3f74fd7ccf50 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 18 Dec 2023 16:38:59 +0000 Subject: [PATCH 4/4] relabel: blank replacement deletes label post-regexp If `cfg.TargetLabel` is a template like `$1`, it won't match any labels, so no point calling `lb.Del` with it. Similarly if `target` is not a valid label name, it won't match any labels, so don't call with that either. The intention seems to have been that a blank _value_ would delete the target, so change that code to use `target` instead of `cfg.TargetLabel`. Signed-off-by: Bryan Boreham --- model/relabel/relabel.go | 3 +-- model/relabel/relabel_test.go | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/model/relabel/relabel.go b/model/relabel/relabel.go index 1947e6273..d29c3d07a 100644 --- a/model/relabel/relabel.go +++ b/model/relabel/relabel.go @@ -274,12 +274,11 @@ func relabel(cfg *Config, lb *labels.Builder) (keep bool) { } target := model.LabelName(cfg.Regex.ExpandString([]byte{}, cfg.TargetLabel, val, indexes)) if !target.IsValid() { - lb.Del(cfg.TargetLabel) break } res := cfg.Regex.ExpandString([]byte{}, cfg.Replacement, val, indexes) if len(res) == 0 { - lb.Del(cfg.TargetLabel) + lb.Del(string(target)) break } lb.Set(string(target), string(res)) diff --git a/model/relabel/relabel_test.go b/model/relabel/relabel_test.go index 2b8fc911d..517b9b822 100644 --- a/model/relabel/relabel_test.go +++ b/model/relabel/relabel_test.go @@ -214,6 +214,25 @@ func TestRelabel(t *testing.T) { "a": "boo", }), }, + { + // Blank replacement should delete the label. + input: labels.FromMap(map[string]string{ + "a": "foo", + "f": "baz", + }), + relabel: []*Config{ + { + SourceLabels: model.LabelNames{"a"}, + Regex: MustNewRegexp("(f).*"), + TargetLabel: "$1", + Replacement: "$2", + Action: Replace, + }, + }, + output: labels.FromMap(map[string]string{ + "a": "foo", + }), + }, { input: labels.FromMap(map[string]string{ "a": "foo",