From 0fa2dbdcdcff70eef9768139ad71ede16beb5e1d Mon Sep 17 00:00:00 2001 From: Alexander Akhmetov Date: Fri, 7 Mar 2025 12:09:39 +0100 Subject: [PATCH] Alerting: Generate deterministic UID for folders created for Prometheus rule conversion (#101760) --- pkg/services/ngalert/store/namespace.go | 59 ++++++++- pkg/services/ngalert/store/namespace_test.go | 123 +++++++++++++++++++ 2 files changed, 181 insertions(+), 1 deletion(-) diff --git a/pkg/services/ngalert/store/namespace.go b/pkg/services/ngalert/store/namespace.go index 2e00ed40371..63100cb7392 100644 --- a/pkg/services/ngalert/store/namespace.go +++ b/pkg/services/ngalert/store/namespace.go @@ -3,7 +3,10 @@ package store import ( "context" "errors" + "fmt" + "hash/fnv" "sort" + "strconv" "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/services/dashboards" @@ -89,7 +92,14 @@ func (st DBstore) GetNamespaceByTitle(ctx context.Context, title string, orgID i } // GetOrCreateNamespaceByTitle gets or creates a namespace by title in the specified folder. +// +// To avoid race conditions when two concurrent requests try to create the same folder, +// we create folders with a deterministic UID based on the parent UID, title, and organization ID. func (st DBstore) GetOrCreateNamespaceByTitle(ctx context.Context, title string, orgID int64, user identity.Requester, parentUID string) (*folder.Folder, error) { + if len(title) == 0 { + return nil, fmt.Errorf("title is empty") + } + var f *folder.Folder var err error @@ -99,7 +109,14 @@ func (st DBstore) GetOrCreateNamespaceByTitle(ctx context.Context, title string, } if f == nil { + // Generate a deterministic UID with an alerting prefix + uid, err := generateAlertingFolderUID(title, parentUID, orgID) + if err != nil { + return nil, fmt.Errorf("error creating a new folder: %w", err) + } + cmd := &folder.CreateFolderCommand{ + UID: uid, OrgID: orgID, Title: title, SignedInUser: user, @@ -107,9 +124,49 @@ func (st DBstore) GetOrCreateNamespaceByTitle(ctx context.Context, title string, } f, err = st.FolderService.Create(ctx, cmd) if err != nil { - return nil, err + // Handle potential race condition where another request might have created + // the folder between our check and creation attempt + existingFolder, lookupErr := st.GetNamespaceByTitle(ctx, title, orgID, user, parentUID) + if lookupErr == nil { + return existingFolder, nil + } + + // If we couldn't find it, return errors + return nil, fmt.Errorf("failed to get or create folder: %w", errors.Join( + fmt.Errorf("create folder: %w", err), + fmt.Errorf("lookup folder: %w", lookupErr), + )) } } return f, nil } + +// generateAlertingFolderUID creates a deterministic UID for folders +// based on the title and parent UID to avoid race conditions when multiple +// identical folders are created concurrently +func generateAlertingFolderUID(title string, parentUID string, orgID int64) (string, error) { + h := fnv.New64a() + + hashData := [][]byte{ + []byte(parentUID), + {0}, // separator + []byte(title), + {0}, + []byte(strconv.FormatInt(orgID, 10)), + } + + // Add hashData strings to the hash with a separator between them + for _, data := range hashData { + _, err := h.Write(data) + if err != nil { + return "", err + } + } + + // Create a deterministic string with alerting prefix + base36 := strconv.FormatUint(h.Sum64(), 36) + uid := fmt.Sprintf("alerting-%s", base36) + + return uid, nil +} diff --git a/pkg/services/ngalert/store/namespace_test.go b/pkg/services/ngalert/store/namespace_test.go index 7693122cee6..6cc1c610dba 100644 --- a/pkg/services/ngalert/store/namespace_test.go +++ b/pkg/services/ngalert/store/namespace_test.go @@ -2,6 +2,7 @@ package store import ( "context" + "strings" "testing" "github.com/google/uuid" @@ -206,6 +207,13 @@ func TestIntegration_GetOrCreateNamespaceByTitle(t *testing.T) { return store } + t.Run("should return error when title is empty", func(t *testing.T) { + store := setupStore(t) + _, err := store.GetOrCreateNamespaceByTitle(context.Background(), "", 1, u, folder.RootFolderUID) + require.Error(t, err) + require.Contains(t, err.Error(), "title is empty") + }) + t.Run("should create folder when it does not exist", func(t *testing.T) { store := setupStore(t) @@ -317,6 +325,65 @@ func TestIntegration_GetOrCreateNamespaceByTitle(t *testing.T) { require.Equal(t, folder3.UID, gotFolder3.UID) require.Equal(t, folder.RootFolderUID, gotFolder3.ParentUID) }) + + t.Run("should create folder with deterministic UID and handle race conditions", func(t *testing.T) { + store := setupStore(t) + + folderTitle := "race condition test folder" + parentUID := folder.RootFolderUID + + // Calculate the expected UID that would be generated + expectedUID, err := generateAlertingFolderUID(folderTitle, parentUID, 1) + require.NoError(t, err) + + // Create a folder first, simulating another concurrent call that succeeded first + createFolder(t, store, expectedUID, folderTitle, 1, parentUID) + + // Now try to create a folder with the same title and parent + // This should not create a duplicate folder but return the existing one + f, err := store.GetOrCreateNamespaceByTitle(context.Background(), folderTitle, 1, u, parentUID) + require.NoError(t, err) + require.Equal(t, expectedUID, f.UID, "Should return folder with same UID as would be deterministically generated") + require.Equal(t, folderTitle, f.Title) + require.Equal(t, parentUID, f.ParentUID) + + // Verify only one folder was created + folders, err := store.FolderService.GetFolders( + context.Background(), + folder.GetFoldersQuery{ + OrgID: 1, + WithFullpath: true, + SignedInUser: u, + }, + ) + require.NoError(t, err) + require.Len(t, folders, 1, "Only one folder should exist") + }) + + t.Run("should handle special characters in folder titles", func(t *testing.T) { + store := setupStore(t) + specialTitles := []string{ + "folder/with/slashes", + "folder with spaces", + "folder-with-dashes", + "folder_with_underscores", + "folder.with.dots", + "!@#$%^&*()", + } + + for _, title := range specialTitles { + t.Run(title, func(t *testing.T) { + f, err := store.GetOrCreateNamespaceByTitle(context.Background(), title, 1, u, folder.RootFolderUID) + require.NoError(t, err) + require.Equal(t, title, f.Title) + + // Verify retrieval works + retrieved, err := store.GetNamespaceByTitle(context.Background(), title, 1, u, folder.RootFolderUID) + require.NoError(t, err) + require.Equal(t, f.UID, retrieved.UID) + }) + } + }) } func TestIntegration_GetNamespaceChildren(t *testing.T) { @@ -417,3 +484,59 @@ func TestIntegration_GetNamespaceChildren(t *testing.T) { require.ElementsMatch(t, []string{rootFolder1, rootFolder2}, []string{children[0].UID, children[1].UID}) }) } + +func TestGenerateAlertingFolderUID(t *testing.T) { + const orgID int64 = 1 + + t.Run("should generate deterministic UIDs for same inputs", func(t *testing.T) { + title := "Test Folder" + parentUID := "parent123" + + uid1, err := generateAlertingFolderUID(title, parentUID, orgID) + require.NoError(t, err) + require.True(t, util.IsValidShortUID(uid1), "Generated UID should be valid") + + uid2, err := generateAlertingFolderUID(title, parentUID, orgID) + require.NoError(t, err) + require.True(t, util.IsValidShortUID(uid2), "Generated UID should be valid") + + require.Equal(t, uid1, uid2, "UIDs should be identical for same inputs") + require.True(t, strings.HasPrefix(uid1, "alerting-"), "UID should have alerting prefix") + }) + + t.Run("should generate different UIDs for different inputs", func(t *testing.T) { + uid1, err := generateAlertingFolderUID("Folder1", "parent1", orgID) + require.NoError(t, err) + require.True(t, util.IsValidShortUID(uid1), "Generated UID should be valid") + + uid2, err := generateAlertingFolderUID("Folder2", "parent1", orgID) + require.NoError(t, err) + require.True(t, util.IsValidShortUID(uid2), "Generated UID should be valid") + + uid3, err := generateAlertingFolderUID("Folder1", "parent2", orgID) + require.NoError(t, err) + require.True(t, util.IsValidShortUID(uid3), "Generated UID should be valid") + + require.NotEqual(t, uid1, uid2, "UIDs should differ for different titles") + require.NotEqual(t, uid1, uid3, "UIDs should differ for different parent UIDs") + }) + + t.Run("should handle special characters in folder titles", func(t *testing.T) { + specialTitles := []string{ + "folder/with/slashes", + "folder with spaces", + "folder-with-dashes", + "folder_with_underscores", + "folder.with.dots", + "!@#$%^&*()", + } + + for _, title := range specialTitles { + t.Run(title, func(t *testing.T) { + uid, err := generateAlertingFolderUID(title, "parent123", orgID) + require.NoError(t, err) + require.True(t, util.IsValidShortUID(uid), "Generated UID should be valid") + }) + } + }) +}