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 <lubronzhan@gmail.com>

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"
+)

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 {

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"
+
+	contourv1 "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:               contourv1.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:               contourv1.KindHTTPProxy,
+					Name:               "c",
+					Namespace:          "b",
+					PathMatchCondition: prefixSegment("/path1"),
+					QueryParamMatchConditions: []QueryParamMatchCondition{
+						{Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact},
+					},
+				},
+				{
+					Kind:               contourv1.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:               contourv1.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:               contourv1.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:               contourv1.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:               contourv1.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:               contourv1.KindHTTPProxy,
+					Name:               "c",
+					Namespace:          "b",
+					PathMatchCondition: prefixSegment("/differentpath2"),
+					QueryParamMatchConditions: []QueryParamMatchCondition{
+						{Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact},
+					},
+				},
+				{
+					Kind:               contourv1.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)
+		})
+	}
+}

internal/dag/gatewayapi_processor.go

@@ -18,6 +18,7 @@ import (
 	"fmt"
 	"net/http"
 	"regexp"
+	"sort"
 	"strings"
 	"time"
 
@@ -155,8 +156,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{})
 	}
 
@@ -2398,3 +2401,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
+}