Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support UDP listeners in Network Load Balancer #4980

Merged
merged 10 commits into from
Jul 14, 2023
Merged

feat: Support UDP listeners in Network Load Balancer #4980

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d053f7b
Initial support for NLB UDP listeners
tjhorner Jun 11, 2023
cb15323
Fix (NetworkLoadBalancerListener).exposedPorts for TLS listeners and …
tjhorner Jun 13, 2023
5c4523b
Additional tests; revert erroneous documentation change
tjhorner Jun 13, 2023
163bce3
More descriptive comment
tjhorner Jun 13, 2023
2834406
Add UDP listener docs
tjhorner Jun 13, 2023
7c5a96a
Merge branch 'mainline' into feature/nlb-udp-ports
Lou1415926 Jun 14, 2023
b46913e
Merge branch 'aws:mainline' into feature/nlb-udp-ports
tjhorner Jun 21, 2023
413dbf4
convert lower case altogether
Lou1415926 Jul 13, 2023
1b27c0b
Merge branch 'mainline' into feature/nlb-udp-ports
Lou1415926 Jul 14, 2023
ea15d8a
Merge branch 'mainline' into feature/nlb-udp-ports
mergify[bot] Jul 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions internal/pkg/manifest/lb_web_svc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2641,6 +2641,9 @@ func TestLoadBalancedWebService_ExposedPorts(t *testing.T) {
TargetPort: aws.Int(8083),
TargetContainer: aws.String("xray"),
},
{
Port: aws.String("8084/udp"),
},
},
},
},
Expand All @@ -2653,6 +2656,12 @@ func TestLoadBalancedWebService_ExposedPorts(t *testing.T) {
Protocol: "tcp",
isDefinedByContainer: true,
},
{
Port: 8084,
ContainerName: "frontend",
Protocol: "udp",
isDefinedByContainer: false,
},
},
"xray": {
{
Expand Down
15 changes: 12 additions & 3 deletions internal/pkg/manifest/svc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ func (cfg NetworkLoadBalancerListener) exposedPorts(exposedPorts []ExposedPort,
if cfg.IsEmpty() {
return nil, nil
}
nlbPort, _, err := ParsePortMapping(cfg.Port)
nlbPort, nlbProtocol, err := ParsePortMapping(cfg.Port)
if err != nil {
return nil, err
}
Expand All @@ -513,19 +513,28 @@ func (cfg NetworkLoadBalancerListener) exposedPorts(exposedPorts []ExposedPort,
if cfg.TargetPort != nil {
targetPort = uint16(aws.IntValue(cfg.TargetPort))
}
targetProtocol := strings.ToLower(TCP)
if nlbProtocol != nil {
protocol := strings.ToLower(aws.StringValue(nlbProtocol))
// Expose TCP port for TLS listeners.
if protocol != strings.ToLower(TLS) {
targetProtocol = protocol
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL! Will update to use this

}
for _, exposedPort := range exposedPorts {
if targetPort == exposedPort.Port {
if targetPort == exposedPort.Port && targetProtocol == exposedPort.Protocol {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that's fine. Thanks!

return nil, nil
}
}
targetContainer := workloadName
if cfg.TargetContainer != nil {
targetContainer = aws.StringValue(cfg.TargetContainer)
}

return []ExposedPort{
{
Port: targetPort,
Protocol: "tcp",
Protocol: targetProtocol,
ContainerName: targetContainer,
},
}, nil
Expand Down
2 changes: 1 addition & 1 deletion internal/pkg/manifest/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ var (

essentialContainerDependsOnValidStatuses = []string{dependsOnStart, dependsOnHealthy}
dependsOnValidStatuses = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess, dependsOnHealthy}
nlbValidProtocols = []string{TCP, TLS}
nlbValidProtocols = []string{TCP, udp, TLS}
validContainerProtocols = []string{TCP, udp}
tracingValidVendors = []string{awsXRAY}
ecsRollingUpdateStrategies = []string{ECSDefaultRollingUpdateStrategy, ECSRecreateRollingUpdateStrategy}
Expand Down
14 changes: 6 additions & 8 deletions internal/pkg/manifest/validate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1767,7 +1767,7 @@ func TestNetworkLoadBalancerConfiguration_validate(t *testing.T) {
},
},
wantedErrorMsgPrefix: `validate "nlb": `,
wantedError: fmt.Errorf(`validate "port": invalid protocol tps; valid protocols include TCP and TLS`),
wantedError: fmt.Errorf(`validate "port": invalid protocol tps; valid protocols include TCP, UDP and TLS`),
},
"fail if protocol is not recognized in additional listeners": {
nlb: NetworkLoadBalancerConfiguration{
Expand All @@ -1781,7 +1781,7 @@ func TestNetworkLoadBalancerConfiguration_validate(t *testing.T) {
},
},
wantedErrorMsgPrefix: `validate "nlb": `,
wantedError: fmt.Errorf(`validate "additional_listeners[0]": validate "port": invalid protocol tps; valid protocols include TCP and TLS`),
wantedError: fmt.Errorf(`validate "additional_listeners[0]": validate "port": invalid protocol tps; valid protocols include TCP, UDP and TLS`),
},
"success if tcp": {
nlb: NetworkLoadBalancerConfiguration{
Expand All @@ -1790,15 +1790,14 @@ func TestNetworkLoadBalancerConfiguration_validate(t *testing.T) {
},
},
},
"error if udp": {
"success if udp": {
nlb: NetworkLoadBalancerConfiguration{
Listener: NetworkLoadBalancerListener{
Port: aws.String("161/udp"),
},
},
wantedError: fmt.Errorf(`validate "port": invalid protocol udp; valid protocols include TCP and TLS`),
},
"error if udp in additional listeners": {
"success if udp in additional listeners": {
nlb: NetworkLoadBalancerConfiguration{
Listener: NetworkLoadBalancerListener{
Port: aws.String("161/tcp"),
Expand All @@ -1809,7 +1808,6 @@ func TestNetworkLoadBalancerConfiguration_validate(t *testing.T) {
},
},
},
wantedError: fmt.Errorf(`validate "additional_listeners[0]": validate "port": invalid protocol udp; valid protocols include TCP and TLS`),
},
"error if additional listeners are defined before main listener": {
nlb: NetworkLoadBalancerConfiguration{
Expand Down Expand Up @@ -1846,7 +1844,7 @@ func TestNetworkLoadBalancerConfiguration_validate(t *testing.T) {
Port: aws.String("443/TCP_udp"),
},
},
wantedError: fmt.Errorf(`validate "port": invalid protocol TCP_udp; valid protocols include TCP and TLS`),
wantedError: fmt.Errorf(`validate "port": invalid protocol TCP_udp; valid protocols include TCP, UDP and TLS`),
},
"error if tcp_udp in additional listeners": {
nlb: NetworkLoadBalancerConfiguration{
Expand All @@ -1859,7 +1857,7 @@ func TestNetworkLoadBalancerConfiguration_validate(t *testing.T) {
},
},
},
wantedError: fmt.Errorf(`validate "additional_listeners[0]": validate "port": invalid protocol TCP_udp; valid protocols include TCP and TLS`),
wantedError: fmt.Errorf(`validate "additional_listeners[0]": validate "port": invalid protocol TCP_udp; valid protocols include TCP, UDP and TLS`),
},
"error if hosted zone is set": {
nlb: NetworkLoadBalancerConfiguration{
Expand Down
2 changes: 1 addition & 1 deletion site/content/docs/include/nlb-additionallisteners.en.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<span class="parent-field">nlb.additional_listeners.</span><a id="nlb-additional-listeners-port" href="#nlb-additional-listeners-port" class="field">`port`</a> <span class="type">String</span>
Required. The additional port and protocol for the Network Load Balancer to listen on.

Accepted protocols include `tcp` and `tls`. If the protocol is not specified, `tcp` is used by default.
Accepted protocols include `tcp`, `udp` and `tls`. If the protocol is not specified, `tcp` is used by default.

<span class="parent-field">nlb.additional_listeners.</span><a id="nlb-additional-listeners-healthcheck" href="#nlb-additional-listeners-healthcheck" class="field">`healthcheck`</a> <span class="type">Map</span>
Specify the health check configuration for your additional listener on the Network Load Balancer.
Expand Down
2 changes: 1 addition & 1 deletion site/content/docs/include/nlb.en.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ at least one of Application Load Balancer and Network Load Balancer must be enab
<span class="parent-field">nlb.</span><a id="nlb-port" href="#nlb-port" class="field">`port`</a> <span class="type">String</span>
Required. The port and protocol for the Network Load Balancer to listen on.

Accepted protocols include `tcp` and `tls`. If the protocol is not specified, `tcp` is used by default. For example:
Accepted protocols include `tcp`, `udp` and `tls`. If the protocol is not specified, `tcp` is used by default. For example:
```yaml
nlb:
port: 80
Expand Down
2 changes: 1 addition & 1 deletion site/content/docs/manifest/lb-web-service.en.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

- port: 8084/tcp # Traffic on port 8084/tcp is forwarded to the main container, on port 8084.
- port: 8085/tcp # Traffic on port 8085/tcp is forwarded to the sidecar "envoy", on port 3000.
target_port: 3000
Expand Down
2 changes: 1 addition & 1 deletion site/content/docs/manifest/lb-web-service.ja.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@

nlb:
port: 8080/tcp # Traffic on port 8080/tcp is forwarded to the main container, on port 8080.
additional_rules:
additional_listeners:
- port: 8084/tcp # Traffic on port 8084/tcp is forwarded to the main container, on port 8084.
- port: 8085/tcp # Traffic on port 8085/tcp is forwarded to the sidecar "envoy", on port 3000.
target_port: 3000
Expand Down