From 0a07ebcfaeb4275dda012068eddaec84af1615b7 Mon Sep 17 00:00:00 2001 From: Zhiying Lin Date: Tue, 18 Feb 2025 15:04:07 +0800 Subject: [PATCH] address comment + add test --- .../internalserviceexport/controller.go | 2 +- .../member/serviceexport/controller.go | 5 +- .../controller_integration_test.go | 17 +++--- .../trafficmanager/validator/profile.go | 2 +- test/e2e/framework/workload_manager.go | 36 +++++++++++ test/e2e/traffic_manager_test.go | 59 +++++++++++++++++++ 6 files changed, 109 insertions(+), 12 deletions(-) diff --git a/pkg/controllers/member/internalserviceexport/controller.go b/pkg/controllers/member/internalserviceexport/controller.go index 32fe22f7..09fd6195 100644 --- a/pkg/controllers/member/internalserviceexport/controller.go +++ b/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 } diff --git a/pkg/controllers/member/serviceexport/controller.go b/pkg/controllers/member/serviceexport/controller.go index daf43010..f76c4d90 100644 --- a/pkg/controllers/member/serviceexport/controller.go +++ b/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) { diff --git a/pkg/controllers/member/serviceexport/controller_integration_test.go b/pkg/controllers/member/serviceexport/controller_integration_test.go index a471096f..268d4149 100644 --- a/pkg/controllers/member/serviceexport/controller_integration_test.go +++ b/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) }) }) diff --git a/test/common/trafficmanager/validator/profile.go b/test/common/trafficmanager/validator/profile.go index 7b2d6c81..afd15ea4 100644 --- a/test/common/trafficmanager/validator/profile.go +++ b/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 diff --git a/test/e2e/framework/workload_manager.go b/test/e2e/framework/workload_manager.go index b8df4a96..49110915 100644 --- a/test/e2e/framework/workload_manager.go +++ b/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() { diff --git a/test/e2e/traffic_manager_test.go b/test/e2e/traffic_manager_test.go index 15ed605c..998104fc 100644 --- a/test/e2e/traffic_manager_test.go +++ b/test/e2e/traffic_manager_test.go @@ -14,10 +14,12 @@ import ( . "github.com/onsi/gomega" 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" "sigs.k8s.io/controller-runtime/pkg/client" + fleetnetv1alpha1 "go.goms.io/fleet-networking/api/v1alpha1" fleetnetv1beta1 "go.goms.io/fleet-networking/api/v1beta1" "go.goms.io/fleet-networking/pkg/common/objectmeta" "go.goms.io/fleet-networking/pkg/common/uniquename" @@ -348,6 +350,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 +642,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) + }) }) })