diff --git a/cmd/pint/tests/0025_config.txt b/cmd/pint/tests/0025_config.txt index 745ce5a2..ea589e7f 100644 --- a/cmd/pint/tests/0025_config.txt +++ b/cmd/pint/tests/0025_config.txt @@ -35,6 +35,7 @@ level=INFO msg="Loading configuration file" path=.pint.hcl "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" diff --git a/cmd/pint/tests/0113_config_env_expand.txt b/cmd/pint/tests/0113_config_env_expand.txt index 493364ae..a33214b4 100644 --- a/cmd/pint/tests/0113_config_env_expand.txt +++ b/cmd/pint/tests/0113_config_env_expand.txt @@ -40,6 +40,7 @@ level=INFO msg="Loading configuration file" path=.pint.hcl "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" diff --git a/cmd/pint/tests/0177_rule_name.txt b/cmd/pint/tests/0177_rule_name.txt new file mode 100644 index 00000000..affdf6b8 --- /dev/null +++ b/cmd/pint/tests/0177_rule_name.txt @@ -0,0 +1,28 @@ +pint.error --no-color lint rules +! stdout . +cmp stderr stderr.txt + +-- stderr.txt -- +level=INFO msg="Loading configuration file" path=.pint.hcl +level=INFO msg="Finding all rules to check" paths=["rules"] +rules/01.yml:4 Bug: alerting rule name must match `^rec:.+$`. (rule/name) + 4 | - alert: foo + +level=INFO msg="Problems found" Bug=1 +level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher" +-- rules/01.yml -- +groups: +- name: foo + rules: + - alert: foo + expr: up == 0 + - alert: rec:foo + expr: up == 0 + +-- .pint.hcl -- +rule { + name "rec:.+" { + severity = "bug" + comment = "must use rec: prefix" + } +} diff --git a/docs/changelog.md b/docs/changelog.md index 5f3c9693..e7dabe43 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -5,6 +5,7 @@ ### Added - Added [alerts/absent](checks/alerts/absent.md) check. +- Added [rule/name](checks/rule/name.md) check - #1020. ### Changed diff --git a/docs/checks/rule/name.md b/docs/checks/rule/name.md new file mode 100644 index 00000000..c22c475c --- /dev/null +++ b/docs/checks/rule/name.md @@ -0,0 +1,109 @@ +--- +layout: default +parent: Checks +grand_parent: Documentation +--- + +# rule/name + +This check allows to match rule names: + +- `alert` for alerting rules +- `record` for recording rules + +## Configuration + +Syntax: + +```js +name "$pattern" { + comment = "..." + severity = "bug|warning|info" +} +``` + +- `$pattern` - regexp pattern to match the name on, this can be templated + to reference checked rule fields, see [Configuration](../../configuration.md) + for details. +- `comment` - set a custom comment that will be added to reported problems. +- `severity` - set custom severity for reported issues, defaults to a information. + +## How to enable it + +This check is not enabled by default as it requires explicit configuration +to work. +To enable it add one or more `rule {...}` blocks and specify all required +labels there. + +Example that will require all recording rules to have `rec:` prefix: + +```js +rule { + match { + kind = "recording" + } + + label "rec:.+" { + comment = "ALl recording rules must use the `rec:` prefix." + severity = "bug" + } +} +``` + +## How to disable it + +You can disable this check globally by adding this config block: + +```js +checks { + disabled = ["rule/name"] +} +``` + +You can also disable it for all rules inside given file by adding +a comment anywhere in that file. Example: + +```yaml +# pint file/disable rule/name +``` + +Or you can disable it per rule by adding a comment to it. Example: + +```yaml +# pint disable rule/name +``` + +If you want to disable only individual instances of this check +you can add a more specific comment. + +```yaml +# pint disable rule/name($pattern) +``` + +Example pint rule: + +```js +label "rec:.+" { + comment = "ALl recording rules must use the `rec:` prefix." + severity = "bug" +} +``` + +Example comment disabling that rule: + +```yaml +# pint disable rule/name($rec:.+$) +``` + +## How to snooze it + +You can disable this check until given time by adding a comment to it. Example: + +```yaml +# pint snooze $TIMESTAMP rule/name +``` + +Where `$TIMESTAMP` is either use [RFC3339](https://www.rfc-editor.org/rfc/rfc3339) +formatted or `YYYY-MM-DD`. +Adding this comment will disable `rule/name` _until_ `$TIMESTAMP`, after that +check will be re-enabled. diff --git a/internal/checks/base.go b/internal/checks/base.go index 13300bdb..b4ded357 100644 --- a/internal/checks/base.go +++ b/internal/checks/base.go @@ -33,6 +33,7 @@ var ( RuleDependencyCheckName, RuleDuplicateCheckName, RuleForCheckName, + RuleNameCheckName, LabelCheckName, RuleLinkCheckName, RejectCheckName, diff --git a/internal/checks/rule_name.go b/internal/checks/rule_name.go new file mode 100644 index 00000000..a6be3ddd --- /dev/null +++ b/internal/checks/rule_name.go @@ -0,0 +1,69 @@ +package checks + +import ( + "context" + "fmt" + + "github.com/cloudflare/pint/internal/discovery" + "github.com/cloudflare/pint/internal/parser" +) + +const ( + RuleNameCheckName = "rule/name" +) + +func NewRuleNameCheck(re *TemplatedRegexp, comment string, severity Severity) RuleNameCheck { + return RuleNameCheck{ + re: re, + comment: comment, + severity: severity, + } +} + +type RuleNameCheck struct { + re *TemplatedRegexp + comment string + severity Severity +} + +func (c RuleNameCheck) Meta() CheckMeta { + return CheckMeta{ + States: []discovery.ChangeType{ + discovery.Noop, + discovery.Added, + discovery.Modified, + discovery.Moved, + }, + IsOnline: false, + } +} + +func (c RuleNameCheck) String() string { + return fmt.Sprintf("%s(%s)", RuleNameCheckName, c.re.anchored) +} + +func (c RuleNameCheck) Reporter() string { + return RuleNameCheckName +} + +func (c RuleNameCheck) Check(_ context.Context, _ discovery.Path, rule parser.Rule, _ []discovery.Entry) (problems []Problem) { + if rule.AlertingRule != nil && !c.re.MustExpand(rule).MatchString(rule.AlertingRule.Alert.Value) { + problems = append(problems, Problem{ + Lines: rule.AlertingRule.Alert.Lines, + Reporter: c.Reporter(), + Text: fmt.Sprintf("alerting rule name must match `%s`.", c.re.anchored), + Details: maybeComment(c.comment), + Severity: c.severity, + }) + } + if rule.RecordingRule != nil && !c.re.MustExpand(rule).MatchString(rule.RecordingRule.Record.Value) { + problems = append(problems, Problem{ + Lines: rule.RecordingRule.Record.Lines, + Reporter: c.Reporter(), + Text: fmt.Sprintf("recording rule name must match `%s`.", c.re.anchored), + Details: maybeComment(c.comment), + Severity: c.severity, + }) + } + return problems +} diff --git a/internal/checks/rule_name_test.go b/internal/checks/rule_name_test.go new file mode 100644 index 00000000..5ac508f3 --- /dev/null +++ b/internal/checks/rule_name_test.go @@ -0,0 +1,77 @@ +package checks_test + +import ( + "testing" + + "github.com/cloudflare/pint/internal/checks" + "github.com/cloudflare/pint/internal/parser" + "github.com/cloudflare/pint/internal/promapi" +) + +func TestRuleName(t *testing.T) { + testCases := []checkTest{ + { + description: "doesn't ignore rules with syntax errors", + content: "- record: foo\n expr: sum(foo) without(\n", + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewRuleNameCheck(checks.MustTemplatedRegexp("total:.+"), "some text", checks.Warning) + }, + prometheus: noProm, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Lines: parser.LineRange{ + First: 1, + Last: 1, + }, + Reporter: checks.RuleNameCheckName, + Text: "recording rule name must match `^total:.+$`.", + Details: "Rule comment: some text", + Severity: checks.Warning, + }, + } + }, + }, + { + description: "doesn't ignore rules with syntax errors", + content: "- record: total:foo\n expr: sum(foo) without(\n", + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewRuleNameCheck(checks.MustTemplatedRegexp("total:.+"), "some text", checks.Warning) + }, + prometheus: noProm, + problems: noProblems, + }, + { + description: "doesn't ignore rules with syntax errors", + content: "- alert: foo\n expr: sum(foo) without(\n", + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewRuleNameCheck(checks.MustTemplatedRegexp("total:.+"), "some text", checks.Warning) + }, + prometheus: noProm, + problems: func(_ string) []checks.Problem { + return []checks.Problem{ + { + Lines: parser.LineRange{ + First: 1, + Last: 1, + }, + Reporter: checks.RuleNameCheckName, + Text: "alerting rule name must match `^total:.+$`.", + Details: "Rule comment: some text", + Severity: checks.Warning, + }, + } + }, + }, + { + description: "doesn't ignore rules with syntax errors", + content: "- alert: total:foo\n expr: sum(foo) without(\n", + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewRuleNameCheck(checks.MustTemplatedRegexp("total:.+"), "some text", checks.Warning) + }, + prometheus: noProm, + problems: noProblems, + }, + } + runTests(t, testCases) +} diff --git a/internal/config/__snapshots__/config_test.snap b/internal/config/__snapshots__/config_test.snap index 2e1dff49..991ab14f 100755 --- a/internal/config/__snapshots__/config_test.snap +++ b/internal/config/__snapshots__/config_test.snap @@ -159,6 +159,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -198,6 +199,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -251,6 +253,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -307,6 +310,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -360,6 +364,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -402,6 +407,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -462,6 +468,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -523,6 +530,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -571,6 +579,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -668,6 +677,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -729,6 +739,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -789,6 +800,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -849,6 +861,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -909,6 +922,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -969,6 +983,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -1029,6 +1044,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -1083,6 +1099,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -1138,6 +1155,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -1192,6 +1210,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -1246,6 +1265,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -1300,6 +1320,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -1354,6 +1375,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -1408,6 +1430,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -1463,6 +1486,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -1517,6 +1541,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -1572,6 +1597,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -1632,6 +1658,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -1694,6 +1721,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -1744,6 +1772,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -1798,6 +1827,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -1851,6 +1881,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -1916,6 +1947,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -1986,6 +2018,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -2092,6 +2125,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -2155,6 +2189,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -2231,6 +2266,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -2290,6 +2326,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -2352,6 +2389,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -2415,6 +2453,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -2500,6 +2539,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -2564,6 +2604,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -2627,6 +2668,7 @@ "rule/dependency", "rule/duplicate", "rule/for", + "rule/name", "rule/label", "rule/link", "rule/reject" @@ -2672,3 +2714,52 @@ ] } --- + +[TestGetChecksForRule/name - 1] +{ + "ci": { + "baseBranch": "master", + "maxCommits": 20 + }, + "parser": {}, + "checks": { + "enabled": [ + "alerts/absent", + "alerts/annotation", + "alerts/count", + "alerts/external_labels", + "alerts/for", + "alerts/template", + "labels/conflict", + "promql/aggregate", + "alerts/comparison", + "promql/fragile", + "promql/range_query", + "promql/rate", + "promql/regexp", + "promql/syntax", + "promql/vector_matching", + "query/cost", + "promql/counter", + "promql/series", + "rule/dependency", + "rule/duplicate", + "rule/for", + "rule/name", + "rule/label", + "rule/link", + "rule/reject" + ] + }, + "owners": {}, + "rules": [ + { + "name": [ + { + "key": "total:.+" + } + ] + } + ] +} +--- diff --git a/internal/config/config_test.go b/internal/config/config_test.go index f5b488af..ab6b278b 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1546,6 +1546,31 @@ rule { checks.RuleLinkCheckName + "(^https?://(.+)$)", }, }, + { + title: "name", + config: ` +rule { + name "total:.+" {} +} +`, + entry: discovery.Entry{ + State: discovery.Modified, + Path: discovery.Path{ + Name: "rules.yml", + SymlinkTarget: "rules.yml", + }, + Rule: newRule(t, "- record: foo\n expr: sum(foo)\n"), + }, + checks: []string{ + checks.SyntaxCheckName, + checks.AlertForCheckName, + checks.ComparisonCheckName, + checks.TemplateCheckName, + checks.FragileCheckName, + checks.RegexpCheckName, + checks.RuleNameCheckName + "(^total:.+$)", + }, + }, { title: "two prometheus servers / disable checks via file/disable comment", config: ` @@ -2222,6 +2247,20 @@ func TestConfigErrors(t *testing.T) { }`, err: "prometheusQuery discovery requires at least one template", }, + { + config: `rule { + name "....+++" {} +}`, + err: "error parsing regexp: invalid nested repetition operator: `++`", + }, + { + config: `rule { + name "xxx" { + severity = "xxx" + } +}`, + err: "unknown severity: xxx", + }, } dir := t.TempDir() diff --git a/internal/config/rule.go b/internal/config/rule.go index 9eba89d2..7a29a0b0 100644 --- a/internal/config/rule.go +++ b/internal/config/rule.go @@ -25,6 +25,7 @@ type Rule struct { KeepFiringFor *ForSettings `hcl:"keep_firing_for,block" json:"keep_firing_for,omitempty"` Reject []RejectSettings `hcl:"reject,block" json:"reject,omitempty"` RuleLink []RuleLinkSettings `hcl:"link,block" json:"link,omitempty"` + RuleName []RuleNameSettings `hcl:"name,block" json:"name,omitempty"` } func (rule Rule) validate() (err error) { @@ -94,6 +95,12 @@ func (rule Rule) validate() (err error) { } } + for _, name := range rule.RuleName { + if err = name.validate(); err != nil { + return err + } + } + return nil } @@ -272,6 +279,15 @@ func (rule Rule) resolveChecks(ctx context.Context, path string, r parser.Rule, }) } + for _, name := range rule.RuleName { + re := checks.MustTemplatedRegexp(name.Regex) + severity := name.getSeverity(checks.Information) + enabled = append(enabled, checkMeta{ + name: checks.RuleNameCheckName, + check: checks.NewRuleNameCheck(re, name.Comment, severity), + }) + } + return enabled } diff --git a/internal/config/rule_name.go b/internal/config/rule_name.go new file mode 100644 index 00000000..46549c41 --- /dev/null +++ b/internal/config/rule_name.go @@ -0,0 +1,33 @@ +package config + +import ( + "github.com/cloudflare/pint/internal/checks" +) + +type RuleNameSettings struct { + Regex string `hcl:",label" json:"key,omitempty"` + Comment string `hcl:"comment,optional" json:"comment,omitempty"` + Severity string `hcl:"severity,optional" json:"severity,omitempty"` +} + +func (rs RuleNameSettings) validate() error { + if _, err := checks.NewTemplatedRegexp(rs.Regex); err != nil { + return err + } + + if rs.Severity != "" { + if _, err := checks.ParseSeverity(rs.Severity); err != nil { + return err + } + } + + return nil +} + +func (rs RuleNameSettings) getSeverity(fallback checks.Severity) checks.Severity { + if rs.Severity != "" { + sev, _ := checks.ParseSeverity(rs.Severity) + return sev + } + return fallback +}