Lock when cleaning-up external services (#78589)

This commit is contained in:
Xavi Lacasa 2023-11-24 17:44:14 +01:00 committed by GitHub
parent 4b439b7f52
commit 29853f624e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 44 additions and 19 deletions

View File

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

View File

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