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

@@ -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 {

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{}{

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
 }

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),
 			},
 		},
 		{

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)
+			}
 		})
 	}
 }

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)