Resolve comments

Signed-off-by: Lubron Zhan <[email protected]>

changelogs/unreleased/6188-lubronzhan-minor.md

@@ -5,4 +5,6 @@ It's possible that multiple HTTPRoutes will define the same Match conditions. In
 - The oldest Route based on creation timestamp. For example, a Route with a creation timestamp of “2020-09-08 01:02:03” is given precedence over a Route with a creation timestamp of “2020-09-08 01:02:04”.
 - The Route appearing first in alphabetical order (namespace/name) for example, foo/bar is given precedence over foo/baz.
 
-With above ordering, any HTTPRoute that ranks lower is marked with the `Accepted: False` Condition and Reason `RuleMatchConflict `.
+With above ordering, any HTTPRoute that ranks lower, will be marked with below conditions accordionly
+1. If only partial rules under this HTTPRoute are conflicted, it's marked with `Accepted: True` and `PartiallyInvalid: true` Conditions and Reason: `RuleMatchPartiallyConflict`.
+2. If all the rules under this HTTPRoute are conflicted, it's marked with `Accepted: False` Condition and Reason `RuleMatchConflict`.

internal/dag/gatewayapi_processor.go

@@ -2462,7 +2462,7 @@ func handlePathRewritePrefixRemoval(p *PathRewritePolicy, mc *matchConditions) *
 }
 
 // sort routes based on creationTimestamp in ascending order
-// if creationTimestamps are the same, sort based on name, then namespace in alphetical ascending order
+// if creationTimestamps are the same, sort based on namespaced name ("<namespace>/<name>") in alphetical ascending order
 func sortRoutes(m map[types.NamespacedName]*gatewayapi_v1.HTTPRoute) []*gatewayapi_v1.HTTPRoute {
 	routes := []*gatewayapi_v1.HTTPRoute{}
 	for _, r := range m {

test/e2e/gateway/http_route_conflict_match_test.go

@@ -17,6 +17,7 @@ package gateway
 
 import (
 	. "github.com/onsi/ginkgo/v2"
+	"github.com/stretchr/testify/require"
 	meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	gatewayapi_v1 "sigs.k8s.io/gateway-api/apis/v1"
@@ -56,7 +57,8 @@ func testHTTPRouteConflictMatch(namespace string, gateway types.NamespacedName)
 				},
 			},
 		}
-		f.CreateHTTPRouteAndWaitFor(route1, e2e.HTTPRouteAccepted)
+		_, ok := f.CreateHTTPRouteAndWaitFor(route1, e2e.HTTPRouteAccepted)
+		require.True(f.T(), ok)
 
 		By("create httproute-2 with conflicted matches")
 		route2 := &gatewayapi_v1.HTTPRoute{
@@ -81,6 +83,7 @@ func testHTTPRouteConflictMatch(namespace string, gateway types.NamespacedName)
 				},
 			},
 		}
-		f.CreateHTTPRouteAndWaitFor(route2, e2e.HTTPRouteNotAcceptedDueToConflict)
+		_, ok := f.CreateHTTPRouteAndWaitFor(route2, e2e.HTTPRouteNotAcceptedDueToConflict)
+		require.True(f.T(), ok)
 	})
 }

test/e2e/gateway/http_route_partially_conflict_match_test.go

@@ -61,7 +61,8 @@ func testHTTPRoutePartiallyConflictMatch(namespace string, gateway types.Namespa
 				},
 			},
 		}
-		f.CreateHTTPRouteAndWaitFor(route1, e2e.HTTPRouteAccepted)
+		_, ok := f.CreateHTTPRouteAndWaitFor(route1, e2e.HTTPRouteAccepted)
+		require.True(f.T(), ok)
 
 		By("create httproute-2 with only partially conflicted matches")
 		route2 := &gatewayapi_v1.HTTPRoute{
@@ -93,13 +94,8 @@ func testHTTPRoutePartiallyConflictMatch(namespace string, gateway types.Namespa
 			},
 		}
 		// Partially accepted
-		f.CreateHTTPRouteAndWaitFor(route2, e2e.HTTPRoutePartiallyAccepted)
-		// Still has Accepted: true
-		require.Eventually(f.T(), func() bool {
-			if err := f.Client.Get(context.TODO(), client.ObjectKeyFromObject(route2), route2); err != nil {
-				return false
-			}
-			return e2e.HTTPRouteAccepted(route2)
-		}, time.Minute*2, f.RetryInterval)
+		f.CreateHTTPRouteAndWaitFor(route2, func(...) {
+			return e2e.HTTPRoutePartiallyInvalid(route2) && e2e.HTTPRouteAccepted(route2)
+		})
 	})
 }

test/e2e/gatewayapi_predicates.go

@@ -131,24 +131,24 @@ func HTTPRouteNotAcceptedDueToConflict(route *gatewayapi_v1.HTTPRoute) bool {
 
 	for _, gw := range route.Status.Parents {
 		if conditionExistsWithAllKeys(gw.Conditions, string(gatewayapi_v1.RouteConditionAccepted), meta_v1.ConditionFalse, string(status.ReasonRouteRuleMatchConflict), status.MessageRouteRuleMatchConflict) {
-			return false
+			return true
 		}
 	}
 
 	return false
 }
 
-// HTTPRoutePartiallyAccepted returns true if the route has a .status.conditions
+// HTTPRoutePartiallyInvalid returns true if the route has a .status.conditions
 // entry of "PartiallyInvalid: true" && "Reason: RuleMatchPartiallyConflict" && "Message:
 // HTTPRoute's Match has partial conflict with other HTTPRoute's Match".
-func HTTPRoutePartiallyAccepted(route *gatewayapi_v1.HTTPRoute) bool {
+func HTTPRoutePartiallyInvalid(route *gatewayapi_v1.HTTPRoute) bool {
 	if route == nil {
 		return false
 	}
 
 	for _, gw := range route.Status.Parents {
 		if conditionExistsWithAllKeys(gw.Conditions, string(gatewayapi_v1.RouteConditionPartiallyInvalid), meta_v1.ConditionTrue, string(status.ReasonRouteRuleMatchPartiallyConflict), status.MessageRouteRuleMatchPartiallyConflict) {
-			return false
+			return true
 		}
 	}