Merge pull request #13308 from bboreham/validate-relabel

relabel: improve logic for target labels

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")
 	}
@@ -117,7 +121,13 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) 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 {
@@ -264,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))

model/relabel/relabel_test.go

@@ -14,6 +14,7 @@
 package relabel
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/prometheus/common/model"
@@ -213,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",
@@ -334,7 +354,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{
 				{
@@ -349,18 +369,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
@@ -565,6 +585,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...)
@@ -575,6 +596,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