From b52642d02377e381af53fb45569eeab2ee193916 Mon Sep 17 00:00:00 2001 From: lubronzhan Date: Thu, 8 Feb 2024 00:24:27 -0800 Subject: [PATCH 01/15] Add tests for routes, add two map to help find duplicate route Add logic to filter Route before adding it to vhost Signed-off-by: lubronzhan --- apis/projectcontour/v1/httpproxy.go | 4 + internal/dag/dag.go | 88 ++++++++ internal/dag/dag_test.go | 179 ++++++++++++++++ internal/dag/gatewayapi_processor.go | 26 ++- internal/dag/gatewayapi_processor_test.go | 241 ++++++++++++++++++++++ 5 files changed, 537 insertions(+), 1 deletion(-) diff --git a/apis/projectcontour/v1/httpproxy.go b/apis/projectcontour/v1/httpproxy.go index 976795d6903..520702c0781 100644 --- a/apis/projectcontour/v1/httpproxy.go +++ b/apis/projectcontour/v1/httpproxy.go @@ -1517,3 +1517,7 @@ type SlowStartPolicy struct { // +kubebuilder:validation:Enum=grpcroutes;tlsroutes;extensionservices;backendtlspolicies type Feature string + +const ( + KindHTTPProxy = "HTTPProxy" +) diff --git a/internal/dag/dag.go b/internal/dag/dag.go index 85fb54a6a34..e2eed934e8b 100644 --- a/internal/dag/dag.go +++ b/internal/dag/dag.go @@ -27,6 +27,7 @@ import ( core_v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" + contour_v1 "github.com/projectcontour/contour/apis/projectcontour/v1" "github.com/projectcontour/contour/internal/status" "github.com/projectcontour/contour/internal/timeout" ) @@ -394,6 +395,23 @@ func (r *Route) HasPathRegex() bool { return ok } +const KindHTTPProxy = "HTTPProxy" + +// SameRoute returns whether the route is the same as passed in route +func (r *Route) SameRoute(route *Route) bool { + if r == nil && route == nil { + return true + } + if r == nil || route == nil { + return false + } + return r.Name == route.Name && r.Namespace == route.Namespace +} + +func (r *Route) IsHTTPProxy() bool { + return r.Kind == contour_v1.KindHTTPProxy +} + // RouteTimeoutPolicy defines the timeout policy for a route. type RouteTimeoutPolicy struct { // ResponseTimeout is the timeout applied to the response @@ -749,15 +767,85 @@ type VirtualHost struct { IPFilterRules []IPFilterRule Routes map[string]*Route + + // key is path + headerKey + RouteHeaderMatchMap map[string]*Route + + // key is path + queryParamsKey + RouteQueryParamMatchMap map[string]*Route } func (v *VirtualHost) AddRoute(route *Route) { if v.Routes == nil { v.Routes = make(map[string]*Route) } + // check if it's HTTPProxy type and there is existing RouteHeader or RouteQueryParams match this route candidate + if route.IsHTTPProxy() && v.hasMatchConflict(route) { + return + } v.Routes[conditionsToString(route)] = route } +func (v *VirtualHost) hasMatchConflict(route *Route) bool { + if v.hasHeaderMatchConflict(route) || v.hasQueryParamMatchConflict(route) { + return true + } + // 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.RouteHeaderMatchMap { + if r.SameRoute(route) { + delete(v.RouteHeaderMatchMap, k) + } + } + for k, r := range v.RouteQueryParamMatchMap { + if r.SameRoute(route) { + delete(v.RouteQueryParamMatchMap, k) + } + } + // add headerMatchCondition and QueryParamMatchCondition to maps + for _, hmc := range route.HeaderMatchConditions { + v.RouteHeaderMatchMap[route.PathMatchCondition.String()+","+hmc.String()] = route + } + for _, rqmc := range route.QueryParamMatchConditions { + v.RouteQueryParamMatchMap[route.PathMatchCondition.String()+","+rqmc.String()] = route + } + + return false +} + +// check if there is already a route with same header match exist +func (v *VirtualHost) hasHeaderMatchConflict(route *Route) bool { + if v.RouteHeaderMatchMap == nil { + v.RouteHeaderMatchMap = make(map[string]*Route) + return false + } + for _, hmc := range route.HeaderMatchConditions { + if r, ok := v.RouteHeaderMatchMap[route.PathMatchCondition.String()+","+hmc.String()]; ok { + // if it's not the same route, then it's conflict + if r.SameRoute(route) { + return true + } + } + } + return false +} + +// check if there is already a route with same header match exist +func (v *VirtualHost) hasQueryParamMatchConflict(route *Route) bool { + if v.RouteQueryParamMatchMap == nil { + v.RouteQueryParamMatchMap = make(map[string]*Route) + return false + } + for _, qpmc := range route.QueryParamMatchConditions { + if r, ok := v.RouteQueryParamMatchMap[route.PathMatchCondition.String()+","+qpmc.String()]; ok { + // if it's not the same route, then it's conflict + if r.SameRoute(route) { + return true + } + } + } + return false +} + func conditionsToString(r *Route) string { s := []string{r.PathMatchCondition.String()} for _, cond := range r.HeaderMatchConditions { diff --git a/internal/dag/dag_test.go b/internal/dag/dag_test.go index 3849ce71e50..b4b8f7a9fd4 100644 --- a/internal/dag/dag_test.go +++ b/internal/dag/dag_test.go @@ -20,6 +20,8 @@ import ( "github.com/stretchr/testify/require" core_v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" + + contour_v1 "github.com/projectcontour/contour/apis/projectcontour/v1" ) func TestVirtualHostValid(t *testing.T) { @@ -265,3 +267,180 @@ func TestServiceClusterRebalance(t *testing.T) { }) } } + +func TestAddRoute(t *testing.T) { + cases := map[string]struct { + vHost VirtualHost + rs []Route + expectCount int + }{ + "3 different routes all get added": { + vHost: VirtualHost{ + Routes: map[string]*Route{}, + }, + rs: []Route{ + { + Kind: contour_v1.KindHTTPProxy, + 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: contour_v1.KindHTTPProxy, + Name: "c", + Namespace: "b", + PathMatchCondition: prefixSegment("/path1"), + QueryParamMatchConditions: []QueryParamMatchCondition{ + {Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact}, + }, + }, + { + Kind: contour_v1.KindHTTPProxy, + Name: "f", + Namespace: "g", + PathMatchCondition: prefixSegment("/path1"), + QueryParamMatchConditions: []QueryParamMatchCondition{ + {Name: "param-2", Value: "value-2", MatchType: QueryParamMatchTypeExact}, + }, + }, + }, + expectCount: 3, + }, + "3 routes, 1 and 2 has header conflict, 2 and 3 has query param conflict, only 1st one gets added": { + vHost: VirtualHost{ + Routes: map[string]*Route{}, + }, + rs: []Route{ + { + Kind: contour_v1.KindHTTPProxy, + 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}, + {Name: "param-2", Value: "value-2", MatchType: QueryParamMatchTypeExact}, + }, + }, + { + Kind: contour_v1.KindHTTPProxy, + Name: "c", + 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: contour_v1.KindHTTPProxy, + Name: "f", + Namespace: "g", + PathMatchCondition: prefixSegment("/path1"), + QueryParamMatchConditions: []QueryParamMatchCondition{ + {Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact}, + }, + }, + }, + expectCount: 1, + }, + "3 routes, 1 and 2 has header conflict, 2 and 3 has query param conflict, only 1st one gets added, but all has different paths, all get added": { + vHost: VirtualHost{ + Routes: map[string]*Route{}, + }, + rs: []Route{ + { + Kind: contour_v1.KindHTTPProxy, + 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: contour_v1.KindHTTPProxy, + Name: "c", + Namespace: "b", + PathMatchCondition: prefixSegment("/differentpath2"), + QueryParamMatchConditions: []QueryParamMatchCondition{ + {Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact}, + }, + }, + { + Kind: contour_v1.KindHTTPProxy, + Name: "f", + Namespace: "g", + PathMatchCondition: prefixSegment("/differentpath3"), + QueryParamMatchConditions: []QueryParamMatchCondition{ + {Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact}, + }, + }, + }, + expectCount: 3, + }, + "3 routes, 1 and 2 has header conflict, 2 and 3 has query param conflict, only 1st one gets added, but all are not http proxy, all get added": { + vHost: VirtualHost{ + Routes: map[string]*Route{}, + }, + rs: []Route{ + { + Kind: "HTTPRoute", + 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}, + {Name: "param-2", Value: "value-2", MatchType: QueryParamMatchTypeExact}, + }, + }, + { + Kind: "HTTPRoute", + Name: "c", + 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: "HTTPRoute", + Name: "f", + Namespace: "g", + PathMatchCondition: prefixSegment("/path1"), + QueryParamMatchConditions: []QueryParamMatchCondition{ + {Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact}, + }, + }, + }, + expectCount: 3, + }, + } + + for n, c := range cases { + t.Run(n, func(t *testing.T) { + for i := range c.rs { + c.vHost.AddRoute(&c.rs[i]) + } + + assert.Len(t, c.vHost.Routes, c.expectCount) + }) + } +} diff --git a/internal/dag/gatewayapi_processor.go b/internal/dag/gatewayapi_processor.go index 83759524dd1..24e9aeb4301 100644 --- a/internal/dag/gatewayapi_processor.go +++ b/internal/dag/gatewayapi_processor.go @@ -17,6 +17,7 @@ import ( "fmt" "net/http" "regexp" + "sort" "strings" "time" @@ -153,8 +154,10 @@ 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 p.source.httproutes { + for _, httpRoute := range sortedHTTPRoutes { p.processRoute(KindHTTPRoute, httpRoute, httpRoute.Spec.ParentRefs, gatewayNotProgrammedCondition, listenerInfos, listenerAttachedRoutes, &gatewayapi_v1.HTTPRoute{}) } @@ -2410,3 +2413,24 @@ func handlePathRewritePrefixRemoval(p *PathRewritePolicy, mc *matchConditions) * return p } + +// sort routes based on age in ascending order +// if creation times are the same, sort based on name in 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) { + if routes[i].Name == routes[j].Name { + return routes[i].Namespace < routes[j].Namespace + } + return routes[i].Name < routes[j].Name + } + return routes[i].CreationTimestamp.Before(&routes[j].CreationTimestamp) + }) + + return routes +} diff --git a/internal/dag/gatewayapi_processor_test.go b/internal/dag/gatewayapi_processor_test.go index bf7e5cf71f3..c5e93d4182c 100644 --- a/internal/dag/gatewayapi_processor_test.go +++ b/internal/dag/gatewayapi_processor_test.go @@ -16,12 +16,14 @@ package dag import ( "fmt" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" core_v1 "k8s.io/api/core/v1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/utils/ptr" gatewayapi_v1 "sigs.k8s.io/gateway-api/apis/v1" @@ -768,3 +770,242 @@ func TestGetListenersForRouteParentRef(t *testing.T) { }) } } + +func TestSortRoutes(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.HTTPRoute + expected []*gatewayapi_v1.HTTPRoute + }{ + { + name: "3 httproutes, with different timestamp, earlier one should be first ", + m: map[types.NamespacedName]*gatewayapi_v1.HTTPRoute{ + { + 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.HTTPRoute{ + { + 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 timestamp, smaller name comes first ", + m: map[types.NamespacedName]*gatewayapi_v1.HTTPRoute{ + { + 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.HTTPRoute{ + { + 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: "mixed order, two with same creation timestamp, two with same name", + m: map[types.NamespacedName]*gatewayapi_v1.HTTPRoute{ + { + 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.HTTPRoute{ + { + 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.HTTPRoute{ + { + 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.HTTPRoute{ + { + 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 := sortRoutes(tc.m) + assert.Equal(t, tc.expected, res) + }) + } +} From a63c56f0fa7ec3b5905daf02c2a7473e63ffb8e2 Mon Sep 17 00:00:00 2001 From: lubronzhan Date: Tue, 13 Feb 2024 22:47:57 -0800 Subject: [PATCH 02/15] Update logic to use header+queryParam as key for map to filter conflicts Signed-off-by: lubronzhan --- apis/projectcontour/v1/httpproxy.go | 4 -- internal/dag/dag.go | 87 ++++++++++------------------ internal/dag/dag_test.go | 65 +++++++++++++-------- internal/dag/gatewayapi_processor.go | 4 +- 4 files changed, 74 insertions(+), 86 deletions(-) diff --git a/apis/projectcontour/v1/httpproxy.go b/apis/projectcontour/v1/httpproxy.go index 520702c0781..976795d6903 100644 --- a/apis/projectcontour/v1/httpproxy.go +++ b/apis/projectcontour/v1/httpproxy.go @@ -1517,7 +1517,3 @@ type SlowStartPolicy struct { // +kubebuilder:validation:Enum=grpcroutes;tlsroutes;extensionservices;backendtlspolicies type Feature string - -const ( - KindHTTPProxy = "HTTPProxy" -) diff --git a/internal/dag/dag.go b/internal/dag/dag.go index e2eed934e8b..cabed060d88 100644 --- a/internal/dag/dag.go +++ b/internal/dag/dag.go @@ -24,10 +24,10 @@ import ( "strings" "time" + "golang.org/x/exp/maps" core_v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" - contour_v1 "github.com/projectcontour/contour/apis/projectcontour/v1" "github.com/projectcontour/contour/internal/status" "github.com/projectcontour/contour/internal/timeout" ) @@ -408,8 +408,8 @@ func (r *Route) SameRoute(route *Route) bool { return r.Name == route.Name && r.Namespace == route.Namespace } -func (r *Route) IsHTTPProxy() bool { - return r.Kind == contour_v1.KindHTTPProxy +func (r *Route) IsHTTPRoute() bool { + return r.Kind == KindHTTPRoute } // RouteTimeoutPolicy defines the timeout policy for a route. @@ -768,82 +768,59 @@ type VirtualHost struct { Routes map[string]*Route - // key is path + headerKey - RouteHeaderMatchMap map[string]*Route - - // key is path + queryParamsKey - RouteQueryParamMatchMap map[string]*Route + // key is path + headerKey + queryParam + RouteHeaderAndQueryParamMatchMap map[string]*Route } func (v *VirtualHost) AddRoute(route *Route) { if v.Routes == nil { v.Routes = make(map[string]*Route) } - // check if it's HTTPProxy type and there is existing RouteHeader or RouteQueryParams match this route candidate - if route.IsHTTPProxy() && v.hasMatchConflict(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 { - if v.hasHeaderMatchConflict(route) || v.hasQueryParamMatchConflict(route) { - return true + 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.RouteHeaderMatchMap { - if r.SameRoute(route) { - delete(v.RouteHeaderMatchMap, k) - } - } - for k, r := range v.RouteQueryParamMatchMap { + for k, r := range v.RouteHeaderAndQueryParamMatchMap { if r.SameRoute(route) { - delete(v.RouteQueryParamMatchMap, k) + delete(v.RouteHeaderAndQueryParamMatchMap, k) } } - // add headerMatchCondition and QueryParamMatchCondition to maps - for _, hmc := range route.HeaderMatchConditions { - v.RouteHeaderMatchMap[route.PathMatchCondition.String()+","+hmc.String()] = route - } - for _, rqmc := range route.QueryParamMatchConditions { - v.RouteQueryParamMatchMap[route.PathMatchCondition.String()+","+rqmc.String()] = route - } - - return false -} - -// check if there is already a route with same header match exist -func (v *VirtualHost) hasHeaderMatchConflict(route *Route) bool { - if v.RouteHeaderMatchMap == nil { - v.RouteHeaderMatchMap = make(map[string]*Route) + // 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) return false } - for _, hmc := range route.HeaderMatchConditions { - if r, ok := v.RouteHeaderMatchMap[route.PathMatchCondition.String()+","+hmc.String()]; ok { - // if it's not the same route, then it's conflict - if r.SameRoute(route) { - return true - } - } - } - return false + + return true } -// check if there is already a route with same header match exist -func (v *VirtualHost) hasQueryParamMatchConflict(route *Route) bool { - if v.RouteQueryParamMatchMap == nil { - v.RouteQueryParamMatchMap = make(map[string]*Route) - return false - } - for _, qpmc := range route.QueryParamMatchConditions { - if r, ok := v.RouteQueryParamMatchMap[route.PathMatchCondition.String()+","+qpmc.String()]; ok { - // if it's not the same route, then it's conflict - if r.SameRoute(route) { - 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 } } - return false + return tmpMap, false } func conditionsToString(r *Route) string { diff --git a/internal/dag/dag_test.go b/internal/dag/dag_test.go index b4b8f7a9fd4..577c07dca38 100644 --- a/internal/dag/dag_test.go +++ b/internal/dag/dag_test.go @@ -20,8 +20,6 @@ import ( "github.com/stretchr/testify/require" core_v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" - - contour_v1 "github.com/projectcontour/contour/apis/projectcontour/v1" ) func TestVirtualHostValid(t *testing.T) { @@ -280,7 +278,7 @@ func TestAddRoute(t *testing.T) { }, rs: []Route{ { - Kind: contour_v1.KindHTTPProxy, + Kind: KindHTTPRoute, Name: "a", Namespace: "b", PathMatchCondition: prefixSegment("/path1"), @@ -289,7 +287,7 @@ func TestAddRoute(t *testing.T) { }, }, { - Kind: contour_v1.KindHTTPProxy, + Kind: KindHTTPRoute, Name: "c", Namespace: "b", PathMatchCondition: prefixSegment("/path1"), @@ -298,7 +296,7 @@ func TestAddRoute(t *testing.T) { }, }, { - Kind: contour_v1.KindHTTPProxy, + Kind: KindHTTPRoute, Name: "f", Namespace: "g", PathMatchCondition: prefixSegment("/path1"), @@ -309,13 +307,13 @@ func TestAddRoute(t *testing.T) { }, expectCount: 3, }, - "3 routes, 1 and 2 has header conflict, 2 and 3 has query param conflict, only 1st one gets added": { + "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": { vHost: VirtualHost{ Routes: map[string]*Route{}, }, rs: []Route{ { - Kind: contour_v1.KindHTTPProxy, + Kind: KindHTTPRoute, Name: "a", Namespace: "b", PathMatchCondition: prefixSegment("/path1"), @@ -324,40 +322,48 @@ func TestAddRoute(t *testing.T) { }, QueryParamMatchConditions: []QueryParamMatchCondition{ {Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact}, - {Name: "param-2", Value: "value-2", MatchType: QueryParamMatchTypeExact}, }, }, { - Kind: contour_v1.KindHTTPProxy, + Kind: KindHTTPRoute, Name: "c", 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]+)?"}, + {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: contour_v1.KindHTTPProxy, + 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-1", Value: "value-1", MatchType: QueryParamMatchTypeExact}, + {Name: "param-2", Value: "value-2", MatchType: QueryParamMatchTypeExact}, }, }, }, - expectCount: 1, + expectCount: 2, }, - "3 routes, 1 and 2 has header conflict, 2 and 3 has query param conflict, only 1st one gets added, but all has different paths, all get added": { + "3 routes, 1 and 2 have conflict, 1 and 3 have conflict, but all has different paths, all get added": { vHost: VirtualHost{ Routes: map[string]*Route{}, }, rs: []Route{ { - Kind: contour_v1.KindHTTPProxy, + Kind: KindHTTPRoute, Name: "a", Namespace: "b", PathMatchCondition: prefixSegment("/differentpath1"), @@ -370,36 +376,42 @@ func TestAddRoute(t *testing.T) { }, }, { - Kind: contour_v1.KindHTTPProxy, + Kind: KindHTTPRoute, Name: "c", Namespace: "b", PathMatchCondition: prefixSegment("/differentpath2"), + 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: contour_v1.KindHTTPProxy, + 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-1", Value: "value-1", MatchType: QueryParamMatchTypeExact}, + {Name: "param-2", Value: "value-2", MatchType: QueryParamMatchTypeExact}, }, }, }, expectCount: 3, }, - "3 routes, 1 and 2 has header conflict, 2 and 3 has query param conflict, only 1st one gets added, but all are not http proxy, all get added": { + "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: "HTTPRoute", + Kind: "GRPCRoute", Name: "a", Namespace: "b", - PathMatchCondition: prefixSegment("/path1"), + PathMatchCondition: prefixSegment("/path"), HeaderMatchConditions: []HeaderMatchCondition{ {Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"}, }, @@ -409,10 +421,10 @@ func TestAddRoute(t *testing.T) { }, }, { - Kind: "HTTPRoute", + Kind: "GRPCRoute", Name: "c", Namespace: "b", - PathMatchCondition: prefixSegment("/path1"), + PathMatchCondition: prefixSegment("/path"), HeaderMatchConditions: []HeaderMatchCondition{ {Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"}, }, @@ -421,12 +433,15 @@ func TestAddRoute(t *testing.T) { }, }, { - Kind: "HTTPRoute", + Kind: "GRPCRoute", Name: "f", Namespace: "g", - PathMatchCondition: prefixSegment("/path1"), + 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}, }, }, }, diff --git a/internal/dag/gatewayapi_processor.go b/internal/dag/gatewayapi_processor.go index 24e9aeb4301..f9ee8db4f2d 100644 --- a/internal/dag/gatewayapi_processor.go +++ b/internal/dag/gatewayapi_processor.go @@ -2414,8 +2414,8 @@ func handlePathRewritePrefixRemoval(p *PathRewritePolicy, mc *matchConditions) * return p } -// sort routes based on age in ascending order -// if creation times are the same, sort based on name in ascending order +// sort routes based on creationTimestamp in ascending order +// if creationTimestamps are the same, sort based on name, then namespace in alphetical ascending order func sortRoutes(m map[types.NamespacedName]*gatewayapi_v1.HTTPRoute) []*gatewayapi_v1.HTTPRoute { routes := []*gatewayapi_v1.HTTPRoute{} for _, r := range m { From 5c3aeafc4c377eda43155041d96cb5b751766891 Mon Sep 17 00:00:00 2001 From: lubronzhan Date: Wed, 14 Feb 2024 00:01:24 -0800 Subject: [PATCH 03/15] Update logic to treat each route object as a single match object to compare with other match object Signed-off-by: lubronzhan --- internal/dag/dag.go | 49 ++--------- internal/dag/dag_test.go | 127 +++++---------------------- internal/dag/gatewayapi_processor.go | 52 ++++++++--- internal/status/routeconditions.go | 1 + 4 files changed, 71 insertions(+), 158 deletions(-) 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 f9ee8db4f2d..1dc40b102c1 100644 --- a/internal/dag/gatewayapi_processor.go +++ b/internal/dag/gatewayapi_processor.go @@ -1440,22 +1440,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 From 349ddb5bd58938ff3a396548a6ec9918faa3c3ee Mon Sep 17 00:00:00 2001 From: lubronzhan Date: Wed, 14 Feb 2024 00:07:42 -0800 Subject: [PATCH 04/15] Release note Signed-off-by: lubronzhan --- changelogs/unreleased/6188-lubronzhan-minor.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changelogs/unreleased/6188-lubronzhan-minor.md diff --git a/changelogs/unreleased/6188-lubronzhan-minor.md b/changelogs/unreleased/6188-lubronzhan-minor.md new file mode 100644 index 00000000000..06f95492d1d --- /dev/null +++ b/changelogs/unreleased/6188-lubronzhan-minor.md @@ -0,0 +1,8 @@ +## 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 is marked with Conflict condition. From 41fe9578991090d90dddf786172f28188dc9a159 Mon Sep 17 00:00:00 2001 From: lubronzhan Date: Thu, 15 Feb 2024 00:03:13 -0800 Subject: [PATCH 05/15] Final refactor on logic and some unite tests Signed-off-by: lubronzhan --- internal/dag/dag.go | 26 +-- internal/dag/dag_test.go | 3 +- internal/dag/gatewayapi_processor.go | 60 ++++--- internal/dag/gatewayapi_processor_test.go | 95 +++++++++++ internal/dag/status_test.go | 190 +++++++++++++++++++++- 5 files changed, 321 insertions(+), 53 deletions(-) diff --git a/internal/dag/dag.go b/internal/dag/dag.go index ac996497099..deb3a002e6c 100644 --- a/internal/dag/dag.go +++ b/internal/dag/dag.go @@ -394,23 +394,6 @@ func (r *Route) HasPathRegex() bool { return ok } -const KindHTTPProxy = "HTTPProxy" - -// SameRoute returns whether the route is the same as passed in route -func (r *Route) SameRoute(route *Route) bool { - if r == nil && route == nil { - return true - } - if r == nil || route == nil { - return false - } - return r.Name == route.Name && r.Namespace == route.Namespace -} - -func (r *Route) IsHTTPRoute() bool { - return r.Kind == KindHTTPRoute -} - // RouteTimeoutPolicy defines the timeout policy for a route. type RouteTimeoutPolicy struct { // ResponseTimeout is the timeout applied to the response @@ -780,13 +763,10 @@ func (v *VirtualHost) AddRoute(route *Route) { v.Routes[conditionsToString(route)] = route } -// HasConflictRoute returns true HTTPRoute type and there is existing Path + Headers +// HasConflictRoute returns true if there is existing Path + Headers // + QueryParams combination match this route candidate. -func (v *VirtualHost) HasConflictHTTPRoute(route *Route) bool { - if !route.IsHTTPRoute() { - return false - } - if r, ok := v.Routes[conditionsToString(route)]; ok && !r.SameRoute(route) { +func (v *VirtualHost) HasConflictRoute(route *Route) bool { + if _, ok := v.Routes[conditionsToString(route)]; ok { return true } return false diff --git a/internal/dag/dag_test.go b/internal/dag/dag_test.go index 762a4447feb..97d7e6f201f 100644 --- a/internal/dag/dag_test.go +++ b/internal/dag/dag_test.go @@ -278,7 +278,6 @@ func TestHasConflictHTTPRoute(t *testing.T) { }, rs: []Route{ { - Kind: KindHTTPRoute, Name: "a", Namespace: "b", PathMatchCondition: prefixSegment("/path1"), @@ -368,7 +367,7 @@ func TestHasConflictHTTPRoute(t *testing.T) { t.Run(n, func(t *testing.T) { c.vHost.AddRoute(&c.rs[0]) - assert.Equal(t, c.vHost.HasConflictHTTPRoute(&c.rs[1]), c.expectConflict) + assert.Equal(t, c.vHost.HasConflictRoute(&c.rs[1]), c.expectConflict) }) } } diff --git a/internal/dag/gatewayapi_processor.go b/internal/dag/gatewayapi_processor.go index 1dc40b102c1..ad8a842baa2 100644 --- a/internal/dag/gatewayapi_processor.go +++ b/internal/dag/gatewayapi_processor.go @@ -1165,6 +1165,7 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener( listener *listenerInfo, hosts sets.Set[string], ) { + routes := []*Route{} for ruleIndex, rule := range route.Spec.Rules { // Get match conditions for the rule. var matchconditions []*matchConditions @@ -1403,14 +1404,15 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener( // the same priority. priority := uint8(ruleIndex) - // Get our list of routes based on whether it's a redirect or a cluster-backed route. + // Get our list of routes under current rule based on whether it's a redirect + // or a cluster-backed route. // Note that we can end up with multiple routes here since the match conditions are // logically "OR"-ed, which we express as multiple routes, each with one of the // conditions, all with the same action. - var routes []*Route + var routesPerRule []*Route if redirect != nil { - routes = p.redirectRoutes( + routesPerRule = p.redirectRoutes( matchconditions, requestHeaderPolicy, responseHeaderPolicy, @@ -1426,7 +1428,7 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener( if !ok { continue } - routes = p.clusterRoutes( + routesPerRule = p.clusterRoutes( matchconditions, clusters, totalWeight, @@ -1440,30 +1442,34 @@ 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) - } + + routes = append(routes, routesPerRule...) + } + + // check all the routes at once in case there is conclict + if p.hasConflictRoute(listener, hosts, routes) { + // skip adding the route to svhost or vhost since it's marked as conflict route + 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) } } } - } } @@ -1474,12 +1480,12 @@ func (p *GatewayAPIProcessor) hasConflictRoute(listener *listenerInfo, hosts set switch { case listener.tlsSecret != nil: svhost := p.dag.EnsureSecureVirtualHost(listener.dagListenerName, host) - if svhost.HasConflictHTTPRoute(route) { + if svhost.HasConflictRoute(route) { return true } default: vhost := p.dag.EnsureVirtualHost(listener.dagListenerName, host) - if vhost.HasConflictHTTPRoute(route) { + if vhost.HasConflictRoute(route) { return true } } diff --git a/internal/dag/gatewayapi_processor_test.go b/internal/dag/gatewayapi_processor_test.go index c5e93d4182c..9f52f4d7bf1 100644 --- a/internal/dag/gatewayapi_processor_test.go +++ b/internal/dag/gatewayapi_processor_test.go @@ -1009,3 +1009,98 @@ func TestSortRoutes(t *testing.T) { }) } } + +func TestHasConflictRoute(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.HTTPRoute + routes []*Route + expected []*gatewayapi_v1.HTTPRoute + }{ + { + name: "3 httproutes, with different timestamp, earlier one should be first ", + m: map[types.NamespacedName]*gatewayapi_v1.HTTPRoute{ + { + 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.HTTPRoute{ + { + 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), + }, + }, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + processor := &GatewayAPIProcessor{ + FieldLogger: fixture.NewTestLogger(t), + } + listener := &listenerInfo{ + listener: gatewayapi_v1.Listener{ + Name: "http-1", + AllowedRoutes: &gatewayapi_v1.AllowedRoutes{ + Namespaces: &gatewayapi_v1.RouteNamespaces{ + From: ref.To(gatewayapi_v1.NamespacesFromSame), + }, + }, + }, + allowedKinds: []gatewayapi_v1.Kind{"HTTPRoute"}, + ready: true, + } + hosts := sets.New( + "test.projectcontour.io", + "test1.projectcontour.io", + ) + + res := processor.hasConflictRoute(listener, hosts, tc.routes) + assert.Equal(t, tc.expected, res) + }) + } +} diff --git a/internal/dag/status_test.go b/internal/dag/status_test.go index 7ed92689c56..3b0b9cb06de 100644 --- a/internal/dag/status_test.go +++ b/internal/dag/status_test.go @@ -16,6 +16,7 @@ package dag import ( "fmt" "testing" + "time" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" @@ -5510,7 +5511,7 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { "test.projectcontour.io", }, Rules: []gatewayapi_v1.HTTPRouteRule{{ - Matches: gatewayapi.HTTPRouteMatch(gatewayapi_v1.PathMatchPathPrefix, "/"), + Matches: gatewayapi.HTTPRouteMatch(gatewayapi_v1.PathMatchExact, "/"), BackendRefs: gatewayapi.HTTPBackendRef("kuard", 8080, 1), }}, }, @@ -5545,6 +5546,193 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { wantGatewayStatusUpdate: validGatewayStatusUpdate("http", gatewayapi_v1.HTTPProtocolType, 2), }) + run(t, "3 httproutes, but duplicate match condition between these 3. The 2 rank lower gets Conflict condition ", testcase{ + objs: []any{ + kuardService, + // first HTTPRoute with oldest creationTimestamp + &gatewayapi_v1.HTTPRoute{ + TypeMeta: meta_v1.TypeMeta{ + Kind: KindHTTPRoute, + }, + ObjectMeta: meta_v1.ObjectMeta{ + Name: "basic-1", + Namespace: "default", + CreationTimestamp: meta_v1.NewTime(time.Date(2020, time.Month(2), 21, 1, 10, 30, 0, time.UTC)), + }, + Spec: gatewayapi_v1.HTTPRouteSpec{ + CommonRouteSpec: gatewayapi_v1.CommonRouteSpec{ + ParentRefs: []gatewayapi_v1.ParentReference{gatewayapi.GatewayParentRef("projectcontour", "contour")}, + }, + Hostnames: []gatewayapi_v1.Hostname{ + "test.projectcontour.io", + }, + Rules: []gatewayapi_v1.HTTPRouteRule{{ + Matches: []gatewayapi_v1.HTTPRouteMatch{ + { + Path: &gatewayapi_v1.HTTPPathMatch{ + Type: ref.To(gatewayapi_v1.PathMatchPathPrefix), + Value: ref.To("/"), + }, + Headers: []gatewayapi_v1.HTTPHeaderMatch{ + { + Type: ref.To(gatewayapi_v1.HeaderMatchExact), // <---- unknown type to break the test + Name: gatewayapi_v1.HTTPHeaderName("foo"), + Value: "bar", + }, + }, + QueryParams: []gatewayapi_v1.HTTPQueryParamMatch{ + { + Type: ref.To(gatewayapi_v1.QueryParamMatchRegularExpression), + Name: "param-1", + Value: "valid-[a-z]?-[A-Za-z]+-[0=9]+-\\d+", + }, + }, + }, + { + Path: &gatewayapi_v1.HTTPPathMatch{ + Type: ref.To(gatewayapi_v1.PathMatchPathPrefix), + Value: ref.To("/"), + }, + Headers: []gatewayapi_v1.HTTPHeaderMatch{ + { + Type: ref.To(gatewayapi_v1.HeaderMatchExact), + Name: gatewayapi_v1.HTTPHeaderName("a"), + Value: "b", + }, + }, + }, + }, + + BackendRefs: gatewayapi.HTTPBackendRef("kuard", 8080, 1), + }}, + }, + }, + // second HTTPRoute with 2nd oldest creationTimestamp, conflict with 1st HTTPRoute + &gatewayapi_v1.HTTPRoute{ + TypeMeta: meta_v1.TypeMeta{ + Kind: KindHTTPRoute, + }, + ObjectMeta: meta_v1.ObjectMeta{ + Name: "basic-2", + Namespace: "default", + CreationTimestamp: meta_v1.NewTime(time.Date(2021, time.Month(2), 21, 1, 10, 30, 0, time.UTC)), + }, + Spec: gatewayapi_v1.HTTPRouteSpec{ + CommonRouteSpec: gatewayapi_v1.CommonRouteSpec{ + ParentRefs: []gatewayapi_v1.ParentReference{gatewayapi.GatewayParentRef("projectcontour", "contour")}, + }, + Hostnames: []gatewayapi_v1.Hostname{ + "test.projectcontour.io", + }, + Rules: []gatewayapi_v1.HTTPRouteRule{{ + Matches: []gatewayapi_v1.HTTPRouteMatch{ + { + Path: &gatewayapi_v1.HTTPPathMatch{ + Type: ref.To(gatewayapi_v1.PathMatchPathPrefix), + Value: ref.To("/"), + }, + Headers: []gatewayapi_v1.HTTPHeaderMatch{ + { + Type: ref.To(gatewayapi_v1.HeaderMatchExact), + Name: gatewayapi_v1.HTTPHeaderName("a"), + Value: "b", + }, + }, + }, + }, + + BackendRefs: gatewayapi.HTTPBackendRef("kuard", 8080, 1), + }}, + }, + }, + // 3rd HTTPRoute with newest creationTimestamp, conflict with 1st HTTPRoute + &gatewayapi_v1.HTTPRoute{ + TypeMeta: meta_v1.TypeMeta{ + Kind: KindHTTPRoute, + }, + ObjectMeta: meta_v1.ObjectMeta{ + Name: "basic-3", + Namespace: "default", + CreationTimestamp: meta_v1.NewTime(time.Date(2022, time.Month(2), 21, 1, 10, 30, 0, time.UTC)), + }, + Spec: gatewayapi_v1.HTTPRouteSpec{ + CommonRouteSpec: gatewayapi_v1.CommonRouteSpec{ + ParentRefs: []gatewayapi_v1.ParentReference{gatewayapi.GatewayParentRef("projectcontour", "contour")}, + }, + Hostnames: []gatewayapi_v1.Hostname{ + "test.projectcontour.io", + }, + Rules: []gatewayapi_v1.HTTPRouteRule{{ + Matches: []gatewayapi_v1.HTTPRouteMatch{ + { + Path: &gatewayapi_v1.HTTPPathMatch{ + Type: ref.To(gatewayapi_v1.PathMatchPathPrefix), + Value: ref.To("/"), + }, + Headers: []gatewayapi_v1.HTTPHeaderMatch{ + { + Type: ref.To(gatewayapi_v1.HeaderMatchExact), + Name: gatewayapi_v1.HTTPHeaderName("foo"), + Value: "bar", + }, + }, + QueryParams: []gatewayapi_v1.HTTPQueryParamMatch{ + { + Type: ref.To(gatewayapi_v1.QueryParamMatchRegularExpression), + Name: "param-1", + Value: "valid-[a-z]?-[A-Za-z]+-[0=9]+-\\d+", + }, + }, + }, + }, + + BackendRefs: gatewayapi.HTTPBackendRef("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(), + routeAcceptedHTTPRouteCondition(), + }, + }, + }, + }, + { + FullName: types.NamespacedName{Namespace: "default", Name: "basic-2"}, + RouteParentStatuses: []*gatewayapi_v1.RouteParentStatus{ + { + ParentRef: gatewayapi.GatewayParentRef("projectcontour", "contour"), + Conditions: []meta_v1.Condition{ + routeAcceptedFalse(status.ReasonRouteConflict, "HTTPRoute's Match has conflict with other HTTPRoute's Match"), + routeResolvedRefsCondition(), + }, + }, + }, + }, + { + FullName: types.NamespacedName{Namespace: "default", Name: "basic-3"}, + RouteParentStatuses: []*gatewayapi_v1.RouteParentStatus{ + { + ParentRef: gatewayapi.GatewayParentRef("projectcontour", "contour"), + Conditions: []meta_v1.Condition{ + routeAcceptedFalse(status.ReasonRouteConflict, "HTTPRoute's Match has conflict with other HTTPRoute's Match"), + 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, "prefix path match not starting with '/' for httproute", testcase{ objs: []any{ kuardService, From 73b30c08496626736e8bd7ac84ac160d1199dd67 Mon Sep 17 00:00:00 2001 From: lubronzhan Date: Thu, 15 Feb 2024 11:33:09 -0800 Subject: [PATCH 06/15] Fix conflict between upstremam Signed-off-by: lubronzhan --- internal/dag/gatewayapi_processor_test.go | 2 +- internal/dag/status_test.go | 28 +++++++++++------------ 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/internal/dag/gatewayapi_processor_test.go b/internal/dag/gatewayapi_processor_test.go index 9f52f4d7bf1..c238ec96777 100644 --- a/internal/dag/gatewayapi_processor_test.go +++ b/internal/dag/gatewayapi_processor_test.go @@ -1087,7 +1087,7 @@ func TestHasConflictRoute(t *testing.T) { Name: "http-1", AllowedRoutes: &gatewayapi_v1.AllowedRoutes{ Namespaces: &gatewayapi_v1.RouteNamespaces{ - From: ref.To(gatewayapi_v1.NamespacesFromSame), + From: ptr.To(gatewayapi_v1.NamespacesFromSame), }, }, }, diff --git a/internal/dag/status_test.go b/internal/dag/status_test.go index 3b0b9cb06de..7ca96addde3 100644 --- a/internal/dag/status_test.go +++ b/internal/dag/status_test.go @@ -5570,19 +5570,19 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { Matches: []gatewayapi_v1.HTTPRouteMatch{ { Path: &gatewayapi_v1.HTTPPathMatch{ - Type: ref.To(gatewayapi_v1.PathMatchPathPrefix), - Value: ref.To("/"), + Type: ptr.To(gatewayapi_v1.PathMatchPathPrefix), + Value: ptr.To("/"), }, Headers: []gatewayapi_v1.HTTPHeaderMatch{ { - Type: ref.To(gatewayapi_v1.HeaderMatchExact), // <---- unknown type to break the test + Type: ptr.To(gatewayapi_v1.HeaderMatchExact), // <---- unknown type to break the test Name: gatewayapi_v1.HTTPHeaderName("foo"), Value: "bar", }, }, QueryParams: []gatewayapi_v1.HTTPQueryParamMatch{ { - Type: ref.To(gatewayapi_v1.QueryParamMatchRegularExpression), + Type: ptr.To(gatewayapi_v1.QueryParamMatchRegularExpression), Name: "param-1", Value: "valid-[a-z]?-[A-Za-z]+-[0=9]+-\\d+", }, @@ -5590,12 +5590,12 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { }, { Path: &gatewayapi_v1.HTTPPathMatch{ - Type: ref.To(gatewayapi_v1.PathMatchPathPrefix), - Value: ref.To("/"), + Type: ptr.To(gatewayapi_v1.PathMatchPathPrefix), + Value: ptr.To("/"), }, Headers: []gatewayapi_v1.HTTPHeaderMatch{ { - Type: ref.To(gatewayapi_v1.HeaderMatchExact), + Type: ptr.To(gatewayapi_v1.HeaderMatchExact), Name: gatewayapi_v1.HTTPHeaderName("a"), Value: "b", }, @@ -5628,12 +5628,12 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { Matches: []gatewayapi_v1.HTTPRouteMatch{ { Path: &gatewayapi_v1.HTTPPathMatch{ - Type: ref.To(gatewayapi_v1.PathMatchPathPrefix), - Value: ref.To("/"), + Type: ptr.To(gatewayapi_v1.PathMatchPathPrefix), + Value: ptr.To("/"), }, Headers: []gatewayapi_v1.HTTPHeaderMatch{ { - Type: ref.To(gatewayapi_v1.HeaderMatchExact), + Type: ptr.To(gatewayapi_v1.HeaderMatchExact), Name: gatewayapi_v1.HTTPHeaderName("a"), Value: "b", }, @@ -5666,19 +5666,19 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { Matches: []gatewayapi_v1.HTTPRouteMatch{ { Path: &gatewayapi_v1.HTTPPathMatch{ - Type: ref.To(gatewayapi_v1.PathMatchPathPrefix), - Value: ref.To("/"), + Type: ptr.To(gatewayapi_v1.PathMatchPathPrefix), + Value: ptr.To("/"), }, Headers: []gatewayapi_v1.HTTPHeaderMatch{ { - Type: ref.To(gatewayapi_v1.HeaderMatchExact), + Type: ptr.To(gatewayapi_v1.HeaderMatchExact), Name: gatewayapi_v1.HTTPHeaderName("foo"), Value: "bar", }, }, QueryParams: []gatewayapi_v1.HTTPQueryParamMatch{ { - Type: ref.To(gatewayapi_v1.QueryParamMatchRegularExpression), + Type: ptr.To(gatewayapi_v1.QueryParamMatchRegularExpression), Name: "param-1", Value: "valid-[a-z]?-[A-Za-z]+-[0=9]+-\\d+", }, From 61d7a594fcfa122f95e8e081f4575581066ffadc Mon Sep 17 00:00:00 2001 From: lubronzhan Date: Thu, 15 Feb 2024 22:13:22 -0800 Subject: [PATCH 07/15] More unit test Signed-off-by: lubronzhan --- internal/dag/dag_test.go | 34 ++- internal/dag/gatewayapi_processor_test.go | 252 +++++++++++++++++----- 2 files changed, 230 insertions(+), 56 deletions(-) diff --git a/internal/dag/dag_test.go b/internal/dag/dag_test.go index 97d7e6f201f..fe1c32feeaa 100644 --- a/internal/dag/dag_test.go +++ b/internal/dag/dag_test.go @@ -266,7 +266,7 @@ func TestServiceClusterRebalance(t *testing.T) { } } -func TestHasConflictHTTPRoute(t *testing.T) { +func TestHasConflictRouteForVirtualHost(t *testing.T) { cases := map[string]struct { vHost VirtualHost rs []Route @@ -329,6 +329,38 @@ func TestHasConflictHTTPRoute(t *testing.T) { }, expectConflict: true, }, + "2 different routes with same path and header, but different query params, 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, don't expect conflict": { vHost: VirtualHost{ Routes: map[string]*Route{}, diff --git a/internal/dag/gatewayapi_processor_test.go b/internal/dag/gatewayapi_processor_test.go index c238ec96777..36c3d6527ff 100644 --- a/internal/dag/gatewayapi_processor_test.go +++ b/internal/dag/gatewayapi_processor_test.go @@ -1011,69 +1011,193 @@ func TestSortRoutes(t *testing.T) { } func TestHasConflictRoute(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) + host1 := "test.projectcontour.io" + host2 := "test1.projectcontour.io" + hosts := sets.New( + host1, + host2, + ) + listener := &listenerInfo{ + listener: gatewayapi_v1.Listener{ + Name: "l1", + AllowedRoutes: &gatewayapi_v1.AllowedRoutes{ + Namespaces: &gatewayapi_v1.RouteNamespaces{ + From: ptr.To(gatewayapi_v1.NamespacesFromSame), + }, + }, + }, + dagListenerName: "l1", + allowedKinds: []gatewayapi_v1.Kind{KindHTTPRoute}, + ready: true, + } + tlsListener := &listenerInfo{ + listener: gatewayapi_v1.Listener{ + Name: "ltls", + AllowedRoutes: &gatewayapi_v1.AllowedRoutes{ + Namespaces: &gatewayapi_v1.RouteNamespaces{ + From: ptr.To(gatewayapi_v1.NamespacesFromSame), + }, + }, + }, + allowedKinds: []gatewayapi_v1.Kind{KindHTTPRoute}, + ready: true, + dagListenerName: "ltls", + tlsSecret: &Secret{}, + } tests := []struct { - name string - m map[types.NamespacedName]*gatewayapi_v1.HTTPRoute - routes []*Route - expected []*gatewayapi_v1.HTTPRoute + name string + existingRoutes []*Route + routes []*Route + listener *listenerInfo + expectedConflict bool }{ { - name: "3 httproutes, with different timestamp, earlier one should be first ", - m: map[types.NamespacedName]*gatewayapi_v1.HTTPRoute{ + name: "There are 2 existing route, the 3rd route to add doesn't have conflict, listen doesn't have tls, no conflict expected", + existingRoutes: []*Route{ { - Namespace: "ns", Name: "name1", - }: { - ObjectMeta: meta_v1.ObjectMeta{ - Namespace: "ns", - Name: "name3", - CreationTimestamp: meta_v1.NewTime(time3), + Name: "route1", + Namespace: "default", + 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}, }, }, { - Namespace: "ns", Name: "name2", - }: { - ObjectMeta: meta_v1.ObjectMeta{ - Namespace: "ns", - Name: "name2", - CreationTimestamp: meta_v1.NewTime(time2), + Kind: KindHTTPRoute, + Name: "route2", + Namespace: "default", + PathMatchCondition: prefixSegment("/path2"), + HeaderMatchConditions: []HeaderMatchCondition{ + {Name: "version", Value: "2", MatchType: "exact", Invert: false}, }, }, + }, + routes: []*Route{ { - Namespace: "ns", Name: "name3", - }: { - ObjectMeta: meta_v1.ObjectMeta{ - Namespace: "ns", - Name: "name1", - CreationTimestamp: meta_v1.NewTime(time1), + Kind: KindHTTPRoute, + Name: "route3", + Namespace: "default", + PathMatchCondition: prefixSegment("/path2"), + HeaderMatchConditions: []HeaderMatchCondition{ + {Name: "e-tag", Value: "abc", MatchType: "contains", Invert: true}, }, }, }, - expected: []*gatewayapi_v1.HTTPRoute{ + listener: listener, + }, + { + name: "There are 2 existing route, the 3rd route to add doesn't have conflict, listen has tls, no conflict expected", + existingRoutes: []*Route{ { - ObjectMeta: meta_v1.ObjectMeta{ - Namespace: "ns", - Name: "name1", - CreationTimestamp: meta_v1.NewTime(time1), + Name: "route1", + Namespace: "default", + 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}, }, }, { - ObjectMeta: meta_v1.ObjectMeta{ - Namespace: "ns", - Name: "name2", - CreationTimestamp: meta_v1.NewTime(time2), + Kind: KindHTTPRoute, + Name: "route2", + Namespace: "default", + PathMatchCondition: prefixSegment("/path2"), + HeaderMatchConditions: []HeaderMatchCondition{ + {Name: "version", Value: "2", MatchType: "exact", Invert: false}, }, }, + }, + routes: []*Route{ { - ObjectMeta: meta_v1.ObjectMeta{ - Namespace: "ns", - Name: "name3", - CreationTimestamp: meta_v1.NewTime(time3), + Kind: KindHTTPRoute, + Name: "route3", + Namespace: "default", + PathMatchCondition: prefixSegment("/path2"), + HeaderMatchConditions: []HeaderMatchCondition{ + {Name: "e-tag", Value: "abc", MatchType: "contains", Invert: true}, + }, + }, + }, + listener: tlsListener, + }, + { + name: "There are 2 existing route, the 3rd route to add is conflict with 2nd route, listen doesn't have tls, expect conflicts", + existingRoutes: []*Route{ + { + Name: "route1", + Namespace: "default", + 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: "route2", + Namespace: "default", + PathMatchCondition: prefixSegment("/path2"), + HeaderMatchConditions: []HeaderMatchCondition{ + {Name: "version", Value: "2", MatchType: "exact", Invert: false}, + }, + QueryParamMatchConditions: []QueryParamMatchCondition{ + {Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact}, + }, + }, + }, + routes: []*Route{ + { + Kind: KindHTTPRoute, + Name: "route3", + Namespace: "default", + PathMatchCondition: prefixSegment("/path2"), + HeaderMatchConditions: []HeaderMatchCondition{ + {Name: "version", Value: "2", MatchType: "exact", Invert: false}, + }, + QueryParamMatchConditions: []QueryParamMatchCondition{ + {Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact}, }, }, }, + listener: listener, + expectedConflict: true, + }, + { + name: "There are 1 existing route, the 2nd route to add is conflict with 1st route, listen has tls, expect conflicts", + existingRoutes: []*Route{ + { + Kind: KindHTTPRoute, + Name: "route1", + Namespace: "default", + PathMatchCondition: prefixSegment("/path2"), + HeaderMatchConditions: []HeaderMatchCondition{ + {Name: "version", Value: "2", MatchType: "exact", Invert: false}, + }, + QueryParamMatchConditions: []QueryParamMatchCondition{ + {Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact}, + }, + }, + }, + routes: []*Route{ + { + Kind: KindHTTPRoute, + Name: "route2", + Namespace: "default", + PathMatchCondition: prefixSegment("/path2"), + HeaderMatchConditions: []HeaderMatchCondition{ + {Name: "version", Value: "2", MatchType: "exact", Invert: false}, + }, + QueryParamMatchConditions: []QueryParamMatchCondition{ + {Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact}, + }, + }, + }, + listener: tlsListener, + expectedConflict: true, }, } @@ -1081,26 +1205,44 @@ func TestHasConflictRoute(t *testing.T) { t.Run(tc.name, func(t *testing.T) { processor := &GatewayAPIProcessor{ FieldLogger: fixture.NewTestLogger(t), - } - listener := &listenerInfo{ - listener: gatewayapi_v1.Listener{ - Name: "http-1", - AllowedRoutes: &gatewayapi_v1.AllowedRoutes{ - Namespaces: &gatewayapi_v1.RouteNamespaces{ - From: ptr.To(gatewayapi_v1.NamespacesFromSame), + dag: &DAG{ + Listeners: map[string]*Listener{ + tlsListener.dagListenerName: { + svhostsByName: map[string]*SecureVirtualHost{ + host1: { + VirtualHost: VirtualHost{ + Name: host1, + Routes: make(map[string]*Route), + }, + }, + }, + }, + listener.dagListenerName: { + vhostsByName: map[string]*VirtualHost{ + host2: { + Routes: make(map[string]*Route), + }, + }, }, }, }, - allowedKinds: []gatewayapi_v1.Kind{"HTTPRoute"}, - ready: true, } - hosts := sets.New( - "test.projectcontour.io", - "test1.projectcontour.io", - ) + for host := range hosts { + for _, route := range tc.existingRoutes { + switch { + case tc.listener.tlsSecret != nil: + svhost := processor.dag.EnsureSecureVirtualHost(tc.listener.dagListenerName, host) + svhost.Secret = listener.tlsSecret + svhost.AddRoute(route) + default: + vhost := processor.dag.EnsureVirtualHost(tc.listener.dagListenerName, host) + vhost.AddRoute(route) + } + } + } - res := processor.hasConflictRoute(listener, hosts, tc.routes) - assert.Equal(t, tc.expected, res) + res := processor.hasConflictRoute(tc.listener, hosts, tc.routes) + assert.Equal(t, tc.expectedConflict, res) }) } } From 49ac07cd58e291b82a9e3820c1cc42e87522be8b Mon Sep 17 00:00:00 2001 From: lubronzhan Date: Thu, 15 Feb 2024 23:59:15 -0800 Subject: [PATCH 08/15] E2E test Signed-off-by: lubronzhan --- internal/dag/gatewayapi_processor.go | 8 +- internal/dag/gatewayapi_processor_test.go | 57 +++++++++++- internal/dag/status_test.go | 3 +- test/e2e/gateway/gateway_test.go | 2 + .../gateway/http_route_conflict_match_test.go | 86 +++++++++++++++++++ test/e2e/gatewayapi_predicates.go | 29 +++++++ 6 files changed, 178 insertions(+), 7 deletions(-) create mode 100644 test/e2e/gateway/http_route_conflict_match_test.go diff --git a/internal/dag/gatewayapi_processor.go b/internal/dag/gatewayapi_processor.go index ad8a842baa2..2a9dc960641 100644 --- a/internal/dag/gatewayapi_processor.go +++ b/internal/dag/gatewayapi_processor.go @@ -1446,7 +1446,7 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener( routes = append(routes, routesPerRule...) } - // check all the routes at once in case there is conclict + // check all the routes at once in case there is conflict if p.hasConflictRoute(listener, hosts, routes) { // skip adding the route to svhost or vhost since it's marked as conflict route routeAccessor.AddCondition( @@ -2460,10 +2460,8 @@ func sortRoutes(m map[types.NamespacedName]*gatewayapi_v1.HTTPRoute) []*gatewaya 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) { - if routes[i].Name == routes[j].Name { - return routes[i].Namespace < routes[j].Namespace - } - return routes[i].Name < routes[j].Name + return fmt.Sprintf("%s/%s", routes[i].Namespace, routes[i].Name) < + fmt.Sprintf("%s/%s", routes[j].Namespace, routes[j].Name) } return routes[i].CreationTimestamp.Before(&routes[j].CreationTimestamp) }) diff --git a/internal/dag/gatewayapi_processor_test.go b/internal/dag/gatewayapi_processor_test.go index 36c3d6527ff..c78ef4142c6 100644 --- a/internal/dag/gatewayapi_processor_test.go +++ b/internal/dag/gatewayapi_processor_test.go @@ -836,7 +836,7 @@ func TestSortRoutes(t *testing.T) { }, }, { - name: "3 httproutes with same creation timestamp, smaller name comes first ", + name: "3 httproutes with same creation timestamps, same namespaces, smaller name comes first", m: map[types.NamespacedName]*gatewayapi_v1.HTTPRoute{ { Namespace: "ns", Name: "name3", @@ -890,6 +890,61 @@ func TestSortRoutes(t *testing.T) { }, }, }, + { + name: "3 httproutes with same creation timestamp, smaller namespaces comes first", + m: map[types.NamespacedName]*gatewayapi_v1.HTTPRoute{ + { + 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.HTTPRoute{ + { + 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.HTTPRoute{ diff --git a/internal/dag/status_test.go b/internal/dag/status_test.go index 7ca96addde3..e97f7f953c1 100644 --- a/internal/dag/status_test.go +++ b/internal/dag/status_test.go @@ -5511,6 +5511,7 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { "test.projectcontour.io", }, Rules: []gatewayapi_v1.HTTPRouteRule{{ + // changed to different matches in case there is conflict with above HTTPRoute Matches: gatewayapi.HTTPRouteMatch(gatewayapi_v1.PathMatchExact, "/"), BackendRefs: gatewayapi.HTTPBackendRef("kuard", 8080, 1), }}, @@ -5575,7 +5576,7 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { }, Headers: []gatewayapi_v1.HTTPHeaderMatch{ { - Type: ptr.To(gatewayapi_v1.HeaderMatchExact), // <---- unknown type to break the test + Type: ptr.To(gatewayapi_v1.HeaderMatchExact), Name: gatewayapi_v1.HTTPHeaderName("foo"), Value: "bar", }, diff --git a/test/e2e/gateway/gateway_test.go b/test/e2e/gateway/gateway_test.go index c0287e53279..ad1b0781100 100644 --- a/test/e2e/gateway/gateway_test.go +++ b/test/e2e/gateway/gateway_test.go @@ -181,6 +181,8 @@ var _ = Describe("Gateway API", func() { f.NamespacedTest("gateway-request-redirect-rule", testWithHTTPGateway(testRequestRedirectRule)) f.NamespacedTest("gateway-backend-tls-policy", testWithHTTPGateway(testBackendTLSPolicy)) + + f.NamespacedTest("gateway-httproute-conflict-match", testWithHTTPGateway(testHTTPRouteConflictMatch)) }) Describe("Gateway with one HTTP listener and one HTTPS listener", func() { diff --git a/test/e2e/gateway/http_route_conflict_match_test.go b/test/e2e/gateway/http_route_conflict_match_test.go new file mode 100644 index 00000000000..b62a539744a --- /dev/null +++ b/test/e2e/gateway/http_route_conflict_match_test.go @@ -0,0 +1,86 @@ +// 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" + 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 testHTTPRouteConflictMatch(namespace string, gateway types.NamespacedName) { + Specify("Creates two http routes, second one has conflict match condition as the first one", func() { + By("create httproute-1 first") + route1 := &gatewayapi_v1.HTTPRoute{ + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: namespace, + Name: "httproute-1", + }, + Spec: gatewayapi_v1.HTTPRouteSpec{ + 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.HTTPRouteRule{ + { + Matches: []gatewayapi_v1.HTTPRouteMatch{ + {QueryParams: gatewayapi.HTTPQueryParamMatches(map[string]string{"animal": "whale"})}, + }, + BackendRefs: gatewayapi.HTTPBackendRef("echo-1", 80, 1), + }, + { + Matches: []gatewayapi_v1.HTTPRouteMatch{ + {QueryParams: gatewayapi.HTTPQueryParamMatches(map[string]string{"animal": "dolphin"})}, + }, + BackendRefs: gatewayapi.HTTPBackendRef("echo-2", 80, 1), + }, + }, + }, + } + f.CreateHTTPRouteAndWaitFor(route1, e2e.HTTPRouteAccepted) + + By("create httproute-2 with conflicted matches") + route2 := &gatewayapi_v1.HTTPRoute{ + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: namespace, + Name: "httproute-2", + }, + Spec: gatewayapi_v1.HTTPRouteSpec{ + 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.HTTPRouteRule{ + { + Matches: []gatewayapi_v1.HTTPRouteMatch{ + {QueryParams: gatewayapi.HTTPQueryParamMatches(map[string]string{"animal": "whale"})}, + }, + BackendRefs: gatewayapi.HTTPBackendRef("echo-1", 80, 1), + }, + }, + }, + } + f.CreateHTTPRouteAndWaitFor(route2, e2e.HTTPRouteNotAcceptedDueToConflict) + }) +} diff --git a/test/e2e/gatewayapi_predicates.go b/test/e2e/gatewayapi_predicates.go index a9ac70d2ee6..88b1e88082c 100644 --- a/test/e2e/gatewayapi_predicates.go +++ b/test/e2e/gatewayapi_predicates.go @@ -19,6 +19,8 @@ import ( 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" + + "github.com/projectcontour/contour/internal/status" ) // GatewayClassAccepted returns true if the gateway has a .status.conditions @@ -119,6 +121,23 @@ func HTTPRouteAccepted(route *gatewayapi_v1.HTTPRoute) bool { return false } +// HTTPRouteNotAcceptedDueToConflict returns true if the route has a .status.conditions +// entry of "Accepted: false" && "Reason: RouteConflict" && "Message: HTTPRoute's Match has +// conflict with other HTTPRoute's Match" +func HTTPRouteNotAcceptedDueToConflict(route *gatewayapi_v1.HTTPRoute) 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.ReasonRouteConflict), "HTTPRoute's Match has conflict with other HTTPRoute's Match") { + return false + } + } + + return false +} + // HTTPRouteIgnoredByContour returns true if the route has an empty .status.parents.conditions list func HTTPRouteIgnoredByContour(route *gatewayapi_v1.HTTPRoute) bool { if route == nil { @@ -194,3 +213,13 @@ func conditionExists(conditions []meta_v1.Condition, conditionType string, condi return false } + +func conditionExistsWithAllKeys(conditions []meta_v1.Condition, conditionType string, conditionStatus meta_v1.ConditionStatus, reason, message string) bool { + for _, cond := range conditions { + if cond.Type == conditionType && cond.Status == conditionStatus && cond.Reason == reason && cond.Message == message { + return true + } + } + + return false +} From 62fb194972bddad81058e9198c1bd64373bc065f Mon Sep 17 00:00:00 2001 From: Lubron Zhan Date: Wed, 28 Feb 2024 11:45:15 -0800 Subject: [PATCH 09/15] Basic comment resolvement Signed-off-by: Lubron Zhan --- changelogs/unreleased/6188-lubronzhan-minor.md | 2 +- internal/dag/dag.go | 3 --- internal/dag/gatewayapi_processor.go | 6 +++--- internal/dag/status_test.go | 10 +++++----- internal/status/routeconditions.go | 2 +- test/e2e/gatewayapi_predicates.go | 2 +- 6 files changed, 11 insertions(+), 14 deletions(-) diff --git a/changelogs/unreleased/6188-lubronzhan-minor.md b/changelogs/unreleased/6188-lubronzhan-minor.md index 06f95492d1d..2e22f68b01e 100644 --- a/changelogs/unreleased/6188-lubronzhan-minor.md +++ b/changelogs/unreleased/6188-lubronzhan-minor.md @@ -5,4 +5,4 @@ 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 is marked with Conflict condition. +With above ordering, any HTTPRoute that ranks lower is marked with the `Accepted: False` Condition and Reason `RuleMatchConflict `. diff --git a/internal/dag/dag.go b/internal/dag/dag.go index deb3a002e6c..b2bf912c4bd 100644 --- a/internal/dag/dag.go +++ b/internal/dag/dag.go @@ -749,9 +749,6 @@ type VirtualHost struct { IPFilterRules []IPFilterRule Routes map[string]*Route - - // key is path + headerKey + queryParam - RouteHeaderAndQueryParamMatchMap map[string]*Route } // Add route to VirtualHosts.Routes map. diff --git a/internal/dag/gatewayapi_processor.go b/internal/dag/gatewayapi_processor.go index 2a9dc960641..f5c9779e6be 100644 --- a/internal/dag/gatewayapi_processor.go +++ b/internal/dag/gatewayapi_processor.go @@ -1452,7 +1452,7 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener( routeAccessor.AddCondition( gatewayapi_v1.RouteConditionAccepted, meta_v1.ConditionFalse, - status.ReasonRouteConflict, + status.ReasonRouteRuleMatchConflict, "HTTPRoute's Match has conflict with other HTTPRoute's Match", ) } else { @@ -2460,8 +2460,8 @@ func sortRoutes(m map[types.NamespacedName]*gatewayapi_v1.HTTPRoute) []*gatewaya 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 fmt.Sprintf("%s/%s", routes[i].Namespace, routes[i].Name) < - fmt.Sprintf("%s/%s", routes[j].Namespace, routes[j].Name) + return k8s.NamespacedNameOf(routes[i]).String() < + k8s.NamespacedNameOf(routes[j]).String() } return routes[i].CreationTimestamp.Before(&routes[j].CreationTimestamp) }) diff --git a/internal/dag/status_test.go b/internal/dag/status_test.go index e97f7f953c1..c6307b46d2d 100644 --- a/internal/dag/status_test.go +++ b/internal/dag/status_test.go @@ -5558,7 +5558,7 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { ObjectMeta: meta_v1.ObjectMeta{ Name: "basic-1", Namespace: "default", - CreationTimestamp: meta_v1.NewTime(time.Date(2020, time.Month(2), 21, 1, 10, 30, 0, time.UTC)), + CreationTimestamp: meta_v1.Date(2020, time.Month(2), 21, 1, 10, 30, 0, time.UTC), }, Spec: gatewayapi_v1.HTTPRouteSpec{ CommonRouteSpec: gatewayapi_v1.CommonRouteSpec{ @@ -5616,7 +5616,7 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { ObjectMeta: meta_v1.ObjectMeta{ Name: "basic-2", Namespace: "default", - CreationTimestamp: meta_v1.NewTime(time.Date(2021, time.Month(2), 21, 1, 10, 30, 0, time.UTC)), + CreationTimestamp: meta_v1.Date(2021, time.Month(2), 21, 1, 10, 30, 0, time.UTC), }, Spec: gatewayapi_v1.HTTPRouteSpec{ CommonRouteSpec: gatewayapi_v1.CommonRouteSpec{ @@ -5654,7 +5654,7 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { ObjectMeta: meta_v1.ObjectMeta{ Name: "basic-3", Namespace: "default", - CreationTimestamp: meta_v1.NewTime(time.Date(2022, time.Month(2), 21, 1, 10, 30, 0, time.UTC)), + CreationTimestamp: meta_v1.Date(2022, time.Month(2), 21, 1, 10, 30, 0, time.UTC), }, Spec: gatewayapi_v1.HTTPRouteSpec{ CommonRouteSpec: gatewayapi_v1.CommonRouteSpec{ @@ -5711,7 +5711,7 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { { ParentRef: gatewayapi.GatewayParentRef("projectcontour", "contour"), Conditions: []meta_v1.Condition{ - routeAcceptedFalse(status.ReasonRouteConflict, "HTTPRoute's Match has conflict with other HTTPRoute's Match"), + routeAcceptedFalse(status.ReasonRouteRuleMatchConflict, "HTTPRoute's Match has conflict with other HTTPRoute's Match"), routeResolvedRefsCondition(), }, }, @@ -5723,7 +5723,7 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { { ParentRef: gatewayapi.GatewayParentRef("projectcontour", "contour"), Conditions: []meta_v1.Condition{ - routeAcceptedFalse(status.ReasonRouteConflict, "HTTPRoute's Match has conflict with other HTTPRoute's Match"), + routeAcceptedFalse(status.ReasonRouteRuleMatchConflict, "HTTPRoute's Match has conflict with other HTTPRoute's Match"), routeResolvedRefsCondition(), }, }, diff --git a/internal/status/routeconditions.go b/internal/status/routeconditions.go index 8e454e93bdf..76d3591438d 100644 --- a/internal/status/routeconditions.go +++ b/internal/status/routeconditions.go @@ -37,7 +37,7 @@ const ( ReasonInvalidPathMatch gatewayapi_v1.RouteConditionReason = "InvalidPathMatch" ReasonInvalidMethodMatch gatewayapi_v1.RouteConditionReason = "InvalidMethodMatch" ReasonInvalidGateway gatewayapi_v1.RouteConditionReason = "InvalidGateway" - ReasonRouteConflict gatewayapi_v1.RouteConditionReason = "RouteConflict" + ReasonRouteRuleMatchConflict gatewayapi_v1.RouteConditionReason = "RuleMatchConflict" ) // RouteStatusUpdate represents an atomic update to a diff --git a/test/e2e/gatewayapi_predicates.go b/test/e2e/gatewayapi_predicates.go index 88b1e88082c..4bd18359e9c 100644 --- a/test/e2e/gatewayapi_predicates.go +++ b/test/e2e/gatewayapi_predicates.go @@ -130,7 +130,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.ReasonRouteConflict), "HTTPRoute's Match has conflict with other HTTPRoute's Match") { + if conditionExistsWithAllKeys(gw.Conditions, string(gatewayapi_v1.RouteConditionAccepted), meta_v1.ConditionFalse, string(status.ReasonRouteRuleMatchConflict), "HTTPRoute's Match has conflict with other HTTPRoute's Match") { return false } } From 8bfad9dab6295ece837098074e34109fbea12801 Mon Sep 17 00:00:00 2001 From: Lubron Zhan Date: Thu, 29 Feb 2024 19:02:06 -0800 Subject: [PATCH 10/15] Update the partially match condition Signed-off-by: Lubron Zhan --- internal/dag/dag.go | 5 +- internal/dag/dag_test.go | 67 ++++++++++++- internal/dag/gatewayapi_processor.go | 61 +++++++----- internal/status/routeconditions.go | 13 +-- test/e2e/gateway/gateway_test.go | 2 + ...ttp_route_partially_conflict_match_test.go | 99 +++++++++++++++++++ test/e2e/gatewayapi_predicates.go | 21 +++- 7 files changed, 229 insertions(+), 39 deletions(-) create mode 100644 test/e2e/gateway/http_route_partially_conflict_match_test.go diff --git a/internal/dag/dag.go b/internal/dag/dag.go index b2bf912c4bd..9c67318e0c8 100644 --- a/internal/dag/dag.go +++ b/internal/dag/dag.go @@ -761,9 +761,10 @@ func (v *VirtualHost) AddRoute(route *Route) { } // HasConflictRoute returns true if there is existing Path + Headers -// + QueryParams combination match this route candidate. +// + QueryParams combination match this route candidate and also they are same kind of Route. func (v *VirtualHost) HasConflictRoute(route *Route) bool { - if _, ok := v.Routes[conditionsToString(route)]; ok { + // 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 diff --git a/internal/dag/dag_test.go b/internal/dag/dag_test.go index fe1c32feeaa..47e2c533ce3 100644 --- a/internal/dag/dag_test.go +++ b/internal/dag/dag_test.go @@ -272,12 +272,13 @@ func TestHasConflictRouteForVirtualHost(t *testing.T) { rs []Route expectConflict bool }{ - "2 different routes with same path and header, expect conflict": { + "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"), @@ -297,7 +298,7 @@ func TestHasConflictRouteForVirtualHost(t *testing.T) { }, expectConflict: true, }, - "2 different routes with same path and header and query params, expect conflict": { + "2 different routes with same path and header and query params, with same kind, expect conflict": { vHost: VirtualHost{ Routes: map[string]*Route{}, }, @@ -329,7 +330,65 @@ func TestHasConflictRouteForVirtualHost(t *testing.T) { }, expectConflict: true, }, - "2 different routes with same path and header, but different query params, don't expect conflict": { + "2 different routes with same path and header, but different kind, expect no conflict": { + vHost: VirtualHost{ + Routes: map[string]*Route{}, + }, + rs: []Route{ + { + Kind: KindTCPRoute, + 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: KindTCPRoute, + 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{}, }, @@ -361,7 +420,7 @@ func TestHasConflictRouteForVirtualHost(t *testing.T) { }, expectConflict: false, }, - "2 different routes with different path, don't expect conflict": { + "2 different routes with different path, with same kind, don't expect conflict": { vHost: VirtualHost{ Routes: map[string]*Route{}, }, diff --git a/internal/dag/gatewayapi_processor.go b/internal/dag/gatewayapi_processor.go index f5c9779e6be..de6622f302e 100644 --- a/internal/dag/gatewayapi_processor.go +++ b/internal/dag/gatewayapi_processor.go @@ -1165,7 +1165,8 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener( listener *listenerInfo, hosts sets.Set[string], ) { - routes := []*Route{} + // 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 @@ -1404,15 +1405,14 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener( // the same priority. priority := uint8(ruleIndex) - // Get our list of routes under current rule based on whether it's a redirect - // or a cluster-backed route. + // Get our list of routes based on whether it's a redirect or a cluster-backed route. // Note that we can end up with multiple routes here since the match conditions are // logically "OR"-ed, which we express as multiple routes, each with one of the // conditions, all with the same action. - var routesPerRule []*Route + var routes []*Route if redirect != nil { - routesPerRule = p.redirectRoutes( + routes = p.redirectRoutes( matchconditions, requestHeaderPolicy, responseHeaderPolicy, @@ -1428,7 +1428,7 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener( if !ok { continue } - routesPerRule = p.clusterRoutes( + routes = p.clusterRoutes( matchconditions, clusters, totalWeight, @@ -1443,33 +1443,44 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener( timeoutPolicy) } - routes = append(routes, routesPerRule...) + // 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++ + } } - // check all the routes at once in case there is conflict - if p.hasConflictRoute(listener, hosts, routes) { - // skip adding the route to svhost or vhost since it's marked as conflict route + 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, "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) - } - } - } + } else if invalidRuleCnt > 0 { + routeAccessor.AddCondition( + gatewayapi_v1.RouteConditionPartiallyInvalid, + meta_v1.ConditionTrue, + status.ReasonRouteRuleMatchPartiallyConflict, + "Dropped Rule: HTTPRoute's rule(s) has(ve) been droped because of conflict against other HTTPRoute's rule(s)", + ) } } diff --git a/internal/status/routeconditions.go b/internal/status/routeconditions.go index 76d3591438d..2edeff3e50a 100644 --- a/internal/status/routeconditions.go +++ b/internal/status/routeconditions.go @@ -32,12 +32,13 @@ const ( ) const ( - ReasonDegraded gatewayapi_v1.RouteConditionReason = "Degraded" - ReasonAllBackendRefsHaveZeroWeights gatewayapi_v1.RouteConditionReason = "AllBackendRefsHaveZeroWeights" - ReasonInvalidPathMatch gatewayapi_v1.RouteConditionReason = "InvalidPathMatch" - ReasonInvalidMethodMatch gatewayapi_v1.RouteConditionReason = "InvalidMethodMatch" - ReasonInvalidGateway gatewayapi_v1.RouteConditionReason = "InvalidGateway" - ReasonRouteRuleMatchConflict gatewayapi_v1.RouteConditionReason = "RuleMatchConflict" + ReasonDegraded gatewayapi_v1.RouteConditionReason = "Degraded" + ReasonAllBackendRefsHaveZeroWeights gatewayapi_v1.RouteConditionReason = "AllBackendRefsHaveZeroWeights" + ReasonInvalidPathMatch gatewayapi_v1.RouteConditionReason = "InvalidPathMatch" + ReasonInvalidMethodMatch gatewayapi_v1.RouteConditionReason = "InvalidMethodMatch" + ReasonInvalidGateway gatewayapi_v1.RouteConditionReason = "InvalidGateway" + ReasonRouteRuleMatchConflict gatewayapi_v1.RouteConditionReason = "RuleMatchConflict" + ReasonRouteRuleMatchPartiallyConflict gatewayapi_v1.RouteConditionReason = "RuleMatchPartiallyConflict" ) // RouteStatusUpdate represents an atomic update to a diff --git a/test/e2e/gateway/gateway_test.go b/test/e2e/gateway/gateway_test.go index ad1b0781100..506a414ed1a 100644 --- a/test/e2e/gateway/gateway_test.go +++ b/test/e2e/gateway/gateway_test.go @@ -183,6 +183,8 @@ var _ = Describe("Gateway API", func() { f.NamespacedTest("gateway-backend-tls-policy", testWithHTTPGateway(testBackendTLSPolicy)) f.NamespacedTest("gateway-httproute-conflict-match", testWithHTTPGateway(testHTTPRouteConflictMatch)) + + f.NamespacedTest("gateway-httproute-partially-conflict-match", testWithHTTPGateway(testHTTPRoutePartiallyConflictMatch)) }) Describe("Gateway with one HTTP listener and one HTTPS listener", func() { diff --git a/test/e2e/gateway/http_route_partially_conflict_match_test.go b/test/e2e/gateway/http_route_partially_conflict_match_test.go new file mode 100644 index 00000000000..be279d7bb98 --- /dev/null +++ b/test/e2e/gateway/http_route_partially_conflict_match_test.go @@ -0,0 +1,99 @@ +// 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" + 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 testHTTPRoutePartiallyConflictMatch(namespace string, gateway types.NamespacedName) { + Specify("Creates two http routes, second one has conflict match condition as the first one", func() { + By("create httproute-1 first") + route1 := &gatewayapi_v1.HTTPRoute{ + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: namespace, + Name: "httproute-1", + }, + Spec: gatewayapi_v1.HTTPRouteSpec{ + 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.HTTPRouteRule{ + { + Matches: []gatewayapi_v1.HTTPRouteMatch{ + {QueryParams: gatewayapi.HTTPQueryParamMatches(map[string]string{"animal": "whale"})}, + }, + BackendRefs: gatewayapi.HTTPBackendRef("echo-1", 80, 1), + }, + { + Matches: []gatewayapi_v1.HTTPRouteMatch{ + {QueryParams: gatewayapi.HTTPQueryParamMatches(map[string]string{"animal": "dolphin"})}, + }, + BackendRefs: gatewayapi.HTTPBackendRef("echo-2", 80, 1), + }, + }, + }, + } + f.CreateHTTPRouteAndWaitFor(route1, e2e.HTTPRouteAccepted) + + By("create httproute-2 with only partially conflicted matches") + route2 := &gatewayapi_v1.HTTPRoute{ + ObjectMeta: meta_v1.ObjectMeta{ + Namespace: namespace, + Name: "httproute-2", + }, + Spec: gatewayapi_v1.HTTPRouteSpec{ + 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.HTTPRouteRule{ + { + Matches: []gatewayapi_v1.HTTPRouteMatch{ + {QueryParams: gatewayapi.HTTPQueryParamMatches(map[string]string{"animal": "whale"})}, + }, + BackendRefs: gatewayapi.HTTPBackendRef("echo-1", 80, 1), + }, + { + Matches: []gatewayapi_v1.HTTPRouteMatch{ + {QueryParams: gatewayapi.HTTPQueryParamMatches(map[string]string{"no conflict": "no joke", "no conflict again": "no joke"})}, + }, + BackendRefs: gatewayapi.HTTPBackendRef("echo-2", 80, 1), + }, + }, + }, + } + f.CreateHTTPRouteAndWaitFor(route2, e2e.HTTPRoutePartiallyAccepted) + // 1) Drop Rule(s): With this approach, implementations will drop the + // invalid Route Rule(s) until they are fully valid again. The message + // for this condition MUST start with the prefix "Dropped Rule" and + // include information about which Rules have been dropped. In this + // state, the "Accepted" condition MUST be set to "True" with the latest + // generation of the resource. + f.CreateHTTPRouteAndWaitFor(route2, e2e.HTTPRouteAccepted) + }) +} diff --git a/test/e2e/gatewayapi_predicates.go b/test/e2e/gatewayapi_predicates.go index 4bd18359e9c..27501f81c3d 100644 --- a/test/e2e/gatewayapi_predicates.go +++ b/test/e2e/gatewayapi_predicates.go @@ -122,8 +122,8 @@ func HTTPRouteAccepted(route *gatewayapi_v1.HTTPRoute) bool { } // HTTPRouteNotAcceptedDueToConflict returns true if the route has a .status.conditions -// entry of "Accepted: false" && "Reason: RouteConflict" && "Message: HTTPRoute's Match has -// conflict with other HTTPRoute's Match" +// entry of "Accepted: false" && "Reason: RouteMatchConflict" && "Message: HTTPRoute's Match has +// conflict with other HTTPRoute's Match". func HTTPRouteNotAcceptedDueToConflict(route *gatewayapi_v1.HTTPRoute) bool { if route == nil { return false @@ -138,6 +138,23 @@ func HTTPRouteNotAcceptedDueToConflict(route *gatewayapi_v1.HTTPRoute) bool { return false } +// HTTPRoutePartiallyAccepted returns true if the route has a .status.conditions +// entry of "PartiallyInvalid: true" && "Reason: RuleMatchPartiallyConflict" && "Message: +// HTTPRoute's Match has partial conflict with other HTTPRoute's Match". +func HTTPRoutePartiallyAccepted(route *gatewayapi_v1.HTTPRoute) 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), "Dropped Rule: HTTPRoute's rule(s) has(ve) been droped because of conflict against other HTTPRoute's rule(s)") { + return false + } + } + + return false +} + // HTTPRouteIgnoredByContour returns true if the route has an empty .status.parents.conditions list func HTTPRouteIgnoredByContour(route *gatewayapi_v1.HTTPRoute) bool { if route == nil { From bdc2ed8bd390bf338adafbe82d6a5682fe76acde Mon Sep 17 00:00:00 2001 From: Lubron Zhan Date: Fri, 1 Mar 2024 11:00:26 -0800 Subject: [PATCH 11/15] Refactor Signed-off-by: Lubron Zhan --- internal/dag/gatewayapi_processor.go | 4 ++-- internal/status/routeconditions.go | 3 +++ test/e2e/gatewayapi_predicates.go | 4 ++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/internal/dag/gatewayapi_processor.go b/internal/dag/gatewayapi_processor.go index de6622f302e..7bcd6d322c6 100644 --- a/internal/dag/gatewayapi_processor.go +++ b/internal/dag/gatewayapi_processor.go @@ -1472,14 +1472,14 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener( gatewayapi_v1.RouteConditionAccepted, meta_v1.ConditionFalse, status.ReasonRouteRuleMatchConflict, - "HTTPRoute's Match has conflict with other HTTPRoute's Match", + status.MessageRouteRuleMatchConflict, ) } else if invalidRuleCnt > 0 { routeAccessor.AddCondition( gatewayapi_v1.RouteConditionPartiallyInvalid, meta_v1.ConditionTrue, status.ReasonRouteRuleMatchPartiallyConflict, - "Dropped Rule: HTTPRoute's rule(s) has(ve) been droped because of conflict against other HTTPRoute's rule(s)", + status.MessageRouteRuleMatchPartiallyConflict, ) } } diff --git a/internal/status/routeconditions.go b/internal/status/routeconditions.go index 2edeff3e50a..fa9bb809366 100644 --- a/internal/status/routeconditions.go +++ b/internal/status/routeconditions.go @@ -39,6 +39,9 @@ const ( ReasonInvalidGateway gatewayapi_v1.RouteConditionReason = "InvalidGateway" 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)" ) // RouteStatusUpdate represents an atomic update to a diff --git a/test/e2e/gatewayapi_predicates.go b/test/e2e/gatewayapi_predicates.go index 27501f81c3d..baf7e563922 100644 --- a/test/e2e/gatewayapi_predicates.go +++ b/test/e2e/gatewayapi_predicates.go @@ -130,7 +130,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), "HTTPRoute's Match has conflict with other HTTPRoute's Match") { + if conditionExistsWithAllKeys(gw.Conditions, string(gatewayapi_v1.RouteConditionAccepted), meta_v1.ConditionFalse, string(status.ReasonRouteRuleMatchConflict), status.MessageRouteRuleMatchConflict) { return false } } @@ -147,7 +147,7 @@ func HTTPRoutePartiallyAccepted(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), "Dropped Rule: HTTPRoute's rule(s) has(ve) been droped because of conflict against other HTTPRoute's rule(s)") { + if conditionExistsWithAllKeys(gw.Conditions, string(gatewayapi_v1.RouteConditionPartiallyInvalid), meta_v1.ConditionTrue, string(status.ReasonRouteRuleMatchPartiallyConflict), status.MessageRouteRuleMatchPartiallyConflict) { return false } } From a6b1f5e515ab1fa376fdbbaad66bec8e33f5cfc7 Mon Sep 17 00:00:00 2001 From: Lubron Zhan Date: Fri, 1 Mar 2024 13:38:23 -0800 Subject: [PATCH 12/15] Update the test Signed-off-by: Lubron Zhan --- test/e2e/gateway/http_route_partially_conflict_match_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/gateway/http_route_partially_conflict_match_test.go b/test/e2e/gateway/http_route_partially_conflict_match_test.go index be279d7bb98..ffd165f351e 100644 --- a/test/e2e/gateway/http_route_partially_conflict_match_test.go +++ b/test/e2e/gateway/http_route_partially_conflict_match_test.go @@ -80,7 +80,7 @@ func testHTTPRoutePartiallyConflictMatch(namespace string, gateway types.Namespa }, { Matches: []gatewayapi_v1.HTTPRouteMatch{ - {QueryParams: gatewayapi.HTTPQueryParamMatches(map[string]string{"no conflict": "no joke", "no conflict again": "no joke"})}, + {QueryParams: gatewayapi.HTTPQueryParamMatches(map[string]string{"no-conflict": "no-joke", "no-conflict-again": "no-joke"})}, }, BackendRefs: gatewayapi.HTTPBackendRef("echo-2", 80, 1), }, From 225106226d8d3b004c941160cdee7900572f10fc Mon Sep 17 00:00:00 2001 From: Lubron Zhan Date: Fri, 1 Mar 2024 14:20:13 -0800 Subject: [PATCH 13/15] Update check command Signed-off-by: Lubron Zhan --- .../gateway/http_route_conflict_match_test.go | 2 +- ...ttp_route_partially_conflict_match_test.go | 22 ++++++++++++------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/test/e2e/gateway/http_route_conflict_match_test.go b/test/e2e/gateway/http_route_conflict_match_test.go index b62a539744a..f79e52c899d 100644 --- a/test/e2e/gateway/http_route_conflict_match_test.go +++ b/test/e2e/gateway/http_route_conflict_match_test.go @@ -26,7 +26,7 @@ import ( ) func testHTTPRouteConflictMatch(namespace string, gateway types.NamespacedName) { - Specify("Creates two http routes, second one has conflict match condition as the first one", func() { + Specify("Creates two http routes, second one has conflict match against the first one, report Accepted: false", func() { By("create httproute-1 first") route1 := &gatewayapi_v1.HTTPRoute{ ObjectMeta: meta_v1.ObjectMeta{ diff --git a/test/e2e/gateway/http_route_partially_conflict_match_test.go b/test/e2e/gateway/http_route_partially_conflict_match_test.go index ffd165f351e..e04bf503402 100644 --- a/test/e2e/gateway/http_route_partially_conflict_match_test.go +++ b/test/e2e/gateway/http_route_partially_conflict_match_test.go @@ -16,9 +16,14 @@ package gateway import ( + "context" + "time" + . "github.com/onsi/ginkgo/v2" + "github.com/stretchr/testify/require" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" gatewayapi_v1 "sigs.k8s.io/gateway-api/apis/v1" "github.com/projectcontour/contour/internal/gatewayapi" @@ -26,7 +31,7 @@ import ( ) func testHTTPRoutePartiallyConflictMatch(namespace string, gateway types.NamespacedName) { - Specify("Creates two http routes, second one has conflict match condition as the first one", func() { + Specify("Creates two http routes, second one has partial conflict match against the first one, has partially match condition", func() { By("create httproute-1 first") route1 := &gatewayapi_v1.HTTPRoute{ ObjectMeta: meta_v1.ObjectMeta{ @@ -87,13 +92,14 @@ func testHTTPRoutePartiallyConflictMatch(namespace string, gateway types.Namespa }, }, } + // Partially accepted f.CreateHTTPRouteAndWaitFor(route2, e2e.HTTPRoutePartiallyAccepted) - // 1) Drop Rule(s): With this approach, implementations will drop the - // invalid Route Rule(s) until they are fully valid again. The message - // for this condition MUST start with the prefix "Dropped Rule" and - // include information about which Rules have been dropped. In this - // state, the "Accepted" condition MUST be set to "True" with the latest - // generation of the resource. - f.CreateHTTPRouteAndWaitFor(route2, e2e.HTTPRouteAccepted) + // Still has Accepted: true + require.Eventually(f.T(), func() bool { + if err := f.Client.Get(context.TODO(), client.ObjectKeyFromObject(route2), route2); err != nil { + return false + } + return e2e.HTTPRouteAccepted(route2) + }, time.Minute*2, f.RetryInterval) }) } From c6187e2f0e00a7d5d7a237c072b3fd4076fe3789 Mon Sep 17 00:00:00 2001 From: lubronzhan Date: Tue, 12 Mar 2024 19:29:54 -0700 Subject: [PATCH 14/15] Use different route Signed-off-by: lubronzhan --- internal/dag/dag_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/dag/dag_test.go b/internal/dag/dag_test.go index 47e2c533ce3..3e161d26274 100644 --- a/internal/dag/dag_test.go +++ b/internal/dag/dag_test.go @@ -336,7 +336,7 @@ func TestHasConflictRouteForVirtualHost(t *testing.T) { }, rs: []Route{ { - Kind: KindTCPRoute, + Kind: KindGRPCRoute, Name: "a", Namespace: "b", PathMatchCondition: prefixSegment("/path1"), @@ -374,7 +374,7 @@ func TestHasConflictRouteForVirtualHost(t *testing.T) { }, }, { - Kind: KindTCPRoute, + Kind: KindGRPCRoute, Name: "c", Namespace: "d", PathMatchCondition: prefixSegment("/path1"), From 2ae98ccb2c92d3beb597dfe81028f1a0610e77cb Mon Sep 17 00:00:00 2001 From: Lubron Zhan Date: Mon, 15 Apr 2024 18:18:32 -0700 Subject: [PATCH 15/15] Resolve comments Signed-off-by: Lubron Zhan --- changelogs/unreleased/6188-lubronzhan-minor.md | 4 +++- internal/dag/gatewayapi_processor.go | 2 +- .../gateway/http_route_conflict_match_test.go | 7 +++++-- ...http_route_partially_conflict_match_test.go | 18 +++++------------- test/e2e/gatewayapi_predicates.go | 8 ++++---- 5 files changed, 18 insertions(+), 21 deletions(-) diff --git a/changelogs/unreleased/6188-lubronzhan-minor.md b/changelogs/unreleased/6188-lubronzhan-minor.md index 2e22f68b01e..73240450ad5 100644 --- a/changelogs/unreleased/6188-lubronzhan-minor.md +++ b/changelogs/unreleased/6188-lubronzhan-minor.md @@ -5,4 +5,6 @@ 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 is marked with the `Accepted: False` Condition and Reason `RuleMatchConflict `. +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`. diff --git a/internal/dag/gatewayapi_processor.go b/internal/dag/gatewayapi_processor.go index 7bcd6d322c6..662d8ed1ffc 100644 --- a/internal/dag/gatewayapi_processor.go +++ b/internal/dag/gatewayapi_processor.go @@ -2462,7 +2462,7 @@ func handlePathRewritePrefixRemoval(p *PathRewritePolicy, mc *matchConditions) * } // sort routes based on creationTimestamp in ascending order -// if creationTimestamps are the same, sort based on name, then namespace in alphetical 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 { routes := []*gatewayapi_v1.HTTPRoute{} for _, r := range m { diff --git a/test/e2e/gateway/http_route_conflict_match_test.go b/test/e2e/gateway/http_route_conflict_match_test.go index f79e52c899d..a06458e19b4 100644 --- a/test/e2e/gateway/http_route_conflict_match_test.go +++ b/test/e2e/gateway/http_route_conflict_match_test.go @@ -17,6 +17,7 @@ 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" @@ -56,7 +57,8 @@ func testHTTPRouteConflictMatch(namespace string, gateway types.NamespacedName) }, }, } - f.CreateHTTPRouteAndWaitFor(route1, e2e.HTTPRouteAccepted) + _, ok := f.CreateHTTPRouteAndWaitFor(route1, e2e.HTTPRouteAccepted) + require.True(f.T(), ok) By("create httproute-2 with conflicted matches") route2 := &gatewayapi_v1.HTTPRoute{ @@ -81,6 +83,7 @@ func testHTTPRouteConflictMatch(namespace string, gateway types.NamespacedName) }, }, } - f.CreateHTTPRouteAndWaitFor(route2, e2e.HTTPRouteNotAcceptedDueToConflict) + _, ok = f.CreateHTTPRouteAndWaitFor(route2, e2e.HTTPRouteNotAcceptedDueToConflict) + require.True(f.T(), ok) }) } diff --git a/test/e2e/gateway/http_route_partially_conflict_match_test.go b/test/e2e/gateway/http_route_partially_conflict_match_test.go index e04bf503402..4e3ae38d71a 100644 --- a/test/e2e/gateway/http_route_partially_conflict_match_test.go +++ b/test/e2e/gateway/http_route_partially_conflict_match_test.go @@ -16,14 +16,10 @@ package gateway import ( - "context" - "time" - . "github.com/onsi/ginkgo/v2" "github.com/stretchr/testify/require" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" gatewayapi_v1 "sigs.k8s.io/gateway-api/apis/v1" "github.com/projectcontour/contour/internal/gatewayapi" @@ -61,7 +57,8 @@ func testHTTPRoutePartiallyConflictMatch(namespace string, gateway types.Namespa }, }, } - f.CreateHTTPRouteAndWaitFor(route1, e2e.HTTPRouteAccepted) + _, ok := f.CreateHTTPRouteAndWaitFor(route1, e2e.HTTPRouteAccepted) + require.True(f.T(), ok) By("create httproute-2 with only partially conflicted matches") route2 := &gatewayapi_v1.HTTPRoute{ @@ -93,13 +90,8 @@ func testHTTPRoutePartiallyConflictMatch(namespace string, gateway types.Namespa }, } // Partially accepted - f.CreateHTTPRouteAndWaitFor(route2, e2e.HTTPRoutePartiallyAccepted) - // Still has Accepted: true - require.Eventually(f.T(), func() bool { - if err := f.Client.Get(context.TODO(), client.ObjectKeyFromObject(route2), route2); err != nil { - return false - } - return e2e.HTTPRouteAccepted(route2) - }, time.Minute*2, f.RetryInterval) + f.CreateHTTPRouteAndWaitFor(route2, func(*gatewayapi_v1.HTTPRoute) bool { + return e2e.HTTPRoutePartiallyInvalid(route2) && e2e.HTTPRouteAccepted(route2) + }) }) } diff --git a/test/e2e/gatewayapi_predicates.go b/test/e2e/gatewayapi_predicates.go index baf7e563922..22194a67e85 100644 --- a/test/e2e/gatewayapi_predicates.go +++ b/test/e2e/gatewayapi_predicates.go @@ -131,24 +131,24 @@ 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) { - return false + return true } } return false } -// HTTPRoutePartiallyAccepted returns true if the route has a .status.conditions +// HTTPRoutePartiallyInvalid returns true if the route has a .status.conditions // entry of "PartiallyInvalid: true" && "Reason: RuleMatchPartiallyConflict" && "Message: // HTTPRoute's Match has partial conflict with other HTTPRoute's Match". -func HTTPRoutePartiallyAccepted(route *gatewayapi_v1.HTTPRoute) bool { +func HTTPRoutePartiallyInvalid(route *gatewayapi_v1.HTTPRoute) 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), status.MessageRouteRuleMatchPartiallyConflict) { - return false + return true } }