fix: Refactor HTTPRoutePolicy status handling and tests

Streamline HTTPRoutePolicy status updates by introducing helper functions and improving condition handling.
Activate previously skipped tests for HTTPRoutePolicy conflicts and status changes.
Replace manual status modifications with structured condition updates across ingress and gateway controllers.

Author: dspo

internal/controller/httproute_controller.go

@@ -114,7 +114,7 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	hr := new(gatewayv1.HTTPRoute)
 	if err := r.Get(ctx, req.NamespacedName, hr); err != nil {
 		if client.IgnoreNotFound(err) == nil {
-			if err := r.updateHTTPRoutePolicyStatusOnDeleting(req.NamespacedName); err != nil {
+			if err := r.updateHTTPRoutePolicyStatusOnDeleting(ctx, req.NamespacedName); err != nil {
 				return ctrl.Result{}, err
 			}
 			hr.Namespace = req.Namespace

internal/controller/httproutepolicy.go

@@ -13,6 +13,7 @@
 package controller
 
 import (
+	"cmp"
 	"context"
 	"slices"
 
@@ -59,26 +60,25 @@ func (r *HTTPRouteReconciler) processHTTPRoutePolicies(tctx *provider.TranslateC
 		var (
 			policy         = list.Items[i]
 			namespacedName = types.NamespacedName{Namespace: policy.GetNamespace(), Name: policy.GetName()}
-
-			status  = false
-			reason  = string(v1alpha2.PolicyReasonConflicted)
-			message = "HTTPRoutePolicy conflict with others target to the HTTPRoute"
+			condition      metav1.Condition
 		)
-		if _, conflict := conflicts[namespacedName]; !conflict {
-			status = true
-			reason = string(v1alpha2.PolicyReasonAccepted)
-			message = ""
-
+		if _, conflict := conflicts[namespacedName]; conflict {
+			condition.Status = metav1.ConditionFalse
+			condition.Reason = string(v1alpha2.PolicyReasonConflicted)
+			condition.Message = "HTTPRoutePolicy conflict with others target to the HTTPRoute"
+		} else {
 			tctx.HTTPRoutePolicies = append(tctx.HTTPRoutePolicies, policy)
 		}
-		modifyHTTPRoutePolicyStatus(httpRoute.Spec.ParentRefs, &policy, status, reason, message)
-		tctx.StatusUpdaters = append(tctx.StatusUpdaters, &policy)
+
+		if updated := setAncestorsForHTTPRoutePolicyStatus(httpRoute.Spec.ParentRefs, &policy, condition); updated {
+			tctx.StatusUpdaters = append(tctx.StatusUpdaters, &policy)
+		}
 	}
 
 	return nil
 }
 
-func (r *HTTPRouteReconciler) updateHTTPRoutePolicyStatusOnDeleting(nn types.NamespacedName) error {
+func (r *HTTPRouteReconciler) updateHTTPRoutePolicyStatusOnDeleting(ctx context.Context, nn types.NamespacedName) error {
 	var (
 		list v1alpha1.HTTPRoutePolicyList
 		key  = indexer.GenIndexKeyWithGK(gatewayv1.GroupName, "HTTPRoute", nn.Namespace, nn.Name)
@@ -104,7 +104,7 @@ func (r *HTTPRouteReconciler) updateHTTPRoutePolicyStatusOnDeleting(nn types.Nam
 			parentRefs = append(parentRefs, httpRoute.Spec.ParentRefs...)
 		}
 		// delete AncestorRef which is not exist in the all parentRefs for each policy
-		updateDeleteAncestors(r.Client, r.Log, policy, parentRefs)
+		updateDeleteAncestors(ctx, r.Client, r.Log, policy, parentRefs)
 	}
 
 	return nil
@@ -124,33 +124,32 @@ func (r *IngressReconciler) processHTTPRoutePolicies(tctx *provider.TranslateCon
 	}
 
 	var (
-		status  = false
-		reason  = string(v1alpha2.PolicyReasonConflicted)
-		message = "HTTPRoutePolicy conflict with others target to the Ingress"
+		condition metav1.Condition
 	)
-	if conflict := checkPoliciesConflict(list.Items); !conflict {
-		status = true
-		reason = string(v1alpha2.PolicyReasonAccepted)
-		message = ""
-
+	if conflict := checkPoliciesConflict(list.Items); conflict {
+		condition.Status = metav1.ConditionFalse
+		condition.Reason = string(v1alpha2.PolicyReasonConflicted)
+		condition.Message = "HTTPRoutePolicy conflict with others target to the Ingress"
+	} else {
 		tctx.HTTPRoutePolicies = list.Items
 	}
 
 	for i := range list.Items {
 		policy := list.Items[i]
-		modifyHTTPRoutePolicyStatus(tctx.RouteParentRefs, &policy, status, reason, message)
-		tctx.StatusUpdaters = append(tctx.StatusUpdaters, &policy)
+		if updated := setAncestorsForHTTPRoutePolicyStatus(tctx.RouteParentRefs, &policy, condition); updated {
+			tctx.StatusUpdaters = append(tctx.StatusUpdaters, &policy)
+		}
 	}
 
 	return nil
 }
 
-func (r *IngressReconciler) updateHTTPRoutePolicyStatusOnDeleting(nn types.NamespacedName) error {
+func (r *IngressReconciler) updateHTTPRoutePolicyStatusOnDeleting(ctx context.Context, nn types.NamespacedName) error {
 	var (
 		list v1alpha1.HTTPRoutePolicyList
 		key  = indexer.GenIndexKeyWithGK(networkingv1.GroupName, "Ingress", nn.Namespace, nn.Name)
 	)
-	if err := r.List(context.Background(), &list, client.MatchingFields{indexer.PolicyTargetRefs: key}); err != nil {
+	if err := r.List(ctx, &list, client.MatchingFields{indexer.PolicyTargetRefs: key}); err != nil {
 		return err
 	}
 	var (
@@ -164,7 +163,7 @@ func (r *IngressReconciler) updateHTTPRoutePolicyStatusOnDeleting(nn types.Names
 			parentRef, ok := ingress2ParentRef[namespacedName]
 			if !ok {
 				var ingress networkingv1.Ingress
-				if err := r.Get(context.Background(), namespacedName, &ingress); err != nil {
+				if err := r.Get(ctx, namespacedName, &ingress); err != nil {
 					continue
 				}
 				ingressClass, err := r.getIngressClass(&ingress)
@@ -173,33 +172,29 @@ func (r *IngressReconciler) updateHTTPRoutePolicyStatusOnDeleting(nn types.Names
 				}
 				parentRef = gatewayv1.ParentReference{
 					Group: ptr.To(gatewayv1.Group(ingressClass.GroupVersionKind().Group)),
-					Kind:  ptr.To(gatewayv1.Kind("IngressClass")),
+					Kind:  ptr.To(gatewayv1.Kind(KindIngressClass)),
 					Name:  gatewayv1.ObjectName(ingressClass.Name),
 				}
 				ingress2ParentRef[namespacedName] = parentRef
 			}
 			parentRefs = append(parentRefs, parentRef)
 		}
 		// delete AncestorRef which is not exist in the all parentRefs
-		updateDeleteAncestors(r.Client, r.Log, policy, parentRefs)
+		updateDeleteAncestors(ctx, r.Client, r.Log, policy, parentRefs)
 	}
 
 	return nil
 }
 
-func modifyHTTPRoutePolicyStatus(parentRefs []gatewayv1.ParentReference, policy *v1alpha1.HTTPRoutePolicy, status bool, reason, message string) {
-	condition := metav1.Condition{
-		Type:               string(v1alpha2.PolicyConditionAccepted),
-		Status:             metav1.ConditionTrue,
+func setAncestorsForHTTPRoutePolicyStatus(parentRefs []gatewayv1.ParentReference, policy *v1alpha1.HTTPRoutePolicy, condition metav1.Condition) bool {
+	return SetAncestors(&policy.Status, parentRefs, metav1.Condition{
+		Type:               cmp.Or(condition.Type, string(v1alpha2.PolicyConditionAccepted)),
+		Status:             cmp.Or(condition.Status, metav1.ConditionTrue),
 		ObservedGeneration: policy.GetGeneration(),
 		LastTransitionTime: metav1.Now(),
-		Reason:             reason,
-		Message:            message,
-	}
-	if !status {
-		condition.Status = metav1.ConditionFalse
-	}
-	_ = SetAncestors(&policy.Status, parentRefs, condition)
+		Reason:             cmp.Or(condition.Reason, string(v1alpha2.PolicyReasonAccepted)),
+		Message:            condition.Message,
+	})
 }
 
 // checkPoliciesConflict determines if there is a conflict among the given HTTPRoutePolicy objects based on their priority values.
@@ -234,15 +229,15 @@ func findPoliciesWhichTargetRefTheRule(ruleName *gatewayv1.SectionName, kind str
 }
 
 // updateDeleteAncestors removes ancestor references from HTTPRoutePolicy statuses that are no longer present in the provided parentRefs.
-func updateDeleteAncestors(client client.Client, logger logr.Logger, policy v1alpha1.HTTPRoutePolicy, parentRefs []gatewayv1.ParentReference) {
+func updateDeleteAncestors(ctx context.Context, client client.Client, logger logr.Logger, policy v1alpha1.HTTPRoutePolicy, parentRefs []gatewayv1.ParentReference) {
 	length := len(policy.Status.Ancestors)
-	policy.Status.Ancestors = slices.DeleteFunc(policy.Status.Ancestors, func(status v1alpha2.PolicyAncestorStatus) bool {
+	policy.Status.Ancestors = slices.DeleteFunc(policy.Status.Ancestors, func(ancestor v1alpha2.PolicyAncestorStatus) bool {
 		return !slices.ContainsFunc(parentRefs, func(ref gatewayv1.ParentReference) bool {
-			return parentRefValueEqual(status.AncestorRef, ref)
+			return parentRefValueEqual(ancestor.AncestorRef, ref)
 		})
 	})
 	if length != len(policy.Status.Ancestors) {
-		if err := client.Status().Update(context.Background(), &policy); err != nil {
+		if err := client.Status().Update(ctx, &policy); err != nil {
 			logger.Error(err, "failed to update HTTPRoutePolicy status")
 		}
 	}

internal/controller/ingress_controller.go

@@ -112,7 +112,7 @@ func (r *IngressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 	ingress := new(networkingv1.Ingress)
 	if err := r.Get(ctx, req.NamespacedName, ingress); err != nil {
 		if client.IgnoreNotFound(err) == nil {
-			if err := r.updateHTTPRoutePolicyStatusOnDeleting(req.NamespacedName); err != nil {
+			if err := r.updateHTTPRoutePolicyStatusOnDeleting(ctx, req.NamespacedName); err != nil {
 				return ctrl.Result{}, err
 			}
 
@@ -148,7 +148,7 @@ func (r *IngressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 
 	tctx.RouteParentRefs = append(tctx.RouteParentRefs, gatewayv1.ParentReference{
 		Group: ptr.To(gatewayv1.Group(ingressClass.GroupVersionKind().Group)),
-		Kind:  ptr.To(gatewayv1.Kind("IngressClass")),
+		Kind:  ptr.To(gatewayv1.Kind(KindIngressClass)),
 		Name:  gatewayv1.ObjectName(ingressClass.Name),
 	})
 

test/e2e/framework/utils.go

@@ -20,20 +20,32 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
+	"log"
 	"net/http"
 	"net/url"
+	"slices"
 	"strings"
 	"sync"
 	"time"
 
 	"github.com/gavv/httpexpect/v2"
+	"github.com/gruntwork-io/terratest/modules/testing"
 	"github.com/onsi/gomega"
+	"github.com/pkg/errors"
+	"github.com/stretchr/testify/require"
 	"golang.org/x/net/html"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/tools/remotecommand"
 	"k8s.io/utils/ptr"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	gatewayv1 "sigs.k8s.io/gateway-api/apis/v1"
+	"sigs.k8s.io/gateway-api/conformance/utils/kubernetes"
+
+	"github.com/apache/apisix-ingress-controller/api/v1alpha1"
 )
 
 func (f *Framework) NewExpectResponse(httpBody any) *httpexpect.Response {
@@ -369,6 +381,43 @@ func CreateTestZipFile(sourceCode, metadata string) ([]byte, error) {
 	return zipBuffer.Bytes(), nil
 }
 
+func HTTPRoutePolicyMustHaveCondition(t testing.TestingT, client client.Client, timeout time.Duration, refNN, hrpNN types.NamespacedName,
+	condition metav1.Condition) {
+	err := EventuallyHTTPRoutePolicyHaveStatus(client, timeout, hrpNN, func(httpRoutePolicy v1alpha1.HTTPRoutePolicy, status v1alpha1.PolicyStatus) bool {
+		for _, ancestor := range status.Ancestors {
+			if err := kubernetes.ConditionsHaveLatestObservedGeneration(&httpRoutePolicy, ancestor.Conditions); err != nil {
+				log.Printf("HTTPRoutePolicy %s (parentRef=%v) %v", hrpNN, parentRefToString(ancestor.AncestorRef), err)
+				return false
+			}
+
+			if ancestor.AncestorRef.Name == gatewayv1.ObjectName(refNN.Name) &&
+				(ancestor.AncestorRef.Namespace == nil || refNN.Namespace == "" || string(*ancestor.AncestorRef.Namespace) == refNN.Namespace) {
+				if findConditionInList(ancestor.Conditions, condition) {
+					log.Printf("found condition %v in list [%v] for %s reference %s", condition, ancestor.Conditions, hrpNN, refNN)
+					return true
+				} else {
+					log.Printf("not found condition %v in list [%v] for %s reference %s", condition, ancestor.Conditions, hrpNN, refNN)
+				}
+			}
+		}
+		return false
+	})
+
+	require.NoError(t, err, "error waiting for HTTPRoutePolicy status to have a Condition matching expectations")
+}
+
+func EventuallyHTTPRoutePolicyHaveStatus(client client.Client, timeout time.Duration, hrpNN types.NamespacedName,
+	f func(httpRoutePolicy v1alpha1.HTTPRoutePolicy, status v1alpha1.PolicyStatus) bool) error {
+	_ = v1alpha1.AddToScheme(client.Scheme())
+	return wait.PollUntilContextTimeout(context.Background(), time.Second, timeout, true, func(ctx context.Context) (done bool, err error) {
+		var httpRoutePolicy v1alpha1.HTTPRoutePolicy
+		if err = client.Get(ctx, hrpNN, &httpRoutePolicy); err != nil {
+			return false, errors.Errorf("error fetching HTTPRoutePolicy %v: %v", hrpNN, err)
+		}
+		return f(httpRoutePolicy, httpRoutePolicy.Status), nil
+	})
+}
+
 func addFileToZip(zipWriter *zip.Writer, fileName, fileContent string) error {
 	file, err := zipWriter.Create(fileName)
 	if err != nil {
@@ -378,3 +427,18 @@ func addFileToZip(zipWriter *zip.Writer, fileName, fileContent string) error {
 	_, err = file.Write([]byte(fileContent))
 	return err
 }
+
+func parentRefToString(p gatewayv1.ParentReference) string {
+	if p.Namespace != nil && *p.Namespace != "" {
+		return fmt.Sprintf("%v/%v", p.Namespace, p.Name)
+	}
+	return string(p.Name)
+}
+
+func findConditionInList(conditions []metav1.Condition, expected metav1.Condition) bool {
+	return slices.ContainsFunc(conditions, func(item metav1.Condition) bool {
+		// an empty Status string means "Match any status".
+		// an empty Reason string means "Match any reason".
+		return expected.Type == item.Type && (expected.Status == "" || expected.Status == item.Status) && (expected.Reason == "" || expected.Reason == item.Reason)
+	})
+}