From 8059efc554561b52825f4f339a725965a6272d38 Mon Sep 17 00:00:00 2001 From: Douglas Camata <159076+douglascamata@users.noreply.github.com> Date: Wed, 3 Jan 2024 17:35:56 +0100 Subject: [PATCH 1/5] Make LabelCheck assert values are strings --- internal/checks/rule_label.go | 9 +++++++++ internal/checks/rule_label_test.go | 26 ++++++++++++++++++++++++++ internal/parser/models.go | 2 ++ 3 files changed, 37 insertions(+) diff --git a/internal/checks/rule_label.go b/internal/checks/rule_label.go index 8ed2179a..c79c395f 100644 --- a/internal/checks/rule_label.go +++ b/internal/checks/rule_label.go @@ -96,6 +96,15 @@ func (c LabelCheck) checkRecordingRule(rule parser.Rule) (problems []Problem) { return problems } + if val.Tag == "!!bool" { + problems = append(problems, Problem{ + Lines: val.Lines, + Reporter: c.Reporter(), + Text: fmt.Sprintf("`%s` label value must be a string.", c.keyRe.original), + Severity: c.severity, + }) + } + if c.tokenRe != nil { for _, match := range c.tokenRe.MustExpand(rule).FindAllString(val.Value, -1) { problems = append(problems, c.checkValue(rule, match, val.Lines)...) diff --git a/internal/checks/rule_label_test.go b/internal/checks/rule_label_test.go index 77886cf4..248eef9a 100644 --- a/internal/checks/rule_label_test.go +++ b/internal/checks/rule_label_test.go @@ -89,6 +89,32 @@ func TestLabelCheck(t *testing.T) { } }, }, + { + description: "label is not a string in recording rule / required", + content: "- record: foo\n expr: rate(foo[1m])\n labels:\n foo: true\n", + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewLabelCheck( + checks.MustTemplatedRegexp("foo"), + checks.MustRawTemplatedRegexp("\\w+"), + checks.MustTemplatedRegexp(".*"), + nil, + true, + checks.Bug, + ) + }, + prometheus: noProm, + problems: func(uri string) []checks.Problem { + return []checks.Problem{{ + Lines: parser.LineRange{ + First: 4, + Last: 4, + }, + Reporter: checks.LabelCheckName, + Text: "`foo` label value must be a string.", + Severity: checks.Bug, + }} + }, + }, { description: "missing label in recording rule / not required", content: "- record: foo\n expr: rate(foo[1m])\n labels:\n foo: bar\n", 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 From 570893d27dfb81c4537b925ae675fd6ee92f4e72 Mon Sep 17 00:00:00 2001 From: Douglas Camata <159076+douglascamata@users.noreply.github.com> Date: Wed, 3 Jan 2024 17:36:10 +0100 Subject: [PATCH 2/5] Add .idea to gitignore --- .gitignore | 1 + 1 file changed, 1 insertion(+) 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 From 682f79ff1dbb8b5449693166cb3d0510e4d08d60 Mon Sep 17 00:00:00 2001 From: Douglas Camata <159076+douglascamata@users.noreply.github.com> Date: Wed, 3 Jan 2024 20:23:36 +0100 Subject: [PATCH 3/5] Refactor solution into a different checker --- .../tests/0166_label_value_not_a_string.txt | 34 ++++++++ internal/checks/base.go | 1 + internal/checks/rule_label.go | 9 -- internal/checks/rule_label_test.go | 26 ------ internal/checks/rule_label_value_type.go | 82 +++++++++++++++++++ internal/checks/rule_label_value_type_test.go | 34 ++++++++ internal/config/config.go | 4 + 7 files changed, 155 insertions(+), 35 deletions(-) create mode 100644 cmd/pint/tests/0166_label_value_not_a_string.txt create mode 100644 internal/checks/rule_label_value_type.go create mode 100644 internal/checks/rule_label_value_type_test.go 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.go b/internal/checks/rule_label.go index c79c395f..8ed2179a 100644 --- a/internal/checks/rule_label.go +++ b/internal/checks/rule_label.go @@ -96,15 +96,6 @@ func (c LabelCheck) checkRecordingRule(rule parser.Rule) (problems []Problem) { return problems } - if val.Tag == "!!bool" { - problems = append(problems, Problem{ - Lines: val.Lines, - Reporter: c.Reporter(), - Text: fmt.Sprintf("`%s` label value must be a string.", c.keyRe.original), - Severity: c.severity, - }) - } - if c.tokenRe != nil { for _, match := range c.tokenRe.MustExpand(rule).FindAllString(val.Value, -1) { problems = append(problems, c.checkValue(rule, match, val.Lines)...) diff --git a/internal/checks/rule_label_test.go b/internal/checks/rule_label_test.go index 248eef9a..77886cf4 100644 --- a/internal/checks/rule_label_test.go +++ b/internal/checks/rule_label_test.go @@ -89,32 +89,6 @@ func TestLabelCheck(t *testing.T) { } }, }, - { - description: "label is not a string in recording rule / required", - content: "- record: foo\n expr: rate(foo[1m])\n labels:\n foo: true\n", - checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck( - checks.MustTemplatedRegexp("foo"), - checks.MustRawTemplatedRegexp("\\w+"), - checks.MustTemplatedRegexp(".*"), - nil, - true, - checks.Bug, - ) - }, - prometheus: noProm, - problems: func(uri string) []checks.Problem { - return []checks.Problem{{ - Lines: parser.LineRange{ - First: 4, - Last: 4, - }, - Reporter: checks.LabelCheckName, - Text: "`foo` label value must be a string.", - Severity: checks.Bug, - }} - }, - }, { description: "missing label in recording rule / not required", content: "- record: foo\n expr: rate(foo[1m])\n labels:\n foo: bar\n", diff --git a/internal/checks/rule_label_value_type.go b/internal/checks/rule_label_value_type.go new file mode 100644 index 00000000..df5186f7 --- /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 == "!!bool" { + 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..42004847 --- /dev/null +++ b/internal/checks/rule_label_value_type_test.go @@ -0,0 +1,34 @@ +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", + 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: "`foo` label value must be a string, got `!!bool`.", + Severity: checks.Bug, + }} + }, + }, + } + 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) From 586e1f5ab3531f689271f3502772ebd608910002 Mon Sep 17 00:00:00 2001 From: Douglas Camata <159076+douglascamata@users.noreply.github.com> Date: Wed, 3 Jan 2024 20:27:09 +0100 Subject: [PATCH 4/5] Add more unit tests --- internal/checks/rule_label_value_type_test.go | 43 ++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/internal/checks/rule_label_value_type_test.go b/internal/checks/rule_label_value_type_test.go index 42004847..b4f04aba 100644 --- a/internal/checks/rule_label_value_type_test.go +++ b/internal/checks/rule_label_value_type_test.go @@ -24,11 +24,52 @@ func TestRuleLabelValueTypeCheck(t *testing.T) { Last: 4, }, Reporter: checks.RuleLabelValueTypeName, - Text: "`foo` label value must be a string, got `!!bool`.", + Text: "recording rule `foo` has label `foo` with non-string value, got `!!bool`.", 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) } From 536e08536e5a5a46cc96c0a7554421c9c9c6183a Mon Sep 17 00:00:00 2001 From: Douglas Camata <159076+douglascamata@users.noreply.github.com> Date: Wed, 3 Jan 2024 20:36:33 +0100 Subject: [PATCH 5/5] Fix tag check --- internal/checks/rule_label_value_type.go | 2 +- internal/checks/rule_label_value_type_test.go | 29 +++++++++++++------ 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/internal/checks/rule_label_value_type.go b/internal/checks/rule_label_value_type.go index df5186f7..64fe7a50 100644 --- a/internal/checks/rule_label_value_type.go +++ b/internal/checks/rule_label_value_type.go @@ -68,7 +68,7 @@ func (c RuleLabelValueTypeCheck) checkAlertingRule(rule parser.Rule) (problems [ func (c RuleLabelValueTypeCheck) checkRuleLabelsValueType(ruleName, ruleType string, labels *parser.YamlMap, problems []Problem) []Problem { for _, label := range labels.Items { - if label.Value.Tag == "!!bool" { + if label.Value.Tag != "!!str" { problems = append(problems, Problem{ Lines: label.Value.Lines, Reporter: c.Reporter(), diff --git a/internal/checks/rule_label_value_type_test.go b/internal/checks/rule_label_value_type_test.go index b4f04aba..429ae63c 100644 --- a/internal/checks/rule_label_value_type_test.go +++ b/internal/checks/rule_label_value_type_test.go @@ -12,21 +12,32 @@ 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", + 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, + 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, }, - 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, + }, + } }, }, {