[feat gw api] Add Gateway Class Reconciler + Config merging logic. #4163

zac-nixon · 2025-05-01T05:42:20Z

Description

Adds a reconciler to process Gateway Class changes, including changes to LoadBalancer configurations attached to a Gateway Class.

Adds basic logic to ensure that Gateway reconciler will only progress Gateways that have an accepted Gateway Class.

Adds a configuration merger that implements merging logic to handle GatewayClass and Gateway LoadBalancer configs. [TODO: Add documentation]

Refactored to drop pointer references to the the lb config in the gateway model builders, this removes the need for nil checks in all of the model builders.

Checklist

Added tests that cover your change (if possible)
[] Added/modified documentation as required (such as the README.md, or the docs directory)
Manually tested
Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Backfilled missing tests for code in same general area 🎉
Refactored something and made the world a better place 🌟

k8s-ci-robot · 2025-05-01T05:42:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zac-nixon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [zac-nixon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pkg/config/controller_config.go

pkg/gateway/config_merger.go

shraddhabang · 2025-05-01T17:54:42Z

pkg/gateway/config_merger.go

+		merged.EnableICMP = lowPriority.Spec.EnableICMP
+	}
+
+	if highPriority.Spec.ManageBackendSecurityGroupRules != nil {


I wish Go had ternary testing operation :(

controllers/gateway/gateway_controller.go

shuqz · 2025-05-01T20:35:10Z

config/crd/gateway/gateway-crds.yaml

+                  mergingMode defines the merge behavior when both the Gateway and GatewayClass have a defined LoadBalancerConfiguration.
+                  This field is only honored for the configuration attached to the GatewayClass.
+                enum:
+                - prefer-gateway


i am think should we be more specific in naming, cuz the name mergingMode does not imply it is for lbConfig and enum does not imply it too. Users can read doc for sure but maybe we can try to make name more explicit

The merging mode only exists within the LoadBalancer configuration, I felt it would be redundant to re-state that the merging mode is specific to the LB.

shuqz · 2025-05-01T20:35:16Z

controllers/gateway/eventhandlers/gatewayclass/load_balancer_configuration_events.go

+
+func (h *enqueueRequestsForLoadBalancerConfigurationEvent) Delete(ctx context.Context, e event.TypedDeleteEvent[*elbv2gw.LoadBalancerConfiguration], queue workqueue.TypedRateLimitingInterface[reconcile.Request]) {
+	lbconfig := e.Object
+	h.logger.V(1).Info("enqueue loadbalancerconfiguration delete event", "loadbalancerconfiguration", lbconfig.Name)


do we also wanna logging namespace? (if name can be same for different namespace)

yeah good idea

shuqz · 2025-05-01T21:43:38Z

pkg/gateway/config_merger.go

+	}
+}
+
+func (merger *configMergerImpl) performTakeOneMerges(merged *elbv2gw.LoadBalancerConfigurationSpec, highPriority elbv2gw.LoadBalancerConfiguration, lowPriority elbv2gw.LoadBalancerConfiguration) {


it looks like in this function, it is assume if a property is not specified in highPriority spec, it will be specified in low priority spec, but what if lowPriority.Spec.LoadBalancerName is also nil (unless it is never gonna be the case), and if we originally have a default value for LoadBalancerName, then after this function, the default value will be overriden and it is gonna be nil

I don't follow -- The merged spec starts off w/ a blank spec, so it's all zero values. We will always use the high priority value (if it exists) otherwise we use the low priority value, which could be a real value or a zero value. If it's a zero value, the merged spec still has a zero value which is correct.

I don't think it's possible we ever start with a default value for the load balancer name.

i see, thanks for the explanation

shraddhabang · 2025-05-01T22:04:45Z

controllers/gateway/gateway_controller.go

+
+func (r *gatewayReconciler) getLoadBalancerConfigForGateway(ctx context.Context, k8sClient client.Client, gw *gwv1.Gateway, gwClass *gwv1.GatewayClass) (elbv2gw.LoadBalancerConfiguration, error) {
+
+	// If the Gateway Class isn't accepted, we shouldn't try to reconcile this Gateway.


Can we move this check outside of this function in the main reconciler so that this function sticks to just getting the lb config.

shraddhabang · 2025-05-01T22:07:27Z

controllers/gateway/gateway_controller.go

+			if storedVersion != nil {
+				safeVersion = *storedVersion
+			}
+			return elbv2gw.LoadBalancerConfiguration{}, errors.Errorf("GatewayClass [%s] hasn't processed latest loadbalancer config. Processed version %s, Latest version %s", gwClass.Name, safeVersion, gatewayClassLBConfig.ResourceVersion)


Instead of throwing error, do we want to retry/wait for gwclass to complete its processing its cfg?

Throwing an error is the same thing as wait + retry.

shraddhabang · 2025-05-01T22:16:42Z

controllers/gateway/eventhandlers/load_balancer_configuration_events.go

@@ -62,7 +65,7 @@ func (h *enqueueRequestsForLoadBalancerConfigurationEvent) Generic(ctx context.C
 }

 func (h *enqueueRequestsForLoadBalancerConfigurationEvent) enqueueImpactedService(ctx context.Context, lbconfig *elbv2gw.LoadBalancerConfiguration, queue workqueue.TypedRateLimitingInterface[reconcile.Request]) {
-	gwClasses := GetImpactedGatewayClassesFromLbConfig(ctx, h.k8sClient, lbconfig, h.gwController)
+	gwClasses := GetImpactedGatewayClassesFromLbConfig(ctx, h.k8sClient, lbconfig, h.gwControllerSet)


Since we have separate controller for gateway class? Do we even need to enqueue gwclass for LB config changes in gateway controller?

Yeah.

They serve two different purposes.

In the GatewayClass reconciler, it's meant to validate the configuration is valid and move the Gateway Class status to Accepted.

In the Gateway reconciler, it's meant to materialize the ELB resources according to the Gateway config.

shraddhabang · 2025-05-01T22:25:46Z

controllers/gateway/eventhandlers/gatewayclass/load_balancer_configuration_events.go

+}
+
+func (h *enqueueRequestsForLoadBalancerConfigurationEvent) Create(ctx context.Context, e event.TypedCreateEvent[*elbv2gw.LoadBalancerConfiguration], queue workqueue.TypedRateLimitingInterface[reconcile.Request]) {
+	// We don't need to respond to create events.


What if the gwclass is created first referencing a non-existing cfg, user realises his mistake as they see not found status on the gwclass and decides to create the lbcfg now? We need to listen to this event and enqueue the gwclass referencing it to update its status.

This should already been handled, because we have the event queued for the gwclass creation. Since it failed to find the config, it will keep reconciling. Eventually when the config is created, everything should just work.

controllers/gateway/gatway_class_controller.go

…andler

shraddhabang · 2025-05-02T04:20:17Z

/lgtm
/approved

k8s-ci-robot requested review from M00nF1sh and wweiwei-li May 1, 2025 05:42

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 1, 2025

zac-nixon mentioned this pull request May 1, 2025

[feat:gw-api] add support for capacity reservation #4162

Merged

6 tasks

zac-nixon force-pushed the main branch from 90f4fc9 to 0570423 Compare May 1, 2025 17:04

shraddhabang reviewed May 1, 2025

View reviewed changes

pkg/config/controller_config.go Outdated Show resolved Hide resolved

shraddhabang reviewed May 1, 2025

View reviewed changes

pkg/gateway/config_merger.go Show resolved Hide resolved

shraddhabang reviewed May 1, 2025

View reviewed changes

controllers/gateway/gateway_controller.go Outdated Show resolved Hide resolved

zac-nixon force-pushed the main branch from 0570423 to d3a5244 Compare May 1, 2025 20:04

shuqz reviewed May 1, 2025

View reviewed changes

zac-nixon force-pushed the main branch from d3a5244 to 7d69e39 Compare May 1, 2025 21:24

shuqz reviewed May 1, 2025

View reviewed changes

shraddhabang reviewed May 1, 2025

View reviewed changes

controllers/gateway/gatway_class_controller.go Show resolved Hide resolved

zac-nixon added 6 commits May 1, 2025 16:05

refactor to remove ptr to lb config

3149c32

[gw api] gateway class reconciler, config generation

a5dd7d7

rebase

c06102c

add config merger tests

a28d8c3

fix cert discovery

6138a59

rebase yet again

773cd6f

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 1, 2025

refactor config resolver to make it testable, remove unneeded event h…

6521abb

…andler

zac-nixon force-pushed the main branch from 7d69e39 to 6521abb Compare May 2, 2025 02:39

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2025

k8s-ci-robot assigned shraddhabang May 2, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 2, 2025

k8s-ci-robot merged commit a3c4fc6 into kubernetes-sigs:main May 2, 2025
9 checks passed

zac-nixon mentioned this pull request May 6, 2025

🥳 aws-load-balancer-controller v2.13.0 Automated Release! 🥑 aws/eks-charts#1214

Merged


		func (r gatewayReconciler) getLoadBalancerConfigForGateway(ctx context.Context, k8sClient client.Client, gw gwv1.Gateway, gwClass *gwv1.GatewayClass) (elbv2gw.LoadBalancerConfiguration, error) {

		// If the Gateway Class isn't accepted, we shouldn't try to reconcile this Gateway.

[feat gw api] Add Gateway Class Reconciler + Config merging logic. #4163

[feat gw api] Add Gateway Class Reconciler + Config merging logic. #4163

Uh oh!

Conversation

zac-nixon commented May 1, 2025

Description

Checklist

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Uh oh!

k8s-ci-robot commented May 1, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shraddhabang commented May 2, 2025

Uh oh!

Uh oh!

Uh oh!