diff --git a/cmd/pint/tests/0077_strict_error_owner.txt b/cmd/pint/tests/0077_strict_error_owner.txt index 8544d3d9..ab92d5a2 100644 --- a/cmd/pint/tests/0077_strict_error_owner.txt +++ b/cmd/pint/tests/0077_strict_error_owner.txt @@ -6,20 +6,17 @@ cmp stderr stderr.txt level=INFO msg="Finding all rules to check" paths=["rules"] 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 + 5 | expr: up == 0 6 | annotations: 7 | 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" Bug=1 Warning=1 +level=INFO msg="Problems found" Bug=1 level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher" -- rules/strict.yml -- groups: - name: foo rules: - alert: foo bar - expr: 0 + expr: up == 0 annotations: foo: bar diff --git a/docs/changelog.md b/docs/changelog.md index ac784a5d..3799e3ed 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -7,6 +7,27 @@ - [rule/dependency](checks/rule/dependency.md) check will now warn if an alerting rule that's being removed in a pull request is being used inside `ALERTS{alertname="..."}` or `ALERTS_FOR_STATE{alertname="..."}` queries. +- pint will now perform extra validation of YAML files to ensure that all values + are mappings and strings. + This will error on rules where, for example, label value is unquoted number: + + Bad rule: + + ```yaml + - alert: DeadMansSwitch + expr: vector(1) + labels: + priority: 5 + ``` + + Good rule: + + ```yaml + - alert: DeadMansSwitch + expr: vector(1) + labels: + priority: "5" + ``` ### Changed diff --git a/internal/parser/models.go b/internal/parser/models.go index 37a7e64c..b5be74da 100644 --- a/internal/parser/models.go +++ b/internal/parser/models.go @@ -26,6 +26,13 @@ func nodeLines(node *yaml.Node, offset int) (lr LineRange) { return lr } +func nodeValue(node *yaml.Node) string { + if node.Alias != nil { + return node.Alias.Value + } + return node.Value +} + func mergeComments(node *yaml.Node) (comments []string) { if node.HeadComment != "" { comments = append(comments, node.HeadComment) @@ -63,10 +70,7 @@ func (yn *YamlNode) IsIdentical(b *YamlNode) bool { func newYamlNode(node *yaml.Node, offset int) *YamlNode { n := YamlNode{ Lines: nodeLines(node, offset), - Value: node.Value, - } - if node.Alias != nil { - n.Value = node.Alias.Value + Value: nodeValue(node), } return &n } @@ -79,10 +83,7 @@ func newYamlNodeWithKey(key, node *yaml.Node, offset int) *YamlNode { First: min(keyLines.First, valLines.First), Last: max(keyLines.Last, valLines.Last), }, - Value: node.Value, - } - if node.Alias != nil { - n.Value = node.Alias.Value + Value: nodeValue(node), } return &n } diff --git a/internal/parser/parser.go b/internal/parser/parser.go index c76f9634..f8f076ec 100644 --- a/internal/parser/parser.go +++ b/internal/parser/parser.go @@ -122,6 +122,16 @@ func parseRule(content []byte, node *yaml.Node, offset int) (rule Rule, _ bool, var keepFiringForPart *YamlNode var annotationsPart *YamlMap + var recordNode *yaml.Node + var alertNode *yaml.Node + var exprNode *yaml.Node + var forNode *yaml.Node + var keepFiringForNode *yaml.Node + var labelsNode *yaml.Node + var annotationsNode *yaml.Node + labelsNodes := map[*yaml.Node]*yaml.Node{} + annotationsNodes := map[*yaml.Node]*yaml.Node{} + var key *yaml.Node unknownKeys := []*yaml.Node{} @@ -158,47 +168,55 @@ func parseRule(content []byte, node *yaml.Node, offset int) (rule Rule, _ bool, switch key.Value { case recordKey: if recordPart != nil { - return duplicatedKeyError(lines, part.Line+offset, recordKey, nil) + return duplicatedKeyError(lines, part.Line+offset, recordKey) } + recordNode = part recordPart = newYamlNodeWithKey(key, part, offset) lines.Last = max(lines.Last, recordPart.Lines.Last) case alertKey: if alertPart != nil { - return duplicatedKeyError(lines, part.Line+offset, alertKey, nil) + return duplicatedKeyError(lines, part.Line+offset, alertKey) } + alertNode = part alertPart = newYamlNodeWithKey(key, part, offset) lines.Last = max(lines.Last, alertPart.Lines.Last) case exprKey: if exprPart != nil { - return duplicatedKeyError(lines, part.Line+offset, exprKey, nil) + return duplicatedKeyError(lines, part.Line+offset, exprKey) } + exprNode = part exprPart = newPromQLExpr(key, part, offset) lines.Last = max(lines.Last, exprPart.Value.Lines.Last) case forKey: if forPart != nil { - return duplicatedKeyError(lines, part.Line+offset, forKey, nil) + return duplicatedKeyError(lines, part.Line+offset, forKey) } + forNode = part forPart = newYamlNodeWithKey(key, part, offset) lines.Last = max(lines.Last, forPart.Lines.Last) + case keepFiringForKey: + if keepFiringForPart != nil { + return duplicatedKeyError(lines, part.Line+offset, keepFiringForKey) + } + keepFiringForNode = part + keepFiringForPart = newYamlNodeWithKey(key, part, offset) + lines.Last = max(lines.Last, keepFiringForPart.Lines.Last) case labelsKey: if labelsPart != nil { - return duplicatedKeyError(lines, part.Line+offset, labelsKey, nil) + return duplicatedKeyError(lines, part.Line+offset, labelsKey) } + labelsNode = part + labelsNodes = mappingNodes(part) labelsPart = newYamlMap(key, part, offset) lines.Last = max(lines.Last, labelsPart.Lines.Last) case annotationsKey: if annotationsPart != nil { - return duplicatedKeyError(lines, part.Line+offset, annotationsKey, nil) + return duplicatedKeyError(lines, part.Line+offset, annotationsKey) } + annotationsNode = part + annotationsNodes = mappingNodes(part) annotationsPart = newYamlMap(key, part, offset) lines.Last = max(lines.Last, annotationsPart.Lines.Last) - - case keepFiringForKey: - if keepFiringForPart != nil { - return duplicatedKeyError(lines, part.Line+offset, keepFiringForKey, nil) - } - keepFiringForPart = newYamlNodeWithKey(key, part, offset) - lines.Last = max(lines.Last, keepFiringForPart.Lines.Last) default: unknownKeys = append(unknownKeys, key) } @@ -345,6 +363,37 @@ func parseRule(content []byte, node *yaml.Node, offset int) (rule Rule, _ bool, } } + for key, part := range map[string]*yaml.Node{ + recordKey: recordNode, + alertKey: alertNode, + exprKey: exprNode, + forKey: forNode, + keepFiringForKey: keepFiringForNode, + } { + if part != nil && !isTag(part.ShortTag(), "!!str") { + return invalidValueError(lines, part.Line+offset, key, "string", describeTag(part.ShortTag())) + } + } + for key, part := range map[string]*yaml.Node{ + labelsKey: labelsNode, + annotationsKey: annotationsNode, + } { + if part != nil && !isTag(part.ShortTag(), "!!map") { + return invalidValueError(lines, part.Line+offset, key, "mapping", describeTag(part.ShortTag())) + } + } + + for section, parts := range map[string]map[*yaml.Node]*yaml.Node{ + labelsKey: labelsNodes, + annotationsKey: annotationsNodes, + } { + for key, value := range parts { + if !isTag(value.ShortTag(), "!!str") { + return invalidValueError(lines, value.Line+offset, fmt.Sprintf("%s %s", section, nodeValue(key)), "string", describeTag(value.ShortTag())) + } + } + } + if recordPart != nil && exprPart != nil { rule = Rule{ Lines: lines, @@ -381,7 +430,7 @@ func unpackNodes(node *yaml.Node) []*yaml.Node { nodes := make([]*yaml.Node, 0, len(node.Content)) var isMerge bool for _, part := range node.Content { - if part.Tag == "!!merge" && part.Value == "<<" { + if part.ShortTag() == "!!merge" && part.Value == "<<" { isMerge = true } @@ -482,7 +531,7 @@ func resolveMapAlias(part, parent *yaml.Node) *yaml.Node { return &node } -func duplicatedKeyError(lines LineRange, line int, key string, err error) (Rule, bool, error) { +func duplicatedKeyError(lines LineRange, line int, key string) (Rule, bool, error) { rule := Rule{ Lines: lines, Error: ParseError{ @@ -490,5 +539,56 @@ func duplicatedKeyError(lines LineRange, line int, key string, err error) (Rule, Err: fmt.Errorf("duplicated %s key", key), }, } - return rule, false, err + return rule, false, nil +} + +func invalidValueError(lines LineRange, line int, key, expectedKind, gotKind string) (Rule, bool, error) { + rule := Rule{ + Lines: lines, + Error: ParseError{ + Line: line, + Err: fmt.Errorf("%s value must be a YAML %s, got %s instead", key, expectedKind, gotKind), + }, + } + return rule, false, nil +} + +func isTag(tag, expected string) bool { + if tag == "!!null" { + return true + } + return tag == expected +} + +func describeTag(tag string) string { + switch tag { + case "": + return "unknown type" + case "!!str": + return "string" + case "!!int": + return "integer" + case "!!seq": + return "list" + case "!!map": + return "mapping" + case "!!binary": + return "binary data" + default: + return strings.TrimLeft(tag, "!") + } +} + +func mappingNodes(node *yaml.Node) map[*yaml.Node]*yaml.Node { + m := make(map[*yaml.Node]*yaml.Node, len(node.Content)) + var key *yaml.Node + for _, child := range node.Content { + if key != nil { + m[key] = child + key = nil + } else { + key = child + } + } + return m } diff --git a/internal/parser/parser_test.go b/internal/parser/parser_test.go index 75bb6094..719ae660 100644 --- a/internal/parser/parser_test.go +++ b/internal/parser/parser_test.go @@ -5,6 +5,8 @@ import ( "strconv" "testing" + "github.com/stretchr/testify/require" + "github.com/cloudflare/pint/internal/comments" "github.com/cloudflare/pint/internal/parser" @@ -14,26 +16,24 @@ import ( func TestParse(t *testing.T) { type testCaseT struct { - content []byte - output []parser.Rule - shouldError bool + content []byte + output []parser.Rule + err string } testCases := []testCaseT{ { - content: nil, - output: nil, - shouldError: false, + content: nil, + output: nil, }, { - content: []byte{}, - output: nil, - shouldError: false, + content: []byte{}, + output: nil, }, { - content: []byte(string("! !00 \xf6")), - output: nil, - shouldError: true, + content: []byte(string("! !00 \xf6")), + output: nil, + err: "yaml: incomplete UTF-8 octet sequence", }, { content: []byte("- 0: 0\n 00000000: 000000\n 000000:00000000000: 00000000\n 00000000000:000000: 0000000000000000000000000000000000\n 000000: 0000000\n expr: |"), @@ -90,16 +90,16 @@ func TestParse(t *testing.T) { }, }, { - content: []byte("- record: - foo\n"), - shouldError: true, + content: []byte("- record: - foo\n"), + err: "yaml: block sequence entries are not allowed in this context", }, { - content: []byte("- record: foo expr: sum(\n"), - shouldError: true, + content: []byte("- record: foo expr: sum(\n"), + err: "yaml: mapping values are not allowed in this context", }, { - content: []byte("- record\n\texpr: foo\n"), - shouldError: true, + content: []byte("- record\n\texpr: foo\n"), + err: "yaml: line 2: found a tab character that violates indentation", }, { content: []byte(` @@ -444,7 +444,6 @@ func TestParse(t *testing.T) { }, }, }, - shouldError: false, }, { content: []byte(` @@ -509,7 +508,6 @@ groups: }, }, }, - shouldError: false, }, { content: []byte(`- alert: Down @@ -629,7 +627,6 @@ groups: }, }, }, - shouldError: false, }, { content: []byte(`- alert: Foo @@ -1293,7 +1290,6 @@ data: }, }, }, - shouldError: false, }, { content: []byte(string(` @@ -1478,7 +1474,7 @@ data: foo: ` + string("\xed\xbf\xbf")), // Label values are invalid only if they aren't valid UTF-8 strings // which also makes them unparsable by YAML. - shouldError: true, + err: "yaml: invalid Unicode character", }, { content: []byte(` @@ -1534,6 +1530,155 @@ data: }, }, }, + // Tag tests + { + content: []byte(` +- record: 5 + expr: bar +`), + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 2, Last: 3}, + Error: parser.ParseError{Err: fmt.Errorf("invalid recording rule name: 5"), Line: 2}, + }, + }, + }, + { + content: []byte(` +- alert: 5 + expr: bar +`), + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 2, Last: 3}, + Error: parser.ParseError{Err: fmt.Errorf("alert value must be a YAML string, got integer instead"), Line: 2}, + }, + }, + }, + { + content: []byte(` +- record: foo + expr: 5 +`), + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 2, Last: 3}, + Error: parser.ParseError{Err: fmt.Errorf("expr value must be a YAML string, got integer instead"), Line: 3}, + }, + }, + }, + { + content: []byte(` +- alert: foo + expr: bar + for: 5 +`), + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 2, Last: 4}, + Error: parser.ParseError{Err: fmt.Errorf("for value must be a YAML string, got integer instead"), Line: 4}, + }, + }, + }, + { + content: []byte(` +- alert: foo + expr: bar + keep_firing_for: 5 +`), + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 2, Last: 4}, + Error: parser.ParseError{Err: fmt.Errorf("keep_firing_for value must be a YAML string, got integer instead"), Line: 4}, + }, + }, + }, + { + content: []byte(` +- record: foo + expr: bar + labels: [] +`), + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 2, Last: 4}, + Error: parser.ParseError{Err: fmt.Errorf("labels value must be a YAML mapping, got list instead"), Line: 4}, + }, + }, + }, + { + content: []byte(` +- alert: foo + expr: bar + labels: {} + annotations: [] +`), + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 2, Last: 5}, + Error: parser.ParseError{Err: fmt.Errorf("annotations value must be a YAML mapping, got list instead"), Line: 5}, + }, + }, + }, + { + content: []byte(` +- alert: foo + expr: bar + labels: + foo: 3 + annotations: + bar: "5" +`), + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 2, Last: 7}, + Error: parser.ParseError{Err: fmt.Errorf("labels foo value must be a YAML string, got integer instead"), Line: 5}, + }, + }, + }, + { + content: []byte(` +- alert: foo + expr: bar + labels: {} + annotations: + foo: "3" + bar: 5 +`), + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 2, Last: 7}, + Error: parser.ParseError{Err: fmt.Errorf("annotations bar value must be a YAML string, got integer instead"), Line: 7}, + }, + }, + }, + // Multi-document tests + { + content: []byte("---\n- expr: foo\n record: foo\n---\n- expr: bar\n"), + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 2, Last: 3}, + RecordingRule: &parser.RecordingRule{ + Record: parser.YamlNode{ + Lines: parser.LineRange{First: 3, Last: 3}, + Value: "foo", + }, + Expr: parser.PromQLExpr{ + Value: &parser.YamlNode{ + Lines: parser.LineRange{First: 2, Last: 2}, + Value: "foo", + }, + Query: &parser.PromQLNode{Expr: "foo"}, + }, + }, + }, + // FIXME + // { + // Lines: parser.LineRange{First: 2, Last: 2}, + // Error: parser.ParseError{Err: fmt.Errorf("incomplete rule, no alert or record key"), Line: 2}, + // }, + }, + }, } alwaysEqual := cmp.Comparer(func(_, _ interface{}) bool { return true }) @@ -1556,13 +1701,15 @@ data: for i, tc := range testCases { t.Run(strconv.Itoa(i+1), func(t *testing.T) { + t.Logf("--- Content ---%s--- END ---", tc.content) + p := parser.NewParser() output, err := p.Parse(tc.content) - hadError := err != nil - if hadError != tc.shouldError { - t.Errorf("Parse() returned err=%v, expected=%v", err, tc.shouldError) - return + if tc.err != "" { + require.EqualError(t, err, tc.err) + } else { + require.NoError(t, err) } if diff := cmp.Diff(tc.output, output, ignorePrometheusExpr, sameErrorText); diff != "" {