Secrets: Implement Secret Plugin required flag and fatal crash on startup (#52552)

* add special handling on the plugin gathering side to check whether secrets manager plugins are enabled or not

* show disabled badge in front end if the plugin is not enabled

* Only show error in disabled badge hover if one is present (otherwise it shows "undefined")

* refactor to make use of fields already available in the DTO

* fix typo

* if there is no error returned for the plugin, just show 'disabled'

* fix typo

* Update public/app/features/plugins/admin/components/Badges/PluginDisabledBadge.tsx

Co-authored-by: Levente Balogh <balogh.levente.hu@gmail.com>

* Update frontendsettings.go

add clarifying comment

* fix unit test

* rework task to use new frontend property combined with plugin type to determine if the plugin should be disabled

* Update helpers.test.ts

revert test change

* fix unit test

* show custom uninstall message if the plugin is a secrets manager

* bogus commit to trigger precommit

* undo commit

* run precommit manually

* add some consts

* refactor a bit to pull plugin error management up a level

* re-add code squashed in merge

* fix compile issues

* add code to set plugin error fatal flag after secret migration

* refactor to move plugin startup out of Should Check func

* re-add important check

* make plugin startup errors fatal the first time we set a secret on the plugin

* rename func to make intent clearler

* remove unnecessary duplicate code from plugin mig

* fix compile error

* fix more compile errors

* add some extra logging to secrets migration

* have remote_plugin secret service managed plugin error fatal flag directly

* add blank file for eventual unit tests

* fix linting issues

* changes from PR review

* quick bit of cleanup

* add comment explaining design decision

* move more common test helpers to file

* slightly update to first time Get secret call

* add unit tests

* remove override func from provider

* fix linting issues

* add test cleanup step

* add some comments about refactoring to hacky test function

Co-authored-by: Levente Balogh <balogh.levente.hu@gmail.com>

pkg/services/datasources/service/secrets_mig.go

@@ -2,8 +2,10 @@ package service
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/grafana/grafana/pkg/infra/kvstore"
+	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/services/datasources"
 	"github.com/grafana/grafana/pkg/services/featuremgmt"
 )
@@ -21,6 +23,7 @@ type DataSourceSecretMigrationService struct {
 	dataSourcesService datasources.DataSourceService
 	kvStore            *kvstore.NamespacedKVStore
 	features           featuremgmt.FeatureToggles
+	log                log.Logger
 }
 
 func ProvideDataSourceMigrationService(
@@ -32,6 +35,7 @@ func ProvideDataSourceMigrationService(
 		dataSourcesService: dataSourcesService,
 		kvStore:            kvstore.WithNamespace(kvStore, 0, secretType),
 		features:           features,
+		log:                log.New("secrets.migration"),
 	}
 }
 
@@ -40,7 +44,7 @@ func (s *DataSourceSecretMigrationService) Migrate(ctx context.Context) error {
 	if err != nil {
 		return err
 	}
-
+	s.log.Debug(fmt.Sprint("secret migration status is ", migrationStatus))
 	// If this flag is true, delete secrets from the legacy secrets store as they are migrated
 	disableSecretsCompatibility := s.features.IsEnabled(featuremgmt.FlagDisableSecretsCompatibility)
 	// If migration hasn't happened, migrate to unified secrets and keep copy in legacy
@@ -51,6 +55,7 @@ func (s *DataSourceSecretMigrationService) Migrate(ctx context.Context) error {
 	needMigration := migrationStatus != completeSecretMigrationValue && disableSecretsCompatibility
 
 	if needCompatibility || needMigration {
+		s.log.Debug("performing secret migration", "needs migration", needMigration, "needs compatibility", needCompatibility)
 		query := &datasources.GetAllDataSourcesQuery{}
 		err := s.dataSourcesService.GetAllDataSources(ctx, query)
 		if err != nil {
@@ -85,15 +90,17 @@ func (s *DataSourceSecretMigrationService) Migrate(ctx context.Context) error {
 			}
 		}
 
+		var newMigStatus string
 		if disableSecretsCompatibility {
-			err = s.kvStore.Set(ctx, secretMigrationStatusKey, completeSecretMigrationValue)
+			newMigStatus = completeSecretMigrationValue
 		} else {
-			err = s.kvStore.Set(ctx, secretMigrationStatusKey, compatibleSecretMigrationValue)
+			newMigStatus = compatibleSecretMigrationValue
 		}
-
+		err = s.kvStore.Set(ctx, secretMigrationStatusKey, newMigStatus)
 		if err != nil {
 			return err
 		}
+		s.log.Debug(fmt.Sprint("set secret migration status to ", newMigStatus))
 	}
 
 	return nil

pkg/services/secrets/kvstore/cache.go

@@ -76,3 +76,7 @@ func (kv *CachedKVStore) Rename(ctx context.Context, orgId int64, namespace stri
 	}
 	return nil
 }
+
+func (kv *CachedKVStore) GetUnwrappedStore() SecretsKVStore {
+	return kv.store
+}

pkg/services/secrets/kvstore/helpers.go

@@ -1,81 +0,0 @@
-package kvstore
-
-import (
-	"context"
-	"testing"
-
-	"github.com/grafana/grafana/pkg/infra/log"
-	"github.com/grafana/grafana/pkg/services/secrets/database"
-	"github.com/grafana/grafana/pkg/services/secrets/manager"
-	"github.com/grafana/grafana/pkg/services/sqlstore"
-)
-
-func SetupTestService(t *testing.T) SecretsKVStore {
-	t.Helper()
-
-	sqlStore := sqlstore.InitTestDB(t)
-	store := database.ProvideSecretsStore(sqlstore.InitTestDB(t))
-	secretsService := manager.SetupTestService(t, store)
-
-	kv := &secretsKVStoreSQL{
-		sqlStore:       sqlStore,
-		log:            log.New("secrets.kvstore"),
-		secretsService: secretsService,
-		decryptionCache: decryptionCache{
-			cache: make(map[int64]cachedDecrypted),
-		},
-	}
-
-	return kv
-}
-
-// In memory kv store used for testing
-type FakeSecretsKVStore struct {
-	store map[Key]string
-}
-
-func NewFakeSecretsKVStore() FakeSecretsKVStore {
-	return FakeSecretsKVStore{store: make(map[Key]string)}
-}
-
-func (f FakeSecretsKVStore) Get(ctx context.Context, orgId int64, namespace string, typ string) (string, bool, error) {
-	value := f.store[buildKey(orgId, namespace, typ)]
-	found := value != ""
-	return value, found, nil
-}
-
-func (f FakeSecretsKVStore) Set(ctx context.Context, orgId int64, namespace string, typ string, value string) error {
-	f.store[buildKey(orgId, namespace, typ)] = value
-	return nil
-}
-
-func (f FakeSecretsKVStore) Del(ctx context.Context, orgId int64, namespace string, typ string) error {
-	delete(f.store, buildKey(orgId, namespace, typ))
-	return nil
-}
-
-func (f FakeSecretsKVStore) Keys(ctx context.Context, orgId int64, namespace string, typ string) ([]Key, error) {
-	res := make([]Key, 0)
-	for k := range f.store {
-		if k.OrgId == orgId && k.Namespace == namespace && k.Type == typ {
-			res = append(res, k)
-		}
-	}
-	return res, nil
-}
-
-func (f FakeSecretsKVStore) Rename(ctx context.Context, orgId int64, namespace string, typ string, newNamespace string) error {
-	f.store[buildKey(orgId, newNamespace, typ)] = f.store[buildKey(orgId, namespace, typ)]
-	delete(f.store, buildKey(orgId, namespace, typ))
-	return nil
-}
-
-func buildKey(orgId int64, namespace string, typ string) Key {
-	return Key{
-		OrgId:     orgId,
-		Namespace: namespace,
-		Type:      typ,
-	}
-}
-
-var _ SecretsKVStore = FakeSecretsKVStore{}

pkg/services/secrets/kvstore/kvstore.go

@@ -2,9 +2,13 @@ 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/services/featuremgmt"
 	"github.com/grafana/grafana/pkg/services/secrets"
 	"github.com/grafana/grafana/pkg/services/sqlstore"
 )
@@ -14,7 +18,13 @@ const (
 	AllOrganizations = -1
 )
 
-func ProvideService(sqlStore sqlstore.Store, secretsService secrets.Service, remoteCheck UseRemoteSecretsPluginCheck) SecretsKVStore {
+func ProvideService(
+	sqlStore sqlstore.Store,
+	secretsService secrets.Service,
+	remoteCheck UseRemoteSecretsPluginCheck,
+	kvstore kvstore.KVStore,
+	features featuremgmt.FeatureToggles,
+) (SecretsKVStore, error) {
 	var store SecretsKVStore
 	logger := log.New("secrets.kvstore")
 	store = &secretsKVStoreSQL{
@@ -26,21 +36,34 @@ func ProvideService(sqlStore sqlstore.Store, secretsService secrets.Service, rem
 		},
 	}
 	if remoteCheck.ShouldUseRemoteSecretsPlugin() {
-		logger.Debug("secrets kvstore is using a remote plugin for secrets management")
+		// Attempt to start the plugin
-		secretsPlugin, err := remoteCheck.GetPlugin()
+		secretsPlugin, err := remoteCheck.StartAndReturnPlugin(context.Background())
-		if err != nil {
+		namespacedKVStore := GetNamespacedKVStore(kvstore)
-			logger.Error("plugin client was nil, falling back to SQL implementation")
+		if err != nil || secretsPlugin == nil {
+			if isFatal, err2 := isPluginStartupErrorFatal(context.Background(), namespacedKVStore); isFatal || err2 != 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
+				}
+				return nil, err
+			}
+			logger.Error("error starting secrets plugin, falling back to SQL implementation")
 		} else {
 			store = &secretsKVStorePlugin{
 				secretsPlugin:                  secretsPlugin,
 				secretsService:                 secretsService,
 				log:                            logger,
+				kvstore:                        namespacedKVStore,
+				backwardsCompatibilityDisabled: features.IsEnabled(featuremgmt.FlagDisableSecretsCompatibility),
 			}
 		}
 	} else {
 		logger.Debug("secrets kvstore is using the default (SQL) implementation for secrets management")
 	}
-	return NewCachedKVStore(store, 5*time.Second, 5*time.Minute)
+
+	return NewCachedKVStore(store, 5*time.Second, 5*time.Minute), nil
 }
 
 // SecretsKVStore is an interface for k/v store.
@@ -94,3 +117,23 @@ 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/model.go

@@ -4,6 +4,11 @@ import (
 	"time"
 )
 
+const (
+	QuitOnPluginStartupFailureKey = "quit_on_secrets_plugin_startup_failure"
+	PluginNamespace               = "secretsmanagerplugin"
+)
+
 // Item stored in k/v store.
 type Item struct {
 	Id        int64

pkg/services/secrets/kvstore/plugin_fatal_crash_test.go

@@ -0,0 +1,149 @@
+package kvstore
+
+import (
+	"context"
+	"errors"
+	"sync"
+	"testing"
+
+	"github.com/grafana/grafana/pkg/infra/kvstore"
+	"github.com/grafana/grafana/pkg/services/secrets/fakes"
+	secretsManager "github.com/grafana/grafana/pkg/services/secrets/manager"
+	"github.com/grafana/grafana/pkg/services/sqlstore"
+	"github.com/grafana/grafana/pkg/services/sqlstore/mockstore"
+	"github.com/grafana/grafana/pkg/setting"
+	"github.com/stretchr/testify/require"
+	"gopkg.in/ini.v1"
+)
+
+// Set fatal flag to true, then simulate a plugin start failure
+// Should result in an error from the secret store provider
+func TestFatalPluginErr_PluginFailsToStartWithFatalFlagSet(t *testing.T) {
+	svc, _, _, err := setupFatalCrashTest(t, true, true, false)
+	require.Error(t, err)
+	require.Nil(t, svc)
+}
+
+// Set fatal flag to false, then simulate a plugin start failure
+// Should result in the secret store provider returning the sql impl
+func TestFatalPluginErr_PluginFailsToStartWithFatalFlagNotSet(t *testing.T) {
+	svc, _, _, err := setupFatalCrashTest(t, true, false, false)
+	require.NoError(t, err)
+	require.IsType(t, &CachedKVStore{}, svc)
+	cachedKv, _ := svc.(*CachedKVStore)
+	require.IsType(t, &secretsKVStoreSQL{}, cachedKv.GetUnwrappedStore())
+}
+
+// With fatal flag not set, store a secret in the plugin while backwards compatibility is disabled
+// Should result in the fatal flag going from unset -> set to true
+func TestFatalPluginErr_FatalFlagGetsSetWithBackwardsCompatDisabled(t *testing.T) {
+	svc, kvstore, _, err := setupFatalCrashTest(t, false, false, true)
+	require.NoError(t, err)
+	require.NotNil(t, svc)
+	err = svc.Set(context.Background(), 0, "datasource", "postgres", "my secret")
+	require.NoError(t, err)
+	isFatal, err := isPluginStartupErrorFatal(context.Background(), GetNamespacedKVStore(kvstore))
+	require.NoError(t, err)
+	require.True(t, isFatal)
+}
+
+// With fatal flag set, retrieve a secret from the plugin while backwards compatibility is enabled
+// Should result in the fatal flag going from set to true -> unset
+func TestFatalPluginErr_FatalFlagGetsUnSetWithBackwardsCompatEnabled(t *testing.T) {
+	svc, kvstore, _, err := setupFatalCrashTest(t, false, true, false)
+	require.NoError(t, err)
+	require.NotNil(t, svc)
+	val, exists, err := svc.Get(context.Background(), 0, "datasource", "postgres")
+	require.NoError(t, err)
+	require.NotNil(t, val)
+	require.True(t, exists)
+	isFatal, err := isPluginStartupErrorFatal(context.Background(), GetNamespacedKVStore(kvstore))
+	require.NoError(t, err)
+	require.False(t, isFatal)
+}
+
+// With fatal flag unset, do a migration with backwards compatibility disabled. When unified secrets are deleted, return an error on the first deletion
+// Should result in the fatal flag remaining unset
+func TestFatalPluginErr_MigrationTestWithErrorDeletingUnifiedSecrets(t *testing.T) {
+	svc, kvstore, _, err := setupFatalCrashTest(t, false, false, true)
+	require.NoError(t, err)
+
+	migration := setupTestMigratorServiceWithDeletionError(t, svc, &mockstore.SQLStoreMock{
+		ExpectedError: errors.New("random error"),
+	}, kvstore)
+	err = migration.Migrate(context.Background())
+	require.Error(t, err)
+	isFatal, err := isPluginStartupErrorFatal(context.Background(), GetNamespacedKVStore(kvstore))
+	require.NoError(t, err)
+	require.False(t, isFatal)
+}
+
+func setupFatalCrashTest(
+	t *testing.T,
+	shouldRemoteCheckError bool,
+	isPluginErrorFatal bool,
+	isBackwardsCompatDisabled bool,
+) (SecretsKVStore, kvstore.KVStore, *sqlstore.SQLStore, error) {
+	t.Helper()
+	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)
+	t.Cleanup(func() {
+		fatalFlagOnce = sync.Once{}
+	})
+	return svc, kvstore, sqlStore, err
+}
+
+func setupTestMigratorServiceWithDeletionError(
+	t *testing.T,
+	secretskv SecretsKVStore,
+	sqlStore sqlstore.Store,
+	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()
+	secretsService := secretsManager.SetupTestService(t, fakes.NewFakeSecretsStore())
+	getAllFuncOverride := func(ctx context.Context) ([]Item, error) {
+		items := make([]Item, 0)
+		var orgId int64 = 1
+		str := "random string"
+		items = append(items, Item{
+			Id:        1,
+			OrgId:     &orgId,
+			Type:      &str,
+			Namespace: &str,
+			Value:     "bogus",
+		})
+		return items, nil
+	}
+	migratorService := ProvidePluginSecretMigrationService(
+		secretskv,
+		cfg,
+		sqlStore,
+		secretsService,
+		remoteCheck,
+		kvstore,
+	)
+	// TODO refactor Migrator to allow us to override the entire sqlstore with a mock instead
+	migratorService.overrideGetAllFunc(getAllFuncOverride)
+	return migratorService
+}

pkg/services/secrets/kvstore/plugin_mig.go

@@ -3,6 +3,7 @@ package kvstore
 import (
 	"context"
 
+	"github.com/grafana/grafana/pkg/infra/kvstore"
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/services/secrets"
 	"github.com/grafana/grafana/pkg/services/sqlstore"
@@ -18,6 +19,8 @@ type PluginSecretMigrationService struct {
 	sqlStore       sqlstore.Store
 	secretsService secrets.Service
 	remoteCheck    UseRemoteSecretsPluginCheck
+	kvstore        kvstore.KVStore
+	getAllFunc     func(ctx context.Context) ([]Item, error)
 }
 
 func ProvidePluginSecretMigrationService(
@@ -26,6 +29,7 @@ func ProvidePluginSecretMigrationService(
 	sqlStore sqlstore.Store,
 	secretsService secrets.Service,
 	remoteCheck UseRemoteSecretsPluginCheck,
+	kvstore kvstore.KVStore,
 ) *PluginSecretMigrationService {
 	return &PluginSecretMigrationService{
 		secretsStore:   secretsStore,
@@ -34,13 +38,13 @@ func ProvidePluginSecretMigrationService(
 		sqlStore:       sqlStore,
 		secretsService: secretsService,
 		remoteCheck:    remoteCheck,
+		kvstore:        kvstore,
 	}
 }
 
 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) &&
+	if s.cfg.SectionWithEnvOverrides("secrets").Key("migrate_to_plugin").MustBool(false) && s.remoteCheck.ShouldUseRemoteSecretsPlugin() {
-		s.remoteCheck.ShouldUseRemoteSecretsPlugin() {
 		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.
@@ -51,6 +55,14 @@ func (s *PluginSecretMigrationService) Migrate(ctx context.Context) error {
 			decryptionCache: decryptionCache{
 				cache: make(map[int64]cachedDecrypted),
 			},
+			GetAllFuncOverride: s.getAllFunc,
+		}
+
+		// before we start migrating, check see if plugin startup failures were already fatal
+		namespacedKVStore := GetNamespacedKVStore(s.kvstore)
+		wasFatal, err := isPluginStartupErrorFatal(ctx, namespacedKVStore)
+		if err != nil {
+			s.logger.Warn("unabled to determine whether plugin startup failures are fatal - continuing migration anyway.")
 		}
 
 		allSec, err := secretsSql.GetAll(ctx)
@@ -66,9 +78,19 @@ func (s *PluginSecretMigrationService) Migrate(ctx context.Context) error {
 		}
 		s.logger.Debug("migrated unified secrets to plugin", "number of secrets", len(allSec))
 		// as no err was returned, when we delete all the secrets from the sql store
-		for _, sec := range allSec {
+		for index, sec := range allSec {
 			err = secretsSql.Del(ctx, *sec.OrgId, *sec.Namespace, *sec.Type)
 			if err != nil {
+				s.logger.Error("plugin migrator encountered error while deleting unified secrets")
+				if index == 0 && !wasFatal {
+					// old unified secrets still exists, so plugin startup errors are still not fatal, unless they were before we started
+					err := setPluginStartupErrorFatal(ctx, namespacedKVStore, false)
+					if err != nil {
+						s.logger.Error("error reverting plugin failure fatal status", "error", err.Error())
+					} else {
+						s.logger.Debug("application will continue to function without the secrets plugin")
+					}
+				}
 				return err
 			}
 		}
@@ -76,3 +98,10 @@ func (s *PluginSecretMigrationService) Migrate(ctx context.Context) error {
 	}
 	return nil
 }
+
+// This is here to support testing and should normally not be called
+// An edge case we are unit testing requires the GetAll function to return a value, but the Del function to return an error.
+// This is not possible with the code as written, so this override function is a workaround. Should be refactored.
+func (s *PluginSecretMigrationService) overrideGetAllFunc(getAllFunc func(ctx context.Context) ([]Item, error)) {
+	s.getAllFunc = getAllFunc
+}

pkg/services/secrets/kvstore/plugin_mig_test.go

@@ -4,11 +4,12 @@ import (
 	"context"
 	"testing"
 
+	"github.com/grafana/grafana/pkg/infra/kvstore"
 	"github.com/grafana/grafana/pkg/infra/log"
-	"github.com/grafana/grafana/pkg/plugins/backendplugin/secretsmanagerplugin"
 	"github.com/grafana/grafana/pkg/services/secrets/fakes"
 	secretsManager "github.com/grafana/grafana/pkg/services/secrets/manager"
 	"github.com/grafana/grafana/pkg/services/sqlstore"
+
 	"github.com/grafana/grafana/pkg/setting"
 	"github.com/stretchr/testify/require"
 	"gopkg.in/ini.v1"
@@ -82,13 +83,13 @@ func setupTestMigratorService(t *testing.T) (*PluginSecretMigrationService, Secr
 	// this is to init the sql secret store inside the migration
 	sqlStore := sqlstore.InitTestDB(t)
 	secretsService := secretsManager.SetupTestService(t, fakes.NewFakeSecretsStore())
-
 	migratorService := ProvidePluginSecretMigrationService(
 		secretsStoreForPlugin,
 		cfg,
 		sqlStore,
 		secretsService,
 		remoteCheck,
+		kvstore.ProvideService(sqlStore),
 	)
 
 	secretsSql := &secretsKVStoreSQL{
@@ -102,20 +103,3 @@ func setupTestMigratorService(t *testing.T) (*PluginSecretMigrationService, Secr
 
 	return migratorService, secretsStoreForPlugin, secretsSql
 }
-
-//
-type mockRemoteSecretsPluginCheck struct {
-	UseRemoteSecretsPluginCheck
-}
-
-func provideMockRemotePluginCheck() *mockRemoteSecretsPluginCheck {
-	return &mockRemoteSecretsPluginCheck{}
-}
-
-func (c *mockRemoteSecretsPluginCheck) ShouldUseRemoteSecretsPlugin() bool {
-	return true
-}
-
-func (c *mockRemoteSecretsPluginCheck) GetPlugin() (secretsmanagerplugin.SecretsManagerPlugin, error) {
-	return nil, nil
-}

pkg/services/secrets/kvstore/remote_plugin.go

@@ -2,21 +2,30 @@ package kvstore
 
 import (
 	"context"
+	"sync"
 
+	"github.com/grafana/grafana/pkg/infra/kvstore"
 	"github.com/grafana/grafana/pkg/infra/log"
 	smp "github.com/grafana/grafana/pkg/plugins/backendplugin/secretsmanagerplugin"
 	"github.com/grafana/grafana/pkg/services/datasources"
 	"github.com/grafana/grafana/pkg/services/secrets"
 )
 
+var (
+	fatalFlagOnce sync.Once
+)
+
 // secretsKVStorePlugin provides a key/value store backed by the Grafana plugin gRPC interface
 type secretsKVStorePlugin struct {
 	log                            log.Logger
 	secretsPlugin                  smp.SecretsManagerPlugin
 	secretsService                 secrets.Service
+	kvstore                        *kvstore.NamespacedKVStore
+	backwardsCompatibilityDisabled bool
 }
 
 // Get an item from the store
+// If it is the first time a secret has been retrieved and backwards compatibility is disabled, mark plugin startup errors fatal
 func (kv *secretsKVStorePlugin) Get(ctx context.Context, orgId int64, namespace string, typ string) (string, bool, error) {
 	req := &smp.GetSecretRequest{
 		KeyDescriptor: &smp.Key{
@@ -32,10 +41,15 @@ func (kv *secretsKVStorePlugin) Get(ctx context.Context, orgId int64, namespace
 		err = wrapUserFriendlySecretError(res.UserFriendlyError)
 	}
 
+	if res.Exists {
+		updateFatalFlag(ctx, *kv)
+	}
+
 	return res.DecryptedValue, res.Exists, err
 }
 
 // Set an item in the store
+// If it is the first time a secret has been set and backwards compatibility is disabled, mark plugin startup errors fatal
 func (kv *secretsKVStorePlugin) Set(ctx context.Context, orgId int64, namespace string, typ string, value string) error {
 	req := &smp.SetSecretRequest{
 		KeyDescriptor: &smp.Key{
@@ -51,6 +65,8 @@ func (kv *secretsKVStorePlugin) Set(ctx context.Context, orgId int64, namespace
 		err = wrapUserFriendlySecretError(res.UserFriendlyError)
 	}
 
+	updateFatalFlag(ctx, *kv)
+
 	return err
 }
 
@@ -124,6 +140,26 @@ func parseKeys(keys []*smp.Key) []Key {
 	return newKeys
 }
 
+func updateFatalFlag(ctx context.Context, skv secretsKVStorePlugin) {
+	// This function makes the most sense in here because it handles all possible scenarios:
+	//   - User changed backwards compatibility flag, so we have to migrate secrets either to or from the plugin (get or set)
+	//   - Migration is on, so we migrate secrets to the plugin (set)
+	//   - User doesn't migrate, but stores a new secret in the plugin (set)
+	// Rather than updating the flag in several places, it is cleaner to just do this check once
+	// Very early on. Once backwards compatibility to legacy secrets is gone in Grafana 10, this can go away as well
+	fatalFlagOnce.Do(func() {
+		var err error
+		if isFatal, _ := isPluginStartupErrorFatal(ctx, skv.kvstore); !isFatal && skv.backwardsCompatibilityDisabled {
+			err = setPluginStartupErrorFatal(ctx, skv.kvstore, true)
+		} else if isFatal && !skv.backwardsCompatibilityDisabled {
+			err = setPluginStartupErrorFatal(ctx, skv.kvstore, false)
+		}
+		if err != nil {
+			skv.log.Error("failed to set plugin error fatal flag", err.Error())
+		}
+	})
+}
+
 func wrapUserFriendlySecretError(ufe string) datasources.ErrDatasourceSecretsPluginUserFriendly {
 	return datasources.ErrDatasourceSecretsPluginUserFriendly{Err: ufe}
 }

pkg/services/secrets/kvstore/remote_plugin_check.go

@@ -1,26 +1,34 @@
 package kvstore
 
 import (
+	"context"
+
+	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/plugins/backendplugin/secretsmanagerplugin"
 )
 
 type UseRemoteSecretsPluginCheck interface {
 	ShouldUseRemoteSecretsPlugin() bool
-	GetPlugin() (secretsmanagerplugin.SecretsManagerPlugin, error)
+	StartAndReturnPlugin(ctx context.Context) (secretsmanagerplugin.SecretsManagerPlugin, error)
 }
 
 type OSSRemoteSecretsPluginCheck struct {
-	UseRemoteSecretsPluginCheck
+	log log.Logger
 }
 
 func ProvideRemotePluginCheck() *OSSRemoteSecretsPluginCheck {
-	return &OSSRemoteSecretsPluginCheck{}
+	return &OSSRemoteSecretsPluginCheck{
+		log: log.New("ossremotesecretsplugincheck"),
+	}
 }
 
-func (c *OSSRemoteSecretsPluginCheck) ShouldUseRemoteSecretsPlugin() bool {
+func (c OSSRemoteSecretsPluginCheck) ShouldUseRemoteSecretsPlugin() bool {
 	return false
 }
 
-func (c *OSSRemoteSecretsPluginCheck) GetPlugin() (secretsmanagerplugin.SecretsManagerPlugin, error) {
+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/sql.go

@@ -17,6 +17,8 @@ type secretsKVStoreSQL struct {
 	sqlStore        sqlstore.Store
 	secretsService  secrets.Service
 	decryptionCache decryptionCache
+	// This is here to support testing and should normally not be set
+	GetAllFuncOverride func(ctx context.Context) ([]Item, error)
 }
 
 type decryptionCache struct {
@@ -227,6 +229,9 @@ func (kv *secretsKVStoreSQL) Rename(ctx context.Context, orgId int64, namespace
 // GetAll this returns all the secrets stored in the database. This is not part of the kvstore interface as we
 // only need it for migration from sql to plugin at this moment
 func (kv *secretsKVStoreSQL) GetAll(ctx context.Context) ([]Item, error) {
+	if kv.GetAllFuncOverride != nil {
+		return kv.GetAllFuncOverride(ctx)
+	}
 	var items []Item
 	err := kv.sqlStore.WithDbSession(ctx, func(dbSession *sqlstore.DBSession) error {
 		return dbSession.Find(&items)

pkg/services/secrets/kvstore/test_helpers.go

@@ -0,0 +1,158 @@
+package kvstore
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/plugins/backendplugin/secretsmanagerplugin"
+	"github.com/grafana/grafana/pkg/services/featuremgmt"
+	"github.com/grafana/grafana/pkg/services/secrets/database"
+	"github.com/grafana/grafana/pkg/services/secrets/manager"
+	"github.com/grafana/grafana/pkg/services/sqlstore"
+	"google.golang.org/grpc"
+)
+
+func SetupTestService(t *testing.T) SecretsKVStore {
+	t.Helper()
+
+	sqlStore := sqlstore.InitTestDB(t)
+	store := database.ProvideSecretsStore(sqlstore.InitTestDB(t))
+	secretsService := manager.SetupTestService(t, store)
+
+	kv := &secretsKVStoreSQL{
+		sqlStore:       sqlStore,
+		log:            log.New("secrets.kvstore"),
+		secretsService: secretsService,
+		decryptionCache: decryptionCache{
+			cache: make(map[int64]cachedDecrypted),
+		},
+	}
+
+	return kv
+}
+
+// In memory kv store used for testing
+type FakeSecretsKVStore struct {
+	store map[Key]string
+}
+
+func NewFakeSecretsKVStore() FakeSecretsKVStore {
+	return FakeSecretsKVStore{store: make(map[Key]string)}
+}
+
+func (f FakeSecretsKVStore) Get(ctx context.Context, orgId int64, namespace string, typ string) (string, bool, error) {
+	value := f.store[buildKey(orgId, namespace, typ)]
+	found := value != ""
+	return value, found, nil
+}
+
+func (f FakeSecretsKVStore) Set(ctx context.Context, orgId int64, namespace string, typ string, value string) error {
+	f.store[buildKey(orgId, namespace, typ)] = value
+	return nil
+}
+
+func (f FakeSecretsKVStore) Del(ctx context.Context, orgId int64, namespace string, typ string) error {
+	delete(f.store, buildKey(orgId, namespace, typ))
+	return nil
+}
+
+func (f FakeSecretsKVStore) Keys(ctx context.Context, orgId int64, namespace string, typ string) ([]Key, error) {
+	res := make([]Key, 0)
+	for k := range f.store {
+		if k.OrgId == orgId && k.Namespace == namespace && k.Type == typ {
+			res = append(res, k)
+		}
+	}
+	return res, nil
+}
+
+func (f FakeSecretsKVStore) Rename(ctx context.Context, orgId int64, namespace string, typ string, newNamespace string) error {
+	f.store[buildKey(orgId, newNamespace, typ)] = f.store[buildKey(orgId, namespace, typ)]
+	delete(f.store, buildKey(orgId, namespace, typ))
+	return nil
+}
+
+func buildKey(orgId int64, namespace string, typ string) Key {
+	return Key{
+		OrgId:     orgId,
+		Namespace: namespace,
+		Type:      typ,
+	}
+}
+
+// Fake feature toggle - only need to check the backwards compatibility disabled flag
+type fakeFeatureToggles struct {
+	returnValue bool
+}
+
+func NewFakeFeatureToggles(t *testing.T, returnValue bool) featuremgmt.FeatureToggles {
+	t.Helper()
+	return fakeFeatureToggles{
+		returnValue: returnValue,
+	}
+}
+
+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{}
+
+func (c *fakeGRPCSecretsPlugin) GetSecret(ctx context.Context, in *secretsmanagerplugin.GetSecretRequest, opts ...grpc.CallOption) (*secretsmanagerplugin.GetSecretResponse, error) {
+	return &secretsmanagerplugin.GetSecretResponse{
+		DecryptedValue: "bogus",
+		Exists:         true,
+	}, nil
+}
+
+func (c *fakeGRPCSecretsPlugin) SetSecret(ctx context.Context, in *secretsmanagerplugin.SetSecretRequest, opts ...grpc.CallOption) (*secretsmanagerplugin.SetSecretResponse, error) {
+	return &secretsmanagerplugin.SetSecretResponse{}, nil
+}
+
+func (c *fakeGRPCSecretsPlugin) DeleteSecret(ctx context.Context, in *secretsmanagerplugin.DeleteSecretRequest, opts ...grpc.CallOption) (*secretsmanagerplugin.DeleteSecretResponse, error) {
+	return &secretsmanagerplugin.DeleteSecretResponse{}, nil
+}
+
+func (c *fakeGRPCSecretsPlugin) ListSecrets(ctx context.Context, in *secretsmanagerplugin.ListSecretsRequest, opts ...grpc.CallOption) (*secretsmanagerplugin.ListSecretsResponse, error) {
+	return &secretsmanagerplugin.ListSecretsResponse{
+		Keys: make([]*secretsmanagerplugin.Key, 0),
+	}, nil
+}
+
+func (c *fakeGRPCSecretsPlugin) RenameSecret(ctx context.Context, in *secretsmanagerplugin.RenameSecretRequest, opts ...grpc.CallOption) (*secretsmanagerplugin.RenameSecretResponse, error) {
+	return &secretsmanagerplugin.RenameSecretResponse{}, nil
+}
+
+var _ SecretsKVStore = FakeSecretsKVStore{}
+var _ secretsmanagerplugin.SecretsManagerPlugin = &fakeGRPCSecretsPlugin{}