Skip to content

Enumerate all local CUR buckets in the auto-generated payer policy#103

Merged
khill2018 merged 7 commits into
developfrom
claude/enumerate-cur-buckets-gnd3X
May 12, 2026
Merged

Enumerate all local CUR buckets in the auto-generated payer policy#103
khill2018 merged 7 commits into
developfrom
claude/enumerate-cur-buckets-gnd3X

Conversation

@silvexis

@silvexis silvexis commented May 8, 2026

Copy link
Copy Markdown
Member

Summary

  • Discovery Lambda now emits MasterPayerBillingBucketArns — a comma-separated ARN list (both bucket and bucket/* forms) covering every locally-owned CUR bucket it found, not just the one chosen as primary.
  • master_payer.yaml uses Fn::Split on that string directly as the CZTier0BillingBucket20260420 Resource: list, so a payer account with multiple CURs gets every CUR bucket granted to the CloudZero role.
  • The create-new-CUR path is unchanged in shape (discovery is empty there by construction). MasterPayerBillingBucketName / Path keep their primary-bucket semantics for the notification flow.

Files touched

  • services/discovery/src/app.py — new get_all_local_cur_bucket_names + format_bucket_arns; new output key in DEFAULT_OUTPUT and OUTPUT_SCHEMA.
  • services/discovery/template.yaml — new output MasterPayerBillingBucketArns.
  • services/connected_account.yaml, services/connected_account_dev.yaml — pass the new output as a parameter to the master-payer nested stack.
  • services/account_type/master_payer.yaml — new MasterPayerBillingBucketArns parameter; CZTier0BillingBucket20260420 Resource: becomes !Split over the ARN list (or the single new-bucket pair when creating a CUR).
  • services/discovery/tests/unit/test_app.py — updated full-output assertions + new multi-CUR enumeration test.

Test plan

  • python -m pytest services/discovery/tests/unit/test_app.py — 13 passed (12 existing + 1 new).
  • cfn-lint on all four touched templates — only the pre-existing IsOrganizationMasterAccount not used warning.
  • Stack deploy in a payer account with one CUR — policy rendering identical to today.
  • Stack deploy in a payer account with two CURs — policy includes both bucket-pair ARNs.
  • Stack deploy in a payer account with no existing CUR — policy contains only the freshly created cz-cur-hourly-csv-${StackId} bucket pair.

https://claude.ai/code/session_01KRtx6n6mePz1Bi5N8Me1FM


Generated by Claude Code

claude and others added 3 commits May 8, 2026 13:13
So a customer with multiple CURs in their payer account gets every CUR
bucket granted to the CloudZero role, not just the one the discovery
Lambda happened to pick as primary.

Discovery now emits MasterPayerBillingBucketArns - a comma-separated ARN
list (both bucket and bucket/* forms) covering every locally-owned CUR
bucket. The master-payer template uses Fn::Split on it directly as the
CZTier0BillingBucket20260420 statement's Resource list. The create-new-CUR
path keeps emitting the new bucket pair (discovery is empty there by
construction). MasterPayerBillingBucketName/Path keep their primary-bucket
semantics for the notification flow.

https://claude.ai/code/session_01KRtx6n6mePz1Bi5N8Me1FM
Note in the top-level README that the auto-generated role grants
s3:Get*/s3:List* on every CUR bucket discovered in the account, and add
a section to policies/README.md explaining that the canonical single
{{BillingBucketName}} ARN pair is expanded at deploy time.

https://claude.ai/code/session_01KRtx6n6mePz1Bi5N8Me1FM
Covers the mixed-local/remote case: prior tests covered all-local and
all-remote, but not a payer with both. Verifies get_all_local_cur_bucket_names
keeps the two local CUR buckets and drops the one whose report references a
non-local (remote-account-owned) bucket.

Commit Generated with AI

Co-Authored-By: AI
@khill2018 khill2018 marked this pull request as ready for review May 11, 2026 15:07
@khill2018 khill2018 requested a review from a team as a code owner May 11, 2026 15:07
@khill2018 khill2018 changed the base branch from master to develop May 11, 2026 15:07
@silvexis silvexis requested a review from khill2018 May 11, 2026 15:08
Comment thread services/account_type/master_payer.yaml
Comment thread services/account_type/master_payer.yaml
Comment thread services/discovery/src/app.py
@Cloudzero Cloudzero deleted a comment from greptile-apps Bot May 12, 2026
@khill2018

Copy link
Copy Markdown
Contributor

@greptile

@qiuz-cz qiuz-cz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Cloudzero Cloudzero deleted a comment from greptile-apps Bot May 12, 2026
@khill2018

Copy link
Copy Markdown
Contributor

@greptile

@greptile-apps

greptile-apps Bot commented May 12, 2026

Copy link
Copy Markdown

Greptile Summary

This PR extends the Discovery Lambda to enumerate all locally-owned CUR buckets (not just the schema-valid primary) and threads that comma-separated ARN list through to the master-payer IAM policy, so a payer account with multiple CUR reports gets s3:Get*/s3:List* on every CUR bucket without a stack redeployment when the primary CUR changes.

  • get_all_local_cur_bucket_names + format_bucket_arns — schema-agnostic enumeration in app.py produces a comma-joined ARN pair list (bucket and bucket/*) for every locally-owned CUR bucket; the choice to skip keep_valid filtering is explicitly documented and deliberate.
  • HasMasterPayerBillingBucketArns guard in master_payer.yaml — the !Split over the ARN list is protected by a condition that falls back to the single-bucket !Sub pair when the parameter is empty, preventing the [""] invalid-IAM-resource failure that was flagged in the prior review round.
  • Test coverage — two new integration-style unit tests cover the multi-CUR enumeration path and the remote-bucket exclusion path; all existing assertions were updated with the new output key.

Confidence Score: 5/5

The change is safe to merge — all three issues raised in the previous review round have been addressed, and the new code paths are covered by targeted unit tests.

The HasMasterPayerBillingBucketArns guard correctly prevents !Split from receiving an empty string. coeffects_buckets is a pure dict read, so calling it in both get_cur_bucket_if_local and get_all_local_cur_bucket_names adds no extra API calls. Bucket names cannot contain commas, making the comma-delimited ARN format safe. The schema-agnostic enumeration intent is clearly documented. No new defects found.

No files require special attention.

Important Files Changed

Filename Overview
services/discovery/src/app.py Adds get_all_local_cur_bucket_names (schema-agnostic bucket enumeration) and format_bucket_arns; emits MasterPayerBillingBucketArns from discover_master_payer_account. Logic is correct; coeffects_buckets is a pure dict read so the apparent double-call with get_cur_bucket_if_local incurs no extra API round-trips.
services/account_type/master_payer.yaml Adds MasterPayerBillingBucketArns parameter and HasMasterPayerBillingBucketArns condition; rewrites CZTier0BillingBucket20260420 Resource: to use !Split over the ARN list with a safe fallback when the parameter is empty, addressing the empty-string !Split concern from the prior review.
services/discovery/tests/unit/test_app.py Updates all full-output assertions to include MasterPayerBillingBucketArns; adds two new tests covering multi-CUR enumeration and remote-bucket exclusion. Coverage looks complete for the new code paths.
services/connected_account.yaml Passes the new MasterPayerBillingBucketArns output from the Discovery stack into the master-payer nested stack; one-line wiring change, correct.
services/connected_account_dev.yaml Same one-line wiring change as connected_account.yaml for the dev variant; correct and consistent.
services/discovery/template.yaml Exposes the new MasterPayerBillingBucketArns output from the Lambda custom resource; straightforward addition.
docs/releases/1.0.95.md New release notes accurately documenting multi-CUR enumeration, the HasMasterPayerBillingBucketArns guard, and AWS CUR-per-account limits.
README.md Adds a brief note explaining multi-CUR bucket policy behaviour to the payer account section.
policies/README.md Documents the runtime expansion of CZTier0BillingBucket20260420 Resource list at deploy time.

Reviews (3): Last reviewed commit: "codeql not working, github outage need t..." | Re-trigger Greptile

@khill2018 khill2018 merged commit aa371c3 into develop May 12, 2026
4 checks passed
@khill2018 khill2018 deleted the claude/enumerate-cur-buckets-gnd3X branch May 12, 2026 16:56
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.

4 participants