Skip to content

fix: no extra port mapping in main container when nlb target is a sidecar #3819

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

Merged
mergify merged 3 commits into from
Jul 27, 2022
Merged

fix: no extra port mapping in main container when nlb target is a sidecar #3819

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
c19a30c
add extra container port to the main container only if nlb target con…
Lou1415926 Jul 26, 2022
13ca12c
small refactor
Lou1415926 Jul 26, 2022
5bbb68f
Merge branch 'mainline' into fix/nlb/sidecars/3768
mergify[bot] Jul 27, 2022
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
2 changes: 0 additions & 2 deletions internal/pkg/deploy/cloudformation/stack/testdata/workloads/svc-nlb-prod.stack.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,6 @@ Resources: # If a bucket URL is specified, that means the template exists.
awslogs-stream-prefix: copilot
PortMappings:
- ContainerPort: !Ref ContainerPort
- ContainerPort: 82
Protocol: tcp
- Name: tls
Image: 1234567890.dkr.ecr.us-west-2.amazonaws.com/proxy:cicdtest
PortMappings:
Expand Down
15 changes: 6 additions & 9 deletions internal/pkg/template/templates/workloads/partials/cf/workload-container.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,14 @@
{{- if eq .WorkloadType "Load Balanced Web Service"}}
PortMappings:
- ContainerPort: !Ref ContainerPort
{{- if .NLB}}
{{if ne .NLB.Listener.TargetPort .NLB.MainContainerPort}} {{/*No need to add additional port if the target port is the same as image port*/}}
- ContainerPort: {{.NLB.Listener.TargetPort}}
{{- if eq .NLB.Listener.Protocol "UDP" }}
Protocol: udp
{{- else }}
Protocol: tcp
{{- end }}
{{- if .NLB}} {{ $nlbListener := .NLB.Listener }}
{{- if and (eq $nlbListener.TargetContainer .WorkloadName) (ne $nlbListener.TargetPort .NLB.MainContainerPort)}}
Copy link
Copy Markdown
Contributor

@efekarakus efekarakus Jul 26, 2022

Choose a reason for hiding this comment

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

:ack: this looks good, what about this feedback from the issue:

It appears that the cf partial below is not sidecar aware, and assumes that the nlb target is always the main workload container.

To me it sounded like the NLB target group is pointing to the wrong port?

Copy link
Copy Markdown
Contributor Author

@Lou1415926 Lou1415926 Jul 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

With a manifest like this:

nlb:
  port: 80/tcp
  target_container: envoy

image:
  location: blah
  port: 10000

sidecars:
  envoy:
    # envoy admin port
    port: 9901
    image: public.ecr.aws/hashicorp/envoy-alpine:latest

The NLB target group's target port will be 9901, not 10000, with the current implementation and as expected!

My understanding for the quoted comment was specifically for the code:

copilot-cli/internal/pkg/template/templates/workloads/partials/cf/workload-container.yml

Line 33 in 2ac6b01

{{if ne .NLB.Listener.TargetPort .NLB.MainContainerPort}} {{/*No need to add additional port if the target port is the same as image port*/}}

The (bugged) code assume the target container is main container, and add the target port to main container's port mapping if it's not the same as the container port. It fails to take into consideration the possibility that the target container could be a sidecar, in which case, it does not need to add the extra port mapping to the main container.

Copy link
Copy Markdown
Contributor

@efekarakus efekarakus Jul 27, 2022

Choose a reason for hiding this comment

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

Thanks for the explanation, totally makes sense!

{{/*No need to add additional port if the target port is the same as image port*/}}
- ContainerPort: {{$nlbListener.TargetPort}}
Protocol: {{if eq $nlbListener.Protocol "UDP"}}udp{{else}}tcp{{end}}
{{- end}}
{{- end}}
{{- end}}
{{- end}} {{/* end if eq .WorkloadType "Load Balanced Web Service"*/}}
{{- if eq .WorkloadType "Backend Service"}}
PortMappings: !If [ExposePort, [{ContainerPort: !Ref ContainerPort}], !Ref "AWS::NoValue"]
{{- end}}
Expand Down