feat(helm): per-role ServiceAccounts — least-privilege IAM seam (IRSA) - #556
Conversation
…IRSA) Each role opts into its own ServiceAccount (falling back to the shared one), so with IRSA the three roles carry three IAM identities: querier read-only, receiver Get/List/Put with no delete, compactor the sole s3:DeleteObject holder. Verified against the code: production deletes exist only in compaction, plus the querier's best-effort RFC 0033 cache write-through — documented with a scoped-key policy alternative since a publish failure never fails a query by contract. The IRSA/static-keys mutual-exclusion render check generalizes over the shared + per-role accounts, and a role-arn on a non-created role SA fails render with the same guidance as the shared case. README gains the Per-role IAM section: the permission table, the IRSA install example, the IAM policy shape, S3-versioning as the independent second control on deletes, and the SPIRE/cert-reload rotation seam. Closes #555. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe Helm chart adds optional role-specific ServiceAccounts for receiver, querier, and compactor workloads, expands IRSA credential validation, wires workloads to role-specific accounts, documents IAM policies, and bumps the chart version. ChangesPer-role IAM support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HelmValues
participant HelmHelpers
participant ServiceAccountTemplate
participant WorkloadTemplates
HelmValues->>HelmHelpers: provide per-role ServiceAccount settings
HelmHelpers->>ServiceAccountTemplate: resolve names and validate IRSA configuration
ServiceAccountTemplate->>WorkloadTemplates: expose role-specific ServiceAccounts
WorkloadTemplates->>WorkloadTemplates: assign receiver, querier, and compactor accounts
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds Helm chart support and documentation for least-privilege, per-role ServiceAccounts (IRSA seam), plus generalized render-time validation around IRSA vs static S3 keys.
Changes:
- Introduces optional per-role ServiceAccount values for receiver/querier/compactor and updates workloads to use role-scoped SA names with fallback to the shared SA.
- Extends Helm render-time validation to cover both shared and per-role IRSA role-arn annotations (and maintains mutual exclusion with static S3 secrets).
- Documents the per-role IAM model and bumps chart version to 0.4.0.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| deploy/helm/ourios/values.yaml | Adds per-role ServiceAccount value blocks and clarifies shared SA comments for IRSA usage. |
| deploy/helm/ourios/templates/serviceaccount.yaml | Renders optional role-scoped ServiceAccounts when enabled per role. |
| deploy/helm/ourios/templates/receiver-statefulset.yaml | Switches receiver pods to use role-aware ServiceAccount name helper. |
| deploy/helm/ourios/templates/querier-deployment.yaml | Switches querier pods to use role-aware ServiceAccount name helper. |
| deploy/helm/ourios/templates/compactor-deployment.yaml | Switches compactor pods to use role-aware ServiceAccount name helper. |
| deploy/helm/ourios/templates/_helpers.tpl | Adds role SA name helper and expands IRSA/static-keys validation across per-role SAs. |
| deploy/helm/ourios/README.md | Documents per-role IAM split and updates values table accordingly. |
| deploy/helm/ourios/Chart.yaml | Bumps Helm chart version to 0.4.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… shared-SA opt-out example Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/helm/ourios/templates/_helpers.tpl (1)
70-89: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGenerate unique names for role-specific ServiceAccounts.
When a role sets
serviceAccount.create=truewithoutserviceAccount.name, falling back toourios.serviceAccountNamegives every role the same name. The per-role rendering loop then emits multiple ServiceAccounts with an identical name, and workloads cannot receive distinct IRSA identities. Use a generated role-scoped default such as<fullname>-receiver, while retaining the shared fallback only when per-role creation is disabled.Suggested resolution
-# create=true with no custom name falls back to the shared ServiceAccount name +# create=true with no custom name uses a role-scoped generated name +printf "%s-%s" (include "ourios.fullname" $root) $role🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/ourios/templates/_helpers.tpl` around lines 70 - 89, The roleServiceAccountName helper must generate a unique role-scoped name when the role’s serviceAccount.create is true but no name is provided. Update that branch to use the fullname plus role suffix default, while preserving the explicitly configured name and shared ourios.serviceAccountName fallback when role-specific creation is disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@deploy/helm/ourios/templates/_helpers.tpl`:
- Around line 70-89: The roleServiceAccountName helper must generate a unique
role-scoped name when the role’s serviceAccount.create is true but no name is
provided. Update that branch to use the fullname plus role suffix default, while
preserving the explicitly configured name and shared ourios.serviceAccountName
fallback when role-specific creation is disabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c17cce3c-a1cb-4b57-975d-0670fe2cbea3
📒 Files selected for processing (4)
deploy/helm/ourios/README.mddeploy/helm/ourios/templates/_helpers.tpldeploy/helm/ourios/templates/serviceaccount.yamldeploy/helm/ourios/values.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- deploy/helm/ourios/templates/serviceaccount.yaml
- deploy/helm/ourios/values.yaml
- deploy/helm/ourios/README.md
…cket Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
deploy/helm/ourios/values.yaml:113
- The receiver
serviceAccountcomment sayscreate=true + empty name renders <release>-ourios-receiver, but the actual rendered name is<ourios.fullname>-receiver. When the Helm release name already contains the chart name (common for a release namedourios/ourios-prod),ourios.fullnamebecomes the release name and the generated ServiceAccount is<release>-receiver, not<release>-ourios-receiver. Updating the comment avoids misconfiguring IRSA trust-policy subjects.
# that role needs Get/List/Put but NOT s3:DeleteObject. `name`:
# create=true + empty name renders `<release>-ourios-receiver`;
# create=true + name overrides that; create=false + name binds an
# EXISTING ServiceAccount of that name (managed out-of-band).
…e>-<role> Truncate the base before joining the role suffix so a maximal fullname can never collapse the three role ServiceAccounts into one name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
…compaction Verified in code: the ingester's production store calls are put_blocking alone (the one list_blocking is in a test); Manifest publish-CAS lives in the compaction path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
…ation Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
What
Closes #555 — the least-privilege object-store story, as chart support + documentation:
receiver/querier/compactor.serviceAccount.{create,annotations,name}, default off → shared SA, fully backwards-compatible). With IRSA each role carries its own IAM identity;name-only binds an existing SA. Chart version → 0.4.0.DeleteObjectholder), IRSA install example, IAM policy shape with prefix scoping, S3-versioning as the independent second control on deletes, non-AWS equivalents, and the SPIRE/cert-reload rotation seam (RFC 0030) with an honest note that chart-level TLS values aren't exposed yet.Verified
compaction.rs; the querier's RFC 0033 cache write-through (and v1-key cleanup) is best-effort by contract — documented explicitly, with a scoped-keyPutObject/DeleteObjectalternative (audit/tenant_id=*/template_map*) for operators who want the cache warm under a read-only querier.helm lintclean; render matrix exercised: default (shared SA, unchanged output), full per-role IRSA split (3 SAs with arns, each workload binding its own), name-only fallback, and both failure modes (arn-without-create per role; secret + any arn) firing with actionable messages.Invariants / hazards
No data-path change. The compactor-is-the-only-sweeper invariant is untouched; the delete split enforces hazard-#4's ownership boundary at the IAM layer rather than only by convention.
🤖 Generated with Claude Code
https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
Summary by CodeRabbit