Merge pull request #3932 from consideRatio/pr/s3-readonly

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

deployer/commands/generate/billing/importers.py

@@ -16,7 +16,6 @@ def build_gcp_query(cluster: dict, service_id=None):
 
     Returns:
         string: query string
-    """ """
     """
     bq = cluster["gcp"]["billing"]["bigquery"]
 

docs/helper-programs/generate-hub-features-table.py

@@ -155,6 +155,11 @@ def parse_terraform_value_files_for_features(terraform_config):
         hub_cloud_permissions = terraform_config.get("hub_cloud_permissions", None)
         if hub_cloud_permissions:
             for hub_slug, permissions in hub_cloud_permissions.items():
+                # The permission object doesn't have the same structure in AWS
+                # as for GCP currently, and requestor_pays is only available for
+                # GCP currently. The logic below works, but needs an update if
+                # GCP aligns with the same structure as AWS, or if
+                # requestor_pays config is added for AWS.
                 features[hub_slug] = {
                     "user_buckets": True,
                     "requestor_pays": permissions.get("requestor_pays", False),

terraform/aws/bucket-access.tf

@@ -0,0 +1,150 @@
+/*
+  Creates one aws_s3_bucket_policy per bucket - there can't be more than one as
+  they otherwise replace each other when applied.
+
+  The bucket policies grant bucket specific permissions to specific IAM Roles
+  based on them having `bucket_admin_access` or `bucket_readonly_access`
+  referencing the bucket via `var.hub_cloud_permissions`.
+*/
+
+locals {
+  /*
+    The bucket_role_actions local variable defined below is a list of objects
+    generated from `var.hub_cloud_permissions` roles and their respective
+    bucket_admin_access and bucket_readonly_access lists.
+
+    If for example `var.hub_cloud_permissions` is:
+
+    hub_cloud_permissions:
+      staging:
+        user-sa:
+          bucket_admin_access: [scratch-staging]
+      sciencecore:
+        user-sa:
+          bucket_admin_access: [scratch-sciencecore]
+          bucket_readonly_access: [persistent-sciencecore]
+        admin-sa:
+          bucket_admin_access: [scratch-sciencecore, persistent-sciencecore]
+
+    Then, the `local.bucket_role_actions` will look like below, with one list
+    item for each element in all `bucket_admin/readonly_access` lists:
+
+    bucket_role_actions:
+      - bucket: scratch-staging
+        role: staging
+        actions: ["s3:*"]
+      - bucket: scratch-sciencecore
+        role: sciencecore
+        actions: ["s3:*"]
+      - bucket: scratch-sciencecore
+        role: sciencecore-admin-sa
+        actions: ["s3:*"]
+      - bucket: persistent-sciencecore
+        role: sciencecore
+        actions: ["s3:ListBucket", "s3:GetObject", "s3:GetObjectVersion"]
+      - bucket: persistent-sciencecore
+        role: sciencecore
+        actions: ["s3:*"]
+  */
+  bucket_role_actions = flatten([
+    for hub, hub_value in var.hub_cloud_permissions : [
+      for role, role_value in hub_value : flatten([
+        [
+          for bucket in role_value.bucket_admin_access : {
+            bucket = bucket
+            // role should match the id set in irsa.tf
+            role    = role == "user-sa" ? hub : "${hub}-${role}"
+            actions = ["s3:*"]
+          }
+        ],
+        [
+          for bucket in role_value.bucket_readonly_access : {
+            bucket = bucket
+            // role should match the id set in irsa.tf
+            role = role == "user-sa" ? hub : "${hub}-${role}"
+            actions = [
+              "s3:ListBucket",
+              "s3:GetObject",
+              "s3:GetObjectVersion",
+            ]
+          }
+        ],
+      ])
+    ]
+  ])
+}
+
+locals {
+  /*
+    The `local.bucket_role_actions_lists` variable defined below is reprocessing
+    `local.bucket_role_actions` to a dictionary with one key per bucket with
+    associated permissions.
+
+    Below is an example value `local.bucket_role_actions_lists` could take. This
+    example value is aligned with the example value for
+    `var.hub_cloud_permissions` and `local.bucket_role_actions` in the code
+    block above.
+
+    bucket_role_actions_lists:
+      scratch-staging:
+        - bucket: scratch-staging
+          role: staging
+          actions: ["s3:*"]
+      scratch-sciencecore:
+        - bucket: scratch-sciencecore
+          role: sciencecore
+          actions: ["s3:*"]
+        - bucket: scratch-sciencecore
+          role: sciencecore-admin-sa
+          actions: ["s3:*"]
+      persistent-sciencecore:
+        - bucket: persistent-sciencecore
+          role: sciencecore
+          actions: ["s3:ListBucket", "s3:GetObject", "s3:GetObjectVersion"]
+        - bucket: persistent-sciencecore
+          role: sciencecore
+          actions: ["s3:*"]
+  */
+  bucket_role_actions_lists = {
+    for bucket, _ in var.user_buckets :
+    bucket => [for bra in local.bucket_role_actions : bra if bra.bucket == bucket]
+    // Filter out user_buckets not mentioned in hub_cloud_permissions
+    if length([for bra in local.bucket_role_actions : bra if bra.bucket == bucket]) != 0
+  }
+}
+
+
+
+data "aws_iam_policy_document" "bucket_policy" {
+  for_each = local.bucket_role_actions_lists
+
+  // Only one policy document can be declared per bucket, so we provide multiple
+  // "statement" in this policy.
+  dynamic "statement" {
+    for_each = { for index, bra in each.value : "${bra.bucket}.${bra.role}" => bra }
+
+    content {
+      effect  = "Allow"
+      actions = statement.value.actions
+      principals {
+        type = "AWS"
+        identifiers = [
+          aws_iam_role.irsa_role[statement.value.role].arn
+        ]
+      }
+      resources = [
+        # Grant access only to the bucket and its contents
+        aws_s3_bucket.user_buckets[statement.value.bucket].arn,
+        "${aws_s3_bucket.user_buckets[statement.value.bucket].arn}/*",
+      ]
+    }
+  }
+}
+
+// There can only be one of these per bucket, if more are defined they will end
+// up replacing each other without terraform indicating there is trouble.
+resource "aws_s3_bucket_policy" "user_bucket_access" {
+  for_each = local.bucket_role_actions_lists
+  bucket   = aws_s3_bucket.user_buckets[each.key].id
+  policy   = data.aws_iam_policy_document.bucket_policy[each.key].json
+}

terraform/aws/buckets.tf

@@ -1,7 +1,6 @@
 resource "aws_s3_bucket" "user_buckets" {
   for_each = var.user_buckets
   bucket   = lower("${var.cluster_name}-${each.key}")
-
 }
 
 resource "aws_s3_bucket_lifecycle_configuration" "user_bucket_expiry" {
@@ -36,44 +35,6 @@ resource "aws_s3_bucket_lifecycle_configuration" "user_bucket_expiry" {
   }
 }
 
-locals {
-  # Nested for loop, thanks to https://www.daveperrett.com/articles/2021/08/19/nested-for-each-with-terraform/
-  bucket_permissions = distinct(flatten([
-    for hub_name, permissions in var.hub_cloud_permissions : [
-      for bucket_name in permissions.bucket_admin_access : {
-        hub_name    = hub_name
-        bucket_name = bucket_name
-      }
-    ]
-  ]))
-}
-
-data "aws_iam_policy_document" "bucket_access" {
-  for_each = { for bp in local.bucket_permissions : "${bp.hub_name}.${bp.bucket_name}" => bp }
-  statement {
-    effect  = "Allow"
-    actions = ["s3:*"]
-    principals {
-      type = "AWS"
-      identifiers = [
-        aws_iam_role.irsa_role[each.value.hub_name].arn
-      ]
-    }
-    resources = [
-      # Grant access only to the bucket and its contents
-      aws_s3_bucket.user_buckets[each.value.bucket_name].arn,
-      "${aws_s3_bucket.user_buckets[each.value.bucket_name].arn}/*"
-    ]
-  }
-}
-
-resource "aws_s3_bucket_policy" "user_bucket_access" {
-
-  for_each = { for bp in local.bucket_permissions : "${bp.hub_name}.${bp.bucket_name}" => bp }
-  bucket   = aws_s3_bucket.user_buckets[each.value.bucket_name].id
-  policy   = data.aws_iam_policy_document.bucket_access[each.key].json
-}
-
 output "buckets" {
   value       = { for b, _ in var.user_buckets : b => aws_s3_bucket.user_buckets[b].id }
   description = <<-EOT

terraform/aws/irsa.tf

@@ -1,20 +1,41 @@
-data "aws_caller_identity" "current" {}
+/*
+  This file provides resources _per hub and role_. Each role is tied to a
+  specific k8s ServiceAccount allowed to assume the role.
+
+  - Role                 - for use by k8s ServiceAccount (user-sa, admin-sa)
+  - Policy               - if extra_iam_policy is declared
+  - RolePolicyAttachment - if extra_iam_policy is declared
+*/
 
+data "aws_caller_identity" "current" {}
 data "aws_partition" "current" {}
 
-resource "aws_iam_role" "irsa_role" {
-  for_each = var.hub_cloud_permissions
-  name     = "${var.cluster_name}-${each.key}"
 
-  assume_role_policy = data.aws_iam_policy_document.irsa_role_assume[each.key].json
+
+locals {
+  # Nested for loop, thanks to https://www.daveperrett.com/articles/2021/08/19/nested-for-each-with-terraform/
+  hub_to_role_mapping = flatten([
+    for hub, hub_value in var.hub_cloud_permissions : [
+      for ksa_name, cloud_permissions in hub_value : {
+        // Most hubs only use `user-sa`, so we use just the hub name for the IAM
+        // role for user-sa. `user-sa` was also the only service account supported
+        // for a long time, so this special casing reduces the amount of work
+        // we needed to do to introduce other service accounts.
+        iam_role_name     = ksa_name == "user-sa" ? hub : "${hub}-${ksa_name}"
+        hub               = hub
+        ksa_name          = ksa_name
+        cloud_permissions = cloud_permissions
+      }
+    ]
+  ])
 }
 
-data "aws_iam_policy_document" "irsa_role_assume" {
-  for_each = var.hub_cloud_permissions
-  statement {
 
-    effect = "Allow"
 
+data "aws_iam_policy_document" "irsa_role_assume" {
+  for_each = { for index, hr in local.hub_to_role_mapping : hr.iam_role_name => hr }
+  statement {
+    effect  = "Allow"
     actions = ["sts:AssumeRoleWithWebIdentity"]
 
     principals {
@@ -28,29 +49,41 @@ data "aws_iam_policy_document" "irsa_role_assume" {
       test     = "StringEquals"
       variable = "${replace(data.aws_eks_cluster.cluster.identity[0].oidc[0].issuer, "https://", "")}:sub"
       values = [
-        "system:serviceaccount:${each.key}:user-sa"
+        "system:serviceaccount:${each.value.hub}:${each.value.ksa_name}"
       ]
     }
   }
 }
 
+resource "aws_iam_role" "irsa_role" {
+  for_each = { for index, hr in local.hub_to_role_mapping : hr.iam_role_name => hr }
+  name     = "${var.cluster_name}-${each.key}"
+
+  assume_role_policy = data.aws_iam_policy_document.irsa_role_assume[each.key].json
+}
+
+
+
 resource "aws_iam_policy" "extra_user_policy" {
-  for_each    = { for hub_name, value in var.hub_cloud_permissions : hub_name => value if value.extra_iam_policy != "" }
-  name        = "${var.cluster_name}-${each.key}-extra-user-policy"
+  for_each = { for index, hr in local.hub_to_role_mapping : hr.iam_role_name => hr if hr.cloud_permissions.extra_iam_policy != "" }
+  name     = "${var.cluster_name}-${each.key}-extra-user-policy"
+
   description = "Extra permissions granted to users on hub ${each.key} on ${var.cluster_name}"
-  policy      = each.value.extra_iam_policy
+  policy      = each.value.cloud_permissions.extra_iam_policy
 }
 
 resource "aws_iam_role_policy_attachment" "extra_user_policy" {
-  for_each   = { for hub_name, value in var.hub_cloud_permissions : hub_name => value if value.extra_iam_policy != "" }
+  for_each   = { for index, hr in local.hub_to_role_mapping : hr.iam_role_name => hr if hr.cloud_permissions.extra_iam_policy != "" }
   role       = aws_iam_role.irsa_role[each.key].name
   policy_arn = aws_iam_policy.extra_user_policy[each.key].arn
 }
 
+
+
 output "kubernetes_sa_annotations" {
   value = {
-    for k, v in var.hub_cloud_permissions :
-    k => "eks.amazonaws.com/role-arn: ${aws_iam_role.irsa_role[k].arn}"
+    for index, hr in local.hub_to_role_mapping :
+    hr.iam_role_name => "eks.amazonaws.com/role-arn: ${aws_iam_role.irsa_role[hr.iam_role_name].arn}"
   }
   description = <<-EOT
   Annotations to apply to userServiceAccount in each hub to enable cloud permissions for them.

terraform/aws/projects/2i2c-aws-us.tfvars

@@ -31,30 +31,36 @@ user_buckets = {
 
 hub_cloud_permissions = {
   "staging" : {
-    bucket_admin_access : ["scratch-staging"],
-    extra_iam_policy : ""
+    "user-sa" : {
+      bucket_admin_access : ["scratch-staging"],
+    },
   },
   "dask-staging" : {
-    bucket_admin_access : ["scratch-dask-staging"],
-    extra_iam_policy : ""
+    "user-sa" : {
+      bucket_admin_access : ["scratch-dask-staging"],
+    },
   },
   "showcase" : {
-    bucket_admin_access : [
-      "scratch-researchdelight",
-      "persistent-showcase"
-    ],
-    extra_iam_policy : ""
+    "user-sa" : {
+      bucket_admin_access : [
+        "scratch-researchdelight",
+        "persistent-showcase",
+      ],
+    },
   },
   "ncar-cisl" : {
-    bucket_admin_access : ["scratch-ncar-cisl"],
-    extra_iam_policy : ""
+    "user-sa" : {
+      bucket_admin_access : ["scratch-ncar-cisl"],
+    },
   },
   "go-bgc" : {
-    bucket_admin_access : ["scratch-go-bgc"],
-    extra_iam_policy : ""
+    "user-sa" : {
+      bucket_admin_access : ["scratch-go-bgc"],
+    },
   },
   "itcoocean" : {
-    bucket_admin_access : ["scratch-itcoocean"],
-    extra_iam_policy : ""
+    "user-sa" : {
+      bucket_admin_access : ["scratch-itcoocean"],
+    },
   },
 }

terraform/aws/projects/bican.tfvars

@@ -16,11 +16,13 @@ user_buckets = {
 
 hub_cloud_permissions = {
   "staging" : {
-    bucket_admin_access : ["scratch-staging"],
-    extra_iam_policy : ""
+    "user-sa" : {
+      bucket_admin_access : ["scratch-staging"],
+    },
   },
   "prod" : {
-    bucket_admin_access : ["scratch"],
-    extra_iam_policy : ""
+    "user-sa" : {
+      bucket_admin_access : ["scratch"],
+    },
   },
-}
+}