From 9b4c8f6be28823c604aab50febcd32013aa4212c Mon Sep 17 00:00:00 2001 From: frazou <4470714+Frazew@users.noreply.github.com> Date: Thu, 13 Feb 2025 10:48:33 +0100 Subject: [PATCH] rulefmt: support YAML aliases for Alert/Record/Expr (#14957) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * rulefmt: add tests with YAML aliases for Alert/Record/Expr Altough somewhat discouraged in favour of using proper configuration management tools to generate full YAML, it can still be useful in some situations to use YAML anchors/aliases in rules. The current implementation is however confusing: aliases will work everywhere except on the alert/record name and expr This first commit adds (failing) tests to illustrate the issue, the next one fixes it. The YAML test file is intentionally filled with anchors and aliases. Although this is probably not representative of a real-world use case (which would have less of them), it errs on the safer side. Signed-off-by: François HORTA * rulefmt: support YAML aliases for Alert/Record/Expr This fixes the use of YAML aliases in alert/recording rule names and expressions. A side effect of this change is that the RuleNode YAML type is no longer propagated deeper in the codebase, instead the generic Rule type can now be used. Signed-off-by: François HORTA * rulefmt: Add test for YAML merge combined with aliases Currently this does work, but adding a test for the related functionally here makes sense. Signed-off-by: David Leadbeater * rulefmt: Rebase to latest changes Signed-off-by: David Leadbeater --------- Signed-off-by: François HORTA Signed-off-by: David Leadbeater Co-authored-by: David Leadbeater --- cmd/promtool/main.go | 8 +-- model/rulefmt/rulefmt.go | 75 ++++++++++++++---------- model/rulefmt/rulefmt_test.go | 27 +++++++++ model/rulefmt/testdata/test_aliases.yaml | 57 ++++++++++++++++++ rules/manager.go | 8 +-- rules/manager_test.go | 8 +-- 6 files changed, 139 insertions(+), 44 deletions(-) create mode 100644 model/rulefmt/testdata/test_aliases.yaml diff --git a/cmd/promtool/main.go b/cmd/promtool/main.go index 4275b7b572..00280500ed 100644 --- a/cmd/promtool/main.go +++ b/cmd/promtool/main.go @@ -992,11 +992,11 @@ func checkDuplicates(groups []rulefmt.RuleGroup) []compareRuleType { return duplicates } -func ruleMetric(rule rulefmt.RuleNode) string { - if rule.Alert.Value != "" { - return rule.Alert.Value +func ruleMetric(rule rulefmt.Rule) string { + if rule.Alert != "" { + return rule.Alert } - return rule.Record.Value + return rule.Record } var checkMetricsUsage = strings.TrimSpace(` diff --git a/model/rulefmt/rulefmt.go b/model/rulefmt/rulefmt.go index 3fc3fa0437..7b99c5fa33 100644 --- a/model/rulefmt/rulefmt.go +++ b/model/rulefmt/rulefmt.go @@ -92,7 +92,7 @@ type RuleGroups struct { } type ruleGroups struct { - Groups []yaml.Node `yaml:"groups"` + Groups []ruleGroupNode `yaml:"groups"` } // Validate validates all rules in the rule groups. @@ -128,9 +128,9 @@ func (g *RuleGroups) Validate(node ruleGroups) (errs []error) { set[g.Name] = struct{}{} for i, r := range g.Rules { - for _, node := range g.Rules[i].Validate() { - var ruleName yaml.Node - if r.Alert.Value != "" { + for _, node := range r.Validate(node.Groups[j].Rules[i]) { + var ruleName string + if r.Alert != "" { ruleName = r.Alert } else { ruleName = r.Record @@ -138,7 +138,7 @@ func (g *RuleGroups) Validate(node ruleGroups) (errs []error) { errs = append(errs, &Error{ Group: g.Name, Rule: i + 1, - RuleName: ruleName.Value, + RuleName: ruleName, Err: node, }) } @@ -154,7 +154,18 @@ type RuleGroup struct { Interval model.Duration `yaml:"interval,omitempty"` QueryOffset *model.Duration `yaml:"query_offset,omitempty"` Limit int `yaml:"limit,omitempty"` - Rules []RuleNode `yaml:"rules"` + Rules []Rule `yaml:"rules"` + Labels map[string]string `yaml:"labels,omitempty"` +} + +// ruleGroupNode adds yaml.v3 layer to support line and columns outputs for invalid rule groups. +type ruleGroupNode struct { + yaml.Node + Name string `yaml:"name"` + Interval model.Duration `yaml:"interval,omitempty"` + QueryOffset *model.Duration `yaml:"query_offset,omitempty"` + Limit int `yaml:"limit,omitempty"` + Rules []ruleNode `yaml:"rules"` Labels map[string]string `yaml:"labels,omitempty"` } @@ -169,8 +180,8 @@ type Rule struct { Annotations map[string]string `yaml:"annotations,omitempty"` } -// RuleNode adds yaml.v3 layer to support line and column outputs for invalid rules. -type RuleNode struct { +// ruleNode adds yaml.v3 layer to support line and column outputs for invalid rules. +type ruleNode struct { Record yaml.Node `yaml:"record,omitempty"` Alert yaml.Node `yaml:"alert,omitempty"` Expr yaml.Node `yaml:"expr"` @@ -181,64 +192,64 @@ type RuleNode struct { } // Validate the rule and return a list of encountered errors. -func (r *RuleNode) Validate() (nodes []WrappedError) { - if r.Record.Value != "" && r.Alert.Value != "" { +func (r *Rule) Validate(node ruleNode) (nodes []WrappedError) { + if r.Record != "" && r.Alert != "" { nodes = append(nodes, WrappedError{ err: errors.New("only one of 'record' and 'alert' must be set"), - node: &r.Record, - nodeAlt: &r.Alert, + node: &node.Record, + nodeAlt: &node.Alert, }) } - if r.Record.Value == "" && r.Alert.Value == "" { + if r.Record == "" && r.Alert == "" { nodes = append(nodes, WrappedError{ err: errors.New("one of 'record' or 'alert' must be set"), - node: &r.Record, - nodeAlt: &r.Alert, + node: &node.Record, + nodeAlt: &node.Alert, }) } - if r.Expr.Value == "" { + if r.Expr == "" { nodes = append(nodes, WrappedError{ err: errors.New("field 'expr' must be set in rule"), - node: &r.Expr, + node: &node.Expr, }) - } else if _, err := parser.ParseExpr(r.Expr.Value); err != nil { + } else if _, err := parser.ParseExpr(r.Expr); err != nil { nodes = append(nodes, WrappedError{ err: fmt.Errorf("could not parse expression: %w", err), - node: &r.Expr, + node: &node.Expr, }) } - if r.Record.Value != "" { + if r.Record != "" { if len(r.Annotations) > 0 { nodes = append(nodes, WrappedError{ err: errors.New("invalid field 'annotations' in recording rule"), - node: &r.Record, + node: &node.Record, }) } if r.For != 0 { nodes = append(nodes, WrappedError{ err: errors.New("invalid field 'for' in recording rule"), - node: &r.Record, + node: &node.Record, }) } if r.KeepFiringFor != 0 { nodes = append(nodes, WrappedError{ err: errors.New("invalid field 'keep_firing_for' in recording rule"), - node: &r.Record, + node: &node.Record, }) } - if !model.IsValidMetricName(model.LabelValue(r.Record.Value)) { + if !model.IsValidMetricName(model.LabelValue(r.Record)) { nodes = append(nodes, WrappedError{ - err: fmt.Errorf("invalid recording rule name: %s", r.Record.Value), - node: &r.Record, + err: fmt.Errorf("invalid recording rule name: %s", r.Record), + node: &node.Record, }) } // While record is a valid UTF-8 it's common mistake to put PromQL expression in the record name. // Disallow "{}" chars. - if strings.Contains(r.Record.Value, "{") || strings.Contains(r.Record.Value, "}") { + if strings.Contains(r.Record, "{") || strings.Contains(r.Record, "}") { nodes = append(nodes, WrappedError{ - err: fmt.Errorf("braces present in the recording rule name; should it be in expr?: %s", r.Record.Value), - node: &r.Record, + err: fmt.Errorf("braces present in the recording rule name; should it be in expr?: %s", r.Record), + node: &node.Record, }) } } @@ -274,8 +285,8 @@ func (r *RuleNode) Validate() (nodes []WrappedError) { // testTemplateParsing checks if the templates used in labels and annotations // of the alerting rules are parsed correctly. -func testTemplateParsing(rl *RuleNode) (errs []error) { - if rl.Alert.Value == "" { +func testTemplateParsing(rl *Rule) (errs []error) { + if rl.Alert == "" { // Not an alerting rule. return errs } @@ -292,7 +303,7 @@ func testTemplateParsing(rl *RuleNode) (errs []error) { tmpl := template.NewTemplateExpander( context.TODO(), strings.Join(append(defs, text), ""), - "__alert_"+rl.Alert.Value, + "__alert_"+rl.Alert, tmplData, model.Time(timestamp.FromTime(time.Now())), nil, diff --git a/model/rulefmt/rulefmt_test.go b/model/rulefmt/rulefmt_test.go index 9e191820c5..e8ac3077bd 100644 --- a/model/rulefmt/rulefmt_test.go +++ b/model/rulefmt/rulefmt_test.go @@ -33,6 +33,33 @@ func TestParseFileSuccess(t *testing.T) { require.Empty(t, errs, "unexpected errors parsing file") } +func TestParseFileSuccessWithAliases(t *testing.T) { + exprString := `sum without(instance) (rate(errors_total[5m])) +/ +sum without(instance) (rate(requests_total[5m])) +` + rgs, errs := ParseFile("testdata/test_aliases.yaml", false) + require.Empty(t, errs, "unexpected errors parsing file") + for _, rg := range rgs.Groups { + require.Equal(t, "HighAlert", rg.Rules[0].Alert) + require.Equal(t, "critical", rg.Rules[0].Labels["severity"]) + require.Equal(t, "stuff's happening with {{ $.labels.service }}", rg.Rules[0].Annotations["description"]) + + require.Equal(t, "new_metric", rg.Rules[1].Record) + + require.Equal(t, "HighAlert", rg.Rules[2].Alert) + require.Equal(t, "critical", rg.Rules[2].Labels["severity"]) + require.Equal(t, "stuff's happening with {{ $.labels.service }}", rg.Rules[0].Annotations["description"]) + + require.Equal(t, "HighAlert2", rg.Rules[3].Alert) + require.Equal(t, "critical", rg.Rules[3].Labels["severity"]) + + for _, rule := range rg.Rules { + require.Equal(t, exprString, rule.Expr) + } + } +} + func TestParseFileFailure(t *testing.T) { for _, c := range []struct { filename string diff --git a/model/rulefmt/testdata/test_aliases.yaml b/model/rulefmt/testdata/test_aliases.yaml new file mode 100644 index 0000000000..326259b0b6 --- /dev/null +++ b/model/rulefmt/testdata/test_aliases.yaml @@ -0,0 +1,57 @@ +groups: + - name: my-group-name + interval: 30s # defaults to global interval + rules: + - &highalert + alert: &alertname HighAlert + expr: &expr | + sum without(instance) (rate(errors_total[5m])) + / + sum without(instance) (rate(requests_total[5m])) + for: 5m + labels: + severity: &severity critical + annotations: + description: &description "stuff's happening with {{ $.labels.service }}" + + # Mix recording rules in the same list + - record: &recordname "new_metric" + expr: *expr + labels: + abc: edf + uvw: xyz + + - alert: *alertname + expr: *expr + for: 5m + labels: + severity: *severity + annotations: + description: *description + + - <<: *highalert + alert: HighAlert2 + + - name: my-another-name + interval: 30s # defaults to global interval + rules: + - alert: *alertname + expr: *expr + for: 5m + labels: + severity: *severity + annotations: + description: *description + + - record: *recordname + expr: *expr + + - alert: *alertname + expr: *expr + labels: + severity: *severity + annotations: + description: *description + + - <<: *highalert + alert: HighAlert2 diff --git a/rules/manager.go b/rules/manager.go index b1d3e8e3d6..6e96df2168 100644 --- a/rules/manager.go +++ b/rules/manager.go @@ -324,16 +324,16 @@ func (m *Manager) LoadGroups( rules := make([]Rule, 0, len(rg.Rules)) for _, r := range rg.Rules { - expr, err := m.opts.GroupLoader.Parse(r.Expr.Value) + expr, err := m.opts.GroupLoader.Parse(r.Expr) if err != nil { return nil, []error{fmt.Errorf("%s: %w", fn, err)} } mLabels := FromMaps(rg.Labels, r.Labels) - if r.Alert.Value != "" { + if r.Alert != "" { rules = append(rules, NewAlertingRule( - r.Alert.Value, + r.Alert, expr, time.Duration(r.For), time.Duration(r.KeepFiringFor), @@ -347,7 +347,7 @@ func (m *Manager) LoadGroups( continue } rules = append(rules, NewRecordingRule( - r.Record.Value, + r.Record, expr, mLabels, )) diff --git a/rules/manager_test.go b/rules/manager_test.go index 34b77dd40c..6c32b6d0f1 100644 --- a/rules/manager_test.go +++ b/rules/manager_test.go @@ -842,7 +842,7 @@ func TestUpdate(t *testing.T) { // Change group rules and reload. for i, g := range rgs.Groups { for j, r := range g.Rules { - rgs.Groups[i].Rules[j].Expr.SetString(fmt.Sprintf("%s * 0", r.Expr.Value)) + rgs.Groups[i].Rules[j].Expr = fmt.Sprintf("%s * 0", r.Expr) } } reloadAndValidate(rgs, t, tmpFile, ruleManager, ogs) @@ -869,9 +869,9 @@ func formatRules(r *rulefmt.RuleGroups) ruleGroupsTest { rtmp := []rulefmt.Rule{} for _, r := range g.Rules { rtmp = append(rtmp, rulefmt.Rule{ - Record: r.Record.Value, - Alert: r.Alert.Value, - Expr: r.Expr.Value, + Record: r.Record, + Alert: r.Alert, + Expr: r.Expr, For: r.For, Labels: r.Labels, Annotations: r.Annotations,