Skip to content

fix(iac): let member-account roles tag purchased EC2 reservations - #773

Merged
cristim merged 4 commits into
mainfrom
fix/702-member-account-createtags
Oct 9, 2026
Merged

cristim merged 4 commits into
mainfrom
fix/702-member-account-createtags

Conversation

@cristim

@cristim cristim commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Summary

Part of #702, EC2 half only. The member-account purchase roles had no ec2:CreateTags. The Go client tags the reserved instance it just bought with the purchase idempotency tag; in a member account that call was denied, the failure is only logged, and the pre-buy tag guard then finds no tag and can buy again on a retry.

Added to all six federation templates (cross-account and target, Terraform and CloudFormation, plus aws-cross-account-cli.sh.tmpl and aws-wif-cli.sh.tmpl) and to the legacy cloudformation/stacks/CUDly-CrossAccount/template.yaml: ec2:CreateTags in its own statement, Resource: arn:aws:ec2:*:*:reserved-instances/*, no ec2:CreateAction condition (PurchaseReservedInstancesOffering takes no TagSpecifications, so a standalone CreateTags carries no CreateAction and the call would be denied with one). In the CLI templates, which had one flat Resource: "*" statement, this is a second statement so the grant is not widened to every EC2 resource.

Redshift is NOT part of this PR

An earlier version of this PR also granted redshift:CreateTags and redshift:DescribeTags on *. The coordinator's least-privilege review (Opus, at 40e38a6) found that grant unconditioned, and that Redshift may not support tagging reserved nodes at all (the AWS Service Authorization Reference lists no reservednode resource type), so it could add risk and fix nothing. Those actions are removed from all seven policy files. The Redshift part of #702 stays open until it is verified against a real account (a sandbox CreateTags/DescribeTags call against a reserved-node ARN, owner decision 21). #779 (host runtime role Redshift tags) depends on the same answer.

What is verified

Run locally (macOS), no applies and no cloud calls:

  • scripts/check-aws-iam-parity.sh passes; scripts/test-aws-iam-parity.sh 7/7 (two drift cases: dropping ec2:CreateTags from the target Terraform and from the target CloudFormation template).
  • TestMemberAccountPoliciesTagReservedInstancesOnly reads all seven policy files (both CLI heredocs, the four federation templates, the legacy stack) and asserts ec2:CreateTags appears once, scoped to reserved-instances/*, and redshift:CreateTags is absent. Mutants, each run: widening ec2:CreateTags to * in a Terraform file, in a CloudFormation file and in the legacy stack all fail it; reintroducing redshift:CreateTags fails it. go test ./internal/iacfiles passes; go test ./internal/api -run Federation passes; terraform fmt -check clean.

Known limits

  • ec2:CreateTags is scoped by resource type only; it can still retag reserved instances the customer did not buy through CUDly. A tag-key condition (aws:TagKeys plus aws:RequestTag/Tool) was left out by decision of the coordinator review.
  • The Terraform files are checked by a brace-balanced text scan, not an HCL parser; the CloudFormation files by a YAML parse.
  • Nothing was exercised against a real AWS account: that the member role can now write the tag is source and reference level only.
  • Existing member accounts keep their deployed role until the owner re-applies the template (or re-runs the CLI onboarding). This PR applies nothing.
  • Not in this PR: surfacing a tag-write failure on the execution record (the swallow is in cloud-commitments-go ec2/client.go:203-205, plus platform handling); OpenSearch AddTags is granted by no template (INF-10: es:AddTags granted by no IaC template although code performs post-purchase OpenSearch RI tagging #42).
  • No -race, no full-repo test run locally; CI does the full run.

Review history

deployment-investigation-kimi: design approved at d09ff5a, lint blocker QF1002 fixed at 40e38a6, delta approved. Coordinator Opus least-privilege review at 40e38a6: request changes (remove the Redshift half, extend the scoping test to all seven files), done at 9c315d2 as a new commit.

Refs #702

- Outside the host account the purchase idempotency tag was never
  written: the member-account purchase roles had no ec2:CreateTags and
  no Redshift tag actions, the failure is only logged, and the pre-buy
  tag guard then finds no tag and can buy again on a retry.
- Add ec2:CreateTags scoped to arn:aws:ec2:*:*:reserved-instances/* as
  its own statement, and redshift:CreateTags and redshift:DescribeTags
  on "*" (no reserved-node resource type exists for those actions), to
  all six federation templates (cross-account and target, Terraform and
  CloudFormation, plus both CLI onboarding scripts) and to the legacy
  CUDly-CrossAccount stack. The CLI heredocs get a second statement so
  ec2:CreateTags is not widened to every EC2 resource.
- Tests: two parity-script drift cases (dropping ec2:CreateTags from
  target Terraform, dropping redshift:DescribeTags from the WIF CLI
  template) and a Go test that pins the scoping in both CLI templates;
  the scoping test fails if ec2:CreateTags is added to the flat list.
- Existing member accounts keep their deployed role until the template
  is re-applied.

Refs #702
@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 9 billable files and costs up to $2.25.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 20 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 86 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 229bf089-a834-424f-a346-921f15d577ad

📥 Commits

Reviewing files that changed from the base of the PR and between 225ad35 and c09d4ad.


📒 Files selected for processing (9)
  • cloudformation/stacks/CUDly-CrossAccount/template.yaml
  • iac/federation/aws-cross-account/cloudformation/template.yaml
  • iac/federation/aws-cross-account/terraform/main.tf
  • iac/federation/aws-target/cloudformation/template.yaml
  • iac/federation/aws-target/terraform/main.tf
  • internal/iacfiles/templates/aws-cross-account-cli.sh.tmpl
  • internal/iacfiles/templates/aws-wif-cli.sh.tmpl
  • internal/iacfiles/templates_aws_tagging_test.go
  • scripts/test-aws-iam-parity.sh


  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@cristim cristim added urgency/this-sprint Within the current sprint priority/p1 Next up; this sprint severity/high Significant harm impact/many Affects most users effort/m Days type/bug Defect triaged Item has been triaged labels Oct 9, 2026
@cristim

cristim commented Oct 9, 2026

Copy link
Copy Markdown
Member Author

Subject: PR 773 (#702 IaC half) @ d09ff5a - REQUEST CHANGES (1 CI blocker), design verified

BLOCKER L1: CI "Lint Code" FAILED at this SHA: internal/iacfiles/templates_aws_tagging_test.go:50 QF1002 "could use tagged switch on action" (staticcheck). Your new test file. Fix and push; I re-review the delta. (Also mergeStateStatus UNKNOWN, other checks still running when I looked.)

Verified at the SHA (scratch copy of the PR tree via git archive; no worktree, no cloud calls):
(1) Go client needs: providers/aws@9962786e0695 ec2 client.go:297 CreateTags; redshift client.go:289 DescribeTags, :386 CreateTags (ARN reservednode). Also OpenSearch client.go:362 calls opensearch AddTags post-purchase; no template grants it. Not new/not a guard (OpenSearch dedupes by reservation ID, call is best-effort per its own comment), so out of scope, FYI only.
(2) AWS Service Authorization Reference fetched live (servicereference.us-east-1.amazonaws.com): ec2:CreateTags lists resource type reserved-instances (cond keys ec2:CreateAction, ec2:Region); redshift:CreateTags/DescribeTags list cluster, eventsubscription, hsm*, integration, parametergroup, securitygroup*, snapshot*, subnetgroup, usagelimit and NO reservednode, so "" is the narrowest scope that can match. Correct.
(3) Variants: all six federation files + legacy CUDly-CrossAccount parsed (YAML structure walk / HCL / JSON): identical grants everywhere: ec2:CreateTags only on arn:aws:ec2:
::reserved-instances/ (own statement), redshift:CreateTags+DescribeTags on "" in the existing Redshift statement. No tag: or broad tagging action anywhere. Host runtime roles (CUDly stack, fargate, lambda) already have the scoped ec2:CreateTags, no Redshift tag actions: matches the PR's stated out-of-scope. Repo-wide grep of PurchaseOffering found no other member-account policy flavor.
(4) Both CLI heredocs extracted and json-parsed: valid; 2 statements ("
" with 34 actions, then the reserved-instances ARN with ec2:CreateTags). terraform fmt -check clean; terraform validate OK on both dirs; cfn-lint: only pre-existing E0000 on the target template (and a pre-existing W3005 on cross-account).
Runs: check-aws-iam-parity OK; test-aws-iam-parity 7/7; go test ./internal/iacfiles 182 passed; go test ./internal/api -run Federation 83 passed.
(5) Mutations: add ec2:CreateTags to the flat CLI list -> Go test FAILS; widen the CLI second statement to "" -> Go test FAILS; drop redshift:CreateTags from cross-account CFN -> parity FAILS (rc 1). The new Go test against main's CLI templates FAILS ("redshift:CreateTags and redshift:DescribeTags must each appear once"), so it is not vacuous.
(6) Privilege: ec2 grant is reserved-instances-only. redshift:CreateTags on "
" can add/overwrite tags on any Redshift resource in the account (clusters, snapshots...) and, because the reference has no narrower option, cannot be scoped by ARN; ABAC setups that use Redshift tags for access control could be influenced. Acceptable and documented, but note it in the PR body; could add an aws:RequestTag/aws:TagKeys condition (the action supports those keys) as a follow-up if you want it narrower.
Non-blocking gaps found by mutation (not caught by any test):
G1 widening the ec2:CreateTags Resource to "*" in the four non-CLI federation files (target TF/CFN, cross-account TF/CFN) passes parity and the Go test (they only guard the CLI templates; the extractor ignores Resource).
G2 removing the ec2/redshift tag actions from cloudformation/stacks/CUDly-CrossAccount passes everything (not in the parity set), consistent with the plan's "verify by grep".
Not required for this PR; say so in the body or add a small Resource assertion for the four files.
Plan review (asked by watchdog): the plan file (plans/702-member-account-createtags.md, "Review passes") records "Pass 1 (independent adversarial reviewer, sonnet, read-only)" with 9 findings that the PR then reflects (reservednode scope dropped, CLI second statement, Go test, host role cut), and "Pass 2: closed on agreement". I found NO bus message or PR comment with that verdict, and the reviewer is not named, so I cannot confirm who/what it was or independently that it was not the writer's own subagent. The substance was real: the findings it records match the SAR facts I just verified. If the gate needs the plan review attributable, the writer should post it or I can record that I verified its findings (this review does).
Gate note: Sonnet review per origin/main (PR #658). Verdict: APPROVE design, REQUEST CHANGES for L1; I will re-review the delta once pushed and CI is green.

- Lint Code reported QF1002 on the untagged switch in
  TestAWSCLITemplatesTagReservedInstancesOnly; switch on the action
  string instead. No behavior change.
@cristim

cristim commented Oct 9, 2026

Copy link
Copy Markdown
Member Author

Subject: PR 773 delta d09ff5a..40e38a6 - L1 CLOSED, APPROVE conditional on green CI

Delta is exactly one file (internal/iacfiles/templates_aws_tagging_test.go, +3/-3): switch { case action == ... } -> tagged switch action { case "ec2:CreateTags": ... case "redshift:CreateTags", "redshift:DescribeTags": ... }. Semantically identical (same asserts, same counters). Checked: go test ./internal/iacfiles ok at the head; both mutations still FAIL the test (add ec2:CreateTags to the flat WIF CLI list; widen the cross-account CLI second statement to "*"). CI "Lint Code" is no longer among the non-green checks (QF1002 cleared in CI, not just locally).
Your notes accepted: G1/G2 and the Redshift privilege stated under "Known limits"; plan-review attribution (writer-spawned read-only sonnet subagent) is honest in the PR body and my verification against the AWS reference is the independent check; no separate plan review needed from my side.
Conditional: other checks (Unit, Integration, Docker, Security, E2E, parity, terraform validate) were still queued/running at 40e38a6 (BLOCKED). Approval requires every check green and CLEAN at this exact SHA; merge only on codex-watchdog's OK. Any new commit restarts this.

…policies

- Coordinator least-privilege review of PR 773: redshift:CreateTags on
  "*" has no condition and Redshift may not support tagging reserved
  nodes at all, so it could add risk and fix nothing. Remove
  redshift:CreateTags and redshift:DescribeTags from the six member
  account policies and the legacy CUDly-CrossAccount stack. The
  Redshift part of #702 stays open until it is verified against a real
  account.
- The EC2 grant is unchanged: ec2:CreateTags on
  arn:aws:ec2:*:*:reserved-instances/*, no CreateAction condition.
- The scoping test now reads all seven policy files (both CLI
  heredocs, the four federation templates and the legacy stack) and
  asserts ec2:CreateTags appears once, scoped to reserved instances,
  and that redshift:CreateTags is absent. It fails when ec2:CreateTags
  is widened to "*" in a Terraform file, a CloudFormation file or the
  legacy stack, and when redshift:CreateTags is reintroduced.
- Parity drift case 7 now drops ec2:CreateTags from the target
  CloudFormation template.

Refs #702
@cristim cristim changed the title fix(iac): let member-account roles tag purchased reservations fix(iac): let member-account roles tag purchased EC2 reservations Oct 9, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim
cristim merged commit 7e2a3d8 into main Oct 9, 2026
25 checks passed
@cristim

cristim commented Oct 9, 2026

Copy link
Copy Markdown
Member Author

Merge note: squash-merged at 7e2a3d8, matched head c09d4ad. Review: Opus least-privilege review APPROVE at 9c315d2; delta 9c315d2..c09d4ad is a test-only prealloc lint fix, checked by the coordinator. CI: 25/25 pass, state CLEAN, no unresolved defect threads. Local verification on the archived head (real test runs, not mocks of the IAM files): go test ./internal/iacfiles/... exit 0 (187 passed); scripts/test-aws-iam-parity.sh exit 0 (7 passed, 0 failed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/many Affects most users priority/p1 Next up; this sprint severity/high Significant harm triaged Item has been triaged type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant