diff --git a/pkg/services/sqlstore/migrations/ualert/alert_rule.go b/pkg/services/sqlstore/migrations/ualert/alert_rule.go index a5647bee4e2..cbcc0f2118d 100644 --- a/pkg/services/sqlstore/migrations/ualert/alert_rule.go +++ b/pkg/services/sqlstore/migrations/ualert/alert_rule.go @@ -10,7 +10,6 @@ import ( legacymodels "github.com/grafana/grafana/pkg/models" ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/tsdb/graphite" - "github.com/grafana/grafana/pkg/util" ) const ( @@ -106,7 +105,6 @@ func addMigrationInfo(da *dashAlert) (map[string]string, map[string]string) { func (m *migration) makeAlertRule(cond condition, da dashAlert, folderUID string) (*alertRule, error) { lbls, annotations := addMigrationInfo(&da) - name := normalizeRuleName(da.Name) annotations["message"] = da.Message var err error @@ -115,10 +113,17 @@ func (m *migration) makeAlertRule(cond condition, da dashAlert, folderUID string return nil, fmt.Errorf("failed to migrate alert rule queries: %w", err) } + uid, err := m.seenUIDs.generateUid() + if err != nil { + return nil, fmt.Errorf("failed to migrate alert rule: %w", err) + } + + name := normalizeRuleName(da.Name, uid) + ar := &alertRule{ OrgID: da.OrgId, Title: name, // TODO: Make sure all names are unique, make new name on constraint insert error. - UID: util.GenerateShortUID(), + UID: uid, Condition: cond.Condition, Data: data, IntervalSeconds: ruleAdjustInterval(da.Frequency), @@ -286,13 +291,12 @@ func transExecErr(s string) (string, error) { return "", fmt.Errorf("unrecognized Execution Error setting %v", s) } -func normalizeRuleName(daName string) string { +func normalizeRuleName(daName string, uid string) string { // If we have to truncate, we're losing data and so there is higher risk of uniqueness conflicts. - // Append a UID to the suffix to forcibly break any collisions. + // Append the UID to the suffix to forcibly break any collisions. if len(daName) > DefaultFieldMaxLength { - uniq := util.GenerateShortUID() - trunc := DefaultFieldMaxLength - 1 - len(uniq) - daName = daName[:trunc] + "_" + uniq + trunc := DefaultFieldMaxLength - 1 - len(uid) + daName = daName[:trunc] + "_" + uid } return daName diff --git a/pkg/services/sqlstore/migrations/ualert/channel.go b/pkg/services/sqlstore/migrations/ualert/channel.go index 01981b47a5d..3e4d181e4b7 100644 --- a/pkg/services/sqlstore/migrations/ualert/channel.go +++ b/pkg/services/sqlstore/migrations/ualert/channel.go @@ -4,7 +4,6 @@ import ( "crypto/md5" "encoding/base64" "encoding/json" - "errors" "fmt" "regexp" "sort" @@ -15,7 +14,6 @@ import ( "github.com/grafana/grafana/pkg/components/simplejson" ngModels "github.com/grafana/grafana/pkg/services/ngalert/models" - "github.com/grafana/grafana/pkg/util" ) type notificationChannel struct { @@ -177,22 +175,9 @@ func (m *migration) getNotificationChannelMap() (channelsPerOrg, defaultChannels // Create a notifier (PostableGrafanaReceiver) from a legacy notification channel func (m *migration) createNotifier(c *notificationChannel) (*PostableGrafanaReceiver, error) { - uid := c.Uid - if uid == "" { - new, err := m.generateChannelUID() - if err != nil { - return nil, err - } - m.mg.Logger.Info("Legacy notification had an empty uid, generating a new one", "id", c.ID, "uid", new) - uid = new - } - if _, seen := m.seenChannelUIDs[uid]; seen { - new, err := m.generateChannelUID() - if err != nil { - return nil, err - } - m.mg.Logger.Warn("Legacy notification had a UID that collides with a migrated record, generating a new one", "id", c.ID, "old", uid, "new", new) - uid = new + uid, err := m.determineChannelUid(c) + if err != nil { + return nil, err } settings, secureSettings, err := migrateSettingsToSecureSettings(c.Type, c.Settings, c.SecureSettings) @@ -350,16 +335,27 @@ func (m *migration) filterReceiversForAlert(name string, channelIDs []uidOrID, r return filteredReceiverNames } -func (m *migration) generateChannelUID() (string, error) { - for i := 0; i < 5; i++ { - gen := util.GenerateShortUID() - if _, ok := m.seenChannelUIDs[gen]; !ok { - m.seenChannelUIDs[gen] = struct{}{} - return gen, nil +func (m *migration) determineChannelUid(c *notificationChannel) (string, error) { + legacyUid := c.Uid + if legacyUid == "" { + newUid, err := m.seenUIDs.generateUid() + if err != nil { + return "", err } + m.mg.Logger.Info("Legacy notification had an empty uid, generating a new one", "id", c.ID, "uid", newUid) + return newUid, nil } - return "", errors.New("failed to generate UID for notification channel") + if m.seenUIDs.contains(legacyUid) { + newUid, err := m.seenUIDs.generateUid() + if err != nil { + return "", err + } + m.mg.Logger.Warn("Legacy notification had a UID that collides with a migrated record, generating a new one", "id", c.ID, "old", legacyUid, "new", newUid) + return newUid, nil + } + + return legacyUid, nil } // Some settings were migrated from settings to secure settings in between. diff --git a/pkg/services/sqlstore/migrations/ualert/testing.go b/pkg/services/sqlstore/migrations/ualert/testing.go index d238a7ce4f5..229b782458d 100644 --- a/pkg/services/sqlstore/migrations/ualert/testing.go +++ b/pkg/services/sqlstore/migrations/ualert/testing.go @@ -16,6 +16,8 @@ func newTestMigration(t *testing.T) *migration { Logger: log.New("test"), }, - seenChannelUIDs: make(map[string]struct{}), + seenUIDs: uidSet{ + set: make(map[string]struct{}), + }, } } diff --git a/pkg/services/sqlstore/migrations/ualert/ualert.go b/pkg/services/sqlstore/migrations/ualert/ualert.go index f8c60169737..29765e1ccd1 100644 --- a/pkg/services/sqlstore/migrations/ualert/ualert.go +++ b/pkg/services/sqlstore/migrations/ualert/ualert.go @@ -4,6 +4,7 @@ import ( "context" "encoding/base64" "encoding/json" + "errors" "fmt" "os" "path/filepath" @@ -74,8 +75,9 @@ func AddDashAlertMigration(mg *migrator.Migrator) { mg.Logger.Error("alert migration error: could not clear alert migration for removing data", "error", err) } mg.AddMigration(migTitle, &migration{ - seenChannelUIDs: make(map[string]struct{}), - silences: make(map[int64][]*pb.MeshSilence), + // We deduplicate for case-insensitive matching in MySQL-compatible backend flavours because they use case-insensitive collation. + seenUIDs: uidSet{set: make(map[string]struct{}), caseInsensitive: mg.Dialect.SupportEngine()}, + silences: make(map[int64][]*pb.MeshSilence), }) // If unified alerting is disabled and upgrade migration has been run case !mg.Cfg.UnifiedAlerting.IsEnabled() && migrationRun: @@ -226,8 +228,8 @@ type migration struct { sess *xorm.Session mg *migrator.Migrator - seenChannelUIDs map[string]struct{} - silences map[int64][]*pb.MeshSilence + seenUIDs uidSet + silences map[int64][]*pb.MeshSilence } func (m *migration) SQL(dialect migrator.Dialect) string { @@ -883,3 +885,45 @@ func (c updateRulesOrderInGroup) Exec(sess *xorm.Session, migrator *migrator.Mig } return nil } + +// uidSet is a wrapper around map[string]struct{} and util.GenerateShortUID() which aims help generate uids in quick +// succession while taking into consideration case sensitivity requirements. if caseInsensitive is true, all generated +// uids must also be unique when compared in a case-insensitive manner. +type uidSet struct { + set map[string]struct{} + caseInsensitive bool +} + +// contains checks whether the given uid has already been generated in this uidSet. +func (s *uidSet) contains(uid string) bool { + dedup := uid + if s.caseInsensitive { + dedup = strings.ToLower(dedup) + } + _, seen := s.set[dedup] + return seen +} + +// add adds the given uid to the uidSet. +func (s *uidSet) add(uid string) { + dedup := uid + if s.caseInsensitive { + dedup = strings.ToLower(dedup) + } + s.set[dedup] = struct{}{} +} + +// generateUid will generate a new unique uid that is not already contained in the uidSet. +// If it fails to create one that has not already been generated it will make multiple, but not unlimited, attempts. +// If all attempts are exhausted an error will be returned. +func (s *uidSet) generateUid() (string, error) { + for i := 0; i < 5; i++ { + gen := util.GenerateShortUID() + if !s.contains(gen) { + s.add(gen) + return gen, nil + } + } + + return "", errors.New("failed to generate UID") +} diff --git a/pkg/services/sqlstore/migrations/ualert/ualert_test.go b/pkg/services/sqlstore/migrations/ualert/ualert_test.go index 09b767ce075..1f75906b74b 100644 --- a/pkg/services/sqlstore/migrations/ualert/ualert_test.go +++ b/pkg/services/sqlstore/migrations/ualert/ualert_test.go @@ -8,10 +8,11 @@ import ( "strings" "testing" - "github.com/grafana/grafana/pkg/components/simplejson" - "github.com/grafana/grafana/pkg/util" "github.com/prometheus/alertmanager/pkg/labels" "github.com/stretchr/testify/require" + + "github.com/grafana/grafana/pkg/components/simplejson" + "github.com/grafana/grafana/pkg/util" ) var MigTitle = migTitle @@ -172,3 +173,23 @@ func Test_getAlertFolderNameFromDashboard(t *testing.T) { require.Contains(t, folder, dash.Uid) }) } + +func Test_shortUIDCaseInsensitiveConflicts(t *testing.T) { + s := uidSet{ + set: make(map[string]struct{}), + caseInsensitive: true, + } + + // 10000 uids seems to be enough to cause a collision in almost every run if using util.GenerateShortUID directly. + for i := 0; i < 10000; i++ { + _, _ = s.generateUid() + } + + // check if any are case-insensitive duplicates. + deduped := make(map[string]struct{}) + for k := range s.set { + deduped[strings.ToLower(k)] = struct{}{} + } + + require.Equal(t, len(s.set), len(deduped)) +}