feat(comet_eks): byo_s3_irsa_roles — customer bring-your-own-S3 IAM (DND-1423) (#48)

* feat(comet_eks): add byo_s3_irsa_roles for customer bring-your-own-S3 (DND-1423)

Add a reusable, opt-in `byo_s3_irsa_roles` map that provisions the IAM that
accompanies a customer-supplied S3 bucket: one IRSA role + a scoped
customer-managed policy + attachment per map entry. The trusted Kubernetes
ServiceAccounts (the IRSA :sub condition) are an explicit input, so the trust
list is codified and reviewable in PRs.

This generalizes the BYO-S3 access role that was previously created out-of-band
during onboarding (e.g. Zoox's ZooxS3Access), whose invisible trust list let a
missing ServiceAccount silently break ClickHouse remote backups for >=7 days
(DND-1413). It follows the existing Loki IRSA triplet exactly and reuses the
upstream iam-role-for-service-accounts-eks module to build the web-identity
trust.

- Permissions are scoped to the supplied bucket ARN(s) by default (not
  s3:::*), so the feature is least-privilege from day one.
- role_name_override / policy_name_override let an existing out-of-band role or
  policy be adopted in place via `terraform import` (stable ARN -> IRSA
  annotations keep working) instead of being recreated.
- Empty map default -> no effect on any existing consumer.

New MINOR (new feature family): release as v1.21.0.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(comet_eks): validate byo_s3_irsa_roles inputs at plan time (DND-1423 PR review)

Address Baz review on PR #48. The variable only length-checked its lists, so
malformed inputs passed `terraform validate` and failed at apply (or silently
produced broken IAM/IRSA):

- bucket_arns: require an exact `arn:aws:s3:::<bucket>` per entry — reject
  wildcards (`arn:aws:s3:::*`, which would reintroduce the over-broad grant this
  feature exists to remove) and trailing `/*`/object keys (main.tf already
  appends `/*`, so a trailing glob yields `<bucket>/*/*`).
- namespace_service_accounts: require `<namespace>:<sa-name>` — a malformed
  subject silently produces a trust condition no pod can satisfy.
- policy_name_override: add IAM policy-name validation (1-128 chars) for parity
  with the existing role_name_override check.

Verified the regexes accept the intended Zoox values and reject the bad cases;
`terraform validate` passes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: jms200

main.tf

@@ -327,6 +327,9 @@ module "comet_eks" {
   loki_s3_bucket_arn          = var.enable_s3 && var.enable_loki_bucket ? module.comet_s3[0].comet_loki_bucket_arn : null
   loki_iam_role_name_override = var.loki_iam_role_name_override
 
+  # DND-1423: Bring-your-own-S3 IRSA roles (customer-supplied buckets)
+  byo_s3_irsa_roles = var.byo_s3_irsa_roles
+
   # CloudWatch Exporter IRSA for scraping AWS managed service metrics
   enable_cloudwatch_exporter                 = var.enable_cloudwatch_exporter
   cloudwatch_exporter_iam_role_name_override = var.cloudwatch_exporter_iam_role_name_override

modules/comet_eks/main.tf

@@ -828,6 +828,89 @@ module "loki_irsa_role" {
   )
 }
 
+################################################################
+#### BYO-S3 IRSA Roles (customer-supplied bucket) - DND-1423 ###
+################################################################
+# One IRSA role + scoped customer-managed policy per byo_s3_irsa_roles entry.
+# Generalizes the out-of-band ZooxS3Access pattern: a set of ServiceAccounts
+# gets scoped access to a customer's own S3 bucket. The upstream IRSA module
+# builds the OIDC web-identity trust (:sub/:aud) from namespace_service_accounts.
+locals {
+  # Default action set matches the live ClickHouse-backup S3 usage.
+  byo_s3_default_actions = [
+    "s3:GetObject",
+    "s3:PutObject",
+    "s3:DeleteObject",
+    "s3:ListBucket",
+    "s3:AbortMultipartUpload",
+    "s3:ListMultipartUploadParts",
+    "s3:ListBucketMultipartUploads",
+  ]
+}
+
+data "aws_iam_policy_document" "byo_s3" {
+  for_each = var.byo_s3_irsa_roles
+
+  statement {
+    effect  = "Allow"
+    actions = coalesce(each.value.actions, local.byo_s3_default_actions)
+    # Scoped to the supplied bucket(s) - bucket ARN (for ListBucket) + objects.
+    resources = flatten([
+      for arn in each.value.bucket_arns : [arn, "${arn}/*"]
+    ])
+  }
+}
+
+resource "aws_iam_policy" "byo_s3" {
+  for_each = var.byo_s3_irsa_roles
+
+  # name when adopting an existing policy in place; name_prefix otherwise.
+  name        = each.value.policy_name_override
+  name_prefix = each.value.policy_name_override == null ? "${var.environment}-byo-s3-${each.key}-" : null
+  description = "BYO-S3 access for ${each.key} on ${var.environment} cluster (DND-1423)"
+  policy      = data.aws_iam_policy_document.byo_s3[each.key].json
+
+  tags = merge(
+    var.common_tags,
+    {
+      Name = coalesce(each.value.policy_name_override, "${var.environment}-byo-s3-${each.key}")
+    }
+  )
+}
+
+module "byo_s3_irsa_role" {
+  source  = "terraform-aws-modules/iam/aws//modules/iam-role-for-service-accounts-eks"
+  version = "~> 5.39"
+
+  for_each = var.byo_s3_irsa_roles
+
+  role_name = coalesce(each.value.role_name_override, "${var.environment}-byo-s3-${each.key}")
+
+  role_policy_arns = {
+    byo_s3 = aws_iam_policy.byo_s3[each.key].arn
+  }
+
+  oidc_providers = {
+    ex = {
+      provider_arn               = module.eks.oidc_provider_arn
+      namespace_service_accounts = each.value.namespace_service_accounts
+    }
+  }
+
+  depends_on = [
+    module.eks,
+    aws_iam_policy.byo_s3
+  ]
+
+  tags = merge(
+    var.common_tags,
+    {
+      Name        = coalesce(each.value.role_name_override, "${var.environment}-byo-s3-${each.key}")
+      Description = "IRSA role granting listed ServiceAccounts scoped access to a customer-supplied S3 bucket"
+    }
+  )
+}
+
 ##################################################
 #### CloudWatch Exporter IRSA Role and Policy ####
 ##################################################

modules/comet_eks/variables.tf

@@ -412,6 +412,77 @@ variable "loki_s3_bucket_arn" {
   default     = null
 }
 
+# DND-1423: Bring-your-own-S3 IRSA roles. When a customer supplies their own S3
+# bucket (e.g. ClickHouse remote backups), the pods that touch it need an IAM
+# role assumable via IRSA and a policy scoped to that bucket. Each map entry
+# provisions one such role + customer-managed policy + attachment. This
+# generalizes roles previously created out-of-band during BYO-S3 onboarding
+# (e.g. Zoox's ZooxS3Access) so the trusted ServiceAccounts are codified and
+# reviewable. The map key is a short logical name used in resource naming.
+variable "byo_s3_irsa_roles" {
+  description = "Map of bring-your-own-S3 IRSA roles. Each entry grants the listed Kubernetes ServiceAccounts (via IRSA web-identity) scoped access to a customer-supplied S3 bucket. Empty by default (feature off)."
+  type = map(object({
+    # ARNs of the customer-supplied bucket(s). Permissions are scoped to these
+    # (bucket + bucket/*) - do NOT pass arn:aws:s3:::* here.
+    bucket_arns = list(string)
+    # ServiceAccounts allowed to assume the role, as "<namespace>:<sa-name>".
+    namespace_service_accounts = list(string)
+    # Optional S3 action set. Null => the ClickHouse-backup default action set.
+    actions = optional(list(string))
+    # Optional overrides to adopt an existing out-of-band role/policy in place
+    # (e.g. role_name_override="ZooxS3Access") via terraform import without
+    # recreating it (recreation would change the ARN and break IRSA annotations).
+    role_name_override   = optional(string)
+    policy_name_override = optional(string)
+  }))
+  default = {}
+
+  validation {
+    condition = alltrue([
+      for k, v in var.byo_s3_irsa_roles :
+      length(v.bucket_arns) > 0 && length(v.namespace_service_accounts) > 0
+    ])
+    error_message = "Each byo_s3_irsa_roles entry must set at least one bucket_arn and one namespace_service_account."
+  }
+  # Each bucket_arn must be an exact bucket ARN: arn:aws:s3:::<bucket>. No globs
+  # (arn:aws:s3:::* would reintroduce the over-broad grant this feature removes)
+  # and no trailing /* (main.tf appends /* itself -> would yield <bucket>/*/*).
+  validation {
+    condition = alltrue(flatten([
+      for k, v in var.byo_s3_irsa_roles : [
+        for arn in v.bucket_arns : can(regex("^arn:aws:s3:::[a-z0-9][a-z0-9.-]{1,61}[a-z0-9]$", arn))
+      ]
+    ]))
+    error_message = "byo_s3_irsa_roles[*].bucket_arns entries must be an exact bucket ARN 'arn:aws:s3:::<bucket>' (no wildcards, no trailing /*, no object key)."
+  }
+  # Each entry must be "<namespace>:<sa-name>" (the format the upstream IRSA
+  # module expands into system:serviceaccount:<ns>:<sa>). A malformed subject
+  # silently produces a trust condition no pod can satisfy.
+  validation {
+    condition = alltrue(flatten([
+      for k, v in var.byo_s3_irsa_roles : [
+        for s in v.namespace_service_accounts : can(regex("^[a-z0-9][a-z0-9.-]*:[a-z0-9][a-z0-9.-]*$", s))
+      ]
+    ]))
+    error_message = "byo_s3_irsa_roles[*].namespace_service_accounts entries must be '<namespace>:<sa-name>' (both non-empty, lowercase DNS-safe, exactly one colon)."
+  }
+  validation {
+    condition = alltrue([
+      for k, v in var.byo_s3_irsa_roles :
+      v.role_name_override == null ? true : can(regex("^[a-zA-Z0-9+=,.@_-]{1,64}$", v.role_name_override))
+    ])
+    error_message = "byo_s3_irsa_roles[*].role_name_override must match ^[a-zA-Z0-9+=,.@_-]{1,64}$."
+  }
+  # IAM customer-managed policy name: 1-128 chars from [a-zA-Z0-9+=,.@_-].
+  validation {
+    condition = alltrue([
+      for k, v in var.byo_s3_irsa_roles :
+      v.policy_name_override == null ? true : can(regex("^[a-zA-Z0-9+=,.@_-]{1,128}$", v.policy_name_override))
+    ])
+    error_message = "byo_s3_irsa_roles[*].policy_name_override must match ^[a-zA-Z0-9+=,.@_-]{1,128}$."
+  }
+}
+
 variable "enable_monitoring_setup" {
   description = "Enable monitoring namespace and Grafana credentials secret"
   type        = bool

variables.tf

@@ -77,6 +77,21 @@ variable "loki_iam_role_name_override" {
   }
 }
 
+# DND-1423: Bring-your-own-S3 IRSA roles (see modules/comet_eks/variables.tf for
+# the full schema). Empty by default => feature off. Set an entry per customer
+# BYO bucket that pods must reach via IRSA (e.g. ClickHouse remote backups).
+variable "byo_s3_irsa_roles" {
+  description = "Map of bring-your-own-S3 IRSA roles. Each entry grants listed Kubernetes ServiceAccounts (via IRSA) scoped access to a customer-supplied S3 bucket. Empty by default."
+  type = map(object({
+    bucket_arns                = list(string)
+    namespace_service_accounts = list(string)
+    actions                    = optional(list(string))
+    role_name_override         = optional(string)
+    policy_name_override       = optional(string)
+  }))
+  default = {}
+}
+
 variable "external_secrets_iam_role_name_override" {
   description = "Override the External Secrets IRSA role name. Null keeps the computed <environment>-external-secrets name."
   type        = string