diff --git a/internal/dag/dag.go b/internal/dag/dag.go index cabed060d88..ac996497099 100644 --- a/internal/dag/dag.go +++ b/internal/dag/dag.go @@ -24,7 +24,6 @@ import ( "strings" "time" - "golang.org/x/exp/maps" core_v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" @@ -772,55 +771,25 @@ type VirtualHost struct { RouteHeaderAndQueryParamMatchMap 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) } - // check if it's HTTPRoute type and there is existing RouteHeader + RouteQueryParams combination match this route candidate - if route.IsHTTPRoute() && v.hasMatchConflict(route) { - return - } + v.Routes[conditionsToString(route)] = route } -func (v *VirtualHost) hasMatchConflict(route *Route) bool { - hasConflict := false - if v.RouteHeaderAndQueryParamMatchMap == nil { - v.RouteHeaderAndQueryParamMatchMap = make(map[string]*Route) - } - // first clean up maps in case there are stale match that were removed from Route CR, but not from the map - for k, r := range v.RouteHeaderAndQueryParamMatchMap { - if r.SameRoute(route) { - delete(v.RouteHeaderAndQueryParamMatchMap, k) - } - } - // add headerMatchCondition and QueryParamMatchCondition combination to the map - tmpMap, hasConflict := v.getMapIfNoMatchConflicts(route) - // no conflict, then add the tmpMap content to map - if !hasConflict { - maps.Copy(v.RouteHeaderAndQueryParamMatchMap, tmpMap) +// HasConflictRoute returns true HTTPRoute type and there is existing Path + Headers +// + QueryParams combination match this route candidate. +func (v *VirtualHost) HasConflictHTTPRoute(route *Route) bool { + if !route.IsHTTPRoute() { return false } - - return true -} - -func (v *VirtualHost) getMapIfNoMatchConflicts(route *Route) (map[string]*Route, bool) { - key := "%s,%s,%s" - tmpMap := make(map[string]*Route) - for _, headerMath := range route.HeaderMatchConditions { - for _, queryParamMatch := range route.QueryParamMatchConditions { - newKey := fmt.Sprintf(key, route.PathMatchCondition.String(), headerMath.String(), queryParamMatch.String()) - if r, ok := v.RouteHeaderAndQueryParamMatchMap[newKey]; ok { - // if it's not the same route, then it's conflict - if !r.SameRoute(route) { - return nil, true - } - } - tmpMap[newKey] = route - } + if r, ok := v.Routes[conditionsToString(route)]; ok && !r.SameRoute(route) { + return true } - return tmpMap, false + return false } func conditionsToString(r *Route) string { diff --git a/internal/dag/dag_test.go b/internal/dag/dag_test.go index 577c07dca38..762a4447feb 100644 --- a/internal/dag/dag_test.go +++ b/internal/dag/dag_test.go @@ -266,13 +266,13 @@ func TestServiceClusterRebalance(t *testing.T) { } } -func TestAddRoute(t *testing.T) { +func TestHasConflictHTTPRoute(t *testing.T) { cases := map[string]struct { - vHost VirtualHost - rs []Route - expectCount int + vHost VirtualHost + rs []Route + expectConflict bool }{ - "3 different routes all get added": { + "2 different routes with same path and header, expect conflict": { vHost: VirtualHost{ Routes: map[string]*Route{}, }, @@ -288,26 +288,17 @@ func TestAddRoute(t *testing.T) { }, { Kind: KindHTTPRoute, - Name: "c", - Namespace: "b", - PathMatchCondition: prefixSegment("/path1"), - QueryParamMatchConditions: []QueryParamMatchCondition{ - {Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact}, - }, - }, - { - Kind: KindHTTPRoute, - Name: "f", - Namespace: "g", + Name: "b", + Namespace: "c", PathMatchCondition: prefixSegment("/path1"), - QueryParamMatchConditions: []QueryParamMatchCondition{ - {Name: "param-2", Value: "value-2", MatchType: QueryParamMatchTypeExact}, + HeaderMatchConditions: []HeaderMatchCondition{ + {Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"}, }, }, }, - expectCount: 3, + expectConflict: true, }, - "3 routes, 1 and 2 have conflict, 2 and 3 have conflict, but 3 doesn't have conflict with 1, 1 and 3 one gets added": { + "2 different routes with same path and header and query params, expect conflict": { vHost: VirtualHost{ Routes: map[string]*Route{}, }, @@ -327,37 +318,19 @@ func TestAddRoute(t *testing.T) { { Kind: KindHTTPRoute, Name: "c", - Namespace: "b", + 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]+)?"}, - {Name: "x-request-id", MatchType: "present"}, - {Name: "e-tag", Value: "abcdef", MatchType: "contains"}, - {Name: "x-timeout", Value: "infinity", MatchType: "contains", Invert: true}, }, QueryParamMatchConditions: []QueryParamMatchCondition{ {Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact}, - {Name: "param-2", Value: "value-2", MatchType: QueryParamMatchTypeExact}, - }, - }, - { - Kind: KindHTTPRoute, - Name: "f", - Namespace: "g", - PathMatchCondition: prefixSegment("/path1"), - HeaderMatchConditions: []HeaderMatchCondition{ - {Name: "x-request-id", MatchType: "present"}, - {Name: "e-tag", Value: "abcdef", MatchType: "contains"}, - {Name: "x-timeout", Value: "infinity", MatchType: "contains", Invert: true}, - }, - QueryParamMatchConditions: []QueryParamMatchCondition{ - {Name: "param-2", Value: "value-2", MatchType: QueryParamMatchTypeExact}, }, }, }, - expectCount: 2, + expectConflict: true, }, - "3 routes, 1 and 2 have conflict, 1 and 3 have conflict, but all has different paths, all get added": { + "2 different routes with different path, don't expect conflict": { vHost: VirtualHost{ Routes: map[string]*Route{}, }, @@ -366,20 +339,7 @@ func TestAddRoute(t *testing.T) { Kind: KindHTTPRoute, Name: "a", Namespace: "b", - PathMatchCondition: prefixSegment("/differentpath1"), - 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}, - {Name: "param-2", Value: "value-2", MatchType: QueryParamMatchTypeExact}, - }, - }, - { - Kind: KindHTTPRoute, - Name: "c", - Namespace: "b", - PathMatchCondition: prefixSegment("/differentpath2"), + PathMatchCondition: prefixSegment("/path2"), HeaderMatchConditions: []HeaderMatchCondition{ {Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"}, }, @@ -389,42 +349,9 @@ func TestAddRoute(t *testing.T) { }, { Kind: KindHTTPRoute, - Name: "f", - Namespace: "g", - PathMatchCondition: prefixSegment("/differentpath3"), - HeaderMatchConditions: []HeaderMatchCondition{ - {Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"}, - }, - QueryParamMatchConditions: []QueryParamMatchCondition{ - {Name: "param-2", Value: "value-2", MatchType: QueryParamMatchTypeExact}, - }, - }, - }, - expectCount: 3, - }, - "3 routes, 1 and 2 have conflict, 1 and 3 have conflict, but all are not httproute, all get added": { - vHost: VirtualHost{ - Routes: map[string]*Route{}, - }, - rs: []Route{ - { - Kind: "GRPCRoute", - Name: "a", - Namespace: "b", - PathMatchCondition: prefixSegment("/path"), - 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}, - {Name: "param-2", Value: "value-2", MatchType: QueryParamMatchTypeExact}, - }, - }, - { - Kind: "GRPCRoute", Name: "c", - Namespace: "b", - PathMatchCondition: prefixSegment("/path"), + 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]+)?"}, }, @@ -432,30 +359,16 @@ func TestAddRoute(t *testing.T) { {Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact}, }, }, - { - Kind: "GRPCRoute", - Name: "f", - Namespace: "g", - PathMatchCondition: prefixSegment("/path"), - HeaderMatchConditions: []HeaderMatchCondition{ - {Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"}, - }, - QueryParamMatchConditions: []QueryParamMatchCondition{ - {Name: "param-2", Value: "value-2", MatchType: QueryParamMatchTypeExact}, - }, - }, }, - expectCount: 3, + expectConflict: false, }, } for n, c := range cases { t.Run(n, func(t *testing.T) { - for i := range c.rs { - c.vHost.AddRoute(&c.rs[i]) - } + c.vHost.AddRoute(&c.rs[0]) - assert.Len(t, c.vHost.Routes, c.expectCount) + assert.Equal(t, c.vHost.HasConflictHTTPRoute(&c.rs[1]), c.expectConflict) }) } } diff --git a/internal/dag/gatewayapi_processor.go b/internal/dag/gatewayapi_processor.go index ef588e66c83..5b8d7bc6e25 100644 --- a/internal/dag/gatewayapi_processor.go +++ b/internal/dag/gatewayapi_processor.go @@ -1428,22 +1428,52 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener( pathRewritePolicy, timeoutPolicy) } + if p.hasConflictRoute(listener, hosts, routes) { + routeAccessor.AddCondition( + gatewayapi_v1.RouteConditionAccepted, + meta_v1.ConditionFalse, + status.ReasonRouteConflict, + "HTTPRoute's Match has conflict with other HTTPRoute's Match ", + ) + } else { + // 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) + } + } + } + } - // 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) + } +} + +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.HasConflictHTTPRoute(route) { + return true + } + default: + vhost := p.dag.EnsureVirtualHost(listener.dagListenerName, host) + if vhost.HasConflictHTTPRoute(route) { + return true } } } } + return false } func (p *GatewayAPIProcessor) computeGRPCRouteForListener(route *gatewayapi_v1alpha2.GRPCRoute, routeAccessor *status.RouteParentStatusUpdate, listener *listenerInfo, hosts sets.Set[string]) bool { diff --git a/internal/status/routeconditions.go b/internal/status/routeconditions.go index b90d212d9fb..8e454e93bdf 100644 --- a/internal/status/routeconditions.go +++ b/internal/status/routeconditions.go @@ -37,6 +37,7 @@ const ( ReasonInvalidPathMatch gatewayapi_v1.RouteConditionReason = "InvalidPathMatch" ReasonInvalidMethodMatch gatewayapi_v1.RouteConditionReason = "InvalidMethodMatch" ReasonInvalidGateway gatewayapi_v1.RouteConditionReason = "InvalidGateway" + ReasonRouteConflict gatewayapi_v1.RouteConditionReason = "RouteConflict" ) // RouteStatusUpdate represents an atomic update to a