From b16ce175e5c3fcdbe7db44fbf106b98dd49531cb Mon Sep 17 00:00:00 2001 From: Todd Date: Fri, 12 Nov 2021 09:46:40 -0700 Subject: [PATCH] Fix secrets clearing bug in update. Changes to update count behavior for catalogs (#1691) * Fix secret clearing bug in update. Always return 1 catalog updated if we can find a catalog w/ a matching version. --- internal/host/plugin/loopback.go | 20 ++++++++++++- internal/host/plugin/loopback_test.go | 28 +++++++++++++++++-- .../host/plugin/repository_host_catalog.go | 13 ++++----- .../plugin/repository_host_catalog_test.go | 7 +++-- .../host_catalog_service_test.go | 25 +++++++++++++++-- .../api/hostcatalogs/host_catalog_test.go | 10 +++++++ 6 files changed, 88 insertions(+), 15 deletions(-) diff --git a/internal/host/plugin/loopback.go b/internal/host/plugin/loopback.go index cfd3c06fdb..34f32a69a0 100644 --- a/internal/host/plugin/loopback.go +++ b/internal/host/plugin/loopback.go @@ -38,6 +38,7 @@ func NewLoopbackPlugin() plgpb.HostPluginServiceServer { } ret.OnCreateCatalogFn = ret.onCreateCatalog ret.OnCreateSetFn = ret.onCreateSet + ret.OnUpdateCatalogFn = ret.onUpdateCatalog ret.OnDeleteSetFn = ret.onDeleteSet ret.ListHostsFn = ret.listHosts return ret @@ -57,7 +58,7 @@ func (l *loopbackPlugin) onCreateCatalog(ctx context.Context, req *plgpb.OnCreat }, nil } } - return nil, nil + return &plgpb.OnCreateCatalogResponse{}, nil } func (l *loopbackPlugin) onCreateSet(ctx context.Context, req *plgpb.OnCreateSetRequest) (*plgpb.OnCreateSetResponse, error) { @@ -95,6 +96,23 @@ func (l *loopbackPlugin) onCreateSet(ctx context.Context, req *plgpb.OnCreateSet return nil, nil } +func (l *loopbackPlugin) onUpdateCatalog(ctx context.Context, req *plgpb.OnUpdateCatalogRequest) (*plgpb.OnUpdateCatalogResponse, error) { + const op = "plugin.(loopbackPlugin).onUpdateCatalog" + if req == nil { + return nil, errors.New(ctx, errors.InvalidParameter, op, "req is nil") + } + if cat := req.GetNewCatalog(); cat != nil { + if secrets := cat.GetSecrets(); secrets != nil { + return &plgpb.OnUpdateCatalogResponse{ + Persisted: &plgpb.HostCatalogPersisted{ + Secrets: secrets, + }, + }, nil + } + } + return &plgpb.OnUpdateCatalogResponse{}, nil +} + func (l *loopbackPlugin) onDeleteSet(ctx context.Context, req *plgpb.OnDeleteSetRequest) (*plgpb.OnDeleteSetResponse, error) { const op = "plugin.(loopbackPlugin).onDeleteSet" if req == nil { diff --git a/internal/host/plugin/loopback_test.go b/internal/host/plugin/loopback_test.go index 6bd0dfcf1b..691a797363 100644 --- a/internal/host/plugin/loopback_test.go +++ b/internal/host/plugin/loopback_test.go @@ -19,8 +19,8 @@ func TestLoopbackPlugin(t *testing.T) { plg := NewLoopbackPlugin() secretsMap := map[string]interface{}{ - "foo": "bar", - "baz": true, + "key1": "key2", + "baz": true, } secrets, err := structpb.NewStruct(secretsMap) require.NoError(err) @@ -38,6 +38,30 @@ func TestLoopbackPlugin(t *testing.T) { require.NotNil(catResp.GetPersisted().GetSecrets()) assert.EqualValues(secretsMap, catResp.GetPersisted().GetSecrets().AsMap()) + newSecretsMap := map[string]interface{}{ + "key1": "key2", + "baz": true, + } + newSecrets, err := structpb.NewStruct(newSecretsMap) + require.NoError(err) + + // First, test that if we give it secrets, those secrets come back as + // persisted data + upResp, err := plg.OnUpdateCatalog(ctx, &plgpb.OnUpdateCatalogRequest{ + CurrentCatalog: &hostcatalogs.HostCatalog{}, + NewCatalog: &hostcatalogs.HostCatalog{ + Secrets: newSecrets, + }, + Persisted: &plgpb.HostCatalogPersisted{ + Secrets: secrets, + }, + }) + require.NoError(err) + require.NotNil(upResp) + require.NotNil(upResp.GetPersisted()) + require.NotNil(upResp.GetPersisted().GetSecrets()) + assert.EqualValues(newSecretsMap, upResp.GetPersisted().GetSecrets().AsMap()) + // Add data to some sets hostInfo1 := map[string]interface{}{ loopbackPluginHostInfoAttrField: map[string]interface{}{ diff --git a/internal/host/plugin/repository_host_catalog.go b/internal/host/plugin/repository_host_catalog.go index 42afeb81c4..74464774a5 100644 --- a/internal/host/plugin/repository_host_catalog.go +++ b/internal/host/plugin/repository_host_catalog.go @@ -247,11 +247,8 @@ func (r *Repository) UpdateCatalog(ctx context.Context, c *HostCatalog, version 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. + // field, as secrets do not have a database entry. 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)) } @@ -443,10 +440,10 @@ func (r *Repository) UpdateCatalog(ctx context.Context, c *HostCatalog, version 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 - } + // Even if we didn't update any records, if we were able to find the record + // with the appropriate version, returning 1 row updated is consistant with + // static's update catalog behavior. + numUpdated := 1 return returnedCatalog, plg, numUpdated, nil } diff --git a/internal/host/plugin/repository_host_catalog_test.go b/internal/host/plugin/repository_host_catalog_test.go index 9eb7fb6656..832a079643 100644 --- a/internal/host/plugin/repository_host_catalog_test.go +++ b/internal/host/plugin/repository_host_catalog_test.go @@ -987,9 +987,10 @@ func TestRepository_UpdateCatalog(t *testing.T) { name: "update secrets", changeFuncs: []changeHostCatalogFunc{changeSecrets(map[string]interface{}{ "three": "four", + "five": "six", })}, version: 2, - fieldMask: []string{"secrets.three"}, + fieldMask: []string{"secrets.three", "secrets.five"}, wantCheckFuncs: []checkFunc{ checkVersion(3), checkUpdateCatalogRequestPersistedSecrets(map[string]interface{}{ @@ -997,9 +998,11 @@ func TestRepository_UpdateCatalog(t *testing.T) { }), checkUpdateCatalogRequestSecrets(map[string]interface{}{ "three": "four", + "five": "six", }), checkSecrets(map[string]interface{}{ "three": "four", + "five": "six", }), checkNumUpdated(1), checkVerifyCatalogOplog(oplog.OpType_OP_TYPE_UPDATE), @@ -1024,7 +1027,7 @@ func TestRepository_UpdateCatalog(t *testing.T) { checkSecrets(map[string]interface{}{ "one": "two", }), - checkNumUpdated(db.NoRowsAffected), + checkNumUpdated(1), }, }, { diff --git a/internal/servers/controller/handlers/host_catalogs/host_catalog_service_test.go b/internal/servers/controller/handlers/host_catalogs/host_catalog_service_test.go index 515de10588..95863b6bea 100644 --- a/internal/servers/controller/handlers/host_catalogs/host_catalog_service_test.go +++ b/internal/servers/controller/handlers/host_catalogs/host_catalog_service_test.go @@ -1354,7 +1354,7 @@ func TestUpdate_Plugin(t *testing.T) { name := "test" plg := host.TestPlugin(t, conn, name) plgm := map[string]plgpb.HostPluginServiceClient{ - plg.GetPublicId(): plugin.NewWrappingPluginClient(&plugin.TestPluginServer{}), + plg.GetPublicId(): plugin.NewWrappingPluginClient(plugin.NewLoopbackPlugin()), } repoFn := func() (*static.Repository, error) { @@ -1422,6 +1422,11 @@ func TestUpdate_Plugin(t *testing.T) { c.Attributes = i } } + updateSecrets := func(i *structpb.Struct) updateFn { + return func(c *pb.HostCatalog) { + c.Secrets = i + } + } cases := []struct { name string @@ -1461,6 +1466,20 @@ func TestUpdate_Plugin(t *testing.T) { assert.Equal(t, map[string]interface{}{"newkey": "newvalue"}, in.GetAttributes().AsMap()) }, }, + { + name: "Update secrets", + masks: []string{"secrets.key1", "secrets.key2"}, + changes: []updateFn{ + clearReadOnlyFields(), + updateSecrets(func() *structpb.Struct { + attr, err := structpb.NewStruct(map[string]interface{}{ + "key1": "val1", + }) + require.NoError(t, err) + return attr + }()), + }, + }, { name: "Multiple Paths in single string", masks: []string{"name,description"}, @@ -1615,7 +1634,9 @@ func TestUpdate_Plugin(t *testing.T) { item := got.GetItem() assert.Equal(uint32(2), item.GetVersion()) assert.Greater(item.GetUpdatedTime().AsTime().UnixNano(), item.GetCreatedTime().AsTime().UnixNano()) - tc.check(t, item) + if tc.check != nil { + tc.check(t, item) + } }) } } diff --git a/internal/tests/api/hostcatalogs/host_catalog_test.go b/internal/tests/api/hostcatalogs/host_catalog_test.go index d7c74c39d6..b23d1a4b87 100644 --- a/internal/tests/api/hostcatalogs/host_catalog_test.go +++ b/internal/tests/api/hostcatalogs/host_catalog_test.go @@ -149,6 +149,9 @@ func TestCrud(t *testing.T) { hc, err = hcClient.Read(tc.Context(), hc.Item.Id) checkCatalog("read", hc.Item, err, "foo", 1) + hc, err = hcClient.Update(tc.Context(), hc.Item.Id, hc.Item.Version, hostcatalogs.WithName("foo")) + checkCatalog("update", hc.Item, err, "foo", 1) + hc, err = hcClient.Update(tc.Context(), hc.Item.Id, hc.Item.Version, hostcatalogs.WithName("bar")) checkCatalog("update", hc.Item, err, "bar", 2) @@ -180,6 +183,13 @@ func TestCrud(t *testing.T) { c, err = hcClient.Update(tc.Context(), c.Item.Id, c.Item.Version, hostcatalogs.DefaultName()) checkCatalog("update", c.Item, err, "", 3) + c, err = hcClient.Update(tc.Context(), c.Item.Id, 0, hostcatalogs.WithAutomaticVersioning(true), + hostcatalogs.WithSecrets(map[string]interface{}{ + "key1": "val1", + "key2": "val2", + })) + checkCatalog("update", c.Item, err, "", 4) + c, err = hcClient.Read(tc.Context(), c.Item.Id) assert.NoError(err)