Add support for TCP_UDP to NLB TargetGroups and Listeners (rebase) #3807
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lyda The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @lyda! |
|
Hi @lyda. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
|
Thanks for taking this up, I get to subscribe to another pull request now. Hope you have much better luck than the last one. Just in time for HTTP3 going more mainstream! |
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
lyda
force-pushed
the
tcpudp-lyda
branch
4 times, most recently
from
b025fee to
23e2f43
Compare
|
OK, I think I've addressed the issues with the rebase. But the verify step is now failing. Any ideas why? |
|
@M00nF1sh / @oliviassss : Hey there, I think this is done and reviewable. I've run it in a test EKS system and it does what I needed it to do. However I'm not an expert in this code base or with load balancers so would love feedback from yourselves. Thank you! |
lyda
force-pushed
the
tcpudp-lyda
branch
3 times, most recently
from
1fb8e33 to
2ab64b4
Compare
k8s-ci-robot
added
size/XXL
|
I've done manual testing and it worked fine. There really aren't docs to update. The information that the current aws-load-balancer-controller doesn't support listening to UDP and TCP on the same port number is information you'll find in the issues. Is there a slack/discord/irc channel I can chat about this to address any remaining issues? |
I believe developers (I'm not one of them) are somewhat reachable via https://slack.k8s.io/, channel |
| @@ -87,6 +87,9 @@ const ( | |||
|
|
|||
| // NetworkingProtocolUDP is the UDP protocol. | |||
| NetworkingProtocolUDP NetworkingProtocol = "UDP" | |||
|
|
|||
| // NetworkingProtocolTCP_UDP is the TCP_UDP protocol. | |||
| NetworkingProtocolTCP_UDP NetworkingProtocol = "TCP_UDP" | |||
There was a problem hiding this comment.
we don't need this.
the networking rules here is meant for security groups(which don't have a tcp_udp rule protocol)
instead of TCP_UDP, we should just generate two rules in TGB's networking, one for TCP and another one for UDP.
There was a problem hiding this comment.
I'm sorry, but we do. This is for AWS load balancer target groups, not security groups, and as much I find it annoying, TCP_UDP is a protocol for AWS load balancers. It's used in the single case where the TCP and UDP port numbers are the same. So, for example, if you're implementing a domain name server and need to listen on port 53 for both TCP and UDP, the load balancer must be configured to have a target group protocol of TCP_UDP. I have no idea why they did this, but they did.
| } | ||
| } | ||
|
|
||
| // execute build listener | ||
| for _, port := range t.service.Spec.Ports { |
There was a problem hiding this comment.
why not just loop over portMap instead of t.service.Spec.Ports. (we don't need this portMap[key] check this way)
There was a problem hiding this comment.
Because it's trying to find the case of the same port number being listened to on TCP and UDP. portMap is empty and is being used to find duplicate port numbers.
pkg/service/model_build_listener.go
Outdated
| @@ -234,3 +256,24 @@ func (t *defaultModelBuildTask) buildListenerAttributes(ctx context.Context, svc | |||
| } | |||
| return attributes, nil | |||
| } | |||
|
|
|||
| func mergeServicePortsForListener(ports []corev1.ServicePort) corev1.ServicePort { | |||
There was a problem hiding this comment.
this works but have two issues:
- we are hacking the k8s
corev1.ServicePortconcept by introduce a newcorev1.Protocol("TCP_UDP"), which don't exists in k8s, and in theory is an malformed `corev1.ServicePort. (TCP_UDP is not a validate Protocol enum) - this don't generate a warning/error if unsupported protocol/protocol conbination is found.
- this exists as a "glue" code to make the existing
buildListenersfunction happy, but in the long run would make the code harder to understand and maintain.
Instead, i think a better option would be:
- modify buildListeners to accept a list of corev1.ServicePort. inside buildListeners, it first validate whether this list of corev1.ServicePort can form a single listener(and error out when cannot), and generate listener of correct protocol.
There was a problem hiding this comment.
I'm confused, that's what buildListeners now does. It loops through the corev1.ServicePorts and determines if there's a case where the same port number exists for TCP and UDP. It then synthesises a single corev1.ServicePort from the original two that uses the TCP_UDP protocol that AWS load balancers support.
Either way it then calls buildListener.
I could change everything from buildListener on down to use an internal data structure that includes a slice of corev1.ServicePorts as well as the protocol being used for the load balancer. That seems like a rather complex change. And it will still have the rats nest of conditionals the current code has. Plus all corev1.ServicePort stuff would operate off the first element - so might as well just include a single corev1.ServicePort. And at that point all I've done is avoided abusing an enum.
That said, point number two is definitely valid and I'm now returning errors for a few scenarios up the chain.
|
Does it make sense to mark this PR as closing #1608? That should cause GitHub to link to this PR from that ticket, rather than relying on the comments. (It might also keep k8s-ci-robot from closing that issue as stale while this PR is in-progress... maybe?) I note that this PR isn't solving the actual use-case for which that ticket was opened, but the bulk of the discussion in that ticket was for this use-case, so tying them together makes sense to me. A solution for the original use-case would revolve around some kind of new |
|
Awesome, thanks for the review. I'll work through these this week. |
Good point. I'll turn that off because this is completely different to the original issue and just addresses a comment within. Sorry I missed that. |
There was a problem hiding this comment.
OK, I think I've gone through them all. I see your point on buildListener but I don't think the change will make it any better. The code will still have to pick through the three "protocols" (I really do dislike AWS's TCP_UDP "feature" here so I have to put quotes around protocols at least once) and that's going to make the config translation code more complex than just the binary choices before.
The real decision is: do you want to support this feature for the few of us who need it and pay the complexity price or do you not. I completely understand either choice. I would prefer the former obviously because I have zero interest in maintaining a fork, but that's a me problem, not a you problem.
| @@ -87,6 +87,9 @@ const ( | |||
|
|
|||
| // NetworkingProtocolUDP is the UDP protocol. | |||
| NetworkingProtocolUDP NetworkingProtocol = "UDP" | |||
|
|
|||
| // NetworkingProtocolTCP_UDP is the TCP_UDP protocol. | |||
| NetworkingProtocolTCP_UDP NetworkingProtocol = "TCP_UDP" | |||
There was a problem hiding this comment.
I'm sorry, but we do. This is for AWS load balancer target groups, not security groups, and as much I find it annoying, TCP_UDP is a protocol for AWS load balancers. It's used in the single case where the TCP and UDP port numbers are the same. So, for example, if you're implementing a domain name server and need to listen on port 53 for both TCP and UDP, the load balancer must be configured to have a target group protocol of TCP_UDP. I have no idea why they did this, but they did.
| } | ||
| } | ||
|
|
||
| // execute build listener | ||
| for _, port := range t.service.Spec.Ports { |
There was a problem hiding this comment.
Because it's trying to find the case of the same port number being listened to on TCP and UDP. portMap is empty and is being used to find duplicate port numbers.
pkg/service/model_build_listener.go
Outdated
| @@ -234,3 +256,24 @@ func (t *defaultModelBuildTask) buildListenerAttributes(ctx context.Context, svc | |||
| } | |||
| return attributes, nil | |||
| } | |||
|
|
|||
| func mergeServicePortsForListener(ports []corev1.ServicePort) corev1.ServicePort { | |||
There was a problem hiding this comment.
I'm confused, that's what buildListeners now does. It loops through the corev1.ServicePorts and determines if there's a case where the same port number exists for TCP and UDP. It then synthesises a single corev1.ServicePort from the original two that uses the TCP_UDP protocol that AWS load balancers support.
Either way it then calls buildListener.
I could change everything from buildListener on down to use an internal data structure that includes a slice of corev1.ServicePorts as well as the protocol being used for the load balancer. That seems like a rather complex change. And it will still have the rats nest of conditionals the current code has. Plus all corev1.ServicePort stuff would operate off the first element - so might as well just include a single corev1.ServicePort. And at that point all I've done is avoided abusing an enum.
That said, point number two is definitely valid and I'm now returning errors for a few scenarios up the chain.
k8s-ci-robot
added
the
needs-rebase
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]>
k8s-ci-robot
removed
the
needs-rebase
This is a work in progress - I need to test it.
Issue
#1608 (comment)
And based on this PR: #2275
Description
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.
Checklist
README.md, or thedocsdirectory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯