From 12a7953a3112c0478a3f5178bf16b5650910e14d Mon Sep 17 00:00:00 2001 From: Albert Lloveras Date: Fri, 16 Aug 2024 13:51:58 +1000 Subject: [PATCH] PR Feedback: Re-work error reporting and tests --- pkg/networking/security_group_resolver.go | 30 ++++----- .../security_group_resolver_test.go | 63 ++++++++++++------- 2 files changed, 56 insertions(+), 37 deletions(-) diff --git a/pkg/networking/security_group_resolver.go b/pkg/networking/security_group_resolver.go index 024348040..3b807729d 100644 --- a/pkg/networking/security_group_resolver.go +++ b/pkg/networking/security_group_resolver.go @@ -34,23 +34,31 @@ type defaultSecurityGroupResolver struct { } func (r *defaultSecurityGroupResolver) ResolveViaNameOrID(ctx context.Context, sgNameOrIDs []string) ([]string, error) { - sgIDs, sgNames := r.splitIntoSgNameAndIDs(sgNameOrIDs) var resolvedSGs []*ec2sdk.SecurityGroup + var errMessages []string + + sgIDs, sgNames := r.splitIntoSgNameAndIDs(sgNameOrIDs) if len(sgIDs) > 0 { sgs, err := r.resolveViaGroupID(ctx, sgIDs) if err != nil { - return nil, err + errMessages = append(errMessages, err.Error()) + } else { + resolvedSGs = append(resolvedSGs, sgs...) } - resolvedSGs = append(resolvedSGs, sgs...) } if len(sgNames) > 0 { sgs, err := r.resolveViaGroupName(ctx, sgNames) if err != nil { - return nil, err + errMessages = append(errMessages, err.Error()) + } else { + resolvedSGs = append(resolvedSGs, sgs...) } - resolvedSGs = append(resolvedSGs, sgs...) + } + + if len(errMessages) > 0 { + return nil, errors.Errorf("couldn't find all security groups: %s", strings.Join(errMessages, ", ")) } resolvedSGIDs := make([]string, 0, len(resolvedSGs)) @@ -77,11 +85,7 @@ func (r *defaultSecurityGroupResolver) resolveViaGroupID(ctx context.Context, sg } if len(sgIDs) != len(resolvedSGIDs) { - return nil, errors.Errorf( - "couldn't find all securityGroups, requested ids: [%s], found: [%s]", - strings.Join(sgIDs, ", "), - strings.Join(resolvedSGIDs, ", "), - ) + return nil, errors.Errorf("requested ids [%s] but found [%s]", strings.Join(sgIDs, ", "), strings.Join(resolvedSGIDs, ", ")) } return sgs, nil @@ -120,11 +124,7 @@ func (r *defaultSecurityGroupResolver) resolveViaGroupName(ctx context.Context, resolvedSGNames = algorithm.RemoveSliceDuplicates(resolvedSGNames) if len(sgNames) != len(resolvedSGNames) { - return nil, errors.Errorf( - "couldn't find all securityGroups, requested names: [%s], found: [%s]", - strings.Join(sgNames, ", "), - strings.Join(resolvedSGNames, ", "), - ) + return nil, errors.Errorf("requested names [%s] but found [%s]", strings.Join(sgNames, ", "), strings.Join(resolvedSGNames, ", ")) } return sgs, nil diff --git a/pkg/networking/security_group_resolver_test.go b/pkg/networking/security_group_resolver_test.go index 2bd9dcd63..bd2c663c5 100644 --- a/pkg/networking/security_group_resolver_test.go +++ b/pkg/networking/security_group_resolver_test.go @@ -204,7 +204,6 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) { name: "describe by id returns error", args: args{ nameOrIDs: []string{ - "sg group name", "sg-id", }, describeSGCalls: []describeSecurityGroupsAsListCall{ @@ -216,24 +215,21 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) { }, }, }, - wantErr: errors.New("Describe.Error: unable to describe security groups"), + wantErr: errors.New("couldn't find all security groups: Describe.Error: unable to describe security groups"), }, { name: "describe by name returns error", args: args{ nameOrIDs: []string{ "sg group name", - "sg-id", }, describeSGCalls: []describeSecurityGroupsAsListCall{ { req: &ec2sdk.DescribeSecurityGroupsInput{ Filters: []*ec2sdk.Filter{ { - Name: awssdk.String("tag:Name"), - Values: awssdk.StringSlice([]string{ - "sg group name", - }), + Name: awssdk.String("tag:Name"), + Values: awssdk.StringSlice([]string{"sg group name"}), }, { Name: awssdk.String("vpc-id"), @@ -243,22 +239,12 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) { }, err: awserr.New("Describe.Error", "unable to describe security groups", nil), }, - { - req: &ec2sdk.DescribeSecurityGroupsInput{ - GroupIds: awssdk.StringSlice([]string{"sg-id"}), - }, - resp: []*ec2sdk.SecurityGroup{ - { - GroupId: awssdk.String("sg-id"), - }, - }, - }, }, }, - wantErr: errors.New("Describe.Error: unable to describe security groups"), + wantErr: errors.New("couldn't find all security groups: Describe.Error: unable to describe security groups"), }, { - name: "unable to resolve all security group ids", + name: "unable to resolve security groups by id", args: args{ nameOrIDs: []string{ "sg-id1", @@ -277,10 +263,10 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) { }, }, }, - wantErr: errors.New("couldn't find all securityGroups, requested ids: [sg-id1, sg-id404], found: [sg-id1]"), + wantErr: errors.New("couldn't find all security groups: requested ids [sg-id1, sg-id404] but found [sg-id1]"), }, { - name: "unable to resolve all security groups names", + name: "unable to resolve security groups by name", args: args{ nameOrIDs: []string{ "sg group one", @@ -314,7 +300,40 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) { }, }, }, - wantErr: errors.New("couldn't find all securityGroups, requested names: [sg group one, sg group two], found: [sg group one]"), + wantErr: errors.New("couldn't find all security groups: requested names [sg group one, sg group two] but found [sg group one]"), + }, + { + name: "unable to resolve all security groups by ids and names", + args: args{ + nameOrIDs: []string{ + "sg-08982de7", + "sg group one", + }, + describeSGCalls: []describeSecurityGroupsAsListCall{ + { + req: &ec2sdk.DescribeSecurityGroupsInput{ + GroupIds: awssdk.StringSlice([]string{"sg-08982de7"}), + }, + resp: []*ec2sdk.SecurityGroup{}, + }, + { + req: &ec2sdk.DescribeSecurityGroupsInput{ + Filters: []*ec2sdk.Filter{ + { + Name: awssdk.String("tag:Name"), + Values: awssdk.StringSlice([]string{"sg group one"}), + }, + { + Name: awssdk.String("vpc-id"), + Values: awssdk.StringSlice([]string{defaultVPCID}), + }, + }, + }, + resp: []*ec2sdk.SecurityGroup{}, + }, + }, + }, + wantErr: errors.New("couldn't find all security groups: requested ids [sg-08982de7] but found [], requested names [sg group one] but found []"), }, }