Skip to content

fix(terraform/aws): grant OpenSearch RI actions instead of legacy es names#1217

Open
cristim wants to merge 1 commit into
mainfrom
fix/inf-01-opensearch-iam-actions
Open

fix(terraform/aws): grant OpenSearch RI actions instead of legacy es names#1217
cristim wants to merge 1 commit into
mainfrom
fix/inf-01-opensearch-iam-actions

Conversation

@cristim

@cristim cristim commented Jun 11, 2026

Copy link
Copy Markdown
Member

Problem

Closes #1149 (review finding INF-01, P2).

Both AWS runtime Terraform modules (terraform/modules/compute/aws/lambda/main.tf, terraform/modules/compute/aws/fargate/main.tf) granted only the legacy Elasticsearch-era reserved-instance actions:

  • es:DescribeReservedElasticsearchInstances
  • es:DescribeReservedElasticsearchInstanceOfferings
  • es:PurchaseReservedElasticsearchInstanceOffering

The application uses the OpenSearch SDK (opensearch.NewFromConfig in providers/aws/services/opensearch/client.go), whose DescribeReservedInstances, DescribeReservedInstanceOfferings and PurchaseReservedInstanceOffering operations authorize against the distinct new-style action names. On any Terraform-deployed Lambda or Fargate runtime, host-account OpenSearch RI describe/offering/purchase calls returned AccessDenied. The parallel CloudFormation stack already grants the correct actions, confirming TF drift.

Fix

Replace the three legacy actions with the new-style ones in both runtime modules, matching the CloudFormation stack:

  • es:DescribeReservedInstances
  • es:DescribeReservedInstanceOfferings
  • es:PurchaseReservedInstanceOffering

Added a regression test (terraform/modules/compute/aws/iam_actions_test.go) that reads both runtime module files and fails if any legacy es:*ReservedElasticsearch* action is granted or any of the three required OpenSearch actions is missing.

Out of scope, tracked separately: es:AddTags for post-purchase tagging (INF-10) and the broader CFN/TF parity gate (INF-02).

Test evidence

  • Regression test confirmed to FAIL against pre-fix code (both modules flagged for legacy actions and missing new-style actions), then PASS after the fix: go test ./terraform/modules/compute/aws/ -> 3 passed.
  • go build ./... -> success; go vet ./terraform/... -> no issues.
  • terraform fmt -check -recursive terraform/ -> clean.
  • cd terraform/environments/aws && terraform init -backend=false && terraform validate -> "The configuration is valid."

@cristim cristim added triaged Item has been triaged priority/p2 Backlog-worthy severity/high Significant harm urgency/this-quarter Within the quarter impact/few Limited audience effort/s Hours type/bug Defect labels Jun 11, 2026
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 5 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: aa0f5e59-46f0-4a55-8c13-1ffeb3129f4f

📥 Commits

Reviewing files that changed from the base of the PR and between 451a70f and 6603c94.

📒 Files selected for processing (3)
  • terraform/modules/compute/aws/fargate/main.tf
  • terraform/modules/compute/aws/iam_actions_test.go
  • terraform/modules/compute/aws/lambda/main.tf
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/inf-01-opensearch-iam-actions

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

@cristim

cristim commented Jun 11, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim cristim force-pushed the fix/inf-01-opensearch-iam-actions branch from 1e4cad4 to e898cba Compare June 11, 2026 05:16
@cristim cristim force-pushed the fix/inf-01-opensearch-iam-actions branch from e898cba to d0679b8 Compare June 19, 2026 14:46
@cristim

cristim commented Jun 19, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…names

The lambda and fargate runtime modules granted only the legacy
Elasticsearch-era actions (es:DescribeReservedElasticsearchInstances,
es:DescribeReservedElasticsearchInstanceOfferings,
es:PurchaseReservedElasticsearchInstanceOffering), while the application
uses the OpenSearch SDK whose DescribeReservedInstances,
DescribeReservedInstanceOfferings and PurchaseReservedInstanceOffering
operations authorize against the distinct new-style es:* action names.
As a result, every host-account OpenSearch RI describe/offering/purchase
call returned AccessDenied on Terraform deployments, while the parallel
CloudFormation stack already granted the correct actions.

Replace the three legacy actions with the new-style ones in both runtime
modules, matching the CloudFormation stack, and add a regression test
that fails if a runtime module grants legacy Elasticsearch-era actions
or drops any of the OpenSearch actions the client code requires.

Closes #1149
@cristim cristim force-pushed the fix/inf-01-opensearch-iam-actions branch from d0679b8 to 6603c94 Compare June 26, 2026 17:21
@cristim

cristim commented Jun 26, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@cristim

cristim commented Jun 26, 2026

Copy link
Copy Markdown
Member Author

Adversarial review — PR #1217

Reviewed against the stated risk surfaces (action-name rename Elasticsearch→OpenSearch; coordination with sibling #1219; CFN/TF parity; customer impact / in-flight purchases; provider-version interaction; CR-finding overlap with #1219). Net: no blocking findings. One pre-existing UNSTABLE cause (migration-074 collision on stale base) fixed by rebasing onto current main; pushed.

What I checked

  • Action-name rename correctness. AWS renamed the service Elasticsearch→OpenSearch in 2021, but the IAM action namespace stayed es:. The OpenSearch SDK calls DescribeReservedInstances / DescribeReservedInstanceOfferings / PurchaseReservedInstanceOffering, which authorize against es:DescribeReservedInstances, es:DescribeReservedInstanceOfferings, es:PurchaseReservedInstanceOffering — not the legacy es:*ReservedElasticsearch* names and not aoss:* (that namespace is for OpenSearch Serverless, a separate product). The PR uses the correct names. Confirmed against the actual providers/aws/services/opensearch/client.go call sites (lines 24-26 declare the three interface methods; lines 89-94, 188-194, 229-244 invoke them). All three SDK ops are needed, and all three new es:* actions are granted in both lambda/main.tf and fargate/main.tf.
  • Coordination with sibling PR fix(iac/aws): reconcile IAM action drift and add CFN/TF parity gate #1219 (IAM-parity gate). fix(iac/aws): reconcile IAM action drift and add CFN/TF parity gate #1219 explicitly says it "replaces legacy es:*ReservedElasticsearch* actions with the new-style es:* 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)". So whichever of {fix(terraform/aws): grant OpenSearch RI actions instead of legacy es names #1217, fix(iac/aws): reconcile IAM action drift and add CFN/TF parity gate #1219} lands first wins on the three ES action lines; the second will rebase cleanly because the diff content is identical there. After this PR, both runtime modules carry the same set of es:* actions as the CFN stack, so the fix(iac/aws): reconcile IAM action drift and add CFN/TF parity gate #1219 aws-iam-parity gate will see green on the ES dimension regardless of merge order.
  • CFN/TF parity. Verified: cloudformation/stacks/CUDly/template.yaml:438-440 and cloudformation/stacks/CUDly-CrossAccount/template.yaml:132-134 already carry the new-style actions. After PR fix(terraform/aws): grant OpenSearch RI actions instead of legacy es names #1217, TF matches CFN on the ES RI surface. The legacy es:*ReservedElasticsearch* string survives nowhere outside the test docstrings (which describe what the regression guard protects against).
  • Customer impact / in-flight purchases. This is a runtime-IAM permission expansion (replacing actions the Lambda/Fargate role had been granted but the application never called) with a contraction (removing legacy actions the application also never called) — both names map to the same managed service, but only the new-style names are accepted by the OpenSearch SDK control plane. Any OpenSearch RI purchase that was in-flight via the Terraform-deployed runtime was already 403'ing pre-PR; the legacy actions have been dead grants for years (since the 2021 service rename). The CFN stack has long had the correct actions, so any actually-functioning OpenSearch RI flow was already authorized via the CFN path. No revoke of a permission a real caller relied on; no migration concern.
  • Regression test logic. iam_actions_test.go reads both TF files, fails on any regex match es:\w*ReservedElasticsearch\w* (catches all three legacy names plus any future variant), and fails if any of the three required new-style actions is missing. The required-action membership check uses strings.Contains(content, "\""+action+"\"") which requires the action be quoted — guards against substring false-positives (e.g. a comment mentioning the name unquoted). I re-ran the test in the rebased worktree: 3/3 pass.
  • Per memory feedback_no_silent_fallbacks.md. Removing legacy action grants that were silently 403'ing OpenSearch RI calls is the right move — fail-loud on actually-needed permissions.
  • Per memory feedback_tf_provider_version_align.md. No versions.tf / provider-version changes. The IAM action name list is a plain string list in the policy document — no provider-version-gated acceptance.
  • Per memory feedback_pr_workflow.md. Base is main ✓.

Pre-existing UNSTABLE: migration-074 collision (fixed via rebase)

The PR's pre-commit job was failing on check-migration-conflicts: Duplicate migration number(s) found: 000074. The PR itself adds zero migrations — the duplicate was caused by a stale base. Specifically, the branch's tree had:

Per memory project_migration_number_collisions.md ("migration-number dups only fail in CI (pre-commit --all-files on the merge ref), not local pre-commit"), and per the explicit "Watch for migration-074→077 rebase issue" note in feedback_pr_workflow.md, current main already cleared the collision via PR #1261 (5894580f3 fix(db): renumber audit_actor_stamps migration 074->077 to clear collision).

Action taken. Rebased fix/inf-01-opensearch-iam-actions onto current origin/main (HEAD 451a70f73). The rebase replays cleanly (single conflict-free commit on a path entirely disjoint from the migrations rename). Post-rebase the migrations dir shows 000074_repair_partial_migration_058_067.* plus the renumbered 000077_audit_actor_stamps.* — no duplicates. Local checks pass:

  • go test ./terraform/modules/compute/aws/ → 3/3 pass.
  • terraform fmt -check -recursive terraform/ → clean.
  • cd terraform/environments/aws && terraform init -backend=false && terraform validate → "The configuration is valid."

Force-pushed (--force-with-lease): d0679b83c → 6603c94a7.

CodeRabbit state

The original CR review on 2026-06-11 hit a CR rate limit ("Review limit reached"). The two subsequent @coderabbitai review pings returned "Review finished" but, because CR is incremental, both were silent false-cleans on the rate-limited commit — see memory feedback_pr_workflow.md: "recover from any CR rate-limit with @coderabbitai full review, never the incremental @coderabbitai review, since a throttled pass silently skips the in-flight commits and yields a false-clean". Re-pinged with @coderabbitai full review after this rebase push so CR can actually review the diff for the first time.

Repo-wide CI failures unrelated to this PR

Lint Code, Security Scanning, Integration Tests, E2E Tests, CI Success failures all reproduce on origin/main HEAD (451a70f73) at the same time — Go lint debt, npm-audit findings, an integration-test infrastructure issue, and an E2E flake. Same situation as PR #1244's adversarial review noted. From a code-correctness standpoint this PR is mergeable.

Verdict

No fix needed on PR #1217 itself; the rename of three IAM action names is the entire correctness story and it is correct. UNSTABLE state cleared via the documented migration-074→077 rebase path. Recommend merging once CR is clean on the rebased commit and the unrelated repo-wide CI failures are addressed at the repo level (or accepted as merge-blocked-by-known-issues per project policy).

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@cristim Thanks for the thorough adversarial review. I agree with your conclusion: nothing in #1217 looks blocking on the actual IAM-action rename, and the rebase onto current main addresses the unrelated migration-collision instability.

Your checks on the OpenSearch SDK call surface, the correct es: action names, CFN/TF parity, and the overlap with #1219 all make sense. Given the repo-wide CI failures are reproducing on main, this PR looks code-correct from what you documented, pending the post-rebase full review finishing cleanly and whatever merge policy the repo is using for the known unrelated failures.

(•ㅅ•)


✏️ Learnings added
Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/1217

Timestamp: 2026-06-26T17:23:32.293Z
Learning: In this repository, if a CodeRabbit review was throttled or hit a rate limit, the correct recovery is to request `coderabbitai full review` rather than `coderabbitai review`, because incremental review can silently skip the affected in-flight commit and report a false-clean result.

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/1217

Timestamp: 2026-06-26T17:23:32.293Z
Learning: For the OpenSearch reserved-instances flows used by `providers/aws/services/opensearch/client.go`, the correct IAM permissions remain in the `es:` namespace: `es:DescribeReservedInstances`, `es:DescribeReservedInstanceOfferings`, and `es:PurchaseReservedInstanceOffering`. The legacy `es:*ReservedElasticsearch*` actions and the `aoss:*` namespace are not correct for these SDK calls.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@cristim

cristim commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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

Labels

effort/s Hours impact/few Limited audience priority/p2 Backlog-worthy severity/high Significant harm triaged Item has been triaged type/bug Defect urgency/this-quarter Within the quarter

Projects

None yet

1 participant