diff --git a/pkg/controllers/hub/trafficmanagerbackend/controller.go b/pkg/controllers/hub/trafficmanagerbackend/controller.go index 1c1a2573..8e0e8e5c 100644 --- a/pkg/controllers/hub/trafficmanagerbackend/controller.go +++ b/pkg/controllers/hub/trafficmanagerbackend/controller.go @@ -212,7 +212,17 @@ func (r *Reconciler) handleUpdate(ctx context.Context, backend *fleetnetv1alpha1 // The controller will retry when err is not nil. return ctrl.Result{}, err } - klog.V(2).InfoS("Found the valid trafficManagerProfile", "trafficManagerBackend", backendKObj, "trafficManagerProfile", klog.KObj(profile)) + profileKObj := klog.KObj(profile) + klog.V(2).InfoS("Found the valid trafficManagerProfile", "trafficManagerBackend", backendKObj, "trafficManagerProfile", profileKObj) + + atmProfile, err := r.validateAzureTrafficManagerProfile(ctx, backend, profile) + if err != nil || atmProfile == nil { + // We don't need to requeue the invalid Azure Traffic Manager profile (err == nil and atmProfile == nil) as when + // the profile becomes valid, the controller will be re-triggered again. + // The controller will retry when err is not nil. + return ctrl.Result{}, err + } + klog.V(2).InfoS("Found the valid Azure Traffic Manager Profile", "trafficManagerBackend", backendKObj, "trafficManagerProfile", profileKObj, "atmProfileName", atmProfile.Name) return ctrl.Result{}, nil } @@ -246,6 +256,35 @@ func (r *Reconciler) validateTrafficManagerProfile(ctx context.Context, backend return nil, r.updateTrafficManagerBackendStatus(ctx, backend) } +// validateAzureTrafficManagerProfile returns not nil Azure Traffic Manager profile when the atm profile is valid. +func (r *Reconciler) validateAzureTrafficManagerProfile(ctx context.Context, backend *fleetnetv1alpha1.TrafficManagerBackend, profile *fleetnetv1alpha1.TrafficManagerProfile) (*armtrafficmanager.Profile, error) { + atmProfileName := generateAzureTrafficManagerProfileNameFunc(profile) + backendKObj := klog.KObj(backend) + profileKObj := klog.KObj(profile) + getRes, getErr := r.ProfilesClient.Get(ctx, r.ResourceGroupName, atmProfileName, nil) + if getErr != nil { + if azureerrors.IsNotFound(getErr) { + // We've already checked the TrafficManagerProfile condition before getting Azure resource. + // It may happen when + // 1. customers delete the azure profile manually + // 2. the TrafficManagerProfile info is stale. + // For the case 1, retry won't help to recover the Azure Traffic Manager profile resource. + // For the case 2, the controller will be re-triggered when the TrafficManagerProfile is updated. + klog.ErrorS(getErr, "NotFound Azure Traffic Manager profile", "trafficManagerBackend", backendKObj, "trafficManagerProfile", profileKObj, "atmProfileName", atmProfileName) + // none of the endpoints are accepted by the TrafficManager + setFalseCondition(backend, nil, fmt.Sprintf("Azure Traffic Manager profile %q under %q is not found", atmProfileName, r.ResourceGroupName)) + return nil, r.updateTrafficManagerBackendStatus(ctx, backend) + } + klog.V(2).InfoS("Failed to get Azure Traffic Manager profile", "trafficManagerBackend", backendKObj, "trafficManagerProfile", profileKObj, "atmProfileName", atmProfileName) + setUnknownCondition(backend, fmt.Sprintf("Failed to get the Azure Traffic Manager profile %q under %q: %v", atmProfileName, r.ResourceGroupName, getErr)) + if err := r.updateTrafficManagerBackendStatus(ctx, backend); err != nil { + return nil, err + } + return nil, getErr // need to return the error to requeue the request + } + return &getRes.Profile, nil +} + func setFalseCondition(backend *fleetnetv1alpha1.TrafficManagerBackend, acceptedEndpoints []fleetnetv1alpha1.TrafficManagerEndpointStatus, message string) { cond := metav1.Condition{ Type: string(fleetnetv1alpha1.TrafficManagerBackendConditionAccepted), diff --git a/pkg/controllers/hub/trafficmanagerbackend/controller_integration_test.go b/pkg/controllers/hub/trafficmanagerbackend/controller_integration_test.go index 0cc99100..86d11fcf 100644 --- a/pkg/controllers/hub/trafficmanagerbackend/controller_integration_test.go +++ b/pkg/controllers/hub/trafficmanagerbackend/controller_integration_test.go @@ -6,6 +6,9 @@ Licensed under the MIT license. package trafficmanagerbackend import ( + "context" + "time" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "k8s.io/apimachinery/pkg/api/meta" @@ -19,6 +22,11 @@ import ( "go.goms.io/fleet-networking/test/common/trafficmanager/validator" ) +const ( + timeout = time.Second * 10 + interval = time.Millisecond * 250 +) + func trafficManagerBackendForTest(name, profileName, serviceImportName string) *fleetnetv1alpha1.TrafficManagerBackend { return &fleetnetv1alpha1.TrafficManagerBackend{ ObjectMeta: metav1.ObjectMeta{ @@ -47,6 +55,37 @@ func trafficManagerProfileForTest(name string) *fleetnetv1alpha1.TrafficManagerP } } +func buildFalseCondition() []metav1.Condition { + return []metav1.Condition{ + { + Status: metav1.ConditionFalse, + Type: string(fleetnetv1alpha1.TrafficManagerBackendReasonAccepted), + Reason: string(fleetnetv1alpha1.TrafficManagerBackendReasonInvalid), + }, + } +} + +func buildUnknownCondition() []metav1.Condition { + return []metav1.Condition{ + { + Status: metav1.ConditionUnknown, + Type: string(fleetnetv1alpha1.TrafficManagerBackendReasonAccepted), + Reason: string(fleetnetv1alpha1.TrafficManagerBackendReasonPending), + }, + } +} + +func updateTrafficManagerProfileStatusToTrue(ctx context.Context, profile *fleetnetv1alpha1.TrafficManagerProfile) { + cond := metav1.Condition{ + Status: metav1.ConditionTrue, + Type: string(fleetnetv1alpha1.TrafficManagerProfileConditionProgrammed), + ObservedGeneration: profile.Generation, + Reason: string(fleetnetv1alpha1.TrafficManagerProfileReasonProgrammed), + } + meta.SetStatusCondition(&profile.Status.Conditions, cond) + Expect(k8sClient.Status().Update(ctx, profile)).Should(Succeed()) +} + var _ = Describe("Test TrafficManagerBackend Controller", func() { Context("When creating trafficManagerBackend with invalid profile", Ordered, func() { name := fakeprovider.ValidBackendName @@ -67,13 +106,7 @@ var _ = Describe("Test TrafficManagerBackend Controller", func() { }, Spec: backend.Spec, Status: fleetnetv1alpha1.TrafficManagerBackendStatus{ - Conditions: []metav1.Condition{ - { - Status: metav1.ConditionFalse, - Type: string(fleetnetv1alpha1.TrafficManagerBackendReasonAccepted), - Reason: string(fleetnetv1alpha1.TrafficManagerBackendReasonInvalid), - }, - }, + Conditions: buildFalseCondition(), }, } validator.ValidateTrafficManagerBackend(ctx, k8sClient, &want) @@ -109,7 +142,96 @@ var _ = Describe("Test TrafficManagerBackend Controller", func() { }) It("Validating trafficManagerBackend", func() { - validator.IsTrafficManagerBackendFinalizerAdded(ctx, k8sClient, backendNamespacedName) + want := fleetnetv1alpha1.TrafficManagerBackend{ + ObjectMeta: metav1.ObjectMeta{ + Name: backendName, + Namespace: testNamespace, + Finalizers: []string{objectmeta.TrafficManagerBackendFinalizer}, + }, + Spec: backend.Spec, + Status: fleetnetv1alpha1.TrafficManagerBackendStatus{ + Conditions: buildUnknownCondition(), + }, + } + validator.ValidateTrafficManagerBackend(ctx, k8sClient, &want) + }) + + It("Updating TrafficManagerProfile status to programmed true and it should trigger controller", func() { + By("By updating TrafficManagerProfile status") + updateTrafficManagerProfileStatusToTrue(ctx, profile) + }) + + It("Validating trafficManagerBackend", func() { + want := fleetnetv1alpha1.TrafficManagerBackend{ + ObjectMeta: metav1.ObjectMeta{ + Name: backendName, + Namespace: testNamespace, + Finalizers: []string{objectmeta.TrafficManagerBackendFinalizer}, + }, + Spec: backend.Spec, + Status: fleetnetv1alpha1.TrafficManagerBackendStatus{ + Conditions: buildFalseCondition(), + }, + } + validator.ValidateTrafficManagerBackend(ctx, k8sClient, &want) + }) + + It("Deleting trafficManagerBackend", func() { + err := k8sClient.Delete(ctx, backend) + Expect(err).Should(Succeed(), "failed to delete trafficManagerBackend") + }) + + It("Validating trafficManagerBackend is deleted", func() { + validator.IsTrafficManagerBackendDeleted(ctx, k8sClient, backendNamespacedName) + }) + + It("Deleting trafficManagerProfile", func() { + err := k8sClient.Delete(ctx, profile) + Expect(err).Should(Succeed(), "failed to delete trafficManagerProfile") + }) + + It("Validating trafficManagerProfile is deleted", func() { + validator.IsTrafficManagerProfileDeleted(ctx, k8sClient, profileNamespacedName) + }) + }) + + Context("When creating trafficManagerBackend and failing to get Azure Traffic Manager profile", Ordered, func() { + profileName := fakeprovider.RequestTimeoutProfileName + profileNamespacedName := types.NamespacedName{Namespace: testNamespace, Name: profileName} + var profile *fleetnetv1alpha1.TrafficManagerProfile + backendName := fakeprovider.ValidBackendName + backendNamespacedName := types.NamespacedName{Namespace: testNamespace, Name: backendName} + var backend *fleetnetv1alpha1.TrafficManagerBackend + + It("Creating a new TrafficManagerProfile", func() { + By("By creating a new TrafficManagerProfile") + profile = trafficManagerProfileForTest(profileName) + Expect(k8sClient.Create(ctx, profile)).Should(Succeed()) + }) + + It("Updating TrafficManagerProfile status to programmed true", func() { + By("By updating TrafficManagerProfile status") + updateTrafficManagerProfileStatusToTrue(ctx, profile) + }) + + It("Creating TrafficManagerBackend", func() { + backend = trafficManagerBackendForTest(backendName, profileName, "not-exist") + Expect(k8sClient.Create(ctx, backend)).Should(Succeed()) + }) + + It("Validating trafficManagerBackend", func() { + want := fleetnetv1alpha1.TrafficManagerBackend{ + ObjectMeta: metav1.ObjectMeta{ + Name: backendName, + Namespace: testNamespace, + Finalizers: []string{objectmeta.TrafficManagerBackendFinalizer}, + }, + Spec: backend.Spec, + Status: fleetnetv1alpha1.TrafficManagerBackendStatus{ + Conditions: buildUnknownCondition(), + }, + } + validator.ValidateTrafficManagerBackend(ctx, k8sClient, &want) }) It("Deleting trafficManagerBackend", func() { @@ -117,6 +239,20 @@ var _ = Describe("Test TrafficManagerBackend Controller", func() { Expect(err).Should(Succeed(), "failed to delete trafficManagerBackend") }) + It("Validating trafficManagerBackend cannot be deleted", func() { + validator.ValidateTrafficManagerConsistentlyExist(ctx, k8sClient, backendNamespacedName) + }) + + It("Removing the finalizer from trafficManagerBackend", func() { + Eventually(func() error { + if err := k8sClient.Get(ctx, backendNamespacedName, backend); err != nil { + return err + } + backend.Finalizers = nil + return k8sClient.Update(ctx, backend) + }, timeout, interval).Should(Succeed(), "failed to remove trafficManagerBackend finalizer") + }) + It("Validating trafficManagerBackend is deleted", func() { validator.IsTrafficManagerBackendDeleted(ctx, k8sClient, backendNamespacedName) }) @@ -145,6 +281,11 @@ var _ = Describe("Test TrafficManagerBackend Controller", func() { Expect(k8sClient.Create(ctx, profile)).Should(Succeed()) }) + It("Updating TrafficManagerProfile status to programmed true", func() { + By("By updating TrafficManagerProfile status") + updateTrafficManagerProfileStatusToTrue(ctx, profile) + }) + It("Creating TrafficManagerBackend", func() { backend = trafficManagerBackendForTest(backendName, profileName, "not-exist") Expect(k8sClient.Create(ctx, backend)).Should(Succeed()) @@ -187,6 +328,11 @@ var _ = Describe("Test TrafficManagerBackend Controller", func() { Expect(k8sClient.Create(ctx, profile)).Should(Succeed()) }) + It("Updating TrafficManagerProfile status to programmed true", func() { + By("By updating TrafficManagerProfile status") + updateTrafficManagerProfileStatusToTrue(ctx, profile) + }) + It("Creating TrafficManagerBackend", func() { backend = trafficManagerBackendForTest(backendName, profileName, "not-exist") Expect(k8sClient.Create(ctx, backend)).Should(Succeed()) @@ -229,7 +375,7 @@ var _ = Describe("Test TrafficManagerBackend Controller", func() { Expect(k8sClient.Create(ctx, profile)).Should(Succeed()) }) - It("Updating TrafficManagerProfile status to accepted false", func() { + It("Updating TrafficManagerProfile status to programmed false", func() { By("By updating TrafficManagerProfile status") cond := metav1.Condition{ Status: metav1.ConditionFalse, @@ -255,13 +401,7 @@ var _ = Describe("Test TrafficManagerBackend Controller", func() { }, Spec: backend.Spec, Status: fleetnetv1alpha1.TrafficManagerBackendStatus{ - Conditions: []metav1.Condition{ - { - Status: metav1.ConditionFalse, - Type: string(fleetnetv1alpha1.TrafficManagerBackendReasonAccepted), - Reason: string(fleetnetv1alpha1.TrafficManagerBackendReasonInvalid), - }, - }, + Conditions: buildFalseCondition(), }, } validator.ValidateTrafficManagerBackend(ctx, k8sClient, &want) @@ -288,13 +428,7 @@ var _ = Describe("Test TrafficManagerBackend Controller", func() { }, Spec: backend.Spec, Status: fleetnetv1alpha1.TrafficManagerBackendStatus{ - Conditions: []metav1.Condition{ - { - Status: metav1.ConditionUnknown, - Type: string(fleetnetv1alpha1.TrafficManagerBackendReasonAccepted), - Reason: string(fleetnetv1alpha1.TrafficManagerBackendReasonPending), - }, - }, + Conditions: buildUnknownCondition(), }, } validator.ValidateTrafficManagerBackend(ctx, k8sClient, &want) diff --git a/pkg/controllers/hub/trafficmanagerbackend/suite_test.go b/pkg/controllers/hub/trafficmanagerbackend/suite_test.go index c2bf080d..69bf4f53 100644 --- a/pkg/controllers/hub/trafficmanagerbackend/suite_test.go +++ b/pkg/controllers/hub/trafficmanagerbackend/suite_test.go @@ -21,6 +21,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" + "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/manager" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" @@ -54,7 +55,9 @@ func TestAPIs(t *testing.T) { } var _ = BeforeSuite(func() { - klog.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + logger := zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)) + klog.SetLogger(logger) + log.SetLogger(logger) ctx, cancel = context.WithCancel(context.TODO()) diff --git a/pkg/controllers/hub/trafficmanagerprofile/suite_test.go b/pkg/controllers/hub/trafficmanagerprofile/suite_test.go index 20db6ca5..f9b7fcc9 100644 --- a/pkg/controllers/hub/trafficmanagerprofile/suite_test.go +++ b/pkg/controllers/hub/trafficmanagerprofile/suite_test.go @@ -21,6 +21,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" + "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/manager" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" @@ -53,7 +54,9 @@ func TestAPIs(t *testing.T) { } var _ = BeforeSuite(func() { - klog.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + logger := zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)) + klog.SetLogger(logger) + log.SetLogger(logger) ctx, cancel = context.WithCancel(context.TODO()) diff --git a/test/common/trafficmanager/fakeprovider/profile.go b/test/common/trafficmanager/fakeprovider/profile.go index 0aa76fce..fa4b7ad4 100644 --- a/test/common/trafficmanager/fakeprovider/profile.go +++ b/test/common/trafficmanager/fakeprovider/profile.go @@ -30,6 +30,7 @@ const ( ConflictErrProfileName = "conflict-err-profile" InternalServerErrProfileName = "internal-server-err-profile" ThrottledErrProfileName = "throttled-err-profile" + RequestTimeoutProfileName = "request-timeout-profile" ValidBackendName = "valid-backend" ServiceImportName = "test-import" @@ -116,6 +117,8 @@ func ProfileGet(_ context.Context, resourceGroupName string, profileName string, Location: ptr.To("global"), }} resp.SetResponse(http.StatusOK, profileResp, nil) + case RequestTimeoutProfileName: + errResp.SetResponseError(http.StatusRequestTimeout, "RequestTimeoutError") default: errResp.SetResponseError(http.StatusNotFound, "NotFoundError") } diff --git a/test/common/trafficmanager/validator/backend.go b/test/common/trafficmanager/validator/backend.go index 7fb2ce0b..7b283d90 100644 --- a/test/common/trafficmanager/validator/backend.go +++ b/test/common/trafficmanager/validator/backend.go @@ -67,3 +67,14 @@ func IsTrafficManagerBackendDeleted(ctx context.Context, k8sClient client.Client return nil }, timeout, interval).Should(gomega.Succeed(), "Failed to remove trafficManagerBackend %s ", name) } + +// ValidateTrafficManagerConsistentlyExist validates whether the backend consistently exists. +func ValidateTrafficManagerConsistentlyExist(ctx context.Context, k8sClient client.Client, name types.NamespacedName) { + gomega.Consistently(func() error { + backend := &fleetnetv1alpha1.TrafficManagerBackend{} + if err := k8sClient.Get(ctx, name, backend); errors.IsNotFound(err) { + return fmt.Errorf("trafficManagerBackend %s does not exist: %w", name, err) + } + return nil + }, duration, interval).Should(gomega.Succeed(), "Failed to find trafficManagerBackend %s ", name) +} diff --git a/test/common/trafficmanager/validator/profile.go b/test/common/trafficmanager/validator/profile.go index fa867749..c3a9cf60 100644 --- a/test/common/trafficmanager/validator/profile.go +++ b/test/common/trafficmanager/validator/profile.go @@ -24,6 +24,8 @@ import ( const ( timeout = time.Second * 10 interval = time.Millisecond * 250 + // duration used by consistently + duration = time.Second * 30 ) var (