Gateway API: handle Route conflicts with GRPCRoute.Matches (projectco…

…ntour#6566) Signed-off-by: Lubron Zhan <lubronzhan@gmail.com> Signed-off-by: Geoff Macartney <geoff.macartney@sky.uk>
chaosbox · Aug 22, 2024 · a01e8a7 · a01e8a7
1 parent 07051d8
commit a01e8a7
Show file tree

Hide file tree

Showing 12 changed files with 905 additions and 66 deletions.
diff --git a/changelogs/CHANGELOG-v1.29.0.md b/changelogs/CHANGELOG-v1.29.0.md
@@ -67,7 +67,7 @@ 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, will be marked with below conditions accordionly
+With above ordering, any HTTPRoute that ranks lower, will be marked with below conditions accordingly
 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`.
 

diff --git a/changelogs/unreleased/6566-lubronzhan-minor.md b/changelogs/unreleased/6566-lubronzhan-minor.md
@@ -0,0 +1,10 @@
+## Gateway API: handle Route conflicts with GRPCRoute.Matches
+
+It's possible that multiple GRPCRoutes 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 GRPCRoute that ranks lower, will be marked with below conditions accordingly:
+1. If only partial rules under this GRPCRoute are conflicted, it's marked with `Accepted: True` and `PartiallyInvalid: true` Conditions and Reason: `RuleMatchPartiallyConflict`.
+2. If all the rules under this GRPCRoute are conflicted, it's marked with `Accepted: False` Condition and Reason `RuleMatchConflict`.
diff --git a/internal/dag/dag_test.go b/internal/dag/dag_test.go
@@ -298,7 +298,7 @@ func TestHasConflictRouteForVirtualHost(t *testing.T) {
 			},
 			expectConflict: true,
 		},
-		"2 different routes with same path and header and query params, with same kind, expect conflict": {
+		"2 different httproutes with same path and header and query params, with same kind, expect conflict": {
 			vHost: VirtualHost{
 				Routes: map[string]*Route{},
 			},
@@ -330,6 +330,38 @@ func TestHasConflictRouteForVirtualHost(t *testing.T) {
 			},
 			expectConflict: true,
 		},
+		"2 different grpcroutes with same path and header and query params, with same kind, expect 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]+)?"},
+					},
+					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: true,
+		},
 		"2 different routes with same path and header, but different kind, expect no conflict": {
 			vHost: VirtualHost{
 				Routes: map[string]*Route{},

diff --git a/internal/dag/gatewayapi_processor.go b/internal/dag/gatewayapi_processor.go
@@ -154,10 +154,8 @@ func (p *GatewayAPIProcessor) Run(dag *DAG, source *KubernetesCache) {
 	// 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 sortedHTTPRoutes {
+	// Process sorted HTTPRoutes.
+	for _, httpRoute := range sortHTTPRoutes(p.source.httproutes) {
 		p.processRoute(KindHTTPRoute, httpRoute, httpRoute.Spec.ParentRefs, gatewayNotProgrammedCondition, listenerInfos, listenerAttachedRoutes, &gatewayapi_v1.HTTPRoute{})
 	}
 
@@ -166,8 +164,8 @@ func (p *GatewayAPIProcessor) Run(dag *DAG, source *KubernetesCache) {
 		p.processRoute(KindTLSRoute, tlsRoute, tlsRoute.Spec.ParentRefs, gatewayNotProgrammedCondition, listenerInfos, listenerAttachedRoutes, &gatewayapi_v1alpha2.TLSRoute{})
 	}
 
-	// Process GRPCRoutes.
-	for _, grpcRoute := range p.source.grpcroutes {
+	// Process sorted GRPCRoutes.
+	for _, grpcRoute := range sortGRPCRoutes(p.source.grpcroutes) {
 		p.processRoute(KindGRPCRoute, grpcRoute, grpcRoute.Spec.ParentRefs, gatewayNotProgrammedCondition, listenerInfos, listenerAttachedRoutes, &gatewayapi_v1.GRPCRoute{})
 	}
 
@@ -1482,9 +1480,9 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener(
 				timeoutPolicy)
 		}
 
-		// check all the routes whether there is conflict against previous rules
+		// 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 the route if there is no conflict at the same rule level.
 			// Add each route to the relevant vhost(s)/svhosts(s).
 			for host := range hosts {
 				for _, route := range routes {
@@ -1507,19 +1505,10 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener(
 
 	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,
-		)
+		addRouteNotAcceptedConditionDueToMatchConflict(routeAccessor, KindHTTPRoute)
 	} else if invalidRuleCnt > 0 {
-		routeAccessor.AddCondition(
-			gatewayapi_v1.RouteConditionPartiallyInvalid,
-			meta_v1.ConditionTrue,
-			status.ReasonRouteRuleMatchPartiallyConflict,
-			status.MessageRouteRuleMatchPartiallyConflict,
-		)
+		// Some of the rules are conflicted, mark it as partially invalid.
+		addRoutePartiallyInvalidConditionDueToMatchPartiallyConflict(routeAccessor, KindHTTPRoute)
 	}
 }
 
@@ -1546,6 +1535,7 @@ func (p *GatewayAPIProcessor) hasConflictRoute(listener *listenerInfo, hosts set
 
 func (p *GatewayAPIProcessor) computeGRPCRouteForListener(route *gatewayapi_v1.GRPCRoute, routeAccessor *status.RouteParentStatusUpdate, listener *listenerInfo, hosts sets.Set[string]) bool {
 	var programmed bool
+	invalidRuleCnt := 0
 	for ruleIndex, rule := range route.Spec.Rules {
 		// Get match conditions for the rule.
 		var matchconditions []*matchConditions
@@ -1668,24 +1658,37 @@ func (p *GatewayAPIProcessor) computeGRPCRouteForListener(route *gatewayapi_v1.G
 			nil,
 		)
 
-		// 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 no conflict at the same rule level.
+			// 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)
+					}
 				}
-
-				programmed = true
 			}
+		} 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.
+		addRouteNotAcceptedConditionDueToMatchConflict(routeAccessor, KindGRPCRoute)
+	} else if invalidRuleCnt > 0 {
+		// Some of the rules are conflicted, mark it as partially invalid.
+		addRoutePartiallyInvalidConditionDueToMatchPartiallyConflict(routeAccessor, KindGRPCRoute)
+	}
+
 	return programmed
 }
 
@@ -2499,9 +2502,9 @@ func handlePathRewritePrefixRemoval(p *PathRewritePolicy, mc *matchConditions) *
 	return p
 }
 
-// sort routes based on creationTimestamp in ascending order
+// sortHTTPRoutes sorts httproutes 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 {
+func sortHTTPRoutes(m map[types.NamespacedName]*gatewayapi_v1.HTTPRoute) []*gatewayapi_v1.HTTPRoute {
 	routes := []*gatewayapi_v1.HTTPRoute{}
 	for _, r := range m {
 		routes = append(routes, r)
@@ -2517,3 +2520,40 @@ func sortRoutes(m map[types.NamespacedName]*gatewayapi_v1.HTTPRoute) []*gatewaya
 
 	return routes
 }
+
+// sortGRPCRoutes sorts grpcroutes based on creationTimestamp in ascending order
+// if creationTimestamps are the same, sort based on namespaced name ("<namespace>/<name>") in alphetical ascending order
+func sortGRPCRoutes(m map[types.NamespacedName]*gatewayapi_v1.GRPCRoute) []*gatewayapi_v1.GRPCRoute {
+	routes := []*gatewayapi_v1.GRPCRoute{}
+	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
+}
+
+func addRouteNotAcceptedConditionDueToMatchConflict(routeAccessor *status.RouteParentStatusUpdate, routeKind string) {
+	routeAccessor.AddCondition(
+		gatewayapi_v1.RouteConditionAccepted,
+		meta_v1.ConditionFalse,
+		status.ReasonRouteRuleMatchConflict,
+		fmt.Sprintf(status.MessageRouteRuleMatchConflict, routeKind, routeKind),
+	)
+}
+
+func addRoutePartiallyInvalidConditionDueToMatchPartiallyConflict(routeAccessor *status.RouteParentStatusUpdate, routeKind string) {
+	routeAccessor.AddCondition(
+		gatewayapi_v1.RouteConditionPartiallyInvalid,
+		meta_v1.ConditionTrue,
+		status.ReasonRouteRuleMatchPartiallyConflict,
+		fmt.Sprintf(status.MessageRouteRuleMatchPartiallyConflict, routeKind, routeKind),
+	)
+}