From 5417cc5cb07d2e6622d6f5dae16c20a03559ce13 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Sun, 28 Nov 2021 12:20:46 -0800 Subject: [PATCH] Fix setting nil attributes on host sets (#1746) This is the analogue of #1743 but for host sets --- internal/host/plugin/repository_host_set.go | 10 +-- .../handlers/host_sets/host_set_service.go | 2 +- .../host_sets/host_set_service_test.go | 14 ++++ testing/controller/controller.go | 81 +++++++++++-------- 4 files changed, 66 insertions(+), 41 deletions(-) diff --git a/internal/host/plugin/repository_host_set.go b/internal/host/plugin/repository_host_set.go index f2b23db50..9388519aa 100644 --- a/internal/host/plugin/repository_host_set.go +++ b/internal/host/plugin/repository_host_set.go @@ -290,13 +290,9 @@ func (r *Repository) UpdateSet(ctx context.Context, scopeId string, s *HostSet, if updateAttributes { dbMask = append(dbMask, "attributes") - if s.Attributes != nil { - newSet.Attributes, err = patchstruct.PatchBytes(newSet.Attributes, s.Attributes) - if err != nil { - return nil, nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op, errors.WithMsg("error in set attribute JSON")) - } - } else { - newSet.Attributes = make([]byte, 0) + newSet.Attributes, err = patchstruct.PatchBytes(newSet.Attributes, s.Attributes) + if err != nil { + return nil, nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op, errors.WithMsg("error in set attribute JSON")) } // Flag the record as needing a sync since we've updated diff --git a/internal/servers/controller/handlers/host_sets/host_set_service.go b/internal/servers/controller/handlers/host_sets/host_set_service.go index 9c47f7e65..03cf4c011 100644 --- a/internal/servers/controller/handlers/host_sets/host_set_service.go +++ b/internal/servers/controller/handlers/host_sets/host_set_service.go @@ -553,7 +553,7 @@ func (s Service) updatePluginInRepo(ctx context.Context, scopeId string, req *pb return nil, nil, nil, errors.Wrap(ctx, err, op, errors.WithMsg("Unable to build host set for update")) } h.PublicId = req.GetId() - dbMask := maskManager[plugin.Subtype].Translate(req.GetUpdateMask().GetPaths(), "attributes.") + dbMask := maskManager[plugin.Subtype].Translate(req.GetUpdateMask().GetPaths(), "attributes") if len(dbMask) == 0 { return nil, 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."}) } diff --git a/internal/servers/controller/handlers/host_sets/host_set_service_test.go b/internal/servers/controller/handlers/host_sets/host_set_service_test.go index c28f6241e..29a8cd64b 100644 --- a/internal/servers/controller/handlers/host_sets/host_set_service_test.go +++ b/internal/servers/controller/handlers/host_sets/host_set_service_test.go @@ -1570,6 +1570,20 @@ func TestUpdate_Plugin(t *testing.T) { assert.Equal(t, map[string]interface{}{"newkey": "newvalue"}, in.GetAttributes().AsMap()) }, }, + { + name: "Set attributes null", + masks: []string{"attributes"}, + changes: []updateFn{ + clearReadOnlyFields(), + updateAttrs(func() *structpb.Struct { + ret, _ := structpb.NewStruct(nil) + return ret + }()), + }, + check: func(t *testing.T, in *pb.HostSet) { + assert.Equal(t, (*structpb.Struct)(nil), in.GetAttributes()) + }, + }, { name: "Update preferred endpoints", masks: []string{"preferred_endpoints"}, diff --git a/testing/controller/controller.go b/testing/controller/controller.go index cdd432d63..1729e5ef0 100644 --- a/testing/controller/controller.go +++ b/testing/controller/controller.go @@ -9,35 +9,6 @@ import ( wrapping "github.com/hashicorp/go-kms-wrapping" ) -func getOpts(opt ...Option) (*controller.TestControllerOpts, error) { - opts := &option{ - tcOptions: &controller.TestControllerOpts{}, - } - for _, o := range opt { - if err := o(opts); err != nil { - return nil, err - } - } - if opts.setWithConfigFile && opts.setWithConfigText { - return nil, fmt.Errorf("Cannot provide both WithConfigFile and WithConfigText") - } - var setDbParams bool - if opts.setDefaultPasswordAuthMethodId || opts.setDefaultOidcAuthMethodId || opts.setDefaultLoginName || opts.setDefaultPassword { - setDbParams = true - } - if opts.setDisableAuthMethodCreation { - if setDbParams { - return nil, fmt.Errorf("Cannot both disable auth method creation and provide auth method parameters") - } - } - if opts.setDisableDatabaseCreation { - if setDbParams { - return nil, fmt.Errorf("Cannot both disable database creation and provide auth method parameters") - } - } - return opts.tcOptions, nil -} - type option struct { tcOptions *controller.TestControllerOpts setWithConfigFile bool @@ -53,10 +24,44 @@ type option struct { setWorkerAuthKms bool setRecoveryKms bool setDatabaseUrl bool + setEnableTemplatedDatabase bool } type Option func(*option) error +func getOpts(opt ...Option) (*controller.TestControllerOpts, error) { + opts := &option{ + tcOptions: &controller.TestControllerOpts{}, + } + for _, o := range opt { + if err := o(opts); err != nil { + return nil, err + } + } + opts.tcOptions.DisableDatabaseTemplate = !opts.setEnableTemplatedDatabase + if opts.setWithConfigFile && opts.setWithConfigText { + return nil, fmt.Errorf("Cannot provide both WithConfigFile and WithConfigText") + } + var setDbParams bool + if opts.setDefaultPasswordAuthMethodId || + opts.setDefaultOidcAuthMethodId || + opts.setDefaultLoginName || + opts.setDefaultPassword { + setDbParams = true + } + if opts.setDisableAuthMethodCreation { + if setDbParams { + return nil, fmt.Errorf("Cannot both disable auth method creation and provide auth method parameters") + } + } + if opts.setDisableDatabaseCreation { + if setDbParams { + return nil, fmt.Errorf("Cannot both disable database creation and provide auth method parameters") + } + } + return opts.tcOptions, nil +} + // WithConfigFile provides the given ConfigFile to the built TestController. // This option cannot be used if WithConfigText is used. func WithConfigFile(f string) Option { @@ -184,6 +189,20 @@ func WithDatabaseUrl(url string) Option { } } +// WithEnableTemplatedDatabase enables usage of a local database test instance +// with a template. Generally outside tests will probably want to use the +// non-templated database, so this defaults to enabling instead of disabling. +func WithEnableTemplatedDatabase(enable bool) Option { + return func(c *option) error { + c.setEnableTemplatedDatabase = enable + return nil + } +} + +type TestController struct { + *controller.TestController +} + // NewTestController blocks until a new TestController is created, returns the url for the TestController and a function // that can be called to tear down the controller after it has been used for testing. func NewTestController(t *testing.T, opt ...Option) *TestController { @@ -194,7 +213,3 @@ func NewTestController(t *testing.T, opt ...Option) *TestController { tc := controller.NewTestController(t, conf) return &TestController{TestController: tc} } - -type TestController struct { - *controller.TestController -}