diff --git a/internal/db/schema/migrations/oss/postgres/20/05_plugin_host.up.sql b/internal/db/schema/migrations/oss/postgres/20/05_plugin_host.up.sql index f212e1aca..a1f655389 100644 --- a/internal/db/schema/migrations/oss/postgres/20/05_plugin_host.up.sql +++ b/internal/db/schema/migrations/oss/postgres/20/05_plugin_host.up.sql @@ -335,63 +335,55 @@ begin; create trigger insert_host_plugin_set_member before insert on host_plugin_set_member for each row execute procedure insert_host_plugin_set_member(); + create table host_set_preferred_endpoint ( + create_time wt_timestamp, + host_set_id wt_public_id not null + constraint host_set_fkey + references host_set(public_id) + on delete cascade + on update cascade, + priority wt_priority, + condition text not null + constraint condition_must_not_be_too_short + check(length(trim(condition)) > 4) -- minimum is 'dns:*' + constraint condition_must_not_be_too_long + check(length(trim(condition)) < 255) + constraint condition_has_valid_prefix + check( + left(trim(condition), 4) = 'dns:' + or + left(trim(condition), 5) = 'cidr:' + ) + constraint condition_does_not_contain_invalid_chars + check( + position('|' in trim(condition)) = 0 + and + position('=' in trim(condition)) = 0 + ), + primary key(host_set_id, priority), + constraint host_set_preferred_endpoint_host_set_id_condition_uq + unique(host_set_id, condition) + ); + + -- host_set_immutable_preferred_endpoint() ensures that endpoint conditions + -- assigned to host sets are immutable. + create function + host_set_immutable_preferred_endpoint() + returns trigger + as $$ + begin + raise exception 'preferred endpoints are immutable'; + end; + $$ language plpgsql; + + create trigger immutable_preferred_endpoint + before update on host_set_preferred_endpoint + for each row execute procedure host_set_immutable_preferred_endpoint(); + insert into oplog_ticket (name, version) values ('host_plugin_catalog', 1), - ('host_plugin_catalog_secret', 1), ('host_plugin_set', 1), ('host_plugin_host', 1); - -- host_plugin_catalog_with_secret is useful for reading a plugin catalog with - -- its associated persisted data. - create view host_plugin_catalog_with_secret as - select - hc.public_id, - hc.scope_id, - hc.plugin_id, - hc.name, - hc.description, - hc.create_time, - hc.update_time, - hc.version, - hc.attributes, - hcs.secret, - hcs.key_id, - hcs.create_time as persisted_create_time, - hcs.update_time as persisted_update_time - from - host_plugin_catalog hc - left outer join host_plugin_catalog_secret hcs on hc.public_id = hcs.catalog_id; - comment on view host_plugin_catalog_with_secret is - 'host plugin catalog with its associated persisted data'; - - -- host_plugin_host_with_value_obj_and_set_memberships is useful for reading a - -- plugin host with its associated value objects (ip addresses, dns names) and - -- set membership as columns with delimited values. The delimiter depends on - -- the value objects (e.g. if they need ordering). - create view host_plugin_host_with_value_obj_and_set_memberships as - select - h.public_id, - h.catalog_id, - h.external_id, - hc.scope_id, - hc.plugin_id, - h.name, - h.description, - h.create_time, - h.update_time, - -- the string_agg(..) column will be null if there are no associated value objects - string_agg(distinct host(hip.address), '|') as ip_addresses, - string_agg(distinct hdns.name, '|') as dns_names, - string_agg(distinct hpsm.set_id, '|') as set_ids - from - host_plugin_host h - join host_plugin_catalog hc on h.catalog_id = hc.public_id - left outer join host_ip_address hip on h.public_id = hip.host_id - left outer join host_dns_name hdns on h.public_id = hdns.host_id - left outer join host_plugin_set_member hpsm on h.public_id = hpsm.host_id - group by h.public_id, hc.plugin_id, hc.scope_id; - comment on view host_plugin_host_with_value_obj_and_set_memberships is - 'host plugin host with its associated value objects'; - commit; diff --git a/internal/db/schema/migrations/oss/postgres/20/06_plugin_host_views.up.sql b/internal/db/schema/migrations/oss/postgres/20/06_plugin_host_views.up.sql new file mode 100644 index 000000000..f3e476457 --- /dev/null +++ b/internal/db/schema/migrations/oss/postgres/20/06_plugin_host_views.up.sql @@ -0,0 +1,85 @@ +begin; + +-- host_plugin_host_set_with_value_obj is useful for reading a plugin host set with its +-- associated value objects (preferred endpoints) as columns with delimited +-- values. The delimiter depends on the value objects (e.g. if they need +-- ordering). +create view host_plugin_host_set_with_value_obj as + select + hs.public_id, + hs.catalog_id, + hc.plugin_id, + hs.name, + hs.description, + hs.create_time, + hs.update_time, + hs.last_sync_time, + hs.need_sync, + hs.sync_interval_seconds, + hs.version, + hs.attributes, + -- the string_agg(..) column will be null if there are no associated value objects + string_agg(distinct concat_ws('=', hspe.priority, hspe.condition), '|') as preferred_endpoints, + string_agg(distinct hpsm.host_id, '|') as host_ids + from + host_plugin_set hs + join host_plugin_catalog hc on hs.catalog_id = hc.public_id + left outer join host_set_preferred_endpoint hspe on hs.public_id = hspe.host_set_id + left outer join host_plugin_set_member hpsm on hs.public_id = hpsm.set_id + group by hs.public_id, hc.plugin_id; +comment on view host_plugin_host_set_with_value_obj is +'host plugin host set with its associated value objects'; + +-- host_plugin_catalog_with_secret is useful for reading a plugin catalog with +-- its associated persisted data. +create view host_plugin_catalog_with_secret as +select + hc.public_id, + hc.scope_id, + hc.plugin_id, + hc.name, + hc.description, + hc.create_time, + hc.update_time, + hc.version, + hc.attributes, + hcs.secret, + hcs.key_id, + hcs.create_time as persisted_create_time, + hcs.update_time as persisted_update_time +from + host_plugin_catalog hc + left outer join host_plugin_catalog_secret hcs on hc.public_id = hcs.catalog_id; +comment on view host_plugin_catalog_with_secret is + 'host plugin catalog with its associated persisted data'; + +-- host_plugin_host_with_value_obj_and_set_memberships is useful for reading a +-- plugin host with its associated value objects (ip addresses, dns names) and +-- set membership as columns with delimited values. The delimiter depends on +-- the value objects (e.g. if they need ordering). +create view host_plugin_host_with_value_obj_and_set_memberships as +select + h.public_id, + h.catalog_id, + h.external_id, + hc.scope_id, + hc.plugin_id, + h.name, + h.description, + h.create_time, + h.update_time, + -- the string_agg(..) column will be null if there are no associated value objects + string_agg(distinct host(hip.address), '|') as ip_addresses, + string_agg(distinct hdns.name, '|') as dns_names, + string_agg(distinct hpsm.set_id, '|') as set_ids +from + host_plugin_host h + join host_plugin_catalog hc on h.catalog_id = hc.public_id + left outer join host_ip_address hip on h.public_id = hip.host_id + left outer join host_dns_name hdns on h.public_id = hdns.host_id + left outer join host_plugin_set_member hpsm on h.public_id = hpsm.host_id +group by h.public_id, hc.plugin_id, hc.scope_id; +comment on view host_plugin_host_with_value_obj_and_set_memberships is + 'host plugin host with its associated value objects'; + +commit; \ No newline at end of file diff --git a/internal/db/schema/migrations/oss/postgres/20/06_preferred_endpoints.up.sql b/internal/db/schema/migrations/oss/postgres/20/06_preferred_endpoints.up.sql deleted file mode 100644 index 2a8ef89cc..000000000 --- a/internal/db/schema/migrations/oss/postgres/20/06_preferred_endpoints.up.sql +++ /dev/null @@ -1,78 +0,0 @@ -begin; - -create table host_set_preferred_endpoint ( - create_time wt_timestamp, - host_set_id wt_public_id not null - constraint host_set_fkey - references host_set(public_id) - on delete cascade - on update cascade, - priority wt_priority, - condition text not null - constraint condition_must_not_be_too_short - check(length(trim(condition)) > 4) -- minimum is 'dns:*' - constraint condition_must_not_be_too_long - check(length(trim(condition)) < 255) - constraint condition_has_valid_prefix - check( - left(trim(condition), 4) = 'dns:' - or - left(trim(condition), 5) = 'cidr:' - ) - constraint condition_does_not_contain_invalid_chars - check( - position('|' in trim(condition)) = 0 - and - position('=' in trim(condition)) = 0 - ), - primary key(host_set_id, priority), - constraint host_set_preferred_endpoint_host_set_id_condition_uq - unique(host_set_id, condition) -); - --- host_set_immutable_preferred_endpoint() ensures that endpoint conditions --- assigned to host sets are immutable. -create function - host_set_immutable_preferred_endpoint() - returns trigger -as $$ -begin - raise exception 'preferred endpoints are immutable'; -end; -$$ language plpgsql; - -create trigger immutable_preferred_endpoint - before update on host_set_preferred_endpoint - for each row execute procedure host_set_immutable_preferred_endpoint(); - --- host_plugin_host_set_with_value_obj is useful for reading a plugin host set with its --- associated value objects (preferred endpoints) as columns with delimited --- values. The delimiter depends on the value objects (e.g. if they need --- ordering). -create view host_plugin_host_set_with_value_obj as - select - hs.public_id, - hs.catalog_id, - hc.plugin_id, - hs.name, - hs.description, - hs.create_time, - hs.update_time, - hs.last_sync_time, - hs.need_sync, - hs.sync_interval_seconds, - hs.version, - hs.attributes, - -- the string_agg(..) column will be null if there are no associated value objects - string_agg(distinct concat_ws('=', hspe.priority, hspe.condition), '|') as preferred_endpoints, - string_agg(distinct hpsm.host_id, '|') as host_ids - from - host_plugin_set hs - join host_plugin_catalog hc on hs.catalog_id = hc.public_id - left outer join host_set_preferred_endpoint hspe on hs.public_id = hspe.host_set_id - left outer join host_plugin_set_member hpsm on hs.public_id = hpsm.set_id - group by hs.public_id, hc.plugin_id; -comment on view host_plugin_host_set_with_value_obj is -'host plugin host set with its associated value objects'; - -commit; \ No newline at end of file diff --git a/internal/host/plugin/repository_host_set.go b/internal/host/plugin/repository_host_set.go index 746115593..40988766e 100644 --- a/internal/host/plugin/repository_host_set.go +++ b/internal/host/plugin/repository_host_set.go @@ -10,7 +10,6 @@ import ( "github.com/hashicorp/boundary/internal/db/timestamp" "github.com/hashicorp/boundary/internal/errors" "github.com/hashicorp/boundary/internal/host" - "github.com/hashicorp/boundary/internal/host/store" "github.com/hashicorp/boundary/internal/kms" "github.com/hashicorp/boundary/internal/libs/endpoint" "github.com/hashicorp/boundary/internal/libs/patchstruct" @@ -358,9 +357,9 @@ func (r *Repository) UpdateSet(ctx context.Context, scopeId string, s *HostSet, ctx, db.StdRetryCnt, db.ExpBackoff{}, - func(r db.Reader, w db.Writer) error { + func(reader db.Reader, w db.Writer) error { returnedSet = newSet.clone() - msgs := make([]*oplog.Message, 0, len(preferredEndpoints)+2) + msgs := make([]*oplog.Message, 0, len(preferredEndpoints) + len(currentSet.PreferredEndpoints) + 2) ticket, err := w.GetTicket(s) if err != nil { return errors.Wrap(ctx, err, op, errors.WithMsg("unable to get ticket")) @@ -391,43 +390,43 @@ func (r *Repository) UpdateSet(ctx context.Context, scopeId string, s *HostSet, switch endpointOp { case endpointOpDelete: // Delete all old endpoint entries. - var peDeleteOplogMsg oplog.Message - pep := &host.PreferredEndpoint{ - PreferredEndpoint: &store.PreferredEndpoint{ - HostSetId: newSet.PublicId, - Priority: 1, - }, + var peps []interface{} + for i := 1; i <= len(currentSet.PreferredEndpoints); i++ { + p := host.AllocPreferredEndpoint() + p.HostSetId, p.Priority = currentSet.GetPublicId(), uint32(i) + peps = append(peps, p) } - _, err := w.Delete(ctx, pep, db.WithWhere("host_set_id = ?", newSet.PublicId), db.NewOplogMsg(&peDeleteOplogMsg)) + deleteOplogMsgs := make([]*oplog.Message, 0, len(peps)) + _, err := w.DeleteItems(ctx, peps, db.NewOplogMsgs(&deleteOplogMsgs)) if err != nil { return errors.Wrap(ctx, err, op) } // Only append the oplog message if an operation was actually // performed. - if peDeleteOplogMsg.Message != nil { + if len(deleteOplogMsgs) > 0 { preferredEndpointsUpdated = true - msgs = append(msgs, &peDeleteOplogMsg) + msgs = append(msgs, deleteOplogMsgs...) } case endpointOpUpdate: // Delete all old endpoint entries. - var peDeleteOplogMsg oplog.Message - pep := &host.PreferredEndpoint{ - PreferredEndpoint: &store.PreferredEndpoint{ - HostSetId: newSet.PublicId, - Priority: 1, - }, + var peps []interface{} + for i := 1; i <= len(currentSet.PreferredEndpoints); i++ { + p := host.AllocPreferredEndpoint() + p.HostSetId, p.Priority = currentSet.GetPublicId(), uint32(i) + peps = append(peps, p) } - _, err := w.Delete(ctx, pep, db.WithWhere("host_set_id = ?", newSet.PublicId), db.NewOplogMsg(&peDeleteOplogMsg)) + deleteOplogMsgs := make([]*oplog.Message, 0, len(peps)) + _, err := w.DeleteItems(ctx, peps, db.WithDebug(true), db.NewOplogMsgs(&deleteOplogMsgs)) if err != nil { return errors.Wrap(ctx, err, op) } // Only append the oplog message if an operation was actually // performed. - if peDeleteOplogMsg.Message != nil { - msgs = append(msgs, &peDeleteOplogMsg) + if len(deleteOplogMsgs) > 0 { + msgs = append(msgs, deleteOplogMsgs...) } // Create the new entries. @@ -469,6 +468,15 @@ func (r *Repository) UpdateSet(ctx context.Context, scopeId string, s *HostSet, } } + hsAgg := &hostSetAgg{PublicId: currentSet.GetPublicId()} + if err := reader.LookupByPublicId(ctx, hsAgg); err != nil { + return errors.Wrap(ctx, err, op, errors.WithMsg("looking up host after update")) + } + returnedSet, err = hsAgg.toHostSet(ctx) + if err != nil { + return errors.Wrap(ctx, err, op, errors.WithMsg("converting from host aggregate to host after update")) + } + if !pluginCalledSuccessfully { _, err = plgClient.OnUpdateSet(ctx, &plgpb.OnUpdateSetRequest{ CurrentSet: currentPlgSet, @@ -714,7 +722,7 @@ func toPluginSet(ctx context.Context, in *HostSet) (*pb.HostSet, error) { Id: in.GetPublicId(), Name: name, Description: description, - PreferredEndpoints: in.GetPreferredEndpoints(), + PreferredEndpoints: in.PreferredEndpoints, } if in.GetAttributes() != nil { attrs := &structpb.Struct{} diff --git a/internal/host/plugin/repository_host_set_test.go b/internal/host/plugin/repository_host_set_test.go index 514241881..6c1ce6302 100644 --- a/internal/host/plugin/repository_host_set_test.go +++ b/internal/host/plugin/repository_host_set_test.go @@ -19,7 +19,6 @@ 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" @@ -411,26 +410,9 @@ func TestRepository_UpdateSet(t *testing.T) { dbKmsCache := kms.TestKms(t, dbConn, dbWrapper) _, projectScope := 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 gotOnUpdateCallCount int - var gotOnUpdateSetRequest *plgpb.OnUpdateSetRequest - var pluginError error testPlugin := hostplg.TestPlugin(t, dbConn, "test") - testPluginMap := map[string]plgpb.HostPluginServiceClient{ - testPlugin.GetPublicId(): &WrappingPluginClient{ - Server: &TestPluginServer{ - OnUpdateSetFn: func(_ context.Context, req *plgpb.OnUpdateSetRequest) (*plgpb.OnUpdateSetResponse, error) { - gotOnUpdateCallCount++ - gotOnUpdateSetRequest = req - return &plgpb.OnUpdateSetResponse{}, pluginError - }, - }, - }, + dummyPluginMap := map[string]plgpb.HostPluginServiceClient{ + testPlugin.GetPublicId(): &WrappingPluginClient{Server: &plgpb.UnimplementedHostPluginServiceServer{}}, } // Set up a test catalog and the secrets for it @@ -451,7 +433,7 @@ func TestRepository_UpdateSet(t *testing.T) { // exists to ensure that we can test for conflicts when setting // the name. const testDuplicateSetName = "duplicate-set-name" - testDuplicateSet := TestSet(t, dbConn, dbKmsCache, sched, testCatalog, testPluginMap) + testDuplicateSet := TestSet(t, dbConn, dbKmsCache, sched, testCatalog, dummyPluginMap) testDuplicateSet.Name = testDuplicateSetName setsUpdated, err := dbRW.Update(ctx, testDuplicateSet, []string{"name"}, []string{}) require.NoError(t, err) @@ -525,176 +507,60 @@ func TestRepository_UpdateSet(t *testing.T) { // 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 ( - gotSet *HostSet - gotNumUpdated int - ) + type checkHostSetFunc func(t *testing.T, got *HostSet) - checkVersion := func(want uint32) checkFunc { - return func(t *testing.T, ctx context.Context) { + checkVersion := func(want uint32) checkHostSetFunc { + return func(t *testing.T, got *HostSet) { t.Helper() - assert := assert.New(t) - assert.Equal(want, gotSet.Version) + assert.Equal(t, want, got.Version, "checkVersion") } } - checkName := func(want string) checkFunc { - return func(t *testing.T, ctx context.Context) { + checkName := func(want string) checkHostSetFunc { + return func(t *testing.T, got *HostSet) { t.Helper() - assert := assert.New(t) - assert.Equal(want, gotSet.Name) + assert.Equal(t, want, got.Name, "checkName") } } - checkDescription := func(want string) checkFunc { - return func(t *testing.T, ctx context.Context) { + checkDescription := func(want string) checkHostSetFunc { + return func(t *testing.T, got *HostSet) { t.Helper() - assert := assert.New(t) - assert.Equal(want, gotSet.Description) + assert.Equal(t, want, got.Description, "checkDescription") } } - checkPreferredEndpoints := func(want []string) checkFunc { - return func(t *testing.T, ctx context.Context) { + checkPreferredEndpoints := func(want []string) checkHostSetFunc { + return func(t *testing.T, got *HostSet) { t.Helper() - assert := assert.New(t) - assert.Equal(want, gotSet.PreferredEndpoints) + assert.Equal(t, want, got.PreferredEndpoints, "checkPreferredEndpoints") } } - checkAttributes := func(want map[string]interface{}) checkFunc { - return func(t *testing.T, ctx context.Context) { + checkAttributes := func(want map[string]interface{}) checkHostSetFunc { + return func(t *testing.T, got *HostSet) { t.Helper() - assert := assert.New(t) - require := require.New(t) st := &structpb.Struct{} - require.NoError(proto.Unmarshal(gotSet.Attributes, st)) - assert.Empty(cmp.Diff(mustStruct(want), st, protocmp.Transform())) + require.NoError(t, proto.Unmarshal(got.Attributes, st)) + assert.Empty(t, cmp.Diff(mustStruct(want), st, protocmp.Transform())) } } - checkNeedSync := func(want bool) checkFunc { - return func(t *testing.T, ctx context.Context) { + checkNeedSync := func(want bool) checkHostSetFunc { + return func(t *testing.T, got *HostSet) { t.Helper() - assert := assert.New(t) - assert.Equal(want, gotSet.NeedSync) + assert.Equal(t, want, got.NeedSync, "checkNeedSync") } } - checkUpdateSetRequestCurrentNameNil := func() checkFunc { - return func(t *testing.T, ctx context.Context) { + checkVerifySetOplog := func(op oplog.OpType) checkHostSetFunc { + return func(t *testing.T, got *HostSet) { t.Helper() - assert := assert.New(t) - assert.Nil(gotOnUpdateSetRequest.CurrentSet.Name) - } - } - - checkUpdateSetRequestNewName := func(want string) checkFunc { - return func(t *testing.T, ctx context.Context) { - t.Helper() - assert := assert.New(t) - assert.Equal(wrapperspb.String(want), gotOnUpdateSetRequest.NewSet.Name) - } - } - - checkUpdateSetRequestNewNameNil := func() checkFunc { - return func(t *testing.T, ctx context.Context) { - t.Helper() - assert := assert.New(t) - assert.Nil(gotOnUpdateSetRequest.NewSet.Name) - } - } - - checkUpdateSetRequestCurrentDescriptionNil := func() checkFunc { - return func(t *testing.T, ctx context.Context) { - t.Helper() - assert := assert.New(t) - assert.Nil(gotOnUpdateSetRequest.CurrentSet.Description) - } - } - - checkUpdateSetRequestNewDescription := func(want string) checkFunc { - return func(t *testing.T, ctx context.Context) { - t.Helper() - assert := assert.New(t) - assert.Equal(wrapperspb.String(want), gotOnUpdateSetRequest.NewSet.Description) - } - } - - checkUpdateSetRequestNewDescriptionNil := func() checkFunc { - return func(t *testing.T, ctx context.Context) { - t.Helper() - assert := assert.New(t) - assert.Nil(gotOnUpdateSetRequest.NewSet.Description) - } - } - - checkUpdateSetRequestCurrentPreferredEndpoints := func(want []string) checkFunc { - return func(t *testing.T, ctx context.Context) { - t.Helper() - assert := assert.New(t) - assert.Equal(want, gotOnUpdateSetRequest.CurrentSet.PreferredEndpoints) - } - } - - checkUpdateSetRequestNewPreferredEndpoints := func(want []string) checkFunc { - return func(t *testing.T, ctx context.Context) { - t.Helper() - assert := assert.New(t) - assert.Equal(want, gotOnUpdateSetRequest.NewSet.PreferredEndpoints) - } - } - - checkUpdateSetRequestNewPreferredEndpointsNil := func() checkFunc { - return func(t *testing.T, ctx context.Context) { - t.Helper() - assert := assert.New(t) - assert.Nil(gotOnUpdateSetRequest.NewSet.PreferredEndpoints) - } - } - - checkUpdateSetRequestCurrentAttributes := 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), gotOnUpdateSetRequest.CurrentSet.Attributes, protocmp.Transform())) - } - } - - checkUpdateSetRequestNewAttributes := 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), gotOnUpdateSetRequest.NewSet.Attributes, protocmp.Transform())) - } - } - - checkUpdateSetRequestPersistedSecrets := 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), gotOnUpdateSetRequest.Persisted.Secrets, protocmp.Transform())) - } - } - - checkNumUpdated := func(want int) checkFunc { - return func(t *testing.T, ctx context.Context) { - t.Helper() - assert := assert.New(t) - assert.Equal(want, gotNumUpdated) - } - } - - checkVerifySetOplog := func(op oplog.OpType) checkFunc { - return func(t *testing.T, ctx context.Context) { - t.Helper() - assert := assert.New(t) - assert.NoError( + assert.NoError(t, db.TestVerifyOplog( t, dbRW, - gotSet.PublicId, + got.PublicId, db.WithOperation(op), db.WithCreateNotBefore(10*time.Second), ), @@ -702,6 +568,92 @@ func TestRepository_UpdateSet(t *testing.T) { } } + type checkPluginReqFunc func(t *testing.T, got *plgpb.OnUpdateSetRequest) + + checkUpdateSetRequestCurrentNameNil := func() checkPluginReqFunc { + return func(t *testing.T, got *plgpb.OnUpdateSetRequest) { + t.Helper() + assert.Nil(t, got.CurrentSet.Name, "checkUpdateSetRequestCurrentNameNil") + } + } + + checkUpdateSetRequestNewName := func(want string) checkPluginReqFunc { + return func(t *testing.T, got *plgpb.OnUpdateSetRequest) { + t.Helper() + assert.Equal(t, wrapperspb.String(want), got.NewSet.Name, "checkUpdateSetRequestNewName") + } + } + + checkUpdateSetRequestNewNameNil := func() checkPluginReqFunc { + return func(t *testing.T, got *plgpb.OnUpdateSetRequest) { + t.Helper() + assert.Nil(t, got.NewSet.Name, "checkUpdateSetRequestNewNameNil") + } + } + + checkUpdateSetRequestCurrentDescriptionNil := func() checkPluginReqFunc { + return func(t *testing.T, got *plgpb.OnUpdateSetRequest) { + t.Helper() + assert.Nil(t, got.CurrentSet.Description, "checkUpdateSetRequestCurrentDescriptionNil") + } + } + + checkUpdateSetRequestNewDescription := func(want string) checkPluginReqFunc { + return func(t *testing.T, got *plgpb.OnUpdateSetRequest) { + t.Helper() + assert.Equal(t, wrapperspb.String(want), got.NewSet.Description, "checkUpdateSetRequestNewDescription") + } + } + + checkUpdateSetRequestNewDescriptionNil := func() checkPluginReqFunc { + return func(t *testing.T, got *plgpb.OnUpdateSetRequest) { + t.Helper() + assert.Nil(t, got.NewSet.Description, "checkUpdateSetRequestNewDescriptionNil") + } + } + + checkUpdateSetRequestCurrentPreferredEndpoints := func(want []string) checkPluginReqFunc { + return func(t *testing.T, got *plgpb.OnUpdateSetRequest) { + t.Helper() + assert.Equal(t, want, got.CurrentSet.PreferredEndpoints, "checkUpdateSetRequestCurrentPreferredEndpoints") + } + } + + checkUpdateSetRequestNewPreferredEndpoints := func(want []string) checkPluginReqFunc { + return func(t *testing.T, got *plgpb.OnUpdateSetRequest) { + t.Helper() + assert.Equal(t, want, got.NewSet.PreferredEndpoints, "checkUpdateSetRequestNewPreferredEndpoints") + } + } + + checkUpdateSetRequestNewPreferredEndpointsNil := func() checkPluginReqFunc { + return func(t *testing.T, got *plgpb.OnUpdateSetRequest) { + t.Helper() + assert.Nil(t, got.NewSet.PreferredEndpoints, "checkUpdateSetRequestNewPreferredEndpointsNil") + } + } + + checkUpdateSetRequestCurrentAttributes := func(want map[string]interface{}) checkPluginReqFunc { + return func(t *testing.T, got *plgpb.OnUpdateSetRequest) { + t.Helper() + assert.Empty(t, cmp.Diff(mustStruct(want), got.CurrentSet.Attributes, protocmp.Transform()), "checkUpdateSetRequestCurrentAttributes") + } + } + + checkUpdateSetRequestNewAttributes := func(want map[string]interface{}) checkPluginReqFunc { + return func(t *testing.T, got *plgpb.OnUpdateSetRequest) { + t.Helper() + assert.Empty(t, cmp.Diff(mustStruct(want), got.NewSet.Attributes, protocmp.Transform()), "checkUpdateSetRequestNewAttributes") + } + } + + checkUpdateSetRequestPersistedSecrets := func(want map[string]interface{}) checkPluginReqFunc { + return func(t *testing.T, got *plgpb.OnUpdateSetRequest) { + t.Helper() + assert.Empty(t, cmp.Diff(mustStruct(want), got.Persisted.Secrets, protocmp.Transform()), "checkUpdateSetRequestPersistedSecrets") + } + } + tests := []struct { name string withScopeId *string @@ -709,9 +661,10 @@ func TestRepository_UpdateSet(t *testing.T) { withPluginError error changeFuncs []changeHostSetFunc version uint32 - fieldMask []string - wantCheckFuncs []checkFunc - wantIsErr errors.Code + fieldMask []string + wantCheckSetFuncs []checkHostSetFunc + wantCheckPluginReqFuncs []checkPluginReqFunc + wantIsErr errors.Code }{ { name: "nil set", @@ -784,16 +737,17 @@ func TestRepository_UpdateSet(t *testing.T) { changeFuncs: []changeHostSetFunc{changeName("foo")}, version: 2, fieldMask: []string{"name"}, - wantCheckFuncs: []checkFunc{ - checkVersion(3), + wantCheckPluginReqFuncs: []checkPluginReqFunc{ checkUpdateSetRequestCurrentNameNil(), checkUpdateSetRequestNewName("foo"), - checkName("foo"), checkUpdateSetRequestPersistedSecrets(map[string]interface{}{ "one": "two", }), + }, + wantCheckSetFuncs: []checkHostSetFunc{ + checkVersion(3), + checkName("foo"), checkNeedSync(false), - checkNumUpdated(1), checkVerifySetOplog(oplog.OpType_OP_TYPE_UPDATE), }, }, @@ -802,16 +756,17 @@ func TestRepository_UpdateSet(t *testing.T) { changeFuncs: []changeHostSetFunc{changeName("")}, version: 2, fieldMask: []string{"name"}, - wantCheckFuncs: []checkFunc{ - checkVersion(2), // Version remains same even though row is updated + wantCheckPluginReqFuncs: []checkPluginReqFunc{ checkUpdateSetRequestCurrentNameNil(), checkUpdateSetRequestNewNameNil(), - checkName(""), checkUpdateSetRequestPersistedSecrets(map[string]interface{}{ "one": "two", }), + }, + wantCheckSetFuncs: []checkHostSetFunc{ + checkVersion(2), // Version remains same even though row is updated + checkName(""), checkNeedSync(false), - checkNumUpdated(1), checkVerifySetOplog(oplog.OpType_OP_TYPE_UPDATE), }, }, @@ -820,16 +775,17 @@ func TestRepository_UpdateSet(t *testing.T) { changeFuncs: []changeHostSetFunc{changeDescription("foo")}, version: 2, fieldMask: []string{"description"}, - wantCheckFuncs: []checkFunc{ - checkVersion(3), + wantCheckPluginReqFuncs: []checkPluginReqFunc{ checkUpdateSetRequestCurrentDescriptionNil(), checkUpdateSetRequestNewDescription("foo"), - checkDescription("foo"), checkUpdateSetRequestPersistedSecrets(map[string]interface{}{ "one": "two", }), + }, + wantCheckSetFuncs: []checkHostSetFunc{ + checkVersion(3), + checkDescription("foo"), checkNeedSync(false), - checkNumUpdated(1), checkVerifySetOplog(oplog.OpType_OP_TYPE_UPDATE), }, }, @@ -838,16 +794,17 @@ func TestRepository_UpdateSet(t *testing.T) { changeFuncs: []changeHostSetFunc{changeDescription("")}, version: 2, fieldMask: []string{"description"}, - wantCheckFuncs: []checkFunc{ - checkVersion(2), // Version remains same even though row is updated + wantCheckPluginReqFuncs: []checkPluginReqFunc{ checkUpdateSetRequestCurrentDescriptionNil(), checkUpdateSetRequestNewDescriptionNil(), - checkDescription(""), checkUpdateSetRequestPersistedSecrets(map[string]interface{}{ "one": "two", }), + }, + wantCheckSetFuncs: []checkHostSetFunc{ + checkVersion(2), // Version remains same even though row is updated + checkDescription(""), checkNeedSync(false), - checkNumUpdated(1), checkVerifySetOplog(oplog.OpType_OP_TYPE_UPDATE), }, }, @@ -856,16 +813,17 @@ func TestRepository_UpdateSet(t *testing.T) { changeFuncs: []changeHostSetFunc{changePreferredEndpoints([]string{"cidr:10.0.0.0/24"})}, version: 2, fieldMask: []string{"PreferredEndpoints"}, - wantCheckFuncs: []checkFunc{ - checkVersion(3), + wantCheckPluginReqFuncs: []checkPluginReqFunc{ checkUpdateSetRequestCurrentPreferredEndpoints([]string{"cidr:192.168.0.0/24", "cidr:192.168.1.0/24", "cidr:172.16.0.0/12"}), checkUpdateSetRequestNewPreferredEndpoints([]string{"cidr:10.0.0.0/24"}), - checkPreferredEndpoints([]string{"cidr:10.0.0.0/24"}), checkUpdateSetRequestPersistedSecrets(map[string]interface{}{ "one": "two", }), + }, + wantCheckSetFuncs: []checkHostSetFunc{ + checkVersion(3), + checkPreferredEndpoints([]string{"cidr:10.0.0.0/24"}), checkNeedSync(false), - checkNumUpdated(1), checkVerifySetOplog(oplog.OpType_OP_TYPE_UPDATE), }, }, @@ -874,16 +832,17 @@ func TestRepository_UpdateSet(t *testing.T) { changeFuncs: []changeHostSetFunc{changePreferredEndpoints(nil)}, version: 2, fieldMask: []string{"PreferredEndpoints"}, - wantCheckFuncs: []checkFunc{ - checkVersion(3), + wantCheckPluginReqFuncs: []checkPluginReqFunc{ checkUpdateSetRequestCurrentPreferredEndpoints([]string{"cidr:192.168.0.0/24", "cidr:192.168.1.0/24", "cidr:172.16.0.0/12"}), checkUpdateSetRequestNewPreferredEndpointsNil(), - checkPreferredEndpoints(nil), checkUpdateSetRequestPersistedSecrets(map[string]interface{}{ "one": "two", }), + }, + wantCheckSetFuncs: []checkHostSetFunc{ + checkVersion(3), + checkPreferredEndpoints(nil), checkNeedSync(false), - checkNumUpdated(1), }, }, { @@ -893,8 +852,7 @@ func TestRepository_UpdateSet(t *testing.T) { })}, version: 2, fieldMask: []string{"attributes"}, - wantCheckFuncs: []checkFunc{ - checkVersion(3), + wantCheckPluginReqFuncs: []checkPluginReqFunc{ checkUpdateSetRequestCurrentAttributes(map[string]interface{}{ "foo": "bar", }), @@ -902,15 +860,17 @@ func TestRepository_UpdateSet(t *testing.T) { "foo": "bar", "baz": "qux", }), + checkUpdateSetRequestPersistedSecrets(map[string]interface{}{ + "one": "two", + }), + }, + wantCheckSetFuncs: []checkHostSetFunc{ + checkVersion(3), checkAttributes(map[string]interface{}{ "foo": "bar", "baz": "qux", }), - checkUpdateSetRequestPersistedSecrets(map[string]interface{}{ - "one": "two", - }), checkNeedSync(true), - checkNumUpdated(1), checkVerifySetOplog(oplog.OpType_OP_TYPE_UPDATE), }, }, @@ -921,22 +881,23 @@ func TestRepository_UpdateSet(t *testing.T) { })}, version: 2, fieldMask: []string{"attributes"}, - wantCheckFuncs: []checkFunc{ - checkVersion(3), + wantCheckPluginReqFuncs: []checkPluginReqFunc{ checkUpdateSetRequestCurrentAttributes(map[string]interface{}{ "foo": "bar", }), checkUpdateSetRequestNewAttributes(map[string]interface{}{ "foo": "baz", }), - checkAttributes(map[string]interface{}{ - "foo": "baz", - }), checkUpdateSetRequestPersistedSecrets(map[string]interface{}{ "one": "two", }), + }, + wantCheckSetFuncs: []checkHostSetFunc{ + checkVersion(3), + checkAttributes(map[string]interface{}{ + "foo": "baz", + }), checkNeedSync(true), - checkNumUpdated(1), checkVerifySetOplog(oplog.OpType_OP_TYPE_UPDATE), }, }, @@ -947,18 +908,19 @@ func TestRepository_UpdateSet(t *testing.T) { })}, version: 2, fieldMask: []string{"attributes"}, - wantCheckFuncs: []checkFunc{ - checkVersion(3), + wantCheckPluginReqFuncs: []checkPluginReqFunc{ checkUpdateSetRequestCurrentAttributes(map[string]interface{}{ "foo": "bar", }), checkUpdateSetRequestNewAttributes(map[string]interface{}{}), - checkAttributes(map[string]interface{}{}), checkUpdateSetRequestPersistedSecrets(map[string]interface{}{ "one": "two", }), + }, + wantCheckSetFuncs: []checkHostSetFunc{ + checkVersion(3), + checkAttributes(map[string]interface{}{}), checkNeedSync(true), - checkNumUpdated(1), checkVerifySetOplog(oplog.OpType_OP_TYPE_UPDATE), }, }, @@ -967,18 +929,19 @@ func TestRepository_UpdateSet(t *testing.T) { changeFuncs: []changeHostSetFunc{changeAttributesNil()}, version: 2, fieldMask: []string{"attributes"}, - wantCheckFuncs: []checkFunc{ - checkVersion(3), + wantCheckPluginReqFuncs: []checkPluginReqFunc{ checkUpdateSetRequestCurrentAttributes(map[string]interface{}{ "foo": "bar", }), checkUpdateSetRequestNewAttributes(map[string]interface{}{}), - checkAttributes(map[string]interface{}{}), checkUpdateSetRequestPersistedSecrets(map[string]interface{}{ "one": "two", }), + }, + wantCheckSetFuncs: []checkHostSetFunc{ + checkVersion(3), + checkAttributes(map[string]interface{}{}), checkNeedSync(true), - checkNumUpdated(1), checkVerifySetOplog(oplog.OpType_OP_TYPE_UPDATE), }, }, @@ -990,8 +953,7 @@ func TestRepository_UpdateSet(t *testing.T) { })}, version: 2, fieldMask: []string{"attributes.a", "attributes.foo"}, - wantCheckFuncs: []checkFunc{ - checkVersion(3), + wantCheckPluginReqFuncs: []checkPluginReqFunc{ checkUpdateSetRequestCurrentAttributes(map[string]interface{}{ "foo": "bar", }), @@ -999,15 +961,17 @@ func TestRepository_UpdateSet(t *testing.T) { "a": "b", "foo": "baz", }), + checkUpdateSetRequestPersistedSecrets(map[string]interface{}{ + "one": "two", + }), + }, + wantCheckSetFuncs: []checkHostSetFunc{ + checkVersion(3), checkAttributes(map[string]interface{}{ "a": "b", "foo": "baz", }), - checkUpdateSetRequestPersistedSecrets(map[string]interface{}{ - "one": "two", - }), checkNeedSync(true), - checkNumUpdated(1), checkVerifySetOplog(oplog.OpType_OP_TYPE_UPDATE), }, }, @@ -1019,19 +983,20 @@ func TestRepository_UpdateSet(t *testing.T) { }, version: 2, fieldMask: []string{"name", "PreferredEndpoints"}, - wantCheckFuncs: []checkFunc{ - checkVersion(3), + wantCheckPluginReqFuncs: []checkPluginReqFunc{ checkUpdateSetRequestCurrentNameNil(), checkUpdateSetRequestNewName("foo"), - checkName("foo"), checkUpdateSetRequestCurrentPreferredEndpoints([]string{"cidr:192.168.0.0/24", "cidr:192.168.1.0/24", "cidr:172.16.0.0/12"}), checkUpdateSetRequestNewPreferredEndpoints([]string{"cidr:10.0.0.0/24"}), - checkPreferredEndpoints([]string{"cidr:10.0.0.0/24"}), checkUpdateSetRequestPersistedSecrets(map[string]interface{}{ "one": "two", }), + }, + wantCheckSetFuncs: []checkHostSetFunc{ + checkVersion(3), + checkName("foo"), + checkPreferredEndpoints([]string{"cidr:10.0.0.0/24"}), checkNeedSync(false), - checkNumUpdated(1), checkVerifySetOplog(oplog.OpType_OP_TYPE_UPDATE), }, }, @@ -1056,7 +1021,7 @@ func TestRepository_UpdateSet(t *testing.T) { dbKmsCache, sched, testCatalog, - testPluginMap, + dummyPluginMap, WithPreferredEndpoints([]string{"cidr:192.168.0.0/24", "cidr:192.168.1.0/24", "cidr:172.16.0.0/12"}), ) // Set some (default) attributes on our test set @@ -1091,14 +1056,34 @@ func TestRepository_UpdateSet(t *testing.T) { t.Run(tt.name, func(t *testing.T) { require := require.New(t) assert := assert.New(t) + + // 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 gotOnUpdateCallCount int + testPluginMap := map[string]plgpb.HostPluginServiceClient{ + testPlugin.GetPublicId(): &WrappingPluginClient{ + Server: &TestPluginServer{ + OnUpdateSetFn: func(_ context.Context, req *plgpb.OnUpdateSetRequest) (*plgpb.OnUpdateSetResponse, error) { + gotOnUpdateCallCount++ + for _, check := range tt.wantCheckPluginReqFuncs { + check(t, req) + } + return &plgpb.OnUpdateSetResponse{}, tt.withPluginError + }, + }, + }, + } + origSet := setupHostSet(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) @@ -1113,9 +1098,7 @@ func TestRepository_UpdateSet(t *testing.T) { scopeId = *tt.withScopeId } - var gotHosts []*Host - var gotPlugin *hostplugin.Plugin - gotSet, gotHosts, gotPlugin, gotNumUpdated, err = repo.UpdateSet(ctx, scopeId, workingSet, tt.version, tt.fieldMask) + gotUpdatedSet, gotHosts, gotPlugin, gotNumUpdated, err := repo.UpdateSet(ctx, scopeId, workingSet, tt.version, tt.fieldMask) t.Cleanup(func() { gotOnUpdateCallCount = 0 }) if tt.wantIsErr != 0 { require.Equal(db.NoRowsAffected, gotNumUpdated) @@ -1124,14 +1107,14 @@ func TestRepository_UpdateSet(t *testing.T) { } require.NoError(err) require.Equal(1, gotOnUpdateCallCount) - t.Cleanup(func() { gotOnUpdateSetRequest = nil }) + assert.Equal(1, gotNumUpdated) // Quick assertion that the set is not nil and that the plugin // ID in the catalog referenced by the set matches the plugin // ID in the returned plugin. - require.NotNil(gotSet) + require.NotNil(gotUpdatedSet) require.NotNil(gotPlugin) - assert.Equal(testCatalog.PublicId, gotSet.CatalogId) + assert.Equal(testCatalog.PublicId, gotUpdatedSet.CatalogId) assert.Equal(testCatalog.PluginId, gotPlugin.PublicId) // Also assert that the hosts returned by the request are the ones that belong to the set @@ -1146,9 +1129,12 @@ func TestRepository_UpdateSet(t *testing.T) { assert.Equal(wantHostMap, gotHostMap) // Perform checks - for _, check := range tt.wantCheckFuncs { - check(t, ctx) + for _, check := range tt.wantCheckSetFuncs { + check(t, gotUpdatedSet) } + + gotLookupSet, gotPlugin, err := repo.LookupSet(ctx, workingSet.GetPublicId()) + assert.Empty(cmp.Diff(gotUpdatedSet, gotLookupSet, protocmp.Transform())) }) } } diff --git a/internal/host/preferred_endpoint.go b/internal/host/preferred_endpoint.go index 9dc32dd5f..621448d85 100644 --- a/internal/host/preferred_endpoint.go +++ b/internal/host/preferred_endpoint.go @@ -59,8 +59,8 @@ func (pe *PreferredEndpoint) validate(ctx context.Context, caller errors.Op) err } // AllocPreferredEndpoint makes an empty one in-memory -func AllocPreferredEndpoint() PreferredEndpoint { - return PreferredEndpoint{ +func AllocPreferredEndpoint() *PreferredEndpoint { + return &PreferredEndpoint{ PreferredEndpoint: &store.PreferredEndpoint{}, } } diff --git a/internal/host/preferred_endpoint_test.go b/internal/host/preferred_endpoint_test.go index 47151564f..a4a6a6ff9 100644 --- a/internal/host/preferred_endpoint_test.go +++ b/internal/host/preferred_endpoint_test.go @@ -61,7 +61,7 @@ func TestPreferredEndpoint_Create(t *testing.T) { want.HostSetId = set.PublicId want.Condition = "cidr:1.2.3.4" want.Priority = 1 - return &want + return want }(), }, { @@ -77,7 +77,7 @@ func TestPreferredEndpoint_Create(t *testing.T) { want.HostSetId = set.PublicId want.Condition = "cidr:1.2.3.4" want.Priority = 1 - return &want + return want }(), wantCreateErr: true, wantCreateIsErr: errors.NotUnique, @@ -141,8 +141,8 @@ func TestPreferredEndpoint_Create(t *testing.T) { assert.NoError(err) } found := host.AllocPreferredEndpoint() - require.NoError(rw.LookupWhere(ctx, &found, "host_set_id = ? and priority = ?", tt.args.hostSetId, tt.args.priority)) - assert.Equal(got, &found) + require.NoError(rw.LookupWhere(ctx, found, "host_set_id = ? and priority = ?", tt.args.hostSetId, tt.args.priority)) + assert.Equal(got, found) } }) }