Skip to content

Commit dd2962d

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #4917 from mtulio/CORS-3214-wavelength-zones-fix-zoneinfo
🐛 fix: update network subnets prio tag failures
2 parents ad71a54 + 42e6c00 commit dd2962d

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)