From 0670e272e3160c40dc37b707239448d7938cb94d Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Fri, 5 Nov 2021 10:43:58 -0700 Subject: [PATCH] internal/host/plugin: Add UpdateCatalog (#1634) * internal/host/plugin: Add UpdateCatalog This adds support for updating plugin-based host catalogs. The workflow is: * The existing catalog is fetched. * Any attribute changes are patched against it, along with other changes such as name and description. * Both the existing catalog, and the state of the catalog as it will exist post-update, along with any persisted secrets, are sent to the plugin via an OnUpdateCatalog request. Upon success, the following is done: * The record is updated if non-secret updates need to be made. * Secrets are updated if they still exist. * If an empty secret set was sent back, secrets are instead deleted. * If the secrets field was instead null, secret update is skipped altogether. There has also been a lot of helper code added to the tests to help make the update tests more readable and reduce boilerplate. While the create tests have not been updated to use these new helpers, it might be a good idea to do so for consistency. * move secret/persisted state closer to plugin call * Update internal/host/plugin/repository_host_catalog.go Co-authored-by: Todd * Update internal/host/plugin/repository_host_catalog_test.go Co-authored-by: Todd * move all test helpers for UpdateCatalog inline * WIP - transparent attributes struct/wire conversion * Revert "WIP - transparent attributes struct/wire conversion" This reverts commit 31af1b1b7e535119aa23bd0aa936da1a80eb5aa2. * deal with multiple attribute masks * Note TODO for upsertQuery for HostCatalogSecret * add version check guard * Update internal/host/plugin/host_catalog_secret.go Co-authored-by: Todd * remove redundant JIRA reference * don't separate number of updated catalogs and secrets * increment version when secrets is the only thing being updated * add another test for updating both catalog and secrets at once * correct comment for mock plugin manager section * add first-part path matching for secrets * update plugin lookup error to return errors.Internal * only add name/description to plugin request if they are non-empty * update duplicate tests to use two project scopes * remove unused test checks * return plugin record along with host catalog in update * move away from using passed in catalog after patching newCatalog * use t.Cleanup() instead of defer Co-authored-by: Todd --- internal/host/plugin/host_catalog_secret.go | 7 +- .../host/plugin/host_catalog_secret_test.go | 4 +- .../host/plugin/repository_host_catalog.go | 312 +++++++- .../plugin/repository_host_catalog_test.go | 751 +++++++++++++++++- 4 files changed, 1062 insertions(+), 12 deletions(-) diff --git a/internal/host/plugin/host_catalog_secret.go b/internal/host/plugin/host_catalog_secret.go index 7daf0abf3..84eda4867 100644 --- a/internal/host/plugin/host_catalog_secret.go +++ b/internal/host/plugin/host_catalog_secret.go @@ -90,7 +90,11 @@ func (c *HostCatalogSecret) decrypt(ctx context.Context, cipher wrapping.Wrapper return nil } -func (c *HostCatalogSecret) insertQuery() (query string, queryValues []interface{}) { +// TODO(ICU-2835): this is necessary as update logic requires a static +// public_id or private_id field. Once post-lookup updates can +// support alternate primary key fields, we can port this to native +// Update and upserts w/OnConflict. +func (c *HostCatalogSecret) upsertQuery() (query string, queryValues []interface{}) { query = upsertHostCatalogSecretQuery queryValues = []interface{}{ sql.Named("catalog_id", c.CatalogId), @@ -118,6 +122,7 @@ func (c *HostCatalogSecret) oplogMessage(opType db.OpType) *oplog.Message { msg.OpType = oplog.OpType_OP_TYPE_CREATE case db.UpdateOp: msg.OpType = oplog.OpType_OP_TYPE_UPDATE + msg.FieldMaskPaths = []string{"secret", "key_id"} case db.DeleteOp: msg.OpType = oplog.OpType_OP_TYPE_DELETE } diff --git a/internal/host/plugin/host_catalog_secret_test.go b/internal/host/plugin/host_catalog_secret_test.go index 9a24404b1..09bb7af02 100644 --- a/internal/host/plugin/host_catalog_secret_test.go +++ b/internal/host/plugin/host_catalog_secret_test.go @@ -135,7 +135,7 @@ func TestHostCatalogSecret_Custom_Queries(t *testing.T) { &structpb.Struct{Fields: map[string]*structpb.Value{"foo": structpb.NewStringValue("bar")}}) require.NoError(t, err) assert.NoError(t, hcs.encrypt(ctx, databaseWrapper)) - q, v := hcs.insertQuery() + q, v := hcs.upsertQuery() _, err = rw.Exec(ctx, q, v) assert.NoError(t, err) @@ -153,7 +153,7 @@ func TestHostCatalogSecret_Custom_Queries(t *testing.T) { &structpb.Struct{Fields: map[string]*structpb.Value{"updated": structpb.NewStringValue("value")}}) require.NoError(t, err) assert.NoError(t, updated.encrypt(ctx, databaseWrapper)) - q, v = updated.insertQuery() + q, v = updated.upsertQuery() _, err = rw.Exec(ctx, q, v) assert.NoError(t, err) diff --git a/internal/host/plugin/repository_host_catalog.go b/internal/host/plugin/repository_host_catalog.go index 6007106d3..dda662693 100644 --- a/internal/host/plugin/repository_host_catalog.go +++ b/internal/host/plugin/repository_host_catalog.go @@ -3,11 +3,13 @@ package plugin import ( "context" "fmt" + "strings" "github.com/hashicorp/boundary/internal/db" "github.com/hashicorp/boundary/internal/errors" "github.com/hashicorp/boundary/internal/host" "github.com/hashicorp/boundary/internal/kms" + "github.com/hashicorp/boundary/internal/libs/patchstruct" "github.com/hashicorp/boundary/internal/observability/event" "github.com/hashicorp/boundary/internal/oplog" hostplugin "github.com/hashicorp/boundary/internal/plugin/host" @@ -18,6 +20,7 @@ import ( "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/structpb" + "google.golang.org/protobuf/types/known/wrapperspb" ) // CreateCatalog inserts c into the repository and returns a new @@ -113,7 +116,7 @@ func (r *Repository) CreateCatalog(ctx context.Context, c *HostCatalog, _ ...Opt if hcSecret != nil { newSecret := hcSecret.clone() - q, v := newSecret.insertQuery() + q, v := newSecret.upsertQuery() rows, err := w.Exec(ctx, q, v) if err != nil { return errors.Wrap(ctx, err, op) @@ -145,6 +148,299 @@ func (r *Repository) CreateCatalog(ctx context.Context, c *HostCatalog, _ ...Opt return newHostCatalog, plg, nil } +// UpdateCatalog updates the repository entry for c.PublicId with the +// values in c for the fields listed in fieldMask. It returns a new +// HostCatalog containing the updated values and a count of the number of +// records updated. c is not changed. +// +// c must contain a valid PublicId. c.Name, c.Description, and +// c.Attributes can be updated; if c.Secrets is present, its contents +// are sent to the plugin (along with any other changes, see below) +// before the update is sent to the database. +// +// An attribute of c will be set to NULL in the database if the +// attribute in c is the zero value and it is included in fieldMask. +// Note that this does not apply to c.Attributes - a null +// c.Attributes is a no-op for modifications. Rather, if fields need +// to be reset, its field in c.Attributes should individually set to +// null. +// +// Updates are sent to OnUpdateCatalog with a full copy of both the +// current catalog, and the state of the new catalog should it be +// updated, along with any secrets included in the new request. This +// request may alter the returned persisted state. Update of the +// record in the database is aborted if this call fails. +func (r *Repository) UpdateCatalog(ctx context.Context, c *HostCatalog, version uint32, fieldMask []string, _ ...Option) (*HostCatalog, *hostplugin.Plugin, int, error) { + const op = "plugin.(Repository).UpdateCatalog" + if c == nil { + return nil, nil, db.NoRowsAffected, errors.New(ctx, errors.InvalidParameter, op, "nil HostCatalog") + } + if c.HostCatalog == nil { + return nil, nil, db.NoRowsAffected, errors.New(ctx, errors.InvalidParameter, op, "nil embedded HostCatalog") + } + if c.PublicId == "" { + return nil, nil, db.NoRowsAffected, errors.New(ctx, errors.InvalidParameter, op, "no public id") + } + if c.ScopeId == "" { + return nil, nil, db.NoRowsAffected, errors.New(ctx, errors.InvalidParameter, op, "no scope id") + } + if len(fieldMask) == 0 { + return nil, nil, db.NoRowsAffected, errors.New(ctx, errors.EmptyFieldMask, op, "empty field mask") + } + + // Get the old catalog first. We patch the record first before + // sending it to the DB for updating so that we can run on + // OnUpdateCatalog. Note that the field masks are still used for + // updating. + currentCatalog, currentCatalogPersisted, err := r.getCatalog(ctx, c.PublicId) + if err != nil { + return nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op, errors.WithMsg(fmt.Sprintf("error looking up catalog with id %q", c.PublicId))) + } + + if currentCatalog == nil { + return nil, nil, db.NoRowsAffected, errors.New(ctx, errors.RecordNotFound, op, fmt.Sprintf("catalog with id %q not found", c.PublicId)) + } + + // Assert the version of the current catalog to make sure we're + // updating the correct one. + if currentCatalog.GetVersion() != version { + return nil, nil, db.NoRowsAffected, errors.New(ctx, errors.VersionMismatch, op, fmt.Sprintf("catalog version mismatch, want=%d, got=%d", currentCatalog.GetVersion(), version)) + } + + // Clone the catalog so that we can set fields. + newCatalog := currentCatalog.clone() + var updateAttributes bool + var dbMask, nullFields []string + for _, f := range fieldMask { + switch { + case strings.EqualFold("name", f) && c.Name == "": + nullFields = append(nullFields, "name") + newCatalog.Name = c.Name + case strings.EqualFold("name", f) && c.Name != "": + dbMask = append(dbMask, "name") + newCatalog.Name = c.Name + case strings.EqualFold("description", f) && c.Description == "": + nullFields = append(nullFields, "description") + newCatalog.Description = c.Description + case strings.EqualFold("description", f) && c.Description != "": + dbMask = append(dbMask, "description") + newCatalog.Description = c.Description + case strings.EqualFold("attributes", strings.Split(f, ".")[0]): + // Flag attributes for updating. While multiple masks may be + // sent, we only need to do this once. + updateAttributes = true + case strings.EqualFold("secrets", strings.Split(f, ".")[0]): + // While in a similar format, secrets are passed along + // wholesale (for the time being). Don't append this mask + // field, as secrets do not have a database entry. Clear the + // secrets out of the working set after. + newCatalog.Secrets = c.Secrets + c.Secrets = nil + + default: + return nil, nil, db.NoRowsAffected, errors.New(ctx, errors.InvalidFieldMask, op, fmt.Sprintf("invalid field mask: %s", f)) + } + } + + if updateAttributes && c.Attributes != nil { + dbMask = append(dbMask, "attributes") + newCatalog.Attributes, err = patchstruct.PatchBytes(newCatalog.Attributes, c.Attributes) + if err != nil { + return nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op, errors.WithMsg("error in catalog attribute JSON")) + } + } + + // Get the plugin for the host catalog - this is to return it back + // after the update is complete. We don't actually do anything with + // the record otherwise. Fetch it here so that if there's an + // integrity error, we don't call the plugin. + plg, err := r.getPlugin(ctx, currentCatalog.GetPluginId()) + if err != nil { + return nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op) + } + + // Get the plugin client. + plgClient, ok := r.plugins[currentCatalog.GetPluginId()] + if !ok || plgClient == nil { + return nil, nil, db.NoRowsAffected, errors.New(ctx, errors.Internal, op, fmt.Sprintf("plugin %q not available", currentCatalog.GetPluginId())) + } + + // Convert the catalog values to API protobuf values, which is what + // we use for the plugin hook calls. + currPlgHc, err := toPluginCatalog(ctx, currentCatalog) + if err != nil { + return nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op) + } + newPlgHc, err := toPluginCatalog(ctx, newCatalog) + if err != nil { + return nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op) + } + + plgResp, err := plgClient.OnUpdateCatalog(ctx, &plgpb.OnUpdateCatalogRequest{ + CurrentCatalog: currPlgHc, + NewCatalog: newPlgHc, + Persisted: currentCatalogPersisted, + }) + if err != nil { + if status.Code(err) != codes.Unimplemented { + return nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op) + } + } + + // Success for OnUpdateCatalog. This means we can start updating. + // First, check for returned persisted data and encrypt it. + var hcSecret *HostCatalogSecret + var deleteSecrets bool + if plgResp != nil && plgResp.GetPersisted().GetSecrets() != nil { + if len(plgResp.GetPersisted().GetSecrets().GetFields()) == 0 { + // Flag the secret to be deleted. + hcSecret, err = newHostCatalogSecret(ctx, currentCatalog.GetPublicId(), plgResp.GetPersisted().GetSecrets()) + if err != nil { + return nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op) + } + + deleteSecrets = true + } else { + hcSecret, err = newHostCatalogSecret(ctx, currentCatalog.GetPublicId(), plgResp.GetPersisted().GetSecrets()) + if err != nil { + return nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op) + } + dbWrapper, err := r.kms.GetWrapper(ctx, newCatalog.ScopeId, kms.KeyPurposeDatabase) + if err != nil { + return nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op, errors.WithMsg("unable to get db wrapper")) + } + if err := hcSecret.encrypt(ctx, dbWrapper); err != nil { + return nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op) + } + } + } + + // Get the oplog. + oplogWrapper, err := r.kms.GetWrapper(ctx, newCatalog.ScopeId, kms.KeyPurposeOplog) + if err != nil { + return nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op, errors.WithMsg("unable to get oplog wrapper")) + } + + var recordUpdated bool + var returnedCatalog *HostCatalog + _, err = r.writer.DoTx( + ctx, + db.StdRetryCnt, + db.ExpBackoff{}, + func(_ db.Reader, w db.Writer) error { + msgs := make([]*oplog.Message, 0, 3) + ticket, err := w.GetTicket(newCatalog) + if err != nil { + return errors.Wrap(ctx, err, op, errors.WithMsg("unable to get ticket")) + } + + if len(dbMask) != 0 || len(nullFields) != 0 { + returnedCatalog = newCatalog.clone() + var cOplogMsg oplog.Message + catalogsUpdated, err := w.Update( + ctx, + returnedCatalog, + dbMask, + nullFields, + db.NewOplogMsg(&cOplogMsg), + db.WithVersion(&version), + ) + if err != nil { + return errors.Wrap(ctx, err, op) + } + if catalogsUpdated != 1 { + return errors.New(ctx, errors.MultipleRecords, op, fmt.Sprintf("expected 1 catalog to be deleted, got %d", catalogsUpdated)) + } + msgs = append(msgs, &cOplogMsg) + recordUpdated = true + } else { + // Returned catalog needs to be the old copy, as no fields in the + // catalog itself are being updated (note: secrets may still be + // updated). + returnedCatalog = currentCatalog.clone() + } + + if hcSecret != nil { + if deleteSecrets { + // We didn't set/encrypt the persisted data because there was + // none returned. Just delete the entry. + deletedSecret := hcSecret.clone() + q, v := deletedSecret.deleteQuery() + var err error + secretsUpdated, err := w.Exec(ctx, q, v) + if err != nil { + return errors.Wrap(ctx, err, op) + } + if secretsUpdated != 1 { + return errors.New(ctx, errors.MultipleRecords, op, fmt.Sprintf("expected 1 catalog secret to be deleted, got %d", secretsUpdated)) + } + msgs = append(msgs, deletedSecret.oplogMessage(db.DeleteOp)) + } else { + // Update the secrets. + updatedSecret := hcSecret.clone() + q, v := updatedSecret.upsertQuery() + var err error + secretsUpdated, err := w.Exec(ctx, q, v) + if err != nil { + return errors.Wrap(ctx, err, op) + } + if secretsUpdated != 1 { + return errors.New(ctx, errors.MultipleRecords, op, fmt.Sprintf("expected 1 catalog secret to be updated, got %d", secretsUpdated)) + } + msgs = append(msgs, updatedSecret.oplogMessage(db.UpdateOp)) + } + + if !recordUpdated { + // we only updated secrets, so we need to increment the + // version of the host catalog manually. + returnedCatalog = newCatalog.clone() + returnedCatalog.Version = uint32(version) + 1 + var cOplogMsg oplog.Message + catalogsUpdated, err := w.Update( + ctx, + returnedCatalog, + []string{"version"}, + []string{}, + db.NewOplogMsg(&cOplogMsg), + db.WithVersion(&version), + ) + if err != nil { + return errors.Wrap(ctx, err, op) + } + if catalogsUpdated != 1 { + return errors.New(ctx, errors.MultipleRecords, op, fmt.Sprintf("expected 1 catalog to be updated, got %d", catalogsUpdated)) + } + msgs = append(msgs, &cOplogMsg) + recordUpdated = true + } + } + + if len(msgs) != 0 { + metadata := newCatalog.oplog(oplog.OpType_OP_TYPE_UPDATE) + if err := w.WriteOplogEntryWith(ctx, oplogWrapper, ticket, metadata, msgs); err != nil { + return errors.Wrap(ctx, err, op, errors.WithMsg("unable to write oplog")) + } + } + + return nil + }, + ) + + if err != nil { + if errors.IsUniqueError(err) { + return nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op, errors.WithMsg(fmt.Sprintf("in %s: name %s already exists", newCatalog.PublicId, newCatalog.Name))) + } + return nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op, errors.WithMsg(fmt.Sprintf("in %s", newCatalog.PublicId))) + } + + var numUpdated int + if recordUpdated { + numUpdated = 1 + } + + return returnedCatalog, plg, numUpdated, nil +} + // LookupCatalog returns the HostCatalog for id. Returns nil, nil if no // HostCatalog is found for id. func (r *Repository) LookupCatalog(ctx context.Context, id string, _ ...Option) (*HostCatalog, *hostplugin.Plugin, error) { @@ -322,9 +618,19 @@ func toPluginCatalog(ctx context.Context, in *HostCatalog) (*pb.HostCatalog, err if in == nil { return nil, errors.New(ctx, errors.InvalidParameter, op, "nil storage plugin") } + var name, description *wrapperspb.StringValue + if inName := in.GetName(); inName != "" { + name = wrapperspb.String(inName) + } + if inDescription := in.GetDescription(); inDescription != "" { + description = wrapperspb.String(inDescription) + } + hc := &pb.HostCatalog{ - Id: in.GetPublicId(), - ScopeId: in.GetScopeId(), + Id: in.GetPublicId(), + ScopeId: in.GetScopeId(), + Name: name, + Description: description, } if in.GetAttributes() != nil { attrs := &structpb.Struct{} diff --git a/internal/host/plugin/repository_host_catalog_test.go b/internal/host/plugin/repository_host_catalog_test.go index e8ac5db4d..e0333af1b 100644 --- a/internal/host/plugin/repository_host_catalog_test.go +++ b/internal/host/plugin/repository_host_catalog_test.go @@ -15,6 +15,7 @@ import ( "github.com/hashicorp/boundary/internal/kms" "github.com/hashicorp/boundary/internal/oplog" hostplg "github.com/hashicorp/boundary/internal/plugin/host" + hostplugin "github.com/hashicorp/boundary/internal/plugin/host" "github.com/hashicorp/boundary/internal/scheduler" plgpb "github.com/hashicorp/boundary/sdk/pbs/plugin" "github.com/stretchr/testify/assert" @@ -22,6 +23,7 @@ import ( "google.golang.org/protobuf/proto" "google.golang.org/protobuf/testing/protocmp" "google.golang.org/protobuf/types/known/structpb" + "google.golang.org/protobuf/types/known/wrapperspb" ) func TestRepository_CreateCatalog(t *testing.T) { @@ -368,12 +370,719 @@ func TestRepository_CreateCatalog(t *testing.T) { }) } -func assertPluginBasedPublicId(t *testing.T, prefix, actual string) { - t.Helper() - assert.NotEmpty(t, actual) - parts := strings.Split(actual, "_") - assert.Equalf(t, 2, len(parts), "want 1 '_' in PublicId, got %d in %q", len(parts), actual) - assert.Equalf(t, prefix, parts[0], "PublicId want prefix: %q, got: %q in %q", prefix, parts[0], actual) +func TestRepository_UpdateCatalog(t *testing.T) { + ctx := context.Background() + dbConn, _ := db.TestSetup(t, "postgres") + dbRW := db.New(dbConn) + dbWrapper := db.TestWrapper(t) + sched := scheduler.TestScheduler(t, dbConn, dbWrapper) + dbKmsCache := kms.TestKms(t, dbConn, dbWrapper) + _, projectScope := iam.TestScopes(t, iam.TestRepo(t, dbConn, dbWrapper)) + _, projectScopeAlt := iam.TestScopes(t, iam.TestRepo(t, dbConn, dbWrapper)) + + // Define a plugin "manager", basically just a map with a mock + // plugin in it. This also includes functionality to capture the + // state, and set an error and the returned secrets to nil. Note + // that the way that this is set up means that the tests cannot run + // in parallel, but there could be other factors affecting that as + // well. + var setRespSecretsNil bool + var gotOnUpdateCatalogRequest *plgpb.OnUpdateCatalogRequest + var pluginError error + testPlugin := hostplg.TestPlugin(t, dbConn, "test") + testPluginMap := map[string]plgpb.HostPluginServiceClient{ + testPlugin.GetPublicId(): &WrappingPluginClient{ + Server: &TestPluginServer{ + OnUpdateCatalogFn: func(_ context.Context, req *plgpb.OnUpdateCatalogRequest) (*plgpb.OnUpdateCatalogResponse, error) { + gotOnUpdateCatalogRequest = req + respSecrets := req.GetNewCatalog().GetSecrets() + if setRespSecretsNil { + respSecrets = nil + setRespSecretsNil = false + } + + return &plgpb.OnUpdateCatalogResponse{Persisted: &plgpb.HostCatalogPersisted{Secrets: respSecrets}}, pluginError + }, + }, + }, + } + + // Set up two existing catalogs for duplicate tests, one in the + // same scope that we are working in mostly, and another in a + // different one. Both are project scopes. + const ( + testDuplicateCatalogNameProjectScope = "duplicate-catalog-name-project-scope" + testDuplicateCatalogNameAltScope = "duplicate-catalog-name-alt-scope" + ) + + // Project scope + existingProjectScopeCatalog := TestCatalog(t, dbConn, projectScope.PublicId, testPlugin.GetPublicId()) + existingProjectScopeCatalog.Name = testDuplicateCatalogNameProjectScope + numCatUpdated, err := dbRW.Update(ctx, existingProjectScopeCatalog, []string{"name"}, []string{}) + require.NoError(t, err) + require.Equal(t, 1, numCatUpdated) + + // Alternate scope + existingAltScopeCatalog := TestCatalog(t, dbConn, projectScopeAlt.PublicId, testPlugin.GetPublicId()) + existingAltScopeCatalog.Name = testDuplicateCatalogNameAltScope + numCatUpdated, err = dbRW.Update(ctx, existingAltScopeCatalog, []string{"name"}, []string{}) + require.NoError(t, err) + require.Equal(t, 1, numCatUpdated) + + // Define some helpers here to make the test table more readable. + type changeHostCatalogFunc func(c *HostCatalog) *HostCatalog + + changePublicId := func(s string) changeHostCatalogFunc { + return func(c *HostCatalog) *HostCatalog { + c.PublicId = s + return c + } + } + + changeScopeId := func(s string) changeHostCatalogFunc { + return func(c *HostCatalog) *HostCatalog { + c.ScopeId = s + return c + } + } + + changeName := func(s string) changeHostCatalogFunc { + return func(c *HostCatalog) *HostCatalog { + c.Name = s + return c + } + } + + changeDescription := func(s string) changeHostCatalogFunc { + return func(c *HostCatalog) *HostCatalog { + c.Description = s + return c + } + } + + changeAttributes := func(m map[string]interface{}) changeHostCatalogFunc { + return func(c *HostCatalog) *HostCatalog { + c.Attributes = mustMarshal(m) + return c + } + } + + changeSecrets := func(m map[string]interface{}) changeHostCatalogFunc { + return func(c *HostCatalog) *HostCatalog { + c.Secrets = mustStruct(m) + return c + } + } + + changeCatalogToNil := func() changeHostCatalogFunc { + return func(_ *HostCatalog) *HostCatalog { + return nil + } + } + + changeEmbeddedCatalogToNil := func() changeHostCatalogFunc { + return func(c *HostCatalog) *HostCatalog { + c.HostCatalog = nil + return c + } + } + + // Define some checks that will be used in the below tests. Some of + // these are re-used, so we define them here. Most of these are + // assertions and no particular one is non-fatal in that they will + // stop execution. Note that these are executed after wantIsErr, so + // if that is set in an individual table test, these will not be + // executed. + // + // Note that we define some state here, similar to how we + // previously defined gotOnUpdateCatalogRequest above next the + // plugin map. + type checkFunc func(t *testing.T, ctx context.Context) + var ( + gotCatalog *HostCatalog + gotNumUpdated int + ) + + checkName := func(want string) checkFunc { + return func(t *testing.T, ctx context.Context) { + t.Helper() + assert := assert.New(t) + assert.Equal(want, gotCatalog.Name) + } + } + + checkDescription := func(want string) checkFunc { + return func(t *testing.T, ctx context.Context) { + t.Helper() + assert := assert.New(t) + assert.Equal(want, gotCatalog.Description) + } + } + + checkVersion := func(want uint32) checkFunc { + return func(t *testing.T, ctx context.Context) { + t.Helper() + assert := assert.New(t) + assert.Equal(want, gotCatalog.Version) + } + } + + checkAttributes := func(want map[string]interface{}) checkFunc { + return func(t *testing.T, ctx context.Context) { + t.Helper() + assert := assert.New(t) + require := require.New(t) + st := &structpb.Struct{} + require.NoError(proto.Unmarshal(gotCatalog.Attributes, st)) + assert.Empty(cmp.Diff(mustStruct(want), st, protocmp.Transform())) + } + } + + checkSecrets := func(want map[string]interface{}) checkFunc { + return func(t *testing.T, ctx context.Context) { + t.Helper() + assert := assert.New(t) + require := require.New(t) + + cSecret := allocHostCatalogSecret() + err := dbRW.LookupWhere(ctx, &cSecret, "catalog_id=?", gotCatalog.GetPublicId()) + require.NoError(err) + require.Empty(cSecret.Secret) + require.NotEmpty(cSecret.CtSecret) + + dbWrapper, err := dbKmsCache.GetWrapper(ctx, gotCatalog.GetScopeId(), kms.KeyPurposeDatabase) + require.NoError(err) + require.NoError(cSecret.decrypt(ctx, dbWrapper)) + + st := &structpb.Struct{} + require.NoError(proto.Unmarshal(cSecret.Secret, st)) + assert.Empty(cmp.Diff(mustStruct(want), st, protocmp.Transform())) + } + } + + checkSecretsDeleted := func() checkFunc { + return func(t *testing.T, ctx context.Context) { + t.Helper() + assert := assert.New(t) + + cSecret := allocHostCatalogSecret() + err := dbRW.LookupWhere(ctx, &cSecret, "catalog_id=?", gotCatalog.GetPublicId()) + assert.Error(err) + assert.True(errors.IsNotFoundError(err)) + } + } + + checkUpdateCatalogRequestCurrentNameNil := func() checkFunc { + return func(t *testing.T, ctx context.Context) { + t.Helper() + assert := assert.New(t) + assert.Nil(gotOnUpdateCatalogRequest.CurrentCatalog.Name) + } + } + + checkUpdateCatalogRequestNewName := func(want string) checkFunc { + return func(t *testing.T, ctx context.Context) { + t.Helper() + assert := assert.New(t) + assert.Equal(wrapperspb.String(want), gotOnUpdateCatalogRequest.NewCatalog.Name) + } + } + + checkUpdateCatalogRequestNewNameNil := func() checkFunc { + return func(t *testing.T, ctx context.Context) { + t.Helper() + assert := assert.New(t) + assert.Nil(gotOnUpdateCatalogRequest.NewCatalog.Name) + } + } + + checkUpdateCatalogRequestCurrentDescriptionNil := func() checkFunc { + return func(t *testing.T, ctx context.Context) { + t.Helper() + assert := assert.New(t) + assert.Nil(gotOnUpdateCatalogRequest.CurrentCatalog.Description) + } + } + + checkUpdateCatalogRequestNewDescription := func(want string) checkFunc { + return func(t *testing.T, ctx context.Context) { + t.Helper() + assert := assert.New(t) + assert.Equal(wrapperspb.String(want), gotOnUpdateCatalogRequest.NewCatalog.Description) + } + } + + checkUpdateCatalogRequestCurrentAttributes := func(want map[string]interface{}) checkFunc { + return func(t *testing.T, ctx context.Context) { + t.Helper() + assert := assert.New(t) + assert.Empty(cmp.Diff(mustStruct(want), gotOnUpdateCatalogRequest.CurrentCatalog.Attributes, protocmp.Transform())) + } + } + + checkUpdateCatalogRequestNewAttributes := func(want map[string]interface{}) checkFunc { + return func(t *testing.T, ctx context.Context) { + t.Helper() + assert := assert.New(t) + assert.Empty(cmp.Diff(mustStruct(want), gotOnUpdateCatalogRequest.NewCatalog.Attributes, protocmp.Transform())) + } + } + + checkUpdateCatalogRequestPersistedSecrets := func(want map[string]interface{}) checkFunc { + return func(t *testing.T, ctx context.Context) { + t.Helper() + assert := assert.New(t) + assert.Empty(cmp.Diff(mustStruct(want), gotOnUpdateCatalogRequest.Persisted.Secrets, protocmp.Transform())) + } + } + + checkUpdateCatalogRequestSecrets := func(want map[string]interface{}) checkFunc { + return func(t *testing.T, ctx context.Context) { + t.Helper() + assert := assert.New(t) + assert.Empty(cmp.Diff(mustStruct(want), gotOnUpdateCatalogRequest.NewCatalog.Secrets, protocmp.Transform())) + // Ensure that the current catalog's secrets value is always zero + assert.Zero(gotOnUpdateCatalogRequest.CurrentCatalog.Secrets) + } + } + + checkNumUpdated := func(want int) checkFunc { + return func(t *testing.T, ctx context.Context) { + t.Helper() + assert := assert.New(t) + assert.Equal(want, gotNumUpdated) + } + } + + checkVerifyCatalogOplog := func(op oplog.OpType) checkFunc { + return func(t *testing.T, ctx context.Context) { + t.Helper() + assert := assert.New(t) + assert.NoError( + db.TestVerifyOplog( + t, + dbRW, + gotCatalog.PublicId, + db.WithOperation(op), + db.WithCreateNotBefore(10*time.Second), + ), + ) + } + } + + tests := []struct { + name string + withEmptyPluginMap bool + withRespSecretsNil bool + withPluginError error + changeFuncs []changeHostCatalogFunc + version uint32 + fieldMask []string + wantCheckFuncs []checkFunc + wantIsErr errors.Code + }{ + { + name: "nil catalog", + changeFuncs: []changeHostCatalogFunc{changeCatalogToNil()}, + wantIsErr: errors.InvalidParameter, + }, + { + name: "nil embedded catalog", + changeFuncs: []changeHostCatalogFunc{changeEmbeddedCatalogToNil()}, + wantIsErr: errors.InvalidParameter, + }, + { + name: "missing public id", + changeFuncs: []changeHostCatalogFunc{changePublicId("")}, + wantIsErr: errors.InvalidParameter, + }, + { + name: "missing scope id", + changeFuncs: []changeHostCatalogFunc{changeScopeId("")}, + wantIsErr: errors.InvalidParameter, + }, + { + name: "empty field mask", + fieldMask: nil, // Should be testing on len + wantIsErr: errors.EmptyFieldMask, + }, + { + name: "bad catalog id", + changeFuncs: []changeHostCatalogFunc{changePublicId("badid")}, + fieldMask: []string{"name"}, + wantIsErr: errors.RecordNotFound, + }, + { + name: "version mismatch", + changeFuncs: []changeHostCatalogFunc{changeName("foo")}, + version: 1, + fieldMask: []string{"name"}, + wantIsErr: errors.VersionMismatch, + }, + { + name: "plugin lookup error", + withEmptyPluginMap: true, + version: 2, + fieldMask: []string{"name"}, + wantIsErr: errors.Internal, + }, + { + name: "plugin invocation error", + withPluginError: errors.New(context.Background(), errors.Internal, "TestRepository_UpdateCatalog/plugin_invocation_error", "test plugin error"), + version: 2, + fieldMask: []string{"name"}, + wantIsErr: errors.Internal, + }, + { + name: "update name (duplicate, same scope)", + changeFuncs: []changeHostCatalogFunc{changeName(testDuplicateCatalogNameProjectScope)}, + version: 2, + fieldMask: []string{"name"}, + wantIsErr: errors.NotUnique, + }, + { + name: "update name", + changeFuncs: []changeHostCatalogFunc{changeName("foo")}, + version: 2, + fieldMask: []string{"name"}, + wantCheckFuncs: []checkFunc{ + checkVersion(3), + checkUpdateCatalogRequestCurrentNameNil(), + checkUpdateCatalogRequestNewName("foo"), + checkName("foo"), + checkSecrets(map[string]interface{}{ + "one": "two", + }), + checkNumUpdated(1), + checkVerifyCatalogOplog(oplog.OpType_OP_TYPE_UPDATE), + }, + }, + { + name: "update name to same", + changeFuncs: []changeHostCatalogFunc{changeName("")}, + version: 2, + fieldMask: []string{"name"}, + wantCheckFuncs: []checkFunc{ + checkVersion(2), // Version remains same even though row is updated + checkUpdateCatalogRequestCurrentNameNil(), + checkUpdateCatalogRequestNewNameNil(), + checkName(""), + checkSecrets(map[string]interface{}{ + "one": "two", + }), + checkNumUpdated(1), + checkVerifyCatalogOplog(oplog.OpType_OP_TYPE_UPDATE), + }, + }, + { + name: "update name (duplicate, different scope)", + changeFuncs: []changeHostCatalogFunc{changeName(testDuplicateCatalogNameAltScope)}, + version: 2, + fieldMask: []string{"name"}, + wantCheckFuncs: []checkFunc{ + checkVersion(3), + checkUpdateCatalogRequestCurrentNameNil(), + checkUpdateCatalogRequestNewName(testDuplicateCatalogNameAltScope), + checkName(testDuplicateCatalogNameAltScope), + checkSecrets(map[string]interface{}{ + "one": "two", + }), + checkNumUpdated(1), + checkVerifyCatalogOplog(oplog.OpType_OP_TYPE_UPDATE), + }, + }, + { + name: "update description", + changeFuncs: []changeHostCatalogFunc{changeDescription("foo")}, + version: 2, + fieldMask: []string{"description"}, + wantCheckFuncs: []checkFunc{ + checkVersion(3), + checkUpdateCatalogRequestCurrentDescriptionNil(), + checkUpdateCatalogRequestNewDescription("foo"), + checkDescription("foo"), + checkSecrets(map[string]interface{}{ + "one": "two", + }), + checkNumUpdated(1), + checkVerifyCatalogOplog(oplog.OpType_OP_TYPE_UPDATE), + }, + }, + { + name: "update attributes (add)", + changeFuncs: []changeHostCatalogFunc{changeAttributes(map[string]interface{}{ + "baz": "qux", + })}, + version: 2, + fieldMask: []string{"attributes"}, + wantCheckFuncs: []checkFunc{ + checkVersion(3), + checkUpdateCatalogRequestCurrentAttributes(map[string]interface{}{ + "foo": "bar", + }), + checkUpdateCatalogRequestNewAttributes(map[string]interface{}{ + "foo": "bar", + "baz": "qux", + }), + checkAttributes(map[string]interface{}{ + "foo": "bar", + "baz": "qux", + }), + checkSecrets(map[string]interface{}{ + "one": "two", + }), + checkNumUpdated(1), + checkVerifyCatalogOplog(oplog.OpType_OP_TYPE_UPDATE), + }, + }, + { + name: "update attributes (overwrite)", + changeFuncs: []changeHostCatalogFunc{changeAttributes(map[string]interface{}{ + "foo": "baz", + })}, + version: 2, + fieldMask: []string{"attributes"}, + wantCheckFuncs: []checkFunc{ + checkVersion(3), + checkUpdateCatalogRequestCurrentAttributes(map[string]interface{}{ + "foo": "bar", + }), + checkUpdateCatalogRequestNewAttributes(map[string]interface{}{ + "foo": "baz", + }), + checkAttributes(map[string]interface{}{ + "foo": "baz", + }), + checkSecrets(map[string]interface{}{ + "one": "two", + }), + checkNumUpdated(1), + checkVerifyCatalogOplog(oplog.OpType_OP_TYPE_UPDATE), + }, + }, + { + name: "update attributes (null)", + changeFuncs: []changeHostCatalogFunc{changeAttributes(map[string]interface{}{ + "foo": nil, + })}, + version: 2, + fieldMask: []string{"attributes"}, + wantCheckFuncs: []checkFunc{ + checkVersion(3), + checkUpdateCatalogRequestCurrentAttributes(map[string]interface{}{ + "foo": "bar", + }), + checkUpdateCatalogRequestNewAttributes(map[string]interface{}{}), + checkAttributes(map[string]interface{}{}), + checkSecrets(map[string]interface{}{ + "one": "two", + }), + checkNumUpdated(1), + checkVerifyCatalogOplog(oplog.OpType_OP_TYPE_UPDATE), + }, + }, + { + name: "update attributes (combined)", + changeFuncs: []changeHostCatalogFunc{changeAttributes(map[string]interface{}{ + "a": "b", + "foo": "baz", + })}, + version: 2, + fieldMask: []string{"attributes.a", "attributes.foo"}, + wantCheckFuncs: []checkFunc{ + checkVersion(3), + checkUpdateCatalogRequestCurrentAttributes(map[string]interface{}{ + "foo": "bar", + }), + checkUpdateCatalogRequestNewAttributes(map[string]interface{}{ + "a": "b", + "foo": "baz", + }), + checkAttributes(map[string]interface{}{ + "a": "b", + "foo": "baz", + }), + checkSecrets(map[string]interface{}{ + "one": "two", + }), + checkNumUpdated(1), + checkVerifyCatalogOplog(oplog.OpType_OP_TYPE_UPDATE), + }, + }, + { + name: "update secrets", + changeFuncs: []changeHostCatalogFunc{changeSecrets(map[string]interface{}{ + "three": "four", + })}, + version: 2, + fieldMask: []string{"secrets.three"}, + wantCheckFuncs: []checkFunc{ + checkVersion(3), + checkUpdateCatalogRequestPersistedSecrets(map[string]interface{}{ + "one": "two", + }), + checkUpdateCatalogRequestSecrets(map[string]interface{}{ + "three": "four", + }), + checkSecrets(map[string]interface{}{ + "three": "four", + }), + checkNumUpdated(1), + checkVerifyCatalogOplog(oplog.OpType_OP_TYPE_UPDATE), + }, + }, + { + name: "update secrets, return nil secrets from plugin", + withRespSecretsNil: true, + changeFuncs: []changeHostCatalogFunc{changeSecrets(map[string]interface{}{ + "three": "four", + })}, + version: 2, + fieldMask: []string{"secrets.three"}, + wantCheckFuncs: []checkFunc{ + checkVersion(2), // no-op + checkUpdateCatalogRequestPersistedSecrets(map[string]interface{}{ + "one": "two", + }), + checkUpdateCatalogRequestSecrets(map[string]interface{}{ + "three": "four", + }), + checkSecrets(map[string]interface{}{ + "one": "two", + }), + checkNumUpdated(db.NoRowsAffected), + }, + }, + { + name: "delete secrets", + changeFuncs: []changeHostCatalogFunc{changeSecrets(map[string]interface{}{})}, + version: 2, + fieldMask: []string{"secrets"}, + wantCheckFuncs: []checkFunc{ + checkVersion(3), + checkUpdateCatalogRequestPersistedSecrets(map[string]interface{}{ + "one": "two", + }), + checkUpdateCatalogRequestSecrets(map[string]interface{}{}), + checkSecretsDeleted(), + checkNumUpdated(1), + checkVerifyCatalogOplog(oplog.OpType_OP_TYPE_UPDATE), + }, + }, + { + name: "update name and secrets", + changeFuncs: []changeHostCatalogFunc{ + changeName("foo"), + changeSecrets(map[string]interface{}{ + "three": "four", + }), + }, + version: 2, + fieldMask: []string{"name", "secrets.three"}, + wantCheckFuncs: []checkFunc{ + checkVersion(3), + checkUpdateCatalogRequestCurrentNameNil(), + checkUpdateCatalogRequestNewName("foo"), + checkUpdateCatalogRequestSecrets(map[string]interface{}{ + "three": "four", + }), + checkName("foo"), + checkSecrets(map[string]interface{}{ + "three": "four", + }), + checkNumUpdated(1), + checkVerifyCatalogOplog(oplog.OpType_OP_TYPE_UPDATE), + }, + }, + } + + // Finally define a function for bringing the test subject catalog. + // This function also returns a function to clean up the catalog + // afterwards. + setupHostCatalog := func(t *testing.T, ctx context.Context) *HostCatalog { + t.Helper() + require := require.New(t) + + cat := TestCatalog(t, dbConn, projectScope.PublicId, testPlugin.GetPublicId()) + // Set some (default) attributes on our test catalog + cat.Attributes = mustMarshal(map[string]interface{}{ + "foo": "bar", + }) + + numCatUpdated, err := dbRW.Update(ctx, cat, []string{"attributes"}, []string{}) + require.NoError(err) + require.Equal(1, numCatUpdated) + + // Set up some secrets + cSecretProto := mustStruct(map[string]interface{}{ + "one": "two", + }) + cSecret, err := newHostCatalogSecret(ctx, cat.GetPublicId(), cSecretProto) + require.NoError(err) + scopeWrapper, err := dbKmsCache.GetWrapper(ctx, cat.GetScopeId(), kms.KeyPurposeDatabase) + require.NoError(err) + require.NoError(cSecret.encrypt(ctx, scopeWrapper)) + cSecretQ, cSecretV := cSecret.upsertQuery() + secretsUpdated, err := dbRW.Exec(ctx, cSecretQ, cSecretV) + require.NoError(err) + require.Equal(1, secretsUpdated) + + t.Cleanup(func() { + t.Helper() + assert := assert.New(t) + n, err := dbRW.Delete(ctx, cat) + assert.NoError(err) + assert.Equal(1, n) + }) + + return cat + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + origCat := setupHostCatalog(t, ctx) + + pluginMap := testPluginMap + if tt.withEmptyPluginMap { + pluginMap = make(map[string]plgpb.HostPluginServiceClient) + } + pluginError = tt.withPluginError + t.Cleanup(func() { pluginError = nil }) + repo, err := NewRepository(dbRW, dbRW, dbKmsCache, sched, pluginMap) + require.NoError(err) + require.NotNil(repo) + + workingCat := origCat.clone() + for _, cf := range tt.changeFuncs { + workingCat = cf(workingCat) + } + + setRespSecretsNil = tt.withRespSecretsNil + var gotPlugin *hostplugin.Plugin + gotCatalog, gotPlugin, gotNumUpdated, err = repo.UpdateCatalog(ctx, workingCat, tt.version, tt.fieldMask) + if tt.wantIsErr != 0 { + require.Equal(db.NoRowsAffected, gotNumUpdated) + require.Truef(errors.Match(errors.T(tt.wantIsErr), err), "want err: %q got: %q", tt.wantIsErr, err) + return + } + require.NoError(err) + t.Cleanup(func() { gotOnUpdateCatalogRequest = nil }) + + // Quick assertion that the catalog is not nil and that the plugin ID in + // the catalog and the plugin ID in the returned plugin match. Use assert + // as that's what our checks use in the table test's defined checks. + assert.NotNil(gotCatalog) + assert.NotNil(gotPlugin) + assert.Equal(gotCatalog.PluginId, gotPlugin.PublicId) + // Perform checks + for _, check := range tt.wantCheckFuncs { + check(t, ctx) + } + }) + } } func TestRepository_LookupCatalog(t *testing.T) { @@ -628,3 +1337,33 @@ func TestRepository_DeleteCatalogX(t *testing.T) { }) } } + +func assertPluginBasedPublicId(t *testing.T, prefix, actual string) { + t.Helper() + assert.NotEmpty(t, actual) + parts := strings.Split(actual, "_") + assert.Equalf(t, 2, len(parts), "want 1 '_' in PublicId, got %d in %q", len(parts), actual) + assert.Equalf(t, prefix, parts[0], "PublicId want prefix: %q, got: %q in %q", prefix, parts[0], actual) +} + +// mustStruct creates a structpb.Struct, and panics if there is an +// error. +func mustStruct(in map[string]interface{}) *structpb.Struct { + out, err := structpb.NewStruct(in) + if err != nil { + panic(err) + } + + return out +} + +// mustMarshal behaves like mustStruct but also converts the Struct +// to wire-format data. +func mustMarshal(in map[string]interface{}) []byte { + b, err := proto.Marshal(mustStruct(in)) + if err != nil { + panic(err) + } + + return b +}