Remove NetworkInfo Finalizer

Add unit-test

Signed-off-by: Wenqi Qiu <[email protected]>

build/yaml/samples/nsx_v1alpha1_networkinfo.yaml

@@ -2,8 +2,6 @@ apiVersion: crd.nsx.vmware.com/v1alpha1
 kind: NetworkInfo
 metadata:
   creationTimestamp: "2024-05-14T02:14:18Z"
-  finalizers:
-  - networkinfo.crd.nsx.vmware.com/finalizer
   generation: 2
   name: kube-system
   namespace: kube-system

pkg/controllers/networkinfo/networkinfo_controller_test.go

@@ -5,11 +5,14 @@ package networkinfo
 
 import (
 	"context"
+	"errors"
+	"fmt"
 	"reflect"
 	"testing"
 
 	"github.com/agiledragon/gomonkey"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"github.com/vmware/vsphere-automation-sdk-go/services/nsxt/model"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -21,7 +24,6 @@ import (
 	"github.com/vmware-tanzu/nsx-operator/pkg/config"
 	"github.com/vmware-tanzu/nsx-operator/pkg/controllers/common"
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx"
-
 	servicecommon "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common"
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/vpc"
 )
@@ -93,11 +95,28 @@ func TestNetworkInfoReconciler_Reconcile(t *testing.T) {
 		wantErr     bool
 	}{
 		{
-			name:        "Empty",
-			prepareFunc: nil,
-			args:        requestArgs,
-			want:        common.ResultNormal,
-			wantErr:     false,
+			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, _ 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, _ 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
+				})
+				return patches
+			},
+			args:    requestArgs,
+			want:    common.ResultNormal,
+			wantErr: false,
 		},
 		{
 			name: "GatewayConnectionReadyInSystemVPC",
@@ -722,3 +741,123 @@ func TestNetworkInfoReconciler_Reconcile(t *testing.T) {
 		})
 	}
 }
+
+func TestNetworkInfoReconciler_deleteStaleVPCs(t *testing.T) {
+	r := createNetworkInfoReconciler()
+
+	ctx := context.TODO()
+	namespace := "test-ns"
+
+	t.Run("shared namespace, skip deletion", func(t *testing.T) {
+		patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, _ string) (bool, error) {
+			return true, nil
+		})
+		defer patches.Reset()
+
+		err := r.deleteStaleVPCs(ctx, namespace, "")
+		require.NoError(t, err)
+	})
+
+	t.Run("non-shared namespace, no VPCs found", func(t *testing.T) {
+		patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, _ string) (bool, error) {
+			return false, nil
+		})
+		patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
+			return nil
+		})
+		defer patches.Reset()
+
+		err := r.deleteStaleVPCs(ctx, namespace, "")
+		require.NoError(t, err)
+	})
+
+	t.Run("failed to delete VPC", func(t *testing.T) {
+		patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, _ string) (bool, error) {
+			return false, nil
+		})
+		patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
+			vpcPath := "/vpc/1"
+			return []*model.Vpc{{Path: &vpcPath}}
+		})
+		patches.ApplyMethod(reflect.TypeOf(r.Service), "DeleteVPC", func(_ *vpc.VPCService, _ string) error {
+			return fmt.Errorf("delete failed")
+		})
+		defer patches.Reset()
+
+		err := r.deleteStaleVPCs(ctx, namespace, "")
+		assert.Error(t, err)
+		assert.Contains(t, err.Error(), "delete failed")
+	})
+
+	t.Run("successful deletion of VPCs", func(t *testing.T) {
+		patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, _ string) (bool, error) {
+			return false, nil
+		})
+		patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
+			vpcPath1 := "/vpc/1"
+			vpcPath2 := "/vpc/2"
+			return []*model.Vpc{{Path: &vpcPath1}, {Path: &vpcPath2}}
+		})
+		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 {
+			return nil
+		})
+		patches.ApplyMethod(reflect.TypeOf(r.Service), "GetNetworkconfigNameFromNS", func(_ *vpc.VPCService, _ string) (string, error) {
+			return "", nil
+		})
+		patches.ApplyFunc(deleteVPCNetworkConfigurationStatus, func(ctx context.Context, client client.Client, ncName string, staleVPCs []*model.Vpc, aliveVPCs []model.Vpc) {
+			return
+		})
+		defer patches.Reset()
+
+		err := r.deleteStaleVPCs(ctx, namespace, "")
+		require.NoError(t, err)
+	})
+}
+
+func TestNetworkInfoReconciler_CollectGarbage(t *testing.T) {
+	r := createNetworkInfoReconciler()
+
+	ctx := context.TODO()
+
+	t.Run("no VPCs found in the store", func(t *testing.T) {
+		patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "ListVPC", func(_ *vpc.VPCService) []model.Vpc {
+			return nil
+		})
+		defer patches.Reset()
+
+		r.CollectGarbage(ctx)
+		// No errors expected
+	})
+
+	t.Run("successful garbage collection", func(t *testing.T) {
+		patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "ListVPC", func(_ *vpc.VPCService) []model.Vpc {
+			vpcPath1 := "/vpc/1"
+			vpcPath2 := "/vpc/2"
+			return []model.Vpc{{Path: &vpcPath1}, {Path: &vpcPath2}}
+		})
+		patches.ApplyMethod(reflect.TypeOf(r.Service), "DeleteVPC", func(_ *vpc.VPCService, _ string) error {
+			return nil
+		})
+		defer patches.Reset()
+
+		r.CollectGarbage(ctx)
+	})
+
+	t.Run("failed to delete VPC", func(t *testing.T) {
+		patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "ListVPC", func(_ *vpc.VPCService) []model.Vpc {
+			vpcPath1 := "/vpc/1"
+			vpcPath2 := "/vpc/2"
+			return []model.Vpc{{Path: &vpcPath1}, {Path: &vpcPath2}}
+		})
+		patches.ApplyMethod(reflect.TypeOf(r.Service), "DeleteVPC", func(_ *vpc.VPCService, _ string) error {
+			return errors.New("deletion error")
+		})
+		defer patches.Reset()
+
+		r.CollectGarbage(ctx)
+	})
+
+}

pkg/controllers/networkinfo/networkinfo_utils.go

@@ -237,10 +237,10 @@ func deleteVPCNetworkConfigurationStatus(ctx context.Context, client client.Clie
 	log.Info("Deleted stale VPCNetworkConfiguration status", "Name", ncName, "nc.Status.VPCs", nc.Status.VPCs, "staleVPCs", staleVPCNames)
 }
 
-func getNamespaceFromNSXVPC(nsxVPC *model.Vpc) string {
+func filterTagFromNSXVPC(nsxVPC *model.Vpc, tagName string) string {
 	tags := nsxVPC.Tags
 	for _, tag := range tags {
-		if *tag.Scope == svccommon.TagScopeNamespace {
+		if *tag.Scope == tagName {
 			return *tag.Tag
 		}
 	}

pkg/nsx/services/common/types.go

@@ -106,7 +106,6 @@ const (
 	T1SecurityPolicyFinalizerName    = "securitypolicy.nsx.vmware.com/finalizer"
 	StaticRouteFinalizerName         = "staticroute.crd.nsx.vmware.com/finalizer"
 	SubnetPortFinalizerName          = "subnetport.crd.nsx.vmware.com/finalizer"
-	NetworkInfoFinalizerName         = "networkinfo.crd.nsx.vmware.com/finalizer"
 	PodFinalizerName                 = "pod.crd.nsx.vmware.com/finalizer"
 	IPPoolFinalizerName              = "ippool.crd.nsx.vmware.com/finalizer"
 	IPAddressAllocationFinalizerName = "ipaddressallocation.crd.nsx.vmware.com/finalizer"

pkg/nsx/services/vpc/vpc.go

@@ -698,13 +698,13 @@ func (s *VPCService) IsLBProviderChanged(existingVPC *model.Vpc, lbProvider LBPr
 	return false
 }
 func (s *VPCService) CreateOrUpdateVPC(obj *v1alpha1.NetworkInfo, nc *common.VPCNetworkConfigInfo, lbProvider LBProvider) (*model.Vpc, error) {
-	// check from VPC store if vpc already exist
+	// check from VPC store if VPC already exist
 	ns := obj.Namespace
 	updateVpc := false
 	nsObj := &v1.Namespace{}
-	// get name obj
+	// get Namespace
 	if err := s.Client.Get(ctx, types.NamespacedName{Name: obj.Namespace}, nsObj); err != nil {
-		log.Error(err, "unable to fetch namespace", "name", obj.Namespace)
+		log.Error(err, "unable to fetch Namespace", "Name", obj.Namespace)
 		return nil, err
 	}
 

pkg/util/utils.go

@@ -30,9 +30,7 @@ import (
 )
 
 const (
-	wcpSystemResource   = "vmware-system-shared-t1"
-	SubnetTypeSubnet    = "subnet"
-	SubnetTypeSubnetSet = "subnetset"
+	wcpSystemResource = "vmware-system-shared-t1"
 )
 
 var (
@@ -312,24 +310,6 @@ func If(condition bool, trueVal, falseVal interface{}) interface{} {
 	}
 }
 
-func GetMapValues(in interface{}) []string {
-	if in == nil {
-		return make([]string, 0)
-	}
-	switch in.(type) {
-	case map[string]string:
-		ssMap := in.(map[string]string)
-		values := make([]string, 0, len(ssMap))
-		for _, v := range ssMap {
-			values = append(values, v)
-		}
-		return values
-	default:
-		log.Info("Unsupported map format")
-		return nil
-	}
-}
-
 // the changes map contains key/value map that you want to change.
 // if giving empty value for a key in changes map like: "mykey":"", that means removing this annotation from k8s resource
 func UpdateK8sResourceAnnotation(client client.Client, ctx context.Context, k8sObj client.Object, changes map[string]string) error {
@@ -423,10 +403,6 @@ func GenerateTruncName(limit int, resName string, prefix, suffix, project, clust
 	return generateDisplayName(common.ConnectorUnderline, resName, prefix, suffix, project, cluster)
 }
 
-func CombineNamespaceName(name, namespace string) string {
-	return fmt.Sprintf("%s/%s", namespace, name)
-}
-
 func BuildBasicTags(cluster string, obj interface{}, namespaceID types.UID) []model.Tag {
 	tags := []model.Tag{
 		{