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

fix: account for tls termination in exposed port validation #5549

Merged
merged 3 commits into from
Dec 11, 2023
Merged

fix: account for tls termination in exposed port validation #5549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
00a1942
write fix and test case
Dec 11, 2023
af50d26
move and simplify logic
Dec 11, 2023
8fff698
Merge branch 'mainline' into manifest/fix-nlb-port-validation
mergify[bot] Dec 11, 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
7 changes: 7 additions & 0 deletions internal/pkg/manifest/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2226,6 +2226,7 @@ func validateAndPopulateNLBListenerPorts(listener NetworkLoadBalancerListener, p
if err != nil {
return err
}

port, err := strconv.ParseUint(aws.StringValue(nlbReceiverPort), 10, 16)
if err != nil {
return err
Expand All @@ -2242,6 +2243,11 @@ func validateAndPopulateNLBListenerPorts(listener NetworkLoadBalancerListener, p
targetProtocol = strings.ToUpper(aws.StringValue(nlbProtocol))
}

// Handle TLS termination of container exposed port protocol
if targetProtocol == TLS {
targetProtocol = TCP
}
Comment on lines 2243 to +2249
Copy link
Contributor

Choose a reason for hiding this comment

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

I really am not a fan of nested if-block, but I think it's more obvious this way that we set the target protocol to TCP if the listener protocol is TLS, not necessarily if the target protocol is "tls". Imagine if we expand target_port to accept a string of form 443/tls (for example to let people configure e2e encryption), then this code might become slightly confusing to refactor.

Suggested change
targetProtocol = strings.ToUpper(aws.StringValue(nlbProtocol))
}
// Handle TLS termination of container exposed port protocol
if targetProtocol == TLS {
targetProtocol = TCP
}
if strings.EqualFold(aws.StringValue(nlbProtocol), "tls") {
targetProtocol = TCP
} else {
targetProtocol = strings.ToUpper(aws.StringValue(nlbProtocol))
}


// Prefer `nlb.target_container`, then existing exposed port mapping, then fallback on name of main container
targetContainer := mainContainerName
if existingContainerNameAndProtocol, ok := portExposedTo[targetPort]; ok {
Expand All @@ -2256,6 +2262,7 @@ func validateAndPopulateNLBListenerPorts(listener NetworkLoadBalancerListener, p

func validateAndPopulateExposedPortMapping(portExposedTo map[uint16]containerNameAndProtocol, targetPort uint16, targetProtocol string, targetContainer string) error {
exposedContainerAndProtocol, alreadyExposed := portExposedTo[targetPort]
targetProtocol = strings.ToUpper(targetProtocol)

// Port is not associated with container and protocol, populate map
if !alreadyExposed {
Expand Down
22 changes: 20 additions & 2 deletions internal/pkg/manifest/validate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4196,7 +4196,25 @@ func TestValidateExposedPorts(t *testing.T) {
},
wanted: nil,
},
"should not error out when nlb target_port is same as that of sidecar container port but sidecar uses non default protocol": {
"should not error out when tls is terminated exposing a tcp port": {
in: validateExposedPortsOpts{
mainContainerName: "mockMainContainer",
mainContainerPort: aws.Uint16(8080),
sidecarConfig: map[string]*SidecarConfig{
"foo": {
Port: aws.String("80/tcp"),
},
},
nlb: &NetworkLoadBalancerConfiguration{
Listener: NetworkLoadBalancerListener{
Port: aws.String("8080/tls"),
TargetPort: aws.Int(80),
},
},
},
wanted: nil,
},
"should return an error when nlb target_port is same as that of sidecar container port but sidecar uses non default protocol": {
in: validateExposedPortsOpts{
mainContainerName: "mockMainContainer",
mainContainerPort: aws.Uint16(8080),
Expand All @@ -4212,7 +4230,7 @@ func TestValidateExposedPorts(t *testing.T) {
},
},
},
wanted: fmt.Errorf(`validate "nlb": container "foo" is exposing the same port 80 with protocol TCP and udp`),
wanted: fmt.Errorf(`validate "nlb": container "foo" is exposing the same port 80 with protocol TCP and UDP`),
},
"should return an error if alb target_port points to one sidecar container port and target_container points to another sidecar container": {
in: validateExposedPortsOpts{
Expand Down