Refactor HTTPRoutePolicy status handling and test logic

Removed unstable test case loop and streamlined HTTPRoutePolicy status condition checks.
Improved condition matching logic and updated related utility functions for better readability and maintainability.
The changes also include minor package import reorganizations and logging improvements across multiple files.

Author: dspo

internal/controller/httproute_controller.go

@@ -17,6 +17,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/api7/gopkg/pkg/log"
 	"github.com/go-logr/logr"
 	"github.com/pkg/errors"
 	"go.uber.org/zap"
@@ -38,8 +39,6 @@ import (
 	gatewayv1 "sigs.k8s.io/gateway-api/apis/v1"
 	"sigs.k8s.io/gateway-api/apis/v1alpha2"
 
-	"github.com/api7/gopkg/pkg/log"
-
 	"github.com/apache/apisix-ingress-controller/api/v1alpha1"
 	"github.com/apache/apisix-ingress-controller/internal/controller/indexer"
 	"github.com/apache/apisix-ingress-controller/internal/provider"

internal/controller/httproutepolicy.go

@@ -190,15 +190,12 @@ func (r *IngressReconciler) updateHTTPRoutePolicyStatusOnDeleting(ctx context.Co
 func modifyHTTPRoutePolicyStatus(parentRefs []gatewayv1.ParentReference, policy *v1alpha1.HTTPRoutePolicy, status bool, reason, message string) {
 	condition := metav1.Condition{
 		Type:               string(v1alpha2.PolicyConditionAccepted),
-		Status:             metav1.ConditionTrue,
+		Status:             ConditionStatus(status),
 		ObservedGeneration: policy.GetGeneration(),
 		LastTransitionTime: metav1.Now(),
 		Reason:             reason,
 		Message:            message,
 	}
-	if !status {
-		condition.Status = metav1.ConditionFalse
-	}
 	_ = SetAncestors(&policy.Status, parentRefs, condition)
 }
 

internal/controller/ingress_controller.go

@@ -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

@@ -23,11 +23,11 @@ import (
 	"log"
 	"net/http"
 	"net/url"
+	"slices"
 	"strings"
 	"sync"
 	"time"
 
-	"github.com/apache/apisix-ingress-controller/api/v1alpha1"
 	"github.com/gavv/httpexpect/v2"
 	"github.com/gruntwork-io/terratest/modules/testing"
 	"github.com/onsi/gomega"
@@ -36,32 +36,18 @@ import (
 	"golang.org/x/net/html"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
-	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"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"
-)
 
-var (
-	scheme_ = runtime.NewScheme()
+	"github.com/apache/apisix-ingress-controller/api/v1alpha1"
 )
 
-func init() {
-	utilruntime.Must(scheme.AddToScheme(scheme_))
-	if err := gatewayv1.Install(scheme_); err != nil {
-		panic(err)
-	}
-	if err := v1alpha1.AddToScheme(scheme_); err != nil {
-		panic(err)
-	}
-}
-
 func (f *Framework) NewExpectResponse(httpBody any) *httpexpect.Response {
 	body, err := json.Marshal(httpBody)
 	f.GomegaT.Expect(err).ShouldNot(gomega.HaveOccurred())
@@ -395,31 +381,32 @@ 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) {
+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 {
-		var (
-			ancestors      = status.Ancestors
-			conditionFound bool
-		)
-		for _, ancestor := range ancestors {
+		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 || string(*ancestor.AncestorRef.Namespace) == refNN.Namespace) {
-				if findConditionInList(t, ancestor.Conditions, condition) {
-					conditionFound = true
+			if ancestor.AncestorRef.Name == gatewayv1.ObjectName(refNN.Name) && (ancestor.AncestorRef.Namespace == nil ||
+				string(*ancestor.AncestorRef.Namespace) == refNN.Namespace) {
+				if findConditionInList(ancestor.Conditions, condition) {
+					return true
+				} else {
+					log.Printf("not found condition %v in list [%v]", condition, ancestor.Conditions)
 				}
 			}
 		}
-		return conditionFound
+		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 {
+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
@@ -447,22 +434,10 @@ func parentRefToString(p gatewayv1.ParentReference) string {
 	return string(p.Name)
 }
 
-func findConditionInList(t testing.TestingT, conditions []metav1.Condition, expected metav1.Condition) bool {
-	for _, cond := range conditions {
-		if cond.Type == expected.Type {
-			// an empty Status string means "Match any status".
-			if expected.Status == "" || cond.Status == expected.Status {
-				// an empty Reason string means "Match any reason".
-				if expected.Reason == "" || cond.Reason == expected.Reason {
-					return true
-				}
-				log.Printf("%s condition Reason set to %s, expected %s", expected.Type, cond.Reason, expected.Reason)
-			}
-
-			log.Printf("%s condition set to Status %s with Reason %v, expected Status %s", expected.Type, cond.Status, cond.Reason, expected.Status)
-		}
-	}
-
-	log.Printf("%s was not in conditions list [%v]", expected.Type, conditions)
-	return false
+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)
+	})
 }

test/e2e/ingress/ingress.go

@@ -20,15 +20,16 @@ import (
 	"strings"
 	"time"
 
-	"github.com/apache/apisix-ingress-controller/api/v1alpha1"
-	"github.com/apache/apisix-ingress-controller/test/e2e/framework"
-	"github.com/apache/apisix-ingress-controller/test/e2e/scaffold"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/stretchr/testify/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
+
+	"github.com/apache/apisix-ingress-controller/api/v1alpha1"
+	"github.com/apache/apisix-ingress-controller/test/e2e/framework"
+	"github.com/apache/apisix-ingress-controller/test/e2e/scaffold"
 )
 
 const _secretName = "test-ingress-tls"
@@ -725,39 +726,36 @@ spec:
 				WithTimeout(8 * time.Second).ProbeEvery(time.Second).Should(Equal(http.StatusOK))
 		})
 
-		// todo: unstable test case, pending for now
-		for i := 0; i < 100; i++ {
-			It("HTTPRoutePolicy status changes on Ingress deleting", func() {
-				By("create Ingress")
-				err := s.CreateResourceFromString(ingressSpec)
-				Expect(err).NotTo(HaveOccurred(), "creating Ingress")
-
-				By("create HTTPRoutePolicy")
-				err = s.CreateResourceFromString(httpRoutePolicySpec0)
-				Expect(err).NotTo(HaveOccurred(), "creating HTTPRoutePolicy")
-				framework.HTTPRoutePolicyMustHaveCondition(s.GinkgoT, s.K8sClient, 8*time.Second,
-					types.NamespacedName{Namespace: s.Namespace(), Name: "apisix"},
-					types.NamespacedName{Namespace: s.Namespace(), Name: "http-route-policy-0"},
-					metav1.Condition{
-						Type:   string(gatewayv1alpha2.PolicyConditionAccepted),
-						Status: metav1.ConditionTrue,
-						Reason: string(gatewayv1alpha2.PolicyReasonAccepted),
-					},
-				)
-
-				By("delete ingress")
-				err = s.DeleteResource("Ingress", "default")
-				Expect(err).NotTo(HaveOccurred(), "delete Ingress")
-
-				err = framework.EventuallyHTTPRoutePolicyHaveStatus(s.K8sClient, 8*time.Second,
-					types.NamespacedName{Namespace: s.Namespace(), Name: "http-route-policy-0"},
-					func(_ v1alpha1.HTTPRoutePolicy, status v1alpha1.PolicyStatus) bool {
-						return len(status.Ancestors) == 0
-					},
-				)
-				Expect(err).NotTo(HaveOccurred(), "expected HTTPRoutePolicy.Status has no Ancestor")
-			})
-		}
+		It("HTTPRoutePolicy status changes on Ingress deleting", func() {
+			By("create Ingress")
+			err := s.CreateResourceFromString(ingressSpec)
+			Expect(err).NotTo(HaveOccurred(), "creating Ingress")
+
+			By("create HTTPRoutePolicy")
+			err = s.CreateResourceFromString(httpRoutePolicySpec0)
+			Expect(err).NotTo(HaveOccurred(), "creating HTTPRoutePolicy")
+			framework.HTTPRoutePolicyMustHaveCondition(s.GinkgoT, s.K8sClient, 8*time.Second,
+				types.NamespacedName{Namespace: s.Namespace(), Name: "apisix"},
+				types.NamespacedName{Namespace: s.Namespace(), Name: "http-route-policy-0"},
+				metav1.Condition{
+					Type:   string(gatewayv1alpha2.PolicyConditionAccepted),
+					Status: metav1.ConditionTrue,
+					Reason: string(gatewayv1alpha2.PolicyReasonAccepted),
+				},
+			)
+
+			By("delete ingress")
+			err = s.DeleteResource("Ingress", "default")
+			Expect(err).NotTo(HaveOccurred(), "delete Ingress")
+
+			err = framework.EventuallyHTTPRoutePolicyHaveStatus(s.K8sClient, 8*time.Second,
+				types.NamespacedName{Namespace: s.Namespace(), Name: "http-route-policy-0"},
+				func(_ v1alpha1.HTTPRoutePolicy, status v1alpha1.PolicyStatus) bool {
+					return len(status.Ancestors) == 0
+				},
+			)
+			Expect(err).NotTo(HaveOccurred(), "expected HTTPRoutePolicy.Status has no Ancestor")
+		})
 	})
 
 	Context("Ingress with GatewayProxy Update", func() {