diff --git a/cmd/pint/tests/0166_invalid_label.txt b/cmd/pint/tests/0166_invalid_label.txt new file mode 100644 index 00000000..d076f5bd --- /dev/null +++ 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 index b7eb48d3..54cec612 100644 --- 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 index 1cf322ab..e8dbf3c5 100644 --- 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 index c039fe81..9d61d20a 100644 --- 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 index 490cc3cc..a14d945e 100644 --- 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 index 091909f3..611634a8 100644 --- 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 index 8e7acf1c..fb0b116e 100644 --- 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 })