feat: Support UDP listeners in Network Load Balancer #4980

tjhorner · 2023-06-13T16:02:48Z

This PR adds basic UDP support for Network Load Balancers. For example, you can now do this in a Load Balanced Web Service:

nlb:
  port: 1812/udp
  # Run the healthcheck against a different TCP port
  healthcheck:
    port: 8080

Additional listeners are also supported:

nlb:
  # [...]
  additional_listeners:
    - port: 1912/udp
    # healthcheck omitted

I also updated the English documentation to reflect this change, but not the Japanese documentation.

This addresses issue #4767.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

…TCP defaults

github-actions · 2023-06-13T16:35:07Z

🍕 Here are the new binary sizes!

Name	New size (kiB)	size (kiB)	Delta (%)
macOS (amd)	51376	50748	🥺 +1.24
macOS (arm)	51584	50956	🥺 +1.23
linux (amd)	45224	44680	🥺 +1.22
linux (arm)	43524	43012	🥺 +1.19
windows (amd)	42052	41552	🥺 +1.20

codecov-commenter · 2023-06-14T19:19:06Z

Codecov Report

Merging #4980 (413dbf4) into mainline (d6c5279) will increase coverage by 24.37%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##           mainline    #4980       +/-   ##
=============================================
+ Coverage     70.03%   94.41%   +24.37%     
=============================================
  Files           289       13      -276     
  Lines         42031     1521    -40510     
  Branches        285      285               
=============================================
- Hits          29435     1436    -27999     
+ Misses        11176       80    -11096     
+ Partials       1420        5     -1415

see 276 files with indirect coverage changes

Lou1415926

Thank you very much for the contribution ❤️ ! This looks great, I just have some small nits!

Lou1415926 · 2023-06-22T16:43:57Z

internal/pkg/manifest/svc.go

+		protocol := strings.ToLower(aws.StringValue(nlbProtocol))
+		// Expose TCP port for TLS listeners.
+		if protocol != strings.ToLower(TLS) {
+			targetProtocol = protocol
+		}


Yay thanks for taking care of this case!!

Small nit - strings.EqualFold is case-insensitive, so we can use it for the comparison here

Suggested change

protocol := strings.ToLower(aws.StringValue(nlbProtocol))

// Expose TCP port for TLS listeners.

if protocol != strings.ToLower(TLS) {

targetProtocol = protocol

}

// Expose TCP port for TLS listeners.

if !strings.EqualFold(aws.StringValue(nlbProtocol), TLS)

targetProtocol = protocol

}

TIL! Will update to use this

Lou1415926 · 2023-06-22T17:02:36Z

internal/pkg/manifest/svc.go

 	for _, exposedPort := range exposedPorts {
-		if targetPort == exposedPort.Port {
+		if targetPort == exposedPort.Port && targetProtocol == exposedPort.Protocol {


nit:

Suggested change

if targetPort == exposedPort.Port && targetProtocol == exposedPort.Protocol {

// NOTE: ECS doesn't support exposing the same port twice.

// This is a feature request to support multiple protocols on the same port: https://github.com/aws/containers-roadmap/issues/850.

if targetPort == exposedPort.Port {

ECS doesn't let you expose the same port twice, even if they are different protocols. Although as a client side tool we don't have to do the same validations as the service, I think I'd prefer erroring out early here.

NLB can listen on a port for TPC_UDP traffic - judging by this design pattern, it is possible that when ECS does support this feature, they will support an "additional protocol" called TCP_UDP, instead of letting people expose the same port twice. This is my own personal intuition though. Here is a related feature request to ECS: aws/containers-roadmap#850.

Anyway, based on this observation, it's likely that we won't need to change this if statement even after they'e supported the feature. So maintenance-wise, this probably won't cause burden in the future anyway.

Sure, that makes sense. I'll add that note and revert the change there.

Wouldn't this just silently fail? No err is being returned, but I'm not sure if consumers of this method would interpret a nil []ExposedPort as a failure.

My bad!! I somehow was thinking this code was written in manifest/validate.go, and was trying to point out that the "uniqueness" of a port is determined by the port only, not port + protocol.

Now that I've had my coffee, I realize this comment doesn't make sense for the code here. This function is called after we've done all the manifest validation. At this point, it should be assumed that the manifest is totally valid - that is, no port is exposed twice.

Therefore, I think we can keep it as you have (port + protocol). Since the validation already happened, there is effectively no difference between if targetPort == exposedPort.Port and if targetPort == exposedPort.Port && targetProtocol == exposedPort.Protocol; however, the latter is clearly and makes more sense 😂

The validation code already checks that "no port is exposed twice" - it doesn't take protocol into consideration. So we are good there as well.

Sorry for the confusion!!

hey there @tjhorner ! Do you want me to take over the code and address the nit, so that this PR can be merged before our next release?

Sure, that's fine. Thanks!

dannyrandall

awesome! thank you @tjhorner!!!

dannyrandall · 2023-06-13T17:25:19Z

site/content/docs/manifest/lb-web-service.en.md

@@ -241,7 +241,7 @@ List of all available properties for a `'Load Balanced Web Service'` manifest. T

        nlb:
          port: 8080/tcp              # Traffic on port 8080/tcp is forwarded to the main container, on port 8080.
-          additional_rules:  
+          additional_listeners:  


good catch!

tjhorner added 4 commits June 13, 2023 08:32

Initial support for NLB UDP listeners

d053f7b

Fix (NetworkLoadBalancerListener).exposedPorts for TLS listeners and …

cb15323

…TCP defaults

Additional tests; revert erroneous documentation change

5c4523b

More descriptive comment

163bce3

tjhorner requested review from hkford and a team as code owners June 13, 2023 16:02

tjhorner requested review from Lou1415926 and removed request for a team June 13, 2023 16:02

tjhorner changed the title ~~Support UDP listeners in Network Load Balancer~~ feat: Support UDP listeners in Network Load Balancer Jun 13, 2023

Add UDP listener docs

2834406

Merge branch 'mainline' into feature/nlb-udp-ports

7c5a96a

Lou1415926 mentioned this pull request Jun 20, 2023

Support exposing UDP ports when using Network Load Balancer #4767

Closed

Merge branch 'aws:mainline' into feature/nlb-udp-ports

b46913e

Lou1415926 approved these changes Jun 22, 2023

View reviewed changes

convert lower case altogether

413dbf4

Lou1415926 force-pushed the feature/nlb-udp-ports branch from 02f0cdd to 413dbf4 Compare July 13, 2023 17:24

dannyrandall approved these changes Jul 13, 2023

View reviewed changes

Lou1415926 and others added 2 commits July 14, 2023 14:35

Merge branch 'mainline' into feature/nlb-udp-ports

1b27c0b

Merge branch 'mainline' into feature/nlb-udp-ports

ea15d8a

mergify bot merged commit 5e6a06e into aws:mainline Jul 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support UDP listeners in Network Load Balancer #4980

feat: Support UDP listeners in Network Load Balancer #4980

tjhorner commented Jun 13, 2023 •

edited

Loading

github-actions bot commented Jun 13, 2023 •

edited

Loading

codecov-commenter commented Jun 14, 2023 •

edited

Loading

Lou1415926 left a comment

Lou1415926 Jun 22, 2023

tjhorner Jun 23, 2023

Lou1415926 Jun 22, 2023

tjhorner Jun 23, 2023

Lou1415926 Jun 23, 2023

Lou1415926 Jul 12, 2023

tjhorner Jul 12, 2023

dannyrandall left a comment

dannyrandall Jun 13, 2023

feat: Support UDP listeners in Network Load Balancer #4980

feat: Support UDP listeners in Network Load Balancer #4980

Conversation

tjhorner commented Jun 13, 2023 • edited Loading

github-actions bot commented Jun 13, 2023 • edited Loading

codecov-commenter commented Jun 14, 2023 • edited Loading

Codecov Report

Lou1415926 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannyrandall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjhorner commented Jun 13, 2023 •

edited

Loading

github-actions bot commented Jun 13, 2023 •

edited

Loading

codecov-commenter commented Jun 14, 2023 •

edited

Loading