[Rebase + Bugfixes] Add support for TCP_UDP Listeners #4161

zac-nixon · 2025-04-29T23:02:53Z

Issue

Original PR - #3807

I've cherry-picked the original commits to make sure @lyda gets their due credit.

Description

This PR augments the original PR with some bug fixes found during testing and adding the support for TCP_UDP listeners behind a service annotation, or a feature flag that will enable the default value to be true for creating TCP_UDP listeners.

Changes:

Added feature flagging, as the new TCP_UDP logic changes the reconcile behavior for users using the same port number multiple times in their service spec.
Refactored into "legacy" and TCP_UDP supported methods. The "legacy" method preserves the existing listener creation behavior.
Refactored the original PR to not modify k8s objects, instead we infer the correct TG / Listener protocol once, and pass that inferred value everywhere it's needed.

TestCases:

Exposed Kube DNS with no TCP_UDP support enabled. UDP listener continued to work.
Exposed Kube DNS with TCP_UDP support enabled via annotation. UDP and TCP listener both worked ->
UDP

while true
do
dig @k8s-kubesyst-kubednsn-c189a7922a-540bf825851eb181.elb.us-east-1.amazonaws.com google.com  | tee -a /tmp/output2
done

TCP

while true         
do
dig @k8s-kubesyst-kubednsn-c189a7922a-540bf825851eb181.elb.us-east-1.amazonaws.com google.com +vc  | tee -a /tmp/output2
done

Ensured that feature flag is honored.
Ensured that security group rules are correctly updated for both instance and IP modes.
Ensured rolling back to v2.12 result in no downtime for UDP traffic [TCP traffic is cut off as v2.12 doesn't know about TCP_UDP]
Ensured flipping between TCP_UDP support values [on, off] works.

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 🌟

The original PR had a test for mergeServicePortsForListener tacked on. That test has been moved into the test for mergeServicePortsForListener. This also fixes the test to deal with the error checking that was added to mergeServicePortsForListener.

Previously, aws-load-balancer-controller ignored extra overlapping ServicePorts defined in the Kubernetes Service spec if the external port numbers were the same even if the protocols were different (e.g. TCP:53, UDP:53). This behavior prevented users from exposing services that support TCP and UDP on the same external load balancer port number. This patch solves the problem by detecting when a user defines multiple ServicePorts for the same external load balancer port number but using TCP and UDP protocols separately. In such situations, a TCP_UDP TargetGroup and Listener are created and SecurityGroup rules are updated accordingly. If more than two ServicePorts are defined, only the first two mergeable ServicePorts are used. Otherwise, the first ServicePort is used. Note: rebasing errors would be my fault -- Kevin Lyda Signed-off-by: Kevin Lyda <[email protected]>

lyda · 2025-04-30T06:15:32Z

Note my PR was primarily a rebase of #2275 by @amorey . Hopefully the third PR's the charm!

shraddhabang · 2025-04-30T18:50:06Z

docs/guide/service/annotations.md

+- <a name="tcp-udp-listener">`service.beta.kubernetes.io/aws-load-balancer-enable-tcp-udp-listener`</a> allows creation of [TCP_UDP](https://docs.aws.amazon.com/elasticloadbalancing/latest/network/load-balancer-listeners.html#listener-configuration) listener type when the service defines a TCP and UDP port on the same port number.
+
+  !!!note ""
+  - To change the default from false to true, use the controller flag `--feature-gates=EnableTCPUDPListener=true` to allow creation of TCP_UDP listeners for all services.


Why do we needa controller level feature flag as we have annotation for this feature? I find it a little bit risky since it will affect all the existing services for the customers if they enable it at the controller level and don't specify false on their service annotations. Should we only ask to enable this feature through annotations as its a informed decision made by them to have TCP_UDP on same port instead of affecting all the services.

It's to make it easy for people to adopt the feature. E.g. a similar ask for using backend security groups: #4145

This way, we give people the flexibility to either specify it per-service that they want the support, or they can specify it at the feature flag level and not have to worry about annotating individual services.

I always consider feature gates to provide a safe way out if they simply dont want to use that feature. Keeping it default to true so that they get this feature enabled by default and let the customer decide the usage of this feature based on annotation values. This gives them the flexibility to disable the feature once and for all if they don't want it irrespective annotation value eg. NLBSecurityGroup and LBCapacityReservation.

But I get that we are adding this to avoid having them use this annotation on each service to make easy adoption. So we can keep it but then lets give them the ability to get out of this for the services on which they dont want to merge TCP_UDP ports by reading the annotation value. This way they have the ability to disable the TCP_UDP ports for the services if they want to and they dont need to add the annotation on the services where they want to merge it. What do you think?

shraddhabang · 2025-04-30T19:10:03Z

pkg/service/model_build_listener.go

 func (t *defaultModelBuildTask) buildListeners(ctx context.Context, scheme elbv2model.LoadBalancerScheme) error {
-	cfg, err := t.buildListenerConfig(ctx)
+
+	if t.enableTCPUDPSupport || t.isTCPUDPEnabledForService(t.service.Annotations) {


If the feature flag is enabled, the annotation value wont matter anymore and the services which does not want to merge TCP_UDP ports will have no option but to merge their ports? What if the customers wants the mix of both type of services?

If they want to control which services get the feature enabled, they have to use the annotation. Setting the controller flag to true should override the annotation value.

Is the suggestion to make the annotation take precedence, if the annotation is present?

Yes. Basically if the feature flag is true, all the services will merge the ports unless specified false by annotation. What do you think?

That sounds reasonable.

i have a follow-up question, it sounds like when both annotation and feature gate are present, sometimes annotation takes precedence and sometimes feature gate does. I get the idea that this is because each case has unique condition, but will this inconsistent behavior confuses user sometimes? how are we helping them with better documentation?

Good call out. I will document this feature flag in order to clarify any confusion. I think the differing behavior is based off of the usage of the flag. In some cases, it's controller wide behavior while other features can operate at the individual resource level.

k8s-ci-robot · 2025-04-30T21:49:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: WestonReed, 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

shraddhabang · 2025-04-30T23:12:05Z

/lgtm
/approved

amorey · 2025-05-01T07:10:39Z

This is awesome! Thank you to @zac-nixon @lyda et al. for getting this feature merged into main! I was very excited to wake up to the close notification this morning.

lyda · 2025-05-01T07:14:37Z

This is awesome! Thank you to @lyda et al. for getting this feature merged into main! I was very excited to wake up to the close notification this morning.

In a world where there are lots of divisive people, nice to see folks from Turkey, Ireland, Ukraine and Washington state (among others) team up to get something done. More like this.

lyda and others added 6 commits April 29, 2025 14:08

Clean up tests

932fcf3

The original PR had a test for mergeServicePortsForListener tacked on. That test has been moved into the test for mergeServicePortsForListener. This also fixes the test to deal with the error checking that was added to mergeServicePortsForListener.

Don't silently ignore errors

dfd2e5f

Remove spurious tabs

0b905c7

Attempt to address remaining errors

a6a7230

bug fixes and refactoring for tcpudp listener

149c3a3

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 29, 2025

k8s-ci-robot requested review from oliviassss and shraddhabang April 29, 2025 23:02

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 29, 2025

zac-nixon mentioned this pull request Apr 29, 2025

Add support for TCP_UDP to NLB TargetGroups and Listeners (rebase) #3807

Closed

6 tasks

shraddhabang reviewed Apr 30, 2025

View reviewed changes

zac-nixon force-pushed the znixon/tcpudp-rebase branch 3 times, most recently from 59dc0dd to dcc1533 Compare April 30, 2025 21:34

WestonReed approved these changes Apr 30, 2025

View reviewed changes

feature flag and add annotation for TCP_UDP

ce478b3

zac-nixon force-pushed the znixon/tcpudp-rebase branch from dcc1533 to ce478b3 Compare April 30, 2025 22:35

k8s-ci-robot assigned shraddhabang Apr 30, 2025

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

zac-nixon merged commit efff73d into kubernetes-sigs:main May 1, 2025
8 of 9 checks passed

zac-nixon deleted the znixon/tcpudp-rebase branch May 1, 2025 03:06

zac-nixon mentioned this pull request May 6, 2025

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

Merged

z0rc mentioned this pull request May 16, 2025

Dual TCP/UDP NLB shared across multiple services #1608

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Rebase + Bugfixes] Add support for TCP_UDP Listeners #4161

[Rebase + Bugfixes] Add support for TCP_UDP Listeners #4161

zac-nixon commented Apr 29, 2025

Uh oh!

lyda commented Apr 30, 2025

Uh oh!

shraddhabang Apr 30, 2025 •

edited

Loading

Uh oh!

zac-nixon Apr 30, 2025

Uh oh!

shraddhabang Apr 30, 2025 •

edited

Loading

Uh oh!

shraddhabang Apr 30, 2025

Uh oh!

zac-nixon Apr 30, 2025

Uh oh!

zac-nixon Apr 30, 2025

Uh oh!

shraddhabang Apr 30, 2025

Uh oh!

zac-nixon Apr 30, 2025

Uh oh!

shuqz Apr 30, 2025

Uh oh!

zac-nixon Apr 30, 2025

Uh oh!

k8s-ci-robot commented Apr 30, 2025

Uh oh!

shraddhabang commented Apr 30, 2025

Uh oh!

Uh oh!

amorey commented May 1, 2025 •

edited

Loading

Uh oh!

lyda commented May 1, 2025

Uh oh!

Uh oh!

[Rebase + Bugfixes] Add support for TCP_UDP Listeners #4161

[Rebase + Bugfixes] Add support for TCP_UDP Listeners #4161

Conversation

zac-nixon commented Apr 29, 2025

Issue

Description

Checklist

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

Uh oh!

lyda commented Apr 30, 2025

Uh oh!

shraddhabang Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shraddhabang Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

k8s-ci-robot commented Apr 30, 2025

Uh oh!

shraddhabang commented Apr 30, 2025

Uh oh!

Uh oh!

amorey commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lyda commented May 1, 2025

Uh oh!

Uh oh!

shraddhabang Apr 30, 2025 •

edited

Loading

shraddhabang Apr 30, 2025 •

edited

Loading

amorey commented May 1, 2025 •

edited

Loading