From a6d928e50e08a519e8375dcf27124a9bab7938d0 Mon Sep 17 00:00:00 2001 From: Matthew Jacobson Date: Thu, 12 Oct 2023 23:40:51 +0100 Subject: [PATCH] Alerting: Prevent cleanup of non-empty folders on migration revert (#76439) Prevent cleanup of non-empty folders on revert --- pkg/services/ngalert/migration/permissions.go | 13 +++ .../ngalert/migration/service_test.go | 102 ++++++++++++++++++ .../ngalert/migration/store/database.go | 51 ++++++++- .../ngalert/migration/store/testing.go | 23 +++- 4 files changed, 180 insertions(+), 9 deletions(-) diff --git a/pkg/services/ngalert/migration/permissions.go b/pkg/services/ngalert/migration/permissions.go index 34c564ecf3a..48894c9617e 100644 --- a/pkg/services/ngalert/migration/permissions.go +++ b/pkg/services/ngalert/migration/permissions.go @@ -383,6 +383,19 @@ func (om *OrgMigration) createFolder(ctx context.Context, orgID int64, title str SignedInUser: getMigrationUser(orgID).(*user.SignedInUser), }) if err != nil { + if errors.Is(err, dashboards.ErrFolderSameNameExists) { + // If the folder already exists, we return the existing folder. + // This isn't perfect since permissions might have been manually modified, + // but the only folders we should be creating here are ones with permission + // hash suffix or general alerting. Neither of which is likely to spuriously + // conflict with an existing folder. + om.log.Warn("Folder already exists, using existing folder", "title", title) + f, err := om.migrationStore.GetFolder(ctx, &folder.GetFolderQuery{OrgID: orgID, Title: &title, SignedInUser: getMigrationUser(orgID)}) + if err != nil { + return nil, err + } + return f, nil + } return nil, err } diff --git a/pkg/services/ngalert/migration/service_test.go b/pkg/services/ngalert/migration/service_test.go index 4bc386a5fe5..0b4c464e974 100644 --- a/pkg/services/ngalert/migration/service_test.go +++ b/pkg/services/ngalert/migration/service_test.go @@ -187,4 +187,106 @@ func TestServiceRevert(t *testing.T) { require.Nil(t, getDashboard(t, x, 1, uid)) } }) + + t.Run("revert skips migrated folders that are not empty", func(t *testing.T) { + sqlStore := db.InitTestDB(t) + x := sqlStore.GetEngine() + alerts = []*legacymodels.Alert{ + createAlert(t, 1, 8, 1, "alert1", []string{"notifier1"}), + } + setupLegacyAlertsTables(t, x, channels, alerts, folders, dashes) + + dashCount, err := x.Table("dashboard").Count(&dashboards.Dashboard{}) + require.NoError(t, err) + require.Equal(t, int64(4), dashCount) + + // Run migration. + ctx := context.Background() + cfg := &setting.Cfg{ + ForceMigration: true, + UnifiedAlerting: setting.UnifiedAlertingSettings{ + Enabled: pointer(true), + }, + } + service := NewTestMigrationService(t, sqlStore, cfg) + + err = service.migrationStore.SetMigrated(ctx, false) + require.NoError(t, err) + + err = service.Run(ctx) + require.NoError(t, err) + + // Verify migration was run. + migrated, err := service.migrationStore.IsMigrated(ctx) + require.NoError(t, err) + require.Equal(t, true, migrated) + + // Verify we created some folders. + newDashCount, err := x.Table("dashboard").Count(&dashboards.Dashboard{}) + require.NoError(t, err) + require.Truef(t, newDashCount > dashCount, "newDashCount: %d should be greater than dashCount: %d", newDashCount, dashCount) + + // Check that dashboards and folders from before migration still exist. + require.NotNil(t, getDashboard(t, x, 1, "dash1-1")) + require.NotNil(t, getDashboard(t, x, 1, "dash2-1")) + require.NotNil(t, getDashboard(t, x, 1, "dash-in-general-1")) + + state, err := service.migrationStore.GetOrgMigrationState(ctx, 1) + require.NoError(t, err) + + // Verify list of created folders. + require.NotEmpty(t, state.CreatedFolders) + var generalAlertingFolder *dashboards.Dashboard + for _, uid := range state.CreatedFolders { + f := getDashboard(t, x, 1, uid) + require.NotNil(t, f) + if f.Slug == "general-alerting" { + generalAlertingFolder = f + } + } + require.NotNil(t, generalAlertingFolder) + + // Create dashboard in general alerting. + newDashes := []*dashboards.Dashboard{ + createDashboard(t, 99, 1, "dash-in-general-alerting-1", generalAlertingFolder.ID, nil), + } + _, err = x.Insert(newDashes) + require.NoError(t, err) + + newF := getDashboard(t, x, 1, "dash-in-general-alerting-1") + require.NotNil(t, newF) + + // Revert migration. + service.cfg.UnifiedAlerting.Enabled = pointer(false) + err = service.Run(ctx) + require.NoError(t, err) + + // Verify revert was run. + migrated, err = service.migrationStore.IsMigrated(ctx) + require.NoError(t, err) + require.Equal(t, false, migrated) + + // Verify we are back to the original count + 2. + newDashCount, err = x.Table("dashboard").Count(&dashboards.Dashboard{}) + require.NoError(t, err) + require.Equalf(t, dashCount+2, newDashCount, "newDashCount: %d should be equal to dashCount + 2: %d after revert", newDashCount, dashCount) + + // Check that dashboards and folders from before migration still exist. + require.NotNil(t, getDashboard(t, x, 1, "dash1-1")) + require.NotNil(t, getDashboard(t, x, 1, "dash2-1")) + require.NotNil(t, getDashboard(t, x, 1, "dash-in-general-1")) + + // Check that the general alerting folder still exists. + require.NotNil(t, getDashboard(t, x, 1, generalAlertingFolder.UID)) + // Check that the new dashboard in general alerting folder still exists. + require.NotNil(t, getDashboard(t, x, 1, "dash-in-general-alerting-1")) + + // Check that other folders created during migration are gone. + for _, uid := range state.CreatedFolders { + if uid == generalAlertingFolder.UID { + continue + } + require.Nil(t, getDashboard(t, x, 1, uid)) + } + }) } diff --git a/pkg/services/ngalert/migration/store/database.go b/pkg/services/ngalert/migration/store/database.go index 72c5efca837..2eecc8924ad 100644 --- a/pkg/services/ngalert/migration/store/database.go +++ b/pkg/services/ngalert/migration/store/database.go @@ -3,10 +3,12 @@ package store import ( "context" "encoding/json" + "errors" "fmt" "os" "path/filepath" "strconv" + "strings" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/kvstore" @@ -239,7 +241,8 @@ func (ms *migrationStore) RevertAllOrgs(ctx context.Context) error { } for _, o := range orgs { if err := ms.DeleteMigratedFolders(ctx, o.ID); err != nil { - return err + ms.log.Warn("Failed to delete migrated folders", "orgID", o.ID, "err", err) + continue } } @@ -292,6 +295,8 @@ func (ms *migrationStore) DeleteMigratedFolders(ctx context.Context, orgID int64 return ms.DeleteFolders(ctx, orgID, summary.CreatedFolders...) } +var ErrFolderNotDeleted = fmt.Errorf("folder not deleted") + // DeleteFolders deletes the folders from the given orgs with the given UIDs. This includes all folder permissions. // If the folder is not empty of all descendants the operation will fail and return an error. func (ms *migrationStore) DeleteFolders(ctx context.Context, orgID int64, uids ...string) error { @@ -299,18 +304,56 @@ func (ms *migrationStore) DeleteFolders(ctx context.Context, orgID int64, uids . return nil } + var errs error usr := accesscontrol.BackgroundUser("ngalert_migration_revert", orgID, org.RoleAdmin, revertPermissions) for _, folderUID := range uids { + // Check if folder is empty. If not, we should not delete it. + uid := folderUID + countCmd := folder.GetDescendantCountsQuery{ + UID: &uid, + OrgID: orgID, + SignedInUser: usr.(*user.SignedInUser), + } + count, err := ms.folderService.GetDescendantCounts(ctx, &countCmd) + if err != nil { + errs = errors.Join(errs, fmt.Errorf("folder %s: %w", folderUID, err)) + continue + } + var descendantCounts []string + var cntErr error + for kind, cnt := range count { + if cnt > 0 { + descendantCounts = append(descendantCounts, fmt.Sprintf("%d %s", cnt, kind)) + if err != nil { + cntErr = errors.Join(cntErr, err) + continue + } + } + } + if cntErr != nil { + errs = errors.Join(errs, fmt.Errorf("folder %s: %w", folderUID, cntErr)) + continue + } + + if len(descendantCounts) > 0 { + errs = errors.Join(errs, fmt.Errorf("folder %s contains descendants: %s", folderUID, strings.Join(descendantCounts, ", "))) + continue + } + cmd := folder.DeleteFolderCommand{ - UID: folderUID, + UID: uid, OrgID: orgID, SignedInUser: usr.(*user.SignedInUser), } - err := ms.folderService.Delete(ctx, &cmd) // Also handles permissions and other related entities. + err = ms.folderService.Delete(ctx, &cmd) // Also handles permissions and other related entities. if err != nil { - return err + errs = errors.Join(errs, fmt.Errorf("folder %s: %w", folderUID, err)) + continue } } + if errs != nil { + return fmt.Errorf("%w: %w", ErrFolderNotDeleted, errs) + } return nil } diff --git a/pkg/services/ngalert/migration/store/testing.go b/pkg/services/ngalert/migration/store/testing.go index c1478cff67b..a21dfa4d415 100644 --- a/pkg/services/ngalert/migration/store/testing.go +++ b/pkg/services/ngalert/migration/store/testing.go @@ -15,19 +15,22 @@ import ( "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" "github.com/grafana/grafana/pkg/services/accesscontrol/ossaccesscontrol" legacyalerting "github.com/grafana/grafana/pkg/services/alerting" - "github.com/grafana/grafana/pkg/services/datasources/guardian" + "github.com/grafana/grafana/pkg/services/dashboards/database" + dashboardservice "github.com/grafana/grafana/pkg/services/dashboards/service" + datasourceGuardian "github.com/grafana/grafana/pkg/services/datasources/guardian" datasourceService "github.com/grafana/grafana/pkg/services/datasources/service" encryptionservice "github.com/grafana/grafana/pkg/services/encryption/service" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/folder/folderimpl" + "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/services/licensing/licensingtest" "github.com/grafana/grafana/pkg/services/ngalert/store" "github.com/grafana/grafana/pkg/services/ngalert/tests/fakes" - "github.com/grafana/grafana/pkg/services/ngalert/testutil" "github.com/grafana/grafana/pkg/services/org/orgimpl" "github.com/grafana/grafana/pkg/services/quota/quotatest" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/services/supportbundles/bundleregistry" + "github.com/grafana/grafana/pkg/services/tag/tagimpl" "github.com/grafana/grafana/pkg/services/team/teamimpl" "github.com/grafana/grafana/pkg/services/user/userimpl" "github.com/grafana/grafana/pkg/setting" @@ -45,8 +48,6 @@ func NewTestMigrationStore(t *testing.T, sqlStore *sqlstore.SQLStore, cfg *setti } bus := bus.ProvideBus(tracing.InitializeTracerForTest()) folderStore := folderimpl.ProvideDashboardFolderStore(sqlStore) - dashboardService, dashboardStore := testutil.SetupDashboardService(t, sqlStore, folderStore, cfg) - folderService := testutil.SetupFolderService(t, cfg, sqlStore, dashboardStore, folderStore, bus) cache := localcache.ProvideService() quotaService := "atest.FakeQuotaService{} @@ -63,6 +64,10 @@ func NewTestMigrationStore(t *testing.T, sqlStore *sqlstore.SQLStore, cfg *setti userSvc, err := userimpl.ProvideService(sqlStore, orgService, cfg, teamSvc, cache, quotaService, bundleregistry.ProvideService()) require.NoError(t, err) + dashboardStore, err := database.ProvideDashboardStore(sqlStore, sqlStore.Cfg, features, tagimpl.ProvideService(sqlStore, cfg), quotaService) + require.NoError(t, err) + folderService := folderimpl.ProvideService(ac, bus, cfg, dashboardStore, folderStore, sqlStore, features) + folderPermissions, err := ossaccesscontrol.ProvideFolderPermissions( features, routeRegister, sqlStore, ac, license, dashboardStore, folderService, acSvc, teamSvc, userSvc) require.NoError(t, err) @@ -70,6 +75,14 @@ func NewTestMigrationStore(t *testing.T, sqlStore *sqlstore.SQLStore, cfg *setti features, routeRegister, sqlStore, ac, license, dashboardStore, folderService, acSvc, teamSvc, userSvc) require.NoError(t, err) + dashboardService, err := dashboardservice.ProvideDashboardServiceImpl( + cfg, dashboardStore, folderStore, nil, + features, folderPermissions, dashboardPermissions, ac, + folderService, + ) + require.NoError(t, err) + guardian.InitAccessControlGuardian(setting.NewCfg(), ac, dashboardService) + err = acSvc.RegisterFixedRoles(context.Background()) require.NoError(t, err) @@ -81,7 +94,7 @@ func NewTestMigrationStore(t *testing.T, sqlStore *sqlstore.SQLStore, cfg *setti alertingStore: &alertingStore, dashboardService: dashboardService, folderService: folderService, - dataSourceCache: datasourceService.ProvideCacheService(cache, sqlStore, guardian.ProvideGuardian()), + dataSourceCache: datasourceService.ProvideCacheService(cache, sqlStore, datasourceGuardian.ProvideGuardian()), folderPermissions: folderPermissions, dashboardPermissions: dashboardPermissions, orgService: orgService,