Skip to content

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

Merged
jms200 merged 2 commits into
mainfrom
jms200/DND-1423/byo-s3-irsa
Jul 22, 2026
Merged

jms200 merged 2 commits into
mainfrom
jms200/DND-1423/byo-s3-irsa

Conversation

@jms200

@jms200 jms200 commented Jul 22, 2026 •

Copy link
Copy Markdown
Collaborator

User description

What

Adds a reusable, opt-in byo_s3_irsa_roles map to the module. Each entry provisions the IAM that accompanies a customer-supplied S3 bucket:

  • one IRSA role (assumable via web-identity by the listed Kubernetes ServiceAccounts),
  • a customer-managed policy scoped to the supplied bucket ARN(s),
  • the attachment.

It follows the existing Loki IRSA triplet exactly (data.aws_iam_policy_document → aws_iam_policy → module ... iam-role-for-service-accounts-eks ~> 5.39), so the OIDC :sub/:aud trust is built by the upstream module from namespace_service_accounts.

byo_s3_irsa_roles = {
  s3access = {
    bucket_arns                = ["arn:aws:s3:::my-bucket"]   # perms scoped to these (+ /*)
    namespace_service_accounts = ["ns:opik-clickhouse", ...]  # drives the IRSA :sub trust
    actions                    = null   # optional; defaults to the CH-backup S3 action set
    role_name_override         = null   # optional; adopt an out-of-band role in place
    policy_name_override       = null   # optional; adopt an out-of-band policy in place
  }
}

Why

Generalizes the BYO-S3 access role that was previously created out-of-band during onboarding (e.g. Zoox's ZooxS3Access). Because that role's trusted-SA list lived only in live AWS and not in code, a missing ServiceAccount went unreviewed and silently broke ClickHouse remote backups for ≥7 days (DND-1413). Codifying it makes the trust list reviewable in PRs, and any future BYO-S3 customer gets the same primitive from day one instead of a hand-rolled role.

Design notes

  • Least-privilege by default — permissions are scoped to the supplied bucket ARN(s), not arn:aws:s3:::*.
  • Adoptable in place — role_name_override / policy_name_override let an existing out-of-band role/policy be adopted via terraform import without recreation (stable ARN → IRSA annotations keep working).
  • Zero blast radius — empty map default; no effect on any existing consumer.
  • SA list is an input, not hardcoded — grounded in live AWS: Zoox trusts 4 SAs, circuit trusts a different 4; the set genuinely varies by install type.

Testing

  • terraform init -backend=false + terraform validate → "the configuration is valid" (only pre-existing deprecation warnings, unrelated to this change).
  • terraform fmt clean.

Release

New MINOR (new feature family) — tag v1.21.0 after merge.

Follow-up (separate PRs, not here)

  • comet-devops Zoox wrapper: bump ?ref → v1.21.0 and adopt ZooxS3Access onto this feature via byo_s3_irsa_roles + import blocks (normalize-on-adopt: trims the out-of-band node-role / s3.amazonaws.com / ad-hoc-user trust statements and scopes perms to the bucket).

Ref: DND-1423 (follow-up to DND-1413).

🤖 Generated with Claude Code


Generated description

Below is a concise technical summary of the changes proposed in this PR:
Add a new byo_s3_irsa_roles input to the root comet_eks module and its modules/comet_eks implementation to provision per-bucket IRSA roles, customer-managed S3 policies, and policy attachments for customer-supplied buckets. Generalize the existing iam-role-for-service-accounts-eks flow so namespace_service_accounts drive OIDC trust while keeping ClickHouse backup access scoped and opt-in.

TopicDetails
BYO S3 access Provision per-bucket IRSA roles and scoped S3 policies for customer-owned buckets via aws_iam_policy_document, aws_iam_policy, and iam-role-for-service-accounts-eks.
Modified files (3)
  • main.tf
  • modules/comet_eks/main.tf
  • variables.tf
Latest Contributors(2)
UserCommitDate
jms200feat(comet_eks): add b...July 22, 2026
alexb@comet.comfeat(providers): upgra...July 20, 2026
Input validation Validate bucket ARNs, ServiceAccount subjects, and optional name overrides so BYO-S3 entries stay safe and adoptable in place.
Modified files (1)
  • modules/comet_eks/variables.tf
Latest Contributors(2)
UserCommitDate
jms200fix(comet_eks): valida...July 22, 2026
alexb@comet.comfeat(providers): upgra...July 20, 2026
Review this PR on Baz | Customize your next review

… (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>
Comment thread modules/comet_eks/variables.tf
Comment thread modules/comet_eks/variables.tf
…423 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>
@jms200
jms200 merged commit 9b0382f into main Jul 22, 2026
5 checks passed
@jms200
jms200 deleted the jms200/DND-1423/byo-s3-irsa branch July 22, 2026 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant