Skip to content
This repository has been archived by the owner on May 31, 2024. It is now read-only.

feat: add "public subnet" mode for lower ongoing costs #283

Merged
merged 23 commits into from
Mar 11, 2022

Conversation

multimeric
Copy link
Contributor

@multimeric multimeric commented Jan 27, 2022

Issue #, if available: #269

Description of Changes

  • Add publicSubnets option to core stack and batch stack
    • For the core stack, this flag makes the stack exclude the VPC endpoints and NAT gateway
    • For the batch stack, this puts the EC2 instances into the public subnet
    • Together, this will reduce ongoing costs for users
  • Take WES Adapter Lambda out of the VPC (see explanation here)
  • Usage is as follows:
    • agc account activate --publicSubnets will create the Agc-Core stack, but without the endpoints/NAT
    • In the context object in agc-project.yaml, there is now a publicSubnets key, which defaults to false. If the core stack was created with --publicSubnets, this must be set to true or the deployment will fail

Description of how you validated changes

  • Updated the golang tests
  • Manually verified:
    • Core stack
      • I ran aws cloudformation get-template on stacks generated in both ways
      • By default: default_core.txt
      • With --publicSubnets: public_core.txt
      • Note that the NAT gateways and VPC endpoints are only present in the first stack
    • Context stack
      • I ran aws cloudformation get-template on stacks generated both ways
      • By default: default_context.txt
      • With publicSubnets: true in the config file: public_context.txt
      • Note how the public stack sets the compute environment to use public subnets, when you look these subnets up in the public_core.txt stack above, whereas the default stack uses private subnets.
    • Successfully ran the examples/demo-nextflow-project after deploying a public account + context stack

Checklist

  • If this change would make any existing documentation invalid, I have included those updates within this PR
  • I have added unit tests that prove my fix is effective or that my feature works. I have not yet done this, but I'm interested in the recommended way of doing this. Ideally we would write assertions using CDK but I note that there is no existing example of this being done.
  • I have linted my code before raising the PR. I ran make format at least. Is that what you want?
  • Title of this Pull Request follows Conventional Commits standard: https://www.conventionalcommits.org/en/v1.0.0/.

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

@multimeric
Copy link
Contributor Author

Okay, I finally got the chance to test this out with a real workflow, and I realised that, when we remove the NAT gateway, the WES Adapter lambda gets put into an isolated subnet, and therefore cannot communicate with AWS. Further, you can't put lambdas into a public subnet. However the simple and clean solution was just to take that lambda out of the VPC entirely: it doesn't actually need to be in one, since it's called by the API gateway and all of its actions are performed through the AWS API. Once I made that change (which is now pushed), the workflow ran successfully! I used examples/demo-nextflow-project.

I've also pulled some upstream changes from the main branch published since I made this PR.

@multimeric
Copy link
Contributor Author

Hi @markjschreiber is there any chance that this might get reviewed? I am happy to fix the merge conflicts again if a review can be scheduled.

@markjschreiber
Copy link
Contributor

Hi @markjschreiber is there any chance that this might get reviewed? I am happy to fix the merge conflicts again if a review can be scheduled.

Sorry for the delay, I have been working on other bug fixes. I will start reviewing today and also ask another engineer to take a look.

@multimeric
Copy link
Contributor Author

Okay, I think I've fixed all the issues from that code review. Thanks for that. Here are example stacks:

@multimeric
Copy link
Contributor Author

New updates from the last round of code review. Here are some new example CFN templates. They demonstrate that the VPC only produces public subnets now:

markjschreiber
markjschreiber previously approved these changes Feb 18, 2022
@multimeric
Copy link
Contributor Author

Please let me know if any more work is needed for this.

@markjschreiber
Copy link
Contributor

@multimeric one of the build checks is failing seemingly due to a linting failure (changed import order). You can probably fix this by running make format and then commiting the result.

@multimeric
Copy link
Contributor Author

Okay I've pulled from upstream and reformatted the code.

markjschreiber
markjschreiber previously approved these changes Mar 9, 2022
@multimeric
Copy link
Contributor Author

@IllyaYalovyy.

waliag87
waliag87 previously approved these changes Mar 10, 2022
@waliag87 waliag87 dismissed stale reviews from markjschreiber and themself via d45635e March 10, 2022 18:44
@waliag87
Copy link
Contributor

@multimeric there appears to be a formatting issue again, likely due to my merge from main. If we can resolve that + get it re-reviewed by Mark and I the PR should be ready to merge.

@multimeric
Copy link
Contributor Author

Okay I've fixed the tests again. This problem keeps happening because changes to upstream break the merge, so the faster this can get approved the better.

@waliag87 waliag87 merged commit d6a67f9 into aws:main Mar 11, 2022
@waliag87
Copy link
Contributor

@multimeric done and merged in.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants