fix(aws-ecs-pattern): allow ScheduledTaskBase to run on a public subnet by floehopper · Pull Request #6624 · aws/aws-cdk

floehopper · 2020-03-08T14:34:27Z

Allow instances of ScheduledFargateTask and ScheduledEc2Task to run in a public subnet via a configuration option.

The default remains that such instances are restricted to run on private subnets, but it is now possible to allow them to run on public subnets if the user is willing to sacrifice the extra security that a private subnet provides in favour of a simpler/cheaper system that does not require a NAT gateway or a NAT instance.

The new unit test schedules a Fargate task to run on a container in a VPC with no private subnet. Before the changes to ScheduledTaskBase in this commit, this test caused the following error:

Error: There are no 'Private' subnet groups in this VPC. Available types: Public

fixes #6312

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

floehopper · 2020-03-08T14:40:15Z

@SoManyHs @pkandasamy91 Here's the PR I promised to address #6312. This is my first aws-cdk PR, so I'm sorry if I've done anything wrong. I'm a bit unclear about the PR Linter error which suggests I need to make a change to a README. Presumably in this case this would be the README in the aws-ecs-pattern package...? Although it's not obvious to me what I would change in there...?

aws-cdk-automation · 2020-03-08T15:19:16Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 53132a8
Result: SUCCEEDED
Build Logs (available for 30 days)

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

PettitWesley

@floehopper some changes

Blocking:

The linter is failing. Please either add this change to the ECS patterns readme or change it from "feat" to "fix", which will remove the requirement for a readme update.
Please rebase
Please see my comment on the unit test

Non-Blocking:

I think a unit test is sufficient for this change; however, you could also update the existing integ test to include this parameter.

Thank you for contributing this change to the CDK :)

packages/@aws-cdk/aws-ecs-patterns/test/fargate/test.scheduled-fargate-task.ts

PettitWesley · 2020-03-12T22:30:51Z

Although it's not obvious to me what I would change in there...?

AFAIK, the change should be made here:
https://github.com/aws/aws-cdk/tree/master/packages/%40aws-cdk/aws-ecs-patterns#scheduled-tasks

Allow instances of ScheduledFargateTask and ScheduledEc2Task to run in a *public* subnet via a configuration option. The default remains that such instances are restricted to run on private subnets, but it is now possible to allow them to run on public subnets if the user is willing to sacrifice the extra security that a private subnet provides in favour of a simpler/cheaper system that does not require a NAT gateway or a NAT instance. The new unit test schedules a Fargate task to run on a container in a VPC with no private subnet. Before the changes to ScheduledTaskBase in this commit, this test caused the following error: Error: There are no 'Private' subnet groups in this VPC. Available types: Public fixes aws#6312

Pull request has been modified.

floehopper · 2020-03-14T17:12:20Z

@PettitWesley

Thanks for the feedback. Sorry for the delay in responding. Please see my responses inline below:

Blocking:

The linter is failing. Please either add this change to the ECS patterns readme or change it from "feat" to "fix", which will remove the requirement for a readme update.

I considered adding a new example to the ECS patterns README, but it seemed weird to add an example for such a specific case when other more common cases are not documented. So I've changed the commit note and PR description from "feat" -> "fix" as you suggested.

However, the linter still seems to be complaining. I wonder whether this is because I force-pushed to the remote branch...?

Please rebase

Done and force-pushed.

Please see my comment on the unit test

I've amended the commit to address your feedback on the unit test by changing the assertion to check that the stack has a AWS::Events::Rule configured to use the public subnet and to have a public IP address. See my response to your specific comment for details.

Non-Blocking:

I think a unit test is sufficient for this change; however, you could also update the existing integ test to include this parameter.

I did look at updating the existing integration test. However, given that this new optional property is not set by default and, in fact, setting it probably isn't the recommended approach, it didn't seem sensible to muddy the waters by changing the existing test.

I did also consider creating a new integration test, but that felt like overkill for such a small change. So on balance, I'm happy that the unit test gives sufficient coverage.

aws-cdk-automation · 2020-03-14T17:31:07Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: f8fdc4f
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/test/fargate/test.scheduled-fargate-task.ts

PettitWesley · 2020-03-14T20:55:39Z

@pkandasamy91 Changing from "feat" to "fix" hasn't appeased the linter. What should we do?

PettitWesley

@floehopper LGTM.

Unfortunately, I think we can't merge this until the linter passes. I'm not sure how to fix that; I will ask someone on Monday.

floehopper · 2020-03-14T22:07:04Z

Unfortunately, I think we can't merge this until the linter passes. I'm not sure how to fix that; I will ask someone on Monday.

👍

floehopper · 2020-03-14T23:07:16Z

@PettitWesley @pkandasamy91

Unfortunately, I think we can't merge this until the linter passes. I'm not sure how to fix that; I will ask someone on Monday.

I've investigated this a bit further and I can confirm that:

The latest PR Linter build ran against the latest/amended commit and failed.
When I run prlint locally for this PR, it passes with the following output:

$ node tools/prlint/index.js mandatoryChanges 6624
Creating un-authenticated GitHub Client
⌛  Fetching PR number 6624
⌛  Fetching files for PR number 6624
⌛  Validating...
✅  Success

So I'm really not sure what's going on! Especially as this other PR which is marked "fix" passes the PR Linter check with no changes to a README.

floehopper · 2020-03-16T16:26:35Z

I'm going to try clicking the "Update branch" button which will merge the latest changes from master into this branch in the hope the new commit might make the PR Linter do the right thing.

…-subnet

Pull request has been modified.

aws-cdk-automation · 2020-03-16T17:11:11Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 215c052
Result: SUCCEEDED
Build Logs (available for 30 days)

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

floehopper · 2020-03-16T17:12:00Z

@PettitWesley Updating the branch seems to have done the trick with the PR Linter. Any chance of an updated review and a merge? Thanks.

…-subnet

aws-cdk-automation · 2020-03-16T20:37:07Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 2a51b22
Result: SUCCEEDED
Build Logs (available for 30 days)

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

…-subnet

Pull request has been modified.

aws-cdk-automation · 2020-03-16T21:47:12Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: cdbd272
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-03-16T21:47:36Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Importantly this includes my fix [1] to allow scheduled tasks to be run on a public subnet. I plan to use this fix in a subsequent commit to simplify the code. [1]: aws/aws-cdk#6624

We can make use of the fix [1] to allow scheduled tasks to run on a public subnet to avoid having to subclass ScheduledFargateTask and override the addTaskDefinitionToEventTarget method, thus simplifying the code considerably. [1]: aws/aws-cdk#6624

floehopper mentioned this pull request Mar 8, 2020

aws-ecs-patterns: Allow ScheduledFargateTask and ScheduledEc2Task to run on a public subnet #6312

Closed

2 tasks

PettitWesley added the @aws-cdk/aws-ecs-patterns Related to ecs-patterns library label Mar 9, 2020

SoManyHs requested a review from PettitWesley March 9, 2020 18:07

SomayaB assigned uttarasridhar Mar 9, 2020

PettitWesley previously requested changes Mar 12, 2020

View reviewed changes

packages/@aws-cdk/aws-ecs-patterns/test/fargate/test.scheduled-fargate-task.ts Show resolved Hide resolved

PettitWesley assigned PettitWesley and unassigned uttarasridhar Mar 12, 2020

floehopper force-pushed the allow-scheduled-task-base-to-run-on-public-subnet branch from 53132a8 to f8fdc4f Compare March 14, 2020 16:46

floehopper changed the title ~~feat(aws-ecs-pattern): allow ScheduledTaskBase to run on a public subnet~~ fix(aws-ecs-pattern): allow ScheduledTaskBase to run on a public subnet Mar 14, 2020

PettitWesley reviewed Mar 14, 2020

View reviewed changes

packages/@aws-cdk/aws-ecs-patterns/test/fargate/test.scheduled-fargate-task.ts Show resolved Hide resolved

PettitWesley previously approved these changes Mar 14, 2020

View reviewed changes

Merge branch 'master' into allow-scheduled-task-base-to-run-on-public…

215c052

…-subnet

PettitWesley previously approved these changes Mar 16, 2020

View reviewed changes

Merge branch 'master' into allow-scheduled-task-base-to-run-on-public…

2a51b22

…-subnet

Merge branch 'master' into allow-scheduled-task-base-to-run-on-public…

cdbd272

…-subnet

PettitWesley approved these changes Mar 16, 2020

View reviewed changes

mergify bot merged commit b9a1408 into aws:master Mar 16, 2020

floehopper deleted the allow-scheduled-task-base-to-run-on-public-subnet branch March 16, 2020 22:04

snyk-bot mentioned this pull request Apr 10, 2020

[Snyk] Upgrade aws-cdk from 1.28.0 to 1.30.0 ammobinDOTca/ammobin-cdk#2

Closed

Conversation

floehopper commented Mar 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

floehopper commented Mar 8, 2020

Uh oh!

aws-cdk-automation commented Mar 8, 2020

AWS CodeBuild CI Report

Uh oh!

PettitWesley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PettitWesley commented Mar 12, 2020

Uh oh!

floehopper commented Mar 14, 2020

Uh oh!

aws-cdk-automation commented Mar 14, 2020

AWS CodeBuild CI Report

Uh oh!

Uh oh!

PettitWesley commented Mar 14, 2020

Uh oh!

PettitWesley left a comment

Choose a reason for hiding this comment

Uh oh!

floehopper commented Mar 14, 2020

Uh oh!

floehopper commented Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

floehopper commented Mar 16, 2020

Uh oh!

aws-cdk-automation commented Mar 16, 2020

AWS CodeBuild CI Report

Uh oh!

floehopper commented Mar 16, 2020

Uh oh!

aws-cdk-automation commented Mar 16, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Mar 16, 2020

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Mar 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

floehopper commented Mar 8, 2020 •

edited

Loading

floehopper commented Mar 14, 2020 •

edited

Loading