From 92defd03abfdaa6b679e6a0d578540646a2c0303 Mon Sep 17 00:00:00 2001 From: Travis Raines <571832+rainest@users.noreply.github.com> Date: Fri, 8 Apr 2022 15:31:11 -0700 Subject: [PATCH] feat(gateway) confirm valid AllowedRoute Check that AllowedRoutes from user-provided Listeners meet the criteria necessary for Kong: Listeners for the same protocol must not specify different filter requirements because Kong combines all routes into a single proxy configuration. --- .../controllers/gateway/gateway_controller.go | 57 +++- .../gateway/gateway_controller_test.go | 266 +++++++++++++++++- 2 files changed, 310 insertions(+), 13 deletions(-) diff --git a/internal/controllers/gateway/gateway_controller.go b/internal/controllers/gateway/gateway_controller.go index 813457009fc..7c93f987cb5 100644 --- a/internal/controllers/gateway/gateway_controller.go +++ b/internal/controllers/gateway/gateway_controller.go @@ -311,11 +311,12 @@ func (r *GatewayReconciler) reconcileUnmanagedGateway(ctx context.Context, log l return ctrl.Result{}, err } - gatewayListeners, err = mergeAllowedRoutes(gateway.Spec.Listeners, gatewayListeners) - if err != nil { - return ctrl.Result{}, err + if !areAllowedRoutesConsistentByProtocol(gateway.Spec.Listeners) { + return ctrl.Result{}, fmt.Errorf("all listeners for a protocol must use the same AllowedRoutes") } + gatewayListeners = mergeAllowedRoutes(gateway.Spec.Listeners, gatewayListeners) + debug(log, gateway, "updating the gateway if any changes to listeners occurred") isChanged, err := r.updateAddressesAndListenersSpec(ctx, gateway, gatewayAddresses, gatewayListeners) if err != nil { @@ -570,21 +571,53 @@ func (r *GatewayReconciler) updateAddressesAndListenersStatus( return false, nil } -func mergeAllowedRoutes(source, target []gatewayv1alpha2.Listener) ([]gatewayv1alpha2.Listener, error) { +// mergeAllowedRoutes takes two sets of listeners, scans the source for AllowedRoutes filters, and copies those into +// listeners in the target set that share the same protocol as the source listener. As implemented, it makes no attempt +// to account for same-protocol listeners in the source set having different AllowedRoutes: it assumes +// areAllowedRoutesConsistentByProtocol has been run first because Kong would not support multiple listeners with +// different protocols anyway +func mergeAllowedRoutes(source, target []gatewayv1alpha2.Listener) []gatewayv1alpha2.Listener { var result []gatewayv1alpha2.Listener - mappings := make(map[string]gatewayv1alpha2.RouteNamespaces) + mappings := make(map[gatewayv1alpha2.ProtocolType]gatewayv1alpha2.RouteNamespaces) for _, listener := range source { - if _, exists := mappings[fmt.Sprintf("%s:%d", listener.Protocol, listener.Port)]; exists { - return nil, fmt.Errorf("multiple listeners for a single protocol/port combination are not supported: %s:%d", - listener.Protocol, listener.Port) - } - mappings[fmt.Sprintf("%s:%d", listener.Protocol, listener.Port)] = *listener.AllowedRoutes.Namespaces + mappings[listener.Protocol] = *listener.AllowedRoutes.Namespaces } for _, listener := range target { - if namespaces, exists := mappings[fmt.Sprintf("%s:%d", listener.Protocol, listener.Port)]; exists { + if namespaces, exists := mappings[listener.Protocol]; exists { + if listener.AllowedRoutes == nil { + listener.AllowedRoutes = &gatewayv1alpha2.AllowedRoutes{} + } listener.AllowedRoutes.Namespaces = &namespaces } result = append(result, listener) } - return result, nil + return result +} + +// areAllowedRoutesConsistentByProtocol returns an error if a set of listeners includes multiple listeners for the same +// protocol that do not use the same AllowedRoutes filters. Kong does not support limiting routes to a specific listen: +// all routes are always served on all listens compatible with their protocol. As such, while we can filter the routes +// we ingest, if we ingest routes from two incompatible filters, we will combine them into a single proxy configuration +// It may be possible to write a new Kong plugin that checks the inbound port/address to de facto apply listen-based +// filters in the future. +func areAllowedRoutesConsistentByProtocol(listeners []gatewayv1alpha2.Listener) bool { + allowedByProtocol := make(map[gatewayv1alpha2.ProtocolType]gatewayv1alpha2.AllowedRoutes) + for _, listener := range listeners { + var allowed gatewayv1alpha2.AllowedRoutes + var exists bool + if allowed, exists = allowedByProtocol[listener.Protocol]; !exists { + allowedByProtocol[listener.Protocol] = *listener.AllowedRoutes + } else { + if allowed.Namespaces.From != listener.AllowedRoutes.Namespaces.From { + return false + } + if *allowed.Namespaces.From == gatewayv1alpha2.NamespacesFromSelector { + if !reflect.DeepEqual(allowed.Namespaces.Selector.MatchLabels, + listener.AllowedRoutes.Namespaces.Selector.MatchLabels) { + return false + } + } + } + } + return true } diff --git a/internal/controllers/gateway/gateway_controller_test.go b/internal/controllers/gateway/gateway_controller_test.go index ade6d81c1dc..1f8b50104e8 100644 --- a/internal/controllers/gateway/gateway_controller_test.go +++ b/internal/controllers/gateway/gateway_controller_test.go @@ -754,6 +754,270 @@ func Test_areListenersEqual(t *testing.T) { } for _, input := range inputs { - assert.Equal(t, input.expected, areListenersEqual(input.l1, input.l2), input.message, input.l1, input.l2) + assert.Equal(t, input.expected, areListenersEqual(input.l1, input.l2), input.message) + } +} + +func Test_mergeAllowedRoutes(t *testing.T) { + same := gatewayv1alpha2.NamespacesFromSame + all := gatewayv1alpha2.NamespacesFromAll + inputs := []struct { + message string + expected []gatewayv1alpha2.Listener + l1 []gatewayv1alpha2.Listener + l2 []gatewayv1alpha2.Listener + }{ + { + message: "empty", + expected: []gatewayv1alpha2.Listener(nil), // TODO what the go-isms here, shouldn't this be empty array, not whatever this is? + l1: []gatewayv1alpha2.Listener{}, + l2: []gatewayv1alpha2.Listener{}, + }, + { + message: "single", + expected: []gatewayv1alpha2.Listener{ + { + Protocol: gatewayv1alpha2.UDPProtocolType, + AllowedRoutes: &gatewayv1alpha2.AllowedRoutes{ + Namespaces: &gatewayv1alpha2.RouteNamespaces{ + From: &same, + }, + }, + }, + }, + l1: []gatewayv1alpha2.Listener{ + { + Protocol: gatewayv1alpha2.UDPProtocolType, + AllowedRoutes: &gatewayv1alpha2.AllowedRoutes{ + Namespaces: &gatewayv1alpha2.RouteNamespaces{ + From: &same, + }, + }, + }, + }, + l2: []gatewayv1alpha2.Listener{ + { + Protocol: gatewayv1alpha2.UDPProtocolType, + }, + }, + }, + { + message: "multiple", + expected: []gatewayv1alpha2.Listener{ + { + Name: "one", + Protocol: gatewayv1alpha2.UDPProtocolType, + AllowedRoutes: &gatewayv1alpha2.AllowedRoutes{ + Namespaces: &gatewayv1alpha2.RouteNamespaces{ + From: &same, + }, + }, + }, + { + Name: "two", + Protocol: gatewayv1alpha2.UDPProtocolType, + AllowedRoutes: &gatewayv1alpha2.AllowedRoutes{ + Namespaces: &gatewayv1alpha2.RouteNamespaces{ + From: &same, + }, + }, + }, + { + Name: "three", + Protocol: gatewayv1alpha2.TCPProtocolType, + AllowedRoutes: &gatewayv1alpha2.AllowedRoutes{ + Namespaces: &gatewayv1alpha2.RouteNamespaces{ + From: &all, + }, + }, + }, + }, + l1: []gatewayv1alpha2.Listener{ + { + Protocol: gatewayv1alpha2.UDPProtocolType, + AllowedRoutes: &gatewayv1alpha2.AllowedRoutes{ + Namespaces: &gatewayv1alpha2.RouteNamespaces{ + From: &same, + }, + }, + }, + { + Protocol: gatewayv1alpha2.TCPProtocolType, + AllowedRoutes: &gatewayv1alpha2.AllowedRoutes{ + Namespaces: &gatewayv1alpha2.RouteNamespaces{ + From: &all, + }, + }, + }, + }, + l2: []gatewayv1alpha2.Listener{ + { + Name: "one", + Protocol: gatewayv1alpha2.UDPProtocolType, + }, + { + Name: "two", + Protocol: gatewayv1alpha2.UDPProtocolType, + }, + { + Name: "three", + Protocol: gatewayv1alpha2.TCPProtocolType, + }, + }, + }, + } + for _, input := range inputs { + assert.Equal(t, input.expected, mergeAllowedRoutes(input.l1, input.l2), input.message) + } +} + +func Test_areAllowedRoutesConsistentByProtocol(t *testing.T) { + same := gatewayv1alpha2.NamespacesFromSame + all := gatewayv1alpha2.NamespacesFromAll + selector := gatewayv1alpha2.NamespacesFromSelector + + inputs := []struct { + expected bool + message string + l []gatewayv1alpha2.Listener + }{ + { + expected: true, + message: "empty", + l: []gatewayv1alpha2.Listener{}, + }, + { + expected: true, + message: "no intersect", + l: []gatewayv1alpha2.Listener{ + { + Protocol: gatewayv1alpha2.UDPProtocolType, + AllowedRoutes: &gatewayv1alpha2.AllowedRoutes{ + Namespaces: &gatewayv1alpha2.RouteNamespaces{ + From: &same, + }, + }, + }, + { + Protocol: gatewayv1alpha2.TCPProtocolType, + AllowedRoutes: &gatewayv1alpha2.AllowedRoutes{ + Namespaces: &gatewayv1alpha2.RouteNamespaces{ + From: &all, + }, + }, + }, + }, + }, + { + expected: true, + message: "same allowed for each listener with same protocol", + l: []gatewayv1alpha2.Listener{ + { + Protocol: gatewayv1alpha2.UDPProtocolType, + AllowedRoutes: &gatewayv1alpha2.AllowedRoutes{ + Namespaces: &gatewayv1alpha2.RouteNamespaces{ + From: &same, + }, + }, + }, + { + Protocol: gatewayv1alpha2.UDPProtocolType, + AllowedRoutes: &gatewayv1alpha2.AllowedRoutes{ + Namespaces: &gatewayv1alpha2.RouteNamespaces{ + From: &same, + }, + }, + }, + { + Protocol: gatewayv1alpha2.TCPProtocolType, + AllowedRoutes: &gatewayv1alpha2.AllowedRoutes{ + Namespaces: &gatewayv1alpha2.RouteNamespaces{ + From: &all, + }, + }, + }, + }, + }, + { + expected: false, + message: "different allowed for listeners with same protocol", + l: []gatewayv1alpha2.Listener{ + { + Protocol: gatewayv1alpha2.UDPProtocolType, + AllowedRoutes: &gatewayv1alpha2.AllowedRoutes{ + Namespaces: &gatewayv1alpha2.RouteNamespaces{ + From: &same, + }, + }, + }, + { + Protocol: gatewayv1alpha2.UDPProtocolType, + AllowedRoutes: &gatewayv1alpha2.AllowedRoutes{ + Namespaces: &gatewayv1alpha2.RouteNamespaces{ + From: &all, + }, + }, + }, + }, + }, + { + expected: true, + message: "same selector", + l: []gatewayv1alpha2.Listener{ + { + Protocol: gatewayv1alpha2.UDPProtocolType, + AllowedRoutes: &gatewayv1alpha2.AllowedRoutes{ + Namespaces: &gatewayv1alpha2.RouteNamespaces{ + From: &selector, + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"key": "value"}, + }, + }, + }, + }, + { + Protocol: gatewayv1alpha2.UDPProtocolType, + AllowedRoutes: &gatewayv1alpha2.AllowedRoutes{ + Namespaces: &gatewayv1alpha2.RouteNamespaces{ + From: &selector, + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"key": "value"}, + }, + }, + }, + }, + }, + }, + { + expected: true, + message: "different selector", + l: []gatewayv1alpha2.Listener{ + { + Protocol: gatewayv1alpha2.UDPProtocolType, + AllowedRoutes: &gatewayv1alpha2.AllowedRoutes{ + Namespaces: &gatewayv1alpha2.RouteNamespaces{ + From: &selector, + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"key": "value"}, + }, + }, + }, + }, + { + Protocol: gatewayv1alpha2.UDPProtocolType, + AllowedRoutes: &gatewayv1alpha2.AllowedRoutes{ + Namespaces: &gatewayv1alpha2.RouteNamespaces{ + From: &selector, + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"key": "notvalue"}, + }, + }, + }, + }, + }, + }, + } + for _, input := range inputs { + assert.Equal(t, input.expected, areAllowedRoutesConsistentByProtocol(input.l), input.message) } }