chore(iac/aws/networking): remove redundant Secrets Manager VPC endpoint

[cristim]

Summary

Removes the AWS Secrets Manager VPC interface endpoint and its dedicated security group / unused outputs. The endpoint is redundant: every environment already runs fck-NAT (enable_nat_gateway = true in terraform/environments/aws/networking.tf:16), so in-VPC Secrets Manager consumers (main Lambda, cleanup Lambda, Fargate task) reach SM over NAT just like they reach ECR and any other non-IPv6 service.

Savings: ~$14/mo (2 AZs × ~$7.20). NAT byte-cost delta is negligible — GetSecretValue payloads are tiny.

What's removed

• aws_vpc_endpoint.secretsmanager (was main.tf:743-754)
• aws_security_group.vpc_endpoints — the endpoint was its sole consumer
• Three outputs (none referenced by any caller per grep): vpc_endpoints_security_group_id, secretsmanager_endpoint_id, secretsmanager_endpoint_dns

What's updated

• File-header comment in main.tf updated to reflect actual topology (NAT present, per-service interface endpoints removed)

Precondition for safety

enable_nat_gateway must remain true in every environment. If a future change turns NAT off, this endpoint (and likely the ECR ones too) must be reintroduced first or in-VPC SM calls will fail.

Test plan

• terraform validate clean in terraform/environments/aws/
• terraform fmt no-diff
• CI pre-commit hooks green (tflint, terraform-fmt, etc.)
• Post-merge terraform plan in a real env should show only the endpoint + SG destroy + three output removals, no other diffs

Summary by CodeRabbit

• Chores
  • Simplified networking by removing automatic provisioning of service interface endpoints and related outputs.
  • Added a safeguard requiring NAT gateway to be enabled when creating private subnets to ensure IPv4 egress to managed services.
• Documentation
  • Clarified module header to reflect that egress is handled by the designated NAT instance.

[cristim]

@coderabbitai review

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ddc2c1ed-2609-4399-a1dc-17f709456213

📥 Commits

Reviewing files that changed from the base of the PR and between 18cc2c9 and 9a1fdce.

📒 Files selected for processing (2)

• terraform/modules/networking/aws/main.tf
• terraform/modules/networking/aws/outputs.tf

💤 Files with no reviewable changes (1)

• terraform/modules/networking/aws/outputs.tf

🚧 Files skipped from review as they are similar to previous changes (1)

• terraform/modules/networking/aws/main.tf

---

📝 Walkthrough

Walkthrough

Removes VPC endpoint resources and outputs from the AWS networking Terraform module, updates module header to state fck-nat handles egress, and adds a precondition on module-managed private subnets requiring enable_nat_gateway for IPv4 egress.

Changes

VPC endpoint infrastructure removal

Layer / File(s)	Summary
Module header documentation update terraform/modules/networking/aws/main.tf	Module header changed to state fck-nat handles egress instead of recommending NAT Gateway/VPC endpoints.
Private subnet precondition terraform/modules/networking/aws/main.tf	Added a precondition to aws_subnet.private that requires enable_nat_gateway when the module creates private subnets, failing plan/apply otherwise.
Remove VPC endpoint resources and outputs terraform/modules/networking/aws/main.tf, terraform/modules/networking/aws/outputs.tf	Deleted aws_security_group.vpc_endpoints, aws_vpc_endpoint.secretsmanager, associated aws_region data source, and removed outputs for VPC endpoint security group ID and Secrets Manager endpoint ID/DNS.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested labels

severity/medium, effort/s

Poem

🐇 I hopped through Terraform, tidy and spry,
I found the endpoints, waved them goodbye.
fck-nat now carries the outbound song,
Outputs removed, configs trimmed along.
A leaner network — brief hop, wandered on.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: removing a redundant Secrets Manager VPC endpoint from the networking module. It is specific, concise, and clearly conveys the primary objective.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch chore/remove-secretsmanager-vpc-endpoint

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

✅ Actions performed

Review triggered.

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.

[coderabbitai]

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 `@terraform/modules/networking/aws/main.tf`:
- Around line 2-4: Add a Terraform validation that fails fast when NAT egress is
disabled but no IPv4 egress endpoints exist: check the variable
enable_nat_gateway and the module's endpoint flags (e.g.,
enable_secrets_manager_endpoint, enable_ecr_endpoint or general
enable_vpc_endpoints) and raise an error if enable_nat_gateway == false AND none
of the IPv4 endpoints are enabled; implement this as a validation block in the
networking module (referencing enable_nat_gateway and the endpoint variables) so
plans error out instead of relying on docs.

🪄 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: 1fe0b8ba-68b8-4296-bfb2-9de68d8e9e46

📥 Commits

Reviewing files that changed from the base of the PR and between 80c20d2 and 18cc2c9.

📒 Files selected for processing (2)

• terraform/modules/networking/aws/main.tf
• terraform/modules/networking/aws/outputs.tf

💤 Files with no reviewable changes (1)

• terraform/modules/networking/aws/outputs.tf

[cristim]

Added lifecycle.precondition on aws_subnet.private: fails at plan time when enable_nat_gateway = false, since that is now the sole IPv4 egress path after removing the Secrets Manager endpoint. terraform fmt -check and terraform validate both pass. @coderabbitai review

[coderabbitai]

Tip

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Show current state of the networking module files
fd -e tf . terraform/modules/networking/aws/ --exec cat {}

Length of output: 23905

---

---

@cristim: The implementation looks solid. A few observations from reading the code:

What works well

• Correct scope: the precondition only evaluates when local.create_vpc = true (count > 0). Callers using use_existing_vpc or use_default_vpc are unaffected — their VPCs own their egress, which is exactly the right boundary.
• Fail-fast semantics: with enable_nat_gateway defaulting to false, any new-VPC deployment that omits an explicit enable_nat_gateway = true will error at plan rather than silently reaching a state where private workloads can't reach Secrets Manager or ECR over IPv4.
• Actionable error message: it names the affected services and calls out the existing-VPC escape hatch.
• Terraform version compatibility: lifecycle.precondition requires Terraform ≥ 1.2; the module already requires >= 1.6.0, so there is no version gap to worry about.

One minor nit — IPv6-only escape hatch not surfaced in the error

If a caller is certain all their workloads are IPv6-native (and the matching AWS services actually support IPv6 endpoints in their region), the precondition still blocks them even though aws_egress_only_internet_gateway.main would provide outbound IPv6. The current wording correctly steers people to NAT or an existing VPC, but you could optionally add var.enable_ipv6 to the condition if you ever want to support a strict IPv6-only path:

condition     = var.enable_nat_gateway || var.enable_ipv6
error_message = "Private subnets require either enable_nat_gateway=true (IPv4 egress via fck-nat) or enable_ipv6=true (IPv6 egress via EIGW). Without at least one, workloads cannot reach AWS APIs."

That said, given that several AWS services still lack dual-stack endpoints (SES, some regional Secrets Manager endpoints), keeping the condition as var.enable_nat_gateway alone is the safer conservative choice and is perfectly reasonable for this repo's current workloads.

[approve]

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

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]

@coderabbitai review

[coderabbitai]

✅ 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]

@coderabbitai review

[coderabbitai]

✅ 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]

Adversarial review notes

Reviewed this as a single-egress-path risk surface (any blast radius that follows from "NAT is now sole IPv4 path to AWS APIs"). The PR was opened ~3 weeks ago, so I also re-verified each assumption against the current main. Findings:

No actionable changes needed

The PR is sound and CR's prior round was addressed. Re-walked the dependency chain:

• NAT is enabled in every consumer env, and there's exactly one. Grepped terraform/environments/ on main for module callers of networking/aws: only terraform/environments/aws/networking.tf exists, and it sets enable_nat_gateway = true explicitly (with the comment "Required for ECR access from private subnets"). No env disables NAT today.
• Precondition is on the right resource. Final state has the lifecycle.precondition only on aws_subnet.private (where count = local.create_vpc ? var.az_count : 0), not duplicated on aws_vpc.main — commit 50d21f90c correctly dropped the duplicate aws_vpc.main precondition. aws_subnet.private is the more accurate trigger: it fires only when the module is actually creating private subnets (skipped under use_existing_vpc / use_default_vpc, whose VPCs own their egress). Caller-supplied VPCs are not blocked.
• Default still safe-by-fail. variable "enable_nat_gateway" still defaults to false in variables.tf, so any new caller of this module that omits the flag will hit the precondition at plan time with an actionable error naming SM/ECR. That is exactly the §"Fail loud" rule applied to a topology change — feedback_no_silent_fallbacks.md compliant.
• No orphan output consumers. Grepped the whole repo on main for secretsmanager_endpoint_id / secretsmanager_endpoint_dns / vpc_endpoints_security_group_id / aws_vpc_endpoint.secretsmanager / aws_security_group.vpc_endpoints — only hits are inside the two files this PR rewrites. Nothing else consumes the removed outputs, so the destroy is safe.
• Egress SG still allows :443 outbound. Lambda/Fargate SGs in compute/ are unchanged by this PR; they already allow 0.0.0.0/0:443 outbound to reach NAT → public SM. The deleted aws_security_group.vpc_endpoints was only ingress-from-VPC for the endpoint side, never on the consumer side.
• Drift check vs main is clean. Merge-base of origin/chore/remove-secretsmanager-vpc-endpoint and origin/main is 451a70f73 (current main HEAD). No sibling PR removed the endpoint first; this PR is still the source of truth for the change, and there is no rebase to do.
• CR ratchet is clean and on the latest head. Latest CR review request 2026-06-19 21:53:16 UTC post-dates the latest pushed commits (2026-06-19 21:41:58 UTC), and CR's reply was "Review finished" with no actionable comments. Earlier rounds raised the NAT-fail-fast concern, which is now implemented.

Considered, no change needed

• Pre-existing UNSTABLE on main, not PR-caused. The 3 failing checks (Lint Code, Integration Tests, Security Scanning) all belong to CI - Build & Test, which is red on main@451a70f73 itself with the same workflow failing. Confirmed via gh run list -b main --workflow="CI - Build & Test" — last 5 runs all failed, including the merge-base commit. The PR's own added/changed surface (terraform-fmt, validate, tflint) all pass. Same shape as the carve-out documented in PR ci(pre-commit): fetch Trivy installer from pinned release tag #1238.
• feedback_no_silent_fallbacks.md applicability. The PR is the canonical example of the rule — replacing a doc-only "NAT must exist" hope with a hard plan-time precondition that names the affected services. No latency-doc change needed in code: SM payloads (GetSecretValue) are sub-KB and traverse NAT once per cold start (cached thereafter by the SDK); the body already calls this out as "negligible".
• feedback_tf_provider_version_align.md applicability. PR doesn't touch any versions.tf or provider constraint blocks; only deletes resources/outputs and edits a header comment + adds a lifecycle.precondition. Rule doesn't apply.
• feedback_pr_workflow.md compliance. Base is main. Confirmed via gh pr view 915 → "baseRefName":"main". Stale-PR concern was raised in the task, but the head commits sit on top of current main HEAD (merge-base == origin/main), so no rebase or major drift exists.
• IPv6-only escape hatch (raised by CR earlier). CR's earlier [approve] comment noted that a strict IPv6-only deployment could legitimately skip NAT via aws_egress_only_internet_gateway.main. The condition stays var.enable_nat_gateway alone, because Secrets Manager interface endpoints (and SES) still lack dual-stack today. Adding an || var.enable_ipv6 escape hatch now would re-open the exact failure mode this PR closes; defer until AWS publishes dual-stack SM endpoints across regions.

Out-of-scope follow-ups

None. The PR's scope is exactly the endpoint removal + fail-fast precondition; the broader main-side CI red surface belongs to the workflows that own those checks.

CR re-requested.

[cristim]

@coderabbitai review

[coderabbitai]

✅ 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.