EKS in AWS Curvenote account #2652

manics · 2023-06-01T21:46:14Z

This is enough to deploy a basic EKS cluster (e.g. kubectl get nodes works). The Terraform is originally copied from an EKS cluster I deployed several months ago, unrelated to jupyter/binder.
Todos include:

OIDC (IRSA and GitHub OIDC)
Test a manual BinderHub deployment
Add docs on the private deployment config (setting up limited IAM roles, creating service linked roles, creating a permissions boundary and require it for iam:CreateRole)
Add docs on how this is setup
Deploy BinderHub via CI

Related to

Deploy a BinderHub on AWS and add it to the federation #2556

The deployment of BinderHub and other applications will be handled in a separate PR

.github/workflows/aws-dev.yml

manics · 2023-06-30T19:45:09Z

.pre-commit-config-terraform.yaml

+# - tflint
+
+# Currently only aws/curvenote is checked
+files: "^terraform/aws/curvenote/"


I think we should run as many static checks as possible on our terraform code since we can't really test them, though we can discuss that later. For now I've limited it to the new AWS Curvenote terraform.

manics · 2023-06-30T21:32:28Z

This is ready for review! This is just the Terraform for deploying the EKS cluster on Curvenote's AWS account. Instructions are in the README.

I've deployed a limited BinderHub to https://3.13.147.101.nip.io/
Since this is just for testing:

maximum pod lifetime in the idle culler is set to 15 minutes though it doesn't always work due to GESIS BinderHub server was accumulating Running pods that were more than 1 day old #2686
I've deployed an ECR registry cleaner which deletes images that haven't been pulled after ~1 day
Single node

I'll open a separate PR for the Helm chart deployment.

manics · 2023-07-10T21:21:27Z

I've pushed some changes so some (but not all) Terraform changes can be automatically deployed by the GitHub workflow I've added here. For example, I think K8s upgrades and changes to node sizes/numbers can be deployed without manual intervention.

This requires some repo changes:

TFPLAN_ARTIFACT_PASSPHRASE secret
aws-curvenote environment with branch restriction and required reviewers for deployments

See the ## Automatic deployment (GitHub workflows) section of the README file included here for details.

If everyone's happy with this plan I'll make the relevant changes in the mybinder.org-deploy repo settings before his is merged.

yuvipanda

Thank you so much for working on this, @manics. I've left some comments inside.

I think two overarching questions for me.

I have so far not trusted terraform plan to run on CI, anywhere. In general, I always recommend humans run it manually and type the 'yes', after making a PR with proposed changes. I've personally accidentally run terraform apply that has deleted entire clusters, user data, etc and had to ctrl-c them - things that went past review by others of both the code and the plan. Due to the intense destruction possible, I've never trusted it to auto run on CI. I'm not opposed to it here because you've already done all the hard work (the github oidc stuff is pretty cool!), but just wanted to ask if you're open to not having terraform apply be run on CI?
I don't think a lot of this is curvenote specific - is a curvenote directory with .tf files fully necessary? Can they not be just a .tfvars file? I am trying to understand what makes it special vs any other AWS setup we might have in the future.
I left another comment inline about the eks module.

In general, I think it's alright to merge this with minor modifications and then evolve this over time.

Thank you so much for working on this!

yuvipanda · 2023-08-08T00:23:24Z

.github/workflows/terraform-deploy-aws-curvenote.yml

+  push:
+    branches:
+      - main
+      - aws-curvenote2


I'm guessing this would need to be removed before merge?

Yes, I'm using it to test the continuous deployment process on my fork. I'll remove it just before merging (or in a follow-up PR).

yuvipanda · 2023-08-08T00:25:05Z

.github/workflows/terraform-deploy-aws-curvenote.yml

+
+      - name: Decrypt plan
+        run: |
+          echo ${{ secrets.TFPLAN_ARTIFACT_PASSPHRASE }} | gpg --batch --yes --passphrase-fd 0 --decrypt --cipher-algo AES256 --output "${TFPLAN}" "${TFPLAN}.gpg"


Can we use anything other than gpg? age perhaps?

I'm using gpg because it's pre-installed in the base environment. This is purely for internal GH workflow use- there's no way to privately pass an artefact between separate jobs, and since this is a public repo it needs to be encrypted. It's not intended to be used by people/maintainers since if we need the full details we can run Terraform anyway, it's just a way to ensure the public can't view any sensitive information in the plan. My plan is to create a random GitHub secret TFPLAN_ARTIFACT_PASSPHRASE and then throw it away so we'll never know the secret.

I'd still love to get rid of it. I basically can't trust myself to ever verify if a GPG commandline is actually doing what it is supposed to do. age can come from apt here, and we can just use it to symmetrically encrypt / decrypt. then:

Make an private age key with age-keygen -o <some-file>. The file is just one line, we put it into a GitHub secret

When we wanna encrypt, echo $KEY > <some-file>; age --identity <some-file> --encrypt --output <encrypted-output-file> <unencrypted-input-file>. We can also pass in input via stdin.

And to decrypt, do it in reverse: echo $KEY > <some-file>; age --identity <some-file> --decrypt --output <unencrypted-output-file> <encrypted-input-file>. The input can come via stdin too.

IMO this is much simpler to understand and verify, and given there's already a lot of complexity here I'd love to not touch GPG at all.

OK, I'll see if I can get that to work.

terraform/aws/curvenote/README.md

yuvipanda · 2023-08-08T00:28:49Z

terraform/aws/curvenote/bootstrap/backend.tf

+# Initial setup of S3 bucket to store tfstate file
+variable "bucket-name" {
+  type = string
+  # python -c 'import random; import string; print("".join(random.choices(string.ascii_lowercase + string.digits,k=12)))'


I'd suggest pwgen -1 12 instead?

Maybe- though pwgen isn't installed on my system, whereas Python is almost guaranteed to be installed on mybinder maintainers' systems.

I was trying to figure out if we should use the secrets module instead, given that random is a PRNG and shouldn't be used for 'security' purposes. However, this is not being used in any cryptographic manner, just as a way to avoid clashes - so is fine.

terraform/aws/curvenote/bootstrap/backend.tf

yuvipanda · 2023-08-08T00:29:53Z

terraform/aws/curvenote/bootstrap/backend.tf

+  bucket = aws_s3_bucket.bucket.id
+  rule {
+    apply_server_side_encryption_by_default {
+      sse_algorithm = "AES256"


Do we need to specify this? I thought it's on by default now https://aws.amazon.com/blogs/aws/amazon-s3-encrypts-new-objects-by-default/

Does that apply to the bucket's default policy, or only to the S3 upload API call?

Pretty sure it applies to the default policy. Can be verified by taking this out and looking in the AWS console to see if this has changed?

terraform/aws/curvenote/eks-cluster.tf

yuvipanda · 2023-08-08T00:31:49Z

terraform/aws/curvenote/eks-cluster.tf

+}
+
+# This assumes the EKS service linked role is already created (or the current user has permissions to create it)
+module "eks" {


I don't want to block this PR on this as it's a significant change, but just a few weeks ago I had to rip out this particular eks module out of terraform I maintain for pangeo-forge - pangeo-forge/pangeo-forge-cloud-federation#4. Ongoing maintanence was difficult and quite buggy unfortunately.

I'm ok with us giving it a try here, but given how simple the pangeo-forge env was I'm a little sad this module didn't work out.

It's been working fine for me, I've used it for a few clusters now. What problems did you run into?

@manics pangeo-forge/pangeo-forge-cloud-federation#3 was the problem when I tried to use it in pangeo-forge, and I couldn't debug what was going on after a few hours. Eventually I just had to tear it all down and rebuild it without the module. It was a similar setup with the 2i2c infra (where we are still using eksctl directly)

terraform/aws/curvenote/k8s-access.tf

terraform/aws/curvenote/outputs.tf

…s module

manics · 2023-08-28T23:04:32Z

I have so far not trusted terraform plan to run on CI, anywhere. In general, I always recommend humans run it manually and type the 'yes', after making a PR with proposed changes. I've personally accidentally run terraform apply that has deleted entire clusters, user data, etc and had to ctrl-c them - things that went past review by others of both the code and the plan. Due to the intense destruction possible, I've never trusted it to auto run on CI. I'm not opposed to it here because you've already done all the hard work (the github oidc stuff is pretty cool!), but just wanted to ask if you're open to not having terraform apply be run on CI?

You're right that it's a potentially very destructive action, but the same applies to Helm or any other continuous deployment process. In practice I've seen many more errors, near-misses, and other problems due to relying on manual terraform deployments in a team environment. Things like using the wrong AWS account profile, mixing up dev and prod, deploying the wrong repo, using the wrong branch, having an out-of-date branch, forgetting to push commits, pushing commits but not deploying, closing a laptop in the middle of a long running deployment leading to a corrupt state file, etc. The two step GitHub job with environment requiring manual approval can be setup to notify particular named users of a pending plan. It's not as good as a full hosted application for managing Terraform deployments, but I think it's good enough given the constraints of a public GitHub repo.

I don't think a lot of this is curvenote specific - is a curvenote directory with .tf files fully necessary? Can they not be just a .tfvars file? I am trying to understand what makes it special vs any other AWS setup we might have in the future.

Good point. I've moved it into a module binder-eks instead of relying on tfvars files. The benefit of the module is you can just do cd <deployment>; terraform apply for each deployment as with all the other clouds. Using variables would require terraform apply -var-file=<deployment>.tfvars which is more error prone.

.github/workflows/terraform-deploy-aws-curvenote.yml

manics · 2023-09-18T14:56:12Z

@yuvipanda Do you consider your remaining comments blockers? Does anyone else have thoughts on merging this?

yuvipanda · 2023-09-18T19:12:35Z

I will schedule some time for wednesday / thursday to give this another thorough looking over! Hopefully merge soon. One comment before that is:

Good point. I've moved it into a module binder-eks instead of relying on tfvars files. The benefit of the module is you can just do cd ; terraform apply for each deployment as with all the other clouds. Using variables would require terraform apply -var-file=.tfvars which is more error prone.

To me, the .tfvars are useful when you need to have many instances. So when we have another AWS binder, we could just add another .tfvars file rather than copy paste a whole directory. Could we not deal with the possible errors of not passing -var-file by making variables as required instead?

manics · 2023-09-19T15:12:10Z

So when we have another AWS binder, we could just add another .tfvars file rather than copy paste a whole directory.

This is a module, so you just reference it as a path, you don't need to copy anything:
https://github.com/manics/mybinder.org-deploy/blob/8c1cfb1c9080a0c325a90f7cac113faa88aa519f/terraform/aws/curvenote/main.tf#L35

An additional advantage of this is it's easy to add additional infrastructure for a particular AWS deployment instead of having lots of conditionals in the shared files, and if we want in future we could put binder-eks/ into a seperate versioned repo so multiple deployments can be updated separately, or it could be reused outside the mybinder.org project.

terraform/aws/curvenote/.terraform.lock.hcl

yuvipanda

Some super minor changes requested, but the primary one would be to replace gpg with age. After that I think this is good to go!

I'm still somewhat skeptical of the EKS module, but only because I really want it to work and have been burnt by that in the past. However, this is generally a fast moving space anyway, and I hope this does work. Let's try it out!

Thank you so much, @manics!

yuvipanda · 2023-09-21T17:31:01Z

terraform/aws/binder-eks/k8s-access.tf

+          StringLike = {
+            "token.actions.githubusercontent.com:sub" = [
+              # GitHub repositories and refs allowed to use this role
+              "repo:jupyterhub/mybinder.org-deploy:ref:refs/heads/main",


This is pretty cool I must say :)

yuvipanda · 2023-09-21T17:31:13Z

terraform/aws/binder-eks/k8s-access.tf

+              # GitHub repositories and refs allowed to use this role
+              "repo:jupyterhub/mybinder.org-deploy:ref:refs/heads/main",
+              # TODO: Remove this, just for development:
+              "repo:manics/mybinder.org-deploy:ref:refs/heads/aws-curvenote",


Just a reminder to get rid of this before merging.

yuvipanda · 2023-09-21T17:31:22Z

terraform/aws/binder-eks/k8s-access.tf

+              # Can't have branch and environment in the same condition
+              # https://github.com/aws-actions/configure-aws-credentials/issues/746
+              "repo:jupyterhub/mybinder.org-deploy:environment:aws-curvenote",
+              # TODO: Remove this, just for development:


yuvipanda · 2023-09-21T17:32:20Z

terraform/aws/binder-eks/vpc.tf

+
+  name = var.cluster_name
+  cidr = "10.0.0.0/16"
+  # azs                  = data.aws_availability_zones.available.names[:1]


Let's get rid of this commented out line, or use it?

yuvipanda · 2023-09-21T17:35:34Z

terraform/aws/curvenote/bootstrap/backend.tf

+# Initial setup of S3 bucket to store tfstate file
+variable "bucket-name" {
+  type = string
+  # python -c 'import random; import string; print("".join(random.choices(string.ascii_lowercase + string.digits,k=12)))'


I was trying to figure out if we should use the secrets module instead, given that random is a PRNG and shouldn't be used for 'security' purposes. However, this is not being used in any cryptographic manner, just as a way to avoid clashes - so is fine.

yuvipanda · 2023-09-21T17:38:45Z

terraform/aws/curvenote/bootstrap/backend.tf

+  bucket = aws_s3_bucket.bucket.id
+  rule {
+    apply_server_side_encryption_by_default {
+      sse_algorithm = "AES256"


Pretty sure it applies to the default policy. Can be verified by taking this out and looking in the AWS console to see if this has changed?

yuvipanda

Thanks a lot, @manics! I think this is ready to merge! I've marked it as approved, so you should merge whenever you feel convenient.

EXCITED TO SEE THIS WORK!

https://github.blog/2022-05-09-supercharging-github-actions-with-job-summaries/

manics · 2023-09-24T20:16:01Z

@yuvipanda Thanks for working through this! I've pushed a couple more minor changes, if no-one else has comments I'll setup the GitHub secret and environment tomorrow and merge.

manics · 2023-09-25T21:31:31Z

I've created the secret TFPLAN_ARTIFACT_SECRET_KEY
and the Environment:

So far I've only added myself to be notified when there is a pending terraform change, but I think we should change that to a GitHub team- to be followed up.

manics force-pushed the aws-curvenote branch 4 times, most recently from dcedf90 to 47b5c33 Compare June 7, 2023 18:31

manics mentioned this pull request Jun 12, 2023

Explore CurveNote AWS credits for mybinder.org #2629

Closed

manics force-pushed the aws-curvenote branch from 11e1248 to b60335c Compare June 13, 2023 22:00

manics mentioned this pull request Jun 13, 2023

deploy.py: add dry-run option + minor refactor to support future deployments #2671

Merged

manics force-pushed the aws-curvenote branch 4 times, most recently from b2a8878 to ccf4887 Compare June 13, 2023 23:00

manics force-pushed the aws-curvenote branch from cb083c0 to 51c8850 Compare June 30, 2023 19:21

manics commented Jun 30, 2023

View reviewed changes

.github/workflows/aws-dev.yml Outdated Show resolved Hide resolved

manics commented Jun 30, 2023

View reviewed changes

manics force-pushed the aws-curvenote branch 2 times, most recently from d8168e0 to b74caf9 Compare June 30, 2023 21:17

manics marked this pull request as ready for review June 30, 2023 21:28

manics changed the title ~~WIP: EKS in AWS Curvenote account~~ EKS in AWS Curvenote account Jun 30, 2023

manics force-pushed the aws-curvenote branch 2 times, most recently from d791115 to b3e6818 Compare June 30, 2023 21:44

manics added 8 commits July 12, 2023 19:45

curvenote-aws: bootstrap and vpc

7e44698

EKS with permissions boundaries and other workarounds

6518eab

Use remote state and dynamodb lock

54057f1

Add serviceaccounts.tf

ece451d

Update version

de030c8

Service link role deployed separately, this now works!

408510a

terraform/aws/curvenote linting/formatting

e061b7c

Move terraform lint to separate workflow

16cccd9

eks: node size cant be changed by terraform

80bdbe2

yuvipanda reviewed Aug 8, 2023

View reviewed changes

manics added 8 commits August 28, 2023 21:20

Move curvenote terraform to generic binder-eks module

6cb8d3c

Add outputs to binder-eks module

02cb10f

aws/curvenote is now just a single file that references the binder-ek…

ea59795

…s module

Use worker_group_1 instead of worker_group-1

55dd529

binder-eks: uncomment outputs

2d4c439

Update AWS/EKS readmes

a735a08

Update pre-commit, cleanup old comments

432f115

Rename bootstrap state bucket var

aebaa80

manics commented Sep 18, 2023

View reviewed changes

.github/workflows/terraform-deploy-aws-curvenote.yml Outdated Show resolved Hide resolved

terraform-deploy-aws-curvenote.yml: remove extra branch

8c1cfb1

minrk reviewed Sep 19, 2023

View reviewed changes

terraform/aws/curvenote/.terraform.lock.hcl Outdated Show resolved Hide resolved

yuvipanda requested changes Sep 21, 2023

View reviewed changes

manics added 5 commits September 22, 2023 14:45

curvenote tfplan: replace gpg with age

d7036e0

curvenote: Install age in GH workflow

af3d573

Fix gh trigger paths for terraform-deploy-aws-curvenote.yml

315ad74

Remove aws_s3_bucket_server_side_encryption_configuration from bootstrap

f48c4f7

Remove dev OIDC access

55666c1

yuvipanda approved these changes Sep 22, 2023

View reviewed changes

manics added 2 commits September 24, 2023 16:52

Output Terraform plan to GH summary

1851b2a

https://github.blog/2022-05-09-supercharging-github-actions-with-job-summaries/

vpc.tf: Remove old commented azs

c00c237

manics merged commit 5cf3383 into jupyterhub:main Sep 25, 2023
2 checks passed

manics deleted the aws-curvenote branch September 25, 2023 21:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EKS in AWS Curvenote account #2652

EKS in AWS Curvenote account #2652

manics commented Jun 1, 2023 •

edited

Loading

manics Jun 30, 2023

manics commented Jun 30, 2023

manics commented Jul 10, 2023 •

edited

Loading

yuvipanda left a comment

yuvipanda Aug 8, 2023

manics Aug 28, 2023

yuvipanda Aug 8, 2023

manics Aug 28, 2023

yuvipanda Sep 18, 2023

manics Sep 19, 2023

yuvipanda Aug 8, 2023

manics Aug 28, 2023

yuvipanda Sep 21, 2023

yuvipanda Aug 8, 2023

manics Aug 28, 2023

yuvipanda Sep 21, 2023

yuvipanda Aug 8, 2023

manics Aug 28, 2023

yuvipanda Aug 29, 2023

manics commented Aug 28, 2023

manics commented Sep 18, 2023

yuvipanda commented Sep 18, 2023

manics commented Sep 19, 2023 •

edited

Loading

yuvipanda left a comment

yuvipanda Sep 21, 2023

yuvipanda Sep 21, 2023

yuvipanda Sep 21, 2023

yuvipanda Sep 21, 2023

yuvipanda Sep 21, 2023

yuvipanda Sep 21, 2023

yuvipanda left a comment

manics commented Sep 24, 2023

manics commented Sep 25, 2023

EKS in AWS Curvenote account #2652

EKS in AWS Curvenote account #2652

Conversation

manics commented Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

manics commented Jun 30, 2023

manics commented Jul 10, 2023 • edited Loading

yuvipanda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manics commented Aug 28, 2023

manics commented Sep 18, 2023

yuvipanda commented Sep 18, 2023

manics commented Sep 19, 2023 • edited Loading

yuvipanda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuvipanda left a comment

Choose a reason for hiding this comment

manics commented Sep 24, 2023

manics commented Sep 25, 2023

manics commented Jun 1, 2023 •

edited

Loading

manics commented Jul 10, 2023 •

edited

Loading

manics commented Sep 19, 2023 •

edited

Loading