Secrets: Move license check to secret store plugin (#53185)

* Move license check to secret store plugin

* Use shouldUseRemoteSecretsPlugin for migration

* Encapsulate plugin startup logic in sync.Once

* Remove global logger from startup functions

* Fix issues with wire and secrets plugin check

* Remove todo for plugin fatal error

* Rename fatalErr variable to be less confusing

* Fix merge conflicts

* Fix issue with grafana-cli wire and opentsdb

* Remove duplicated import on remote plugin

* Rename plugin check in favor of error return value

* Remove unnecessary import on grafana-cli wireexts_oss

* Remove unnecessary import on grafana wireexts_oss

* Reset sync.Once during test setup

* Remove unrelated opentsdb change on grafana-cli wire

* Readd opentsdb change on grafana-cli wire

pkg/services/secrets/kvstore/kvstore.go

@@ -2,15 +2,16 @@ package kvstore
 
 import (
 	"context"
-	"errors"
-	"fmt"
 	"time"
 
 	"github.com/grafana/grafana/pkg/infra/kvstore"
 	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/plugins"
+	"github.com/grafana/grafana/pkg/plugins/backendplugin/secretsmanagerplugin"
 	"github.com/grafana/grafana/pkg/services/featuremgmt"
 	"github.com/grafana/grafana/pkg/services/secrets"
 	"github.com/grafana/grafana/pkg/services/sqlstore"
+	"github.com/grafana/grafana/pkg/setting"
 )
 
 const (
@@ -21,12 +22,13 @@ const (
 func ProvideService(
 	sqlStore sqlstore.Store,
 	secretsService secrets.Service,
-	remoteCheck UseRemoteSecretsPluginCheck,
+	pluginsManager plugins.SecretsPluginManager,
 	kvstore kvstore.KVStore,
 	features featuremgmt.FeatureToggles,
+	cfg *setting.Cfg,
 ) (SecretsKVStore, error) {
+	var logger = log.New("secrets.kvstore")
 	var store SecretsKVStore
-	logger := log.New("secrets.kvstore")
 	store = &secretsKVStoreSQL{
 		sqlStore:       sqlStore,
 		secretsService: secretsService,
@@ -35,21 +37,24 @@ func ProvideService(
 			cache: make(map[int64]cachedDecrypted),
 		},
 	}
-	if remoteCheck.ShouldUseRemoteSecretsPlugin() {
+	err := EvaluateRemoteSecretsPlugin(pluginsManager, cfg)
+	if err != nil {
+		logger.Debug(err.Error())
+	} else {
 		// Attempt to start the plugin
-		secretsPlugin, err := remoteCheck.StartAndReturnPlugin(context.Background())
+		var secretsPlugin secretsmanagerplugin.SecretsManagerPlugin
+		secretsPlugin, err = startAndReturnPlugin(pluginsManager, context.Background())
 		namespacedKVStore := GetNamespacedKVStore(kvstore)
 		if err != nil || secretsPlugin == nil {
-			if isFatal, err2 := isPluginStartupErrorFatal(context.Background(), namespacedKVStore); isFatal || err2 != nil {
+			logger.Error("failed to start remote secrets management plugin", "msg", err.Error())
+			if isFatal, readErr := isPluginStartupErrorFatal(context.Background(), namespacedKVStore); isFatal || readErr != nil {
 				// plugin error was fatal or there was an error determining if the error was fatal
 				logger.Error("secrets management plugin is required to start -- exiting app")
-				if err2 != nil {
-					// TODO decide whether an error here should actually crash the app
-					return nil, err2
+				if readErr != nil {
+					return nil, readErr
 				}
 				return nil, err
 			}
-			logger.Error("error starting secrets plugin, falling back to SQL implementation")
 		} else {
 			store = &secretsKVStorePlugin{
 				secretsPlugin:                  secretsPlugin,
@@ -59,7 +64,9 @@ func ProvideService(
 				backwardsCompatibilityDisabled: features.IsEnabled(featuremgmt.FlagDisableSecretsCompatibility),
 			}
 		}
-	} else {
+	}
+
+	if err != nil {
 		logger.Debug("secrets kvstore is using the default (SQL) implementation for secrets management")
 	}
 
@@ -117,23 +124,3 @@ func (kv *FixedKVStore) Rename(ctx context.Context, newNamespace string) error {
 	kv.Namespace = newNamespace
 	return nil
 }
-
-// Helpers to determine whether plugin startup failures are fatal to the app
-func GetNamespacedKVStore(kv kvstore.KVStore) *kvstore.NamespacedKVStore {
-	return kvstore.WithNamespace(kv, kvstore.AllOrganizations, PluginNamespace)
-}
-
-func isPluginStartupErrorFatal(ctx context.Context, kvstore *kvstore.NamespacedKVStore) (bool, error) {
-	_, exists, err := kvstore.Get(ctx, QuitOnPluginStartupFailureKey)
-	if err != nil {
-		return false, errors.New(fmt.Sprint("error retrieving key ", QuitOnPluginStartupFailureKey, " from kvstore. error: ", err.Error()))
-	}
-	return exists, nil
-}
-
-func setPluginStartupErrorFatal(ctx context.Context, kvstore *kvstore.NamespacedKVStore, isFatal bool) error {
-	if !isFatal {
-		return kvstore.Del(ctx, QuitOnPluginStartupFailureKey)
-	}
-	return kvstore.Set(ctx, QuitOnPluginStartupFailureKey, "true")
-}

pkg/services/secrets/kvstore/plugin_fatal_crash_test.go

@@ -80,25 +80,23 @@ func TestFatalPluginErr_MigrationTestWithErrorDeletingUnifiedSecrets(t *testing.
 
 func setupFatalCrashTest(
 	t *testing.T,
-	shouldRemoteCheckError bool,
+	shouldFailOnStart bool,
 	isPluginErrorFatal bool,
 	isBackwardsCompatDisabled bool,
 ) (SecretsKVStore, kvstore.KVStore, *sqlstore.SQLStore, error) {
 	t.Helper()
+	fatalFlagOnce = sync.Once{}
+	startupOnce = sync.Once{}
+	cfg := setupTestConfig(t)
 	sqlStore := sqlstore.InitTestDB(t)
 	secretService := fakes.FakeSecretsService{}
-	var remoteCheck *mockRemoteSecretsPluginCheck
-	if shouldRemoteCheckError {
-		remoteCheck = provideMockRemotePluginCheckWithErr()
-	} else {
-		remoteCheck = provideMockRemotePluginCheck()
-	}
 	kvstore := kvstore.ProvideService(sqlStore)
 	if isPluginErrorFatal {
 		_ = setPluginStartupErrorFatal(context.Background(), GetNamespacedKVStore(kvstore), true)
 	}
 	features := NewFakeFeatureToggles(t, isBackwardsCompatDisabled)
-	svc, err := ProvideService(sqlStore, secretService, remoteCheck, kvstore, features)
+	manager := NewFakeSecretsPluginManager(t, shouldFailOnStart)
+	svc, err := ProvideService(sqlStore, secretService, manager, kvstore, features, cfg)
 	t.Cleanup(func() {
 		fatalFlagOnce = sync.Once{}
 	})
@@ -112,15 +110,9 @@ func setupTestMigratorServiceWithDeletionError(
 	kvstore kvstore.KVStore,
 ) *PluginSecretMigrationService {
 	t.Helper()
-	rawCfg := `
-		[secrets]
-		use_plugin = true
-		migrate_to_plugin = true
-		`
-	raw, err := ini.Load([]byte(rawCfg))
-	require.NoError(t, err)
-	cfg := &setting.Cfg{Raw: raw}
-	remoteCheck := provideMockRemotePluginCheck()
+	fatalFlagOnce = sync.Once{}
+	startupOnce = sync.Once{}
+	cfg := setupTestConfig(t)
 	secretsService := secretsManager.SetupTestService(t, fakes.NewFakeSecretsStore())
 	getAllFuncOverride := func(ctx context.Context) ([]Item, error) {
 		items := make([]Item, 0)
@@ -135,15 +127,28 @@ func setupTestMigratorServiceWithDeletionError(
 		})
 		return items, nil
 	}
+	manager := NewFakeSecretsPluginManager(t, false)
 	migratorService := ProvidePluginSecretMigrationService(
 		secretskv,
 		cfg,
 		sqlStore,
 		secretsService,
-		remoteCheck,
 		kvstore,
+		manager,
 	)
 	// TODO refactor Migrator to allow us to override the entire sqlstore with a mock instead
 	migratorService.overrideGetAllFunc(getAllFuncOverride)
 	return migratorService
 }
+
+func setupTestConfig(t *testing.T) *setting.Cfg {
+	t.Helper()
+	rawCfg := `
+		[secrets]
+		use_plugin = true
+		migrate_to_plugin = true
+		`
+	raw, err := ini.Load([]byte(rawCfg))
+	require.NoError(t, err)
+	return &setting.Cfg{Raw: raw}
+}

pkg/services/secrets/kvstore/plugin_mig.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/grafana/grafana/pkg/infra/kvstore"
 	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/plugins"
 	"github.com/grafana/grafana/pkg/services/secrets"
 	"github.com/grafana/grafana/pkg/services/sqlstore"
 	"github.com/grafana/grafana/pkg/setting"
@@ -19,8 +20,8 @@ type PluginSecretMigrationService struct {
 	logger         log.Logger
 	sqlStore       sqlstore.Store
 	secretsService secrets.Service
-	remoteCheck    UseRemoteSecretsPluginCheck
 	kvstore        kvstore.KVStore
+	manager        plugins.SecretsPluginManager
 	getAllFunc     func(ctx context.Context) ([]Item, error)
 }
 
@@ -29,8 +30,8 @@ func ProvidePluginSecretMigrationService(
 	cfg *setting.Cfg,
 	sqlStore sqlstore.Store,
 	secretsService secrets.Service,
-	remoteCheck UseRemoteSecretsPluginCheck,
 	kvstore kvstore.KVStore,
+	manager plugins.SecretsPluginManager,
 ) *PluginSecretMigrationService {
 	return &PluginSecretMigrationService{
 		secretsStore:   secretsStore,
@@ -38,14 +39,14 @@ func ProvidePluginSecretMigrationService(
 		logger:         log.New("sec-plugin-mig"),
 		sqlStore:       sqlStore,
 		secretsService: secretsService,
-		remoteCheck:    remoteCheck,
 		kvstore:        kvstore,
+		manager:        manager,
 	}
 }
 
 func (s *PluginSecretMigrationService) Migrate(ctx context.Context) error {
 	// Check if we should migrate to plugin - default false
-	if s.cfg.SectionWithEnvOverrides("secrets").Key("migrate_to_plugin").MustBool(false) && s.remoteCheck.ShouldUseRemoteSecretsPlugin() {
+	if err := EvaluateRemoteSecretsPlugin(s.manager, s.cfg); err == nil {
 		s.logger.Debug("starting migration of unified secrets to the plugin")
 		// we need to instantiate the secretsKVStore as this is not on wire, and in this scenario,
 		// the secrets store would be the plugin.

pkg/services/secrets/kvstore/plugin_mig_test.go

@@ -77,19 +77,18 @@ func setupTestMigratorService(t *testing.T) (*PluginSecretMigrationService, Secr
 	cfg := &setting.Cfg{Raw: raw}
 	// this would be the plugin - mocked at the moment
 	secretsStoreForPlugin := NewFakeSecretsKVStore()
-	// Mocked remote plugin check, always return true
-	remoteCheck := provideMockRemotePluginCheck()
 
 	// this is to init the sql secret store inside the migration
 	sqlStore := sqlstore.InitTestDB(t)
 	secretsService := secretsManager.SetupTestService(t, fakes.NewFakeSecretsStore())
+	manager := NewFakeSecretsPluginManager(t, false)
 	migratorService := ProvidePluginSecretMigrationService(
 		secretsStoreForPlugin,
 		cfg,
 		sqlStore,
 		secretsService,
-		remoteCheck,
 		kvstore.ProvideService(sqlStore),
+		manager,
 	)
 
 	secretsSql := &secretsKVStoreSQL{

pkg/services/secrets/kvstore/remote_plugin.go

@@ -2,17 +2,24 @@ package kvstore
 
 import (
 	"context"
+	"errors"
+	"fmt"
 	"sync"
 
 	"github.com/grafana/grafana/pkg/infra/kvstore"
 	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/plugins"
 	smp "github.com/grafana/grafana/pkg/plugins/backendplugin/secretsmanagerplugin"
 	"github.com/grafana/grafana/pkg/services/datasources"
 	"github.com/grafana/grafana/pkg/services/secrets"
+	"github.com/grafana/grafana/pkg/setting"
 )
 
 var (
-	fatalFlagOnce sync.Once
+	fatalFlagOnce             sync.Once
+	startupOnce               sync.Once
+	errPluginDisabledByConfig = errors.New("remote secret managements plugin disabled because the property `secrets.use_plugin` is not set to `true`")
+	errPluginNotInstalled     = errors.New("remote secret managements plugin disabled because there is no installed plugin of type `secretsmanager`")
 )
 
 // secretsKVStorePlugin provides a key/value store backed by the Grafana plugin gRPC interface
@@ -163,3 +170,45 @@ func updateFatalFlag(ctx context.Context, skv secretsKVStorePlugin) {
 func wrapUserFriendlySecretError(ufe string) datasources.ErrDatasourceSecretsPluginUserFriendly {
 	return datasources.ErrDatasourceSecretsPluginUserFriendly{Err: ufe}
 }
+
+func GetNamespacedKVStore(kv kvstore.KVStore) *kvstore.NamespacedKVStore {
+	return kvstore.WithNamespace(kv, kvstore.AllOrganizations, PluginNamespace)
+}
+
+func isPluginStartupErrorFatal(ctx context.Context, kvstore *kvstore.NamespacedKVStore) (bool, error) {
+	_, exists, err := kvstore.Get(ctx, QuitOnPluginStartupFailureKey)
+	if err != nil {
+		return false, errors.New(fmt.Sprint("error retrieving key ", QuitOnPluginStartupFailureKey, " from kvstore. error: ", err.Error()))
+	}
+	return exists, nil
+}
+
+func setPluginStartupErrorFatal(ctx context.Context, kvstore *kvstore.NamespacedKVStore, isFatal bool) error {
+	if !isFatal {
+		return kvstore.Del(ctx, QuitOnPluginStartupFailureKey)
+	}
+	return kvstore.Set(ctx, QuitOnPluginStartupFailureKey, "true")
+}
+
+func EvaluateRemoteSecretsPlugin(mg plugins.SecretsPluginManager, cfg *setting.Cfg) error {
+	usePlugin := cfg.SectionWithEnvOverrides("secrets").Key("use_plugin").MustBool()
+	if !usePlugin {
+		return errPluginDisabledByConfig
+	}
+	pluginInstalled := mg.SecretsManager() != nil
+	if !pluginInstalled {
+		return errPluginNotInstalled
+	}
+	return nil
+}
+
+func startAndReturnPlugin(mg plugins.SecretsPluginManager, ctx context.Context) (smp.SecretsManagerPlugin, error) {
+	var err error
+	startupOnce.Do(func() {
+		err = mg.SecretsManager().Start(ctx)
+	})
+	if err != nil {
+		return nil, err
+	}
+	return mg.SecretsManager().SecretsManager, nil
+}

pkg/services/secrets/kvstore/remote_plugin_check.go

@@ -1,34 +0,0 @@
-package kvstore
-
-import (
-	"context"
-
-	"github.com/grafana/grafana/pkg/infra/log"
-	"github.com/grafana/grafana/pkg/plugins/backendplugin/secretsmanagerplugin"
-)
-
-type UseRemoteSecretsPluginCheck interface {
-	ShouldUseRemoteSecretsPlugin() bool
-	StartAndReturnPlugin(ctx context.Context) (secretsmanagerplugin.SecretsManagerPlugin, error)
-}
-
-type OSSRemoteSecretsPluginCheck struct {
-	log log.Logger
-}
-
-func ProvideRemotePluginCheck() *OSSRemoteSecretsPluginCheck {
-	return &OSSRemoteSecretsPluginCheck{
-		log: log.New("ossremotesecretsplugincheck"),
-	}
-}
-
-func (c OSSRemoteSecretsPluginCheck) ShouldUseRemoteSecretsPlugin() bool {
-	return false
-}
-
-func (c OSSRemoteSecretsPluginCheck) StartAndReturnPlugin(ctx context.Context) (secretsmanagerplugin.SecretsManagerPlugin, error) {
-	c.log.Warn("OSSRemoteSecretsPluginCheck.StartAndReturnPlugin() was called by mistake. Secrets Manager plugins are enterprise only.")
-	return nil, nil
-}
-
-var _ UseRemoteSecretsPluginCheck = OSSRemoteSecretsPluginCheck{}

pkg/services/secrets/kvstore/test_helpers.go

@@ -6,6 +6,8 @@ import (
 	"testing"
 
 	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/plugins"
+	"github.com/grafana/grafana/pkg/plugins/backendplugin"
 	"github.com/grafana/grafana/pkg/plugins/backendplugin/secretsmanagerplugin"
 	"github.com/grafana/grafana/pkg/services/featuremgmt"
 	"github.com/grafana/grafana/pkg/services/secrets/database"
@@ -98,34 +100,6 @@ func (f fakeFeatureToggles) IsEnabled(feature string) bool {
 	return f.returnValue
 }
 
-// Fake Remote Secrets Plugin Check
-type mockRemoteSecretsPluginCheck struct {
-	UseRemoteSecretsPluginCheck
-	shouldReturnError bool
-}
-
-func provideMockRemotePluginCheck() *mockRemoteSecretsPluginCheck {
-	return &mockRemoteSecretsPluginCheck{}
-}
-
-func provideMockRemotePluginCheckWithErr() *mockRemoteSecretsPluginCheck {
-	return &mockRemoteSecretsPluginCheck{
-		shouldReturnError: true,
-	}
-}
-
-func (c *mockRemoteSecretsPluginCheck) ShouldUseRemoteSecretsPlugin() bool {
-	return true
-}
-
-func (c *mockRemoteSecretsPluginCheck) StartAndReturnPlugin(ctx context.Context) (secretsmanagerplugin.SecretsManagerPlugin, error) {
-	var err error
-	if c.shouldReturnError {
-		err = errors.New("bogus")
-	}
-	return &fakeGRPCSecretsPlugin{}, err
-}
-
 // Fake grpc secrets plugin impl
 type fakeGRPCSecretsPlugin struct{}
 
@@ -156,3 +130,38 @@ func (c *fakeGRPCSecretsPlugin) RenameSecret(ctx context.Context, in *secretsman
 
 var _ SecretsKVStore = FakeSecretsKVStore{}
 var _ secretsmanagerplugin.SecretsManagerPlugin = &fakeGRPCSecretsPlugin{}
+
+// Fake plugin manager
+type fakePluginManager struct {
+	shouldFailOnStart bool
+}
+
+func (mg *fakePluginManager) SecretsManager() *plugins.Plugin {
+	p := &plugins.Plugin{
+		SecretsManager: &fakeGRPCSecretsPlugin{},
+	}
+	p.RegisterClient(&fakePluginClient{
+		shouldFailOnStart: mg.shouldFailOnStart,
+	})
+	return p
+}
+
+func NewFakeSecretsPluginManager(t *testing.T, shouldFailOnStart bool) plugins.SecretsPluginManager {
+	t.Helper()
+	return &fakePluginManager{
+		shouldFailOnStart: shouldFailOnStart,
+	}
+}
+
+// Fake plugin client
+type fakePluginClient struct {
+	shouldFailOnStart bool
+	backendplugin.Plugin
+}
+
+func (pc *fakePluginClient) Start(_ context.Context) error {
+	if pc.shouldFailOnStart {
+		return errors.New("failed to start")
+	}
+	return nil
+}