Skip to content
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.

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

fix(aws-ecs-pattern): allow ScheduledTaskBase to run on a public subnet #6624

Conversation

floehopper
Copy link
Contributor

@floehopper floehopper commented Mar 8, 2020

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
Copy link
Contributor Author

@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
Copy link
Collaborator

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 PettitWesley added the @aws-cdk/aws-ecs-patterns Related to ecs-patterns library label Mar 9, 2020
Copy link
Contributor

@PettitWesley PettitWesley left a comment

Choose a reason for hiding this comment

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

@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 :)

@PettitWesley
Copy link
Contributor

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
@floehopper floehopper force-pushed the allow-scheduled-task-base-to-run-on-public-subnet branch from 53132a8 to f8fdc4f Compare March 14, 2020 16:46
@mergify mergify bot dismissed PettitWesley’s stale review March 14, 2020 16:47

Pull request has been modified.

@floehopper 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
@floehopper
Copy link
Contributor Author

@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
Copy link
Collaborator

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

@PettitWesley
Copy link
Contributor

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

PettitWesley
PettitWesley previously approved these changes Mar 14, 2020
Copy link
Contributor

@PettitWesley PettitWesley left a comment

Choose a reason for hiding this comment

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

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

floehopper commented Mar 14, 2020

@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:

  1. The latest PR Linter build ran against the latest/amended commit and failed.
  2. 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
Copy link
Contributor Author

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.

@mergify mergify bot dismissed PettitWesley’s stale review March 16, 2020 16:27

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

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
Copy link
Contributor Author

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

PettitWesley
PettitWesley previously approved these changes Mar 16, 2020
@aws-cdk-automation
Copy link
Collaborator

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

@mergify mergify bot dismissed PettitWesley’s stale review March 16, 2020 21:04

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

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
Copy link
Contributor

mergify bot commented Mar 16, 2020

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).

@mergify mergify bot merged commit b9a1408 into aws:master Mar 16, 2020
@floehopper floehopper deleted the allow-scheduled-task-base-to-run-on-public-subnet branch March 16, 2020 22:04
floehopper added a commit to freerange/google-drive-backup that referenced this pull request Mar 22, 2020
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
floehopper added a commit to freerange/google-drive-backup that referenced this pull request Mar 22, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-ecs-patterns: Allow ScheduledFargateTask and ScheduledEc2Task to run on a public subnet
4 participants