From 5b5aa484dae02e79492fe65636ad0877df62abf2 Mon Sep 17 00:00:00 2001 From: Hugo <10965479+hugoghx@users.noreply.github.com> Date: Wed, 4 Sep 2024 15:05:47 +0100 Subject: [PATCH] feat(host/plugin): Extract plugin client functionality to pkg func --- internal/host/plugin/repository.go | 18 +++++++ .../host/plugin/repository_host_catalog.go | 24 ++++----- .../plugin/repository_host_catalog_test.go | 51 ------------------- internal/host/plugin/repository_host_set.go | 31 +++++------ internal/host/plugin/repository_test.go | 43 ++++++++++++++++ 5 files changed, 89 insertions(+), 78 deletions(-) diff --git a/internal/host/plugin/repository.go b/internal/host/plugin/repository.go index 87a3fa4ab3..73bdd5dbc1 100644 --- a/internal/host/plugin/repository.go +++ b/internal/host/plugin/repository.go @@ -8,15 +8,19 @@ package plugin import ( "context" + "fmt" "github.com/hashicorp/boundary/internal/db" "github.com/hashicorp/boundary/internal/errors" "github.com/hashicorp/boundary/internal/host" "github.com/hashicorp/boundary/internal/kms" "github.com/hashicorp/boundary/internal/scheduler" + apihc "github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/hostcatalogs" plgpb "github.com/hashicorp/boundary/sdk/pbs/plugin" ) +var pluginClientFactoryFn = pluginClientFactory + // A Repository stores and retrieves the persistent types in the plugin // package. It is not safe to use a repository concurrently. type Repository struct { @@ -74,3 +78,17 @@ func NewRepository(ctx context.Context, r db.Reader, w db.Writer, kms *kms.Kms, defaultLimit: opts.WithLimit, }, nil } + +func pluginClientFactory(ctx context.Context, hc *apihc.HostCatalog, controllerClients map[string]plgpb.HostPluginServiceClient) (plgpb.HostPluginServiceClient, error) { + const op = "plugin.getPluginClient" + if hc == nil { + return nil, errors.New(ctx, errors.Internal, op, "host catalog object not present") + } + + cl, ok := controllerClients[hc.GetPluginId()] + if !ok || cl == nil { + return nil, errors.New(ctx, errors.Internal, op, fmt.Sprintf("controller plugin %q not available", hc.GetPluginId())) + } + + return cl, nil +} diff --git a/internal/host/plugin/repository_host_catalog.go b/internal/host/plugin/repository_host_catalog.go index a4846e9719..5448863fcb 100644 --- a/internal/host/plugin/repository_host_catalog.go +++ b/internal/host/plugin/repository_host_catalog.go @@ -128,9 +128,9 @@ func (r *Repository) CreateCatalog(ctx context.Context, c *HostCatalog, _ ...Opt if err != nil { return nil, nil, errors.Wrap(ctx, err, op) } - plgClient, ok := r.plugins[c.GetPluginId()] - if !ok || plgClient == nil { - return nil, nil, errors.New(ctx, errors.InvalidParameter, op, fmt.Sprintf("plugin %q not available", c.GetPluginId())) + plgClient, err := pluginClientFactoryFn(ctx, plgHc, r.plugins) + if err != nil { + return nil, nil, errors.Wrap(ctx, err, op) } if plgHc.GetAttributes() != nil { @@ -358,12 +358,6 @@ func (r *Repository) UpdateCatalog(ctx context.Context, c *HostCatalog, version return nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op) } - // Get the plugin client. - plgClient, ok := r.plugins[currentCatalog.GetPluginId()] - if !ok || plgClient == nil { - return nil, nil, db.NoRowsAffected, errors.New(ctx, errors.Internal, op, fmt.Sprintf("plugin %q not available", currentCatalog.GetPluginId())) - } - // Convert the catalog values to API protobuf values, which is what // we use for the plugin hook calls. currPlgHc, err := toPluginCatalog(ctx, currentCatalog, plg) @@ -375,6 +369,11 @@ func (r *Repository) UpdateCatalog(ctx context.Context, c *HostCatalog, version return nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op) } + plgClient, err := pluginClientFactoryFn(ctx, newPlgHc, r.plugins) + if err != nil { + return nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op) + } + if updateAttributes { if newPlgHc.GetAttributes() != nil { if err := normalizeCatalogAttributes(ctx, plgClient, newPlgHc); err != nil { @@ -639,9 +638,9 @@ func (r *Repository) DeleteCatalog(ctx context.Context, id string, _ ...Option) plgSets = append(plgSets, ps) } - plgClient, ok := r.plugins[c.GetPluginId()] - if !ok || plgClient == nil { - return 0, errors.New(ctx, errors.InvalidParameter, op, fmt.Sprintf("plugin %q not available", c.GetPluginId())) + plgClient, err := pluginClientFactoryFn(ctx, plgHc, r.plugins) + if err != nil { + return db.NoRowsAffected, errors.Wrap(ctx, err, op) } _, err = plgClient.OnDeleteCatalog(ctx, &plgpb.OnDeleteCatalogRequest{ Catalog: plgHc, @@ -749,6 +748,7 @@ func toPluginCatalog(ctx context.Context, in *HostCatalog, plg *plg.Plugin) (*pb Name: name, Description: description, WorkerFilter: workerFilter, + PluginId: in.GetPluginId(), Plugin: toPluginInfo(plg), } if len(in.GetSecretsHmac()) > 0 { diff --git a/internal/host/plugin/repository_host_catalog_test.go b/internal/host/plugin/repository_host_catalog_test.go index 5f26818470..f1c03f189a 100644 --- a/internal/host/plugin/repository_host_catalog_test.go +++ b/internal/host/plugin/repository_host_catalog_test.go @@ -260,26 +260,6 @@ func TestRepository_CreateCatalog(t *testing.T) { }(), wantPluginCalled: true, }, - { - name: "valid-with-worker-filter", - in: &HostCatalog{ - HostCatalog: &store.HostCatalog{ - ProjectId: prj.GetPublicId(), - PluginId: plg.GetPublicId(), - Attributes: []byte{}, - WorkerFilter: `"test" in "/tags/type"`, - }, - }, - want: &HostCatalog{ - HostCatalog: &store.HostCatalog{ - ProjectId: prj.GetPublicId(), - PluginId: plg.GetPublicId(), - Attributes: []byte{}, - WorkerFilter: `"test" in "/tags/type"`, - }, - }, - wantPluginCalled: true, - }, } for _, tt := range tests { @@ -634,13 +614,6 @@ func TestRepository_UpdateCatalog(t *testing.T) { } } - changeWorkerFilter := func(s string) changeHostCatalogFunc { - return func(c *HostCatalog) *HostCatalog { - c.WorkerFilter = s - return c - } - } - // Define some checks that will be used in the below tests. Some of // these are re-used, so we define them here. Most of these are // assertions and no particular one is non-fatal in that they will @@ -845,14 +818,6 @@ func TestRepository_UpdateCatalog(t *testing.T) { } } - checkWorkerFilter := func(want string) checkFunc { - return func(t *testing.T, ctx context.Context) { - t.Helper() - assert := assert.New(t) - assert.Equal(want, gotCatalog.WorkerFilter) - } - } - tests := []struct { name string withEmptyPluginMap bool @@ -1213,22 +1178,6 @@ func TestRepository_UpdateCatalog(t *testing.T) { checkVerifyCatalogOplog(oplog.OpType_OP_TYPE_UPDATE), }, }, - { - name: "update worker filter", - changeFuncs: []changeHostCatalogFunc{changeWorkerFilter(`"test" in "/tags/type"`)}, - version: 2, - fieldMask: []string{"WorkerFilter"}, - wantCheckFuncs: []checkFunc{ - checkVersion(3), - checkSecretsHmac(true), - checkWorkerFilter(`"test" in "/tags/type"`), - checkSecrets(map[string]any{ - "one": "two", - }), - checkNumUpdated(1), - checkVerifyCatalogOplog(oplog.OpType_OP_TYPE_UPDATE), - }, - }, } // Finally define a function for bringing the test subject catalog. diff --git a/internal/host/plugin/repository_host_set.go b/internal/host/plugin/repository_host_set.go index de912ed424..a3b32c7857 100644 --- a/internal/host/plugin/repository_host_set.go +++ b/internal/host/plugin/repository_host_set.go @@ -119,11 +119,6 @@ func (r *Repository) CreateSet(ctx context.Context, projectId string, s *HostSet s.LastSyncTime = timestamp.New(time.Unix(0, 0)) s.NeedSync = true - plgClient, ok := r.plugins[c.GetPluginId()] - if !ok || plgClient == nil { - return nil, nil, errors.New(ctx, errors.Internal, op, fmt.Sprintf("plugin %q not available", c.GetPluginId())) - } - plg, err := r.getPlugin(ctx, c.GetPluginId()) if err != nil { return nil, nil, errors.Wrap(ctx, err, op) @@ -137,6 +132,11 @@ func (r *Repository) CreateSet(ctx context.Context, projectId string, s *HostSet return nil, nil, errors.Wrap(ctx, err, op) } + plgClient, err := pluginClientFactoryFn(ctx, plgHc, r.plugins) + if err != nil { + return nil, nil, errors.Wrap(ctx, err, op) + } + if plgHs.GetAttributes() != nil { if err := normalizeSetAttributes(ctx, plgClient, plgHc, plgHs); err != nil { return nil, nil, errors.Wrap(ctx, err, op) @@ -363,12 +363,6 @@ func (r *Repository) UpdateSet(ctx context.Context, projectId string, s *HostSet return nil, nil, nil, db.NoRowsAffected, errors.New(ctx, errors.InvalidParameter, op, fmt.Sprintf("catalog id %q not in project id %q", newSet.CatalogId, projectId)) } - // Get the plugin client. - plgClient, ok := r.plugins[catalog.GetPluginId()] - if !ok || plgClient == nil { - return nil, nil, nil, db.NoRowsAffected, errors.New(ctx, errors.Internal, op, fmt.Sprintf("plugin %q not available", catalog.GetPluginId())) - } - // Convert the catalog values to API protobuf values, which is what // we use for the plugin hook calls. plgHc, err := toPluginCatalog(ctx, catalog, plg) @@ -384,6 +378,11 @@ func (r *Repository) UpdateSet(ctx context.Context, projectId string, s *HostSet return nil, nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op) } + plgClient, err := pluginClientFactoryFn(ctx, plgHc, r.plugins) + if err != nil { + return nil, nil, nil, db.NoRowsAffected, errors.Wrap(ctx, err, op) + } + if updateAttributes { if newPlgSet.GetAttributes() != nil { if err := normalizeSetAttributes(ctx, plgClient, plgHc, newPlgSet); err != nil { @@ -737,10 +736,6 @@ func (r *Repository) DeleteSet(ctx context.Context, projectId string, publicId s return db.NoRowsAffected, nil } - plgClient, ok := r.plugins[plg.GetPublicId()] - if !ok || plgClient == nil { - return 0, errors.New(ctx, errors.InvalidParameter, op, fmt.Sprintf("plugin %q not available", c.GetPluginId())) - } plgHc, err := toPluginCatalog(ctx, c, plg) if err != nil { return 0, errors.Wrap(ctx, err, op) @@ -749,6 +744,12 @@ func (r *Repository) DeleteSet(ctx context.Context, projectId string, publicId s if err != nil { return 0, errors.Wrap(ctx, err, op) } + + plgClient, err := pluginClientFactoryFn(ctx, plgHc, r.plugins) + if err != nil { + return 0, errors.Wrap(ctx, err, op) + } + // Even if the plugin returns an error, we ignore it and proceed with // deleting the set, hence we don't check the error here. This is because we // may get errors from the plugin that we can't do anything about (say, it's diff --git a/internal/host/plugin/repository_test.go b/internal/host/plugin/repository_test.go index 5ca68f738e..fafc404eba 100644 --- a/internal/host/plugin/repository_test.go +++ b/internal/host/plugin/repository_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/hashicorp/boundary/internal/scheduler" + "github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/hostcatalogs" plgpb "github.com/hashicorp/boundary/sdk/pbs/plugin" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -168,3 +169,45 @@ func TestRepository_New(t *testing.T) { }) } } + +func TestPluginClientFactory(t *testing.T) { + t.Run("nilHostCatalog", func(t *testing.T) { + cl, err := pluginClientFactory( + context.Background(), + nil, + map[string]plgpb.HostPluginServiceClient{}, + ) + require.ErrorContains(t, err, "host catalog object not present") + require.Nil(t, cl) + }) + + t.Run("pluginDoesntExist", func(t *testing.T) { + cl, err := pluginClientFactory( + context.Background(), + &hostcatalogs.HostCatalog{PluginId: "not_present"}, + map[string]plgpb.HostPluginServiceClient{}, + ) + require.ErrorContains(t, err, "controller plugin \"not_present\" not available") + require.Nil(t, cl) + }) + + t.Run("pluginNilClient", func(t *testing.T) { + cl, err := pluginClientFactory( + context.Background(), + &hostcatalogs.HostCatalog{PluginId: "present_but_nil"}, + map[string]plgpb.HostPluginServiceClient{"present_but_nil": nil}, + ) + require.ErrorContains(t, err, "controller plugin \"present_but_nil\" not available") + require.Nil(t, cl) + }) + + t.Run("success", func(t *testing.T) { + cl, err := pluginClientFactory( + context.Background(), + &hostcatalogs.HostCatalog{PluginId: "success"}, + map[string]plgpb.HostPluginServiceClient{"success": plgpb.NewHostPluginServiceClient(nil)}, + ) + require.NoError(t, err) + require.NotNil(t, cl) + }) +}