From 728dd0d70e5774b3514690db63daad2f2eadef6c Mon Sep 17 00:00:00 2001 From: Zhiying Lin <54013513+zhiying-lin@users.noreply.github.com> Date: Tue, 5 Nov 2024 09:51:43 +0800 Subject: [PATCH] feat: handle invalid profile when handling backend update (#207) --- .../hub/trafficmanagerbackend/controller.go | 82 +++++++++++- .../controller_integration_test.go | 125 +++++++++++++++++- .../trafficmanager/validator/backend.go | 24 ++++ .../trafficmanager/validator/profile.go | 8 +- 4 files changed, 233 insertions(+), 6 deletions(-) diff --git a/pkg/controllers/hub/trafficmanagerbackend/controller.go b/pkg/controllers/hub/trafficmanagerbackend/controller.go index 443be06e..1c1a2573 100644 --- a/pkg/controllers/hub/trafficmanagerbackend/controller.go +++ b/pkg/controllers/hub/trafficmanagerbackend/controller.go @@ -16,6 +16,8 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/trafficmanager/armtrafficmanager" "golang.org/x/sync/errgroup" apierrors "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/klog/v2" ctrl "sigs.k8s.io/controller-runtime" @@ -24,6 +26,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/reconcile" + "go.goms.io/fleet/pkg/utils/condition" "go.goms.io/fleet/pkg/utils/controller" fleetnetv1alpha1 "go.goms.io/fleet-networking/api/v1alpha1" @@ -200,10 +203,87 @@ func isEndpointOwnedByBackend(backend *fleetnetv1alpha1.TrafficManagerBackend, e return strings.HasPrefix(strings.ToLower(endpoint), generateAzureTrafficManagerEndpointNamePrefixFunc(backend)) } -func (r *Reconciler) handleUpdate(_ context.Context, _ *fleetnetv1alpha1.TrafficManagerBackend) (ctrl.Result, error) { +func (r *Reconciler) handleUpdate(ctx context.Context, backend *fleetnetv1alpha1.TrafficManagerBackend) (ctrl.Result, error) { + backendKObj := klog.KObj(backend) + profile, err := r.validateTrafficManagerProfile(ctx, backend) + if err != nil || profile == nil { + // We don't need to requeue the invalid Profile (err == nil and profile == nil) because 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 trafficManagerProfile", "trafficManagerBackend", backendKObj, "trafficManagerProfile", klog.KObj(profile)) return ctrl.Result{}, nil } +// validateTrafficManagerProfile returns not nil profile when the profile is valid. +func (r *Reconciler) validateTrafficManagerProfile(ctx context.Context, backend *fleetnetv1alpha1.TrafficManagerBackend) (*fleetnetv1alpha1.TrafficManagerProfile, error) { + backendKObj := klog.KObj(backend) + var cond metav1.Condition + profile := &fleetnetv1alpha1.TrafficManagerProfile{} + if getProfileErr := r.Client.Get(ctx, types.NamespacedName{Name: backend.Spec.Profile.Name, Namespace: backend.Namespace}, profile); getProfileErr != nil { + if apierrors.IsNotFound(getProfileErr) { + klog.V(2).InfoS("NotFound trafficManagerProfile", "trafficManagerBackend", backendKObj, "trafficManagerProfile", backend.Spec.Profile.Name) + setFalseCondition(backend, nil, fmt.Sprintf("TrafficManagerProfile %q is not found", backend.Spec.Profile.Name)) + return nil, r.updateTrafficManagerBackendStatus(ctx, backend) + } + klog.ErrorS(getProfileErr, "Failed to get trafficManagerProfile", "trafficManagerBackend", backendKObj, "trafficManagerProfile", backend.Spec.Profile.Name) + setUnknownCondition(backend, fmt.Sprintf("Failed to get the trafficManagerProfile %q: %v", backend.Spec.Profile.Name, getProfileErr)) + if err := r.updateTrafficManagerBackendStatus(ctx, backend); err != nil { + return nil, err + } + return nil, getProfileErr // need to return the error to requeue the request + } + programmedCondition := meta.FindStatusCondition(profile.Status.Conditions, string(fleetnetv1alpha1.TrafficManagerProfileConditionProgrammed)) + if condition.IsConditionStatusTrue(programmedCondition, profile.GetGeneration()) { + return profile, nil // return directly if the trafficManagerProfile is programmed + } else if condition.IsConditionStatusFalse(programmedCondition, profile.GetGeneration()) { + setFalseCondition(backend, nil, fmt.Sprintf("Invalid trafficManagerProfile %q: %v", backend.Spec.Profile.Name, programmedCondition.Message)) + } else { + setUnknownCondition(backend, fmt.Sprintf("In the processing of trafficManagerProfile %q", backend.Spec.Profile.Name)) + } + klog.V(2).InfoS("Profile has not been accepted and updating the status", "trafficManagerBackend", backendKObj, "condition", cond) + return nil, r.updateTrafficManagerBackendStatus(ctx, backend) +} + +func setFalseCondition(backend *fleetnetv1alpha1.TrafficManagerBackend, acceptedEndpoints []fleetnetv1alpha1.TrafficManagerEndpointStatus, message string) { + cond := metav1.Condition{ + Type: string(fleetnetv1alpha1.TrafficManagerBackendConditionAccepted), + Status: metav1.ConditionFalse, + ObservedGeneration: backend.Generation, + Reason: string(fleetnetv1alpha1.TrafficManagerBackendReasonInvalid), + Message: message, + } + if len(acceptedEndpoints) == 0 { + backend.Status.Endpoints = []fleetnetv1alpha1.TrafficManagerEndpointStatus{} + } else { + backend.Status.Endpoints = acceptedEndpoints + } + meta.SetStatusCondition(&backend.Status.Conditions, cond) +} + +func setUnknownCondition(backend *fleetnetv1alpha1.TrafficManagerBackend, message string) { + cond := metav1.Condition{ + Type: string(fleetnetv1alpha1.TrafficManagerBackendConditionAccepted), + Status: metav1.ConditionUnknown, + ObservedGeneration: backend.Generation, + Reason: string(fleetnetv1alpha1.TrafficManagerBackendReasonPending), + Message: message, + } + backend.Status.Endpoints = []fleetnetv1alpha1.TrafficManagerEndpointStatus{} + meta.SetStatusCondition(&backend.Status.Conditions, cond) +} + +func (r *Reconciler) updateTrafficManagerBackendStatus(ctx context.Context, backend *fleetnetv1alpha1.TrafficManagerBackend) error { + backendKObj := klog.KObj(backend) + if err := r.Client.Status().Update(ctx, backend); err != nil { + klog.ErrorS(err, "Failed to update trafficManagerBackend status", "trafficManagerBackend", backendKObj) + return controller.NewUpdateIgnoreConflictError(err) + } + klog.V(2).InfoS("Updated trafficManagerBackend status", "trafficManagerBackend", backendKObj, "status", backend.Status) + return nil +} + // SetupWithManager sets up the controller with the Manager. func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager) error { // set up an index for efficient trafficManagerBackend lookup diff --git a/pkg/controllers/hub/trafficmanagerbackend/controller_integration_test.go b/pkg/controllers/hub/trafficmanagerbackend/controller_integration_test.go index 1e44f9dd..0cc99100 100644 --- a/pkg/controllers/hub/trafficmanagerbackend/controller_integration_test.go +++ b/pkg/controllers/hub/trafficmanagerbackend/controller_integration_test.go @@ -8,11 +8,13 @@ package trafficmanagerbackend import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" fleetnetv1alpha1 "go.goms.io/fleet-networking/api/v1alpha1" + "go.goms.io/fleet-networking/pkg/common/objectmeta" "go.goms.io/fleet-networking/test/common/trafficmanager/fakeprovider" "go.goms.io/fleet-networking/test/common/trafficmanager/validator" ) @@ -57,7 +59,24 @@ var _ = Describe("Test TrafficManagerBackend Controller", func() { }) It("Validating trafficManagerBackend", func() { - validator.IsTrafficManagerBackendFinalizerAdded(ctx, k8sClient, namespacedName) + want := fleetnetv1alpha1.TrafficManagerBackend{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: testNamespace, + Finalizers: []string{objectmeta.TrafficManagerBackendFinalizer}, + }, + Spec: backend.Spec, + Status: fleetnetv1alpha1.TrafficManagerBackendStatus{ + Conditions: []metav1.Condition{ + { + Status: metav1.ConditionFalse, + Type: string(fleetnetv1alpha1.TrafficManagerBackendReasonAccepted), + Reason: string(fleetnetv1alpha1.TrafficManagerBackendReasonInvalid), + }, + }, + }, + } + validator.ValidateTrafficManagerBackend(ctx, k8sClient, &want) }) It("Deleting trafficManagerBackend", func() { @@ -195,4 +214,108 @@ var _ = Describe("Test TrafficManagerBackend Controller", func() { validator.IsTrafficManagerProfileDeleted(ctx, k8sClient, profileNamespacedName) }) }) + + Context("When creating trafficManagerBackend with not accepted profile", Ordered, func() { + profileName := fakeprovider.ValidProfileWithEndpointsName + 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 accepted false", func() { + By("By updating TrafficManagerProfile status") + cond := metav1.Condition{ + Status: metav1.ConditionFalse, + Type: string(fleetnetv1alpha1.TrafficManagerProfileConditionProgrammed), + ObservedGeneration: profile.Generation, + Reason: string(fleetnetv1alpha1.TrafficManagerProfileReasonInvalid), + } + meta.SetStatusCondition(&profile.Status.Conditions, cond) + Expect(k8sClient.Status().Update(ctx, profile)).Should(Succeed()) + }) + + 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: []metav1.Condition{ + { + Status: metav1.ConditionFalse, + Type: string(fleetnetv1alpha1.TrafficManagerBackendReasonAccepted), + Reason: string(fleetnetv1alpha1.TrafficManagerBackendReasonInvalid), + }, + }, + }, + } + validator.ValidateTrafficManagerBackend(ctx, k8sClient, &want) + }) + + It("Updating TrafficManagerProfile status to accepted unknown and it should trigger controller", func() { + By("By updating TrafficManagerProfile status") + cond := metav1.Condition{ + Status: metav1.ConditionUnknown, + Type: string(fleetnetv1alpha1.TrafficManagerProfileConditionProgrammed), + ObservedGeneration: profile.Generation, + Reason: string(fleetnetv1alpha1.TrafficManagerProfileReasonPending), + } + meta.SetStatusCondition(&profile.Status.Conditions, cond) + Expect(k8sClient.Status().Update(ctx, profile)).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: []metav1.Condition{ + { + Status: metav1.ConditionUnknown, + Type: string(fleetnetv1alpha1.TrafficManagerBackendReasonAccepted), + Reason: string(fleetnetv1alpha1.TrafficManagerBackendReasonPending), + }, + }, + }, + } + 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) + }) + }) }) diff --git a/test/common/trafficmanager/validator/backend.go b/test/common/trafficmanager/validator/backend.go index a36fd33a..7fb2ce0b 100644 --- a/test/common/trafficmanager/validator/backend.go +++ b/test/common/trafficmanager/validator/backend.go @@ -9,6 +9,8 @@ import ( "context" "fmt" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/onsi/gomega" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" @@ -19,6 +21,13 @@ import ( "go.goms.io/fleet-networking/pkg/common/objectmeta" ) +var ( + cmpTrafficManagerBackendOptions = cmp.Options{ + commonCmpOptions, + cmpopts.IgnoreFields(fleetnetv1alpha1.TrafficManagerBackend{}, "TypeMeta"), + } +) + // IsTrafficManagerBackendFinalizerAdded validates whether the backend is created with the finalizer or not. func IsTrafficManagerBackendFinalizerAdded(ctx context.Context, k8sClient client.Client, name types.NamespacedName) { gomega.Eventually(func() error { @@ -33,6 +42,21 @@ func IsTrafficManagerBackendFinalizerAdded(ctx context.Context, k8sClient client }, timeout, interval).Should(gomega.Succeed(), "Failed to add finalizer to trafficManagerBackend %s", name) } +// ValidateTrafficManagerBackend validates the trafficManagerBackend object. +func ValidateTrafficManagerBackend(ctx context.Context, k8sClient client.Client, want *fleetnetv1alpha1.TrafficManagerBackend) { + key := types.NamespacedName{Name: want.Name, Namespace: want.Namespace} + backend := &fleetnetv1alpha1.TrafficManagerBackend{} + gomega.Eventually(func() error { + if err := k8sClient.Get(ctx, key, backend); err != nil { + return err + } + if diff := cmp.Diff(want, backend, cmpTrafficManagerBackendOptions); diff != "" { + return fmt.Errorf("trafficManagerBackend mismatch (-want, +got) :\n%s", diff) + } + return nil + }, timeout, interval).Should(gomega.Succeed(), "Get() trafficManagerBackend mismatch") +} + // IsTrafficManagerBackendDeleted validates whether the backend is deleted or not. func IsTrafficManagerBackendDeleted(ctx context.Context, k8sClient client.Client, name types.NamespacedName) { gomega.Eventually(func() error { diff --git a/test/common/trafficmanager/validator/profile.go b/test/common/trafficmanager/validator/profile.go index 84a06603..fa867749 100644 --- a/test/common/trafficmanager/validator/profile.go +++ b/test/common/trafficmanager/validator/profile.go @@ -30,15 +30,15 @@ var ( commonCmpOptions = cmp.Options{ cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion", "UID", "CreationTimestamp", "ManagedFields", "Generation"), cmpopts.IgnoreFields(metav1.OwnerReference{}, "UID"), - } - cmpTrafficManagerProfileOptions = cmp.Options{ - commonCmpOptions, - cmpopts.IgnoreFields(fleetnetv1alpha1.TrafficManagerProfile{}, "TypeMeta"), cmpopts.IgnoreFields(metav1.Condition{}, "Message", "LastTransitionTime", "ObservedGeneration"), cmpopts.SortSlices(func(c1, c2 metav1.Condition) bool { return c1.Type < c2.Type }), } + cmpTrafficManagerProfileOptions = cmp.Options{ + commonCmpOptions, + cmpopts.IgnoreFields(fleetnetv1alpha1.TrafficManagerProfile{}, "TypeMeta"), + } ) // ValidateTrafficManagerProfile validates the trafficManagerProfile object.