Skip to content

fixup(sg-resolver): Allow multiple SGs with the same Name tag #3775

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 16, 2024
Merged

fixup(sg-resolver): Allow multiple SGs with the same Name tag #3775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41f7943
fixup(sg-resolver): Allow multiple SGs with the same Name tag
alloveras Jul 22, 2024
12a7953
PR Feedback: Re-work error reporting and tests
alloveras Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions pkg/algorithm/slices.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package algorithm

import "cmp"

// RemoveSliceDuplicates returns a copy of the slice without duplicate entries.
func RemoveSliceDuplicates[S ~[]E, E cmp.Ordered](s S) []E {
result := make([]E, 0, len(s))
found := make(map[E]struct{}, len(s))

for _, x := range s {
if _, ok := found[x]; !ok {
found[x] = struct{}{}
result = append(result, x)
}
}

return result
}
46 changes: 46 additions & 0 deletions pkg/algorithm/slices_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package algorithm

import (
"testing"

"github.com/stretchr/testify/assert"
)

func Test_RemoveSliceDuplicates(t *testing.T) {
type args struct {
data []string
}
tests := []struct {
name string
args args
want []string
}{
{
name: "empty",
args: args{
data: []string{},
},
want: []string{},
},
{
name: "no duplicate entries",
args: args{
data: []string{"a", "b", "c", "d"},
},
want: []string{"a", "b", "c", "d"},
},
{
name: "with duplicates",
args: args{
data: []string{"a", "b", "a", "c", "b"},
},
want: []string{"a", "b", "c"},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := RemoveSliceDuplicates(tt.args.data)
assert.Equal(t, tt.want, got)
})
}
}
56 changes: 48 additions & 8 deletions pkg/networking/security_group_resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
awssdk "github.com/aws/aws-sdk-go/aws"
ec2sdk "github.com/aws/aws-sdk-go/service/ec2"
"github.com/pkg/errors"
"sigs.k8s.io/aws-load-balancer-controller/pkg/algorithm"
"sigs.k8s.io/aws-load-balancer-controller/pkg/aws/services"
)

Expand All @@ -33,44 +34,66 @@ 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))
for _, sg := range resolvedSGs {
resolvedSGIDs = append(resolvedSGIDs, awssdk.StringValue(sg.GroupId))
}
if len(resolvedSGIDs) != len(sgNameOrIDs) {
return nil, errors.Errorf("couldn't find all securityGroups, nameOrIDs: %v, found: %v", sgNameOrIDs, resolvedSGIDs)
}

return resolvedSGIDs, nil
}

func (r *defaultSecurityGroupResolver) resolveViaGroupID(ctx context.Context, sgIDs []string) ([]*ec2sdk.SecurityGroup, error) {
req := &ec2sdk.DescribeSecurityGroupsInput{
GroupIds: awssdk.StringSlice(sgIDs),
}

sgs, err := r.ec2Client.DescribeSecurityGroupsAsList(ctx, req)
if err != nil {
return nil, err
}

resolvedSGIDs := make([]string, 0, len(sgs))
for _, sg := range sgs {
resolvedSGIDs = append(resolvedSGIDs, awssdk.StringValue(sg.GroupId))
}

if len(sgIDs) != len(resolvedSGIDs) {
return nil, errors.Errorf("requested ids [%s] but found [%s]", strings.Join(sgIDs, ", "), strings.Join(resolvedSGIDs, ", "))
}

return sgs, nil
}

func (r *defaultSecurityGroupResolver) resolveViaGroupName(ctx context.Context, sgNames []string) ([]*ec2sdk.SecurityGroup, error) {
sgNames = algorithm.RemoveSliceDuplicates(sgNames)

req := &ec2sdk.DescribeSecurityGroupsInput{
Filters: []*ec2sdk.Filter{
{
Expand All @@ -83,10 +106,27 @@ func (r *defaultSecurityGroupResolver) resolveViaGroupName(ctx context.Context,
},
},
}

sgs, err := r.ec2Client.DescribeSecurityGroupsAsList(ctx, req)
if err != nil {
return nil, err
}

resolvedSGNames := make([]string, 0, len(sgs))
for _, sg := range sgs {
Copy link
Collaborator

@oliviassss oliviassss Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to add nil check for the sgs and sg.Tags?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the Golang specification tells us that these would be unnecessarily redundant.

A "for" statement with a "range" clause iterates through all entries of an array, slice, string or map, values received on a channel, or integer values from zero to an upper limit [Go 1.22]. For each entry it assigns iteration values to corresponding iteration variables if present and then executes the block.
...
For an array, pointer to array, or slice value a, the index iteration [...] For a nil slice, the number of iterations is 0.

Source: https://go.dev/ref/spec#For_range

Example: https://go.dev/play/p/vBe3S9lbhvh

That said, if you feel strongly about having them or is a common pattern in the codebase I am more than happy to add an extra commit with the additional nil checks 😉

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah for the sgs it was fine, but how about sg.Tags? I know in this function we are filtering by tags, so the returned sgs should have Tags field, but just in case if some code changes in the future and we miss this, it will error out because the controller does not check whether there's a Tag spec.

Copy link
Contributor Author

@alloveras alloveras Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but how about sg.Tags?

I think for sg.Tags the reasoning is the the same 🤔? Also, if the listing code changes and the returned SecurityGroup struct removes/renames the Tags field, that will produce a compiler error and, hence, we will detect it immediately. Am I missing something?

for _, tag := range sg.Tags {
if awssdk.StringValue(tag.Key) == "Name" {
resolvedSGNames = append(resolvedSGNames, awssdk.StringValue(tag.Value))
}
}
}

resolvedSGNames = algorithm.RemoveSliceDuplicates(resolvedSGNames)

if len(sgNames) != len(resolvedSGNames) {
return nil, errors.Errorf("requested names [%s] but found [%s]", strings.Join(sgNames, ", "), strings.Join(resolvedSGNames, ", "))
}

return sgs, nil
}

Expand Down
123 changes: 105 additions & 18 deletions pkg/networking/security_group_resolver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,15 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
resp: []*ec2sdk.SecurityGroup{
{
GroupId: awssdk.String("sg-0912f63b"),
Tags: []*ec2sdk.Tag{
{Key: awssdk.String("Name"), Value: awssdk.String("sg group one")},
},
},
{
GroupId: awssdk.String("sg-08982de7"),
Tags: []*ec2sdk.Tag{
{Key: awssdk.String("Name"), Value: awssdk.String("sg group two")},
},
},
},
},
Expand All @@ -101,6 +107,50 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
"sg-0912f63b",
},
},
{
name: "single name multiple ids",
args: args{
nameOrIDs: []string{
"sg group one",
},
describeSGCalls: []describeSecurityGroupsAsListCall{
{
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{
{
GroupId: awssdk.String("sg-id1"),
Tags: []*ec2sdk.Tag{
{Key: awssdk.String("Name"), Value: awssdk.String("sg group one")},
},
},
{
GroupId: awssdk.String("sg-id2"),
Tags: []*ec2sdk.Tag{
{Key: awssdk.String("Name"), Value: awssdk.String("sg group one")},
},
},
},
},
},
},
want: []string{
"sg-id1",
"sg-id2",
},
},
{
name: "mixed group name and id",
args: args{
Expand All @@ -127,6 +177,9 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
resp: []*ec2sdk.SecurityGroup{
{
GroupId: awssdk.String("sg-0912f63b"),
Tags: []*ec2sdk.Tag{
{Key: awssdk.String("Name"), Value: awssdk.String("sg group one")},
},
},
},
},
Expand All @@ -151,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{
Expand All @@ -163,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"),
Expand All @@ -190,27 +239,38 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
},
err: awserr.New("Describe.Error", "unable to describe security groups", nil),
},
},
},
wantErr: errors.New("couldn't find all security groups: Describe.Error: unable to describe security groups"),
},
{
name: "unable to resolve security groups by id",
args: args{
nameOrIDs: []string{
"sg-id1",
"sg-id404",
},
describeSGCalls: []describeSecurityGroupsAsListCall{
{
req: &ec2sdk.DescribeSecurityGroupsInput{
GroupIds: awssdk.StringSlice([]string{"sg-id"}),
GroupIds: awssdk.StringSlice([]string{"sg-id1", "sg-id404"}),
},
resp: []*ec2sdk.SecurityGroup{
{
GroupId: awssdk.String("sg-id"),
GroupId: awssdk.String("sg-id1"),
},
},
},
},
},
wantErr: errors.New("Describe.Error: unable to describe security groups"),
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",
name: "unable to resolve security groups by name",
args: args{
nameOrIDs: []string{
"sg group one",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we keep a negative test case for the mix name&ids ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed 12a7953 to add the negative test for mix name & ids as well as improve error reporting a little bit.

"sg-id1",
"sg-id404",
"sg group two",
},
describeSGCalls: []describeSecurityGroupsAsListCall{
{
Expand All @@ -220,6 +280,7 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
Name: awssdk.String("tag:Name"),
Values: awssdk.StringSlice([]string{
"sg group one",
"sg group two",
}),
},
{
Expand All @@ -231,22 +292,48 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
resp: []*ec2sdk.SecurityGroup{
{
GroupId: awssdk.String("sg-0912f63b"),
Tags: []*ec2sdk.Tag{
{Key: awssdk.String("Name"), Value: awssdk.String("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-id1", "sg-id404"}),
GroupIds: awssdk.StringSlice([]string{"sg-08982de7"}),
},
resp: []*ec2sdk.SecurityGroup{
{
GroupId: awssdk.String("sg-id1"),
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 securityGroups, nameOrIDs: [sg group one sg-id1 sg-id404], found: [sg-id1 sg-0912f63b]"),
wantErr: errors.New("couldn't find all security groups: requested ids [sg-08982de7] but found [], requested names [sg group one] but found []"),
},
}

Expand Down