feat(ecs): allow load balancing to any container and port of service #4107

iamhopaul123 · 2019-09-16T21:45:53Z

Add a loadBalancerTarget() method to ECS's Service, which allows load balancing
to containers other than the default container and to ports beside the first mapped port.

Services can be registered to multiple ELBv2 target groups at the same time if necessary.

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

mergify · 2019-09-16T21:46:30Z

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

iamhopaul123 · 2019-09-16T22:15:01Z

@karlpatr please review.

aws-cdk-automation · 2019-09-16T22:32:26Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

karlpatr

I think this is much cleaner than my own first attempt, which I'll be closing in favor of this code. I look forward to seeing it published so I can adopt my project code.

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

karlpatr · 2019-09-17T00:45:20Z

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

@@ -232,6 +295,66 @@ export abstract class BaseService extends Resource
    return ret;
  }

+  public loadBalancerTarget(options: LoadBalancerTargetOptions): IEcsLoadBalancerTarget {


I'm assuming doc comments are a next step after these reviews, but it will be important to highlight how this factory function is used since it's not a common CDK pattern (from what I can tell)

Yeah, I think this docstring needs a rewrite. Some things I want to highlight:

This method is called to register the

It doesn't have a side effect, so I wouldn't phrase it like this.

Don't call this function alone.

Avoid using negatives. Tell me what I should be doing instead!

Suggested phrasing:

Return a load balancing target for a specific container and port Use this function to create a load balancer target if you want to load balance to another container than the first essential container or the first mapped port on the container. Use the return value of this function where you would normally use a load balancer target, instead of the `Service` object itself. @example listener.addTarget(service.loadBalancerTarget({ containerName: 'MyContainer', containerPort: 1234 }));

Very helpful feedback. Thanks!

karlpatr · 2019-09-17T00:57:47Z

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

+    };
+  }
+
+  public registerContainerTargets(containers: TargetProps[]) {


It looks like this is implemented in terms loadBalancerTarget; that's a good thing for maintenance and answers my earlier concerns about feature parity, but again leaves me wondering if 2 different interfaces is a boon in practice if I'm trying to discover how to use the system.

Yes, I'm not sure why we need to invert the call pattern here. Why is the current mechanism of targetGroup.addTarget() not good enough?

Using registerContainerTargets we could potentially make some setting (e.g., target group port) opinionated so that users don't need to set up some properties. Also this is a team decision and we feel like it would make more senses for some users to start from service instead of listener or target group. However, we might be wrong since we aren't real users. I agree that this implementation needs some tweaks, if we decide to keep it.

Would you be open to separating the concerns?

As in, add the fine-grained target control in this PR, and add the convenience function in a future PR? I would like to separate them out very cleanly.

If you insist on adding it, what I would like to do is mark the function and every new interface that is used to support it @experimental.

I am more inclined to put it in another PR so that I can invite my teammate to review and help refine the implementation, because we feel like this is a behavior we should support.

packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts

rix0rrr

I think maybe it's also worthwhile to reimplement the default attachToApplicationLoadBalancer() as such to make sure we converge to a single code path as quickly as possible:

class BaseService {
  public attachToApplictionLoadBalancer(args: Args) {
    return this.defaultLoadBalancerTarget.attachToApplicationLoadBalancer(args);
  }

  private get defaultLoadBalancerTarget() {
    return this.loadBalancerTarget({
      containerName: this.taskDefinition.defaultContainer!.containerName,
      containerPort: this.taskDefinition.defaultContainer!.containerPort,
    });
  }
}

Put into the docstring of XxxService that if it is used as a load balancer target, it will route to the default port of the default container, and that people should use loadBalancerTarget() to route traffic somewhere else.

Don't forget to describe this in README.md as well, and give an example of the new usage.

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

rix0rrr · 2019-09-17T08:48:22Z

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

+    };
+  }
+
+  public registerContainerTargets(containers: TargetProps[]) {


Yes, I'm not sure why we need to invert the call pattern here. Why is the current mechanism of targetGroup.addTarget() not good enough?

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

rix0rrr · 2019-09-17T09:00:12Z

packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts

+      };
+    }
+
+    if (this.networkMode === NetworkMode.BRIDGE) {


Is the order of ifs here correct? Do we really check port mappings before checking network mode?

Not sure if I fully understand what you mean. Is there any problem with the logic: check if port mapping exists then determine the security group port to open?

If the user somehow specifies a host port in bridge mode (and this is a public function so they could call it directly with too much information) I believe the early return would cause an incorrect binding to that port instead of the ephemeral range. Moving this above the port check would fix that edge case.

But the user specifies a host port in bridge mode, it should return the host port right? I think it returns the ephemeral range only if the network mode is bridge and the host port is 0 (or undefined). See hostPort.

You're right, disregard this one :)

Just trying to be sure. This looks and feels like hairy logic.

packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts

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

packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts

aws-cdk-automation · 2019-09-17T22:42:35Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr

I like it. Almost there, just some small niggles.

packages/@aws-cdk/aws-ecs/README.md

rix0rrr · 2019-09-18T08:48:09Z

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

@@ -232,6 +295,66 @@ export abstract class BaseService extends Resource
    return ret;
  }

+  public loadBalancerTarget(options: LoadBalancerTargetOptions): IEcsLoadBalancerTarget {


Yeah, I think this docstring needs a rewrite. Some things I want to highlight:

This method is called to register the

It doesn't have a side effect, so I wouldn't phrase it like this.

Don't call this function alone.

Avoid using negatives. Tell me what I should be doing instead!

Suggested phrasing:

Return a load balancing target for a specific container and port Use this function to create a load balancer target if you want to load balance to another container than the first essential container or the first mapped port on the container. Use the return value of this function where you would normally use a load balancer target, instead of the `Service` object itself. @example listener.addTarget(service.loadBalancerTarget({ containerName: 'MyContainer', containerPort: 1234 }));

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

rix0rrr · 2019-09-18T08:52:13Z

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

+    };
+  }
+
+  public registerContainerTargets(containers: TargetProps[]) {


Would you be open to separating the concerns?

As in, add the fine-grained target control in this PR, and add the convenience function in a future PR? I would like to separate them out very cleanly.

If you insist on adding it, what I would like to do is mark the function and every new interface that is used to support it @experimental.

packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts

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

aws-cdk-automation · 2019-09-18T21:35:32Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

…4107) Add a `loadBalancerTarget()` method to ECS's Service, which allows load balancing to containers other than the default container and to ports beside the first mapped port. Services can be registered to multiple ELBv2 target groups at the same time if necessary.

iamhopaul123 requested review from rix0rrr, SoManyHs and piradeepk September 16, 2019 21:45

iamhopaul123 added the pr/do-not-merge This PR should not be merged at this time. label Sep 16, 2019

karlpatr reviewed Sep 17, 2019

View reviewed changes

rix0rrr changed the title ~~feat(ecs): multiple target group support~~ feat(ecs): more control over load balancer attachments Sep 17, 2019

rix0rrr changed the title ~~feat(ecs): more control over load balancer attachments~~ feat(ecs): more control over load balancer targeting Sep 17, 2019

rix0rrr reviewed Sep 17, 2019

View reviewed changes

iamhopaul123 force-pushed the multiple-target-groups branch from 495912b to f498e8d Compare September 17, 2019 21:56

rix0rrr reviewed Sep 18, 2019

View reviewed changes

iamhopaul123 added 2 commits September 18, 2019 12:15

Add multiple target group support implementation

41c7e2c

Refactor and add CLB support

0f623e4

iamhopaul123 force-pushed the multiple-target-groups branch 2 times, most recently from 7e108fd to e6c97ad Compare September 18, 2019 19:19

Refactor CLB, fix README, add CLB integ test, and separate new API

5e4135e

iamhopaul123 force-pushed the multiple-target-groups branch from e6c97ad to 5e4135e Compare September 18, 2019 20:50

aws deleted a comment from aws-cdk-automation Sep 18, 2019

rix0rrr changed the title ~~feat(ecs): more control over load balancer targeting~~ feat(ecs): allow load balancing to any container and port of service Sep 19, 2019

rix0rrr approved these changes Sep 19, 2019

View reviewed changes

rix0rrr merged commit c3b3c93 into aws:master Sep 19, 2019

iamhopaul123 mentioned this pull request Oct 4, 2019

feat(ecs): add a new API for registering ECS targets #4212

Merged

piradeepk mentioned this pull request Oct 7, 2019

[ECS] Support Multiple Target Groups #3715

Closed

2 tasks

lbjay mentioned this pull request Jan 15, 2020

Does ApplicationLoadBalancedFargateService support task definitions with multiple containers/ports? #5506

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ecs): allow load balancing to any container and port of service #4107

feat(ecs): allow load balancing to any container and port of service #4107

iamhopaul123 commented Sep 16, 2019 •

edited by rix0rrr

mergify bot commented Sep 16, 2019

iamhopaul123 commented Sep 16, 2019

aws-cdk-automation commented Sep 16, 2019

karlpatr left a comment

karlpatr Sep 17, 2019

rix0rrr Sep 18, 2019

iamhopaul123 Sep 18, 2019

karlpatr Sep 17, 2019

rix0rrr Sep 17, 2019

iamhopaul123 Sep 17, 2019

rix0rrr Sep 18, 2019

iamhopaul123 Sep 18, 2019

rix0rrr left a comment •

edited

rix0rrr Sep 17, 2019

rix0rrr Sep 17, 2019

iamhopaul123 Sep 17, 2019

karlpatr Sep 17, 2019

iamhopaul123 Sep 17, 2019

karlpatr Sep 17, 2019

rix0rrr Sep 18, 2019

aws-cdk-automation commented Sep 17, 2019

rix0rrr left a comment

rix0rrr Sep 18, 2019

rix0rrr Sep 18, 2019

aws-cdk-automation commented Sep 18, 2019

feat(ecs): allow load balancing to any container and port of service #4107

feat(ecs): allow load balancing to any container and port of service #4107

Conversation

iamhopaul123 commented Sep 16, 2019 • edited by rix0rrr

mergify bot commented Sep 16, 2019

iamhopaul123 commented Sep 16, 2019

aws-cdk-automation commented Sep 16, 2019

AWS CodeBuild CI Report

karlpatr 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

Choose a reason for hiding this comment

rix0rrr left a comment • edited

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

aws-cdk-automation commented Sep 17, 2019

AWS CodeBuild CI Report

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Sep 18, 2019

AWS CodeBuild CI Report

iamhopaul123 commented Sep 16, 2019 •

edited by rix0rrr

rix0rrr left a comment •

edited