diff --git a/cmd/pint/tests/0207_locked_rule.txt b/cmd/pint/tests/0207_locked_rule.txt new file mode 100644 index 00000000..0bc587b2 --- /dev/null +++ b/cmd/pint/tests/0207_locked_rule.txt @@ -0,0 +1,35 @@ +! exec pint --no-color -l debug lint rules +! stdout . +cmp stderr stderr.txt + +-- stderr.txt -- +level=INFO msg="Loading configuration file" path=.pint.hcl +level=DEBUG msg="Adding pint config to the parser exclude list" path=.pint.hcl +level=INFO msg="Finding all rules to check" paths=["rules"] +level=DEBUG msg="File parsed" path=rules/0001.yaml rules=1 +level=DEBUG msg="Glob finder completed" count=1 +level=DEBUG msg="Generated all Prometheus servers" count=0 +level=DEBUG msg="Found recording rule" path=rules/0001.yaml record=colo_job:up:byinstance lines=6-7 state=noop +level=DEBUG msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","alerts/comparison","alerts/template","promql/fragile","promql/regexp","promql/aggregate(job:true)"] path=rules/0001.yaml rule=colo_job:up:byinstance +rules/0001.yaml:7 Bug: `job` label is required and should be preserved when aggregating `^.+$` rules, use `by(job, ...)`. (promql/aggregate) + 7 | expr: sum(byinstance) by(instance) + +level=INFO msg="Problems found" Bug=1 +level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher" +-- rules/0001.yaml -- +groups: +- name: foo + rules: + + # pint disable promql/aggregate(job:true) + - record: colo_job:up:byinstance + expr: sum(byinstance) by(instance) + +-- .pint.hcl -- +rule { + locked = true + aggregate ".+" { + keep = [ "job" ] + severity = "bug" + } +} diff --git a/docs/changelog.md b/docs/changelog.md index c2b545fb..c8fda6ff 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -15,6 +15,9 @@ } ``` +- Rules configured in `pint` config can now be locked - when a rule is locked it cannot + be disabled by users by adding a `# pint disable ...` or `# pint snooze ...` comments. + ### Fixed - The console reporter won't color the output if `--no-color` flag is set. diff --git a/docs/configuration.md b/docs/configuration.md index dcd65fee..5dbadc64 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -527,6 +527,7 @@ Syntax: ```js rule { + locked = true|false match { path = "(.+)" state = [ "any|added|modified|renamed|removed|unmodified", ... ] @@ -569,6 +570,8 @@ rule { } ``` +- `locked` - if set to `true` this rule will be locked, which means that it cannot be disabled using + `# pint disable ...` or `# pint snooze ...` comments. - `match:path` - only files matching this pattern will be checked by this rule. - `match:state` - only match rules based on their state. Default value for `state` depends on the pint command that is being run, for `pint ci` the default value is `["added", "modified", "renamed"]`, diff --git a/internal/config/__snapshots__/config_test.snap b/internal/config/__snapshots__/config_test.snap index 1109f4a7..0bfa3cac 100755 --- a/internal/config/__snapshots__/config_test.snap +++ b/internal/config/__snapshots__/config_test.snap @@ -3285,3 +3285,139 @@ ] } --- + +[TestGetChecksForRule/multiple_checks_and_disable_comment_/_locked_rule - 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", + "rule/report" + ] + }, + "owners": {}, + "rules": [ + { + "aggregate": [ + { + "name": ".+", + "severity": "bug", + "keep": [ + "job" + ] + } + ] + }, + { + "aggregate": [ + { + "name": ".+", + "comment": "this is rule comment", + "severity": "bug", + "strip": [ + "instance", + "rack" + ] + } + ], + "locked": true + } + ] +} +--- + +[TestGetChecksForRule/multiple_checks_and_snooze_comment_/_locked_rule - 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", + "rule/report" + ] + }, + "owners": {}, + "rules": [ + { + "aggregate": [ + { + "name": ".+", + "severity": "bug", + "keep": [ + "job" + ] + } + ] + }, + { + "aggregate": [ + { + "name": ".+", + "comment": "this is rule comment", + "severity": "bug", + "strip": [ + "instance", + "rack" + ] + } + ], + "locked": true + } + ] +} +--- diff --git a/internal/config/config.go b/internal/config/config.go index 1b8fb243..bc41c9ec 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -104,7 +104,7 @@ func (cfg *Config) GetChecksForEntry(ctx context.Context, gen *PrometheusGenerat if !isMatch(ctx, entry, pr.ignore, pr.match) { continue } - if pr.isEnabled(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry, cfg.Rules) { + if pr.isEnabled(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry, cfg.Rules, pr.locked) { enabled = append(enabled, pr.check) } } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 3ec48214..4f271bd5 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -2192,6 +2192,88 @@ rule { checks.ReportCheckName, }, }, + { + title: "multiple checks and disable comment / locked rule", + config: ` +rule { + locked = false + aggregate ".+" { + severity = "bug" + keep = ["job"] + } +} +rule { + locked = true + aggregate ".+" { + comment = "this is rule comment" + severity = "bug" + strip = ["instance", "rack"] + } +}`, + entry: discovery.Entry{ + State: discovery.Modified, + Path: discovery.Path{ + Name: "rules.yml", + SymlinkTarget: "rules.yml", + }, + Rule: newRule(t, ` +- record: foo + # pint disable promql/aggregate + expr: sum(foo) +`), + }, + checks: []string{ + checks.SyntaxCheckName, + checks.AlertForCheckName, + checks.ComparisonCheckName, + checks.TemplateCheckName, + checks.FragileCheckName, + checks.RegexpCheckName, + checks.AggregationCheckName + "(instance:false)", + checks.AggregationCheckName + "(rack:false)", + }, + }, + { + title: "multiple checks and snooze comment / locked rule", + config: ` +rule { + locked = false + aggregate ".+" { + severity = "bug" + keep = ["job"] + } +} +rule { + locked = true + aggregate ".+" { + comment = "this is rule comment" + severity = "bug" + strip = ["instance", "rack"] + } +}`, + entry: discovery.Entry{ + State: discovery.Modified, + Path: discovery.Path{ + Name: "rules.yml", + SymlinkTarget: "rules.yml", + }, + Rule: newRule(t, ` +- record: foo + # pint snooze 2099-11-28 promql/aggregate + expr: sum(foo) +`), + }, + checks: []string{ + checks.SyntaxCheckName, + checks.AlertForCheckName, + checks.ComparisonCheckName, + checks.TemplateCheckName, + checks.FragileCheckName, + checks.RegexpCheckName, + checks.AggregationCheckName + "(instance:false)", + checks.AggregationCheckName + "(rack:false)", + }, + }, } dir := t.TempDir() diff --git a/internal/config/parsed_rule.go b/internal/config/parsed_rule.go index 79bb1ee4..e7c1fcbd 100644 --- a/internal/config/parsed_rule.go +++ b/internal/config/parsed_rule.go @@ -16,6 +16,18 @@ type parsedRule struct { name string check checks.RuleChecker tags []string + locked bool +} + +func newParsedRule(rule Rule, defaultStates []string, name string, check checks.RuleChecker, tags []string) parsedRule { + return parsedRule{ + match: defaultRuleMatch(rule.Match, defaultStates), + ignore: rule.Ignore, + name: name, + check: check, + tags: tags, + locked: rule.Locked, + } } func isMatch(ctx context.Context, e discovery.Entry, ignore, match []Match) bool { @@ -41,14 +53,14 @@ func isMatch(ctx context.Context, e discovery.Entry, ignore, match []Match) bool return true } -func (rule parsedRule) isEnabled(ctx context.Context, enabled, disabled []string, checks []checks.RuleChecker, e discovery.Entry, cfgRules []Rule) bool { +func (rule parsedRule) isEnabled(ctx context.Context, enabled, disabled []string, checks []checks.RuleChecker, e discovery.Entry, cfgRules []Rule, locked bool) bool { // Entry state is not what the check is for. if !slices.Contains(rule.check.Meta().States, e.State) { return false } // Check if check is disabled for specific Prometheus rule. - if !isEnabled(enabled, e.DisabledChecks, e.Rule, rule.name, rule.check, rule.tags) { + if !isEnabled(enabled, e.DisabledChecks, e.Rule, rule.name, rule.check, rule.tags, locked) { return false } @@ -69,7 +81,7 @@ func (rule parsedRule) isEnabled(ctx context.Context, enabled, disabled []string } // Check if rule was disabled globally. - if !isEnabled(enabled, disabled, e.Rule, rule.name, rule.check, rule.tags) { + if !isEnabled(enabled, disabled, e.Rule, rule.name, rule.check, rule.tags, locked) { return false } // Check if rule was already enabled. @@ -216,20 +228,22 @@ func parseRule(rule Rule, prometheusServers []*promapi.FailoverGroup, defaultSta } severity := aggr.getSeverity(checks.Warning) for _, label := range aggr.Keep { - rules = append(rules, parsedRule{ - match: defaultRuleMatch(rule.Match, defaultStates), - ignore: rule.Ignore, - name: checks.AggregationCheckName, - check: checks.NewAggregationCheck(nameRegex, label, true, aggr.Comment, severity), - }) + rules = append(rules, newParsedRule( + rule, + defaultStates, + checks.AggregationCheckName, + checks.NewAggregationCheck(nameRegex, label, true, aggr.Comment, severity), + nil, + )) } for _, label := range aggr.Strip { - rules = append(rules, parsedRule{ - match: defaultRuleMatch(rule.Match, defaultStates), - ignore: rule.Ignore, - name: checks.AggregationCheckName, - check: checks.NewAggregationCheck(nameRegex, label, false, aggr.Comment, severity), - }) + rules = append(rules, newParsedRule( + rule, + defaultStates, + checks.AggregationCheckName, + checks.NewAggregationCheck(nameRegex, label, false, aggr.Comment, severity), + nil, + )) } } } @@ -238,13 +252,13 @@ func parseRule(rule Rule, prometheusServers []*promapi.FailoverGroup, defaultSta severity := rule.Cost.getSeverity(checks.Bug) evalDur, _ := parseDuration(rule.Cost.MaxEvaluationDuration) for _, prom := range prometheusServers { - rules = append(rules, parsedRule{ - match: defaultRuleMatch(rule.Match, defaultStates), - ignore: rule.Ignore, - name: checks.CostCheckName, - check: checks.NewCostCheck(prom, rule.Cost.MaxSeries, rule.Cost.MaxTotalSamples, rule.Cost.MaxPeakSamples, evalDur, rule.Cost.Comment, severity), - tags: prom.Tags(), - }) + rules = append(rules, newParsedRule( + rule, + defaultStates, + checks.CostCheckName, + checks.NewCostCheck(prom, rule.Cost.MaxSeries, rule.Cost.MaxTotalSamples, rule.Cost.MaxPeakSamples, evalDur, rule.Cost.Comment, severity), + prom.Tags(), + )) } } @@ -258,12 +272,13 @@ func parseRule(rule Rule, prometheusServers []*promapi.FailoverGroup, defaultSta valueRegex = checks.MustTemplatedRegexp(ann.Value) } severity := ann.getSeverity(checks.Warning) - rules = append(rules, parsedRule{ - match: defaultRuleMatch(rule.Match, defaultStates), - ignore: rule.Ignore, - name: checks.AnnotationCheckName, - check: checks.NewAnnotationCheck(checks.MustTemplatedRegexp(ann.Key), tokenRegex, valueRegex, ann.Values, ann.Required, ann.Comment, severity), - }) + rules = append(rules, newParsedRule( + rule, + defaultStates, + checks.AnnotationCheckName, + checks.NewAnnotationCheck(checks.MustTemplatedRegexp(ann.Key), tokenRegex, valueRegex, ann.Values, ann.Required, ann.Comment, severity), + nil, + )) } } @@ -277,12 +292,13 @@ func parseRule(rule Rule, prometheusServers []*promapi.FailoverGroup, defaultSta valueRegex = checks.MustTemplatedRegexp(lab.Value) } severity := lab.getSeverity(checks.Warning) - rules = append(rules, parsedRule{ - match: defaultRuleMatch(rule.Match, defaultStates), - ignore: rule.Ignore, - name: checks.LabelCheckName, - check: checks.NewLabelCheck(checks.MustTemplatedRegexp(lab.Key), tokenRegex, valueRegex, lab.Values, lab.Required, lab.Comment, severity), - }) + rules = append(rules, newParsedRule( + rule, + defaultStates, + checks.LabelCheckName, + checks.NewLabelCheck(checks.MustTemplatedRegexp(lab.Key), tokenRegex, valueRegex, lab.Values, lab.Required, lab.Comment, severity), + nil, + )) } } @@ -301,13 +317,13 @@ func parseRule(rule Rule, prometheusServers []*promapi.FailoverGroup, defaultSta } severity := rule.Alerts.getSeverity(checks.Information) for _, prom := range prometheusServers { - rules = append(rules, parsedRule{ - match: defaultRuleMatch(rule.Match, defaultStates), - ignore: rule.Ignore, - name: checks.AlertsCheckName, - check: checks.NewAlertsCheck(prom, qRange, qStep, qResolve, rule.Alerts.MinCount, rule.Alerts.Comment, severity), - tags: prom.Tags(), - }) + rules = append(rules, newParsedRule( + rule, + defaultStates, + checks.AlertsCheckName, + checks.NewAlertsCheck(prom, qRange, qStep, qResolve, rule.Alerts.MinCount, rule.Alerts.Comment, severity), + prom.Tags(), + )) } } @@ -316,36 +332,40 @@ func parseRule(rule Rule, prometheusServers []*promapi.FailoverGroup, defaultSta severity := reject.getSeverity(checks.Bug) re := checks.MustTemplatedRegexp(reject.Regex) if reject.LabelKeys { - rules = append(rules, parsedRule{ - match: defaultRuleMatch(rule.Match, defaultStates), - ignore: rule.Ignore, - name: checks.RejectCheckName, - check: checks.NewRejectCheck(true, false, re, nil, severity), - }) + rules = append(rules, newParsedRule( + rule, + defaultStates, + checks.RejectCheckName, + checks.NewRejectCheck(true, false, re, nil, severity), + nil, + )) } if reject.LabelValues { - rules = append(rules, parsedRule{ - match: defaultRuleMatch(rule.Match, defaultStates), - ignore: rule.Ignore, - name: checks.RejectCheckName, - check: checks.NewRejectCheck(true, false, nil, re, severity), - }) + rules = append(rules, newParsedRule( + rule, + defaultStates, + checks.RejectCheckName, + checks.NewRejectCheck(true, false, nil, re, severity), + nil, + )) } if reject.AnnotationKeys { - rules = append(rules, parsedRule{ - match: defaultRuleMatch(rule.Match, defaultStates), - ignore: rule.Ignore, - name: checks.RejectCheckName, - check: checks.NewRejectCheck(false, true, re, nil, severity), - }) + rules = append(rules, newParsedRule( + rule, + defaultStates, + checks.RejectCheckName, + checks.NewRejectCheck(false, true, re, nil, severity), + nil, + )) } if reject.AnnotationValues { - rules = append(rules, parsedRule{ - match: defaultRuleMatch(rule.Match, defaultStates), - ignore: rule.Ignore, - name: checks.RejectCheckName, - check: checks.NewRejectCheck(false, true, nil, re, severity), - }) + rules = append(rules, newParsedRule( + rule, + defaultStates, + checks.RejectCheckName, + checks.NewRejectCheck(false, true, nil, re, severity), + nil, + )) } } } @@ -359,63 +379,69 @@ func parseRule(rule Rule, prometheusServers []*promapi.FailoverGroup, defaultSta } else { timeout = time.Minute } - rules = append(rules, parsedRule{ - match: defaultRuleMatch(rule.Match, defaultStates), - ignore: rule.Ignore, - name: checks.RuleLinkCheckName, - check: checks.NewRuleLinkCheck(re, link.URI, timeout, link.Headers, link.Comment, severity), - }) + rules = append(rules, newParsedRule( + rule, + defaultStates, + checks.RuleLinkCheckName, + checks.NewRuleLinkCheck(re, link.URI, timeout, link.Headers, link.Comment, severity), + nil, + )) } if rule.For != nil { severity, minFor, maxFor := rule.For.resolve() - rules = append(rules, parsedRule{ - match: defaultRuleMatch(rule.Match, defaultStates), - ignore: rule.Ignore, - name: checks.RuleForCheckName, - check: checks.NewRuleForCheck(checks.RuleForFor, minFor, maxFor, rule.For.Comment, severity), - }) + rules = append(rules, newParsedRule( + rule, + defaultStates, + checks.RuleForCheckName, + checks.NewRuleForCheck(checks.RuleForFor, minFor, maxFor, rule.For.Comment, severity), + nil, + )) } if rule.KeepFiringFor != nil { severity, minFor, maxFor := rule.KeepFiringFor.resolve() - rules = append(rules, parsedRule{ - match: defaultRuleMatch(rule.Match, defaultStates), - ignore: rule.Ignore, - name: checks.RuleForCheckName, - check: checks.NewRuleForCheck(checks.RuleForKeepFiringFor, minFor, maxFor, rule.KeepFiringFor.Comment, severity), - }) + rules = append(rules, newParsedRule( + rule, + defaultStates, + checks.RuleForCheckName, + checks.NewRuleForCheck(checks.RuleForKeepFiringFor, minFor, maxFor, rule.KeepFiringFor.Comment, severity), + nil, + )) } for _, name := range rule.RuleName { re := checks.MustTemplatedRegexp(name.Regex) severity := name.getSeverity(checks.Information) - rules = append(rules, parsedRule{ - match: defaultRuleMatch(rule.Match, defaultStates), - ignore: rule.Ignore, - name: checks.RuleNameCheckName, - check: checks.NewRuleNameCheck(re, name.Comment, severity), - }) + rules = append(rules, newParsedRule( + rule, + defaultStates, + checks.RuleNameCheckName, + checks.NewRuleNameCheck(re, name.Comment, severity), + nil, + )) } if rule.RangeQuery != nil { severity := rule.RangeQuery.getSeverity(checks.Warning) limit, _ := parseDuration(rule.RangeQuery.Max) - rules = append(rules, parsedRule{ - match: defaultRuleMatch(rule.Match, defaultStates), - ignore: rule.Ignore, - name: checks.CostCheckName, - check: checks.NewRangeQueryCheck(nil, limit, rule.RangeQuery.Comment, severity), - }) + rules = append(rules, newParsedRule( + rule, + defaultStates, + checks.CostCheckName, + checks.NewRangeQueryCheck(nil, limit, rule.RangeQuery.Comment, severity), + nil, + )) } if rule.Report != nil { - rules = append(rules, parsedRule{ - match: defaultRuleMatch(rule.Match, defaultStates), - ignore: rule.Ignore, - name: checks.CostCheckName, - check: checks.NewReportCheck(rule.Report.Comment, rule.Report.getSeverity()), - }) + rules = append(rules, newParsedRule( + rule, + defaultStates, + checks.CostCheckName, + checks.NewReportCheck(rule.Report.Comment, rule.Report.getSeverity()), + nil, + )) } return rules diff --git a/internal/config/rule.go b/internal/config/rule.go index 63609a57..6c778eb7 100644 --- a/internal/config/rule.go +++ b/internal/config/rule.go @@ -28,6 +28,7 @@ type Rule struct { Reject []RejectSettings `hcl:"reject,block" json:"reject,omitempty"` RuleLink []RuleLinkSettings `hcl:"link,block" json:"link,omitempty"` RuleName []RuleNameSettings `hcl:"name,block" json:"name,omitempty"` + Locked bool `hcl:"locked,optional" json:"locked,omitempty"` } func (rule Rule) validate() (err error) { @@ -130,11 +131,7 @@ func (rule Rule) validate() (err error) { return nil } -func isEnabled(enabledChecks, disabledChecks []string, rule parser.Rule, name string, check checks.RuleChecker, promTags []string) bool { - if check.Meta().AlwaysEnabled { - return true - } - +func isDisabledForRule(rule parser.Rule, name string, check checks.RuleChecker, promTags []string) bool { matches := []string{ name, check.String(), @@ -142,7 +139,6 @@ func isEnabled(enabledChecks, disabledChecks []string, rule parser.Rule, name st for _, tag := range promTags { matches = append(matches, fmt.Sprintf("%s(+%s)", name, tag)) } - for _, disable := range comments.Only[comments.Disable](rule.Comments, comments.DisableType) { for _, match := range matches { if match == disable.Match { @@ -151,7 +147,7 @@ func isEnabled(enabledChecks, disabledChecks []string, rule parser.Rule, name st slog.String("check", check.String()), slog.String("match", match), ) - return false + return true } } } @@ -167,10 +163,21 @@ func isEnabled(enabledChecks, disabledChecks []string, rule parser.Rule, name st slog.String("match", snooze.Match), slog.Time("until", snooze.Until), ) - return false + return true } } } + return false +} + +func isEnabled(enabledChecks, disabledChecks []string, rule parser.Rule, name string, check checks.RuleChecker, promTags []string, locked bool) bool { + if check.Meta().AlwaysEnabled { + return true + } + + if !locked && isDisabledForRule(rule, name, check, promTags) { + return false + } for _, c := range disabledChecks { if c == name || c == check.String() {