Merge pull request #834 from cloudflare/labels

More accurate validation on labels and annotations
cloudflare · Feb 23, 2024 · 6bb10cd · 6bb10cd
2 parents d9e7b3b + a138844
commit 6bb10cd
Show file tree

Hide file tree

Showing 15 changed files with 273 additions and 5,251 deletions.
diff --git a/cmd/pint/tests/0037_disable_checks.txt b/cmd/pint/tests/0037_disable_checks.txt
@@ -12,8 +12,8 @@ level=DEBUG msg="Starting query workers" name=prom uri=http://127.0.0.1 workers=
 level=DEBUG msg="Generated all Prometheus servers" count=1
 level=DEBUG msg="Found alerting rule" path=rules/0001.yml alert=default-for lines=1-3
 level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/template","promql/fragile","promql/regexp","promql/vector_matching(prom)","rule/duplicate(prom)","labels/conflict(prom)"] path=rules/0001.yml rule=default-for
-level=DEBUG msg="Found recording rule" path=rules/0001.yml record=sum-job lines=5-6
-level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/template","promql/fragile","promql/regexp","promql/vector_matching(prom)","rule/duplicate(prom)","labels/conflict(prom)","promql/aggregate(job:true)"] path=rules/0001.yml rule=sum-job
+level=DEBUG msg="Found recording rule" path=rules/0001.yml record=sum:job lines=5-6
+level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/template","promql/fragile","promql/regexp","promql/vector_matching(prom)","rule/duplicate(prom)","labels/conflict(prom)","promql/aggregate(job:true)"] path=rules/0001.yml rule=sum:job
 level=DEBUG msg="Found alerting rule" path=rules/0001.yml alert=no-comparison lines=8-9
 level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/template","promql/fragile","promql/regexp","promql/vector_matching(prom)","rule/duplicate(prom)","labels/conflict(prom)"] path=rules/0001.yml rule=no-comparison
 rules/0001.yml:6 Warning: `job` label is required and should be preserved when aggregating `^.+$` rules, use `by(job, ...)`. (promql/aggregate)
@@ -26,7 +26,7 @@ level=DEBUG msg="Stopping query workers" name=prom uri=http://127.0.0.1
   expr: foo > 1
   for: 0m
 
-- record: sum-job
+- record: sum:job
   expr: sum(foo)
 
 - alert: no-comparison

diff --git a/cmd/pint/tests/0038_disable_checks_regex.txt b/cmd/pint/tests/0038_disable_checks_regex.txt
@@ -14,7 +14,7 @@ level=INFO msg="Problems found" Warning=1
   expr: foo > 1
   for: 0m
 
-- record: sum-job
+- record: sum:job
   expr: sum(foo)
 
 - alert: no-comparison

diff --git a/cmd/pint/tests/0077_strict_error_owner.txt b/cmd/pint/tests/0077_strict_error_owner.txt
@@ -4,19 +4,22 @@ cmp stderr stderr.txt
 
 -- stderr.txt --
 level=INFO msg="Finding all rules to check" paths=["rules"]
-rules/strict.yml:4 Fatal: YAML parser returned an error when reading this file: `"foo bar": invalid field 'annotations' in recording rule`. (yaml/parse)
- 4 |   - record: foo bar
+rules/strict.yml:4-7 Bug: `rule/owner` comments are required in all files, please add a `# pint file/owner $owner` somewhere in this file and/or `# pint rule/owner $owner` on top of each rule. (rule/owner)
+ 4 |   - alert: foo bar
+ 5 |     expr: 0
+ 6 |     annotations:
+ 7 |       foo: bar
 
-rules/strict.yml:4 Fatal: YAML parser returned an error when reading this file: `"foo bar": invalid recording rule name: foo bar`. (yaml/parse)
- 4 |   - record: foo bar
+rules/strict.yml:5 Warning: Alert query doesn't have any condition, it will always fire if the metric exists. (alerts/comparison)
+ 5 |     expr: 0
 
-level=INFO msg="Problems found" Fatal=2
+level=INFO msg="Problems found" Bug=1 Warning=1
 level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher"
 -- rules/strict.yml --
 groups:
 - name: foo
   rules:
-  - record: foo bar
+  - alert: foo bar
     expr: 0
     annotations:
       foo: bar
diff --git a/cmd/pint/tests/0111_snooze.txt b/cmd/pint/tests/0111_snooze.txt
@@ -8,12 +8,12 @@ level=INFO msg="Finding all rules to check" paths=["rules"]
 level=DEBUG msg="File parsed" path=rules/0001.yml rules=1
 level=DEBUG msg="Glob finder completed" count=1
 level=DEBUG msg="Generated all Prometheus servers" count=0
-level=DEBUG msg="Found recording rule" path=rules/0001.yml record=sum-job lines=2-3
+level=DEBUG msg="Found recording rule" path=rules/0001.yml record=sum:job lines=2-3
 level=DEBUG msg="Check snoozed by comment" check=promql/aggregate(job:true) match=promql/aggregate until="2099-11-28T10:24:18Z"
-level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/0001.yml rule=sum-job
+level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/0001.yml rule=sum:job
 -- rules/0001.yml --
 # pint snooze 2099-11-28T10:24:18Z promql/aggregate
-- record: sum-job
+- record: sum:job
   expr: sum(foo)
 
 -- .pint.hcl --

diff --git a/cmd/pint/tests/0112_expired_snooze.txt b/cmd/pint/tests/0112_expired_snooze.txt
@@ -8,16 +8,16 @@ level=INFO msg="Finding all rules to check" paths=["rules"]
 level=DEBUG msg="File parsed" path=rules/0001.yml rules=1
 level=DEBUG msg="Glob finder completed" count=1
 level=DEBUG msg="Generated all Prometheus servers" count=0
-level=DEBUG msg="Found recording rule" path=rules/0001.yml record=sum-job lines=2-3
-level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp","promql/aggregate(job:true)"] path=rules/0001.yml rule=sum-job
+level=DEBUG msg="Found recording rule" path=rules/0001.yml record=sum:job lines=2-3
+level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp","promql/aggregate(job:true)"] path=rules/0001.yml rule=sum:job
 rules/0001.yml:3 Bug: `job` label is required and should be preserved when aggregating `^.+$` rules, use `by(job, ...)`. (promql/aggregate)
  3 |   expr: sum(foo)
 
 level=INFO msg="Problems found" Bug=1
 level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher"
 -- rules/0001.yml --
 # pint snooze 2000-11-28T10:24:18Z promql/aggregate
-- record: sum-job
+- record: sum:job
   expr: sum(foo)
 
 -- .pint.hcl --

diff --git a/cmd/pint/tests/0116_file_snooze.txt b/cmd/pint/tests/0116_file_snooze.txt
@@ -10,15 +10,15 @@ level=DEBUG msg="Check snoozed by comment" check=alerts/for match=alerts/for unt
 level=DEBUG msg="File parsed" path=rules/0001.yml rules=2
 level=DEBUG msg="Glob finder completed" count=2
 level=DEBUG msg="Generated all Prometheus servers" count=0
-level=DEBUG msg="Found recording rule" path=rules/0001.yml record=sum-job lines=4-5
-level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/0001.yml rule=sum-job
+level=DEBUG msg="Found recording rule" path=rules/0001.yml record=sum:job lines=4-5
+level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/0001.yml rule=sum:job
 level=DEBUG msg="Found alerting rule" path=rules/0001.yml alert=Down lines=7-9
 level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/comparison","alerts/template","promql/fragile","promql/regexp"] path=rules/0001.yml rule=Down
 -- rules/0001.yml --
 # pint file/snooze 2099-11-28T10:24:18Z promql/aggregate(job:true)
 # pint file/snooze 2099-11-28T10:24:18Z alerts/for
 
-- record: sum-job
+- record: sum:job
   expr: sum(foo)
 
 - alert: Down

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_external_labels_test.go b/internal/checks/alerts_external_labels_test.go
@@ -28,11 +28,11 @@ func TestAlertsExternalLabelsCountCheck(t *testing.T) {
   expr: up{job="foo"} == 0
   annotations:
     summary: "{{ $labels.job }} is down"
-    "{{.ExternalLabels.cluster}}": "This is {{ .ExternalLabels.cluster }} cluster"
+    cluster: "This is {{ .ExternalLabels.cluster }} cluster"
   labels:
     job: "{{ $labels.job }}"
-    cluster: "{{ $externalLabels.cluster }} / {{ $externalLabels.cluster }}"
-    "{{ $externalLabels.cluster }}": "{{ $externalLabels.cluster }}"
+    twice: "{{ $externalLabels.cluster }} / {{ $externalLabels.cluster }}"
+    cluster: "{{ $externalLabels.cluster }}"
 `
 
 	testCases := []checkTest{
@@ -136,26 +136,6 @@ func TestAlertsExternalLabelsCountCheck(t *testing.T) {
 						Details:  alertsExternalLabelsDetails("prom", uri),
 						Severity: checks.Bug,
 					},
-					{
-						Lines: parser.LineRange{
-							First: 10,
-							Last:  10,
-						},
-						Reporter: checks.AlertsExternalLabelsCheckName,
-						Text:     alertsExternalLabelsText("prom", uri, "cluster"),
-						Details:  alertsExternalLabelsDetails("prom", uri),
-						Severity: checks.Bug,
-					},
-					{
-						Lines: parser.LineRange{
-							First: 6,
-							Last:  6,
-						},
-						Reporter: checks.AlertsExternalLabelsCheckName,
-						Text:     alertsExternalLabelsText("prom", uri, "cluster"),
-						Details:  alertsExternalLabelsDetails("prom", uri),
-						Severity: checks.Bug,
-					},
 					{
 						Lines: parser.LineRange{
 							First: 6,

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
@@ -120,44 +120,6 @@ func TestTemplateCheck(t *testing.T) {
 			prometheus:  noProm,
 			problems:    noProblems,
 		},
-		{
-			description: "{{ $value}} in label key",
-			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,
-					},
-				}
-			},
-		},
-		{
-			description: "{{ $value }} in label key",
-			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,
-					},
-				}
-			},
-		},
 		{
 			description: "{{$value}} in label value",
 			content:     "- alert: foo\n  expr: sum(foo)\n  labels:\n    foo: bar\n    baz: '{{$value}}'\n",