From 647f6e65c54c3e18e04c7c7f2d9fc046bfdb3c45 Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Mon, 17 Jan 2022 13:11:50 +0000 Subject: [PATCH] Refactor config loading --- internal/checks/query_cost.go | 3 + internal/config/config.go | 99 ++++++++--------- internal/config/config_test.go | 194 +++++++++++++++++++++++++++++++++ internal/config/reject.go | 5 - internal/config/reject_test.go | 1 - internal/config/rule.go | 87 +++++++++------ 6 files changed, 300 insertions(+), 89 deletions(-) diff --git a/internal/checks/query_cost.go b/internal/checks/query_cost.go index de249924..9aafccdd 100644 --- a/internal/checks/query_cost.go +++ b/internal/checks/query_cost.go @@ -28,6 +28,9 @@ type CostCheck struct { } func (c CostCheck) String() string { + if c.maxSeries > 0 { + return fmt.Sprintf("%s(%s:%d)", CostCheckName, c.prom.Name(), c.maxSeries) + } return fmt.Sprintf("%s(%s)", CostCheckName, c.prom.Name()) } diff --git a/internal/config/config.go b/internal/config/config.go index 82a7a895..d8701b06 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -4,7 +4,6 @@ import ( "encoding/json" "fmt" "os" - "strings" "time" "github.com/cloudflare/pint/internal/checks" @@ -71,20 +70,23 @@ func (cfg Config) String() string { func (cfg *Config) GetChecksForRule(path string, r parser.Rule) []checks.RuleChecker { enabled := []checks.RuleChecker{} - if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.SyntaxCheckName, r, nil) { - enabled = append(enabled, checks.NewSyntaxCheck()) - } - - if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.AlertForCheckName, r, nil) { - enabled = append(enabled, checks.NewAlertsForCheck()) - } - - if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.ComparisonCheckName, r, nil) { - enabled = append(enabled, checks.NewComparisonCheck()) - } - - if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.TemplateCheckName, r, nil) { - enabled = append(enabled, checks.NewTemplateCheck()) + allChecks := []checkMeta{ + { + name: checks.SyntaxCheckName, + check: checks.NewSyntaxCheck(), + }, + { + name: checks.AlertForCheckName, + check: checks.NewAlertsForCheck(), + }, + { + name: checks.ComparisonCheckName, + check: checks.NewComparisonCheck(), + }, + { + name: checks.TemplateCheckName, + check: checks.NewTemplateCheck(), + }, } proms := []*promapi.Prometheus{} @@ -100,47 +102,41 @@ func (cfg *Config) GetChecksForRule(path string, r parser.Rule) []checks.RuleChe } } - for _, prom := range proms { - if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.RateCheckName, r, prom) { - enabled = append(enabled, checks.NewRateCheck(prom)) - } + for _, p := range proms { + allChecks = append(allChecks, checkMeta{ + name: checks.RateCheckName, + check: checks.NewRateCheck(p), + }) + allChecks = append(allChecks, checkMeta{ + name: checks.SeriesCheckName, + check: checks.NewSeriesCheck(p), + }) + allChecks = append(allChecks, checkMeta{ + name: checks.VectorMatchingCheckName, + check: checks.NewVectorMatchingCheck(p), + }) } - for _, prom := range proms { - if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.SeriesCheckName, r, prom) { - - enabled = append(enabled, checks.NewSeriesCheck(prom)) - } + for _, rule := range cfg.Rules { + allChecks = append(allChecks, rule.resolveChecks(path, r, cfg.Checks.Enabled, cfg.Checks.Disabled, proms)...) } - for _, prom := range proms { - if isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, checks.VectorMatchingCheckName, r, prom) { - enabled = append(enabled, checks.NewVectorMatchingCheck(prom)) + for _, cm := range allChecks { + // check if rule was disabled + if !isEnabled(cfg.Checks.Enabled, cfg.Checks.Disabled, r, cm.name, cm.check) { + continue } - } - - for _, rule := range cfg.Rules { - for _, c := range rule.resolveChecks(path, r, cfg.Checks.Enabled, cfg.Checks.Disabled, proms) { - // FIXME redundant ? - if r.HasComment(fmt.Sprintf("disable %s", removeRedundantSpaces(c.String()))) { - log.Debug(). - Str("path", path). - Str("check", c.String()). - Msg("Check disabled by comment") - continue - } - // check if rule was already enabled - var v bool - for _, er := range enabled { - if er.String() == c.String() { - v = true - break - } - } - if !v { - enabled = append(enabled, c) + // check if rule was already enabled + var v bool + for _, er := range enabled { + if er.String() == cm.check.String() { + v = true + break } } + if !v { + enabled = append(enabled, cm.check) + } } el := []string{} @@ -272,6 +268,7 @@ func parseDuration(d string) (time.Duration, error) { return time.Duration(mdur), nil } -func removeRedundantSpaces(line string) string { - return strings.Join(strings.Fields(line), " ") +type checkMeta struct { + name string + check checks.RuleChecker } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index af885b70..aa497e70 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -288,6 +288,8 @@ rule { rule { cost { bytesPerSample = 4096 + maxSeries = 10000 + severity = "warning" } } `, @@ -377,6 +379,198 @@ rule { checks.LabelCheckName + "(team:false)", }, }, + { + title: "multiple cost checks", + config: ` +prometheus "prom1" { + uri = "http://localhost" + timeout = "1s" + paths = [ "rules.yml" ] +} +prometheus "prom2" { + uri = "http://localhost" + timeout = "1s" + paths = [ "rules.yml" ] +} +rule { + cost { + bytesPerSample = 4096 + severity = "info" + } +} +rule { + cost { + bytesPerSample = 4096 + maxSeries = 10000 + severity = "warning" + } +} +rule { + cost { + bytesPerSample = 4096 + maxSeries = 20000 + severity = "bug" + } +} +`, + path: "rules.yml", + rule: newRule(t, ` +# pint disable query/series +# pint disable promql/rate +# pint disable promql/vector_matching(prom1) +# pint disable promql/vector_matching(prom2) +- record: foo + expr: sum(foo) +`), + checks: []string{ + checks.SyntaxCheckName, + checks.AlertForCheckName, + checks.ComparisonCheckName, + checks.TemplateCheckName, + checks.CostCheckName + "(prom1)", + checks.CostCheckName + "(prom2)", + checks.CostCheckName + "(prom1:10000)", + checks.CostCheckName + "(prom2:10000)", + checks.CostCheckName + "(prom1:20000)", + checks.CostCheckName + "(prom2:20000)", + }, + }, + { + title: "reject rules", + config: ` +rule { + reject "http://.+" { + label_keys = true + label_values = true + } + reject ".* +.*" { + annotation_keys = true + label_keys = true + } + reject "" { + annotation_values = true + severity = "bug" + } +} +`, + path: "rules.yml", + rule: newRule(t, "- record: foo\n expr: sum(foo)\n"), + checks: []string{ + checks.SyntaxCheckName, + checks.AlertForCheckName, + checks.ComparisonCheckName, + checks.TemplateCheckName, + checks.RejectCheckName + "(key=~'^http://.+$')", + checks.RejectCheckName + "(val=~'^http://.+$')", + checks.RejectCheckName + "(key=~'^.* +.*$')", + checks.RejectCheckName + "(val=~'^$')", + }, + }, + { + title: "rule with label match / type mismatch", + config: ` +rule { + match { + kind = "alerting" + label "cluster" { + value = "prod" + } + } + label "priority" { + severity = "bug" + value = "(1|2|3|4|5)" + required = true + } +} +`, + path: "rules.yml", + rule: newRule(t, "- record: foo\n expr: sum(foo)\n"), + checks: []string{ + checks.SyntaxCheckName, + checks.AlertForCheckName, + checks.ComparisonCheckName, + checks.TemplateCheckName, + }, + }, + { + title: "rule with label match / no label", + config: ` +rule { + match { + kind = "alerting" + label "cluster" { + value = "prod" + } + } + label "priority" { + severity = "bug" + value = "(1|2|3|4|5)" + required = true + } +} +`, + path: "rules.yml", + rule: newRule(t, "- alert: foo\n expr: sum(foo)\n"), + checks: []string{ + checks.SyntaxCheckName, + checks.AlertForCheckName, + checks.ComparisonCheckName, + checks.TemplateCheckName, + }, + }, + { + title: "rule with label match / label mismatch", + config: ` +rule { + match { + kind = "alerting" + label "cluster" { + value = "prod" + } + } + label "priority" { + severity = "bug" + value = "(1|2|3|4|5)" + required = true + } +} +`, + path: "rules.yml", + rule: newRule(t, "- alert: foo\n expr: sum(foo)\n labels:\n cluster: dev\n"), + checks: []string{ + checks.SyntaxCheckName, + checks.AlertForCheckName, + checks.ComparisonCheckName, + checks.TemplateCheckName, + }, + }, + { + title: "rule with label match / label match", + config: ` +rule { + match { + kind = "alerting" + label "cluster" { + value = "prod" + } + } + label "priority" { + severity = "bug" + value = "(1|2|3|4|5)" + required = true + } +} +`, + path: "rules.yml", + rule: newRule(t, "- alert: foo\n expr: sum(foo)\n labels:\n cluster: prod\n"), + checks: []string{ + checks.SyntaxCheckName, + checks.AlertForCheckName, + checks.ComparisonCheckName, + checks.TemplateCheckName, + checks.LabelCheckName + "(priority:true)", + }, + }, } dir := t.TempDir() diff --git a/internal/config/reject.go b/internal/config/reject.go index 20880b8b..2d82ed94 100644 --- a/internal/config/reject.go +++ b/internal/config/reject.go @@ -1,7 +1,6 @@ package config import ( - "errors" "regexp" "github.com/cloudflare/pint/internal/checks" @@ -17,10 +16,6 @@ type RejectSettings struct { } func (rs RejectSettings) validate() error { - if rs.Regex == "" { - return errors.New("reject key must be set") - } - if _, err := regexp.Compile(rs.Regex); err != nil { return err } diff --git a/internal/config/reject_test.go b/internal/config/reject_test.go index 9576ae16..bc4bddeb 100644 --- a/internal/config/reject_test.go +++ b/internal/config/reject_test.go @@ -23,7 +23,6 @@ func TestRejectSettings(t *testing.T) { }, { conf: RejectSettings{}, - err: errors.New("reject key must be set"), }, { conf: RejectSettings{ diff --git a/internal/config/rule.go b/internal/config/rule.go index 5398a0b9..26bc246a 100644 --- a/internal/config/rule.go +++ b/internal/config/rule.go @@ -156,8 +156,8 @@ type Rule struct { Reject []RejectSettings `hcl:"reject,block" json:"reject,omitempty"` } -func (rule Rule) resolveChecks(path string, r parser.Rule, enabledChecks, disabledChecks []string, prometheusServers []*promapi.Prometheus) []checks.RuleChecker { - enabled := []checks.RuleChecker{} +func (rule Rule) resolveChecks(path string, r parser.Rule, enabledChecks, disabledChecks []string, prometheusServers []*promapi.Prometheus) []checkMeta { + enabled := []checkMeta{} if rule.Ignore != nil && rule.Ignore.IsMatch(path, r) { return enabled @@ -175,47 +175,59 @@ func (rule Rule) resolveChecks(path string, r parser.Rule, enabledChecks, disabl } severity := aggr.getSeverity(checks.Warning) for _, label := range aggr.Keep { - if isEnabled(enabledChecks, disabledChecks, checks.AggregationCheckName, r, nil) { - enabled = append(enabled, checks.NewAggregationCheck(nameRegex, label, true, severity)) - } + enabled = append(enabled, checkMeta{ + name: checks.AggregationCheckName, + check: checks.NewAggregationCheck(nameRegex, label, true, severity), + }) } for _, label := range aggr.Strip { - if isEnabled(enabledChecks, disabledChecks, checks.AggregationCheckName, r, nil) { - enabled = append(enabled, checks.NewAggregationCheck(nameRegex, label, false, severity)) - } + enabled = append(enabled, checkMeta{ + name: checks.AggregationCheckName, + check: checks.NewAggregationCheck(nameRegex, label, false, severity), + }) } } } - if rule.Cost != nil && isEnabled(enabledChecks, disabledChecks, checks.CostCheckName, r, nil) { + if rule.Cost != nil { severity := rule.Cost.getSeverity(checks.Bug) for _, prom := range prometheusServers { - enabled = append(enabled, checks.NewCostCheck(prom, rule.Cost.BytesPerSample, rule.Cost.MaxSeries, severity)) + enabled = append(enabled, checkMeta{ + name: checks.CostCheckName, + check: checks.NewCostCheck(prom, rule.Cost.BytesPerSample, rule.Cost.MaxSeries, severity), + }) } } - if len(rule.Annotation) > 0 && isEnabled(enabledChecks, disabledChecks, checks.AnnotationCheckName, r, nil) { + if len(rule.Annotation) > 0 { for _, ann := range rule.Annotation { var valueRegex *regexp.Regexp if ann.Value != "" { valueRegex = strictRegex(ann.Value) } severity := ann.getSeverity(checks.Warning) - enabled = append(enabled, checks.NewAnnotationCheck(ann.Key, valueRegex, ann.Required, severity)) + enabled = append(enabled, checkMeta{ + name: checks.AnnotationCheckName, + check: checks.NewAnnotationCheck(ann.Key, valueRegex, ann.Required, severity), + }) } } - if len(rule.Label) > 0 && isEnabled(enabledChecks, disabledChecks, checks.LabelCheckName, r, nil) { + + if len(rule.Label) > 0 { for _, lab := range rule.Label { var valueRegex *regexp.Regexp if lab.Value != "" { valueRegex = strictRegex(lab.Value) } severity := lab.getSeverity(checks.Warning) - enabled = append(enabled, checks.NewLabelCheck(lab.Key, valueRegex, lab.Required, severity)) + enabled = append(enabled, checkMeta{ + name: checks.LabelCheckName, + check: checks.NewLabelCheck(lab.Key, valueRegex, lab.Required, severity), + }) } } - if rule.Alerts != nil && isEnabled(enabledChecks, disabledChecks, checks.AlertsCheckName, r, nil) { + if rule.Alerts != nil { qRange := time.Hour * 24 if rule.Alerts.Range != "" { qRange, _ = parseDuration(rule.Alerts.Range) @@ -229,28 +241,40 @@ func (rule Rule) resolveChecks(path string, r parser.Rule, enabledChecks, disabl qResolve, _ = parseDuration(rule.Alerts.Resolve) } for _, prom := range prometheusServers { - enabled = append(enabled, checks.NewAlertsCheck(prom, qRange, qStep, qResolve)) + enabled = append(enabled, checkMeta{ + name: checks.AlertsCheckName, + check: checks.NewAlertsCheck(prom, qRange, qStep, qResolve), + }) } } - if len(rule.Reject) > 0 && isEnabled(enabledChecks, disabledChecks, checks.RejectCheckName, r, nil) { + if len(rule.Reject) > 0 { for _, reject := range rule.Reject { severity := reject.getSeverity(checks.Bug) + re := strictRegex(reject.Regex) if reject.LabelKeys { - re := strictRegex(reject.Regex) - enabled = append(enabled, checks.NewRejectCheck(true, false, re, nil, severity)) + enabled = append(enabled, checkMeta{ + name: checks.RejectCheckName, + check: checks.NewRejectCheck(true, false, re, nil, severity), + }) } if reject.LabelValues { - re := strictRegex(reject.Regex) - enabled = append(enabled, checks.NewRejectCheck(true, false, nil, re, severity)) + enabled = append(enabled, checkMeta{ + name: checks.RejectCheckName, + check: checks.NewRejectCheck(true, false, nil, re, severity), + }) } if reject.AnnotationKeys { - re := strictRegex(reject.Regex) - enabled = append(enabled, checks.NewRejectCheck(false, true, re, nil, severity)) + enabled = append(enabled, checkMeta{ + name: checks.RejectCheckName, + check: checks.NewRejectCheck(false, true, re, nil, severity), + }) } if reject.AnnotationValues { - re := strictRegex(reject.Regex) - enabled = append(enabled, checks.NewRejectCheck(false, true, nil, re, severity)) + enabled = append(enabled, checkMeta{ + name: checks.RejectCheckName, + check: checks.NewRejectCheck(false, true, nil, re, severity), + }) } } } @@ -258,17 +282,16 @@ func (rule Rule) resolveChecks(path string, r parser.Rule, enabledChecks, disabl return enabled } -func isEnabled(enabledChecks, disabledChecks []string, name string, rule parser.Rule, prom *promapi.Prometheus) bool { +func isEnabled(enabledChecks, disabledChecks []string, rule parser.Rule, name string, check checks.RuleChecker) bool { + instance := check.String() comments := []string{ - fmt.Sprintf("disable %s", removeRedundantSpaces(name)), - } - if prom != nil { - comments = append(comments, fmt.Sprintf("disable %s(%s)", removeRedundantSpaces(name), prom.Name())) + fmt.Sprintf("disable %s", name), + fmt.Sprintf("disable %s", instance), } for _, comment := range comments { if rule.HasComment(comment) { log.Debug(). - Str("check", name). + Str("check", instance). Str("comment", comment). Msg("Check disabled by comment") return false @@ -276,7 +299,7 @@ func isEnabled(enabledChecks, disabledChecks []string, name string, rule parser. } for _, c := range disabledChecks { - if c == name { + if c == name || c == instance { return false } }