address comment + add test

pkg/controllers/member/internalserviceexport/controller.go

@@ -106,7 +106,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		// There is no need to report the conflicts back.
 		// For example, the serviceExport is no longer valid or valid with 0 weight.
 		// In these cases, there is no need to create internalServiceExport.
-		klog.V(2).InfoS("Ignoring deleted internalServiceExport", "internalServiceExport", internalSvcExportRef)
+		klog.V(2).InfoS("Ignoring deleting internalServiceExport", "internalServiceExport", internalSvcExportRef)
 		return ctrl.Result{}, nil
 	}
 

pkg/controllers/member/serviceexport/controller.go

@@ -40,7 +40,6 @@ import (
 
 const (
 	svcExportValidCondReason                 = "ServiceIsValid"
-	svcExportWeightZeroReason                = "ServiceExportWithZeroWeight"
 	svcExportInvalidNotFoundCondReason       = "ServiceNotFound"
 	svcExportInvalidIneligibleCondReason     = "ServiceIneligible"
 	svcExportPendingConflictResolutionReason = "ServicePendingConflictResolution"
@@ -210,9 +209,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		expectedValidCond := metav1.Condition{
 			Type:               string(fleetnetv1alpha1.ServiceExportValid),
 			Status:             metav1.ConditionTrue,
-			Reason:             svcExportWeightZeroReason,
+			Reason:             svcExportValidCondReason,
 			ObservedGeneration: svcExport.Generation,
-			Message:            fmt.Sprintf("Unexported service %s/%s with 0 weight", svcExport.Namespace, svcExport.Name),
+			Message:            fmt.Sprintf("Exported service %s/%s with 0 weight", svcExport.Namespace, svcExport.Name),
 		}
 		// Since the annotation won't change the generation, we compare the message here.
 		if condition.EqualConditionWithMessage(validCond, &expectedValidCond) {

pkg/controllers/member/serviceexport/controller_integration_test.go

@@ -543,9 +543,10 @@ var _ = Describe("serviceexport controller", func() {
 
 			By("update the serviceExport in the member cluster")
 			Expect(memberClient.Get(ctx, svcOrSvcExportKey, svcExport)).Should(Succeed())
-			svcExport.Annotations = map[string]string{
-				objectmeta.ServiceExportAnnotationWeight: strconv.Itoa(3837),
+			if svcExport.Annotations == nil {
+				svcExport.Annotations = make(map[string]string)
 			}
+			svcExport.Annotations[objectmeta.ServiceExportAnnotationWeight] = strconv.Itoa(3837)
 			Expect(memberClient.Update(ctx, svcExport)).Should(Succeed())
 
 			By("make sure the serviceExport is marked as invalid")
@@ -568,7 +569,8 @@ var _ = Describe("serviceexport controller", func() {
 			}, eventuallyTimeout, eventuallyInterval).Should(Succeed())
 
 			By("make sure the service is still exported")
-			Expect(serviceIsExportedToHubActual(svc.Spec.Type, false, ptr.To(int64(weight)))).Should(Succeed())
+			err = serviceIsExportedToHubActual(svc.Spec.Type, false, ptr.To(int64(weight)))()
+			Expect(err).Should(Succeed(), "Service is not exported to hub: %v", err)
 		})
 
 		It("annotation weight is set to zero", func() {
@@ -583,13 +585,13 @@ var _ = Describe("serviceexport controller", func() {
 			}
 			Expect(memberClient.Update(ctx, svcExport)).Should(Succeed())
 
-			By("make sure the serviceExport is marked as invalid")
+			By("make sure the serviceExport is marked as valid")
 			expectedCond := metav1.Condition{
 				Type:               string(fleetnetv1alpha1.ServiceExportValid),
 				Status:             metav1.ConditionTrue,
-				Reason:             svcExportWeightZeroReason,
+				Reason:             svcExportValidCondReason,
 				ObservedGeneration: svcExport.Generation,
-				Message:            fmt.Sprintf("Unexported service %s/%s with 0 weight", svcExport.Namespace, svcExport.Name),
+				Message:            fmt.Sprintf("Exported service %s/%s with 0 weight", svcExport.Namespace, svcExport.Name),
 			}
 			Eventually(func() error {
 				svcExport := &fleetnetv1alpha1.ServiceExport{}
@@ -602,7 +604,8 @@ var _ = Describe("serviceexport controller", func() {
 			}, eventuallyTimeout, eventuallyInterval).Should(Succeed(), "Get() serviceExport mismatch")
 
 			By("make sure the service is unexported")
-			Expect(serviceIsNotExportedActual()).Should(Succeed(), "Failed to unexport the service")
+			err := serviceIsNotExportedActual()
+			Expect(err).Should(Succeed(), "Failed to unexport the service: %v", err)
 		})
 	})
 

test/common/trafficmanager/validator/profile.go

@@ -24,7 +24,7 @@ import (
 )
 
 const (
-	timeout  = time.Second * 120 // need more time to create azure resources
+	timeout  = time.Second * 180 // need more time to create azure resources
 	interval = time.Millisecond * 250
 	// duration used by consistently
 	duration = time.Second * 30

test/e2e/framework/workload_manager.go

@@ -13,9 +13,11 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -29,6 +31,9 @@ import (
 	"go.goms.io/fleet-networking/pkg/common/uniquename"
 )
 
+// ignoredCondFields are fields that should be ignored when comparing conditions.
+var ignoredCondFields = cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime")
+
 // WorkloadManager represents a suite of variables of operations required to test exporting an service and more.
 type WorkloadManager struct {
 	Fleet              *Fleet
@@ -248,6 +253,37 @@ func (wm *WorkloadManager) UpdateServiceType(ctx context.Context, cluster *Clust
 	return nil
 }
 
+// UpdateServiceExportWeight updates the service export weight in the member cluster.
+func (wm *WorkloadManager) UpdateServiceExportWeight(ctx context.Context, cluster *Cluster, weight int) error {
+	var svcExport fleetnetv1alpha1.ServiceExport
+	if err := cluster.kubeClient.Get(ctx, types.NamespacedName{Namespace: wm.namespace, Name: wm.service.Name}, &svcExport); err != nil {
+		return fmt.Errorf("failed to get service export %s in cluster %s: %w", wm.service.Name, cluster.Name(), err)
+	}
+	if svcExport.Annotations == nil {
+		svcExport.Annotations = make(map[string]string)
+	}
+	svcExport.Annotations[objectmeta.ServiceExportAnnotationWeight] = fmt.Sprintf("%d", weight)
+	if err := cluster.kubeClient.Update(ctx, &svcExport); err != nil {
+		return fmt.Errorf("failed to update service export %s in cluster %s: %w", svcExport.Name, cluster.Name(), err)
+	}
+	return nil
+}
+
+// ValidateServiceExportCondition validates the service export condition in the member cluster.
+// The function will update the `wantCondition` using the latest generation of the serviceExport.
+func (wm *WorkloadManager) ValidateServiceExportCondition(ctx context.Context, cluster *Cluster, wantCondition metav1.Condition) error {
+	var svcExport fleetnetv1alpha1.ServiceExport
+	if err := cluster.kubeClient.Get(ctx, types.NamespacedName{Namespace: wm.namespace, Name: wm.service.Name}, &svcExport); err != nil {
+		return fmt.Errorf("failed to get service export %s in cluster %s: %w", wm.service.Name, cluster.Name(), err)
+	}
+	wantCondition.ObservedGeneration = svcExport.Generation
+	gotCondition := meta.FindStatusCondition(svcExport.Status.Conditions, wantCondition.Type)
+	if diff := cmp.Diff(gotCondition, &wantCondition, ignoredCondFields); diff != "" {
+		return fmt.Errorf("serviceExport condition (-got, +want): %s", diff)
+	}
+	return nil
+}
+
 // RemoveWorkload deletes workload(deployment and its service) from member clusters.
 func (wm *WorkloadManager) RemoveWorkload(ctx context.Context) error {
 	for _, m := range wm.Fleet.MemberClusters() {

test/e2e/traffic_manager_test.go

@@ -6,16 +6,17 @@ package e2e
 
 import (
 	"fmt"
-	"os"
-
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v4"
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/trafficmanager/armtrafficmanager"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	fleetnetv1alpha1 "go.goms.io/fleet-networking/api/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/utils/ptr"
+	"os"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	fleetnetv1beta1 "go.goms.io/fleet-networking/api/v1beta1"
@@ -348,6 +349,7 @@ var _ = Describe("Test exporting service via Azure traffic manager", Ordered, fu
 				Eventually(func() error {
 					return wm.AddServiceDNSLabel(ctx, memberClusters[i], memberDNSLabels[i])
 				}, framework.PollTimeout, framework.PollInterval).Should(Succeed(), "Failed to add DNS label to the service")
+				By(fmt.Sprintf("Added DNS label to the service on %s", memberClusters[i].Name()))
 			}
 
 			By("Exporting service with DNS label assigned")
@@ -639,6 +641,62 @@ var _ = Describe("Test exporting service via Azure traffic manager", Ordered, fu
 			atmProfile = buildDesiredATMProfile(profile, status.Endpoints)
 			atmValidator.ValidateProfile(ctx, atmProfileName, atmProfile)
 		})
+
+		It("Updating the serviceExport weight to 0", func() {
+			By("Updating the serviceExport weight on member-1")
+			Eventually(func() error {
+				return wm.UpdateServiceExportWeight(ctx, memberClusters[0], 0)
+			}, framework.PollTimeout, framework.PollInterval).Should(Succeed(), "Failed to add DNS label to the service")
+
+			By("Validating the serviceExport condition")
+			wantValidConditionWithZeroWeight := metav1.Condition{
+				Type:    string(fleetnetv1alpha1.ServiceExportValid),
+				Status:  metav1.ConditionTrue,
+				Reason:  "ServiceIsValid",
+				Message: fmt.Sprintf("Exported service %s/%s with 0 weight", wm.ServiceExport().Namespace, wm.ServiceExport().Name),
+			}
+			By("Validating serviceExport valid condition on member-1")
+			Eventually(func() error {
+				return wm.ValidateServiceExportCondition(ctx, memberClusters[0], wantValidConditionWithZeroWeight)
+			}, framework.PollTimeout, framework.PollInterval).Should(Succeed(), "Failed to validate the valid condition on serviceExport")
+
+			By("Validating the trafficManagerBackend status")
+			wantEndpoints := []fleetnetv1beta1.TrafficManagerEndpointStatus{
+				{
+					Weight: ptr.To(int64(100)),
+					Target: ptr.To(fmt.Sprintf(azureDNSFormat, memberDNSLabels[1], clusterLocation)),
+					From: &fleetnetv1beta1.FromCluster{
+						ClusterStatus: fleetnetv1beta1.ClusterStatus{Cluster: memberClusters[1].Name()},
+						Weight:        ptr.To(int64(1)),
+					},
+				},
+			}
+			status := validator.ValidateTrafficManagerBackendIfAcceptedAndIgnoringEndpointName(ctx, hubClient, backendName, true, wantEndpoints)
+			validator.ValidateTrafficManagerBackendStatusAndIgnoringEndpointNameConsistently(ctx, hubClient, backendName, status)
+
+			By("Validating the Azure traffic manager profile")
+			atmProfile = buildDesiredATMProfile(profile, status.Endpoints)
+			atmValidator.ValidateProfile(ctx, atmProfileName, atmProfile)
+
+			By("Updating the serviceExport weight on member-2")
+			Eventually(func() error {
+				return wm.UpdateServiceExportWeight(ctx, memberClusters[1], 0)
+			}, framework.PollTimeout, framework.PollInterval).Should(Succeed(), "Failed to add DNS label to the service")
+
+			By("Validating serviceExport valid condition on member-2")
+			Eventually(func() error {
+				return wm.ValidateServiceExportCondition(ctx, memberClusters[1], wantValidConditionWithZeroWeight)
+			}, framework.PollTimeout, framework.PollInterval).Should(Succeed(), "Failed to validate the valid condition on serviceExport")
+
+			By("Validating the trafficManagerBackend status")
+			// the serviceImport is invalid in this case
+			status = validator.ValidateTrafficManagerBackendIfAcceptedAndIgnoringEndpointName(ctx, hubClient, backendName, false, nil)
+			validator.ValidateTrafficManagerBackendStatusAndIgnoringEndpointNameConsistently(ctx, hubClient, backendName, status)
+
+			By("Validating the Azure traffic manager profile")
+			atmProfile = buildDesiredATMProfile(profile, status.Endpoints)
+			atmValidator.ValidateProfile(ctx, atmProfileName, atmProfile)
+		})
 	})
 })