Remove staticroute finalizer

The finalizer may block the deletion since the NSX resource may take long time
to delete. Remove the finalizer so the deleting operation will return
immediately

pkg/controllers/staticroute/staticroute_controller.go

@@ -18,9 +18,6 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
-	"sigs.k8s.io/controller-runtime/pkg/event"
-	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
 	"github.com/vmware-tanzu/nsx-operator/pkg/apis/vpc/v1alpha1"
 
@@ -72,28 +69,61 @@ func deleteSuccess(r *StaticRouteReconciler, _ context.Context, o *v1alpha1.Stat
 	metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteSuccessTotal, common.MetricResTypeStaticRoute)
 }
 
+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)
+			return err
+		}
+		log.Info("successfully deleted StaticRoute", "StaticRouteUID", uid, "nsxStaticRouteId", *item.Id)
+	}
+	return nil
+}
 func (r *StaticRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	obj := &v1alpha1.StaticRoute{}
 	log.Info("reconciling staticroute CR", "staticroute", req.NamespacedName)
 	metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerSyncTotal, common.MetricResTypeStaticRoute)
 
 	if err := r.Client.Get(ctx, req.NamespacedName, obj); err != nil {
-		log.Error(err, "unable to fetch static route CR", "req", req.NamespacedName)
-		return ResultNormal, client.IgnoreNotFound(err)
-	}
-
-	if obj.ObjectMeta.DeletionTimestamp.IsZero() {
-		metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerUpdateTotal, common.MetricResTypeStaticRoute)
-		if !controllerutil.ContainsFinalizer(obj, commonservice.StaticRouteFinalizerName) {
-			controllerutil.AddFinalizer(obj, commonservice.StaticRouteFinalizerName)
-			if err := r.Client.Update(ctx, obj); err != nil {
-				log.Error(err, "add finalizer", "staticroute", req.NamespacedName)
-				updateFail(r, ctx, obj, &err)
+		if client.IgnoreNotFound(err) == nil {
+			if err := r.deleteStaticRouteByName(req.Namespace, req.Name); err != nil {
 				return ResultRequeue, err
+			} else {
+				return ResultNormal, nil
 			}
-			log.V(1).Info("added finalizer on staticroute CR", "staticroute", req.NamespacedName)
 		}
+		log.Error(err, "unable to fetch static route CR", "req", req.NamespacedName)
+		return ResultRequeue, err
+	}
 
+	if obj.ObjectMeta.DeletionTimestamp.IsZero() {
 		if err := r.Service.CreateOrUpdateStaticRoute(req.Namespace, obj); err != nil {
 			updateFail(r, ctx, obj, &err)
 			// TODO: if error is not retriable, not requeue
@@ -105,27 +135,14 @@ func (r *StaticRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		}
 		updateSuccess(r, ctx, obj)
 	} else {
-		if controllerutil.ContainsFinalizer(obj, commonservice.StaticRouteFinalizerName) {
-			metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteTotal, common.MetricResTypeStaticRoute)
-			// TODO, update the value from 'default' to actual value， get OrgID, ProjectID, VPCID depending on obj.Namespace from vpc store
-			if err := r.Service.DeleteStaticRoute(obj); err != nil {
-				log.Error(err, "delete failed, would retry exponentially", "staticroute", req.NamespacedName)
-				deleteFail(r, ctx, obj, &err)
-				return ResultRequeue, err
-			}
-			controllerutil.RemoveFinalizer(obj, commonservice.StaticRouteFinalizerName)
-			if err := r.Client.Update(ctx, obj); err != nil {
-				deleteFail(r, ctx, obj, &err)
-				return ResultRequeue, err
-			}
-			log.V(1).Info("removed finalizer", "staticroute", req.NamespacedName)
-			deleteSuccess(r, ctx, obj)
-		} else {
-			// only print a message because it's not a normal case
-			log.Info("finalizers cannot be recognized", "staticroute", req.NamespacedName)
+		metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteTotal, common.MetricResTypeStaticRoute)
+		if err := r.Service.DeleteStaticRoute(obj); err != nil {
+			log.Error(err, "delete failed, would retry exponentially", "staticroute", req.NamespacedName)
+			deleteFail(r, ctx, obj, &err)
+			return ResultRequeue, err
 		}
+		deleteSuccess(r, ctx, obj)
 	}
-
 	return ResultNormal, nil
 }
 
@@ -198,12 +215,6 @@ func getExistingConditionOfType(conditionType v1alpha1.StaticRouteStatusConditio
 func (r *StaticRouteReconciler) setupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&v1alpha1.StaticRoute{}).
-		WithEventFilter(predicate.Funcs{
-			DeleteFunc: func(e event.DeleteEvent) bool {
-				// Suppress Delete events to avoid filtering them out in the Reconcile function
-				return false
-			},
-		}).
 		WithOptions(
 			controller.Options{
 				MaxConcurrentReconciles: common.NumReconcile(),

pkg/controllers/staticroute/staticroute_controller_test.go

@@ -18,6 +18,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/sets"
 	controllerruntime "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -175,23 +176,14 @@ func TestStaticRouteReconciler_Reconcile(t *testing.T) {
 	ctx := context.Background()
 	req := controllerruntime.Request{NamespacedName: types.NamespacedName{Namespace: "dummy", Name: "dummy"}}
 
-	// not found
-	errNotFound := errors.New("not found")
-	k8sClient.EXPECT().Get(ctx, gomock.Any(), gomock.Any()).Return(errNotFound)
+	// get error
+	errUnknowError := errors.New("unknown error")
+	k8sClient.EXPECT().Get(ctx, gomock.Any(), gomock.Any()).Return(errUnknowError)
 	_, err := r.Reconcile(ctx, req)
-	assert.Equal(t, err, errNotFound)
+	assert.Equal(t, err, errUnknowError)
 
-	// DeletionTimestamp.IsZero = ture, client update failed
 	sp := &v1alpha1.StaticRoute{}
-	k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil)
-	err = errors.New("Update failed")
-	k8sClient.EXPECT().Update(ctx, gomock.Any(), gomock.Any()).Return(err)
 	fakewriter := fakeStatusWriter{}
-	k8sClient.EXPECT().Status().Return(fakewriter)
-	_, ret := r.Reconcile(ctx, req)
-	assert.Equal(t, err, ret)
-
-	//  DeletionTimestamp.IsZero = false, Finalizers doesn't include util.FinalizerName
 	k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error {
 		v1sp := obj.(*v1alpha1.StaticRoute)
 		time := metav1.Now()
@@ -200,74 +192,54 @@ func TestStaticRouteReconciler_Reconcile(t *testing.T) {
 	})
 
 	patch := gomonkey.ApplyMethod(reflect.TypeOf(service), "DeleteStaticRoute", func(_ *staticroute.StaticRouteService, obj *v1alpha1.StaticRoute) error {
-		assert.FailNow(t, "should not be called")
 		return nil
 	})
 
-	k8sClient.EXPECT().Update(ctx, gomock.Any(), gomock.Any()).Return(nil)
-	_, ret = r.Reconcile(ctx, req)
-	assert.Equal(t, ret, nil)
-	patch.Reset()
-
-	//  DeletionTimestamp.IsZero = false, Finalizers include util.FinalizerName
-	k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error {
-		v1sp := obj.(*v1alpha1.StaticRoute)
-		time := metav1.Now()
-		v1sp.ObjectMeta.DeletionTimestamp = &time
-		v1sp.Finalizers = []string{common.StaticRouteFinalizerName}
-		return nil
-	})
-	patch = gomonkey.ApplyMethod(reflect.TypeOf(service), "DeleteStaticRoute", func(_ *staticroute.StaticRouteService, obj *v1alpha1.StaticRoute) error {
-		return nil
-	})
-	_, ret = r.Reconcile(ctx, req)
+	_, ret := r.Reconcile(ctx, req)
 	assert.Equal(t, ret, nil)
 	patch.Reset()
 
-	//  DeletionTimestamp.IsZero = false, Finalizers include util.FinalizerName, DeleteStaticRoute fail
+	//  DeletionTimestamp.IsZero = false,  DeleteStaticRoute fail
 	k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error {
 		v1sp := obj.(*v1alpha1.StaticRoute)
 		time := metav1.Now()
 		v1sp.ObjectMeta.DeletionTimestamp = &time
-		v1sp.Finalizers = []string{common.StaticRouteFinalizerName}
 		return nil
 	})
 	patch = gomonkey.ApplyMethod(reflect.TypeOf(service), "DeleteStaticRoute", func(_ *staticroute.StaticRouteService, obj *v1alpha1.StaticRoute) error {
 		return errors.New("delete failed")
 	})
-
-	k8sClient.EXPECT().Status().Times(2).Return(fakewriter)
+	k8sClient.EXPECT().Status().Times(1).Return(fakewriter)
 	_, ret = r.Reconcile(ctx, req)
 	assert.NotEqual(t, ret, nil)
 	patch.Reset()
 
-	//  DeletionTimestamp.IsZero = true, Finalizers include util.FinalizerName, CreateorUpdateStaticRoute fail
+	//  DeletionTimestamp.IsZero = true,  CreateorUpdateStaticRoute fail
 	k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error {
 		v1sp := obj.(*v1alpha1.StaticRoute)
 		v1sp.ObjectMeta.DeletionTimestamp = nil
-		v1sp.Finalizers = []string{common.StaticRouteFinalizerName}
 		return nil
 	})
 
 	patch = gomonkey.ApplyMethod(reflect.TypeOf(service), "CreateOrUpdateStaticRoute", func(_ *staticroute.StaticRouteService, namespace string, obj *v1alpha1.StaticRoute) error {
 		return errors.New("create failed")
 	})
+	k8sClient.EXPECT().Status().Times(1).Return(fakewriter)
 	_, ret = r.Reconcile(ctx, req)
 	assert.NotEqual(t, ret, nil)
 	patch.Reset()
 
-	//  DeletionTimestamp.IsZero = true, Finalizers include util.FinalizerName, CreateorUpdateStaticRoute succ
+	//  DeletionTimestamp.IsZero = true,  CreateorUpdateStaticRoute succ
 	k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error {
 		v1sp := obj.(*v1alpha1.StaticRoute)
 		v1sp.ObjectMeta.DeletionTimestamp = nil
-		v1sp.Finalizers = []string{common.StaticRouteFinalizerName}
 		return nil
 	})
 
+	k8sClient.EXPECT().Status().Times(1).Return(fakewriter)
 	patch = gomonkey.ApplyMethod(reflect.TypeOf(service), "CreateOrUpdateStaticRoute", func(_ *staticroute.StaticRouteService, namespace string, obj *v1alpha1.StaticRoute) error {
 		return nil
 	})
-	k8sClient.EXPECT().Status().Times(1).Return(fakewriter)
 	_, ret = r.Reconcile(ctx, req)
 	assert.Equal(t, ret, nil)
 	patch.Reset()
@@ -366,3 +338,111 @@ func TestStaticRouteReconciler_Start(t *testing.T) {
 	err := r.Start(mgr)
 	assert.NotEqual(t, err, nil)
 }
+func TestStaticRouteReconciler_listStaticRouteCRIDs(t *testing.T) {
+	mockCtl := gomock.NewController(t)
+	k8sClient := mock_client.NewMockClient(mockCtl)
+	r := &StaticRouteReconciler{
+		Client: k8sClient,
+		Scheme: nil,
+	}
+
+	ctx := context.Background()
+
+	// list returns an error
+	errList := errors.New("list error")
+	k8sClient.EXPECT().List(ctx, gomock.Any()).Return(errList)
+	_, err := r.listStaticRouteCRIDs()
+	assert.Equal(t, err, errList)
+
+	// list returns no error, but no items
+	k8sClient.EXPECT().List(ctx, gomock.Any()).DoAndReturn(func(_ context.Context, list client.ObjectList, _ ...client.ListOption) error {
+		staticRouteList := list.(*v1alpha1.StaticRouteList)
+		staticRouteList.Items = []v1alpha1.StaticRoute{}
+		return nil
+	})
+	crIDs, err := r.listStaticRouteCRIDs()
+	assert.NoError(t, err)
+	assert.Equal(t, 0, crIDs.Len())
+
+	// list returns items
+	k8sClient.EXPECT().List(ctx, gomock.Any()).DoAndReturn(func(_ context.Context, list client.ObjectList, _ ...client.ListOption) error {
+		staticRouteList := list.(*v1alpha1.StaticRouteList)
+		staticRouteList.Items = []v1alpha1.StaticRoute{
+			{ObjectMeta: metav1.ObjectMeta{UID: "uid1"}},
+			{ObjectMeta: metav1.ObjectMeta{UID: "uid2"}},
+		}
+		return nil
+	})
+	crIDs, err = r.listStaticRouteCRIDs()
+	assert.NoError(t, err)
+	assert.Equal(t, 2, crIDs.Len())
+	assert.True(t, crIDs.Has("uid1"))
+	assert.True(t, crIDs.Has("uid2"))
+}
+func TestStaticRouteReconciler_deleteStaticRouteByName(t *testing.T) {
+	mockCtl := gomock.NewController(t)
+	defer mockCtl.Finish()
+
+	k8sClient := mock_client.NewMockClient(mockCtl)
+	mockStaticRouteClient := mocks.NewMockStaticRoutesClient(mockCtl)
+
+	service := &staticroute.StaticRouteService{
+		Service: common.Service{
+			NSXClient: &nsx.Client{
+				StaticRouteClient: mockStaticRouteClient,
+			},
+			NSXConfig: &config.NSXOperatorConfig{
+				NsxConfig: &config.NsxConfig{
+					EnforcementPoint: "vmc-enforcementpoint",
+				},
+			},
+		},
+	}
+
+	r := &StaticRouteReconciler{
+		Client:  k8sClient,
+		Scheme:  nil,
+		Service: service,
+	}
+
+	// listStaticRouteCRIDs returns an error
+	errList := errors.New("list error")
+	patch := gomonkey.ApplyPrivateMethod(reflect.TypeOf(r), "listStaticRouteCRIDs", func(_ *StaticRouteReconciler) (sets.Set[string], error) {
+		return nil, errList
+	})
+	defer patch.Reset()
+
+	err := r.deleteStaticRouteByName("dummy-name", "dummy-ns")
+	assert.Equal(t, err, errList)
+
+	// listStaticRouteCRIDs returns items, and deletion fails
+	patch.Reset()
+	patch.ApplyPrivateMethod(reflect.TypeOf(r), "listStaticRouteCRIDs", func(_ *StaticRouteReconciler) (sets.Set[string], error) {
+		return sets.New[string]("uid1"), nil
+	})
+	patch.ApplyMethod(reflect.TypeOf(service), "ListStaticRouteByName", func(_ *staticroute.StaticRouteService, _ string, _ string) []*model.StaticRoutes {
+		return []*model.StaticRoutes{
+			{
+				Id:   pointy.String("route-id-1"),
+				Path: pointy.String("/orgs/org123/projects/pro123/vpcs/vpc123/static-routes/route-id-1"),
+				Tags: []model.Tag{{Scope: pointy.String(common.TagScopeStaticRouteCRUID), Tag: pointy.String("uid1")}},
+			},
+			{
+				Id:   pointy.String("route-id-2"),
+				Path: pointy.String("/orgs/org123/projects/pro123/vpcs/vpc123/static-routes/route-id-2"),
+				Tags: []model.Tag{{Scope: pointy.String(common.TagScopeStaticRouteCRUID), Tag: pointy.String("uid2")}},
+			},
+		}
+	})
+
+	patch.ApplyMethod(reflect.TypeOf(service), "DeleteStaticRouteByPath", func(_ *staticroute.StaticRouteService, orgId string, projectId string, vpcId string, uid string) error {
+		if uid == "route-id-2" {
+			return errors.New("delete failed")
+		}
+		return nil
+	})
+
+	err = r.deleteStaticRouteByName("dummy-name", "dummy-ns")
+	assert.Error(t, err)
+	patch.Reset()
+}

pkg/nsx/services/common/types.go

@@ -106,7 +106,6 @@ const (
 
 	SecurityPolicyFinalizerName      = "securitypolicy.crd.nsx.vmware.com/finalizer"
 	NetworkPolicyFinalizerName       = "networkpolicy.crd.nsx.vmware.com/finalizer"
-	StaticRouteFinalizerName         = "staticroute.crd.nsx.vmware.com/finalizer"
 	SubnetFinalizerName              = "subnet.crd.nsx.vmware.com/finalizer"
 	SubnetSetFinalizerName           = "subnetset.crd.nsx.vmware.com/finalizer"
 	SubnetPortFinalizerName          = "subnetport.crd.nsx.vmware.com/finalizer"

pkg/nsx/services/staticroute/staticroute.go

@@ -40,6 +40,7 @@ func InitializeStaticRoute(commonService common.Service, vpcService common.VPCSe
 	staticRouteStore := &StaticRouteStore{}
 	staticRouteStore.Indexer = cache.NewIndexer(keyFunc, cache.Indexers{
 		common.TagScopeStaticRouteCRUID: indexFunc,
+		common.TagScopeNamespace:        indexStaticRouteNamespace,
 	})
 	staticRouteStore.BindingType = model.StaticRoutesBindingType()
 	staticRouteService.StaticRouteStore = staticRouteStore
@@ -147,7 +148,18 @@ func (service *StaticRouteService) DeleteStaticRoute(obj *v1alpha1.StaticRoute)
 	}
 	return service.DeleteStaticRouteByPath(vpcResourceInfo.OrgID, vpcResourceInfo.ProjectID, vpcResourceInfo.VPCID, id)
 }
-
+func (service *StaticRouteService) ListStaticRouteByName(ns, name string) []*model.StaticRoutes {
+	var result []*model.StaticRoutes
+	staticroutes := service.StaticRouteStore.GetByIndex(common.TagScopeNamespace, ns)
+	for _, staticroute := range staticroutes {
+		sr := staticroute.(*model.StaticRoutes)
+		tagname := nsxutil.FindTag(sr.Tags, common.TagScopeStaticRouteCRName)
+		if tagname == name {
+			result = append(result, staticroute.(*model.StaticRoutes))
+		}
+	}
+	return result
+}
 func (service *StaticRouteService) ListStaticRoute() []*model.StaticRoutes {
 	staticRoutes := service.StaticRouteStore.List()
 	staticRouteSet := []*model.StaticRoutes{}