diff --git a/pkg/services/ngalert/api.go b/pkg/services/ngalert/api.go index 005639c2a12..1c390f78619 100644 --- a/pkg/services/ngalert/api.go +++ b/pkg/services/ngalert/api.go @@ -15,12 +15,12 @@ import ( func (ng *AlertNG) registerAPIEndpoints() { ng.RouteRegister.Group("/api/alert-definitions", func(alertDefinitions routing.RouteRegister) { alertDefinitions.Get("", middleware.ReqSignedIn, api.Wrap(ng.listAlertDefinitions)) - alertDefinitions.Get("/eval/:alertDefinitionId", ng.validateOrgAlertDefinition, api.Wrap(ng.alertDefinitionEvalEndpoint)) + alertDefinitions.Get("/eval/:alertDefinitionUID", ng.validateOrgAlertDefinition, api.Wrap(ng.alertDefinitionEvalEndpoint)) alertDefinitions.Post("/eval", middleware.ReqSignedIn, binding.Bind(evalAlertConditionCommand{}), api.Wrap(ng.conditionEvalEndpoint)) - alertDefinitions.Get("/:alertDefinitionId", ng.validateOrgAlertDefinition, api.Wrap(ng.getAlertDefinitionEndpoint)) - alertDefinitions.Delete("/:alertDefinitionId", ng.validateOrgAlertDefinition, api.Wrap(ng.deleteAlertDefinitionEndpoint)) + alertDefinitions.Get("/:alertDefinitionUID", ng.validateOrgAlertDefinition, api.Wrap(ng.getAlertDefinitionEndpoint)) + alertDefinitions.Delete("/:alertDefinitionUID", ng.validateOrgAlertDefinition, api.Wrap(ng.deleteAlertDefinitionEndpoint)) alertDefinitions.Post("/", middleware.ReqSignedIn, binding.Bind(saveAlertDefinitionCommand{}), api.Wrap(ng.createAlertDefinitionEndpoint)) - alertDefinitions.Put("/:alertDefinitionId", ng.validateOrgAlertDefinition, binding.Bind(updateAlertDefinitionCommand{}), api.Wrap(ng.updateAlertDefinitionEndpoint)) + alertDefinitions.Put("/:alertDefinitionUID", ng.validateOrgAlertDefinition, binding.Bind(updateAlertDefinitionCommand{}), api.Wrap(ng.updateAlertDefinitionEndpoint)) }) ng.RouteRegister.Group("/api/ngalert/", func(schedulerRouter routing.RouteRegister) { @@ -52,11 +52,11 @@ func (ng *AlertNG) conditionEvalEndpoint(c *models.ReqContext, dto evalAlertCond }) } -// alertDefinitionEvalEndpoint handles GET /api/alert-definitions/eval/:dashboardId/:panelId/:refId". +// alertDefinitionEvalEndpoint handles GET /api/alert-definitions/eval/:alertDefinitionUID. func (ng *AlertNG) alertDefinitionEvalEndpoint(c *models.ReqContext) api.Response { - alertDefinitionID := c.ParamsInt64(":alertDefinitionId") + alertDefinitionUID := c.ParamsEscape(":alertDefinitionUID") - condition, err := ng.LoadAlertCondition(alertDefinitionID) + condition, err := ng.LoadAlertCondition(alertDefinitionUID, c.SignedInUser.OrgId) if err != nil { return api.Error(400, "Failed to load alert definition conditions", err) } @@ -67,7 +67,7 @@ func (ng *AlertNG) alertDefinitionEvalEndpoint(c *models.ReqContext) api.Respons evalResults, err := eval.ConditionEval(condition, timeNow()) if err != nil { - return api.Error(400, "Failed to evaludate alert", err) + return api.Error(400, "Failed to evaluate alert", err) } frame := evalResults.AsDataFrame() @@ -85,44 +85,41 @@ func (ng *AlertNG) alertDefinitionEvalEndpoint(c *models.ReqContext) api.Respons }) } -// getAlertDefinitionEndpoint handles GET /api/alert-definitions/:alertDefinitionId. +// getAlertDefinitionEndpoint handles GET /api/alert-definitions/:alertDefinitionUID. func (ng *AlertNG) getAlertDefinitionEndpoint(c *models.ReqContext) api.Response { - alertDefinitionID := c.ParamsInt64(":alertDefinitionId") + alertDefinitionUID := c.ParamsEscape(":alertDefinitionUID") - query := getAlertDefinitionByIDQuery{ - ID: alertDefinitionID, + query := getAlertDefinitionByUIDQuery{ + UID: alertDefinitionUID, + OrgID: c.SignedInUser.OrgId, } - if err := ng.getAlertDefinitionByID(&query); err != nil { + if err := ng.getAlertDefinitionByUID(&query); err != nil { return api.Error(500, "Failed to get alert definition", err) } return api.JSON(200, &query.Result) } -// deleteAlertDefinitionEndpoint handles DELETE /api/alert-definitions/:alertDefinitionId. +// deleteAlertDefinitionEndpoint handles DELETE /api/alert-definitions/:alertDefinitionUID. func (ng *AlertNG) deleteAlertDefinitionEndpoint(c *models.ReqContext) api.Response { - alertDefinitionID := c.ParamsInt64(":alertDefinitionId") + alertDefinitionUID := c.ParamsEscape(":alertDefinitionUID") - cmd := deleteAlertDefinitionByIDCommand{ - ID: alertDefinitionID, + cmd := deleteAlertDefinitionByUIDCommand{ + UID: alertDefinitionUID, OrgID: c.SignedInUser.OrgId, } - if err := ng.deleteAlertDefinitionByID(&cmd); err != nil { + if err := ng.deleteAlertDefinitionByUID(&cmd); err != nil { return api.Error(500, "Failed to delete alert definition", err) } - if cmd.RowsAffected != 1 { - ng.log.Warn("unexpected number of rows affected on alert definition delete", "definitionID", alertDefinitionID, "rowsAffected", cmd.RowsAffected) - } - return api.Success("Alert definition deleted") } -// updateAlertDefinitionEndpoint handles PUT /api/alert-definitions/:alertDefinitionId. +// updateAlertDefinitionEndpoint handles PUT /api/alert-definitions/:alertDefinitionUID. func (ng *AlertNG) updateAlertDefinitionEndpoint(c *models.ReqContext, cmd updateAlertDefinitionCommand) api.Response { - cmd.ID = c.ParamsInt64(":alertDefinitionId") + cmd.UID = c.ParamsEscape(":alertDefinitionUID") cmd.OrgID = c.SignedInUser.OrgId if err := ng.validateCondition(cmd.Condition, c.SignedInUser); err != nil { @@ -133,11 +130,7 @@ func (ng *AlertNG) updateAlertDefinitionEndpoint(c *models.ReqContext, cmd updat return api.Error(500, "Failed to update alert definition", err) } - if cmd.RowsAffected != 1 { - ng.log.Warn("unexpected number of rows affected on alert definition update", "definitionID", cmd.ID, "rowsAffected", cmd.RowsAffected) - } - - return api.Success("Alert definition updated") + return api.JSON(200, cmd.Result) } // createAlertDefinitionEndpoint handles POST /api/alert-definitions. @@ -152,7 +145,7 @@ func (ng *AlertNG) createAlertDefinitionEndpoint(c *models.ReqContext, cmd saveA return api.Error(500, "Failed to create alert definition", err) } - return api.JSON(200, util.DynMap{"id": cmd.Result.ID}) + return api.JSON(200, cmd.Result) } // listAlertDefinitions handles GET /api/alert-definitions. diff --git a/pkg/services/ngalert/common_test.go b/pkg/services/ngalert/common_test.go index 22441d4185c..aae7b41bd8a 100644 --- a/pkg/services/ngalert/common_test.go +++ b/pkg/services/ngalert/common_test.go @@ -80,6 +80,6 @@ func createTestAlertDefinition(t *testing.T, ng *AlertNG, intervalSeconds int64) } err := ng.saveAlertDefinition(&cmd) require.NoError(t, err) - t.Logf("alert definition: %d with interval: %d created", cmd.Result.ID, intervalSeconds) + t.Logf("alert definition: %v with interval: %d created", cmd.Result.getKey(), intervalSeconds) return cmd.Result } diff --git a/pkg/services/ngalert/database.go b/pkg/services/ngalert/database.go index e09a30e818f..19fa92a929a 100644 --- a/pkg/services/ngalert/database.go +++ b/pkg/services/ngalert/database.go @@ -9,9 +9,10 @@ import ( "github.com/grafana/grafana/pkg/util" ) -func getAlertDefinitionByID(alertDefinitionID int64, sess *sqlstore.DBSession) (*AlertDefinition, error) { - alertDefinition := AlertDefinition{} - has, err := sess.ID(alertDefinitionID).Get(&alertDefinition) +func getAlertDefinitionByUID(sess *sqlstore.DBSession, alertDefinitionUID string, orgID int64) (*AlertDefinition, error) { + // we consider optionally enabling some caching + alertDefinition := AlertDefinition{OrgID: orgID, UID: alertDefinitionUID} + has, err := sess.Get(&alertDefinition) if !has { return nil, errAlertDefinitionNotFound } @@ -23,20 +24,19 @@ func getAlertDefinitionByID(alertDefinitionID int64, sess *sqlstore.DBSession) ( // deleteAlertDefinitionByID is a handler for deleting an alert definition. // It returns models.ErrAlertDefinitionNotFound if no alert definition is found for the provided ID. -func (ng *AlertNG) deleteAlertDefinitionByID(cmd *deleteAlertDefinitionByIDCommand) error { +func (ng *AlertNG) deleteAlertDefinitionByUID(cmd *deleteAlertDefinitionByUIDCommand) error { return ng.SQLStore.WithTransactionalDbSession(context.Background(), func(sess *sqlstore.DBSession) error { - res, err := sess.Exec("DELETE FROM alert_definition WHERE id = ?", cmd.ID) + res, err := sess.Exec("DELETE FROM alert_definition WHERE uid = ? AND org_id = ?", cmd.UID, cmd.OrgID) if err != nil { return err } - rowsAffected, err := res.RowsAffected() + _, err = res.RowsAffected() if err != nil { return err } - cmd.RowsAffected = rowsAffected - _, err = sess.Exec("DELETE FROM alert_definition_version WHERE alert_definition_id = ?", cmd.ID) + _, err = sess.Exec("DELETE FROM alert_definition_version WHERE alert_definition_uid = ?", cmd.UID) if err != nil { return err } @@ -45,11 +45,11 @@ func (ng *AlertNG) deleteAlertDefinitionByID(cmd *deleteAlertDefinitionByIDComma }) } -// getAlertDefinitionByID is a handler for retrieving an alert definition from that database by its ID. +// getAlertDefinitionByUID is a handler for retrieving an alert definition from that database by its UID and organisation ID. // It returns models.ErrAlertDefinitionNotFound if no alert definition is found for the provided ID. -func (ng *AlertNG) getAlertDefinitionByID(query *getAlertDefinitionByIDQuery) error { - return ng.SQLStore.WithTransactionalDbSession(context.Background(), func(sess *sqlstore.DBSession) error { - alertDefinition, err := getAlertDefinitionByID(query.ID, sess) +func (ng *AlertNG) getAlertDefinitionByUID(query *getAlertDefinitionByUIDQuery) error { + return ng.SQLStore.WithDbSession(context.Background(), func(sess *sqlstore.DBSession) error { + alertDefinition, err := getAlertDefinitionByUID(sess, query.UID, query.OrgID) if err != nil { return err } @@ -118,42 +118,14 @@ func (ng *AlertNG) saveAlertDefinition(cmd *saveAlertDefinitionCommand) error { // It returns models.ErrAlertDefinitionNotFound if no alert definition is found for the provided ID. func (ng *AlertNG) updateAlertDefinition(cmd *updateAlertDefinitionCommand) error { return ng.SQLStore.WithTransactionalDbSession(context.Background(), func(sess *sqlstore.DBSession) error { - alertDefinition := &AlertDefinition{ - ID: cmd.ID, - Title: cmd.Title, - Condition: cmd.Condition.RefID, - Data: cmd.Condition.QueriesAndExpressions, - OrgID: cmd.OrgID, - } - if cmd.IntervalSeconds != nil { - alertDefinition.IntervalSeconds = *cmd.IntervalSeconds - } - - if err := ng.validateAlertDefinition(alertDefinition, true); err != nil { - return err - } - - if err := alertDefinition.preSave(); err != nil { - return err - } - - existingAlertDefinition, err := getAlertDefinitionByID(alertDefinition.ID, sess) + existingAlertDefinition, err := getAlertDefinitionByUID(sess, cmd.UID, cmd.OrgID) if err != nil { if errors.Is(err, errAlertDefinitionNotFound) { - cmd.Result = alertDefinition - cmd.RowsAffected = 0 return nil } return err } - alertDefinition.Version = existingAlertDefinition.Version + 1 - - affectedRows, err := sess.ID(cmd.ID).Update(alertDefinition) - if err != nil { - return err - } - title := cmd.Title if title == "" { title = existingAlertDefinition.Title @@ -171,30 +143,55 @@ func (ng *AlertNG) updateAlertDefinition(cmd *updateAlertDefinitionCommand) erro intervalSeconds = &existingAlertDefinition.IntervalSeconds } + // explicitly set all fields regardless of being provided or not + alertDefinition := &AlertDefinition{ + ID: existingAlertDefinition.ID, + Title: title, + Condition: condition, + Data: data, + OrgID: existingAlertDefinition.OrgID, + IntervalSeconds: *intervalSeconds, + UID: existingAlertDefinition.UID, + } + + if err := ng.validateAlertDefinition(alertDefinition, true); err != nil { + return err + } + + if err := alertDefinition.preSave(); err != nil { + return err + } + + alertDefinition.Version = existingAlertDefinition.Version + 1 + + _, err = sess.ID(existingAlertDefinition.ID).Update(alertDefinition) + if err != nil { + return err + } + alertDefVersion := AlertDefinitionVersion{ AlertDefinitionID: alertDefinition.ID, - AlertDefinitionUID: existingAlertDefinition.UID, - ParentVersion: existingAlertDefinition.Version, + AlertDefinitionUID: alertDefinition.UID, + ParentVersion: alertDefinition.Version, Version: alertDefinition.Version, - Condition: condition, + Condition: alertDefinition.Condition, Created: alertDefinition.Updated, - Title: title, - Data: data, - IntervalSeconds: *intervalSeconds, + Title: alertDefinition.Title, + Data: alertDefinition.Data, + IntervalSeconds: alertDefinition.IntervalSeconds, } if _, err := sess.Insert(alertDefVersion); err != nil { return err } cmd.Result = alertDefinition - cmd.RowsAffected = affectedRows return nil }) } // getOrgAlertDefinitions is a handler for retrieving alert definitions of specific organisation. func (ng *AlertNG) getOrgAlertDefinitions(query *listAlertDefinitionsQuery) error { - return ng.SQLStore.WithTransactionalDbSession(context.Background(), func(sess *sqlstore.DBSession) error { + return ng.SQLStore.WithDbSession(context.Background(), func(sess *sqlstore.DBSession) error { alertDefinitions := make([]*AlertDefinition, 0) q := "SELECT * FROM alert_definition WHERE org_id = ?" if err := sess.SQL(q, query.OrgID).Find(&alertDefinitions); err != nil { @@ -207,9 +204,9 @@ func (ng *AlertNG) getOrgAlertDefinitions(query *listAlertDefinitionsQuery) erro } func (ng *AlertNG) getAlertDefinitions(query *listAlertDefinitionsQuery) error { - return ng.SQLStore.WithTransactionalDbSession(context.Background(), func(sess *sqlstore.DBSession) error { + return ng.SQLStore.WithDbSession(context.Background(), func(sess *sqlstore.DBSession) error { alerts := make([]*AlertDefinition, 0) - q := "SELECT id, interval_seconds, version FROM alert_definition" + q := "SELECT uid, org_id, interval_seconds, version FROM alert_definition" if err := sess.SQL(q).Find(&alerts); err != nil { return err } @@ -218,7 +215,6 @@ func (ng *AlertNG) getAlertDefinitions(query *listAlertDefinitionsQuery) error { return nil }) } - func generateNewAlertDefinitionUID(sess *sqlstore.DBSession, orgID int64) (string, error) { for i := 0; i < 3; i++ { uid := util.GenerateShortUID() diff --git a/pkg/services/ngalert/database_test.go b/pkg/services/ngalert/database_test.go index eebc82dbbde..8a43f60e492 100644 --- a/pkg/services/ngalert/database_test.go +++ b/pkg/services/ngalert/database_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/grafana/grafana/pkg/registry" "github.com/grafana/grafana/pkg/services/ngalert/eval" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -63,6 +64,8 @@ func TestCreatingAlertDefinition(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { ng := setupTestEnv(t) + t.Cleanup(registry.ClearOverrides) + q := saveAlertDefinitionCommand{ OrgID: 1, Title: tc.inputTitle, @@ -103,14 +106,15 @@ func TestCreatingAlertDefinition(t *testing.T) { } func TestUpdatingAlertDefinition(t *testing.T) { - mockTimeNow() - defer resetTimeNow() - t.Run("zero rows affected when updating unknown alert", func(t *testing.T) { + mockTimeNow() + defer resetTimeNow() + ng := setupTestEnv(t) + t.Cleanup(registry.ClearOverrides) q := updateAlertDefinitionCommand{ - ID: 1, + UID: "unknown", OrgID: 1, Title: "something completely different", Condition: eval.Condition{ @@ -134,11 +138,15 @@ func TestUpdatingAlertDefinition(t *testing.T) { err := ng.updateAlertDefinition(&q) require.NoError(t, err) - assert.Equal(t, int64(0), q.RowsAffected) }) t.Run("updating existing alert", func(t *testing.T) { + mockTimeNow() + defer resetTimeNow() + ng := setupTestEnv(t) + t.Cleanup(registry.ClearOverrides) + var initialInterval int64 = 120 alertDefinition := createTestAlertDefinition(t, ng, initialInterval) created := alertDefinition.Updated @@ -159,7 +167,7 @@ func TestUpdatingAlertDefinition(t *testing.T) { inputOrgID: alertDefinition.OrgID, inputTitle: "something completely different", expectedIntervalSeconds: initialInterval, - expectedUpdated: time.Unix(2, 0).UTC(), + expectedUpdated: time.Unix(1, 0).UTC(), }, { desc: "should update interval if it's provided", @@ -167,7 +175,7 @@ func TestUpdatingAlertDefinition(t *testing.T) { inputOrgID: alertDefinition.OrgID, inputTitle: "something completely different", expectedIntervalSeconds: customInterval, - expectedUpdated: time.Unix(3, 0).UTC(), + expectedUpdated: time.Unix(2, 0).UTC(), }, { desc: "should not update organisation if it's provided", @@ -175,7 +183,7 @@ func TestUpdatingAlertDefinition(t *testing.T) { inputOrgID: 0, inputTitle: "something completely different", expectedIntervalSeconds: customInterval, - expectedUpdated: time.Unix(4, 0).UTC(), + expectedUpdated: time.Unix(3, 0).UTC(), }, { desc: "should not update alert definition if the name it's too big", @@ -187,7 +195,7 @@ func TestUpdatingAlertDefinition(t *testing.T) { } q := updateAlertDefinitionCommand{ - ID: (*alertDefinition).ID, + UID: (*alertDefinition).UID, Title: "something completely different", Condition: eval.Condition{ RefID: "B", @@ -224,20 +232,16 @@ func TestUpdatingAlertDefinition(t *testing.T) { case tc.expectedError != nil: require.Error(t, err) - getAlertDefinitionByIDQuery := getAlertDefinitionByIDQuery{ID: (*alertDefinition).ID} - err = ng.getAlertDefinitionByID(&getAlertDefinitionByIDQuery) - require.NoError(t, err) - assert.Equal(t, previousAlertDefinition.Title, getAlertDefinitionByIDQuery.Result.Title) - assert.Equal(t, previousAlertDefinition.Condition, getAlertDefinitionByIDQuery.Result.Condition) - assert.Equal(t, len(previousAlertDefinition.Data), len(getAlertDefinitionByIDQuery.Result.Data)) - assert.Equal(t, previousAlertDefinition.IntervalSeconds, getAlertDefinitionByIDQuery.Result.IntervalSeconds) - assert.Equal(t, previousAlertDefinition.Updated, getAlertDefinitionByIDQuery.Result.Updated) - assert.Equal(t, previousAlertDefinition.Version, getAlertDefinitionByIDQuery.Result.Version) - assert.Equal(t, previousAlertDefinition.OrgID, getAlertDefinitionByIDQuery.Result.OrgID) - assert.Equal(t, previousAlertDefinition.UID, getAlertDefinitionByIDQuery.Result.UID) + assert.Equal(t, previousAlertDefinition.Title, q.Result.Title) + assert.Equal(t, previousAlertDefinition.Condition, q.Result.Condition) + assert.Equal(t, len(previousAlertDefinition.Data), len(q.Result.Data)) + assert.Equal(t, previousAlertDefinition.IntervalSeconds, q.Result.IntervalSeconds) + assert.Equal(t, previousAlertDefinition.Updated, q.Result.Updated) + assert.Equal(t, previousAlertDefinition.Version, q.Result.Version) + assert.Equal(t, previousAlertDefinition.OrgID, q.Result.OrgID) + assert.Equal(t, previousAlertDefinition.UID, q.Result.UID) default: require.NoError(t, err) - assert.Equal(t, int64(1), q.RowsAffected) assert.Equal(t, int64(1), q.Result.ID) assert.True(t, q.Result.Updated.After(lastUpdated)) assert.Equal(t, tc.expectedUpdated, q.Result.Updated) @@ -245,19 +249,16 @@ func TestUpdatingAlertDefinition(t *testing.T) { assert.Equal(t, alertDefinition.OrgID, q.Result.OrgID) - getAlertDefinitionByIDQuery := getAlertDefinitionByIDQuery{ID: (*alertDefinition).ID} - err = ng.getAlertDefinitionByID(&getAlertDefinitionByIDQuery) - require.NoError(t, err) - assert.Equal(t, "something completely different", getAlertDefinitionByIDQuery.Result.Title) - assert.Equal(t, "B", getAlertDefinitionByIDQuery.Result.Condition) - assert.Equal(t, 1, len(getAlertDefinitionByIDQuery.Result.Data)) - assert.Equal(t, tc.expectedUpdated, getAlertDefinitionByIDQuery.Result.Updated) - assert.Equal(t, tc.expectedIntervalSeconds, getAlertDefinitionByIDQuery.Result.IntervalSeconds) - assert.Equal(t, previousAlertDefinition.Version+1, getAlertDefinitionByIDQuery.Result.Version) - assert.Equal(t, alertDefinition.OrgID, getAlertDefinitionByIDQuery.Result.OrgID) - assert.Equal(t, alertDefinition.UID, getAlertDefinitionByIDQuery.Result.UID) + assert.Equal(t, "something completely different", q.Result.Title) + assert.Equal(t, "B", q.Result.Condition) + assert.Equal(t, 1, len(q.Result.Data)) + assert.Equal(t, tc.expectedUpdated, q.Result.Updated) + assert.Equal(t, tc.expectedIntervalSeconds, q.Result.IntervalSeconds) + assert.Equal(t, previousAlertDefinition.Version+1, q.Result.Version) + assert.Equal(t, alertDefinition.OrgID, q.Result.OrgID) + assert.Equal(t, alertDefinition.UID, q.Result.UID) - previousAlertDefinition = getAlertDefinitionByIDQuery.Result + previousAlertDefinition = q.Result } }) @@ -269,29 +270,30 @@ func TestUpdatingAlertDefinition(t *testing.T) { func TestDeletingAlertDefinition(t *testing.T) { t.Run("zero rows affected when deleting unknown alert", func(t *testing.T) { ng := setupTestEnv(t) + t.Cleanup(registry.ClearOverrides) - q := deleteAlertDefinitionByIDCommand{ - ID: 1, + q := deleteAlertDefinitionByUIDCommand{ + UID: "unknown", OrgID: 1, } - err := ng.deleteAlertDefinitionByID(&q) + err := ng.deleteAlertDefinitionByUID(&q) require.NoError(t, err) - assert.Equal(t, int64(0), q.RowsAffected) }) t.Run("deleting successfully existing alert", func(t *testing.T) { ng := setupTestEnv(t) + t.Cleanup(registry.ClearOverrides) + alertDefinition := createTestAlertDefinition(t, ng, 60) - q := deleteAlertDefinitionByIDCommand{ - ID: (*alertDefinition).ID, + q := deleteAlertDefinitionByUIDCommand{ + UID: (*alertDefinition).UID, OrgID: 1, } - err := ng.deleteAlertDefinitionByID(&q) + err := ng.deleteAlertDefinitionByUID(&q) require.NoError(t, err) - assert.Equal(t, int64(1), q.RowsAffected) }) } diff --git a/pkg/services/ngalert/middleware.go b/pkg/services/ngalert/middleware.go index 253baca9d41..77566d3c213 100644 --- a/pkg/services/ngalert/middleware.go +++ b/pkg/services/ngalert/middleware.go @@ -5,10 +5,10 @@ import ( ) func (ng *AlertNG) validateOrgAlertDefinition(c *models.ReqContext) { - id := c.ParamsInt64(":alertDefinitionId") - query := getAlertDefinitionByIDQuery{ID: id} + uid := c.ParamsEscape(":alertDefinitionUID") + query := getAlertDefinitionByUIDQuery{UID: uid, OrgID: c.SignedInUser.OrgId} - if err := ng.getAlertDefinitionByID(&query); err != nil { + if err := ng.getAlertDefinitionByUID(&query); err != nil { c.JsonApiErr(404, "Alert definition not found", nil) return } diff --git a/pkg/services/ngalert/models.go b/pkg/services/ngalert/models.go index 707906d4939..ad54a0c5205 100644 --- a/pkg/services/ngalert/models.go +++ b/pkg/services/ngalert/models.go @@ -23,6 +23,19 @@ type AlertDefinition struct { UID string `xorm:"uid"` } +type alertDefinitionKey struct { + orgID int64 + definitionUID string +} + +func (k alertDefinitionKey) String() string { + return fmt.Sprintf("{orgID: %d, definitionUID: %s}", k.orgID, k.definitionUID) +} + +func (alertDefinition *AlertDefinition) getKey() alertDefinitionKey { + return alertDefinitionKey{orgID: alertDefinition.OrgID, definitionUID: alertDefinition.UID} +} + // AlertDefinitionVersion is the model for alert definition versions in Alerting NG. type AlertDefinitionVersion struct { ID int64 `xorm:"pk autoincr 'id'"` @@ -44,19 +57,17 @@ var ( errAlertDefinitionNotFound = fmt.Errorf("could not find alert definition") ) -// getAlertDefinitionByIDQuery is the query for retrieving/deleting an alert definition by ID. -type getAlertDefinitionByIDQuery struct { - ID int64 +// getAlertDefinitionByUIDQuery is the query for retrieving/deleting an alert definition by UID and organisation ID. +type getAlertDefinitionByUIDQuery struct { + UID string OrgID int64 Result *AlertDefinition } -type deleteAlertDefinitionByIDCommand struct { - ID int64 +type deleteAlertDefinitionByUIDCommand struct { + UID string OrgID int64 - - RowsAffected int64 } // saveAlertDefinitionCommand is the query for saving a new alert definition. @@ -71,15 +82,13 @@ type saveAlertDefinitionCommand struct { // updateAlertDefinitionCommand is the query for updating an existing alert definition. type updateAlertDefinitionCommand struct { - ID int64 `json:"-"` Title string `json:"title"` OrgID int64 `json:"-"` Condition eval.Condition `json:"condition"` IntervalSeconds *int64 `json:"interval_seconds"` UID string `json:"-"` - RowsAffected int64 - Result *AlertDefinition + Result *AlertDefinition } type evalAlertConditionCommand struct { diff --git a/pkg/services/ngalert/ngalert.go b/pkg/services/ngalert/ngalert.go index 9399f1dd7e7..ff21b6c42dc 100644 --- a/pkg/services/ngalert/ngalert.go +++ b/pkg/services/ngalert/ngalert.go @@ -77,12 +77,12 @@ func (ng *AlertNG) AddMigration(mg *migrator.Migrator) { } // LoadAlertCondition returns a Condition object for the given alertDefinitionID. -func (ng *AlertNG) LoadAlertCondition(alertDefinitionID int64) (*eval.Condition, error) { - getAlertDefinitionByIDQuery := getAlertDefinitionByIDQuery{ID: alertDefinitionID} - if err := ng.getAlertDefinitionByID(&getAlertDefinitionByIDQuery); err != nil { +func (ng *AlertNG) LoadAlertCondition(alertDefinitionUID string, orgID int64) (*eval.Condition, error) { + q := getAlertDefinitionByUIDQuery{UID: alertDefinitionUID, OrgID: orgID} + if err := ng.getAlertDefinitionByUID(&q); err != nil { return nil, err } - alertDefinition := getAlertDefinitionByIDQuery.Result + alertDefinition := q.Result err := ng.validateAlertDefinition(alertDefinition, true) if err != nil { diff --git a/pkg/services/ngalert/schedule.go b/pkg/services/ngalert/schedule.go index 1210728ccf3..74cf5616296 100644 --- a/pkg/services/ngalert/schedule.go +++ b/pkg/services/ngalert/schedule.go @@ -13,8 +13,8 @@ import ( "golang.org/x/sync/errgroup" ) -func (ng *AlertNG) definitionRoutine(grafanaCtx context.Context, definitionID int64, evalCh <-chan *evalContext) error { - ng.log.Debug("alert definition routine started", "definitionID", definitionID) +func (ng *AlertNG) definitionRoutine(grafanaCtx context.Context, key alertDefinitionKey, evalCh <-chan *evalContext) error { + ng.log.Debug("alert definition routine started", "key", key) evalRunning := false var start, end time.Time @@ -32,14 +32,14 @@ func (ng *AlertNG) definitionRoutine(grafanaCtx context.Context, definitionID in // fetch latest alert definition version if alertDefinition == nil || alertDefinition.Version < ctx.version { - q := getAlertDefinitionByIDQuery{ID: definitionID} - err := ng.getAlertDefinitionByID(&q) + q := getAlertDefinitionByUIDQuery{OrgID: key.orgID, UID: key.definitionUID} + err := ng.getAlertDefinitionByUID(&q) if err != nil { - ng.schedule.log.Error("failed to fetch alert definition", "alertDefinitionID", alertDefinition.ID) + ng.schedule.log.Error("failed to fetch alert definition", "key", key) return err } alertDefinition = q.Result - ng.schedule.log.Debug("new alert definition version fetched", "alertDefinitionID", alertDefinition.ID, "version", alertDefinition.Version) + ng.schedule.log.Debug("new alert definition version fetched", "key", key, "version", alertDefinition.Version) } condition := eval.Condition{ @@ -50,11 +50,11 @@ func (ng *AlertNG) definitionRoutine(grafanaCtx context.Context, definitionID in results, err := eval.ConditionEval(&condition, ctx.now) end = timeNow() if err != nil { - ng.schedule.log.Error("failed to evaluate alert definition", "definitionID", definitionID, "attempt", attempt, "now", ctx.now, "duration", end.Sub(start), "error", err) + ng.schedule.log.Error("failed to evaluate alert definition", "key", key, "attempt", attempt, "now", ctx.now, "duration", end.Sub(start), "error", err) return err } for _, r := range results { - ng.schedule.log.Info("alert definition result", "definitionID", definitionID, "attempt", attempt, "now", ctx.now, "duration", end.Sub(start), "instance", r.Instance, "state", r.State.String()) + ng.schedule.log.Info("alert definition result", "key", key, "attempt", attempt, "now", ctx.now, "duration", end.Sub(start), "instance", r.Instance, "state", r.State.String()) } return nil } @@ -64,7 +64,7 @@ func (ng *AlertNG) definitionRoutine(grafanaCtx context.Context, definitionID in defer func() { evalRunning = false if ng.schedule.evalApplied != nil { - ng.schedule.evalApplied(definitionID, ctx.now) + ng.schedule.evalApplied(key, ctx.now) } }() @@ -75,9 +75,9 @@ func (ng *AlertNG) definitionRoutine(grafanaCtx context.Context, definitionID in } } }() - case id := <-ng.schedule.stop: - if id == definitionID { - ng.schedule.log.Debug("stopping alert definition routine", "definitionID", definitionID) + case k := <-ng.schedule.stop: + if k == key { + ng.schedule.log.Debug("stopping alert definition routine", "key", key) // interrupt evaluation if it's running return nil } @@ -95,7 +95,7 @@ type schedule struct { registry alertDefinitionRegistry // broadcast channel for stopping definition routines - stop chan int64 + stop chan alertDefinitionKey maxAttempts int64 @@ -106,17 +106,17 @@ type schedule struct { // evalApplied is only used for tests: test code can set it to non-nil // function, and then it'll be called from the event loop whenever the // message from evalApplied is handled. - evalApplied func(int64, time.Time) + evalApplied func(alertDefinitionKey, time.Time) log log.Logger } // newScheduler returns a new schedule. -func newScheduler(c clock.Clock, baseInterval time.Duration, logger log.Logger, evalApplied func(int64, time.Time)) *schedule { +func newScheduler(c clock.Clock, baseInterval time.Duration, logger log.Logger, evalApplied func(alertDefinitionKey, time.Time)) *schedule { ticker := alerting.NewTicker(c.Now(), time.Second*0, c, int64(baseInterval.Seconds())) sch := schedule{ - registry: alertDefinitionRegistry{alertDefinitionInfo: make(map[int64]alertDefinitionInfo)}, - stop: make(chan int64), + registry: alertDefinitionRegistry{alertDefinitionInfo: make(map[alertDefinitionKey]alertDefinitionInfo)}, + stop: make(chan alertDefinitionKey), maxAttempts: maxAttempts, clock: c, baseInterval: baseInterval, @@ -161,37 +161,39 @@ func (ng *AlertNG) alertingTicker(grafanaCtx context.Context) error { registeredDefinitions := ng.schedule.registry.keyMap() type readyToRunItem struct { - id int64 + key alertDefinitionKey definitionInfo alertDefinitionInfo } readyToRun := make([]readyToRunItem, 0) for _, item := range alertDefinitions { - itemID := item.ID + itemUID := item.UID + itemOrgID := item.OrgID + key := item.getKey() itemVersion := item.Version - newRoutine := !ng.schedule.registry.exists(itemID) - definitionInfo := ng.schedule.registry.getOrCreateInfo(itemID, itemVersion) + newRoutine := !ng.schedule.registry.exists(key) + definitionInfo := ng.schedule.registry.getOrCreateInfo(key, itemVersion) invalidInterval := item.IntervalSeconds%int64(ng.schedule.baseInterval.Seconds()) != 0 if newRoutine && !invalidInterval { dispatcherGroup.Go(func() error { - return ng.definitionRoutine(ctx, itemID, definitionInfo.ch) + return ng.definitionRoutine(ctx, key, definitionInfo.ch) }) } if invalidInterval { // this is expected to be always false // give that we validate interval during alert definition updates - ng.schedule.log.Debug("alert definition with invalid interval will be ignored: interval should be divided exactly by scheduler interval", "definitionID", itemID, "interval", time.Duration(item.IntervalSeconds)*time.Second, "scheduler interval", ng.schedule.baseInterval) + ng.schedule.log.Debug("alert definition with invalid interval will be ignored: interval should be divided exactly by scheduler interval", "definitionUID", itemUID, "orgID", itemOrgID, "interval", time.Duration(item.IntervalSeconds)*time.Second, "scheduler interval", ng.schedule.baseInterval) continue } itemFrequency := item.IntervalSeconds / int64(ng.schedule.baseInterval.Seconds()) if item.IntervalSeconds != 0 && tickNum%itemFrequency == 0 { - readyToRun = append(readyToRun, readyToRunItem{id: itemID, definitionInfo: definitionInfo}) + readyToRun = append(readyToRun, readyToRunItem{key: key, definitionInfo: definitionInfo}) } // remove the alert definition from the registered alert definitions - delete(registeredDefinitions, itemID) + delete(registeredDefinitions, key) } var step int64 = 0 @@ -208,9 +210,9 @@ func (ng *AlertNG) alertingTicker(grafanaCtx context.Context) error { } // unregister and stop routines of the deleted alert definitions - for id := range registeredDefinitions { - ng.schedule.stop <- id - ng.schedule.registry.del(id) + for key := range registeredDefinitions { + ng.schedule.stop <- key + ng.schedule.registry.del(key) } case <-grafanaCtx.Done(): err := dispatcherGroup.Wait() @@ -221,42 +223,42 @@ func (ng *AlertNG) alertingTicker(grafanaCtx context.Context) error { type alertDefinitionRegistry struct { mu sync.Mutex - alertDefinitionInfo map[int64]alertDefinitionInfo + alertDefinitionInfo map[alertDefinitionKey]alertDefinitionInfo } // getOrCreateInfo returns the channel for the specific alert definition // if it does not exists creates one and returns it -func (r *alertDefinitionRegistry) getOrCreateInfo(definitionID int64, definitionVersion int64) alertDefinitionInfo { +func (r *alertDefinitionRegistry) getOrCreateInfo(key alertDefinitionKey, definitionVersion int64) alertDefinitionInfo { r.mu.Lock() defer r.mu.Unlock() - info, ok := r.alertDefinitionInfo[definitionID] + info, ok := r.alertDefinitionInfo[key] if !ok { - r.alertDefinitionInfo[definitionID] = alertDefinitionInfo{ch: make(chan *evalContext), version: definitionVersion} - return r.alertDefinitionInfo[definitionID] + r.alertDefinitionInfo[key] = alertDefinitionInfo{ch: make(chan *evalContext), version: definitionVersion} + return r.alertDefinitionInfo[key] } info.version = definitionVersion - r.alertDefinitionInfo[definitionID] = info + r.alertDefinitionInfo[key] = info return info } -func (r *alertDefinitionRegistry) exists(definitionID int64) bool { +func (r *alertDefinitionRegistry) exists(key alertDefinitionKey) bool { r.mu.Lock() defer r.mu.Unlock() - _, ok := r.alertDefinitionInfo[definitionID] + _, ok := r.alertDefinitionInfo[key] return ok } -func (r *alertDefinitionRegistry) del(definitionID int64) { +func (r *alertDefinitionRegistry) del(key alertDefinitionKey) { r.mu.Lock() defer r.mu.Unlock() - delete(r.alertDefinitionInfo, definitionID) + delete(r.alertDefinitionInfo, key) } -func (r *alertDefinitionRegistry) iter() <-chan int64 { - c := make(chan int64) +func (r *alertDefinitionRegistry) iter() <-chan alertDefinitionKey { + c := make(chan alertDefinitionKey) f := func() { r.mu.Lock() @@ -272,10 +274,10 @@ func (r *alertDefinitionRegistry) iter() <-chan int64 { return c } -func (r *alertDefinitionRegistry) keyMap() map[int64]struct{} { - definitionsIDs := make(map[int64]struct{}) - for definitionID := range r.iter() { - definitionsIDs[definitionID] = struct{}{} +func (r *alertDefinitionRegistry) keyMap() map[alertDefinitionKey]struct{} { + definitionsIDs := make(map[alertDefinitionKey]struct{}) + for k := range r.iter() { + definitionsIDs[k] = struct{}{} } return definitionsIDs } diff --git a/pkg/services/ngalert/schedule_test.go b/pkg/services/ngalert/schedule_test.go index 918036a5764..0082cf08f39 100644 --- a/pkg/services/ngalert/schedule_test.go +++ b/pkg/services/ngalert/schedule_test.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "runtime" - "strconv" "strings" "testing" "time" @@ -18,8 +17,8 @@ import ( ) type evalAppliedInfo struct { - alertDefID int64 - now time.Time + alertDefKey alertDefinitionKey + now time.Time } func TestAlertingTicker(t *testing.T) { @@ -39,8 +38,8 @@ func TestAlertingTicker(t *testing.T) { evalAppliedCh := make(chan evalAppliedInfo, len(alerts)) - ng.schedule.evalApplied = func(alertDefID int64, now time.Time) { - evalAppliedCh <- evalAppliedInfo{alertDefID: alertDefID, now: now} + ng.schedule.evalApplied = func(alertDefKey alertDefinitionKey, now time.Time) { + evalAppliedCh <- evalAppliedInfo{alertDefKey: alertDefKey, now: now} } ctx := context.Background() @@ -50,7 +49,7 @@ func TestAlertingTicker(t *testing.T) { }() runtime.Gosched() - expectedAlertDefinitionsEvaluated := []int64{alerts[1].ID} + expectedAlertDefinitionsEvaluated := []alertDefinitionKey{alerts[1].getKey()} t.Run(fmt.Sprintf("on 1st tick alert definitions: %s should be evaluated", concatenate(expectedAlertDefinitionsEvaluated)), func(t *testing.T) { tick := advanceClock(t, mockedClock) assertEvalRun(t, evalAppliedCh, tick, expectedAlertDefinitionsEvaluated...) @@ -59,42 +58,42 @@ func TestAlertingTicker(t *testing.T) { // change alert definition interval to three seconds var threeSecInterval int64 = 3 err := ng.updateAlertDefinition(&updateAlertDefinitionCommand{ - ID: alerts[0].ID, + UID: alerts[0].UID, IntervalSeconds: &threeSecInterval, OrgID: alerts[0].OrgID, }) require.NoError(t, err) - t.Logf("alert definition: %d interval reset to: %d", alerts[0].ID, threeSecInterval) + t.Logf("alert definition: %v interval reset to: %d", alerts[0].getKey(), threeSecInterval) - expectedAlertDefinitionsEvaluated = []int64{alerts[1].ID} + expectedAlertDefinitionsEvaluated = []alertDefinitionKey{alerts[1].getKey()} t.Run(fmt.Sprintf("on 2nd tick alert definition: %s should be evaluated", concatenate(expectedAlertDefinitionsEvaluated)), func(t *testing.T) { tick := advanceClock(t, mockedClock) assertEvalRun(t, evalAppliedCh, tick, expectedAlertDefinitionsEvaluated...) }) - expectedAlertDefinitionsEvaluated = []int64{alerts[1].ID, alerts[0].ID} + expectedAlertDefinitionsEvaluated = []alertDefinitionKey{alerts[1].getKey(), alerts[0].getKey()} t.Run(fmt.Sprintf("on 3rd tick alert definitions: %s should be evaluated", concatenate(expectedAlertDefinitionsEvaluated)), func(t *testing.T) { tick := advanceClock(t, mockedClock) assertEvalRun(t, evalAppliedCh, tick, expectedAlertDefinitionsEvaluated...) }) - expectedAlertDefinitionsEvaluated = []int64{alerts[1].ID} + expectedAlertDefinitionsEvaluated = []alertDefinitionKey{alerts[1].getKey()} t.Run(fmt.Sprintf("on 4th tick alert definitions: %s should be evaluated", concatenate(expectedAlertDefinitionsEvaluated)), func(t *testing.T) { tick := advanceClock(t, mockedClock) assertEvalRun(t, evalAppliedCh, tick, expectedAlertDefinitionsEvaluated...) }) - err = ng.deleteAlertDefinitionByID(&deleteAlertDefinitionByIDCommand{ID: alerts[1].ID}) + err = ng.deleteAlertDefinitionByUID(&deleteAlertDefinitionByUIDCommand{UID: alerts[1].UID, OrgID: alerts[1].OrgID}) require.NoError(t, err) - t.Logf("alert definition: %d deleted", alerts[1].ID) + t.Logf("alert definition: %v deleted", alerts[1].getKey()) - expectedAlertDefinitionsEvaluated = []int64{} + expectedAlertDefinitionsEvaluated = []alertDefinitionKey{} t.Run(fmt.Sprintf("on 5th tick alert definitions: %s should be evaluated", concatenate(expectedAlertDefinitionsEvaluated)), func(t *testing.T) { tick := advanceClock(t, mockedClock) assertEvalRun(t, evalAppliedCh, tick, expectedAlertDefinitionsEvaluated...) }) - expectedAlertDefinitionsEvaluated = []int64{alerts[0].ID} + expectedAlertDefinitionsEvaluated = []alertDefinitionKey{alerts[0].getKey()} t.Run(fmt.Sprintf("on 6th tick alert definitions: %s should be evaluated", concatenate(expectedAlertDefinitionsEvaluated)), func(t *testing.T) { tick := advanceClock(t, mockedClock) assertEvalRun(t, evalAppliedCh, tick, expectedAlertDefinitionsEvaluated...) @@ -103,29 +102,29 @@ func TestAlertingTicker(t *testing.T) { // create alert definition with one second interval alerts = append(alerts, createTestAlertDefinition(t, ng, 1)) - expectedAlertDefinitionsEvaluated = []int64{alerts[2].ID} + expectedAlertDefinitionsEvaluated = []alertDefinitionKey{alerts[2].getKey()} t.Run(fmt.Sprintf("on 7th tick alert definitions: %s should be evaluated", concatenate(expectedAlertDefinitionsEvaluated)), func(t *testing.T) { tick := advanceClock(t, mockedClock) assertEvalRun(t, evalAppliedCh, tick, expectedAlertDefinitionsEvaluated...) }) } -func assertEvalRun(t *testing.T, ch <-chan evalAppliedInfo, tick time.Time, ids ...int64) { +func assertEvalRun(t *testing.T, ch <-chan evalAppliedInfo, tick time.Time, keys ...alertDefinitionKey) { timeout := time.After(time.Second) - expected := make(map[int64]struct{}, len(ids)) - for _, id := range ids { - expected[id] = struct{}{} + expected := make(map[alertDefinitionKey]struct{}, len(keys)) + for _, k := range keys { + expected[k] = struct{}{} } for { select { case info := <-ch: - _, ok := expected[info.alertDefID] - t.Logf("alert definition: %d evaluated at: %v", info.alertDefID, info.now) + _, ok := expected[info.alertDefKey] + t.Logf("alert definition: %v evaluated at: %v", info.alertDefKey, info.now) assert.True(t, ok) assert.Equal(t, tick, info.now) - delete(expected, info.alertDefID) + delete(expected, info.alertDefKey) if len(expected) == 0 { return } @@ -144,10 +143,10 @@ func advanceClock(t *testing.T, mockedClock *clock.Mock) time.Time { // t.Logf("Tick: %v", mockedClock.Now()) } -func concatenate(ids []int64) string { - s := make([]string, len(ids)) - for _, id := range ids { - s = append(s, strconv.FormatInt(id, 10)) +func concatenate(keys []alertDefinitionKey) string { + s := make([]string, len(keys)) + for _, k := range keys { + s = append(s, k.String()) } return fmt.Sprintf("[%s]", strings.TrimLeft(strings.Join(s, ","), ",")) }