From 8d5eddaaa9cd1a02ba224636101617ff5e720a88 Mon Sep 17 00:00:00 2001 From: Rebecca Date: Mon, 21 Sep 2026 13:27:47 -0700 Subject: [PATCH] refactor(host): address PR feedback on dry-run validation blocks --- plugin/service/host/plugin.go | 36 +++++++++++++++++------------- plugin/service/host/plugin_test.go | 8 +++---- 2 files changed, 25 insertions(+), 19 deletions(-) diff --git a/plugin/service/host/plugin.go b/plugin/service/host/plugin.go index 9caf819..db16211 100644 --- a/plugin/service/host/plugin.go +++ b/plugin/service/host/plugin.go @@ -46,6 +46,7 @@ type hostSetQuery struct { const defaultSessionName = "boundary-default" const dryRunOperationErrorCode = "DryRunOperation" +const errBuildDescribeInstancesInput = "error building DescribeInstances input: %s" // Ensure that we are implementing HostPluginServiceServer var _ pb.HostPluginServiceServer = (*HostPlugin)(nil) @@ -97,17 +98,18 @@ func (p *HostPlugin) OnCreateCatalog(ctx context.Context, req *pb.OnCreateCatalo return nil, errors.BadRequestStatusf("error setting up persisted state: %s", err) } - // perform dry run to ensure we can interact with AWS as expected. opts := []ec2Option{} if catalogAttributes.DualStack { opts = append(opts, WithDualStack(catalogAttributes.DualStack)) } input, err := buildDescribeInstancesInput(&SetAttributes{}, true) if err != nil { - return nil, errors.BadRequestStatusf("error building DescribeInstances input: %s", err) + return nil, fmt.Errorf(errBuildDescribeInstancesInput, err) } + // Run a DescribeInstances request with DryRun set to true to ensure + // we can interact with AWS resources as expected. if _, err := checkHosts(ctx, catalogState, input, opts, nil); err != nil { - return nil, err + return nil, fmt.Errorf("error performing DescribeInstances dry run: %s", err) } persistedProto, err := catalogState.toProto() @@ -191,17 +193,18 @@ func (p *HostPlugin) OnUpdateCatalog(ctx context.Context, req *pb.OnUpdateCatalo return nil, errors.BadRequestStatusf("error loading persisted state: %s", err) } - // New: perform dry run to ensure we can interact with AWS as expected. opts := []ec2Option{} if newCatalogAttributes.DualStack { opts = append(opts, WithDualStack(newCatalogAttributes.DualStack)) } input, err := buildDescribeInstancesInput(&SetAttributes{}, true) if err != nil { - return nil, errors.BadRequestStatusf("error building DescribeInstances input: %s", err) + return nil, fmt.Errorf(errBuildDescribeInstancesInput, err) } + // Run a DescribeInstances request with DryRun set to true to ensure + // we can interact with AWS resources as expected. if _, err := checkHosts(ctx, newCatalogState, input, opts, nil); err != nil { - return nil, err + return nil, fmt.Errorf("error performing DescribeInstances dry run: %s", err) } // Replace the existing credential state. @@ -241,17 +244,18 @@ func (p *HostPlugin) OnUpdateCatalog(ctx context.Context, req *pb.OnUpdateCatalo return nil, errors.BadRequestStatusf("error loading persisted state: %s", err) } - // New: perform dry run to ensure we can interact with AWS as expected. opts := []ec2Option{} if newCatalogAttributes.DualStack { opts = append(opts, WithDualStack(newCatalogAttributes.DualStack)) } input, err := buildDescribeInstancesInput(&SetAttributes{}, true) if err != nil { - return nil, errors.BadRequestStatusf("error building DescribeInstances input: %s", err) + return nil, fmt.Errorf(errBuildDescribeInstancesInput, err) } + // Run a DescribeInstances request with DryRun set to true to ensure + // we can interact with AWS resources as expected. if _, err := checkHosts(ctx, catalogState, input, opts, nil); err != nil { - return nil, err + return nil, fmt.Errorf("error performing DescribeInstances dry run: %s", err) } persistedProto, err := catalogState.toProto() @@ -395,17 +399,18 @@ func (p *HostPlugin) OnCreateSet(ctx context.Context, req *pb.OnCreateSetRequest return nil, err } - // New: perform dry run to ensure we can interact with AWS as expected. opts := []ec2Option{} if catalogAttributes.DualStack { opts = append(opts, WithDualStack(catalogAttributes.DualStack)) } input, err := buildDescribeInstancesInput(setAttrs, true) if err != nil { - return nil, errors.BadRequestStatusf("error building DescribeInstances input: %s", err) + return nil, fmt.Errorf(errBuildDescribeInstancesInput, err) } + // Run a DescribeInstances request with DryRun set to true to ensure + // we can interact with AWS resources as expected. if _, err := checkHosts(ctx, catalogState, input, opts, nil); err != nil { - return nil, err + return nil, fmt.Errorf("error performing DescribeInstances dry run: %s", err) } return &pb.OnCreateSetResponse{}, nil @@ -461,17 +466,18 @@ func (p *HostPlugin) OnUpdateSet(ctx context.Context, req *pb.OnUpdateSetRequest return nil, err } - // New: perform dry run to ensure we can interact with AWS as expected. opts := []ec2Option{} if catalogAttributes.DualStack { opts = append(opts, WithDualStack(catalogAttributes.DualStack)) } input, err := buildDescribeInstancesInput(setAttrs, true) if err != nil { - return nil, errors.BadRequestStatusf("error building DescribeInstances input: %s", err) + return nil, fmt.Errorf(errBuildDescribeInstancesInput, err) } + // Run a DescribeInstances request with DryRun set to true to ensure + // we can interact with AWS resources as expected. if _, err := checkHosts(ctx, catalogState, input, opts, nil); err != nil { - return nil, err + return nil, fmt.Errorf("error performing DescribeInstances dry run: %s", err) } return &pb.OnUpdateSetResponse{}, nil diff --git a/plugin/service/host/plugin_test.go b/plugin/service/host/plugin_test.go index 19a8299..48582f4 100644 --- a/plugin/service/host/plugin_test.go +++ b/plugin/service/host/plugin_test.go @@ -742,7 +742,7 @@ func TestPluginOnCreateCatalogErr(t *testing.T) { testMockEC2WithDescribeInstancesError(errors.New(testDescribeInstancesError)), )), }, - // New: checkHosts returns the DescribeInstances error as-is rather than + // checkHosts returns the DescribeInstances error as-is rather than // wrapping it in a gRPC FailedPrecondition status like the old // dryRunValidation did. A plain Go error maps to codes.Unknown. expectedErrContains: testDescribeInstancesError, @@ -997,7 +997,7 @@ func TestPluginOnUpdateCatalogErr(t *testing.T) { testMockEC2WithDescribeInstancesError(errors.New(testDescribeInstancesError)), )), }, - // New: checkHosts returns the DesribeInstances errror as-is rather than wrapping it + //checkHosts returns the DesribeInstances errror as-is rather than wrapping it // in a gRpc FailedPrecodndition status like the old dryRunValidiation did. // A plain Go error maps to codeds.Unknown. expectedErrContains: testDescribeInstancesError, @@ -1042,7 +1042,7 @@ func TestPluginOnUpdateCatalogErr(t *testing.T) { testMockEC2WithDescribeInstancesError(errors.New(testDescribeInstancesError)), )), }, - // New: checkHosts returns the DescribeInstances error as-is rather than wrapping it + // checkHosts returns the DescribeInstances error as-is rather than wrapping it // in a gRPC FailedPrecondition status like the old dryRunValidiation did. // A plain Go error maps to codes.Unknown. expectedErrContains: testDescribeInstancesError, @@ -1220,7 +1220,7 @@ func TestPluginOnUpdateCatalogErr(t *testing.T) { testMockEC2WithDescribeInstancesError(fmt.Errorf("oops there was an error")), )), }, - // New: checkHosts returns the DescribeInstances error as-is rather than wrapping it + // checkHosts returns the DescribeInstances error as-is rather than wrapping it // in a gRPC FailedPrecondition status like the old dryRunValidiation did. // A plain Go error maps to codes.Unknown. expectedErrContains: "oops there was an error",