fix(iac/aws): reconcile IAM action drift and add CFN/TF parity gate#1219
Conversation
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 5 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 Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughIAM action lists across CloudFormation, Terraform, and CLI IaC templates are reconciled to close permission drift identified in INF-02: compute modules (lambda/fargate) gain CE coverage actions and corrected OpenSearch API names; federation and CUDly templates add CostExplorer statements; a new ChangesAWS IAM Drift Fix and Parity Gate
Sequence Diagram(s)sequenceDiagram
participant CI as CI Job (aws-iam-parity)
participant Check as check-aws-iam-parity.sh
participant Test as test-aws-iam-parity.sh
participant Files as IaC Source Files
CI->>Check: run (REPO_ROOT)
Check->>Files: extract_actions(CFN runtime template)
Check->>Files: extract_actions(TF lambda template)
Check->>Files: extract_actions(TF fargate template)
Check->>Check: compare_pair runtime sets
Check->>Files: extract_actions(cross-account CFN, exclude orgs)
Check->>Files: extract_actions(cross-account TF, exclude orgs)
Check->>Check: compare_pair federation cross-account
Check->>Files: extract_actions(target CFN/TF/CLI x4, exclude orgs)
Check->>Check: compare_pair federation target variants
Check-->>CI: exit 0 (no drift) or exit 1 (drift found)
CI->>Test: run self-tests (fixture trees)
Test->>Check: run_case real repo (expect 0)
Test->>Check: run_case TF CE drift fixture (expect 1)
Test->>Check: run_case CFN CE drift fixture (expect 1)
Test->>Check: run_case missing file fixture (expect 2)
Test-->>CI: exit 0 (all cases passed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
d0792f0 to
7a16435
Compare
|
Rebased onto main to pick up the CI fixes from #1220 (lint MinSavingsUSD rename, govulncheck, e2e/integration test repairs). The previous Lint/Security/E2E/Integration failures were inherited from the old base, none touched files in this PR. Known remaining breakage on main itself: the pre-commit gocyclo hook fails on recEffectiveSavingsPct (complexity 12, introduced by #1206), tracked in #1222. That failure is unrelated to this PR and will show up on any branch until #1222 is fixed. Local verification on the rebased head:
|
|
Correction to the previous comment: the rebase did not clear Lint, Security Scanning, or Integration Tests, because main's own CI on its current HEAD (4108e51, post #1220) still fails those same three jobs (run 27325319311). Filed #1223 to track that breakage; the pre-commit gocyclo failure remains tracked in #1222. None of the failures involve files touched by this PR. PR-relevant checks on the rebased head are green: AWS IAM actions parity (the new gate), Azure role parity, Validate Terraform (aws/azure), Unit Tests, Docker build, Snyk, both sanity workflows, CodeRabbit. E2E (Docker Buildx setup) and Validate Terraform (gcp) (Setup Terraform step) failed in runner setup before reaching any code; re-ran both. |
7a16435 to
06820db
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@scripts/check-aws-iam-parity.sh`:
- Around line 45-49: The --root flag handling in the case statement does not
validate that a value is provided after the flag. When --root is passed without
a value, accessing $2 results in an unbound variable error and exit code 1. Add
a check in the --root case to verify that $2 exists (not empty or unset), and if
it does not, echo a usage error message to stderr and exit with code 2 before
attempting to assign REPO_ROOT="$2".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11a73212-eaa9-4d84-8422-1a02d0d76a00
📒 Files selected for processing (12)
.github/workflows/ci.ymlcloudformation/stacks/CUDly/template.yamliac/federation/aws-cross-account/cloudformation/template.yamliac/federation/aws-cross-account/terraform/main.tfiac/federation/aws-target/cloudformation/template.yamliac/federation/aws-target/terraform/main.tfinternal/iacfiles/templates/aws-cross-account-cli.sh.tmplinternal/iacfiles/templates/aws-wif-cli.sh.tmplscripts/check-aws-iam-parity.shscripts/test-aws-iam-parity.shterraform/modules/compute/aws/fargate/main.tfterraform/modules/compute/aws/lambda/main.tf
06820db to
c89eae6
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
… fix PR #1219 reconciles the AWS IAM action lists between the CloudFormation stack, the Terraform lambda/fargate modules, and the federation CFN/TF/CLI templates -- but existing customer deployments do not auto-update. Bundles deployed before this PR continue to silently degrade until re-applied: federation roles lack the CE Get*Coverage actions (coverage-targeted sizing assumes zero coverage), the cross-account CFN flavor lacks the optional EnableOrgDiscovery parameter and the legacy CE statement, and runtime deployments lack the new EC2 RI exchange actions and the new-style es:* OpenSearch actions. Add a Notices entry mirroring the existing 2026-04-22 federation-bundle precedent so the operational impact is visible to upgrading customers.
Adversarial review (PR #1219)Reviewed against the INF-02 finding (#1163) and the risk surfaces in the brief: CFN/TF IAM action drift, action additions vs removals, customer migration impact, parity gate implementation, least-privilege regressions, CI tool pinning, and tests. The fix is solid; one in-scope CHANGELOG entry pushed ( Verified
Fix pushed
Follow-up filed
UNSTABLE stateThree failing checks ( VerdictLGTM. CR re-ping below. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
… fix PR #1219 reconciles the AWS IAM action lists between the CloudFormation stack, the Terraform lambda/fargate modules, and the federation CFN/TF/CLI templates -- but existing customer deployments do not auto-update. Bundles deployed before this PR continue to silently degrade until re-applied: federation roles lack the CE Get*Coverage actions (coverage-targeted sizing assumes zero coverage), the cross-account CFN flavor lacks the optional EnableOrgDiscovery parameter and the legacy CE statement, and runtime deployments lack the new EC2 RI exchange actions and the new-style es:* OpenSearch actions. Add a Notices entry mirroring the existing 2026-04-22 federation-bundle precedent so the operational impact is visible to upgrading customers.
b3fa979 to
dba4f96
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
… fix PR #1219 reconciles the AWS IAM action lists between the CloudFormation stack, the Terraform lambda/fargate modules, and the federation CFN/TF/CLI templates -- but existing customer deployments do not auto-update. Bundles deployed before this PR continue to silently degrade until re-applied: federation roles lack the CE Get*Coverage actions (coverage-targeted sizing assumes zero coverage), the cross-account CFN flavor lacks the optional EnableOrgDiscovery parameter and the legacy CE statement, and runtime deployments lack the new EC2 RI exchange actions and the new-style es:* OpenSearch actions. Add a Notices entry mirroring the existing 2026-04-22 federation-bundle precedent so the operational impact is visible to upgrading customers.
dba4f96 to
e582d60
Compare
|
Rebased onto origin/main (2448a02, which merged #1217). es: rename conflict resolved: #1217 already applied the Parity gate re-verified:
Terraform gates: New head: e582d60 @coderabbitai review |
|
✅ Action performedReview finished.
|
… fix PR #1219 reconciles the AWS IAM action lists between the CloudFormation stack, the Terraform lambda/fargate modules, and the federation CFN/TF/CLI templates -- but existing customer deployments do not auto-update. Bundles deployed before this PR continue to silently degrade until re-applied: federation roles lack the CE Get*Coverage actions (coverage-targeted sizing assumes zero coverage), the cross-account CFN flavor lacks the optional EnableOrgDiscovery parameter and the legacy CE statement, and runtime deployments lack the new EC2 RI exchange actions and the new-style es:* OpenSearch actions. Add a Notices entry mirroring the existing 2026-04-22 federation-bundle precedent so the operational impact is visible to upgrading customers.
|
@coderabbitai review |
e582d60 to
13cb361
Compare
✅ Action performedReview finished.
|
The CloudFormation stack, the Terraform lambda/fargate modules, and the federation CFN/TF/CLI templates each granted a different, partially insufficient IAM action set for the same application code: - CFN stack lacked savingsplans:DescribeSavingsPlansOfferings (called in the purchase pre-flight) and the EC2 RI exchange-quote actions. - TF lambda/fargate lacked ce:GetReservationCoverage and ce:GetSavingsPlansCoverage (coverage-targeted sizing silently degraded to zero existing coverage), ec2:DescribeRegions, and rds:DescribeDBInstances; they also still granted the legacy es:*ReservedElasticsearch* actions instead of the new-style es:* actions the OpenSearch SDK calls require (overlaps #1149). - The federation cross-account CFN template lacked the optional org-discovery statement its Terraform sibling has, and all newer federation templates lost the Cost Explorer statement the legacy CUDly-CrossAccount template grants, breaking CE-based recommendations for federated accounts. - savingsplans:DescribeSavingsPlanRates was granted by TF only and has no caller anywhere in the repo; dropped. Reconcile all lists to a single canonical set per deployment role and add scripts/check-aws-iam-parity.sh, the AWS sibling of check-azure-role-parity.sh, wired into ci.yml as a gating job. The check compares the application-owned IAM namespaces across the runtime trio (CFN stack vs TF lambda vs TF fargate), the federation cross-account CFN/TF pair (including org discovery), and the federation core set across both flavors plus the embedded CLI onboarding templates. scripts/test-aws-iam-parity.sh replicates the pre-fix drift shapes in fixture trees and asserts the check fails on them; the check was also verified to fail against the pre-fix tree and pass after reconciliation. Closes #1163
Under set -euo pipefail, passing --root without a following path value hit `$2` as an unbound variable, exiting 1 with a bash error instead of the intended exit 2 plus a usage message. Guard the argument before dereferencing it and emit an explicit "--root requires a path value" error. Add a self-test case asserting exit 2 for the bare --root flag.
… fix PR #1219 reconciles the AWS IAM action lists between the CloudFormation stack, the Terraform lambda/fargate modules, and the federation CFN/TF/CLI templates -- but existing customer deployments do not auto-update. Bundles deployed before this PR continue to silently degrade until re-applied: federation roles lack the CE Get*Coverage actions (coverage-targeted sizing assumes zero coverage), the cross-account CFN flavor lacks the optional EnableOrgDiscovery parameter and the legacy CE statement, and runtime deployments lack the new EC2 RI exchange actions and the new-style es:* OpenSearch actions. Add a Notices entry mirroring the existing 2026-04-22 federation-bundle precedent so the operational impact is visible to upgrading customers.
13cb361 to
d45ffc5
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Merged to main after pre-merge adversarial review (rebase-then-merge through #1217/#1364/#1389): additive read-only CE/RI-exchange IAM actions (no new wildcards, Resource=* only where the API lacks resource scoping) plus a hermetic CFN-vs-TF IAM parity gate proven to catch injected drift. terraform validate + parity gate green. |
1 participant
Problem
Closes #1163 (review finding INF-02, P2).
The CloudFormation stack, the Terraform lambda/fargate modules, and the federation CFN/TF/CLI templates each encode their own copy of the IAM action list for the same application code, and the copies drifted:
savingsplans:DescribeSavingsPlansOfferings(called in the Savings Plans purchase pre-flight, so CFN deployments 403 there) and the EC2 RI exchange-quote actions.ce:GetReservationCoverage/ce:GetSavingsPlansCoverage(coverage-targeted sizing silently degrades to "assume zero existing coverage"),ec2:DescribeRegions, andrds:DescribeDBInstances.CUDly-CrossAccounttemplate grants, breaking CE-based recommendation flows for federated accounts.check-azure-role-parity.sh); AWS had none.Fix
savingsplans:DescribeSavingsPlansOfferings, +ec2:GetReservedInstancesExchangeQuote, +ec2:AcceptReservedInstancesExchangeQuote, +organizations:DescribeOrganization.ce:GetReservationCoverage, +ce:GetSavingsPlansCoverage, +ec2:DescribeRegions, +rds:DescribeDBInstances, +organizations:DescribeAccount; legacyes:*ReservedElasticsearch*actions replaced with the new-stylees:*actions the OpenSearch SDK calls require (overlaps INF-01: Terraform runtime IAM grants legacy es:*ReservedElasticsearch* actions; code calls the OpenSearch API, breaking OpenSearch RI flows on TF deployments #1149, which tracks the same TF-side gap); droppedsavingsplans:DescribeSavingsPlanRates, which has no caller anywhere in the repo.aws-cross-accountandaws-targetCFN + TF templates and in both embedded CLI onboarding templates; added an opt-inEnableOrgDiscoveryparameter + conditionalOrganizationsDiscoverystatement to the cross-account CFN template, mirroring the Terraform module'senable_org_discovery(default false in both).scripts/check-aws-iam-parity.sh, the AWS sibling ofcheck-azure-role-parity.sh. It extracts the application-owned IAM namespaces (ce,ec2,rds,elasticache,es,redshift,memorydb,savingsplans,organizations) and asserts parity across: the runtime trio (CFN stack vs TF lambda vs TF fargate), the federation cross-account CFN/TF pair (including org discovery), and the federation core set across both flavors plus the CLI templates.scripts/test-aws-iam-parity.shself-tests and wired both intoci.ymlas a gatingaws-iam-parityjob (added toci-successneeds).Test evidence
scripts/check-aws-iam-parity.shagainst a pristineorigin/maincheckout: exit 1, reporting exactly the drift instances from the finding (CE Coverage, exchange quotes,DescribeSavingsPlansOfferings, legacy/newesnames, federation org statement). Against this branch: exit 0.scripts/test-aws-iam-parity.sh: 4/4 pass. The drift fixtures replicate the real pre-fix failure shapes (TF lambda missing the CE Coverage actions; federation CFN missing the org statement) and the check fails on them; a missing compared file fails loud with exit 2.cfn-linton all three touched CFN templates: findings identical to the pre-existing baseline onmain(no new findings).terraform fmt -check -recursiveclean;actionlint .github/workflows/ci.ymlclean.go build ./...andgo test ./internal/iacfiles/... ./internal/api/ -run "Federation|Template|Iac|IaC": 71 tests pass (covers the edited embedded templates).Notes
README.mdcontains an illustrative minimal CLI policy that is not an IaC artifact and is not covered by the gate.Summary by CodeRabbit
New Features
Tests
Chores