Adds Terraform files as alternative Cloud Formation #26

GregHilstonHop · 2021-03-30T14:28:49Z

Hey Metaflow,

This pull request adds a Terraform stack for deploying Metaflow to AWS. As requested by the Metaflow CONTRIBUTING.md file, we've checked for an existing Github issue before creating our own and this pull request can be considered a direct response to the existing Metaflow Github issue #38. Additionally, as requested, we've reached out to the Metaflow team on the Gitter chatroom to ensure this submission provides value and is aligned with Metaflow's direction.

We're aware of the outstanding pull request 14 that also adds Terraform code. Since pull request 14 and the discussion on Metaflow Github issue #38 have been stale for some time, we decided we'd use this opportunity to share our Terraform implementation.

Technical Breakdown

We've designed the Terraform code to be composed of three separates projects:

infra
metaflow
sagemaker-notebook

Which are documented in the various README.md files included in this pull request, but for convenience we'll provide a high level overview here.

`infra`

Mostly stands up and configures the VPC.

`metaflow`

This has been separated into four modules based on the service architecture.

Depends on the output of the infra projects.

Computation

Sets up the Batch AWS resources to support remote experiment execution.

Datastore

Sets up the RDS and S3 AWS resources to provide storage for the Metadata Service.

Metadata Service

Sets up the API Gateway, load balancer and ECS cluster that powers the Metadata Service.

Step Functions

Sets up the Step Function AWS resources to provide productionified remote experiment executions.

`sagemaker-notebook`

Sets up the Sagemaker Notebook AWS resources to provide a central location to interact with the Metadata Service.

How To Deploy

This is documented in the pull request and can be found under metaflow-tools/aws/terraform/README/md

Co-authored-by: Gregory Hilston <66077036+GregHilstonHop@users.noreply.github.com> Co-authored-by: Ankur Kalra <64661093+ankur-hop@users.noreply.github.com>

savingoyal · 2021-03-30T15:11:49Z

Thanks @GregHilstonHop! We will start reviewing this!

oavdeev · 2021-03-31T23:55:45Z

This is really cool @GregHilstonHop , I've played with this a bit today. The doc is great, I've been able to run everything pretty easily. As someone who tried to get a working tf setup for AWS Batch at $JOB-1, I know it is far from trivial!

I wonder if it be possible to separate VPC setup bits from Metaflow-specific resources in infra (like that IAM policy)? I imagine a lot of people would already have some kind of preferred VPC configuration already in place, maybe using a module like terraform-aws-vpc. It would be cool if this module played nicely with this kind of setup, and had an option to provide subnets ids and whatnot externally.

In fact it seems like it should be possible to turn metaflow into a module that people can use directly from this Github repo. What do you think?

GregHilstonHop · 2021-04-01T13:18:09Z

Hey @oavdeev , thanks for trying it out, very happy you're having a great experience and enjoying the documentation!

You're absolutely right that setting up an AWS Batch environment is not a simple task :)

I think you have a really good point about how we don't want to shoehorn people into being required to stand up a VPC exactly as the infra project dictates. I think the separating the VPC from the Metaflow-specific resources is a really good idea and would make this much more approachable for those already with AWS resources.

Since this pull request is rather large, clocking in at >3k lines, I'm wondering if you and the Metaflow team would be open to us creating a Github issue for your idea and tackling this on a subsequent PR.

That way I/we/whoever attacks this followup, can submit a much smaller and more focused PR.

Thoughts oavdeev/metaflow team?

aws/terraform/infra/s3.tf

aws/terraform/infra/vpc.tf

aws/terraform/README.md

oavdeev · 2021-04-02T18:22:04Z

That way I/we/whoever attacks this followup, can submit a much smaller and more focused PR.

I agree, this could be done as a followup. I've left some comments, and create the issue here #27

…ollision

…tring concat

@oavdeev

Taking @oavdeev's suggestion to make Metadata API authentication description more easily understandable Co-authored-by: Oleg Avdeev <oleg.v.avdeev@gmail.com>

…o terraform

GregHilstonHop · 2021-04-08T15:24:42Z

I agree, this could be done as a followup. I've left some comments, and create the issue here #27

Awesome! Thank you for creating that Github issue!

I ended up taking all your advice and added it on this branch.

savingoyal · 2021-04-10T19:56:26Z

Great! I will test out the PR early next week!

GregHilstonHop · 2021-04-13T11:54:11Z

Awesome excited to hear your thoughts and feedback!

aws/terraform/metaflow/outputs.tf

…maker prefixes

* add dynamo access to the batch service role like: https://github.com/Netflix/metaflow-tools/blob/master/aws/cloudformation/metaflow-cfn-template.yml#L1066 * fix: use variable since policy may not resolve

kldavis4 · 2021-04-28T19:43:28Z

hi @savingoyal has anyone had a chance to look at this yet?

We've done the work to move the metaflow bits to a standalone module, so if no one has had a chance to review yet, I'm wondering if it makes sense to update this PR with the modularized version instead of doing it separately for #27

savingoyal · 2021-04-28T20:45:42Z

Sure, it would be great if you can update the PR with the modularized version. We can get started with the review right after!

GregHilstonHop · 2021-04-29T11:17:12Z

@kldavis4 How do you want to go about updating this branch?

Do you want to submit a PR for the work you did for issue #27 to merge said work into this branch?

kldavis4 · 2021-04-29T13:20:18Z

@GregHilstonHop yes, I'll open a PR in your repo for this branch later today

oavdeev · 2021-07-07T19:09:19Z

aws/terraform/modules/metaflow/iam.tf

+  statement {
+    effect = "Allow"
+
+    # TODO - reduce to Encrypt, Decrypt?


nit: looks like this TODO is done :)

queueburt · 2021-07-21T16:58:09Z

aws/terraform/infra/data.tf

+*/
+data "aws_availability_zones" "available" {
+  state = "available"
+}


Little micro-bug here. For accounts using AWS Localzones, this occasionally returns those AZ's for the VPC deployment, and LZ doesn't support RDS or NLB at the moment. A filter stanza like the following can correct that:

filter { name = "opt-in-status" values = ["opt-in-not-required"] }

aws/terraform/modules/metaflow/modules/metadata-service/api-gateway.tf

- For accounts using AWS Localzones, this occasionally returns those AZ's for the VPC deployment, and LZ doesn't support RDS or NLB at the moment

pr feedback

queueburt · 2021-07-27T23:22:08Z

aws/terraform/modules/metaflow/modules/metadata-service/api-gateway.tf

+
+  api_stages {
+    api_id = aws_api_gateway_rest_api.this.id
+    stage  = aws_api_gateway_deployment.this.stage_name


Did you mean stage = aws_api_gateway_stage.this.stage_name here?

yes that makes more sense 👍🏻 I'll update

queueburt · 2021-07-27T23:47:37Z

aws/terraform/modules/metaflow/modules/metadata-service/ec2.tf

+}
+
+resource "aws_lb_target_group" "db_migrate" {
+  name        = "${var.resource_prefix}dbtrgtgrp${var.resource_suffix}"


This might be a cascading issue that affects other stuff, but I'm going to draw attention to it here since I've hit it twice. Certain resources, target groups included, have a 32 character limit. The baseline example.tfvars and this naming convention result in a name that ends up being 33 characters. I fix it by changing this line to "${var.resource_prefix}dbtg${var.resource_suffix}". We should definitely ensure that the template works out-of-the-box with the provided variables either by shortening this line or providing shorter variable defaults, but it's still worth calling out that these naming conventions teeter dangerously close to character limits, and even a wee bit of user-fiddling in the labeling could result in overflow. It's not a deal-breaker, but I'd anticipate running into it in the wild occasionally.

pr feedback: refing wrong entity for stage_name

queueburt · 2021-08-03T17:27:49Z

aws/terraform/README.md

+
+Initialize the terraform:
+
+`cd infra && terraform init`


cd infra is called out in three different spots, one for each section. I assume the intention was for it to indicate the specific project name, e.g. cd infra, cd metaflow, and cd sagemaker. Just a copy-paste type-o, perhaps?

queueburt · 2021-08-03T17:31:34Z

aws/terraform/modules/metaflow/modules/metadata-service/ec2.tf

+  tags = var.standard_tags
+}
+
+resource "aws_lb_target_group" "this" {


Refer to the issue below: https://github.com/Netflix/metaflow-tools/pull/26/files#r677874253. Hit this problem with character limit again with this resource when I tested on Gov Cloud. Gov Cloud's region identifier is longer (us-gov-west-1), so public sector users will hit this limit both for this resource as well as the DB Target Group I called out below.

queueburt · 2021-08-03T17:40:32Z

aws/terraform/metaflow/iam-custom-role.tf

+    ]
+
+    resources = [
+      "arn:${var.iam_partition}:iam:${local.aws_region}:${local.aws_account_id}:role/${local.resource_prefix}*${local.resource_suffix}"


I've only gotten a chance to test this in Gov Cloud, and not in its entirety due to another issue I'll highlight elsewhere, but this wildcard structure seems to confuse Terraform pretty dramatically. I was able to get it to at least apply using the following syntax: "arn:${var.iam_partition}:iam:${local.aws_region}:${local.aws_account_id}:role/${local.resource_prefix}-*-${local.resource_suffix}". I'm uncertain if the policies will work, though, because I'm stalled at the same KMS S3 issue we saw with the Batch user. I assume we'll just need to apply the same permission setup here.

I've created a shorted region name (4 character) mapping dict to hopefully resolve this

queueburt · 2021-08-03T17:42:13Z

aws/terraform/metaflow/iam-custom-role.tf

+    effect = "Allow"
+
+    actions = [
+      "kms:Decrypt",


Placing this here as a highlight. I'm hitting the same S3 forbidden errors and KMS denials as I had for the Batch role. I assume we'll need the same KMS adjustment for this role as we did for that one.

I'm wondering about the resource specified here. It does look like the same as what is present in the cloudformation template, but it seems like it should be: arn:${var.iam_partition}:kms:${local.aws_region}:${local.aws_account_id}:key/*

The other kms permissions are not using a wildcard and have a specific key resource

I think this is actually unrelated to the above. The "metaflow" policy on the custom role had a passrole permission with a region in the arn which was causing the role not to be fully generated. Pushed a commit to fix and verified role/policy is generated now

Pr feedback

fix: reduce target group name len to avoid issues with resourcing naming limits

queueburt · 2021-08-09T20:22:24Z

aws/terraform/metaflow/iam-custom-role.tf

+    ]
+
+    resources = [
+      "arn:${var.iam_partition}:events:${local.aws_region}:${local.aws_account_id}:rule:*",


I believe this needs to be arn:${var.iam_partition}:events:${local.aws_region}:${local.aws_account_id}:rule/*

kldavis4 and others added 6 commits March 24, 2021 17:14

initial revision of terraform

1bb2ad4

Co-authored-by: Gregory Hilston <66077036+GregHilstonHop@users.noreply.github.com> Co-authored-by: Ankur Kalra <64661093+ankur-hop@users.noreply.github.com>

Pins Terraform versions to lowest version required

1e65502

Removes Docker from prerequisites as its not needed

d3c0406

Updates and adds README.md documentation

0fda528

Makes README.md title header more generic

6e9ed79

Refactors capitalization and adds type, satisfying PR comments

bdcb389

savingoyal self-requested a review March 30, 2021 15:11

Removes trailing newline

fd00bf6

oavdeev reviewed Apr 2, 2021

View reviewed changes

aws/terraform/infra/s3.tf Outdated Show resolved Hide resolved

aws/terraform/infra/vpc.tf Outdated Show resolved Hide resolved

aws/terraform/README.md Show resolved Hide resolved

oavdeev mentioned this pull request Apr 2, 2021

Have a nice standalone terraform module for Metaflow #27

Closed

GregHilstonHop and others added 6 commits April 8, 2021 10:59

Adds the datasets s3 bucket name to accepted as a variable to avoid c…

fc4cbd6

…ollision

Uses aws_availability_zones to select availability zones instead of s…

217061d

…tring concat

Adds Metadata API auth explanation

271d2fa

Taking @oavdeev's suggestion to make Metadata API authentication description more easily understandable Co-authored-by: Oleg Avdeev <oleg.v.avdeev@gmail.com>

Fixes variable name to match what's defined

a6519d1

fixes in missing quotes

4557d4f

Merge branch 'terraform' of github.com:hopservices/metaflow-tools int…

8183316

…o terraform

kldavis4 reviewed Apr 15, 2021

View reviewed changes

aws/terraform/metaflow/outputs.tf Outdated Show resolved Hide resolved

GregHilstonHop and others added 2 commits April 15, 2021 15:56

Renames Metaflow output to notate Metaflow instead of misleading Sage…

b0718b4

…maker prefixes

Add dynamodb permissions to the batch service role (#2)

3e51de9

* add dynamo access to the batch service role like: https://github.com/Netflix/metaflow-tools/blob/master/aws/cloudformation/metaflow-cfn-template.yml#L1066 * fix: use variable since policy may not resolve

doc: update docs based on latest changes (#7)

26d143e

oavdeev reviewed Jul 7, 2021

View reviewed changes

queueburt reviewed Jul 21, 2021

View reviewed changes

aws/terraform/modules/metaflow/modules/metadata-service/api-gateway.tf Show resolved Hide resolved

kldavis4 and others added 5 commits July 21, 2021 19:08

add missing kms permissions related to s3 (#8)

7885ece

pr feedback: add filter to az data source

f64decd

- For accounts using AWS Localzones, this occasionally returns those AZ's for the VPC deployment, and LZ doesn't support RDS or NLB at the moment

pr feedback: best practice for stage / stage deployment in api gateway

a9c9bf3

skip dynamodb perms if step functions are disabled

4d91003

Merge pull request #9 from hopservices/pr_feedback_rob

52877a3

pr feedback

queueburt reviewed Jul 27, 2021

View reviewed changes

kldavis4 and others added 2 commits July 30, 2021 16:12

pr feedback: ref wrong entity for stage_name

09cfa91

Merge pull request #10 from hopservices/fix_stage_name

92a3dbf

pr feedback: refing wrong entity for stage_name

queueburt reviewed Aug 3, 2021

View reviewed changes

kldavis4 and others added 8 commits August 3, 2021 19:13

fix copypasta

5807d74

fix passrole resource to not specify account id

c0b76f6

shorten the region suffix to overcome issues with name lengths

98f6d04

Merge pull request #11 from hopservices/pr_feedback

2f6a37b

Pr feedback

fix: reduce target group name len to avoid issues with limits

e61ce8b

Merge pull request #12 from hopservices/reduce_target_group_name_length

4e791bd

fix: reduce target group name len to avoid issues with resourcing naming limits

add missing locals.tf

12f34fc

fix incorrect batch permission

ef9a935

queueburt reviewed Aug 9, 2021

View reviewed changes

fix bad rule resource in iam-custom-role

534b440

savingoyal approved these changes Aug 11, 2021

View reviewed changes

queueburt approved these changes Aug 11, 2021

View reviewed changes

savingoyal merged commit e6f5ba4 into Netflix:master Aug 11, 2021

Adds Terraform files as alternative Cloud Formation #26

Adds Terraform files as alternative Cloud Formation #26

Uh oh!

Conversation

GregHilstonHop commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Technical Breakdown

infra

metaflow

Computation

Datastore

Metadata Service

Step Functions

sagemaker-notebook

How To Deploy

Uh oh!

savingoyal commented Mar 30, 2021

Uh oh!

oavdeev commented Mar 31, 2021

Uh oh!

GregHilstonHop commented Apr 1, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oavdeev commented Apr 2, 2021

Uh oh!

GregHilstonHop commented Apr 8, 2021

Uh oh!

savingoyal commented Apr 10, 2021

Uh oh!

GregHilstonHop commented Apr 13, 2021

Uh oh!

Uh oh!

kldavis4 commented Apr 28, 2021

Uh oh!

savingoyal commented Apr 28, 2021

Uh oh!

GregHilstonHop commented Apr 29, 2021

Uh oh!

kldavis4 commented Apr 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GregHilstonHop commented Mar 30, 2021 •

edited

Loading

`infra`

`metaflow`

`sagemaker-notebook`