Alerting: Prevent uid collision in migration when db is case-insensitive (#60494)

* Alerting: Prevent short uid collision in legacy migration when db is case-insensitive

Two factors come into play that cause sporadic uid conflicts during legacy alert migration:
- MySQL and MySQL-compatible backends use case-insensitive collation.
- Our short uid generator is not a uniform RNG and generates uids in such a way that generations in quick succession have a higher probability of creating similar uids.

Normally we would be guaranteed unique short uid generation, however if the source alphabet contains
duplicate characters (for example, if we use case-insensitive comparison) this guarantee is void.

Generating even ~1000 uids in quick succession is nearly guaranteed to create a case-insensitive
duplicate.

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(uid)
-		trunc := DefaultFieldMaxLength - 1 - len(uniq)
+		daName = daName[:trunc] + "_" + uid
-		daName = daName[:trunc] + "_" + uniq
 	}
 
 	return daName

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
+	uid, err := m.determineChannelUid(c)
-	if uid == "" {
+	if err != nil {
-		new, err := m.generateChannelUID()
+		return nil, err
-		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
 	}
 
 	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) {
+func (m *migration) determineChannelUid(c *notificationChannel) (string, error) {
-	for i := 0; i < 5; i++ {
+	legacyUid := c.Uid
-		gen := util.GenerateShortUID()
+	if legacyUid == "" {
-		if _, ok := m.seenChannelUIDs[gen]; !ok {
+		newUid, err := m.seenUIDs.generateUid()
-			m.seenChannelUIDs[gen] = struct{}{}
+		if err != nil {
-			return gen, 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.

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{}),
+		},
 	}
 }

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{}),
+			// We deduplicate for case-insensitive matching in MySQL-compatible backend flavours because they use case-insensitive collation.
-			silences:        make(map[int64][]*pb.MeshSilence),
+			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{}
+	seenUIDs uidSet
-	silences        map[int64][]*pb.MeshSilence
+	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")
+}

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))
+}