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

@@ -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
 	}
 

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

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,17 +304,55 @@ 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
 }

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 := &quotatest.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,