Skip to content

Commit f845ff5

Browse files
kishorjkishorj
committed
relax cluster tag requirement on subnets
1 parent 67494bd commit f845ff5

File tree

2 files changed

+202
-38
lines changed

2 files changed

+202
-38
lines changed

‎pkg/networking/subnet_resolver.go

Lines changed: 44 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -112,12 +112,7 @@ func (r *defaultSubnetsResolver) ResolveViaDiscovery(ctx context.Context, opts .
112112
case elbv2model.LoadBalancerSchemeInternetFacing:
113113
subnetRoleTagKey = TagKeySubnetPublicELB
114114
}
115-
clusterResourceTagKey := fmt.Sprintf("kubernetes.io/cluster/%s", r.clusterName)
116115
req := &ec2sdk.DescribeSubnetsInput{Filters: []*ec2sdk.Filter{
117-
{
118-
Name: awssdk.String("tag:" + clusterResourceTagKey),
119-
Values: awssdk.StringSlice([]string{"owned", "shared"}),
120-
},
121116
{
122117
Name: awssdk.String("tag:" + subnetRoleTagKey),
123118
Values: awssdk.StringSlice([]string{"", "1"}),
@@ -128,17 +123,31 @@ func (r *defaultSubnetsResolver) ResolveViaDiscovery(ctx context.Context, opts .
128123
},
129124
}}
130125

131-
subnets, err := r.ec2Client.DescribeSubnetsAsList(ctx, req)
126+
allSubnets, err := r.ec2Client.DescribeSubnetsAsList(ctx, req)
132127
if err != nil {
133128
return nil, err
134129
}
130+
var subnets []*ec2sdk.Subnet
131+
for _, subnet := range allSubnets {
132+
if r.checkSubnetIsNotTaggedForOtherClusters(subnet) {
133+
subnets = append(subnets, subnet)
134+
}
135+
}
135136
subnetsByAZ := mapSDKSubnetsByAZ(subnets)
136137
chosenSubnets := make([]*ec2sdk.Subnet, 0, len(subnetsByAZ))
137138
for az, subnets := range subnetsByAZ {
138139
if len(subnets) == 1 {
139140
chosenSubnets = append(chosenSubnets, subnets[0])
140141
} else if len(subnets) > 1 {
141142
sort.Slice(subnets, func(i, j int) bool {
143+
clusterTagI := r.checkSubnetHasClusterTag(subnets[i])
144+
clusterTagJ := r.checkSubnetHasClusterTag(subnets[j])
145+
if clusterTagI != clusterTagJ {
146+
if clusterTagI {
147+
return true
148+
}
149+
return false
150+
}
142151
return awssdk.StringValue(subnets[i].SubnetId) < awssdk.StringValue(subnets[j].SubnetId)
143152
})
144153
r.logger.Info("multiple subnet in the same AvailabilityZone", "AvailabilityZone", az,
@@ -298,3 +307,32 @@ func sortSubnetsByID(subnets []*ec2sdk.Subnet) {
298307
return awssdk.StringValue(subnets[i].SubnetId) < awssdk.StringValue(subnets[j].SubnetId)
299308
})
300309
}
310+
311+
// checkSubnetHasClusterTag checks if the subnet is tagged for the current cluster
312+
func (r *defaultSubnetsResolver) checkSubnetHasClusterTag(subnet *ec2sdk.Subnet) bool {
313+
clusterResourceTagKey := fmt.Sprintf("kubernetes.io/cluster/%s", r.clusterName)
314+
for _, tag := range subnet.Tags {
315+
if clusterResourceTagKey == awssdk.StringValue(tag.Key) {
316+
return true
317+
}
318+
}
319+
return false
320+
}
321+
322+
// checkSubnetIsNotTaggedForOtherClusters checks whether the subnet is tagged for the current cluster
323+
// or it doesn't contain the cluster tag at all. If the subnet contains a tag for other clusters, then
324+
// this check returns false so that the subnet does not used for the load balancer.
325+
func (r *defaultSubnetsResolver) checkSubnetIsNotTaggedForOtherClusters(subnet *ec2sdk.Subnet) bool {
326+
clusterResourceTagPrefix := "kubernetes.io/cluster"
327+
clusterResourceTagKey := fmt.Sprintf("kubernetes.io/cluster/%s", r.clusterName)
328+
for _, tag := range subnet.Tags {
329+
tagKey := awssdk.StringValue(tag.Key)
330+
if tagKey == clusterResourceTagKey {
331+
return true
332+
}
333+
if strings.HasPrefix(tagKey, clusterResourceTagPrefix) {
334+
return false
335+
}
336+
}
337+
return true
338+
}

‎pkg/networking/subnet_resolver_test.go

Lines changed: 158 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,6 @@ func Test_defaultSubnetsResolver_ResolveViaDiscovery(t *testing.T) {
4545
{
4646
input: &ec2sdk.DescribeSubnetsInput{
4747
Filters: []*ec2sdk.Filter{
48-
{
49-
Name: awssdk.String("tag:kubernetes.io/cluster/kube-cluster"),
50-
Values: awssdk.StringSlice([]string{"owned", "shared"}),
51-
},
5248
{
5349
Name: awssdk.String("tag:kubernetes.io/role/elb"),
5450
Values: awssdk.StringSlice([]string{"", "1"}),
@@ -102,10 +98,6 @@ func Test_defaultSubnetsResolver_ResolveViaDiscovery(t *testing.T) {
10298
{
10399
input: &ec2sdk.DescribeSubnetsInput{
104100
Filters: []*ec2sdk.Filter{
105-
{
106-
Name: awssdk.String("tag:kubernetes.io/cluster/kube-cluster"),
107-
Values: awssdk.StringSlice([]string{"owned", "shared"}),
108-
},
109101
{
110102
Name: awssdk.String("tag:kubernetes.io/role/internal-elb"),
111103
Values: awssdk.StringSlice([]string{"", "1"}),
@@ -159,10 +151,6 @@ func Test_defaultSubnetsResolver_ResolveViaDiscovery(t *testing.T) {
159151
{
160152
input: &ec2sdk.DescribeSubnetsInput{
161153
Filters: []*ec2sdk.Filter{
162-
{
163-
Name: awssdk.String("tag:kubernetes.io/cluster/kube-cluster"),
164-
Values: awssdk.StringSlice([]string{"owned", "shared"}),
165-
},
166154
{
167155
Name: awssdk.String("tag:kubernetes.io/role/internal-elb"),
168156
Values: awssdk.StringSlice([]string{"", "1"}),
@@ -194,10 +182,6 @@ func Test_defaultSubnetsResolver_ResolveViaDiscovery(t *testing.T) {
194182
{
195183
input: &ec2sdk.DescribeSubnetsInput{
196184
Filters: []*ec2sdk.Filter{
197-
{
198-
Name: awssdk.String("tag:kubernetes.io/cluster/kube-cluster"),
199-
Values: awssdk.StringSlice([]string{"owned", "shared"}),
200-
},
201185
{
202186
Name: awssdk.String("tag:kubernetes.io/role/internal-elb"),
203187
Values: awssdk.StringSlice([]string{"", "1"}),
@@ -241,10 +225,6 @@ func Test_defaultSubnetsResolver_ResolveViaDiscovery(t *testing.T) {
241225
{
242226
input: &ec2sdk.DescribeSubnetsInput{
243227
Filters: []*ec2sdk.Filter{
244-
{
245-
Name: awssdk.String("tag:kubernetes.io/cluster/kube-cluster"),
246-
Values: awssdk.StringSlice([]string{"owned", "shared"}),
247-
},
248228
{
249229
Name: awssdk.String("tag:kubernetes.io/role/internal-elb"),
250230
Values: awssdk.StringSlice([]string{"", "1"}),
@@ -282,10 +262,6 @@ func Test_defaultSubnetsResolver_ResolveViaDiscovery(t *testing.T) {
282262
{
283263
input: &ec2sdk.DescribeSubnetsInput{
284264
Filters: []*ec2sdk.Filter{
285-
{
286-
Name: awssdk.String("tag:kubernetes.io/cluster/kube-cluster"),
287-
Values: awssdk.StringSlice([]string{"owned", "shared"}),
288-
},
289265
{
290266
Name: awssdk.String("tag:kubernetes.io/role/internal-elb"),
291267
Values: awssdk.StringSlice([]string{"", "1"}),
@@ -349,10 +325,6 @@ func Test_defaultSubnetsResolver_ResolveViaDiscovery(t *testing.T) {
349325
{
350326
input: &ec2sdk.DescribeSubnetsInput{
351327
Filters: []*ec2sdk.Filter{
352-
{
353-
Name: awssdk.String("tag:kubernetes.io/cluster/kube-cluster"),
354-
Values: awssdk.StringSlice([]string{"owned", "shared"}),
355-
},
356328
{
357329
Name: awssdk.String("tag:kubernetes.io/role/internal-elb"),
358330
Values: awssdk.StringSlice([]string{"", "1"}),
@@ -396,10 +368,6 @@ func Test_defaultSubnetsResolver_ResolveViaDiscovery(t *testing.T) {
396368
{
397369
input: &ec2sdk.DescribeSubnetsInput{
398370
Filters: []*ec2sdk.Filter{
399-
{
400-
Name: awssdk.String("tag:kubernetes.io/cluster/kube-cluster"),
401-
Values: awssdk.StringSlice([]string{"owned", "shared"}),
402-
},
403371
{
404372
Name: awssdk.String("tag:kubernetes.io/role/internal-elb"),
405373
Values: awssdk.StringSlice([]string{"", "1"}),
@@ -422,6 +390,164 @@ func Test_defaultSubnetsResolver_ResolveViaDiscovery(t *testing.T) {
422390
},
423391
wantErr: errors.New("some error"),
424392
},
393+
{
394+
name: "subnet with cluster tag gets precedence",
395+
fields: fields{
396+
vpcID: "vpc-1",
397+
clusterName: "kube-cluster",
398+
describeSubnetsAsListCalls: []describeSubnetsAsListCall{
399+
{
400+
input: &ec2sdk.DescribeSubnetsInput{
401+
Filters: []*ec2sdk.Filter{
402+
{
403+
Name: awssdk.String("tag:kubernetes.io/role/elb"),
404+
Values: awssdk.StringSlice([]string{"", "1"}),
405+
},
406+
{
407+
Name: awssdk.String("vpc-id"),
408+
Values: awssdk.StringSlice([]string{"vpc-1"}),
409+
},
410+
},
411+
},
412+
output: []*ec2sdk.Subnet{
413+
{
414+
SubnetId: awssdk.String("subnet-1"),
415+
AvailabilityZone: awssdk.String("us-west-2b"),
416+
VpcId: awssdk.String("vpc-1"),
417+
},
418+
{
419+
SubnetId: awssdk.String("subnet-2"),
420+
AvailabilityZone: awssdk.String("us-west-2b"),
421+
VpcId: awssdk.String("vpc-1"),
422+
Tags: []*ec2sdk.Tag{
423+
{
424+
Key: awssdk.String("kubernetes.io/cluster/kube-cluster"),
425+
Value: awssdk.String("owned"),
426+
},
427+
},
428+
},
429+
},
430+
},
431+
},
432+
},
433+
args: args{
434+
opts: []SubnetsResolveOption{
435+
WithSubnetsResolveLBType(elbv2model.LoadBalancerTypeNetwork),
436+
WithSubnetsResolveLBScheme(elbv2model.LoadBalancerSchemeInternetFacing),
437+
},
438+
},
439+
want: []*ec2sdk.Subnet{
440+
{
441+
SubnetId: awssdk.String("subnet-2"),
442+
AvailabilityZone: awssdk.String("us-west-2b"),
443+
VpcId: awssdk.String("vpc-1"),
444+
Tags: []*ec2sdk.Tag{
445+
{
446+
Key: awssdk.String("kubernetes.io/cluster/kube-cluster"),
447+
Value: awssdk.String("owned"),
448+
},
449+
},
450+
},
451+
},
452+
},
453+
{
454+
name: "subnets tagged for some other clusters get ignored",
455+
fields: fields{
456+
vpcID: "vpc-1",
457+
clusterName: "kube-cluster",
458+
describeSubnetsAsListCalls: []describeSubnetsAsListCall{
459+
{
460+
input: &ec2sdk.DescribeSubnetsInput{
461+
Filters: []*ec2sdk.Filter{
462+
{
463+
Name: awssdk.String("tag:kubernetes.io/role/elb"),
464+
Values: awssdk.StringSlice([]string{"", "1"}),
465+
},
466+
{
467+
Name: awssdk.String("vpc-id"),
468+
Values: awssdk.StringSlice([]string{"vpc-1"}),
469+
},
470+
},
471+
},
472+
output: []*ec2sdk.Subnet{
473+
{
474+
SubnetId: awssdk.String("subnet-1"),
475+
AvailabilityZone: awssdk.String("us-west-2a"),
476+
VpcId: awssdk.String("vpc-1"),
477+
Tags: []*ec2sdk.Tag{
478+
{
479+
Key: awssdk.String("kubernetes.io/cluster/some-other-cluster"),
480+
Value: awssdk.String("owned"),
481+
},
482+
},
483+
},
484+
{
485+
SubnetId: awssdk.String("subnet-3"),
486+
AvailabilityZone: awssdk.String("us-west-2a"),
487+
VpcId: awssdk.String("vpc-1"),
488+
Tags: []*ec2sdk.Tag{
489+
{
490+
Key: awssdk.String("kubernetes.io/cluster/kube-cluster"),
491+
Value: awssdk.String("owned"),
492+
},
493+
},
494+
},
495+
{
496+
SubnetId: awssdk.String("subnet-4"),
497+
AvailabilityZone: awssdk.String("us-west-2b"),
498+
VpcId: awssdk.String("vpc-1"),
499+
Tags: []*ec2sdk.Tag{
500+
{
501+
Key: awssdk.String("kubernetes.io/cluster/no-cluster"),
502+
Value: awssdk.String("owned"),
503+
},
504+
},
505+
},
506+
{
507+
SubnetId: awssdk.String("subnet-2"),
508+
AvailabilityZone: awssdk.String("us-west-2a"),
509+
VpcId: awssdk.String("vpc-1"),
510+
Tags: []*ec2sdk.Tag{
511+
{
512+
Key: awssdk.String("kubernetes.io/cluster/kube-cluster"),
513+
Value: awssdk.String("owned"),
514+
},
515+
},
516+
},
517+
{
518+
SubnetId: awssdk.String("subnet-5"),
519+
AvailabilityZone: awssdk.String("us-west-2c"),
520+
VpcId: awssdk.String("vpc-1"),
521+
},
522+
},
523+
},
524+
},
525+
},
526+
args: args{
527+
opts: []SubnetsResolveOption{
528+
WithSubnetsResolveLBType(elbv2model.LoadBalancerTypeNetwork),
529+
WithSubnetsResolveLBScheme(elbv2model.LoadBalancerSchemeInternetFacing),
530+
},
531+
},
532+
want: []*ec2sdk.Subnet{
533+
{
534+
SubnetId: awssdk.String("subnet-2"),
535+
AvailabilityZone: awssdk.String("us-west-2a"),
536+
VpcId: awssdk.String("vpc-1"),
537+
Tags: []*ec2sdk.Tag{
538+
{
539+
Key: awssdk.String("kubernetes.io/cluster/kube-cluster"),
540+
Value: awssdk.String("owned"),
541+
},
542+
},
543+
},
544+
{
545+
SubnetId: awssdk.String("subnet-5"),
546+
AvailabilityZone: awssdk.String("us-west-2c"),
547+
VpcId: awssdk.String("vpc-1"),
548+
},
549+
},
550+
},
425551
}
426552

427553
for _, tt := range tests {

0 commit comments

Comments
 (0)