diff --git a/docs/changelog.md b/docs/changelog.md index 341042cd..8cca27a9 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -11,6 +11,27 @@ to disable reports on smelly selector - [#1096](https://github.com/cloudflare/pint/issues/1096). - Added `--json` flag to both `pint lint` and `pint ci` commands, this enables writing a JSON file with the report of all problems [#606](https://github.com/cloudflare/pint/issues/606). +- Checks can be enabled or disabled specifically for some Prometheus rules via `rule {}` config blocks. + Adding `enable` or `disable` option with a list of checks names allows to selectively enable or disable + checks only for Prometheus rules that match given `rule {}` definition. + Enabling checks only for matching rules will only work if these checks are disabled globally via + `check { disabled = [] }` config block. + For example to disable `promql/rate` check for all rules except alerting rules in the `rules/critical` folder: + + ```js + checks { + # This will disable promql/rate by default. + disabled = [ "promql/rate" ] + } + rule { + match { + path = "rules/critical/.*" + kind = "alerting" + } + # This will enable promql/rate only for Prometheus rules matching all our match conditions above. + enable = [ "promql/rate" ] + } + ``` ### Fixed diff --git a/docs/configuration.md b/docs/configuration.md index 1b7fba3e..006b8544 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -551,6 +551,9 @@ rule { ignore { ... } ignore { ... } + enable = [ "..." ] + disable = [ "..." ] + [ check definition ] ... [ check definition ] @@ -586,6 +589,11 @@ rule { way as `for` match filter. - `ignore` - works exactly like `match` but does the opposite - any alerting or recording rule matching all conditions defined on `ignore` will not be checked by this `rule` block. +- `enable` - list of check names to enable for any Prometheus rule matching this block. + Enabling checks here will overwrite `check { disable = [...] }` settings, but won't + enable checks disable specifically for some Prometheus rule via `# pint disable ...` comments. +- `disable` - list of check names to disable for any Prometheus rule matching this block. + This takes precedence over `enable` option above. Note: both `match` and `ignore` require all defined filters to be satisfied to work. If multiple `match` and/or `ignore` rules are present any of them needs to match for the rule to @@ -657,3 +665,20 @@ rule { check { ... } } ``` + +Disable `promql/rate` check for all rules except alerting rules in the `rules/critical` folder: + +```js +checks { + # This will disable promql/rate by default. + disabled = [ "promql/rate" ] +} +rule { + match { + path = "rules/critical/.*" + kind = "alerting" + } + # This will enable promql/rate only for Prometheus rules matching all our match conditions above. + enable = [ "promql/rate" ] +} +``` diff --git a/internal/config/__snapshots__/config_test.snap b/internal/config/__snapshots__/config_test.snap index 2b5f3404..cafacae9 100755 --- a/internal/config/__snapshots__/config_test.snap +++ b/internal/config/__snapshots__/config_test.snap @@ -2969,3 +2969,216 @@ ] } --- + +[TestGetChecksForRule/check_disabled_globally_but_enabled_via_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" + ], + "disabled": [ + "alerts/template", + "alerts/external_labels", + "rule/duplicate", + "alerts/absent", + "promql/series", + "promql/vector_matching" + ] + }, + "owners": {}, + "prometheus": [ + { + "name": "prom", + "uri": "http://localhost", + "timeout": "1s", + "uptime": "up", + "concurrency": 16, + "rateLimit": 100, + "required": false + } + ], + "rules": [ + { + "disable": [ + "rule/duplicate" + ] + }, + { + "match": [ + { + "kind": "alerting" + } + ], + "disable": [ + "promql/series" + ] + }, + { + "enable": [ + "promql/series" + ] + } + ] +} +--- + +[TestGetChecksForRule/check_enabled_globally_but_disabled_via_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" + ] + }, + "owners": {}, + "rules": [ + { + "match": [ + { + "kind": "recording" + } + ], + "disable": [ + "rule/duplicate" + ] + } + ] +} +--- + +[TestGetChecksForRule/two_prometheus_servers_/_check_disable_via_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" + ], + "disabled": [ + "alerts/template", + "promql/regexp" + ] + }, + "owners": {}, + "prometheus": [ + { + "name": "prom1", + "uri": "http://localhost/1", + "timeout": "1s", + "uptime": "up", + "concurrency": 16, + "rateLimit": 100, + "required": false + }, + { + "name": "prom2", + "uri": "http://localhost/2", + "timeout": "1s", + "uptime": "up", + "concurrency": 16, + "rateLimit": 100, + "required": false + } + ], + "rules": [ + { + "match": [ + { + "path": "rules.yml" + } + ], + "disable": [ + "promql/series", + "promql/range_query", + "rule/duplicate", + "promql/vector_matching", + "promql/counter" + ] + } + ] +} +--- diff --git a/internal/config/config.go b/internal/config/config.go index f1b91393..1b8fb243 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -86,21 +86,26 @@ func (cfg *Config) GetChecksForEntry(ctx context.Context, gen *PrometheusGenerat defaultMatch := []Match{{State: defaultStates}} proms := gen.ServersForPath(entry.Path.Name) + parsedRules := make([]parsedRule, 0, len(cfg.Rules)) if entry.PathError != nil || entry.Rule.Error.Err != nil { check := checks.NewErrorCheck(entry) - enabled = parsedRule{ + parsedRules = append(parsedRules, parsedRule{ match: defaultMatch, name: check.Reporter(), check: check, - }.entryChecks(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry) + }) } else { - for _, pr := range baseRules(proms, defaultMatch) { - enabled = pr.entryChecks(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry) - } + parsedRules = append(parsedRules, baseRules(proms, defaultMatch)...) for _, rule := range cfg.Rules { - for _, pr := range parseRule(rule, proms, defaultStates) { - enabled = pr.entryChecks(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry) - } + parsedRules = append(parsedRules, parseRule(rule, proms, defaultStates)...) + } + } + for _, pr := range parsedRules { + if !isMatch(ctx, entry, pr.ignore, pr.match) { + continue + } + if pr.isEnabled(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry, cfg.Rules) { + enabled = append(enabled, pr.check) } } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 7598bb6e..1a989741 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -2042,6 +2042,125 @@ rule { checks.AggregationCheckName + "(rack:false)", }, }, + { + title: "check disabled globally but enabled via rule{}", + config: ` +prometheus "prom" { + uri = "http://localhost" + timeout = "1s" +} +checks { + disabled = [ "alerts/template", "alerts/external_labels", "rule/duplicate", "alerts/absent", "promql/series", "promql/vector_matching" ] +} +rule { + disable = [ "rule/duplicate" ] +} +rule { + match { kind = "alerting" } + disable = [ "promql/series" ] +} +rule { + enable = [ "promql/series" ] +} +`, + entry: discovery.Entry{ + State: discovery.Modified, + Path: discovery.Path{ + Name: "rules.yml", + SymlinkTarget: "rules.yml", + }, + Rule: newRule(t, ` +- record: foo + expr: sum(foo) +`), + DisabledChecks: []string{"promql/rate", "promql/range_query"}, + }, + checks: []string{ + checks.SyntaxCheckName, + checks.AlertForCheckName, + checks.ComparisonCheckName, + checks.FragileCheckName, + checks.RegexpCheckName, + checks.SeriesCheckName + "(prom)", + checks.LabelsConflictCheckName + "(prom)", + checks.CounterCheckName + "(prom)", + }, + }, + { + title: "check enabled globally but disabled via rule{}", + config: ` +rule { + match { + kind = "recording" + } + disable = ["rule/duplicate"] +} +`, + entry: discovery.Entry{ + State: discovery.Modified, + Path: discovery.Path{ + Name: "rules.yml", + SymlinkTarget: "rules.yml", + }, + Rule: newRule(t, ` +- record: foo + expr: sum(foo) +`), + DisabledChecks: []string{"alerts/template", "alerts/external_labels", "alerts/absent"}, + }, + checks: []string{ + checks.SyntaxCheckName, + checks.AlertForCheckName, + checks.ComparisonCheckName, + checks.FragileCheckName, + checks.RegexpCheckName, + }, + }, + { + title: "two prometheus servers / check disable via rule {}", + config: ` +prometheus "prom1" { + uri = "http://localhost/1" + timeout = "1s" +} +prometheus "prom2" { + uri = "http://localhost/2" + timeout = "1s" +} +checks { + disabled = [ "alerts/template", "promql/regexp" ] +} +rule { + match { + path = "rules.yml" + } + disable = ["promql/series", "promql/range_query", "rule/duplicate", "promql/vector_matching", "promql/counter"] +} +`, + 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 alerts/absent + expr: sum(foo) +# pint file/disable promql/vector_matching +`), + DisabledChecks: []string{"promql/rate"}, + }, + checks: []string{ + checks.SyntaxCheckName, + checks.AlertForCheckName, + checks.ComparisonCheckName, + checks.FragileCheckName, + checks.LabelsConflictCheckName + "(prom1)", + checks.AlertsExternalLabelsCheckName + "(prom1)", + checks.LabelsConflictCheckName + "(prom2)", + checks.AlertsExternalLabelsCheckName + "(prom2)", + }, + }, } dir := t.TempDir() @@ -2418,6 +2537,18 @@ func TestConfigErrors(t *testing.T) { }`, err: "unknown rule state: foo", }, + { + config: `rule { + enable = ["bob"] +}`, + err: "unknown check name bob", + }, + { + config: `rule { + disable = ["bob"] +}`, + err: "unknown check name bob", + }, } dir := t.TempDir() diff --git a/internal/config/parsed_rule.go b/internal/config/parsed_rule.go index 568d1f2b..2c0aeb93 100644 --- a/internal/config/parsed_rule.go +++ b/internal/config/parsed_rule.go @@ -18,39 +18,59 @@ type parsedRule struct { tags []string } -func (rule parsedRule) entryChecks(ctx context.Context, enabled, disabled []string, checks []checks.RuleChecker, e discovery.Entry) []checks.RuleChecker { - for _, ignore := range rule.ignore { +func isMatch(ctx context.Context, e discovery.Entry, ignore, match []Match) bool { + for _, ignore := range ignore { if ignore.IsMatch(ctx, e.Path.Name, e) { - return checks + return false } } - if len(rule.match) > 0 { + if len(match) > 0 { var found bool - for _, match := range rule.match { + for _, match := range match { if match.IsMatch(ctx, e.Path.Name, e) { found = true break } } if !found { - return checks + return false } } + return true +} + +func (rule parsedRule) isEnabled(ctx context.Context, enabled, disabled []string, checks []checks.RuleChecker, e discovery.Entry, cfgRules []Rule) bool { // Entry state is not what the check is for. if !slices.Contains(rule.check.Meta().States, e.State) { - return checks + return false } - // Check if check is disabled for specific rule. + // Check if check is disabled for specific Prometheus rule. if !isEnabled(enabled, e.DisabledChecks, e.Rule, rule.name, rule.check, rule.tags) { - return checks + return false + } + + var enabledByConfigRule bool + for _, cfgRule := range cfgRules { + if !isMatch(ctx, e, cfgRule.Ignore, cfgRule.Match) { + continue + } + if slices.Contains(cfgRule.Disable, rule.name) { + return false + } + if slices.Contains(cfgRule.Enable, rule.name) { + enabledByConfigRule = true + } + } + if enabledByConfigRule { + return true } // Check if rule was disabled globally. if !isEnabled(enabled, disabled, e.Rule, rule.name, rule.check, rule.tags) { - return checks + return false } // Check if rule was already enabled. var v bool @@ -60,11 +80,7 @@ func (rule parsedRule) entryChecks(ctx context.Context, enabled, disabled []stri break } } - if !v { - checks = append(checks, rule.check) - } - - return checks + return !v } func defaultMatchStates(cmd ContextCommandVal) []string { diff --git a/internal/config/rule.go b/internal/config/rule.go index a8cb8f4d..9b8699e2 100644 --- a/internal/config/rule.go +++ b/internal/config/rule.go @@ -14,6 +14,8 @@ import ( type Rule struct { Match []Match `hcl:"match,block" json:"match,omitempty"` Ignore []Match `hcl:"ignore,block" json:"ignore,omitempty"` + Enable []string `hcl:"enable,optional" json:"enable,omitempty"` + Disable []string `hcl:"disable,optional" json:"disable,omitempty"` Aggregate []AggregateSettings `hcl:"aggregate,block" json:"aggregate,omitempty"` Annotation []AnnotationSettings `hcl:"annotation,block" json:"annotation,omitempty"` Label []AnnotationSettings `hcl:"label,block" json:"label,omitempty"` @@ -40,6 +42,18 @@ func (rule Rule) validate() (err error) { } } + for _, name := range rule.Enable { + if err = validateCheckName(name); err != nil { + return err + } + } + + for _, name := range rule.Disable { + if err = validateCheckName(name); err != nil { + return err + } + } + for _, aggr := range rule.Aggregate { if err = aggr.validate(); err != nil { return err