Skip to content

Commit

Permalink
Update logic to treat each route object as a single match object to c…
Browse files Browse the repository at this point in the history 
…ompare with other match object

Signed-off-by: lubronzhan <lubronzhan@gmail.com>
  • Loading branch information
@lubronzhan
lubronzhan committed Feb 14, 2024
1 parent 57793b1 commit 55309ee
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 158 deletions.
49 changes: 9 additions & 40 deletions internal/dag/dag.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"strings"
"time"

"golang.org/x/exp/maps"
core_v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/types"

Expand Down Expand Up @@ -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 {
Expand Down
127 changes: 20 additions & 107 deletions internal/dag/dag_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{},
},
Expand All @@ -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{},
},
Expand All @@ -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{},
},
Expand All @@ -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]+)?"},
},
Expand All @@ -389,73 +349,26 @@ 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]+)?"},
},
QueryParamMatchConditions: []QueryParamMatchCondition{
{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)
})
}
}
52 changes: 41 additions & 11 deletions internal/dag/gatewayapi_processor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 ",
)

Check warning on line 1437 in internal/dag/gatewayapi_processor.go

View check run for this annotation

Codecov / codecov/patch

internal/dag/gatewayapi_processor.go#L1432-L1437 

Added lines #L1432 - L1437 were not covered by tests
} 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

Check warning on line 1466 in internal/dag/gatewayapi_processor.go

View check run for this annotation

Codecov / codecov/patch

internal/dag/gatewayapi_processor.go#L1466 

Added line #L1466 was not covered by tests
}
default:
vhost := p.dag.EnsureVirtualHost(listener.dagListenerName, host)
if vhost.HasConflictHTTPRoute(route) {
return true

Check warning on line 1471 in internal/dag/gatewayapi_processor.go

View check run for this annotation

Codecov / codecov/patch

internal/dag/gatewayapi_processor.go#L1471 

Added line #L1471 was not covered by tests
}
}
}
}
return false
}

func (p *GatewayAPIProcessor) computeGRPCRouteForListener(route *gatewayapi_v1alpha2.GRPCRoute, routeAccessor *status.RouteParentStatusUpdate, listener *listenerInfo, hosts sets.Set[string]) bool {
Expand Down
1 change: 1 addition & 0 deletions internal/status/routeconditions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 55309ee

Please sign in to comment.