Final refactor on logic and some unite tests

Signed-off-by: lubronzhan <lubronzhan@gmail.com>

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

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)
 		})
 	}
 }

internal/dag/gatewayapi_processor.go

@@ -1153,6 +1153,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
@@ -1391,14 +1392,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,
@@ -1414,7 +1416,7 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener(
 			if !ok {
 				continue
 			}
-			routes = p.clusterRoutes(
+			routesPerRule = p.clusterRoutes(
 				matchconditions,
 				clusters,
 				totalWeight,
@@ -1428,30 +1430,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)
 				}
 			}
 		}
-
 	}
 }
 
@@ -1462,12 +1468,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
 				}
 			}

internal/dag/gatewayapi_processor_test.go

@@ -1009,3 +1009,100 @@ 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",
+				"test2.projectcontour.io",
+				"test3.projectcontour.io",
+			)
+
+			res := processor.hasConflictRoute(listener, hosts, tc.routes)
+			assert.Equal(t, tc.expected, res)
+		})
+	}
+}