Skip to content

Commit

Permalink
Remove id comparison when deleting nsx resource by NamespacedName
Browse files Browse the repository at this point in the history 
Signed-off-by: Yanjun Zhou <[email protected]>
  • Loading branch information
@yanjunz97
yanjunz97 committed Jan 7, 2025
1 parent ccc66e7 commit f1b4a20
Show file tree
Hide file tree
Showing 15 changed files with 56 additions and 319 deletions.
10 changes: 0 additions & 10 deletions pkg/controllers/networkpolicy/networkpolicy_controller.go
View file
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
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
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
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
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
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
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
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

0 comments on commit f1b4a20

Please sign in to comment.