Add update catalog support for plugin based catalogs in handler and go sdk (#1673)

* Create catalog service handler for updating plugin.  Add tests for handler and run make fmt. Silently drop attributes where the value is nil when creating the catalog or set.
This commit is contained in:
Todd authored and GitHub committed 2021-11-09 14:42:48 -07:00
1 parent 48ac8b8f0b
commit 050553ffcf
12 files changed
+437 -23

No files matched your search

@@ -60,6 +60,14 @@ func (r *Repository) CreateCatalog(ctx context.Context, c *HostCatalog, _ ...Opt
}
c.PublicId = id
// Use PatchBytes' functionality that does not add keys where the values
// are nil to the resulting struct since we do not want to store nil valued
// attributes.
c.Attributes, err = patchstruct.PatchBytes([]byte{}, c.Attributes)
if err != nil {
return nil, nil, errors.Wrap(ctx, err, op)
}
plgHc, err := toPluginCatalog(ctx, c)
if err != nil {
return nil, nil, errors.Wrap(ctx, err, op)
@@ -188,7 +188,10 @@ func TestRepository_CreateCatalog(t *testing.T) {
ScopeId: prj.GetPublicId(),
PluginId: plg.GetPublicId(),
Attributes: func() []byte {
st, err := structpb.NewStruct(map[string]interface{}{"k1": "foo"})
st, err := structpb.NewStruct(map[string]interface{}{
"k1": "foo",
"nilkey": nil,
})
require.NoError(t, err)
b, err := proto.Marshal(st)
require.NoError(t, err)
@@ -11,6 +11,7 @@ import (
"github.com/hashicorp/boundary/internal/host"
"github.com/hashicorp/boundary/internal/kms"
"github.com/hashicorp/boundary/internal/libs/endpoint"
"github.com/hashicorp/boundary/internal/libs/patchstruct"
"github.com/hashicorp/boundary/internal/oplog"
hostplugin "github.com/hashicorp/boundary/internal/plugin/host"
pb "github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/hostsets"
@@ -50,6 +51,15 @@ func (r *Repository) CreateSet(ctx context.Context, scopeId string, s *HostSet,
}
s = s.clone()
// Use PatchBytes' functionality that does not add keys where the values
// are nil to the resulting struct since we do not want to store nil valued
// attributes.
var err error
s.Attributes, err = patchstruct.PatchBytes([]byte{}, s.Attributes)
if err != nil {
return nil, nil, errors.Wrap(ctx, err, op)
}
c, per, err := r.getCatalog(ctx, s.CatalogId)
if err != nil {
return nil, nil, errors.Wrap(ctx, err, op, errors.WithMsg("looking up catalog"))
@@ -186,7 +186,12 @@ func TestRepository_CreateSet(t *testing.T) {
CatalogId: catalog.PublicId,
Description: ("test-description-repo"),
Attributes: func() []byte {
b, err := proto.Marshal(&structpb.Struct{Fields: map[string]*structpb.Value{"k1": structpb.NewStringValue("foo")}})
st, err := structpb.NewStruct(map[string]interface{}{
"k1": "foo",
"removed": nil,
})
require.NoError(t, err)
b, err := proto.Marshal(st)
require.NoError(t, err)
return b
}(),
+3 -2
View File
@@ -71,8 +71,9 @@ func patchM(dst, src map[string]interface{}) map[string]interface{} {
// If the value in dst a map, continue to patch
dst[k] = patchM(y, x)
} else {
// Overwrite
dst[k] = x
// Overwrite after stripping out keys to nil values
newX := patchM(make(map[string]interface{}), x)
dst[k] = newX
}
default:
+42 -3
View File
@@ -1,8 +1,12 @@
package patchstruct
import (
"strings"
"testing"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/hashicorp/boundary/internal/host"
"github.com/stretchr/testify/require"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/types/known/structpb"
@@ -75,6 +79,24 @@ var testCases = []testCase{
"foo": "bar",
},
},
{
name: "nested with nil",
dst: nil,
src: map[string]interface{}{
"a": "b",
"nested": map[string]interface{}{
"c": "d",
"e": nil,
},
"f": nil,
},
expected: map[string]interface{}{
"a": "b",
"nested": map[string]interface{}{
"c": "d",
},
},
},
{
name: "overwrite with map in src",
dst: map[string]interface{}{
@@ -111,6 +133,17 @@ var testCases = []testCase{
"foo": "bar",
},
},
{
name: "nil dst with src nil value",
dst: nil,
src: map[string]interface{}{
"foo": "bar",
"fiz": nil,
},
expected: map[string]interface{}{
"foo": "bar",
},
},
}
func TestPatchStruct(t *testing.T) {
@@ -130,7 +163,11 @@ func TestPatchStruct(t *testing.T) {
}
actual := PatchStruct(dst, src)
require.Equal(mustStruct(tc.expected), actual)
require.Empty(cmp.Diff(mustStruct(tc.expected), actual,
cmpopts.IgnoreUnexported(structpb.Struct{}, structpb.Value{}),
cmpopts.SortSlices(func(x, y interface{}) bool {
return x.(*host.IpAddress).Address < y.(*host.IpAddress).Address
})))
require.Equal(dstOrig, dst)
require.Equal(srcOrig, src)
})
@@ -155,12 +192,14 @@ func TestPatchBytesErr(t *testing.T) {
t.Run("dst", func(t *testing.T) {
require := require.New(t)
_, err := PatchBytes([]byte("foo"), nil)
require.EqualError(err, "error reading destination data: proto: cannot parse invalid wire-format data")
require.ErrorIs(err, proto.Error)
require.True(strings.HasPrefix(err.Error(), "error reading destination data: "))
})
t.Run("src", func(t *testing.T) {
require := require.New(t)
_, err := PatchBytes(nil, []byte("foo"))
require.EqualError(err, "error reading source data: proto: cannot parse invalid wire-format data")
require.ErrorIs(err, proto.Error)
require.True(strings.HasPrefix(err.Error(), "error reading source data: "))
})
}
@@ -326,7 +326,7 @@ func (s Service) UpdateHostCatalog(ctx context.Context, req *pbs.UpdateHostCatal
if authResults.Error != nil {
return nil, authResults.Error
}
hc, err := s.updateInRepo(ctx, authResults.Scope.GetId(), req.GetId(), req.GetUpdateMask().GetPaths(), req.GetItem())
hc, plg, err := s.updateInRepo(ctx, authResults.Scope.GetId(), req)
if err != nil {
return nil, err
}
@@ -338,6 +338,9 @@ func (s Service) UpdateHostCatalog(ctx context.Context, req *pbs.UpdateHostCatal
outputOpts := make([]handlers.Option, 0, 3)
outputOpts = append(outputOpts, handlers.WithOutputFields(&outputFields))
if plg != nil {
outputOpts = append(outputOpts, handlers.WithPlugin(plg))
}
if outputFields.Has(globals.ScopeField) {
outputOpts = append(outputOpts, handlers.WithScope(authResults.Scope))
}
@@ -510,8 +513,8 @@ func (s Service) createInRepo(ctx context.Context, projId string, req *pbs.Creat
return hc, plg, err
}
func (s Service) updateInRepo(ctx context.Context, projId, id string, mask []string, item *pb.HostCatalog) (*static.HostCatalog, error) {
const op = "host_catalogs.(Service).updateInRepo"
func (s Service) updateStaticInRepo(ctx context.Context, projId, id string, mask []string, item *pb.HostCatalog) (*static.HostCatalog, error) {
const op = "host_catalogs.(Service).updateStaticInRepo"
h, err := toStorageStaticCatalog(ctx, projId, item)
if err != nil {
return nil, errors.Wrap(ctx, err, op, errors.WithMsg("unable to build host catalog for update"))
@@ -536,6 +539,44 @@ func (s Service) updateInRepo(ctx context.Context, projId, id string, mask []str
return out, nil
}
func (s Service) updatePluginInRepo(ctx context.Context, projId, id string, mask []string, item *pb.HostCatalog) (*plugin.HostCatalog, *plugins.PluginInfo, error) {
const op = "host_catalogs.(Service).updatePluginInRepo"
h, err := toStoragePluginCatalog(ctx, projId, "", item)
if err != nil {
return nil, nil, errors.Wrap(ctx, err, op, errors.WithMsg("unable to build host catalog for update"))
}
version := item.GetVersion()
h.PublicId = id
dbMask := maskManager.Translate(mask, "attributes", "secrets")
if len(dbMask) == 0 {
return nil, nil, handlers.InvalidArgumentErrorf("No valid fields included in the update mask.", map[string]string{"update_mask": "No valid fields provided in the update mask."})
}
repo, err := s.pluginHostRepoFn()
if err != nil {
return nil, nil, errors.Wrap(ctx, err, op)
}
out, plg, rowsUpdated, err := repo.UpdateCatalog(ctx, h, version, dbMask)
if err != nil {
return nil, nil, errors.Wrap(ctx, err, op, errors.WithMsg("unable to update host catalog"))
}
if rowsUpdated == 0 {
return nil, nil, handlers.NotFoundErrorf("Host Catalog %q doesn't exist or incorrect version provided.", id)
}
return out, toPluginInfo(plg), nil
}
func (s Service) updateInRepo(ctx context.Context, projId string, req *pbs.UpdateHostCatalogRequest) (hc host.Catalog, plg *plugins.PluginInfo, err error) {
const op = "host_catalogs.(Service).updateInRepo"
switch host.SubtypeFromId(req.GetId()) {
case static.Subtype:
hc, err = s.updateStaticInRepo(ctx, projId, req.GetId(), req.GetUpdateMask().GetPaths(), req.GetItem())
case plugin.Subtype:
hc, plg, err = s.updatePluginInRepo(ctx, projId, req.GetId(), req.GetUpdateMask().GetPaths(), req.GetItem())
}
return
}
func (s Service) deleteFromRepo(ctx context.Context, id string) (bool, error) {
const op = "host_catalogs.(Service).deleteFromRepo"
rows := 0
@@ -793,9 +834,19 @@ func validateUpdateRequest(req *pbs.UpdateHostCatalogRequest) error {
if req.GetItem().GetType() != "" && host.SubtypeFromType(req.GetItem().GetType()) != static.Subtype {
badFields[globals.TypeField] = "Cannot modify resource type."
}
if req.GetItem().GetPlugin() != nil {
badFields[globals.PluginField] = "This field is unused for this type of host catalog."
}
case plugin.Subtype:
if req.GetItem().GetType() != "" && host.SubtypeFromType(req.GetItem().GetType()) != plugin.Subtype {
badFields[globals.TypeField] = "Cannot modify resource type."
}
if req.GetItem().GetPlugin() != nil {
badFields[globals.PluginField] = "This is a read only field."
}
}
return badFields
}, static.HostCatalogPrefix)
}, static.HostCatalogPrefix, plugin.HostCatalogPrefix)
}
func validateDeleteRequest(req *pbs.DeleteHostCatalogRequest) error {
@@ -33,6 +33,7 @@ import (
"google.golang.org/protobuf/testing/protocmp"
"google.golang.org/protobuf/types/known/structpb"
"google.golang.org/protobuf/types/known/timestamppb"
"google.golang.org/protobuf/types/known/wrapperspb"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
@@ -972,7 +973,7 @@ func TestCreate_Plugin(t *testing.T) {
}
}
func TestUpdate(t *testing.T) {
func TestUpdate_Static(t *testing.T) {
t.Parallel()
conn, _ := db.TestSetup(t, "postgres")
wrapper := db.TestWrapper(t)
@@ -1339,3 +1340,282 @@ func TestUpdate(t *testing.T) {
})
}
}
func TestUpdate_Plugin(t *testing.T) {
t.Parallel()
conn, _ := db.TestSetup(t, "postgres")
wrapper := db.TestWrapper(t)
kms := kms.TestKms(t, conn, wrapper)
sche := scheduler.TestScheduler(t, conn, wrapper)
iamRepo := iam.TestRepo(t, conn, wrapper)
_, proj := iam.TestScopes(t, iamRepo)
rw := db.New(conn)
name := "test"
plg := host.TestPlugin(t, conn, name)
plgm := map[string]plgpb.HostPluginServiceClient{
plg.GetPublicId(): plugin.NewWrappingPluginClient(&plugin.TestPluginServer{}),
}
repoFn := func() (*static.Repository, error) {
return static.NewRepository(rw, rw, kms)
}
pluginHostRepo := func() (*plugin.Repository, error) {
return plugin.NewRepository(rw, rw, kms, sche, plgm)
}
pluginRepo := func() (*host.Repository, error) {
return host.NewRepository(rw, rw, kms)
}
iamRepoFn := func() (*iam.Repository, error) {
return iamRepo, nil
}
tested, err := host_catalogs.NewService(repoFn, pluginHostRepo, pluginRepo, iamRepoFn)
require.NoError(t, err, "Failed to create a new host catalog service.")
ctx := auth.DisabledAuthTestContext(iamRepoFn, proj.GetPublicId())
freshCatalog := func(t *testing.T) *pb.HostCatalog {
attr, err := structpb.NewStruct(map[string]interface{}{
"foo": "bar",
})
require.NoError(t, err)
resp, err := tested.CreateHostCatalog(ctx, &pbs.CreateHostCatalogRequest{Item: &pb.HostCatalog{
ScopeId: proj.GetPublicId(),
PluginId: plg.GetPublicId(),
Name: wrapperspb.String("default"),
Description: wrapperspb.String("default"),
Type: "plugin",
Attributes: attr,
}})
require.NoError(t, err)
id := resp.GetItem().GetId()
t.Cleanup(func() {
_, err := tested.DeleteHostCatalog(ctx, &pbs.DeleteHostCatalogRequest{Id: id})
require.NoError(t, err)
})
return resp.GetItem()
}
type updateFn func(catalog *pb.HostCatalog)
clearReadOnlyFields := func() updateFn {
return func(c *pb.HostCatalog) {
c.Id = ""
c.Plugin = nil
c.AuthorizedActions = nil
c.AuthorizedCollectionActions = nil
c.CreatedTime = nil
c.UpdatedTime = nil
}
}
updateName := func(i *wrappers.StringValue) updateFn {
return func(c *pb.HostCatalog) {
c.Name = i
}
}
updateDesc := func(i *wrappers.StringValue) updateFn {
return func(c *pb.HostCatalog) {
c.Description = i
}
}
updateAttrs := func(i *structpb.Struct) updateFn {
return func(c *pb.HostCatalog) {
c.Attributes = i
}
}
cases := []struct {
name string
masks []string
changes []updateFn
check func(*testing.T, *pb.HostCatalog)
err error
}{
{
name: "Update an Existing HostCatalog",
masks: []string{"name", "description"},
changes: []updateFn{
clearReadOnlyFields(),
updateName(wrapperspb.String("new")),
updateDesc(wrapperspb.String("desc")),
},
check: func(t *testing.T, in *pb.HostCatalog) {
assert.Equal(t, "new", in.Name.GetValue())
assert.Equal(t, "desc", in.Description.GetValue())
},
},
{
name: "Update arbitrary attribute for catalog",
masks: []string{"attributes.newkey", "attributes.foo"},
changes: []updateFn{
clearReadOnlyFields(),
updateAttrs(func() *structpb.Struct {
attr, err := structpb.NewStruct(map[string]interface{}{
"newkey": "newvalue",
"foo": nil,
})
require.NoError(t, err)
return attr
}()),
},
check: func(t *testing.T, in *pb.HostCatalog) {
assert.Equal(t, map[string]interface{}{"newkey": "newvalue"}, in.GetAttributes().AsMap())
},
},
{
name: "Multiple Paths in single string",
masks: []string{"name,description"},
changes: []updateFn{
clearReadOnlyFields(),
updateName(wrapperspb.String("new")),
updateDesc(wrapperspb.String("desc")),
},
check: func(t *testing.T, in *pb.HostCatalog) {
assert.Equal(t, "new", in.Name.GetValue())
assert.Equal(t, "desc", in.Description.GetValue())
},
},
{
name: "No Update Mask",
changes: []updateFn{
clearReadOnlyFields(),
updateName(wrapperspb.String("new")),
updateDesc(wrapperspb.String("desc")),
},
err: handlers.ApiErrorWithCode(codes.InvalidArgument),
},
{
name: "Empty Path",
masks: []string{},
changes: []updateFn{
clearReadOnlyFields(),
updateName(wrapperspb.String("new")),
updateDesc(wrapperspb.String("desc")),
},
err: handlers.ApiErrorWithCode(codes.InvalidArgument),
},
{
name: "Only non-existant paths in Mask",
masks: []string{"nonexistant_field"},
changes: []updateFn{
clearReadOnlyFields(),
updateName(wrapperspb.String("new")),
updateDesc(wrapperspb.String("desc")),
},
err: handlers.ApiErrorWithCode(codes.InvalidArgument),
},
{
name: "Unset Name",
masks: []string{"name"},
changes: []updateFn{
clearReadOnlyFields(),
updateName(nil),
updateDesc(wrapperspb.String("ignored")),
},
check: func(t *testing.T, in *pb.HostCatalog) {
assert.Nil(t, in.Name)
assert.Equal(t, "default", in.Description.GetValue())
},
},
{
name: "Unset Description",
masks: []string{"description"},
changes: []updateFn{
clearReadOnlyFields(),
updateName(wrapperspb.String("ignored")),
updateDesc(nil),
},
check: func(t *testing.T, in *pb.HostCatalog) {
assert.Nil(t, in.Description)
assert.Equal(t, "default", in.Name.GetValue())
},
},
{
name: "Update Only Name",
masks: []string{"name"},
changes: []updateFn{
clearReadOnlyFields(),
updateName(wrapperspb.String("updated")),
updateDesc(wrapperspb.String("ignored")),
},
check: func(t *testing.T, in *pb.HostCatalog) {
assert.Equal(t, "updated", in.Name.GetValue())
assert.Equal(t, "default", in.Description.GetValue())
},
},
{
name: "Update Only Description",
masks: []string{"description"},
changes: []updateFn{
clearReadOnlyFields(),
updateName(wrapperspb.String("ignored")),
updateDesc(wrapperspb.String("updated")),
},
check: func(t *testing.T, in *pb.HostCatalog) {
assert.Equal(t, "default", in.Name.GetValue())
assert.Equal(t, "updated", in.Description.GetValue())
},
},
{
name: "Cant change type",
masks: []string{"type"},
changes: []updateFn{
clearReadOnlyFields(),
func(c *pb.HostCatalog) {
c.Type = "static"
},
},
err: handlers.ApiErrorWithCode(codes.InvalidArgument),
},
{
name: "Cant specify Updated Time",
masks: []string{"updated_time"},
changes: []updateFn{
clearReadOnlyFields(),
func(c *pb.HostCatalog) {
c.UpdatedTime = timestamppb.Now()
},
},
err: handlers.ApiErrorWithCode(codes.InvalidArgument),
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
assert, require := assert.New(t), require.New(t)
hc := freshCatalog(t)
id := hc.GetId()
for _, f := range tc.changes {
f(hc)
}
req := &pbs.UpdateHostCatalogRequest{
Id: id,
Item: hc,
UpdateMask: &field_mask.FieldMask{Paths: tc.masks},
}
// Test some bad versions
req.Item.Version = 2
_, gErr := tested.UpdateHostCatalog(ctx, req)
require.Error(gErr)
req.Item.Version = 0
_, gErr = tested.UpdateHostCatalog(ctx, req)
require.Error(gErr)
req.Item.Version = 1
got, gErr := tested.UpdateHostCatalog(ctx, req)
if tc.err != nil {
require.Error(gErr)
assert.True(errors.Is(gErr, tc.err), "UpdateHostCatalog(%+v) got error %v, wanted %v", req, gErr, tc.err)
return
}
require.NoError(gErr)
require.NotNil(got)
item := got.GetItem()
assert.Equal(uint32(2), item.GetVersion())
assert.Greater(item.GetUpdatedTime().AsTime().UnixNano(), item.GetCreatedTime().AsTime().UnixNano())
tc.check(t, item)
})
}
}
@@ -37,7 +37,7 @@ var (
// IdActions contains the set of actions that can be performed on
// individual resources
idActionsTypeMap = map[subtypes.Subtype]action.ActionSet{
static.Subtype: action.ActionSet{
static.Subtype: {
action.NoOp,
action.Read,
action.Update,
@@ -46,7 +46,7 @@ var (
action.SetHosts,
action.RemoveHosts,
},
plugin.Subtype: action.ActionSet{
plugin.Subtype: {
action.NoOp,
action.Read,
action.Update,
@@ -815,7 +815,7 @@ func TestCreate_Plugin(t *testing.T) {
}
hc := plugin.TestCatalog(t, conn, proj.GetPublicId(), plg.GetPublicId())
testAttrs, err := structpb.NewStruct(map[string]interface{}{
attrs := map[string]interface{}{
"int": 1,
"zero int": 0,
"string": "foo",
@@ -834,7 +834,13 @@ func TestCreate_Plugin(t *testing.T) {
"bool": true,
"zero bool": false,
},
})
}
testInputAttrs, err := structpb.NewStruct(attrs)
require.NoError(t, err)
// The result should clear out all keys with nil values...
delete(attrs, "zero bytes")
delete(attrs["nested"].(map[string]interface{}), "zero bytes")
testOutputAttrs, err := structpb.NewStruct(attrs)
require.NoError(t, err)
prefEndpoints := []string{"cidr:1.2.3.4", "cidr:2.3.4.5/24"}
@@ -879,7 +885,7 @@ func TestCreate_Plugin(t *testing.T) {
Name: &wrappers.StringValue{Value: "With Attributes"},
Description: &wrappers.StringValue{Value: "desc"},
Type: plugin.Subtype.String(),
Attributes: testAttrs,
Attributes: testInputAttrs,
}},
res: &pbs.CreateHostSetResponse{
Uri: fmt.Sprintf("host-sets/%s_", plugin.HostSetPrefix),
@@ -895,7 +901,7 @@ func TestCreate_Plugin(t *testing.T) {
Description: &wrappers.StringValue{Value: "desc"},
Type: plugin.Subtype.String(),
AuthorizedActions: testAuthorizedActions[plugin.Subtype],
Attributes: testAttrs,
Attributes: testOutputAttrs,
},
},
},
@@ -169,6 +169,17 @@ func TestCrud(t *testing.T) {
hostcatalogs.WithAttributes(map[string]interface{}{"foo": "bar"}))
require.NoError(err)
c, err = hcClient.Update(tc.Context(), c.Item.Id, c.Item.Version, hostcatalogs.WithName("bar"),
hostcatalogs.WithAttributes(map[string]interface{}{"key": "val", "foo": nil}),
hostcatalogs.WithSecrets(map[string]interface{}{"secretkey": "secretval"}))
checkCatalog("update", c.Item, err, "bar", 2)
assert.Contains(c.Item.Attributes, "key")
assert.NotContains(c.Item.Attributes, "foo")
assert.Empty(c.Item.Secrets)
c, err = hcClient.Update(tc.Context(), c.Item.Id, c.Item.Version, hostcatalogs.DefaultName())
checkCatalog("update", c.Item, err, "", 3)
c, err = hcClient.Read(tc.Context(), c.Item.Id)
assert.NoError(err)
+5 -5
View File
@@ -99,20 +99,20 @@ func TestPluginHosts(t *testing.T) {
hset, err := hostsets.NewClient(client).Create(tc.Context(), hc.Item.Id, hostsets.WithAttributes(map[string]interface{}{
"host_info": []interface{}{
map[string]interface{}{
"external_id": "test1",
"external_id": "test1",
"ip_addresses": []string{"10.0.0.1", "192.168.1.1"},
"dns_names": []string{"foo.hashicorp.com", "boundaryproject.io"},
"dns_names": []string{"foo.hashicorp.com", "boundaryproject.io"},
},
map[string]interface{}{
"external_id": "test2",
"external_id": "test2",
"ip_addresses": []string{"10.0.0.2", "192.168.1.2"},
"dns_names": []string{"foo2.hashicorp.com", "boundaryproject2.io"},
"dns_names": []string{"foo2.hashicorp.com", "boundaryproject2.io"},
},
},
}))
require.NoError(err)
require.NotNil(hset)
time.Sleep(1*time.Second)
time.Sleep(1 * time.Second)
hClient := hosts.NewClient(client)
hl, err := hClient.List(tc.Context(), hc.Item.Id)