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

Remove id comparison when deleting nsx resource by NamespacedName #994

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Remove id comparison when deleting nsx resource by NamespacedName #994

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
10 changes: 0 additions & 10 deletions pkg/controllers/networkpolicy/networkpolicy_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
18 changes: 3 additions & 15 deletions pkg/controllers/networkpolicy/networkpolicy_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand All @@ -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()
}
Expand Down
14 changes: 2 additions & 12 deletions pkg/controllers/pod/pod_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 the unique identity as only one worker can deal with the NamespacedName key 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
}

Expand Down
16 changes: 5 additions & 11 deletions pkg/controllers/pod/pod_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -502,7 +503,7 @@ func TestSubnetPortReconciler_deleteSubnetPortByPodName(t *testing.T) {
},
{
Scope: &nameScope,
Tag: &podName,
Tag: &podName1,
},
},
}
Expand All @@ -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)
Expand All @@ -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)
}
10 changes: 0 additions & 10 deletions pkg/controllers/securitypolicy/securitypolicy_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
23 changes: 3 additions & 20 deletions pkg/controllers/securitypolicy/securitypolicy_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{},
Expand All @@ -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"),
Expand All @@ -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()
}
Expand Down
31 changes: 3 additions & 28 deletions pkg/controllers/staticroute/staticroute_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,41 +47,16 @@ 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)
log.Info("Deleting StaticRoute", "Namespace", ns, "Name", name, "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.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
}
Expand Down
83 changes: 12 additions & 71 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,7 +18,6 @@ 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 @@ -257,15 +256,15 @@ func TestStaticRouteReconciler_GarbageCollector(t *testing.T) {
},
},
}
patch := gomonkey.ApplyMethod(reflect.TypeOf(service), "ListStaticRoute", func(_ *staticroute.StaticRouteService) []model.StaticRoutes {
a := []model.StaticRoutes{}
patch := gomonkey.ApplyMethod(reflect.TypeOf(service), "ListStaticRoute", func(_ *staticroute.StaticRouteService) []*model.StaticRoutes {
a := []*model.StaticRoutes{}
id1 := "2345"
path := "/orgs/org123/projects/pro123/vpcs/vpc123/static-routes/123"
tag1 := []model.Tag{{Scope: pointy.String(common.TagScopeStaticRouteCRUID), Tag: pointy.String("2345")}}
a = append(a, model.StaticRoutes{Id: &id1, Path: &path, Tags: tag1})
a = append(a, &model.StaticRoutes{Id: &id1, Path: &path, Tags: tag1})
id2 := "1234"
tag2 := []model.Tag{{Scope: pointy.String(common.TagScopeStaticRouteCRUID), Tag: pointy.String("1234")}}
a = append(a, model.StaticRoutes{Id: &id2, Path: &path, Tags: tag2})
a = append(a, &model.StaticRoutes{Id: &id2, Path: &path, Tags: tag2})
return a
})
patch.ApplyMethod(reflect.TypeOf(service), "DeleteStaticRouteByPath", func(_ *staticroute.StaticRouteService, orgId string, projectId string, vpcId string, uid string) error {
Expand Down Expand Up @@ -294,11 +293,11 @@ func TestStaticRouteReconciler_GarbageCollector(t *testing.T) {

// local store has same item as k8s cache
patch.Reset()
patch.ApplyMethod(reflect.TypeOf(service), "ListStaticRoute", func(_ *staticroute.StaticRouteService) []model.StaticRoutes {
a := []model.StaticRoutes{}
patch.ApplyMethod(reflect.TypeOf(service), "ListStaticRoute", func(_ *staticroute.StaticRouteService) []*model.StaticRoutes {
a := []*model.StaticRoutes{}
id := "1234"
tag2 := []model.Tag{{Scope: pointy.String(common.TagScopeStaticRouteCRUID), Tag: pointy.String(id)}}
a = append(a, model.StaticRoutes{Id: &id, Tags: tag2})
a = append(a, &model.StaticRoutes{Id: &id, Tags: tag2})
return a
})
patch.ApplyMethod(reflect.TypeOf(service), "DeleteStaticRoute", func(_ *staticroute.StaticRouteService, obj *v1alpha1.StaticRoute) error {
Expand All @@ -316,8 +315,8 @@ func TestStaticRouteReconciler_GarbageCollector(t *testing.T) {

// local store has no item
patch.Reset()
patch.ApplyMethod(reflect.TypeOf(service), "ListStaticRoute", func(_ *staticroute.StaticRouteService) []model.StaticRoutes {
return []model.StaticRoutes{}
patch.ApplyMethod(reflect.TypeOf(service), "ListStaticRoute", func(_ *staticroute.StaticRouteService) []*model.StaticRoutes {
return []*model.StaticRoutes{}
})
patch.ApplyMethod(reflect.TypeOf(service), "DeleteStaticRoute", func(_ *staticroute.StaticRouteService, obj *v1alpha1.StaticRoute) error {
assert.FailNow(t, "should not be called")
Expand All @@ -341,53 +340,10 @@ func TestStaticRouteReconciler_Start(t *testing.T) {
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{
Expand All @@ -404,27 +360,12 @@ func TestStaticRouteReconciler_deleteStaticRouteByName(t *testing.T) {
}

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 {
// deletion fails
patch := gomonkey.ApplyMethod(reflect.TypeOf(service), "ListStaticRouteByName", func(_ *staticroute.StaticRouteService, _ string, _ string) []*model.StaticRoutes {
return []*model.StaticRoutes{
{
Id: pointy.String("route-id-1"),
Expand All @@ -446,7 +387,7 @@ func TestStaticRouteReconciler_deleteStaticRouteByName(t *testing.T) {
return nil
})

err = r.deleteStaticRouteByName("dummy-name", "dummy-ns")
err := r.deleteStaticRouteByName("dummy-name", "dummy-ns")
assert.Error(t, err)
patch.Reset()
}
Loading
Loading