mirror of https://github.com/grafana/grafana
ExtSvcAuth: Clean up orphaned external services on start up (#77951)
* Plugin: Remove external service on plugin removal * Early exit no service account * Add log * WIP * Cable OAuth2Server client removal * Move function lower * Add function to test removal * Add test to RemoveExternalService * Test RemoveExtSvcAccount * remove apostrophy in comment * Add cfg to plugin installer to check features * Add feature flag check in the service registration service * Comments * Move metrics Inc * Initialize map * Reorder * Initialize mutex as well * Add HasExternalService as suggested * WIP: CleanUpOrphanedExternalServices * Commit suggestion Co-authored-by: linoman <2051016+linoman@users.noreply.github.com> * Nit on test. Co-authored-by: linoman <2051016+linoman@users.noreply.github.com> * oauthserver return names * Name is not Slug * Use plugin ID not slug * Add background job * remove negation on feature check * Add test to the CleanUp function * Test GetExternalServiceNames * rename test * Add test for ExtSvcAccountsService_GetExternalServiceNames * Add a todo * Add todo * Option based on mix * Rewrite a bit the comment * Opinionated choice use slugs instead of names everywhere * Nit. * Comments and re-ordering * Comment * Add log * Add context --------- Co-authored-by: linoman <2051016+linoman@users.noreply.github.com>pull/78263/head
Gabriel MABILLE
2 years ago
committed by
GitHub
parent
6b6b209e1c
commit
ba717454e1
@ -0,0 +1,122 @@ |
||||
package registry |
||||
|
||||
import ( |
||||
"context" |
||||
"sync" |
||||
"testing" |
||||
|
||||
"github.com/grafana/grafana/pkg/infra/log" |
||||
"github.com/grafana/grafana/pkg/services/extsvcauth" |
||||
"github.com/grafana/grafana/pkg/services/extsvcauth/tests" |
||||
"github.com/grafana/grafana/pkg/services/featuremgmt" |
||||
"github.com/stretchr/testify/mock" |
||||
"github.com/stretchr/testify/require" |
||||
) |
||||
|
||||
type TestEnv struct { |
||||
r *Registry |
||||
oauthReg *tests.ExternalServiceRegistryMock |
||||
saReg *tests.ExternalServiceRegistryMock |
||||
} |
||||
|
||||
func setupTestEnv(t *testing.T) *TestEnv { |
||||
env := TestEnv{} |
||||
env.oauthReg = tests.NewExternalServiceRegistryMock(t) |
||||
env.saReg = tests.NewExternalServiceRegistryMock(t) |
||||
env.r = &Registry{ |
||||
features: featuremgmt.WithFeatures(featuremgmt.FlagExternalServiceAuth, featuremgmt.FlagExternalServiceAccounts), |
||||
logger: log.New("extsvcauth.registry.test"), |
||||
oauthReg: env.oauthReg, |
||||
saReg: env.saReg, |
||||
extSvcProviders: map[string]extsvcauth.AuthProvider{}, |
||||
lock: sync.Mutex{}, |
||||
} |
||||
return &env |
||||
} |
||||
|
||||
func TestRegistry_CleanUpOrphanedExternalServices(t *testing.T) { |
||||
tests := []struct { |
||||
name string |
||||
init func(*TestEnv) |
||||
}{ |
||||
{ |
||||
name: "should not clean up when every service registered", |
||||
init: func(te *TestEnv) { |
||||
// Have registered two services one requested a service account, the other requested to be an oauth client
|
||||
te.r.extSvcProviders = map[string]extsvcauth.AuthProvider{"sa-svc": extsvcauth.ServiceAccounts, "oauth-svc": extsvcauth.OAuth2Server} |
||||
|
||||
te.oauthReg.On("GetExternalServiceNames", mock.Anything).Return([]string{"oauth-svc"}, nil) |
||||
// Also return the external service account attached to the OAuth Server
|
||||
te.saReg.On("GetExternalServiceNames", mock.Anything).Return([]string{"sa-svc", "oauth-svc"}, nil) |
||||
}, |
||||
}, |
||||
{ |
||||
name: "should clean up an orphaned service account", |
||||
init: func(te *TestEnv) { |
||||
// Have registered two services one requested a service account, the other requested to be an oauth client
|
||||
te.r.extSvcProviders = map[string]extsvcauth.AuthProvider{"sa-svc": extsvcauth.ServiceAccounts, "oauth-svc": extsvcauth.OAuth2Server} |
||||
|
||||
te.oauthReg.On("GetExternalServiceNames", mock.Anything).Return([]string{"oauth-svc"}, nil) |
||||
// Also return the external service account attached to the OAuth Server
|
||||
te.saReg.On("GetExternalServiceNames", mock.Anything).Return([]string{"sa-svc", "orphaned-sa-svc", "oauth-svc"}, nil) |
||||
|
||||
te.saReg.On("RemoveExternalService", mock.Anything, "orphaned-sa-svc").Return(nil) |
||||
}, |
||||
}, |
||||
{ |
||||
name: "should clean up an orphaned OAuth Client", |
||||
init: func(te *TestEnv) { |
||||
// Have registered two services one requested a service account, the other requested to be an oauth client
|
||||
te.r.extSvcProviders = map[string]extsvcauth.AuthProvider{"sa-svc": extsvcauth.ServiceAccounts, "oauth-svc": extsvcauth.OAuth2Server} |
||||
|
||||
te.oauthReg.On("GetExternalServiceNames", mock.Anything).Return([]string{"oauth-svc", "orphaned-oauth-svc"}, nil) |
||||
// Also return the external service account attached to the OAuth Server
|
||||
te.saReg.On("GetExternalServiceNames", mock.Anything).Return([]string{"sa-svc", "orphaned-oauth-svc", "oauth-svc"}, nil) |
||||
|
||||
te.oauthReg.On("RemoveExternalService", mock.Anything, "orphaned-oauth-svc").Return(nil) |
||||
}, |
||||
}, |
||||
} |
||||
for _, tt := range tests { |
||||
t.Run(tt.name, func(t *testing.T) { |
||||
env := setupTestEnv(t) |
||||
tt.init(env) |
||||
|
||||
err := env.r.CleanUpOrphanedExternalServices(context.Background()) |
||||
require.NoError(t, err) |
||||
|
||||
env.oauthReg.AssertExpectations(t) |
||||
env.saReg.AssertExpectations(t) |
||||
}) |
||||
} |
||||
} |
||||
|
||||
func TestRegistry_GetExternalServiceNames(t *testing.T) { |
||||
tests := []struct { |
||||
name string |
||||
init func(*TestEnv) |
||||
want []string |
||||
}{ |
||||
{ |
||||
name: "should deduplicate names", |
||||
init: func(te *TestEnv) { |
||||
te.saReg.On("GetExternalServiceNames", mock.Anything).Return([]string{"sa-svc", "oauth-svc"}, nil) |
||||
te.oauthReg.On("GetExternalServiceNames", mock.Anything).Return([]string{"oauth-svc"}, nil) |
||||
}, |
||||
want: []string{"sa-svc", "oauth-svc"}, |
||||
}, |
||||
} |
||||
for _, tt := range tests { |
||||
t.Run(tt.name, func(t *testing.T) { |
||||
env := setupTestEnv(t) |
||||
tt.init(env) |
||||
|
||||
names, err := env.r.GetExternalServiceNames(context.Background()) |
||||
require.NoError(t, err) |
||||
require.EqualValues(t, tt.want, names) |
||||
|
||||
env.oauthReg.AssertExpectations(t) |
||||
env.saReg.AssertExpectations(t) |
||||
}) |
||||
} |
||||
} |
||||
@ -0,0 +1,119 @@ |
||||
// Code generated by mockery v2.35.2. DO NOT EDIT.
|
||||
|
||||
package tests |
||||
|
||||
import ( |
||||
context "context" |
||||
|
||||
extsvcauth "github.com/grafana/grafana/pkg/services/extsvcauth" |
||||
mock "github.com/stretchr/testify/mock" |
||||
) |
||||
|
||||
// ExternalServiceRegistryMock is an autogenerated mock type for the ExternalServiceRegistry type
|
||||
type ExternalServiceRegistryMock struct { |
||||
mock.Mock |
||||
} |
||||
|
||||
// GetExternalServiceNames provides a mock function with given fields: ctx
|
||||
func (_m *ExternalServiceRegistryMock) GetExternalServiceNames(ctx context.Context) ([]string, error) { |
||||
ret := _m.Called(ctx) |
||||
|
||||
var r0 []string |
||||
var r1 error |
||||
if rf, ok := ret.Get(0).(func(context.Context) ([]string, error)); ok { |
||||
return rf(ctx) |
||||
} |
||||
if rf, ok := ret.Get(0).(func(context.Context) []string); ok { |
||||
r0 = rf(ctx) |
||||
} else { |
||||
if ret.Get(0) != nil { |
||||
r0 = ret.Get(0).([]string) |
||||
} |
||||
} |
||||
|
||||
if rf, ok := ret.Get(1).(func(context.Context) error); ok { |
||||
r1 = rf(ctx) |
||||
} else { |
||||
r1 = ret.Error(1) |
||||
} |
||||
|
||||
return r0, r1 |
||||
} |
||||
|
||||
// HasExternalService provides a mock function with given fields: ctx, name
|
||||
func (_m *ExternalServiceRegistryMock) HasExternalService(ctx context.Context, name string) (bool, error) { |
||||
ret := _m.Called(ctx, name) |
||||
|
||||
var r0 bool |
||||
var r1 error |
||||
if rf, ok := ret.Get(0).(func(context.Context, string) (bool, error)); ok { |
||||
return rf(ctx, name) |
||||
} |
||||
if rf, ok := ret.Get(0).(func(context.Context, string) bool); ok { |
||||
r0 = rf(ctx, name) |
||||
} else { |
||||
r0 = ret.Get(0).(bool) |
||||
} |
||||
|
||||
if rf, ok := ret.Get(1).(func(context.Context, string) error); ok { |
||||
r1 = rf(ctx, name) |
||||
} else { |
||||
r1 = ret.Error(1) |
||||
} |
||||
|
||||
return r0, r1 |
||||
} |
||||
|
||||
// RemoveExternalService provides a mock function with given fields: ctx, name
|
||||
func (_m *ExternalServiceRegistryMock) RemoveExternalService(ctx context.Context, name string) error { |
||||
ret := _m.Called(ctx, name) |
||||
|
||||
var r0 error |
||||
if rf, ok := ret.Get(0).(func(context.Context, string) error); ok { |
||||
r0 = rf(ctx, name) |
||||
} else { |
||||
r0 = ret.Error(0) |
||||
} |
||||
|
||||
return r0 |
||||
} |
||||
|
||||
// SaveExternalService provides a mock function with given fields: ctx, cmd
|
||||
func (_m *ExternalServiceRegistryMock) SaveExternalService(ctx context.Context, cmd *extsvcauth.ExternalServiceRegistration) (*extsvcauth.ExternalService, error) { |
||||
ret := _m.Called(ctx, cmd) |
||||
|
||||
var r0 *extsvcauth.ExternalService |
||||
var r1 error |
||||
if rf, ok := ret.Get(0).(func(context.Context, *extsvcauth.ExternalServiceRegistration) (*extsvcauth.ExternalService, error)); ok { |
||||
return rf(ctx, cmd) |
||||
} |
||||
if rf, ok := ret.Get(0).(func(context.Context, *extsvcauth.ExternalServiceRegistration) *extsvcauth.ExternalService); ok { |
||||
r0 = rf(ctx, cmd) |
||||
} else { |
||||
if ret.Get(0) != nil { |
||||
r0 = ret.Get(0).(*extsvcauth.ExternalService) |
||||
} |
||||
} |
||||
|
||||
if rf, ok := ret.Get(1).(func(context.Context, *extsvcauth.ExternalServiceRegistration) error); ok { |
||||
r1 = rf(ctx, cmd) |
||||
} else { |
||||
r1 = ret.Error(1) |
||||
} |
||||
|
||||
return r0, r1 |
||||
} |
||||
|
||||
// NewExternalServiceRegistryMock creates a new instance of ExternalServiceRegistryMock. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations.
|
||||
// The first argument is typically a *testing.T value.
|
||||
func NewExternalServiceRegistryMock(t interface { |
||||
mock.TestingT |
||||
Cleanup(func()) |
||||
}) *ExternalServiceRegistryMock { |
||||
mock := &ExternalServiceRegistryMock{} |
||||
mock.Mock.Test(t) |
||||
|
||||
t.Cleanup(func() { mock.AssertExpectations(t) }) |
||||
|
||||
return mock |
||||
} |
||||
Loading…
Reference in new issue