FIX: Context being cancelled and better Policy status (#1012)

* FIX: Context being cancelled and better Policy status The GetKuadrantFromTopology was somehow cancelling the context of functions that called it. This improves the policy status of ratelimit policies when there is no kuadrant CR on cluster, or it was removed. --------- Signed-off-by: Jim Fitzpatrick <[email protected]>
Kuadrant · Nov 14, 2024 · 87be3fb · 87be3fb
1 parent 17b283c
commit 87be3fb
Show file tree

Hide file tree

Showing 19 changed files with 118 additions and 147 deletions.
diff --git a/controllers/auth_policy_status_updater.go b/controllers/auth_policy_status_updater.go
@@ -115,6 +115,10 @@ func (r *AuthPolicyStatusUpdater) UpdateStatus(ctx context.Context, _ []controll
 }
 
 func (r *AuthPolicyStatusUpdater) enforcedCondition(policy *kuadrantv1.AuthPolicy, topology *machinery.Topology, state *sync.Map) *metav1.Condition {
+	kObj := GetKuadrantFromTopology(topology)
+	if kObj == nil {
+		return kuadrant.EnforcedCondition(policy, kuadrant.NewErrSystemResource("kuadrant"), false)
+	}
 	policyKind := kuadrantv1.AuthPolicyGroupKind.Kind
 
 	effectivePolicies, ok := state.Load(StateEffectiveAuthPolicies)
@@ -182,9 +186,9 @@ func (r *AuthPolicyStatusUpdater) enforcedCondition(policy *kuadrantv1.AuthPolic
 	var componentsToSync []string
 
 	// check the status of Authorino
-	authorino, err := GetAuthorinoFromTopology(topology)
-	if err != nil {
-		return kuadrant.EnforcedCondition(policy, kuadrant.NewErrUnknown(policyKind, err), false)
+	authorino := GetAuthorinoFromTopology(topology)
+	if authorino == nil {
+		return kuadrant.EnforcedCondition(policy, kuadrant.NewErrSystemResource("authornio"), false)
 	}
 	if !meta.IsStatusConditionTrue(lo.Map(authorino.Status.Conditions, authorinoOperatorConditionToProperConditionFunc), string(authorinooperatorv1beta1.ConditionReady)) {
 		componentsToSync = append(componentsToSync, kuadrantv1beta1.AuthorinoGroupKind.Kind)

diff --git a/controllers/auth_workflow_helpers.go b/controllers/auth_workflow_helpers.go
@@ -34,21 +34,21 @@ var (
 	ErrMissingStateEffectiveAuthPolicies = fmt.Errorf("missing auth effective policies stored in the reconciliation state")
 )
 
-func GetAuthorinoFromTopology(topology *machinery.Topology) (*authorinooperatorv1beta1.Authorino, error) {
-	kuadrant, err := GetKuadrantFromTopology(topology)
-	if err != nil {
-		return nil, err
+func GetAuthorinoFromTopology(topology *machinery.Topology) *authorinooperatorv1beta1.Authorino {
+	kuadrant := GetKuadrantFromTopology(topology)
+	if kuadrant == nil {
+		return nil
 	}
 
 	authorinoObj, found := lo.Find(topology.Objects().Children(kuadrant), func(child machinery.Object) bool {
 		return child.GroupVersionKind().GroupKind() == kuadrantv1beta1.AuthorinoGroupKind
 	})
 	if !found {
-		return nil, ErrMissingAuthorino
+		return nil
 	}
 
 	authorino := authorinoObj.(*controller.RuntimeObject).Object.(*authorinooperatorv1beta1.Authorino)
-	return authorino, nil
+	return authorino
 }
 
 func AuthObjectLabels() labels.Set {

diff --git a/controllers/authconfigs_reconciler.go b/controllers/authconfigs_reconciler.go
@@ -2,7 +2,6 @@ package controllers
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"reflect"
 	"sync"
@@ -47,13 +46,10 @@ func (r *AuthConfigsReconciler) Subscription() controller.Subscription {
 func (r *AuthConfigsReconciler) Reconcile(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, state *sync.Map) error {
 	logger := controller.LoggerFromContext(ctx).WithName("AuthConfigsReconciler")
 
-	authorino, err := GetAuthorinoFromTopology(topology)
-	if err != nil {
-		if errors.Is(err, ErrMissingKuadrant) || errors.Is(err, ErrMissingAuthorino) {
-			logger.V(1).Info(err.Error())
-			return nil
-		}
-		return err
+	authorino := GetAuthorinoFromTopology(topology)
+	if authorino == nil {
+		logger.V(1).Info("authorino resource not found in the topology")
+		return nil
 	}
 	authConfigsNamespace := authorino.GetNamespace()
 

diff --git a/controllers/authorino_reconciler.go b/controllers/authorino_reconciler.go
@@ -2,7 +2,6 @@ package controllers
 
 import (
 	"context"
-	"errors"
 	"sync"
 
 	v1beta2 "github.com/kuadrant/authorino-operator/api/v1beta1"
@@ -42,14 +41,9 @@ func (r *AuthorinoReconciler) Reconcile(ctx context.Context, _ []controller.Reso
 	logger.Info("reconciling authorino resource", "status", "started")
 	defer logger.Info("reconciling authorino resource", "status", "completed")
 
-	kobj, err := GetKuadrantFromTopology(topology)
-	if err != nil {
-		if errors.Is(err, ErrMissingKuadrant) {
-			logger.Info("kuadrant resource not found, ignoring", "status", "skipping")
-			return err
-		}
-		logger.Error(err, "cannot find Kuadrant resource", "status", "error")
-		return err
+	kobj := GetKuadrantFromTopology(topology)
+	if kobj == nil {
+		return nil
 	}
 
 	aobjs := lo.FilterMap(topology.Objects().Objects().Children(kobj), func(item machinery.Object, _ int) (machinery.Object, bool) {

diff --git a/controllers/effective_auth_policies_reconciler.go b/controllers/effective_auth_policies_reconciler.go
@@ -3,7 +3,6 @@ package controllers
 import (
 	"context"
 	"encoding/json"
-	"errors"
 	"sync"
 
 	"github.com/kuadrant/policy-machinery/controller"
@@ -35,14 +34,12 @@ func (r *EffectiveAuthPolicyReconciler) Subscription() controller.Subscription {
 
 func (r *EffectiveAuthPolicyReconciler) Reconcile(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, state *sync.Map) error {
 	logger := controller.LoggerFromContext(ctx).WithName("EffectiveAuthPolicyReconciler")
+	logger.V(1).Info("generate effective auth policy", "status", "started")
+	defer logger.V(1).Info("generate effective auth policy", "status", "completed")
 
-	kuadrant, err := GetKuadrantFromTopology(topology)
-	if err != nil {
-		if errors.Is(err, ErrMissingKuadrant) {
-			logger.V(1).Info(err.Error())
-			return nil
-		}
-		return err
+	kuadrant := GetKuadrantFromTopology(topology)
+	if kuadrant == nil {
+		return nil
 	}
 
 	effectivePolicies := r.calculateEffectivePolicies(ctx, topology, kuadrant, state)

diff --git a/controllers/effective_ratelimit_policies_reconciler.go b/controllers/effective_ratelimit_policies_reconciler.go
@@ -3,7 +3,6 @@ package controllers
 import (
 	"context"
 	"encoding/json"
-	"errors"
 	"sync"
 
 	"github.com/kuadrant/policy-machinery/controller"
@@ -35,14 +34,12 @@ func (r *EffectiveRateLimitPolicyReconciler) Subscription() controller.Subscript
 
 func (r *EffectiveRateLimitPolicyReconciler) Reconcile(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, state *sync.Map) error {
 	logger := controller.LoggerFromContext(ctx).WithName("EffectiveRateLimitPolicyReconciler")
+	logger.V(1).Info("generating effective rate limit policy", "status", "started")
+	defer logger.V(1).Info("generating effective rate limit policy", "status", "completed")
 
-	kuadrant, err := GetKuadrantFromTopology(topology)
-	if err != nil {
-		if errors.Is(err, ErrMissingKuadrant) {
-			logger.V(1).Info(err.Error())
-			return nil
-		}
-		return err
+	kuadrant := GetKuadrantFromTopology(topology)
+	if kuadrant == nil {
+		return nil
 	}
 
 	effectivePolicies := r.calculateEffectivePolicies(ctx, topology, kuadrant, state)

diff --git a/controllers/envoy_gateway_auth_cluster_reconciler.go b/controllers/envoy_gateway_auth_cluster_reconciler.go
@@ -2,7 +2,6 @@ package controllers
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"sync"
 
@@ -53,13 +52,9 @@ func (r *EnvoyGatewayAuthClusterReconciler) Reconcile(ctx context.Context, _ []c
 	logger.V(1).Info("building envoy gateway auth clusters")
 	defer logger.V(1).Info("finished building envoy gateway auth clusters")
 
-	kuadrant, err := GetKuadrantFromTopology(topology)
-	if err != nil {
-		if errors.Is(err, ErrMissingKuadrant) {
-			logger.V(1).Info(err.Error())
-			return nil
-		}
-		return err
+	kuadrant := GetKuadrantFromTopology(topology)
+	if kuadrant == nil {
+		return nil
 	}
 
 	authorinoObj, found := lo.Find(topology.Objects().Children(kuadrant), func(child machinery.Object) bool {

diff --git a/controllers/envoy_gateway_ratelimit_cluster_reconciler.go b/controllers/envoy_gateway_ratelimit_cluster_reconciler.go
@@ -2,7 +2,6 @@ package controllers
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"sync"
 
@@ -53,13 +52,9 @@ func (r *EnvoyGatewayRateLimitClusterReconciler) Reconcile(ctx context.Context,
 	logger.V(1).Info("building envoy gateway rate limit clusters")
 	defer logger.V(1).Info("finished building envoy gateway rate limit clusters")
 
-	kuadrant, err := GetKuadrantFromTopology(topology)
-	if err != nil {
-		if errors.Is(err, ErrMissingKuadrant) {
-			logger.V(1).Info(err.Error())
-			return nil
-		}
-		return err
+	kuadrant := GetKuadrantFromTopology(topology)
+	if kuadrant == nil {
+		return nil
 	}
 
 	limitadorObj, found := lo.Find(topology.Objects().Children(kuadrant), func(child machinery.Object) bool {

diff --git a/controllers/istio_auth_cluster_reconciler.go b/controllers/istio_auth_cluster_reconciler.go
@@ -2,7 +2,6 @@ package controllers
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"sync"
 
@@ -53,13 +52,9 @@ func (r *IstioAuthClusterReconciler) Reconcile(ctx context.Context, _ []controll
 	logger.V(1).Info("building istio auth clusters")
 	defer logger.V(1).Info("finished building istio auth clusters")
 
-	kuadrant, err := GetKuadrantFromTopology(topology)
-	if err != nil {
-		if errors.Is(err, ErrMissingKuadrant) {
-			logger.V(1).Info(err.Error())
-			return nil
-		}
-		return err
+	kuadrant := GetKuadrantFromTopology(topology)
+	if kuadrant == nil {
+		return nil
 	}
 
 	authorinoObj, found := lo.Find(topology.Objects().Children(kuadrant), func(child machinery.Object) bool {

diff --git a/controllers/istio_ratelimit_cluster_reconciler.go b/controllers/istio_ratelimit_cluster_reconciler.go
@@ -2,7 +2,6 @@ package controllers
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"sync"
 
@@ -53,13 +52,9 @@ func (r *IstioRateLimitClusterReconciler) Reconcile(ctx context.Context, _ []con
 	logger.V(1).Info("building istio rate limit clusters")
 	defer logger.V(1).Info("finished building istio rate limit clusters")
 
-	kuadrant, err := GetKuadrantFromTopology(topology)
-	if err != nil {
-		if errors.Is(err, ErrMissingKuadrant) {
-			logger.V(1).Info(err.Error())
-			return nil
-		}
-		return err
+	kuadrant := GetKuadrantFromTopology(topology)
+	if kuadrant == nil {
+		return nil
 	}
 
 	limitadorObj, found := lo.Find(topology.Objects().Children(kuadrant), func(child machinery.Object) bool {

diff --git a/controllers/kuadrant_status_updater.go b/controllers/kuadrant_status_updater.go
@@ -49,9 +49,8 @@ func (r *KuadrantStatusUpdater) Reconcile(ctx context.Context, _ []controller.Re
 	logger.Info("reconciling kuadrant status", "status", "started")
 	defer logger.Info("reconciling kuadrant status", "status", "completed")
 
-	kObj, err := GetKuadrantFromTopology(topology)
-	if err != nil {
-		logger.V(1).Error(err, "error getting kuadrant from topology", "status", "error")
+	kObj := GetKuadrantFromTopology(topology)
+	if kObj == nil {
 		return nil
 	}
 
@@ -143,10 +142,10 @@ func (r *KuadrantStatusUpdater) readyCondition(topology *machinery.Topology, log
 }
 
 func checkLimitadorReady(topology *machinery.Topology, logger logr.Logger) *string {
-	limitadorObj, err := GetLimitadorFromTopology(topology)
-	if err != nil {
-		logger.V(1).Error(err, "failed getting Limitador resource from topology", "status", "error")
-		return ptr.To(err.Error())
+	limitadorObj := GetLimitadorFromTopology(topology)
+	if limitadorObj == nil {
+		logger.V(1).Info("failed getting Limitador resource from topology", "status", "error")
+		return ptr.To("limitador resoure not in topology")
 	}
 
 	statusConditionReady := meta.FindStatusCondition(limitadorObj.Status.Conditions, "Ready")
@@ -161,10 +160,10 @@ func checkLimitadorReady(topology *machinery.Topology, logger logr.Logger) *stri
 }
 
 func checkAuthorinoAvailable(topology *machinery.Topology, logger logr.Logger) *string {
-	authorinoObj, err := GetAuthorinoFromTopology(topology)
-	if err != nil {
-		logger.V(1).Error(err, "failed getting Authorino resource from topology", "status", "error")
-		return ptr.To(err.Error())
+	authorinoObj := GetAuthorinoFromTopology(topology)
+	if authorinoObj == nil {
+		logger.V(1).Info("failed getting Authorino resource from topology", "status", "error")
+		return ptr.To("authorino resoure not in topology")
 	}
 
 	readyCondition := authorino.FindAuthorinoStatusCondition(authorinoObj.Status.Conditions, "Ready")

diff --git a/controllers/limitador_limits_reconciler.go b/controllers/limitador_limits_reconciler.go
@@ -2,7 +2,6 @@ package controllers
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"sync"
 
@@ -43,13 +42,10 @@ func (r *LimitadorLimitsReconciler) Subscription() controller.Subscription {
 func (r *LimitadorLimitsReconciler) Reconcile(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, state *sync.Map) error {
 	logger := controller.LoggerFromContext(ctx).WithName("LimitadorLimitsReconciler")
 
-	limitador, err := GetLimitadorFromTopology(topology)
-	if err != nil {
-		if errors.Is(err, ErrMissingKuadrant) || errors.Is(err, ErrMissingLimitador) {
-			logger.V(1).Info(err.Error())
-			return nil
-		}
-		return err
+	limitador := GetLimitadorFromTopology(topology)
+	if limitador == nil {
+		logger.V(1).Info("not limitador resources found in topology")
+		return nil
 	}
 
 	desiredLimits, err := r.buildLimitadorLimits(ctx, state)

diff --git a/controllers/limitador_reconciler.go b/controllers/limitador_reconciler.go
@@ -2,7 +2,6 @@ package controllers
 
 import (
 	"context"
-	"errors"
 	"sync"
 
 	limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1"
@@ -43,14 +42,9 @@ func (r *LimitadorReconciler) Reconcile(ctx context.Context, _ []controller.Reso
 	logger.Info("reconciling limtador resource", "status", "started")
 	defer logger.Info("reconciling limitador resource", "status", "completed")
 
-	kobj, err := GetKuadrantFromTopology(topology)
-	if err != nil {
-		if errors.Is(err, ErrMissingKuadrant) {
-			logger.Info("kuadrant resource not found, ignoring", "status", "skipping")
-			return err
-		}
-		logger.Error(err, "cannot find Kuadrant resource", "status", "error")
-		return err
+	kobj := GetKuadrantFromTopology(topology)
+	if kobj == nil {
+		return nil
 	}
 
 	lobjs := lo.FilterMap(topology.Objects().Objects().Items(), func(item machinery.Object, _ int) (machinery.Object, bool) {

diff --git a/controllers/ratelimit_policy_status_updater.go b/controllers/ratelimit_policy_status_updater.go
@@ -113,6 +113,10 @@ func (r *RateLimitPolicyStatusUpdater) UpdateStatus(ctx context.Context, _ []con
 }
 
 func (r *RateLimitPolicyStatusUpdater) enforcedCondition(policy *kuadrantv1.RateLimitPolicy, topology *machinery.Topology, state *sync.Map) *metav1.Condition {
+	kObj := GetKuadrantFromTopology(topology)
+	if kObj == nil {
+		return kuadrant.EnforcedCondition(policy, kuadrant.NewErrSystemResource("kuadrant"), false)
+	}
 	policyKind := kuadrantv1.RateLimitPolicyGroupKind.Kind
 
 	effectivePolicies, ok := state.Load(StateEffectiveRateLimitPolicies)
@@ -173,9 +177,9 @@ func (r *RateLimitPolicyStatusUpdater) enforcedCondition(policy *kuadrantv1.Rate
 	if limitadorLimitsModified, stateLimitadorLimitsModifiedPresent := state.Load(StateLimitadorLimitsModified); stateLimitadorLimitsModifiedPresent && limitadorLimitsModified.(bool) {
 		componentsToSync = append(componentsToSync, kuadrantv1beta1.LimitadorGroupKind.Kind)
 	} else {
-		limitador, err := GetLimitadorFromTopology(topology)
-		if err != nil {
-			return kuadrant.EnforcedCondition(policy, kuadrant.NewErrUnknown(policyKind, err), false)
+		limitador := GetLimitadorFromTopology(topology)
+		if limitador == nil {
+			return kuadrant.EnforcedCondition(policy, kuadrant.NewErrSystemResource("limitador"), false)
 		}
 		if !meta.IsStatusConditionTrue(limitador.Status.Conditions, limitadorv1alpha1.StatusConditionReady) {
 			componentsToSync = append(componentsToSync, kuadrantv1beta1.LimitadorGroupKind.Kind)