From 68479d844bed475ce8e258a66d4a230e048768dc Mon Sep 17 00:00:00 2001 From: Stephanie Hingtgen Date: Mon, 6 Jan 2025 10:05:22 -0700 Subject: [PATCH] Orgs: Remove dependency on dashboard table for deletion (#98501) --- pkg/api/http_server.go | 4 +- pkg/api/org.go | 2 +- pkg/api/org_test.go | 1 + pkg/server/wire.go | 1 + pkg/services/dashboards/dashboard.go | 2 + .../dashboards/dashboard_service_mock.go | 18 ++++++++ pkg/services/dashboards/database/database.go | 10 +++++ .../dashboards/database/database_test.go | 26 +++++++++++ .../dashboards/service/dashboard_service.go | 32 ++++++++++++++ .../service/dashboard_service_test.go | 32 ++++++++++++++ pkg/services/dashboards/store_mock.go | 18 ++++++++ .../folder/folderimpl/sqlstore_test.go | 8 +++- pkg/services/org/org.go | 5 ++- pkg/services/org/orgimpl/org.go | 5 --- pkg/services/org/orgimpl/org_delete_svc.go | 43 +++++++++++++++++++ pkg/services/org/orgimpl/store.go | 5 +-- pkg/services/org/orgtest/fake.go | 16 +++++-- 17 files changed, 212 insertions(+), 16 deletions(-) create mode 100644 pkg/services/org/orgimpl/org_delete_svc.go diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index c30d12447a6..a05f7897f72 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -205,6 +205,7 @@ type HTTPServer struct { tempUserService tempUser.Service loginAttemptService loginAttempt.Service orgService org.Service + orgDeletionService org.DeletionService TeamService team.Service accesscontrolService accesscontrol.Service annotationsRepo annotations.Repository @@ -264,7 +265,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi secretsMigrator secrets.Migrator, secretsPluginManager plugins.SecretsPluginManager, secretsService secrets.Service, secretsPluginMigrator spm.SecretMigrationProvider, secretsStore secretsKV.SecretsKVStore, publicDashboardsApi *publicdashboardsApi.Api, userService user.Service, tempUserService tempUser.Service, - loginAttemptService loginAttempt.Service, orgService org.Service, teamService team.Service, + loginAttemptService loginAttempt.Service, orgService org.Service, orgDeletionService org.DeletionService, teamService team.Service, accesscontrolService accesscontrol.Service, navTreeService navtree.Service, annotationRepo annotations.Repository, tagService tag.Service, searchv2HTTPService searchV2.SearchHTTPService, oauthTokenService oauthtoken.OAuthTokenService, statsService stats.Service, authnService authn.Service, pluginsCDNService *pluginscdn.Service, promGatherer prometheus.Gatherer, @@ -356,6 +357,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi tempUserService: tempUserService, loginAttemptService: loginAttemptService, orgService: orgService, + orgDeletionService: orgDeletionService, TeamService: teamService, navTreeService: navTreeService, accesscontrolService: accesscontrolService, diff --git a/pkg/api/org.go b/pkg/api/org.go index 882ebfb866d..cd4a847c2f0 100644 --- a/pkg/api/org.go +++ b/pkg/api/org.go @@ -297,7 +297,7 @@ func (hs *HTTPServer) DeleteOrgByID(c *contextmodel.ReqContext) response.Respons return response.Error(http.StatusBadRequest, "Can not delete org for current user", nil) } - if err := hs.orgService.Delete(c.Req.Context(), &org.DeleteOrgCommand{ID: orgID}); err != nil { + if err := hs.orgDeletionService.Delete(c.Req.Context(), &org.DeleteOrgCommand{ID: orgID}); err != nil { if errors.Is(err, org.ErrOrgNotFound) { return response.Error(http.StatusNotFound, "Failed to delete organization. ID not found", nil) } diff --git a/pkg/api/org_test.go b/pkg/api/org_test.go index 77042fb896c..39a15ba1b22 100644 --- a/pkg/api/org_test.go +++ b/pkg/api/org_test.go @@ -228,6 +228,7 @@ func TestAPIEndpoint_DeleteOrgs(t *testing.T) { server := SetupAPITestServer(t, func(hs *HTTPServer) { hs.Cfg = setting.NewCfg() hs.orgService = &orgtest.FakeOrgService{ExpectedOrg: &org.Org{}} + hs.orgDeletionService = &orgtest.FakeOrgDeletionService{} hs.userService = &usertest.FakeUserService{ExpectedSignedInUser: &user.SignedInUser{OrgID: 1}} hs.accesscontrolService = actest.FakeService{ExpectedPermissions: tt.permission} hs.authnService = &authntest.FakeService{} diff --git a/pkg/server/wire.go b/pkg/server/wire.go index 97e13c79e49..7a749adf4ae 100644 --- a/pkg/server/wire.go +++ b/pkg/server/wire.go @@ -337,6 +337,7 @@ var wireBasicSet = wire.NewSet( starApi.ProvideApi, userimpl.ProvideService, orgimpl.ProvideService, + orgimpl.ProvideDeletionService, statsimpl.ProvideService, grpccontext.ProvideContextHandler, grpcserver.ProvideService, diff --git a/pkg/services/dashboards/dashboard.go b/pkg/services/dashboards/dashboard.go index 429ef8071cc..abdc067eddb 100644 --- a/pkg/services/dashboards/dashboard.go +++ b/pkg/services/dashboards/dashboard.go @@ -16,6 +16,7 @@ import ( type DashboardService interface { BuildSaveDashboardCommand(ctx context.Context, dto *SaveDashboardDTO, validateProvisionedDashboard bool) (*SaveDashboardCommand, error) DeleteDashboard(ctx context.Context, dashboardId int64, dashboardUID string, orgId int64) error + DeleteAllDashboards(ctx context.Context, orgID int64) error FindDashboards(ctx context.Context, query *FindPersistedDashboardsQuery) ([]DashboardSearchProjection, error) // GetDashboard fetches a dashboard. // To fetch a dashboard under root by title should set the folder UID to point to an empty string @@ -60,6 +61,7 @@ type DashboardProvisioningService interface { //go:generate mockery --name Store --structname FakeDashboardStore --inpackage --filename store_mock.go type Store interface { DeleteDashboard(ctx context.Context, cmd *DeleteDashboardCommand) error + DeleteAllDashboards(ctx context.Context, orgID int64) error DeleteOrphanedProvisionedDashboards(ctx context.Context, cmd *DeleteOrphanedProvisionedDashboardsCommand) error FindDashboards(ctx context.Context, query *FindPersistedDashboardsQuery) ([]DashboardSearchProjection, error) GetDashboard(ctx context.Context, query *GetDashboardQuery) (*Dashboard, error) diff --git a/pkg/services/dashboards/dashboard_service_mock.go b/pkg/services/dashboards/dashboard_service_mock.go index 557305cc545..120d94c4ff8 100644 --- a/pkg/services/dashboards/dashboard_service_mock.go +++ b/pkg/services/dashboards/dashboard_service_mock.go @@ -120,6 +120,24 @@ func (_m *FakeDashboardService) DeleteDashboard(ctx context.Context, dashboardId return r0 } +// DeleteAllDashboards provides a mock function with given fields: ctx, orgID +func (_m *FakeDashboardService) DeleteAllDashboards(ctx context.Context, orgID int64) error { + ret := _m.Called(ctx, orgID) + + if len(ret) == 0 { + panic("no return value specified for DeleteDashboard") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, int64) error); ok { + r0 = rf(ctx, orgID) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // FindDashboards provides a mock function with given fields: ctx, query func (_m *FakeDashboardService) FindDashboards(ctx context.Context, query *FindPersistedDashboardsQuery) ([]DashboardSearchProjection, error) { ret := _m.Called(ctx, query) diff --git a/pkg/services/dashboards/database/database.go b/pkg/services/dashboards/database/database.go index 6c3c789284e..320a23a4dfd 100644 --- a/pkg/services/dashboards/database/database.go +++ b/pkg/services/dashboards/database/database.go @@ -667,6 +667,16 @@ func (d *dashboardStore) deleteDashboard(cmd *dashboards.DeleteDashboardCommand, return nil } +func (d *dashboardStore) DeleteAllDashboards(ctx context.Context, orgID int64) error { + ctx, span := tracer.Start(ctx, "dashboards.database.DeleteAllDashboards") + defer span.End() + + return d.store.WithTransactionalDbSession(ctx, func(sess *db.Session) error { + _, err := sess.Where("org_id = ?", orgID).Delete(&dashboards.Dashboard{}) + return err + }) +} + // FIXME: Remove me and handle nested deletions in the service with the DashboardPermissionsService func (d *dashboardStore) deleteResourcePermissions(sess *db.Session, orgID int64, resourceScope string) error { // retrieve all permissions for the resource scope and org id diff --git a/pkg/services/dashboards/database/database_test.go b/pkg/services/dashboards/database/database_test.go index fac6028f4df..7efa8be2bc5 100644 --- a/pkg/services/dashboards/database/database_test.go +++ b/pkg/services/dashboards/database/database_test.go @@ -240,6 +240,32 @@ func TestIntegrationDashboardDataAccess(t *testing.T) { require.NotContains(t, terms, "delete this") }) + t.Run("Should be able to delete all dashboards for an org", func(t *testing.T) { + setup() + dash1 := insertTestDashboard(t, dashboardStore, "delete me", 1, 0, "", false, "delete this") + dash2 := insertTestDashboard(t, dashboardStore, "delete me2", 1, 0, "", false, "delete this2") + dash3 := insertTestDashboard(t, dashboardStore, "dont delete me", 2, 0, "", false, "dont delete me") + + err := dashboardStore.DeleteAllDashboards(context.Background(), 1) + require.NoError(t, err) + + // no dashboards should exist for org 1 + queryResult, err := dashboardStore.GetDashboards(context.Background(), &dashboards.GetDashboardsQuery{ + OrgID: 1, + DashboardUIDs: []string{dash1.UID, dash2.UID}, + }) + require.NoError(t, err) + assert.Equal(t, len(queryResult), 0) + + // but we should still have one for org 2 + queryResult, err = dashboardStore.GetDashboards(context.Background(), &dashboards.GetDashboardsQuery{ + OrgID: 2, + DashboardUIDs: []string{dash3.UID}, + }) + require.NoError(t, err) + assert.Equal(t, len(queryResult), 1) + }) + t.Run("Should be able to create dashboard", func(t *testing.T) { setup() cmd := dashboards.SaveDashboardCommand{ diff --git a/pkg/services/dashboards/service/dashboard_service.go b/pkg/services/dashboards/service/dashboard_service.go index 0531772ec57..81a56fb5462 100644 --- a/pkg/services/dashboards/service/dashboard_service.go +++ b/pkg/services/dashboards/service/dashboard_service.go @@ -511,6 +511,15 @@ func (dr *DashboardServiceImpl) DeleteDashboard(ctx context.Context, dashboardId return dr.deleteDashboard(ctx, dashboardId, dashboardUID, orgId, true) } +// DeleteAllDashboards will delete all dashboards within a given org. +func (dr *DashboardServiceImpl) DeleteAllDashboards(ctx context.Context, orgId int64) error { + if dr.features.IsEnabledGlobally(featuremgmt.FlagKubernetesCliDashboards) { + return dr.deleteAllDashboardThroughK8s(ctx, orgId) + } + + return dr.dashboardStore.DeleteAllDashboards(ctx, orgId) +} + func (dr *DashboardServiceImpl) GetDashboardByPublicUid(ctx context.Context, dashboardPublicUid string) (*dashboards.Dashboard, error) { return nil, nil } @@ -1178,6 +1187,29 @@ func (dr *DashboardServiceImpl) saveDashboardThroughK8s(ctx context.Context, cmd return finalDash, nil } +func (dr *DashboardServiceImpl) deleteAllDashboardThroughK8s(ctx context.Context, orgID int64) error { + // create a new context - prevents issues when the request stems from the k8s api itself + // otherwise the context goes through the handlers twice and causes issues + newCtx, cancel, err := dr.getK8sContext(ctx) + if err != nil { + return err + } else if cancel != nil { + defer cancel() + } + + client, ok := dr.k8sclient.getClient(newCtx, orgID) + if !ok { + return fmt.Errorf("could not get k8s client") + } + + err = client.DeleteCollection(newCtx, v1.DeleteOptions{}, v1.ListOptions{}) + if err != nil { + return err + } + + return nil +} + func (dr *DashboardServiceImpl) deleteDashboardThroughK8s(ctx context.Context, cmd *dashboards.DeleteDashboardCommand) error { // create a new context - prevents issues when the request stems from the k8s api itself // otherwise the context goes through the handlers twice and causes issues diff --git a/pkg/services/dashboards/service/dashboard_service_test.go b/pkg/services/dashboards/service/dashboard_service_test.go index 197947f7012..8baebad32cb 100644 --- a/pkg/services/dashboards/service/dashboard_service_test.go +++ b/pkg/services/dashboards/service/dashboard_service_test.go @@ -305,6 +305,11 @@ func (m *mockResourceInterface) Delete(ctx context.Context, name string, options return args.Error(0) } +func (m *mockResourceInterface) DeleteCollection(ctx context.Context, options metav1.DeleteOptions, listOptions metav1.ListOptions) error { + args := m.Called(ctx, options, listOptions) + return args.Error(0) +} + func setupK8sDashboardTests(service *DashboardServiceImpl) (context.Context, *mockDashK8sCli, *mockResourceInterface) { k8sClientMock := new(mockDashK8sCli) k8sResourceMock := new(mockResourceInterface) @@ -644,6 +649,33 @@ func TestDeleteDashboard(t *testing.T) { }) } +func TestDeleteAllDashboards(t *testing.T) { + fakeStore := dashboards.FakeDashboardStore{} + defer fakeStore.AssertExpectations(t) + service := &DashboardServiceImpl{ + cfg: setting.NewCfg(), + dashboardStore: &fakeStore, + } + + t.Run("Should fallback to dashboard store if Kubernetes feature flags are not enabled", func(t *testing.T) { + service.features = featuremgmt.WithFeatures() + fakeStore.On("DeleteAllDashboards", mock.Anything, mock.Anything).Return(nil).Once() + err := service.DeleteAllDashboards(context.Background(), 1) + require.NoError(t, err) + fakeStore.AssertExpectations(t) + }) + + t.Run("Should use Kubernetes client if feature flags are enabled", func(t *testing.T) { + ctx, k8sClientMock, k8sResourceMock := setupK8sDashboardTests(service) + k8sClientMock.On("getClient", mock.Anything, int64(1)).Return(k8sResourceMock, true).Once() + k8sResourceMock.On("DeleteCollection", mock.Anything, mock.Anything, mock.Anything).Return(nil).Once() + + err := service.DeleteAllDashboards(ctx, 1) + require.NoError(t, err) + k8sClientMock.AssertExpectations(t) + }) +} + func TestSearchDashboards(t *testing.T) { fakeStore := dashboards.FakeDashboardStore{} defer fakeStore.AssertExpectations(t) diff --git a/pkg/services/dashboards/store_mock.go b/pkg/services/dashboards/store_mock.go index c690918bdcc..93121e0eff6 100644 --- a/pkg/services/dashboards/store_mock.go +++ b/pkg/services/dashboards/store_mock.go @@ -94,6 +94,24 @@ func (_m *FakeDashboardStore) DeleteDashboard(ctx context.Context, cmd *DeleteDa return r0 } +// DeleteAllDashboards provides a mock function with given fields: ctx, orgID +func (_m *FakeDashboardStore) DeleteAllDashboards(ctx context.Context, orgID int64) error { + ret := _m.Called(ctx, orgID) + + if len(ret) == 0 { + panic("no return value specified for DeleteDashboard") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, int64) error); ok { + r0 = rf(ctx, orgID) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // DeleteDashboardsInFolders provides a mock function with given fields: ctx, request func (_m *FakeDashboardStore) DeleteDashboardsInFolders(ctx context.Context, request *DeleteDashboardsInFolderRequest) error { ret := _m.Called(ctx, request) diff --git a/pkg/services/folder/folderimpl/sqlstore_test.go b/pkg/services/folder/folderimpl/sqlstore_test.go index 7d639b92e8b..e76607cb401 100644 --- a/pkg/services/folder/folderimpl/sqlstore_test.go +++ b/pkg/services/folder/folderimpl/sqlstore_test.go @@ -11,10 +11,12 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/uuid" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/org/orgimpl" @@ -975,10 +977,14 @@ func CreateOrg(t *testing.T, db db.DB, cfg *setting.Cfg) int64 { orgService, err := orgimpl.ProvideService(db, cfg, quotatest.New(false, nil)) require.NoError(t, err) + dashSvc := &dashboards.FakeDashboardService{} + dashSvc.On("DeleteAllDashboards", mock.Anything, mock.Anything).Return(nil) + deleteOrgService, err := orgimpl.ProvideDeletionService(db, cfg, dashSvc) + require.NoError(t, err) orgID, err := orgService.GetOrCreate(context.Background(), "test-org") require.NoError(t, err) t.Cleanup(func() { - err = orgService.Delete(context.Background(), &org.DeleteOrgCommand{ID: orgID}) + err = deleteOrgService.Delete(context.Background(), &org.DeleteOrgCommand{ID: orgID}) require.NoError(t, err) }) diff --git a/pkg/services/org/org.go b/pkg/services/org/org.go index cb531aea33e..34b492630a5 100644 --- a/pkg/services/org/org.go +++ b/pkg/services/org/org.go @@ -16,7 +16,6 @@ type Service interface { GetByName(context.Context, *GetOrgByNameQuery) (*Org, error) CreateWithMember(context.Context, *CreateOrgCommand) (*Org, error) UpdateAddress(context.Context, *UpdateOrgAddressCommand) error - Delete(context.Context, *DeleteOrgCommand) error GetOrCreate(context.Context, string) (int64, error) AddOrgUser(context.Context, *AddOrgUserCommand) error UpdateOrgUser(context.Context, *UpdateOrgUserCommand) error @@ -25,3 +24,7 @@ type Service interface { SearchOrgUsers(context.Context, *SearchOrgUsersQuery) (*SearchOrgUsersQueryResult, error) RegisterDelete(query string) } + +type DeletionService interface { + Delete(context.Context, *DeleteOrgCommand) error +} diff --git a/pkg/services/org/orgimpl/org.go b/pkg/services/org/orgimpl/org.go index 47ecc8d44e1..423a4bc8b8d 100644 --- a/pkg/services/org/orgimpl/org.go +++ b/pkg/services/org/orgimpl/org.go @@ -138,11 +138,6 @@ func (s *Service) UpdateAddress(ctx context.Context, cmd *org.UpdateOrgAddressCo return s.store.UpdateAddress(ctx, cmd) } -// TODO: refactor service to call store CRUD method -func (s *Service) Delete(ctx context.Context, cmd *org.DeleteOrgCommand) error { - return s.store.Delete(ctx, cmd) -} - func (s *Service) GetOrCreate(ctx context.Context, orgName string) (int64, error) { var orga = &org.Org{} var err error diff --git a/pkg/services/org/orgimpl/org_delete_svc.go b/pkg/services/org/orgimpl/org_delete_svc.go new file mode 100644 index 00000000000..e2aba89fc50 --- /dev/null +++ b/pkg/services/org/orgimpl/org_delete_svc.go @@ -0,0 +1,43 @@ +package orgimpl + +import ( + "context" + + "github.com/grafana/grafana/pkg/infra/db" + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/services/dashboards" + "github.com/grafana/grafana/pkg/services/org" + "github.com/grafana/grafana/pkg/setting" +) + +type DeletionService struct { + store store + cfg *setting.Cfg + log log.Logger + dashSvc dashboards.DashboardService +} + +func ProvideDeletionService(db db.DB, cfg *setting.Cfg, dashboardService dashboards.DashboardService) (org.DeletionService, error) { + log := log.New("org deletion service") + s := &DeletionService{ + store: &sqlStore{ + db: db, + dialect: db.GetDialect(), + log: log, + }, + cfg: cfg, + dashSvc: dashboardService, + log: log, + } + + return s, nil +} + +func (s *DeletionService) Delete(ctx context.Context, cmd *org.DeleteOrgCommand) error { + err := s.dashSvc.DeleteAllDashboards(ctx, cmd.ID) + if err != nil { + return err + } + + return s.store.Delete(ctx, cmd) +} diff --git a/pkg/services/org/orgimpl/store.go b/pkg/services/org/orgimpl/store.go index b1d5a711747..d56f941e34c 100644 --- a/pkg/services/org/orgimpl/store.go +++ b/pkg/services/org/orgimpl/store.go @@ -220,9 +220,8 @@ func (ss *sqlStore) Delete(ctx context.Context, cmd *org.DeleteOrgCommand) error } deletes := []string{ - "DELETE FROM star WHERE EXISTS (SELECT 1 FROM dashboard WHERE org_id = ? AND star.dashboard_uid = dashboard.uid)", - "DELETE FROM dashboard_tag WHERE EXISTS (SELECT 1 FROM dashboard WHERE org_id = ? AND dashboard_tag.dashboard_id = dashboard.id)", - "DELETE FROM dashboard WHERE org_id = ?", + "DELETE FROM star WHERE org_id = ?", + "DELETE FROM dashboard_tag WHERE org_id = ?", "DELETE FROM api_key WHERE org_id = ?", "DELETE FROM data_source WHERE org_id = ?", "DELETE FROM org_user WHERE org_id = ?", diff --git a/pkg/services/org/orgtest/fake.go b/pkg/services/org/orgtest/fake.go index 4477ea44d51..0ec1c6e848c 100644 --- a/pkg/services/org/orgtest/fake.go +++ b/pkg/services/org/orgtest/fake.go @@ -75,10 +75,6 @@ func (f *FakeOrgService) UpdateAddress(ctx context.Context, cmd *org.UpdateOrgAd return f.ExpectedError } -func (f *FakeOrgService) Delete(ctx context.Context, cmd *org.DeleteOrgCommand) error { - return f.ExpectedError -} - func (f *FakeOrgService) GetOrCreate(ctx context.Context, orgName string) (int64, error) { return f.ExpectedOrg.ID, f.ExpectedError } @@ -107,3 +103,15 @@ func (f *FakeOrgService) SearchOrgUsers(ctx context.Context, query *org.SearchOr func (f *FakeOrgService) RegisterDelete(query string) { } + +type FakeOrgDeletionService struct { + ExpectedError error +} + +func NewOrgDeletionServiceFake() *FakeOrgDeletionService { + return &FakeOrgDeletionService{} +} + +func (f *FakeOrgDeletionService) Delete(ctx context.Context, cmd *org.DeleteOrgCommand) error { + return f.ExpectedError +}