Initial commit of AWS infrastructure - #15567
Conversation
cjllanwarne
left a comment
There was a problem hiding this comment.
I didn't look at the main.yml file in detail yet. Perhaps we could do a walkthrough and/or a manual setup attempt using it?
In the meantime, I have a handful of minor comments about the easier to understand sections 😆
| }, | ||
| { | ||
| "ParameterKey": "DomainName", | ||
| "ParameterValue": "hail.is" |
There was a problem hiding this comment.
this is the GCP domain. We probably want aws.hail.is here, assuming this is supposed to be the broad specific parameters file?
There was a problem hiding this comment.
Yep, changed this. I thought we needed hail.is for organization_domain but that should be aws.hail.is as well.
| }, | ||
| { | ||
| "ParameterKey": "IsBroadProduction", | ||
| "ParameterValue": "false" |
There was a problem hiding this comment.
What is this key used for?
There was a problem hiding this comment.
We talked offline about replacing this with parameters that control individual parts of our config instead of one parameter that toggles everything potentially unique to the Broad setup.
| "ParameterKey": "SubnetCidrBlockA", | ||
| "ParameterValue": "172.31.0.0/20" | ||
| }, | ||
| { | ||
| "ParameterKey": "SubnetCidrBlockB", | ||
| "ParameterValue": "172.31.16.0/20" | ||
| }, | ||
| { | ||
| "ParameterKey": "SubnetCidrBlockC", | ||
| "ParameterValue": "172.31.32.0/20" |
There was a problem hiding this comment.
What are these used for?
Do we need all of them?
What happens if we run out?
Is there a single /19 or /18 range that covers the same range?
There was a problem hiding this comment.
EKS requires clusters to be able to provision pods across multiple zones for availability. CFN uses these ranges to create subnets in different availability zones. If we run out of IPs, the easiest fix is probably to scale horizontally and add another subnet (either in an existing zone or a brand new one).
| - Install the AWS CLI and pass it credentials using `aws login`. | ||
| - Set the default region for the AWS CLI using `aws configure`. The Hail backend is deployed in `us-east-1.` | ||
| - Check out the Hail repository and switch to the `$HAIL/infra/aws-broad` directory. | ||
| - Edit the default parameter values in `parameters.json` to match the desired configuration for your cluster. |
There was a problem hiding this comment.
On the GCP side, we say to create a new directory in the repo like infra/aws/{instancename}/ to store the parameterized configurations.
Does that make sense here? (And move the top level directory from infra/aws-broad to infra/aws with a broad specific infra/aws/broad/parameters.json example config?
There was a problem hiding this comment.
That sounds better. I moved these files to infra/aws and the parameter file for our config to infra/aws/hail-is/parameters.json.
There was a problem hiding this comment.
Pull request overview
Introduces an initial AWS infrastructure scaffold (CloudFormation + parameters + README) intended to create an EKS cluster and supporting AWS resources as a base for later bootstrapping Hail services.
Changes:
- Add
infra/aws-broad/main.ymlCloudFormation template for EKS, IAM roles, ECR, S3, and aglobal-configsecret. - Add
infra/aws-broad/parameters.jsonwith default network / domain parameters. - Add
infra/aws-broad/README.mdwith instructions to create the stacks.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| infra/aws-broad/README.md | Documents AWS prerequisites and stack creation steps. |
| infra/aws-broad/parameters.json | Supplies default CloudFormation parameter values (CIDRs, domain, production flag). |
| infra/aws-broad/main.yml | Defines the core AWS infrastructure (EKS, node groups, buckets, roles, registries, and global config). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SecretString: | ||
| Fn::ToJsonString: | ||
| cloud: "aws" | ||
| batch_gcp_regions: !Ref AWS::Region | ||
| batch_logs_storage_uri: !Sub "s3://${VdcHailBatchBucket}" | ||
| test_storage_uri: !Sub "s3://${VdcHailTestBucket}" |
There was a problem hiding this comment.
It's probably better to replace this with batch_aws_regions in the long run.
# Conflicts: # infra/aws-broad/README.md
e88cbf6 to
f8a3d97
Compare
987a81f to
10e7329
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (11)
infra/aws/main.yaml:33
batch_gcp_regionsmust be valid JSON (it is parsed viajson.loads(global_config['batch_gcp_regions'])). The current value expands to something like[ us-east-1 ](unquoted), which is invalid JSON and will raise at runtime if any code path reads it.
cloud: "aws"
batch_gcp_regions: !Sub "[ ${AWS::Region} ]"
batch_logs_storage_uri: !Sub "s3://${VdcHailBatchBucket}"
infra/aws/main.yaml:572
- CloudFormation Export names must be unique within an account/region. Using a fixed export name (
VdcNetwork) will collide if more than one stack is deployed; include the stack name in the export.
Export:
Name: VdcNetwork
infra/aws/main.yaml:577
- CloudFormation Export names must be unique within an account/region. Using a fixed export name (
VdcSubnet) will collide if more than one stack is deployed; include the stack name in the export.
Export:
Name: VdcSubnet
infra/aws/main.yaml:582
- CloudFormation Export names must be unique within an account/region. Using a fixed export name (
VdcNodeRole) will collide if more than one stack is deployed; include the stack name in the export.
Export:
Name: VdcNodeRole
infra/aws/README.md:7
- The prerequisites mention
aws login, but the AWS CLI command for SSO auth isaws sso login(and other credential providers exist). As written, this is likely to confuse users and suggests a non-existent command.
- Install the AWS CLI and pass it credentials using `aws configure` or `aws login`.
infra/aws/README.md:8
- The region string includes a trailing period inside the code span (
us-east-1.), which makes it look like part of the region name and can lead to copy/paste mistakes.
- Set the default region for the AWS CLI using `aws configure`. The Hail backend is deployed in `us-east-1.`
infra/aws/README.md:24
- The command uses a fixed stack name (
hail-vdc) even though the preceding line introducesINSTANCE_NAME. This prevents multiple independent deployments and contradicts the idea of per-instance directories; it also includes--disable-rollback, which can leave partially-created resources and ongoing costs by default.
export INSTANCE_NAME=<name for this instance of Hail>
aws cloudformation create-stack --stack-name hail-vdc --template-body file://main.yaml --role-arn $STACK_ROLE_ARN --parameters file://${INSTANCE_NAME}/parameters.json --capabilities CAPABILITY_NAMED_IAM CAPABILITY_AUTO_EXPAND --disable-rollback
infra/aws/main.yaml:29
- The Secrets Manager secret name is hard-coded to
global-config. This will collide if multiple stacks are created in the same account/region (and makes cleanup harder). Consider scoping it to the stack name.
Type: "AWS::SecretsManager::Secret"
Properties:
Name: "global-config"
SecretString:
infra/aws/main.yaml:218
- The EKS cluster name is hard-coded to
vdc. EKS cluster names must be unique per account/region; using a fixed name prevents multiple deployments and clashes with the per-instance workflow described in the README.
RoleArn: !GetAtt VdcClusterRole.Arn
Name: "vdc"
UpgradePolicy:
infra/aws/main.yaml:567
- CloudFormation Export names must be unique within an account/region. Using a fixed export name (
VdcClusterName) will collide if more than one stack is deployed; include the stack name in the export.
This issue also appears in the following locations of the same file:
- line 571
- line 576
- line 581
Export:
Name: VdcClusterName
infra/aws/hail-is/parameters.json:25
PublicAccessIpListis aCommaDelimitedList, but this example value includes spaces after commas. CloudFormation does not reliably trim whitespace for list items, which can produce invalid CIDRs like" 69.173.64.0/19"and cause stack creation to fail.
"ParameterKey": "PublicAccessIpList",
"ParameterValue": "69.173.64.0/18, 69.173.64.0/19, 69.173.96.0/19, 69.173.95.0/24, 69.173.97.0/24"
},
| ``` | ||
| export STACK_ROLE_NAME=hail-cloudformation-role | ||
| export STACK_ROLE_ARN=$(aws iam create-role --role-name $STACK_ROLE_NAME --assume-role-policy-document "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":{\"Service\":\"cloudformation.amazonaws.com\"},\"Action\":\"sts:AssumeRole\"}]}" --query "Role.Arn" --output text) | ||
| aws iam attach-role-policy --role-name $STACK_ROLE_NAME --policy-arn arn:aws:iam::aws:policy/AdministratorAccess |
| internal_ip: !GetAtt VdcInternalGatewayAddress.PrimaryPrivateIpAddress | ||
| ip: !GetAtt VdcCluster.Endpoint # use the cluster endpoint in place of a static endpoint | ||
| kubernetes_server_url: !GetAtt VdcCluster.Endpoint |
There was a problem hiding this comment.
This is intentional for now.
Change Description
Implementing #15413
First pass at setting up the necessary cloud resources to run Hail on AWS. The intended result of following these instructions is an EKS cluster with no Hail services running that is ready to be bootstrapped by the existing bootstrap scripts (after slight modification).
Security Assessment
Impact Rating
Impact Description
This change cannot have any impact on the actual Hail production instance, but it will be used to create a new Hail Batch instance which will eventually be production.
Appsec Review