From 7593fc0a209fa6399ef6bbc116baf62c0507970a Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Wed, 26 Jan 2022 11:33:31 +0100 Subject: [PATCH] Provisioning: Fix duplicate validation when multiple organizations have been configured (#44151) Fix duplicate validation when multiple organizations have been configured. This makes sure that all duplicate validation is being made for each organization. Fixes #44126 --- .../provisioning/dashboards/validator.go | 78 ++++++++----- .../provisioning/dashboards/validator_test.go | 109 +++++++++++++++--- 2 files changed, 142 insertions(+), 45 deletions(-) diff --git a/pkg/services/provisioning/dashboards/validator.go b/pkg/services/provisioning/dashboards/validator.go index e5402b28382..bfd39d4e7ae 100644 --- a/pkg/services/provisioning/dashboards/validator.go +++ b/pkg/services/provisioning/dashboards/validator.go @@ -56,61 +56,75 @@ func newDuplicateValidator(logger log.Logger, readers []*FileReader) duplicateVa return duplicateValidator{logger: logger, readers: readers} } -func (c *duplicateValidator) getDuplicates() *duplicateEntries { - duplicates := duplicateEntries{ - Titles: make(map[dashboardIdentity]*duplicate), - UIDs: make(map[string]*duplicate), - } +func (c *duplicateValidator) getDuplicates() map[int64]duplicateEntries { + duplicatesByOrg := map[int64]duplicateEntries{} for _, reader := range c.readers { readerName := reader.Cfg.Name + orgID := reader.Cfg.OrgID tracker := reader.getUsageTracker() - for uid, times := range tracker.uidUsage { - if _, ok := duplicates.UIDs[uid]; !ok { - duplicates.UIDs[uid] = newDuplicate() + if _, exists := duplicatesByOrg[orgID]; !exists { + duplicatesByOrg[orgID] = duplicateEntries{ + Titles: make(map[dashboardIdentity]*duplicate), + UIDs: make(map[string]*duplicate), } - duplicates.UIDs[uid].Sum += times - duplicates.UIDs[uid].InvolvedReaders[readerName] = struct{}{} + } + + for uid, times := range tracker.uidUsage { + if _, ok := duplicatesByOrg[orgID].UIDs[uid]; !ok { + duplicatesByOrg[orgID].UIDs[uid] = newDuplicate() + } + duplicatesByOrg[orgID].UIDs[uid].Sum += times + duplicatesByOrg[orgID].UIDs[uid].InvolvedReaders[readerName] = struct{}{} } for id, times := range tracker.titleUsage { - if _, ok := duplicates.Titles[id]; !ok { - duplicates.Titles[id] = newDuplicate() + if _, ok := duplicatesByOrg[orgID].Titles[id]; !ok { + duplicatesByOrg[orgID].Titles[id] = newDuplicate() } - duplicates.Titles[id].Sum += times - duplicates.Titles[id].InvolvedReaders[readerName] = struct{}{} + duplicatesByOrg[orgID].Titles[id].Sum += times + duplicatesByOrg[orgID].Titles[id].InvolvedReaders[readerName] = struct{}{} } } - return &duplicates + return duplicatesByOrg } -func (c *duplicateValidator) logWarnings(duplicates *duplicateEntries) { - for uid, usage := range duplicates.UIDs { - if usage.Sum > 1 { - c.logger.Warn("the same UID is used more than once", "uid", uid, "times", usage.Sum, "providers", - keysToSlice(usage.InvolvedReaders)) +func (c *duplicateValidator) logWarnings(duplicatesByOrg map[int64]duplicateEntries) { + for orgID, duplicates := range duplicatesByOrg { + for uid, usage := range duplicates.UIDs { + if usage.Sum > 1 { + c.logger.Warn("the same UID is used more than once", "orgId", orgID, "uid", uid, "times", usage.Sum, "providers", + keysToSlice(usage.InvolvedReaders)) + } } - } - for id, usage := range duplicates.Titles { - if usage.Sum > 1 { - c.logger.Warn("dashboard title is not unique in folder", "title", id.title, "folderID", id.folderID, "times", - usage.Sum, "providers", keysToSlice(usage.InvolvedReaders)) + for id, usage := range duplicates.Titles { + if usage.Sum > 1 { + c.logger.Warn("dashboard title is not unique in folder", "orgId", orgID, "title", id.title, "folderID", id.folderID, "times", + usage.Sum, "providers", keysToSlice(usage.InvolvedReaders)) + } } } } -func (c *duplicateValidator) takeAwayWritePermissions(duplicates *duplicateEntries) { - involvedReaders := duplicates.InvolvedReaders() +func (c *duplicateValidator) takeAwayWritePermissions(duplicatesByOrg map[int64]duplicateEntries) { + // reset write permissions for all readers for _, reader := range c.readers { - _, isReaderWithDuplicates := involvedReaders[reader.Cfg.Name] - // We restrict reader permissions to write to the database here to prevent overloading - reader.changeWritePermissions(isReaderWithDuplicates) + reader.changeWritePermissions(false) + } - if isReaderWithDuplicates { - c.logger.Warn("dashboards provisioning provider has no database write permissions because of duplicates", "provider", reader.Cfg.Name) + for orgID, duplicates := range duplicatesByOrg { + involvedReaders := duplicates.InvolvedReaders() + for _, reader := range c.readers { + _, exists := involvedReaders[reader.Cfg.Name] + + if exists { + // We restrict reader permissions to write to the database here to prevent overloading + reader.changeWritePermissions(true) + c.logger.Warn("dashboards provisioning provider has no database write permissions because of duplicates", "provider", reader.Cfg.Name, "orgId", orgID) + } } } } diff --git a/pkg/services/provisioning/dashboards/validator_test.go b/pkg/services/provisioning/dashboards/validator_test.go index d3bbcdada79..e1e588e75f9 100644 --- a/pkg/services/provisioning/dashboards/validator_test.go +++ b/pkg/services/provisioning/dashboards/validator_test.go @@ -62,13 +62,13 @@ func TestDuplicatesValidator(t *testing.T) { duplicates := duplicateValidator.getDuplicates() - require.Equal(t, uint8(2), duplicates.UIDs["Z-phNqGmz"].Sum) - uidUsageReaders := keysToSlice(duplicates.UIDs["Z-phNqGmz"].InvolvedReaders) + require.Equal(t, uint8(2), duplicates[1].UIDs["Z-phNqGmz"].Sum) + uidUsageReaders := keysToSlice(duplicates[1].UIDs["Z-phNqGmz"].InvolvedReaders) sort.Strings(uidUsageReaders) require.Equal(t, []string{"first", "second"}, uidUsageReaders) - require.Equal(t, uint8(2), duplicates.Titles[identity].Sum) - titleUsageReaders := keysToSlice(duplicates.Titles[identity].InvolvedReaders) + require.Equal(t, uint8(2), duplicates[1].Titles[identity].Sum) + titleUsageReaders := keysToSlice(duplicates[1].Titles[identity].InvolvedReaders) sort.Strings(titleUsageReaders) require.Equal(t, []string{"first", "second"}, titleUsageReaders) @@ -77,14 +77,20 @@ func TestDuplicatesValidator(t *testing.T) { require.True(t, reader2.isDatabaseAccessRestricted()) }) - t.Run("Duplicates validator should restrict write access only for readers with duplicates", func(t *testing.T) { + t.Run("Duplicates validator should not collect info about duplicate UIDs and titles within folders for different orgs", func(t *testing.T) { + const folderName = "duplicates-validator-folder" + folderID, err := getOrCreateFolderID(context.Background(), cfg, fakeService, folderName) + require.NoError(t, err) + + identity := dashboardIdentity{folderID: folderID, title: "Grafana"} + cfg1 := &config{ - Name: "first", Type: "file", OrgID: 1, Folder: "duplicates-validator-folder", - Options: map[string]interface{}{"path": twoDashboardsWithUID}, + Name: "first", Type: "file", OrgID: 1, Folder: folderName, + Options: map[string]interface{}{"path": dashboardContainingUID}, } cfg2 := &config{ - Name: "second", Type: "file", OrgID: 1, Folder: "root", - Options: map[string]interface{}{"path": defaultDashboards}, + Name: "second", Type: "file", OrgID: 2, Folder: folderName, + Options: map[string]interface{}{"path": dashboardContainingUID}, } reader1, err := NewDashboardFileReader(cfg1, logger, nil) @@ -103,23 +109,100 @@ func TestDuplicatesValidator(t *testing.T) { duplicates := duplicateValidator.getDuplicates() + require.Equal(t, uint8(1), duplicates[1].UIDs["Z-phNqGmz"].Sum) + uidUsageReaders := keysToSlice(duplicates[1].UIDs["Z-phNqGmz"].InvolvedReaders) + sort.Strings(uidUsageReaders) + require.Equal(t, []string{"first"}, uidUsageReaders) + + require.Equal(t, uint8(1), duplicates[2].UIDs["Z-phNqGmz"].Sum) + uidUsageReaders = keysToSlice(duplicates[2].UIDs["Z-phNqGmz"].InvolvedReaders) + sort.Strings(uidUsageReaders) + require.Equal(t, []string{"second"}, uidUsageReaders) + + require.Equal(t, uint8(1), duplicates[1].Titles[identity].Sum) + titleUsageReaders := keysToSlice(duplicates[1].Titles[identity].InvolvedReaders) + sort.Strings(titleUsageReaders) + require.Equal(t, []string{"first"}, titleUsageReaders) + + require.Equal(t, uint8(1), duplicates[2].Titles[identity].Sum) + titleUsageReaders = keysToSlice(duplicates[2].Titles[identity].InvolvedReaders) + sort.Strings(titleUsageReaders) + require.Equal(t, []string{"second"}, titleUsageReaders) + + duplicateValidator.validate() + require.False(t, reader1.isDatabaseAccessRestricted()) + require.False(t, reader2.isDatabaseAccessRestricted()) + }) + + t.Run("Duplicates validator should restrict write access only for readers with duplicates", func(t *testing.T) { + cfg1 := &config{ + Name: "first", Type: "file", OrgID: 1, Folder: "duplicates-validator-folder", + Options: map[string]interface{}{"path": twoDashboardsWithUID}, + } + cfg2 := &config{ + Name: "second", Type: "file", OrgID: 1, Folder: "root", + Options: map[string]interface{}{"path": defaultDashboards}, + } + cfg3 := &config{ + Name: "third", Type: "file", OrgID: 2, Folder: "duplicates-validator-folder", + Options: map[string]interface{}{"path": twoDashboardsWithUID}, + } + + reader1, err := NewDashboardFileReader(cfg1, logger, nil) + require.NoError(t, err) + + reader2, err := NewDashboardFileReader(cfg2, logger, nil) + require.NoError(t, err) + + reader3, err := NewDashboardFileReader(cfg3, logger, nil) + require.NoError(t, err) + + duplicateValidator := newDuplicateValidator(logger, []*FileReader{reader1, reader2, reader3}) + + err = reader1.walkDisk(context.Background()) + require.NoError(t, err) + + err = reader2.walkDisk(context.Background()) + require.NoError(t, err) + + err = reader3.walkDisk(context.Background()) + require.NoError(t, err) + + duplicates := duplicateValidator.getDuplicates() + folderID, err := getOrCreateFolderID(context.Background(), cfg, fakeService, cfg1.Folder) require.NoError(t, err) identity := dashboardIdentity{folderID: folderID, title: "Grafana"} - require.Equal(t, uint8(2), duplicates.UIDs["Z-phNqGmz"].Sum) - uidUsageReaders := keysToSlice(duplicates.UIDs["Z-phNqGmz"].InvolvedReaders) + require.Equal(t, uint8(2), duplicates[1].UIDs["Z-phNqGmz"].Sum) + uidUsageReaders := keysToSlice(duplicates[1].UIDs["Z-phNqGmz"].InvolvedReaders) sort.Strings(uidUsageReaders) require.Equal(t, []string{"first"}, uidUsageReaders) - require.Equal(t, uint8(2), duplicates.Titles[identity].Sum) - titleUsageReaders := keysToSlice(duplicates.Titles[identity].InvolvedReaders) + require.Equal(t, uint8(2), duplicates[1].Titles[identity].Sum) + titleUsageReaders := keysToSlice(duplicates[1].Titles[identity].InvolvedReaders) sort.Strings(titleUsageReaders) require.Equal(t, []string{"first"}, titleUsageReaders) + folderID, err = getOrCreateFolderID(context.Background(), cfg3, fakeService, cfg3.Folder) + require.NoError(t, err) + + identity = dashboardIdentity{folderID: folderID, title: "Grafana"} + + require.Equal(t, uint8(2), duplicates[2].UIDs["Z-phNqGmz"].Sum) + uidUsageReaders = keysToSlice(duplicates[2].UIDs["Z-phNqGmz"].InvolvedReaders) + sort.Strings(uidUsageReaders) + require.Equal(t, []string{"third"}, uidUsageReaders) + + require.Equal(t, uint8(2), duplicates[2].Titles[identity].Sum) + titleUsageReaders = keysToSlice(duplicates[2].Titles[identity].InvolvedReaders) + sort.Strings(titleUsageReaders) + require.Equal(t, []string{"third"}, titleUsageReaders) + duplicateValidator.validate() require.True(t, reader1.isDatabaseAccessRestricted()) require.False(t, reader2.isDatabaseAccessRestricted()) + require.True(t, reader3.isDatabaseAccessRestricted()) }) }