From 4db0decc5cceda74ba9ce27ac2fbf672e89fb9cc Mon Sep 17 00:00:00 2001 From: Matt Romanelli Date: Thu, 24 Sep 2026 17:14:29 -0500 Subject: [PATCH] bug(go) gRPC status codes were getting overwritten by the handlers (#94) * bug(go) gRPC status codes were getting overwritten by the handlers **Problem** * If the gRPC handlers received an error with a gRPC status code from checkHosts, it got overwritten by a plain error value. **Solution** * Added a check to each checkHosts call that passes on the error received (with its gRPC status) **Testing** * Added new unit tests for when the handlers receive an error from checkHosts with a status code. * Existing unit tests and terraform tests passed --- plugin/service/host/plugin.go | 15 +++ plugin/service/host/plugin_test.go | 171 +++++++++++++++++++++++++++++ 2 files changed, 186 insertions(+) diff --git a/plugin/service/host/plugin.go b/plugin/service/host/plugin.go index 4d19768..741f627 100644 --- a/plugin/service/host/plugin.go +++ b/plugin/service/host/plugin.go @@ -115,6 +115,9 @@ func (p *HostPlugin) OnCreateCatalog(ctx context.Context, req *pb.OnCreateCatalo // 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 { + if _, ok := status.FromError(err); ok { + return nil, err + } return nil, fmt.Errorf(errDryRunFailed, err) } @@ -219,6 +222,9 @@ func (p *HostPlugin) OnUpdateCatalog(ctx context.Context, req *pb.OnUpdateCatalo // 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 { + if _, ok := status.FromError(err); ok { + return nil, err + } return nil, fmt.Errorf(errDryRunFailed, err) } @@ -270,6 +276,9 @@ func (p *HostPlugin) OnUpdateCatalog(ctx context.Context, req *pb.OnUpdateCatalo // 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 { + if _, ok := status.FromError(err); ok { + return nil, err + } return nil, fmt.Errorf(errDryRunFailed, err) } @@ -425,6 +434,9 @@ func (p *HostPlugin) OnCreateSet(ctx context.Context, req *pb.OnCreateSetRequest // 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 { + if _, ok := status.FromError(err); ok { + return nil, err + } return nil, fmt.Errorf(errDryRunFailed, err) } @@ -492,6 +504,9 @@ func (p *HostPlugin) OnUpdateSet(ctx context.Context, req *pb.OnUpdateSetRequest // 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 { + if _, ok := status.FromError(err); ok { + return nil, err + } return nil, fmt.Errorf(errDryRunFailed, err) } diff --git a/plugin/service/host/plugin_test.go b/plugin/service/host/plugin_test.go index f8edf3f..a352fc8 100644 --- a/plugin/service/host/plugin_test.go +++ b/plugin/service/host/plugin_test.go @@ -810,6 +810,38 @@ func TestPluginOnCreateCatalogErr(t *testing.T) { expectedErrContains: testDescribeInstancesError, expectedErrCode: codes.Unknown, }, + { + name: "dry run status error preserves gRPC code", + req: &pb.OnCreateCatalogRequest{ + Catalog: &hostcatalogs.HostCatalog{ + Secrets: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + credential.ConstAccessKeyId: structpb.NewStringValue("AKIA_foobar"), + credential.ConstSecretAccessKey: structpb.NewStringValue("bazqux"), + }, + }, + Attrs: &hostcatalogs.HostCatalog_Attributes{ + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + credential.ConstRegion: structpb.NewStringValue("us-west-2"), + credential.ConstDisableCredentialRotation: structpb.NewBoolValue(true), + }, + }, + }, + }, + }, + catalogOpts: []awsCatalogPersistedStateOption{ + withTestEC2APIFunc(newTestMockEC2( + nil, + testMockEC2WithDescribeInstancesError(awserrors.TestAwsError("AccessDenied", "not authorized to perform ec2:DescribeInstances")), + )), + }, + // checkHosts wraps the AWS error via ParseAWSError into a gRPC + // PermissionDenied status. OnCreateCatalog's dry-run error handler + // should preserve that status code, not re-wrap it as Unknown. + expectedErrContains: "invalid credentials", + expectedErrCode: codes.PermissionDenied, + }, { // BUG FIX: role_arn set, no static keys. AssumeRole mock returns // an error. ValidateCreds now eagerly calls Retrieve() which forces @@ -1327,6 +1359,48 @@ func TestPluginOnUpdateCatalogErr(t *testing.T) { expectedErrContains: "oops there was an error", expectedErrCode: codes.Unknown, }, + { + name: "final dry run gRPC status error preserves code", + req: &pb.OnUpdateCatalogRequest{ + CurrentCatalog: &hostcatalogs.HostCatalog{ + Attrs: &hostcatalogs.HostCatalog_Attributes{ + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + credential.ConstRegion: structpb.NewStringValue("us-west-2"), + }, + }, + }, + }, + NewCatalog: &hostcatalogs.HostCatalog{ + Attrs: &hostcatalogs.HostCatalog_Attributes{ + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + credential.ConstRegion: structpb.NewStringValue("us-west-2"), + }, + }, + }, + }, + Persisted: &pb.HostCatalogPersisted{ + Secrets: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + credential.ConstAccessKeyId: structpb.NewStringValue("AKIA_foobar"), + credential.ConstSecretAccessKey: structpb.NewStringValue("bazqux"), + credential.ConstCredsLastRotatedTime: structpb.NewStringValue("2006-01-02T15:04:05+07:00"), + }, + }, + }, + }, + catalogOpts: []awsCatalogPersistedStateOption{ + withTestEC2APIFunc(newTestMockEC2( + nil, + testMockEC2WithDescribeInstancesError(awserrors.TestAwsError("AccessDenied", "not authorized to perform ec2:DescribeInstances")), + )), + }, + // ParseAWSError maps AccessDenied to PermissionDenied. The dry-run + // error handler should preserve that gRPC status, not wrap it as Unknown. + expectedErrContains: "invalid credentials", + expectedErrCode: codes.PermissionDenied, + }, } for _, tc := range cases { @@ -1681,6 +1755,55 @@ func TestPluginOnCreateSetErr(t *testing.T) { expectedErrContains: testDescribeInstancesError, expectedErrCode: codes.Unknown, }, + { + name: "dry run gRPC status error preserves code", + req: &pb.OnCreateSetRequest{ + Catalog: &hostcatalogs.HostCatalog{ + Attrs: &hostcatalogs.HostCatalog_Attributes{ + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + credential.ConstRegion: structpb.NewStringValue("us-west-2"), + credential.ConstDisableCredentialRotation: structpb.NewBoolValue(true), + }, + }, + }, + }, + Persisted: &pb.HostCatalogPersisted{ + Secrets: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + credential.ConstAccessKeyId: structpb.NewStringValue("AKIA_foobar"), + credential.ConstSecretAccessKey: structpb.NewStringValue("bazqux"), + }, + }, + }, + Set: &hostsets.HostSet{ + Id: "foobar", + Attrs: &hostsets.HostSet_Attributes{ + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstDescribeInstancesFilters: structpb.NewListValue( + &structpb.ListValue{ + Values: []*structpb.Value{ + structpb.NewStringValue("tag-key=foo"), + }, + }, + ), + }, + }, + }, + }, + }, + catalogOpts: []awsCatalogPersistedStateOption{ + withTestEC2APIFunc(newTestMockEC2( + nil, + testMockEC2WithDescribeInstancesError(awserrors.TestAwsError("AccessDenied", "not authorized to perform ec2:DescribeInstances")), + )), + }, + // ParseAWSError maps AccessDenied to PermissionDenied. The dry-run + // error handler should preserve that gRPC status, not wrap it as Unknown. + expectedErrContains: "invalid credentials", + expectedErrCode: codes.PermissionDenied, + }, } for _, tc := range cases { @@ -1944,6 +2067,54 @@ func TestPluginOnUpdateSetErr(t *testing.T) { expectedErrContains: testDescribeInstancesError, expectedErrCode: codes.Unknown, }, + { + name: "dry run gRPC status error preserves code", + req: &pb.OnUpdateSetRequest{ + Catalog: &hostcatalogs.HostCatalog{ + Attrs: &hostcatalogs.HostCatalog_Attributes{ + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + credential.ConstRegion: structpb.NewStringValue("us-west-2"), + credential.ConstDisableCredentialRotation: structpb.NewBoolValue(true), + }, + }, + }, + }, + Persisted: &pb.HostCatalogPersisted{ + Secrets: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + credential.ConstAccessKeyId: structpb.NewStringValue("AKIA_foobar"), + credential.ConstSecretAccessKey: structpb.NewStringValue("bazqux"), + }, + }, + }, + NewSet: &hostsets.HostSet{ + Attrs: &hostsets.HostSet_Attributes{ + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + ConstDescribeInstancesFilters: structpb.NewListValue( + &structpb.ListValue{ + Values: []*structpb.Value{ + structpb.NewStringValue("tag-key=foo"), + }, + }, + ), + }, + }, + }, + }, + }, + catalogOpts: []awsCatalogPersistedStateOption{ + withTestEC2APIFunc(newTestMockEC2( + nil, + testMockEC2WithDescribeInstancesError(awserrors.TestAwsError("AccessDenied", "not authorized to perform ec2:DescribeInstances")), + )), + }, + // ParseAWSError maps AccessDenied to PermissionDenied. The dry-run + // error handler should preserve that gRPC status, not wrap it as Unknown. + expectedErrContains: "invalid credentials", + expectedErrCode: codes.PermissionDenied, + }, } for _, tc := range cases {