From edfaa6220330b7421c6d63d2b85d35e155b52391 Mon Sep 17 00:00:00 2001 From: Zachary Nixon Date: Wed, 13 Aug 2025 15:32:58 -0700 Subject: [PATCH 1/3] small refactor to stop passing around route reconciler --- controllers/gateway/gateway_controller.go | 16 +++++++--------- main.go | 6 +----- .../routeutils/listener_attachment_helper.go | 4 ++-- pkg/gateway/routeutils/loader.go | 18 ++++++++++-------- pkg/gateway/routeutils/loader_test.go | 6 +++--- .../routeutils/route_listener_mapper.go | 4 ++-- .../routeutils/route_listener_mapper_test.go | 2 +- .../routeutils/route_reconciler_utils.go | 6 +++++- 8 files changed, 31 insertions(+), 31 deletions(-) diff --git a/controllers/gateway/gateway_controller.go b/controllers/gateway/gateway_controller.go index 8b91934e4b..b5ee15901e 100644 --- a/controllers/gateway/gateway_controller.go +++ b/controllers/gateway/gateway_controller.go @@ -51,13 +51,13 @@ const ( var _ Reconciler = &gatewayReconciler{} // NewNLBGatewayReconciler constructs a gateway reconciler to handle specifically for NLB gateways -func NewNLBGatewayReconciler(routeLoader routeutils.Loader, referenceCounter referencecounter.ServiceReferenceCounter, cloud services.Cloud, k8sClient client.Client, eventRecorder record.EventRecorder, controllerConfig config.ControllerConfig, finalizerManager k8s.FinalizerManager, networkingManager networking.NetworkingManager, networkingSGReconciler networking.SecurityGroupReconciler, networkingSGManager networking.SecurityGroupManager, elbv2TaggingManager elbv2deploy.TaggingManager, subnetResolver networking.SubnetsResolver, vpcInfoProvider networking.VPCInfoProvider, backendSGProvider networking.BackendSGProvider, sgResolver networking.SecurityGroupResolver, logger logr.Logger, metricsCollector lbcmetrics.MetricCollector, reconcileCounters *metricsutil.ReconcileCounters, routeReconciler routeutils.RouteReconciler) Reconciler { - return newGatewayReconciler(constants.NLBGatewayController, elbv2model.LoadBalancerTypeNetwork, controllerConfig.NLBGatewayMaxConcurrentReconciles, constants.NLBGatewayTagPrefix, shared_constants.NLBGatewayFinalizer, routeLoader, referenceCounter, routeutils.L4RouteFilter, cloud, k8sClient, eventRecorder, controllerConfig, finalizerManager, networkingSGReconciler, networkingManager, networkingSGManager, elbv2TaggingManager, subnetResolver, vpcInfoProvider, backendSGProvider, sgResolver, nlbAddons, logger, metricsCollector, reconcileCounters.IncrementNLBGateway, routeReconciler) +func NewNLBGatewayReconciler(routeLoader routeutils.Loader, referenceCounter referencecounter.ServiceReferenceCounter, cloud services.Cloud, k8sClient client.Client, eventRecorder record.EventRecorder, controllerConfig config.ControllerConfig, finalizerManager k8s.FinalizerManager, networkingManager networking.NetworkingManager, networkingSGReconciler networking.SecurityGroupReconciler, networkingSGManager networking.SecurityGroupManager, elbv2TaggingManager elbv2deploy.TaggingManager, subnetResolver networking.SubnetsResolver, vpcInfoProvider networking.VPCInfoProvider, backendSGProvider networking.BackendSGProvider, sgResolver networking.SecurityGroupResolver, logger logr.Logger, metricsCollector lbcmetrics.MetricCollector, reconcileCounters *metricsutil.ReconcileCounters) Reconciler { + return newGatewayReconciler(constants.NLBGatewayController, elbv2model.LoadBalancerTypeNetwork, controllerConfig.NLBGatewayMaxConcurrentReconciles, constants.NLBGatewayTagPrefix, shared_constants.NLBGatewayFinalizer, routeLoader, referenceCounter, routeutils.L4RouteFilter, cloud, k8sClient, eventRecorder, controllerConfig, finalizerManager, networkingSGReconciler, networkingManager, networkingSGManager, elbv2TaggingManager, subnetResolver, vpcInfoProvider, backendSGProvider, sgResolver, nlbAddons, logger, metricsCollector, reconcileCounters.IncrementNLBGateway) } // NewALBGatewayReconciler constructs a gateway reconciler to handle specifically for ALB gateways -func NewALBGatewayReconciler(routeLoader routeutils.Loader, cloud services.Cloud, k8sClient client.Client, referenceCounter referencecounter.ServiceReferenceCounter, eventRecorder record.EventRecorder, controllerConfig config.ControllerConfig, finalizerManager k8s.FinalizerManager, networkingManager networking.NetworkingManager, networkingSGReconciler networking.SecurityGroupReconciler, networkingSGManager networking.SecurityGroupManager, elbv2TaggingManager elbv2deploy.TaggingManager, subnetResolver networking.SubnetsResolver, vpcInfoProvider networking.VPCInfoProvider, backendSGProvider networking.BackendSGProvider, sgResolver networking.SecurityGroupResolver, logger logr.Logger, metricsCollector lbcmetrics.MetricCollector, reconcileCounters *metricsutil.ReconcileCounters, routeReconciler routeutils.RouteReconciler) Reconciler { - return newGatewayReconciler(constants.ALBGatewayController, elbv2model.LoadBalancerTypeApplication, controllerConfig.ALBGatewayMaxConcurrentReconciles, constants.ALBGatewayTagPrefix, shared_constants.ALBGatewayFinalizer, routeLoader, referenceCounter, routeutils.L7RouteFilter, cloud, k8sClient, eventRecorder, controllerConfig, finalizerManager, networkingSGReconciler, networkingManager, networkingSGManager, elbv2TaggingManager, subnetResolver, vpcInfoProvider, backendSGProvider, sgResolver, albAddons, logger, metricsCollector, reconcileCounters.IncrementALBGateway, routeReconciler) +func NewALBGatewayReconciler(routeLoader routeutils.Loader, cloud services.Cloud, k8sClient client.Client, referenceCounter referencecounter.ServiceReferenceCounter, eventRecorder record.EventRecorder, controllerConfig config.ControllerConfig, finalizerManager k8s.FinalizerManager, networkingManager networking.NetworkingManager, networkingSGReconciler networking.SecurityGroupReconciler, networkingSGManager networking.SecurityGroupManager, elbv2TaggingManager elbv2deploy.TaggingManager, subnetResolver networking.SubnetsResolver, vpcInfoProvider networking.VPCInfoProvider, backendSGProvider networking.BackendSGProvider, sgResolver networking.SecurityGroupResolver, logger logr.Logger, metricsCollector lbcmetrics.MetricCollector, reconcileCounters *metricsutil.ReconcileCounters) Reconciler { + return newGatewayReconciler(constants.ALBGatewayController, elbv2model.LoadBalancerTypeApplication, controllerConfig.ALBGatewayMaxConcurrentReconciles, constants.ALBGatewayTagPrefix, shared_constants.ALBGatewayFinalizer, routeLoader, referenceCounter, routeutils.L7RouteFilter, cloud, k8sClient, eventRecorder, controllerConfig, finalizerManager, networkingSGReconciler, networkingManager, networkingSGManager, elbv2TaggingManager, subnetResolver, vpcInfoProvider, backendSGProvider, sgResolver, albAddons, logger, metricsCollector, reconcileCounters.IncrementALBGateway) } // newGatewayReconciler constructs a reconciler that responds to gateway object changes @@ -68,7 +68,7 @@ func newGatewayReconciler(controllerName string, lbType elbv2model.LoadBalancerT networkingManager networking.NetworkingManager, networkingSGManager networking.SecurityGroupManager, elbv2TaggingManager elbv2deploy.TaggingManager, subnetResolver networking.SubnetsResolver, vpcInfoProvider networking.VPCInfoProvider, backendSGProvider networking.BackendSGProvider, sgResolver networking.SecurityGroupResolver, supportedAddons []addon.Addon, logger logr.Logger, metricsCollector lbcmetrics.MetricCollector, - reconcileTracker func(namespaceName types.NamespacedName), routeReconciler routeutils.RouteReconciler) Reconciler { + reconcileTracker func(namespaceName types.NamespacedName)) Reconciler { trackingProvider := tracking.NewDefaultProvider(gatewayTagPrefix, controllerConfig.ClusterName) modelBuilder := gatewaymodel.NewModelBuilder(subnetResolver, vpcInfoProvider, cloud.VpcID(), lbType, trackingProvider, elbv2TaggingManager, controllerConfig, cloud.EC2(), cloud.ELBV2(), cloud.ACM(), controllerConfig.FeatureGates, controllerConfig.ClusterName, controllerConfig.DefaultTags, sets.New(controllerConfig.ExternalManagedTags...), controllerConfig.DefaultSSLPolicy, controllerConfig.DefaultTargetType, controllerConfig.DefaultLoadBalancerScheme, backendSGProvider, sgResolver, controllerConfig.EnableBackendSecurityGroup, controllerConfig.DisableRestrictedSGRules, controllerConfig.IngressConfig.AllowedCertificateAuthorityARNs, supportedAddons, logger) @@ -96,7 +96,6 @@ func newGatewayReconciler(controllerName string, lbType elbv2model.LoadBalancerT metricsCollector: metricsCollector, reconcileTracker: reconcileTracker, cfgResolver: cfgResolver, - routeReconciler: routeReconciler, serviceReferenceCounter: serviceReferenceCounter, gatewayConditionUpdater: prepareGatewayConditionUpdate, } @@ -123,8 +122,7 @@ type gatewayReconciler struct { serviceReferenceCounter referencecounter.ServiceReferenceCounter gatewayConditionUpdater func(gw *gwv1.Gateway, targetConditionType string, newStatus metav1.ConditionStatus, reason string, message string) bool - cfgResolver gatewayConfigResolver - routeReconciler routeutils.RouteReconciler + cfgResolver gatewayConfigResolver } //+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=referencegrants,verbs=get;list;watch;patch @@ -211,7 +209,7 @@ func (r *gatewayReconciler) reconcileHelper(ctx context.Context, req reconcile.R return err } - allRoutes, err := r.gatewayLoader.LoadRoutesForGateway(ctx, *gw, r.routeFilter, r.routeReconciler) + allRoutes, err := r.gatewayLoader.LoadRoutesForGateway(ctx, *gw, r.routeFilter) if err != nil { var loaderErr routeutils.LoaderError diff --git a/main.go b/main.go index 306440338b..685ce1cbb9 100644 --- a/main.go +++ b/main.go @@ -103,7 +103,6 @@ type gatewayControllerConfig struct { sgResolver networking.SecurityGroupResolver metricsCollector lbcmetrics.MetricCollector reconcileCounters *metricsutil.ReconcileCounters - routeReconciler routeutils.RouteReconciler serviceReferenceCounter referencecounter.ServiceReferenceCounter networkingManager networking.NetworkingManager } @@ -243,7 +242,6 @@ func main() { sgResolver: sgResolver, metricsCollector: lbcMetricsCollector, reconcileCounters: reconcileCounters, - routeReconciler: routeReconciler, networkingManager: networkingManager, serviceReferenceCounter: serviceReferenceCounter, } @@ -251,7 +249,7 @@ func main() { enabledControllers := sets.Set[string]{} routeLoaderCreator := sync.OnceValue(func() routeutils.Loader { - return routeutils.NewLoader(mgr.GetClient(), mgr.GetLogger().WithName("gateway-route-loader")) + return routeutils.NewLoader(mgr.GetClient(), routeReconciler, mgr.GetLogger().WithName("gateway-route-loader")) }) // Setup NLB Gateway controller if enabled @@ -450,7 +448,6 @@ func setupGatewayController(ctx context.Context, mgr ctrl.Manager, cfg *gatewayC logger, cfg.metricsCollector, cfg.reconcileCounters, - cfg.routeReconciler, ) case gateway_constants.ALBGatewayController: reconciler = gateway.NewALBGatewayReconciler( @@ -472,7 +469,6 @@ func setupGatewayController(ctx context.Context, mgr ctrl.Manager, cfg *gatewayC logger, cfg.metricsCollector, cfg.reconcileCounters, - cfg.routeReconciler, ) default: return fmt.Errorf("unknown controller type: %s", controllerType) diff --git a/pkg/gateway/routeutils/listener_attachment_helper.go b/pkg/gateway/routeutils/listener_attachment_helper.go index 491a587eca..c33f104785 100644 --- a/pkg/gateway/routeutils/listener_attachment_helper.go +++ b/pkg/gateway/routeutils/listener_attachment_helper.go @@ -14,7 +14,7 @@ import ( // listenerAttachmentHelper is an internal utility interface that can be used to determine if a listener will allow // a route to attach to it. type listenerAttachmentHelper interface { - listenerAllowsAttachment(ctx context.Context, gw gwv1.Gateway, listener gwv1.Listener, route preLoadRouteDescriptor, deferredRouteReconciler RouteReconciler, hostnamesFromHttpRoutes map[types.NamespacedName][]gwv1.Hostname, hostnamesFromGrpcRoutes map[types.NamespacedName][]gwv1.Hostname) (bool, error) + listenerAllowsAttachment(ctx context.Context, gw gwv1.Gateway, listener gwv1.Listener, route preLoadRouteDescriptor, deferredRouteReconciler RouteReconcilerSubmitter, hostnamesFromHttpRoutes map[types.NamespacedName][]gwv1.Hostname, hostnamesFromGrpcRoutes map[types.NamespacedName][]gwv1.Hostname) (bool, error) } var _ listenerAttachmentHelper = &listenerAttachmentHelperImpl{} @@ -34,7 +34,7 @@ func newListenerAttachmentHelper(k8sClient client.Client, logger logr.Logger) li // listenerAllowsAttachment utility method to determine if a listener will allow a route to connect using // Gateway API rules to determine compatibility between lister and route. -func (attachmentHelper *listenerAttachmentHelperImpl) listenerAllowsAttachment(ctx context.Context, gw gwv1.Gateway, listener gwv1.Listener, route preLoadRouteDescriptor, deferredRouteReconciler RouteReconciler, hostnamesFromHttpRoutes map[types.NamespacedName][]gwv1.Hostname, hostnamesFromGrpcRoutes map[types.NamespacedName][]gwv1.Hostname) (bool, error) { +func (attachmentHelper *listenerAttachmentHelperImpl) listenerAllowsAttachment(ctx context.Context, gw gwv1.Gateway, listener gwv1.Listener, route preLoadRouteDescriptor, deferredRouteReconciler RouteReconcilerSubmitter, hostnamesFromHttpRoutes map[types.NamespacedName][]gwv1.Hostname, hostnamesFromGrpcRoutes map[types.NamespacedName][]gwv1.Hostname) (bool, error) { // check namespace namespaceOK, err := attachmentHelper.namespaceCheck(ctx, gw, listener, route) if err != nil { diff --git a/pkg/gateway/routeutils/loader.go b/pkg/gateway/routeutils/loader.go index e8fc688994..52235eeaef 100644 --- a/pkg/gateway/routeutils/loader.go +++ b/pkg/gateway/routeutils/loader.go @@ -48,21 +48,23 @@ var L7RouteFilter LoadRouteFilter = &routeFilterImpl{ // Loader will load all data Kubernetes that are pertinent to a gateway (Routes, Services, Target Group Configurations). // It will output the data using a map which maps listener port to the various routing rules for that port. type Loader interface { - LoadRoutesForGateway(ctx context.Context, gw gwv1.Gateway, filter LoadRouteFilter, reconciler RouteReconciler) (map[int32][]RouteDescriptor, error) + LoadRoutesForGateway(ctx context.Context, gw gwv1.Gateway, filter LoadRouteFilter) (map[int32][]RouteDescriptor, error) } var _ Loader = &loaderImpl{} type loaderImpl struct { mapper listenerToRouteMapper + routeSubmitter RouteReconcilerSubmitter k8sClient client.Client logger logr.Logger allRouteLoaders map[RouteKind]func(context context.Context, client client.Client, opts ...client.ListOption) ([]preLoadRouteDescriptor, error) } -func NewLoader(k8sClient client.Client, logger logr.Logger) Loader { +func NewLoader(k8sClient client.Client, routeSubmitter RouteReconcilerSubmitter, logger logr.Logger) Loader { return &loaderImpl{ mapper: newListenerToRouteMapper(k8sClient, logger.WithName("route-mapper")), + routeSubmitter: routeSubmitter, k8sClient: k8sClient, allRouteLoaders: allRoutes, logger: logger, @@ -70,7 +72,7 @@ func NewLoader(k8sClient client.Client, logger logr.Logger) Loader { } // LoadRoutesForGateway loads all relevant data for a single Gateway. -func (l *loaderImpl) LoadRoutesForGateway(ctx context.Context, gw gwv1.Gateway, filter LoadRouteFilter, deferredRouteReconciler RouteReconciler) (map[int32][]RouteDescriptor, error) { +func (l *loaderImpl) LoadRoutesForGateway(ctx context.Context, gw gwv1.Gateway, filter LoadRouteFilter) (map[int32][]RouteDescriptor, error) { // 1. Load all relevant routes according to the filter loadedRoutes := make([]preLoadRouteDescriptor, 0) @@ -88,13 +90,13 @@ func (l *loaderImpl) LoadRoutesForGateway(ctx context.Context, gw gwv1.Gateway, // 2. Remove routes that aren't granted attachment by the listener. // Map any routes that are granted attachment to the listener port that allows the attachment. - mappedRoutes, err := l.mapper.mapGatewayAndRoutes(ctx, gw, loadedRoutes, deferredRouteReconciler) + mappedRoutes, err := l.mapper.mapGatewayAndRoutes(ctx, gw, loadedRoutes, l.routeSubmitter) if err != nil { return nil, err } // 3. Load the underlying resource(s) for each route that is configured. - loadedRoute, err := l.loadChildResources(ctx, mappedRoutes, deferredRouteReconciler, gw) + loadedRoute, err := l.loadChildResources(ctx, mappedRoutes, gw) if err != nil { return nil, err } @@ -102,7 +104,7 @@ func (l *loaderImpl) LoadRoutesForGateway(ctx context.Context, gw gwv1.Gateway, // update status for accepted routes for _, routeList := range loadedRoute { for _, route := range routeList { - deferredRouteReconciler.Enqueue( + l.routeSubmitter.Enqueue( GenerateRouteData(true, true, string(gwv1.RouteConditionAccepted), RouteStatusInfoAcceptedMessage, route.GetRouteNamespacedName(), route.GetRouteKind(), route.GetRouteGeneration(), gw), ) } @@ -111,7 +113,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, deferredRouteReconciler RouteReconciler, gw gwv1.Gateway) (map[int32][]RouteDescriptor, error) { +func (l *loaderImpl) loadChildResources(ctx context.Context, preloadedRoutes map[int][]preLoadRouteDescriptor, gw gwv1.Gateway) (map[int32][]RouteDescriptor, error) { // Cache to reduce duplicate route lookups. // Kind -> [NamespacedName:Previously Loaded Descriptor] resourceCache := make(map[string]RouteDescriptor) @@ -135,7 +137,7 @@ func (l *loaderImpl) loadChildResources(ctx context.Context, preloadedRoutes map for _, lare := range loadAttachedRulesErrors { var loaderErr LoaderError if errors.As(lare.Err, &loaderErr) { - deferredRouteReconciler.Enqueue( + l.routeSubmitter.Enqueue( GenerateRouteData(false, false, string(loaderErr.GetRouteReason()), loaderErr.GetRouteMessage(), preloadedRoute.GetRouteNamespacedName(), preloadedRoute.GetRouteKind(), preloadedRoute.GetRouteGeneration(), gw), ) } diff --git a/pkg/gateway/routeutils/loader_test.go b/pkg/gateway/routeutils/loader_test.go index 23f8828f8b..0a4e256b76 100644 --- a/pkg/gateway/routeutils/loader_test.go +++ b/pkg/gateway/routeutils/loader_test.go @@ -18,7 +18,7 @@ type mockMapper struct { mapToReturn map[int][]preLoadRouteDescriptor } -func (m *mockMapper) mapGatewayAndRoutes(context context.Context, gw gwv1.Gateway, routes []preLoadRouteDescriptor, routeReconciler RouteReconciler) (map[int][]preLoadRouteDescriptor, error) { +func (m *mockMapper) mapGatewayAndRoutes(context context.Context, gw gwv1.Gateway, routes []preLoadRouteDescriptor, routeReconciler RouteReconcilerSubmitter) (map[int][]preLoadRouteDescriptor, error) { assert.ElementsMatch(m.t, m.expectedRoutes, routes) return m.mapToReturn, nil } @@ -224,11 +224,11 @@ func TestLoadRoutesForGateway(t *testing.T) { }, allRouteLoaders: allRouteLoaders, logger: logr.Discard(), + routeSubmitter: NewMockRouteReconciler(), } filter := &routeFilterImpl{acceptedKinds: tc.acceptedKinds} - mockReconciler := NewMockRouteReconciler() - result, err := loader.LoadRoutesForGateway(context.Background(), gwv1.Gateway{}, filter, mockReconciler) + result, err := loader.LoadRoutesForGateway(context.Background(), gwv1.Gateway{}, filter) if tc.expectError { assert.Error(t, err) return diff --git a/pkg/gateway/routeutils/route_listener_mapper.go b/pkg/gateway/routeutils/route_listener_mapper.go index c2bde2782a..8117873718 100644 --- a/pkg/gateway/routeutils/route_listener_mapper.go +++ b/pkg/gateway/routeutils/route_listener_mapper.go @@ -11,7 +11,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, deferredRouteReconciler RouteReconciler) (map[int][]preLoadRouteDescriptor, error) + mapGatewayAndRoutes(context context.Context, gw gwv1.Gateway, routes []preLoadRouteDescriptor, deferredRouteReconciler RouteReconcilerSubmitter) (map[int][]preLoadRouteDescriptor, error) } var _ listenerToRouteMapper = &listenerToRouteMapperImpl{} @@ -31,7 +31,7 @@ func newListenerToRouteMapper(k8sClient client.Client, logger logr.Logger) liste } // mapGatewayAndRoutes will map route to the corresponding listener ports using the Gateway API spec rules. -func (ltr *listenerToRouteMapperImpl) mapGatewayAndRoutes(ctx context.Context, gw gwv1.Gateway, routes []preLoadRouteDescriptor, deferredRouteReconciler RouteReconciler) (map[int][]preLoadRouteDescriptor, error) { +func (ltr *listenerToRouteMapperImpl) mapGatewayAndRoutes(ctx context.Context, gw gwv1.Gateway, routes []preLoadRouteDescriptor, deferredRouteReconciler RouteReconcilerSubmitter) (map[int][]preLoadRouteDescriptor, error) { result := make(map[int][]preLoadRouteDescriptor) // First filter out any routes that are not intended for this Gateway. diff --git a/pkg/gateway/routeutils/route_listener_mapper_test.go b/pkg/gateway/routeutils/route_listener_mapper_test.go index 8641e3e974..f4b895a2d3 100644 --- a/pkg/gateway/routeutils/route_listener_mapper_test.go +++ b/pkg/gateway/routeutils/route_listener_mapper_test.go @@ -20,7 +20,7 @@ func makeListenerAttachmentMapKey(listener gwv1.Listener, route preLoadRouteDesc return fmt.Sprintf("%s-%d-%s-%s", listener.Name, listener.Port, nsn.Name, nsn.Namespace) } -func (m *mockListenerAttachmentHelper) listenerAllowsAttachment(ctx context.Context, gw gwv1.Gateway, listener gwv1.Listener, route preLoadRouteDescriptor, routeReconciler RouteReconciler, hostnamesFromHttpRoutes map[types.NamespacedName][]gwv1.Hostname, hostnamesFromGrpcRoutes map[types.NamespacedName][]gwv1.Hostname) (bool, error) { +func (m *mockListenerAttachmentHelper) listenerAllowsAttachment(ctx context.Context, gw gwv1.Gateway, listener gwv1.Listener, route preLoadRouteDescriptor, routeReconciler RouteReconcilerSubmitter, hostnamesFromHttpRoutes map[types.NamespacedName][]gwv1.Hostname, hostnamesFromGrpcRoutes map[types.NamespacedName][]gwv1.Hostname) (bool, error) { k := makeListenerAttachmentMapKey(listener, route) return m.attachmentMap[k], nil } diff --git a/pkg/gateway/routeutils/route_reconciler_utils.go b/pkg/gateway/routeutils/route_reconciler_utils.go index a09e956248..93acbcdcc9 100644 --- a/pkg/gateway/routeutils/route_reconciler_utils.go +++ b/pkg/gateway/routeutils/route_reconciler_utils.go @@ -37,8 +37,12 @@ type ParentRefGateway struct { } type RouteReconciler interface { - Enqueue(routeData RouteData) Run() + Enqueue(routeData RouteData) +} + +type RouteReconcilerSubmitter interface { + Enqueue(routeData RouteData) } // constants From 31a6c4b91d490e1c3afbb2bad59083e83de02a07 Mon Sep 17 00:00:00 2001 From: Zachary Nixon Date: Thu, 14 Aug 2025 09:42:09 -0700 Subject: [PATCH 2/3] fix status route status overwrite --- .../elbv2.k8s.aws_ingressclassparams.yaml | 2 +- .../routeutils/listener_attachment_helper.go | 35 ++---- .../listener_attachment_helper_test.go | 41 +++++-- pkg/gateway/routeutils/loader.go | 45 +++++-- pkg/gateway/routeutils/loader_test.go | 114 +++++++++++++++--- .../routeutils/mock_route_reconciler.go | 10 +- .../routeutils/route_listener_mapper.go | 15 ++- .../routeutils/route_listener_mapper_test.go | 10 +- pkg/ingress/model_build_frontend_nlb.go | 2 - ...del_build_load_balancer_attributes_test.go | 2 - ...load_balancer_capacity_reservation_test.go | 2 - test/e2e/ingress/utils.go | 2 - 12 files changed, 196 insertions(+), 84 deletions(-) diff --git a/config/crd/bases/elbv2.k8s.aws_ingressclassparams.yaml b/config/crd/bases/elbv2.k8s.aws_ingressclassparams.yaml index cdcd53f3ce..6b980ebe2b 100644 --- a/config/crd/bases/elbv2.k8s.aws_ingressclassparams.yaml +++ b/config/crd/bases/elbv2.k8s.aws_ingressclassparams.yaml @@ -11,7 +11,7 @@ spec: kind: IngressClassParams listKind: IngressClassParamsList plural: ingressclassparams - singular: ingressclassparam + singular: ingressclassparams scope: Cluster versions: - additionalPrinterColumns: diff --git a/pkg/gateway/routeutils/listener_attachment_helper.go b/pkg/gateway/routeutils/listener_attachment_helper.go index c33f104785..c64017889b 100644 --- a/pkg/gateway/routeutils/listener_attachment_helper.go +++ b/pkg/gateway/routeutils/listener_attachment_helper.go @@ -14,7 +14,7 @@ import ( // listenerAttachmentHelper is an internal utility interface that can be used to determine if a listener will allow // a route to attach to it. type listenerAttachmentHelper interface { - listenerAllowsAttachment(ctx context.Context, gw gwv1.Gateway, listener gwv1.Listener, route preLoadRouteDescriptor, deferredRouteReconciler RouteReconcilerSubmitter, hostnamesFromHttpRoutes map[types.NamespacedName][]gwv1.Hostname, hostnamesFromGrpcRoutes map[types.NamespacedName][]gwv1.Hostname) (bool, error) + listenerAllowsAttachment(ctx context.Context, gw gwv1.Gateway, listener gwv1.Listener, route preLoadRouteDescriptor, hostnamesFromHttpRoutes map[types.NamespacedName][]gwv1.Hostname, hostnamesFromGrpcRoutes map[types.NamespacedName][]gwv1.Hostname) (bool, *RouteData, error) } var _ listenerAttachmentHelper = &listenerAttachmentHelperImpl{} @@ -34,40 +34,33 @@ func newListenerAttachmentHelper(k8sClient client.Client, logger logr.Logger) li // listenerAllowsAttachment utility method to determine if a listener will allow a route to connect using // Gateway API rules to determine compatibility between lister and route. -func (attachmentHelper *listenerAttachmentHelperImpl) listenerAllowsAttachment(ctx context.Context, gw gwv1.Gateway, listener gwv1.Listener, route preLoadRouteDescriptor, deferredRouteReconciler RouteReconcilerSubmitter, hostnamesFromHttpRoutes map[types.NamespacedName][]gwv1.Hostname, hostnamesFromGrpcRoutes map[types.NamespacedName][]gwv1.Hostname) (bool, error) { +func (attachmentHelper *listenerAttachmentHelperImpl) listenerAllowsAttachment(ctx context.Context, gw gwv1.Gateway, listener gwv1.Listener, route preLoadRouteDescriptor, hostnamesFromHttpRoutes map[types.NamespacedName][]gwv1.Hostname, hostnamesFromGrpcRoutes map[types.NamespacedName][]gwv1.Hostname) (bool, *RouteData, error) { // check namespace namespaceOK, err := attachmentHelper.namespaceCheck(ctx, gw, listener, route) if err != nil { - return false, err + return false, nil, err } if !namespaceOK { - deferredRouteReconciler.Enqueue( - GenerateRouteData(false, true, string(gwv1.RouteReasonNotAllowedByListeners), RouteStatusInfoRejectedMessageNamespaceNotMatch, route.GetRouteNamespacedName(), route.GetRouteKind(), route.GetRouteGeneration(), gw), - ) - - return false, nil + rd := GenerateRouteData(false, true, string(gwv1.RouteReasonNotAllowedByListeners), RouteStatusInfoRejectedMessageNamespaceNotMatch, route.GetRouteNamespacedName(), route.GetRouteKind(), route.GetRouteGeneration(), gw) + return false, &rd, nil } // check kind kindOK := attachmentHelper.kindCheck(listener, route) if !kindOK { - deferredRouteReconciler.Enqueue( - GenerateRouteData(false, true, string(gwv1.RouteReasonNotAllowedByListeners), RouteStatusInfoRejectedMessageKindNotMatch, route.GetRouteNamespacedName(), route.GetRouteKind(), route.GetRouteGeneration(), gw), - ) - return false, nil + rd := GenerateRouteData(false, true, string(gwv1.RouteReasonNotAllowedByListeners), RouteStatusInfoRejectedMessageKindNotMatch, route.GetRouteNamespacedName(), route.GetRouteKind(), route.GetRouteGeneration(), gw) + return false, &rd, nil } // check hostname if (route.GetRouteKind() == HTTPRouteKind || route.GetRouteKind() == GRPCRouteKind || route.GetRouteKind() == TLSRouteKind) && route.GetHostnames() != nil { hostnameOK, err := attachmentHelper.hostnameCheck(listener, route) if err != nil { - return false, err + return false, nil, err } if !hostnameOK { - deferredRouteReconciler.Enqueue( - GenerateRouteData(false, true, string(gwv1.RouteReasonNoMatchingListenerHostname), RouteStatusInfoRejectedMessageNoMatchingHostname, route.GetRouteNamespacedName(), route.GetRouteKind(), route.GetRouteGeneration(), gw), - ) - return false, nil + rd := GenerateRouteData(false, true, string(gwv1.RouteReasonNoMatchingListenerHostname), RouteStatusInfoRejectedMessageNoMatchingHostname, route.GetRouteNamespacedName(), route.GetRouteKind(), route.GetRouteGeneration(), gw) + return false, &rd, nil } } @@ -76,14 +69,12 @@ func (attachmentHelper *listenerAttachmentHelperImpl) listenerAllowsAttachment(c hostnameUniquenessOK, conflictRoute := attachmentHelper.crossServingHostnameUniquenessCheck(route, hostnamesFromHttpRoutes, hostnamesFromGrpcRoutes) if !hostnameUniquenessOK { message := fmt.Sprintf("HTTPRoute and GRPCRoute have overlap hostname, attachment is rejected. Conflict route: %s", conflictRoute) - deferredRouteReconciler.Enqueue( - GenerateRouteData(false, true, string(gwv1.RouteReasonNotAllowedByListeners), message, route.GetRouteNamespacedName(), route.GetRouteKind(), route.GetRouteGeneration(), gw), - ) - return false, nil + rd := GenerateRouteData(false, true, string(gwv1.RouteReasonNotAllowedByListeners), message, route.GetRouteNamespacedName(), route.GetRouteKind(), route.GetRouteGeneration(), gw) + return false, &rd, nil } } - return true, nil + return true, nil, nil } // namespaceCheck namespace check implements the Gateway API spec for namespace matching between listener diff --git a/pkg/gateway/routeutils/listener_attachment_helper_test.go b/pkg/gateway/routeutils/listener_attachment_helper_test.go index 7ea0df846d..8f22cd31cf 100644 --- a/pkg/gateway/routeutils/listener_attachment_helper_test.go +++ b/pkg/gateway/routeutils/listener_attachment_helper_test.go @@ -22,12 +22,19 @@ func (mnss *mockNamespaceSelector) getNamespacesFromSelector(_ context.Context, } func Test_listenerAllowsAttachment(t *testing.T) { + + type expectedRouteStatus struct { + reason string + message string + } + testCases := []struct { - name string - gwNamespace string - routeNamespace string - listenerProtocol gwv1.ProtocolType - expected bool + name string + gwNamespace string + routeNamespace string + listenerProtocol gwv1.ProtocolType + expectedStatusUpdate *expectedRouteStatus + expected bool }{ { name: "namespace and kind are ok", @@ -41,12 +48,20 @@ func Test_listenerAllowsAttachment(t *testing.T) { gwNamespace: "ns1", routeNamespace: "ns2", listenerProtocol: gwv1.HTTPProtocolType, + expectedStatusUpdate: &expectedRouteStatus{ + reason: string(gwv1.RouteReasonNotAllowedByListeners), + message: RouteStatusInfoRejectedMessageNamespaceNotMatch, + }, }, { name: "kind is not ok", gwNamespace: "ns1", routeNamespace: "ns1", listenerProtocol: gwv1.TLSProtocolType, + expectedStatusUpdate: &expectedRouteStatus{ + reason: string(gwv1.RouteReasonNotAllowedByListeners), + message: RouteStatusInfoRejectedMessageKindNotMatch, + }, }, } @@ -72,12 +87,22 @@ func Test_listenerAllowsAttachment(t *testing.T) { } hostnameFromHttpRoute := map[types.NamespacedName][]gwv1.Hostname{} hostnameFromGrpcRoute := map[types.NamespacedName][]gwv1.Hostname{} - mockReconciler := NewMockRouteReconciler() - result, err := attachmentHelper.listenerAllowsAttachment(context.Background(), gw, gwv1.Listener{ + result, statusUpdate, err := attachmentHelper.listenerAllowsAttachment(context.Background(), gw, gwv1.Listener{ Protocol: tc.listenerProtocol, - }, route, mockReconciler, hostnameFromHttpRoute, hostnameFromGrpcRoute) + }, route, hostnameFromHttpRoute, hostnameFromGrpcRoute) assert.NoError(t, err) assert.Equal(t, tc.expected, result) + if tc.expectedStatusUpdate == nil { + assert.Nil(t, statusUpdate) + } else { + assert.NotNil(t, statusUpdate) + assert.Equal(t, gw.Name, statusUpdate.ParentRefGateway.Name) + assert.Equal(t, gw.Namespace, statusUpdate.ParentRefGateway.Namespace) + assert.Equal(t, route.GetRouteNamespacedName().Name, statusUpdate.RouteMetadata.RouteName) + assert.Equal(t, route.GetRouteNamespacedName().Namespace, statusUpdate.RouteMetadata.RouteNamespace) + assert.Equal(t, tc.expectedStatusUpdate.message, statusUpdate.RouteStatusInfo.Message) + assert.Equal(t, tc.expectedStatusUpdate.reason, statusUpdate.RouteStatusInfo.Reason) + } }) } } diff --git a/pkg/gateway/routeutils/loader.go b/pkg/gateway/routeutils/loader.go index 52235eeaef..8c851dd03f 100644 --- a/pkg/gateway/routeutils/loader.go +++ b/pkg/gateway/routeutils/loader.go @@ -76,6 +76,23 @@ func (l *loaderImpl) LoadRoutesForGateway(ctx context.Context, gw gwv1.Gateway, // 1. Load all relevant routes according to the filter loadedRoutes := make([]preLoadRouteDescriptor, 0) + + routeStatusUpdates := make([]RouteData, 0) + + defer func() { + seenCache := sets.NewString() + // As we process the failures first, we ensure that we don't flip flop the route status from + // failed -> ok. + for _, v := range routeStatusUpdates { + k := generateRouteDataCacheKey(v) + if seenCache.Has(k) { + continue + } + seenCache.Insert(k) + l.routeSubmitter.Enqueue(v) + } + }() + for route, loader := range l.allRouteLoaders { applicable := filter.IsApplicable(route) l.logger.V(1).Info("Processing route", "route", route, "is applicable", applicable) @@ -90,13 +107,17 @@ func (l *loaderImpl) LoadRoutesForGateway(ctx context.Context, gw gwv1.Gateway, // 2. Remove routes that aren't granted attachment by the listener. // Map any routes that are granted attachment to the listener port that allows the attachment. - mappedRoutes, err := l.mapper.mapGatewayAndRoutes(ctx, gw, loadedRoutes, l.routeSubmitter) + mappedRoutes, statusUpdates, err := l.mapper.mapGatewayAndRoutes(ctx, gw, loadedRoutes) + + routeStatusUpdates = append(routeStatusUpdates, statusUpdates...) + if err != nil { return nil, err } // 3. Load the underlying resource(s) for each route that is configured. - loadedRoute, err := l.loadChildResources(ctx, mappedRoutes, gw) + loadedRoute, childRouteLoadUpdates, err := l.loadChildResources(ctx, mappedRoutes, gw) + routeStatusUpdates = append(routeStatusUpdates, childRouteLoadUpdates...) if err != nil { return nil, err } @@ -104,21 +125,19 @@ func (l *loaderImpl) LoadRoutesForGateway(ctx context.Context, gw gwv1.Gateway, // update status for accepted routes for _, routeList := range loadedRoute { for _, route := range routeList { - l.routeSubmitter.Enqueue( - GenerateRouteData(true, true, string(gwv1.RouteConditionAccepted), RouteStatusInfoAcceptedMessage, route.GetRouteNamespacedName(), route.GetRouteKind(), route.GetRouteGeneration(), gw), - ) + routeStatusUpdates = append(routeStatusUpdates, GenerateRouteData(true, true, string(gwv1.RouteConditionAccepted), RouteStatusInfoAcceptedMessage, route.GetRouteNamespacedName(), route.GetRouteKind(), route.GetRouteGeneration(), gw)) } } return loadedRoute, nil } // loadChildResources responsible for loading all resources that a route descriptor references. -func (l *loaderImpl) loadChildResources(ctx context.Context, preloadedRoutes map[int][]preLoadRouteDescriptor, gw gwv1.Gateway) (map[int32][]RouteDescriptor, error) { +func (l *loaderImpl) loadChildResources(ctx context.Context, preloadedRoutes map[int][]preLoadRouteDescriptor, gw gwv1.Gateway) (map[int32][]RouteDescriptor, []RouteData, error) { // Cache to reduce duplicate route lookups. // Kind -> [NamespacedName:Previously Loaded Descriptor] resourceCache := make(map[string]RouteDescriptor) - loadedRouteData := make(map[int32][]RouteDescriptor) + failedRoutes := make([]RouteData, 0) for port, preloadedRouteList := range preloadedRoutes { for _, preloadedRoute := range preloadedRouteList { @@ -137,12 +156,10 @@ func (l *loaderImpl) loadChildResources(ctx context.Context, preloadedRoutes map for _, lare := range loadAttachedRulesErrors { var loaderErr LoaderError if errors.As(lare.Err, &loaderErr) { - l.routeSubmitter.Enqueue( - GenerateRouteData(false, false, string(loaderErr.GetRouteReason()), loaderErr.GetRouteMessage(), preloadedRoute.GetRouteNamespacedName(), preloadedRoute.GetRouteKind(), preloadedRoute.GetRouteGeneration(), gw), - ) + failedRoutes = append(failedRoutes, GenerateRouteData(false, false, string(loaderErr.GetRouteReason()), loaderErr.GetRouteMessage(), preloadedRoute.GetRouteNamespacedName(), preloadedRoute.GetRouteKind(), preloadedRoute.GetRouteGeneration(), gw)) } if lare.Fatal { - return nil, lare.Err + return nil, failedRoutes, lare.Err } } } @@ -151,5 +168,9 @@ func (l *loaderImpl) loadChildResources(ctx context.Context, preloadedRoutes map } } - return loadedRouteData, nil + return loadedRouteData, failedRoutes, nil +} + +func generateRouteDataCacheKey(rd RouteData) string { + return fmt.Sprintf("%s-%s-%s-%s-%s", rd.RouteMetadata.RouteName, rd.RouteMetadata.RouteNamespace, rd.RouteMetadata.RouteKind, rd.ParentRefGateway.Name, rd.ParentRefGateway.Namespace) } diff --git a/pkg/gateway/routeutils/loader_test.go b/pkg/gateway/routeutils/loader_test.go index 0a4e256b76..b99acea49c 100644 --- a/pkg/gateway/routeutils/loader_test.go +++ b/pkg/gateway/routeutils/loader_test.go @@ -4,6 +4,7 @@ import ( "context" "github.com/go-logr/logr" "github.com/stretchr/testify/assert" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/client" @@ -13,14 +14,15 @@ import ( ) type mockMapper struct { - t *testing.T - expectedRoutes []preLoadRouteDescriptor - mapToReturn map[int][]preLoadRouteDescriptor + t *testing.T + expectedRoutes []preLoadRouteDescriptor + mapToReturn map[int][]preLoadRouteDescriptor + routeStatusUpdates []RouteData } -func (m *mockMapper) mapGatewayAndRoutes(context context.Context, gw gwv1.Gateway, routes []preLoadRouteDescriptor, routeReconciler RouteReconcilerSubmitter) (map[int][]preLoadRouteDescriptor, error) { +func (m *mockMapper) mapGatewayAndRoutes(context context.Context, gw gwv1.Gateway, routes []preLoadRouteDescriptor) (map[int][]preLoadRouteDescriptor, []RouteData, error) { assert.ElementsMatch(m.t, m.expectedRoutes, routes) - return m.mapToReturn, nil + return m.mapToReturn, m.routeStatusUpdates, nil } var _ RouteDescriptor = &mockRoute{} @@ -151,18 +153,21 @@ func TestLoadRoutesForGateway(t *testing.T) { } testCases := []struct { - name string - acceptedKinds sets.Set[RouteKind] - expectedMap map[int32][]RouteDescriptor - expectedPreloadMap map[int][]preLoadRouteDescriptor - expectedPreMappedRoutes []preLoadRouteDescriptor - expectError bool + name string + acceptedKinds sets.Set[RouteKind] + expectedMap map[int32][]RouteDescriptor + expectedPreloadMap map[int][]preLoadRouteDescriptor + expectedPreMappedRoutes []preLoadRouteDescriptor + mapperRouteStatusUpdates []RouteData + expectedReconcileQueue map[string]bool // generateRouteDataCacheKey -> succeeded + expectError bool }{ { name: "filter allows no routes", acceptedKinds: make(sets.Set[RouteKind]), expectedPreMappedRoutes: make([]preLoadRouteDescriptor, 0), expectedMap: make(map[int32][]RouteDescriptor), + expectedReconcileQueue: map[string]bool{}, }, { name: "filter only allows http route", @@ -174,6 +179,11 @@ func TestLoadRoutesForGateway(t *testing.T) { expectedMap: map[int32][]RouteDescriptor{ 80: loadedHTTPRoutes, }, + expectedReconcileQueue: map[string]bool{ + "http1-http1-ns-HTTPRoute-gw-gw-ns": true, + "http2-http2-ns-HTTPRoute-gw-gw-ns": true, + "http3-http3-ns-HTTPRoute-gw-gw-ns": true, + }, }, { name: "filter only allows http route, multiple ports", @@ -187,6 +197,11 @@ func TestLoadRoutesForGateway(t *testing.T) { 80: loadedHTTPRoutes, 443: loadedHTTPRoutes, }, + expectedReconcileQueue: map[string]bool{ + "http1-http1-ns-HTTPRoute-gw-gw-ns": true, + "http2-http2-ns-HTTPRoute-gw-gw-ns": true, + "http3-http3-ns-HTTPRoute-gw-gw-ns": true, + }, }, { name: "filter only allows tcp route", @@ -198,6 +213,11 @@ func TestLoadRoutesForGateway(t *testing.T) { expectedMap: map[int32][]RouteDescriptor{ 80: loadedTCPRoutes, }, + expectedReconcileQueue: map[string]bool{ + "tcp1-tcp1-ns-TCPRoute-gw-gw-ns": true, + "tcp2-tcp2-ns-TCPRoute-gw-gw-ns": true, + "tcp3-tcp3-ns-TCPRoute-gw-gw-ns": true, + }, }, { name: "filter allows both route kinds", @@ -211,30 +231,92 @@ func TestLoadRoutesForGateway(t *testing.T) { 80: loadedTCPRoutes, 443: loadedHTTPRoutes, }, + expectedReconcileQueue: map[string]bool{ + "http1-http1-ns-HTTPRoute-gw-gw-ns": true, + "http2-http2-ns-HTTPRoute-gw-gw-ns": true, + "http3-http3-ns-HTTPRoute-gw-gw-ns": true, + "tcp1-tcp1-ns-TCPRoute-gw-gw-ns": true, + "tcp2-tcp2-ns-TCPRoute-gw-gw-ns": true, + "tcp3-tcp3-ns-TCPRoute-gw-gw-ns": true, + }, + }, + { + name: "failed route should lead to only failed version status getting published", + acceptedKinds: sets.New[RouteKind](TCPRouteKind, HTTPRouteKind), + expectedPreMappedRoutes: append(preLoadHTTPRoutes, preLoadTCPRoutes...), + expectedPreloadMap: map[int][]preLoadRouteDescriptor{ + 80: preLoadTCPRoutes, + 443: preLoadHTTPRoutes, + }, + expectedMap: map[int32][]RouteDescriptor{ + 80: loadedTCPRoutes, + 443: loadedHTTPRoutes, + }, + expectedReconcileQueue: map[string]bool{ + "http1-http1-ns-HTTPRoute-gw-gw-ns": true, + "http2-http2-ns-HTTPRoute-gw-gw-ns": true, + "http3-http3-ns-HTTPRoute-gw-gw-ns": true, + "tcp1-tcp1-ns-TCPRoute-gw-gw-ns": true, + "tcp2-tcp2-ns-TCPRoute-gw-gw-ns": false, + "tcp3-tcp3-ns-TCPRoute-gw-gw-ns": true, + }, + mapperRouteStatusUpdates: []RouteData{ + { + RouteStatusInfo: RouteStatusInfo{ + Accepted: false, + }, + RouteMetadata: RouteMetadata{ + RouteName: "tcp2", + RouteNamespace: "tcp2-ns", + RouteKind: string(TCPRouteKind), + RouteGeneration: 0, + }, + ParentRefGateway: ParentRefGateway{ + Name: "gw", + Namespace: "gw-ns", + }, + }, + }, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + routeReconciler := NewMockRouteReconciler() loader := loaderImpl{ mapper: &mockMapper{ - t: t, - expectedRoutes: tc.expectedPreMappedRoutes, - mapToReturn: tc.expectedPreloadMap, + t: t, + expectedRoutes: tc.expectedPreMappedRoutes, + mapToReturn: tc.expectedPreloadMap, + routeStatusUpdates: tc.mapperRouteStatusUpdates, }, allRouteLoaders: allRouteLoaders, logger: logr.Discard(), - routeSubmitter: NewMockRouteReconciler(), + routeSubmitter: routeReconciler, } filter := &routeFilterImpl{acceptedKinds: tc.acceptedKinds} - result, err := loader.LoadRoutesForGateway(context.Background(), gwv1.Gateway{}, filter) + result, err := loader.LoadRoutesForGateway(context.Background(), gwv1.Gateway{ObjectMeta: v1.ObjectMeta{ + Name: "gw", + Namespace: "gw-ns", + }}, filter) if tc.expectError { assert.Error(t, err) return } + assert.NoError(t, err) assert.Equal(t, tc.expectedMap, result) + assert.Equal(t, len(tc.expectedReconcileQueue), len(routeReconciler.Enqueued)) + + for _, actual := range routeReconciler.Enqueued { + ak := generateRouteDataCacheKey(actual.RouteData) + + v, ok := tc.expectedReconcileQueue[ak] + assert.True(t, ok) + assert.Equal(t, v, actual.RouteData.RouteStatusInfo.Accepted, ak) + } + }) } } diff --git a/pkg/gateway/routeutils/mock_route_reconciler.go b/pkg/gateway/routeutils/mock_route_reconciler.go index 938d4dcfec..790c73a146 100644 --- a/pkg/gateway/routeutils/mock_route_reconciler.go +++ b/pkg/gateway/routeutils/mock_route_reconciler.go @@ -14,13 +14,9 @@ func NewMockRouteReconciler() *MockRouteReconciler { } func (m *MockRouteReconciler) Enqueue(routeData RouteData) { - if m.Enqueued == nil { - m.Enqueued = make([]EnqueuedType, 0) - } else { - m.Enqueued = append(m.Enqueued, EnqueuedType{ - routeData, - }) - } + m.Enqueued = append(m.Enqueued, EnqueuedType{ + routeData, + }) } func (m *MockRouteReconciler) Run() { diff --git a/pkg/gateway/routeutils/route_listener_mapper.go b/pkg/gateway/routeutils/route_listener_mapper.go index 8117873718..0e1fce5ffe 100644 --- a/pkg/gateway/routeutils/route_listener_mapper.go +++ b/pkg/gateway/routeutils/route_listener_mapper.go @@ -11,7 +11,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, deferredRouteReconciler RouteReconcilerSubmitter) (map[int][]preLoadRouteDescriptor, error) + mapGatewayAndRoutes(context context.Context, gw gwv1.Gateway, routes []preLoadRouteDescriptor) (map[int][]preLoadRouteDescriptor, []RouteData, error) } var _ listenerToRouteMapper = &listenerToRouteMapperImpl{} @@ -31,8 +31,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. -func (ltr *listenerToRouteMapperImpl) mapGatewayAndRoutes(ctx context.Context, gw gwv1.Gateway, routes []preLoadRouteDescriptor, deferredRouteReconciler RouteReconcilerSubmitter) (map[int][]preLoadRouteDescriptor, error) { +func (ltr *listenerToRouteMapperImpl) mapGatewayAndRoutes(ctx context.Context, gw gwv1.Gateway, routes []preLoadRouteDescriptor) (map[int][]preLoadRouteDescriptor, []RouteData, error) { result := make(map[int][]preLoadRouteDescriptor) + failedRoutes := make([]RouteData, 0) // First filter out any routes that are not intended for this Gateway. routesForGateway := make([]preLoadRouteDescriptor, 0) @@ -57,9 +58,13 @@ func (ltr *listenerToRouteMapperImpl) mapGatewayAndRoutes(ctx context.Context, g continue } - allowedAttachment, err := ltr.listenerAttachmentHelper.listenerAllowsAttachment(ctx, gw, listener, route, deferredRouteReconciler, hostnamesFromHttpRoutes, hostnamesFromGrpcRoutes) + allowedAttachment, failedRouteData, err := ltr.listenerAttachmentHelper.listenerAllowsAttachment(ctx, gw, listener, route, hostnamesFromHttpRoutes, hostnamesFromGrpcRoutes) if err != nil { - return nil, err + return nil, failedRoutes, err + } + + if failedRouteData != nil { + failedRoutes = append(failedRoutes, *failedRouteData) } ltr.logger.V(1).Info("listener allows attachment", "route", route.GetRouteNamespacedName(), "allowedAttachment", allowedAttachment) @@ -69,5 +74,5 @@ func (ltr *listenerToRouteMapperImpl) mapGatewayAndRoutes(ctx context.Context, g } } } - return result, nil + return result, failedRoutes, nil } diff --git a/pkg/gateway/routeutils/route_listener_mapper_test.go b/pkg/gateway/routeutils/route_listener_mapper_test.go index f4b895a2d3..0f7bf62d16 100644 --- a/pkg/gateway/routeutils/route_listener_mapper_test.go +++ b/pkg/gateway/routeutils/route_listener_mapper_test.go @@ -20,9 +20,9 @@ func makeListenerAttachmentMapKey(listener gwv1.Listener, route preLoadRouteDesc return fmt.Sprintf("%s-%d-%s-%s", listener.Name, listener.Port, nsn.Name, nsn.Namespace) } -func (m *mockListenerAttachmentHelper) listenerAllowsAttachment(ctx context.Context, gw gwv1.Gateway, listener gwv1.Listener, route preLoadRouteDescriptor, routeReconciler RouteReconcilerSubmitter, hostnamesFromHttpRoutes map[types.NamespacedName][]gwv1.Hostname, hostnamesFromGrpcRoutes map[types.NamespacedName][]gwv1.Hostname) (bool, error) { +func (m *mockListenerAttachmentHelper) listenerAllowsAttachment(ctx context.Context, gw gwv1.Gateway, listener gwv1.Listener, route preLoadRouteDescriptor, hostnamesFromHttpRoutes map[types.NamespacedName][]gwv1.Hostname, hostnamesFromGrpcRoutes map[types.NamespacedName][]gwv1.Hostname) (bool, *RouteData, error) { k := makeListenerAttachmentMapKey(listener, route) - return m.attachmentMap[k], nil + return m.attachmentMap[k], nil, nil } type mockRouteAttachmentHelper struct { @@ -313,9 +313,7 @@ func Test_mapGatewayAndRoutes(t *testing.T) { }, logger: logr.Discard(), } - - mockReconciler := NewMockRouteReconciler() - result, err := mapper.mapGatewayAndRoutes(context.Background(), tc.gw, tc.routes, mockReconciler) + result, statusUpdates, err := mapper.mapGatewayAndRoutes(context.Background(), tc.gw, tc.routes) if tc.expectErr { assert.Error(t, err) @@ -325,6 +323,8 @@ func Test_mapGatewayAndRoutes(t *testing.T) { assert.NoError(t, err) assert.Equal(t, len(tc.expected), len(result)) + assert.Equal(t, 0, len(statusUpdates)) + for k, v := range tc.expected { assert.ElementsMatch(t, v, result[k]) } diff --git a/pkg/ingress/model_build_frontend_nlb.go b/pkg/ingress/model_build_frontend_nlb.go index 416a186ced..ae366b80dc 100644 --- a/pkg/ingress/model_build_frontend_nlb.go +++ b/pkg/ingress/model_build_frontend_nlb.go @@ -753,8 +753,6 @@ func buildFrontendNlbResourceID(resourceType string, protocol elbv2model.Protoco func mergeHealthCheckField[T comparable](fieldName string, finalValue **T, currentValue *T, explicit map[string]bool, explicitFields map[string]bool, configIndex int) error { if explicit[fieldName] { if explicitFields[fieldName] { - fmt.Printf("*finalValue (%T): %v\n", **finalValue, **finalValue) - fmt.Printf("newValue (%T): %v\n", *currentValue, *currentValue) if **finalValue != *currentValue { return errors.Errorf("conflicting %s, config %d: %v, previous: %v", fieldName, configIndex+1, *currentValue, **finalValue) } diff --git a/pkg/ingress/model_build_load_balancer_attributes_test.go b/pkg/ingress/model_build_load_balancer_attributes_test.go index 4c623bd8ee..ae98c3a4c3 100644 --- a/pkg/ingress/model_build_load_balancer_attributes_test.go +++ b/pkg/ingress/model_build_load_balancer_attributes_test.go @@ -1,7 +1,6 @@ package ingress import ( - "fmt" "testing" "github.com/pkg/errors" @@ -376,7 +375,6 @@ func Test_defaultModelBuildTask_buildIngressLoadBalancerAttributes(t *testing.T) } got, err := task.buildIngressLoadBalancerAttributes(tt.args.ing) if tt.wantErr != nil { - fmt.Println(err) assert.EqualError(t, err, tt.wantErr.Error()) } else { assert.NoError(t, err) diff --git a/pkg/ingress/model_build_load_balancer_capacity_reservation_test.go b/pkg/ingress/model_build_load_balancer_capacity_reservation_test.go index 5549e6ebe6..f29dff4cbc 100644 --- a/pkg/ingress/model_build_load_balancer_capacity_reservation_test.go +++ b/pkg/ingress/model_build_load_balancer_capacity_reservation_test.go @@ -2,7 +2,6 @@ package ingress import ( "context" - "fmt" "github.com/pkg/errors" "github.com/stretchr/testify/assert" networking "k8s.io/api/networking/v1" @@ -444,7 +443,6 @@ func Test_defaultModelBuildTask_buildIngressLoadBalancerMinimumCapacity(t *testi } got, err := task.buildIngressLoadBalancerMinimumCapacity(tt.args.ing) if tt.wantErr != nil { - fmt.Println(err) assert.EqualError(t, err, tt.wantErr.Error()) } else { assert.NoError(t, err) diff --git a/test/e2e/ingress/utils.go b/test/e2e/ingress/utils.go index 609143e708..dc4c82a8ee 100644 --- a/test/e2e/ingress/utils.go +++ b/test/e2e/ingress/utils.go @@ -1,7 +1,6 @@ package ingress import ( - "fmt" "strings" networking "k8s.io/api/networking/v1" @@ -19,7 +18,6 @@ func FindIngressDNSName(ing *networking.Ingress) string { func FindIngressTwoDNSName(ing *networking.Ingress) (albDNS string, nlbDNS string) { for _, ingSTS := range ing.Status.LoadBalancer.Ingress { if ingSTS.Hostname != "" { - fmt.Println("ingSTS.Hostname", ingSTS.Hostname) if strings.Contains(ingSTS.Hostname, "elb.amazonaws.com") { albDNS = ingSTS.Hostname } else { From e7d61c5141f68ee9686a4e0cd58e0b6242e95a34 Mon Sep 17 00:00:00 2001 From: Zachary Nixon Date: Fri, 15 Aug 2025 11:37:14 -0700 Subject: [PATCH 3/3] fix crd generation for ingressclassparams --- apis/elbv2/v1beta1/ingressclassparams_types.go | 2 +- config/crd/bases/elbv2.k8s.aws_ingressclassparams.yaml | 2 +- helm/aws-load-balancer-controller/crds/crds.yaml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/apis/elbv2/v1beta1/ingressclassparams_types.go b/apis/elbv2/v1beta1/ingressclassparams_types.go index 7c68bc70e7..f68ceeeeed 100644 --- a/apis/elbv2/v1beta1/ingressclassparams_types.go +++ b/apis/elbv2/v1beta1/ingressclassparams_types.go @@ -192,7 +192,7 @@ type IngressClassParamsSpec struct { } // +kubebuilder:object:root=true -// +kubebuilder:resource:scope=Cluster +// +kubebuilder:resource:scope=Cluster,singular=ingressclassparam // +kubebuilder:storageversion // +kubebuilder:printcolumn:name="GROUP-NAME",type="string",JSONPath=".spec.group.name",description="The Ingress Group name" // +kubebuilder:printcolumn:name="SCHEME",type="string",JSONPath=".spec.scheme",description="The AWS Load Balancer scheme" diff --git a/config/crd/bases/elbv2.k8s.aws_ingressclassparams.yaml b/config/crd/bases/elbv2.k8s.aws_ingressclassparams.yaml index 6b980ebe2b..cdcd53f3ce 100644 --- a/config/crd/bases/elbv2.k8s.aws_ingressclassparams.yaml +++ b/config/crd/bases/elbv2.k8s.aws_ingressclassparams.yaml @@ -11,7 +11,7 @@ spec: kind: IngressClassParams listKind: IngressClassParamsList plural: ingressclassparams - singular: ingressclassparams + singular: ingressclassparam scope: Cluster versions: - additionalPrinterColumns: diff --git a/helm/aws-load-balancer-controller/crds/crds.yaml b/helm/aws-load-balancer-controller/crds/crds.yaml index 7e43cb1732..15145de6af 100644 --- a/helm/aws-load-balancer-controller/crds/crds.yaml +++ b/helm/aws-load-balancer-controller/crds/crds.yaml @@ -10,7 +10,7 @@ spec: kind: IngressClassParams listKind: IngressClassParamsList plural: ingressclassparams - singular: ingressclassparams + singular: ingressclassparam scope: Cluster versions: - additionalPrinterColumns: