From 0d2ee90ff1c2ea6b1c8ecb2b7e0f8db0aa64a2ce Mon Sep 17 00:00:00 2001 From: Moustafa Baiou Date: Thu, 28 Aug 2025 13:04:57 -0400 Subject: [PATCH] Alerting: Fix copying of recording rule fields Recording rule fields were not being copied correctly when duplicating an alert rule. This manifests as missing `TargetDataSourceUID` fields from the `Record` part of the rule when rules in a group are re-ordered. Added some additional tests to ensure we cover the generation of recording rules in tests and fixed the copying logic to ensure all fields are copied correctly. (cherry picked from commit c73b3ccf6e4298ded84c34b8e53f5a70916ed3f5) --- pkg/services/ngalert/models/alert_rule.go | 5 +- .../ngalert/models/alert_rule_test.go | 49 +++++++++++++++++++ pkg/services/ngalert/models/testing.go | 12 +++++ pkg/services/ngalert/tests/fakes/rules.go | 6 ++- 4 files changed, 69 insertions(+), 3 deletions(-) diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index fb5ecad3970..ce6e7f10b00 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -794,8 +794,9 @@ func (alertRule *AlertRule) Copy() *AlertRule { if alertRule.Record != nil { result.Record = &Record{ - From: alertRule.Record.From, - Metric: alertRule.Record.Metric, + From: alertRule.Record.From, + Metric: alertRule.Record.Metric, + TargetDatasourceUID: alertRule.Record.TargetDatasourceUID, } } diff --git a/pkg/services/ngalert/models/alert_rule_test.go b/pkg/services/ngalert/models/alert_rule_test.go index 5c702ddedec..fd3c10451ac 100644 --- a/pkg/services/ngalert/models/alert_rule_test.go +++ b/pkg/services/ngalert/models/alert_rule_test.go @@ -1012,6 +1012,13 @@ func TestAlertRuleCopy(t *testing.T) { copied := rule.Copy() require.NotSame(t, rule.Metadata.PrometheusStyleRule, copied.Metadata.PrometheusStyleRule) }) + t.Run("should return an exact copy of recording rule", func(t *testing.T) { + for i := 0; i < 100; i++ { + rule := RuleGen.With(RuleGen.WithAllRecordingRules()).GenerateRef() + copied := rule.Copy() + require.Empty(t, rule.Diff(copied)) + } + }) } // This test makes sure the default generator @@ -1051,6 +1058,48 @@ func TestGeneratorFillsAllFields(t *testing.T) { require.FailNow(t, "AlertRule generator does not populate fields", "skipped fields: %v", maps.Keys(fields)) } +func TestGeneratorFillsAllRecordingRuleFields(t *testing.T) { + ignoredFields := map[string]struct{}{ + "ID": {}, + "IsPaused": {}, + "NoDataState": {}, + "ExecErrState": {}, + "Condition": {}, + "KeepFiringFor": {}, + "MissingSeriesEvalsToResolve": {}, + "For": {}, + "NotificationSettings": {}, + } + + tpe := reflect.TypeOf(AlertRule{}) + fields := make(map[string]struct{}, tpe.NumField()) + for i := 0; i < tpe.NumField(); i++ { + if _, ok := ignoredFields[tpe.Field(i).Name]; ok { + continue + } + fields[tpe.Field(i).Name] = struct{}{} + } + + for i := 0; i < 1000; i++ { + rule := RuleGen.With(RuleGen.WithAllRecordingRules()).Generate() + v := reflect.ValueOf(rule) + + for j := 0; j < tpe.NumField(); j++ { + field := tpe.Field(j) + value := v.Field(j) + if !value.IsValid() || value.Kind() == reflect.Ptr && value.IsNil() || value.IsZero() { + continue + } + delete(fields, field.Name) + if len(fields) == 0 { + return + } + } + } + + require.FailNow(t, "AlertRule generator does not populate fields", "skipped fields: %v", maps.Keys(fields)) +} + func TestValidateAlertRule(t *testing.T) { t.Run("keepFiringFor", func(t *testing.T) { testCases := []struct { diff --git a/pkg/services/ngalert/models/testing.go b/pkg/services/ngalert/models/testing.go index b81e9225fa6..424144dd0f8 100644 --- a/pkg/services/ngalert/models/testing.go +++ b/pkg/services/ngalert/models/testing.go @@ -561,6 +561,14 @@ func (a *AlertRuleMutators) WithAllRecordingRules() AlertRuleMutator { } } +func (a *AlertRuleMutators) WithoutTargetDataSource() AlertRuleMutator { + return func(rule *AlertRule) { + if rule.Record != nil { + rule.Record.TargetDatasourceUID = "" + } + } +} + func (a *AlertRuleMutators) WithMetric(metric string) AlertRuleMutator { return func(rule *AlertRule) { if rule.Record == nil { @@ -1363,10 +1371,14 @@ func ConvertToRecordingRule(rule *AlertRule) { if rule.Record.Metric == "" { rule.Record.Metric = fmt.Sprintf("some_metric_%s", util.GenerateShortUID()) } + if rule.Record.TargetDatasourceUID == "" { + rule.Record.TargetDatasourceUID = util.GenerateShortUID() + } rule.Condition = "" rule.NoDataState = "" rule.ExecErrState = "" rule.For = 0 + rule.KeepFiringFor = 0 rule.NotificationSettings = nil rule.MissingSeriesEvalsToResolve = nil } diff --git a/pkg/services/ngalert/tests/fakes/rules.go b/pkg/services/ngalert/tests/fakes/rules.go index cc5b70bf0b3..ba23e27048d 100644 --- a/pkg/services/ngalert/tests/fakes/rules.go +++ b/pkg/services/ngalert/tests/fakes/rules.go @@ -229,7 +229,11 @@ func (f *RuleStore) ListAlertRules(_ context.Context, q *models.ListAlertRulesQu } } - ruleList = append(ruleList, r) + if q.ReceiverName != "" && (len(r.NotificationSettings) < 1 || r.NotificationSettings[0].Receiver != q.ReceiverName) { + continue + } + copyR := models.CopyRule(r) + ruleList = append(ruleList, copyR) } return ruleList, nil