From f85ce1342a3f7868441962075524d226ea65f4fe 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 | 11 +++++ pkg/services/ngalert/tests/fakes/rules.go | 6 ++- 4 files changed, 68 insertions(+), 3 deletions(-) diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index 14f999202e3..66efe741635 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -766,8 +766,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 2a2dd0abf66..71f29e2b368 100644 --- a/pkg/services/ngalert/models/alert_rule_test.go +++ b/pkg/services/ngalert/models/alert_rule_test.go @@ -980,6 +980,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 @@ -1019,6 +1026,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("ExecErrState & NoDataState", func(t *testing.T) { testCases := []struct { diff --git a/pkg/services/ngalert/models/testing.go b/pkg/services/ngalert/models/testing.go index eb9d838f8df..10e2d9276d0 100644 --- a/pkg/services/ngalert/models/testing.go +++ b/pkg/services/ngalert/models/testing.go @@ -537,6 +537,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 { @@ -1338,6 +1346,9 @@ 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 = "" diff --git a/pkg/services/ngalert/tests/fakes/rules.go b/pkg/services/ngalert/tests/fakes/rules.go index 10bc282e8a8..98ac7df025e 100644 --- a/pkg/services/ngalert/tests/fakes/rules.go +++ b/pkg/services/ngalert/tests/fakes/rules.go @@ -220,7 +220,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