Skip to content

Commit 240df04

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #4899 from mtulio/CORS-3214-fix-clean-rtb-onfailure
🐛 fix/network/rtb: delete rtb handling err when failed to create routes
2 parents d3c59d9 + 283c07b commit 240df04

File tree

2 files changed

+199
-68
lines changed

2 files changed

+199
-68
lines changed

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

Lines changed: 33 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -214,6 +214,32 @@ func (s *Service) describeVpcRouteTablesBySubnet() (map[string]*ec2.RouteTable,
214214
return res, nil
215215
}
216216

217+
func (s *Service) deleteRouteTable(rt *ec2.RouteTable) error {
218+
for _, as := range rt.Associations {
219+
if as.SubnetId == nil {
220+
continue
221+
}
222+
223+
if _, err := s.EC2Client.DisassociateRouteTableWithContext(context.TODO(), &ec2.DisassociateRouteTableInput{AssociationId: as.RouteTableAssociationId}); err != nil {
224+
record.Warnf(s.scope.InfraCluster(), "FailedDisassociateRouteTable", "Failed to disassociate managed RouteTable %q from Subnet %q: %v", *rt.RouteTableId, *as.SubnetId, err)
225+
return errors.Wrapf(err, "failed to disassociate route table %q from subnet %q", *rt.RouteTableId, *as.SubnetId)
226+
}
227+
228+
record.Eventf(s.scope.InfraCluster(), "SuccessfulDisassociateRouteTable", "Disassociated managed RouteTable %q from subnet %q", *rt.RouteTableId, *as.SubnetId)
229+
s.scope.Debug("Deleted association between route table and subnet", "route-table-id", *rt.RouteTableId, "subnet-id", *as.SubnetId)
230+
}
231+
232+
if _, err := s.EC2Client.DeleteRouteTableWithContext(context.TODO(), &ec2.DeleteRouteTableInput{RouteTableId: rt.RouteTableId}); err != nil {
233+
record.Warnf(s.scope.InfraCluster(), "FailedDeleteRouteTable", "Failed to delete managed RouteTable %q: %v", *rt.RouteTableId, err)
234+
return errors.Wrapf(err, "failed to delete route table %q", *rt.RouteTableId)
235+
}
236+
237+
record.Eventf(s.scope.InfraCluster(), "SuccessfulDeleteRouteTable", "Deleted managed RouteTable %q", *rt.RouteTableId)
238+
s.scope.Info("Deleted route table", "route-table-id", *rt.RouteTableId)
239+
240+
return nil
241+
}
242+
217243
func (s *Service) deleteRouteTables() error {
218244
if s.scope.VPC().IsUnmanaged(s.scope.Name()) {
219245
s.scope.Trace("Skipping routing tables deletion in unmanaged mode")
@@ -226,27 +252,10 @@ func (s *Service) deleteRouteTables() error {
226252
}
227253

228254
for _, rt := range rts {
229-
for _, as := range rt.Associations {
230-
if as.SubnetId == nil {
231-
continue
232-
}
233-
234-
if _, err := s.EC2Client.DisassociateRouteTableWithContext(context.TODO(), &ec2.DisassociateRouteTableInput{AssociationId: as.RouteTableAssociationId}); err != nil {
235-
record.Warnf(s.scope.InfraCluster(), "FailedDisassociateRouteTable", "Failed to disassociate managed RouteTable %q from Subnet %q: %v", *rt.RouteTableId, *as.SubnetId, err)
236-
return errors.Wrapf(err, "failed to disassociate route table %q from subnet %q", *rt.RouteTableId, *as.SubnetId)
237-
}
238-
239-
record.Eventf(s.scope.InfraCluster(), "SuccessfulDisassociateRouteTable", "Disassociated managed RouteTable %q from subnet %q", *rt.RouteTableId, *as.SubnetId)
240-
s.scope.Debug("Deleted association between route table and subnet", "route-table-id", *rt.RouteTableId, "subnet-id", *as.SubnetId)
241-
}
242-
243-
if _, err := s.EC2Client.DeleteRouteTableWithContext(context.TODO(), &ec2.DeleteRouteTableInput{RouteTableId: rt.RouteTableId}); err != nil {
244-
record.Warnf(s.scope.InfraCluster(), "FailedDeleteRouteTable", "Failed to delete managed RouteTable %q: %v", *rt.RouteTableId, err)
245-
return errors.Wrapf(err, "failed to delete route table %q", *rt.RouteTableId)
255+
err := s.deleteRouteTable(rt)
256+
if err != nil {
257+
return err
246258
}
247-
248-
record.Eventf(s.scope.InfraCluster(), "SuccessfulDeleteRouteTable", "Deleted managed RouteTable %q", *rt.RouteTableId)
249-
s.scope.Info("Deleted route table", "route-table-id", *rt.RouteTableId)
250259
}
251260
return nil
252261
}
@@ -302,8 +311,11 @@ func (s *Service) createRouteTableWithRoutes(routes []*ec2.Route, isPublic bool,
302311
}
303312
return true, nil
304313
}, awserrors.RouteTableNotFound, awserrors.NATGatewayNotFound, awserrors.GatewayNotFound); err != nil {
305-
// TODO(vincepri): cleanup the route table if this fails.
306314
record.Warnf(s.scope.InfraCluster(), "FailedCreateRoute", "Failed to create route %s for RouteTable %q: %v", route.GoString(), *out.RouteTable.RouteTableId, err)
315+
errDel := s.deleteRouteTable(out.RouteTable)
316+
if errDel != nil {
317+
record.Warnf(s.scope.InfraCluster(), "FailedDeleteRouteTable", "Failed to delete managed RouteTable %q: %v", *out.RouteTable.RouteTableId, errDel)
318+
}
307319
return nil, errors.Wrapf(err, "failed to create route in route table %q: %s", *out.RouteTable.RouteTableId, route.GoString())
308320
}
309321
record.Eventf(s.scope.InfraCluster(), "SuccessfulCreateRoute", "Created route %s for RouteTable %q", route.GoString(), *out.RouteTable.RouteTableId)

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

Lines changed: 166 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -519,6 +519,44 @@ func TestReconcileRouteTables(t *testing.T) {
519519
}, nil)
520520
},
521521
},
522+
{
523+
name: "failed to create route, delete route table and fail",
524+
input: &infrav1.NetworkSpec{
525+
VPC: infrav1.VPCSpec{
526+
InternetGatewayID: aws.String("igw-01"),
527+
ID: "vpc-rtbs",
528+
Tags: infrav1.Tags{
529+
infrav1.ClusterTagKey("test-cluster"): "owned",
530+
},
531+
},
532+
Subnets: infrav1.Subnets{
533+
infrav1.SubnetSpec{
534+
ID: "subnet-rtbs-public",
535+
IsPublic: true,
536+
NatGatewayID: aws.String("nat-01"),
537+
AvailabilityZone: "us-east-1a",
538+
},
539+
},
540+
},
541+
expect: func(m *mocks.MockEC2APIMockRecorder) {
542+
m.DescribeRouteTablesWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DescribeRouteTablesInput{})).
543+
Return(&ec2.DescribeRouteTablesOutput{}, nil)
544+
545+
m.CreateRouteTableWithContext(context.TODO(), matchRouteTableInput(&ec2.CreateRouteTableInput{VpcId: aws.String("vpc-rtbs")})).
546+
Return(&ec2.CreateRouteTableOutput{RouteTable: &ec2.RouteTable{RouteTableId: aws.String("rt-1")}}, nil)
547+
548+
m.CreateRouteWithContext(context.TODO(), gomock.Eq(&ec2.CreateRouteInput{
549+
GatewayId: aws.String("igw-01"),
550+
DestinationCidrBlock: aws.String("0.0.0.0/0"),
551+
RouteTableId: aws.String("rt-1"),
552+
})).
553+
Return(nil, awserrors.NewNotFound("MissingParameter"))
554+
555+
m.DeleteRouteTableWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DeleteRouteTableInput{})).
556+
Return(&ec2.DeleteRouteTableOutput{}, nil)
557+
},
558+
err: errors.New(`failed to create route in route table "rt-1"`),
559+
},
522560
}
523561

524562
for _, tc := range testCases {
@@ -560,59 +598,65 @@ func TestReconcileRouteTables(t *testing.T) {
560598
}
561599
}
562600

601+
// Delete Route Table(s).
602+
var (
603+
stubEc2RouteTablePrivate = &ec2.RouteTable{
604+
RouteTableId: aws.String("route-table-private"),
605+
Associations: []*ec2.RouteTableAssociation{
606+
{
607+
SubnetId: nil,
608+
},
609+
},
610+
Routes: []*ec2.Route{
611+
{
612+
DestinationCidrBlock: aws.String("0.0.0.0/0"),
613+
NatGatewayId: aws.String("outdated-nat-01"),
614+
},
615+
},
616+
}
617+
stubEc2RouteTablePublicWithAssociations = &ec2.RouteTable{
618+
RouteTableId: aws.String("route-table-public"),
619+
Associations: []*ec2.RouteTableAssociation{
620+
{
621+
SubnetId: aws.String("subnet-routetables-public"),
622+
RouteTableAssociationId: aws.String("route-table-public"),
623+
},
624+
},
625+
Routes: []*ec2.Route{
626+
{
627+
DestinationCidrBlock: aws.String("0.0.0.0/0"),
628+
GatewayId: aws.String("igw-01"),
629+
},
630+
},
631+
Tags: []*ec2.Tag{
632+
{
633+
Key: aws.String("kubernetes.io/cluster/test-cluster"),
634+
Value: aws.String("owned"),
635+
},
636+
{
637+
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"),
638+
Value: aws.String("common"),
639+
},
640+
{
641+
Key: aws.String("Name"),
642+
Value: aws.String("test-cluster-rt-public-us-east-1a"),
643+
},
644+
{
645+
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"),
646+
Value: aws.String("owned"),
647+
},
648+
},
649+
}
650+
)
651+
563652
func TestDeleteRouteTables(t *testing.T) {
564653
mockCtrl := gomock.NewController(t)
565654
defer mockCtrl.Finish()
566655

567656
describeRouteTableOutput := &ec2.DescribeRouteTablesOutput{
568657
RouteTables: []*ec2.RouteTable{
569-
{
570-
RouteTableId: aws.String("route-table-private"),
571-
Associations: []*ec2.RouteTableAssociation{
572-
{
573-
SubnetId: nil,
574-
},
575-
},
576-
Routes: []*ec2.Route{
577-
{
578-
DestinationCidrBlock: aws.String("0.0.0.0/0"),
579-
NatGatewayId: aws.String("outdated-nat-01"),
580-
},
581-
},
582-
},
583-
{
584-
RouteTableId: aws.String("route-table-public"),
585-
Associations: []*ec2.RouteTableAssociation{
586-
{
587-
SubnetId: aws.String("subnet-routetables-public"),
588-
RouteTableAssociationId: aws.String("route-table-public"),
589-
},
590-
},
591-
Routes: []*ec2.Route{
592-
{
593-
DestinationCidrBlock: aws.String("0.0.0.0/0"),
594-
GatewayId: aws.String("igw-01"),
595-
},
596-
},
597-
Tags: []*ec2.Tag{
598-
{
599-
Key: aws.String("kubernetes.io/cluster/test-cluster"),
600-
Value: aws.String("owned"),
601-
},
602-
{
603-
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"),
604-
Value: aws.String("common"),
605-
},
606-
{
607-
Key: aws.String("Name"),
608-
Value: aws.String("test-cluster-rt-public-us-east-1a"),
609-
},
610-
{
611-
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"),
612-
Value: aws.String("owned"),
613-
},
614-
},
615-
},
658+
stubEc2RouteTablePrivate,
659+
stubEc2RouteTablePublicWithAssociations,
616660
},
617661
}
618662

@@ -730,6 +774,81 @@ func TestDeleteRouteTables(t *testing.T) {
730774
}
731775
}
732776

777+
func TestDeleteRouteTable(t *testing.T) {
778+
mockCtrl := gomock.NewController(t)
779+
defer mockCtrl.Finish()
780+
781+
testCases := []struct {
782+
name string
783+
input *ec2.RouteTable
784+
expect func(m *mocks.MockEC2APIMockRecorder)
785+
wantErr bool
786+
}{
787+
{
788+
name: "Should delete route table successfully",
789+
input: stubEc2RouteTablePrivate,
790+
expect: func(m *mocks.MockEC2APIMockRecorder) {
791+
m.DeleteRouteTableWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DeleteRouteTableInput{})).
792+
Return(&ec2.DeleteRouteTableOutput{}, nil)
793+
},
794+
},
795+
{
796+
name: "Should return error if delete route table fails",
797+
input: stubEc2RouteTablePrivate,
798+
expect: func(m *mocks.MockEC2APIMockRecorder) {
799+
m.DeleteRouteTableWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DeleteRouteTableInput{})).
800+
Return(nil, awserrors.NewNotFound("not found"))
801+
},
802+
wantErr: true,
803+
},
804+
{
805+
name: "Should return error if disassociate route table fails",
806+
input: stubEc2RouteTablePublicWithAssociations,
807+
expect: func(m *mocks.MockEC2APIMockRecorder) {
808+
m.DisassociateRouteTableWithContext(context.TODO(), gomock.Eq(&ec2.DisassociateRouteTableInput{
809+
AssociationId: aws.String("route-table-public"),
810+
})).Return(nil, awserrors.NewNotFound("not found"))
811+
},
812+
wantErr: true,
813+
},
814+
}
815+
816+
for _, tc := range testCases {
817+
t.Run(tc.name, func(t *testing.T) {
818+
g := NewWithT(t)
819+
ec2Mock := mocks.NewMockEC2API(mockCtrl)
820+
821+
scheme := runtime.NewScheme()
822+
_ = infrav1.AddToScheme(scheme)
823+
client := fake.NewClientBuilder().WithScheme(scheme).Build()
824+
scope, err := scope.NewClusterScope(scope.ClusterScopeParams{
825+
Client: client,
826+
Cluster: &clusterv1.Cluster{
827+
ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"},
828+
},
829+
AWSCluster: &infrav1.AWSCluster{
830+
ObjectMeta: metav1.ObjectMeta{Name: "test"},
831+
Spec: infrav1.AWSClusterSpec{},
832+
},
833+
})
834+
g.Expect(err).NotTo(HaveOccurred())
835+
if tc.expect != nil {
836+
tc.expect(ec2Mock.EXPECT())
837+
}
838+
839+
s := NewService(scope)
840+
s.EC2Client = ec2Mock
841+
842+
err = s.deleteRouteTable(tc.input)
843+
if tc.wantErr {
844+
g.Expect(err).To(HaveOccurred())
845+
return
846+
}
847+
g.Expect(err).NotTo(HaveOccurred())
848+
})
849+
}
850+
}
851+
733852
type routeTableInputMatcher struct {
734853
routeTableInput *ec2.CreateRouteTableInput
735854
}

0 commit comments

Comments
 (0)