fix: do not reconcile status of target objects multiple times (#607)

controllers/target_status_controller.go

@@ -110,11 +110,11 @@ func (r *TargetStatusReconciler) reconcileResourcesForPolicyKind(parentCtx conte
 		return err
 	}
 	policies := topology.PoliciesFromGateway(gw)
-	gatewayPolicyExists := len(policies) > 0 && utils.Index(policies, func(p kuadrantgatewayapi.Policy) bool { return kuadrantgatewayapi.IsTargetRefGateway(p.GetTargetRef()) }) >= 0
 
 	var errs error
 
 	// if no policies of a kind affecting the gateway → remove condition from the gateway and routes
+	gatewayPolicyExists := len(policies) > 0 && utils.Index(policies, func(p kuadrantgatewayapi.Policy) bool { return kuadrantgatewayapi.IsTargetRefGateway(p.GetTargetRef()) }) >= 0
 	if !gatewayPolicyExists {
 		// remove the condition from the gateway
 		conditionType := policyAffectedConditionType(policyKind)
@@ -136,26 +136,28 @@ func (r *TargetStatusReconciler) reconcileResourcesForPolicyKind(parentCtx conte
 		}
 	}
 
-	var gatewayStatusUpdated bool
+	reconciledTargets := make(map[string]struct{})
 
-	// update the status of the gateway and its routes
-	for i := range policies {
-		policy := policies[i]
-		condition := buildPolicyAffectedCondition(policy)
+	reconcileFunc := func(policy kuadrantgatewayapi.Policy) error {
+		targetRefKey := targetRefKey(policy)
 
 		// update status of targeted route
 		if route := topology.GetPolicyHTTPRoute(policy); route != nil {
-			if err := r.addRouteCondition(ctx, route, condition, gatewayKey, kuadrant.ControllerName); err != nil {
+			if _, updated := reconciledTargets[targetRefKey]; updated { // do not update the same route twice
+				return nil
+			}
+			if err := r.addRouteCondition(ctx, route, buildPolicyAffectedCondition(policy), gatewayKey, kuadrant.ControllerName); err != nil {
 				errs = errors.Join(errs, err)
 			}
-			continue
+			reconciledTargets[targetRefKey] = struct{}{}
+			return nil
 		}
 
-		// update status of the gateway if not already updated before after observing another policy of same kind
-		// this assumes that the gateway is targeted by at most one policy of each kind
-		if gatewayStatusUpdated {
-			continue
+		// update status of targeted gateway and routes not targeted by any policy
+		if _, updated := reconciledTargets[targetRefKey]; updated { // do not update the same gateway twice
+			return nil
 		}
+		condition := buildPolicyAffectedCondition(policy)
 		if c := meta.FindStatusCondition(gw.Status.Conditions, condition.Type); c != nil && c.Status == condition.Status && c.Reason == condition.Reason && c.Message == condition.Message && c.ObservedGeneration == gw.GetGeneration() {
 			logger.V(1).Info("condition already up-to-date, skipping", "condition", condition.Type, "status", condition.Status)
 		} else {
@@ -173,7 +175,21 @@ func (r *TargetStatusReconciler) reconcileResourcesForPolicyKind(parentCtx conte
 				return err
 			}
 		}
-		gatewayStatusUpdated = true
+		reconciledTargets[targetRefKey] = struct{}{}
+
+		return nil
+	}
+
+	// update for policies with status condition Accepted: true
+	for i := range utils.Filter(policies, kuadrantgatewayapi.IsPolicyAccepted) {
+		policy := policies[i]
+		errs = errors.Join(errs, reconcileFunc(policy))
+	}
+
+	// update for policies with status condition Accepted: false
+	for i := range utils.Filter(policies, kuadrantgatewayapi.IsNotPolicyAccepted) {
+		policy := policies[i]
+		errs = errors.Join(errs, reconcileFunc(policy))
 	}
 
 	return errs
@@ -393,3 +409,8 @@ func buildPolicyAffectedCondition(policy kuadrantgatewayapi.Policy) metav1.Condi
 func policyAffectedConditionType(policyKind string) string {
 	return fmt.Sprintf(PolicyAffectedConditionPattern, policyKind)
 }
+
+func targetRefKey(policy kuadrantgatewayapi.Policy) string {
+	targetRef := policy.GetTargetRef()
+	return fmt.Sprintf("%s.%s/%s/%s", targetRef.Group, targetRef.Kind, ptr.Deref(targetRef.Namespace, gatewayapiv1.Namespace(policy.GetNamespace())), targetRef.Name)
+}

controllers/target_status_controller_test.go

@@ -99,6 +99,20 @@ var _ = Describe("Target status reconciler", Ordered, func() {
 		return condition.Status == metav1.ConditionTrue && strings.Contains(condition.Message, policyKey.String())
 	}
 
+	routeNotAffected := func(ctx context.Context, routeName, conditionType string, policyKey client.ObjectKey) bool {
+		route := &gatewayapiv1.HTTPRoute{}
+		err := k8sClient.Get(ctx, client.ObjectKey{Name: routeName, Namespace: testNamespace}, route)
+		if err != nil {
+			return false
+		}
+		routeParentStatus, found := utils.Find(route.Status.RouteStatus.Parents, findRouteParentStatusFunc(route, client.ObjectKey{Name: testGatewayName, Namespace: testNamespace}, kuadrant.ControllerName))
+		if !found {
+			return false
+		}
+		condition := meta.FindStatusCondition(routeParentStatus.Conditions, conditionType)
+		return condition.Status == metav1.ConditionFalse && strings.Contains(condition.Message, policyKey.String())
+	}
+
 	targetsAffected := func(ctx context.Context, policyKey client.ObjectKey, conditionType string, targetRef gatewayapiv1alpha2.PolicyTargetReference, routeNames ...string) bool {
 		switch string(targetRef.Kind) {
 		case "Gateway":
@@ -177,6 +191,44 @@ var _ = Describe("Target status reconciler", Ordered, func() {
 			Eventually(policyAcceptedAndTargetsAffected(ctx, policy)).WithContext(ctx).Should(BeTrue())
 		}, testTimeOut)
 
+		It("Adds truthy PolicyAffected status condition if there is at least one policy accepted", func(ctx SpecContext) {
+			routePolicy1 := policyFactory(func(p *v1beta2.AuthPolicy) {
+				p.Name = "route-auth-1"
+			})
+			Expect(k8sClient.Create(ctx, routePolicy1)).To(Succeed())
+
+			Eventually(policyAcceptedAndTargetsAffected(ctx, routePolicy1)).WithContext(ctx).Should(BeTrue())
+
+			routePolicy2 := policyFactory(func(p *v1beta2.AuthPolicy) { // another policy that targets the same route. this policy will not be accepted
+				p.Name = "route-auth-2"
+			})
+			Expect(k8sClient.Create(ctx, routePolicy2)).To(Succeed())
+
+			Eventually(func() bool {
+				return policyAcceptedAndTargetsAffected(ctx, routePolicy1)() &&
+					!isAuthPolicyAccepted(ctx, routePolicy2)() && !routeAffected(ctx, testHTTPRouteName, policyAffectedCondition, client.ObjectKeyFromObject(routePolicy2))
+			}).WithContext(ctx).Should(BeTrue())
+		}, testTimeOut)
+
+		It("Adds falsey PolicyAffected status condition if no policy is accepted", func(ctx SpecContext) {
+			routePolicy1 := policyFactory(func(p *v1beta2.AuthPolicy) { // create a policy with an invalid route selector so the policy is not accepted
+				p.Name = "route-auth-1"
+				p.Spec.Defaults.RouteSelectors = []v1beta2.RouteSelector{{Hostnames: []gatewayapiv1.Hostname{"invalid.example.com"}}}
+			})
+			Expect(k8sClient.Create(ctx, routePolicy1)).To(Succeed())
+
+			routePolicy2 := policyFactory(func(p *v1beta2.AuthPolicy) { // create another policy with an invalid route selector so the policy is not accepted
+				p.Name = "route-auth-2"
+				p.Spec.Defaults.RouteSelectors = []v1beta2.RouteSelector{{Hostnames: []gatewayapiv1.Hostname{"invalid.example.com"}}}
+			})
+			Expect(k8sClient.Create(ctx, routePolicy2)).To(Succeed())
+
+			Eventually(func() bool {
+				return !isAuthPolicyAccepted(ctx, routePolicy1)() && routeNotAffected(ctx, testHTTPRouteName, policyAffectedCondition, client.ObjectKeyFromObject(routePolicy1)) &&
+					!isAuthPolicyAccepted(ctx, routePolicy2)() && !routeAffected(ctx, testHTTPRouteName, policyAffectedCondition, client.ObjectKeyFromObject(routePolicy2))
+			}).WithContext(ctx).Should(BeTrue())
+		}, testTimeOut)
+
 		It("removes PolicyAffected status condition from the targeted route when the policy is deleted", func(ctx SpecContext) {
 			policy := policyFactory()
 			Expect(k8sClient.Create(ctx, policy)).To(Succeed())

pkg/library/gatewayapi/types_test.go

@@ -24,6 +24,7 @@ type TestPolicy struct {
 	metav1.ObjectMeta `json:"metadata,omitempty"`
 
 	TargetRef gatewayapiv1alpha2.PolicyTargetReference `json:"targetRef"`
+	Status    FakePolicyStatus                         `json:"status"`
 }
 
 func (p *TestPolicy) PolicyClass() PolicyClass {
@@ -35,7 +36,7 @@ func (p *TestPolicy) GetTargetRef() gatewayapiv1alpha2.PolicyTargetReference {
 }
 
 func (p *TestPolicy) GetStatus() PolicyStatus {
-	return &FakePolicyStatus{}
+	return &p.Status
 }
 
 func (p *TestPolicy) DeepCopyObject() runtime.Object {
@@ -61,10 +62,12 @@ func (p *TestPolicy) DeepCopyInto(out *TestPolicy) {
 	p.TargetRef.DeepCopyInto(&out.TargetRef)
 }
 
-type FakePolicyStatus struct{}
+type FakePolicyStatus struct {
+	Conditions []metav1.Condition
+}
 
 func (s *FakePolicyStatus) GetConditions() []metav1.Condition {
-	return nil
+	return s.Conditions
 }
 
 func TestPolicyByCreationTimestamp(t *testing.T) {

pkg/library/gatewayapi/utils.go

@@ -7,6 +7,7 @@ import (
 	"strings"
 
 	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1"
@@ -91,6 +92,16 @@ func IsHTTPRouteAccepted(httpRoute *gatewayapiv1.HTTPRoute) bool {
 	return len(acceptedParentRefs) == len(httpRoute.Spec.ParentRefs)
 }
 
+func IsPolicyAccepted(policy Policy) bool {
+	condition := meta.FindStatusCondition(policy.GetStatus().GetConditions(), string(gatewayapiv1alpha2.PolicyConditionAccepted))
+	return condition != nil && condition.Status == metav1.ConditionTrue
+}
+
+func IsNotPolicyAccepted(policy Policy) bool {
+	condition := meta.FindStatusCondition(policy.GetStatus().GetConditions(), string(gatewayapiv1alpha2.PolicyConditionAccepted))
+	return condition == nil || condition.Status != metav1.ConditionTrue
+}
+
 // GetRouteAcceptedGatewayParentKeys returns the object keys of all gateways that have accepted a given route
 func GetRouteAcceptedGatewayParentKeys(route *gatewayapiv1.HTTPRoute) []client.ObjectKey {
 	acceptedParentRefs := GetRouteAcceptedParentRefs(route)

pkg/library/gatewayapi/utils_test.go

@@ -15,6 +15,9 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1"
+	gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
+
+	"github.com/kuadrant/kuadrant-operator/pkg/library/utils"
 )
 
 func TestGetGatewayWorkloadSelector(t *testing.T) {
@@ -238,6 +241,89 @@ func TestIsHTTPRouteAccepted(t *testing.T) {
 	}
 }
 
+type isPolicyAcceptedTestCase struct {
+	name     string
+	policy   Policy
+	expected bool
+}
+
+var isPolicyAcceptedTestCases = []isPolicyAcceptedTestCase{
+	{
+		name:     "no status",
+		policy:   &TestPolicy{},
+		expected: false,
+	},
+	{
+		name: "no policy accepted condition",
+		policy: &TestPolicy{
+			Status: FakePolicyStatus{
+				Conditions: []metav1.Condition{
+					{
+						Type:   "Other",
+						Status: metav1.ConditionTrue,
+					},
+				},
+			},
+		},
+		expected: false,
+	},
+	{
+		name: "truthy accepted condition",
+		policy: &TestPolicy{
+			Status: FakePolicyStatus{
+				Conditions: []metav1.Condition{
+					{
+						Type:   string(gatewayapiv1alpha2.PolicyConditionAccepted),
+						Status: metav1.ConditionTrue,
+					},
+				},
+			},
+		},
+		expected: true,
+	},
+	{
+		name: "falsey accepted condition",
+		policy: &TestPolicy{
+			Status: FakePolicyStatus{
+				Conditions: []metav1.Condition{
+					{
+						Type:   string(gatewayapiv1alpha2.PolicyConditionAccepted),
+						Status: metav1.ConditionFalse,
+					},
+				},
+			},
+		},
+		expected: false,
+	},
+}
+
+func TestIsPolicyAccepted(t *testing.T) {
+	testCases := isPolicyAcceptedTestCases
+	for _, tc := range testCases {
+		t.Run(tc.name, func(subT *testing.T) {
+			res := IsPolicyAccepted(tc.policy)
+			if res != tc.expected {
+				subT.Errorf("result (%t) does not match expected (%t)", res, tc.expected)
+			}
+		})
+	}
+}
+
+func TestIsNotPolicyAccepted(t *testing.T) {
+	testCases := utils.Map(isPolicyAcceptedTestCases, func(tc isPolicyAcceptedTestCase) isPolicyAcceptedTestCase {
+		tc.expected = !tc.expected
+		return tc
+	})
+	for _, tc := range testCases {
+		t.Run(tc.name, func(subT *testing.T) {
+			res := IsNotPolicyAccepted(tc.policy)
+			if res != tc.expected {
+				subT.Errorf("result (%t) does not match expected (%t)", res, tc.expected)
+			}
+		})
+	}
+}
+
 func TestGetRouteAcceptedParentRefs(t *testing.T) {
 	testCases := []struct {
 		name     string