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",