From 77484a3fc9a8c7250dd141319dd29010d4d39ffd Mon Sep 17 00:00:00 2001 From: rebecca-yoh Date: Wed, 23 Sep 2026 13:42:24 -0700 Subject: [PATCH] feat(host): replace dryRunValidation with checkHosts in catalog and set handlers (ICU-19979) (#87) * temporary commit * feat(host): replace dryRunValidation with checkHosts in catalog and set handlers * refactor(host): clean up comments and fix typos in dry-run validation blocks * refactor(host): address PR feedback on dry-run validation blocks * refactor(host): remove dead dryRunValidation function and its tests * refactor(host): extract dry run error message as a constant * refactor(host): propagate BadRequestStatusf from buildFilters through buildDescribeInstancesInput --------- Co-authored-by: ryanflynn9299 --- plugin/service/host/plugin.go | 96 +++++++++++++++--------------- plugin/service/host/plugin_test.go | 87 ++++++++------------------- 2 files changed, 72 insertions(+), 111 deletions(-) diff --git a/plugin/service/host/plugin.go b/plugin/service/host/plugin.go index 51b3f2e..64d2afa 100644 --- a/plugin/service/host/plugin.go +++ b/plugin/service/host/plugin.go @@ -46,6 +46,8 @@ type hostSetQuery struct { const defaultSessionName = "boundary-default" const dryRunOperationErrorCode = "DryRunOperation" +const errBuildDescribeInstancesInput = "error building DescribeInstances input: %s" +const errDryRunFailed = "error performing DescribeInstances dry run: %s" // Ensure that we are implementing HostPluginServiceServer var _ pb.HostPluginServiceServer = (*HostPlugin)(nil) @@ -97,13 +99,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)) } - if st := dryRunValidation(ctx, catalogState, opts); st != nil { - return nil, st.Err() + input, err := buildDescribeInstancesInput(&SetAttributes{}, true) + if err != nil { + return nil, status.Errorf(codes.Unknown, 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, catalogAttributes.TargetAccount); err != nil { + return nil, fmt.Errorf(errDryRunFailed, err) } persistedProto, err := catalogState.toProto() @@ -186,12 +193,19 @@ func (p *HostPlugin) OnUpdateCatalog(ctx context.Context, req *pb.OnUpdateCatalo if err != nil { return nil, errors.BadRequestStatusf("error loading persisted state: %s", err) } + opts := []ec2Option{} if newCatalogAttributes.DualStack { opts = append(opts, WithDualStack(newCatalogAttributes.DualStack)) } - if st := dryRunValidation(ctx, newCatalogState, opts); st != nil { - return nil, st.Err() + input, err := buildDescribeInstancesInput(&SetAttributes{}, true) + if err != nil { + return nil, status.Errorf(codes.Unknown, 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, newCatalogAttributes.TargetAccount); err != nil { + return nil, fmt.Errorf(errDryRunFailed, err) } // Replace the existing credential state. @@ -231,13 +245,18 @@ func (p *HostPlugin) OnUpdateCatalog(ctx context.Context, req *pb.OnUpdateCatalo return nil, errors.BadRequestStatusf("error loading persisted state: %s", err) } - // perform dry run to ensure we can interact with AWS as expected. opts := []ec2Option{} if newCatalogAttributes.DualStack { opts = append(opts, WithDualStack(newCatalogAttributes.DualStack)) } - if st := dryRunValidation(ctx, catalogState, opts); st != nil { - return nil, st.Err() + input, err := buildDescribeInstancesInput(&SetAttributes{}, true) + if err != nil { + return nil, status.Errorf(codes.Unknown, 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, newCatalogAttributes.TargetAccount); err != nil { + return nil, fmt.Errorf(errDryRunFailed, err) } persistedProto, err := catalogState.toProto() @@ -381,18 +400,20 @@ func (p *HostPlugin) OnCreateSet(ctx context.Context, req *pb.OnCreateSetRequest return nil, err } - describeInstanceFilters, err := buildFilters(setAttrs) - if err != nil { - return nil, errors.BadRequestStatusf("error building set filters: %s", err) - } - opts := []ec2Option{} if catalogAttributes.DualStack { opts = append(opts, WithDualStack(catalogAttributes.DualStack)) } - if st := dryRunValidation(ctx, catalogState, opts, describeInstanceFilters...); st != nil { - return nil, st.Err() + input, err := buildDescribeInstancesInput(setAttrs, true) + if err != nil { + return nil, status.Errorf(codes.Unknown, 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, catalogAttributes.TargetAccount); err != nil { + return nil, fmt.Errorf(errDryRunFailed, err) + } + return &pb.OnCreateSetResponse{}, nil } @@ -446,17 +467,18 @@ func (p *HostPlugin) OnUpdateSet(ctx context.Context, req *pb.OnUpdateSetRequest return nil, err } - describeInstanceFilters, err := buildFilters(setAttrs) - if err != nil { - return nil, errors.BadRequestStatusf("error building set filters: %s", err) - } - opts := []ec2Option{} if catalogAttributes.DualStack { opts = append(opts, WithDualStack(catalogAttributes.DualStack)) } - if st := dryRunValidation(ctx, catalogState, opts, describeInstanceFilters...); st != nil { - return nil, st.Err() + input, err := buildDescribeInstancesInput(setAttrs, true) + if err != nil { + return nil, status.Errorf(codes.Unknown, 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, catalogAttributes.TargetAccount); err != nil { + return nil, fmt.Errorf(errDryRunFailed, err) } return &pb.OnUpdateSetResponse{}, nil @@ -611,11 +633,11 @@ func buildFilters(attrs *SetAttributes) ([]types.Filter, error) { splitFilter := strings.Split(filterAttr, "=") switch { case len(splitFilter) != 2: - return nil, fmt.Errorf("expected filter %q to contain a single equal sign", filterAttr) + return nil, errors.BadRequestStatusf("expected filter %q to contain a single equal sign", filterAttr) case len(splitFilter[0]) == 0: - return nil, fmt.Errorf("filter %q contains an empty filter key", filterAttr) + return nil, errors.BadRequestStatusf("filter %q contains an empty filter key", filterAttr) case len(splitFilter[1]) == 0: - return nil, fmt.Errorf("filter %q contains an empty value", filterAttr) + return nil, errors.BadRequestStatusf("filter %q contains an empty value", filterAttr) } filterKey, filterValue := splitFilter[0], splitFilter[1] @@ -650,7 +672,7 @@ func buildDescribeInstancesInput(attrs *SetAttributes, dryRun bool) (*ec2.Descri filters, err := buildFilters(attrs) if err != nil { - return nil, fmt.Errorf("error building filters: %w", err) + return nil, err } return &ec2.DescribeInstancesInput{ @@ -832,28 +854,6 @@ func ec2ClientForTarget( return ec2.NewFromConfig(targetCfg, ec2Opts...), nil } -// dryRunValidation performs an AWS DescribeInstances call to verify the state's -// credentials, the host listing functionality as well as the filters, if any -// are passed in. This function can therefore be used for both host catalog and -// host set validation. -func dryRunValidation(ctx context.Context, state *awsCatalogPersistedState, ec2Opts []ec2Option, filters ...types.Filter) *status.Status { - if state == nil { - return status.New(codes.InvalidArgument, "persisted state is required") - } - - ec2Client, err := state.EC2Client(ctx, ec2Opts...) - if err != nil { - return status.New(codes.InvalidArgument, fmt.Sprintf("error getting EC2 client: %s", err)) - } - - _, err = ec2Client.DescribeInstances(ctx, &ec2.DescribeInstancesInput{Filters: filters}) - if err != nil { - return status.New(codes.FailedPrecondition, fmt.Sprintf("aws describe instances failed: %s", err)) - } - - return nil -} - // appendDistinct will append the elements to the slice // if an element is not nil, empty, and does not exist in slice. func appendDistinct(slice []string, elems ...*string) []string { diff --git a/plugin/service/host/plugin_test.go b/plugin/service/host/plugin_test.go index 80d342f..060d5c9 100644 --- a/plugin/service/host/plugin_test.go +++ b/plugin/service/host/plugin_test.go @@ -742,8 +742,11 @@ func TestPluginOnCreateCatalogErr(t *testing.T) { testMockEC2WithDescribeInstancesError(errors.New(testDescribeInstancesError)), )), }, - expectedErrContains: "aws describe instances failed: DescribeInstances error", - expectedErrCode: codes.FailedPrecondition, + // 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, + expectedErrCode: codes.Unknown, }, } @@ -994,8 +997,11 @@ func TestPluginOnUpdateCatalogErr(t *testing.T) { testMockEC2WithDescribeInstancesError(errors.New(testDescribeInstancesError)), )), }, - expectedErrContains: "aws describe instances failed: DescribeInstances error", - expectedErrCode: codes.FailedPrecondition, + //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, + expectedErrCode: codes.Unknown, }, { name: "new incoming dynamic credential update dry run error", @@ -1036,8 +1042,11 @@ func TestPluginOnUpdateCatalogErr(t *testing.T) { testMockEC2WithDescribeInstancesError(errors.New(testDescribeInstancesError)), )), }, - expectedErrContains: "aws describe instances failed: DescribeInstances error", - expectedErrCode: codes.FailedPrecondition, + // 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, + expectedErrCode: codes.Unknown, }, { name: "replace creds error", @@ -1211,8 +1220,11 @@ func TestPluginOnUpdateCatalogErr(t *testing.T) { testMockEC2WithDescribeInstancesError(fmt.Errorf("oops there was an error")), )), }, - expectedErrContains: "aws describe instances failed: oops there was an error", - expectedErrCode: codes.FailedPrecondition, + // 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", + expectedErrCode: codes.Unknown, }, } @@ -1565,8 +1577,8 @@ func TestPluginOnCreateSetErr(t *testing.T) { testMockEC2WithDescribeInstancesError(errors.New(testDescribeInstancesError)), )), }, - expectedErrContains: fmt.Sprintf("aws describe instances failed: %s", testDescribeInstancesError), - expectedErrCode: codes.FailedPrecondition, + expectedErrContains: testDescribeInstancesError, + expectedErrCode: codes.Unknown, }, } @@ -1828,8 +1840,8 @@ func TestPluginOnUpdateSetErr(t *testing.T) { testMockEC2WithDescribeInstancesError(errors.New(testDescribeInstancesError)), )), }, - expectedErrContains: fmt.Sprintf("aws describe instances failed: %s", testDescribeInstancesError), - expectedErrCode: codes.FailedPrecondition, + expectedErrContains: testDescribeInstancesError, + expectedErrCode: codes.Unknown, }, } @@ -3086,54 +3098,3 @@ func TestAppendDistinct(t *testing.T) { }) } } - -func TestDryRunValidation(t *testing.T) { - t.Run("nil credential state", func(t *testing.T) { - st := dryRunValidation(context.Background(), nil, nil) - require.NotNil(t, st) - require.Equal(t, codes.InvalidArgument.String(), st.Code().String()) - require.Equal(t, "persisted state is required", st.Message()) - }) - - t.Run("ec2ClientErr", func(t *testing.T) { - st := dryRunValidation(context.Background(), &awsCatalogPersistedState{ - AwsCredentialPersistedState: &credential.AwsCredentialPersistedState{ - CredentialsConfig: &awsutil.CredentialsConfig{}, - }, - testEC2APIFunc: func(...aws.Config) (EC2API, error) { - return nil, fmt.Errorf("oops ec2 client err") - }, - }, []ec2Option{}) - require.NotNil(t, st) - require.Equal(t, codes.InvalidArgument.String(), st.Code().String()) - require.Equal(t, "error getting EC2 client: oops ec2 client err", st.Message()) - }) - - t.Run("describeInstancesErr", func(t *testing.T) { - st := dryRunValidation(context.Background(), &awsCatalogPersistedState{ - AwsCredentialPersistedState: &credential.AwsCredentialPersistedState{ - CredentialsConfig: &awsutil.CredentialsConfig{ - AccessKey: "AKIAfoo", - SecretKey: "baz", - }, - }, - testEC2APIFunc: newTestMockEC2(nil, testMockEC2WithDescribeInstancesError(fmt.Errorf("oops describe instances error"))), - }, []ec2Option{}) - require.NotNil(t, st) - require.Equal(t, codes.FailedPrecondition.String(), st.Code().String()) - require.Equal(t, "aws describe instances failed: oops describe instances error", st.Message()) - }) - - t.Run("success", func(t *testing.T) { - st := dryRunValidation(context.Background(), &awsCatalogPersistedState{ - AwsCredentialPersistedState: &credential.AwsCredentialPersistedState{ - CredentialsConfig: &awsutil.CredentialsConfig{ - AccessKey: "AKIAfoo", - SecretKey: "baz", - }, - }, - testEC2APIFunc: newTestMockEC2(nil, testMockEC2WithDescribeInstancesOutput(&ec2.DescribeInstancesOutput{})), - }, []ec2Option{}) - require.Nil(t, st) - }) -}