public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Alerting: Remove constraints for uniqueness of rule title (#102067)

Browse Source 
* fix having duplicated names in same group in the UI

---------

Co-authored-by: Sonia Aguilar <soniaaguilarpeiron@gmail.com>
This commit is contained in:
Yuri Tseretyan
2025-03-18 13:27:44 -04:00
committed by GitHub
co-authored by Sonia Aguilar
parent e948376a95
commit e39b17d701
14 changed files with 78 additions and 333 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/services/ngalert/api/api_provisioning.go
+2 -10
View File
@@ -8,6 +8,8 @@ import (
"regexp"
"strings"
alertmanager_config "github.com/prometheus/alertmanager/config"
"github.com/grafana/grafana/pkg/api/response"
"github.com/grafana/grafana/pkg/apimachinery/identity"
"github.com/grafana/grafana/pkg/infra/log"
@@ -21,7 +23,6 @@ import (
"github.com/grafana/grafana/pkg/services/ngalert/provisioning"
"github.com/grafana/grafana/pkg/services/ngalert/store"
"github.com/grafana/grafana/pkg/util"
alertmanager_config "github.com/prometheus/alertmanager/config"
)
const disableProvenanceHeaderName = "X-Disable-Provenance"
@@ -357,9 +358,6 @@ func (srv *ProvisioningSrv) RoutePostAlertRule(c *contextmodel.ReqContext, ar de
return ErrResp(http.StatusBadRequest, err, "")
}
if err != nil {
if errors.Is(err, alerting_models.ErrAlertRuleUniqueConstraintViolation) {
return ErrResp(http.StatusBadRequest, err, "")
}
if errors.Is(err, store.ErrOptimisticLock) {
return ErrResp(http.StatusConflict, err, "")
}
@@ -396,9 +394,6 @@ func (srv *ProvisioningSrv) RoutePutAlertRule(c *contextmodel.ReqContext, ar def
updated.UID = UID
provenance := determineProvenance(c)
updatedAlertRule, err := srv.alertRules.UpdateAlertRule(c.Req.Context(), c.SignedInUser, updated, alerting_models.Provenance(provenance))
if errors.Is(err, alerting_models.ErrAlertRuleUniqueConstraintViolation) {
return ErrResp(http.StatusBadRequest, err, "")
}
if errors.Is(err, alerting_models.ErrAlertRuleNotFound) {
return response.Empty(http.StatusNotFound)
}
@@ -514,9 +509,6 @@ func (srv *ProvisioningSrv) RoutePutAlertRuleGroup(c *contextmodel.ReqContext, a
}
provenance := determineProvenance(c)
err = srv.alertRules.ReplaceRuleGroup(c.Req.Context(), c.SignedInUser, groupModel, alerting_models.Provenance(provenance))
if errors.Is(err, alerting_models.ErrAlertRuleUniqueConstraintViolation) {
return ErrResp(http.StatusBadRequest, err, "")
}
if errors.Is(err, alerting_models.ErrAlertRuleFailedValidation) {
return ErrResp(http.StatusBadRequest, err, "")
}
pkg/services/ngalert/models/alert_rule.go
+4 -5
View File
@@ -34,11 +34,10 @@ var (
// ErrAlertRuleFailedGenerateUniqueUID is an error for failure to generate alert rule UID
ErrAlertRuleFailedGenerateUniqueUID = errors.New("failed to generate alert rule UID")
// ErrCannotEditNamespace is an error returned if the user does not have permissions to edit the namespace
ErrCannotEditNamespace = errors.New("user does not have permissions to edit the namespace")
ErrRuleGroupNamespaceNotFound = errors.New("rule group not found under this namespace")
ErrAlertRuleFailedValidation = errors.New("invalid alert rule")
ErrAlertRuleUniqueConstraintViolation = errors.New("rule title under the same organisation and folder should be unique")
ErrQuotaReached = errors.New("quota has been exceeded")
ErrCannotEditNamespace = errors.New("user does not have permissions to edit the namespace")
ErrRuleGroupNamespaceNotFound = errors.New("rule group not found under this namespace")
ErrAlertRuleFailedValidation = errors.New("invalid alert rule")
ErrQuotaReached = errors.New("quota has been exceeded")
// ErrNoDashboard is returned when the alert rule does not have a Dashboard UID
// in its annotations or the dashboard does not exist.
ErrNoDashboard = errors.New("no dashboard")
pkg/services/ngalert/models/errors.go
+2
View File
@@ -28,11 +28,13 @@ func ErrAlertRuleConflictVerbose(existingPartialRule, rule AlertRule, underlying
"RuleUID": rule.UID,
"Title": rule.Title,
"NamespaceUID": rule.NamespaceUID,
"RuleGroup": rule.RuleGroup,
},
"Existing": map[string]any{
"RuleUID": existingPartialRule.UID,
"Title": existingPartialRule.Title,
"NamespaceUID": existingPartialRule.NamespaceUID,
"RuleGroup": existingPartialRule.RuleGroup,
},
"Error": underlying.Error(),
}, Error: underlying})
pkg/services/ngalert/store/alert_rule.go
+4 -18
View File
@@ -15,7 +15,6 @@ import (
"github.com/grafana/grafana/pkg/apimachinery/identity"
"github.com/grafana/grafana/pkg/infra/db"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/dashboards"
"github.com/grafana/grafana/pkg/services/featuremgmt"
@@ -374,7 +373,7 @@ func (st DBstore) InsertAlertRules(ctx context.Context, user *ngmodels.UserUID,
for i := range newRules {
if _, err := sess.Insert(&newRules[i]); err != nil {
if st.SQLStore.GetDialect().IsUniqueConstraintViolation(err) {
return ruleConstraintViolationToErr(sess, rules[i], err, st.Logger)
return ruleConstraintViolationToErr(rules[i], err)
}
return fmt.Errorf("failed to create new rules: %w", err)
}
@@ -432,7 +431,7 @@ func (st DBstore) UpdateAlertRules(ctx context.Context, user *ngmodels.UserUID,
if updated, err := sess.ID(r.Existing.ID).AllCols().Omit("rule_guid").Update(converted); err != nil || updated == 0 {
if err != nil {
if st.SQLStore.GetDialect().IsUniqueConstraintViolation(err) {
return ruleConstraintViolationToErr(sess, r.New, err, st.Logger)
return ruleConstraintViolationToErr(r.New, err)
}
return fmt.Errorf("failed to update rule [%s] %s: %w", r.New.UID, r.New.Title, err)
}
@@ -1201,22 +1200,9 @@ func (st DBstore) RenameTimeIntervalInNotificationSettings(
return result, nil, st.UpdateAlertRules(ctx, &ngmodels.AlertingUserUID, updates)
}
func ruleConstraintViolationToErr(sess *db.Session, rule ngmodels.AlertRule, err error, logger log.Logger) error {
func ruleConstraintViolationToErr(rule ngmodels.AlertRule, err error) error {
msg := err.Error()
if strings.Contains(msg, "UQE_alert_rule_org_id_namespace_uid_title") || strings.Contains(msg, "alert_rule.org_id, alert_rule.namespace_uid, alert_rule.title") {
// return verbose conflicting alert rule error response
// see: https://github.com/grafana/grafana/issues/89755
var fetched_uid string
var existingPartialAlertRule ngmodels.AlertRule
ok, uid_fetch_err := sess.Table("alert_rule").Cols("uid").Where("org_id = ? AND title = ? AND namespace_uid = ?", rule.OrgID, rule.Title, rule.NamespaceUID).Get(&fetched_uid)
if uid_fetch_err != nil {
logger.Error("Error fetching uid from alert_rule table", "reason", uid_fetch_err.Error())
}
if ok {
existingPartialAlertRule = ngmodels.AlertRule{UID: fetched_uid, Title: rule.Title, NamespaceUID: rule.NamespaceUID}
}
return ngmodels.ErrAlertRuleConflictVerbose(existingPartialAlertRule, rule, ngmodels.ErrAlertRuleUniqueConstraintViolation)
} else if strings.Contains(msg, "UQE_alert_rule_org_id_uid") || strings.Contains(msg, "alert_rule.org_id, alert_rule.uid") {
if strings.Contains(msg, "UQE_alert_rule_org_id_uid") || strings.Contains(msg, "alert_rule.org_id, alert_rule.uid") {
// return verbose conflicting alert rule error response
// see: https://github.com/grafana/grafana/issues/89755
existingPartialAlertRule := ngmodels.AlertRule{UID: rule.UID}
pkg/services/ngalert/store/alert_rule_test.go
+2 -260
View File
@@ -230,257 +230,6 @@ func TestIntegrationUpdateAlertRules(t *testing.T) {
})
}
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)
folderService := setupFolderService(t, sqlStore, cfg, featuremgmt.WithFeatures())
b := &fakeBus{}
store := createTestStore(sqlStore, folderService, &logtest.Fake{}, cfg.UnifiedAlerting, b)
gen := models.RuleGen
createRuleInFolder := func(title string, orgID int64, namespaceUID string) *models.AlertRule {
gen := gen.With(
gen.WithOrgID(orgID),
gen.WithIntervalMatching(store.Cfg.BaseInterval),
gen.WithNamespaceUID(namespaceUID),
)
return createRule(t, store, gen)
}
t.Run("should handle update chains without unique constraint violation", func(t *testing.T) {
rule1 := createRuleInFolder("chain-rule1", 1, "my-namespace")
rule2 := createRuleInFolder("chain-rule2", 1, "my-namespace")
newRule1 := models.CopyRule(rule1)
newRule2 := models.CopyRule(rule2)
newRule1.Title = rule2.Title
newRule2.Title = util.GenerateShortUID()
err := store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{
Existing: rule1,
New: *newRule1,
}, {
Existing: rule2,
New: *newRule2,
},
})
require.NoError(t, err)
dbrule1 := &alertRule{}
dbrule2 := &alertRule{}
err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error {
exist, err := sess.Table(alertRule{}).ID(rule1.ID).Get(dbrule1)
if err != nil {
return err
}
require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule1.ID))
exist, err = sess.Table(alertRule{}).ID(rule2.ID).Get(dbrule2)
if err != nil {
return err
}
require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule2.ID))
return nil
})
require.NoError(t, err)
require.Equal(t, newRule1.Title, dbrule1.Title)
require.Equal(t, newRule2.Title, dbrule2.Title)
})
t.Run("should handle update chains with cycle without unique constraint violation", func(t *testing.T) {
rule1 := createRuleInFolder("cycle-rule1", 1, "my-namespace")
rule2 := createRuleInFolder("cycle-rule2", 1, "my-namespace")
rule3 := createRuleInFolder("cycle-rule3", 1, "my-namespace")
newRule1 := models.CopyRule(rule1)
newRule2 := models.CopyRule(rule2)
newRule3 := models.CopyRule(rule3)
newRule1.Title = rule2.Title
newRule2.Title = rule3.Title
newRule3.Title = rule1.Title
err := store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{
Existing: rule1,
New: *newRule1,
}, {
Existing: rule2,
New: *newRule2,
}, {
Existing: rule3,
New: *newRule3,
},
})
require.NoError(t, err)
dbrule1 := &alertRule{}
dbrule2 := &alertRule{}
dbrule3 := &alertRule{}
err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error {
exist, err := sess.Table(alertRule{}).ID(rule1.ID).Get(dbrule1)
if err != nil {
return err
}
require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule1.ID))
exist, err = sess.Table(alertRule{}).ID(rule2.ID).Get(dbrule2)
if err != nil {
return err
}
require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule2.ID))
exist, err = sess.Table(alertRule{}).ID(rule3.ID).Get(dbrule3)
if err != nil {
return err
}
require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule3.ID))
return nil
})
require.NoError(t, err)
require.Equal(t, newRule1.Title, dbrule1.Title)
require.Equal(t, newRule2.Title, dbrule2.Title)
require.Equal(t, newRule3.Title, dbrule3.Title)
})
t.Run("should handle case-insensitive intermediate collision without unique constraint violation", func(t *testing.T) {
rule1 := createRuleInFolder("case-cycle-rule1", 1, "my-namespace")
rule2 := createRuleInFolder("case-cycle-rule2", 1, "my-namespace")
newRule1 := models.CopyRule(rule1)
newRule2 := models.CopyRule(rule2)
newRule1.Title = strings.ToUpper(rule2.Title)
newRule2.Title = strings.ToUpper(rule1.Title)
err := store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{
Existing: rule1,
New: *newRule1,
}, {
Existing: rule2,
New: *newRule2,
},
})
require.NoError(t, err)
dbrule1 := &alertRule{}
dbrule2 := &alertRule{}
err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error {
exist, err := sess.Table(alertRule{}).ID(rule1.ID).Get(dbrule1)
if err != nil {
return err
}
require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule1.ID))
exist, err = sess.Table(alertRule{}).ID(rule2.ID).Get(dbrule2)
if err != nil {
return err
}
require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule2.ID))
return nil
})
require.NoError(t, err)
require.Equal(t, newRule1.Title, dbrule1.Title)
require.Equal(t, newRule2.Title, dbrule2.Title)
})
t.Run("should handle update multiple chains in different folders without unique constraint violation", func(t *testing.T) {
rule1 := createRuleInFolder("multi-cycle-rule1", 1, "my-namespace")
rule2 := createRuleInFolder("multi-cycle-rule2", 1, "my-namespace")
rule3 := createRuleInFolder("multi-cycle-rule1", 1, "my-namespace2")
rule4 := createRuleInFolder("multi-cycle-rule2", 1, "my-namespace2")
newRule1 := models.CopyRule(rule1)
newRule2 := models.CopyRule(rule2)
newRule3 := models.CopyRule(rule3)
newRule4 := models.CopyRule(rule4)
newRule1.Title = rule2.Title
newRule2.Title = rule1.Title
newRule3.Title = rule4.Title
newRule4.Title = rule3.Title
err := store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{
Existing: rule1,
New: *newRule1,
}, {
Existing: rule2,
New: *newRule2,
}, {
Existing: rule3,
New: *newRule3,
}, {
Existing: rule4,
New: *newRule4,
},
})
require.NoError(t, err)
dbrule1 := &alertRule{}
dbrule2 := &alertRule{}
dbrule3 := &alertRule{}
dbrule4 := &alertRule{}
err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error {
exist, err := sess.Table(alertRule{}).ID(rule1.ID).Get(dbrule1)
if err != nil {
return err
}
require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule1.ID))
exist, err = sess.Table(alertRule{}).ID(rule2.ID).Get(dbrule2)
if err != nil {
return err
}
require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule2.ID))
exist, err = sess.Table(alertRule{}).ID(rule3.ID).Get(dbrule3)
if err != nil {
return err
}
require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule3.ID))
exist, err = sess.Table(alertRule{}).ID(rule4.ID).Get(dbrule4)
if err != nil {
return err
}
require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", rule4.ID))
return nil
})
require.NoError(t, err)
require.Equal(t, newRule1.Title, dbrule1.Title)
require.Equal(t, newRule2.Title, dbrule2.Title)
require.Equal(t, newRule3.Title, dbrule3.Title)
require.Equal(t, newRule4.Title, dbrule4.Title)
})
t.Run("should fail with unique constraint violation", func(t *testing.T) {
rule1 := createRuleInFolder("unique-rule1", 1, "my-namespace")
rule2 := createRuleInFolder("unique-rule2", 1, "my-namespace")
newRule1 := models.CopyRule(rule1)
newRule2 := models.CopyRule(rule2)
newRule2.Title = newRule1.Title
err := store.UpdateAlertRules(context.Background(), &usr, []models.UpdateRule{{
Existing: rule2,
New: *newRule2,
},
})
require.ErrorIs(t, err, models.ErrAlertRuleUniqueConstraintViolation)
require.NotEqual(t, newRule2.UID, "")
require.NotEqual(t, newRule2.Title, "")
require.NotEqual(t, newRule2.NamespaceUID, "")
require.ErrorContains(t, err, newRule2.UID)
require.ErrorContains(t, err, newRule2.Title)
require.ErrorContains(t, err, newRule2.NamespaceUID)
})
}
func TestIntegration_GetAlertRulesForScheduling(t *testing.T) {
if testing.Short() {
t.Skip("skipping integration test")
@@ -1078,18 +827,11 @@ func TestIntegrationInsertAlertRules(t *testing.T) {
_, 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) {
t.Run("should not 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(), &usr, []models.AlertRule{*cp})
require.ErrorIs(t, err, models.ErrAlertRuleConflictBase)
require.ErrorIs(t, err, models.ErrAlertRuleUniqueConstraintViolation)
require.NotEqual(t, rules[0].UID, "")
require.NotEqual(t, rules[0].Title, "")
require.NotEqual(t, rules[0].NamespaceUID, "")
require.ErrorContains(t, err, rules[0].UID)
require.ErrorContains(t, err, rules[0].Title)
require.ErrorContains(t, err, rules[0].NamespaceUID)
require.NoError(t, err)
})
t.Run("should not let insert rules with the same UID", func(t *testing.T) {
cp := models.CopyRule(&rules[0])
pkg/services/sqlstore/migrations/migrations.go
+2
View File
@@ -153,4 +153,6 @@ func (oss *OSSMigrations) AddMigration(mg *Migrator) {
ualert.AddAlertRuleMissingSeriesEvalsToResolve(mg)
accesscontrol.AddDatasourceDrilldownRemovalMigration(mg)
ualert.DropTitleUniqueIndexMigration(mg)
}
pkg/services/sqlstore/migrations/ualert/tables.go
+8 -3
View File
@@ -195,6 +195,8 @@ func alertInstanceMigration(mg *migrator.Migrator) {
}))
}
var titleUniqueIndex = &migrator.Index{Cols: []string{"org_id", "namespace_uid", "title"}, Type: migrator.UniqueIndex}
func addAlertRuleMigrations(mg *migrator.Migrator, defaultIntervalSeconds int64) {
// DO NOT EDIT
alertRule := migrator.Table{
@@ -245,9 +247,7 @@ func addAlertRuleMigrations(mg *migrator.Migrator, defaultIntervalSeconds int64)
Cols: []string{"org_id", "title"}, Type: migrator.UniqueIndex,
}))
mg.AddMigration("add index in alert_rule on org_id, namespase_uid and title columns", migrator.NewAddIndexMigration(alertRule, &migrator.Index{
Cols: []string{"org_id", "namespace_uid", "title"}, Type: migrator.UniqueIndex,
}))
mg.AddMigration("add index in alert_rule on org_id, namespase_uid and title columns", migrator.NewAddIndexMigration(alertRule, titleUniqueIndex))
mg.AddMigration("add dashboard_uid column to alert_rule", migrator.NewAddColumnMigration(
migrator.Table{Name: "alert_rule"},
@@ -571,3 +571,8 @@ func (c extractAlertmanagerConfigurationHistory) Exec(sess *xorm.Session, migrat
}
return nil
}
func DropTitleUniqueIndexMigration(mg *migrator.Migrator) {
mg.AddMigration("remove title in folder unique index",
migrator.NewDropIndexMigration(migrator.Table{Name: "alert_rule"}, titleUniqueIndex))
}