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)

* Fix issues with NetworkInfo reconciler that not delete NSX VPC in time

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.

* Filter out the Namespace which is in terminating state
  • Loading branch information
@wenyingd
wenyingd authored Dec 13, 2024
1 parent 7c783d4 commit 652107c
Show file tree
Hide file tree
Showing 5 changed files with 144 additions and 141 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 652107c

Please sign in to comment.