diff --git a/pkg/login/social/socialimpl/service_test.go b/pkg/login/social/socialimpl/service_test.go index 9beeae7abd3..9dc51e9448e 100644 --- a/pkg/login/social/socialimpl/service_test.go +++ b/pkg/login/social/socialimpl/service_test.go @@ -73,7 +73,7 @@ func TestSocialService_ProvideService(t *testing.T) { accessControl := acimpl.ProvideAccessControl(cfg) sqlStore := db.InitTestDB(t) - ssoSettingsSvc := ssosettingsimpl.ProvideService(cfg, sqlStore, accessControl, routing.NewRouteRegister(), featuremgmt.WithFeatures(), secrets, &usagestats.UsageStatsMock{}) + ssoSettingsSvc := ssosettingsimpl.ProvideService(cfg, sqlStore, accessControl, routing.NewRouteRegister(), featuremgmt.WithFeatures(), secrets, &usagestats.UsageStatsMock{}, nil) for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { diff --git a/pkg/services/ssosettings/ssosettingsimpl/metrics.go b/pkg/services/ssosettings/ssosettingsimpl/metrics.go new file mode 100644 index 00000000000..229abf9d431 --- /dev/null +++ b/pkg/services/ssosettings/ssosettingsimpl/metrics.go @@ -0,0 +1,31 @@ +package ssosettingsimpl + +import "github.com/prometheus/client_golang/prometheus" + +const ( + metricsNamespace = "grafana" + metricsSubSystem = "ssosettings" +) + +type metrics struct { + reloadFailures *prometheus.CounterVec +} + +func newMetrics(reg prometheus.Registerer) *metrics { + m := &metrics{ + reloadFailures: prometheus.NewCounterVec(prometheus.CounterOpts{ + Namespace: metricsNamespace, + Subsystem: metricsSubSystem, + Name: "setting_reload_failures_total", + Help: "Number of SSO Setting reload failures.", + }, []string{"provider"}), + } + + if reg != nil { + reg.MustRegister( + m.reloadFailures, + ) + } + + return m +} diff --git a/pkg/services/ssosettings/ssosettingsimpl/service.go b/pkg/services/ssosettings/ssosettingsimpl/service.go index 894725efef9..16047c3ab67 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service.go +++ b/pkg/services/ssosettings/ssosettingsimpl/service.go @@ -21,6 +21,7 @@ import ( "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/strategies" "github.com/grafana/grafana/pkg/setting" + "github.com/prometheus/client_golang/prometheus" ) var _ ssosettings.Service = (*Service)(nil) @@ -31,6 +32,7 @@ type Service struct { store ssosettings.Store ac ac.AccessControl secrets secrets.Service + metrics *metrics fbStrategies []ssosettings.FallbackStrategy reloadables map[string]ssosettings.Reloadable @@ -38,7 +40,7 @@ type Service struct { func ProvideService(cfg *setting.Cfg, sqlStore db.DB, ac ac.AccessControl, routeRegister routing.RouteRegister, features featuremgmt.FeatureToggles, - secrets secrets.Service, usageStats usagestats.Service) *Service { + secrets secrets.Service, usageStats usagestats.Service, registerer prometheus.Registerer) *Service { strategies := []ssosettings.FallbackStrategy{ strategies.NewOAuthStrategy(cfg), // register other strategies here, for example SAML @@ -53,6 +55,7 @@ func ProvideService(cfg *setting.Cfg, sqlStore db.DB, ac ac.AccessControl, ac: ac, fbStrategies: strategies, secrets: secrets, + metrics: newMetrics(registerer), reloadables: make(map[string]ssosettings.Reloadable), } @@ -193,13 +196,9 @@ func (s *Service) Upsert(ctx context.Context, settings *models.SSOSettings) erro return err } - go func() { - settings.Settings = overrideMaps(storedSettings.Settings, settings.Settings, secrets) - err = social.Reload(context.Background(), *settings) - if err != nil { - s.logger.Error("failed to reload the provider", "provider", settings.Provider, "error", err) - } - }() + settings.Settings = overrideMaps(storedSettings.Settings, settings.Settings, secrets) + + go s.reload(social, settings.Provider, *settings) return nil } @@ -212,7 +211,34 @@ func (s *Service) Delete(ctx context.Context, provider string) error { if !s.isProviderConfigurable(provider) { return ssosettings.ErrNotConfigurable } - return s.store.Delete(ctx, provider) + + social, ok := s.reloadables[provider] + if !ok { + return ssosettings.ErrInvalidProvider.Errorf("provider %s not found in reloadables", provider) + } + + err := s.store.Delete(ctx, provider) + if err != nil { + return err + } + + currentSettings, err := s.GetForProvider(ctx, provider) + if err != nil { + s.logger.Error("failed to get current settings, skipping reload", "provider", provider, "error", err) + return nil + } + + go s.reload(social, provider, *currentSettings) + + return nil +} + +func (s *Service) reload(reloadable ssosettings.Reloadable, provider string, currentSettings models.SSOSettings) { + err := reloadable.Reload(context.Background(), currentSettings) + if err != nil { + s.metrics.reloadFailures.WithLabelValues(provider).Inc() + s.logger.Error("failed to reload the provider", "provider", provider, "error", err) + } } func (s *Service) Reload(ctx context.Context, provider string) { @@ -327,6 +353,7 @@ func (s *Service) doReload(ctx context.Context) { err = connector.Reload(ctx, *setting) if err != nil { + s.metrics.reloadFailures.WithLabelValues(provider).Inc() s.logger.Error("failed to reload SSO Settings", "provider", provider, "err", err) continue } diff --git a/pkg/services/ssosettings/ssosettingsimpl/service_test.go b/pkg/services/ssosettings/ssosettingsimpl/service_test.go index 7537dbe369c..b29e2a509d8 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service_test.go +++ b/pkg/services/ssosettings/ssosettingsimpl/service_test.go @@ -10,6 +10,7 @@ import ( "testing" "time" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -1007,17 +1008,45 @@ func TestService_Delete(t *testing.T) { t.Run("successfully delete SSO settings", func(t *testing.T) { env := setupTestEnv(t) + var wg sync.WaitGroup + wg.Add(1) + provider := social.AzureADProviderName - env.store.ExpectedError = nil + reloadable := ssosettingstests.NewMockReloadable(t) + env.reloadables[provider] = reloadable + + env.fallbackStrategy.ExpectedConfigs = map[string]map[string]any{ + provider: { + "client_id": "client-id", + "client_secret": "client-secret", + "enabled": true, + }, + } + + reloadable.On("Reload", mock.Anything, mock.MatchedBy(func(settings models.SSOSettings) bool { + wg.Done() + return settings.Provider == provider && + settings.ID == "" && + maps.Equal(settings.Settings, map[string]any{ + "client_id": "client-id", + "client_secret": "client-secret", + "enabled": true, + }) + })).Return(nil).Once() err := env.service.Delete(context.Background(), provider) require.NoError(t, err) + + // wait for the goroutine first to assert the Reload call + wg.Wait() }) - t.Run("SSO settings not found for the specified provider", func(t *testing.T) { + t.Run("return error if SSO setting was not found for the specified provider", func(t *testing.T) { env := setupTestEnv(t) provider := social.AzureADProviderName + reloadable := ssosettingstests.NewMockReloadable(t) + env.reloadables[provider] = reloadable env.store.ExpectedError = ssosettings.ErrNotFound err := env.service.Delete(context.Background(), provider) @@ -1036,7 +1065,7 @@ func TestService_Delete(t *testing.T) { require.ErrorIs(t, err, ssosettings.ErrNotConfigurable) }) - t.Run("store fails to delete the SSO settings for the specified provider", func(t *testing.T) { + t.Run("return error when store fails to delete the SSO settings for the specified provider", func(t *testing.T) { env := setupTestEnv(t) provider := social.AzureADProviderName @@ -1046,6 +1075,22 @@ func TestService_Delete(t *testing.T) { require.Error(t, err) require.NotErrorIs(t, err, ssosettings.ErrNotFound) }) + + t.Run("return successfully when the deletion was successful but reloading the settings fail", func(t *testing.T) { + env := setupTestEnv(t) + + provider := social.AzureADProviderName + reloadable := ssosettingstests.NewMockReloadable(t) + env.reloadables[provider] = reloadable + + env.store.GetFn = func(ctx context.Context, provider string) (*models.SSOSettings, error) { + return nil, errors.New("failed to get sso settings") + } + + err := env.service.Delete(context.Background(), provider) + + require.NoError(t, err) + }) } func TestService_DoReload(t *testing.T) { @@ -1223,6 +1268,7 @@ func setupTestEnv(t *testing.T) testEnv { ac: accessControl, fbStrategies: []ssosettings.FallbackStrategy{fallbackStrategy}, reloadables: reloadables, + metrics: newMetrics(prometheus.NewRegistry()), secrets: secrets, }