Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the delete subnet flow #1033

Merged
merged 1 commit into from
Jan 16, 2023
+64 −1
Merged

Fix the delete subnet flow #1033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion cloud/scope/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,11 +312,35 @@ func (s *ClusterScope) ensureSubnetUnique(subnetName string) (*vpcv1.Subnet, err
func (s *ClusterScope) DeleteSubnet() error {
subnetID := *s.IBMVPCCluster.Status.Subnet.ID

// Lists the subnet available and compare before deleting to avoid any failure(404) later
if found, err := func() (bool, error) {
//TODO: Add paging mechanism and use that function everywhere
subnets, _, err := s.IBMVPCClient.ListSubnets(&vpcv1.ListSubnetsOptions{})
if err != nil {
return false, err
}

for _, subnet := range subnets.Subnets {
if *subnet.ID == subnetID {
return true, nil
}
}
return false, nil
}(); err != nil {
return err
} else if !found {
s.Logger.V(3).Info("No subnets found with ID", "Subnet ID", subnetID)
return nil
}

// get the pgw id for given subnet, so we can delete it later
getPGWOptions := &vpcv1.GetSubnetPublicGatewayOptions{}
getPGWOptions.SetID(subnetID)
pgw, _, err := s.IBMVPCClient.GetSubnetPublicGateway(getPGWOptions)
if pgw != nil && err == nil { // public gateway found
if err != nil {
return err
}
if pgw != nil { // public gateway found
// Unset the public gateway for subnet first
err = s.detachPublicGateway(subnetID, *pgw.ID)
if err != nil {
Expand Down
32 changes: 32 additions & 0 deletions cloud/scope/cluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,11 @@ func TestDeleteSubnet(t *testing.T) {
publicGateway := &vpcv1.PublicGateway{
ID: core.StringPtr("foo-public-gateway-id"),
}
subnet := &vpcv1.SubnetCollection{
Subnets: []vpcv1.Subnet{
{ID: core.StringPtr("foo-vpc-subnet-id")},
},
}

t.Run("Should delete subnet", func(t *testing.T) {
g := NewWithT(t)
Expand All @@ -591,6 +596,7 @@ func TestDeleteSubnet(t *testing.T) {
scope := setupClusterScope(clusterName, mockvpc)
scope.IBMVPCCluster.Spec = vpcCluster.Spec
scope.IBMVPCCluster.Status = vpcCluster.Status
mockvpc.EXPECT().ListSubnets(gomock.AssignableToTypeOf(&vpcv1.ListSubnetsOptions{})).Return(subnet, &core.DetailedResponse{}, nil)
mockvpc.EXPECT().GetSubnetPublicGateway(gomock.AssignableToTypeOf(&vpcv1.GetSubnetPublicGatewayOptions{})).Return(publicGateway, &core.DetailedResponse{}, nil)
mockvpc.EXPECT().UnsetSubnetPublicGateway(gomock.AssignableToTypeOf(&vpcv1.UnsetSubnetPublicGatewayOptions{})).Return(&core.DetailedResponse{}, nil)
mockvpc.EXPECT().DeletePublicGateway(gomock.AssignableToTypeOf(&vpcv1.DeletePublicGatewayOptions{})).Return(&core.DetailedResponse{}, nil)
Expand All @@ -606,6 +612,7 @@ func TestDeleteSubnet(t *testing.T) {
scope := setupClusterScope(clusterName, mockvpc)
scope.IBMVPCCluster.Spec = vpcCluster.Spec
scope.IBMVPCCluster.Status = vpcCluster.Status
mockvpc.EXPECT().ListSubnets(gomock.AssignableToTypeOf(&vpcv1.ListSubnetsOptions{})).Return(subnet, &core.DetailedResponse{}, nil)
mockvpc.EXPECT().GetSubnetPublicGateway(gomock.AssignableToTypeOf(&vpcv1.GetSubnetPublicGatewayOptions{})).Return(publicGateway, &core.DetailedResponse{}, nil)
mockvpc.EXPECT().UnsetSubnetPublicGateway(gomock.AssignableToTypeOf(&vpcv1.UnsetSubnetPublicGatewayOptions{})).Return(&core.DetailedResponse{}, errors.New("Error when unsetting publicgateway for subnet"))
err := scope.DeleteSubnet()
Expand All @@ -619,6 +626,7 @@ func TestDeleteSubnet(t *testing.T) {
scope := setupClusterScope(clusterName, mockvpc)
scope.IBMVPCCluster.Spec = vpcCluster.Spec
scope.IBMVPCCluster.Status = vpcCluster.Status
mockvpc.EXPECT().ListSubnets(gomock.AssignableToTypeOf(&vpcv1.ListSubnetsOptions{})).Return(subnet, &core.DetailedResponse{}, nil)
mockvpc.EXPECT().GetSubnetPublicGateway(gomock.AssignableToTypeOf(&vpcv1.GetSubnetPublicGatewayOptions{})).Return(publicGateway, &core.DetailedResponse{}, nil)
mockvpc.EXPECT().UnsetSubnetPublicGateway(gomock.AssignableToTypeOf(&vpcv1.UnsetSubnetPublicGatewayOptions{})).Return(&core.DetailedResponse{}, nil)
mockvpc.EXPECT().DeletePublicGateway(gomock.AssignableToTypeOf(&vpcv1.DeletePublicGatewayOptions{})).Return(&core.DetailedResponse{}, errors.New("Error when deleting publicgateway for subnet"))
Expand All @@ -633,13 +641,37 @@ func TestDeleteSubnet(t *testing.T) {
scope := setupClusterScope(clusterName, mockvpc)
scope.IBMVPCCluster.Spec = vpcCluster.Spec
scope.IBMVPCCluster.Status = vpcCluster.Status
mockvpc.EXPECT().ListSubnets(gomock.AssignableToTypeOf(&vpcv1.ListSubnetsOptions{})).Return(subnet, &core.DetailedResponse{}, nil)
mockvpc.EXPECT().GetSubnetPublicGateway(gomock.AssignableToTypeOf(&vpcv1.GetSubnetPublicGatewayOptions{})).Return(publicGateway, &core.DetailedResponse{}, nil)
mockvpc.EXPECT().UnsetSubnetPublicGateway(gomock.AssignableToTypeOf(&vpcv1.UnsetSubnetPublicGatewayOptions{})).Return(&core.DetailedResponse{}, nil)
mockvpc.EXPECT().DeletePublicGateway(gomock.AssignableToTypeOf(&vpcv1.DeletePublicGatewayOptions{})).Return(&core.DetailedResponse{}, nil)
mockvpc.EXPECT().DeleteSubnet(gomock.AssignableToTypeOf(&vpcv1.DeleteSubnetOptions{})).Return(&core.DetailedResponse{}, errors.New("Error when deleting subnet"))
err := scope.DeleteSubnet()
g.Expect(err).To(Not(BeNil()))
})

t.Run("Error listing subnets", func(t *testing.T) {
g := NewWithT(t)
mockController, mockvpc := setup(t)
t.Cleanup(mockController.Finish)
scope := setupClusterScope(clusterName, mockvpc)
scope.IBMVPCCluster.Spec = vpcCluster.Spec
scope.IBMVPCCluster.Status = vpcCluster.Status
mockvpc.EXPECT().ListSubnets(gomock.AssignableToTypeOf(&vpcv1.ListSubnetsOptions{})).Return(nil, &core.DetailedResponse{}, errors.New("Error listing subnets"))
err := scope.DeleteSubnet()
g.Expect(err).To(Not(BeNil()))
})
t.Run("Subnet doesn't exist", func(t *testing.T) {
g := NewWithT(t)
mockController, mockvpc := setup(t)
t.Cleanup(mockController.Finish)
scope := setupClusterScope(clusterName, mockvpc)
scope.IBMVPCCluster.Spec = vpcCluster.Spec
scope.IBMVPCCluster.Status = vpcCluster.Status
mockvpc.EXPECT().ListSubnets(gomock.AssignableToTypeOf(&vpcv1.ListSubnetsOptions{})).Return(&vpcv1.SubnetCollection{Subnets: []vpcv1.Subnet{}}, &core.DetailedResponse{}, nil)
err := scope.DeleteSubnet()
g.Expect(err).To(BeNil())
})
})
}

Expand Down
7 changes: 7 additions & 0 deletions controllers/ibmvpccluster_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,7 @@ func TestIBMVPCClusterReconciler_delete(t *testing.T) {
g.Expect(clusterScope.IBMVPCCluster.Finalizers).To(ContainElement(infrav1beta2.ClusterFinalizer))
})
getPGWOptions := &vpcv1.GetSubnetPublicGatewayOptions{ID: pointer.String("capi-subnet-id")}
subnet := &vpcv1.SubnetCollection{Subnets: []vpcv1.Subnet{{ID: core.StringPtr("capi-subnet-id")}}}
pgw := &vpcv1.PublicGateway{ID: pointer.String("capi-pgw-id")}
unsetPGWOptions := &vpcv1.UnsetSubnetPublicGatewayOptions{ID: pointer.String("capi-subnet-id")}
deleteSubnetOptions := &vpcv1.DeleteSubnetOptions{ID: pointer.String("capi-subnet-id")}
Expand All @@ -493,6 +494,7 @@ func TestIBMVPCClusterReconciler_delete(t *testing.T) {
setup(t)
t.Cleanup(teardown)
mockvpc.EXPECT().ListInstances(listVSIOpts).Return(instancelist, response, nil)
mockvpc.EXPECT().ListSubnets(&vpcv1.ListSubnetsOptions{}).Return(subnet, response, nil)
mockvpc.EXPECT().GetSubnetPublicGateway(getPGWOptions).Return(pgw, response, nil)
mockvpc.EXPECT().UnsetSubnetPublicGateway(unsetPGWOptions).Return(response, nil)
mockvpc.EXPECT().DeletePublicGateway(deletePGWOptions).Return(response, nil)
Expand All @@ -507,6 +509,7 @@ func TestIBMVPCClusterReconciler_delete(t *testing.T) {
setup(t)
t.Cleanup(teardown)
mockvpc.EXPECT().ListInstances(listVSIOpts).Return(instancelist, response, nil)
mockvpc.EXPECT().ListSubnets(&vpcv1.ListSubnetsOptions{}).Return(subnet, response, nil)
mockvpc.EXPECT().GetSubnetPublicGateway(getPGWOptions).Return(pgw, response, nil)
mockvpc.EXPECT().UnsetSubnetPublicGateway(unsetPGWOptions).Return(response, nil)
mockvpc.EXPECT().DeletePublicGateway(deletePGWOptions).Return(response, nil)
Expand All @@ -522,6 +525,7 @@ func TestIBMVPCClusterReconciler_delete(t *testing.T) {
setup(t)
t.Cleanup(teardown)
mockvpc.EXPECT().ListInstances(listVSIOpts).Return(instancelist, response, nil)
mockvpc.EXPECT().ListSubnets(&vpcv1.ListSubnetsOptions{}).Return(subnet, response, nil)
mockvpc.EXPECT().GetSubnetPublicGateway(getPGWOptions).Return(pgw, response, nil)
mockvpc.EXPECT().UnsetSubnetPublicGateway(unsetPGWOptions).Return(response, nil)
mockvpc.EXPECT().DeletePublicGateway(deletePGWOptions).Return(response, nil)
Expand All @@ -537,6 +541,7 @@ func TestIBMVPCClusterReconciler_delete(t *testing.T) {
setup(t)
t.Cleanup(teardown)
mockvpc.EXPECT().ListInstances(listVSIOpts).Return(instancelist, response, nil)
mockvpc.EXPECT().ListSubnets(&vpcv1.ListSubnetsOptions{}).Return(subnet, response, nil)
mockvpc.EXPECT().GetSubnetPublicGateway(getPGWOptions).Return(pgw, response, nil)
mockvpc.EXPECT().UnsetSubnetPublicGateway(unsetPGWOptions).Return(response, nil)
mockvpc.EXPECT().DeletePublicGateway(deletePGWOptions).Return(response, nil)
Expand Down Expand Up @@ -620,12 +625,14 @@ func TestIBMVPCClusterLBReconciler_delete(t *testing.T) {
g.Expect(clusterScope.IBMVPCCluster.Finalizers).To(ContainElement(infrav1beta2.ClusterFinalizer))
})
t.Run("Should successfully delete IBMVPCCluster and remove the finalizer", func(t *testing.T) {
subnet := &vpcv1.SubnetCollection{Subnets: []vpcv1.Subnet{{ID: core.StringPtr("capi-subnet-id")}}}
g := NewWithT(t)
mockController, mockvpc, clusterScope, reconciler := setup(t)
t.Cleanup(mockController.Finish)
pgw := &vpcv1.PublicGateway{ID: pointer.String("capi-pgw-id")}
mockvpc.EXPECT().ListInstances(gomock.AssignableToTypeOf(&vpcv1.ListInstancesOptions{})).Return(instancelist, &core.DetailedResponse{}, nil)
mockvpc.EXPECT().ListLoadBalancers(gomock.AssignableToTypeOf(&vpcv1.ListLoadBalancersOptions{})).Return(&vpcv1.LoadBalancerCollection{}, &core.DetailedResponse{}, nil)
mockvpc.EXPECT().ListSubnets(gomock.AssignableToTypeOf(&vpcv1.ListSubnetsOptions{})).Return(subnet, &core.DetailedResponse{}, nil)
mockvpc.EXPECT().GetSubnetPublicGateway(gomock.AssignableToTypeOf(&vpcv1.GetSubnetPublicGatewayOptions{})).Return(pgw, &core.DetailedResponse{}, nil)
mockvpc.EXPECT().UnsetSubnetPublicGateway(gomock.AssignableToTypeOf(&vpcv1.UnsetSubnetPublicGatewayOptions{})).Return(&core.DetailedResponse{}, nil)
mockvpc.EXPECT().DeletePublicGateway(gomock.AssignableToTypeOf(&vpcv1.DeletePublicGatewayOptions{})).Return(&core.DetailedResponse{}, nil)
Expand Down