More accurate validation on labels and annotations

cloudflare · Feb 21, 2024 · 19327a7 · 19327a7
1 parent d9e7b3b
commit 19327a7
Show file tree

Hide file tree

Showing 7 changed files with 168 additions and 34 deletions.
diff --git a/cmd/pint/tests/0166_invalid_label.txt b/cmd/pint/tests/0166_invalid_label.txt
@@ -0,0 +1,26 @@
+pint.error --no-color lint rules
+! stdout .
+cmp stderr stderr.txt
+
+-- stderr.txt --
+level=INFO msg="Finding all rules to check" paths=["rules"]
+rules/1.yaml:7 Fatal: This rule is not a valid Prometheus rule: `invalid annotation name: {{ $value }}`. (yaml/parse)
+ 7 |       "{{ $value }}": "down"
+
+rules/1.yaml:11 Fatal: This rule is not a valid Prometheus rule: `invalid label name: {{ $value }}`. (yaml/parse)
+ 11 |       "{{ $value }}": "down"
+
+level=INFO msg="Problems found" Fatal=2
+level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher"
+-- rules/1.yaml --
+groups:
+- name: g1
+  rules:
+  - alert: Service Down
+    expr: up == 0
+    annotations:
+      "{{ $value }}": "down"
+  - alert: Service Down
+    expr: up == 0
+    labels:
+      "{{ $value }}": "down"
diff --git a/docs/changelog.md b/docs/changelog.md
@@ -5,6 +5,7 @@
 ### Changed
 
 - A large part of rule parsing code was refactored and more problems will now be deduplicated.
+- Improved validation of labels and annotations.
 
 ## v0.53.0
 

diff --git a/internal/checks/alerts_template.go b/internal/checks/alerts_template.go
@@ -158,19 +158,6 @@ func (c TemplateCheck) Check(ctx context.Context, _ string, rule parser.Rule, _
 					Severity: Fatal,
 				})
 			}
-			// check key
-			for _, msg := range checkForValueInLabels(label.Key.Value, label.Key.Value) {
-				problems = append(problems, Problem{
-					Lines: parser.LineRange{
-						First: label.Key.Lines.First,
-						Last:  label.Value.Lines.Last,
-					},
-					Reporter: c.Reporter(),
-					Text:     msg,
-					Severity: Bug,
-				})
-			}
-			// check value
 			for _, msg := range checkForValueInLabels(label.Key.Value, label.Value.Value) {
 				problems = append(problems, Problem{
 					Lines: parser.LineRange{

diff --git a/internal/checks/alerts_template_test.go b/internal/checks/alerts_template_test.go
@@ -125,19 +125,7 @@ func TestTemplateCheck(t *testing.T) {
 			content:     "- alert: foo\n  expr: sum(foo)\n  labels:\n    foo: bar\n    '{{ $value}}': bar\n",
 			checker:     newTemplateCheck,
 			prometheus:  noProm,
-			problems: func(_ string) []checks.Problem {
-				return []checks.Problem{
-					{
-						Lines: parser.LineRange{
-							First: 5,
-							Last:  5,
-						},
-						Reporter: checks.TemplateCheckName,
-						Text:     "Using `$value` in labels will generate a new alert on every value change, move it to annotations.",
-						Severity: checks.Bug,
-					},
-				}
-			},
+			problems:    noProblems,
 		},
 		{
 			description: "{{ $value }} in label key",

diff --git a/internal/discovery/discovery.go b/internal/discovery/discovery.go
@@ -28,6 +28,9 @@ var ignoredErrors = []string{
 	"could not parse expression: ",
 	"cannot unmarshal !!seq into rulefmt.ruleGroups",
 	": template: __",
+	"invalid label name: ",
+	"invalid annotation name: ",
+	"invalid recording rule name: ",
 }
 
 type FileIgnoreError struct {

diff --git a/internal/parser/parser.go b/internal/parser/parser.go
@@ -8,6 +8,7 @@ import (
 	"gopkg.in/yaml.v3"
 
 	"github.com/cloudflare/pint/internal/comments"
+	"github.com/prometheus/common/model"
 )
 
 const (
@@ -266,6 +267,53 @@ func parseRule(content []byte, node *yaml.Node, offset int) (rule Rule, _ bool,
 		return rule, false, err
 	}
 
+	if recordPart != nil && !model.IsValidMetricName(model.LabelValue(recordPart.Value)) {
+		return Rule{
+			Lines: lines,
+			Error: ParseError{
+				Line: recordPart.Lines.First,
+				Err:  fmt.Errorf("invalid recording rule name: %s", recordPart.Value),
+			},
+		}, false, err
+	}
+
+	if (recordPart != nil || alertPart != nil) && labelsPart != nil {
+		for _, lab := range labelsPart.Items {
+			if !model.LabelName(lab.Key.Value).IsValid() || lab.Key.Value == model.MetricNameLabel {
+				return Rule{
+					Lines: lines,
+					Error: ParseError{
+						Line: lab.Key.Lines.First,
+						Err:  fmt.Errorf("invalid label name: %s", lab.Key.Value),
+					},
+				}, false, err
+			}
+			if !model.LabelValue(lab.Value.Value).IsValid() {
+				return Rule{
+					Lines: lines,
+					Error: ParseError{
+						Line: lab.Key.Lines.First,
+						Err:  fmt.Errorf("invalid label value: %s", lab.Value.Value),
+					},
+				}, false, err
+			}
+		}
+	}
+
+	if alertPart != nil && annotationsPart != nil {
+		for _, ann := range annotationsPart.Items {
+			if !model.LabelName(ann.Key.Value).IsValid() {
+				return Rule{
+					Lines: lines,
+					Error: ParseError{
+						Line: ann.Key.Lines.First,
+						Err:  fmt.Errorf("invalid annotation name: %s", ann.Key.Value),
+					},
+				}, false, err
+			}
+		}
+	}
+
 	if recordPart != nil && exprPart != nil {
 		rule = Rule{
 			Lines: lines,

diff --git a/internal/parser/parser_test.go b/internal/parser/parser_test.go
@@ -522,8 +522,7 @@ groups:
   annotations:
     uri: https://docs.example.com/down.html
 
-- record: >
-    foo
+- record: foo
   expr: |-
     bar
     /
@@ -596,15 +595,15 @@ groups:
 					},
 				},
 				{
-					Lines: parser.LineRange{First: 11, Last: 17},
+					Lines: parser.LineRange{First: 11, Last: 16},
 					RecordingRule: &parser.RecordingRule{
 						Record: parser.YamlNode{
-							Lines: parser.LineRange{First: 11, Last: 12},
-							Value: "foo\n",
+							Lines: parser.LineRange{First: 11, Last: 11},
+							Value: "foo",
 						},
 						Expr: parser.PromQLExpr{
 							Value: &parser.YamlNode{
-								Lines: parser.LineRange{First: 13, Last: 16},
+								Lines: parser.LineRange{First: 12, Last: 15},
 								Value: "bar\n/\nbaz > 1",
 							},
 							Query: &parser.PromQLNode{
@@ -621,9 +620,9 @@ groups:
 							},
 						},
 						Labels: &parser.YamlMap{
-							Lines: parser.LineRange{First: 17, Last: 17},
+							Lines: parser.LineRange{First: 16, Last: 16},
 							Key: &parser.YamlNode{
-								Lines: parser.LineRange{First: 17, Last: 17},
+								Lines: parser.LineRange{First: 16, Last: 16},
 								Value: "labels",
 							},
 						},
@@ -1403,6 +1402,88 @@ data:
 				},
 			},
 		},
+		{
+			content: []byte(`
+- record: invalid metric name
+  expr: bar
+`),
+			output: []parser.Rule{
+				{
+					Lines: parser.LineRange{First: 2, Last: 3},
+					Error: parser.ParseError{Err: fmt.Errorf("invalid recording rule name: invalid metric name"), Line: 2},
+				},
+			},
+		},
+		{
+			content: []byte(`
+- record: foo
+  expr: bar
+  labels:
+    "foo bar": yes
+`),
+			output: []parser.Rule{
+				{
+					Lines: parser.LineRange{First: 2, Last: 5},
+					Error: parser.ParseError{Err: fmt.Errorf("invalid label name: foo bar"), Line: 5},
+				},
+			},
+		},
+		{
+			content: []byte(`
+- alert: foo
+  expr: bar
+  labels:
+    "foo bar": yes
+`),
+			output: []parser.Rule{
+				{
+					Lines: parser.LineRange{First: 2, Last: 5},
+					Error: parser.ParseError{Err: fmt.Errorf("invalid label name: foo bar"), Line: 5},
+				},
+			},
+		},
+		{
+			content: []byte(`
+- alert: foo
+  expr: bar
+  labels:
+    "{{ $value }}": yes
+`),
+			output: []parser.Rule{
+				{
+					Lines: parser.LineRange{First: 2, Last: 5},
+					Error: parser.ParseError{Err: fmt.Errorf("invalid label name: {{ $value }}"), Line: 5},
+				},
+			},
+		},
+		{
+			content: []byte(`
+- alert: foo
+  expr: bar
+  annotations:
+    "foo bar": yes
+`),
+			output: []parser.Rule{
+				{
+					Lines: parser.LineRange{First: 2, Last: 5},
+					Error: parser.ParseError{Err: fmt.Errorf("invalid annotation name: foo bar"), Line: 5},
+				},
+			},
+		},
+		{
+			content: []byte(`
+- alert: foo
+  expr: bar
+  annotations:
+    "{{ $value }}": yes
+`),
+			output: []parser.Rule{
+				{
+					Lines: parser.LineRange{First: 2, Last: 5},
+					Error: parser.ParseError{Err: fmt.Errorf("invalid annotation name: {{ $value }}"), Line: 5},
+				},
+			},
+		},
 	}
 
 	alwaysEqual := cmp.Comparer(func(_, _ interface{}) bool { return true })