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 12, 2024 · 1aff0b2 · 1aff0b2
1 parent 365b170
commit 1aff0b2
Show file tree

Hide file tree

Showing 16 changed files with 86 additions and 111 deletions.
diff --git a/controllers/auth_workflow_helpers.go b/controllers/auth_workflow_helpers.go
@@ -35,9 +35,9 @@ var (
 )
 
 func GetAuthorinoFromTopology(topology *machinery.Topology) (*authorinooperatorv1beta1.Authorino, error) {
-	kuadrant, err := GetKuadrantFromTopology(topology)
-	if err != nil {
-		return nil, err
+	kuadrant := GetKuadrantFromTopology(topology)
+	if kuadrant == nil {
+		return nil, ErrMissingKuadrant
 	}
 
 	authorinoObj, found := lo.Find(topology.Objects().Children(kuadrant), func(child machinery.Object) bool {

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
@@ -52,9 +52,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
 	}
 

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)

diff --git a/controllers/ratelimit_workflow_helpers.go b/controllers/ratelimit_workflow_helpers.go
@@ -40,9 +40,9 @@ var (
 )
 
 func GetLimitadorFromTopology(topology *machinery.Topology) (*limitadorv1alpha1.Limitador, error) {
-	kuadrant, err := GetKuadrantFromTopology(topology)
-	if err != nil {
-		return nil, err
+	kuadrant := GetKuadrantFromTopology(topology)
+	if kuadrant == nil {
+		return nil, nil
 	}
 
 	limitadorObj, found := lo.Find(topology.Objects().Children(kuadrant), func(child machinery.Object) bool {

diff --git a/controllers/state_of_the_world.go b/controllers/state_of_the_world.go
@@ -454,17 +454,17 @@ func finalStepsWorkflow(client *dynamic.DynamicClient, isIstioInstalled, isEnvoy
 
 var ErrMissingKuadrant = fmt.Errorf("missing kuadrant object in topology")
 
-func GetKuadrantFromTopology(topology *machinery.Topology) (*kuadrantv1beta1.Kuadrant, error) {
+func GetKuadrantFromTopology(topology *machinery.Topology) *kuadrantv1beta1.Kuadrant {
 	kuadrants := lo.FilterMap(topology.Objects().Roots(), func(root machinery.Object, _ int) (controller.Object, bool) {
 		o, isSortable := root.(controller.Object)
 		return o, isSortable && root.GroupVersionKind().GroupKind() == kuadrantv1beta1.KuadrantGroupKind && o.GetDeletionTimestamp() == nil
 	})
 	if len(kuadrants) == 0 {
-		return nil, ErrMissingKuadrant
+		return nil
 	}
 	sort.Sort(controller.ObjectsByCreationTimestamp(kuadrants))
 	kuadrant, _ := kuadrants[0].(*kuadrantv1beta1.Kuadrant)
-	return kuadrant, nil
+	return kuadrant
 }
 
 func KuadrantManagedObjectLabels() labels.Set {

diff --git a/controllers/state_of_the_world_test.go b/controllers/state_of_the_world_test.go
@@ -66,54 +66,43 @@ func TestGetKuadrant(t *testing.T) {
 		return topology
 	}
 	tests := []struct {
-		name    string
-		args    args
-		want    *kuadrantv1beta1.Kuadrant
-		wantErr bool
+		name string
+		args args
+		want *kuadrantv1beta1.Kuadrant
 	}{
 		{
-			name:    "oldest is first",
-			args:    args{topology: newTopology([]*kuadrantv1beta1.Kuadrant{expected, unexpected})},
-			want:    expected,
-			wantErr: false,
+			name: "oldest is first",
+			args: args{topology: newTopology([]*kuadrantv1beta1.Kuadrant{expected, unexpected})},
+			want: expected,
 		}, {
-			name:    "oldest is second",
-			args:    args{topology: newTopology([]*kuadrantv1beta1.Kuadrant{unexpected, expected})},
-			want:    expected,
-			wantErr: false,
+			name: "oldest is second",
+			args: args{topology: newTopology([]*kuadrantv1beta1.Kuadrant{unexpected, expected})},
+			want: expected,
 		},
 		{
-			name:    "Empty list is passed",
-			args:    args{topology: newTopology([]*kuadrantv1beta1.Kuadrant{})},
-			want:    nil,
-			wantErr: true,
+			name: "Empty list is passed",
+			args: args{topology: newTopology([]*kuadrantv1beta1.Kuadrant{})},
+			want: nil,
 		},
 		{
-			name:    "only item is marked for deletion",
-			args:    args{topology: newTopology([]*kuadrantv1beta1.Kuadrant{deleted})},
-			want:    nil,
-			wantErr: true,
+			name: "only item is marked for deletion",
+			args: args{topology: newTopology([]*kuadrantv1beta1.Kuadrant{deleted})},
+			want: nil,
 		},
 		{
-			name:    "first item is marked for deletion",
-			args:    args{topology: newTopology([]*kuadrantv1beta1.Kuadrant{deleted, expected})},
-			want:    expected,
-			wantErr: false,
+			name: "first item is marked for deletion",
+			args: args{topology: newTopology([]*kuadrantv1beta1.Kuadrant{deleted, expected})},
+			want: expected,
 		},
 		{
-			name:    "all items is marked for deletion",
-			args:    args{topology: newTopology([]*kuadrantv1beta1.Kuadrant{deleted, deleted})},
-			want:    nil,
-			wantErr: true,
+			name: "all items is marked for deletion",
+			args: args{topology: newTopology([]*kuadrantv1beta1.Kuadrant{deleted, deleted})},
+			want: nil,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := GetKuadrantFromTopology(tt.args.topology)
-			if (err != nil) != tt.wantErr {
-				t.Errorf("GetKuadrantFromTopology() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
+			got := GetKuadrantFromTopology(tt.args.topology)
 			if !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("GetKuadrantFromTopology() got = %v, want %v", got, tt.want)
 			}

diff --git a/pkg/kuadrant/conditions.go b/pkg/kuadrant/conditions.go
@@ -18,6 +18,7 @@ const (
 	PolicyReasonOverridden        gatewayapiv1alpha2.PolicyConditionReason = "Overridden"
 	PolicyReasonUnknown           gatewayapiv1alpha2.PolicyConditionReason = "Unknown"
 	PolicyReasonMissingDependency gatewayapiv1alpha2.PolicyConditionReason = "MissingDependency"
+	PolicyReasonMissingResource   gatewayapiv1alpha2.PolicyConditionReason = "MissingResource"
 )
 
 // ConditionMarshal marshals the set of conditions as a JSON array, sorted by condition type.

diff --git a/pkg/kuadrant/errors.go b/pkg/kuadrant/errors.go
@@ -238,3 +238,23 @@ func (e ErrDependencyNotInstalled) Error() string {
 func (e ErrDependencyNotInstalled) Reason() gatewayapiv1alpha2.PolicyConditionReason {
 	return PolicyReasonMissingDependency
 }
+
+func NewErrSystemResource(resourceName string) ErrSystemResource {
+	return ErrSystemResource{
+		resourceName: resourceName,
+	}
+}
+
+var _ PolicyError = ErrSystemResource{}
+
+type ErrSystemResource struct {
+	resourceName string
+}
+
+func (e ErrSystemResource) Error() string {
+	return fmt.Sprintf("%s is not installed, please create resource", e.resourceName)
+}
+
+func (e ErrSystemResource) Reason() gatewayapiv1alpha2.PolicyConditionReason {
+	return PolicyReasonMissingResource
+}