aws, terraform: support creation of more than a single IAM Role, and add bucket_readonly_access config

[consideRatio]

New feature

• Adds terraform config bucket_readonly_access under hub_cloud_permissions

Changed

The AWS terraform variable hub_cloud_permissions is changed from having a structure of:

hub_cloud_permissions:
  <hub name>:
    bucket_admin_access: []
    extra_iam_policy: ""

to:

hub_cloud_permissions:
  <hub name / k8s namespace>:
    <aws iam role name / k8s service account name>
      bucket_admin_access: []
      bucket_readonly_access: []
      extra_iam_policy: ""

Where the fields bucket_admin_access, bucket_readonly_access, and extra_iam_policy have also been made optional.

terraform apply changes

This change is meant to not cause issues when terraform apply is used for existing clusters. It will however cause a change to a resource - the bucket access policy.

`terraform apply` safe-to-apply changes

Terraform will perform the following actions:

  # aws_s3_bucket_policy.user_bucket_access["prod.scratch"] will be destroyed
  # (because key ["prod.scratch"] is not in for_each map)
  - resource "aws_s3_bucket_policy" "user_bucket_access" {
      - bucket = "bican-scratch" -> null
      - id     = "bican-scratch" -> null
      - policy = jsonencode(
            {
              - Statement = [
                  - {
                      - Action    = "s3:*"
                      - Effect    = "Allow"
                      - Principal = {
                          - AWS = "arn:aws:iam::533267153382:role/bican-prod"
                        }
                      - Resource  = [
                          - "arn:aws:s3:::bican-scratch/*",
                          - "arn:aws:s3:::bican-scratch",
                        ]
                      - Sid       = ""
                    },
                ]
              - Version   = "2012-10-17"
            }
        ) -> null
    }

  # aws_s3_bucket_policy.user_bucket_access["scratch"] will be created
  + resource "aws_s3_bucket_policy" "user_bucket_access" {
      + bucket = "bican-scratch"
      + id     = (known after apply)
      + policy = jsonencode(
            {
              + Statement = [
                  + {
                      + Action    = "s3:*"
                      + Effect    = "Allow"
                      + Principal = {
                          + AWS = "arn:aws:iam::533267153382:role/bican-prod"
                        }
                      + Resource  = [
                          + "arn:aws:s3:::bican-scratch/*",
                          + "arn:aws:s3:::bican-scratch",
                        ]
                      + Sid       = ""
                    },
                ]
              + Version   = "2012-10-17"
            }
        )
    }

  # aws_s3_bucket_policy.user_bucket_access["scratch-staging"] will be created
  + resource "aws_s3_bucket_policy" "user_bucket_access" {
      + bucket = "bican-scratch-staging"
      + id     = (known after apply)
      + policy = jsonencode(
            {
              + Statement = [
                  + {
                      + Action    = "s3:*"
                      + Effect    = "Allow"
                      + Principal = {
                          + AWS = "arn:aws:iam::533267153382:role/bican-staging"
                        }
                      + Resource  = [
                          + "arn:aws:s3:::bican-scratch-staging/*",
                          + "arn:aws:s3:::bican-scratch-staging",
                        ]
                      + Sid       = ""
                    },
                ]
              + Version   = "2012-10-17"
            }
        )
    }

  # aws_s3_bucket_policy.user_bucket_access["staging.scratch-staging"] will be destroyed
  # (because key ["staging.scratch-staging"] is not in for_each map)
  - resource "aws_s3_bucket_policy" "user_bucket_access" {
      - bucket = "bican-scratch-staging" -> null
      - id     = "bican-scratch-staging" -> null
      - policy = jsonencode(
            {
              - Statement = [
                  - {
                      - Action    = "s3:*"
                      - Effect    = "Allow"
                      - Principal = {
                          - AWS = "arn:aws:iam::533267153382:role/bican-staging"
                        }
                      - Resource  = [
                          - "arn:aws:s3:::bican-scratch-staging/*",
                          - "arn:aws:s3:::bican-scratch-staging",
                        ]
                      - Sid       = ""
                    },
                ]
              - Version   = "2012-10-17"
            }
        ) -> null
    }

Plan: 2 to add, 0 to change, 2 to destroy.

Limitations and possible issues

• hub_cloud_permissions structure is now changed for AWS but not for GCP, making them have different structure but otherwise very similar structure.
• bucket_readonly_access is only implemented for AWS with this PR
• dask-gateway is hardcoded to use user-sa and can't securely use whatever the user is having I think.
  So, if we configure jupyterhub.custom.singleuserAdmin.serviceAccountName: admin-sa, users requests for dask clusters (dask-scheduler pod + dask-worker pods) they won't have the same service account.

Reamining work before merge

• There is a docs helper script creating a table of features, this code needs to be updated as the structure of
• Check misc docstrings etc
• terraform apply across all clusters so this change doesn't cause confusion for other people later

Remaining work after merge

We decided that we'll get this merged first, and then write docs as we wanted to prio other things.

• Write documentation about this functionality
  Tracked by Update infra docs to reflect recent terraform aws changes #3987

Related

• Fixes https://github.com/2i2c-org/meta/issues/979
• It can also fix other similar requests:
  • [Support] cryocloud: Allow bucket access based on hub admin status #2886
  • [Support] 2i2c-aws-us/itcoocean: help to setup s3 bucket (admin/non-admin with readwrite/readonly access) #3038

[consideRatio]

Planning

@yuvipanda I've rebased and finalized this PR on all current k8s clusters (kitware included). This is what I consider remains:

1. Review (-> iteration) -> Approved
2. Erik applies the safe-to-apply-no-real-change changes to all AWS clusters, and while doing that checks carefully that only the expected things are applied
3. Erik merges PR
4. Erik opens issue to track writing docs about this feature

[yuvipanda]

Thanks for scoping and working on this, @consideRatio. The overall structure looks good to me - it causes minimal churn right now while also allowing us to expand this in the future if we wish. Splitting the files is also a good call.

I've left some comments about naming, as well as made two PRs to your PR with respect to some doc strings and comments. With those addressed, I think this is good to go.

I didn't test the actual effects of these, although I did test terraform apply and observed the changes to be ok. I think you've already tested them in various places. I would say we should make sure the extra_iam_permission hubs work correctly, and make sure we verify those after apply.

Thanks!

[consideRatio]

Thank you for reviewing this carefully and making it easy for me to address your input @yuvipanda! All review comments addressed in pushed commits.

I've previously checked that these changes work end to end in opensci/sciencecore - and I think they have also successfully been used by the community. I've also checked after the new commits have been made, that terraform apply leads to No changes in opensci cluster as expected.

If this looks good to you now, I've outlined the next steps I intend to take in #3932 (comment)

[yuvipanda]

Thanks @consideRatio. Your action plan sounds good to me. Please proceed.

[consideRatio]

Thank you for meeting with me about what led to this and reviewing efforts @yuvipanda!!