Add TCPRoute and UDPRoute Support for L4 Load Balancing #3688
Conversation
|
Hi @Skcey! Welcome to the project! 🎉 Thanks for opening this pull request! |
|
✅ All required contributors have signed the F5 CLA for this PR. Thank you! |
|
I have hereby read the F5 CLA and agree to its terms |
|
recheck |
|
@Skcey Can you please rebase this PR? There are a couple changes that went into main that are causing compilation errors. Also, the clusterrole in the helm chart needs to be updated to support these routes and their statuses so the controller can actually handle them. |
| type PortInfo struct { | ||
| Port int32 | ||
| Protocol corev1.Protocol | ||
| } | ||
|
|
There was a problem hiding this comment.
Just curious, why did you make this struct?
From the way this is being used, it feels like you can reference Protocol on its own.
For example, on line 144, we can define ports := make(map[int32]corev1.Protocol)
Line 155 then becomes ports[int32(listener.Port)] = protocol
Then the loop on line 473 becomes this
for port, protocol := range ports {
servicePort := corev1.ServicePort{
Name: fmt.Sprintf("port-%d", port),
Port: port,
TargetPort: intstr.FromInt32(port),
Protocol: protocol,Would love to know what you think though. Do please tell me if I'm overlooking anything.
| if len(l.L4Routes) > 1 { | ||
| fmt.Printf( | ||
| "WARN: Listener %s has %d TCPRoutes, which is not supported. Skipping.", | ||
| l.Name, | ||
| len(l.L4Routes), | ||
| ) | ||
| continue | ||
| } |
There was a problem hiding this comment.
If this warning occurs, will the user see this information in their resource status?
There was a problem hiding this comment.
We shouldn't be using print statements in the code anyway. We have a logger to use. Regardless, we should be doing validation and setting status conditions before we even get to this point in the code. It would be when we build the graph, which is the initial step. If we're building the configuration here, this validation should have already occurred.
There was a problem hiding this comment.
Hi @Skcey thanks so much for this contribution! The core architecture looks good and follows the existing patterns well. As a first pass, I'd like to see a few improvements:
- Replace
fmt.Printfdebug statements with proper structured logging using logr.Logger - Fix the serviceResolver.Resolve() calls in TCP/UDP upstream functions to include the missing logger parameter
- Add comprehensive unit test coverage for all new functions, following existing test patterns
- As Saylor pointed out above, the RBAC permissions need to be updated and your branch needs to be rebased
- If possible, could we reduce code duplication between TCP and UDP implementations
Please feel free to reach out with any questions or if you need any help with anything!
| "k8s.io/apimachinery/pkg/util/validation/field" | ||
| "sigs.k8s.io/gateway-api/apis/v1alpha2" | ||
|
|
||
| "github.com/nginx/nginx-gateway-fabric/internal/controller/state/conditions" |
There was a problem hiding this comment.
| "github.com/nginx/nginx-gateway-fabric/internal/controller/state/conditions" | |
| "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/conditions" |
| "k8s.io/apimachinery/pkg/util/validation/field" | ||
| "sigs.k8s.io/gateway-api/apis/v1alpha2" | ||
|
|
||
| "github.com/nginx/nginx-gateway-fabric/internal/controller/state/conditions" |
There was a problem hiding this comment.
| "github.com/nginx/nginx-gateway-fabric/internal/controller/state/conditions" | |
| "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/conditions" |
|
|
||
| allowedAddressType := getAllowedAddressType(ipFamily) | ||
|
|
||
| eps, err := serviceResolver.Resolve(ctx, br.SvcNsName, br.ServicePort, allowedAddressType) |
There was a problem hiding this comment.
This function call needs to be updated as it's missing the logger parameter - see the pattern in buildUpstreams:
eps, err := svcResolver.Resolve(ctx, logger, br.SvcNsName, br.ServicePort, allowedAddressType)
|
|
||
| allowedAddressType := getAllowedAddressType(ipFamily) | ||
|
|
||
| eps, err := serviceResolver.Resolve(ctx, br.SvcNsName, br.ServicePort, allowedAddressType) |
There was a problem hiding this comment.
As above - this function call needs to be updated as it's missing the logger parameter - see the pattern in buildUpstreams:
eps, err := svcResolver.Resolve(ctx, logger, br.SvcNsName, br.ServicePort, allowedAddressType)
github-project-automation
bot
moved this from 🆕 New
to 🏗 In Progress
in NGINX Gateway Fabric
Thanks for the reminder! I’ll keep these issues in mind when I modify the code later. It’s just that I might not have time to update the code for about a week. |
Thank you so much for your feedback! I will definitely make improvements based on these points. However, I’ll be quite busy over the next week and may not be able to work on this task. I expect to have time to make the changes and refinements the week after next |
Proposed changes
Problem:
NGF currently lacks support for TCPRoute and UDPRoute resources, which are essential for managing Layer 4 (TCP/UDP) traffic via the Gateway API.
Solution:
This PR adds basic support for TCPRoute and UDPRoute to NGF, enabling Layer 4 load balancing. Key implementations include:
Testing:
Packaged into an image and tested in my environment.
Closes #3687
Checklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.