From dbda5e58523d5d417df4c0c65e11c2316fb16a18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Ma=C5=82ek?= Date: Thu, 1 Aug 2024 13:08:55 +0200 Subject: [PATCH] refactor: split APIAuthRefValid into 2 conditions: APIAuthValid and APIAuthResolvedRef --- controller/konnect/conditions.go | 42 +++--- controller/konnect/constraints.go | 9 +- controller/konnect/reconciler_generic.go | 130 +++++++++++++----- controller/konnect/reconciler_generic_test.go | 3 + go.mod | 2 +- go.sum | 4 +- 6 files changed, 136 insertions(+), 54 deletions(-) diff --git a/controller/konnect/conditions.go b/controller/konnect/conditions.go index e3f57558e..877442931 100644 --- a/controller/konnect/conditions.go +++ b/controller/konnect/conditions.go @@ -14,30 +14,38 @@ const ( ) const ( - // KonnectAPIAuthConfigurationValidConditionType is the type of the condition - // that indicates whether the APIAuth configuration is valid. - KonnectAPIAuthConfigurationValidConditionType = "Valid" + // KonnectEntityAPIAuthConfigurationResolvedRefConditionType is the type of the + // condition that indicates whether the APIAuth configuration reference is + // valid and points to an existing APIAuth configuration. + KonnectEntityAPIAuthConfigurationResolvedRefConditionType = "APIAuthResolvedRef" - // KonnectAPIAuthConfigurationReasonValid is the reason used with the Valid - // condition type indiciating that the APIAuth configuration is valid. - KonnectAPIAuthConfigurationReasonValid = "Valid" - // KonnectAPIAuthConfigurationReasonValid is the reason used with the Valid - // condition type indiciating that the APIAuth configuration is invalid. - KonnectAPIAuthConfigurationReasonInvalid = "Invalid" + // KonnectEntityAPIAuthConfigurationResolvedRefReasonResolvedRef is the reason + // used with the APIAuthResolvedRef condition type indicating that the APIAuth + // configuration reference has been resolved. + KonnectEntityAPIAuthConfigurationResolvedRefReasonResolvedRef = "ResolvedRef" + // KonnectEntityAPIAuthConfigurationResolvedRefReasonRefNotFound is the reason + // used with the APIAuthResolvedRef condition type indicating that the APIAuth + // configuration reference could not be resolved. + KonnectEntityAPIAuthConfigurationResolvedRefReasonRefNotFound = "RefNotFound" + // KonnectEntityAPIAuthConfigurationResolvedRefReasonRefNotFound is the reason + // used with the APIAuthResolvedRef condition type indicating that the APIAuth + // configuration reference is invalid and could not be resolved. + // Condition message can contain more information about the error. + KonnectEntityAPIAuthConfigurationResolvedRefReasonRefInvalid = "RefInvalid" ) const ( - // KonnectEntityAPIAuthConfigurationRefValidConditionType is the type of the - // condition that indicates whether the APIAuth configuration reference is - // valid and points to an existing, valid APIAuth configuration. - KonnectEntityAPIAuthConfigurationRefValidConditionType = "APIAuthRefValid" + // KonnectEntityAPIAuthConfigurationValidConditionType is the type of the + // condition that indicates whether the referenced APIAuth configuration is + // valid. + KonnectEntityAPIAuthConfigurationValidConditionType = "APIAuthValid" - // KonnectEntityAPIAuthConfigurationRefReasonValid is the reason used with the + // KonnectEntityAPIAuthConfigurationReasonValid is the reason used with the // APIAuthRefValid condition type indicating that the APIAuth configuration // referenced by the entity is valid. - KonnectEntityAPIAuthConfigurationRefReasonValid = "Valid" - // KonnectEntityAPIAuthConfigurationRefReasonValid is the reason used with the + KonnectEntityAPIAuthConfigurationReasonValid = "Valid" + // KonnectEntityAPIAuthConfigurationReasonInvalid is the reason used with the // APIAuthRefValid condition type indicating that the APIAuth configuration // referenced by the entity is invalid. - KonnectEntityAPIAuthConfigurationRefReasonInvalid = "Invalid" + KonnectEntityAPIAuthConfigurationReasonInvalid = "Invalid" ) diff --git a/controller/konnect/constraints.go b/controller/konnect/constraints.go index ddde4d213..7efa03e95 100644 --- a/controller/konnect/constraints.go +++ b/controller/konnect/constraints.go @@ -6,6 +6,8 @@ import ( configurationv1 "github.com/kong/kubernetes-configuration/api/configuration/v1" configurationv1alpha1 "github.com/kong/kubernetes-configuration/api/configuration/v1alpha1" + configurationv1beta1 "github.com/kong/kubernetes-configuration/api/configuration/v1beta1" + konnectv1alpha1 "github.com/kong/kubernetes-configuration/api/konnect/v1alpha1" ) // SupportedKonnectEntityType is an interface that all Konnect entity types @@ -13,7 +15,8 @@ import ( type SupportedKonnectEntityType interface { configurationv1alpha1.KongService | configurationv1alpha1.KongRoute | - configurationv1.KongConsumer + configurationv1.KongConsumer | + configurationv1beta1.KongConsumerGroup // TODO: add other types GetTypeName() string @@ -33,10 +36,10 @@ type EntityType[ GetObjectMeta() metav1.Object client.Object - // Added methods + // Additional method which are used in reconciling Konnect entities. GetConditions() []metav1.Condition SetConditions([]metav1.Condition) GetKonnectStatus() *configurationv1alpha1.KonnectEntityStatus - GetKonnectAPIAuthConfigurationRef() configurationv1alpha1.KonnectAPIAuthConfigurationRef + GetKonnectAPIAuthConfigurationRef() konnectv1alpha1.KonnectAPIAuthConfigurationRef } diff --git a/controller/konnect/reconciler_generic.go b/controller/konnect/reconciler_generic.go index 46ab87872..b3320a7e3 100644 --- a/controller/konnect/reconciler_generic.go +++ b/controller/konnect/reconciler_generic.go @@ -16,9 +16,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/kong/gateway-operator/controller/pkg/log" + "github.com/kong/gateway-operator/pkg/consts" k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes" - configurationv1alpha1 "github.com/kong/kubernetes-configuration/api/configuration/v1alpha1" + konnectv1alpha1 "github.com/kong/kubernetes-configuration/api/konnect/v1alpha1" ) const ( @@ -103,7 +104,7 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile( log.Debug(logger, "reconciling", ent) var ( - apiAuth configurationv1alpha1.KonnectAPIAuthConfiguration + apiAuth konnectv1alpha1.KonnectAPIAuthConfiguration apiAuthRef = types.NamespacedName{ Name: ent.GetKonnectAPIAuthConfigurationRef().Name, Namespace: ent.GetNamespace(), @@ -112,44 +113,78 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile( } ) if err := r.Client.Get(ctx, apiAuthRef, &apiAuth); err != nil { + if k8serrors.IsNotFound(err) { + if res, err := updateStatusWithCondition( + ctx, r.Client, ent, + KonnectEntityAPIAuthConfigurationResolvedRefConditionType, + metav1.ConditionFalse, + KonnectEntityAPIAuthConfigurationResolvedRefReasonRefNotFound, + fmt.Sprintf("Referenced KonnectAPIAuthConfiguration %s not found", apiAuthRef), + ); err != nil || res.Requeue { + return ctrl.Result{}, err + } + + return ctrl.Result{}, nil + } + + if res, err := updateStatusWithCondition( + ctx, r.Client, ent, + KonnectEntityAPIAuthConfigurationResolvedRefConditionType, + metav1.ConditionFalse, + KonnectEntityAPIAuthConfigurationResolvedRefReasonRefInvalid, + fmt.Sprintf("KonnectAPIAuthConfiguration reference %s is invalid: %v", apiAuthRef, err), + ); err != nil || res.Requeue { + return ctrl.Result{}, err + } + return ctrl.Result{}, fmt.Errorf("failed to get KonnectAPIAuthConfiguration: %w", err) } - // Check if the referenced APIAuthConfiguration is valid. - if cond, present := k8sutils.GetCondition(KonnectAPIAuthConfigurationValidConditionType, &apiAuth.Status); present && cond.Status != metav1.ConditionTrue { - // If the referenced APIAuthConfiguration is not valid, set the - // "APIAuthRefValid" condition to false and return. - k8sutils.SetCondition( - k8sutils.NewConditionWithGeneration( - KonnectEntityAPIAuthConfigurationRefValidConditionType, - metav1.ConditionFalse, - KonnectEntityAPIAuthConfigurationRefReasonInvalid, - fmt.Sprintf("referenced KonnectAPIAuthConfiguration %s is invalid", apiAuthRef), - ent.GetGeneration(), - ), - ent, - ) - + // Update the status if the reference is resolved and it's not as expected. + if cond, present := k8sutils.GetCondition(KonnectEntityAPIAuthConfigurationResolvedRefConditionType, &apiAuth.Status); !present || + cond.Status != metav1.ConditionTrue || + cond.ObservedGeneration != apiAuth.GetGeneration() || + cond.Reason != KonnectEntityAPIAuthConfigurationResolvedRefReasonResolvedRef { + if res, err := updateStatusWithCondition( + ctx, r.Client, ent, + KonnectEntityAPIAuthConfigurationResolvedRefConditionType, + metav1.ConditionTrue, + KonnectEntityAPIAuthConfigurationResolvedRefReasonResolvedRef, + fmt.Sprintf("KonnectAPIAuthConfiguration reference %s is resolved", apiAuthRef), + ); err != nil || res.Requeue { + return ctrl.Result{}, err + } return ctrl.Result{}, nil } - // If the referenced APIAuthConfiguration is valid, set the "APIAuthRefValid" - // condition to true. - k8sutils.SetCondition( - k8sutils.NewConditionWithGeneration( - KonnectEntityAPIAuthConfigurationRefValidConditionType, + // Check if the referenced APIAuthConfiguration is valid. + if cond, present := k8sutils.GetCondition(KonnectEntityAPIAuthConfigurationValidConditionType, &apiAuth.Status); !present || cond.Status != metav1.ConditionTrue { + if res, err := updateStatusWithCondition( + ctx, r.Client, ent, + KonnectEntityAPIAuthConfigurationValidConditionType, + metav1.ConditionFalse, + KonnectEntityAPIAuthConfigurationReasonInvalid, + fmt.Sprintf("referenced KonnectAPIAuthConfiguration %s is invalid", apiAuthRef), + ); err != nil || res.Requeue { + return ctrl.Result{}, err + } + + return ctrl.Result{}, nil + } else if cond.ObservedGeneration != apiAuth.GetGeneration() || + cond.Reason != KonnectEntityAPIAuthConfigurationReasonValid { + // If the referenced APIAuthConfiguration is valid, set the "APIAuthValid" + // condition to true. Only perform the update if the Reason or ObservedGeneration + // has changed. + if res, err := updateStatusWithCondition( + ctx, r.Client, ent, + KonnectEntityAPIAuthConfigurationValidConditionType, metav1.ConditionTrue, - KonnectEntityAPIAuthConfigurationRefReasonValid, + KonnectEntityAPIAuthConfigurationReasonValid, fmt.Sprintf("referenced KonnectAPIAuthConfiguration %s is valid", apiAuthRef), - ent.GetGeneration(), - ), - ent, - ) - if err := r.Client.Status().Update(ctx, ent); err != nil { - if k8serrors.IsConflict(err) { - return ctrl.Result{Requeue: true}, nil + ); err != nil || res.Requeue { + return ctrl.Result{}, err } - return ctrl.Result{}, fmt.Errorf("failed to update status with %s condition: %w", KonnectEntityAPIAuthConfigurationRefValidConditionType, err) + return ctrl.Result{}, nil } // NOTE: We need to create a new SDK instance for each reconciliation @@ -261,3 +296,36 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile( RequeueAfter: configurableSyncPeriod, }, nil } + +func updateStatusWithCondition[T SupportedKonnectEntityType, TEnt EntityType[T]]( + ctx context.Context, + client client.Client, + ent TEnt, + 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", + KonnectEntityAPIAuthConfigurationResolvedRefConditionType, err, + ) + } + + return ctrl.Result{}, nil +} diff --git a/controller/konnect/reconciler_generic_test.go b/controller/konnect/reconciler_generic_test.go index 261754324..e60adfc33 100644 --- a/controller/konnect/reconciler_generic_test.go +++ b/controller/konnect/reconciler_generic_test.go @@ -16,6 +16,9 @@ import ( func TestNewKonnectEntityReconciler(t *testing.T) { testNewKonnectEntityReconciler(t, configurationv1.KongConsumer{}) + // GetTypeName() is missing. + // https://github.com/Kong/kubernetes-configuration/pull/15 fixes that. + // testNewKonnectEntityReconciler(t, configurationv1beta1.KongConsumerGroup{}) testNewKonnectEntityReconciler(t, configurationv1alpha1.KongService{}) testNewKonnectEntityReconciler(t, configurationv1alpha1.KongRoute{}) } diff --git a/go.mod b/go.mod index 5b4df399d..ada3cc42d 100644 --- a/go.mod +++ b/go.mod @@ -17,7 +17,7 @@ require ( github.com/cloudflare/cfssl v1.6.5 github.com/go-logr/logr v1.4.2 github.com/google/uuid v1.6.0 - github.com/kong/kubernetes-configuration v0.0.0-20240726122834-c10e8297f9e6 + github.com/kong/kubernetes-configuration v0.0.0-20240801095618-0ed30e4054cb github.com/kong/kubernetes-ingress-controller/v3 v3.2.3 github.com/kong/kubernetes-telemetry v0.1.4 github.com/kong/kubernetes-testing-framework v0.47.1 diff --git a/go.sum b/go.sum index 109d8b277..c755df408 100644 --- a/go.sum +++ b/go.sum @@ -217,8 +217,8 @@ github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck= github.com/kong/go-kong v0.57.0 h1:e+4bHTzcO0xhFPVyGIUtlO+B4E4l14k55NH8Vjw6ORY= github.com/kong/go-kong v0.57.0/go.mod h1:gyNwyP1fzztT6sX/0/ygMQ30OiRMIQ51b2jSfstMrcU= -github.com/kong/kubernetes-configuration v0.0.0-20240726122834-c10e8297f9e6 h1:mi0P9KZSYbI1bffC+dwz0lesjHgd3AS4W/762C6qJHs= -github.com/kong/kubernetes-configuration v0.0.0-20240726122834-c10e8297f9e6/go.mod h1:kZTKzwQ68Wk2n8W8Em0RsYTL2yVNbCWU+5b9w1WU+Hs= +github.com/kong/kubernetes-configuration v0.0.0-20240801095618-0ed30e4054cb h1:GUMgT3qqNjLV1Wdl8D6fskEaN/vof/FRhRbicIkUKpg= +github.com/kong/kubernetes-configuration v0.0.0-20240801095618-0ed30e4054cb/go.mod h1:kZTKzwQ68Wk2n8W8Em0RsYTL2yVNbCWU+5b9w1WU+Hs= github.com/kong/kubernetes-ingress-controller/v3 v3.2.3 h1:SQ/0hfceGmsvzbkCUxiJUv1ELcFRp4d6IzvYGfHct9o= github.com/kong/kubernetes-ingress-controller/v3 v3.2.3/go.mod h1:gshVZnDU2FTe/95I3vSJPsH2kyB8zR+GpUIieCyt8C4= github.com/kong/kubernetes-telemetry v0.1.4 h1:Yz7OlECxWKgNRG1wJ5imA4+H0dQEpdU9d86uhwUVpu4=