diff --git a/pkg/services/extsvcauth/registry/service.go b/pkg/services/extsvcauth/registry/service.go index 1c07c4011b9..d12528e7660 100644 --- a/pkg/services/extsvcauth/registry/service.go +++ b/pkg/services/extsvcauth/registry/service.go @@ -22,6 +22,10 @@ var lockTimeConfig = serverlock.LockTimeConfig{ MaxWait: 5 * time.Second, } +type serverLocker interface { + LockExecuteAndReleaseWithRetries(context.Context, string, serverlock.LockTimeConfig, func(ctx context.Context), ...serverlock.RetryOpt) error +} + type Registry struct { features featuremgmt.FeatureToggles logger log.Logger @@ -30,7 +34,7 @@ type Registry struct { extSvcProviders map[string]extsvcauth.AuthProvider lock sync.Mutex - serverLock *serverlock.ServerLockService + serverLock serverLocker } func ProvideExtSvcRegistry(oauthServer *oasimpl.OAuth2ServiceImpl, saSvc *extsvcaccounts.ExtSvcAccountsService, serverLock *serverlock.ServerLockService, features featuremgmt.FeatureToggles) *Registry { @@ -47,28 +51,39 @@ func ProvideExtSvcRegistry(oauthServer *oasimpl.OAuth2ServiceImpl, saSvc *extsvc // CleanUpOrphanedExternalServices remove external services present in store that have not been registered on startup. func (r *Registry) CleanUpOrphanedExternalServices(ctx context.Context) error { - extsvcs, err := r.retrieveExtSvcProviders(ctx) - if err != nil { - r.logger.Error("Could not retrieve external services from store", "error", err.Error()) - return err - } - for name, provider := range extsvcs { - // The service did not register this time. Removed. - if _, ok := r.extSvcProviders[slugify.Slugify(name)]; !ok { - r.logger.Info("Detected removed External Service", "service", name, "provider", provider) - switch provider { - case extsvcauth.ServiceAccounts: - if err := r.saReg.RemoveExternalService(ctx, name); err != nil { - return err - } - case extsvcauth.OAuth2Server: - if err := r.oauthReg.RemoveExternalService(ctx, name); err != nil { - return err + var errCleanUp error + + errLock := r.serverLock.LockExecuteAndReleaseWithRetries(ctx, "ext-svc-clean-up", lockTimeConfig, func(ctx context.Context) { + extsvcs, err := r.retrieveExtSvcProviders(ctx) + if err != nil { + r.logger.Error("Could not retrieve external services from store", "error", err.Error()) + errCleanUp = err + return + } + for name, provider := range extsvcs { + // The service did not register this time. Removed. + if _, ok := r.extSvcProviders[slugify.Slugify(name)]; !ok { + r.logger.Info("Detected removed External Service", "service", name, "provider", provider) + switch provider { + case extsvcauth.ServiceAccounts: + if err := r.saReg.RemoveExternalService(ctx, name); err != nil { + errCleanUp = err + return + } + case extsvcauth.OAuth2Server: + if err := r.oauthReg.RemoveExternalService(ctx, name); err != nil { + errCleanUp = err + return + } } } } + }) + if errLock != nil { + return errLock } - return nil + + return errCleanUp } // HasExternalService returns whether an external service has been saved with that name. diff --git a/pkg/services/extsvcauth/registry/service_test.go b/pkg/services/extsvcauth/registry/service_test.go index d09e0c4477a..f43f0b327f0 100644 --- a/pkg/services/extsvcauth/registry/service_test.go +++ b/pkg/services/extsvcauth/registry/service_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/infra/serverlock" "github.com/grafana/grafana/pkg/services/extsvcauth" "github.com/grafana/grafana/pkg/services/extsvcauth/tests" "github.com/grafana/grafana/pkg/services/featuremgmt" @@ -18,6 +19,14 @@ type TestEnv struct { saReg *tests.ExternalServiceRegistryMock } +// Never lock in tests +type fakeServerLock struct{} + +func (f *fakeServerLock) LockExecuteAndReleaseWithRetries(ctx context.Context, actionName string, timeConfig serverlock.LockTimeConfig, fn func(ctx context.Context), retryOpts ...serverlock.RetryOpt) error { + fn(ctx) + return nil +} + func setupTestEnv(t *testing.T) *TestEnv { env := TestEnv{} env.oauthReg = tests.NewExternalServiceRegistryMock(t) @@ -28,6 +37,7 @@ func setupTestEnv(t *testing.T) *TestEnv { oauthReg: env.oauthReg, saReg: env.saReg, extSvcProviders: map[string]extsvcauth.AuthProvider{}, + serverLock: &fakeServerLock{}, } return &env }