From b1e58eb47eae29085d69478623f7916ac596d587 Mon Sep 17 00:00:00 2001 From: Ryan McKinley Date: Mon, 6 Feb 2023 17:44:37 -0800 Subject: [PATCH] Chore: Replace short UID generation with more standard UUIDs (#62731) --- go.sum | 4 --- pkg/api/dashboard_test.go | 1 - pkg/api/folder_test.go | 2 -- pkg/services/dashboards/database/database.go | 23 +------------ pkg/services/dashboards/errors.go | 21 +++++------- pkg/services/folder/folderimpl/folder.go | 4 --- pkg/services/folder/folderimpl/folder_test.go | 1 - .../shorturls/shorturlimpl/shorturl.go | 9 ++++-- .../sqlstore/migrations/ualert/permissions.go | 26 ++------------- pkg/util/shortid_generator.go | 32 +++++++++++++------ pkg/util/shortid_generator_test.go | 25 +++++++++++++-- 11 files changed, 64 insertions(+), 84 deletions(-) diff --git a/go.sum b/go.sum index bb438a17172..9ae2767b731 100644 --- a/go.sum +++ b/go.sum @@ -1248,8 +1248,6 @@ github.com/gorilla/websocket v1.4.1/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/ad github.com/gorilla/websocket v1.4.2/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/adAjf1fMHhE= github.com/gorilla/websocket v1.5.0 h1:PPwGk2jz7EePpoHN/+ClbZu8SPxiqlu12wZP/3sWmnc= github.com/gorilla/websocket v1.5.0/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/adAjf1fMHhE= -github.com/grafana/alerting v0.0.0-20230125210216-facc6b27b9e0 h1:BzkQNnj+eevX30EMqJiUS1w3CPoGc8kp7pDf/ari/4Y= -github.com/grafana/alerting v0.0.0-20230125210216-facc6b27b9e0/go.mod h1:NoSLbfmUwE+omWFReFrLtbtOItmvTbuQERJ6XFYp9ME= github.com/grafana/alerting v0.0.0-20230203015918-0e4e2675d7aa h1:ue2fctL9LHJWYw9V+R1O/uWLNTfAr/KU1EUFRsqWlK4= github.com/grafana/alerting v0.0.0-20230203015918-0e4e2675d7aa/go.mod h1:NoSLbfmUwE+omWFReFrLtbtOItmvTbuQERJ6XFYp9ME= github.com/grafana/codejen v0.0.3 h1:tAWxoTUuhgmEqxJPOLtJoxlPBbMULFwKFOcRsPRPXDw= @@ -1268,8 +1266,6 @@ github.com/grafana/grafana-google-sdk-go v0.0.0-20211104130251-b190293eaf58 h1:2 github.com/grafana/grafana-google-sdk-go v0.0.0-20211104130251-b190293eaf58/go.mod h1:Vo2TKWfDVmNTELBUM+3lkrZvFtBws0qSZdXhQxRdJrE= github.com/grafana/grafana-plugin-sdk-go v0.94.0/go.mod h1:3VXz4nCv6wH5SfgB3mlW39s+c+LetqSCjFj7xxPC5+M= github.com/grafana/grafana-plugin-sdk-go v0.114.0/go.mod h1:D7x3ah+1d4phNXpbnOaxa/osSaZlwh9/ZUnGGzegRbk= -github.com/grafana/grafana-plugin-sdk-go v0.147.0 h1:VavvJOa/Ubs+wzalzWIl+FQmdaD4vEK8KVYU0a8rf+E= -github.com/grafana/grafana-plugin-sdk-go v0.147.0/go.mod h1:NMgO3t2gR5wyLx8bWZ9CTmpDk5Txp4wYFccFLHdYn3Q= github.com/grafana/grafana-plugin-sdk-go v0.148.0 h1:M8v6L9agAFMlZMnak1yInII+aVF5FjZ1Qv4Q+GANyk4= github.com/grafana/grafana-plugin-sdk-go v0.148.0/go.mod h1:NMgO3t2gR5wyLx8bWZ9CTmpDk5Txp4wYFccFLHdYn3Q= github.com/grafana/phlare/api v0.1.2 h1:1jrwd3KnsXMzj/tJih9likx5EvbY3pbvLbDqAAYem30= diff --git a/pkg/api/dashboard_test.go b/pkg/api/dashboard_test.go index b16759f4144..4ce9ccdf906 100644 --- a/pkg/api/dashboard_test.go +++ b/pkg/api/dashboard_test.go @@ -682,7 +682,6 @@ func TestDashboardAPIEndpoint(t *testing.T) { {SaveError: dashboards.ErrDashboardTitleEmpty, ExpectedStatusCode: 400}, {SaveError: dashboards.ErrDashboardFolderCannotHaveParent, ExpectedStatusCode: 400}, {SaveError: alerting.ValidationError{Reason: "Mu"}, ExpectedStatusCode: 422}, - {SaveError: dashboards.ErrDashboardFailedGenerateUniqueUid, ExpectedStatusCode: 500}, {SaveError: dashboards.ErrDashboardTypeMismatch, ExpectedStatusCode: 400}, {SaveError: dashboards.ErrDashboardFolderWithSameNameAsDashboard, ExpectedStatusCode: 400}, {SaveError: dashboards.ErrDashboardWithSameNameAsFolder, ExpectedStatusCode: 400}, diff --git a/pkg/api/folder_test.go b/pkg/api/folder_test.go index f5a56a1f74b..d9c8c0127bb 100644 --- a/pkg/api/folder_test.go +++ b/pkg/api/folder_test.go @@ -71,7 +71,6 @@ func TestFoldersAPIEndpoint(t *testing.T) { {Error: dashboards.ErrFolderAccessDenied, ExpectedStatusCode: 403}, {Error: dashboards.ErrFolderNotFound, ExpectedStatusCode: 404}, {Error: dashboards.ErrFolderVersionMismatch, ExpectedStatusCode: 412}, - {Error: dashboards.ErrFolderFailedGenerateUniqueUid, ExpectedStatusCode: 500}, } cmd := folder.CreateFolderCommand{ @@ -124,7 +123,6 @@ func TestFoldersAPIEndpoint(t *testing.T) { {Error: dashboards.ErrFolderAccessDenied, ExpectedStatusCode: 403}, {Error: dashboards.ErrFolderNotFound, ExpectedStatusCode: 404}, {Error: dashboards.ErrFolderVersionMismatch, ExpectedStatusCode: 412}, - {Error: dashboards.ErrFolderFailedGenerateUniqueUid, ExpectedStatusCode: 500}, } title := "Folder upd" diff --git a/pkg/services/dashboards/database/database.go b/pkg/services/dashboards/database/database.go index 1102e8c091f..bdacac546d5 100644 --- a/pkg/services/dashboards/database/database.go +++ b/pkg/services/dashboards/database/database.go @@ -453,11 +453,7 @@ func saveDashboard(sess *db.Session, cmd *dashboards.SaveDashboardCommand, emitE } if dash.UID == "" { - uid, err := generateNewDashboardUid(sess, dash.OrgID) - if err != nil { - return nil, err - } - dash.SetUID(uid) + dash.SetUID(util.GenerateShortUID()) } parentVersion := dash.Version @@ -536,23 +532,6 @@ func saveDashboard(sess *db.Session, cmd *dashboards.SaveDashboardCommand, emitE return dash, nil } -func generateNewDashboardUid(sess *db.Session, orgId int64) (string, error) { - for i := 0; i < 3; i++ { - uid := util.GenerateShortUID() - - exists, err := sess.Where("org_id=? AND uid=?", orgId, uid).Get(&dashboards.Dashboard{}) - if err != nil { - return "", err - } - - if !exists { - return uid, nil - } - } - - return "", dashboards.ErrDashboardFailedGenerateUniqueUid -} - func saveProvisionedData(sess *db.Session, provisioning *dashboards.DashboardProvisioning, dashboard *dashboards.Dashboard) error { result := &dashboards.DashboardProvisioning{} diff --git a/pkg/services/dashboards/errors.go b/pkg/services/dashboards/errors.go index a7ec2f8a0cc..7adb67e2e85 100644 --- a/pkg/services/dashboards/errors.go +++ b/pkg/services/dashboards/errors.go @@ -54,10 +54,6 @@ var ( Reason: "Multiple dashboards with the same slug exists", StatusCode: 412, } - ErrDashboardFailedGenerateUniqueUid = DashboardErr{ - Reason: "Failed to generate unique dashboard id", - StatusCode: 500, - } ErrDashboardTypeMismatch = DashboardErr{ Reason: "Dashboard cannot be changed to a folder", StatusCode: 400, @@ -126,15 +122,14 @@ var ( Status: "not-found", } - ErrFolderNotFound = errors.New("folder not found") - ErrFolderVersionMismatch = errors.New("the folder has been changed by someone else") - ErrFolderTitleEmpty = errors.New("folder title cannot be empty") - ErrFolderWithSameUIDExists = errors.New("a folder/dashboard with the same uid already exists") - ErrFolderInvalidUID = errors.New("invalid uid for folder provided") - ErrFolderSameNameExists = errors.New("a folder or dashboard in the general folder with the same name already exists") - ErrFolderFailedGenerateUniqueUid = errors.New("failed to generate unique folder ID") - ErrFolderAccessDenied = errors.New("access denied to folder") - ErrFolderContainsAlertRules = errors.New("folder contains alert rules") + ErrFolderNotFound = errors.New("folder not found") + ErrFolderVersionMismatch = errors.New("the folder has been changed by someone else") + ErrFolderTitleEmpty = errors.New("folder title cannot be empty") + ErrFolderWithSameUIDExists = errors.New("a folder/dashboard with the same uid already exists") + ErrFolderInvalidUID = errors.New("invalid uid for folder provided") + ErrFolderSameNameExists = errors.New("a folder or dashboard in the general folder with the same name already exists") + ErrFolderAccessDenied = errors.New("access denied to folder") + ErrFolderContainsAlertRules = errors.New("folder contains alert rules") ) // DashboardErr represents a dashboard error. diff --git a/pkg/services/folder/folderimpl/folder.go b/pkg/services/folder/folderimpl/folder.go index a61e9efde88..3d4ea736581 100644 --- a/pkg/services/folder/folderimpl/folder.go +++ b/pkg/services/folder/folderimpl/folder.go @@ -805,9 +805,5 @@ func toFolderError(err error) error { return dashboards.ErrFolderNotFound } - if errors.Is(err, dashboards.ErrDashboardFailedGenerateUniqueUid) { - err = dashboards.ErrFolderFailedGenerateUniqueUid - } - return err } diff --git a/pkg/services/folder/folderimpl/folder_test.go b/pkg/services/folder/folderimpl/folder_test.go index b7f02a8f828..1bfea0b8e92 100644 --- a/pkg/services/folder/folderimpl/folder_test.go +++ b/pkg/services/folder/folderimpl/folder_test.go @@ -300,7 +300,6 @@ func TestIntegrationFolderService(t *testing.T) { {ActualError: dashboards.ErrDashboardWithSameUIDExists, ExpectedError: dashboards.ErrFolderWithSameUIDExists}, {ActualError: dashboards.ErrDashboardVersionMismatch, ExpectedError: dashboards.ErrFolderVersionMismatch}, {ActualError: dashboards.ErrDashboardNotFound, ExpectedError: dashboards.ErrFolderNotFound}, - {ActualError: dashboards.ErrDashboardFailedGenerateUniqueUid, ExpectedError: dashboards.ErrFolderFailedGenerateUniqueUid}, {ActualError: dashboards.ErrDashboardInvalidUid, ExpectedError: dashboards.ErrDashboardInvalidUid}, } diff --git a/pkg/services/shorturls/shorturlimpl/shorturl.go b/pkg/services/shorturls/shorturlimpl/shorturl.go index 95e295233c4..5269b9683e5 100644 --- a/pkg/services/shorturls/shorturlimpl/shorturl.go +++ b/pkg/services/shorturls/shorturlimpl/shorturl.go @@ -9,7 +9,7 @@ import ( "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/services/shorturls" "github.com/grafana/grafana/pkg/services/user" - "github.com/grafana/grafana/pkg/util" + "github.com/teris-io/shortid" ) var getTime = time.Now @@ -44,10 +44,15 @@ func (s ShortURLService) CreateShortURL(ctx context.Context, user *user.SignedIn return nil, shorturls.ErrShortURLInvalidPath.Errorf("path cannot contain '../': %s", relPath) } + uid, err := shortid.Generate() + if err != nil { + return nil, shorturls.ErrShortURLInternal.Errorf("failed to generate uid: %w", err) + } + now := time.Now().Unix() shortURL := shorturls.ShortUrl{ OrgId: user.OrgID, - Uid: util.GenerateShortUID(), + Uid: uid, Path: relPath, CreatedBy: user.UserID, CreatedAt: now, diff --git a/pkg/services/sqlstore/migrations/ualert/permissions.go b/pkg/services/sqlstore/migrations/ualert/permissions.go index 271a8b2f5fc..7d4789ae975 100644 --- a/pkg/services/sqlstore/migrations/ualert/permissions.go +++ b/pkg/services/sqlstore/migrations/ualert/permissions.go @@ -101,12 +101,7 @@ func (m *folderHelper) createFolder(orgID int64, title string) (*dashboard, erro }), } dash := cmd.getDashboardModel() - - uid, err := m.generateNewDashboardUid(dash.OrgId) - if err != nil { - return nil, err - } - dash.setUid(uid) + dash.setUid(util.GenerateShortUID()) parentVersion := dash.Version dash.setVersion(1) @@ -116,7 +111,7 @@ func (m *folderHelper) createFolder(orgID int64, title string) (*dashboard, erro dash.UpdatedBy = FOLDER_CREATED_BY metrics.MApiDashboardInsert.Inc() - if _, err = m.sess.Insert(dash); err != nil { + if _, err := m.sess.Insert(dash); err != nil { return nil, err } @@ -138,23 +133,6 @@ func (m *folderHelper) createFolder(orgID int64, title string) (*dashboard, erro return dash, nil } -func (m *folderHelper) generateNewDashboardUid(orgId int64) (string, error) { - for i := 0; i < 3; i++ { - uid := util.GenerateShortUID() - - exists, err := m.sess.Where("org_id=? AND uid=?", orgId, uid).Get(&dashboards.Dashboard{}) - if err != nil { - return "", err - } - - if !exists { - return uid, nil - } - } - - return "", dashboards.ErrDashboardFailedGenerateUniqueUid -} - // based on SQLStore.UpdateDashboardACL() // it should be called from inside a transaction func (m *folderHelper) setACL(orgID int64, dashboardID int64, items []*dashboardACL) error { diff --git a/pkg/util/shortid_generator.go b/pkg/util/shortid_generator.go index 767ea1a9f3e..ba4ad4d9745 100644 --- a/pkg/util/shortid_generator.go +++ b/pkg/util/shortid_generator.go @@ -1,21 +1,22 @@ package util import ( + "math/rand" "regexp" + "time" - "github.com/teris-io/shortid" + "github.com/google/uuid" ) -var allowedChars = shortid.DefaultABC +var uidrand = rand.New(rand.NewSource(time.Now().UnixNano())) +var alphaRunes = []rune("abcdefghijklmnopqrstuvwxyz") +var hexLetters = []rune("abcdef") +// Legacy UID pattern var validUIDPattern = regexp.MustCompile(`^[a-zA-Z0-9\-\_]*$`).MatchString -func init() { - gen, _ := shortid.New(1, allowedChars, 1) - shortid.SetDefault(gen) -} - // IsValidShortUID checks if short unique identifier contains valid characters +// NOTE: future Grafana UIDs will need conform to https://github.com/kubernetes/apimachinery/blob/master/pkg/util/validation/validation.go#L43 func IsValidShortUID(uid string) bool { return validUIDPattern(uid) } @@ -25,7 +26,20 @@ func IsShortUIDTooLong(uid string) bool { return len(uid) > 40 } -// GenerateShortUID generates a short unique identifier. +// GenerateShortUID will generate a UUID that can also be a k8s name +// it is guaranteed to have a character as the first letter +// This UID will be a valid k8s name func GenerateShortUID() string { - return shortid.MustGenerate() + uid, err := uuid.NewRandom() + if err != nil { + // This should never happen... but this seems better than a panic + for i := range uid { + uid[i] = byte(uidrand.Intn(255)) + } + } + uuid := uid.String() + if rune(uuid[0]) < rune('a') { + return string(hexLetters[uidrand.Intn(len(hexLetters))]) + uuid[1:] + } + return uuid } diff --git a/pkg/util/shortid_generator_test.go b/pkg/util/shortid_generator_test.go index 2e0fc0cc6d7..b582520a825 100644 --- a/pkg/util/shortid_generator_test.go +++ b/pkg/util/shortid_generator_test.go @@ -3,17 +3,38 @@ package util import ( "testing" + "github.com/google/uuid" "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/util/validation" ) func TestAllowedCharMatchesUidPattern(t *testing.T) { - for _, c := range allowedChars { + for _, c := range alphaRunes { if !IsValidShortUID(string(c)) { t.Fatalf("charset for creating new shortids contains chars not present in uid pattern") } } } +func TestRandomUIDs(t *testing.T) { + for i := 0; i < 100; i++ { + v := GenerateShortUID() + if !IsValidShortUID(v) { + t.Fatalf("charset for creating new shortids contains chars not present in uid pattern") + } + validation := validation.IsQualifiedName(v) + if validation != nil { + t.Fatalf("created invalid name: %v", validation) + } + + _, err := uuid.Parse(v) + require.NoError(t, err) + + //fmt.Println(v) + } + // t.FailNow() +} + func TestIsShortUIDTooLong(t *testing.T) { var tests = []struct { name string @@ -22,7 +43,7 @@ func TestIsShortUIDTooLong(t *testing.T) { }{ { name: "when the length of uid is longer than 40 chars then IsShortUIDTooLong should return true", - uid: allowedChars, + uid: string(alphaRunes) + string(alphaRunes), expected: true, }, {