feat(ecs-patterns): add listener port as a property for network/application load balanced services #4825

pahud · 2019-11-02T17:05:20Z

Allow users to specify the listener port as input to application/network load balanced fargate/ecs services.

If a port is specified, both the target group and listener port will be overridden.
If a port is not specified, the target group port will be defaulted to 80 and the listener port will default to 80 if the protocol is HTTP or 443 if the protocol is HTTPS.

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

…her than the default 80(fix #4793) - feat(ecs-patterns): public facing NLB fronted fargate tasks with assignPublicIp enabled should allow all ipv4 traffic to the ingress port on the fargate task

mergify · 2019-11-02T17:09:29Z

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

aws-cdk-automation · 2019-11-02T17:36:40Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-11-04T18:27:44Z

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 · 2019-11-04T18:44:00Z

assignPublicIp enabled should allow all ipv4 traffic

I disagree with this statement. I have multiple AWS services that are meant to be accessed only from inside a corporate VPN. It has public IPs because the traffic comes from outside the AWS account, but the access to the service behind the NLB is still restricted with a prefix list specifying the corporate IP block.

I don't use the patterns helper functions, but if this change is accepted the configuration I support will be impossible to build because the assumption made is not globally true.

I strongly encourage making any change that automatically reduces the security of a system for convenience explicitly opt-in.

piradeepk · 2019-11-04T18:50:55Z

assignPublicIp enabled should allow all ipv4 traffic

I disagree with this statement. I have multiple AWS services that are meant to be accessed only from inside a corporate VPN. It has public IPs because the traffic comes from outside the AWS account, but the access to the service behind the NLB is still restricted with a prefix list specifying the corporate IP block.

I don't use the patterns helper functions, but if this change is accepted the configuration I support will be impossible to build because the assumption made is not globally true.

I strongly encourage making any change that automatically reduces the security of a system for convenience explicitly opt-in.

I'm in agreement here, I don't think the use-case you handle can be described as the general use-case. I would rather not reduce the security levels of the construct and would leave it up to the user to do it after the fact if they prefer.

Since your PR is it's around allowing different listener ports (nlb/alb), please reduce the scope of this PR to only allow a listener port to be input to the construct and make any additional changes in a separate PR.

packages/@aws-cdk/aws-ecs-patterns/lib/fargate/network-load-balanced-fargate-service.ts

packages/@aws-cdk/aws-ecs-patterns/lib/base/network-load-balanced-service-base.ts

aws-cdk-automation · 2019-11-04T19:21:15Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

Pull request has been modified.

aws-cdk-automation · 2019-11-04T21:18:17Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

- remove non-related logic to another PR

aws-cdk-automation · 2019-11-05T03:24:17Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts

packages/@aws-cdk/aws-ecs-patterns/lib/base/network-load-balanced-service-base.ts

Pull request has been modified.

aws-cdk-automation · 2019-11-05T19:12:21Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

piradeepk · 2019-11-05T23:22:55Z

Can you also add a unit test to test when you specify a different port?

aws-cdk-automation · 2019-11-05T23:48:46Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

✔ setting NLB special listener port to create the listener ✔ setting ALB special listener port to create the listener ✔ setting ALB HTTPS protocol to create the listener on 443 ✔ setting ALB HTTP protocol to create the listener on 80 ✔ setting ALB without any protocol or listenerPort to create the listener on 80

aws-cdk-automation · 2019-11-06T00:47:12Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-11-06T00:48:04Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-11-06T01:20:54Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

pahud · 2019-11-06T01:27:27Z

Hi @pkandasamy91

All tests passed including some additional test coverage(73525c7). Can you kindly review it again?

piradeepk · 2019-11-06T16:30:02Z

@pahud, thanks for adding the tests, but you have yet to resolve the comments from the previous review.

aws-cdk-automation · 2019-11-06T16:52:17Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts

aws-cdk-automation · 2019-11-07T01:00:12Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

pahud · 2019-11-07T01:21:51Z

@pkandasamy91

Just fixed the lines as suggested and all tests passed.

aws-cdk-automation · 2019-11-07T17:38:44Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

piradeepk

Ship it! Thanks for taking the time to contribute and resolve this issue!

mergify · 2019-11-07T18:59:25Z

Thank you for contributing! Your pull request is now being automatically merged.

- feat(ecs-patterns): allow to specify different NLB listener port ot…

14de430

…her than the default 80(fix #4793) - feat(ecs-patterns): public facing NLB fronted fargate tasks with assignPublicIp enabled should allow all ipv4 traffic to the ingress port on the fargate task

pahud requested a review from SoManyHs as a code owner November 2, 2019 17:05

pahud changed the title ~~- feat(ecs-patterns): allow to specify different NLB listener port~~ feat(ecs-patterns): allow to specify different NLB listener port Nov 2, 2019

pahud mentioned this pull request Nov 2, 2019

ECS: Service should autoconfigure security groups when attached to NLB #1490

Open

SomayaB added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Nov 4, 2019

SomayaB assigned SoManyHs and piradeepk Nov 4, 2019

piradeepk self-requested a review November 4, 2019 17:56

Merge branch 'master' into issue4793

ca3ca00

Merge branch 'master' into issue4793

aaeb0db

piradeepk previously requested changes Nov 4, 2019

View reviewed changes

piradeepk unassigned SoManyHs Nov 4, 2019

Merge branch 'master' into issue4793

3d34944

pahud added 2 commits November 5, 2019 10:55

- support ApplicationLoadBalancedServiceBase as well

d412ae9

- remove non-related logic to another PR

Merge branch 'issue4793' of github.com:pahud/aws-cdk into issue4793

a1a1a51

pahud changed the title ~~feat(ecs-patterns): allow to specify different NLB listener port~~ feat(ecs-patterns): allow to specify different NLB and ALB listener port Nov 5, 2019

pahud requested a review from piradeepk November 5, 2019 03:28

pahud mentioned this pull request Nov 5, 2019

feat(ecs-patterns): enable assignPublicIp on NLB Fargate Services by default #4856

Closed

piradeepk previously requested changes Nov 5, 2019

View reviewed changes

Merge branch 'master' into issue4793

4f5e981

Merge branch 'master' into issue4793

91123b6

pahud added 3 commits November 6, 2019 08:17

add the testing

5e1caf4

Merge branch 'issue4793' of github.com:pahud/aws-cdk into issue4793

04faa95

minor lint fix

37fc285

pahud requested a review from piradeepk November 6, 2019 01:27

Merge branch 'master' into issue4793

42f20b2

pahud added 2 commits November 7, 2019 08:32

minor fix

22c4561

Merge branch 'issue4793' of github.com:pahud/aws-cdk into issue4793

0dc3a47

pahud commented Nov 7, 2019

View reviewed changes

packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts Show resolved Hide resolved

pahud commented Nov 7, 2019

View reviewed changes

packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts Show resolved Hide resolved

Merge branch 'master' into issue4793

0c1bf42

piradeepk changed the title ~~feat(ecs-patterns): allow to specify different NLB and ALB listener port~~ feat(ecs-patterns): add listener port as a property for network and application load balanced services Nov 7, 2019

piradeepk changed the title ~~feat(ecs-patterns): add listener port as a property for network and application load balanced services~~ feat(ecs-patterns): add listener port as a property for network/application load balanced services Nov 7, 2019

piradeepk approved these changes Nov 7, 2019

View reviewed changes

mergify bot merged commit 20b8e5d into aws:master Nov 7, 2019

feat(ecs-patterns): add listener port as a property for network/application load balanced services #4825

feat(ecs-patterns): add listener port as a property for network/application load balanced services #4825

Uh oh!

Conversation

pahud commented Nov 2, 2019 • edited by piradeepk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify bot commented Nov 2, 2019

Uh oh!

aws-cdk-automation commented Nov 2, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Nov 4, 2019

AWS CodeBuild CI Report

Uh oh!

karlpatr commented Nov 4, 2019

Uh oh!

piradeepk commented Nov 4, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aws-cdk-automation commented Nov 4, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Nov 4, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Nov 5, 2019

AWS CodeBuild CI Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aws-cdk-automation commented Nov 5, 2019

AWS CodeBuild CI Report

Uh oh!

piradeepk commented Nov 5, 2019

Uh oh!

aws-cdk-automation commented Nov 5, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Nov 6, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Nov 6, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Nov 6, 2019

AWS CodeBuild CI Report

Uh oh!

pahud commented Nov 6, 2019

Uh oh!

piradeepk commented Nov 6, 2019

Uh oh!

aws-cdk-automation commented Nov 6, 2019

AWS CodeBuild CI Report

Uh oh!

Uh oh!

Uh oh!

aws-cdk-automation commented Nov 7, 2019

AWS CodeBuild CI Report

Uh oh!

pahud commented Nov 7, 2019

Uh oh!

aws-cdk-automation commented Nov 7, 2019

AWS CodeBuild CI Report

Uh oh!

piradeepk left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Nov 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

pahud commented Nov 2, 2019 •

edited by piradeepk

Loading