Skip to content

Commit e1d32f4

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #3775 from alloveras/main
fixup(sg-resolver): Allow multiple SGs with the same Name tag
2 parents cb5271e + 12a7953 commit e1d32f4

File tree

4 files changed

+217
-26
lines changed

4 files changed

+217
-26
lines changed

‎pkg/algorithm/slices.go

+18
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
package algorithm
2+
3+
import "cmp"
4+
5+
// RemoveSliceDuplicates returns a copy of the slice without duplicate entries.
6+
func RemoveSliceDuplicates[S ~[]E, E cmp.Ordered](s S) []E {
7+
result := make([]E, 0, len(s))
8+
found := make(map[E]struct{}, len(s))
9+
10+
for _, x := range s {
11+
if _, ok := found[x]; !ok {
12+
found[x] = struct{}{}
13+
result = append(result, x)
14+
}
15+
}
16+
17+
return result
18+
}

‎pkg/algorithm/slices_test.go

+46
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
package algorithm
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
)
8+
9+
func Test_RemoveSliceDuplicates(t *testing.T) {
10+
type args struct {
11+
data []string
12+
}
13+
tests := []struct {
14+
name string
15+
args args
16+
want []string
17+
}{
18+
{
19+
name: "empty",
20+
args: args{
21+
data: []string{},
22+
},
23+
want: []string{},
24+
},
25+
{
26+
name: "no duplicate entries",
27+
args: args{
28+
data: []string{"a", "b", "c", "d"},
29+
},
30+
want: []string{"a", "b", "c", "d"},
31+
},
32+
{
33+
name: "with duplicates",
34+
args: args{
35+
data: []string{"a", "b", "a", "c", "b"},
36+
},
37+
want: []string{"a", "b", "c"},
38+
},
39+
}
40+
for _, tt := range tests {
41+
t.Run(tt.name, func(t *testing.T) {
42+
got := RemoveSliceDuplicates(tt.args.data)
43+
assert.Equal(t, tt.want, got)
44+
})
45+
}
46+
}

‎pkg/networking/security_group_resolver.go

+48-8
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
awssdk "github.com/aws/aws-sdk-go/aws"
88
ec2sdk "github.com/aws/aws-sdk-go/service/ec2"
99
"github.com/pkg/errors"
10+
"sigs.k8s.io/aws-load-balancer-controller/pkg/algorithm"
1011
"sigs.k8s.io/aws-load-balancer-controller/pkg/aws/services"
1112
)
1213

@@ -33,44 +34,66 @@ type defaultSecurityGroupResolver struct {
3334
}
3435

3536
func (r *defaultSecurityGroupResolver) ResolveViaNameOrID(ctx context.Context, sgNameOrIDs []string) ([]string, error) {
36-
sgIDs, sgNames := r.splitIntoSgNameAndIDs(sgNameOrIDs)
3737
var resolvedSGs []*ec2sdk.SecurityGroup
38+
var errMessages []string
39+
40+
sgIDs, sgNames := r.splitIntoSgNameAndIDs(sgNameOrIDs)
41+
3842
if len(sgIDs) > 0 {
3943
sgs, err := r.resolveViaGroupID(ctx, sgIDs)
4044
if err != nil {
41-
return nil, err
45+
errMessages = append(errMessages, err.Error())
46+
} else {
47+
resolvedSGs = append(resolvedSGs, sgs...)
4248
}
43-
resolvedSGs = append(resolvedSGs, sgs...)
4449
}
50+
4551
if len(sgNames) > 0 {
4652
sgs, err := r.resolveViaGroupName(ctx, sgNames)
4753
if err != nil {
48-
return nil, err
54+
errMessages = append(errMessages, err.Error())
55+
} else {
56+
resolvedSGs = append(resolvedSGs, sgs...)
4957
}
50-
resolvedSGs = append(resolvedSGs, sgs...)
5158
}
59+
60+
if len(errMessages) > 0 {
61+
return nil, errors.Errorf("couldn't find all security groups: %s", strings.Join(errMessages, ", "))
62+
}
63+
5264
resolvedSGIDs := make([]string, 0, len(resolvedSGs))
5365
for _, sg := range resolvedSGs {
5466
resolvedSGIDs = append(resolvedSGIDs, awssdk.StringValue(sg.GroupId))
5567
}
56-
if len(resolvedSGIDs) != len(sgNameOrIDs) {
57-
return nil, errors.Errorf("couldn't find all securityGroups, nameOrIDs: %v, found: %v", sgNameOrIDs, resolvedSGIDs)
58-
}
68+
5969
return resolvedSGIDs, nil
6070
}
6171

6272
func (r *defaultSecurityGroupResolver) resolveViaGroupID(ctx context.Context, sgIDs []string) ([]*ec2sdk.SecurityGroup, error) {
6373
req := &ec2sdk.DescribeSecurityGroupsInput{
6474
GroupIds: awssdk.StringSlice(sgIDs),
6575
}
76+
6677
sgs, err := r.ec2Client.DescribeSecurityGroupsAsList(ctx, req)
6778
if err != nil {
6879
return nil, err
6980
}
81+
82+
resolvedSGIDs := make([]string, 0, len(sgs))
83+
for _, sg := range sgs {
84+
resolvedSGIDs = append(resolvedSGIDs, awssdk.StringValue(sg.GroupId))
85+
}
86+
87+
if len(sgIDs) != len(resolvedSGIDs) {
88+
return nil, errors.Errorf("requested ids [%s] but found [%s]", strings.Join(sgIDs, ", "), strings.Join(resolvedSGIDs, ", "))
89+
}
90+
7091
return sgs, nil
7192
}
7293

7394
func (r *defaultSecurityGroupResolver) resolveViaGroupName(ctx context.Context, sgNames []string) ([]*ec2sdk.SecurityGroup, error) {
95+
sgNames = algorithm.RemoveSliceDuplicates(sgNames)
96+
7497
req := &ec2sdk.DescribeSecurityGroupsInput{
7598
Filters: []*ec2sdk.Filter{
7699
{
@@ -83,10 +106,27 @@ func (r *defaultSecurityGroupResolver) resolveViaGroupName(ctx context.Context,
83106
},
84107
},
85108
}
109+
86110
sgs, err := r.ec2Client.DescribeSecurityGroupsAsList(ctx, req)
87111
if err != nil {
88112
return nil, err
89113
}
114+
115+
resolvedSGNames := make([]string, 0, len(sgs))
116+
for _, sg := range sgs {
117+
for _, tag := range sg.Tags {
118+
if awssdk.StringValue(tag.Key) == "Name" {
119+
resolvedSGNames = append(resolvedSGNames, awssdk.StringValue(tag.Value))
120+
}
121+
}
122+
}
123+
124+
resolvedSGNames = algorithm.RemoveSliceDuplicates(resolvedSGNames)
125+
126+
if len(sgNames) != len(resolvedSGNames) {
127+
return nil, errors.Errorf("requested names [%s] but found [%s]", strings.Join(sgNames, ", "), strings.Join(resolvedSGNames, ", "))
128+
}
129+
90130
return sgs, nil
91131
}
92132

‎pkg/networking/security_group_resolver_test.go

+105-18
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,15 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
8888
resp: []*ec2sdk.SecurityGroup{
8989
{
9090
GroupId: awssdk.String("sg-0912f63b"),
91+
Tags: []*ec2sdk.Tag{
92+
{Key: awssdk.String("Name"), Value: awssdk.String("sg group one")},
93+
},
9194
},
9295
{
9396
GroupId: awssdk.String("sg-08982de7"),
97+
Tags: []*ec2sdk.Tag{
98+
{Key: awssdk.String("Name"), Value: awssdk.String("sg group two")},
99+
},
94100
},
95101
},
96102
},
@@ -101,6 +107,50 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
101107
"sg-0912f63b",
102108
},
103109
},
110+
{
111+
name: "single name multiple ids",
112+
args: args{
113+
nameOrIDs: []string{
114+
"sg group one",
115+
},
116+
describeSGCalls: []describeSecurityGroupsAsListCall{
117+
{
118+
req: &ec2sdk.DescribeSecurityGroupsInput{
119+
Filters: []*ec2sdk.Filter{
120+
{
121+
Name: awssdk.String("tag:Name"),
122+
Values: awssdk.StringSlice([]string{
123+
"sg group one",
124+
}),
125+
},
126+
{
127+
Name: awssdk.String("vpc-id"),
128+
Values: awssdk.StringSlice([]string{defaultVPCID}),
129+
},
130+
},
131+
},
132+
resp: []*ec2sdk.SecurityGroup{
133+
{
134+
GroupId: awssdk.String("sg-id1"),
135+
Tags: []*ec2sdk.Tag{
136+
{Key: awssdk.String("Name"), Value: awssdk.String("sg group one")},
137+
},
138+
},
139+
{
140+
GroupId: awssdk.String("sg-id2"),
141+
Tags: []*ec2sdk.Tag{
142+
{Key: awssdk.String("Name"), Value: awssdk.String("sg group one")},
143+
},
144+
},
145+
},
146+
},
147+
},
148+
},
149+
want: []string{
150+
"sg-id1",
151+
"sg-id2",
152+
},
153+
},
104154
{
105155
name: "mixed group name and id",
106156
args: args{
@@ -127,6 +177,9 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
127177
resp: []*ec2sdk.SecurityGroup{
128178
{
129179
GroupId: awssdk.String("sg-0912f63b"),
180+
Tags: []*ec2sdk.Tag{
181+
{Key: awssdk.String("Name"), Value: awssdk.String("sg group one")},
182+
},
130183
},
131184
},
132185
},
@@ -151,7 +204,6 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
151204
name: "describe by id returns error",
152205
args: args{
153206
nameOrIDs: []string{
154-
"sg group name",
155207
"sg-id",
156208
},
157209
describeSGCalls: []describeSecurityGroupsAsListCall{
@@ -163,24 +215,21 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
163215
},
164216
},
165217
},
166-
wantErr: errors.New("Describe.Error: unable to describe security groups"),
218+
wantErr: errors.New("couldn't find all security groups: Describe.Error: unable to describe security groups"),
167219
},
168220
{
169221
name: "describe by name returns error",
170222
args: args{
171223
nameOrIDs: []string{
172224
"sg group name",
173-
"sg-id",
174225
},
175226
describeSGCalls: []describeSecurityGroupsAsListCall{
176227
{
177228
req: &ec2sdk.DescribeSecurityGroupsInput{
178229
Filters: []*ec2sdk.Filter{
179230
{
180-
Name: awssdk.String("tag:Name"),
181-
Values: awssdk.StringSlice([]string{
182-
"sg group name",
183-
}),
231+
Name: awssdk.String("tag:Name"),
232+
Values: awssdk.StringSlice([]string{"sg group name"}),
184233
},
185234
{
186235
Name: awssdk.String("vpc-id"),
@@ -190,27 +239,38 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
190239
},
191240
err: awserr.New("Describe.Error", "unable to describe security groups", nil),
192241
},
242+
},
243+
},
244+
wantErr: errors.New("couldn't find all security groups: Describe.Error: unable to describe security groups"),
245+
},
246+
{
247+
name: "unable to resolve security groups by id",
248+
args: args{
249+
nameOrIDs: []string{
250+
"sg-id1",
251+
"sg-id404",
252+
},
253+
describeSGCalls: []describeSecurityGroupsAsListCall{
193254
{
194255
req: &ec2sdk.DescribeSecurityGroupsInput{
195-
GroupIds: awssdk.StringSlice([]string{"sg-id"}),
256+
GroupIds: awssdk.StringSlice([]string{"sg-id1", "sg-id404"}),
196257
},
197258
resp: []*ec2sdk.SecurityGroup{
198259
{
199-
GroupId: awssdk.String("sg-id"),
260+
GroupId: awssdk.String("sg-id1"),
200261
},
201262
},
202263
},
203264
},
204265
},
205-
wantErr: errors.New("Describe.Error: unable to describe security groups"),
266+
wantErr: errors.New("couldn't find all security groups: requested ids [sg-id1, sg-id404] but found [sg-id1]"),
206267
},
207268
{
208-
name: "unable to resolve all security groups",
269+
name: "unable to resolve security groups by name",
209270
args: args{
210271
nameOrIDs: []string{
211272
"sg group one",
212-
"sg-id1",
213-
"sg-id404",
273+
"sg group two",
214274
},
215275
describeSGCalls: []describeSecurityGroupsAsListCall{
216276
{
@@ -220,6 +280,7 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
220280
Name: awssdk.String("tag:Name"),
221281
Values: awssdk.StringSlice([]string{
222282
"sg group one",
283+
"sg group two",
223284
}),
224285
},
225286
{
@@ -231,22 +292,48 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
231292
resp: []*ec2sdk.SecurityGroup{
232293
{
233294
GroupId: awssdk.String("sg-0912f63b"),
295+
Tags: []*ec2sdk.Tag{
296+
{Key: awssdk.String("Name"), Value: awssdk.String("sg group one")},
297+
},
234298
},
235299
},
236300
},
301+
},
302+
},
303+
wantErr: errors.New("couldn't find all security groups: requested names [sg group one, sg group two] but found [sg group one]"),
304+
},
305+
{
306+
name: "unable to resolve all security groups by ids and names",
307+
args: args{
308+
nameOrIDs: []string{
309+
"sg-08982de7",
310+
"sg group one",
311+
},
312+
describeSGCalls: []describeSecurityGroupsAsListCall{
237313
{
238314
req: &ec2sdk.DescribeSecurityGroupsInput{
239-
GroupIds: awssdk.StringSlice([]string{"sg-id1", "sg-id404"}),
315+
GroupIds: awssdk.StringSlice([]string{"sg-08982de7"}),
240316
},
241-
resp: []*ec2sdk.SecurityGroup{
242-
{
243-
GroupId: awssdk.String("sg-id1"),
317+
resp: []*ec2sdk.SecurityGroup{},
318+
},
319+
{
320+
req: &ec2sdk.DescribeSecurityGroupsInput{
321+
Filters: []*ec2sdk.Filter{
322+
{
323+
Name: awssdk.String("tag:Name"),
324+
Values: awssdk.StringSlice([]string{"sg group one"}),
325+
},
326+
{
327+
Name: awssdk.String("vpc-id"),
328+
Values: awssdk.StringSlice([]string{defaultVPCID}),
329+
},
244330
},
245331
},
332+
resp: []*ec2sdk.SecurityGroup{},
246333
},
247334
},
248335
},
249-
wantErr: errors.New("couldn't find all securityGroups, nameOrIDs: [sg group one sg-id1 sg-id404], found: [sg-id1 sg-0912f63b]"),
336+
wantErr: errors.New("couldn't find all security groups: requested ids [sg-08982de7] but found [], requested names [sg group one] but found []"),
250337
},
251338
}
252339

0 commit comments

Comments
 (0)