From d11ae9cda92b87de4ee48830adb6ee21bb5bf3d2 Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Wed, 13 Dec 2023 18:33:39 +0000 Subject: [PATCH] More advanced label checks --- .github/spellcheck/wordlist.txt | 1 + cmd/pint/tests/0007_alerts.txt | 4 +- cmd/pint/tests/0058_templated_check.txt | 2 +- .../tests/0137_annotation_regex_key_fail.txt | 2 +- docs/changelog.md | 13 + docs/checks/alerts/annotation.md | 40 ++- docs/checks/rule/label.md | 48 +++- docs/examples/labels.hcl | 36 +++ internal/checks/alerts_annotation.go | 80 +++++- internal/checks/alerts_annotation_test.go | 86 ++++-- internal/checks/rule_label.go | 104 +++++-- internal/checks/rule_label_test.go | 195 ++++++++++--- internal/checks/template.go | 15 + .../config/__snapshots__/config_test.snap | 268 ++++++++++-------- internal/config/annotation.go | 14 +- internal/config/annotation_test.go | 7 + internal/config/config_test.go | 13 +- internal/config/rule.go | 14 +- internal/parser/models.go | 8 +- internal/parser/parser_test.go | 123 ++++++++ 20 files changed, 832 insertions(+), 241 deletions(-) create mode 100644 docs/examples/labels.hcl diff --git a/.github/spellcheck/wordlist.txt b/.github/spellcheck/wordlist.txt index 2769522e..58809ddf 100644 --- a/.github/spellcheck/wordlist.txt +++ b/.github/spellcheck/wordlist.txt @@ -45,6 +45,7 @@ templated Thanos TLS toc +tokenize uber UI unmarshal diff --git a/cmd/pint/tests/0007_alerts.txt b/cmd/pint/tests/0007_alerts.txt index 97347d44..2ebd1758 100644 --- a/cmd/pint/tests/0007_alerts.txt +++ b/cmd/pint/tests/0007_alerts.txt @@ -24,10 +24,10 @@ rules/0001.yml:9-10 Warning: `severity` label is required. (rule/label) 9 | - alert: ServiceIsDown 10 | expr: up == 0 -rules/0001.yml:14 Warning: `severity` label value must match `^critical|warning|info$`. (rule/label) +rules/0001.yml:14 Warning: `severity` label value `bad` must match `^critical|warning|info$`. (rule/label) 14 | severity: bad -rules/0001.yml:16 Bug: `url` annotation value must match `^https://wiki.example.com/page/(.+).html$`. (alerts/annotation) +rules/0001.yml:16 Bug: `url` annotation value `bad` must match `^https://wiki.example.com/page/(.+).html$`. (alerts/annotation) 16 | url: bad rules/0002.yml:5 Fatal: Template failed to parse with this error: `undefined variable "$label"`. (alerts/template) diff --git a/cmd/pint/tests/0058_templated_check.txt b/cmd/pint/tests/0058_templated_check.txt index 08cf13b6..7328e9aa 100644 --- a/cmd/pint/tests/0058_templated_check.txt +++ b/cmd/pint/tests/0058_templated_check.txt @@ -10,7 +10,7 @@ rules/0001.yml:4-6 Bug: `alert_for` annotation is required. (alerts/annotation) 5 | expr: up == 0 6 | for: 5m -rules/0001.yml:12 Bug: `alert_for` annotation value must match `^{{ $for }}$`. (alerts/annotation) +rules/0001.yml:12 Bug: `alert_for` annotation value `4m` must match `^{{ $for }}$`. (alerts/annotation) 12 | alert_for: 4m level=INFO msg="Problems found" Bug=2 diff --git a/cmd/pint/tests/0137_annotation_regex_key_fail.txt b/cmd/pint/tests/0137_annotation_regex_key_fail.txt index 4f40260b..f880d3a8 100644 --- a/cmd/pint/tests/0137_annotation_regex_key_fail.txt +++ b/cmd/pint/tests/0137_annotation_regex_key_fail.txt @@ -5,7 +5,7 @@ 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/0001.yml:4 Bug: `annotation_.*` annotation value must match `^bar$`. (alerts/annotation) +rules/0001.yml:4 Bug: `annotation_.*` annotation value `foo` must match `^bar$`. (alerts/annotation) 4 | annotation_foo: foo level=INFO msg="Problems found" Bug=1 diff --git a/docs/changelog.md b/docs/changelog.md index 9d6161a7..ae65aeda 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -2,10 +2,23 @@ ## v0.52.0 +### Added + +- Both [alerts/annotation](checks/alerts/annotation.md) and [rule/label](checks/rule/label.md) + now support more advance validation of label and annotation values with extra `token` option. + In addition to the `value` regexp matching you can also validate values against a static + list of allowed values using new `values` option. + See both checks documentation for detail. + ### Changed - More reports will now be merged into a single comments when using BitBucket. +### Fixed + +- Fixed YAML anchor parsing. +- Fixed regexp matching for label names in [rule/label](checks/rule/label.md). + ## v0.51.1 ### Fixed diff --git a/docs/checks/alerts/annotation.md b/docs/checks/alerts/annotation.md index 6d29dee8..576e812e 100644 --- a/docs/checks/alerts/annotation.md +++ b/docs/checks/alerts/annotation.md @@ -15,7 +15,9 @@ Syntax: ```js annotation "$pattern" { severity = "bug|warning|info" + token = "(.*)" value = "(.*)" + values = ["...", ...] required = true|false } ``` @@ -24,8 +26,16 @@ annotation "$pattern" { to reference checked rule fields, see [Configuration](../../configuration.md) for details. - `severity` - set custom severity for reported issues, defaults to a warning. -- `value` - optional value pattern to enforce, if not set only the. -- `required` - if `true` pint will require every alert to have this annotation set, +- `token` - optional regexp to tokenize annotation value before validating it. + By default the whole annotation value is validated against `value` regexp or + the `values` list. If you want to break the value into sub-strings and + validate each of them independently you can do this by setting `token` + to a regexp that captures a single sub-string. +- `value` - optional value regexp to enforce, if not set only pint will only + check if the annotation exists. +- `values` - optional list of allowed values - this is alternative to using + `value` regexp. Set this to the list of all possible valid annotation values. +- `required` - if `true` pint will require every rule to have this annotation set, if `false` it will only check values where annotation is set. ## How to enable it @@ -76,6 +86,32 @@ rule { } ``` +If you have an annotation that can contain multiple different values as a single string, +for example `components: "db api memcached"`, and you want to ensure only valid values +are included then use `token` and `values`. +By setting `token` to a regexp that matches only a sequence of letters (`[a-zA-Z]+`) +you tell pint to split `"db api memcached"` into `["db", "api", "memcached"]`. +Then it iterates this list and checks each element independently. +This allows you to have validation for multi-value strings. + +{% raw %} + +```js +rule { + annotation "components" { + required = true + token = "[a-zA-Z]+" + values = [ + "prometheus", + "db", + "memcached", + "api", + "storage", + ] + } +} +``` + {% endraw %} ## How to disable it diff --git a/docs/checks/rule/label.md b/docs/checks/rule/label.md index 898ba4f3..d9532bcd 100644 --- a/docs/checks/rule/label.md +++ b/docs/checks/rule/label.md @@ -18,18 +18,28 @@ Syntax: ```js label "$pattern" { severity = "bug|warning|info" - value = "..." + token = "(.*)" + value = "(.*)" + values = ["...", ...] required = true|false } ``` - `$pattern` - regexp pattern to match label name on, this can be templated to reference checked rule fields, see [Configuration](../../configuration.md) - for details -- `severity` - set custom severity for reported issues, defaults to a warning -- `value` - optional value pattern to enforce, if not set only the + for details. +- `severity` - set custom severity for reported issues, defaults to a warning. +- `token` - optional regexp to tokenize label value before validating it. + By default the whole label value is validated against `value` regexp or + the `values` list. If you want to break the value into sub-strings and + validate each of them independently you can do this by setting `token` + to a regexp that captures a single sub-string. +- `value` - optional value regexp to enforce, if not set only pint will only + check if the label exists. +- `values` - optional list of allowed values - this is alternative to using + `value` regexp. Set this to the list of all possible valid label values. - `required` - if `true` pint will require every rule to have this label set, - if `false` it will only check values where label is set + if `false` it will only check values where label is set. ## How to enable it @@ -75,6 +85,34 @@ rule { {% endraw %} +If you have a label that can contain multiple different values as a single string, +for example `components: "db api memcached"`, and you want to ensure only valid values +are included then use `token` and `values`. +By setting `token` to a regexp that matches only a sequence of letters (`[a-zA-Z]+`) +you tell pint to split `"db api memcached"` into `["db", "api", "memcached"]`. +Then it iterates this list and checks each element independently. +This allows you to have validation for multi-value strings. + +{% raw %} + +```js +rule { + label "components" { + required = true + token = "[a-zA-Z]+" + values = [ + "prometheus", + "db", + "memcached", + "api", + "storage", + ] + } +} +``` + +{% endraw %} + ## How to disable it You can disable this check globally by adding this config block: diff --git a/docs/examples/labels.hcl b/docs/examples/labels.hcl new file mode 100644 index 00000000..07238d38 --- /dev/null +++ b/docs/examples/labels.hcl @@ -0,0 +1,36 @@ +# This example shows how to enforce labels where the label value itself can +# be a string with one or more sub-strings in it. +# For example when you have a 'components' label that can be any of these: +# components: 'db' +# components: 'api' +# components: 'proxy' +# components: 'db api' +# components: 'db api proxy' +# components: 'proxy api db' +# components: 'proxy db' + +rule { + # Only run these checks on alerting rules. + # Ignore recording rules. + match { + kind = "alerting" + } + label "components" { + # Every alerting rule must have this label set. + required = true + # If any alerting rule fails our check pint will report his as a 'Bug' + # severity problem, which will fail (exit with non-zero exit code) + # when running 'pint lint' or 'pint ci'. + # Set it to 'warning' if you don't want to fail pint runs. + severity = "bug" + # Split label value into sub-strings using the 'token' regexp. + # \w is an alias for [0-9A-Za-z_] match. + # Notice that we must escape '\' in HCL config files. + token = "\\w+" + # This is the list of allowed values. + values = [ + "db", + "api", + "proxy", + ] +} diff --git a/internal/checks/alerts_annotation.go b/internal/checks/alerts_annotation.go index 7c49e307..9c4f90ae 100644 --- a/internal/checks/alerts_annotation.go +++ b/internal/checks/alerts_annotation.go @@ -3,22 +3,35 @@ package checks import ( "context" "fmt" + "strconv" + "strings" "github.com/cloudflare/pint/internal/discovery" "github.com/cloudflare/pint/internal/parser" + + "golang.org/x/exp/slices" ) const ( AnnotationCheckName = "alerts/annotation" ) -func NewAnnotationCheck(keyRe, valueRe *TemplatedRegexp, isReguired bool, severity Severity) AnnotationCheck { - return AnnotationCheck{keyRe: keyRe, valueRe: valueRe, isReguired: isReguired, severity: severity} +func NewAnnotationCheck(keyRe, tokenRe, valueRe *TemplatedRegexp, values []string, isReguired bool, severity Severity) AnnotationCheck { + return AnnotationCheck{ + keyRe: keyRe, + tokenRe: tokenRe, + valueRe: valueRe, + values: values, + isReguired: isReguired, + severity: severity, + } } type AnnotationCheck struct { keyRe *TemplatedRegexp + tokenRe *TemplatedRegexp valueRe *TemplatedRegexp + values []string isReguired bool severity Severity } @@ -63,24 +76,15 @@ func (c AnnotationCheck) Check(_ context.Context, _ string, rule parser.Rule, _ return problems } - var foundAnnotation bool + annotations := make([]*parser.YamlKeyValue, 0, len(rule.AlertingRule.Annotations.Items)) for _, annotation := range rule.AlertingRule.Annotations.Items { if c.keyRe.MustExpand(rule).MatchString(annotation.Key.Value) { - foundAnnotation = true - if c.valueRe != nil && !c.valueRe.MustExpand(rule).MatchString(annotation.Value.Value) { - problems = append(problems, Problem{ - Lines: annotation.Value.Position.Lines, - Reporter: c.Reporter(), - Text: fmt.Sprintf("`%s` annotation value must match `%s`.", c.keyRe.original, c.valueRe.anchored), - Severity: c.severity, - }) - return problems - } + annotations = append(annotations, annotation) } } - if !foundAnnotation && c.isReguired { + if len(annotations) == 0 && c.isReguired { problems = append(problems, Problem{ Lines: rule.AlertingRule.Annotations.Lines(), Reporter: c.Reporter(), @@ -90,5 +94,51 @@ func (c AnnotationCheck) Check(_ context.Context, _ string, rule parser.Rule, _ return problems } - return nil + for _, ann := range annotations { + if c.tokenRe != nil { + for _, match := range c.tokenRe.MustExpand(rule).FindAllString(ann.Value.Value, -1) { + problems = append(problems, c.checkValue(rule, match, ann.Value.Position.Lines)...) + } + } else { + problems = append(problems, c.checkValue(rule, ann.Value.Value, ann.Value.Position.Lines)...) + } + } + + return problems +} + +func (c AnnotationCheck) checkValue(rule parser.Rule, value string, lines []int) (problems []Problem) { + if c.valueRe != nil && !c.valueRe.MustExpand(rule).MatchString(value) { + problems = append(problems, Problem{ + Lines: lines, + Reporter: c.Reporter(), + Text: fmt.Sprintf("`%s` annotation value `%s` must match `%s`.", c.keyRe.original, value, c.valueRe.anchored), + Severity: c.severity, + }) + } + if len(c.values) > 0 { + if !slices.Contains(c.values, value) { + var details strings.Builder + details.WriteString("List of allowed values:\n\n") + for i, allowed := range c.values { + details.WriteString("- `") + details.WriteString(allowed) + details.WriteString("`\n") + if i >= 5 && len(c.values) > 8 { + details.WriteString("\nAnd ") + details.WriteString(strconv.Itoa(len(c.values) - i - 1)) + details.WriteString(" other value(s).") + break + } + } + problems = append(problems, Problem{ + Lines: lines, + Reporter: c.Reporter(), + Text: fmt.Sprintf("`%s` annotation value `%s` is not one of valid values.", c.keyRe.original, value), + Details: details.String(), + Severity: c.severity, + }) + } + } + return problems } diff --git a/internal/checks/alerts_annotation_test.go b/internal/checks/alerts_annotation_test.go index 2861c8a0..40067ca1 100644 --- a/internal/checks/alerts_annotation_test.go +++ b/internal/checks/alerts_annotation_test.go @@ -13,7 +13,7 @@ func TestAnnotationCheck(t *testing.T) { description: "ignores recording rules", content: "- record: foo\n expr: sum(foo) without(\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), checks.MustTemplatedRegexp("critical"), true, checks.Warning) + return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical"), nil, true, checks.Warning) }, prometheus: noProm, problems: noProblems, @@ -22,7 +22,7 @@ func TestAnnotationCheck(t *testing.T) { 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.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), checks.MustTemplatedRegexp("critical"), true, checks.Warning) + return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical"), nil, true, checks.Warning) }, prometheus: noProm, problems: func(uri string) []checks.Problem { @@ -40,7 +40,7 @@ func TestAnnotationCheck(t *testing.T) { description: "no annotations / required", content: "- alert: foo\n expr: sum(foo)\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), checks.MustTemplatedRegexp("critical"), true, checks.Warning) + return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical"), nil, true, checks.Warning) }, prometheus: noProm, problems: func(uri string) []checks.Problem { @@ -58,7 +58,7 @@ func TestAnnotationCheck(t *testing.T) { description: "no annotations / not required", content: "- alert: foo\n expr: sum(foo)\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), checks.MustTemplatedRegexp("critical"), false, checks.Warning) + return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical"), nil, false, checks.Warning) }, prometheus: noProm, problems: noProblems, @@ -67,7 +67,7 @@ func TestAnnotationCheck(t *testing.T) { description: "missing annotation / required", content: "- alert: foo\n expr: sum(foo)\n annotations:\n foo: bar\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), checks.MustTemplatedRegexp("critical"), true, checks.Warning) + return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical"), nil, true, checks.Warning) }, prometheus: noProm, problems: func(uri string) []checks.Problem { @@ -85,7 +85,7 @@ func TestAnnotationCheck(t *testing.T) { description: "missing annotation / not required", content: "- alert: foo\n expr: sum(foo)\n annotations:\n foo: bar\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), checks.MustTemplatedRegexp("critical"), false, checks.Warning) + return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical"), nil, false, checks.Warning) }, prometheus: noProm, problems: noProblems, @@ -94,7 +94,7 @@ func TestAnnotationCheck(t *testing.T) { description: "wrong annotation value / required", content: "- alert: foo\n expr: sum(foo)\n annotations:\n severity: bar\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), checks.MustTemplatedRegexp("critical"), true, checks.Warning) + return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical"), nil, true, checks.Warning) }, prometheus: noProm, problems: func(uri string) []checks.Problem { @@ -102,7 +102,7 @@ func TestAnnotationCheck(t *testing.T) { { Lines: []int{4}, Reporter: checks.AnnotationCheckName, - Text: "`severity` annotation value must match `^critical$`.", + Text: "`severity` annotation value `bar` must match `^critical$`.", Severity: checks.Warning, }, } @@ -112,7 +112,7 @@ func TestAnnotationCheck(t *testing.T) { description: "wrong annotation value / not required", content: "- alert: foo\n expr: sum(foo)\n annotations:\n severity: bar\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), checks.MustTemplatedRegexp("critical"), false, checks.Warning) + return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical"), nil, false, checks.Warning) }, prometheus: noProm, problems: func(uri string) []checks.Problem { @@ -120,7 +120,7 @@ func TestAnnotationCheck(t *testing.T) { { Lines: []int{4}, Reporter: checks.AnnotationCheckName, - Text: "`severity` annotation value must match `^critical$`.", + Text: "`severity` annotation value `bar` must match `^critical$`.", Severity: checks.Warning, }, } @@ -130,7 +130,7 @@ func TestAnnotationCheck(t *testing.T) { description: "valid annotation / required", content: "- alert: foo\n expr: sum(foo)\n annotations:\n severity: info\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), checks.MustTemplatedRegexp("critical|info|debug"), true, checks.Warning) + return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical|info|debug"), nil, true, checks.Warning) }, prometheus: noProm, problems: noProblems, @@ -139,7 +139,7 @@ func TestAnnotationCheck(t *testing.T) { description: "valid annotation / not required", content: "- alert: foo\n expr: sum(foo)\n annotations:\n severity: info\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), checks.MustTemplatedRegexp("critical|info|debug"), false, checks.Warning) + return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical|info|debug"), nil, false, checks.Warning) }, prometheus: noProm, problems: noProblems, @@ -148,7 +148,7 @@ func TestAnnotationCheck(t *testing.T) { description: "templated annotation value / passing", content: "- alert: foo\n expr: sum(foo)\n for: 5m\n annotations:\n for: 5m\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("for"), checks.MustTemplatedRegexp("{{ $for }}"), true, checks.Bug) + return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("for"), nil, checks.MustTemplatedRegexp("{{ $for }}"), nil, true, checks.Bug) }, prometheus: noProm, problems: noProblems, @@ -157,7 +157,7 @@ func TestAnnotationCheck(t *testing.T) { description: "templated annotation value / passing", content: "- alert: foo\n expr: sum(foo)\n for: 5m\n annotations:\n for: 4m\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("for"), checks.MustTemplatedRegexp("{{ $for }}"), true, checks.Bug) + return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("for"), nil, checks.MustTemplatedRegexp("{{ $for }}"), nil, true, checks.Bug) }, prometheus: noProm, problems: func(uri string) []checks.Problem { @@ -165,7 +165,7 @@ func TestAnnotationCheck(t *testing.T) { { Lines: []int{5}, Reporter: checks.AnnotationCheckName, - Text: "`for` annotation value must match `^{{ $for }}$`.", + Text: "`for` annotation value `4m` must match `^{{ $for }}$`.", Severity: checks.Bug, }, } @@ -175,7 +175,7 @@ func TestAnnotationCheck(t *testing.T) { description: "valid annotation key regex / required", content: "- alert: foo\n expr: sum(foo)\n annotations:\n annotation_1: info\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("annotation_.*"), checks.MustTemplatedRegexp("critical|info|debug"), true, checks.Warning) + return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("annotation_.*"), nil, checks.MustTemplatedRegexp("critical|info|debug"), nil, true, checks.Warning) }, prometheus: noProm, problems: noProblems, @@ -184,7 +184,7 @@ func TestAnnotationCheck(t *testing.T) { description: "valid annotation key regex / not required", content: "- alert: foo\n expr: sum(foo)\n annotations:\n annotation_1: info\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("annotation_.*"), checks.MustTemplatedRegexp("critical|info|debug"), false, checks.Warning) + return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("annotation_.*"), nil, checks.MustTemplatedRegexp("critical|info|debug"), nil, false, checks.Warning) }, prometheus: noProm, problems: noProblems, @@ -193,7 +193,7 @@ func TestAnnotationCheck(t *testing.T) { description: "wrong annotation key regex value / required", content: "- alert: foo\n expr: sum(foo)\n annotations:\n annotation_1: bar\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("annotation_.*"), checks.MustTemplatedRegexp("critical"), true, checks.Warning) + return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("annotation_.*"), nil, checks.MustTemplatedRegexp("critical"), nil, true, checks.Warning) }, prometheus: noProm, problems: func(uri string) []checks.Problem { @@ -201,7 +201,7 @@ func TestAnnotationCheck(t *testing.T) { { Lines: []int{4}, Reporter: checks.AnnotationCheckName, - Text: "`annotation_.*` annotation value must match `^critical$`.", + Text: "`annotation_.*` annotation value `bar` must match `^critical$`.", Severity: checks.Warning, }, } @@ -211,7 +211,7 @@ func TestAnnotationCheck(t *testing.T) { description: "wrong annotation key regex value / not required", content: "- alert: foo\n expr: sum(foo)\n annotations:\n annotation_1: bar\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("annotation_.*"), checks.MustTemplatedRegexp("critical"), false, checks.Warning) + return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("annotation_.*"), nil, checks.MustTemplatedRegexp("critical"), nil, false, checks.Warning) }, prometheus: noProm, problems: func(uri string) []checks.Problem { @@ -219,12 +219,56 @@ func TestAnnotationCheck(t *testing.T) { { Lines: []int{4}, Reporter: checks.AnnotationCheckName, - Text: "`annotation_.*` annotation value must match `^critical$`.", + Text: "`annotation_.*` annotation value `bar` must match `^critical$`.", Severity: checks.Warning, }, } }, }, + { + description: "invalid value / token / valueRe", + content: "- alert: foo\n expr: rate(foo[1m])\n annotations:\n components: api db\n", + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewAnnotationCheck(checks.MustTemplatedRegexp("components"), checks.MustRawTemplatedRegexp("\\w+"), checks.MustTemplatedRegexp("api|memcached"), nil, false, checks.Bug) + }, + prometheus: noProm, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Lines: []int{4}, + Reporter: checks.AnnotationCheckName, + Text: "`components` annotation value `db` must match `^api|memcached$`.", + Severity: checks.Bug, + }, + } + }, + }, + { + description: "invalid value / token / values", + content: "- alert: foo\n expr: rate(foo[1m])\n annotations:\n components: api db\n", + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewAnnotationCheck( + checks.MustTemplatedRegexp("components"), + checks.MustRawTemplatedRegexp("\\w+"), + nil, + []string{"api", "memcached", "storage", "prometheus", "kvm", "mysql", "memsql", "haproxy", "postgresql"}, + false, + checks.Bug, + ) + }, + prometheus: noProm, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Lines: []int{4}, + Reporter: checks.AnnotationCheckName, + Text: "`components` annotation value `db` is not one of valid values.", + Details: "List of allowed values:\n\n- `api`\n- `memcached`\n- `storage`\n- `prometheus`\n- `kvm`\n- `mysql`\n\nAnd 3 other value(s).", + Severity: checks.Bug, + }, + } + }, + }, } runTests(t, testCases) } diff --git a/internal/checks/rule_label.go b/internal/checks/rule_label.go index 4274e27d..095a6c6e 100644 --- a/internal/checks/rule_label.go +++ b/internal/checks/rule_label.go @@ -3,6 +3,9 @@ package checks import ( "context" "fmt" + "slices" + "strconv" + "strings" "github.com/cloudflare/pint/internal/discovery" "github.com/cloudflare/pint/internal/parser" @@ -12,13 +15,22 @@ const ( LabelCheckName = "rule/label" ) -func NewLabelCheck(key string, valueRe *TemplatedRegexp, isReguired bool, severity Severity) LabelCheck { - return LabelCheck{key: key, valueRe: valueRe, isReguired: isReguired, severity: severity} +func NewLabelCheck(keyRe, tokenRe, valueRe *TemplatedRegexp, values []string, isReguired bool, severity Severity) LabelCheck { + return LabelCheck{ + keyRe: keyRe, + tokenRe: tokenRe, + valueRe: valueRe, + values: values, + isReguired: isReguired, + severity: severity, + } } type LabelCheck struct { - key string + keyRe *TemplatedRegexp + tokenRe *TemplatedRegexp valueRe *TemplatedRegexp + values []string isReguired bool severity Severity } @@ -36,7 +48,10 @@ func (c LabelCheck) Meta() CheckMeta { } func (c LabelCheck) String() string { - return fmt.Sprintf("%s(%s:%v)", LabelCheckName, c.key, c.isReguired) + if c.valueRe != nil { + return fmt.Sprintf("%s(%s=~%s:%v)", LabelCheckName, c.keyRe.original, c.valueRe.anchored, c.isReguired) + } + return fmt.Sprintf("%s(%s:%v)", LabelCheckName, c.keyRe.original, c.isReguired) } func (c LabelCheck) Reporter() string { @@ -61,28 +76,34 @@ func (c LabelCheck) checkRecordingRule(rule parser.Rule) (problems []Problem) { problems = append(problems, Problem{ Lines: rule.Lines(), Reporter: c.Reporter(), - Text: fmt.Sprintf("`%s` label is required.", c.key), + Text: fmt.Sprintf("`%s` label is required.", c.keyRe.original), Severity: c.severity, }) } return problems } - val := rule.RecordingRule.Labels.GetValue(c.key) + val := rule.RecordingRule.Labels.GetValue(c.keyRe.original) if val == nil { if c.isReguired { problems = append(problems, Problem{ Lines: rule.RecordingRule.Labels.Lines(), Reporter: c.Reporter(), - Text: fmt.Sprintf("`%s` label is required.", c.key), + Text: fmt.Sprintf("`%s` label is required.", c.keyRe.original), Severity: c.severity, }) } return problems } - problems = append(problems, c.checkValue(rule, val)...) + if c.tokenRe != nil { + for _, match := range c.tokenRe.MustExpand(rule).FindAllString(val.Value, -1) { + problems = append(problems, c.checkValue(rule, match, val.Position.Lines)...) + } + return problems + } + problems = append(problems, c.checkValue(rule, val.Value, val.Position.Lines)...) return problems } @@ -92,39 +113,76 @@ func (c LabelCheck) checkAlertingRule(rule parser.Rule) (problems []Problem) { problems = append(problems, Problem{ Lines: rule.Lines(), Reporter: c.Reporter(), - Text: fmt.Sprintf("`%s` label is required.", c.key), + Text: fmt.Sprintf("`%s` label is required.", c.keyRe.original), Severity: c.severity, }) } return problems } - val := rule.AlertingRule.Labels.GetValue(c.key) - if val == nil { - if c.isReguired { - problems = append(problems, Problem{ - Lines: rule.AlertingRule.Labels.Lines(), - Reporter: c.Reporter(), - Text: fmt.Sprintf("`%s` label is required.", c.key), - Severity: c.severity, - }) + labels := make([]*parser.YamlKeyValue, 0, len(rule.AlertingRule.Labels.Items)) + + for _, lab := range rule.AlertingRule.Labels.Items { + if c.keyRe.MustExpand(rule).MatchString(lab.Key.Value) { + labels = append(labels, lab) } + } + + if len(labels) == 0 && c.isReguired { + problems = append(problems, Problem{ + Lines: rule.AlertingRule.Labels.Lines(), + Reporter: c.Reporter(), + Text: fmt.Sprintf("`%s` label is required.", c.keyRe.original), + Severity: c.severity, + }) return problems } - problems = append(problems, c.checkValue(rule, val)...) + for _, lab := range labels { + if c.tokenRe != nil { + for _, match := range c.tokenRe.MustExpand(rule).FindAllString(lab.Value.Value, -1) { + problems = append(problems, c.checkValue(rule, match, lab.Value.Position.Lines)...) + } + } else { + problems = append(problems, c.checkValue(rule, lab.Value.Value, lab.Value.Position.Lines)...) + } + } return problems } -func (c LabelCheck) checkValue(rule parser.Rule, val *parser.YamlNode) (problems []Problem) { - if c.valueRe != nil && !c.valueRe.MustExpand(rule).MatchString(val.Value) { +func (c LabelCheck) checkValue(rule parser.Rule, value string, lines []int) (problems []Problem) { + if c.valueRe != nil && !c.valueRe.MustExpand(rule).MatchString(value) { problems = append(problems, Problem{ - Lines: val.Position.Lines, + Lines: lines, Reporter: c.Reporter(), - Text: fmt.Sprintf("`%s` label value must match `%s`.", c.key, c.valueRe.anchored), + Text: fmt.Sprintf("`%s` label value `%s` must match `%s`.", c.keyRe.original, value, c.valueRe.anchored), Severity: c.severity, }) } + if len(c.values) > 0 { + if !slices.Contains(c.values, value) { + var details strings.Builder + details.WriteString("List of allowed values:\n\n") + for i, allowed := range c.values { + details.WriteString("- `") + details.WriteString(allowed) + details.WriteString("`\n") + if i >= 5 && len(c.values) > 8 { + details.WriteString("\nAnd ") + details.WriteString(strconv.Itoa(len(c.values) - i - 1)) + details.WriteString(" other value(s).") + break + } + } + problems = append(problems, Problem{ + Lines: lines, + Reporter: c.Reporter(), + Text: fmt.Sprintf("`%s` label value `%s` is not one of valid values.", c.keyRe.original, value), + Details: details.String(), + Severity: c.severity, + }) + } + } return problems } diff --git a/internal/checks/rule_label_test.go b/internal/checks/rule_label_test.go index bcd3239e..a88473ee 100644 --- a/internal/checks/rule_label_test.go +++ b/internal/checks/rule_label_test.go @@ -13,14 +13,14 @@ func TestLabelCheck(t *testing.T) { 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.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning) + return checks.NewLabelCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical"), nil, true, checks.Warning) }, prometheus: noProm, problems: func(uri string) []checks.Problem { return []checks.Problem{ { Lines: []int{1, 2}, - Reporter: "rule/label", + Reporter: checks.LabelCheckName, Text: "`severity` label is required.", Severity: checks.Warning, }, @@ -31,14 +31,14 @@ func TestLabelCheck(t *testing.T) { description: "no labels in recording rule / required", content: "- record: foo\n expr: rate(foo[1m])\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning) + return checks.NewLabelCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical"), nil, true, checks.Warning) }, prometheus: noProm, problems: func(uri string) []checks.Problem { return []checks.Problem{ { Lines: []int{1, 2}, - Reporter: "rule/label", + Reporter: checks.LabelCheckName, Text: "`severity` label is required.", Severity: checks.Warning, }, @@ -49,24 +49,31 @@ func TestLabelCheck(t *testing.T) { description: "no labels in recording rule / not required", content: "- record: foo\n expr: rate(foo[1m])\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning) + return checks.NewLabelCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical"), nil, false, checks.Warning) }, prometheus: noProm, problems: noProblems, }, { description: "missing label in recording rule / required", - content: "- record: foo\n expr: rate(foo[1m])\n labels:\n foo: bar\n", + content: "- record: foo\n expr: rate(foo[1m])\n labels:\n foo: bar\n bob: alice\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning) + return checks.NewLabelCheck( + checks.MustTemplatedRegexp("sev.+"), + checks.MustRawTemplatedRegexp("\\w+"), + checks.MustTemplatedRegexp("critical"), + nil, + true, + checks.Warning, + ) }, prometheus: noProm, problems: func(uri string) []checks.Problem { return []checks.Problem{ { - Lines: []int{3, 4}, - Reporter: "rule/label", - Text: "`severity` label is required.", + Lines: []int{3, 4, 5}, + Reporter: checks.LabelCheckName, + Text: "`sev.+` label is required.", Severity: checks.Warning, }, } @@ -76,7 +83,7 @@ func TestLabelCheck(t *testing.T) { description: "missing label in recording rule / not required", content: "- record: foo\n expr: rate(foo[1m])\n labels:\n foo: bar\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning) + return checks.NewLabelCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical"), nil, false, checks.Warning) }, prometheus: noProm, problems: noProblems, @@ -85,15 +92,15 @@ func TestLabelCheck(t *testing.T) { description: "invalid value in recording rule / required", content: "- record: foo\n expr: rate(foo[1m])\n labels:\n severity: warning\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning) + return checks.NewLabelCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical"), nil, true, checks.Warning) }, prometheus: noProm, problems: func(uri string) []checks.Problem { return []checks.Problem{ { Lines: []int{4}, - Reporter: "rule/label", - Text: "`severity` label value must match `^critical$`.", + Reporter: checks.LabelCheckName, + Text: "`severity` label value `warning` must match `^critical$`.", Severity: checks.Warning, }, } @@ -103,15 +110,15 @@ func TestLabelCheck(t *testing.T) { description: "invalid value in recording rule / not required", content: "- record: foo\n expr: rate(foo[1m])\n labels:\n severity: warning\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning) + return checks.NewLabelCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical"), nil, false, checks.Warning) }, prometheus: noProm, problems: func(uri string) []checks.Problem { return []checks.Problem{ { Lines: []int{4}, - Reporter: "rule/label", - Text: "`severity` label value must match `^critical$`.", + Reporter: checks.LabelCheckName, + Text: "`severity` label value `warning` must match `^critical$`.", Severity: checks.Warning, }, } @@ -121,15 +128,22 @@ func TestLabelCheck(t *testing.T) { description: "typo in recording rule / required", content: "- record: foo\n expr: rate(foo[1m])\n labels:\n priority: 2a\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("priority", checks.MustTemplatedRegexp("(1|2|3)"), true, checks.Warning) + return checks.NewLabelCheck( + checks.MustTemplatedRegexp("priority"), + nil, + checks.MustTemplatedRegexp("(1|2|3)"), + nil, + true, + checks.Warning, + ) }, prometheus: noProm, problems: func(uri string) []checks.Problem { return []checks.Problem{ { Lines: []int{4}, - Reporter: "rule/label", - Text: "`priority` label value must match `^(1|2|3)$`.", + Reporter: checks.LabelCheckName, + Text: "`priority` label value `2a` must match `^(1|2|3)$`.", Severity: checks.Warning, }, } @@ -139,15 +153,22 @@ func TestLabelCheck(t *testing.T) { description: "typo in recording rule / not required", content: "- record: foo\n expr: rate(foo[1m])\n labels:\n priority: 2a\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("priority", checks.MustTemplatedRegexp("(1|2|3)"), false, checks.Warning) + return checks.NewLabelCheck( + checks.MustTemplatedRegexp("priority"), + nil, + checks.MustTemplatedRegexp("(1|2|3)"), + nil, + false, + checks.Warning, + ) }, prometheus: noProm, problems: func(uri string) []checks.Problem { return []checks.Problem{ { Lines: []int{4}, - Reporter: "rule/label", - Text: "`priority` label value must match `^(1|2|3)$`.", + Reporter: checks.LabelCheckName, + Text: "`priority` label value `2a` must match `^(1|2|3)$`.", Severity: checks.Warning, }, } @@ -157,14 +178,14 @@ func TestLabelCheck(t *testing.T) { description: "no labels in alerting rule / required", content: "- alert: foo\n expr: rate(foo[1m])\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning) + return checks.NewLabelCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical"), nil, true, checks.Warning) }, prometheus: noProm, problems: func(uri string) []checks.Problem { return []checks.Problem{ { Lines: []int{1, 2}, - Reporter: "rule/label", + Reporter: checks.LabelCheckName, Text: "`severity` label is required.", Severity: checks.Warning, }, @@ -175,7 +196,7 @@ func TestLabelCheck(t *testing.T) { description: "no labels in alerting rule / not required", content: "- alert: foo\n expr: rate(foo[1m])\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning) + return checks.NewLabelCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical"), nil, false, checks.Warning) }, prometheus: noProm, problems: noProblems, @@ -184,14 +205,14 @@ func TestLabelCheck(t *testing.T) { description: "missing label in alerting rule / required", content: "- alert: foo\n expr: rate(foo[1m])\n labels:\n foo: bar\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), true, checks.Warning) + return checks.NewLabelCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical"), nil, true, checks.Warning) }, prometheus: noProm, problems: func(uri string) []checks.Problem { return []checks.Problem{ { Lines: []int{3, 4}, - Reporter: "rule/label", + Reporter: checks.LabelCheckName, Text: "`severity` label is required.", Severity: checks.Warning, }, @@ -202,7 +223,7 @@ func TestLabelCheck(t *testing.T) { description: "missing label in alerting rule / not required", content: "- alert: foo\n expr: rate(foo[1m])\n labels:\n foo: bar\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical"), false, checks.Warning) + return checks.NewLabelCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical"), nil, false, checks.Warning) }, prometheus: noProm, problems: noProblems, @@ -211,15 +232,15 @@ func TestLabelCheck(t *testing.T) { description: "invalid value in alerting rule / required", content: "- alert: foo\n expr: rate(foo[1m])\n labels:\n severity: warning\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical|info"), true, checks.Warning) + return checks.NewLabelCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical|info"), nil, true, checks.Warning) }, prometheus: noProm, problems: func(uri string) []checks.Problem { return []checks.Problem{ { Lines: []int{4}, - Reporter: "rule/label", - Text: "`severity` label value must match `^critical|info$`.", + Reporter: checks.LabelCheckName, + Text: "`severity` label value `warning` must match `^critical|info$`.", Severity: checks.Warning, }, } @@ -229,15 +250,15 @@ func TestLabelCheck(t *testing.T) { description: "invalid value in alerting rule / not required", content: "- alert: foo\n expr: rate(foo[1m])\n labels:\n severity: warning\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical|info"), false, checks.Warning) + return checks.NewLabelCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical|info"), nil, false, checks.Warning) }, prometheus: noProm, problems: func(uri string) []checks.Problem { return []checks.Problem{ { Lines: []int{4}, - Reporter: "rule/label", - Text: "`severity` label value must match `^critical|info$`.", + Reporter: checks.LabelCheckName, + Text: "`severity` label value `warning` must match `^critical|info$`.", Severity: checks.Warning, }, } @@ -247,7 +268,7 @@ func TestLabelCheck(t *testing.T) { description: "valid recording rule / required", content: "- record: foo\n expr: rate(foo[1m])\n labels:\n severity: critical\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical|info"), true, checks.Warning) + return checks.NewLabelCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical|info"), nil, true, checks.Warning) }, prometheus: noProm, problems: noProblems, @@ -256,7 +277,7 @@ func TestLabelCheck(t *testing.T) { description: "valid recording rule / not required", content: "- record: foo\n expr: rate(foo[1m])\n labels:\n severity: critical\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical|info"), false, checks.Warning) + return checks.NewLabelCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical|info"), nil, false, checks.Warning) }, prometheus: noProm, problems: noProblems, @@ -265,7 +286,7 @@ func TestLabelCheck(t *testing.T) { description: "valid alerting rule / required", content: "- alert: foo\n expr: rate(foo[1m])\n labels:\n severity: info\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical|info"), true, checks.Warning) + return checks.NewLabelCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical|info"), nil, true, checks.Warning) }, prometheus: noProm, problems: noProblems, @@ -274,7 +295,7 @@ func TestLabelCheck(t *testing.T) { description: "valid alerting rule / not required", content: "- alert: foo\n expr: rate(foo[1m])\n labels:\n severity: info\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("severity", checks.MustTemplatedRegexp("critical|info"), false, checks.Warning) + return checks.NewLabelCheck(checks.MustTemplatedRegexp("severity"), nil, checks.MustTemplatedRegexp("critical|info"), nil, false, checks.Warning) }, prometheus: noProm, problems: noProblems, @@ -283,7 +304,7 @@ func TestLabelCheck(t *testing.T) { description: "templated label value / passing", content: "- alert: foo\n expr: sum(foo)\n for: 5m\n labels:\n for: 'must wait 5m to fire'\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("for", checks.MustTemplatedRegexp("must wait {{$for}} to fire"), true, checks.Warning) + return checks.NewLabelCheck(checks.MustTemplatedRegexp("for"), nil, checks.MustTemplatedRegexp("must wait {{$for}} to fire"), nil, true, checks.Warning) }, prometheus: noProm, problems: noProblems, @@ -292,15 +313,103 @@ func TestLabelCheck(t *testing.T) { description: "templated label value / not passing", content: "- alert: foo\n expr: sum(foo)\n for: 4m\n labels:\n for: 'must wait 5m to fire'\n", checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { - return checks.NewLabelCheck("for", checks.MustTemplatedRegexp("must wait {{$for}} to fire"), true, checks.Warning) + return checks.NewLabelCheck(checks.MustTemplatedRegexp("for"), nil, checks.MustTemplatedRegexp("must wait {{$for}} to fire"), nil, true, checks.Warning) }, prometheus: noProm, problems: func(uri string) []checks.Problem { return []checks.Problem{ { Lines: []int{5}, - Reporter: "rule/label", - Text: "`for` label value must match `^must wait {{$for}} to fire$`.", + Reporter: checks.LabelCheckName, + Text: "`for` label value `must wait 5m to fire` must match `^must wait {{$for}} to fire$`.", + Severity: checks.Warning, + }, + } + }, + }, + { + description: "invalid value in alerting rule / token / valueRe", + content: "- alert: foo\n expr: rate(foo[1m])\n labels:\n components: api db\n", + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewLabelCheck(checks.MustTemplatedRegexp("components"), checks.MustRawTemplatedRegexp("\\w+"), checks.MustTemplatedRegexp("api|memcached"), nil, false, checks.Bug) + }, + prometheus: noProm, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Lines: []int{4}, + Reporter: checks.LabelCheckName, + Text: "`components` label value `db` must match `^api|memcached$`.", + Severity: checks.Bug, + }, + } + }, + }, + { + description: "invalid value in alerting rule / token / values", + content: "- alert: foo\n expr: rate(foo[1m])\n labels:\n components: api db\n", + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewLabelCheck( + checks.MustTemplatedRegexp("components"), + checks.MustRawTemplatedRegexp("\\w+"), + nil, + []string{"api", "memcached", "storage", "prometheus", "kvm", "mysql", "memsql", "haproxy", "postgresql"}, + false, + checks.Bug, + ) + }, + prometheus: noProm, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Lines: []int{4}, + Reporter: checks.LabelCheckName, + Text: "`components` label value `db` is not one of valid values.", + Details: "List of allowed values:\n\n- `api`\n- `memcached`\n- `storage`\n- `prometheus`\n- `kvm`\n- `mysql`\n\nAnd 3 other value(s).", + Severity: checks.Bug, + }, + } + }, + }, + { + description: "invalid value in recording rule / token / valueRe", + content: "- record: foo\n expr: rate(foo[1m])\n labels:\n components: api db\n", + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewLabelCheck(checks.MustTemplatedRegexp("components"), checks.MustRawTemplatedRegexp("\\w+"), checks.MustTemplatedRegexp("api|memcached"), nil, false, checks.Bug) + }, + prometheus: noProm, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Lines: []int{4}, + Reporter: checks.LabelCheckName, + Text: "`components` label value `db` must match `^api|memcached$`.", + Severity: checks.Bug, + }, + } + }, + }, + { + description: "invalid value in recording rule / token / values", + content: "- record: foo\n expr: rate(foo[1m])\n labels:\n components: api db\n", + checker: func(_ *promapi.FailoverGroup) checks.RuleChecker { + return checks.NewLabelCheck( + checks.MustTemplatedRegexp("components"), + checks.MustRawTemplatedRegexp("\\w+"), + nil, + []string{"api", "memcached", "storage", "prometheus", "kvm", "mysql", "memsql", "haproxy"}, + false, + checks.Warning, + ) + }, + prometheus: noProm, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Lines: []int{4}, + Reporter: checks.LabelCheckName, + Text: "`components` label value `db` is not one of valid values.", + Details: "List of allowed values:\n\n- `api`\n- `memcached`\n- `storage`\n- `prometheus`\n- `kvm`\n- `mysql`\n- `memsql`\n- `haproxy`\n", Severity: checks.Warning, }, } diff --git a/internal/checks/template.go b/internal/checks/template.go index 2a4ba4a0..20484852 100644 --- a/internal/checks/template.go +++ b/internal/checks/template.go @@ -19,11 +19,26 @@ func NewTemplatedRegexp(s string) (*TemplatedRegexp, error) { return &tr, nil } +func NewRawTemplatedRegexp(s string) (*TemplatedRegexp, error) { + tr := TemplatedRegexp{anchored: s, original: s} + _, err := tr.Expand(parser.Rule{}) + if err != nil { + return nil, err + } + + return &tr, nil +} + func MustTemplatedRegexp(re string) *TemplatedRegexp { tr, _ := NewTemplatedRegexp(re) return tr } +func MustRawTemplatedRegexp(re string) *TemplatedRegexp { + tr, _ := NewRawTemplatedRegexp(re) + return tr +} + type TemplatedRegexp struct { anchored string original string diff --git a/internal/config/__snapshots__/config_test.snap b/internal/config/__snapshots__/config_test.snap index 0d029783..8e64b1a1 100755 --- a/internal/config/__snapshots__/config_test.snap +++ b/internal/config/__snapshots__/config_test.snap @@ -813,6 +813,17 @@ "severity": "bug" } ] + }, + { + "annotation": [ + { + "key": "summary", + "token": "\\w+", + "value": "foo.+", + "required": true, + "severity": "bug" + } + ] } ], "owners": {} @@ -1050,64 +1061,6 @@ } --- -[TestGetChecksForRule/rule_with_label_match_/_label_match - 1] -{ - "ci": { - "maxCommits": 20, - "baseBranch": "master" - }, - "parser": {}, - "checks": { - "enabled": [ - "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/series", - "rule/dependency", - "rule/duplicate", - "rule/for", - "rule/label", - "rule/link", - "rule/reject" - ] - }, - "rules": [ - { - "match": [ - { - "kind": "alerting", - "label": { - "key": "cluster", - "value": "prod" - } - } - ], - "label": [ - { - "key": "priority", - "value": "(1|2|3|4|5)", - "required": true, - "severity": "bug" - } - ] - } - ], - "owners": {} -} ---- - [TestGetChecksForRule/rule_with_annotation_match_/_no_annotation - 1] { "ci": { @@ -1224,64 +1177,6 @@ } --- -[TestGetChecksForRule/rule_with_annotation_match_/_annotation_match - 1] -{ - "ci": { - "maxCommits": 20, - "baseBranch": "master" - }, - "parser": {}, - "checks": { - "enabled": [ - "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/series", - "rule/dependency", - "rule/duplicate", - "rule/for", - "rule/label", - "rule/link", - "rule/reject" - ] - }, - "rules": [ - { - "match": [ - { - "kind": "alerting", - "annotation": { - "key": "cluster", - "value": "prod" - } - } - ], - "label": [ - { - "key": "priority", - "value": "(1|2|3|4|5)", - "required": true, - "severity": "bug" - } - ] - } - ], - "owners": {} -} ---- - [TestGetChecksForRule/for_match_/_passing - 1] { "ci": { @@ -2562,6 +2457,123 @@ } --- +[TestGetChecksForRule/rule_with_label_match_/_label_match - 1] +{ + "ci": { + "maxCommits": 20, + "baseBranch": "master" + }, + "parser": {}, + "checks": { + "enabled": [ + "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/series", + "rule/dependency", + "rule/duplicate", + "rule/for", + "rule/label", + "rule/link", + "rule/reject" + ] + }, + "rules": [ + { + "match": [ + { + "kind": "alerting", + "label": { + "key": "cluster", + "value": "prod" + } + } + ], + "label": [ + { + "key": "priority", + "value": "(1|2|3|4|5)", + "required": true, + "severity": "bug" + } + ] + } + ], + "owners": {} +} +--- + +[TestGetChecksForRule/rule_with_annotation_match_/_annotation_match - 1] +{ + "ci": { + "maxCommits": 20, + "baseBranch": "master" + }, + "parser": {}, + "checks": { + "enabled": [ + "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/series", + "rule/dependency", + "rule/duplicate", + "rule/for", + "rule/label", + "rule/link", + "rule/reject" + ] + }, + "rules": [ + { + "match": [ + { + "kind": "alerting", + "annotation": { + "key": "cluster", + "value": "prod" + } + } + ], + "label": [ + { + "key": "priority", + "token": "\\w+", + "value": "(1|2|3|4|5)", + "required": true, + "severity": "bug" + } + ] + } + ], + "owners": {} +} +--- + [TestGetChecksForRule/defaults - 2] { "ci": { @@ -3375,6 +3387,17 @@ "severity": "bug" } ] + }, + { + "annotation": [ + { + "key": "summary", + "token": "\\w+", + "value": "foo.+", + "required": true, + "severity": "bug" + } + ] } ], "owners": {} @@ -3915,6 +3938,7 @@ "label": [ { "key": "priority", + "token": "\\w+", "value": "(1|2|3|4|5)", "required": true, "severity": "bug" @@ -5938,6 +5962,17 @@ "severity": "bug" } ] + }, + { + "annotation": [ + { + "key": "summary", + "token": "\\w+", + "value": "foo.+", + "required": true, + "severity": "bug" + } + ] } ], "owners": {} @@ -6478,6 +6513,7 @@ "label": [ { "key": "priority", + "token": "\\w+", "value": "(1|2|3|4|5)", "required": true, "severity": "bug" diff --git a/internal/config/annotation.go b/internal/config/annotation.go index bfbb9873..f22a2ee2 100644 --- a/internal/config/annotation.go +++ b/internal/config/annotation.go @@ -7,10 +7,12 @@ import ( ) type AnnotationSettings struct { - Key string `hcl:",label" json:"key"` - Value string `hcl:"value,optional" json:"value,omitempty"` - Required bool `hcl:"required,optional" json:"required,omitempty"` - Severity string `hcl:"severity,optional" json:"severity,omitempty"` + Key string `hcl:",label" json:"key"` + Token string `hcl:"token,optional" json:"token,omitempty"` + Value string `hcl:"value,optional" json:"value,omitempty"` + Values []string `hcl:"values,optional" json:"values,omitempty"` + Required bool `hcl:"required,optional" json:"required,omitempty"` + Severity string `hcl:"severity,optional" json:"severity,omitempty"` } func (as AnnotationSettings) validate() error { @@ -22,6 +24,10 @@ func (as AnnotationSettings) validate() error { return err } + if _, err := checks.NewRawTemplatedRegexp(as.Token); err != nil { + return err + } + if _, err := checks.NewTemplatedRegexp(as.Value); err != nil { return err } diff --git a/internal/config/annotation_test.go b/internal/config/annotation_test.go index 19450a37..d9802b9b 100644 --- a/internal/config/annotation_test.go +++ b/internal/config/annotation_test.go @@ -30,6 +30,13 @@ func TestAnnotationSettings(t *testing.T) { }, err: errors.New("error parsing regexp: invalid nested repetition operator: `++`"), }, + { + conf: AnnotationSettings{ + Key: "summary", + Token: ".++", + }, + err: errors.New("error parsing regexp: invalid nested repetition operator: `++`"), + }, { conf: AnnotationSettings{ Key: ".+", diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 1108b75c..e630fa6a 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -580,6 +580,14 @@ rule { required = true } } +rule { + annotation "summary" { + token = "\\w+" + value = "foo.+" + severity = "bug" + required = true + } + } `, entry: discovery.Entry{ State: discovery.Modified, @@ -816,7 +824,7 @@ rule { checks.ComparisonCheckName, checks.TemplateCheckName, checks.FragileCheckName, - checks.RegexpCheckName, checks.LabelCheckName + "(priority:true)", + checks.RegexpCheckName, checks.LabelCheckName + "(priority=~^(1|2|3|4|5)$:true)", }, }, { @@ -893,6 +901,7 @@ rule { } label "priority" { severity = "bug" + token = "\\w+" value = "(1|2|3|4|5)" required = true } @@ -909,7 +918,7 @@ rule { checks.ComparisonCheckName, checks.TemplateCheckName, checks.FragileCheckName, - checks.RegexpCheckName, checks.LabelCheckName + "(priority:true)", + checks.RegexpCheckName, checks.LabelCheckName + "(priority=~^(1|2|3|4|5)$:true)", }, }, { diff --git a/internal/config/rule.go b/internal/config/rule.go index 8e631d7a..178d9ea4 100644 --- a/internal/config/rule.go +++ b/internal/config/rule.go @@ -155,28 +155,34 @@ func (rule Rule) resolveChecks(ctx context.Context, path string, r parser.Rule, if len(rule.Annotation) > 0 { for _, ann := range rule.Annotation { - var valueRegex *checks.TemplatedRegexp + var tokenRegex, valueRegex *checks.TemplatedRegexp + if ann.Token != "" { + tokenRegex = checks.MustRawTemplatedRegexp(ann.Token) + } if ann.Value != "" { valueRegex = checks.MustTemplatedRegexp(ann.Value) } severity := ann.getSeverity(checks.Warning) enabled = append(enabled, checkMeta{ name: checks.AnnotationCheckName, - check: checks.NewAnnotationCheck(checks.MustTemplatedRegexp(ann.Key), valueRegex, ann.Required, severity), + check: checks.NewAnnotationCheck(checks.MustTemplatedRegexp(ann.Key), tokenRegex, valueRegex, ann.Values, ann.Required, severity), }) } } if len(rule.Label) > 0 { for _, lab := range rule.Label { - var valueRegex *checks.TemplatedRegexp + var tokenRegex, valueRegex *checks.TemplatedRegexp + if lab.Token != "" { + tokenRegex = checks.MustRawTemplatedRegexp(lab.Token) + } if lab.Value != "" { valueRegex = checks.MustTemplatedRegexp(lab.Value) } severity := lab.getSeverity(checks.Warning) enabled = append(enabled, checkMeta{ name: checks.LabelCheckName, - check: checks.NewLabelCheck(lab.Key, valueRegex, lab.Required, severity), + check: checks.NewLabelCheck(checks.MustTemplatedRegexp(lab.Key), tokenRegex, valueRegex, lab.Values, lab.Required, severity), }) } } diff --git a/internal/parser/models.go b/internal/parser/models.go index 0cd4e8e7..372d62fa 100644 --- a/internal/parser/models.go +++ b/internal/parser/models.go @@ -96,10 +96,14 @@ type YamlNode struct { } func newYamlNode(node *yaml.Node, offset int) *YamlNode { - return &YamlNode{ + n := YamlNode{ Position: NewFilePosition(nodeLines(node, offset)), Value: node.Value, } + if node.Alias != nil { + n.Value = node.Alias.Value + } + return &n } func newYamlKeyValue(key, val *yaml.Node, offset int) *YamlKeyValue { @@ -245,7 +249,7 @@ func newPromQLExpr(key, val *yaml.Node, offset int) *PromQLExpr { Value: newYamlNode(val, offset), } - qlNode, err := DecodeExpr(val.Value) + qlNode, err := DecodeExpr(expr.Value.Value) if err != nil { expr.SyntaxError = err return &expr diff --git a/internal/parser/parser_test.go b/internal/parser/parser_test.go index b381b3df..1bd878c3 100644 --- a/internal/parser/parser_test.go +++ b/internal/parser/parser_test.go @@ -1416,6 +1416,129 @@ data: }, shouldError: false, }, + { + content: []byte(string(` +- alert: Template + expr: &expr up == 0 + labels: + notify: &maybe_escalate_notify chat-alerts +- alert: Service Down + expr: *expr + labels: + notify: *maybe_escalate_notify + summary: foo +`)), + output: []parser.Rule{ + { + AlertingRule: &parser.AlertingRule{ + Alert: parser.YamlKeyValue{ + Key: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{2}}, + Value: "alert", + }, + Value: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{2}}, + Value: "Template", + }, + }, + Expr: parser.PromQLExpr{ + Key: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{3}}, + Value: "expr", + }, + Value: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{3}}, + Value: "up == 0", + }, + Query: &parser.PromQLNode{ + Expr: "up == 0", + Children: []*parser.PromQLNode{ + {Expr: "up"}, + {Expr: "0"}, + }, + }, + }, + Labels: &parser.YamlMap{ + Key: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{4}}, + Value: "labels", + }, + Items: []*parser.YamlKeyValue{ + { + Key: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{5}}, + Value: "notify", + }, + Value: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{5}}, + Value: "chat-alerts", + }, + }, + }, + }, + }, + }, + { + AlertingRule: &parser.AlertingRule{ + Alert: parser.YamlKeyValue{ + Key: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{6}}, + Value: "alert", + }, + Value: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{6}}, + Value: "Service Down", + }, + }, + Expr: parser.PromQLExpr{ + Key: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{7}}, + Value: "expr", + }, + Value: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{7}}, + Value: "up == 0", + }, + Query: &parser.PromQLNode{ + Expr: "up == 0", + Children: []*parser.PromQLNode{ + {Expr: "up"}, + {Expr: "0"}, + }, + }, + }, + Labels: &parser.YamlMap{ + Key: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{8}}, + Value: "labels", + }, + Items: []*parser.YamlKeyValue{ + { + Key: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{9}}, + Value: "notify", + }, + Value: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{9}}, + Value: "chat-alerts", + }, + }, + { + Key: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{10}}, + Value: "summary", + }, + Value: &parser.YamlNode{ + Position: parser.FilePosition{Lines: []int{10}}, + Value: "foo", + }, + }, + }, + }, + }, + }, + }, + }, } alwaysEqual := cmp.Comparer(func(_, _ interface{}) bool { return true })