Merge pull request #1248 from spidernet-io/refactor-code

Fix RemoveFinalizers to improve code reusability

pkg/controller/tunnel/egress_tunnel.go

@@ -133,7 +133,7 @@ func (r *egReconciler) reconcileEGN(ctx context.Context, req reconcile.Request,
 			// For the existence of Node, when the user manually deletes EgressTunnel,
 			// we first release the EgressTunnel and then regenerate it.
 			err := r.releaseEgressTunnel(*egresstunnel, log, func() error {
-				cleanFinalizers(egresstunnel)
+				utils.RemoveFinalizers(&egresstunnel.ObjectMeta, egressTunnelFinalizers)
 				err = r.client.Update(context.Background(), egresstunnel)
 				if err != nil {
 					return err
@@ -156,14 +156,6 @@ func (r *egReconciler) reconcileEGN(ctx context.Context, req reconcile.Request,
 	return reconcile.Result{Requeue: false}, nil
 }
 
-func cleanFinalizers(node *egressv1.EgressTunnel) {
-	for i, item := range node.Finalizers {
-		if item == egressTunnelFinalizers {
-			node.Finalizers = append(node.Finalizers[:i], node.Finalizers[i+1:]...)
-		}
-	}
-}
-
 // reconcileNode reconcile node
 // not goal:
 // - add    node

pkg/controller/tunnel/egress_tunnel_test.go

@@ -194,37 +194,6 @@ func TestEgressTunnelCtrlForNode(t *testing.T) {
 	}
 }
 
-func TestCleanFinalizers(t *testing.T) {
-	tests := []struct {
-		name           string
-		node           *egressv1.EgressTunnel
-		wantFinalizers []string
-	}{
-		{
-			name: "remove egressTunnelFinalizers",
-			node: &egressv1.EgressTunnel{
-				ObjectMeta: v1.ObjectMeta{
-					Finalizers: []string{
-						"keep-this",
-						"keep-that",
-						"egressgateway.spidernet.io/egresstunnel",
-					},
-				},
-			},
-			wantFinalizers: []string{"keep-this", "keep-that"},
-		},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			cleanFinalizers(tc.node)
-			if !slicesEqual(tc.node.Finalizers, tc.wantFinalizers) {
-				t.Errorf("cleanFinalizers() got = %v, want %v", tc.node.Finalizers, tc.wantFinalizers)
-			}
-		})
-	}
-}
-
 func slicesEqual(a, b []string) bool {
 	if len(a) != len(b) {
 		return false

pkg/egressgateway/egress_gateway.go

@@ -30,7 +30,6 @@ import (
 	egress "github.com/spidernet-io/egressgateway/pkg/k8s/apis/v1beta1"
 	"github.com/spidernet-io/egressgateway/pkg/utils"
 	"github.com/spidernet-io/egressgateway/pkg/utils/ip"
-	"github.com/spidernet-io/egressgateway/pkg/utils/slice"
 )
 
 type egnReconciler struct {
@@ -438,9 +437,8 @@ func (r *egnReconciler) reconcileGateway(ctx context.Context, req reconcile.Requ
 			return reconcile.Result{Requeue: true}, err
 		}
 		if count == 0 && egw.Name != "" {
-			log.Info("remove the egressGatewayFinalizer")
-			removeEgressGatewayFinalizer(egw)
 			log.V(1).Info("remove the egressGatewayFinalizer", "ObjectMeta", egw.ObjectMeta)
+			utils.RemoveFinalizers(&egw.ObjectMeta, egressGatewayFinalizers)
 			err = r.client.Update(ctx, egw)
 			if err != nil {
 				log.Error(err, "remove the egressGatewayFinalizer", "ObjectMeta", egw.ObjectMeta)
@@ -1280,13 +1278,6 @@ func countGatewayIP(egw *egress.EgressGateway) (ipv4sFree, ipv6sFree, ipv4sTotal
 	return
 }
 
-// removeEgressGatewayFinalizer if the egress gateway is being deleted
-func removeEgressGatewayFinalizer(egw *egress.EgressGateway) {
-	if containsEgressGatewayFinalizer(egw, egressGatewayFinalizers) {
-		egw.Finalizers = slice.RemoveElement(egw.Finalizers, egressGatewayFinalizers)
-	}
-}
-
 func getPolicyCountByGatewayName(ctx context.Context, client client.Client, name string) (int, error) {
 	var num int
 
@@ -1315,15 +1306,6 @@ func getPolicyCountByGatewayName(ctx context.Context, client client.Client, name
 	return num, nil
 }
 
-func containsEgressGatewayFinalizer(gateway *egress.EgressGateway, finalizer string) bool {
-	for _, f := range gateway.ObjectMeta.Finalizers {
-		if f == finalizer {
-			return true
-		}
-	}
-	return false
-}
-
 type egressPolicyPredicate struct{}
 
 func (p egressPolicyPredicate) Create(_ event.CreateEvent) bool { return true }
@@ -1377,6 +1359,9 @@ func (p egressGatewayPredicate) Update(updateEvent event.UpdateEvent) bool {
 	if !ok {
 		return false
 	}
+	if !reflect.DeepEqual(oldObj.ObjectMeta, newObj.ObjectMeta) {
+		return true
+	}
 	if !reflect.DeepEqual(oldObj.Spec, newObj.Spec) {
 		return true
 	}

pkg/utils/finalizers.go

@@ -0,0 +1,17 @@
+// Copyright 2022 Authors of spidernet-io
+// SPDX-License-Identifier: Apache-2.0
+
+package utils
+
+import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+func RemoveFinalizers(objMeta *metav1.ObjectMeta, finalizers ...string) {
+	for _, f := range finalizers {
+		for i := len(objMeta.Finalizers) - 1; i >= 0; i-- {
+			if objMeta.Finalizers[i] == f {
+				objMeta.Finalizers = append(objMeta.Finalizers[:i], objMeta.Finalizers[i+1:]...)
+				break
+			}
+		}
+	}
+}

pkg/utils/finalizers_test.go

@@ -0,0 +1,63 @@
+// Copyright 2022 Authors of spidernet-io
+// SPDX-License-Identifier: Apache-2.0
+
+package utils
+
+import (
+	"reflect"
+	"testing"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+// TestRemoveFinalizers tests the RemoveFinalizers function.
+func TestRemoveFinalizers(t *testing.T) {
+	tests := []struct {
+		name       string
+		objMeta    metav1.ObjectMeta
+		finalizers []string
+		want       []string
+	}{
+		{
+			name: "remove single finalizer",
+			objMeta: metav1.ObjectMeta{
+				Finalizers: []string{"finalize1", "finalize2", "finalize3"},
+			},
+			finalizers: []string{"finalize2"},
+			want:       []string{"finalize1", "finalize3"},
+		},
+		{
+			name: "remove multiple finalizers",
+			objMeta: metav1.ObjectMeta{
+				Finalizers: []string{"finalize1", "finalize2", "finalize3", "finalize4"},
+			},
+			finalizers: []string{"finalize2", "finalize4"},
+			want:       []string{"finalize1", "finalize3"},
+		},
+		{
+			name: "remove non-existent finalizer",
+			objMeta: metav1.ObjectMeta{
+				Finalizers: []string{"finalize1", "finalize2"},
+			},
+			finalizers: []string{"finalize3"},
+			want:       []string{"finalize1", "finalize2"},
+		},
+		{
+			name: "remove all finalizers",
+			objMeta: metav1.ObjectMeta{
+				Finalizers: []string{"finalize1", "finalize2"},
+			},
+			finalizers: []string{"finalize1", "finalize2"},
+			want:       []string{},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			RemoveFinalizers(&tt.objMeta, tt.finalizers...)
+			if !reflect.DeepEqual(tt.objMeta.Finalizers, tt.want) {
+				t.Errorf("RemoveFinalizers() got = %v, want %v", tt.objMeta.Finalizers, tt.want)
+			}
+		})
+	}
+}