Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove staticroute finalizer #794

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+202 −83
Open

Remove staticroute finalizer #794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 54 additions & 39 deletions pkg/controllers/staticroute/staticroute_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,14 @@ import (
"strings"

v1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
apimachineryruntime "k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/client-go/tools/record"
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"

Expand Down Expand Up @@ -72,28 +70,64 @@ 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
}
TaoZou1 marked this conversation as resolved.
Show resolved Hide resolved

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 {
if apierrors.IsNotFound(err) {
if err := r.deleteStaticRouteByName(req.Namespace, req.Name); err != nil {
return ResultRequeue, err
} else {
return ResultNormal, nil
}
}
log.Error(err, "unable to fetch static route CR", "req", req.NamespacedName)
return ResultNormal, client.IgnoreNotFound(err)
return ResultRequeue, 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)
return ResultRequeue, err
}
log.V(1).Info("added finalizer on staticroute CR", "staticroute", req.NamespacedName)
}

if err := r.Service.CreateOrUpdateStaticRoute(req.Namespace, obj); err != nil {
updateFail(r, ctx, obj, &err)
// TODO: if error is not retriable, not requeue
Expand All @@ -105,27 +139,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
}

Expand Down Expand Up @@ -198,12 +219,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(),
Expand Down
162 changes: 122 additions & 40 deletions pkg/controllers/staticroute/staticroute_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand Down Expand Up @@ -366,3 +338,113 @@ func TestStaticRouteReconciler_Start(t *testing.T) {
err := r.Start(mgr)
assert.NotEqual(t, err, nil)
}

func TestStaticRouteReconciler_listStaticRouteCRIDs(t *testing.T) {
TaoZou1 marked this conversation as resolved.
Show resolved Hide resolved
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()
}
1 change: 0 additions & 1 deletion pkg/nsx/services/common/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ const (

NSXServiceAccountFinalizerName = "nsxserviceaccount.nsx.vmware.com/finalizer"
T1SecurityPolicyFinalizerName = "securitypolicy.nsx.vmware.com/finalizer"
StaticRouteFinalizerName = "staticroute.crd.nsx.vmware.com/finalizer"
NetworkInfoFinalizerName = "networkinfo.crd.nsx.vmware.com/finalizer"
IPPoolFinalizerName = "ippool.crd.nsx.vmware.com/finalizer"
IPAddressAllocationFinalizerName = "ipaddressallocation.crd.nsx.vmware.com/finalizer"
Expand Down
Loading
Loading