Secrets: add better error handling for secret plugin failures when updating datasources (#50542)

* Add protobuf config and generated code, and client wrapper

* wire up loading of secretsmanager plugin, using renderer plugin as a model

* update kvstore provider to check if we should use the grpc plugin. return false always in OSS

* add OSS remote plugin check

* refactor wire gen file

* log which secrets manager is being used

* Fix argument types for remote checker

* Turns out if err != nil, then the result is always nil. Return empty values if there is an error.

* remove duplicate import

* ensure atomicity by adding secret management as a step to sql operations and rolling back if necessary

* Update pkg/services/secrets/kvstore/kvstore.go

Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>

* Update pkg/services/secrets/kvstore/kvstore.go

Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>

* refactor RemotePluginCheck interface to just return the Plugin client directly

* rename struct to something less silly

* add special error handling for remote secrets management

* switch to errors.as instead of type inference

* remove unnecessary rollback call

* just declare error once

* refactor .proto file according to prior PR suggestions

* re-generate protobuf files and fix compilation errors

* only wrap (ergo display in the front end) errors that are user friendly from the plugin

* rename error type to suggest user friendly only

* rename plugin functions to be more descriptive

* change delete message name

* Revert "change delete message name"

This reverts commit 8ca978301eea45d3cc6a22cda15b9d099c533955.

* Revert "rename plugin functions to be more descriptive"

This reverts commit 4355c9b9ff95443f74c0b588d1ffeabb544f1a34.

* fix pointer to pointer problem

* change plugin user error to just hold a string

* fix sequencing problem with datasource updates

* clean up some return statements

* need to wrap multiple transactions with the InTransaction() func in order to keep the lock

* make linter happy

* revert input var name

Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>

pkg/api/datasources.go

@@ -24,6 +24,7 @@ import (
 )
 
 var datasourcesLogger = log.New("datasources")
+var secretsPluginError models.ErrDatasourceSecretsPluginUserFriendly
 
 func (hs *HTTPServer) GetDataSources(c *models.ReqContext) response.Response {
 	query := models.GetDataSourcesQuery{OrgId: c.OrgId, DataSourceLimit: hs.Cfg.DataSourceLimit}
@@ -127,6 +128,9 @@ func (hs *HTTPServer) DeleteDataSourceById(c *models.ReqContext) response.Respon
 
 	err = hs.DataSourcesService.DeleteDataSource(c.Req.Context(), cmd)
 	if err != nil {
+		if errors.As(err, &secretsPluginError) {
+			return response.Error(500, "Failed to delete datasource: "+err.Error(), err)
+		}
 		return response.Error(500, "Failed to delete datasource", err)
 	}
 
@@ -178,6 +182,9 @@ func (hs *HTTPServer) DeleteDataSourceByUID(c *models.ReqContext) response.Respo
 
 	err = hs.DataSourcesService.DeleteDataSource(c.Req.Context(), cmd)
 	if err != nil {
+		if errors.As(err, &secretsPluginError) {
+			return response.Error(500, "Failed to delete datasource: "+err.Error(), err)
+		}
 		return response.Error(500, "Failed to delete datasource", err)
 	}
 
@@ -212,6 +219,9 @@ func (hs *HTTPServer) DeleteDataSourceByName(c *models.ReqContext) response.Resp
 	cmd := &models.DeleteDataSourceCommand{Name: name, OrgID: c.OrgId}
 	err := hs.DataSourcesService.DeleteDataSource(c.Req.Context(), cmd)
 	if err != nil {
+		if errors.As(err, &secretsPluginError) {
+			return response.Error(500, "Failed to delete datasource: "+err.Error(), err)
+		}
 		return response.Error(500, "Failed to delete datasource", err)
 	}
 
@@ -252,6 +262,10 @@ func (hs *HTTPServer) AddDataSource(c *models.ReqContext) response.Response {
 			return response.Error(409, err.Error(), err)
 		}
 
+		if errors.As(err, &secretsPluginError) {
+			return response.Error(500, "Failed to add datasource: "+err.Error(), err)
+		}
+
 		return response.Error(500, "Failed to add datasource", err)
 	}
 
@@ -323,6 +337,10 @@ func (hs *HTTPServer) updateDataSourceByID(c *models.ReqContext, ds *models.Data
 		if errors.Is(err, models.ErrDataSourceUpdatingOldVersion) {
 			return response.Error(409, "Datasource has already been updated by someone else. Please reload and try again", err)
 		}
+
+		if errors.As(err, &secretsPluginError) {
+			return response.Error(500, "Failed to update datasource: "+err.Error(), err)
+		}
 		return response.Error(500, "Failed to update datasource", err)
 	}
 

pkg/models/datasource.go

@@ -81,6 +81,15 @@ func (ds DataSource) AllowedCookies() []string {
 	return []string{}
 }
 
+// Specific error type for grpc secrets management so that we can show more detailed plugin errors to users
+type ErrDatasourceSecretsPluginUserFriendly struct {
+	Err string
+}
+
+func (e ErrDatasourceSecretsPluginUserFriendly) Error() string {
+	return e.Err
+}
+
 // ----------------------
 // COMMANDS
 
@@ -104,6 +113,7 @@ type AddDataSourceCommand struct {
 	UserId                  int64             `json:"-"`
 	ReadOnly                bool              `json:"-"`
 	EncryptedSecureJsonData map[string][]byte `json:"-"`
+	UpdateSecretFn          UpdateSecretFn    `json:"-"`
 
 	Result *DataSource `json:"-"`
 }
@@ -129,6 +139,7 @@ type UpdateDataSourceCommand struct {
 	Id                      int64             `json:"-"`
 	ReadOnly                bool              `json:"-"`
 	EncryptedSecureJsonData map[string][]byte `json:"-"`
+	UpdateSecretFn          UpdateSecretFn    `json:"-"`
 
 	Result *DataSource `json:"-"`
 }
@@ -143,8 +154,13 @@ type DeleteDataSourceCommand struct {
 	OrgID int64
 
 	DeletedDatasourcesCount int64
+
+	UpdateSecretFn UpdateSecretFn
 }
 
+// Function for updating secrets along with datasources, to ensure atomicity
+type UpdateSecretFn func() error
+
 // ---------------------
 // QUERIES
 

pkg/services/datasources/service/datasource_service.go

@@ -142,91 +142,96 @@ func (s *Service) GetDataSourcesByType(ctx context.Context, query *models.GetDat
 }
 
 func (s *Service) AddDataSource(ctx context.Context, cmd *models.AddDataSourceCommand) error {
-	var err error
+	return s.SQLStore.InTransaction(ctx, func(ctx context.Context) error {
-	// this is here for backwards compatibility
+		var err error
-	cmd.EncryptedSecureJsonData, err = s.SecretsService.EncryptJsonData(ctx, cmd.SecureJsonData, secrets.WithoutScope())
+		// this is here for backwards compatibility
-	if err != nil {
+		cmd.EncryptedSecureJsonData, err = s.SecretsService.EncryptJsonData(ctx, cmd.SecureJsonData, secrets.WithoutScope())
-		return err
-	}
-
-	if err := s.SQLStore.AddDataSource(ctx, cmd); err != nil {
-		return err
-	}
-
-	secret, err := json.Marshal(cmd.SecureJsonData)
-	if err != nil {
-		return err
-	}
-
-	err = s.SecretsStore.Set(ctx, cmd.OrgId, cmd.Name, secretType, string(secret))
-	if err != nil {
-		return err
-	}
-
-	if !s.ac.IsDisabled() {
-		// This belongs in Data source permissions, and we probably want
-		// to do this with a hook in the store and rollback on fail.
-		// We can't use events, because there's no way to communicate
-		// failure, and we want "not being able to set default perms"
-		// to fail the creation.
-		permissions := []accesscontrol.SetResourcePermissionCommand{
-			{BuiltinRole: "Viewer", Permission: "Query"},
-			{BuiltinRole: "Editor", Permission: "Query"},
-		}
-		if cmd.UserId != 0 {
-			permissions = append(permissions, accesscontrol.SetResourcePermissionCommand{UserID: cmd.UserId, Permission: "Edit"})
-		}
-		if _, err := s.permissionsService.SetPermissions(ctx, cmd.OrgId, cmd.Result.Uid, permissions...); err != nil {
-			return err
-		}
-	}
-
-	return nil
-}
-
-func (s *Service) DeleteDataSource(ctx context.Context, cmd *models.DeleteDataSourceCommand) error {
-	err := s.SQLStore.DeleteDataSource(ctx, cmd)
-	if err != nil {
-		return err
-	}
-	return s.SecretsStore.Del(ctx, cmd.OrgID, cmd.Name, secretType)
-}
-
-func (s *Service) UpdateDataSource(ctx context.Context, cmd *models.UpdateDataSourceCommand) error {
-	var err error
-
-	query := &models.GetDataSourceQuery{
-		Id:    cmd.Id,
-		OrgId: cmd.OrgId,
-	}
-	err = s.SQLStore.GetDataSource(ctx, query)
-	if err != nil {
-		return err
-	}
-
-	err = s.fillWithSecureJSONData(ctx, cmd, query.Result)
-	if err != nil {
-		return err
-	}
-
-	err = s.SQLStore.UpdateDataSource(ctx, cmd)
-	if err != nil {
-		return err
-	}
-
-	if query.Result.Name != cmd.Name {
-		err = s.SecretsStore.Rename(ctx, cmd.OrgId, query.Result.Name, secretType, cmd.Name)
 		if err != nil {
 			return err
 		}
-	}
 
-	secret, err := json.Marshal(cmd.SecureJsonData)
+		secret, err := json.Marshal(cmd.SecureJsonData)
-	if err != nil {
+		if err != nil {
-		return err
+			return err
-	}
+		}
 
-	return s.SecretsStore.Set(ctx, cmd.OrgId, cmd.Name, secretType, string(secret))
+		cmd.UpdateSecretFn = func() error {
+			return s.SecretsStore.Set(ctx, cmd.OrgId, cmd.Name, secretType, string(secret))
+		}
+
+		if err := s.SQLStore.AddDataSource(ctx, cmd); err != nil {
+			return err
+		}
+
+		if !s.ac.IsDisabled() {
+			// This belongs in Data source permissions, and we probably want
+			// to do this with a hook in the store and rollback on fail.
+			// We can't use events, because there's no way to communicate
+			// failure, and we want "not being able to set default perms"
+			// to fail the creation.
+			permissions := []accesscontrol.SetResourcePermissionCommand{
+				{BuiltinRole: "Viewer", Permission: "Query"},
+				{BuiltinRole: "Editor", Permission: "Query"},
+			}
+			if cmd.UserId != 0 {
+				permissions = append(permissions, accesscontrol.SetResourcePermissionCommand{UserID: cmd.UserId, Permission: "Edit"})
+			}
+			if _, err := s.permissionsService.SetPermissions(ctx, cmd.OrgId, cmd.Result.Uid, permissions...); err != nil {
+				return err
+			}
+		}
+
+		return nil
+	})
+}
+
+func (s *Service) DeleteDataSource(ctx context.Context, cmd *models.DeleteDataSourceCommand) error {
+	return s.SQLStore.InTransaction(ctx, func(ctx context.Context) error {
+		cmd.UpdateSecretFn = func() error {
+			return s.SecretsStore.Del(ctx, cmd.OrgID, cmd.Name, secretType)
+		}
+
+		return s.SQLStore.DeleteDataSource(ctx, cmd)
+	})
+}
+
+func (s *Service) UpdateDataSource(ctx context.Context, cmd *models.UpdateDataSourceCommand) error {
+	return s.SQLStore.InTransaction(ctx, func(ctx context.Context) error {
+		var err error
+
+		query := &models.GetDataSourceQuery{
+			Id:    cmd.Id,
+			OrgId: cmd.OrgId,
+		}
+		err = s.SQLStore.GetDataSource(ctx, query)
+		if err != nil {
+			return err
+		}
+
+		err = s.fillWithSecureJSONData(ctx, cmd, query.Result)
+		if err != nil {
+			return err
+		}
+
+		secret, err := json.Marshal(cmd.SecureJsonData)
+		if err != nil {
+			return err
+		}
+
+		cmd.UpdateSecretFn = func() error {
+			var secretsErr error
+			if query.Result.Name != cmd.Name {
+				secretsErr = s.SecretsStore.Rename(ctx, cmd.OrgId, query.Result.Name, secretType, cmd.Name)
+			}
+			if secretsErr != nil {
+				return secretsErr
+			}
+
+			return s.SecretsStore.Set(ctx, cmd.OrgId, cmd.Name, secretType, string(secret))
+		}
+
+		return s.SQLStore.UpdateDataSource(ctx, cmd)
+	})
 }
 
 func (s *Service) GetDefaultDataSource(ctx context.Context, query *models.GetDefaultDataSourceQuery) error {

pkg/services/secrets/kvstore/remote_plugin.go

@@ -2,9 +2,9 @@ package kvstore
 
 import (
 	"context"
-	"fmt"
 
 	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/models"
 	smp "github.com/grafana/grafana/pkg/plugins/backendplugin/secretsmanagerplugin"
 	"github.com/grafana/grafana/pkg/services/secrets"
 )
@@ -29,7 +29,7 @@ func (kv *secretsKVStorePlugin) Get(ctx context.Context, orgId int64, namespace
 	if err != nil {
 		return "", false, err
 	} else if res.UserFriendlyError != "" {
-		err = fmt.Errorf(res.UserFriendlyError)
+		err = wrapUserFriendlySecretError(res.UserFriendlyError)
 	}
 
 	return res.DecryptedValue, res.Exists, err
@@ -48,7 +48,7 @@ func (kv *secretsKVStorePlugin) Set(ctx context.Context, orgId int64, namespace
 
 	res, err := kv.secretsPlugin.SetSecret(ctx, req)
 	if err == nil && res.UserFriendlyError != "" {
-		err = fmt.Errorf(res.UserFriendlyError)
+		err = wrapUserFriendlySecretError(res.UserFriendlyError)
 	}
 
 	return err
@@ -66,7 +66,7 @@ func (kv *secretsKVStorePlugin) Del(ctx context.Context, orgId int64, namespace
 
 	res, err := kv.secretsPlugin.DeleteSecret(ctx, req)
 	if err == nil && res.UserFriendlyError != "" {
-		err = fmt.Errorf(res.UserFriendlyError)
+		err = wrapUserFriendlySecretError(res.UserFriendlyError)
 	}
 
 	return err
@@ -88,7 +88,7 @@ func (kv *secretsKVStorePlugin) Keys(ctx context.Context, orgId int64, namespace
 	if err != nil {
 		return nil, err
 	} else if res.UserFriendlyError != "" {
-		err = fmt.Errorf(res.UserFriendlyError)
+		err = wrapUserFriendlySecretError(res.UserFriendlyError)
 	}
 
 	return parseKeys(res.Keys), err
@@ -107,7 +107,7 @@ func (kv *secretsKVStorePlugin) Rename(ctx context.Context, orgId int64, namespa
 
 	res, err := kv.secretsPlugin.RenameSecret(ctx, req)
 	if err == nil && res.UserFriendlyError != "" {
-		err = fmt.Errorf(res.UserFriendlyError)
+		err = wrapUserFriendlySecretError(res.UserFriendlyError)
 	}
 
 	return err
@@ -123,3 +123,7 @@ func parseKeys(keys []*smp.Key) []Key {
 
 	return newKeys
 }
+
+func wrapUserFriendlySecretError(ufe string) models.ErrDatasourceSecretsPluginUserFriendly {
+	return models.ErrDatasourceSecretsPluginUserFriendly{Err: ufe}
+}

pkg/services/sqlstore/datasource.go

@@ -116,6 +116,13 @@ func (ss *SQLStore) DeleteDataSource(ctx context.Context, cmd *models.DeleteData
 			}
 		}
 
+		if cmd.UpdateSecretFn != nil {
+			if err := cmd.UpdateSecretFn(); err != nil {
+				sqlog.Error("Failed to update datasource secrets -- rolling back update", "UID", cmd.UID, "name", cmd.Name, "orgId", cmd.OrgID)
+				return err
+			}
+		}
+
 		// Publish data source deletion event
 		sess.publishAfterCommit(&events.DataSourceDeleted{
 			Timestamp: time.Now(),
@@ -181,6 +188,13 @@ func (ss *SQLStore) AddDataSource(ctx context.Context, cmd *models.AddDataSource
 			return err
 		}
 
+		if cmd.UpdateSecretFn != nil {
+			if err := cmd.UpdateSecretFn(); err != nil {
+				sqlog.Error("Failed to update datasource secrets -- rolling back update", "name", cmd.Name, "type", cmd.Type, "orgId", cmd.OrgId)
+				return err
+			}
+		}
+
 		cmd.Result = ds
 
 		sess.publishAfterCommit(&events.DataSourceCreated{
@@ -263,6 +277,13 @@ func (ss *SQLStore) UpdateDataSource(ctx context.Context, cmd *models.UpdateData
 
 		err = updateIsDefaultFlag(ds, sess)
 
+		if cmd.UpdateSecretFn != nil {
+			if err := cmd.UpdateSecretFn(); err != nil {
+				sqlog.Error("Failed to update datasource secrets -- rolling back update", "UID", cmd.Uid, "name", cmd.Name, "type", cmd.Type, "orgId", cmd.OrgId)
+				return err
+			}
+		}
+
 		cmd.Result = ds
 		return err
 	})