handle Route conflicts with HTTPRoute.Matches

changelogs/unreleased/6188-lubronzhan-minor.md

@@ -0,0 +1,10 @@
+## Gateway API: handle Route conflicts with HTTPRoute.Matches
+
+It's possible that multiple HTTPRoutes will define the same Match conditions. In this case the following logic is applied to resolve the conflict:
+
+- 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, 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/dag.go

@@ -751,13 +751,25 @@ type VirtualHost struct {
 	Routes map[string]*Route
 }
 
+// Add route to VirtualHosts.Routes map.
 func (v *VirtualHost) AddRoute(route *Route) {
 	if v.Routes == nil {
 		v.Routes = make(map[string]*Route)
 	}
+
 	v.Routes[conditionsToString(route)] = route
 }
 
+// HasConflictRoute returns true if there is existing Path + Headers
+// + QueryParams combination match this route candidate and also they are same kind of Route.
+func (v *VirtualHost) HasConflictRoute(route *Route) bool {
+	// If match exist and kind is the same kind, return true.
+	if r, ok := v.Routes[conditionsToString(route)]; ok && r.Kind == route.Kind {
+		return true
+	}
+	return false
+}
+
 func conditionsToString(r *Route) string {
 	s := []string{r.PathMatchCondition.String()}
 	for _, cond := range r.HeaderMatchConditions {

internal/dag/dag_test.go

@@ -265,3 +265,200 @@ func TestServiceClusterRebalance(t *testing.T) {
 		})
 	}
 }
+
+func TestHasConflictRouteForVirtualHost(t *testing.T) {
+	cases := map[string]struct {
+		vHost          VirtualHost
+		rs             []Route
+		expectConflict bool
+	}{
+		"2 different routes with same path and header, with same kind, expect conflict": {
+			vHost: VirtualHost{
+				Routes: map[string]*Route{},
+			},
+			rs: []Route{
+				{
+					Kind:               KindHTTPRoute,
+					Name:               "a",
+					Namespace:          "b",
+					PathMatchCondition: prefixSegment("/path1"),
+					HeaderMatchConditions: []HeaderMatchCondition{
+						{Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"},
+					},
+				},
+				{
+					Kind:               KindHTTPRoute,
+					Name:               "b",
+					Namespace:          "c",
+					PathMatchCondition: prefixSegment("/path1"),
+					HeaderMatchConditions: []HeaderMatchCondition{
+						{Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"},
+					},
+				},
+			},
+			expectConflict: true,
+		},
+		"2 different routes with same path and header and query params, with same kind, expect conflict": {
+			vHost: VirtualHost{
+				Routes: map[string]*Route{},
+			},
+			rs: []Route{
+				{
+					Kind:               KindHTTPRoute,
+					Name:               "a",
+					Namespace:          "b",
+					PathMatchCondition: prefixSegment("/path1"),
+					HeaderMatchConditions: []HeaderMatchCondition{
+						{Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"},
+					},
+					QueryParamMatchConditions: []QueryParamMatchCondition{
+						{Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact},
+					},
+				},
+				{
+					Kind:               KindHTTPRoute,
+					Name:               "c",
+					Namespace:          "d",
+					PathMatchCondition: prefixSegment("/path1"),
+					HeaderMatchConditions: []HeaderMatchCondition{
+						{Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"},
+					},
+					QueryParamMatchConditions: []QueryParamMatchCondition{
+						{Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact},
+					},
+				},
+			},
+			expectConflict: true,
+		},
+		"2 different routes with same path and header, but different kind, expect no conflict": {
+			vHost: VirtualHost{
+				Routes: map[string]*Route{},
+			},
+			rs: []Route{
+				{
+					Kind:               KindGRPCRoute,
+					Name:               "a",
+					Namespace:          "b",
+					PathMatchCondition: prefixSegment("/path1"),
+					HeaderMatchConditions: []HeaderMatchCondition{
+						{Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"},
+					},
+				},
+				{
+					Kind:               KindHTTPRoute,
+					Name:               "b",
+					Namespace:          "c",
+					PathMatchCondition: prefixSegment("/path1"),
+					HeaderMatchConditions: []HeaderMatchCondition{
+						{Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"},
+					},
+				},
+			},
+			expectConflict: false,
+		},
+		"2 different routes with same path and header and query params, but different kind, expect no conflict": {
+			vHost: VirtualHost{
+				Routes: map[string]*Route{},
+			},
+			rs: []Route{
+				{
+					Kind:               KindHTTPRoute,
+					Name:               "a",
+					Namespace:          "b",
+					PathMatchCondition: prefixSegment("/path1"),
+					HeaderMatchConditions: []HeaderMatchCondition{
+						{Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"},
+					},
+					QueryParamMatchConditions: []QueryParamMatchCondition{
+						{Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact},
+					},
+				},
+				{
+					Kind:               KindGRPCRoute,
+					Name:               "c",
+					Namespace:          "d",
+					PathMatchCondition: prefixSegment("/path1"),
+					HeaderMatchConditions: []HeaderMatchCondition{
+						{Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"},
+					},
+					QueryParamMatchConditions: []QueryParamMatchCondition{
+						{Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact},
+					},
+				},
+			},
+			expectConflict: false,
+		},
+		"2 different routes with same path and header, but different query params, with same kind, don't expect conflict": {
+			vHost: VirtualHost{
+				Routes: map[string]*Route{},
+			},
+			rs: []Route{
+				{
+					Kind:               KindHTTPRoute,
+					Name:               "a",
+					Namespace:          "b",
+					PathMatchCondition: prefixSegment("/path1"),
+					HeaderMatchConditions: []HeaderMatchCondition{
+						{Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"},
+					},
+					QueryParamMatchConditions: []QueryParamMatchCondition{
+						{Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact},
+					},
+				},
+				{
+					Kind:               KindHTTPRoute,
+					Name:               "c",
+					Namespace:          "d",
+					PathMatchCondition: prefixSegment("/path1"),
+					HeaderMatchConditions: []HeaderMatchCondition{
+						{Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"},
+					},
+					QueryParamMatchConditions: []QueryParamMatchCondition{
+						{Name: "param-2different", Value: "value-2different", MatchType: QueryParamMatchTypePrefix},
+					},
+				},
+			},
+			expectConflict: false,
+		},
+		"2 different routes with different path, with same kind, don't expect conflict": {
+			vHost: VirtualHost{
+				Routes: map[string]*Route{},
+			},
+			rs: []Route{
+				{
+					Kind:               KindHTTPRoute,
+					Name:               "a",
+					Namespace:          "b",
+					PathMatchCondition: prefixSegment("/path2"),
+					HeaderMatchConditions: []HeaderMatchCondition{
+						{Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"},
+					},
+					QueryParamMatchConditions: []QueryParamMatchCondition{
+						{Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact},
+					},
+				},
+				{
+					Kind:               KindHTTPRoute,
+					Name:               "c",
+					Namespace:          "d",
+					PathMatchCondition: prefixSegment("/path1"),
+					HeaderMatchConditions: []HeaderMatchCondition{
+						{Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"},
+					},
+					QueryParamMatchConditions: []QueryParamMatchCondition{
+						{Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact},
+					},
+				},
+			},
+			expectConflict: false,
+		},
+	}
+
+	for n, c := range cases {
+		t.Run(n, func(t *testing.T) {
+			c.vHost.AddRoute(&c.rs[0])
+
+			assert.Equal(t, c.vHost.HasConflictRoute(&c.rs[1]), c.expectConflict)
+		})
+	}
+}

internal/dag/gatewayapi_processor.go

@@ -17,6 +17,7 @@
 	"fmt"
 	"net/http"
 	"regexp"
+	"sort"
 	"strings"
 	"time"
 
@@ -153,8 +154,10 @@
 	// to each Listener so we can set status properly.
 	listenerAttachedRoutes := map[string]int{}
 
+	// sort httproutes based on age/name first
+	sortedHTTPRoutes := sortRoutes(p.source.httproutes)
 	// Process HTTPRoutes.
-	for _, httpRoute := range p.source.httproutes {
+	for _, httpRoute := range sortedHTTPRoutes {
 		p.processRoute(KindHTTPRoute, httpRoute, httpRoute.Spec.ParentRefs, gatewayNotProgrammedCondition, listenerInfos, listenerAttachedRoutes, &gatewayapi_v1.HTTPRoute{})
 	}
 
@@ -1162,6 +1165,8 @@
 	listener *listenerInfo,
 	hosts sets.Set[string],
 ) {
+	// Count number of rules under this Route that are invalid.
+	invalidRuleCnt := 0
 	for ruleIndex, rule := range route.Spec.Rules {
 		// Get match conditions for the rule.
 		var matchconditions []*matchConditions
@@ -1438,21 +1443,66 @@
 				timeoutPolicy)
 		}
 
-		// Add each route to the relevant vhost(s)/svhosts(s).
-		for host := range hosts {
-			for _, route := range routes {
-				switch {
-				case listener.tlsSecret != nil:
-					svhost := p.dag.EnsureSecureVirtualHost(listener.dagListenerName, host)
-					svhost.Secret = listener.tlsSecret
-					svhost.AddRoute(route)
-				default:
-					vhost := p.dag.EnsureVirtualHost(listener.dagListenerName, host)
-					vhost.AddRoute(route)
+		// check all the routes whether there is conflict against previous rules
+		if !p.hasConflictRoute(listener, hosts, routes) {
+			// add the route if there is conflict
+			// Add each route to the relevant vhost(s)/svhosts(s).
+			for host := range hosts {
+				for _, route := range routes {
+					switch {
+					case listener.tlsSecret != nil:
+						svhost := p.dag.EnsureSecureVirtualHost(listener.dagListenerName, host)
+						svhost.Secret = listener.tlsSecret
+						svhost.AddRoute(route)
+					default:
+						vhost := p.dag.EnsureVirtualHost(listener.dagListenerName, host)
+						vhost.AddRoute(route)
+					}
 				}
 			}
+		} else {
+			// Skip adding the routes under this rule.
+			invalidRuleCnt++
 		}
 	}
+
+	if invalidRuleCnt == len(route.Spec.Rules) {
+		// No rules under the route is valid, mark it as not accepted.
+		routeAccessor.AddCondition(
+			gatewayapi_v1.RouteConditionAccepted,
+			meta_v1.ConditionFalse,
+			status.ReasonRouteRuleMatchConflict,
+			status.MessageRouteRuleMatchConflict,
+		)
+	} else if invalidRuleCnt > 0 {
+		routeAccessor.AddCondition(
+			gatewayapi_v1.RouteConditionPartiallyInvalid,
+			meta_v1.ConditionTrue,
+			status.ReasonRouteRuleMatchPartiallyConflict,
+			status.MessageRouteRuleMatchPartiallyConflict,
+		)
+	}
+}
+
+func (p *GatewayAPIProcessor) hasConflictRoute(listener *listenerInfo, hosts sets.Set[string], routes []*Route) bool {
+	// check if there is conflict match first
+	for host := range hosts {
+		for _, route := range routes {
+			switch {
+			case listener.tlsSecret != nil:
+				svhost := p.dag.EnsureSecureVirtualHost(listener.dagListenerName, host)
+				if svhost.HasConflictRoute(route) {
+					return true
+				}
+			default:
+				vhost := p.dag.EnsureVirtualHost(listener.dagListenerName, host)
+				if vhost.HasConflictRoute(route) {
+					return true
+				}
+			}
+		}
+	}
+	return false
 }
 
 func (p *GatewayAPIProcessor) computeGRPCRouteForListener(route *gatewayapi_v1alpha2.GRPCRoute, routeAccessor *status.RouteParentStatusUpdate, listener *listenerInfo, hosts sets.Set[string]) bool {
@@ -2410,3 +2460,22 @@
 
 	return p
 }
+
+// sort routes based on creationTimestamp in 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 {
+		routes = append(routes, r)
+	}
+	sort.SliceStable(routes, func(i, j int) bool {
+		// if the creation time is the same, compare the route name
+		if routes[i].CreationTimestamp.Equal(&routes[j].CreationTimestamp) {
+			return k8s.NamespacedNameOf(routes[i]).String() <
+				k8s.NamespacedNameOf(routes[j]).String()
+		}
+		return routes[i].CreationTimestamp.Before(&routes[j].CreationTimestamp)
+	})
+
+	return routes
+}