From b16e0cb5b7c5326686406767ff61ccaf08afaf94 Mon Sep 17 00:00:00 2001 From: Saloni Date: Tue, 30 Jul 2024 14:21:32 -0600 Subject: [PATCH 1/7] reject route if not allowed by listener --- .../mode/static/state/graph/route_common.go | 32 ++- .../static/state/graph/route_common_test.go | 230 ++++++++++++++++-- 2 files changed, 238 insertions(+), 24 deletions(-) diff --git a/internal/mode/static/state/graph/route_common.go b/internal/mode/static/state/graph/route_common.go index 8fe32a96c4..f719c86e03 100644 --- a/internal/mode/static/state/graph/route_common.go +++ b/internal/mode/static/state/graph/route_common.go @@ -6,6 +6,7 @@ import ( apiv1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" "sigs.k8s.io/controller-runtime/pkg/client" @@ -354,7 +355,11 @@ func tryToAttachRouteToListeners( rk := CreateRouteKey(route.Source) bind := func(l *Listener) (allowed, attached bool) { - if !routeAllowedByListener(l, route.Source.GetNamespace(), gw.Source.Namespace, namespaces) { + if !isRouteNamespaceAllowedByListener(l, route.Source.GetNamespace(), gw.Source.Namespace, namespaces) { + return false, false + } + + if !isRouteKindAllowedByListener(l, route.Source.GetObjectKind()) { return false, false } @@ -502,13 +507,14 @@ func GetMoreSpecificHostname(hostname1, hostname2 string) string { return "" } -func routeAllowedByListener( +// isRouteNamespaceAllowedByListener checks if the route namespace is allowed by the listener. +func isRouteNamespaceAllowedByListener( listener *Listener, routeNS, gwNS string, namespaces map[types.NamespacedName]*apiv1.Namespace, ) bool { - if listener.Source.AllowedRoutes != nil { + if listener.Source.AllowedRoutes != nil && listener.Source.AllowedRoutes.Namespaces != nil { switch *listener.Source.AllowedRoutes.Namespaces.From { case v1.NamespacesFromAll: return true @@ -529,6 +535,26 @@ func routeAllowedByListener( return true } +// isRouteKindAllowedByListener checks if the route kind is allowed by the listener. +// If the listener does not specify allowed kinds, all kinds can attach to it. +// If the listener specifies allowed kinds, the route kind must be in the list. +// If the listener specifies HTTPRoute, a GRPCRoute can be attached to it. +func isRouteKindAllowedByListener(listener *Listener, routeKind schema.ObjectKind) bool { + if listener.Source.AllowedRoutes != nil && listener.Source.AllowedRoutes.Kinds != nil { + for _, kind := range listener.Source.AllowedRoutes.Kinds { + routeKind := v1.Kind(routeKind.GroupVersionKind().Kind) + if kind.Kind == routeKind { + return true + } + if kind.Kind == kinds.HTTPRoute && routeKind == kinds.GRPCRoute { + return true + } + } + return false + } + return true +} + func getHostname(h *v1.Hostname) string { if h == nil { return "" diff --git a/internal/mode/static/state/graph/route_common_test.go b/internal/mode/static/state/graph/route_common_test.go index 51f7c268a2..b8b3219344 100644 --- a/internal/mode/static/state/graph/route_common_test.go +++ b/internal/mode/static/state/graph/route_common_test.go @@ -262,6 +262,9 @@ func TestBindRouteToListeners(t *testing.T) { Namespace: "test", Name: "hr", }, + TypeMeta: metav1.TypeMeta{ + Kind: "HTTPRoute", + }, Spec: gatewayv1.HTTPRouteSpec{ CommonRouteSpec: gatewayv1.CommonRouteSpec{ ParentRefs: []gatewayv1.ParentReference{ @@ -291,9 +294,9 @@ func TestBindRouteToListeners(t *testing.T) { nil, ) - var normalRoute *L7Route - createNormalRoute := func(gateway *gatewayv1.Gateway) *L7Route { - normalRoute = &L7Route{ + var normalHTTPRoute *L7Route + createNormalHTTPRoute := func(gateway *gatewayv1.Gateway) *L7Route { + normalHTTPRoute = &L7Route{ RouteType: RouteTypeHTTP, Source: hr, Spec: L7RouteSpec{ @@ -309,10 +312,11 @@ func TestBindRouteToListeners(t *testing.T) { }, }, } - return normalRoute + return normalHTTPRoute } - getLastNormalRoute := func() *L7Route { - return normalRoute + + getLastNormalHTTPRoute := func() *L7Route { + return normalHTTPRoute } invalidAttachableRoute1 := &L7Route{ @@ -430,6 +434,62 @@ func TestBindRouteToListeners(t *testing.T) { l.Source.Hostname = helpers.GetPointer[gatewayv1.Hostname]("bar.example.com") }) + createGRPCRouteWithSectionNameAndPort := func( + sectionName *gatewayv1.SectionName, + port *gatewayv1.PortNumber, + ) *gatewayv1.GRPCRoute { + return &gatewayv1.GRPCRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "hr", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "GRPCRoute", + }, + Spec: gatewayv1.GRPCRouteSpec{ + CommonRouteSpec: gatewayv1.CommonRouteSpec{ + ParentRefs: []gatewayv1.ParentReference{ + { + Name: gatewayv1.ObjectName(gw.Name), + SectionName: sectionName, + Port: port, + }, + }, + }, + Hostnames: []gatewayv1.Hostname{ + "foo.example.com", + }, + }, + } + } + + gr := createGRPCRouteWithSectionNameAndPort(helpers.GetPointer[gatewayv1.SectionName]("listener-80-1"), nil) + + var normalGRPCRoute *L7Route + createNormalGRPCRoute := func(gateway *gatewayv1.Gateway) *L7Route { + normalGRPCRoute = &L7Route{ + RouteType: RouteTypeGRPC, + Source: gr, + Spec: L7RouteSpec{ + Hostnames: gr.Spec.Hostnames, + }, + Valid: true, + Attachable: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gateway), + SectionName: gr.Spec.ParentRefs[0].SectionName, + }, + }, + } + return normalGRPCRoute + } + + getLastNormalGRPCRoute := func() *L7Route { + return normalGRPCRoute + } + tests := []struct { route *L7Route gateway *Gateway @@ -439,7 +499,7 @@ func TestBindRouteToListeners(t *testing.T) { expectedConditions []conditions.Condition }{ { - route: createNormalRoute(gw), + route: createNormalHTTPRoute(gw), gateway: &Gateway{ Source: gw, Valid: true, @@ -463,7 +523,7 @@ func TestBindRouteToListeners(t *testing.T) { expectedGatewayListeners: []*Listener{ createModifiedListener("listener-80-1", func(l *Listener) { l.Routes = map[RouteKey]*L7Route{ - CreateRouteKey(hr): getLastNormalRoute(), + CreateRouteKey(hr): getLastNormalHTTPRoute(), } }), }, @@ -620,7 +680,7 @@ func TestBindRouteToListeners(t *testing.T) { name: "listener doesn't exist", }, { - route: createNormalRoute(gw), + route: createNormalHTTPRoute(gw), gateway: &Gateway{ Source: gw, Valid: true, @@ -646,7 +706,7 @@ func TestBindRouteToListeners(t *testing.T) { name: "listener isn't valid and attachable", }, { - route: createNormalRoute(gw), + route: createNormalHTTPRoute(gw), gateway: &Gateway{ Source: gw, Valid: true, @@ -720,7 +780,7 @@ func TestBindRouteToListeners(t *testing.T) { name: "route isn't valid", }, { - route: createNormalRoute(gw), + route: createNormalHTTPRoute(gw), gateway: &Gateway{ Source: gw, Valid: false, @@ -746,7 +806,7 @@ func TestBindRouteToListeners(t *testing.T) { name: "invalid gateway", }, { - route: createNormalRoute(gw), + route: createNormalHTTPRoute(gw), gateway: &Gateway{ Source: gw, Valid: true, @@ -773,7 +833,7 @@ func TestBindRouteToListeners(t *testing.T) { createModifiedListener("listener-80-1", func(l *Listener) { l.Valid = false l.Routes = map[RouteKey]*L7Route{ - CreateRouteKey(hr): getLastNormalRoute(), + CreateRouteKey(hr): getLastNormalHTTPRoute(), } }), }, @@ -847,7 +907,7 @@ func TestBindRouteToListeners(t *testing.T) { name: "invalid attachable listener with invalid attachable route", }, { - route: createNormalRoute(gw), + route: createNormalHTTPRoute(gw), gateway: &Gateway{ Source: gw, Valid: true, @@ -889,7 +949,7 @@ func TestBindRouteToListeners(t *testing.T) { name: "route not allowed via labels", }, { - route: createNormalRoute(gw), + route: createNormalHTTPRoute(gw), gateway: &Gateway{ Source: gw, Valid: true, @@ -928,14 +988,14 @@ func TestBindRouteToListeners(t *testing.T) { }, } l.Routes = map[RouteKey]*L7Route{ - CreateRouteKey(hr): getLastNormalRoute(), + CreateRouteKey(hr): getLastNormalHTTPRoute(), } }), }, name: "route allowed via labels", }, { - route: createNormalRoute(gwDiffNamespace), + route: createNormalHTTPRoute(gwDiffNamespace), gateway: &Gateway{ Source: gwDiffNamespace, Valid: true, @@ -973,7 +1033,7 @@ func TestBindRouteToListeners(t *testing.T) { name: "route not allowed via same namespace", }, { - route: createNormalRoute(gw), + route: createNormalHTTPRoute(gw), gateway: &Gateway{ Source: gw, Valid: true, @@ -1008,14 +1068,14 @@ func TestBindRouteToListeners(t *testing.T) { }, } l.Routes = map[RouteKey]*L7Route{ - CreateRouteKey(hr): getLastNormalRoute(), + CreateRouteKey(hr): getLastNormalHTTPRoute(), } }), }, name: "route allowed via same namespace", }, { - route: createNormalRoute(gwDiffNamespace), + route: createNormalHTTPRoute(gwDiffNamespace), gateway: &Gateway{ Source: gwDiffNamespace, Valid: true, @@ -1050,12 +1110,140 @@ func TestBindRouteToListeners(t *testing.T) { }, } l.Routes = map[RouteKey]*L7Route{ - CreateRouteKey(hr): getLastNormalRoute(), + CreateRouteKey(hr): getLastNormalHTTPRoute(), } }), }, name: "route allowed via all namespaces", }, + { + route: createNormalHTTPRoute(gw), + gateway: &Gateway{ + Source: gw, + Valid: true, + Listeners: []*Listener{ + createModifiedListener("listener-80-1", func(l *Listener) { + l.Source.AllowedRoutes = &gatewayv1.AllowedRoutes{ + Kinds: []gatewayv1.RouteGroupKind{ + {Kind: "GRPCRoute"}, + }, + } + }), + }, + }, + expectedSectionNameRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: hr.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + Attached: false, + FailedCondition: staticConds.NewRouteNotAllowedByListeners(), + AcceptedHostnames: map[string][]string{}, + }, + }, + }, + expectedGatewayListeners: []*Listener{ + createModifiedListener("listener-80-1", func(l *Listener) { + l.Source.AllowedRoutes = &gatewayv1.AllowedRoutes{ + Kinds: []gatewayv1.RouteGroupKind{ + {Kind: "GRPCRoute"}, + }, + } + }), + }, + name: "http route not allowed when listener allows only grpc routes", + }, + { + route: createNormalGRPCRoute(gw), + gateway: &Gateway{ + Source: gw, + Valid: true, + Listeners: []*Listener{ + createModifiedListener("listener-80-1", func(l *Listener) { + l.Source.AllowedRoutes = &gatewayv1.AllowedRoutes{ + Kinds: []gatewayv1.RouteGroupKind{ + {Kind: "HTTPRoute"}, + }, + } + l.Routes = map[RouteKey]*L7Route{ + CreateRouteKey(gr): getLastNormalGRPCRoute(), + } + }), + }, + }, + expectedSectionNameRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: gr.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + Attached: true, + AcceptedHostnames: map[string][]string{ + "listener-80-1": {"foo.example.com"}, + }, + }, + }, + }, + expectedGatewayListeners: []*Listener{ + createModifiedListener("listener-80-1", func(l *Listener) { + l.Source.AllowedRoutes = &gatewayv1.AllowedRoutes{ + Kinds: []gatewayv1.RouteGroupKind{ + {Kind: "HTTPRoute"}, + }, + } + l.Routes = map[RouteKey]*L7Route{ + CreateRouteKey(gr): getLastNormalGRPCRoute(), + } + }), + }, + name: "grpc route allowed when listener kind is HTTPRoute", + }, + { + route: createNormalHTTPRoute(gw), + gateway: &Gateway{ + Source: gw, + Valid: true, + Listeners: []*Listener{ + createModifiedListener("listener-80-1", func(l *Listener) { + l.Source.AllowedRoutes = &gatewayv1.AllowedRoutes{ + Kinds: []gatewayv1.RouteGroupKind{ + {Kind: "HTTPRoute"}, + }, + } + l.Routes = map[RouteKey]*L7Route{ + CreateRouteKey(hr): getLastNormalHTTPRoute(), + } + }), + }, + }, + expectedSectionNameRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: hr.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + Attached: true, + AcceptedHostnames: map[string][]string{ + "listener-80-1": {"foo.example.com"}, + }, + }, + }, + }, + expectedGatewayListeners: []*Listener{ + createModifiedListener("listener-80-1", func(l *Listener) { + l.Source.AllowedRoutes = &gatewayv1.AllowedRoutes{ + Kinds: []gatewayv1.RouteGroupKind{ + {Kind: "HTTPRoute"}, + }, + } + l.Routes = map[RouteKey]*L7Route{ + CreateRouteKey(hr): getLastNormalHTTPRoute(), + } + }), + }, + name: "http route allowed when listener kind is HTTPRoute", + }, } namespaces := map[types.NamespacedName]*v1.Namespace{ From d2224bbbf719f21eb415f0f035e7fbb630f35d1d Mon Sep 17 00:00:00 2001 From: Saloni Date: Wed, 31 Jul 2024 18:24:34 -0600 Subject: [PATCH 2/7] allowed routes rules updated --- .../mode/static/state/graph/route_common.go | 27 ++-- .../static/state/graph/route_common_test.go | 127 ++++++++++++------ 2 files changed, 98 insertions(+), 56 deletions(-) diff --git a/internal/mode/static/state/graph/route_common.go b/internal/mode/static/state/graph/route_common.go index f719c86e03..ce23c24b74 100644 --- a/internal/mode/static/state/graph/route_common.go +++ b/internal/mode/static/state/graph/route_common.go @@ -6,7 +6,6 @@ import ( apiv1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/labels" - "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" "sigs.k8s.io/controller-runtime/pkg/client" @@ -359,7 +358,7 @@ func tryToAttachRouteToListeners( return false, false } - if !isRouteKindAllowedByListener(l, route.Source.GetObjectKind()) { + if !isRouteTypeAllowedByListener(l, route.RouteType) { return false, false } @@ -535,24 +534,26 @@ func isRouteNamespaceAllowedByListener( return true } -// isRouteKindAllowedByListener checks if the route kind is allowed by the listener. -// If the listener does not specify allowed kinds, all kinds can attach to it. +// isRouteKindAllowedByListener checks if the route is allowed to attach to the listener. // If the listener specifies allowed kinds, the route kind must be in the list. -// If the listener specifies HTTPRoute, a GRPCRoute can be attached to it. -func isRouteKindAllowedByListener(listener *Listener, routeKind schema.ObjectKind) bool { +// If the listener does not specify allowedRoutes, allowed routes are determined using the listener protocol. +func isRouteTypeAllowedByListener(listener *Listener, routeType RouteType) bool { if listener.Source.AllowedRoutes != nil && listener.Source.AllowedRoutes.Kinds != nil { for _, kind := range listener.Source.AllowedRoutes.Kinds { - routeKind := v1.Kind(routeKind.GroupVersionKind().Kind) - if kind.Kind == routeKind { - return true - } - if kind.Kind == kinds.HTTPRoute && routeKind == kinds.GRPCRoute { - return true + switch kind.Kind { + case kinds.HTTPRoute: + return routeType == RouteTypeHTTP + case kinds.GRPCRoute: + return routeType == RouteTypeGRPC } } return false } - return true + + if listener.Source.Protocol == v1.HTTPProtocolType || listener.Source.Protocol == v1.HTTPSProtocolType { + return routeType == RouteTypeHTTP || routeType == RouteTypeGRPC + } + return false } func getHostname(h *v1.Hostname) string { diff --git a/internal/mode/static/state/graph/route_common_test.go b/internal/mode/static/state/graph/route_common_test.go index b8b3219344..8259101af5 100644 --- a/internal/mode/static/state/graph/route_common_test.go +++ b/internal/mode/static/state/graph/route_common_test.go @@ -228,6 +228,7 @@ func TestBindRouteToListeners(t *testing.T) { Source: gatewayv1.Listener{ Name: gatewayv1.SectionName(name), Hostname: (*gatewayv1.Hostname)(helpers.GetPointer("foo.example.com")), + Protocol: gatewayv1.HTTPProtocolType, }, Valid: true, Attachable: true, @@ -1116,44 +1117,6 @@ func TestBindRouteToListeners(t *testing.T) { }, name: "route allowed via all namespaces", }, - { - route: createNormalHTTPRoute(gw), - gateway: &Gateway{ - Source: gw, - Valid: true, - Listeners: []*Listener{ - createModifiedListener("listener-80-1", func(l *Listener) { - l.Source.AllowedRoutes = &gatewayv1.AllowedRoutes{ - Kinds: []gatewayv1.RouteGroupKind{ - {Kind: "GRPCRoute"}, - }, - } - }), - }, - }, - expectedSectionNameRefs: []ParentRef{ - { - Idx: 0, - Gateway: client.ObjectKeyFromObject(gw), - SectionName: hr.Spec.ParentRefs[0].SectionName, - Attachment: &ParentRefAttachmentStatus{ - Attached: false, - FailedCondition: staticConds.NewRouteNotAllowedByListeners(), - AcceptedHostnames: map[string][]string{}, - }, - }, - }, - expectedGatewayListeners: []*Listener{ - createModifiedListener("listener-80-1", func(l *Listener) { - l.Source.AllowedRoutes = &gatewayv1.AllowedRoutes{ - Kinds: []gatewayv1.RouteGroupKind{ - {Kind: "GRPCRoute"}, - }, - } - }), - }, - name: "http route not allowed when listener allows only grpc routes", - }, { route: createNormalGRPCRoute(gw), gateway: &Gateway{ @@ -1178,10 +1141,9 @@ func TestBindRouteToListeners(t *testing.T) { Gateway: client.ObjectKeyFromObject(gw), SectionName: gr.Spec.ParentRefs[0].SectionName, Attachment: &ParentRefAttachmentStatus{ - Attached: true, - AcceptedHostnames: map[string][]string{ - "listener-80-1": {"foo.example.com"}, - }, + Attached: false, + FailedCondition: staticConds.NewRouteNotAllowedByListeners(), + AcceptedHostnames: map[string][]string{}, }, }, }, @@ -1197,7 +1159,7 @@ func TestBindRouteToListeners(t *testing.T) { } }), }, - name: "grpc route allowed when listener kind is HTTPRoute", + name: "grpc route not allowed when listener kind is HTTPRoute", }, { route: createNormalHTTPRoute(gw), @@ -1772,3 +1734,82 @@ func TestRouteKeyForKind(t *testing.T) { g.Expect(rk).To(Panic()) } + +func TestAllowedRouteType(t *testing.T) { + test := []struct { + listener *Listener + name string + routeType RouteType + expResult bool + }{ + { + name: "httpRoute with listener protocol http", + routeType: RouteTypeHTTP, + listener: &Listener{ + Source: gatewayv1.Listener{ + Protocol: gatewayv1.HTTPProtocolType, + }, + }, + expResult: true, + }, + { + name: "grpcRoute with listener protocol https", + routeType: RouteTypeGRPC, + listener: &Listener{ + Source: gatewayv1.Listener{ + Protocol: gatewayv1.HTTPSProtocolType, + }, + }, + expResult: true, + }, + { + name: "grpcRoute with listener allowedRoutes set to httpRoute is not allowed", + routeType: RouteTypeGRPC, + listener: &Listener{ + Source: gatewayv1.Listener{ + AllowedRoutes: &gatewayv1.AllowedRoutes{ + Kinds: []gatewayv1.RouteGroupKind{ + {Kind: kinds.HTTPRoute}, + }, + }, + }, + }, + expResult: false, + }, + { + name: "httpRoute with listener allowedRoutes set to grpcRoute is not allowed", + routeType: RouteTypeHTTP, + listener: &Listener{ + Source: gatewayv1.Listener{ + AllowedRoutes: &gatewayv1.AllowedRoutes{ + Kinds: []gatewayv1.RouteGroupKind{ + {Kind: kinds.GRPCRoute}, + }, + }, + }, + }, + expResult: false, + }, + { + name: "grpcRoute with listener allowedRoutes set to grpcRoute is allowed", + routeType: RouteTypeGRPC, + listener: &Listener{ + Source: gatewayv1.Listener{ + AllowedRoutes: &gatewayv1.AllowedRoutes{ + Kinds: []gatewayv1.RouteGroupKind{ + {Kind: kinds.GRPCRoute}, + }, + }, + }, + }, + expResult: true, + }, + } + + for _, test := range test { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + g.Expect(isRouteTypeAllowedByListener(test.listener, test.routeType)).To(Equal(test.expResult)) + }) + } +} From aea34cddcd4f23b994d47e9a174c09c01e88cd69 Mon Sep 17 00:00:00 2001 From: Saloni Date: Wed, 31 Jul 2024 21:09:40 -0600 Subject: [PATCH 3/7] update tests --- .../mode/static/state/graph/route_common.go | 4 +- .../static/state/graph/route_common_test.go | 38 +++++++++++++++---- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/internal/mode/static/state/graph/route_common.go b/internal/mode/static/state/graph/route_common.go index ce23c24b74..7e205a0e3e 100644 --- a/internal/mode/static/state/graph/route_common.go +++ b/internal/mode/static/state/graph/route_common.go @@ -550,9 +550,11 @@ func isRouteTypeAllowedByListener(listener *Listener, routeType RouteType) bool return false } - if listener.Source.Protocol == v1.HTTPProtocolType || listener.Source.Protocol == v1.HTTPSProtocolType { + switch listener.Source.Protocol { + case v1.HTTPProtocolType, v1.HTTPSProtocolType: return routeType == RouteTypeHTTP || routeType == RouteTypeGRPC } + return false } diff --git a/internal/mode/static/state/graph/route_common_test.go b/internal/mode/static/state/graph/route_common_test.go index 8259101af5..94538be2cf 100644 --- a/internal/mode/static/state/graph/route_common_test.go +++ b/internal/mode/static/state/graph/route_common_test.go @@ -1743,7 +1743,7 @@ func TestAllowedRouteType(t *testing.T) { expResult bool }{ { - name: "httpRoute with listener protocol http", + name: "httpRoute attaches to listener with protocol http", routeType: RouteTypeHTTP, listener: &Listener{ Source: gatewayv1.Listener{ @@ -1753,7 +1753,7 @@ func TestAllowedRouteType(t *testing.T) { expResult: true, }, { - name: "grpcRoute with listener protocol https", + name: "grpcRoute attaches to listener with protocol https", routeType: RouteTypeGRPC, listener: &Listener{ Source: gatewayv1.Listener{ @@ -1763,7 +1763,31 @@ func TestAllowedRouteType(t *testing.T) { expResult: true, }, { - name: "grpcRoute with listener allowedRoutes set to httpRoute is not allowed", + name: "grpcRoute is not allowed to attach to listener with tcp protocol", + routeType: RouteTypeGRPC, + listener: &Listener{ + Source: gatewayv1.Listener{ + Protocol: gatewayv1.TCPProtocolType, + }, + }, + expResult: false, + }, + { + name: "grpcRoute attaches to listener with allowedRoutes set to grpcRoute", + routeType: RouteTypeGRPC, + listener: &Listener{ + Source: gatewayv1.Listener{ + AllowedRoutes: &gatewayv1.AllowedRoutes{ + Kinds: []gatewayv1.RouteGroupKind{ + {Kind: kinds.GRPCRoute}, + }, + }, + }, + }, + expResult: true, + }, + { + name: "grpcRoute not allowed to attach to listener with allowedRoutes set to httpRoute", routeType: RouteTypeGRPC, listener: &Listener{ Source: gatewayv1.Listener{ @@ -1777,7 +1801,7 @@ func TestAllowedRouteType(t *testing.T) { expResult: false, }, { - name: "httpRoute with listener allowedRoutes set to grpcRoute is not allowed", + name: "httpRoute not allowed to attach to listener with allowedRoutes set to grpcRoute", routeType: RouteTypeHTTP, listener: &Listener{ Source: gatewayv1.Listener{ @@ -1791,18 +1815,18 @@ func TestAllowedRouteType(t *testing.T) { expResult: false, }, { - name: "grpcRoute with listener allowedRoutes set to grpcRoute is allowed", + name: "grpcRoute not allowed to attach to listener with allowedRoutes set to testRoute", routeType: RouteTypeGRPC, listener: &Listener{ Source: gatewayv1.Listener{ AllowedRoutes: &gatewayv1.AllowedRoutes{ Kinds: []gatewayv1.RouteGroupKind{ - {Kind: kinds.GRPCRoute}, + {Kind: gatewayv1.Kind("testRoute")}, }, }, }, }, - expResult: true, + expResult: false, }, } From d4ba0cf255e513f8ad079dd5eb906c30b609adf0 Mon Sep 17 00:00:00 2001 From: Saloni Date: Thu, 1 Aug 2024 13:46:06 -0600 Subject: [PATCH 4/7] fix logic --- internal/mode/static/state/graph/route_common.go | 13 ++++++------- .../mode/static/state/graph/route_common_test.go | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/internal/mode/static/state/graph/route_common.go b/internal/mode/static/state/graph/route_common.go index 7e205a0e3e..6a5648468a 100644 --- a/internal/mode/static/state/graph/route_common.go +++ b/internal/mode/static/state/graph/route_common.go @@ -538,13 +538,12 @@ func isRouteNamespaceAllowedByListener( // If the listener specifies allowed kinds, the route kind must be in the list. // If the listener does not specify allowedRoutes, allowed routes are determined using the listener protocol. func isRouteTypeAllowedByListener(listener *Listener, routeType RouteType) bool { - if listener.Source.AllowedRoutes != nil && listener.Source.AllowedRoutes.Kinds != nil { - for _, kind := range listener.Source.AllowedRoutes.Kinds { - switch kind.Kind { - case kinds.HTTPRoute: - return routeType == RouteTypeHTTP - case kinds.GRPCRoute: - return routeType == RouteTypeGRPC + allowedRoutes := listener.Source.AllowedRoutes + if allowedRoutes != nil && allowedRoutes.Kinds != nil { + for _, kind := range allowedRoutes.Kinds { + if (kind.Kind == kinds.HTTPRoute && routeType == RouteTypeHTTP) || + (kind.Kind == kinds.GRPCRoute && routeType == RouteTypeGRPC) { + return true } } return false diff --git a/internal/mode/static/state/graph/route_common_test.go b/internal/mode/static/state/graph/route_common_test.go index 94538be2cf..181226a7c7 100644 --- a/internal/mode/static/state/graph/route_common_test.go +++ b/internal/mode/static/state/graph/route_common_test.go @@ -1786,6 +1786,21 @@ func TestAllowedRouteType(t *testing.T) { }, expResult: true, }, + { + name: "grpcRoute attaches to listener with allowedRoutes set to grpcRoute and httpRoute", + routeType: RouteTypeGRPC, + listener: &Listener{ + Source: gatewayv1.Listener{ + AllowedRoutes: &gatewayv1.AllowedRoutes{ + Kinds: []gatewayv1.RouteGroupKind{ + {Kind: kinds.HTTPRoute}, + {Kind: kinds.GRPCRoute}, + }, + }, + }, + }, + expResult: true, + }, { name: "grpcRoute not allowed to attach to listener with allowedRoutes set to httpRoute", routeType: RouteTypeGRPC, From 175ac09c9f0f9785a2e0c6b64113ede8fbd953b0 Mon Sep 17 00:00:00 2001 From: Saloni Date: Thu, 1 Aug 2024 14:25:47 -0600 Subject: [PATCH 5/7] add conversion function --- internal/mode/static/state/graph/route_common.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/internal/mode/static/state/graph/route_common.go b/internal/mode/static/state/graph/route_common.go index 6a5648468a..b1e299aa4a 100644 --- a/internal/mode/static/state/graph/route_common.go +++ b/internal/mode/static/state/graph/route_common.go @@ -541,8 +541,7 @@ func isRouteTypeAllowedByListener(listener *Listener, routeType RouteType) bool allowedRoutes := listener.Source.AllowedRoutes if allowedRoutes != nil && allowedRoutes.Kinds != nil { for _, kind := range allowedRoutes.Kinds { - if (kind.Kind == kinds.HTTPRoute && routeType == RouteTypeHTTP) || - (kind.Kind == kinds.GRPCRoute && routeType == RouteTypeGRPC) { + if kind.Kind == convertRouteType(routeType) { return true } } @@ -557,6 +556,17 @@ func isRouteTypeAllowedByListener(listener *Listener, routeType RouteType) bool return false } +func convertRouteType(routeType RouteType) v1.Kind { + switch routeType { + case RouteTypeHTTP: + return kinds.HTTPRoute + case RouteTypeGRPC: + return kinds.GRPCRoute + default: + return "" + } +} + func getHostname(h *v1.Hostname) string { if h == nil { return "" From be047a9c9dd46787f12c5a56db5b0f1d1258fabc Mon Sep 17 00:00:00 2001 From: Saloni Date: Thu, 1 Aug 2024 17:48:59 -0600 Subject: [PATCH 6/7] modify getAndValidateSupportedKind --- .../static/state/change_processor_test.go | 20 +- .../static/state/graph/gateway_listener.go | 27 +- .../state/graph/gateway_listener_test.go | 32 +-- .../mode/static/state/graph/gateway_test.go | 239 +++++++----------- .../mode/static/state/graph/graph_test.go | 9 +- .../mode/static/state/graph/route_common.go | 16 +- .../static/state/graph/route_common_test.go | 94 ++----- 7 files changed, 163 insertions(+), 274 deletions(-) diff --git a/internal/mode/static/state/change_processor_test.go b/internal/mode/static/state/change_processor_test.go index 4b88aeb7de..3e1455b2ca 100644 --- a/internal/mode/static/state/change_processor_test.go +++ b/internal/mode/static/state/change_processor_test.go @@ -519,12 +519,15 @@ var _ = Describe("ChangeProcessor", func() { Source: gw1, Listeners: []*graph.Listener{ { - Name: "listener-80-1", - Source: gw1.Spec.Listeners[0], - Valid: true, - Attachable: true, - Routes: map[graph.RouteKey]*graph.L7Route{routeKey1: expRouteHR1}, - SupportedKinds: []v1.RouteGroupKind{{Kind: kinds.HTTPRoute}}, + Name: "listener-80-1", + Source: gw1.Spec.Listeners[0], + Valid: true, + Attachable: true, + Routes: map[graph.RouteKey]*graph.L7Route{routeKey1: expRouteHR1}, + SupportedKinds: []v1.RouteGroupKind{ + {Kind: v1.Kind(kinds.HTTPRoute), Group: helpers.GetPointer[v1.Group](v1.GroupName)}, + {Kind: v1.Kind(kinds.GRPCRoute), Group: helpers.GetPointer[v1.Group](v1.GroupName)}, + }, }, { Name: "listener-443-1", @@ -533,7 +536,10 @@ var _ = Describe("ChangeProcessor", func() { Attachable: true, Routes: map[graph.RouteKey]*graph.L7Route{routeKey1: expRouteHR1}, ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(diffNsTLSSecret)), - SupportedKinds: []v1.RouteGroupKind{{Kind: kinds.HTTPRoute}}, + SupportedKinds: []v1.RouteGroupKind{ + {Kind: v1.Kind(kinds.HTTPRoute), Group: helpers.GetPointer[v1.Group](v1.GroupName)}, + {Kind: v1.Kind(kinds.GRPCRoute), Group: helpers.GetPointer[v1.Group](v1.GroupName)}, + }, }, }, Valid: true, diff --git a/internal/mode/static/state/graph/gateway_listener.go b/internal/mode/static/state/graph/gateway_listener.go index 7c61d45231..94bceac2da 100644 --- a/internal/mode/static/state/graph/gateway_listener.go +++ b/internal/mode/static/state/graph/gateway_listener.go @@ -11,6 +11,7 @@ import ( v1 "sigs.k8s.io/gateway-api/apis/v1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" ) @@ -228,18 +229,10 @@ func getAndValidateListenerSupportedKinds(listener v1.Listener) ( []conditions.Condition, []v1.RouteGroupKind, ) { - if listener.AllowedRoutes == nil || listener.AllowedRoutes.Kinds == nil { - return nil, []v1.RouteGroupKind{ - { - Kind: kinds.HTTPRoute, - }, - } - } var conds []conditions.Condition + supportedKinds := make([]v1.RouteGroupKind, 0) - supportedKinds := make([]v1.RouteGroupKind, 0, len(listener.AllowedRoutes.Kinds)) - - validHTTPProtocolRouteKind := func(kind v1.RouteGroupKind) bool { + validRouteKind := func(kind v1.RouteGroupKind) bool { if kind.Kind != v1.Kind(kinds.HTTPRoute) && kind.Kind != v1.Kind(kinds.GRPCRoute) { return false } @@ -249,17 +242,25 @@ func getAndValidateListenerSupportedKinds(listener v1.Listener) ( return true } - switch listener.Protocol { - case v1.HTTPProtocolType, v1.HTTPSProtocolType: + if listener.AllowedRoutes != nil && listener.AllowedRoutes.Kinds != nil { for _, kind := range listener.AllowedRoutes.Kinds { - if !validHTTPProtocolRouteKind(kind) { + if !validRouteKind(kind) { msg := fmt.Sprintf("Unsupported route kind \"%s/%s\"", *kind.Group, kind.Kind) conds = append(conds, staticConds.NewListenerInvalidRouteKinds(msg)...) continue } supportedKinds = append(supportedKinds, kind) } + } else { + switch listener.Protocol { + case v1.HTTPProtocolType, v1.HTTPSProtocolType: + supportedKinds = []v1.RouteGroupKind{ + {Kind: v1.Kind(kinds.HTTPRoute), Group: helpers.GetPointer[v1.Group](v1.GroupName)}, + {Kind: v1.Kind(kinds.GRPCRoute), Group: helpers.GetPointer[v1.Group](v1.GroupName)}, + } + } } + return conds, supportedKinds } diff --git a/internal/mode/static/state/graph/gateway_listener_test.go b/internal/mode/static/state/graph/gateway_listener_test.go index 96aba1a66c..534c1eeeda 100644 --- a/internal/mode/static/state/graph/gateway_listener_test.go +++ b/internal/mode/static/state/graph/gateway_listener_test.go @@ -289,11 +289,13 @@ func TestValidateListenerHostname(t *testing.T) { } func TestGetAndValidateListenerSupportedKinds(t *testing.T) { - HTTPRouteGroupKind := []v1.RouteGroupKind{ - { - Kind: kinds.HTTPRoute, - Group: helpers.GetPointer[v1.Group](v1.GroupName), - }, + HTTPRouteGroupKind := v1.RouteGroupKind{ + Kind: kinds.HTTPRoute, + Group: helpers.GetPointer[v1.Group](v1.GroupName), + } + GRPCRouteGroupKind := v1.RouteGroupKind{ + Kind: kinds.GRPCRoute, + Group: helpers.GetPointer[v1.Group](v1.GroupName), } TCPRouteGroupKind := []v1.RouteGroupKind{ { @@ -312,7 +314,6 @@ func TestGetAndValidateListenerSupportedKinds(t *testing.T) { protocol: v1.TCPProtocolType, expectErr: false, name: "unsupported protocol is ignored", - kind: TCPRouteGroupKind, expected: []v1.RouteGroupKind{}, }, { @@ -336,35 +337,30 @@ func TestGetAndValidateListenerSupportedKinds(t *testing.T) { }, { protocol: v1.HTTPProtocolType, - kind: HTTPRouteGroupKind, + kind: []v1.RouteGroupKind{HTTPRouteGroupKind}, expectErr: false, name: "valid HTTP", - expected: HTTPRouteGroupKind, + expected: []v1.RouteGroupKind{HTTPRouteGroupKind}, }, { protocol: v1.HTTPSProtocolType, - kind: HTTPRouteGroupKind, + kind: []v1.RouteGroupKind{HTTPRouteGroupKind}, expectErr: false, name: "valid HTTPS", - expected: HTTPRouteGroupKind, + expected: []v1.RouteGroupKind{HTTPRouteGroupKind}, }, { protocol: v1.HTTPSProtocolType, expectErr: false, name: "valid HTTPS no kind specified", expected: []v1.RouteGroupKind{ - { - Kind: kinds.HTTPRoute, - }, + HTTPRouteGroupKind, GRPCRouteGroupKind, }, }, { protocol: v1.HTTPProtocolType, kind: []v1.RouteGroupKind{ - { - Kind: kinds.HTTPRoute, - Group: helpers.GetPointer[v1.Group](v1.GroupName), - }, + HTTPRouteGroupKind, { Kind: "bad-kind", Group: helpers.GetPointer[v1.Group](v1.GroupName), @@ -372,7 +368,7 @@ func TestGetAndValidateListenerSupportedKinds(t *testing.T) { }, expectErr: true, name: "valid and invalid kinds", - expected: HTTPRouteGroupKind, + expected: []v1.RouteGroupKind{HTTPRouteGroupKind}, }, } diff --git a/internal/mode/static/state/graph/gateway_test.go b/internal/mode/static/state/graph/gateway_test.go index 26f3f48c88..1d58cff1e6 100644 --- a/internal/mode/static/state/graph/gateway_test.go +++ b/internal/mode/static/state/graph/gateway_test.go @@ -347,6 +347,11 @@ func TestBuildGateway(t *testing.T) { Valid: false, } + supportedKindsForListeners := []v1.RouteGroupKind{ + {Kind: v1.Kind(kinds.HTTPRoute), Group: helpers.GetPointer[v1.Group](v1.GroupName)}, + {Kind: v1.Kind(kinds.GRPCRoute), Group: helpers.GetPointer[v1.Group](v1.GroupName)}, + } + tests := []struct { gateway *v1.Gateway gatewayClass *GatewayClass @@ -361,24 +366,20 @@ func TestBuildGateway(t *testing.T) { Source: getLastCreatedGetaway(), Listeners: []*Listener{ { - Name: "foo-80-1", - Source: foo80Listener1, - Valid: true, - Attachable: true, - Routes: map[RouteKey]*L7Route{}, - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + Name: "foo-80-1", + Source: foo80Listener1, + Valid: true, + Attachable: true, + Routes: map[RouteKey]*L7Route{}, + SupportedKinds: supportedKindsForListeners, }, { - Name: "foo-8080", - Source: foo8080Listener, - Valid: true, - Attachable: true, - Routes: map[RouteKey]*L7Route{}, - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + Name: "foo-8080", + Source: foo8080Listener, + Valid: true, + Attachable: true, + Routes: map[RouteKey]*L7Route{}, + SupportedKinds: supportedKindsForListeners, }, }, Valid: true, @@ -400,9 +401,7 @@ func TestBuildGateway(t *testing.T) { Attachable: true, Routes: map[RouteKey]*L7Route{}, ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + SupportedKinds: supportedKindsForListeners, }, { Name: "foo-8443-https", @@ -411,9 +410,7 @@ func TestBuildGateway(t *testing.T) { Attachable: true, Routes: map[RouteKey]*L7Route{}, ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + SupportedKinds: supportedKindsForListeners, }, }, Valid: true, @@ -479,9 +476,7 @@ func TestBuildGateway(t *testing.T) { Attachable: true, Routes: map[RouteKey]*L7Route{}, ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretDiffNamespace)), - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + SupportedKinds: supportedKindsForListeners, }, }, Valid: true, @@ -502,10 +497,8 @@ func TestBuildGateway(t *testing.T) { Conditions: staticConds.NewListenerRefNotPermitted( `Certificate ref to secret diff-ns/secret not permitted by any ReferenceGrant`, ), - Routes: map[RouteKey]*L7Route{}, - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + Routes: map[RouteKey]*L7Route{}, + SupportedKinds: supportedKindsForListeners, }, }, Valid: true, @@ -550,10 +543,8 @@ func TestBuildGateway(t *testing.T) { Conditions: staticConds.NewListenerUnsupportedProtocol( `protocol: Unsupported value: "TCP": supported values: "HTTP", "HTTPS"`, ), - Routes: map[RouteKey]*L7Route{}, - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + Routes: map[RouteKey]*L7Route{}, + SupportedKinds: []v1.RouteGroupKind{}, }, }, Valid: true, @@ -582,10 +573,8 @@ func TestBuildGateway(t *testing.T) { Conditions: staticConds.NewListenerUnsupportedValue( `port: Invalid value: 0: port must be between 1-65535`, ), - Routes: map[RouteKey]*L7Route{}, - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + Routes: map[RouteKey]*L7Route{}, + SupportedKinds: supportedKindsForListeners, }, { Name: "invalid-https-port", @@ -595,10 +584,8 @@ func TestBuildGateway(t *testing.T) { Conditions: staticConds.NewListenerUnsupportedValue( `port: Invalid value: 65536: port must be between 1-65535`, ), - Routes: map[RouteKey]*L7Route{}, - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + Routes: map[RouteKey]*L7Route{}, + SupportedKinds: supportedKindsForListeners, }, { Name: "invalid-protected-port", @@ -608,10 +595,8 @@ func TestBuildGateway(t *testing.T) { Conditions: staticConds.NewListenerUnsupportedValue( `port: Invalid value: 9113: port is already in use as MetricsPort`, ), - Routes: map[RouteKey]*L7Route{}, - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + Routes: map[RouteKey]*L7Route{}, + SupportedKinds: supportedKindsForListeners, }, }, Valid: true, @@ -627,24 +612,20 @@ func TestBuildGateway(t *testing.T) { Source: getLastCreatedGetaway(), Listeners: []*Listener{ { - Name: "invalid-hostname", - Source: invalidHostnameListener, - Valid: false, - Conditions: staticConds.NewListenerUnsupportedValue(invalidHostnameMsg), - Routes: map[RouteKey]*L7Route{}, - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + Name: "invalid-hostname", + Source: invalidHostnameListener, + Valid: false, + Conditions: staticConds.NewListenerUnsupportedValue(invalidHostnameMsg), + Routes: map[RouteKey]*L7Route{}, + SupportedKinds: supportedKindsForListeners, }, { - Name: "invalid-https-hostname", - Source: invalidHTTPSHostnameListener, - Valid: false, - Conditions: staticConds.NewListenerUnsupportedValue(invalidHostnameMsg), - Routes: map[RouteKey]*L7Route{}, - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + Name: "invalid-https-hostname", + Source: invalidHTTPSHostnameListener, + Valid: false, + Conditions: staticConds.NewListenerUnsupportedValue(invalidHostnameMsg), + Routes: map[RouteKey]*L7Route{}, + SupportedKinds: supportedKindsForListeners, }, }, Valid: true, @@ -666,9 +647,7 @@ func TestBuildGateway(t *testing.T) { Conditions: staticConds.NewListenerInvalidCertificateRef( `tls.certificateRefs[0]: Invalid value: test/does-not-exist: secret does not exist`, ), - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + SupportedKinds: supportedKindsForListeners, }, }, Valid: true, @@ -695,34 +674,28 @@ func TestBuildGateway(t *testing.T) { Source: getLastCreatedGetaway(), Listeners: []*Listener{ { - Name: "foo-80-1", - Source: foo80Listener1, - Valid: true, - Attachable: true, - Routes: map[RouteKey]*L7Route{}, - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + Name: "foo-80-1", + Source: foo80Listener1, + Valid: true, + Attachable: true, + Routes: map[RouteKey]*L7Route{}, + SupportedKinds: supportedKindsForListeners, }, { - Name: "foo-8080", - Source: foo8080Listener, - Valid: true, - Attachable: true, - Routes: map[RouteKey]*L7Route{}, - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + Name: "foo-8080", + Source: foo8080Listener, + Valid: true, + Attachable: true, + Routes: map[RouteKey]*L7Route{}, + SupportedKinds: supportedKindsForListeners, }, { - Name: "foo-8081", - Source: foo8081Listener, - Valid: true, - Attachable: true, - Routes: map[RouteKey]*L7Route{}, - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + Name: "foo-8081", + Source: foo8081Listener, + Valid: true, + Attachable: true, + Routes: map[RouteKey]*L7Route{}, + SupportedKinds: supportedKindsForListeners, }, { Name: "foo-443-https-1", @@ -731,9 +704,7 @@ func TestBuildGateway(t *testing.T) { Attachable: true, Routes: map[RouteKey]*L7Route{}, ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + SupportedKinds: supportedKindsForListeners, }, { Name: "foo-8443-https", @@ -742,19 +713,15 @@ func TestBuildGateway(t *testing.T) { Attachable: true, Routes: map[RouteKey]*L7Route{}, ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + SupportedKinds: supportedKindsForListeners, }, { - Name: "bar-80", - Source: bar80Listener, - Valid: true, - Attachable: true, - Routes: map[RouteKey]*L7Route{}, - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + Name: "bar-80", + Source: bar80Listener, + Valid: true, + Attachable: true, + Routes: map[RouteKey]*L7Route{}, + SupportedKinds: supportedKindsForListeners, }, { Name: "bar-443-https", @@ -763,9 +730,7 @@ func TestBuildGateway(t *testing.T) { Attachable: true, Routes: map[RouteKey]*L7Route{}, ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + SupportedKinds: supportedKindsForListeners, }, { Name: "bar-8443-https", @@ -774,9 +739,7 @@ func TestBuildGateway(t *testing.T) { Attachable: true, Routes: map[RouteKey]*L7Route{}, ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + SupportedKinds: supportedKindsForListeners, }, }, Valid: true, @@ -801,37 +764,31 @@ func TestBuildGateway(t *testing.T) { Source: getLastCreatedGetaway(), Listeners: []*Listener{ { - Name: "foo-80-1", - Source: foo80Listener1, - Valid: false, - Attachable: true, - Routes: map[RouteKey]*L7Route{}, - Conditions: staticConds.NewListenerProtocolConflict(conflict80PortMsg), - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + Name: "foo-80-1", + Source: foo80Listener1, + Valid: false, + Attachable: true, + Routes: map[RouteKey]*L7Route{}, + Conditions: staticConds.NewListenerProtocolConflict(conflict80PortMsg), + SupportedKinds: supportedKindsForListeners, }, { - Name: "bar-80", - Source: bar80Listener, - Valid: false, - Attachable: true, - Routes: map[RouteKey]*L7Route{}, - Conditions: staticConds.NewListenerProtocolConflict(conflict80PortMsg), - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + Name: "bar-80", + Source: bar80Listener, + Valid: false, + Attachable: true, + Routes: map[RouteKey]*L7Route{}, + Conditions: staticConds.NewListenerProtocolConflict(conflict80PortMsg), + SupportedKinds: supportedKindsForListeners, }, { - Name: "foo-443", - Source: foo443Listener, - Valid: false, - Attachable: true, - Routes: map[RouteKey]*L7Route{}, - Conditions: staticConds.NewListenerProtocolConflict(conflict443PortMsg), - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + Name: "foo-443", + Source: foo443Listener, + Valid: false, + Attachable: true, + Routes: map[RouteKey]*L7Route{}, + Conditions: staticConds.NewListenerProtocolConflict(conflict443PortMsg), + SupportedKinds: supportedKindsForListeners, }, { Name: "foo-80-https", @@ -841,9 +798,7 @@ func TestBuildGateway(t *testing.T) { Routes: map[RouteKey]*L7Route{}, Conditions: staticConds.NewListenerProtocolConflict(conflict80PortMsg), ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + SupportedKinds: supportedKindsForListeners, }, { Name: "foo-443-https-1", @@ -853,9 +808,7 @@ func TestBuildGateway(t *testing.T) { Routes: map[RouteKey]*L7Route{}, Conditions: staticConds.NewListenerProtocolConflict(conflict443PortMsg), ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + SupportedKinds: supportedKindsForListeners, }, { Name: "bar-443-https", @@ -865,9 +818,7 @@ func TestBuildGateway(t *testing.T) { Routes: map[RouteKey]*L7Route{}, Conditions: staticConds.NewListenerProtocolConflict(conflict443PortMsg), ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), - SupportedKinds: []v1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, + SupportedKinds: supportedKindsForListeners, }, }, Valid: true, diff --git a/internal/mode/static/state/graph/graph_test.go b/internal/mode/static/state/graph/graph_test.go index 2081fd881c..15d1afde17 100644 --- a/internal/mode/static/state/graph/graph_test.go +++ b/internal/mode/static/state/graph/graph_test.go @@ -534,6 +534,11 @@ func TestBuildGraph(t *testing.T) { }, } + supportedKindsForListeners := []gatewayv1.RouteGroupKind{ + {Kind: gatewayv1.Kind(kinds.HTTPRoute), Group: helpers.GetPointer[gatewayv1.Group](gatewayv1.GroupName)}, + {Kind: gatewayv1.Kind(kinds.GRPCRoute), Group: helpers.GetPointer[gatewayv1.Group](gatewayv1.GroupName)}, + } + createExpectedGraphWithGatewayClass := func(gc *gatewayv1.GatewayClass) *Graph { return &Graph{ GatewayClass: &GatewayClass{ @@ -553,7 +558,7 @@ func TestBuildGraph(t *testing.T) { CreateRouteKey(hr1): routeHR1, CreateRouteKey(gr): routeGR, }, - SupportedKinds: []gatewayv1.RouteGroupKind{{Kind: kinds.HTTPRoute}}, + SupportedKinds: supportedKindsForListeners, AllowedRouteLabelSelector: labels.SelectorFromSet(map[string]string{"app": "allowed"}), }, { @@ -563,7 +568,7 @@ func TestBuildGraph(t *testing.T) { Attachable: true, Routes: map[RouteKey]*L7Route{CreateRouteKey(hr3): routeHR3}, ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secret)), - SupportedKinds: []gatewayv1.RouteGroupKind{{Kind: kinds.HTTPRoute}}, + SupportedKinds: supportedKindsForListeners, }, }, Valid: true, diff --git a/internal/mode/static/state/graph/route_common.go b/internal/mode/static/state/graph/route_common.go index b1e299aa4a..20ca4682fc 100644 --- a/internal/mode/static/state/graph/route_common.go +++ b/internal/mode/static/state/graph/route_common.go @@ -538,21 +538,11 @@ func isRouteNamespaceAllowedByListener( // If the listener specifies allowed kinds, the route kind must be in the list. // If the listener does not specify allowedRoutes, allowed routes are determined using the listener protocol. func isRouteTypeAllowedByListener(listener *Listener, routeType RouteType) bool { - allowedRoutes := listener.Source.AllowedRoutes - if allowedRoutes != nil && allowedRoutes.Kinds != nil { - for _, kind := range allowedRoutes.Kinds { - if kind.Kind == convertRouteType(routeType) { - return true - } + for _, kind := range listener.SupportedKinds { + if kind.Kind == convertRouteType(routeType) { + return true } - return false } - - switch listener.Source.Protocol { - case v1.HTTPProtocolType, v1.HTTPSProtocolType: - return routeType == RouteTypeHTTP || routeType == RouteTypeGRPC - } - return false } diff --git a/internal/mode/static/state/graph/route_common_test.go b/internal/mode/static/state/graph/route_common_test.go index 181226a7c7..ceb150ffeb 100644 --- a/internal/mode/static/state/graph/route_common_test.go +++ b/internal/mode/static/state/graph/route_common_test.go @@ -233,6 +233,10 @@ func TestBindRouteToListeners(t *testing.T) { Valid: true, Attachable: true, Routes: map[RouteKey]*L7Route{}, + SupportedKinds: []gatewayv1.RouteGroupKind{ + {Kind: gatewayv1.Kind(kinds.HTTPRoute), Group: helpers.GetPointer[gatewayv1.Group](gatewayv1.GroupName)}, + {Kind: gatewayv1.Kind(kinds.GRPCRoute), Group: helpers.GetPointer[gatewayv1.Group](gatewayv1.GroupName)}, + }, } } createModifiedListener := func(name string, m func(*Listener)) *Listener { @@ -1124,10 +1128,8 @@ func TestBindRouteToListeners(t *testing.T) { Valid: true, Listeners: []*Listener{ createModifiedListener("listener-80-1", func(l *Listener) { - l.Source.AllowedRoutes = &gatewayv1.AllowedRoutes{ - Kinds: []gatewayv1.RouteGroupKind{ - {Kind: "HTTPRoute"}, - }, + l.SupportedKinds = []gatewayv1.RouteGroupKind{ + {Kind: gatewayv1.Kind(kinds.HTTPRoute), Group: helpers.GetPointer[gatewayv1.Group](gatewayv1.GroupName)}, } l.Routes = map[RouteKey]*L7Route{ CreateRouteKey(gr): getLastNormalGRPCRoute(), @@ -1149,10 +1151,8 @@ func TestBindRouteToListeners(t *testing.T) { }, expectedGatewayListeners: []*Listener{ createModifiedListener("listener-80-1", func(l *Listener) { - l.Source.AllowedRoutes = &gatewayv1.AllowedRoutes{ - Kinds: []gatewayv1.RouteGroupKind{ - {Kind: "HTTPRoute"}, - }, + l.SupportedKinds = []gatewayv1.RouteGroupKind{ + {Kind: gatewayv1.Kind(kinds.HTTPRoute), Group: helpers.GetPointer[gatewayv1.Group](gatewayv1.GroupName)}, } l.Routes = map[RouteKey]*L7Route{ CreateRouteKey(gr): getLastNormalGRPCRoute(), @@ -1742,46 +1742,12 @@ func TestAllowedRouteType(t *testing.T) { routeType RouteType expResult bool }{ - { - name: "httpRoute attaches to listener with protocol http", - routeType: RouteTypeHTTP, - listener: &Listener{ - Source: gatewayv1.Listener{ - Protocol: gatewayv1.HTTPProtocolType, - }, - }, - expResult: true, - }, - { - name: "grpcRoute attaches to listener with protocol https", - routeType: RouteTypeGRPC, - listener: &Listener{ - Source: gatewayv1.Listener{ - Protocol: gatewayv1.HTTPSProtocolType, - }, - }, - expResult: true, - }, - { - name: "grpcRoute is not allowed to attach to listener with tcp protocol", - routeType: RouteTypeGRPC, - listener: &Listener{ - Source: gatewayv1.Listener{ - Protocol: gatewayv1.TCPProtocolType, - }, - }, - expResult: false, - }, { name: "grpcRoute attaches to listener with allowedRoutes set to grpcRoute", routeType: RouteTypeGRPC, listener: &Listener{ - Source: gatewayv1.Listener{ - AllowedRoutes: &gatewayv1.AllowedRoutes{ - Kinds: []gatewayv1.RouteGroupKind{ - {Kind: kinds.GRPCRoute}, - }, - }, + SupportedKinds: []gatewayv1.RouteGroupKind{ + {Kind: kinds.GRPCRoute}, }, }, expResult: true, @@ -1790,13 +1756,9 @@ func TestAllowedRouteType(t *testing.T) { name: "grpcRoute attaches to listener with allowedRoutes set to grpcRoute and httpRoute", routeType: RouteTypeGRPC, listener: &Listener{ - Source: gatewayv1.Listener{ - AllowedRoutes: &gatewayv1.AllowedRoutes{ - Kinds: []gatewayv1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - {Kind: kinds.GRPCRoute}, - }, - }, + SupportedKinds: []gatewayv1.RouteGroupKind{ + {Kind: kinds.HTTPRoute}, + {Kind: kinds.GRPCRoute}, }, }, expResult: true, @@ -1805,12 +1767,8 @@ func TestAllowedRouteType(t *testing.T) { name: "grpcRoute not allowed to attach to listener with allowedRoutes set to httpRoute", routeType: RouteTypeGRPC, listener: &Listener{ - Source: gatewayv1.Listener{ - AllowedRoutes: &gatewayv1.AllowedRoutes{ - Kinds: []gatewayv1.RouteGroupKind{ - {Kind: kinds.HTTPRoute}, - }, - }, + SupportedKinds: []gatewayv1.RouteGroupKind{ + {Kind: kinds.HTTPRoute}, }, }, expResult: false, @@ -1819,26 +1777,8 @@ func TestAllowedRouteType(t *testing.T) { name: "httpRoute not allowed to attach to listener with allowedRoutes set to grpcRoute", routeType: RouteTypeHTTP, listener: &Listener{ - Source: gatewayv1.Listener{ - AllowedRoutes: &gatewayv1.AllowedRoutes{ - Kinds: []gatewayv1.RouteGroupKind{ - {Kind: kinds.GRPCRoute}, - }, - }, - }, - }, - expResult: false, - }, - { - name: "grpcRoute not allowed to attach to listener with allowedRoutes set to testRoute", - routeType: RouteTypeGRPC, - listener: &Listener{ - Source: gatewayv1.Listener{ - AllowedRoutes: &gatewayv1.AllowedRoutes{ - Kinds: []gatewayv1.RouteGroupKind{ - {Kind: gatewayv1.Kind("testRoute")}, - }, - }, + SupportedKinds: []gatewayv1.RouteGroupKind{ + {Kind: kinds.GRPCRoute}, }, }, expResult: false, From 38538dce3cd977956bea7c258184689ceeeb65d9 Mon Sep 17 00:00:00 2001 From: Saloni Date: Fri, 2 Aug 2024 14:06:20 -0600 Subject: [PATCH 7/7] fix test description --- internal/mode/static/state/graph/gateway_listener.go | 6 +++++- internal/mode/static/state/graph/gateway_listener_test.go | 2 +- internal/mode/static/state/graph/gateway_test.go | 3 +-- internal/mode/static/state/graph/route_common.go | 4 +--- internal/mode/static/state/graph/route_common_test.go | 8 ++++---- 5 files changed, 12 insertions(+), 11 deletions(-) diff --git a/internal/mode/static/state/graph/gateway_listener.go b/internal/mode/static/state/graph/gateway_listener.go index 94bceac2da..f1f4e6bd9f 100644 --- a/internal/mode/static/state/graph/gateway_listener.go +++ b/internal/mode/static/state/graph/gateway_listener.go @@ -225,12 +225,15 @@ func validateListenerHostname(listener v1.Listener) (conds []conditions.Conditio return nil, true } +// getAndValidateListenerSupportedKinds validates the route kind and returns the supported kinds for the listener. +// The supported kinds are determined based on the listener's allowedRoutes field. +// If the listener does not specify allowedRoutes, listener determines allowed routes based on its protocol. func getAndValidateListenerSupportedKinds(listener v1.Listener) ( []conditions.Condition, []v1.RouteGroupKind, ) { var conds []conditions.Condition - supportedKinds := make([]v1.RouteGroupKind, 0) + var supportedKinds []v1.RouteGroupKind validRouteKind := func(kind v1.RouteGroupKind) bool { if kind.Kind != v1.Kind(kinds.HTTPRoute) && kind.Kind != v1.Kind(kinds.GRPCRoute) { @@ -243,6 +246,7 @@ func getAndValidateListenerSupportedKinds(listener v1.Listener) ( } if listener.AllowedRoutes != nil && listener.AllowedRoutes.Kinds != nil { + supportedKinds = make([]v1.RouteGroupKind, 0, len(listener.AllowedRoutes.Kinds)) for _, kind := range listener.AllowedRoutes.Kinds { if !validRouteKind(kind) { msg := fmt.Sprintf("Unsupported route kind \"%s/%s\"", *kind.Group, kind.Kind) diff --git a/internal/mode/static/state/graph/gateway_listener_test.go b/internal/mode/static/state/graph/gateway_listener_test.go index 534c1eeeda..d60ad7321b 100644 --- a/internal/mode/static/state/graph/gateway_listener_test.go +++ b/internal/mode/static/state/graph/gateway_listener_test.go @@ -314,7 +314,7 @@ func TestGetAndValidateListenerSupportedKinds(t *testing.T) { protocol: v1.TCPProtocolType, expectErr: false, name: "unsupported protocol is ignored", - expected: []v1.RouteGroupKind{}, + expected: nil, }, { protocol: v1.HTTPProtocolType, diff --git a/internal/mode/static/state/graph/gateway_test.go b/internal/mode/static/state/graph/gateway_test.go index 1d58cff1e6..b942c4470f 100644 --- a/internal/mode/static/state/graph/gateway_test.go +++ b/internal/mode/static/state/graph/gateway_test.go @@ -543,8 +543,7 @@ func TestBuildGateway(t *testing.T) { Conditions: staticConds.NewListenerUnsupportedProtocol( `protocol: Unsupported value: "TCP": supported values: "HTTP", "HTTPS"`, ), - Routes: map[RouteKey]*L7Route{}, - SupportedKinds: []v1.RouteGroupKind{}, + Routes: map[RouteKey]*L7Route{}, }, }, Valid: true, diff --git a/internal/mode/static/state/graph/route_common.go b/internal/mode/static/state/graph/route_common.go index 20ca4682fc..f8fedd50d0 100644 --- a/internal/mode/static/state/graph/route_common.go +++ b/internal/mode/static/state/graph/route_common.go @@ -535,8 +535,6 @@ func isRouteNamespaceAllowedByListener( } // isRouteKindAllowedByListener checks if the route is allowed to attach to the listener. -// If the listener specifies allowed kinds, the route kind must be in the list. -// If the listener does not specify allowedRoutes, allowed routes are determined using the listener protocol. func isRouteTypeAllowedByListener(listener *Listener, routeType RouteType) bool { for _, kind := range listener.SupportedKinds { if kind.Kind == convertRouteType(routeType) { @@ -553,7 +551,7 @@ func convertRouteType(routeType RouteType) v1.Kind { case RouteTypeGRPC: return kinds.GRPCRoute default: - return "" + panic(fmt.Sprintf("unsupported route type: %s", routeType)) } } diff --git a/internal/mode/static/state/graph/route_common_test.go b/internal/mode/static/state/graph/route_common_test.go index ceb150ffeb..36b32318a5 100644 --- a/internal/mode/static/state/graph/route_common_test.go +++ b/internal/mode/static/state/graph/route_common_test.go @@ -1743,7 +1743,7 @@ func TestAllowedRouteType(t *testing.T) { expResult bool }{ { - name: "grpcRoute attaches to listener with allowedRoutes set to grpcRoute", + name: "grpcRoute is allowed when listener supports grpcRoute kind", routeType: RouteTypeGRPC, listener: &Listener{ SupportedKinds: []gatewayv1.RouteGroupKind{ @@ -1753,7 +1753,7 @@ func TestAllowedRouteType(t *testing.T) { expResult: true, }, { - name: "grpcRoute attaches to listener with allowedRoutes set to grpcRoute and httpRoute", + name: "grpcRoute is allowed when listener supports grpcRoute and httpRoute kind", routeType: RouteTypeGRPC, listener: &Listener{ SupportedKinds: []gatewayv1.RouteGroupKind{ @@ -1764,7 +1764,7 @@ func TestAllowedRouteType(t *testing.T) { expResult: true, }, { - name: "grpcRoute not allowed to attach to listener with allowedRoutes set to httpRoute", + name: "grpcRoute is allowed when listener supports httpRoute kind", routeType: RouteTypeGRPC, listener: &Listener{ SupportedKinds: []gatewayv1.RouteGroupKind{ @@ -1774,7 +1774,7 @@ func TestAllowedRouteType(t *testing.T) { expResult: false, }, { - name: "httpRoute not allowed to attach to listener with allowedRoutes set to grpcRoute", + name: "httpRoute not allowed when listener supports grpcRoute kind", routeType: RouteTypeHTTP, listener: &Listener{ SupportedKinds: []gatewayv1.RouteGroupKind{