Merge pull request #4440 from shuqz/http-conformance

[feat gw-api]add dedupe in route mapper

Author: k8s-ci-robot

pkg/gateway/routeutils/loader.go

@@ -6,7 +6,6 @@ import (
 
 	"github.com/go-logr/logr"
 	"github.com/pkg/errors"
-	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	gwv1 "sigs.k8s.io/gateway-api/apis/v1"
@@ -150,7 +149,7 @@ func (l *loaderImpl) LoadRoutesForGateway(ctx context.Context, gw gwv1.Gateway,
 }
 
 // loadChildResources responsible for loading all resources that a route descriptor references.
-func (l *loaderImpl) loadChildResources(ctx context.Context, preloadedRoutes map[int][]preLoadRouteDescriptor, compatibleHostnamesByPort map[int32]map[types.NamespacedName][]gwv1.Hostname, gw gwv1.Gateway) (map[int32][]RouteDescriptor, []RouteData, error) {
+func (l *loaderImpl) loadChildResources(ctx context.Context, preloadedRoutes map[int][]preLoadRouteDescriptor, compatibleHostnamesByPort map[int32]map[string][]gwv1.Hostname, gw gwv1.Gateway) (map[int32][]RouteDescriptor, []RouteData, error) {
 	// Cache to reduce duplicate route lookups.
 	// Kind -> [NamespacedName:Previously Loaded Descriptor]
 	resourceCache := make(map[string]RouteDescriptor)
@@ -215,7 +214,7 @@ func (l *loaderImpl) loadChildResources(ctx context.Context, preloadedRoutes map
 	// Set compatible hostnames by port for all routes
 	for _, route := range resourceCache {
 		hostnamesByPort := make(map[int32][]gwv1.Hostname)
-		routeKey := route.GetRouteNamespacedName()
+		routeKey := fmt.Sprintf("%s-%s", route.GetRouteKind(), route.GetRouteNamespacedName())
 		for port, compatibleHostnames := range compatibleHostnamesByPort {
 			if hostnames, exists := compatibleHostnames[routeKey]; exists {
 				hostnamesByPort[port] = hostnames

pkg/gateway/routeutils/loader_test.go

@@ -22,9 +22,9 @@ type mockMapper struct {
 	routeStatusUpdates []RouteData
 }
 
-func (m *mockMapper) mapGatewayAndRoutes(context context.Context, gw gwv1.Gateway, routes []preLoadRouteDescriptor) (map[int][]preLoadRouteDescriptor, map[int32]map[types.NamespacedName][]gwv1.Hostname, []RouteData, error) {
+func (m *mockMapper) mapGatewayAndRoutes(context context.Context, gw gwv1.Gateway, routes []preLoadRouteDescriptor) (map[int][]preLoadRouteDescriptor, map[int32]map[string][]gwv1.Hostname, []RouteData, error) {
 	assert.ElementsMatch(m.t, m.expectedRoutes, routes)
-	return m.mapToReturn, make(map[int32]map[types.NamespacedName][]gwv1.Hostname), m.routeStatusUpdates, nil
+	return m.mapToReturn, make(map[int32]map[string][]gwv1.Hostname), m.routeStatusUpdates, nil
 }
 
 var _ RouteDescriptor = &mockRoute{}

pkg/gateway/routeutils/route_listener_mapper.go

@@ -2,6 +2,7 @@ package routeutils
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/go-logr/logr"
 	"k8s.io/apimachinery/pkg/types"
@@ -12,7 +13,7 @@ import (
 // listenerToRouteMapper is an internal utility that will map a list of routes to the listeners of a gateway
 // if the gateway and/or route are incompatible, then the route is discarded.
 type listenerToRouteMapper interface {
-	mapGatewayAndRoutes(context context.Context, gw gwv1.Gateway, routes []preLoadRouteDescriptor) (map[int][]preLoadRouteDescriptor, map[int32]map[types.NamespacedName][]gwv1.Hostname, []RouteData, error)
+	mapGatewayAndRoutes(context context.Context, gw gwv1.Gateway, routes []preLoadRouteDescriptor) (map[int][]preLoadRouteDescriptor, map[int32]map[string][]gwv1.Hostname, []RouteData, error)
 }
 
 var _ listenerToRouteMapper = &listenerToRouteMapperImpl{}
@@ -33,9 +34,9 @@ func newListenerToRouteMapper(k8sClient client.Client, logger logr.Logger) liste
 
 // mapGatewayAndRoutes will map route to the corresponding listener ports using the Gateway API spec rules.
 // Returns: (routesByPort, compatibleHostnamesByPort, failedRoutes, error)
-func (ltr *listenerToRouteMapperImpl) mapGatewayAndRoutes(ctx context.Context, gw gwv1.Gateway, routes []preLoadRouteDescriptor) (map[int][]preLoadRouteDescriptor, map[int32]map[types.NamespacedName][]gwv1.Hostname, []RouteData, error) {
+func (ltr *listenerToRouteMapperImpl) mapGatewayAndRoutes(ctx context.Context, gw gwv1.Gateway, routes []preLoadRouteDescriptor) (map[int][]preLoadRouteDescriptor, map[int32]map[string][]gwv1.Hostname, []RouteData, error) {
 	result := make(map[int][]preLoadRouteDescriptor)
-	compatibleHostnamesByPort := make(map[int32]map[types.NamespacedName][]gwv1.Hostname)
+	compatibleHostnamesByPort := make(map[int32]map[string][]gwv1.Hostname)
 	failedRoutes := make([]RouteData, 0)
 
 	// First filter out any routes that are not intended for this Gateway.
@@ -48,6 +49,8 @@ func (ltr *listenerToRouteMapperImpl) mapGatewayAndRoutes(ctx context.Context, g
 		}
 	}
 
+	// Dedupe - Check if route already exists for this port before adding
+	seenRoutesPerPort := make(map[int]map[string]bool)
 	// Next, greedily looking for the route to attach to.
 	for _, listener := range gw.Spec.Listeners {
 		// used for cross serving check
@@ -74,16 +77,23 @@ func (ltr *listenerToRouteMapperImpl) mapGatewayAndRoutes(ctx context.Context, g
 
 			if allowedAttachment {
 				port := int32(listener.Port)
-				result[int(port)] = append(result[int(port)], route)
+				routeKey := fmt.Sprintf("%s-%s", route.GetRouteKind(), route.GetRouteNamespacedName())
+				if seenRoutesPerPort[int(port)] == nil {
+					seenRoutesPerPort[int(port)] = make(map[string]bool)
+				}
+				if !seenRoutesPerPort[int(port)][routeKey] {
+					seenRoutesPerPort[int(port)][routeKey] = true
+					result[int(port)] = append(result[int(port)], route)
+				}
 
-				// Store compatible hostnames per port per route
+				// Store compatible hostnames per port per route per kind
 				if compatibleHostnamesByPort[port] == nil {
-					compatibleHostnamesByPort[port] = make(map[types.NamespacedName][]gwv1.Hostname)
+					compatibleHostnamesByPort[port] = make(map[string][]gwv1.Hostname)
 				}
 				// Append hostnames for routes that attach to multiple listeners on the same port
-				routeKey := route.GetRouteNamespacedName()
 				compatibleHostnamesByPort[port][routeKey] = append(compatibleHostnamesByPort[port][routeKey], compatibleHostnames...)
 			}
+
 		}
 	}
 	return result, compatibleHostnamesByPort, failedRoutes, nil

pkg/gateway/routeutils/route_listener_mapper_test.go

@@ -3,12 +3,13 @@ package routeutils
 import (
 	"context"
 	"fmt"
+	"testing"
+
 	"github.com/go-logr/logr"
 	"github.com/stretchr/testify/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	gwv1 "sigs.k8s.io/gateway-api/apis/v1"
-	"testing"
 )
 
 type mockListenerAttachmentHelper struct {
@@ -299,6 +300,99 @@ func Test_mapGatewayAndRoutes(t *testing.T) {
 			name:     "no output",
 			expected: make(map[int][]preLoadRouteDescriptor),
 		},
+		{
+			name: "route attaches to multiple listeners on same port - verify deduplication",
+			gw: gwv1.Gateway{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "gw1",
+					Namespace: "ns-gw",
+				},
+				Spec: gwv1.GatewaySpec{
+					Listeners: []gwv1.Listener{
+						{
+							Name: "listener1-port80",
+							Port: gwv1.PortNumber(80),
+						},
+						{
+							Name: "listener2-port80",
+							Port: gwv1.PortNumber(80),
+						},
+					},
+				},
+			},
+			routes: []preLoadRouteDescriptor{route1},
+			listenerAttachmentMap: map[string]bool{
+				"listener1-port80-80-route1-ns1": true,
+				"listener2-port80-80-route1-ns1": true,
+			},
+			routeListenerMap: map[string]bool{
+				"listener1-port80-80-route1-ns1": true,
+				"listener2-port80-80-route1-ns1": true,
+			},
+			routeGatewayMap: map[string]bool{
+				makeRouteGatewayMapKey(gateway, route1): true,
+			},
+			expected: map[int][]preLoadRouteDescriptor{
+				80: {route1}, // Only one route1, not duplicated
+			},
+		},
+		{
+			name: "different route kinds with same name attach to same listener",
+			gw: gwv1.Gateway{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "gw1",
+					Namespace: "ns-gw",
+				},
+				Spec: gwv1.GatewaySpec{
+					Listeners: []gwv1.Listener{
+						{
+							Name:     "https-listener",
+							Port:     gwv1.PortNumber(443),
+							Protocol: gwv1.HTTPSProtocolType,
+						},
+					},
+				},
+			},
+			routes: []preLoadRouteDescriptor{
+				convertHTTPRoute(gwv1.HTTPRoute{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "my-route",
+						Namespace: "default",
+					},
+				}),
+				convertGRPCRoute(gwv1.GRPCRoute{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "my-route",
+						Namespace: "default",
+					},
+				}),
+			},
+			listenerAttachmentMap: map[string]bool{
+				"https-listener-443-my-route-default": true,
+			},
+			routeListenerMap: map[string]bool{
+				"https-listener-443-my-route-default": true,
+			},
+			routeGatewayMap: map[string]bool{
+				"gw1-ns-gw-my-route-default": true,
+			},
+			expected: map[int][]preLoadRouteDescriptor{
+				443: {
+					convertHTTPRoute(gwv1.HTTPRoute{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "my-route",
+							Namespace: "default",
+						},
+					}),
+					convertGRPCRoute(gwv1.GRPCRoute{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "my-route",
+							Namespace: "default",
+						},
+					}),
+				},
+			},
+		},
 	}
 
 	for _, tc := range testCases {
@@ -313,7 +407,7 @@ func Test_mapGatewayAndRoutes(t *testing.T) {
 				},
 				logger: logr.Discard(),
 			}
-			result, _, statusUpdates, err := mapper.mapGatewayAndRoutes(context.Background(), tc.gw, tc.routes)
+			result, compatibleHostnames, statusUpdates, err := mapper.mapGatewayAndRoutes(context.Background(), tc.gw, tc.routes)
 
 			if tc.expectErr {
 				assert.Error(t, err)
@@ -322,6 +416,7 @@ func Test_mapGatewayAndRoutes(t *testing.T) {
 
 			assert.NoError(t, err)
 			assert.Equal(t, len(tc.expected), len(result))
+			assert.NotNil(t, compatibleHostnames)
 
 			assert.Equal(t, 0, len(statusUpdates))