From 92d6762a3a5736f5ab9bcec24b79d7b5ea9ad31e Mon Sep 17 00:00:00 2001 From: Yuri Tseretyan Date: Fri, 24 Jan 2025 12:09:17 -0500 Subject: [PATCH] Alerting: Store information about user that created\updated alert rule (#99395) * introduce new fields created_by in rule tables * update domain model and compat layer to support UpdatedBy * add alert rule generator mutators for UpdatedBy * ignore UpdatedBy in diff and hash calculation * Add user context to alert rule insert/update operations Updated InsertAlertRules and UpdateAlertRules methods to accept a user context parameter. This change ensures auditability and better tracking of user actions when creating or updating alert rules. Adjusted all relevant calls and interfaces to pass the user context accordingly. * set UpdatedBy in PreSave because this is where Updated is set * Use nil userID for system-initiated updates This ensures differentiation between system and user-initiated changes for better traceability and clarity in update origins. --------- Signed-off-by: Yuri Tseretyan --- .../loki/historian_store_test.go | 4 +- pkg/services/folder/folderimpl/folder_test.go | 2 +- pkg/services/ngalert/api/api_ruler.go | 4 +- pkg/services/ngalert/api/persist.go | 4 +- pkg/services/ngalert/models/alert_rule.go | 19 ++- .../ngalert/models/alert_rule_test.go | 5 + pkg/services/ngalert/models/testing.go | 13 ++ .../ngalert/provisioning/alert_rules.go | 10 +- .../ngalert/provisioning/alert_rules_test.go | 17 ++- pkg/services/ngalert/provisioning/persist.go | 4 +- .../ngalert/schedule/registry_test.go | 1 + pkg/services/ngalert/store/alert_rule.go | 16 +- pkg/services/ngalert/store/alert_rule_test.go | 143 ++++++++++++++---- pkg/services/ngalert/store/compat.go | 10 ++ pkg/services/ngalert/store/deltas.go | 2 +- pkg/services/ngalert/store/models.go | 2 + pkg/services/ngalert/tests/fakes/rules.go | 4 +- pkg/services/ngalert/tests/util.go | 2 +- .../sqlstore/migrations/migrations.go | 2 + .../migrations/ualert/rule_creator_mig.go | 20 +++ .../federated/federatedtests/stats_test.go | 2 +- 21 files changed, 224 insertions(+), 62 deletions(-) create mode 100644 pkg/services/sqlstore/migrations/ualert/rule_creator_mig.go diff --git a/pkg/services/annotations/annotationsimpl/loki/historian_store_test.go b/pkg/services/annotations/annotationsimpl/loki/historian_store_test.go index 242dbcea396..f4e4bf6630a 100644 --- a/pkg/services/annotations/annotationsimpl/loki/historian_store_test.go +++ b/pkg/services/annotations/annotationsimpl/loki/historian_store_test.go @@ -636,7 +636,7 @@ func createAlertRule(t *testing.T, sql *sqlstore.SQLStore, title string, generat rule.PanelID = nil ruleStore := store.SetupStoreForTesting(t, sql) - ids, err := ruleStore.InsertAlertRules(context.Background(), []ngmodels.AlertRule{rule}) + ids, err := ruleStore.InsertAlertRules(context.Background(), nil, []ngmodels.AlertRule{rule}) require.NoError(t, err) result, err := ruleStore.GetAlertRuleByUID(context.Background(), &ngmodels.GetAlertRuleByUIDQuery{OrgID: rule.OrgID, UID: ids[0].UID}) require.NoError(t, err) @@ -668,7 +668,7 @@ func createAlertRuleFromDashboard(t *testing.T, sql *sqlstore.SQLStore, title st rule.PanelID = panelID } ruleStore := store.SetupStoreForTesting(t, sql) - ids, err := ruleStore.InsertAlertRules(context.Background(), []ngmodels.AlertRule{rule}) + ids, err := ruleStore.InsertAlertRules(context.Background(), nil, []ngmodels.AlertRule{rule}) require.NoError(t, err) result, err := ruleStore.GetAlertRuleByUID(context.Background(), &ngmodels.GetAlertRuleByUIDQuery{OrgID: rule.OrgID, UID: ids[0].UID}) require.NoError(t, err) diff --git a/pkg/services/folder/folderimpl/folder_test.go b/pkg/services/folder/folderimpl/folder_test.go index 852542b9ff4..a44b75114e5 100644 --- a/pkg/services/folder/folderimpl/folder_test.go +++ b/pkg/services/folder/folderimpl/folder_test.go @@ -2581,7 +2581,7 @@ func createRule(t *testing.T, store *ngstore.DBstore, folderUID, title string) * gen.WithNamespaceUID(folderUID), gen.WithIntervalSeconds(10), ).Generate() - ids, err := store.InsertAlertRules(context.Background(), []models.AlertRule{rule}) + ids, err := store.InsertAlertRules(context.Background(), nil, []models.AlertRule{rule}) require.NoError(t, err) result, err := store.GetAlertRuleByUID(context.Background(), &models.GetAlertRuleByUIDQuery{OrgID: orgID, UID: ids[0].UID}) diff --git a/pkg/services/ngalert/api/api_ruler.go b/pkg/services/ngalert/api/api_ruler.go index b70801e38d6..55783cfe853 100644 --- a/pkg/services/ngalert/api/api_ruler.go +++ b/pkg/services/ngalert/api/api_ruler.go @@ -443,7 +443,7 @@ func (srv RulerSrv) updateAlertRulesInGroup(c *contextmodel.ReqContext, groupKey New: *update.New, }) } - err = srv.store.UpdateAlertRules(tranCtx, updates) + err = srv.store.UpdateAlertRules(tranCtx, ngmodels.NewUserUID(c.SignedInUser), updates) if err != nil { return fmt.Errorf("failed to update rules: %w", err) } @@ -454,7 +454,7 @@ func (srv RulerSrv) updateAlertRulesInGroup(c *contextmodel.ReqContext, groupKey for _, rule := range finalChanges.New { inserts = append(inserts, *rule) } - added, err := srv.store.InsertAlertRules(tranCtx, inserts) + added, err := srv.store.InsertAlertRules(tranCtx, ngmodels.NewUserUID(c.SignedInUser), inserts) if err != nil { return fmt.Errorf("failed to add rules: %w", err) } diff --git a/pkg/services/ngalert/api/persist.go b/pkg/services/ngalert/api/persist.go index faecfa2ccc4..b05abd5f6df 100644 --- a/pkg/services/ngalert/api/persist.go +++ b/pkg/services/ngalert/api/persist.go @@ -22,8 +22,8 @@ type RuleStore interface { // InsertAlertRules will insert all alert rules passed into the function // and return the map of uuid to id. - InsertAlertRules(ctx context.Context, rule []ngmodels.AlertRule) ([]ngmodels.AlertRuleKeyWithId, error) - UpdateAlertRules(ctx context.Context, rule []ngmodels.UpdateRule) error + InsertAlertRules(ctx context.Context, user *ngmodels.UserUID, rules []ngmodels.AlertRule) ([]ngmodels.AlertRuleKeyWithId, error) + UpdateAlertRules(ctx context.Context, user *ngmodels.UserUID, rules []ngmodels.UpdateRule) error DeleteAlertRulesByUID(ctx context.Context, orgID int64, ruleUID ...string) error // IncreaseVersionForAllRulesInNamespaces Increases version for all rules that have specified namespace uids diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index ef8d15827df..07a5eb86280 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -243,6 +243,21 @@ func SortAlertRuleGroupWithFolderTitle(g []AlertRuleGroupWithFolderFullpath) { }) } +type UserUID string + +func NewUserUID(requester interface{ GetIdentifier() string }) *UserUID { + // use anonymous interface to abstract from identity package, which is part of apimachinery + if requester == nil { + return nil + } + identifier := requester.GetIdentifier() + if identifier == "" { + return nil + } + userUID := UserUID(identifier) + return &userUID +} + // AlertRule is the model for alert rules in unified alerting. type AlertRule struct { ID int64 @@ -251,6 +266,7 @@ type AlertRule struct { Condition string Data []AlertQuery Updated time.Time + UpdatedBy *UserUID IntervalSeconds int64 Version int64 UID string @@ -535,7 +551,7 @@ func (alertRule *AlertRule) GetGroupKey() AlertRuleGroupKey { } // PreSave sets default values and loads the updated model for each alert query. -func (alertRule *AlertRule) PreSave(timeNow func() time.Time) error { +func (alertRule *AlertRule) PreSave(timeNow func() time.Time, userUID *UserUID) error { for i, q := range alertRule.Data { err := q.PreSave() if err != nil { @@ -544,6 +560,7 @@ func (alertRule *AlertRule) PreSave(timeNow func() time.Time) error { alertRule.Data[i] = q } alertRule.Updated = timeNow() + alertRule.UpdatedBy = userUID return nil } diff --git a/pkg/services/ngalert/models/alert_rule_test.go b/pkg/services/ngalert/models/alert_rule_test.go index bb7bfdadf33..7bb1b44a39b 100644 --- a/pkg/services/ngalert/models/alert_rule_test.go +++ b/pkg/services/ngalert/models/alert_rule_test.go @@ -445,6 +445,11 @@ func TestDiff(t *testing.T) { assert.Equal(t, rule2.Updated, diff[0].Right.Interface()) difCnt++ } + if rule1.UpdatedBy != rule2.UpdatedBy { + diff := diffs.GetDiffsForField("UpdatedBy") + assert.Len(t, diff, 1) + difCnt++ + } if rule1.IntervalSeconds != rule2.IntervalSeconds { diff := diffs.GetDiffsForField("IntervalSeconds") assert.Len(t, diff, 1) diff --git a/pkg/services/ngalert/models/testing.go b/pkg/services/ngalert/models/testing.go index adc27759b58..e5b988cf084 100644 --- a/pkg/services/ngalert/models/testing.go +++ b/pkg/services/ngalert/models/testing.go @@ -96,6 +96,11 @@ func (g *AlertRuleGenerator) Generate() AlertRule { ns = append(ns, NotificationSettingsGen()()) } + var updatedBy *UserUID + if rand.Int63()%2 == 0 { + updatedBy = util.Pointer(UserUID(util.GenerateShortUID())) + } + rule := AlertRule{ ID: 0, OrgID: rand.Int63n(1500) + 1, // Prevent OrgID=0 as this does not pass alert rule validation. @@ -103,6 +108,7 @@ func (g *AlertRuleGenerator) Generate() AlertRule { Condition: "A", Data: []AlertQuery{g.GenerateQuery()}, Updated: time.Now().Add(-time.Duration(rand.Intn(100) + 1)), + UpdatedBy: updatedBy, IntervalSeconds: rand.Int63n(60) + 1, Version: rand.Int63n(1500), // Don't generate a rule ID too big for postgres UID: util.GenerateShortUID(), @@ -538,6 +544,12 @@ func (a *AlertRuleMutators) WithRecordFrom(from string) AlertRuleMutator { } } +func (a *AlertRuleMutators) WithUpdatedBy(uid *UserUID) AlertRuleMutator { + return func(r *AlertRule) { + r.UpdatedBy = uid + } +} + func (g *AlertRuleGenerator) GenerateLabels(min, max int, prefix string) data.Labels { count := max if min > max { @@ -618,6 +630,7 @@ func CopyRule(r *AlertRule, mutators ...AlertRuleMutator) *AlertRule { Title: r.Title, Condition: r.Condition, Updated: r.Updated, + UpdatedBy: r.UpdatedBy, IntervalSeconds: r.IntervalSeconds, Version: r.Version, UID: r.UID, diff --git a/pkg/services/ngalert/provisioning/alert_rules.go b/pkg/services/ngalert/provisioning/alert_rules.go index ea618938b58..9b8cd429653 100644 --- a/pkg/services/ngalert/provisioning/alert_rules.go +++ b/pkg/services/ngalert/provisioning/alert_rules.go @@ -232,7 +232,7 @@ func (service *AlertRuleService) CreateAlertRule(ctx context.Context, user ident } } err = service.xact.InTransaction(ctx, func(ctx context.Context) error { - ids, err := service.ruleStore.InsertAlertRules(ctx, []models.AlertRule{ + ids, err := service.ruleStore.InsertAlertRules(ctx, models.NewUserUID(user), []models.AlertRule{ rule, }) if err != nil { @@ -359,7 +359,7 @@ func (service *AlertRuleService) UpdateRuleGroup(ctx context.Context, user ident } } - return service.ruleStore.UpdateAlertRules(ctx, updateRules) + return service.ruleStore.UpdateAlertRules(ctx, models.NewUserUID(user), updateRules) }) } @@ -511,7 +511,7 @@ func (service *AlertRuleService) persistDelta(ctx context.Context, user identity New: *update.New, }) } - if err := service.ruleStore.UpdateAlertRules(ctx, updates); err != nil { + if err := service.ruleStore.UpdateAlertRules(ctx, models.NewUserUID(user), updates); err != nil { return fmt.Errorf("failed to update alert rules: %w", err) } for _, update := range delta.Update { @@ -522,7 +522,7 @@ func (service *AlertRuleService) persistDelta(ctx context.Context, user identity } if len(delta.New) > 0 { - uids, err := service.ruleStore.InsertAlertRules(ctx, withoutNilAlertRules(delta.New)) + uids, err := service.ruleStore.InsertAlertRules(ctx, models.NewUserUID(user), withoutNilAlertRules(delta.New)) if err != nil { return fmt.Errorf("failed to insert alert rules: %w", err) } @@ -618,7 +618,7 @@ func (service *AlertRuleService) UpdateAlertRule(ctx context.Context, user ident return models.AlertRule{}, err } err = service.xact.InTransaction(ctx, func(ctx context.Context) error { - err := service.ruleStore.UpdateAlertRules(ctx, []models.UpdateRule{ + err := service.ruleStore.UpdateAlertRules(ctx, models.NewUserUID(user), []models.UpdateRule{ { Existing: storedRule, New: rule, diff --git a/pkg/services/ngalert/provisioning/alert_rules_test.go b/pkg/services/ngalert/provisioning/alert_rules_test.go index 0ec1814c0ae..ec15a851280 100644 --- a/pkg/services/ngalert/provisioning/alert_rules_test.go +++ b/pkg/services/ngalert/provisioning/alert_rules_test.go @@ -41,8 +41,9 @@ func TestAlertRuleService(t *testing.T) { ruleService := createAlertRuleService(t, nil) var orgID int64 = 1 u := &user.SignedInUser{ - UserID: 1, - OrgID: orgID, + UserUID: util.GenerateShortUID(), + UserID: 1, + OrgID: orgID, } t.Run("group creation should set the right provenance", func(t *testing.T) { @@ -196,7 +197,7 @@ func TestAlertRuleService(t *testing.T) { }, } rule.Metadata = ruleMetadata - r, err := ruleService.ruleStore.InsertAlertRules(context.Background(), []models.AlertRule{rule}) + r, err := ruleService.ruleStore.InsertAlertRules(context.Background(), models.NewUserUID(u), []models.AlertRule{rule}) require.NoError(t, err) require.Len(t, r, 1) @@ -233,7 +234,7 @@ func TestAlertRuleService(t *testing.T) { }, } rule.Metadata = ruleMetadata - r, err := ruleService.ruleStore.InsertAlertRules(context.Background(), []models.AlertRule{rule}) + r, err := ruleService.ruleStore.InsertAlertRules(context.Background(), models.NewUserUID(u), []models.AlertRule{rule}) require.NoError(t, err) require.Len(t, r, 1) @@ -624,7 +625,7 @@ func TestAlertRuleService(t *testing.T) { func TestCreateAlertRule(t *testing.T) { orgID := rand.Int63() - u := &user.SignedInUser{OrgID: orgID} + u := &user.SignedInUser{OrgID: orgID, UserUID: util.GenerateShortUID()} groupKey := models.GenerateGroupKey(orgID) groupIntervalSeconds := int64(30) gen := models.RuleGen @@ -1008,7 +1009,7 @@ func TestUpdateAlertRule(t *testing.T) { rule := models.CopyRule(rules[0]) - _, err := service.ruleStore.InsertAlertRules(context.Background(), []models.AlertRule{*rule}) + _, err := service.ruleStore.InsertAlertRules(context.Background(), models.NewUserUID(u), []models.AlertRule{*rule}) require.NoError(t, err) ac.CanWriteAllRulesFunc = func(ctx context.Context, user identity.Requester) (bool, error) { @@ -1626,7 +1627,7 @@ func TestProvisiongWithFullpath(t *testing.T) { require.NoError(t, err) t.Run("for a rule under a root folder should set the right fullpath", func(t *testing.T) { - r, err := ruleService.ruleStore.InsertAlertRules(context.Background(), []models.AlertRule{ + r, err := ruleService.ruleStore.InsertAlertRules(context.Background(), models.NewUserUID(&signedInUser), []models.AlertRule{ createTestRule("my-cool-group", "my-cool-group", orgID, namespaceUID), }) require.NoError(t, err) @@ -1657,7 +1658,7 @@ func TestProvisiongWithFullpath(t *testing.T) { }) require.NoError(t, err) - r, err := ruleService.ruleStore.InsertAlertRules(context.Background(), []models.AlertRule{ + r, err := ruleService.ruleStore.InsertAlertRules(context.Background(), models.NewUserUID(&signedInUser), []models.AlertRule{ createTestRule("my-cool-group-2", "my-cool-group-2", orgID, otherNamespaceUID), }) require.NoError(t, err) diff --git a/pkg/services/ngalert/provisioning/persist.go b/pkg/services/ngalert/provisioning/persist.go index 286652b07a4..f41ea602927 100644 --- a/pkg/services/ngalert/provisioning/persist.go +++ b/pkg/services/ngalert/provisioning/persist.go @@ -33,8 +33,8 @@ type RuleStore interface { GetAlertRuleByUID(ctx context.Context, query *models.GetAlertRuleByUIDQuery) (*models.AlertRule, error) ListAlertRules(ctx context.Context, query *models.ListAlertRulesQuery) (models.RulesGroup, error) GetRuleGroupInterval(ctx context.Context, orgID int64, namespaceUID string, ruleGroup string) (int64, error) - InsertAlertRules(ctx context.Context, rule []models.AlertRule) ([]models.AlertRuleKeyWithId, error) - UpdateAlertRules(ctx context.Context, rule []models.UpdateRule) error + InsertAlertRules(ctx context.Context, user *models.UserUID, rule []models.AlertRule) ([]models.AlertRuleKeyWithId, error) + UpdateAlertRules(ctx context.Context, user *models.UserUID, rule []models.UpdateRule) error DeleteAlertRulesByUID(ctx context.Context, orgID int64, ruleUID ...string) error GetAlertRulesGroupByRuleUID(ctx context.Context, query *models.GetAlertRulesGroupByRuleUIDQuery) ([]*models.AlertRule, error) } diff --git a/pkg/services/ngalert/schedule/registry_test.go b/pkg/services/ngalert/schedule/registry_test.go index a28ec6265aa..6ee2517a706 100644 --- a/pkg/services/ngalert/schedule/registry_test.go +++ b/pkg/services/ngalert/schedule/registry_test.go @@ -259,6 +259,7 @@ func TestRuleWithFolderFingerprint(t *testing.T) { excludedFields := map[string]struct{}{ "Version": {}, "Updated": {}, + "UpdatedBy": {}, "IntervalSeconds": {}, "Annotations": {}, } diff --git a/pkg/services/ngalert/store/alert_rule.go b/pkg/services/ngalert/store/alert_rule.go index bbc64fe04cb..b248d8e4fee 100644 --- a/pkg/services/ngalert/store/alert_rule.go +++ b/pkg/services/ngalert/store/alert_rule.go @@ -181,7 +181,7 @@ func (st DBstore) GetAlertRulesGroupByRuleUID(ctx context.Context, query *ngmode // InsertAlertRules is a handler for creating/updating alert rules. // Returns the UID and ID of rules that were created in the same order as the input rules. -func (st DBstore) InsertAlertRules(ctx context.Context, rules []ngmodels.AlertRule) ([]ngmodels.AlertRuleKeyWithId, error) { +func (st DBstore) InsertAlertRules(ctx context.Context, user *ngmodels.UserUID, rules []ngmodels.AlertRule) ([]ngmodels.AlertRuleKeyWithId, error) { ids := make([]ngmodels.AlertRuleKeyWithId, 0, len(rules)) keys := make([]ngmodels.AlertRuleKey, 0, len(rules)) return ids, st.SQLStore.WithTransactionalDbSession(ctx, func(sess *db.Session) error { @@ -200,7 +200,7 @@ func (st DBstore) InsertAlertRules(ctx context.Context, rules []ngmodels.AlertRu if err := st.validateAlertRule(r); err != nil { return err } - if err := (&r).PreSave(TimeNow); err != nil { + if err := (&r).PreSave(TimeNow, user); err != nil { return err } @@ -245,7 +245,7 @@ func (st DBstore) InsertAlertRules(ctx context.Context, rules []ngmodels.AlertRu } // UpdateAlertRules is a handler for updating alert rules. -func (st DBstore) UpdateAlertRules(ctx context.Context, rules []ngmodels.UpdateRule) error { +func (st DBstore) UpdateAlertRules(ctx context.Context, user *ngmodels.UserUID, rules []ngmodels.UpdateRule) error { return st.SQLStore.WithTransactionalDbSession(ctx, func(sess *db.Session) error { err := st.preventIntermediateUniqueConstraintViolations(sess, rules) if err != nil { @@ -263,7 +263,7 @@ func (st DBstore) UpdateAlertRules(ctx context.Context, rules []ngmodels.UpdateR if err := st.validateAlertRule(r.New); err != nil { return err } - if err := (&r.New).PreSave(TimeNow); err != nil { + if err := (&r.New).PreSave(TimeNow, user); err != nil { return err } converted, err := alertRuleFromModelsAlertRule(r.New) @@ -956,7 +956,9 @@ func (st DBstore) RenameReceiverInNotificationSettings(ctx context.Context, orgI if dryRun { return result, nil, nil } - return result, nil, st.UpdateAlertRules(ctx, updates) + // Provide empty user identifier to ensure it's clear that the rule update was made by the system + // and not by the user who changed the receiver's title. + return result, nil, st.UpdateAlertRules(ctx, nil, updates) } // RenameTimeIntervalInNotificationSettings renames all rules that use old time interval name to the new name. @@ -1031,7 +1033,9 @@ func (st DBstore) RenameTimeIntervalInNotificationSettings( if dryRun { return result, nil, nil } - return result, nil, st.UpdateAlertRules(ctx, updates) + // Provide empty user identifier to ensure it's clear that the rule update was made by the system + // and not by the user who changed the receiver's title. + return result, nil, st.UpdateAlertRules(ctx, nil, updates) } func ruleConstraintViolationToErr(sess *db.Session, rule ngmodels.AlertRule, err error, logger log.Logger) error { diff --git a/pkg/services/ngalert/store/alert_rule_test.go b/pkg/services/ngalert/store/alert_rule_test.go index dd55c5df488..10ccf9b9faf 100644 --- a/pkg/services/ngalert/store/alert_rule_test.go +++ b/pkg/services/ngalert/store/alert_rule_test.go @@ -47,6 +47,7 @@ func TestIntegrationUpdateAlertRules(t *testing.T) { folderService := setupFolderService(t, sqlStore, cfg, featuremgmt.WithFeatures()) b := &fakeBus{} store := createTestStore(sqlStore, folderService, logger, cfg.UnifiedAlerting, b) + usr := models.UserUID("1234") gen := models.RuleGen gen = gen.With(gen.WithIntervalMatching(store.Cfg.BaseInterval)) @@ -56,7 +57,7 @@ func TestIntegrationUpdateAlertRules(t *testing.T) { rule := createRule(t, store, gen) newRule := models.CopyRule(rule) newRule.Title = util.GenerateShortUID() - err := store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ + err := store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{ Existing: rule, New: *newRule, }, @@ -79,7 +80,7 @@ func TestIntegrationUpdateAlertRules(t *testing.T) { newRule := models.CopyRule(rule) newRule.Record.Metric = "new_metric" - err := store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ + err := store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{ Existing: rule, New: *newRule, }, @@ -104,7 +105,7 @@ func TestIntegrationUpdateAlertRules(t *testing.T) { newRule := models.CopyRule(rule) newRule.Title = util.GenerateShortUID() - err := store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ + err := store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{ Existing: rule, New: *newRule, }, @@ -125,22 +126,78 @@ func TestIntegrationUpdateAlertRules(t *testing.T) { called = true return nil } + t.Cleanup(func() { + b.publishFn = nil + }) newRule := models.CopyRule(rule) newRule.Title = util.GenerateShortUID() - err := store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ + err := store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{ Existing: rule, New: *newRule, }}) require.NoError(t, err) require.True(t, called) }) + + t.Run("should set UpdatedBy", func(t *testing.T) { + rule := createRule(t, store, gen) + newRule := models.CopyRule(rule) + newRule.Title = util.GenerateShortUID() + err := store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{ + Existing: rule, + New: *newRule, + }, + }) + require.NoError(t, err) + + dbrule := &alertRule{} + err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + exist, err := sess.Table(alertRule{}).ID(rule.ID).Get(dbrule) + require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule.ID)) + return err + }) + + require.NoError(t, err) + require.NotNil(t, dbrule.UpdatedBy) + require.Equal(t, string(usr), *dbrule.UpdatedBy) + + t.Run("should set CreatedBy in rule version table", func(t *testing.T) { + dbVersion := &alertRuleVersion{} + err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + exist, err := sess.Table(alertRuleVersion{}).Where("rule_uid = ? AND version = ?", dbrule.UID, dbrule.Version).Get(dbVersion) + require.Truef(t, exist, "new version of the rule does not exist in version table") + return err + }) + require.NoError(t, err) + require.NotNil(t, dbVersion.CreatedBy) + require.Equal(t, *dbrule.UpdatedBy, *dbVersion.CreatedBy) + }) + + t.Run("nil identity should be handled correctly", func(t *testing.T) { + rule.Version++ + newRule.Title = util.GenerateShortUID() + err = store.UpdateAlertRules(context.Background(), nil, []models.UpdateRule{{ + Existing: rule, + New: *newRule, + }, + }) + dbrule := &alertRule{} + err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + exist, err := sess.Table(alertRule{}).ID(rule.ID).Get(dbrule) + require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule.ID)) + return err + }) + assert.Nil(t, dbrule.UpdatedBy) + }) + }) } func TestIntegrationUpdateAlertRulesWithUniqueConstraintViolation(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") } + usr := models.UserUID("test") cfg := setting.NewCfg() cfg.UnifiedAlerting = setting.UnifiedAlertingSettings{BaseInterval: time.Duration(rand.Int63n(100)+1) * time.Second} sqlStore := db.InitTestDB(t) @@ -167,7 +224,7 @@ func TestIntegrationUpdateAlertRulesWithUniqueConstraintViolation(t *testing.T) newRule1.Title = rule2.Title newRule2.Title = util.GenerateShortUID() - err := store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ + err := store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{ Existing: rule1, New: *newRule1, }, { @@ -211,7 +268,7 @@ func TestIntegrationUpdateAlertRulesWithUniqueConstraintViolation(t *testing.T) newRule2.Title = rule3.Title newRule3.Title = rule1.Title - err := store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ + err := store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{ Existing: rule1, New: *newRule1, }, { @@ -263,7 +320,7 @@ func TestIntegrationUpdateAlertRulesWithUniqueConstraintViolation(t *testing.T) newRule1.Title = strings.ToUpper(rule2.Title) newRule2.Title = strings.ToUpper(rule1.Title) - err := store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ + err := store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{ Existing: rule1, New: *newRule1, }, { @@ -310,7 +367,7 @@ func TestIntegrationUpdateAlertRulesWithUniqueConstraintViolation(t *testing.T) newRule3.Title = rule4.Title newRule4.Title = rule3.Title - err := store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ + err := store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{ Existing: rule1, New: *newRule1, }, { @@ -372,7 +429,7 @@ func TestIntegrationUpdateAlertRulesWithUniqueConstraintViolation(t *testing.T) newRule2 := models.CopyRule(rule2) newRule2.Title = newRule1.Title - err := store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ + err := store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{ Existing: rule2, New: *newRule2, }, @@ -704,6 +761,7 @@ func TestIntegrationInsertAlertRules(t *testing.T) { } orgID := int64(1) + usr := models.UserUID("test") sqlStore := db.InitTestDB(t) cfg := setting.NewCfg() cfg.UnifiedAlerting.BaseInterval = 1 * time.Second @@ -724,7 +782,7 @@ func TestIntegrationInsertAlertRules(t *testing.T) { rules := append(gen.GenerateMany(5), recordingRulesGen.GenerateMany(5)...) - ids, err := store.InsertAlertRules(context.Background(), rules) + ids, err := store.InsertAlertRules(context.Background(), &usr, rules) require.NoError(t, err) require.Len(t, ids, len(rules)) @@ -776,12 +834,20 @@ func TestIntegrationInsertAlertRules(t *testing.T) { } }) + t.Run("inserted rules should have UpdatedBy set", func(t *testing.T) { + for _, rule := range dbRules { + if assert.NotNil(t, rule.UpdatedBy) { + assert.Equal(t, usr, *rule.UpdatedBy) + } + } + }) + t.Run("inserted recording rules fail validation if metric name is invalid", func(t *testing.T) { t.Run("invalid UTF-8", func(t *testing.T) { invalidMetric := "my_metric\x80" invalidRule := recordingRulesGen.Generate() invalidRule.Record.Metric = invalidMetric - _, err := store.InsertAlertRules(context.Background(), []models.AlertRule{invalidRule}) + _, err := store.InsertAlertRules(context.Background(), &usr, []models.AlertRule{invalidRule}) require.ErrorIs(t, err, models.ErrAlertRuleFailedValidation) require.ErrorContains(t, err, "metric name for recording rule must be a valid utf8 string") }) @@ -789,7 +855,7 @@ func TestIntegrationInsertAlertRules(t *testing.T) { t.Run("clears fields that should not exist on recording rules", func(t *testing.T) { rule := recordingRulesGen.Generate() - rules, err := store.InsertAlertRules(context.Background(), []models.AlertRule{rule}) + rules, err := store.InsertAlertRules(context.Background(), &usr, []models.AlertRule{rule}) require.NoError(t, err) require.Len(t, rules, 1) ruleUID := rules[0].UID @@ -806,13 +872,13 @@ func TestIntegrationInsertAlertRules(t *testing.T) { }) t.Run("fail to insert rules with same ID", func(t *testing.T) { - _, err = store.InsertAlertRules(context.Background(), []models.AlertRule{rules[0]}) + _, err = store.InsertAlertRules(context.Background(), &usr, []models.AlertRule{rules[0]}) require.ErrorIs(t, err, models.ErrAlertRuleConflictBase) }) t.Run("fail insert rules with the same title in a folder", func(t *testing.T) { cp := models.CopyRule(&rules[0]) cp.UID = cp.UID + "-new" - _, err = store.InsertAlertRules(context.Background(), []models.AlertRule{*cp}) + _, err = store.InsertAlertRules(context.Background(), &usr, []models.AlertRule{*cp}) require.ErrorIs(t, err, models.ErrAlertRuleConflictBase) require.ErrorIs(t, err, models.ErrAlertRuleUniqueConstraintViolation) require.NotEqual(t, rules[0].UID, "") @@ -825,7 +891,7 @@ func TestIntegrationInsertAlertRules(t *testing.T) { t.Run("should not let insert rules with the same UID", func(t *testing.T) { cp := models.CopyRule(&rules[0]) cp.Title = "unique-test-title" - _, err = store.InsertAlertRules(context.Background(), []models.AlertRule{*cp}) + _, err = store.InsertAlertRules(context.Background(), &usr, []models.AlertRule{*cp}) require.ErrorIs(t, err, models.ErrAlertRuleConflictBase) require.ErrorContains(t, err, "rule UID under the same organisation should be unique") }) @@ -833,6 +899,9 @@ func TestIntegrationInsertAlertRules(t *testing.T) { t.Run("should emit event when rules are inserted", func(t *testing.T) { rule := gen.Generate() called := false + t.Cleanup(func() { + b.publishFn = nil + }) b.publishFn = func(ctx context.Context, msg bus.Msg) error { event, ok := msg.(*RuleChangeEvent) require.True(t, ok) @@ -843,11 +912,23 @@ func TestIntegrationInsertAlertRules(t *testing.T) { return nil } - rules, err := store.InsertAlertRules(context.Background(), []models.AlertRule{rule}) + rules, err := store.InsertAlertRules(context.Background(), &usr, []models.AlertRule{rule}) require.NoError(t, err) require.Len(t, rules, 1) require.True(t, called) }) + + t.Run("nil identity should be handled correctly", func(t *testing.T) { + rule := gen.Generate() + ids, err = store.InsertAlertRules(context.Background(), nil, []models.AlertRule{rule}) + require.NoError(t, err) + insertedRule, err := store.GetRuleByID(context.Background(), models.GetAlertRuleByIDQuery{ + ID: ids[0].ID, + OrgID: rule.OrgID, + }) + require.NoError(t, err) + require.Nil(t, insertedRule.UpdatedBy) + }) } func TestIntegrationAlertRulesNotificationSettings(t *testing.T) { @@ -855,6 +936,8 @@ func TestIntegrationAlertRulesNotificationSettings(t *testing.T) { t.Skip("skipping integration test") } + usr := models.UserUID("test") + getKeyMap := func(r []*models.AlertRule) map[models.AlertRuleKey]struct{} { result := make(map[models.AlertRuleKey]struct{}, len(r)) for _, rule := range r { @@ -894,7 +977,7 @@ func TestIntegrationAlertRulesNotificationSettings(t *testing.T) { require.NoError(t, store.SetProvenance(context.Background(), rule, rule.OrgID, p)) } - _, err := store.InsertAlertRules(context.Background(), deref) + _, err := store.InsertAlertRules(context.Background(), &usr, deref) require.NoError(t, err) t.Run("should find rules by receiver name", func(t *testing.T) { @@ -1136,7 +1219,7 @@ func TestIntegrationListNotificationSettings(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") } - + usr := models.UserUID("test") sqlStore := db.InitTestDB(t) folderService := setupFolderService(t, sqlStore, setting.NewCfg(), featuremgmt.WithFeatures()) logger := log.New("test-dbstore") @@ -1168,7 +1251,7 @@ func TestIntegrationListNotificationSettings(t *testing.T) { orgRules := append(rulesWithNotificationsAndReceiver, rulesWithNotificationsAndTimeInterval...) - _, err := store.InsertAlertRules(context.Background(), deref) + _, err := store.InsertAlertRules(context.Background(), &usr, deref) require.NoError(t, err) result, err := store.ListNotificationSettings(context.Background(), models.ListNotificationSettingsQuery{OrgID: 1}) @@ -1258,6 +1341,8 @@ func TestIntegrationGetNamespacesByRuleUID(t *testing.T) { t.Skip("skipping integration test") } + usr := models.UserUID("test") + sqlStore := db.InitTestDB(t) cfg := setting.NewCfg() cfg.UnifiedAlerting.BaseInterval = 1 * time.Second @@ -1267,7 +1352,7 @@ func TestIntegrationGetNamespacesByRuleUID(t *testing.T) { store := createTestStore(sqlStore, folderService, logger, cfg.UnifiedAlerting, b) rules := models.RuleGen.With(models.RuleMuts.WithOrgID(1), models.RuleMuts.WithRandomRecordingRules()).GenerateMany(5) - _, err := store.InsertAlertRules(context.Background(), rules) + _, err := store.InsertAlertRules(context.Background(), &usr, rules) require.NoError(t, err) uids := make([]string, 0, len(rules)) @@ -1306,6 +1391,7 @@ func TestIntegrationRuleGroupsCaseSensitive(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") } + usr := models.UserUID("test") sqlStore := db.InitTestDB(t) cfg := setting.NewCfg() @@ -1329,7 +1415,7 @@ func TestIntegrationRuleGroupsCaseSensitive(t *testing.T) { group2 := gen.With(gen.WithGroupKey(groupKey2)).GenerateMany(1, 3) group3 := gen.With(gen.WithGroupKey(groupKey3)).GenerateMany(1, 3) - _, err := store.InsertAlertRules(context.Background(), append(append(append(misc, group1...), group2...), group3...)) + _, err := store.InsertAlertRules(context.Background(), &usr, append(append(append(misc, group1...), group2...), group3...)) require.NoError(t, err) t.Run("GetAlertRulesGroupByRuleUID", func(t *testing.T) { @@ -1524,6 +1610,7 @@ func TestIntegration_AlertRuleVersionsCleanup(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") } + usr := models.UserUID("test") cfg := setting.NewCfg() cfg.UnifiedAlerting = setting.UnifiedAlertingSettings{ BaseInterval: time.Duration(rand.Int63n(100)+1) * time.Second, @@ -1544,7 +1631,7 @@ func TestIntegration_AlertRuleVersionsCleanup(t *testing.T) { rule := createRule(t, store, generator) firstNewRule := models.CopyRule(rule) firstNewRule.Title = util.GenerateShortUID() - err := store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ + err := store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{ Existing: rule, New: *firstNewRule, }, @@ -1553,7 +1640,7 @@ func TestIntegration_AlertRuleVersionsCleanup(t *testing.T) { firstNewRule.Version = firstNewRule.Version + 1 secondNewRule := models.CopyRule(firstNewRule) secondNewRule.Title = util.GenerateShortUID() - err = store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ + err = store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{ Existing: firstNewRule, New: *secondNewRule, }, @@ -1592,7 +1679,7 @@ func TestIntegration_AlertRuleVersionsCleanup(t *testing.T) { rule := createRule(t, store, generator) oldRule := models.CopyRule(rule) oldRule.Title = "old-record" - err := store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ + err := store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{ Existing: rule, New: *oldRule, }}) // first entry in `rule_version_history` table happens here @@ -1601,19 +1688,19 @@ func TestIntegration_AlertRuleVersionsCleanup(t *testing.T) { rule.Version = rule.Version + 1 middleRule := models.CopyRule(rule) middleRule.Title = "middle-record" - err = store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ + err = store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{ Existing: rule, New: *middleRule, - }}) //second entry in `rule_version_history` table happens here + }}) // second entry in `rule_version_history` table happens here require.NoError(t, err) rule.Version = rule.Version + 1 newerRule := models.CopyRule(rule) newerRule.Title = "newer-record" - err = store.UpdateAlertRules(context.Background(), []models.UpdateRule{{ + err = store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{ Existing: rule, New: *newerRule, - }}) //second entry in `rule_version_history` table happens here + }}) // second entry in `rule_version_history` table happens here require.NoError(t, err) // only the `old-record` should be deleted since limit is set to 1 and there are total 2 records diff --git a/pkg/services/ngalert/store/compat.go b/pkg/services/ngalert/store/compat.go index 7f255b8c7b0..ed36782114b 100644 --- a/pkg/services/ngalert/store/compat.go +++ b/pkg/services/ngalert/store/compat.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/util" "github.com/grafana/grafana/pkg/services/ngalert/models" ) @@ -35,6 +36,10 @@ func alertRuleToModelsAlertRule(ar alertRule, l log.Logger) (models.AlertRule, e IsPaused: ar.IsPaused, } + if ar.UpdatedBy != nil { + result.UpdatedBy = util.Pointer(models.UserUID(*ar.UpdatedBy)) + } + if ar.NoDataState != "" { result.NoDataState, err = models.NoDataStateFromString(ar.NoDataState) if err != nil { @@ -120,6 +125,10 @@ func alertRuleFromModelsAlertRule(ar models.AlertRule) (alertRule, error) { IsPaused: ar.IsPaused, } + if ar.UpdatedBy != nil { + result.UpdatedBy = util.Pointer(string(*ar.UpdatedBy)) + } + // Serialize complex types to JSON strings data, err := json.Marshal(ar.Data) if err != nil { @@ -179,6 +188,7 @@ func alertRuleToAlertRuleVersion(rule alertRule) alertRuleVersion { RestoredFrom: 0, Version: rule.Version, Created: rule.Updated, // assuming the Updated time as the creation time + CreatedBy: rule.UpdatedBy, Title: rule.Title, Condition: rule.Condition, Data: rule.Data, diff --git a/pkg/services/ngalert/store/deltas.go b/pkg/services/ngalert/store/deltas.go index c693d51d3b3..27469f5733a 100644 --- a/pkg/services/ngalert/store/deltas.go +++ b/pkg/services/ngalert/store/deltas.go @@ -9,7 +9,7 @@ import ( ) // AlertRuleFieldsToIgnoreInDiff contains fields that are ignored when calculating the RuleDelta.Diff. -var AlertRuleFieldsToIgnoreInDiff = [...]string{"ID", "Version", "Updated"} +var AlertRuleFieldsToIgnoreInDiff = [...]string{"ID", "Version", "Updated", "UpdatedBy"} type RuleDelta struct { Existing *models.AlertRule diff --git a/pkg/services/ngalert/store/models.go b/pkg/services/ngalert/store/models.go index 39ba210995c..3e47ab90fa4 100644 --- a/pkg/services/ngalert/store/models.go +++ b/pkg/services/ngalert/store/models.go @@ -10,6 +10,7 @@ type alertRule struct { Condition string Data string Updated time.Time + UpdatedBy *string `xorm:"updated_by"` IntervalSeconds int64 Version int64 `xorm:"version"` // this tag makes xorm add optimistic lock (see https://xorm.io/docs/chapter-06/1.lock/) UID string `xorm:"uid"` @@ -46,6 +47,7 @@ type alertRuleVersion struct { Version int64 Created time.Time + CreatedBy *string `xorm:"created_by"` Title string Condition string Data string diff --git a/pkg/services/ngalert/tests/fakes/rules.go b/pkg/services/ngalert/tests/fakes/rules.go index 014de832bfe..3784ee6ed9b 100644 --- a/pkg/services/ngalert/tests/fakes/rules.go +++ b/pkg/services/ngalert/tests/fakes/rules.go @@ -258,7 +258,7 @@ func (f *RuleStore) GetNamespaceByUID(_ context.Context, uid string, orgID int64 return nil, fmt.Errorf("not found") } -func (f *RuleStore) UpdateAlertRules(_ context.Context, q []models.UpdateRule) error { +func (f *RuleStore) UpdateAlertRules(_ context.Context, _ *models.UserUID, q []models.UpdateRule) error { f.mtx.Lock() defer f.mtx.Unlock() f.RecordedOps = append(f.RecordedOps, q) @@ -268,7 +268,7 @@ func (f *RuleStore) UpdateAlertRules(_ context.Context, q []models.UpdateRule) e return nil } -func (f *RuleStore) InsertAlertRules(_ context.Context, q []models.AlertRule) ([]models.AlertRuleKeyWithId, error) { +func (f *RuleStore) InsertAlertRules(_ context.Context, _ *models.UserUID, q []models.AlertRule) ([]models.AlertRuleKeyWithId, error) { f.mtx.Lock() defer f.mtx.Unlock() f.RecordedOps = append(f.RecordedOps, q) diff --git a/pkg/services/ngalert/tests/util.go b/pkg/services/ngalert/tests/util.go index e5ff7a1b10b..6188d6291a9 100644 --- a/pkg/services/ngalert/tests/util.go +++ b/pkg/services/ngalert/tests/util.go @@ -113,7 +113,7 @@ func CreateTestAlertRuleWithLabels(t testing.TB, ctx context.Context, dbstore *s } require.NoError(t, err) - _, err = dbstore.InsertAlertRules(ctx, []models.AlertRule{ + _, err = dbstore.InsertAlertRules(ctx, models.NewUserUID(user), []models.AlertRule{ { ID: 0, diff --git a/pkg/services/sqlstore/migrations/migrations.go b/pkg/services/sqlstore/migrations/migrations.go index 9ba0bcaea9a..317eb39a13d 100644 --- a/pkg/services/sqlstore/migrations/migrations.go +++ b/pkg/services/sqlstore/migrations/migrations.go @@ -141,4 +141,6 @@ func (oss *OSSMigrations) AddMigration(mg *Migrator) { externalsession.AddMigration(mg) accesscontrol.AddReceiverCreateScopeMigration(mg) + + ualert.AddAlertRuleUpdatedByMigration(mg) } diff --git a/pkg/services/sqlstore/migrations/ualert/rule_creator_mig.go b/pkg/services/sqlstore/migrations/ualert/rule_creator_mig.go new file mode 100644 index 00000000000..563972b0ae0 --- /dev/null +++ b/pkg/services/sqlstore/migrations/ualert/rule_creator_mig.go @@ -0,0 +1,20 @@ +package ualert + +import "github.com/grafana/grafana/pkg/services/sqlstore/migrator" + +// AddRuleNotificationSettingsColumns creates a column for notification settings in the alert_rule and alert_rule_version tables. +func AddAlertRuleUpdatedByMigration(mg *migrator.Migrator) { + mg.AddMigration("add created_by column to alert_rule_version table", migrator.NewAddColumnMigration(migrator.Table{Name: "alert_rule_version"}, &migrator.Column{ + Name: "created_by", + Type: migrator.DB_Varchar, + Length: 40, + Nullable: true, + })) + + mg.AddMigration("add updated_by column to alert_rule table", migrator.NewAddColumnMigration(migrator.Table{Name: "alert_rule"}, &migrator.Column{ + Name: "updated_by", + Type: migrator.DB_Varchar, + Length: 40, + Nullable: true, + })) +} diff --git a/pkg/storage/unified/federated/federatedtests/stats_test.go b/pkg/storage/unified/federated/federatedtests/stats_test.go index ff4d04e5ba8..8b67af4fce9 100644 --- a/pkg/storage/unified/federated/federatedtests/stats_test.go +++ b/pkg/storage/unified/federated/federatedtests/stats_test.go @@ -69,7 +69,7 @@ func TestDirectSQLStats(t *testing.T) { // create an alert rule inside of folder test2 ruleStore := ngalertstore.SetupStoreForTesting(t, db) - _, err = ruleStore.InsertAlertRules(context.Background(), []ngmodels.AlertRule{ + _, err = ruleStore.InsertAlertRules(context.Background(), ngmodels.NewUserUID(tempUser), []ngmodels.AlertRule{ { DashboardUID: &folder2UID, UID: "test",