From 23d5cf94c20b91c318f980e52a03908235b6c1bb Mon Sep 17 00:00:00 2001 From: Paurush Garg Date: Fri, 24 Oct 2025 16:28:27 -0700 Subject: [PATCH 1/4] Adding parser for xfunctions in rule validation Signed-off-by: Paurush Garg --- pkg/ruler/manager.go | 46 +++++++++++++++++------ pkg/ruler/xfunction_validation_test.go | 51 ++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 11 deletions(-) create mode 100644 pkg/ruler/xfunction_validation_test.go 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/xfunction_validation_test.go b/pkg/ruler/xfunction_validation_test.go new file mode 100644 index 00000000000..f48b279ec2d --- /dev/null +++ b/pkg/ruler/xfunction_validation_test.go @@ -0,0 +1,51 @@ +package ruler + +import ( + "testing" + + "github.com/prometheus/prometheus/model/rulefmt" +) + +func TestValidateRuleGroup_AcceptsXFunctions(t *testing.T) { + manager := &DefaultMultiTenantManager{} + + // Test rule with XFunction (should now pass) + ruleGroupWithXFunc := rulefmt.RuleGroup{ + Name: "test_group", + Rules: []rulefmt.Rule{ + { + Alert: "TestAlert", + Expr: "xrate(cpu_usage[5m]) > 0.8", // XFunction + }, + }, + } + + errs := manager.ValidateRuleGroup(ruleGroupWithXFunc) + + // Should have no validation errors now + 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) + } +} From 9375da2482cfb40f78de71c4f8ba0df57e7ed789 Mon Sep 17 00:00:00 2001 From: Paurush Garg Date: Wed, 12 Nov 2025 15:53:25 -0800 Subject: [PATCH 2/4] Add ChangeLog Signed-off-by: Paurush Garg --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 021ea74325e..7ee9a19752c 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 +* [FEATURE] Ruler: Add XFunctions validation support. #7111 ## 1.20.0 2025-11-10 From 315cc30a04082d1de2fb34cb7dc3418e1e39c23e Mon Sep 17 00:00:00 2001 From: Paurush Garg Date: Wed, 12 Nov 2025 17:01:15 -0800 Subject: [PATCH 3/4] Adding tests and moving tests to manager_test Signed-off-by: Paurush Garg --- pkg/ruler/manager_test.go | 91 ++++++++++++++++++++++++++ pkg/ruler/xfunction_validation_test.go | 51 --------------- 2 files changed, 91 insertions(+), 51 deletions(-) delete mode 100644 pkg/ruler/xfunction_validation_test.go 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") +} diff --git a/pkg/ruler/xfunction_validation_test.go b/pkg/ruler/xfunction_validation_test.go deleted file mode 100644 index f48b279ec2d..00000000000 --- a/pkg/ruler/xfunction_validation_test.go +++ /dev/null @@ -1,51 +0,0 @@ -package ruler - -import ( - "testing" - - "github.com/prometheus/prometheus/model/rulefmt" -) - -func TestValidateRuleGroup_AcceptsXFunctions(t *testing.T) { - manager := &DefaultMultiTenantManager{} - - // Test rule with XFunction (should now pass) - ruleGroupWithXFunc := rulefmt.RuleGroup{ - Name: "test_group", - Rules: []rulefmt.Rule{ - { - Alert: "TestAlert", - Expr: "xrate(cpu_usage[5m]) > 0.8", // XFunction - }, - }, - } - - errs := manager.ValidateRuleGroup(ruleGroupWithXFunc) - - // Should have no validation errors now - 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) - } -} From f7a7144b04f8909646481c8f0d0c7476cf4d341c Mon Sep 17 00:00:00 2001 From: Paurush Garg Date: Thu, 13 Nov 2025 11:23:47 -0800 Subject: [PATCH 4/4] Update Chnagelog Signed-off-by: Paurush Garg --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ee9a19752c..984b1165900 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +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 -* [FEATURE] Ruler: Add XFunctions validation support. #7111 +* [BUGFIX] Ruler: Add XFunctions validation support. #7111 ## 1.20.0 2025-11-10