fix(iac/aws): grant six required runtime CE and EC2 actions - #2077
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour. 📝 WalkthroughWalkthroughThe runtime IAM definitions now grant Cost Explorer usage access and EC2 Reserved Instance operations. A Go coverage test derives CE and EC2 actions from SDK calls and verifies grants across Lambda, Fargate, and CloudFormation definitions. ChangesRuntime IAM grants and coverage
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant RuntimeSources
participant CoverageTest
participant IAMDefinitions
RuntimeSources->>CoverageTest: SDK CE/EC2 input literals
CoverageTest->>CoverageTest: Derive IAM actions
CoverageTest->>IAMDefinitions: Read runtime IAM actions
IAMDefinitions-->>CoverageTest: Declared grants
CoverageTest-->>CoverageTest: Report missing grants
Merge Risk: ⚪ Minimal · up to The required runtime IAM grants and current source-based coverage are in place. No concrete merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
cristim
added
type/bug
|
Merge-ready, parked for a human decision. CI is green at 27 checks and an independent adversarial review returned four low findings, three of which are fixed in the third commit while the fourth is recorded in the PR body as a stated limit of the guard. It is parked because this PR widens an IAM policy, adding nine actions to the runtime role across three infrastructure flavors. Automatic merging in this run deliberately excludes anything that widens a policy, trust relationship, workload identity binding or RBAC role, regardless of how green the checks are, because a permission grant is not cleanly revertible in the way a code change is: once applied, the role holds it until someone notices. Nothing here looks wrong. Every wildcard resource was checked against the AWS Service Reference and none can be scoped further; The judgment worth a human is whether granting these nine to the runtime role is the right answer at all, versus narrowing what the code calls. That is a product decision about how much authority the deployment should carry, not a correctness question, and it is exactly the kind this run should not make on its own. CodeRabbit has not reviewed this commit: its review quota is exhausted and the next included review is roughly 36 minutes out. That does not block the merge bar on its own, but it is worth knowing when you look. |
|
Native macOS preflight passed at unchanged HEAD 2484ed2: GOTOOLCHAIN=go1.26.6 go test -count=1 -v ./terraform/... (20 top-level tests across two packages), scripts/check-aws-iam-parity.sh, and scripts/test-aws-iam-parity.sh (5/5). Existing shared Go build/module caches were reused and the worktree stayed clean. This verifies static source/IaC consistency only, not deployed IAM or AWS runtime behavior; no cloud operations were performed. The mandatory final-HEAD Claude Fable 5.1 review is not satisfied and is currently blocked by provider quota. No merge performed. |
|
Recovery review found a confirmed blocker in the proposed AWS's OpenSearch Reserved Instances guide explicitly states that resource tagging is unsupported for OpenSearch Reserved Instances. This is not merely missing documentation of an ARN type. The committed The existing issue LeanerCloud/cloud-commitments-platform#42 already tracks this call and missing grant; its suggested permission expansion needs this qualification. Closed #250's suggestion to monitor possible support does not establish current support. No replacement tagging API is assumed to support reservations. PR #2077 will not merge unchanged. Recovery planning will separate the real missing runtime permissions from this unsupported call and define the smallest correction with regression proof. No runtime policy, cloud resource, or purchase was changed during this investigation. This is exact-source and official-documentation evidence, not a live API test. The SNS finding also attached to LeanerCloud/cloud-commitments-platform#42 remains a separate unresolved item. |
|
Independent Astra plan review passed three clean rounds. Recovery is limited to the six CE/EC2 runtime grants required by the actual callers. The new OpenSearch and Redshift tag grants will be removed; OpenSearch is tracked in LeanerCloud/cloud-commitments-platform#42, and the separate Redshift resource-contract gap is now tracked in LeanerCloud/cloud-commitments-go#107. #641/#635 do not establish that tag support. No provider or idempotency redesign is included here. Final committed-HEAD local proof, fresh adversarial review, substantive CodeRabbit review and green CI are still required before merge. |
The Lambda module, the Fargate module and the CloudFormation stack all
grant the same action list, and all three miss actions the application
calls under the runtime role: the ladder baseline (ce:GetCostAndUsage),
the RI Marketplace sell path (ec2:{Create,Describe,Cancel}
ReservedInstancesListing[s]), EC2 SKU enrichment
(ec2:DescribeInstanceTypes) and post-purchase commitment tagging
(ec2:CreateTags scoped to reserved-instances/*, redshift:DescribeTags,
redshift:CreateTags, es:AddTags). The Redshift DescribeTags gap blocks
every Redshift purchase in an account that already holds a reserved
node, because the idempotency lookup refuses to buy on any error.
Closes #1967
Closes #1968
Co-Authored-By: claude-flow <ruv@ruv.net>
Claude-Session: https://claude.ai/code/session_01Fu9uWjxtDFx5HDKeMRt1jC
…by each runtime flavor check-aws-iam-parity.sh compares the templates with each other, so an action missing from all of them passes. Derive the called set from the aws-sdk-go-v2 *Input literals under providers/aws and internal and assert each runtime flavor grants it. Co-Authored-By: claude-flow <ruv@ruv.net> Claude-Session: https://claude.ai/code/session_01Fu9uWjxtDFx5HDKeMRt1jC
Adversarial review found the walker skipped pkg/, whose exchange package builds the RI quote and accept inputs and is reached at runtime from internal/api and internal/server. Both actions happen to be granted, so the blind spot was empty, but the header claimed to cover runtime call sites and did not. Adding pkg/ closes it: removing ec2:GetReservedInstancesExchangeQuote from one flavor now fails naming pkg/exchange/exchange.go:238. Also records two limits the code did not state. The guard is one-way, so a grant no code uses passes silently; that is #1322's subject. And the prefix map covers only the reservation-related services, matching the parity script's scope, so platform namespaces are out of reach and have gaps of their own on #1204. Both were true before; neither was written down, which is how a guard gets trusted for more than it does. The cmd/ exclusion note now says why it is safe rather than only that it is: cmd/server is a runtime entry point, and the exclusion holds only while it imports no SDK service package directly. Co-Authored-By: claude-flow <ruv@ruv.net> Claude-Session: https://claude.ai/code/session_01Fu9uWjxtDFx5HDKeMRt1jC
Keep the six runtime grants backed by existing CE and EC2 requests. Remove reserved-resource tag grants whose API contracts do not support the claimed fix, and narrow the source-action guard to its actual scope. OpenSearch tagging is tracked in #1204; Redshift support is tracked separately in #2092. Neither is repaired by widening runtime IAM.
2484ed2 to
899ab79
Compare
cristim
added
impact/few
cristim
changed the title
|
Rebased onto main@81f2fc3ac44ce47221682fa1c50dec26e4f24eed and narrowed the candidate to six supported CE/EC2 grants at 899ab79. Removed the three new reserved-resource tagging grants; OpenSearch is tracked in LeanerCloud/cloud-commitments-platform#42 and Redshift contract verification in LeanerCloud/cloud-commitments-go#107. Native macOS stdlib IAM tests, exact-main baseline failure, corrected final pass, seven mutation checks and parity checks pass. The source-action guard now states its IAM-evaluation limits. Fresh committed-HEAD Astra review and exact-HEAD CI are being checked separately. Historical throttled review is not approval; please review all four changed files from the current base through the entire final HEAD. @coderabbitai full review |
|
NO CONFIRMED FINDINGS Independent final committed-HEAD review:
I reviewed the complete added test and policy changes, independently traced the relevant committed source, and confirmed Evidence and adversarial checks:
The nine-to-six correction removes Limits and existing behavior:
|
|
Follow-up tracking from the independent review: existing marketplace compensating-cancellation failure is now LeanerCloud/cloud-commitments-platform#335. The cross-account ambient-credential limitation already has LeanerCloud/cloud-commitments-platform#261, and missing status polling/seller checks have LeanerCloud/cloud-commitments-platform#27. None is included or claimed fixed by this four-file IAM change. AWS Sanity reports a successful workflow but its credentialed build/run steps were skipped because the read-only role is not configured; that workflow is not evidence of live AWS acceptance. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="81f2fc3ac44ce47221682fa1c50dec26e4f24eed"
head="899ab790d40cf4bf0475edd7cd32ce2f21481186"
echo '== Commit relationship and changed files =='
git merge-base "$base" "$head"
git diff --name-status "$base" "$head"
git diff --stat "$base" "$head"
echo '== Complete final diff =='
git diff --no-ext-diff --unified=80 "$base" "$head" -- \
cloudformation/stacks/CUDly/template.yaml \
terraform/modules/compute/aws/lambda/main.tf \
terraform/modules/compute/aws/fargate/main.tf \
terraform/modules/compute/aws/runtime_iam_coverage_test.go
echo '== Guard test outline =='
ast-grep outline terraform/modules/compute/aws/runtime_iam_coverage_test.go --items all
echo '== Candidate SDK call sites and imports =='
rg -n -C 5 -P '(?i)(GetCostAndUsageInput|DescribeInstanceTypesInput|CreateReservedInstancesListingInput|DescribeReservedInstancesListingsInput|CancelReservedInstancesListingInput|CreateTagsInput|service/(costexplorer|ec2))' \
providers internal pkg -g '*.go' || true
echo '== IAM action occurrences in changed policy files =='
rg -n -C 4 -P '(ce:GetCostAndUsage|ec2:(DescribeInstanceTypes|CreateReservedInstancesListing|DescribeReservedInstancesListings|CancelReservedInstancesListing|CreateTags))' \
cloudformation/stacks/CUDly/template.yaml \
terraform/modules/compute/aws/lambda/main.tf \
terraform/modules/compute/aws/fargate/main.tf
echo '== Module and repository-root helpers referenced by guard =='
rg -n -C 4 -P '(func\s+(repositoryRoot|moduleRoot)|repositoryRoot\s*\(|moduleRoot\s*\()' \
terraform -g '*.go' || trueLength of output: 120419 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== repoRoot definitions and test package files =='
rg -n -C 5 -P '\brepoRoot\s*\(' -g '*.go' .
fd -e go . terraform/modules/compute/aws -x sh -c 'printf "\n-- %s --\n" "$1"; sed -n "1,24p" "$1"' sh {}
echo '== Scanner-equivalent action derivation and policy presence =='
python3 - <<'PY'
from pathlib import Path
import re
root = Path('.')
roots = [root/'providers/aws', root/'internal', root/'pkg']
svc_prefix = 'github.com/aws/aws-sdk-go-v2/service/'
prefixes = {'costexplorer':'ce', 'ec2':'ec2'}
derived = {}
for directory in roots:
for path in directory.rglob('*.go'):
if path.name.endswith('_test.go'):
continue
text = path.read_text()
aliases = {}
for match in re.finditer(r'(?m)^\s*(?:(?P<alias>[A-Za-z_]\w*)\s+)?(?P<quote>")(?P<import>github\.com/aws/aws-sdk-go-v2/service/[^"]+)"', text):
imp = match.group('import')
rest = imp[len(svc_prefix):]
if '/' in rest or rest not in prefixes:
continue
alias = match.group('alias') or rest
if alias not in ('_', '.'):
aliases[alias] = prefixes[rest]
for alias, prefix in aliases.items():
for match in re.finditer(r'&?\s*' + re.escape(alias) + r'\.([A-Za-z]\w*Input)\s*\{', text):
opname = match.group(1)[:-5]
line = text.count('\n', 0, match.start()) + 1
derived.setdefault(f'{prefix}:{opname}', f'{path.relative_to(root)}:{line}')
for action, site in sorted(derived.items()):
print(f'{action}\t{site}')
print(f'DERIVED_COUNT={len(derived)}')
print('== POLICY CHECK ==')
files = [
root/'terraform/modules/compute/aws/lambda/main.tf',
root/'terraform/modules/compute/aws/fargate/main.tf',
root/'cloudformation/stacks/CUDly/template.yaml',
]
for file in files:
content = re.sub(r'(?m)^[ \t]*#.*$', '', file.read_text())
grants = set(re.findall(r'(?<![A-Za-z])((?:ce|ec2):[A-Z][A-Za-z]+)', content))
missing = sorted(set(derived)-grants)
print(f'{file}: missing={missing}')
PY
echo '== Relevant complete source sections =='
sed -n '70,125p' providers/aws/recommendations/client.go
sed -n '95,205p' providers/aws/recommendations/ondemand_series.go
sed -n '250,300p' providers/aws/services/ec2/client.go
sed -n '1000,1130p' providers/aws/services/ec2/client.go
echo '== IAM roles and policy attachment resources =='
rg -n -C 8 -P 'resource\s+"aws_iam_role"\s+"(lambda|task)"|resource\s+"aws_iam_role_policy"\s+"ri_exchange"|LambdaExecutionRole|LambdaPolicy|AWS::Lambda::Function' \
terraform/modules/compute/aws/lambda/main.tf \
terraform/modules/compute/aws/fargate/main.tf \
cloudformation/stacks/CUDly/template.yamlLength of output: 40366
I reviewed the complete final diff from The six added actions match the direct CE and EC2 SDK request literals. All three IaC flavors grant every one of the 19 actions currently derived by the guard. The Terraform policies attach to the Lambda application role and Fargate task role. The CloudFormation policy attaches to the role used by
The source-action guard correctly reuses the package-level This result is a static source and IaC review. It does not verify deployed IAM evaluation or AWS Marketplace behavior. ✅ Action performedFull review finished. |
|
Merged PR #2077 as efd6e95 after native macOS verification, fresh-context independent Astra review, substantive clean full CodeRabbit review and all28 exact-HEAD check/status entries passing. Reviewed HEAD899ab790d and merge commit have the identical tree39f40be04a616331f686ec61250d0398fbd99c4a; parent is the reviewed base81f2fc3. Verified: six missing CE/EC2 grants across allthree runtime IaC flavors; native stdlib Go/race/parity tests; exact-main failure, corrected candidate pass and seven omission/comment mutations; source, Allow/role attachment and RI-only tag scope inspected. Final evidence is linked in the PR body. No checks were bypassed. Post-merge workflows are being watched. No manual apply, purchase, listing, cancellation or tagging was performed. Deployed IAM acceptance, seller eligibility and cross-account runtime behavior are not claimed verified. AWS Sanity credentialed steps were skipped due to absent read-only role configuration. Follow-ups remain LeanerCloud/cloud-commitments-platform#42 and LeanerCloud/cloud-commitments-go#107 for unsupported/unproved tagging contracts; LeanerCloud/cloud-commitments-platform#261 for Marketplace credentials, LeanerCloud/cloud-commitments-platform#27 for polling/seller checks and LeanerCloud/cloud-commitments-platform#335 for failed compensating cancellation. This merge addresses the missing-grant scope only; auto-closure of #1967/#1968 does not close those separate issues. |
|
Post-merge verification completed: all eight workflows at merge commit efd6e95 passed, including CI Build & Test and the AWS, Azure and GCP deployment workflows with health/smoke steps. All watcher processes were collected. The merged tree remains identical to the locally verified and independently reviewed candidate. This is deployment-pipeline evidence, not a live permission evaluation, purchase, marketplace listing/cancellation or tag acceptance test. No manual cloud operation or cache/worktree deletion was performed. Separate gaps remain tracked in LeanerCloud/cloud-commitments-platform#42, LeanerCloud/cloud-commitments-go#107, LeanerCloud/cloud-commitments-platform#261, LeanerCloud/cloud-commitments-platform#27 and LeanerCloud/cloud-commitments-platform#335. |
1 participant
Summary
Closes #1967 and #1968.
Add six missing CE/EC2 permissions to the application's Lambda and Fargate runtime policies and the CloudFormation stack. These permissions support existing ladder baseline, instance-sizing, Standard RI marketplace, and RI-tagging calls.
ce:GetCostAndUsageproviders/aws/recommendations/ondemand_series.go:111*, current request has no billing-view ARNec2:DescribeInstanceTypesproviders/aws/recommendations/client.go:92*ec2:CreateReservedInstancesListingproviders/aws/services/ec2/client.go:1041*ec2:DescribeReservedInstancesListingsproviders/aws/services/ec2/client.go:1070*ec2:CancelReservedInstancesListingproviders/aws/services/ec2/client.go:1111*ec2:CreateTagsproviders/aws/services/ec2/client.go:283arn:aws:ec2:*:*:reserved-instances/*The EC2 listing/describe actions do not support resource-level scoping. Tagging remains in a separate Allow statement restricted to Reserved Instances. Runtime role attachments and the actual request/resource contracts were inspected directly. Marketplace authorization and DB claim/release behavior are unchanged. The valid listing scenario uses a Standard RI; convertible RIs are rejected before the AWS call.
Scope correction
Recovery HEAD
899ab790d40cf4bf0475edd7cd32ce2f21481186is rebased ontomain@81f2fc3ac44ce47221682fa1c50dec26e4f24eed.The earlier nine-action proposal included unsupported or unproved reserved-resource tagging assumptions. This revision removes its new
es:AddTags,redshift:DescribeTags, andredshift:CreateTagsgrants. OpenSearch explicitly does not support RI tagging; its existing provider behavior is tracked in LeanerCloud/cloud-commitments-platform#42. Redshift reserved-node tag support is unproved and tracked separately in LeanerCloud/cloud-commitments-go#107, with the related lookup optimization in LeanerCloud/cloud-commitments-go#87. Widening IAM does not establish API resource support.Only four files change. Provider implementation, federation policies, bootstrap permissions, trust policies and existing shipped guards remain unchanged.
Regression guard and local proof
The new stdlib-only Go guard derives CE/EC2 action names from SDK input literals in
providers/aws,internal, andpkg, including import aliases. It checks all three runtime policy files and has non-vacuity checks plus a six-action floor. This catches action omissions shared by every IaC flavor, which parity alone misses.It is an action-presence guard, not IAM evaluation. Effect, resource scope, role attachment, explicit Deny, and inline comments require separate review. Other namespaces and unused grants are outside this guard's scope.
Native macOS Go 1.26.6 with shared compiler/module caches:
go test -count=1 -p 1 -parallel 1 -v ./terraform/modules/compute/aws: all five top-level tests and 15 subtests pass.scripts/test-aws-iam-parity.sh: 5/5 pass;scripts/check-aws-iam-parity.sh: pass.Exact-HEAD CI passed, including root/module tests, integration and E2E tests, security scans, macOS/Linux MCP builds, Docker build, pre-commit hooks and Terraform validation. Terraform formatting/linting uses CI's 1.10.5 pin, not the different installed local version. No local provider initialization, root SDK build, private build cache, Docker or Windows verification was performed.
Final review gates and limits
Fresh-context Astra reviewed committed
899ab790dwith no confirmed findings, including an independent native race-enabled test pass: verbatim verdict.CodeRabbit substantively reviewed the complete four-file diff from
81f2fc3through899ab790dwith no confirmed findings: full review. Its status completed successfully at 22:27:52 UTC on September 14. Historical throttled status is not used as approval.All 28 check/status entries pass at this exact HEAD, including CI Build & Test and pre-commit. AWS Sanity's credentialed steps were skipped because its read-only role is not configured; its green workflow is not live AWS acceptance evidence.
No cloud apply, purchase, marketplace listing/cancellation, tag mutation, or deployed IAM acceptance test was run. These checks establish the source/IaC correction, not seller eligibility, SCP/session policy behavior or deployed role state. A real ladder plan and marketplace status remain unverified without the corresponding staging authority and resources.
Existing separate Marketplace gaps remain tracked: cross-account credential selection in LeanerCloud/cloud-commitments-platform#261, polling/seller checks in LeanerCloud/cloud-commitments-platform#27, and failed compensating cancellation in LeanerCloud/cloud-commitments-platform#335. These are not claimed fixed by this policy change.