From 03aca68f2d20a87cf4e3d5c96a80090f9def4195 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Ma=C5=82ek?= Date: Sun, 20 Oct 2024 20:07:43 +0200 Subject: [PATCH] fix(konnect): make setting status conditions not update on every reconciliation --- .../konnect/reconciler_certificateref.go | 13 +- controller/konnect/reconciler_generic.go | 68 ++--- controller/konnect/reconciler_keysetref.go | 9 +- .../konnect/reconciler_konnectapiauth.go | 7 +- controller/konnect/reconciler_serviceref.go | 13 +- controller/konnect/reconciler_upstreamref.go | 13 +- controller/pkg/patch/statuscondition.go | 63 ++++ controller/pkg/patch/statuscondition_test.go | 288 ++++++++++++++++++ 8 files changed, 399 insertions(+), 75 deletions(-) create mode 100644 controller/pkg/patch/statuscondition.go create mode 100644 controller/pkg/patch/statuscondition_test.go diff --git a/controller/konnect/reconciler_certificateref.go b/controller/konnect/reconciler_certificateref.go index 382652085..38eaae036 100644 --- a/controller/konnect/reconciler_certificateref.go +++ b/controller/konnect/reconciler_certificateref.go @@ -13,6 +13,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/kong/gateway-operator/controller/konnect/constraints" + "github.com/kong/gateway-operator/controller/pkg/patch" k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes" configurationv1alpha1 "github.com/kong/kubernetes-configuration/api/configuration/v1alpha1" @@ -50,7 +51,7 @@ func handleKongCertificateRef[T constraints.SupportedKonnectEntityType, TEnt con } err := cl.Get(ctx, nn, cert) if err != nil { - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.KongCertificateRefValidConditionType, metav1.ConditionFalse, @@ -79,7 +80,7 @@ func handleKongCertificateRef[T constraints.SupportedKonnectEntityType, TEnt con cond, ok := k8sutils.GetCondition(konnectv1alpha1.KonnectEntityProgrammedConditionType, cert) if !ok || cond.Status != metav1.ConditionTrue { ent.SetKonnectID("") - if res, err := updateStatusWithCondition( + if res, err := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.KongCertificateRefValidConditionType, metav1.ConditionFalse, @@ -111,7 +112,7 @@ func handleKongCertificateRef[T constraints.SupportedKonnectEntityType, TEnt con sni.Status.Konnect.CertificateID = cert.GetKonnectID() } - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.KongCertificateRefValidConditionType, metav1.ConditionTrue, @@ -133,7 +134,7 @@ func handleKongCertificateRef[T constraints.SupportedKonnectEntityType, TEnt con } cp, err := getCPForRef(ctx, cl, cpRef, ent.GetNamespace()) if err != nil { - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.ControlPlaneRefValidConditionType, metav1.ConditionFalse, @@ -156,7 +157,7 @@ func handleKongCertificateRef[T constraints.SupportedKonnectEntityType, TEnt con cond, ok = k8sutils.GetCondition(konnectv1alpha1.KonnectEntityProgrammedConditionType, cp) if !ok || cond.Status != metav1.ConditionTrue || cond.ObservedGeneration != cp.GetGeneration() { - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.ControlPlaneRefValidConditionType, metav1.ConditionFalse, @@ -173,7 +174,7 @@ func handleKongCertificateRef[T constraints.SupportedKonnectEntityType, TEnt con resource.SetControlPlaneID(cp.Status.ID) } - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.ControlPlaneRefValidConditionType, metav1.ConditionTrue, diff --git a/controller/konnect/reconciler_generic.go b/controller/konnect/reconciler_generic.go index 38532c804..e80c47def 100644 --- a/controller/konnect/reconciler_generic.go +++ b/controller/konnect/reconciler_generic.go @@ -20,6 +20,7 @@ import ( "github.com/kong/gateway-operator/controller/konnect/constraints" "github.com/kong/gateway-operator/controller/konnect/ops" "github.com/kong/gateway-operator/controller/pkg/log" + "github.com/kong/gateway-operator/controller/pkg/patch" "github.com/kong/gateway-operator/pkg/consts" k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes" @@ -336,7 +337,7 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile( var apiAuth konnectv1alpha1.KonnectAPIAuthConfiguration if err := r.Client.Get(ctx, apiAuthRef, &apiAuth); err != nil { if k8serrors.IsNotFound(err) { - if res, err := updateStatusWithCondition( + if res, err := patch.StatusWithCondition( ctx, r.Client, ent, konnectv1alpha1.KonnectEntityAPIAuthConfigurationResolvedRefConditionType, metav1.ConditionFalse, @@ -349,7 +350,7 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile( return ctrl.Result{}, nil } - if res, err := updateStatusWithCondition( + if res, err := patch.StatusWithCondition( ctx, r.Client, ent, konnectv1alpha1.KonnectEntityAPIAuthConfigurationResolvedRefConditionType, metav1.ConditionFalse, @@ -367,7 +368,7 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile( cond.Status != metav1.ConditionTrue || cond.ObservedGeneration != ent.GetGeneration() || cond.Reason != konnectv1alpha1.KonnectEntityAPIAuthConfigurationResolvedRefReasonResolvedRef { - if res, err := updateStatusWithCondition( + if res, err := patch.StatusWithCondition( ctx, r.Client, ent, konnectv1alpha1.KonnectEntityAPIAuthConfigurationResolvedRefConditionType, metav1.ConditionTrue, @@ -386,7 +387,7 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile( // If it's invalid then set the "APIAuthValid" status condition on // the entity to False with "Invalid" reason. - if res, err := updateStatusWithCondition( + if res, err := patch.StatusWithCondition( ctx, r.Client, ent, konnectv1alpha1.KonnectEntityAPIAuthConfigurationValidConditionType, metav1.ConditionFalse, @@ -408,7 +409,7 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile( cond.ObservedGeneration != ent.GetGeneration() || cond.Message != conditionMessageReferenceKonnectAPIAuthConfigurationValid(apiAuthRef) { - if res, err := updateStatusWithCondition( + if res, err := patch.StatusWithCondition( ctx, r.Client, ent, konnectv1alpha1.KonnectEntityAPIAuthConfigurationValidConditionType, metav1.ConditionTrue, @@ -422,7 +423,7 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile( token, err := getTokenFromKonnectAPIAuthConfiguration(ctx, r.Client, &apiAuth) if err != nil { - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, r.Client, &apiAuth, konnectv1alpha1.KonnectEntityAPIAuthConfigurationValidConditionType, metav1.ConditionFalse, @@ -458,7 +459,7 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile( if controllerutil.RemoveFinalizer(ent, KonnectCleanupFinalizer) { if err := ops.Delete[T, TEnt](ctx, sdk, r.Client, ent); err != nil { - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, r.Client, ent, konnectv1alpha1.KonnectEntityProgrammedConditionType, metav1.ConditionFalse, @@ -522,7 +523,7 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile( } // Regardless of the error, set the status as it can contain the Konnect ID - // and/or status conditions set in Create() and that ID will be needed to + // and/or status ops set in Create() and that ID will be needed to // update/delete the object in Konnect. if err := r.Client.Status().Patch(ctx, ent, client.MergeFrom(obj)); err != nil { if k8serrors.IsConflict(err) { @@ -589,39 +590,6 @@ type EntityWithControlPlaneRef interface { GetControlPlaneID() string } -func updateStatusWithCondition[T interface { - client.Object - k8sutils.ConditionsAware -}]( - ctx context.Context, - client client.Client, - ent T, - conditionType consts.ConditionType, - conditionStatus metav1.ConditionStatus, - conditionReason consts.ConditionReason, - conditionMessage string, -) (ctrl.Result, error) { - k8sutils.SetCondition( - k8sutils.NewConditionWithGeneration( - conditionType, - conditionStatus, - conditionReason, - conditionMessage, - ent.GetGeneration(), - ), - ent, - ) - - if err := client.Status().Update(ctx, ent); err != nil { - if k8serrors.IsConflict(err) { - return ctrl.Result{Requeue: true}, nil - } - return ctrl.Result{}, fmt.Errorf("failed to update status with %s condition: %w", conditionType, err) - } - - return ctrl.Result{}, nil -} - func getCPForRef( ctx context.Context, cl client.Client, @@ -819,7 +787,7 @@ func handleKongConsumerRef[T constraints.SupportedKonnectEntityType, TEnt constr } if err := cl.Get(ctx, nn, &consumer); err != nil { - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.KongConsumerRefValidConditionType, metav1.ConditionFalse, @@ -847,7 +815,7 @@ func handleKongConsumerRef[T constraints.SupportedKonnectEntityType, TEnt constr cond, ok := k8sutils.GetCondition(konnectv1alpha1.KonnectEntityProgrammedConditionType, &consumer) if !ok || cond.Status != metav1.ConditionTrue { ent.SetKonnectID("") - if res, err := updateStatusWithCondition( + if res, err := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.KongConsumerRefValidConditionType, metav1.ConditionFalse, @@ -882,7 +850,7 @@ func handleKongConsumerRef[T constraints.SupportedKonnectEntityType, TEnt constr ) } - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.KongConsumerRefValidConditionType, metav1.ConditionTrue, @@ -901,7 +869,7 @@ func handleKongConsumerRef[T constraints.SupportedKonnectEntityType, TEnt constr } cp, err := getCPForRef(ctx, cl, cpRef, ent.GetNamespace()) if err != nil { - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.ControlPlaneRefValidConditionType, metav1.ConditionFalse, @@ -921,7 +889,7 @@ func handleKongConsumerRef[T constraints.SupportedKonnectEntityType, TEnt constr cond, ok = k8sutils.GetCondition(konnectv1alpha1.KonnectEntityProgrammedConditionType, cp) if !ok || cond.Status != metav1.ConditionTrue || cond.ObservedGeneration != cp.GetGeneration() { - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.ControlPlaneRefValidConditionType, metav1.ConditionFalse, @@ -938,7 +906,7 @@ func handleKongConsumerRef[T constraints.SupportedKonnectEntityType, TEnt constr resource.SetControlPlaneID(cp.Status.ID) } - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.ControlPlaneRefValidConditionType, metav1.ConditionTrue, @@ -1047,7 +1015,7 @@ func handleControlPlaneRef[T constraints.SupportedKonnectEntityType, TEnt constr nn.Namespace = cpRef.KonnectNamespacedRef.Namespace } if err := cl.Get(ctx, nn, &cp); err != nil { - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.ControlPlaneRefValidConditionType, metav1.ConditionFalse, @@ -1067,7 +1035,7 @@ func handleControlPlaneRef[T constraints.SupportedKonnectEntityType, TEnt constr cond, ok := k8sutils.GetCondition(konnectv1alpha1.KonnectEntityProgrammedConditionType, &cp) if !ok || cond.Status != metav1.ConditionTrue || cond.ObservedGeneration != cp.GetGeneration() { - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.ControlPlaneRefValidConditionType, metav1.ConditionFalse, @@ -1111,7 +1079,7 @@ func handleControlPlaneRef[T constraints.SupportedKonnectEntityType, TEnt constr resource.SetControlPlaneID(cp.Status.ID) } - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.ControlPlaneRefValidConditionType, metav1.ConditionTrue, diff --git a/controller/konnect/reconciler_keysetref.go b/controller/konnect/reconciler_keysetref.go index a0fc00e59..6c4a044b1 100644 --- a/controller/konnect/reconciler_keysetref.go +++ b/controller/konnect/reconciler_keysetref.go @@ -14,6 +14,7 @@ import ( ctrllog "sigs.k8s.io/controller-runtime/pkg/log" "github.com/kong/gateway-operator/controller/konnect/constraints" + "github.com/kong/gateway-operator/controller/pkg/patch" k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes" configurationv1alpha1 "github.com/kong/kubernetes-configuration/api/configuration/v1alpha1" @@ -34,7 +35,7 @@ func handleKongKeySetRef[T constraints.SupportedKonnectEntityType, TEnt constrai if key.Status.Konnect != nil && key.Status.Konnect.GetKeySetID() != "" { // Reset the KeySetID in the status and set the condition to True. key.Status.Konnect.KeySetID = "" - if res, err := updateStatusWithCondition(ctx, cl, key, + if res, err := patch.StatusWithCondition(ctx, cl, key, konnectv1alpha1.KeySetRefValidConditionType, metav1.ConditionTrue, konnectv1alpha1.KeySetRefReasonValid, @@ -70,7 +71,7 @@ func handleKongKeySetRef[T constraints.SupportedKonnectEntityType, TEnt constrai Namespace: ent.GetNamespace(), } if err := cl.Get(ctx, nn, &keySet); err != nil { - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.KeySetRefValidConditionType, metav1.ConditionFalse, @@ -102,7 +103,7 @@ func handleKongKeySetRef[T constraints.SupportedKonnectEntityType, TEnt constrai // Verify that the KongKeySet is programmed. cond, ok := k8sutils.GetCondition(konnectv1alpha1.KonnectEntityProgrammedConditionType, &keySet) if !ok || cond.Status != metav1.ConditionTrue { - if res, err := updateStatusWithCondition( + if res, err := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.KeySetRefValidConditionType, metav1.ConditionFalse, @@ -133,7 +134,7 @@ func handleKongKeySetRef[T constraints.SupportedKonnectEntityType, TEnt constrai key.Status.Konnect.KeySetID = keySet.Status.Konnect.GetKonnectID() } - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.KeySetRefValidConditionType, metav1.ConditionTrue, diff --git a/controller/konnect/reconciler_konnectapiauth.go b/controller/konnect/reconciler_konnectapiauth.go index e4adf584c..27f075984 100644 --- a/controller/konnect/reconciler_konnectapiauth.go +++ b/controller/konnect/reconciler_konnectapiauth.go @@ -17,6 +17,7 @@ import ( "github.com/kong/gateway-operator/controller/konnect/ops" "github.com/kong/gateway-operator/controller/pkg/log" + "github.com/kong/gateway-operator/controller/pkg/patch" k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes" konnectv1alpha1 "github.com/kong/kubernetes-configuration/api/konnect/v1alpha1" @@ -114,7 +115,7 @@ func (r *KonnectAPIAuthConfigurationReconciler) Reconcile( token, err := getTokenFromKonnectAPIAuthConfiguration(ctx, r.client, &apiAuth) if err != nil { - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, r.client, &apiAuth, konnectv1alpha1.KonnectEntityAPIAuthConfigurationValidConditionType, metav1.ConditionFalse, @@ -154,7 +155,7 @@ func (r *KonnectAPIAuthConfigurationReconciler) Reconcile( apiAuth.Status.OrganizationID = "" apiAuth.Status.ServerURL = serverURL.String() - res, errUpdate := updateStatusWithCondition( + res, errUpdate := patch.StatusWithCondition( ctx, r.client, &apiAuth, konnectv1alpha1.KonnectEntityAPIAuthConfigurationValidConditionType, metav1.ConditionFalse, @@ -192,7 +193,7 @@ func (r *KonnectAPIAuthConfigurationReconciler) Reconcile( apiAuth.Status.OrganizationID = *respOrg.MeOrganization.ID apiAuth.Status.ServerURL = serverURL.String() - res, err := updateStatusWithCondition( + res, err := patch.StatusWithCondition( ctx, r.client, &apiAuth, konnectv1alpha1.KonnectEntityAPIAuthConfigurationValidConditionType, metav1.ConditionTrue, diff --git a/controller/konnect/reconciler_serviceref.go b/controller/konnect/reconciler_serviceref.go index 3d2e1ac41..7b84ac373 100644 --- a/controller/konnect/reconciler_serviceref.go +++ b/controller/konnect/reconciler_serviceref.go @@ -13,6 +13,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/kong/gateway-operator/controller/konnect/constraints" + "github.com/kong/gateway-operator/controller/pkg/patch" k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes" configurationv1alpha1 "github.com/kong/kubernetes-configuration/api/configuration/v1alpha1" @@ -55,7 +56,7 @@ func handleKongServiceRef[T constraints.SupportedKonnectEntityType, TEnt constra } if err := cl.Get(ctx, nn, &svc); err != nil { - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.KongServiceRefValidConditionType, metav1.ConditionFalse, @@ -79,7 +80,7 @@ func handleKongServiceRef[T constraints.SupportedKonnectEntityType, TEnt constra cond, ok := k8sutils.GetCondition(konnectv1alpha1.KonnectEntityProgrammedConditionType, &svc) if !ok || cond.Status != metav1.ConditionTrue { ent.SetKonnectID("") - if res, err := updateStatusWithCondition( + if res, err := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.KongServiceRefValidConditionType, metav1.ConditionFalse, @@ -112,7 +113,7 @@ func handleKongServiceRef[T constraints.SupportedKonnectEntityType, TEnt constra route.Status.Konnect.ServiceID = svc.Status.Konnect.GetKonnectID() } - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.KongServiceRefValidConditionType, metav1.ConditionTrue, @@ -131,7 +132,7 @@ func handleKongServiceRef[T constraints.SupportedKonnectEntityType, TEnt constra } cp, err := getCPForRef(ctx, cl, cpRef, ent.GetNamespace()) if err != nil { - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.ControlPlaneRefValidConditionType, metav1.ConditionFalse, @@ -151,7 +152,7 @@ func handleKongServiceRef[T constraints.SupportedKonnectEntityType, TEnt constra cond, ok = k8sutils.GetCondition(konnectv1alpha1.KonnectEntityProgrammedConditionType, cp) if !ok || cond.Status != metav1.ConditionTrue || cond.ObservedGeneration != cp.GetGeneration() { - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.ControlPlaneRefValidConditionType, metav1.ConditionFalse, @@ -171,7 +172,7 @@ func handleKongServiceRef[T constraints.SupportedKonnectEntityType, TEnt constra resource.SetControlPlaneID(cp.Status.ID) } - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.ControlPlaneRefValidConditionType, metav1.ConditionTrue, diff --git a/controller/konnect/reconciler_upstreamref.go b/controller/konnect/reconciler_upstreamref.go index e196a102d..ec1a4d459 100644 --- a/controller/konnect/reconciler_upstreamref.go +++ b/controller/konnect/reconciler_upstreamref.go @@ -13,6 +13,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/kong/gateway-operator/controller/konnect/constraints" + "github.com/kong/gateway-operator/controller/pkg/patch" k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes" configurationv1alpha1 "github.com/kong/kubernetes-configuration/api/configuration/v1alpha1" @@ -52,7 +53,7 @@ func handleKongUpstreamRef[T constraints.SupportedKonnectEntityType, TEnt constr } err := cl.Get(ctx, nn, kongUpstream) if err != nil { - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.KongUpstreamRefValidConditionType, metav1.ConditionFalse, @@ -81,7 +82,7 @@ func handleKongUpstreamRef[T constraints.SupportedKonnectEntityType, TEnt constr cond, ok := k8sutils.GetCondition(konnectv1alpha1.KonnectEntityProgrammedConditionType, kongUpstream) if !ok || cond.Status != metav1.ConditionTrue { ent.SetKonnectID("") - if res, err := updateStatusWithCondition( + if res, err := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.KongUpstreamRefValidConditionType, metav1.ConditionFalse, @@ -113,7 +114,7 @@ func handleKongUpstreamRef[T constraints.SupportedKonnectEntityType, TEnt constr target.Status.Konnect.UpstreamID = kongUpstream.GetKonnectID() } - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.KongUpstreamRefValidConditionType, metav1.ConditionTrue, @@ -135,7 +136,7 @@ func handleKongUpstreamRef[T constraints.SupportedKonnectEntityType, TEnt constr } cp, err := getCPForRef(ctx, cl, cpRef, ent.GetNamespace()) if err != nil { - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.ControlPlaneRefValidConditionType, metav1.ConditionFalse, @@ -158,7 +159,7 @@ func handleKongUpstreamRef[T constraints.SupportedKonnectEntityType, TEnt constr cond, ok = k8sutils.GetCondition(konnectv1alpha1.KonnectEntityProgrammedConditionType, cp) if !ok || cond.Status != metav1.ConditionTrue || cond.ObservedGeneration != cp.GetGeneration() { - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.ControlPlaneRefValidConditionType, metav1.ConditionFalse, @@ -175,7 +176,7 @@ func handleKongUpstreamRef[T constraints.SupportedKonnectEntityType, TEnt constr resource.SetControlPlaneID(cp.Status.ID) } - if res, errStatus := updateStatusWithCondition( + if res, errStatus := patch.StatusWithCondition( ctx, cl, ent, konnectv1alpha1.ControlPlaneRefValidConditionType, metav1.ConditionTrue, diff --git a/controller/pkg/patch/statuscondition.go b/controller/pkg/patch/statuscondition.go new file mode 100644 index 000000000..940ed2527 --- /dev/null +++ b/controller/pkg/patch/statuscondition.go @@ -0,0 +1,63 @@ +package patch + +import ( + "context" + "fmt" + + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/kong/gateway-operator/pkg/consts" + k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes" +) + +// StatusWithCondition patches the status of the provided object with the +// given condition. +// If the condition is already set and it's as expected, it returns without patching. +func StatusWithCondition[T interface { + client.Object + k8sutils.ConditionsAware +}]( + ctx context.Context, + cl client.Client, + ent T, + conditionType consts.ConditionType, + conditionStatus metav1.ConditionStatus, + conditionReason consts.ConditionReason, + conditionMessage string, +) (ctrl.Result, error) { + cond, ok := k8sutils.GetCondition(conditionType, ent) + if ok && + cond.Status == conditionStatus && + cond.Reason == string(conditionReason) && + cond.Message == conditionMessage && + cond.ObservedGeneration == ent.GetGeneration() { + // If the condition is already set and it's as expected, return. + // We don't want to bump the condition's LastTransitionTime which + // could cause unnecessary requeues. + return ctrl.Result{}, nil + } + + old := ent.DeepCopyObject().(client.Object) + k8sutils.SetCondition( + k8sutils.NewConditionWithGeneration( + conditionType, + conditionStatus, + conditionReason, + conditionMessage, + ent.GetGeneration(), + ), + ent, + ) + + if err := cl.Status().Patch(ctx, ent, client.MergeFrom(old)); err != nil { + if k8serrors.IsConflict(err) { + return ctrl.Result{Requeue: true}, nil + } + return ctrl.Result{}, fmt.Errorf("failed to patch status with %s condition: %w", conditionType, err) + } + + return ctrl.Result{}, nil +} diff --git a/controller/pkg/patch/statuscondition_test.go b/controller/pkg/patch/statuscondition_test.go new file mode 100644 index 000000000..f1510601c --- /dev/null +++ b/controller/pkg/patch/statuscondition_test.go @@ -0,0 +1,288 @@ +package patch + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + operatorv1beta1 "github.com/kong/gateway-operator/api/v1beta1" + "github.com/kong/gateway-operator/modules/manager/scheme" + "github.com/kong/gateway-operator/pkg/consts" + k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes" +) + +func TestPatchStatusWithCondition(t *testing.T) { + tests := []struct { + name string + obj interface { + client.Object + GetConditions() []metav1.Condition + SetConditions([]metav1.Condition) + } + conditionType consts.ConditionType + conditionStatus metav1.ConditionStatus + conditionReason consts.ConditionReason + conditionMessage string + expectedResult ctrl.Result + expectedConditions []metav1.Condition + expectedError bool + interceptorFunc interceptor.Funcs + }{ + { + name: "condition is already set and as expected", + obj: &operatorv1beta1.DataPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dp1", + Generation: 1, + }, + Status: operatorv1beta1.DataPlaneStatus{ + Conditions: []metav1.Condition{ + { + Type: string(consts.ReadyType), + Status: metav1.ConditionTrue, + Reason: string(consts.ResolvedRefsReason), + Message: "Resource is available", + ObservedGeneration: 1, + }, + }, + }, + }, + conditionType: consts.ReadyType, + conditionStatus: metav1.ConditionTrue, + conditionReason: consts.ResolvedRefsReason, + conditionMessage: "Resource is available", + expectedResult: ctrl.Result{}, + expectedConditions: []metav1.Condition{ + { + Type: string(consts.ReadyType), + Status: metav1.ConditionTrue, + Reason: string(consts.ResolvedRefsReason), + Message: "Resource is available", + ObservedGeneration: 1, + }, + }, + }, + { + name: "condition needs to be updated due to different condition status", + obj: &operatorv1beta1.DataPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dp1", + Generation: 1, + }, + Status: operatorv1beta1.DataPlaneStatus{ + Conditions: []metav1.Condition{ + { + Type: string(consts.ReadyType), + Status: metav1.ConditionFalse, + Reason: string(consts.ResolvedRefsReason), + Message: "", + ObservedGeneration: 1, + }, + }, + }, + }, + conditionType: consts.ReadyType, + conditionStatus: metav1.ConditionTrue, + conditionReason: consts.ResolvedRefsReason, + conditionMessage: "", + expectedResult: ctrl.Result{}, + expectedConditions: []metav1.Condition{ + { + Type: string(consts.ReadyType), + Status: metav1.ConditionTrue, + Reason: string(consts.ResolvedRefsReason), + Message: "", + ObservedGeneration: 1, + }, + }, + }, + { + name: "condition needs to be updated due to different condition observed generation", + obj: &operatorv1beta1.DataPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dp1", + Generation: 2, + }, + Status: operatorv1beta1.DataPlaneStatus{ + Conditions: []metav1.Condition{ + { + Type: string(consts.ReadyType), + Status: metav1.ConditionTrue, + Reason: string(consts.ResolvedRefsReason), + Message: "", + ObservedGeneration: 1, + }, + }, + }, + }, + conditionType: consts.ReadyType, + conditionStatus: metav1.ConditionTrue, + conditionReason: consts.ResolvedRefsReason, + conditionMessage: "", + expectedResult: ctrl.Result{}, + expectedConditions: []metav1.Condition{ + { + Type: string(consts.ReadyType), + Status: metav1.ConditionTrue, + Reason: string(consts.ResolvedRefsReason), + Message: "", + ObservedGeneration: 2, + }, + }, + }, + { + name: "condition needs to be updated due to different condition reason", + obj: &operatorv1beta1.DataPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dp1", + Generation: 1, + }, + Status: operatorv1beta1.DataPlaneStatus{ + Conditions: []metav1.Condition{ + { + Type: string(consts.ReadyType), + Status: metav1.ConditionFalse, + Reason: string(consts.ResourceReadyReason), + Message: "", + ObservedGeneration: 1, + }, + }, + }, + }, + conditionType: consts.ReadyType, + conditionStatus: metav1.ConditionFalse, + conditionReason: consts.DependenciesNotReadyReason, + conditionMessage: "", + expectedResult: ctrl.Result{}, + expectedConditions: []metav1.Condition{ + { + Type: string(consts.ReadyType), + Status: metav1.ConditionFalse, + Reason: string(consts.DependenciesNotReadyReason), + Message: "", + ObservedGeneration: 1, + }, + }, + }, + { + name: "new condition needs to be set on object without conditions", + obj: &operatorv1beta1.DataPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dp1", + Generation: 1, + }, + Status: operatorv1beta1.DataPlaneStatus{}, + }, + conditionType: consts.ReadyType, + conditionStatus: metav1.ConditionTrue, + conditionReason: consts.ResolvedRefsReason, + conditionMessage: "Resource is available", + expectedResult: ctrl.Result{}, + expectedConditions: []metav1.Condition{ + { + Type: string(consts.ReadyType), + Status: metav1.ConditionTrue, + Reason: string(consts.ResolvedRefsReason), + Message: "Resource is available", + ObservedGeneration: 1, + }, + }, + }, + { + name: "conflict triggers requeue", + obj: &operatorv1beta1.DataPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dp1", + Generation: 1, + }, + Status: operatorv1beta1.DataPlaneStatus{}, + }, + conditionType: consts.ReadyType, + conditionStatus: metav1.ConditionTrue, + conditionReason: consts.ResolvedRefsReason, + conditionMessage: "Resource is available", + expectedResult: ctrl.Result{ + Requeue: true, + }, + interceptorFunc: interceptor.Funcs{ + SubResourcePatch: func(ctx context.Context, client client.Client, subResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { + return &k8serrors.StatusError{ + ErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Reason: metav1.StatusReasonConflict, + }, + } + }, + }, + }, + { + name: "error", + obj: &operatorv1beta1.DataPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dp1", + Generation: 1, + }, + Status: operatorv1beta1.DataPlaneStatus{}, + }, + conditionType: consts.ReadyType, + conditionStatus: metav1.ConditionTrue, + conditionReason: consts.ResolvedRefsReason, + conditionMessage: "Resource is available", + expectedError: true, + interceptorFunc: interceptor.Funcs{ + SubResourcePatch: func(ctx context.Context, client client.Client, subResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { + return &k8serrors.StatusError{ + ErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Reason: metav1.StatusReason("unknown"), + }, + } + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + cl := fake.NewClientBuilder(). + WithObjects(tt.obj). + WithStatusSubresource(tt.obj). + WithScheme(scheme.Get()). + WithInterceptorFuncs(tt.interceptorFunc). + Build() + + result, err := StatusWithCondition( + ctx, cl, tt.obj, + tt.conditionType, tt.conditionStatus, tt.conditionReason, tt.conditionMessage, + ) + + assert.Equal(t, tt.expectedResult, result) + if tt.expectedError { + require.Error(t, err) + return + } + + require.NoError(t, err) + + for _, expectedCond := range tt.expectedConditions { + actualCond, ok := k8sutils.GetCondition(consts.ConditionType(expectedCond.Type), tt.obj) + if !ok { + t.Fatalf("condition %s not found", expectedCond.Type) + } + assert.Equal(t, expectedCond.Status, actualCond.Status) + assert.Equal(t, expectedCond.Reason, actualCond.Reason) + assert.Equal(t, expectedCond.Message, actualCond.Message) + assert.Equal(t, expectedCond.ObservedGeneration, actualCond.ObservedGeneration) + } + }) + } +}