diff --git a/internal/dag/dag_test.go b/internal/dag/dag_test.go index 3e161d26274..4aa7555f283 100644 --- 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 index 638ded0a939..bdbe3d5ab4e 100644 --- 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{}) } @@ -1507,19 +1505,9 @@ 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, - ) + addRoutePartiallyInvalidConditionDueToMatchPartiallyConflict(routeAccessor, KindHTTPRoute) } } @@ -1546,6 +1534,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 +1657,36 @@ 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 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) + } } - - 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 { + addRoutePartiallyInvalidConditionDueToMatchPartiallyConflict(routeAccessor, KindGRPCRoute) + } + return programmed } @@ -2499,9 +2500,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 ("/") 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 +2518,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 ("/") 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), + ) +} diff --git a/internal/dag/gatewayapi_processor_test.go b/internal/dag/gatewayapi_processor_test.go index 43d2c60beab..463229636a7 100644 --- a/internal/dag/gatewayapi_processor_test.go +++ b/internal/dag/gatewayapi_processor_test.go @@ -1133,7 +1133,303 @@ func TestSortRoutes(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - res := sortRoutes(tc.m) + + res := sortHTTPRoutes(tc.m) + assert.Equal(t, tc.expected, res) + }) + } +} + +func TestSortGRPCRoutes(t *testing.T) { + time1 := time.Date(2021, time.Month(2), 21, 1, 10, 30, 0, time.UTC) + time2 := time.Date(2022, time.Month(2), 21, 1, 10, 30, 0, time.UTC) + time3 := time.Date(2023, time.Month(2), 21, 1, 10, 30, 0, time.UTC) + tests := []struct { + name string + m map[types.NamespacedName]*gatewayapi_v1.GRPCRoute + expected []*gatewayapi_v1.GRPCRoute + }{ + { + name: "3 httproutes, with different timestamp, earlier one should be first ", + m: map[types.NamespacedName]*gatewayapi_v1.GRPCRoute{ + { + Namespace: "ns", Name: "name1", + }: { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns", + Name: "name3", + CreationTimestamp: meta_v1.NewTime(time3), + }, + }, + { + Namespace: "ns", Name: "name2", + }: { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns", + Name: "name2", + CreationTimestamp: meta_v1.NewTime(time2), + }, + }, + { + Namespace: "ns", Name: "name3", + }: { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns", + Name: "name1", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + }, + expected: []*gatewayapi_v1.GRPCRoute{ + { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns", + Name: "name1", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns", + Name: "name2", + CreationTimestamp: meta_v1.NewTime(time2), + }, + }, + { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns", + Name: "name3", + CreationTimestamp: meta_v1.NewTime(time3), + }, + }, + }, + }, + { + name: "3 httproutes with same creation timestamps, same namespaces, smaller name comes first", + m: map[types.NamespacedName]*gatewayapi_v1.GRPCRoute{ + { + Namespace: "ns", Name: "name3", + }: { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns", + Name: "name3", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + { + Namespace: "ns", Name: "name2", + }: { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns", + Name: "name2", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + { + Namespace: "ns", Name: "name1", + }: { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns", + Name: "name1", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + }, + expected: []*gatewayapi_v1.GRPCRoute{ + { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns", + Name: "name1", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns", + Name: "name2", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns", + Name: "name3", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + }, + }, + { + name: "3 httproutes with same creation timestamp, smaller namespaces comes first", + m: map[types.NamespacedName]*gatewayapi_v1.GRPCRoute{ + { + Namespace: "ns3", Name: "name1", + }: { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns3", + Name: "name3", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + { + Namespace: "ns2", Name: "name2", + }: { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns2", + Name: "name2", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + { + Namespace: "ns1", Name: "name3", + }: { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns1", + Name: "name3", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + }, + expected: []*gatewayapi_v1.GRPCRoute{ + { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns1", + Name: "name3", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns2", + Name: "name2", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns3", + Name: "name3", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + }, + }, + { + name: "mixed order, two with same creation timestamp, two with same name", + m: map[types.NamespacedName]*gatewayapi_v1.GRPCRoute{ + { + Namespace: "ns1", Name: "name2", + }: { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns1", + Name: "name2", + CreationTimestamp: meta_v1.NewTime(time2), + }, + }, + { + Namespace: "ns2", Name: "name2", + }: { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns2", + Name: "name2", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + { + Namespace: "ns1", Name: "name1", + }: { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns1", + Name: "name1", + CreationTimestamp: meta_v1.NewTime(time2), + }, + }, + }, + expected: []*gatewayapi_v1.GRPCRoute{ + { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns2", + Name: "name2", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns1", + Name: "name1", + CreationTimestamp: meta_v1.NewTime(time2), + }, + }, + { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns1", + Name: "name2", + CreationTimestamp: meta_v1.NewTime(time2), + }, + }, + }, + }, + { + name: "same name, same timestamp, different namespace", + m: map[types.NamespacedName]*gatewayapi_v1.GRPCRoute{ + { + Namespace: "ns3", Name: "name", + }: { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns3", + Name: "name", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + { + Namespace: "ns2", Name: "name", + }: { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns2", + Name: "name", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + { + Namespace: "ns1", Name: "name", + }: { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns1", + Name: "name", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + }, + expected: []*gatewayapi_v1.GRPCRoute{ + { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns1", + Name: "name", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns2", + Name: "name", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + { + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: "ns3", + Name: "name", + CreationTimestamp: meta_v1.NewTime(time1), + }, + }, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + + res := sortGRPCRoutes(tc.m) assert.Equal(t, tc.expected, res) }) } diff --git a/internal/dag/status_test.go b/internal/dag/status_test.go index bae3b9a983c..2c6caae88bb 100644 --- a/internal/dag/status_test.go +++ b/internal/dag/status_test.go @@ -5712,7 +5712,7 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { { ParentRef: gatewayapi.GatewayParentRef("projectcontour", "contour"), Conditions: []meta_v1.Condition{ - routeAcceptedFalse(status.ReasonRouteRuleMatchConflict, "HTTPRoute's Match has conflict with other HTTPRoute's Match"), + routeAcceptedFalse(status.ReasonRouteRuleMatchConflict, fmt.Sprintf(status.MessageRouteRuleMatchConflict, KindHTTPRoute, KindHTTPRoute)), routeResolvedRefsCondition(), }, }, @@ -5724,7 +5724,144 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { { ParentRef: gatewayapi.GatewayParentRef("projectcontour", "contour"), Conditions: []meta_v1.Condition{ - routeAcceptedFalse(status.ReasonRouteRuleMatchConflict, "HTTPRoute's Match has conflict with other HTTPRoute's Match"), + routeAcceptedFalse(status.ReasonRouteRuleMatchConflict, fmt.Sprintf(status.MessageRouteRuleMatchConflict, KindHTTPRoute, KindHTTPRoute)), + routeResolvedRefsCondition(), + }, + }, + }, + }, + }, + // is it ok to show the listeners are attached, just it's not accepted because of the conflict + wantGatewayStatusUpdate: validGatewayStatusUpdate("http", gatewayapi_v1.HTTPProtocolType, 3), + }) + + run(t, "3 grpcroutes, but duplicate match condition between these 3. The 2 rank lower gets Conflict condition ", testcase{ + objs: []any{ + kuardService, + // first GRPCRoute with oldest creationTimestamp + &gatewayapi_v1.GRPCRoute{ + TypeMeta: meta_v1.TypeMeta{ + Kind: KindGRPCRoute, + }, + ObjectMeta: meta_v1.ObjectMeta{ + Name: "basic-1", + Namespace: "default", + CreationTimestamp: meta_v1.Date(2020, time.Month(2), 21, 1, 10, 30, 0, time.UTC), + }, + Spec: gatewayapi_v1.GRPCRouteSpec{ + CommonRouteSpec: gatewayapi_v1.CommonRouteSpec{ + ParentRefs: []gatewayapi_v1.ParentReference{gatewayapi.GatewayParentRef("projectcontour", "contour")}, + }, + Hostnames: []gatewayapi_v1.Hostname{ + "test.projectcontour.io", + }, + Rules: []gatewayapi_v1.GRPCRouteRule{{ + Matches: []gatewayapi_v1.GRPCRouteMatch{ + { + Method: gatewayapi.GRPCMethodMatch(gatewayapi_v1.GRPCMethodMatchExact, "com.example.service", "Login"), + }, + { + Method: gatewayapi.GRPCMethodMatch(gatewayapi_v1.GRPCMethodMatchExact, "foo.com.example.service", "Login"), + }, + }, + BackendRefs: gatewayapi.GRPCRouteBackendRef("kuard", 8080, 1), + }}, + }, + }, + // second GRPCRoute with 2nd oldest creationTimestamp, conflict with 1st GRPCRoute + &gatewayapi_v1.GRPCRoute{ + TypeMeta: meta_v1.TypeMeta{ + Kind: KindGRPCRoute, + }, + ObjectMeta: meta_v1.ObjectMeta{ + Name: "basic-2", + Namespace: "default", + CreationTimestamp: meta_v1.Date(2021, time.Month(2), 21, 1, 10, 30, 0, time.UTC), + }, + Spec: gatewayapi_v1.GRPCRouteSpec{ + CommonRouteSpec: gatewayapi_v1.CommonRouteSpec{ + ParentRefs: []gatewayapi_v1.ParentReference{gatewayapi.GatewayParentRef("projectcontour", "contour")}, + }, + Hostnames: []gatewayapi_v1.Hostname{ + "test.projectcontour.io", + }, + Rules: []gatewayapi_v1.GRPCRouteRule{{ + Matches: []gatewayapi_v1.GRPCRouteMatch{ + { + Method: gatewayapi.GRPCMethodMatch(gatewayapi_v1.GRPCMethodMatchExact, "com.example.service", "Login"), + }, + { + Method: gatewayapi.GRPCMethodMatch(gatewayapi_v1.GRPCMethodMatchExact, "foo.com.example.service", "Login"), + }, + }, + BackendRefs: gatewayapi.GRPCRouteBackendRef("kuard", 8080, 1), + }}, + }, + }, + // 3rd GRPCRoute with newest creationTimestamp, conflict with 1st GRPCRoute + &gatewayapi_v1.GRPCRoute{ + TypeMeta: meta_v1.TypeMeta{ + Kind: KindGRPCRoute, + }, + ObjectMeta: meta_v1.ObjectMeta{ + Name: "basic-3", + Namespace: "default", + CreationTimestamp: meta_v1.Date(2022, time.Month(2), 21, 1, 10, 30, 0, time.UTC), + }, + Spec: gatewayapi_v1.GRPCRouteSpec{ + CommonRouteSpec: gatewayapi_v1.CommonRouteSpec{ + ParentRefs: []gatewayapi_v1.ParentReference{gatewayapi.GatewayParentRef("projectcontour", "contour")}, + }, + Hostnames: []gatewayapi_v1.Hostname{ + "test.projectcontour.io", + }, + Rules: []gatewayapi_v1.GRPCRouteRule{{ + Matches: []gatewayapi_v1.GRPCRouteMatch{ + { + Method: gatewayapi.GRPCMethodMatch(gatewayapi_v1.GRPCMethodMatchExact, "com.example.service", "Login"), + }, + { + Method: gatewayapi.GRPCMethodMatch(gatewayapi_v1.GRPCMethodMatchExact, "foo.com.example.service", "Login"), + }, + }, + BackendRefs: gatewayapi.GRPCRouteBackendRef("kuard", 8080, 1), + }}, + }, + }, + }, + + wantRouteConditions: []*status.RouteStatusUpdate{ + { + FullName: types.NamespacedName{Namespace: "default", Name: "basic-1"}, + RouteParentStatuses: []*gatewayapi_v1.RouteParentStatus{ + { + ParentRef: gatewayapi.GatewayParentRef("projectcontour", "contour"), + Conditions: []meta_v1.Condition{ + routeResolvedRefsCondition(), + routeAcceptedGRPCRouteCondition(), + }, + }, + }, + }, + { + FullName: types.NamespacedName{Namespace: "default", Name: "basic-2"}, + RouteParentStatuses: []*gatewayapi_v1.RouteParentStatus{ + { + ParentRef: gatewayapi.GatewayParentRef("projectcontour", "contour"), + Conditions: []meta_v1.Condition{ + routeAcceptedFalse(status.ReasonRouteRuleMatchConflict, fmt.Sprintf(status.MessageRouteRuleMatchConflict, KindGRPCRoute, KindGRPCRoute)), + routeResolvedRefsCondition(), + }, + }, + }, + }, + { + FullName: types.NamespacedName{Namespace: "default", Name: "basic-3"}, + RouteParentStatuses: []*gatewayapi_v1.RouteParentStatus{ + { + ParentRef: gatewayapi.GatewayParentRef("projectcontour", "contour"), + Conditions: []meta_v1.Condition{ + routeAcceptedFalse(status.ReasonRouteRuleMatchConflict, fmt.Sprintf(status.MessageRouteRuleMatchConflict, KindGRPCRoute, KindGRPCRoute)), routeResolvedRefsCondition(), }, }, diff --git a/internal/status/routeconditions.go b/internal/status/routeconditions.go index a3b9fbc3040..ed26405ca67 100644 --- a/internal/status/routeconditions.go +++ b/internal/status/routeconditions.go @@ -40,8 +40,8 @@ const ( ReasonRouteRuleMatchConflict gatewayapi_v1.RouteConditionReason = "RuleMatchConflict" ReasonRouteRuleMatchPartiallyConflict gatewayapi_v1.RouteConditionReason = "RuleMatchPartiallyConflict" - MessageRouteRuleMatchConflict string = "HTTPRoute's Match has conflict with other HTTPRoute's Match" - MessageRouteRuleMatchPartiallyConflict string = "Dropped Rule: HTTPRoute's rule(s) has(ve) been dropped because of conflict against other HTTPRoute's rule(s)" + MessageRouteRuleMatchConflict string = "%s's Match has conflict with other %s's Match" + MessageRouteRuleMatchPartiallyConflict string = "Dropped Rule: %s's rule(s) has(ve) been dropped because of conflict against other %s's rule(s)" ) // RouteStatusUpdate represents an atomic update to a diff --git a/test/e2e/framework.go b/test/e2e/framework.go index d604cb84086..b41bf59b02b 100644 --- a/test/e2e/framework.go +++ b/test/e2e/framework.go @@ -395,6 +395,12 @@ func (f *Framework) CreateBackendTLSPolicyAndWaitFor(route *gatewayapi_v1alpha3. return createAndWaitFor(f.t, f.Client, route, condition, f.RetryInterval, f.RetryTimeout) } +// CreateGRPCRouteAndWaitFor creates the provided GRPCRoute in the Kubernetes API +// and then waits for the specified condition to be true. +func (f *Framework) CreateGRPCRouteAndWaitFor(route *gatewayapi_v1.GRPCRoute, condition func(*gatewayapi_v1.GRPCRoute) bool) bool { + return createAndWaitFor(f.t, f.Client, route, condition, f.RetryInterval, f.RetryTimeout) +} + // CreateNamespace creates a namespace with the given name in the // Kubernetes API or fails the test if it encounters an error. func (f *Framework) CreateNamespace(name string) { diff --git a/test/e2e/gateway/gateway_test.go b/test/e2e/gateway/gateway_test.go index 3e32aad3628..f5cfe470fbd 100644 --- a/test/e2e/gateway/gateway_test.go +++ b/test/e2e/gateway/gateway_test.go @@ -185,6 +185,10 @@ var _ = Describe("Gateway API", func() { f.NamespacedTest("gateway-httproute-conflict-match", testWithHTTPGateway(testHTTPRouteConflictMatch)) f.NamespacedTest("gateway-httproute-partially-conflict-match", testWithHTTPGateway(testHTTPRoutePartiallyConflictMatch)) + + f.NamespacedTest("gateway-grpcroute-conflict-match", testWithHTTPGateway(testGRPCRouteConflictMatch)) + + f.NamespacedTest("gateway-grpcroute-partially-conflict-match", testWithHTTPGateway(testGRPCRoutePartiallyConflictMatch)) }) Describe("Gateway with one HTTP listener and one HTTPS listener", func() { diff --git a/test/e2e/gateway/grpc_route_conflict_match_test.go b/test/e2e/gateway/grpc_route_conflict_match_test.go new file mode 100644 index 00000000000..00bdbf29ef2 --- /dev/null +++ b/test/e2e/gateway/grpc_route_conflict_match_test.go @@ -0,0 +1,95 @@ +// Copyright Project Contour Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build e2e + +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" + + "github.com/projectcontour/contour/internal/gatewayapi" + "github.com/projectcontour/contour/test/e2e" +) + +func testGRPCRouteConflictMatch(namespace string, gateway types.NamespacedName) { + Specify("Creates two GRPCRoutes, second one has conflict match against the first one, report Accepted: false", func() { + cleanup := f.Fixtures.GRPC.Deploy(namespace, "grpc-echo") + + By("create grpcroute-1 first") + route1 := &gatewayapi_v1.GRPCRoute{ + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: namespace, + Name: "grpcroute-1", + }, + Spec: gatewayapi_v1.GRPCRouteSpec{ + Hostnames: []gatewayapi_v1.Hostname{"queryparams.gateway.projectcontour.io"}, + CommonRouteSpec: gatewayapi_v1.CommonRouteSpec{ + ParentRefs: []gatewayapi_v1.ParentReference{ + gatewayapi.GatewayParentRef(gateway.Namespace, gateway.Name), + }, + }, + Rules: []gatewayapi_v1.GRPCRouteRule{{ + Matches: []gatewayapi_v1.GRPCRouteMatch{ + { + Method: gatewayapi.GRPCMethodMatch(gatewayapi_v1.GRPCMethodMatchExact, "com.example.service", "Login"), + }, + { + Method: gatewayapi.GRPCMethodMatch(gatewayapi_v1.GRPCMethodMatchExact, "foo.com.example.service", "Login"), + }, + }, + BackendRefs: gatewayapi.GRPCRouteBackendRef("grpc-echo", 9000, 1), + }}, + }, + } + _, ok := f.CreateGRPCRouteAndWaitFor(route1, e2e.GRPCRouteAccepted) + require.True(f.T(), ok) + + By("create grpcroute-2 with conflicted matches") + route2 := &gatewayapi_v1.GRPCRoute{ + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: namespace, + Name: "grpcroute-2", + }, + Spec: gatewayapi_v1.GRPCRouteSpec{ + Hostnames: []gatewayapi_v1.Hostname{"queryparams.gateway.projectcontour.io"}, + CommonRouteSpec: gatewayapi_v1.CommonRouteSpec{ + ParentRefs: []gatewayapi_v1.ParentReference{ + gatewayapi.GatewayParentRef(gateway.Namespace, gateway.Name), + }, + }, + Rules: []gatewayapi_v1.GRPCRouteRule{ + { + Matches: []gatewayapi_v1.GRPCRouteMatch{ + { + Method: gatewayapi.GRPCMethodMatch(gatewayapi_v1.GRPCMethodMatchExact, "com.example.service", "Login"), + }, + { + Method: gatewayapi.GRPCMethodMatch(gatewayapi_v1.GRPCMethodMatchExact, "bar.com.example.service", "Login"), + }, + }, + BackendRefs: gatewayapi.GRPCRouteBackendRef("grpc-echo", 9000, 1), + }, + }, + }, + } + _, ok = f.CreateGRPCRouteAndWaitFor(route2, e2e.GRPCRouteNotAcceptedDueToConflict) + require.True(f.T(), ok) + + cleanup() + }) +} diff --git a/test/e2e/gateway/grpc_route_partially_conflict_match_test.go b/test/e2e/gateway/grpc_route_partially_conflict_match_test.go new file mode 100644 index 00000000000..1026be40db6 --- /dev/null +++ b/test/e2e/gateway/grpc_route_partially_conflict_match_test.go @@ -0,0 +1,95 @@ +// Copyright Project Contour Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build e2e + +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" + + "github.com/projectcontour/contour/internal/gatewayapi" + "github.com/projectcontour/contour/test/e2e" +) + +func testGRPCRoutePartiallyConflictMatch(namespace string, gateway types.NamespacedName) { + Specify("Creates two GRPCRoutes, second one has partial conflict match against the first one, has partially match condition", func() { + cleanup := f.Fixtures.GRPC.Deploy(namespace, "grpc-echo") + + By("create grpcroute-1 first") + route1 := &gatewayapi_v1.GRPCRoute{ + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: namespace, + Name: "grpcroute-1", + }, + Spec: gatewayapi_v1.GRPCRouteSpec{ + Hostnames: []gatewayapi_v1.Hostname{"queryparams.gateway.projectcontour.io"}, + CommonRouteSpec: gatewayapi_v1.CommonRouteSpec{ + ParentRefs: []gatewayapi_v1.ParentReference{ + gatewayapi.GatewayParentRef(gateway.Namespace, gateway.Name), + }, + }, + Rules: []gatewayapi_v1.GRPCRouteRule{{ + Matches: []gatewayapi_v1.GRPCRouteMatch{ + { + Method: gatewayapi.GRPCMethodMatch(gatewayapi_v1.GRPCMethodMatchExact, "com.example.service", "Login"), + }, + { + Method: gatewayapi.GRPCMethodMatch(gatewayapi_v1.GRPCMethodMatchExact, "foo.com.example.service", "Login"), + }, + }, + BackendRefs: gatewayapi.GRPCRouteBackendRef("grpc-cho", 9000, 1), + }}, + }, + } + _, ok := f.CreateGRPCRouteAndWaitFor(route1, e2e.GRPCRouteAccepted) + require.True(f.T(), ok) + + By("create grpcroute-2 with only partially conflicted matches") + route2 := &gatewayapi_v1.GRPCRoute{ + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: namespace, + Name: "grpcroute-2", + }, + Spec: gatewayapi_v1.GRPCRouteSpec{ + Hostnames: []gatewayapi_v1.Hostname{"queryparams.gateway.projectcontour.io"}, + CommonRouteSpec: gatewayapi_v1.CommonRouteSpec{ + ParentRefs: []gatewayapi_v1.ParentReference{ + gatewayapi.GatewayParentRef(gateway.Namespace, gateway.Name), + }, + }, + Rules: []gatewayapi_v1.GRPCRouteRule{{ + Matches: []gatewayapi_v1.GRPCRouteMatch{ + { + Method: gatewayapi.GRPCMethodMatch(gatewayapi_v1.GRPCMethodMatchExact, "com.example.service", "Login"), + }, + { + Method: gatewayapi.GRPCMethodMatch(gatewayapi_v1.GRPCMethodMatchExact, "foo.com.example.service", "Login"), + }, + }, + BackendRefs: gatewayapi.GRPCRouteBackendRef("grpc-cho", 9000, 1), + }}, + }, + } + // Partially accepted + f.CreateGRPCRouteAndWaitFor(route2, func(*gatewayapi_v1.GRPCRoute) bool { + return e2e.GRPCRoutePartiallyInvalid(route2) && e2e.GRPCRouteAccepted(route2) + }) + + cleanup() + }) +} diff --git a/test/e2e/gatewayapi_predicates.go b/test/e2e/gatewayapi_predicates.go index 63cbe941013..64bdf448fbc 100644 --- a/test/e2e/gatewayapi_predicates.go +++ b/test/e2e/gatewayapi_predicates.go @@ -16,11 +16,14 @@ package e2e import ( + "fmt" + meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" gatewayapi_v1 "sigs.k8s.io/gateway-api/apis/v1" gatewayapi_v1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" gatewayapi_v1alpha3 "sigs.k8s.io/gateway-api/apis/v1alpha3" + "github.com/projectcontour/contour/internal/dag" "github.com/projectcontour/contour/internal/status" ) @@ -131,7 +134,7 @@ 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) { + if conditionExistsWithAllKeys(gw.Conditions, string(gatewayapi_v1.RouteConditionAccepted), meta_v1.ConditionFalse, string(status.ReasonRouteRuleMatchConflict), fmt.Sprintf(status.MessageRouteRuleMatchConflict, dag.KindHTTPRoute, dag.KindHTTPRoute)) { return true } } @@ -148,7 +151,7 @@ func HTTPRoutePartiallyInvalid(route *gatewayapi_v1.HTTPRoute) bool { } for _, gw := range route.Status.Parents { - if conditionExistsWithAllKeys(gw.Conditions, string(gatewayapi_v1.RouteConditionPartiallyInvalid), meta_v1.ConditionTrue, string(status.ReasonRouteRuleMatchPartiallyConflict), status.MessageRouteRuleMatchPartiallyConflict) { + if conditionExistsWithAllKeys(gw.Conditions, string(gatewayapi_v1.RouteConditionPartiallyInvalid), meta_v1.ConditionTrue, string(status.ReasonRouteRuleMatchPartiallyConflict), fmt.Sprintf(status.MessageRouteRuleMatchPartiallyConflict, dag.KindHTTPRoute, dag.KindHTTPRoute)) { return true } } @@ -206,6 +209,56 @@ func TLSRouteAccepted(route *gatewayapi_v1alpha2.TLSRoute) bool { return false } +// GRPCRouteAccepted returns true if the route has a .status.conditions +// entry of "Accepted: true". +func GRPCRouteAccepted(route *gatewayapi_v1.GRPCRoute) bool { + if route == nil { + return false + } + + for _, gw := range route.Status.Parents { + if conditionExists(gw.Conditions, string(gatewayapi_v1.RouteConditionAccepted), meta_v1.ConditionTrue) { + return true + } + } + + return false +} + +// GRPCRouteNotAcceptedDueToConflict returns true if the route has a .status.conditions +// entry of "Accepted: false" && "Reason: RouteMatchConflict" && "Message: GRPCRoute's Match has +// conflict with other GRPCRoute's Match". +func GRPCRouteNotAcceptedDueToConflict(route *gatewayapi_v1.GRPCRoute) bool { + if route == nil { + return false + } + + for _, gw := range route.Status.Parents { + if conditionExistsWithAllKeys(gw.Conditions, string(gatewayapi_v1.RouteConditionAccepted), meta_v1.ConditionFalse, string(status.ReasonRouteRuleMatchConflict), fmt.Sprintf(status.MessageRouteRuleMatchConflict, dag.KindGRPCRoute, dag.KindGRPCRoute)) { + return true + } + } + + return false +} + +// GRPCRoutePartiallyInvalid returns true if the route has a .status.conditions +// entry of "PartiallyInvalid: true" && "Reason: RuleMatchPartiallyConflict" && "Message: +// GRPCRoute's Match has partial conflict with other GRPCRoute's Match". +func GRPCRoutePartiallyInvalid(route *gatewayapi_v1.GRPCRoute) 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), fmt.Sprintf(status.MessageRouteRuleMatchPartiallyConflict, dag.KindGRPCRoute, dag.KindGRPCRoute)) { + return true + } + } + + return false +} + // BackendTLSPolicyAccepted returns true if the backend TLS policy has a .status.conditions // entry of "Accepted: true". func BackendTLSPolicyAccepted(btp *gatewayapi_v1alpha3.BackendTLSPolicy) bool {