Secrets: Add fallback to secrets kvstore plugin (#54056)

* Add fallback to secrets kvstore plugin

* Fix linter issues

* Fix linter issues

* Add deletion error to bool to fake secrets kvstore

* Add fallback to fake secrets kvstore

* Fix fake secrets kvstore fallback setter

* Use Key on Item message for secrets manager protobuf

* Add clarifying comment about fallback

pkg/services/secrets/kvstore/cache.go

@@ -77,6 +77,18 @@ func (kv *CachedKVStore) Rename(ctx context.Context, orgId int64, namespace stri
 	return nil
 }
 
+func (kv *CachedKVStore) GetAll(ctx context.Context) ([]Item, error) {
+	return kv.store.GetAll(ctx)
+}
+
+func (kv *CachedKVStore) Fallback() SecretsKVStore {
+	return kv.store.Fallback()
+}
+
+func (kv *CachedKVStore) SetFallback(store SecretsKVStore) error {
+	return kv.store.SetFallback(store)
+}
+
 func (kv *CachedKVStore) GetUnwrappedStore() SecretsKVStore {
 	return kv.store
 }

pkg/services/secrets/kvstore/kvstore.go

@@ -56,12 +56,16 @@ func ProvideService(
 				return nil, err
 			}
 		} else {
+			// as the plugin is installed, secretsKVStoreSQL is now replaced with
+			// an instance of secretsKVStorePlugin with the sql store as a fallback
+			// (used for migration and in case a secret is not found).
 			store = &secretsKVStorePlugin{
 				secretsPlugin:                  secretsPlugin,
 				secretsService:                 secretsService,
 				log:                            logger,
 				kvstore:                        namespacedKVStore,
 				backwardsCompatibilityDisabled: features.IsEnabled(featuremgmt.FlagDisableSecretsCompatibility),
+				fallback:                       store,
 			}
 		}
 	}
@@ -80,6 +84,9 @@ type SecretsKVStore interface {
 	Del(ctx context.Context, orgId int64, namespace string, typ string) error
 	Keys(ctx context.Context, orgId int64, namespace string, typ string) ([]Key, error)
 	Rename(ctx context.Context, orgId int64, namespace string, typ string, newNamespace string) error
+	GetAll(ctx context.Context) ([]Item, error)
+	Fallback() SecretsKVStore
+	SetFallback(store SecretsKVStore) error
 }
 
 // WithType returns a kvstore wrapper with fixed orgId and type.

pkg/services/secrets/kvstore/plugin_fatal_crash_test.go

@@ -114,19 +114,6 @@ func setupTestMigratorServiceWithDeletionError(
 	startupOnce = sync.Once{}
 	cfg := setupTestConfig(t)
 	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
-	}
 	manager := NewFakeSecretsPluginManager(t, false)
 	migratorService := ProvidePluginSecretMigrationService(
 		secretskv,
@@ -136,8 +123,17 @@ func setupTestMigratorServiceWithDeletionError(
 		kvstore,
 		manager,
 	)
-	// TODO refactor Migrator to allow us to override the entire sqlstore with a mock instead
-	migratorService.overrideGetAllFunc(getAllFuncOverride)
+	fallback := NewFakeSecretsKVStore()
+	var orgId int64 = 1
+	str := "random string"
+	fallback.store[Key{
+		OrgId:     orgId,
+		Type:      str,
+		Namespace: str,
+	}] = "bogus"
+	fallback.delError = true
+	err := secretskv.SetFallback(fallback)
+	require.NoError(t, err)
 	return migratorService
 }
 

pkg/services/secrets/kvstore/plugin_mig.go

@@ -2,6 +2,7 @@ package kvstore
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"github.com/grafana/grafana/pkg/infra/kvstore"
@@ -22,7 +23,6 @@ type PluginSecretMigrationService struct {
 	secretsService secrets.Service
 	kvstore        kvstore.KVStore
 	manager        plugins.SecretsPluginManager
-	getAllFunc     func(ctx context.Context) ([]Item, error)
 }
 
 func ProvidePluginSecretMigrationService(
@@ -36,7 +36,7 @@ func ProvidePluginSecretMigrationService(
 	return &PluginSecretMigrationService{
 		secretsStore:   secretsStore,
 		cfg:            cfg,
-		logger:         log.New("sec-plugin-mig"),
+		logger:         log.New("secret.migration.plugin"),
 		sqlStore:       sqlStore,
 		secretsService: secretsService,
 		kvstore:        kvstore,
@@ -48,16 +48,10 @@ func (s *PluginSecretMigrationService) Migrate(ctx context.Context) error {
 	// Check if we should migrate to plugin - default false
 	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.
-		secretsSql := &secretsKVStoreSQL{
-			sqlStore:       s.sqlStore,
-			secretsService: s.secretsService,
-			log:            s.logger,
-			decryptionCache: decryptionCache{
-				cache: make(map[int64]cachedDecrypted),
-			},
-			GetAllFuncOverride: s.getAllFunc,
+		// we need to get the fallback store since in this scenario the secrets store would be the plugin.
+		fallbackStore := s.secretsStore.Fallback()
+		if fallbackStore == nil {
+			return errors.New("unable to get fallback secret store for migration")
 		}
 
 		// before we start migrating, check see if plugin startup failures were already fatal
@@ -67,7 +61,7 @@ func (s *PluginSecretMigrationService) Migrate(ctx context.Context) error {
 			s.logger.Warn("unable to determine whether plugin startup failures are fatal - continuing migration anyway.")
 		}
 
-		allSec, err := secretsSql.GetAll(ctx)
+		allSec, err := fallbackStore.GetAll(ctx)
 		if err != nil {
 			return nil
 		}
@@ -86,7 +80,7 @@ func (s *PluginSecretMigrationService) Migrate(ctx context.Context) error {
 		for index, sec := range allSec {
 			s.logger.Debug(fmt.Sprintf("Cleaning secret %d of %d", index+1, totalSec), "current", index+1, "secretCount", totalSec)
 
-			err = secretsSql.Del(ctx, *sec.OrgId, *sec.Namespace, *sec.Type)
+			err = fallbackStore.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 {
@@ -105,10 +99,3 @@ 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

@@ -100,5 +100,7 @@ func setupTestMigratorService(t *testing.T) (*PluginSecretMigrationService, Secr
 		},
 	}
 
+	err = secretsStoreForPlugin.SetFallback(secretsSql)
+	require.NoError(t, err)
 	return migratorService, secretsStoreForPlugin, secretsSql
 }

pkg/services/secrets/kvstore/remote_plugin.go

@@ -29,6 +29,7 @@ type secretsKVStorePlugin struct {
 	secretsService                 secrets.Service
 	kvstore                        *kvstore.NamespacedKVStore
 	backwardsCompatibilityDisabled bool
+	fallback                       SecretsKVStore
 }
 
 // Get an item from the store
@@ -136,6 +137,28 @@ func (kv *secretsKVStorePlugin) Rename(ctx context.Context, orgId int64, namespa
 	return err
 }
 
+func (kv *secretsKVStorePlugin) GetAll(ctx context.Context) ([]Item, error) {
+	req := &smp.GetAllSecretsRequest{}
+
+	res, err := kv.secretsPlugin.GetAllSecrets(ctx, req)
+	if err != nil {
+		return nil, err
+	} else if res.UserFriendlyError != "" {
+		err = wrapUserFriendlySecretError(res.UserFriendlyError)
+	}
+
+	return parseItems(res.Items), err
+}
+
+func (kv *secretsKVStorePlugin) Fallback() SecretsKVStore {
+	return kv.fallback
+}
+
+func (kv *secretsKVStorePlugin) SetFallback(store SecretsKVStore) error {
+	kv.fallback = store
+	return nil
+}
+
 func parseKeys(keys []*smp.Key) []Key {
 	var newKeys []Key
 
@@ -147,6 +170,17 @@ func parseKeys(keys []*smp.Key) []Key {
 	return newKeys
 }
 
+func parseItems(items []*smp.Item) []Item {
+	var newItems []Item
+
+	for _, i := range items {
+		newItem := Item{OrgId: &i.Key.OrgId, Namespace: &i.Key.Namespace, Type: &i.Key.Type, Value: i.Value}
+		newItems = append(newItems, newItem)
+	}
+
+	return newItems
+}
+
 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)

pkg/services/secrets/kvstore/sql.go

@@ -3,6 +3,7 @@ package kvstore
 import (
 	"context"
 	"encoding/base64"
+	"errors"
 	"sync"
 	"time"
 
@@ -17,8 +18,6 @@ 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 {
@@ -31,7 +30,10 @@ type cachedDecrypted struct {
 	value   string
 }
 
-var b64 = base64.RawStdEncoding
+var (
+	b64                   = base64.RawStdEncoding
+	errFallbackNotAllowed = errors.New("fallback not allowed for sql secret store")
+)
 
 // Get an item from the store
 func (kv *secretsKVStoreSQL) Get(ctx context.Context, orgId int64, namespace string, typ string) (string, bool, error) {
@@ -210,9 +212,6 @@ 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)
@@ -234,6 +233,14 @@ func (kv *secretsKVStoreSQL) GetAll(ctx context.Context) ([]Item, error) {
 	return items, err
 }
 
+func (kv *secretsKVStoreSQL) Fallback() SecretsKVStore {
+	return nil
+}
+
+func (kv *secretsKVStoreSQL) SetFallback(_ SecretsKVStore) error {
+	return errFallbackNotAllowed
+}
+
 func (kv *secretsKVStoreSQL) getDecryptedValue(ctx context.Context, item Item) ([]byte, error) {
 	kv.decryptionCache.Lock()
 	defer kv.decryptionCache.Unlock()

pkg/services/secrets/kvstore/test_helpers.go

@@ -37,30 +37,35 @@ func SetupTestService(t *testing.T) SecretsKVStore {
 
 // In memory kv store used for testing
 type FakeSecretsKVStore struct {
-	store map[Key]string
+	store    map[Key]string
+	delError bool
+	fallback SecretsKVStore
 }
 
-func NewFakeSecretsKVStore() FakeSecretsKVStore {
-	return FakeSecretsKVStore{store: make(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) {
+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 {
+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 {
+func (f *FakeSecretsKVStore) Del(ctx context.Context, orgId int64, namespace string, typ string) error {
+	if f.delError {
+		return errors.New("bogus")
+	}
 	delete(f.store, buildKey(orgId, namespace, typ))
 	return nil
 }
 
-func (f FakeSecretsKVStore) Keys(ctx context.Context, orgId int64, namespace string, typ string) ([]Key, error) {
+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 {
@@ -70,12 +75,37 @@ func (f FakeSecretsKVStore) Keys(ctx context.Context, orgId int64, namespace str
 	return res, nil
 }
 
-func (f FakeSecretsKVStore) Rename(ctx context.Context, orgId int64, namespace string, typ string, newNamespace string) error {
+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 (f *FakeSecretsKVStore) GetAll(ctx context.Context) ([]Item, error) {
+	items := make([]Item, 0)
+	for k := range f.store {
+		orgId := k.OrgId
+		namespace := k.Namespace
+		typ := k.Type
+		items = append(items, Item{
+			OrgId:     &orgId,
+			Namespace: &namespace,
+			Type:      &typ,
+			Value:     f.store[k],
+		})
+	}
+	return items, nil
+}
+
+func (f *FakeSecretsKVStore) Fallback() SecretsKVStore {
+	return f.fallback
+}
+
+func (f *FakeSecretsKVStore) SetFallback(store SecretsKVStore) error {
+	f.fallback = store
+	return nil
+}
+
 func buildKey(orgId int64, namespace string, typ string) Key {
 	return Key{
 		OrgId:     orgId,
@@ -128,7 +158,17 @@ func (c *fakeGRPCSecretsPlugin) RenameSecret(ctx context.Context, in *secretsman
 	return &secretsmanagerplugin.RenameSecretResponse{}, nil
 }
 
-var _ SecretsKVStore = FakeSecretsKVStore{}
+func (c *fakeGRPCSecretsPlugin) GetAllSecrets(ctx context.Context, in *secretsmanagerplugin.GetAllSecretsRequest, opts ...grpc.CallOption) (*secretsmanagerplugin.GetAllSecretsResponse, error) {
+	return &secretsmanagerplugin.GetAllSecretsResponse{
+		Items: []*secretsmanagerplugin.Item{
+			{
+				Value: "bogus",
+			},
+		},
+	}, nil
+}
+
+var _ SecretsKVStore = &FakeSecretsKVStore{}
 var _ secretsmanagerplugin.SecretsManagerPlugin = &fakeGRPCSecretsPlugin{}
 
 // Fake plugin manager