Skip to content

Commit

Permalink
Fix the issue that NSX VPC not deleted when the Namespace is terminat…
Browse files Browse the repository at this point in the history 
…ing (#948) (#969)

This change is to fix an issue with the NetworkInfo deletion logic that NSX VPC
is not removed if the CR's Namesapce still exists.

The fix is to add a wather on Namespace deletion event and ensure the NSX VPC is
deleted when the K8s Namespace is deleted
  • Loading branch information
@wenyingd
wenyingd authored Dec 13, 2024
1 parent c36fd71 commit 1cac51a
Show file tree
Hide file tree
Showing 4 changed files with 143 additions and 140 deletions.
69 changes: 23 additions & 46 deletions pkg/controllers/networkinfo/networkinfo_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ func (r *NetworkInfoReconciler) Reconcile(ctx context.Context, req ctrl.Request)
networkInfoCR := &v1alpha1.NetworkInfo{}
if err := r.Client.Get(ctx, req.NamespacedName, networkInfoCR); err != nil {
if apierrors.IsNotFound(err) {
if err := r.deleteVPCsByName(ctx, req.Namespace); err != nil {
if err := r.deleteVPCsByNamespace(ctx, req.Namespace); err != nil {
r.StatusUpdater.DeleteFail(req.NamespacedName, nil, err)
return common.ResultRequeue, err
}
Expand All @@ -150,7 +150,7 @@ func (r *NetworkInfoReconciler) Reconcile(ctx context.Context, req ctrl.Request)
// Check if the CR is marked for deletion
if !networkInfoCR.ObjectMeta.DeletionTimestamp.IsZero() {
r.StatusUpdater.IncreaseDeleteTotal()
if err := r.deleteVPCsByID(ctx, networkInfoCR.GetNamespace(), string(networkInfoCR.UID)); err != nil {
if err := r.deleteVPCsByNamespace(ctx, networkInfoCR.GetNamespace()); err != nil {
r.StatusUpdater.DeleteFail(req.NamespacedName, nil, err)
return common.ResultRequeue, err
}
Expand Down Expand Up @@ -408,8 +408,11 @@ func (r *NetworkInfoReconciler) listNamespaceCRsNameIDSet(ctx context.Context) (
nsSet := sets.Set[string]{}
idSet := sets.Set[string]{}
for _, ns := range namespaces.Items {
nsSet.Insert(ns.Name)
idSet.Insert(string(ns.UID))
// Ignore the terminating Namespaces in the list results.
if ns.ObjectMeta.DeletionTimestamp.IsZero() {
nsSet.Insert(ns.Name)
idSet.Insert(string(ns.UID))
}
}
return nsSet, idSet, nil
}
Expand Down Expand Up @@ -459,36 +462,18 @@ func (r *NetworkInfoReconciler) CollectGarbage(ctx context.Context) {
}
}

func (r *NetworkInfoReconciler) fetchStaleVPCsByNamespace(ctx context.Context, ns string) ([]*model.Vpc, error) {
isShared, err := r.Service.IsSharedVPCNamespaceByNS(ctx, ns)
if err != nil {
if apierrors.IsNotFound(err) {
// if the Namespace has been deleted, we never know whether it`s a shared Namespace, The GC will delete the stale VPCs
log.Info("Namespace does not exist while fetching stale VPCs", "Namespace", ns)
return nil, nil
}
return nil, fmt.Errorf("failed to check if Namespace is shared for NS %s: %w", ns, err)
}
if isShared {
log.Info("Shared Namespace, skipping deletion of NSX VPC", "Namespace", ns)
return nil, nil
func (r *NetworkInfoReconciler) deleteVPCsByNamespace(ctx context.Context, ns string) error {
staleVPCs := r.Service.GetVPCsByNamespace(ns)
if len(staleVPCs) == 0 {
return nil
}

return r.Service.GetVPCsByNamespace(ns), nil
}

func (r *NetworkInfoReconciler) deleteVPCsByName(ctx context.Context, ns string) error {
_, idSet, err := r.listNamespaceCRsNameIDSet(ctx)
if err != nil {
log.Error(err, "Failed to list Kubernetes Namespaces")
return fmt.Errorf("failed to list Kubernetes Namespaces while deleting VPCs: %v", err)
}

staleVPCs, err := r.fetchStaleVPCsByNamespace(ctx, ns)
if err != nil {
return err
}

var vpcToDelete []*model.Vpc
for _, nsxVPC := range staleVPCs {
namespaceIDofVPC := filterTagFromNSXVPC(nsxVPC, commonservice.TagScopeNamespaceUID)
Expand All @@ -501,22 +486,6 @@ func (r *NetworkInfoReconciler) deleteVPCsByName(ctx context.Context, ns string)
return r.deleteVPCs(ctx, vpcToDelete, ns)
}

func (r *NetworkInfoReconciler) deleteVPCsByID(ctx context.Context, ns, id string) error {
staleVPCs, err := r.fetchStaleVPCsByNamespace(ctx, ns)
if err != nil {
return err
}

var vpcToDelete []*model.Vpc
for _, nsxVPC := range staleVPCs {
namespaceIDofVPC := filterTagFromNSXVPC(nsxVPC, commonservice.TagScopeNamespaceUID)
if namespaceIDofVPC == id {
vpcToDelete = append(vpcToDelete, nsxVPC)
}
}
return r.deleteVPCs(ctx, vpcToDelete, ns)
}

func (r *NetworkInfoReconciler) deleteVPCs(ctx context.Context, staleVPCs []*model.Vpc, ns string) error {
if len(staleVPCs) == 0 {
log.Info("There is no VPCs found in store, skipping deletion of NSX VPC", "Namespace", ns)
Expand All @@ -538,14 +507,22 @@ func (r *NetworkInfoReconciler) deleteVPCs(ctx context.Context, staleVPCs []*mod
}

// Update the VPCNetworkConfiguration Status
ncName, err := r.Service.GetNetworkconfigNameFromNS(ctx, ns)
if err != nil {
return fmt.Errorf("failed to get VPCNetworkConfiguration for Namespace when deleting stale VPCs %s: %w", ns, err)
vpcNetConfig := r.Service.GetVPCNetworkConfigByNamespace(ns)
if vpcNetConfig != nil {
updateVPCNetworkConfigurationStatusWithAliveVPCs(ctx, r.Client, vpcNetConfig.Name, r.listVPCsByNetworkConfigName)
}
deleteVPCNetworkConfigurationStatus(ctx, r.Client, ncName, staleVPCs, r.Service.ListVPC())
return nil
}

func (r *NetworkInfoReconciler) listVPCsByNetworkConfigName(ncName string) []*model.Vpc {
namespacesUsingNC := r.Service.GetNamespacesByNetworkconfigName(ncName)
aliveVPCs := make([]*model.Vpc, 0)
for _, namespace := range namespacesUsingNC {
aliveVPCs = append(aliveVPCs, r.Service.GetVPCsByNamespace(namespace)...)
}
return aliveVPCs
}

func (r *NetworkInfoReconciler) syncPreCreatedVpcIPs(ctx context.Context) {
// Construct a map for the existing NetworkInfo CRs, the key is its Namespace, and the value is
// the NetworkInfo CR.
Expand Down
174 changes: 101 additions & 73 deletions pkg/controllers/networkinfo/networkinfo_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apimachinery/pkg/util/sets"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/util/workqueue"
controllerruntime "sigs.k8s.io/controller-runtime"
Expand Down Expand Up @@ -154,20 +155,8 @@ func TestNetworkInfoReconciler_Reconcile(t *testing.T) {
{
name: "Empty",
prepareFunc: func(t *testing.T, r *NetworkInfoReconciler, ctx context.Context) (patches *gomonkey.Patches) {
patches = gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return false, nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetNetworkconfigNameFromNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (string, error) {
return "", nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "ListVPC", func(_ *vpc.VPCService) []model.Vpc {
return nil
})
patches.ApplyFunc(deleteVPCNetworkConfigurationStatus, func(ctx context.Context, client client.Client, ncName string, staleVPCs []*model.Vpc, aliveVPCs []model.Vpc) {
return
patches = gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return []*model.Vpc{}
})
return patches
},
Expand Down Expand Up @@ -779,35 +768,41 @@ func TestNetworkInfoReconciler_deleteStaleVPCs(t *testing.T) {
expectErrStr string
}{
{
name: "shared namespace, skip deletion",
name: "no VPC exists for the Namespace,, skip deletion",
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return true, nil
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return []*model.Vpc{}
})
return patches
},
},
{
name: "non-shared namespace, no VPCs found",
}, {
name: "NSX VPC is used by a valid Namespace,, skip deletion",
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return false, nil
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return []*model.Vpc{
{Tags: []model.Tag{
{Scope: servicecommon.String(servicecommon.TagScopeNamespaceUID), Tag: servicecommon.String("vpc1")}}}}
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return nil
patches.ApplyPrivateMethod(reflect.TypeOf(r), "listNamespaceCRsNameIDSet", func(_ *NetworkInfoReconciler, _ context.Context) (sets.Set[string], sets.Set[string], error) {
return sets.Set[string]{}, sets.New[string]("vpc1"), nil
})
return patches
},
},
{
name: "failed to delete VPC",
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return false, nil
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return []*model.Vpc{
{
Tags: []model.Tag{
{Scope: servicecommon.String(servicecommon.TagScopeNamespaceUID), Tag: servicecommon.String("vpc1")},
},
Path: servicecommon.String("/orgs/default/projects/default/vpcs/vpc1"),
}}
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
vpcPath := "/vpc/1"
return []*model.Vpc{{Path: &vpcPath}}
patches.ApplyPrivateMethod(reflect.TypeOf(r), "listNamespaceCRsNameIDSet", func(_ *NetworkInfoReconciler, _ context.Context) (sets.Set[string], sets.Set[string], error) {
return sets.Set[string]{}, sets.Set[string]{}, nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "DeleteVPC", func(_ *vpc.VPCService, _ string) error {
return fmt.Errorf("delete failed")
Expand All @@ -819,26 +814,23 @@ func TestNetworkInfoReconciler_deleteStaleVPCs(t *testing.T) {
{
name: "successful deletion of VPCs",
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return false, nil
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return []*model.Vpc{
{
Tags: []model.Tag{
{Scope: servicecommon.String(servicecommon.TagScopeNamespaceUID), Tag: servicecommon.String("vpc1")},
},
Path: servicecommon.String("/orgs/default/projects/default/vpcs/vpc1"),
}}
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
vpcPath1 := "/vpc/1"
vpcPath2 := "/vpc/2"
displayName1 := "fakeDisplayName"
displayName2 := "fakeDisplayName"
return []*model.Vpc{{Path: &vpcPath1, DisplayName: &displayName1}, {Path: &vpcPath2, DisplayName: &displayName2}}
patches.ApplyPrivateMethod(reflect.TypeOf(r), "listNamespaceCRsNameIDSet", func(_ *NetworkInfoReconciler, _ context.Context) (sets.Set[string], sets.Set[string], error) {
return sets.Set[string]{}, sets.Set[string]{}, nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "DeleteVPC", func(_ *vpc.VPCService, _ string) error {
return nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "ListVPC", func(_ *vpc.VPCService) []model.Vpc {
vpcPath := "/vpc/1"
displayName := "fakeDisplayName"
return []model.Vpc{{Path: &vpcPath, DisplayName: &displayName}}
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetNetworkconfigNameFromNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (string, error) {
return "", nil
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCNetworkConfigByNamespace", func(_ *vpc.VPCService, _ string) *servicecommon.VPCNetworkConfigInfo {
return nil
})
return patches
},
Expand All @@ -860,26 +852,37 @@ func TestNetworkInfoReconciler_deleteStaleVPCs(t *testing.T) {
},
},
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return false, nil
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return []*model.Vpc{
{
Tags: []model.Tag{
{Scope: servicecommon.String(servicecommon.TagScopeNamespaceUID), Tag: servicecommon.String("vpc1")},
},
Path: servicecommon.String("/orgs/default/projects/default/vpcs/vpc1"),
DisplayName: servicecommon.String("fakeDisplayName1"),
}}
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
vpcPath1 := "/vpc/1"
vpcPath2 := "/vpc/2"
displayName1 := "fakeDisplayName1"
displayName2 := "fakeDisplayName2"
return []*model.Vpc{{Path: &vpcPath1, DisplayName: &displayName1}, {Path: &vpcPath2, DisplayName: &displayName2}}
patches.ApplyPrivateMethod(reflect.TypeOf(r), "listNamespaceCRsNameIDSet", func(_ *NetworkInfoReconciler, _ context.Context) (sets.Set[string], sets.Set[string], error) {
return sets.Set[string]{}, sets.Set[string]{}, nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "DeleteVPC", func(_ *vpc.VPCService, _ string) error {
return nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "ListVPC", func(_ *vpc.VPCService) []model.Vpc {
vpcPath := "/vpc/1"
displayName := "fakeDisplayName1"
return []model.Vpc{{Path: &vpcPath, DisplayName: &displayName}}
patches.ApplyPrivateMethod(reflect.TypeOf(r), "listVPCsByNetworkConfigName", func(_ *NetworkInfoReconciler, _ string) []*model.Vpc {
return []*model.Vpc{
{
Tags: []model.Tag{
{Scope: servicecommon.String(servicecommon.TagScopeNamespaceUID), Tag: servicecommon.String("vpc1")},
},
Path: servicecommon.String("/orgs/default/projects/default/vpcs/vpc2"),
DisplayName: servicecommon.String("fakeDisplayName2"),
},
}
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetNetworkconfigNameFromNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (string, error) {
return "fakeNetworkconfigName", nil
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCNetworkConfigByNamespace", func(_ *vpc.VPCService, _ string) *servicecommon.VPCNetworkConfigInfo {
return &servicecommon.VPCNetworkConfigInfo{
Name: "fakeNetworkconfigName",
}
})
return patches
},
Expand All @@ -902,7 +905,7 @@ func TestNetworkInfoReconciler_deleteStaleVPCs(t *testing.T) {
defer patches.Reset()
}

err := r.deleteVPCsByName(ctx, namespace)
err := r.deleteVPCsByNamespace(ctx, namespace)

if tc.expectErrStr != "" {
assert.ErrorContains(t, err, tc.expectErrStr)
Expand All @@ -926,28 +929,30 @@ func TestNetworkInfoReconciler_DeleteNetworkInfo(t *testing.T) {
{
name: "Delete NetworkInfo and Namespace not existed",
existingNamespace: nil,
expectErrStr: "",
expectRes: common.ResultNormal,
req: reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "testNamespace", Name: "testNetworkInfo"}},
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return []*model.Vpc{}
})
return patches
},
expectErrStr: "",
expectRes: common.ResultNormal,
req: reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "testNamespace", Name: "testNetworkInfo"}},
},
{
name: "Delete NetworkInfo with delete stale VPC error",
existingNamespace: &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{Name: "testNamespace", UID: "fakeNamespaceUID"},
},
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service.VpcStore), "GetByIndex", func(_ *vpc.VPCStore, key string, value string) []*model.Vpc {
vpcName := "fakeVPCName"
vpcPath := "fakeVPCPath"
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return []*model.Vpc{
{
DisplayName: &vpcName,
Path: &vpcPath,
},
}
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return false, nil
Tags: []model.Tag{
{Scope: servicecommon.String(servicecommon.TagScopeNamespaceUID), Tag: servicecommon.String("vpc1")},
},
Path: servicecommon.String("/orgs/default/projects/default/vpcs/vpc1"),
}}
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "DeleteVPC", func(_ *vpc.VPCService, _ string) error {
return fmt.Errorf("delete failed")
Expand Down Expand Up @@ -987,7 +992,7 @@ func TestNetworkInfoReconciler_DeleteNetworkInfo(t *testing.T) {
Status: corev1.NamespaceStatus{},
},
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
return gomonkey.ApplyMethod(reflect.TypeOf(r.Service.VpcStore), "GetByIndex", func(_ *vpc.VPCStore, key string, value string) []*model.Vpc {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service.VpcStore), "GetByIndex", func(_ *vpc.VPCStore, key string, value string) []*model.Vpc {
id := "fakeNamespaceUID"
scope := servicecommon.TagScopeNamespaceUID
tag1 := []model.Tag{
Expand All @@ -1004,6 +1009,10 @@ func TestNetworkInfoReconciler_DeleteNetworkInfo(t *testing.T) {
},
}
})
patches.ApplyPrivateMethod(reflect.TypeOf(r), "deleteVPCs", func(_ *NetworkInfoReconciler, _ context.Context, _ []*model.Vpc, ns string) error {
return fmt.Errorf("failed to get VPCNetworkConfiguration for Namespace when deleting stale VPCs")
})
return patches
},
expectErrStr: "failed to get VPCNetworkConfiguration for Namespace when deleting stale VPCs",
existingNetworkInfo: &v1alpha1.NetworkInfo{
Expand Down Expand Up @@ -1410,3 +1419,22 @@ func TestRegisterAllNetworkInfo(t *testing.T) {
err = r.RegisterAllNetworkInfo(context.Background())
assert.NoError(t, err)
}

func TestListVPCsByNetworkConfigName(t *testing.T) {
r := &NetworkInfoReconciler{
Service: &vpc.VPCService{},
}
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetNamespacesByNetworkconfigName", func(_ *vpc.VPCService, _c string) []string {
return []string{"ns1", "ns2"}
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, ns string) []*model.Vpc {
if ns == "ns1" {
return []*model.Vpc{{Id: servicecommon.String("vpc1")}}
}
return []*model.Vpc{{Id: servicecommon.String("vpc2")}}
})
defer patches.Reset()
expVPCs := []*model.Vpc{{Id: servicecommon.String("vpc1")}, {Id: servicecommon.String("vpc2")}}
actVPCs := r.listVPCsByNetworkConfigName("nc1")
assert.ElementsMatch(t, expVPCs, actVPCs)
}
Loading

0 comments on commit 1cac51a

Please sign in to comment.