From c043a8818a978fba05e54420a2f3dcad047f1af6 Mon Sep 17 00:00:00 2001 From: Michael Mandrus <41969079+mmandrus@users.noreply.github.com> Date: Thu, 16 Jun 2022 12:26:57 -0400 Subject: [PATCH] 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 * Update pkg/services/secrets/kvstore/kvstore.go Co-authored-by: Marcus Efraimsson * 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 --- pkg/api/datasources.go | 18 ++ pkg/models/datasource.go | 16 ++ .../datasources/service/datasource_service.go | 165 +++++++++--------- pkg/services/secrets/kvstore/remote_plugin.go | 16 +- pkg/services/sqlstore/datasource.go | 21 +++ 5 files changed, 150 insertions(+), 86 deletions(-) diff --git a/pkg/api/datasources.go b/pkg/api/datasources.go index ee03e50fba9..5f9d654ce01 100644 --- a/pkg/api/datasources.go +++ b/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) } diff --git a/pkg/models/datasource.go b/pkg/models/datasource.go index dc2b3e2614a..2ede3910fd2 100644 --- a/pkg/models/datasource.go +++ b/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 diff --git a/pkg/services/datasources/service/datasource_service.go b/pkg/services/datasources/service/datasource_service.go index d2f32c4ec67..8d2f6907374 100644 --- a/pkg/services/datasources/service/datasource_service.go +++ b/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 - // this is here for backwards compatibility - cmd.EncryptedSecureJsonData, err = s.SecretsService.EncryptJsonData(ctx, cmd.SecureJsonData, secrets.WithoutScope()) - if err != nil { - 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) + return s.SQLStore.InTransaction(ctx, func(ctx context.Context) error { + var err error + // this is here for backwards compatibility + cmd.EncryptedSecureJsonData, err = s.SecretsService.EncryptJsonData(ctx, cmd.SecureJsonData, secrets.WithoutScope()) if err != nil { return err } - } - secret, err := json.Marshal(cmd.SecureJsonData) - if err != nil { - return err - } + secret, err := json.Marshal(cmd.SecureJsonData) + if err != nil { + 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 { diff --git a/pkg/services/secrets/kvstore/remote_plugin.go b/pkg/services/secrets/kvstore/remote_plugin.go index 6cb3ebb91d0..56d496d03a8 100644 --- a/pkg/services/secrets/kvstore/remote_plugin.go +++ b/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} +} diff --git a/pkg/services/sqlstore/datasource.go b/pkg/services/sqlstore/datasource.go index 1c831bec3ab..20f429cbe94 100644 --- a/pkg/services/sqlstore/datasource.go +++ b/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 })