Skip to content

Commit

Permalink
Merge pull request #4912 from k8s-infra-cherrypick-robot/cherry-pick-…
Browse files Browse the repository at this point in the history 
…4899-to-release-2.4

[release-2.4] 🐛 fix/network/rtb: delete rtb handling err when failed to create routes
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Apr 8, 2024
2 parents e670328 + 51af40f commit 2718206
Show file tree
Hide file tree
Showing 2 changed files with 199 additions and 68 deletions.
54 changes: 33 additions & 21 deletions pkg/cloud/services/network/routetables.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,32 @@ func (s *Service) describeVpcRouteTablesBySubnet() (map[string]*ec2.RouteTable,
return res, nil
}

func (s *Service) deleteRouteTable(rt *ec2.RouteTable) error {
for _, as := range rt.Associations {
if as.SubnetId == nil {
continue
}

if _, err := s.EC2Client.DisassociateRouteTableWithContext(context.TODO(), &ec2.DisassociateRouteTableInput{AssociationId: as.RouteTableAssociationId}); err != nil {
record.Warnf(s.scope.InfraCluster(), "FailedDisassociateRouteTable", "Failed to disassociate managed RouteTable %q from Subnet %q: %v", *rt.RouteTableId, *as.SubnetId, err)
return errors.Wrapf(err, "failed to disassociate route table %q from subnet %q", *rt.RouteTableId, *as.SubnetId)
}

record.Eventf(s.scope.InfraCluster(), "SuccessfulDisassociateRouteTable", "Disassociated managed RouteTable %q from subnet %q", *rt.RouteTableId, *as.SubnetId)
s.scope.Debug("Deleted association between route table and subnet", "route-table-id", *rt.RouteTableId, "subnet-id", *as.SubnetId)
}

if _, err := s.EC2Client.DeleteRouteTableWithContext(context.TODO(), &ec2.DeleteRouteTableInput{RouteTableId: rt.RouteTableId}); err != nil {
record.Warnf(s.scope.InfraCluster(), "FailedDeleteRouteTable", "Failed to delete managed RouteTable %q: %v", *rt.RouteTableId, err)
return errors.Wrapf(err, "failed to delete route table %q", *rt.RouteTableId)
}

record.Eventf(s.scope.InfraCluster(), "SuccessfulDeleteRouteTable", "Deleted managed RouteTable %q", *rt.RouteTableId)
s.scope.Info("Deleted route table", "route-table-id", *rt.RouteTableId)

return nil
}

func (s *Service) deleteRouteTables() error {
if s.scope.VPC().IsUnmanaged(s.scope.Name()) {
s.scope.Trace("Skipping routing tables deletion in unmanaged mode")
Expand All @@ -226,27 +252,10 @@ func (s *Service) deleteRouteTables() error {
}

for _, rt := range rts {
for _, as := range rt.Associations {
if as.SubnetId == nil {
continue
}

if _, err := s.EC2Client.DisassociateRouteTableWithContext(context.TODO(), &ec2.DisassociateRouteTableInput{AssociationId: as.RouteTableAssociationId}); err != nil {
record.Warnf(s.scope.InfraCluster(), "FailedDisassociateRouteTable", "Failed to disassociate managed RouteTable %q from Subnet %q: %v", *rt.RouteTableId, *as.SubnetId, err)
return errors.Wrapf(err, "failed to disassociate route table %q from subnet %q", *rt.RouteTableId, *as.SubnetId)
}

record.Eventf(s.scope.InfraCluster(), "SuccessfulDisassociateRouteTable", "Disassociated managed RouteTable %q from subnet %q", *rt.RouteTableId, *as.SubnetId)
s.scope.Debug("Deleted association between route table and subnet", "route-table-id", *rt.RouteTableId, "subnet-id", *as.SubnetId)
}

if _, err := s.EC2Client.DeleteRouteTableWithContext(context.TODO(), &ec2.DeleteRouteTableInput{RouteTableId: rt.RouteTableId}); err != nil {
record.Warnf(s.scope.InfraCluster(), "FailedDeleteRouteTable", "Failed to delete managed RouteTable %q: %v", *rt.RouteTableId, err)
return errors.Wrapf(err, "failed to delete route table %q", *rt.RouteTableId)
err := s.deleteRouteTable(rt)
if err != nil {
return err
}

record.Eventf(s.scope.InfraCluster(), "SuccessfulDeleteRouteTable", "Deleted managed RouteTable %q", *rt.RouteTableId)
s.scope.Info("Deleted route table", "route-table-id", *rt.RouteTableId)
}
return nil
}
Expand Down Expand Up @@ -302,8 +311,11 @@ func (s *Service) createRouteTableWithRoutes(routes []*ec2.Route, isPublic bool,
}
return true, nil
}, awserrors.RouteTableNotFound, awserrors.NATGatewayNotFound, awserrors.GatewayNotFound); err != nil {
// TODO(vincepri): cleanup the route table if this fails.
record.Warnf(s.scope.InfraCluster(), "FailedCreateRoute", "Failed to create route %s for RouteTable %q: %v", route.GoString(), *out.RouteTable.RouteTableId, err)
errDel := s.deleteRouteTable(out.RouteTable)
if errDel != nil {
record.Warnf(s.scope.InfraCluster(), "FailedDeleteRouteTable", "Failed to delete managed RouteTable %q: %v", *out.RouteTable.RouteTableId, errDel)
}
return nil, errors.Wrapf(err, "failed to create route in route table %q: %s", *out.RouteTable.RouteTableId, route.GoString())
}
record.Eventf(s.scope.InfraCluster(), "SuccessfulCreateRoute", "Created route %s for RouteTable %q", route.GoString(), *out.RouteTable.RouteTableId)
Expand Down
213 changes: 166 additions & 47 deletions pkg/cloud/services/network/routetables_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,44 @@ func TestReconcileRouteTables(t *testing.T) {
}, nil)
},
},
{
name: "failed to create route, delete route table and fail",
input: &infrav1.NetworkSpec{
VPC: infrav1.VPCSpec{
InternetGatewayID: aws.String("igw-01"),
ID: "vpc-rtbs",
Tags: infrav1.Tags{
infrav1.ClusterTagKey("test-cluster"): "owned",
},
},
Subnets: infrav1.Subnets{
infrav1.SubnetSpec{
ID: "subnet-rtbs-public",
IsPublic: true,
NatGatewayID: aws.String("nat-01"),
AvailabilityZone: "us-east-1a",
},
},
},
expect: func(m *mocks.MockEC2APIMockRecorder) {
m.DescribeRouteTablesWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DescribeRouteTablesInput{})).
Return(&ec2.DescribeRouteTablesOutput{}, nil)

m.CreateRouteTableWithContext(context.TODO(), matchRouteTableInput(&ec2.CreateRouteTableInput{VpcId: aws.String("vpc-rtbs")})).
Return(&ec2.CreateRouteTableOutput{RouteTable: &ec2.RouteTable{RouteTableId: aws.String("rt-1")}}, nil)

m.CreateRouteWithContext(context.TODO(), gomock.Eq(&ec2.CreateRouteInput{
GatewayId: aws.String("igw-01"),
DestinationCidrBlock: aws.String("0.0.0.0/0"),
RouteTableId: aws.String("rt-1"),
})).
Return(nil, awserrors.NewNotFound("MissingParameter"))

m.DeleteRouteTableWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DeleteRouteTableInput{})).
Return(&ec2.DeleteRouteTableOutput{}, nil)
},
err: errors.New(`failed to create route in route table "rt-1"`),
},
}

for _, tc := range testCases {
Expand Down Expand Up @@ -560,59 +598,65 @@ func TestReconcileRouteTables(t *testing.T) {
}
}

// Delete Route Table(s).
var (
stubEc2RouteTablePrivate = &ec2.RouteTable{
RouteTableId: aws.String("route-table-private"),
Associations: []*ec2.RouteTableAssociation{
{
SubnetId: nil,
},
},
Routes: []*ec2.Route{
{
DestinationCidrBlock: aws.String("0.0.0.0/0"),
NatGatewayId: aws.String("outdated-nat-01"),
},
},
}
stubEc2RouteTablePublicWithAssociations = &ec2.RouteTable{
RouteTableId: aws.String("route-table-public"),
Associations: []*ec2.RouteTableAssociation{
{
SubnetId: aws.String("subnet-routetables-public"),
RouteTableAssociationId: aws.String("route-table-public"),
},
},
Routes: []*ec2.Route{
{
DestinationCidrBlock: aws.String("0.0.0.0/0"),
GatewayId: aws.String("igw-01"),
},
},
Tags: []*ec2.Tag{
{
Key: aws.String("kubernetes.io/cluster/test-cluster"),
Value: aws.String("owned"),
},
{
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"),
Value: aws.String("common"),
},
{
Key: aws.String("Name"),
Value: aws.String("test-cluster-rt-public-us-east-1a"),
},
{
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"),
Value: aws.String("owned"),
},
},
}
)

func TestDeleteRouteTables(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()

describeRouteTableOutput := &ec2.DescribeRouteTablesOutput{
RouteTables: []*ec2.RouteTable{
{
RouteTableId: aws.String("route-table-private"),
Associations: []*ec2.RouteTableAssociation{
{
SubnetId: nil,
},
},
Routes: []*ec2.Route{
{
DestinationCidrBlock: aws.String("0.0.0.0/0"),
NatGatewayId: aws.String("outdated-nat-01"),
},
},
},
{
RouteTableId: aws.String("route-table-public"),
Associations: []*ec2.RouteTableAssociation{
{
SubnetId: aws.String("subnet-routetables-public"),
RouteTableAssociationId: aws.String("route-table-public"),
},
},
Routes: []*ec2.Route{
{
DestinationCidrBlock: aws.String("0.0.0.0/0"),
GatewayId: aws.String("igw-01"),
},
},
Tags: []*ec2.Tag{
{
Key: aws.String("kubernetes.io/cluster/test-cluster"),
Value: aws.String("owned"),
},
{
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"),
Value: aws.String("common"),
},
{
Key: aws.String("Name"),
Value: aws.String("test-cluster-rt-public-us-east-1a"),
},
{
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"),
Value: aws.String("owned"),
},
},
},
stubEc2RouteTablePrivate,
stubEc2RouteTablePublicWithAssociations,
},
}

Expand Down Expand Up @@ -730,6 +774,81 @@ func TestDeleteRouteTables(t *testing.T) {
}
}

func TestDeleteRouteTable(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()

testCases := []struct {
name string
input *ec2.RouteTable
expect func(m *mocks.MockEC2APIMockRecorder)
wantErr bool
}{
{
name: "Should delete route table successfully",
input: stubEc2RouteTablePrivate,
expect: func(m *mocks.MockEC2APIMockRecorder) {
m.DeleteRouteTableWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DeleteRouteTableInput{})).
Return(&ec2.DeleteRouteTableOutput{}, nil)
},
},
{
name: "Should return error if delete route table fails",
input: stubEc2RouteTablePrivate,
expect: func(m *mocks.MockEC2APIMockRecorder) {
m.DeleteRouteTableWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DeleteRouteTableInput{})).
Return(nil, awserrors.NewNotFound("not found"))
},
wantErr: true,
},
{
name: "Should return error if disassociate route table fails",
input: stubEc2RouteTablePublicWithAssociations,
expect: func(m *mocks.MockEC2APIMockRecorder) {
m.DisassociateRouteTableWithContext(context.TODO(), gomock.Eq(&ec2.DisassociateRouteTableInput{
AssociationId: aws.String("route-table-public"),
})).Return(nil, awserrors.NewNotFound("not found"))
},
wantErr: true,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)
ec2Mock := mocks.NewMockEC2API(mockCtrl)

scheme := runtime.NewScheme()
_ = infrav1.AddToScheme(scheme)
client := fake.NewClientBuilder().WithScheme(scheme).Build()
scope, err := scope.NewClusterScope(scope.ClusterScopeParams{
Client: client,
Cluster: &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"},
},
AWSCluster: &infrav1.AWSCluster{
ObjectMeta: metav1.ObjectMeta{Name: "test"},
Spec: infrav1.AWSClusterSpec{},
},
})
g.Expect(err).NotTo(HaveOccurred())
if tc.expect != nil {
tc.expect(ec2Mock.EXPECT())
}

s := NewService(scope)
s.EC2Client = ec2Mock

err = s.deleteRouteTable(tc.input)
if tc.wantErr {
g.Expect(err).To(HaveOccurred())
return
}
g.Expect(err).NotTo(HaveOccurred())
})
}
}

type routeTableInputMatcher struct {
routeTableInput *ec2.CreateRouteTableInput
}
Expand Down

0 comments on commit 2718206

Please sign in to comment.