diff --git a/.gitignore b/.gitignore index b69eb863..4988bd0c 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ /dist/ /.vscode/ /cmd/pint/bench/rules +/.idea diff --git a/cmd/pint/tests/0166_label_value_not_a_string.txt b/cmd/pint/tests/0166_label_value_not_a_string.txt new file mode 100644 index 00000000..03159ca0 --- /dev/null +++ b/cmd/pint/tests/0166_label_value_not_a_string.txt @@ -0,0 +1,34 @@ +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 Bug: alerting rule `colo:test1:alert` has label `job` with non-string value, got `!!bool`. (rule/label/value_type) + 7 | job: true + +rules/1.yaml:11 Bug: recording rule `colo:test1` has label `job` with non-string value, got `!!bool`. (rule/label/value_type) + 11 | job: true + +level=INFO msg="Problems found" Bug=2 +level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher" +-- rules/1.yaml -- +groups: +- name: foo + rules: + - alert: "colo:test1:alert" + expr: sum(foo) > 0 + labels: + job: true + - record: "colo:test1" + expr: sum(foo) + labels: + job: true + - record: "colo:test2" + expr: sum(foo) + labels: + job: "true" + - alert: "colo:test2:alert" + expr: sum(foo) > 0 + labels: + job: "true" diff --git a/internal/checks/base.go b/internal/checks/base.go index 39a1c5fe..89b91c51 100644 --- a/internal/checks/base.go +++ b/internal/checks/base.go @@ -32,6 +32,7 @@ var ( RuleDuplicateCheckName, RuleForCheckName, LabelCheckName, + RuleLabelValueTypeName, RuleLinkCheckName, RejectCheckName, } diff --git a/internal/checks/rule_label_value_type.go b/internal/checks/rule_label_value_type.go new file mode 100644 index 00000000..64fe7a50 --- /dev/null +++ b/internal/checks/rule_label_value_type.go @@ -0,0 +1,82 @@ +package checks + +import ( + "context" + "fmt" + + "github.com/cloudflare/pint/internal/discovery" + "github.com/cloudflare/pint/internal/parser" +) + +const ( + RuleLabelValueTypeName = "rule/label/value_type" +) + +func NewRuleLabelValueTypeCheck() RuleLabelValueTypeCheck { + return RuleLabelValueTypeCheck{} +} + +type RuleLabelValueTypeCheck struct{} + +func (c RuleLabelValueTypeCheck) Meta() CheckMeta { + return CheckMeta{ + States: []discovery.ChangeType{ + discovery.Noop, + discovery.Added, + discovery.Modified, + discovery.Moved, + }, + IsOnline: false, + } +} + +func (c RuleLabelValueTypeCheck) String() string { + return RuleLabelValueTypeName +} + +func (c RuleLabelValueTypeCheck) Reporter() string { + return RuleLabelValueTypeName +} + +func (c RuleLabelValueTypeCheck) Check(ctx context.Context, _ string, rule parser.Rule, _ []discovery.Entry) (problems []Problem) { + if rule.RecordingRule != nil { + problems = append(problems, c.checkRecordingRule(rule)...) + } + + if rule.AlertingRule != nil { + problems = append(problems, c.checkAlertingRule(rule)...) + } + + return problems +} + +func (c RuleLabelValueTypeCheck) checkRecordingRule(rule parser.Rule) (problems []Problem) { + if rule.RecordingRule.Labels == nil { + return problems + } + problems = append(problems, c.checkRuleLabelsValueType(rule.Name(), "recording", rule.RecordingRule.Labels, problems)...) + return problems +} + +func (c RuleLabelValueTypeCheck) checkAlertingRule(rule parser.Rule) (problems []Problem) { + if rule.AlertingRule.Labels == nil { + return problems + } + problems = append(problems, c.checkRuleLabelsValueType(rule.Name(), "alerting", rule.AlertingRule.Labels, problems)...) + return problems +} + +func (c RuleLabelValueTypeCheck) checkRuleLabelsValueType(ruleName, ruleType string, labels *parser.YamlMap, problems []Problem) []Problem { + for _, label := range labels.Items { + if label.Value.Tag != "!!str" { + problems = append(problems, Problem{ + Lines: label.Value.Lines, + Reporter: c.Reporter(), + Text: fmt.Sprintf("%s rule `%s` has label `%s` with non-string value, got `%s`.", ruleType, ruleName, label.Key.Value, label.Value.Tag), + Severity: Bug, + }) + } + + } + return problems +} diff --git a/internal/checks/rule_label_value_type_test.go b/internal/checks/rule_label_value_type_test.go new file mode 100644 index 00000000..429ae63c --- /dev/null +++ b/internal/checks/rule_label_value_type_test.go @@ -0,0 +1,86 @@ +package checks_test + +import ( + "testing" + + "github.com/cloudflare/pint/internal/checks" + "github.com/cloudflare/pint/internal/parser" + "github.com/cloudflare/pint/internal/promapi" +) + +func TestRuleLabelValueTypeCheck(t *testing.T) { + testCases := []checkTest{ + { + description: "label is not a string in recording rule / required", + content: "- record: foo\n expr: rate(foo[1m])\n labels:\n foo: true\n bar: 1\n", + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewRuleLabelValueTypeCheck() + }, + prometheus: noProm, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Lines: parser.LineRange{ + First: 4, + Last: 4, + }, + Reporter: checks.RuleLabelValueTypeName, + Text: "recording rule `foo` has label `foo` with non-string value, got `!!bool`.", + Severity: checks.Bug, + }, + { + Lines: parser.LineRange{ + First: 5, + Last: 5, + }, + Reporter: checks.RuleLabelValueTypeName, + Text: "recording rule `foo` has label `bar` with non-string value, got `!!int`.", + Severity: checks.Bug, + }, + } + }, + }, + { + description: "label is not a string in alerting rule / required", + content: "- alert: foo\n expr: rate(foo[1m]) > 0\n labels:\n foo: true\n", + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewRuleLabelValueTypeCheck() + }, + prometheus: noProm, + problems: func(uri string) []checks.Problem { + return []checks.Problem{{ + Lines: parser.LineRange{ + First: 4, + Last: 4, + }, + Reporter: checks.RuleLabelValueTypeName, + Text: "alerting rule `foo` has label `foo` with non-string value, got `!!bool`.", + Severity: checks.Bug, + }} + }, + }, + { + description: "label is a string in recording rule / required", + content: "- record: foo\n expr: rate(foo[1m]) > 0\n labels:\n foo: \"true\"\n", + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewRuleLabelValueTypeCheck() + }, + prometheus: noProm, + problems: func(uri string) []checks.Problem { + return nil + }, + }, + { + description: "label is a string in alerting rule / required", + content: "- alert: foo\n expr: rate(foo[1m]) > 0\n labels:\n foo: \"true\"\n", + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewRuleLabelValueTypeCheck() + }, + prometheus: noProm, + problems: func(uri string) []checks.Problem { + return nil + }, + }, + } + runTests(t, testCases) +} diff --git a/internal/config/config.go b/internal/config/config.go index 5b7599b2..b0f46d2f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -112,6 +112,10 @@ func (cfg *Config) GetChecksForRule(ctx context.Context, gen *PrometheusGenerato name: checks.RuleDependencyCheckName, check: checks.NewRuleDependencyCheck(), }, + { + name: checks.RuleLabelValueTypeName, + check: checks.NewRuleLabelValueTypeCheck(), + }, } proms := gen.ServersForPath(entry.SourcePath) diff --git a/internal/parser/models.go b/internal/parser/models.go index 37a7e64c..bf7d031e 100644 --- a/internal/parser/models.go +++ b/internal/parser/models.go @@ -44,6 +44,7 @@ func mergeComments(node *yaml.Node) (comments []string) { type YamlNode struct { Value string + Tag string Lines LineRange } @@ -64,6 +65,7 @@ func newYamlNode(node *yaml.Node, offset int) *YamlNode { n := YamlNode{ Lines: nodeLines(node, offset), Value: node.Value, + Tag: node.ShortTag(), } if node.Alias != nil { n.Value = node.Alias.Value