From 20589f76f425092e5e1a8e64a4b11d1d38436071 Mon Sep 17 00:00:00 2001 From: Guilherme Caulada Date: Tue, 6 Sep 2022 16:21:25 -0300 Subject: [PATCH] Secrets: Convert secret migration to a background service (#54676) * Convert secret migration to a background service * Fix merge conflicts * Return concrete type for secret migration provider --- pkg/api/http_server.go | 4 +- pkg/cmd/grafana-cli/runner/wire.go | 5 -- .../backgroundsvcs/background_services.go | 3 + pkg/server/server.go | 56 ++++++++----------- pkg/server/server_test.go | 9 +-- pkg/server/wire.go | 4 +- .../secrets/kvstore/migrations/migrator.go | 22 ++++++-- 7 files changed, 48 insertions(+), 55 deletions(-) diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index c9cf189c311..049d4f97e49 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -147,7 +147,7 @@ type HTTPServer struct { secretsPluginManager plugins.SecretsPluginManager secretsStore secretsKV.SecretsKVStore secretsMigrator secrets.Migrator - secretsPluginMigrator *spm.SecretMigrationServiceImpl + secretsPluginMigrator spm.SecretMigrationProvider DataSourcesService datasources.DataSourceService cleanUpService *cleanup.CleanUpService tracer tracing.Tracer @@ -225,7 +225,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi starService star.Service, csrfService csrf.Service, coremodels *registry.Base, playlistService playlist.Service, apiKeyService apikey.Service, kvStore kvstore.KVStore, secretsMigrator secrets.Migrator, secretsPluginManager plugins.SecretsPluginManager, secretsService secrets.Service, - secretsPluginMigrator *spm.SecretMigrationServiceImpl, secretsStore secretsKV.SecretsKVStore, + secretsPluginMigrator spm.SecretMigrationProvider, secretsStore secretsKV.SecretsKVStore, publicDashboardsApi *publicdashboardsApi.Api, userService user.Service, tempUserService tempUser.Service, loginAttemptService loginAttempt.Service, orgService org.Service, accesscontrolService accesscontrol.Service, dashboardThumbsService dashboardThumbs.Service, ) (*HTTPServer, error) { diff --git a/pkg/cmd/grafana-cli/runner/wire.go b/pkg/cmd/grafana-cli/runner/wire.go index ca69b546132..b57792c6be7 100644 --- a/pkg/cmd/grafana-cli/runner/wire.go +++ b/pkg/cmd/grafana-cli/runner/wire.go @@ -103,7 +103,6 @@ import ( "github.com/grafana/grafana/pkg/services/secrets" secretsDatabase "github.com/grafana/grafana/pkg/services/secrets/database" secretsStore "github.com/grafana/grafana/pkg/services/secrets/kvstore" - secretsMigrations "github.com/grafana/grafana/pkg/services/secrets/kvstore/migrations" secretsManager "github.com/grafana/grafana/pkg/services/secrets/manager" secretsMigrator "github.com/grafana/grafana/pkg/services/secrets/migrator" "github.com/grafana/grafana/pkg/services/serviceaccounts" @@ -318,10 +317,6 @@ var wireSet = wire.NewSet( publicdashboardsApi.ProvideApi, userimpl.ProvideService, orgimpl.ProvideService, - secretsMigrations.ProvideDataSourceMigrationService, - secretsMigrations.ProvideMigrateToPluginService, - secretsMigrations.ProvideSecretMigrationService, - wire.Bind(new(secretsMigrations.SecretMigrationService), new(*secretsMigrations.SecretMigrationServiceImpl)), userauthimpl.ProvideService, ngmetrics.ProvideServiceForTest, wire.Bind(new(sqlstore.TeamStore), new(*sqlstore.SQLStore)), diff --git a/pkg/server/backgroundsvcs/background_services.go b/pkg/server/backgroundsvcs/background_services.go index b1ec4f4ac2b..55a7ea3da3d 100644 --- a/pkg/server/backgroundsvcs/background_services.go +++ b/pkg/server/backgroundsvcs/background_services.go @@ -23,6 +23,7 @@ import ( "github.com/grafana/grafana/pkg/services/provisioning" "github.com/grafana/grafana/pkg/services/rendering" "github.com/grafana/grafana/pkg/services/searchV2" + secretsMigrations "github.com/grafana/grafana/pkg/services/secrets/kvstore/migrations" secretsManager "github.com/grafana/grafana/pkg/services/secrets/manager" "github.com/grafana/grafana/pkg/services/serviceaccounts" samanager "github.com/grafana/grafana/pkg/services/serviceaccounts/manager" @@ -42,6 +43,7 @@ func ProvideBackgroundServiceRegistry( secretsService *secretsManager.SecretsService, remoteCache *remotecache.RemoteCache, thumbnailsService thumbs.Service, StorageService store.StorageService, searchService searchV2.SearchService, entityEventsService store.EntityEventsService, saService *samanager.ServiceAccountsService, authInfoService *authinfoservice.Implementation, + secretMigrationProvider secretsMigrations.SecretMigrationProvider, // Need to make sure these are initialized, is there a better place to put them? _ dashboardsnapshots.Service, _ *alerting.AlertNotificationService, _ serviceaccounts.Service, _ *guardian.Provider, @@ -73,6 +75,7 @@ func ProvideBackgroundServiceRegistry( saService, authInfoService, processManager, + secretMigrationProvider, ) } diff --git a/pkg/server/server.go b/pkg/server/server.go index bd4500dd1bd..aec724df979 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -23,7 +23,6 @@ import ( "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/registry" "github.com/grafana/grafana/pkg/services/provisioning" - secretsMigrations "github.com/grafana/grafana/pkg/services/secrets/kvstore/migrations" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" @@ -44,10 +43,10 @@ type Options struct { func New(opts Options, cfg *setting.Cfg, httpServer *api.HTTPServer, roleRegistry accesscontrol.RoleRegistry, provisioningService provisioning.ProvisioningService, backgroundServiceProvider registry.BackgroundServiceRegistry, usageStatsProvidersRegistry registry.UsageStatsProvidersRegistry, statsCollectorService *statscollector.Service, - secretMigrationService secretsMigrations.SecretMigrationService, userService user.Service, loginAttemptService loginattempt.Service, + userService user.Service, loginAttemptService loginattempt.Service, ) (*Server, error) { statsCollectorService.RegisterProviders(usageStatsProvidersRegistry.GetServices()) - s, err := newServer(opts, cfg, httpServer, roleRegistry, provisioningService, backgroundServiceProvider, secretMigrationService, userService, loginAttemptService) + s, err := newServer(opts, cfg, httpServer, roleRegistry, provisioningService, backgroundServiceProvider, userService, loginAttemptService) if err != nil { return nil, err } @@ -60,30 +59,28 @@ func New(opts Options, cfg *setting.Cfg, httpServer *api.HTTPServer, roleRegistr } func newServer(opts Options, cfg *setting.Cfg, httpServer *api.HTTPServer, roleRegistry accesscontrol.RoleRegistry, - provisioningService provisioning.ProvisioningService, backgroundServiceProvider registry.BackgroundServiceRegistry, - secretMigrationService secretsMigrations.SecretMigrationService, userService user.Service, loginAttemptService loginattempt.Service, + provisioningService provisioning.ProvisioningService, backgroundServiceProvider registry.BackgroundServiceRegistry, userService user.Service, loginAttemptService loginattempt.Service, ) (*Server, error) { rootCtx, shutdownFn := context.WithCancel(context.Background()) childRoutines, childCtx := errgroup.WithContext(rootCtx) s := &Server{ - context: childCtx, - childRoutines: childRoutines, - HTTPServer: httpServer, - provisioningService: provisioningService, - roleRegistry: roleRegistry, - shutdownFn: shutdownFn, - shutdownFinished: make(chan struct{}), - log: log.New("server"), - cfg: cfg, - pidFile: opts.PidFile, - version: opts.Version, - commit: opts.Commit, - buildBranch: opts.BuildBranch, - backgroundServices: backgroundServiceProvider.GetServices(), - secretMigrationService: secretMigrationService, - userService: userService, - loginAttemptService: loginAttemptService, + context: childCtx, + childRoutines: childRoutines, + HTTPServer: httpServer, + provisioningService: provisioningService, + roleRegistry: roleRegistry, + shutdownFn: shutdownFn, + shutdownFinished: make(chan struct{}), + log: log.New("server"), + cfg: cfg, + pidFile: opts.PidFile, + version: opts.Version, + commit: opts.Commit, + buildBranch: opts.BuildBranch, + backgroundServices: backgroundServiceProvider.GetServices(), + userService: userService, + loginAttemptService: loginAttemptService, } return s, nil @@ -107,12 +104,11 @@ type Server struct { buildBranch string backgroundServices []registry.BackgroundService - HTTPServer *api.HTTPServer - roleRegistry accesscontrol.RoleRegistry - provisioningService provisioning.ProvisioningService - secretMigrationService secretsMigrations.SecretMigrationService - userService user.Service - loginAttemptService loginattempt.Service + HTTPServer *api.HTTPServer + roleRegistry accesscontrol.RoleRegistry + provisioningService provisioning.ProvisioningService + userService user.Service + loginAttemptService loginattempt.Service } // init initializes the server and its services. @@ -137,10 +133,6 @@ func (s *Server) init() error { return err } - if err := s.secretMigrationService.Migrate(s.context); err != nil { - return err - } - return s.provisioningService.RunInitProvisioners(s.context) } diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index e1a13d9053d..a11b3cc04a7 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -7,12 +7,9 @@ import ( "testing" "time" - "github.com/grafana/grafana/pkg/infra/serverlock" "github.com/grafana/grafana/pkg/registry" "github.com/grafana/grafana/pkg/server/backgroundsvcs" "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" - "github.com/grafana/grafana/pkg/services/secrets/kvstore/migrations" - "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/services/user/usertest" "github.com/grafana/grafana/pkg/setting" "github.com/stretchr/testify/require" @@ -51,11 +48,7 @@ func (s *testService) IsDisabled() bool { func testServer(t *testing.T, services ...registry.BackgroundService) *Server { t.Helper() - serverLockService := serverlock.ProvideService(sqlstore.InitTestDB(t)) - secretMigrationService := &migrations.SecretMigrationServiceImpl{ - ServerLockService: serverLockService, - } - s, err := newServer(Options{}, setting.NewCfg(), nil, &acimpl.Service{}, nil, backgroundsvcs.NewBackgroundServiceRegistry(services...), secretMigrationService, usertest.NewUserServiceFake(), nil) + s, err := newServer(Options{}, setting.NewCfg(), nil, &acimpl.Service{}, nil, backgroundsvcs.NewBackgroundServiceRegistry(services...), usertest.NewUserServiceFake(), nil) require.NoError(t, err) // Required to skip configuration initialization that causes // DI errors in this test. diff --git a/pkg/server/wire.go b/pkg/server/wire.go index f34d9a8a8b4..1a2a9953d1e 100644 --- a/pkg/server/wire.go +++ b/pkg/server/wire.go @@ -334,8 +334,8 @@ var wireBasicSet = wire.NewSet( secretsMigrations.ProvideDataSourceMigrationService, secretsMigrations.ProvideMigrateToPluginService, secretsMigrations.ProvideMigrateFromPluginService, - secretsMigrations.ProvideSecretMigrationService, - wire.Bind(new(secretsMigrations.SecretMigrationService), new(*secretsMigrations.SecretMigrationServiceImpl)), + secretsMigrations.ProvideSecretMigrationProvider, + wire.Bind(new(secretsMigrations.SecretMigrationProvider), new(*secretsMigrations.SecretMigrationProviderImpl)), userauthimpl.ProvideService, acimpl.ProvideAccessControl, wire.Bind(new(accesscontrol.AccessControl), new(*acimpl.AccessControl)), diff --git a/pkg/services/secrets/kvstore/migrations/migrator.go b/pkg/services/secrets/kvstore/migrations/migrator.go index 3bef598ce16..9ba1122b829 100644 --- a/pkg/services/secrets/kvstore/migrations/migrator.go +++ b/pkg/services/secrets/kvstore/migrations/migrator.go @@ -7,6 +7,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/serverlock" + "github.com/grafana/grafana/pkg/registry" "github.com/grafana/grafana/pkg/setting" ) @@ -19,20 +20,25 @@ type SecretMigrationService interface { Migrate(ctx context.Context) error } -type SecretMigrationServiceImpl struct { +type SecretMigrationProvider interface { + registry.BackgroundService + TriggerPluginMigration(ctx context.Context, toPlugin bool) error +} + +type SecretMigrationProviderImpl struct { services []SecretMigrationService ServerLockService *serverlock.ServerLockService migrateToPluginService *MigrateToPluginService migrateFromPluginService *MigrateFromPluginService } -func ProvideSecretMigrationService( +func ProvideSecretMigrationProvider( cfg *setting.Cfg, serverLockService *serverlock.ServerLockService, dataSourceSecretMigrationService *DataSourceSecretMigrationService, migrateToPluginService *MigrateToPluginService, migrateFromPluginService *MigrateFromPluginService, -) *SecretMigrationServiceImpl { +) *SecretMigrationProviderImpl { services := make([]SecretMigrationService, 0) services = append(services, dataSourceSecretMigrationService) // Plugin migration should always be last; should either migrate to or from, not both @@ -45,7 +51,7 @@ func ProvideSecretMigrationService( services = append(services, migrateToPluginService) } - return &SecretMigrationServiceImpl{ + return &SecretMigrationProviderImpl{ ServerLockService: serverLockService, services: services, migrateToPluginService: migrateToPluginService, @@ -53,9 +59,13 @@ func ProvideSecretMigrationService( } } +func (s *SecretMigrationProviderImpl) Run(ctx context.Context) error { + return s.Migrate(ctx) +} + // Migrate Run migration services. This will block until all services have exited. // This should only be called once at startup -func (s *SecretMigrationServiceImpl) Migrate(ctx context.Context) error { +func (s *SecretMigrationProviderImpl) Migrate(ctx context.Context) error { // Start migration services. return s.ServerLockService.LockExecuteAndRelease(ctx, actionName, time.Minute*10, func(context.Context) { for _, service := range s.services { @@ -71,7 +81,7 @@ func (s *SecretMigrationServiceImpl) Migrate(ctx context.Context) error { } // TriggerPluginMigration Kick off a migration to or from the plugin. This will block until all services have exited. -func (s *SecretMigrationServiceImpl) TriggerPluginMigration(ctx context.Context, toPlugin bool) error { +func (s *SecretMigrationProviderImpl) TriggerPluginMigration(ctx context.Context, toPlugin bool) error { // Don't migrate if there is already one happening return s.ServerLockService.LockExecuteAndRelease(ctx, actionName, time.Minute*10, func(context.Context) { var err error