test

pkg/controllers/hub/trafficmanagerbackend/controller.go

@@ -113,14 +113,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
 		return r.handleDelete(ctx, backend)
 	}
 
-	// register finalizer
-	if !controllerutil.ContainsFinalizer(backend, objectmeta.TrafficManagerBackendFinalizer) {
-		controllerutil.AddFinalizer(backend, objectmeta.TrafficManagerBackendFinalizer)
-		if err := r.Update(ctx, backend); err != nil {
-			klog.ErrorS(err, "Failed to add finalizer to trafficManagerBackend", "trafficManagerBackend", backend)
-			return ctrl.Result{}, controller.NewUpdateIgnoreConflictError(err)
-		}
-	}
 	// TODO: replace the following with defaulter webhook
 	defaulter.SetDefaultsTrafficManagerBackend(backend)
 	return r.handleUpdate(ctx, backend)
@@ -264,6 +256,17 @@ func (r *Reconciler) handleUpdate(ctx context.Context, backend *fleetnetv1beta1.
 	}
 	klog.V(2).InfoS("Found the exported services behind the serviceImport", "trafficManagerBackend", backendKObj, "serviceImport", klog.KObj(serviceImport), "numberOfDesiredEndpoints", len(desiredEndpointsMaps), "numberOfInvalidServices", len(invalidServicesMaps))
 
+	// register finalizer only before creating atm endpoints
+	// So that when a user specifies an invalid resource group of the profile, the controller will fail to create the endpoint because of the 403 error.
+	// Otherwise, the deletion will be stuck because of the 403 error and the finalizer cannot be removed.
+	if !controllerutil.ContainsFinalizer(backend, objectmeta.TrafficManagerBackendFinalizer) {
+		controllerutil.AddFinalizer(backend, objectmeta.TrafficManagerBackendFinalizer)
+		if err := r.Update(ctx, backend); err != nil {
+			klog.ErrorS(err, "Failed to add finalizer to trafficManagerBackend", "trafficManagerBackend", backend)
+			return ctrl.Result{}, controller.NewUpdateIgnoreConflictError(err)
+		}
+	}
+
 	acceptedEndpoints, badEndpointsErr, err := r.updateTrafficManagerEndpointsAndUpdateStatusIfUnknown(ctx, profile.Spec.ResourceGroup, backend, atmProfile, desiredEndpointsMaps)
 	if err != nil {
 		return ctrl.Result{}, err

pkg/controllers/hub/trafficmanagerprofile/controller.go

@@ -95,15 +95,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		return r.handleDelete(ctx, profile)
 	}
 
-	// register finalizer
-	if !controllerutil.ContainsFinalizer(profile, objectmeta.TrafficManagerProfileFinalizer) {
-		controllerutil.AddFinalizer(profile, objectmeta.TrafficManagerProfileFinalizer)
-		if err := r.Update(ctx, profile); err != nil {
-			klog.ErrorS(err, "Failed to add finalizer to trafficManagerProfile", "trafficManagerProfile", profileKRef)
-			return ctrl.Result{}, controller.NewUpdateIgnoreConflictError(err)
-		}
-	}
-
 	// TODO: replace the following with defaulter wehbook
 	defaulter.SetDefaultsTrafficManagerProfile(profile)
 	return r.handleUpdate(ctx, profile)
@@ -161,6 +152,17 @@ func (r *Reconciler) handleUpdate(ctx context.Context, profile *fleetnetv1beta1.
 		}
 	}
 
+	// register finalizer only before creating atm profile
+	// So that when a user specifies an invalid resource group, the controller will fail to create the profile because of the 403 error.
+	// Otherwise, the deletion will be stuck because of the 403 error and the finalizer cannot be removed.
+	if !controllerutil.ContainsFinalizer(profile, objectmeta.TrafficManagerProfileFinalizer) {
+		controllerutil.AddFinalizer(profile, objectmeta.TrafficManagerProfileFinalizer)
+		if err := r.Update(ctx, profile); err != nil {
+			klog.ErrorS(err, "Failed to add finalizer to trafficManagerProfile", "trafficManagerProfile", profileKObj)
+			return ctrl.Result{}, controller.NewUpdateIgnoreConflictError(err)
+		}
+	}
+
 	res, updateErr := r.ProfilesClient.CreateOrUpdate(ctx, profile.Spec.ResourceGroup, atmProfileName, desiredATMProfile, nil)
 	if updateErr != nil {
 		if !errors.As(updateErr, &responseError) {

test/common/trafficmanager/validator/profile.go

@@ -9,8 +9,6 @@ package validator
 import (
 	"context"
 	"fmt"
-	"go.goms.io/fleet-networking/pkg/common/objectmeta"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"time"
 
 	"github.com/google/go-cmp/cmp"
@@ -120,30 +118,3 @@ func IsTrafficManagerProfileDeleted(ctx context.Context, k8sClient client.Client
 		return nil
 	}, timeout, interval).Should(gomega.Succeed(), "Failed to remove trafficManagerProfile %s ", name)
 }
-
-// ValidateTrafficManagerProfileConsistentlyExist validates whether the profile consistently exists.
-func ValidateTrafficManagerProfileConsistentlyExist(ctx context.Context, k8sClient client.Client, name types.NamespacedName) {
-	gomega.Consistently(func() error {
-		profile := &fleetnetv1beta1.TrafficManagerProfile{}
-		if err := k8sClient.Get(ctx, name, profile); errors.IsNotFound(err) {
-			return fmt.Errorf("trafficManagerProfile %s does not exist: %w", name, err)
-		}
-		return nil
-	}, duration, interval).Should(gomega.Succeed(), "Failed to find trafficManagerProfile %s ", name)
-}
-
-// IsTrafficManagerProfileDeletedAfterRemoveFinalizer validates whether the profile is deleted after removing the finalizer.
-func IsTrafficManagerProfileDeletedAfterRemoveFinalizer(ctx context.Context, k8sClient client.Client, name types.NamespacedName) {
-	gomega.Eventually(func() error {
-		profile := &fleetnetv1beta1.TrafficManagerProfile{}
-		if err := k8sClient.Get(ctx, name, profile); err != nil {
-			return fmt.Errorf("failed to get trafficManagerProfile %s: %w", name, err)
-		}
-		controllerutil.RemoveFinalizer(profile, objectmeta.TrafficManagerProfileFinalizer)
-		if err := k8sClient.Update(ctx, profile); err != nil {
-			return fmt.Errorf("failed to update trafficManagerProfile %s: %w", name, err)
-		}
-		return nil
-	}, timeout, interval).Should(gomega.Succeed(), "Failed to remove trafficManagerProfile %s finalizer", name)
-	IsTrafficManagerProfileDeleted(ctx, k8sClient, name)
-}

test/e2e/traffic_manager_test.go

@@ -30,7 +30,7 @@ var (
 	enabled = os.Getenv("ENABLE_TRAFFIC_MANAGER") == "true"
 )
 
-var _ = FDescribe("Test exporting service via Azure traffic manager", Ordered, func() {
+var _ = Describe("Test exporting service via Azure traffic manager", Ordered, func() {
 	var wm *framework.WorkloadManager
 	var profile fleetnetv1beta1.TrafficManagerProfile
 	var profileName types.NamespacedName
@@ -105,8 +105,7 @@ var _ = FDescribe("Test exporting service via Azure traffic manager", Ordered, f
 			Expect(err).Should(SatisfyAny(Succeed(), WithTransform(errors.IsNotFound, BeTrue())), "Failed to delete the trafficManagerProfile")
 
 			By("Validating trafficManagerProfile is deleted")
-			validator.ValidateTrafficManagerProfileConsistentlyExist(ctx, hubClient, invalidProfileName)
-			validator.IsTrafficManagerProfileDeletedAfterRemoveFinalizer(ctx, hubClient, invalidProfileName)
+			validator.IsTrafficManagerProfileDeleted(ctx, hubClient, invalidProfileName)
 		})
 
 		It("Creating trafficManagerBackend with invalid profile", func() {