Skip to content

Commit 42e6c00

Browse files
mtuliomtulio
committed
🐛 fix/network/subnets: update subnets before tag failures
The subnet information from unmanaged subnets, like Availability Zone name discovered when reconciling subnets, is not updated for additional subnets when the are failures to tag subnets for the preceding subnets. For example, given subnetId1 and subnetId2, if the subnet tag failed when setting tags on subnetId1, the subnetId2 will not be discovered and attributes not correctly set. This change enforce subnet updates before trying to apply tags, and not skip the lopp when checking existing subnets. Additionally, a new option was added to the unit for reconcileSubnets(), allowing to ensure expected SubnetSpec attributes from Subnets{} post reconciliation.
1 parent 240df04 commit 42e6c00

File tree

2 files changed

+243
-16
lines changed

2 files changed

+243
-16
lines changed

‎pkg/cloud/services/network/subnets.go

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -133,8 +133,17 @@ func (s *Service) reconcileSubnets() error {
133133
sub := &subnets[i]
134134
existingSubnet := existing.FindEqual(sub)
135135
if existingSubnet != nil {
136-
subnetTags := sub.Tags
136+
if len(sub.ID) > 0 {
137+
// NOTE: Describing subnets assumes the subnet.ID is the same as the subnet's identifier (i.e. subnet-<xyz>),
138+
// if we have a subnet ID specified in the spec, we need to restore it.
139+
existingSubnet.ID = sub.ID
140+
}
141+
142+
// Update subnet spec with the existing subnet details
143+
existingSubnet.DeepCopyInto(sub)
144+
137145
// Make sure tags are up-to-date.
146+
subnetTags := sub.Tags
138147
if err := wait.WaitForWithRetryable(wait.NewBackoff(), func() (bool, error) {
139148
buildParams := s.getSubnetTagParams(unmanagedVPC, existingSubnet.GetResourceID(), existingSubnet.IsPublic, existingSubnet.AvailabilityZone, subnetTags)
140149
tagsBuilder := tags.New(&buildParams, tags.WithEC2(s.EC2Client))
@@ -151,19 +160,8 @@ func (s *Service) reconcileSubnets() error {
151160
// We may not have a permission to tag unmanaged subnets.
152161
// When tagging unmanaged subnet fails, record an event and proceed.
153162
record.Warnf(s.scope.InfraCluster(), "FailedTagSubnet", "Failed tagging unmanaged Subnet %q: %v", existingSubnet.GetResourceID(), err)
154-
break
155-
}
156-
157-
// TODO(vincepri): check if subnet needs to be updated.
158-
159-
if len(sub.ID) > 0 {
160-
// NOTE: Describing subnets assumes the subnet.ID is the same as the subnet's identifier (i.e. subnet-<xyz>),
161-
// if we have a subnet ID specified in the spec, we need to restore it.
162-
existingSubnet.ID = sub.ID
163+
continue
163164
}
164-
165-
// Update subnet spec with the existing subnet details
166-
existingSubnet.DeepCopyInto(sub)
167165
} else if unmanagedVPC {
168166
// If there is no existing subnet and we have an umanaged vpc report an error
169167
record.Warnf(s.scope.InfraCluster(), "FailedMatchSubnet", "Using unmanaged VPC and failed to find existing subnet for specified subnet id %d, cidr %q", sub.GetResourceID(), sub.CidrBlock)

‎pkg/cloud/services/network/subnets_test.go

Lines changed: 232 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,9 @@ func TestReconcileSubnets(t *testing.T) {
4747
input ScopeBuilder
4848
expect func(m *mocks.MockEC2APIMockRecorder)
4949
errorExpected bool
50+
errorMessageExpected string
5051
tagUnmanagedNetworkResources bool
52+
optionalExpectSubnets infrav1.Subnets
5153
}{
5254
{
5355
name: "Unmanaged VPC, disable TagUnmanagedNetworkResources, 2 existing subnets in vpc, 2 subnet in spec, subnets match, with routes, should succeed",
@@ -486,6 +488,194 @@ func TestReconcileSubnets(t *testing.T) {
486488
},
487489
{
488490
name: "Unmanaged VPC, 2 existing matching subnets, subnet tagging fails, should succeed",
491+
input: NewClusterScope().WithNetwork(&infrav1.NetworkSpec{
492+
VPC: infrav1.VPCSpec{
493+
ID: subnetsVPCID,
494+
},
495+
Subnets: []infrav1.SubnetSpec{
496+
{
497+
ID: "subnet-1",
498+
},
499+
},
500+
}).WithTagUnmanagedNetworkResources(true),
501+
expect: func(m *mocks.MockEC2APIMockRecorder) {
502+
m.DescribeSubnetsWithContext(context.TODO(), gomock.Eq(&ec2.DescribeSubnetsInput{
503+
Filters: []*ec2.Filter{
504+
{
505+
Name: aws.String("state"),
506+
Values: []*string{aws.String("pending"), aws.String("available")},
507+
},
508+
{
509+
Name: aws.String("vpc-id"),
510+
Values: []*string{aws.String(subnetsVPCID)},
511+
},
512+
},
513+
})).
514+
Return(&ec2.DescribeSubnetsOutput{
515+
Subnets: []*ec2.Subnet{
516+
{
517+
VpcId: aws.String(subnetsVPCID),
518+
SubnetId: aws.String("subnet-1"),
519+
AvailabilityZone: aws.String("us-east-1a"),
520+
CidrBlock: aws.String("10.0.10.0/24"),
521+
MapPublicIpOnLaunch: aws.Bool(false),
522+
},
523+
},
524+
}, nil)
525+
526+
m.DescribeRouteTablesWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DescribeRouteTablesInput{})).
527+
Return(&ec2.DescribeRouteTablesOutput{
528+
RouteTables: []*ec2.RouteTable{
529+
{
530+
VpcId: aws.String(subnetsVPCID),
531+
Associations: []*ec2.RouteTableAssociation{
532+
{
533+
SubnetId: aws.String("subnet-1"),
534+
RouteTableId: aws.String("rt-12345"),
535+
},
536+
},
537+
Routes: []*ec2.Route{
538+
{
539+
GatewayId: aws.String("igw-12345"),
540+
},
541+
},
542+
},
543+
},
544+
}, nil)
545+
546+
m.DescribeNatGatewaysPagesWithContext(context.TODO(),
547+
gomock.Eq(&ec2.DescribeNatGatewaysInput{
548+
Filter: []*ec2.Filter{
549+
{
550+
Name: aws.String("vpc-id"),
551+
Values: []*string{aws.String(subnetsVPCID)},
552+
},
553+
{
554+
Name: aws.String("state"),
555+
Values: []*string{aws.String("pending"), aws.String("available")},
556+
},
557+
},
558+
}),
559+
gomock.Any()).Return(nil)
560+
561+
m.CreateTagsWithContext(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{
562+
Resources: aws.StringSlice([]string{"subnet-1"}),
563+
Tags: []*ec2.Tag{
564+
{
565+
Key: aws.String("kubernetes.io/cluster/test-cluster"),
566+
Value: aws.String("shared"),
567+
},
568+
{
569+
Key: aws.String("kubernetes.io/role/elb"),
570+
Value: aws.String("1"),
571+
},
572+
},
573+
})).
574+
Return(&ec2.CreateTagsOutput{}, fmt.Errorf("tagging failed"))
575+
},
576+
tagUnmanagedNetworkResources: true,
577+
},
578+
{
579+
name: "Unmanaged VPC, 2 existing matching subnets, subnet tagging fails with subnet update, should succeed",
580+
input: NewClusterScope().WithNetwork(&infrav1.NetworkSpec{
581+
VPC: infrav1.VPCSpec{
582+
ID: subnetsVPCID,
583+
},
584+
Subnets: []infrav1.SubnetSpec{
585+
{
586+
ID: "subnet-1",
587+
},
588+
},
589+
}).WithTagUnmanagedNetworkResources(true),
590+
optionalExpectSubnets: infrav1.Subnets{
591+
{
592+
ID: "subnet-1",
593+
ResourceID: "subnet-1",
594+
AvailabilityZone: "us-east-1a",
595+
CidrBlock: "10.0.10.0/24",
596+
IsPublic: true,
597+
Tags: infrav1.Tags{},
598+
},
599+
},
600+
expect: func(m *mocks.MockEC2APIMockRecorder) {
601+
m.DescribeSubnetsWithContext(context.TODO(), gomock.Eq(&ec2.DescribeSubnetsInput{
602+
Filters: []*ec2.Filter{
603+
{
604+
Name: aws.String("state"),
605+
Values: []*string{aws.String("pending"), aws.String("available")},
606+
},
607+
{
608+
Name: aws.String("vpc-id"),
609+
Values: []*string{aws.String(subnetsVPCID)},
610+
},
611+
},
612+
})).
613+
Return(&ec2.DescribeSubnetsOutput{
614+
Subnets: []*ec2.Subnet{
615+
{
616+
VpcId: aws.String(subnetsVPCID),
617+
SubnetId: aws.String("subnet-1"),
618+
AvailabilityZone: aws.String("us-east-1a"),
619+
CidrBlock: aws.String("10.0.10.0/24"),
620+
MapPublicIpOnLaunch: aws.Bool(false),
621+
},
622+
},
623+
}, nil)
624+
625+
m.DescribeRouteTablesWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DescribeRouteTablesInput{})).
626+
Return(&ec2.DescribeRouteTablesOutput{
627+
RouteTables: []*ec2.RouteTable{
628+
{
629+
VpcId: aws.String(subnetsVPCID),
630+
Associations: []*ec2.RouteTableAssociation{
631+
{
632+
SubnetId: aws.String("subnet-1"),
633+
RouteTableId: aws.String("rt-12345"),
634+
},
635+
},
636+
Routes: []*ec2.Route{
637+
{
638+
GatewayId: aws.String("igw-12345"),
639+
},
640+
},
641+
},
642+
},
643+
}, nil)
644+
645+
m.DescribeNatGatewaysPagesWithContext(context.TODO(),
646+
gomock.Eq(&ec2.DescribeNatGatewaysInput{
647+
Filter: []*ec2.Filter{
648+
{
649+
Name: aws.String("vpc-id"),
650+
Values: []*string{aws.String(subnetsVPCID)},
651+
},
652+
{
653+
Name: aws.String("state"),
654+
Values: []*string{aws.String("pending"), aws.String("available")},
655+
},
656+
},
657+
}),
658+
gomock.Any()).Return(nil)
659+
660+
m.CreateTagsWithContext(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{
661+
Resources: aws.StringSlice([]string{"subnet-1"}),
662+
Tags: []*ec2.Tag{
663+
{
664+
Key: aws.String("kubernetes.io/cluster/test-cluster"),
665+
Value: aws.String("shared"),
666+
},
667+
{
668+
Key: aws.String("kubernetes.io/role/elb"),
669+
Value: aws.String("1"),
670+
},
671+
},
672+
})).
673+
Return(&ec2.CreateTagsOutput{}, fmt.Errorf("tagging failed"))
674+
},
675+
tagUnmanagedNetworkResources: true,
676+
},
677+
{
678+
name: "Unmanaged VPC, 2 existing matching subnets, subnet tagging fails second call, should succeed",
489679
input: NewClusterScope().WithNetwork(&infrav1.NetworkSpec{
490680
VPC: infrav1.VPCSpec{
491681
ID: subnetsVPCID,
@@ -524,7 +714,7 @@ func TestReconcileSubnets(t *testing.T) {
524714
{
525715
VpcId: aws.String(subnetsVPCID),
526716
SubnetId: aws.String("subnet-2"),
527-
AvailabilityZone: aws.String("us-east-1a"),
717+
AvailabilityZone: aws.String("us-east-1b"),
528718
CidrBlock: aws.String("10.0.20.0/24"),
529719
MapPublicIpOnLaunch: aws.Bool(false),
530720
},
@@ -548,6 +738,20 @@ func TestReconcileSubnets(t *testing.T) {
548738
},
549739
},
550740
},
741+
{
742+
VpcId: aws.String(subnetsVPCID),
743+
Associations: []*ec2.RouteTableAssociation{
744+
{
745+
SubnetId: aws.String("subnet-2"),
746+
RouteTableId: aws.String("rt-22222"),
747+
},
748+
},
749+
Routes: []*ec2.Route{
750+
{
751+
GatewayId: aws.String("igw-12345"),
752+
},
753+
},
754+
},
551755
},
552756
}, nil)
553757

@@ -566,7 +770,7 @@ func TestReconcileSubnets(t *testing.T) {
566770
}),
567771
gomock.Any()).Return(nil)
568772

569-
m.CreateTagsWithContext(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{
773+
secondSubnetTag := m.CreateTagsWithContext(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{
570774
Resources: aws.StringSlice([]string{"subnet-1"}),
571775
Tags: []*ec2.Tag{
572776
{
@@ -579,7 +783,22 @@ func TestReconcileSubnets(t *testing.T) {
579783
},
580784
},
581785
})).
582-
Return(&ec2.CreateTagsOutput{}, fmt.Errorf("tagging failed"))
786+
Return(&ec2.CreateTagsOutput{}, nil)
787+
788+
m.CreateTagsWithContext(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{
789+
Resources: aws.StringSlice([]string{"subnet-2"}),
790+
Tags: []*ec2.Tag{
791+
{
792+
Key: aws.String("kubernetes.io/cluster/test-cluster"),
793+
Value: aws.String("shared"),
794+
},
795+
{
796+
Key: aws.String("kubernetes.io/role/elb"),
797+
Value: aws.String("1"),
798+
},
799+
},
800+
})).
801+
Return(&ec2.CreateTagsOutput{}, fmt.Errorf("tagging failed")).After(secondSubnetTag)
583802
},
584803
tagUnmanagedNetworkResources: true,
585804
},
@@ -2341,6 +2560,16 @@ func TestReconcileSubnets(t *testing.T) {
23412560
if !tc.errorExpected && err != nil {
23422561
t.Fatalf("got an unexpected error: %v", err)
23432562
}
2563+
if tc.errorExpected && err != nil && len(tc.errorMessageExpected) > 0 {
2564+
if err.Error() != tc.errorMessageExpected {
2565+
t.Fatalf("got an unexpected error message: %v", err)
2566+
}
2567+
}
2568+
if len(tc.optionalExpectSubnets) > 0 {
2569+
if !cmp.Equal(s.scope.Subnets(), tc.optionalExpectSubnets) {
2570+
t.Errorf("got unexpect Subnets():\n%v", cmp.Diff(s.scope.Subnets(), tc.optionalExpectSubnets))
2571+
}
2572+
}
23442573
})
23452574
}
23462575
}

0 commit comments

Comments
 (0)