diff --git a/CHANGELOG.md b/CHANGELOG.md index 021ea74325e..984b1165900 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ * [ENHANCEMENT] Distributor: Add a label references validation for remote write v2 request. #7074 * [ENHANCEMENT] Distributor: Add count, spans, and buckets validations for native histogram. #7072 * [BUGFIX] Ring: Change DynamoDB KV to retry indefinitely for WatchKey. #7088 +* [BUGFIX] Ruler: Add XFunctions validation support. #7111 ## 1.20.0 2025-11-10 diff --git a/pkg/ruler/manager.go b/pkg/ruler/manager.go index 8a23b37f333..49d259956ac 100644 --- a/pkg/ruler/manager.go +++ b/pkg/ruler/manager.go @@ -6,6 +6,7 @@ import ( "io" "net/http" "os" + "strings" "sync" "time" @@ -24,6 +25,7 @@ import ( "github.com/weaveworks/common/user" "golang.org/x/net/context/ctxhttp" + "github.com/cortexproject/cortex/pkg/parser" "github.com/cortexproject/cortex/pkg/ring/client" "github.com/cortexproject/cortex/pkg/ruler/rulespb" ) @@ -457,19 +459,41 @@ func (*DefaultMultiTenantManager) ValidateRuleGroup(g rulefmt.RuleGroup) []error } for i, r := range g.Rules { + // Use Cortex parser for expression validation (supports XFunctions) + if r.Expr != "" { + if _, err := parser.ParseExpr(r.Expr); err != nil { + var ruleName string + if r.Alert != "" { + ruleName = r.Alert + } else { + ruleName = r.Record + } + errs = append(errs, &rulefmt.Error{ + Group: g.Name, + Rule: i, + RuleName: ruleName, + Err: rulefmt.WrappedError{}, + }) + } + } + + // Validate other rule fields using Prometheus validation for _, err := range r.Validate(rulefmt.RuleNode{}) { - var ruleName string - if r.Alert != "" { - ruleName = r.Alert - } else { - ruleName = r.Record + // Skip expression validation errors since we handle them above + if !strings.Contains(err.Error(), "could not parse expression") { + var ruleName string + if r.Alert != "" { + ruleName = r.Alert + } else { + ruleName = r.Record + } + errs = append(errs, &rulefmt.Error{ + Group: g.Name, + Rule: i, + RuleName: ruleName, + Err: err, + }) } - errs = append(errs, &rulefmt.Error{ - Group: g.Name, - Rule: i, - RuleName: ruleName, - Err: err, - }) } } diff --git a/pkg/ruler/manager_test.go b/pkg/ruler/manager_test.go index 8abf3c29366..845d0ffc203 100644 --- a/pkg/ruler/manager_test.go +++ b/pkg/ruler/manager_test.go @@ -9,6 +9,7 @@ import ( "github.com/go-kit/log" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/prometheus/model/labels" + "github.com/prometheus/prometheus/model/rulefmt" "github.com/prometheus/prometheus/notifier" promRules "github.com/prometheus/prometheus/rules" "github.com/stretchr/testify/require" @@ -361,3 +362,93 @@ func (m *mockRulesManager) Stop() { m.running.Store(false) close(m.done) } + +func TestValidateRuleGroup_AcceptsXFunctions(t *testing.T) { + manager := &DefaultMultiTenantManager{} + + // Test rule with XFunction + ruleGroupWithXFunc := rulefmt.RuleGroup{ + Name: "test_group", + Rules: []rulefmt.Rule{ + { + Alert: "TestAlert", + Expr: "xrate(cpu_usage[5m]) > 0.8", // XFunction + }, + }, + } + + errs := manager.ValidateRuleGroup(ruleGroupWithXFunc) + + // Should not have validation errors + if len(errs) != 0 { + t.Fatalf("Expected no validation errors for XFunction after fix, got: %v", errs) + } +} + +func TestValidateRuleGroup_AcceptsStandardFunctions(t *testing.T) { + manager := &DefaultMultiTenantManager{} + + // Test rule with standard function (should pass) + ruleGroupStandard := rulefmt.RuleGroup{ + Name: "test_group", + Rules: []rulefmt.Rule{ + { + Alert: "TestAlert", + Expr: "rate(cpu_usage[5m]) > 0.8", // Standard function + }, + }, + } + + errs := manager.ValidateRuleGroup(ruleGroupStandard) + + // Should have no validation errors + if len(errs) != 0 { + t.Fatalf("Expected no validation errors for standard function, got: %v", errs) + } +} + +func TestValidateRuleGroup_RejectsInvalidRules(t *testing.T) { + manager := &DefaultMultiTenantManager{} + + // Test rule with invalid expression syntax + ruleGroupInvalid := rulefmt.RuleGroup{ + Name: "test_group", + Rules: []rulefmt.Rule{ + { + Alert: "TestAlert", + Expr: "invalid_syntax_here >", // Invalid expression + }, + }, + } + + errs := manager.ValidateRuleGroup(ruleGroupInvalid) + + // Should have validation errors and they should be properly propagated + require.NotEmpty(t, errs, "Expected validation errors for invalid expression") + // Verify the error is a rulefmt.Error with proper group information + ruleErr, ok := errs[0].(*rulefmt.Error) + require.True(t, ok, "Error should be of type *rulefmt.Error") + require.Equal(t, "test_group", ruleErr.Group, "Error should contain correct group name") + require.Equal(t, "TestAlert", ruleErr.RuleName, "Error should contain correct rule name") +} + +func TestValidateRuleGroup_RejectsEmptyGroupName(t *testing.T) { + manager := &DefaultMultiTenantManager{} + + // Test rule group with empty name + ruleGroupEmptyName := rulefmt.RuleGroup{ + Name: "", // Empty name + Rules: []rulefmt.Rule{ + { + Alert: "TestAlert", + Expr: "rate(cpu_usage[5m]) > 0.8", + }, + }, + } + + errs := manager.ValidateRuleGroup(ruleGroupEmptyName) + + // Should have validation errors + require.NotEmpty(t, errs, "Expected validation errors for empty group name") + require.Contains(t, errs[0].Error(), "rule group name must not be empty", "Error should mention empty group name") +}