feat(ecs): allow load balancers to connect to any mapped port #3891

karlpatr · 2019-08-30T22:29:47Z

This patch allows explicit override of the existing default container selection when attaching a load balancer to an ecs task. By adding an optional targetPort to the properties of elbv2.AddApplicationTargetsProps and elbv2.AddNetworkTargetsProps and then exposing that property via the elbv2. ITargetGroup, the user can now explicitly declare that they want to connect to any mapped port inside a task.

When an ecs.BaseService is asked to create an attachment, it now loops over all ecs.PortMappings using a pair of internal helper functions to resolve the particular container name and host port to specify for the generated load balancer declaration. If the Load Balancer has a security group, the target port is also used to override the security ingress necessary to connect it to the Service. If no targetPort has been declared the previous functionality is used for backward compatibility. If a targetPort that isn't used by a container has been declared the system will throw an error saying load balancers can't be connected to unmapped ports.

I considered using the existing port on the elbv2.ITargetGroup to perform the mapping, but the comment declares it is the load balancer's listening port and that is how it was used in unit tests. Instead of making a breaking change by adding new meaning to that field I elected to create an explicit and optional one. I chose the name targetPort because that is the name used in the pattern module for load balanced ECS prefabs.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

* Proof of concept for explicit host port override when linking an elbv2.ITargetGroup to an ECS cluster/task * Added tests for new functionality, updated comments

mergify · 2019-08-30T22:30:25Z

Pull Request Checklist

Testing

Unit test added (prefer to add a new test rather than modify existing tests)
CLI change? Re-run/add CLI integration tests

Documentation

Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
README: update module README
Design: for significant features, follow the design process

Title uses the format type(scope): text

Type: fix, feat, refactor go into CHANGELOG, chore is hidden
Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
Style: use all lower-case, do not end with a period

Description

Rationale: describe rationale of change and approach taken
Issues: Indicate issues fixed via: fixes #xxx or closes #xxx
Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>

Sensitive Modules (requires 2 PR approvers)

IAM document library (in @aws-cdk/aws-iam)
EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
Grant APIs (if not based on official documentation with a reference)

mergify · 2019-08-30T22:30:28Z

Pull Request Checklist

Testing

Unit test added (prefer to add a new test rather than modify existing tests)
CLI change? Re-run/add CLI integration tests

Documentation

Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
README: update module README
Design: for significant features, follow the design process

Title uses the format type(scope): text

Type: fix, feat, refactor go into CHANGELOG, chore is hidden
Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
Style: use all lower-case, do not end with a period

Description

Rationale: describe rationale of change and approach taken
Issues: Indicate issues fixed via: fixes #xxx or closes #xxx
Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>

Sensitive Modules (requires 2 PR approvers)

IAM document library (in @aws-cdk/aws-iam)
EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
Grant APIs (if not based on official documentation with a reference)

rix0rrr · 2019-09-02T12:58:43Z

I considered using the existing port on the elbv2.ITargetGroup to perform the mapping, but the comment declares it is the load balancer's listening port and that is how it was used in unit tests.

Well, the documentation looks to be slightly off. It says the "listener"s port, but it's actually the port on which the instances in the target group are listening. I'd prefer using port for this functionality, as opposed to introducing a new targetPort parameter (which seems like it ultimately would mean the same thing).

I've got a question for you in the issue though: can you tell me why switching the order of your containers around doesn't work for you?

karlpatr · 2019-09-03T17:33:57Z

I responded to the question in the issue.

I can also quickly convert this code to use port instead of targetPort so long as it is understood this is a breaking change that impacts all uses of ECS because the current code ignores the incoming port entirely. The current load tests in the ECS package will break, for instance, and anyone who followed the instructions in the documentation will also be broken if they used a different port in the container than for their load balancer.

If you believe the correctness of using the port as designed moving forward outweighs the impact of having backward incompatibility and breaking installs in the wild, I will consolidate.

karlpatr · 2019-09-03T21:50:48Z

Updating the meaning of "port" in ITargetGroup to be the attachment to host instead of the load balancer port causes a need to rebuild load balancers due to a property change in the output json (for example group LBPublicListenerECSGroupD6A32205 in the integration tests). The updated mappings work as intended (I have deployed my infrastructure from this branch), but I think forcing existing infrastructure to rebuild is another layer of risk that repurposing the existing field would cause; as a CDK user I would not want a minor revision to cause any changes to my stable infra. The patch as written does not have this side effect.

karlpatr · 2019-09-03T23:07:11Z

For sake of discussion, this is what the differences would look like if the meaning of port changed instead of adding targetPort: https://github.com/karlpatr/aws-cdk/pull/2/files ... if that is 100% the decided direction to go I can just merge this patch into this PR.

iamhopaul123 · 2019-09-04T22:14:37Z

Hi @karlpatr, can you take a look at this design to see if that will solve the problem (I think multiple ports for one container is covered as one of the use cases)? And also needs your feedback.

iamhopaul123 · 2019-09-04T23:27:35Z

packages/@aws-cdk/aws-ecs/lib/container-definition.ts

+  public _findPortMapping(hostPort: number, protocol: Protocol): PortMapping | undefined {
+    for (const portMapping of this.portMappings) {
+      const p = portMapping.protocol || Protocol.TCP;
+      const h = portMapping.hostPort || portMapping.containerPort;


Will this fail if there are two containers in a service and both container expose port 80 but I want the first container to be attached to a target group and the second container to be attached to another target group? Unless I change the second container port to other than 80 or assign a host port for it.

Yes, this is reliant on having a distinct host port that can be targeted; my expectation while solving this problem was that the host must be able to respond to TCP traffic on distinct ports per requested listener.

I am not familiar enough with the fabric of ECS to know how things like dynamic port mapping are implemented, but I don't see how two incoming TCP connections to the same logical port could reach distinct containers. Specification of container names that implicitly map to host ports is another way to solve the same problem, but I didn't think that appropriate for the approach of extending ITargetGroup (the approach in the proposal doc is more loosely coupled and potentially better IMO).

ECS has three type of network mode: if it is aws_vpc or host, then the host port number should be the same with container port number. However, if it is bridge mode (default for EC2 tasks), you can specify a non-reserved host port for your container port mapping (or leave it undefined or 0), so that possibly two containers can have the same port number (mapped to different host ports). However, if this is the case and host port is not defined, it is hard to track through the host port, which will be any of the port range from 49153 through 65535.

Thanks for the explanation :)

This is probably moving off topic for the PR, but given that the host and container ports must match for aws_vpc or host mode, what happens if you attempt to use two containers with ports that collide in those modes?

It fails when you do cdk deploy.

iamhopaul123 · 2019-09-04T23:31:27Z

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/nlb/network-listener.ts

+   *
+   * @default Determined from target
+   */
+  readonly targetPort?: number;


Since this property is only used for ECS target, IMHO this might cause confusion to other non-ECS users.

That's a fair point; as noted in the discussion above I would have greatly preferred having the port of the target group refer to the ingress port of the connection in all cases, but I didn't want to break compatibility.

iamhopaul123 · 2019-09-05T00:18:14Z

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-listener.ts

@@ -501,6 +502,13 @@ export interface AddApplicationTargetsProps extends AddRuleProps {
   */
  readonly port?: number;

+  /**
+   * The port used to attach to targets.


Also, it seems like targetPort can be hostport or container port, which might lead to confusion. It would be better to only specify container port number (since users might care more about the container port when building microservices).

TargetPort must strictly be the ingress (host) port in this design since it must be accessible from outside the container by a non-ECS resource; as you noted above the container port is potentially not unique in a multi-container task definition. The mapping to host port allows resolution of these potential collisions. If the user specified the container port here the underlying system would need to convert it to a host port anyway.

rix0rrr · 2019-09-12T09:37:55Z

Sorry for taking so long to come back to this.

After loading more context about the CloudFormation model into my brain, I'm confused why this needs to involve changes to the ELB package at all.

Isn't the goal to have more control over the following property (at the CloudFormation level):

    "ServiceD69D759B": {
      "Type": "AWS::ECS::Service",
      "Properties": {
        // ...
        "LoadBalancers": [
          {
            "ContainerName": "web",
            "ContainerPort": 80,   <---- here
            "TargetGroupArn": { "Ref": "LBPublicListenerECSGroupD6A32205" }
          }
        ],

Seems like it would only necessitate changes in the ECS package, so that the correct port can be returned here:

aws-cdk/packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

Line 333 in bace776

containerPort: this.taskDefinition.defaultContainer!.containerPort,

I think a more generic design that @iamhopaul123 is proposing in #3922 will be a better way to achieve the same.

mergify · 2019-09-13T00:18:44Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

rix0rrr · 2019-09-16T07:59:47Z

If it's okay with you @karlpatr I'm going to close this PR. We'll wait for @iamhopaul123 to finish their work in #3922.

Thanks a lot for raising this issue and contributing to the discussion!

TargetGroup connections to non-default ECS ports (#1)

a5b66f2

* Proof of concept for explicit host port override when linking an elbv2.ITargetGroup to an ECS cluster/task * Added tests for new functionality, updated comments

mergify bot added the automerge-enabled label Aug 30, 2019

Merge branch 'master' into master

d5189b7

rix0rrr self-assigned this Sep 2, 2019

rix0rrr requested a review from piradeepk September 2, 2019 12:52

iamhopaul123 reviewed Sep 5, 2019

View reviewed changes

karlpatr mentioned this pull request Sep 9, 2019

design(ecs): multiple target group support design doc #3922

Closed

NGL321 removed the automerge-enabled label Sep 13, 2019

mergify bot added the automerge-enabled label Sep 13, 2019

NGL321 removed the automerge-enabled label Sep 13, 2019

rix0rrr closed this Sep 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ecs): allow load balancers to connect to any mapped port #3891

feat(ecs): allow load balancers to connect to any mapped port #3891

karlpatr commented Aug 30, 2019

mergify bot commented Aug 30, 2019

mergify bot commented Aug 30, 2019

rix0rrr commented Sep 2, 2019

karlpatr commented Sep 3, 2019

karlpatr commented Sep 3, 2019

karlpatr commented Sep 3, 2019

iamhopaul123 commented Sep 4, 2019 •

edited

Loading

iamhopaul123 Sep 4, 2019

karlpatr Sep 9, 2019

iamhopaul123 Sep 9, 2019

karlpatr Sep 9, 2019

iamhopaul123 Sep 9, 2019

iamhopaul123 Sep 4, 2019

karlpatr Sep 9, 2019

iamhopaul123 Sep 5, 2019

karlpatr Sep 9, 2019

rix0rrr commented Sep 12, 2019 •

edited

Loading

mergify bot commented Sep 13, 2019

rix0rrr commented Sep 16, 2019

feat(ecs): allow load balancers to connect to any mapped port #3891

feat(ecs): allow load balancers to connect to any mapped port #3891

Conversation

karlpatr commented Aug 30, 2019

mergify bot commented Aug 30, 2019

Pull Request Checklist

mergify bot commented Aug 30, 2019

Pull Request Checklist

rix0rrr commented Sep 2, 2019

karlpatr commented Sep 3, 2019

karlpatr commented Sep 3, 2019

karlpatr commented Sep 3, 2019

iamhopaul123 commented Sep 4, 2019 • edited Loading

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

Choose a reason for hiding this comment

rix0rrr commented Sep 12, 2019 • edited Loading

mergify bot commented Sep 13, 2019

rix0rrr commented Sep 16, 2019

iamhopaul123 commented Sep 4, 2019 •

edited

Loading

rix0rrr commented Sep 12, 2019 •

edited

Loading