Remove id comparison when deleting nsx resource by NamespacedName (#994)

pkg/controllers/networkpolicy/networkpolicy_controller.go

@@ -177,19 +177,9 @@ func (r *NetworkPolicyReconciler) CollectGarbage(ctx context.Context) {
 }
 
 func (r *NetworkPolicyReconciler) deleteNetworkPolicyByName(ns, name string) error {
-	CRPolicySet, err := r.listNetworkPolciyCRIDs()
-	if err != nil {
-		return err
-	}
-
 	nsxSecurityPolicies := r.Service.ListNetworkPolicyByName(ns, name)
 	for _, item := range nsxSecurityPolicies {
 		uid := nsxutil.FindTag(item.Tags, servicecommon.TagScopeNetworkPolicyUID)
-		if CRPolicySet.Has(uid) {
-			log.Info("Skipping deletion, NetworkPolicy CR still exists in K8s", "networkPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)
-			continue
-		}
-
 		log.Info("Deleting NetworkPolicy", "networkPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)
 		if err := r.Service.DeleteSecurityPolicy(types.UID(uid), false, false, servicecommon.ResourceTypeNetworkPolicy); err != nil {
 			log.Error(err, "Failed to delete NetworkPolicy", "networkPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)

pkg/controllers/networkpolicy/networkpolicy_controller_test.go

@@ -452,20 +452,8 @@ func TestNetworkPolicyReconciler_deleteNetworkPolicyByName(t *testing.T) {
 	objs := []client.Object{}
 	r := createFakeNetworkPolicyReconciler(objs)
 
-	// listNetworkPolciyCRIDs returns an error
-	errList := errors.New("list error")
-	patch := gomonkey.ApplyPrivateMethod(reflect.TypeOf(r), "listNetworkPolciyCRIDs", func(_ *NetworkPolicyReconciler) (sets.Set[string], error) {
-		return nil, errList
-	})
-	err := r.deleteNetworkPolicyByName("dummy-ns", "dummy-name")
-	assert.Equal(t, err, errList)
-
-	// listNetworkPolciyCRIDs returns items, and deletion fails
-	patch.Reset()
-	patch.ApplyPrivateMethod(reflect.TypeOf(r), "listNetworkPolciyCRIDs", func(_ *NetworkPolicyReconciler) (sets.Set[string], error) {
-		return sets.New[string]("uid1"), nil
-	})
-	patch.ApplyMethod(reflect.TypeOf(r.Service), "ListNetworkPolicyByName", func(_ *securitypolicy.SecurityPolicyService, _ string, _ string) []*model.SecurityPolicy {
+	// deletion fails
+	patch := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "ListNetworkPolicyByName", func(_ *securitypolicy.SecurityPolicyService, _ string, _ string) []*model.SecurityPolicy {
 		return []*model.SecurityPolicy{
 			{
 				Id:   pointy.String("sp-id-1"),
@@ -488,7 +476,7 @@ func TestNetworkPolicyReconciler_deleteNetworkPolicyByName(t *testing.T) {
 		return nil
 	})
 
-	err = r.deleteNetworkPolicyByName("dummy-ns", "dummy-name")
+	err := r.deleteNetworkPolicyByName("dummy-ns", "dummy-name")
 	assert.Error(t, err)
 	patch.Reset()
 }

pkg/controllers/pod/pod_controller.go

@@ -223,25 +223,15 @@ func podIsDeleted(pod *v1.Pod) bool {
 }
 
 func (r *PodReconciler) deleteSubnetPortByPodName(ctx context.Context, ns string, name string) error {
-	// When deleting SubnetPort by Name and Namespace, skip the SubnetPort belonging to the existed SubnetPort CR
+	// NamespacedName is a unique identity in store as only one worker can deal with the NamespacedName at a time
 	nsxSubnetPorts := r.SubnetPortService.ListSubnetPortByPodName(ns, name)
 
-	crSubnetPortIDsSet, err := r.SubnetPortService.ListSubnetPortIDsFromCRs(ctx)
-	if err != nil {
-		log.Error(err, "failed to list SubnetPort CRs")
-		return err
-	}
-
 	for _, nsxSubnetPort := range nsxSubnetPorts {
-		if crSubnetPortIDsSet.Has(*nsxSubnetPort.Id) {
-			log.Info("skipping deletion, Pod CR still exists in K8s", "ID", *nsxSubnetPort.Id)
-			continue
-		}
 		if err := r.SubnetPortService.DeleteSubnetPort(nsxSubnetPort); err != nil {
 			return err
 		}
 	}
-	log.Info("successfully deleted nsxSubnetPort for Pod", "namespace", ns, "name", name)
+	log.Info("Successfully deleted nsxSubnetPort for Pod", "Namespace", ns, "Name", name)
 	return nil
 }
 

pkg/controllers/pod/pod_controller_test.go

@@ -489,7 +489,8 @@ func TestSubnetPortReconciler_GetSubnetPathForPod(t *testing.T) {
 func TestSubnetPortReconciler_deleteSubnetPortByPodName(t *testing.T) {
 	subnetportId1 := "subnetport-1"
 	subnetportId2 := "subnetport-2"
-	podName := "pod-1"
+	podName1 := "pod-1"
+	podName2 := "pod-2"
 	namespaceScope := "nsx-op/namespace"
 	ns := "ns"
 	nameScope := "nsx-op/pod_name"
@@ -502,7 +503,7 @@ func TestSubnetPortReconciler_deleteSubnetPortByPodName(t *testing.T) {
 			},
 			{
 				Scope: &nameScope,
-				Tag:   &podName,
+				Tag:   &podName1,
 			},
 		},
 	}
@@ -515,20 +516,13 @@ func TestSubnetPortReconciler_deleteSubnetPortByPodName(t *testing.T) {
 			},
 			{
 				Scope: &nameScope,
-				Tag:   &podName,
+				Tag:   &podName2,
 			},
 		},
 	}
 	r := &PodReconciler{
 		SubnetPortService: &subnetport.SubnetPortService{},
 	}
-	patchesListSubnetPortIDsFromCRs := gomonkey.ApplyFunc((*subnetport.SubnetPortService).ListSubnetPortIDsFromCRs,
-		func(s *subnetport.SubnetPortService, _ context.Context) (sets.Set[string], error) {
-			crSubnetPortIDsSet := sets.New[string]()
-			crSubnetPortIDsSet.Insert("subnetport-1")
-			return crSubnetPortIDsSet, nil
-		})
-	defer patchesListSubnetPortIDsFromCRs.Reset()
 	patchesGetByIndex := gomonkey.ApplyFunc((*subnetport.SubnetPortStore).GetByIndex,
 		func(s *subnetport.SubnetPortStore, key string, value string) []*model.VpcSubnetPort {
 			subnetPorts := make([]*model.VpcSubnetPort, 0)
@@ -542,6 +536,6 @@ func TestSubnetPortReconciler_deleteSubnetPortByPodName(t *testing.T) {
 			return nil
 		})
 	defer patchesDeleteSubnetPort.Reset()
-	err := r.deleteSubnetPortByPodName(context.TODO(), ns, podName)
+	err := r.deleteSubnetPortByPodName(context.TODO(), ns, podName2)
 	assert.Nil(t, err)
 }

pkg/controllers/securitypolicy/securitypolicy_controller.go

@@ -369,19 +369,9 @@ func (r *SecurityPolicyReconciler) CollectGarbage(ctx context.Context) {
 }
 
 func (r *SecurityPolicyReconciler) deleteSecurityPolicyByName(ns, name string) error {
-	CRPolicySet, err := r.listSecurityPolciyCRIDs()
-	if err != nil {
-		return err
-	}
-
 	nsxSecurityPolicies := r.Service.ListSecurityPolicyByName(ns, name)
 	for _, item := range nsxSecurityPolicies {
 		uid := nsxutil.FindTag(item.Tags, servicecommon.TagValueScopeSecurityPolicyUID)
-		if CRPolicySet.Has(uid) {
-			log.Info("Skipping deletion, SecurityPolicy CR still exists in K8s", "securityPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)
-			continue
-		}
-
 		log.Info("Deleting SecurityPolicy", "securityPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)
 		if err := r.Service.DeleteSecurityPolicy(types.UID(uid), false, false, servicecommon.ResourceTypeSecurityPolicy); err != nil {
 			log.Error(err, "Failed to delete SecurityPolicy", "securityPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)

pkg/controllers/securitypolicy/securitypolicy_controller_test.go

@@ -672,10 +672,6 @@ func TestSecurityPolicyReconciler_listSecurityPolciyCRIDsForVPC(t *testing.T) {
 }
 
 func TestSecurityPolicyReconciler_deleteSecuritypolicyByName(t *testing.T) {
-	mockCtl := gomock.NewController(t)
-	defer mockCtl.Finish()
-
-	k8sClient := mock_client.NewMockClient(mockCtl)
 	service := &securitypolicy.SecurityPolicyService{
 		Service: common.Service{
 			NSXClient: &nsx.Client{},
@@ -690,26 +686,13 @@ func TestSecurityPolicyReconciler_deleteSecuritypolicyByName(t *testing.T) {
 		},
 	}
 	r := &SecurityPolicyReconciler{
-		Client:   k8sClient,
 		Scheme:   nil,
 		Service:  service,
 		Recorder: fakeRecorder{},
 	}
 
-	// listSecurityPolciyCRIDs returns an error
-	errList := errors.New("list error")
-	patch := gomonkey.ApplyPrivateMethod(reflect.TypeOf(r), "listSecurityPolciyCRIDs", func(_ *SecurityPolicyReconciler) (sets.Set[string], error) {
-		return nil, errList
-	})
-	err := r.deleteSecurityPolicyByName("dummy-ns", "dummy-name")
-	assert.Equal(t, err, errList)
-
-	// listSecurityPolciyCRIDs returns items, and deletion fails
-	patch.Reset()
-	patch.ApplyPrivateMethod(reflect.TypeOf(r), "listSecurityPolciyCRIDs", func(_ *SecurityPolicyReconciler) (sets.Set[string], error) {
-		return sets.New[string]("uid1"), nil
-	})
-	patch.ApplyMethod(reflect.TypeOf(service), "ListSecurityPolicyByName", func(_ *securitypolicy.SecurityPolicyService, _ string, _ string) []*model.SecurityPolicy {
+	// deletion fails
+	patch := gomonkey.ApplyMethod(reflect.TypeOf(service), "ListSecurityPolicyByName", func(_ *securitypolicy.SecurityPolicyService, _ string, _ string) []*model.SecurityPolicy {
 		return []*model.SecurityPolicy{
 			{
 				Id:   pointy.String("sp-id-1"),
@@ -732,7 +715,7 @@ func TestSecurityPolicyReconciler_deleteSecuritypolicyByName(t *testing.T) {
 		return nil
 	})
 
-	err = r.deleteSecurityPolicyByName("dummy-ns", "dummy-name")
+	err := r.deleteSecurityPolicyByName("dummy-ns", "dummy-name")
 	assert.Error(t, err)
 	patch.Reset()
 }

pkg/controllers/staticroute/staticroute_controller.go

@@ -8,7 +8,6 @@ import (
 	"fmt"
 	"os"
 	"reflect"
-	"strings"
 
 	v1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -47,41 +46,15 @@ type StaticRouteReconciler struct {
 	StatusUpdater common.StatusUpdater
 }
 
-func (r *StaticRouteReconciler) listStaticRouteCRIDs() (sets.Set[string], error) {
-	staticRouteList := &v1alpha1.StaticRouteList{}
-	err := r.Client.List(context.Background(), staticRouteList)
-	if err != nil {
-		log.Error(err, "failed to list StaticRoute CRs")
-		return nil, err
-	}
-
-	CRStaticRouteSet := sets.New[string]()
-	for _, staticroute := range staticRouteList.Items {
-		CRStaticRouteSet.Insert(string(staticroute.UID))
-	}
-	return CRStaticRouteSet, nil
-}
-
 func (r *StaticRouteReconciler) deleteStaticRouteByName(ns, name string) error {
-	CRPolicySet, err := r.listStaticRouteCRIDs()
-	if err != nil {
-		return err
-	}
 	nsxStaticRoutes := r.Service.ListStaticRouteByName(ns, name)
 	for _, item := range nsxStaticRoutes {
-		uid := util.FindTag(item.Tags, commonservice.TagScopeStaticRouteCRUID)
-		if CRPolicySet.Has(uid) {
-			log.Info("skipping deletion, StaticRoute CR still exists in K8s", "staticrouteUID", uid, "nsxStatciRouteId", *item.Id)
-			continue
-		}
-
-		log.Info("deleting StaticRoute", "StaticRouteUID", uid, "nsxStaticRouteId", *item.Id)
-		path := strings.Split(*item.Path, "/")
-		if err := r.Service.DeleteStaticRouteByPath(path[2], path[4], path[6], *item.Id); err != nil {
-			log.Error(err, "failed to delete StaticRoute", "StaticRouteUID", uid, "nsxStaticRouteId", *item.Id)
+		log.Info("Deleting StaticRoute", "Namespace", ns, "Name", name, "nsxStaticRouteId", *item.Id)
+		if err := r.Service.DeleteStaticRoute(item); err != nil {
+			log.Error(err, "Failed to delete StaticRoute", "nsxStaticRouteId", *item.Id)
 			return err
 		}
-		log.Info("successfully deleted StaticRoute", "StaticRouteUID", uid, "nsxStaticRouteId", *item.Id)
+		log.Info("Successfully deleted StaticRoute", "Namespace", ns, "Name", name, "nsxStaticRouteId", *item.Id)
 	}
 	return nil
 }
@@ -118,7 +91,7 @@ func (r *StaticRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		r.StatusUpdater.UpdateSuccess(ctx, obj, setStaticRouteReadyStatusTrue)
 	} else {
 		r.StatusUpdater.IncreaseDeleteTotal()
-		if err := r.Service.DeleteStaticRoute(obj); err != nil {
+		if err := r.Service.DeleteStaticRouteByCR(obj); err != nil {
 			r.StatusUpdater.DeleteFail(req.NamespacedName, nil, err)
 			return ResultRequeue, err
 		}
@@ -247,9 +220,7 @@ func (r *StaticRouteReconciler) CollectGarbage(ctx context.Context) {
 
 		log.V(1).Info("GC collected StaticRoute CR", "UID", elem)
 		r.StatusUpdater.IncreaseDeleteTotal()
-		// get orgId, projectId, staticrouteId from path  "/orgs/<orgId>/projects/<projectId>/vpcs/<vpcId>/static-routes/<srId>"
-		path := strings.Split(*elem.Path, "/")
-		err = r.Service.DeleteStaticRouteByPath(path[2], path[4], path[6], *elem.Id)
+		err = r.Service.DeleteStaticRoute(elem)
 		if err != nil {
 			r.StatusUpdater.IncreaseDeleteFailTotal()
 		} else {