Update dynamic-subnets module to v3

[aknysh]

what

• Updated terraform-aws-dynamic-subnets module from v2.4.2 to v3.0.1
• Added 6 new variables for enhanced subnet configuration:
  • public_subnets_per_az_count and public_subnets_per_az_names for independent public subnet control
  • private_subnets_per_az_count and private_subnets_per_az_names for independent private subnet control
  • nat_gateway_public_subnet_indices for index-based NAT Gateway placement
  • nat_gateway_public_subnet_names for name-based NAT Gateway placement
• Updated AWS Provider requirement from >= 4.9.0, < 6.0.0 to >= 5.0.0 (drops support for v4.x)
• Updated Go to 1.25 and all test dependencies to latest versions (Terratest v0.52.0)
• Added 14 new outputs to expose all dynamic-subnets v3.0.0 capabilities
• Improved test code with helper functions, centralized constants, and fixed S3 cleanup order bug
• Updated README.yaml with comprehensive usage examples and breaking change notice
• Created detailed PRD documenting all changes, migration guide, and backward compatibility considerations

why

• Cost Optimization: New NAT Gateway placement features enable strategic placement to reduce costs by up to 67% (e.g., $270/month savings for 3 AZs by placing NAT in only one public subnet per AZ instead of all public subnets)
• Architectural Flexibility: Separate public/private subnet configuration allows different subnet counts and names per type (e.g., 2 public subnets + 3 private subnets per AZ)
• Enhanced Control: Index-based and name-based NAT placement provides precise control over NAT Gateway location for cost and architecture optimization
• AWS Provider v6.x Support: Updated provider constraint removes upper bound to support latest AWS Provider versions
• Configuration Backward Compatible: All existing Terraform configurations continue to work unchanged (on AWS Provider v5.0+) - new variables default to null and trigger legacy behavior
• Enhanced Outputs: Subnet stats outputs now include NAT Gateway IDs, enabling easier integration with Network Firewall and other resources
• Test Reliability: Go 1.25 and updated dependencies provide latest security fixes and improved test reliability

Configuration Backward Compatibility:
✅ All existing Terraform configurations work unchanged (assuming AWS Provider v5.0+)

• Legacy variables (subnets_per_az_count, subnets_per_az_names) continue to work
• When new variables are not set, behavior is identical to v2.4.2
• No resources will be recreated for existing configurations
• No state migration required

Real-World Cost Savings Example (3 AZs, us-east-1):

• Old: 6 NAT Gateways (2 per AZ) = ~$270/month
• New: 3 NAT Gateways (1 per AZ) = ~$135/month
• Savings: $135/month ($1,620/year)

Key Features:

• Independent control over public and private subnet counts per AZ
• Flexible NAT Gateway placement (index-based or name-based)
• Named subnets with different naming schemes for public vs private
• NAT Gateway IDs exposed in subnet stats outputs
• Configuration backward compatible with existing deployments (on AWS Provider v5.0+)

references

• Dynamic Subnets v3.0.1 Release: https://github.com/cloudposse/terraform-aws-dynamic-subnets/releases/tag/v3.0.1
• Dynamic Subnets PR #226: Separate Public/Private Subnet Configuration and Enhance NAT Gateway Placement cloudposse/terraform-aws-dynamic-subnets#226
• Comprehensive PRD: docs/prd/upgrade-to-dynamic-subnets-v3.md
• AWS Provider v5.0 Migration Guide: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/guides/version-5-upgrade
• AWS NAT Gateway Pricing: https://aws.amazon.com/vpc/pricing/

Summary by CodeRabbit

• New Features
  • Per‑AZ independent public/private subnet counts & names, named subnets, flexible NAT placement (by index or name), VPC Endpoints, optional Shield, additional NAT/network & IPv6 subnet outputs, AZ route‑table maps.
• Documentation
  • Expanded README, upgrade PRD, migration notes, cost‑optimized, HA and production examples, and v3.0.1 release notes.
• Bug Fixes
  • NAT routing fix; NAT IDs exposed in outputs.
• Breaking Changes
  • AWS provider minimum bumped to v5.0+.
• Tests
  • Expanded VPC test suite, new fixtures, and mutual‑exclusivity validation for NAT placement.

[coderabbitai]

Walkthrough

Upgrade aws-vpc to integrate terraform-aws-dynamic-subnets v3.0.1: add independent per‑AZ public/private subnet controls, index- or name‑based NAT gateway placement with plan‑time mutual‑exclusivity validation, new nullable inputs and expanded outputs (per‑AZ maps, named‑subnet stats, NAT/EIP/IPv6 details), AWS provider >=5.0.0, docs/PRD additions, expanded tests, and tooling bumps.

Changes

Cohort / File(s)	Summary
Documentation README.md, README.yaml, src/README.md, docs/prd/upgrade-to-dynamic-subnets-v3.md	Add extensive v3.0.1 docs and PRD: per‑AZ public/private controls, named subnets, index/name NAT placement, cost‑optimization examples, VPC Endpoints/Shield notes, migration and release details.
Module Invocation & Validation src/main.tf, src/variables.tf	Add nullable inputs: public_subnets_per_az_count, public_subnets_per_az_names, private_subnets_per_az_count, private_subnets_per_az_names, nat_gateway_public_subnet_indices, nat_gateway_public_subnet_names. Add nat_placement_conflict local and a null_resource plan‑time mutual‑exclusivity validation. Retain legacy subnets_per_az_* for compatibility; make subnets module depend on validation.
Provider Versions src/versions.tf	Bump AWS provider minimum to >= 5.0.0 (remove upper bound) and add required provider null (hashicorp/null >= 3.0).
Outputs src/outputs.tf	Add outputs: subnet ARNs and IPv6 CIDRs, public/private network ACL IDs, NAT-related outputs (EIP allocation IDs, NAT IDs/IPs, NAT AMI), AZ IDs, az-specific subnet/route-table maps, and named subnet stats maps.
Tests & Fixtures test/component_test.go, test/fixtures/stacks/catalog/*, test/fixtures/stacks/orgs/default/test/tests.yaml	Expand and refactor tests: add helpers (validateVPCProperties, setupS3Cleanup), suite runner TestRunVPCSuite, tests for NAT placement (index/name), separate subnet counts, validation conflict, endpoints/flow logs; add fixtures and include them in test imports.
Test Tooling & Go Modules test/go.mod	Bump Go toolchain to 1.25 and upgrade terratest, testify, aws-sdk-go-v2 and many indirect dependencies for test suite.
Misc / Manifest src/main.tf, src/outputs.tf	Add comments clarifying legacy vs new inputs, ensure subnets module depends on validation, and extend outputs/route-table maps.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Operator as Operator (terraform)
  participant Module as aws-vpc module
  participant Subnets as dynamic-subnets v3.0.1
  participant AWS as AWS API

  Operator->>Module: terraform plan/apply (per‑AZ public/private inputs & NAT placement)
  Module->>Module: validate nat placement (indices XOR names) %%#fff4cc
  alt validation fails
    Module-->>Operator: plan-time error (conflicting NAT placement) %%#ffdede
  else validation passes
    Module->>Subnets: call with public/private per‑AZ counts/names + nat placement args %%#e6f2ff
    alt NAT placement by index
      Subnets->>AWS: create NAT Gateways in subnets selected by index
    else NAT placement by name
      Subnets->>AWS: resolve named subnets and create NAT Gateways
    end
    Subnets->>AWS: create subnets, route tables, ACLs, endpoints
    Subnets-->>Module: return subnet IDs, maps, ARNs, NAT/EIP info
    Module-->>Operator: outputs (az_* maps, named_* stats, NAT EIPs, ACL IDs) %%#eaffea
  end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• Focus areas:
  • src/variables.tf / src/main.tf — new nullable inputs, validations, and plan‑time conflict detection.
  • src/outputs.tf — verify output shapes and consistency with dynamic-subnets v3.0.1.
  • test/component_test.go and fixtures — validate refactor, cleanup logic, and new scenarios (NAT by name/index, validation-conflict).
  • src/versions.tf / test/go.mod — provider and test dependency/tooling bumps.

Possibly related PRs

• Added tests #10 — related test-suite restructuring and helper additions touching tests and fixtures.
• chore: add tests for vpc flow logs #25 — related VPC Flow Logs testing and S3 cleanup/output changes.

Suggested reviewers

• mcalhoun
• goruha

Poem

🐰 I hopped through AZs where named subnets grow,
I nudged NATs by index or name in tidy row,
I gathered EIPs and ARNs into a stack,
Flow logs tucked in buckets, tests cleaned up the track,
Hooray for v3.0.1 — carrots for infra glow! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Update dynamic-subnets module to v3' clearly and concisely summarizes the main change: upgrading the terraform-aws-dynamic-subnets dependency to v3, which is the primary objective of this changeset.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch update-vpc-module

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

README.md (1)

12-31: All changes must be made to README.yaml, not directly to this generated file.

This file is auto-generated by atmos readme from README.yaml. The header clearly states not to edit this file directly. Per the coding guidelines and the file's own header comment, all content changes must be made to README.yaml, then regenerated.

Please ensure the following:

1. All content changes are made exclusively to README.yaml
2. The README.md file is regenerated by running atmos readme
3. Verify that the regenerated README.md matches the intended changes

If you've already made changes to README.yaml, please confirm that README.md is the output of regenerating from that source file.

🧹 Nitpick comments (2)

docs/prd/upgrade-to-dynamic-subnets-v3.md (2)

217-219: Optional: Replace hard tabs with spaces.

The code block contains hard tabs which may render inconsistently across different editors and platforms.

Consider replacing hard tabs with spaces for consistency with the project's 2-space indentation standard. As per coding guidelines.

---

736-739: Optional: Format bare URLs as Markdown links.

The references section contains bare URLs which could be formatted as proper Markdown links for better readability.

Consider formatting as:

-**Dynamic Subnets Module v3.0.0**: https://github.com/cloudposse/terraform-aws-dynamic-subnets/releases/tag/v3.0.0
+**Dynamic Subnets Module v3.0.0**: [Release v3.0.0](https://github.com/cloudposse/terraform-aws-dynamic-subnets/releases/tag/v3.0.0)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6dcfe89 and 61769ba.

⛔ Files ignored due to path filters (1)

• test/go.sum is excluded by !**/*.sum

📒 Files selected for processing (9)

• README.md (7 hunks)
• README.yaml (4 hunks)
• docs/prd/upgrade-to-dynamic-subnets-v3.md (1 hunks)
• src/README.md (5 hunks)
• src/main.tf (2 hunks)
• src/variables.tf (1 hunks)
• src/versions.tf (1 hunks)
• test/component_test.go (1 hunks)
• test/go.mod (3 hunks)

🧰 Additional context used

📓 Path-based instructions (6)

src/{main,variables,outputs,providers,versions,context}.tf

📄 CodeRabbit inference engine (AGENTS.md)

Keep the Terraform component as the single source of truth under src/ with files: main.tf, variables.tf, outputs.tf, providers.tf, versions.tf, context.tf

Files:

• src/variables.tf
• src/versions.tf
• src/main.tf

**/*.tf

📄 CodeRabbit inference engine (AGENTS.md)

**/*.tf: Use 2-space indentation for Terraform files
In Terraform, prefer lower_snake_case for variables and locals; keep resource and data source names descriptive and aligned with Cloud Posse null-label patterns
Run terraform fmt and do not commit formatting violations

Files:

• src/variables.tf
• src/versions.tf
• src/main.tf

{README.yaml,src/{variables,outputs}.tf}

📄 CodeRabbit inference engine (AGENTS.md)

When Terraform inputs/outputs change, update README.yaml and regenerate READMEs

Files:

• src/variables.tf
• README.yaml

{README.yaml,README.md,src/README.md}

📄 CodeRabbit inference engine (AGENTS.md)

Generate README.md (and src/README.md) from README.yaml via atmos/terraform-docs; do not edit generated READMEs manually

Files:

• README.md
• src/README.md
• README.yaml

**/*.{yml,yaml,md}

📄 CodeRabbit inference engine (AGENTS.md)

Use 2-space indentation for YAML and Markdown files

Files:

• README.md
• src/README.md
• README.yaml
• docs/prd/upgrade-to-dynamic-subnets-v3.md

test/**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

Place Go Terratest files under test/ and name them *_test.go using cloudposse/test-helpers and atmos fixtures

Files:

• test/component_test.go

🪛 GitHub Actions: Branch

src/main.tf

[error] 160-160: Terraform configuration formatting changed: updated lines for subnets_per_az_* and NAT gateway var assignments in module 'subnets'. Run 'terraform fmt' to format the code consistently.

🪛 LanguageTool

docs/prd/upgrade-to-dynamic-subnets-v3.md

[grammar] ~271-~271: Use a hyphen to join words.
Context: ...nfiguration than using indices - Easy to understand NAT placement strategy ### E...

(QB_NEW_EN_HYPHEN)

🪛 markdownlint-cli2 (0.18.1)

docs/prd/upgrade-to-dynamic-subnets-v3.md

217-217: Hard tabs
Column: 1

(MD010, no-hard-tabs)

---

218-218: Hard tabs
Column: 1

(MD010, no-hard-tabs)

---

219-219: Hard tabs
Column: 1

(MD010, no-hard-tabs)

---

736-736: Bare URL used

(MD034, no-bare-urls)

---

738-738: Bare URL used

(MD034, no-bare-urls)

---

739-739: Bare URL used

(MD034, no-bare-urls)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Summary

🔇 Additional comments (13)

src/versions.tf (1)

7-7: LGTM! Provider version constraint properly updated.

The AWS provider constraint update from ">= 4.9.0, < 6.0.0" to ">= 5.0.0" correctly aligns with the dynamic-subnets v3.0.0 requirements and removes the upper bound to support AWS Provider v6.x.

README.yaml (1)

5-284: Excellent documentation! Comprehensive coverage of v3.0.0 features.

The README.yaml updates provide thorough documentation of the new capabilities with practical examples covering:

• Cost-optimized NAT placement with real savings calculations
• Named NAT gateway placement strategies
• High-availability NAT configurations
• Separate public/private subnet architectures
• VPC endpoints configuration
• Complete production examples

The documentation strikes a good balance between feature explanation and practical guidance.

test/go.mod (2)

10-127: LGTM! Test dependency updates are appropriate.

The updates to Terratest (v0.52.0) and testify (v1.11.1) align well with the AWS provider v5.x+ requirements and provide improved test reliability.

---

3-5: Go 1.25 is the current stable version.

Go 1.25.3 is the latest stable version as of November 2025, making the upgrade appropriate and timely. The code in test/go.mod correctly targets Go 1.25 with a valid toolchain specification.

docs/prd/upgrade-to-dynamic-subnets-v3.md (1)

1-741: Outstanding PRD documentation!

This comprehensive Product Requirements Document provides excellent coverage of the v3.0.0 upgrade including:

• Clear migration guide with backward compatibility guarantees
• Practical usage examples for all new features
• Detailed test improvements and recommendations
• Cost implications with real-world calculations
• Security considerations and rollback plan

The document will be valuable for both implementers and maintainers.

src/README.md (1)

1-390: LGTM! Generated documentation is comprehensive and correct.

The generated README accurately reflects the v3.0.0 features with:

• Clear feature descriptions and usage patterns
• Correct module/provider version requirements
• Complete inputs/outputs documentation including the 6 new variables
• Practical configuration examples

As per coding guidelines, this file is correctly generated from README.yaml.

src/variables.tf (1)

272-342: Excellent variable design! Backward compatibility properly maintained.

The six new variables are well-designed with:

• Clear, comprehensive descriptions explaining their purpose and usage
• Proper backward compatibility through default = null and nullable = true
• Explicit documentation of conflicts between index-based and name-based NAT placement
• Consistent naming conventions and organization

The variables enable all the new v3.0.0 features while maintaining 100% backward compatibility with existing configurations.

src/main.tf (1)

140-177: LGTM! Module upgrade properly implemented with backward compatibility.

The changes correctly:

• Upgrade the subnets module to v3.0.0
• Pass all six new variables to the module
• Retain legacy variables (subnets_per_az_count, subnets_per_az_names) with clear comments
• Maintain backward compatibility through the variable defaulting mechanism

The implementation follows best practices by clearly documenting which variables are legacy vs new, making future maintenance easier.

test/component_test.go (4)

17-23: Excellent refactoring! Centralized constants improve maintainability.

The introduction of centralized test constants eliminates magic strings and provides a single source of truth for test configuration values.

---

29-56: Outstanding helper functions! Significantly improve test quality.

The new helper functions provide excellent benefits:

• validateVPCProperties: Eliminates ~20+ lines of duplicated assertion code across tests
• setupS3Cleanup: Fixes critical cleanup order bug by using t.Cleanup() to ensure S3 bucket is emptied before component destruction
• Both properly use t.Helper() for accurate error reporting

These improvements enhance test reliability and maintainability.

---

59-279: LGTM! Test refactoring follows Go best practices.

All test functions have been improved with:

• Consistent use of helper functions reducing duplication
• Descriptive assertion messages for easier debugging
• Proper use of require vs assert (require for fatal errors, assert for non-fatal)
• Clear test structure and organization
• Proper cleanup sequencing via the new setupS3Cleanup helper

The tests maintain the same coverage while being more maintainable and reliable.

---

282-290: Good addition! Test suite runner properly configured.

The TestRunVPCSuite function correctly:

• Registers the VPC flow logs bucket dependency
• Initializes and runs the test suite

This ensures proper test execution order and dependency management.

README.md (1)

33-49: Content review (pending proper workflow):

Once this file is properly regenerated from README.yaml, the content changes look good:

• Clear documentation of new v3.0.0 features (independent public/private subnet counts, flexible NAT placement)
• Well-organized examples progressing from basic to production-ready configurations
• Cost optimization examples clearly demonstrate the value proposition (~$135/month savings with concrete numbers)
• Accurate reflection of AWS Provider requirement change to >=5.0.0
• New input variables (public_subnets_per_az_count/names, private_subnets_per_az_count/names, nat_gateway_public_subnet_indices/names) properly documented
• Examples maintain backward compatibility by showing legacy configuration still works

However, this review is conditional on the file being regenerated via the proper atmos readme workflow from README.yaml.

Also applies to: 71-71, 73-432

[aknysh]

@coderabbitai please full review.
related PR cloudposse/terraform-aws-dynamic-subnets#226

[coderabbitai]

🧩 Analysis chain

🌐 Web query:

terraform-aws-dynamic-subnets pull request 226

💡 Result:

I found Pull Request #226 on the cloudposse/terraform-aws-dynamic-subnets repository:

• Title: "Separate Public/Private Subnet Configuration and Enhance NAT Gateway Placement". [1]
• Author / Merger: aknysh merged 20 commits into main. [1]
• Merged: Nov 2, 2025. [1]
• Key changes: separate controls for public vs private subnet counts/names, controlled NAT Gateway placement by index and by subnet name, bug fixes for NAT AZ distribution and cross‑AZ routing, examples and Terratest coverage, maintained backward compatibility. [1]

Source: GitHub PR page. [1]

References
[1] cloudposse/terraform-aws-dynamic-subnets — Pull Request #226 (PR details).

---

🏁 Script executed:

#!/bin/bash

# First, let's examine the main.tf changes to understand the module integration
echo "=== Examining src/main.tf ==="
cat src/main.tf

echo -e "\n=== Examining src/variables.tf new variables ==="
cat src/variables.tf | grep -A 20 "public_subnets_per_az_count\|private_subnets_per_az_count\|nat_gateway_public_subnet"

echo -e "\n=== Examining src/versions.tf ==="
cat src/versions.tf

echo -e "\n=== Checking if there are any outputs changes ==="
if [ -f src/outputs.tf ]; then
  cat src/outputs.tf | head -100
fi

Length of output: 13064

---

🏁 Script executed:

#!/bin/bash

# Continue examining outputs and check the rest of outputs.tf
echo "=== Examining rest of src/outputs.tf ==="
cat src/outputs.tf | tail -100

echo -e "\n=== Checking test changes - looking at test constants and helpers ==="
cat test/component_test.go | grep -A 10 "type ComponentSuite\|func (s \*ComponentSuite) validateVPCProperties\|func (s \*ComponentSuite) setupS3Cleanup"

echo -e "\n=== Checking example test scenarios ==="
grep -n "func (s \*ComponentSuite) Test" test/component_test.go | head -20

echo -e "\n=== Check if there are any breaking changes in variable definitions ==="
cat src/variables.tf | grep -B 2 "subnets_per_az_count\|subnets_per_az_names" | head -40

Length of output: 6487

---

🏁 Script executed:

#!/bin/bash

# Check the PRD document for migration details
echo "=== Checking PRD for upgrade details ==="
if [ -f docs/prd/upgrade-to-dynamic-subnets-v3.md ]; then
  cat docs/prd/upgrade-to-dynamic-subnets-v3.md | head -150
fi

echo -e "\n=== Checking for any potential issues with provider version constraints ==="
# Check if there are any other provider version references
rg "version.*=.*['\"].*5\." src/ --type tf -C2

echo -e "\n=== Check NAT Gateway related outputs ==="
cat src/outputs.tf | grep -i "nat" -A 3 -B 1

echo -e "\n=== Verify the legacy variables are still present and not modified ==="
cat src/variables.tf | grep -A 15 "^variable \"subnets_per_az_count\""
cat src/variables.tf | grep -A 15 "^variable \"subnets_per_az_names\""

Length of output: 8626

---

🏁 Script executed:

#!/bin/bash

# Check if there are validation rules for conflicting NAT variables
echo "=== Checking for variable validation rules ==="
rg "validation" src/variables.tf -A 5 | grep -A 5 -B 5 "nat_gateway"

# Verify the test file structure and test count
echo -e "\n=== Checking test structure and coverage ==="
grep "func (s \*ComponentSuite) Test" test/component_test.go

# Check Go version - verify if 1.25 is correct or a typo
echo -e "\n=== Verifying Go version in test/go.mod ==="
head -5 test/go.mod

# Check if there's any changelog or release notes
echo -e "\n=== Looking for CHANGELOG or release notes ==="
fd -t f "CHANGELOG|RELEASE" . 2>/dev/null | head -5

# Verify module version consistency
echo -e "\n=== Checking all module version references ==="
rg "cloudposse/dynamic-subnets/aws" src/ --type tf

# Check for any hardcoded version numbers in docs
echo -e "\n=== Checking documentation for version references ==="
rg "2\.4\.2|v2\.4\.2|3\.0\.0|v3\.0\.0" README.md | head -10

Length of output: 1625

---

🏁 Script executed:

#!/bin/bash

# Check if Go 1.25 exists - this seems suspicious
echo "=== Checking if Go 1.25 is valid ==="
# Let's check what the actual latest Go version is mentioned in the go.sum
head -20 test/go.sum

# Check the original go.mod to see if this is an intentional upgrade
echo -e "\n=== Checking git diff for go.mod to see the change ==="
git diff HEAD~1 test/go.mod 2>/dev/null | head -30 || echo "Cannot access git history in sandbox"

# Look for any validation or error handling for mutual exclusivity of NAT placement variables
echo -e "\n=== Searching for validation in the dynamic-subnets module call ==="
cat src/main.tf | grep -A 80 "module \"subnets\""

# Check if there are any examples or test files that use the new variables
echo -e "\n=== Looking for usage of new variables in examples ==="
fd -t f -e yaml -e yml examples/ 2>/dev/null | xargs grep -l "public_subnets_per_az\|nat_gateway_public_subnet" 2>/dev/null || echo "No examples directory found"

# Check the README.yaml for examples
echo -e "\n=== Checking README.yaml for new variable usage ==="
grep -A 5 "nat_gateway_public_subnet_indices\|public_subnets_per_az_count" README.yaml | head -20

Length of output: 5368

---

🏁 Script executed:

#!/bin/bash

# Check what tests were added or modified for the new features
echo "=== Looking at test file differences to understand new test coverage ==="
cat test/component_test.go | grep -B 5 -A 20 "public_subnets_per_az\|private_subnets_per_az\|nat_gateway_public_subnet"

# Check if there's a stacks directory with test configurations
echo -e "\n=== Looking for stack configurations ==="
fd -t f -e yaml stacks/ 2>/dev/null | head -10

# Check the actual test stack files
echo -e "\n=== Checking test stack configurations ==="
if [ -d "test/stacks" ]; then
  find test/stacks -name "*.yaml" | head -5 | xargs ls -la
fi

# Let's verify the AWS provider version constraint is correct
echo -e "\n=== Checking if removing upper bound on AWS provider is intentional ==="
cat docs/prd/upgrade-to-dynamic-subnets-v3.md | grep -i "aws provider" -A 3 -B 3

# Check if there are any example configurations in the test stacks
echo -e "\n=== Looking at test catalog vpc configuration ==="
cat test/stacks/catalog/vpc/defaults.yaml 2>/dev/null || echo "File not found"

Length of output: 2003

---

🏁 Script executed:

#!/bin/bash

# Check actual Go releases to verify if 1.25 is valid
echo "=== Checking if there's a comment about Go version ==="
cat test/go.mod

# Look for any TODO or FIXME comments in the code
echo -e "\n=== Checking for TODO/FIXME comments ==="
rg "TODO|FIXME" src/ test/ --type go --type tf -C 2

# Check if the PRD mentions the Go version specifically
echo -e "\n=== Checking PRD for Go version justification ==="
cat docs/prd/upgrade-to-dynamic-subnets-v3.md | grep -i "go 1.2" -A 3 -B 3

# Verify what terraform version constraints are in place
echo -e "\n=== Checking Terraform version requirements ==="
cat src/versions.tf

# Check if there are any breaking change notes
echo -e "\n=== Looking for breaking change documentation ==="
cat docs/prd/upgrade-to-dynamic-subnets-v3.md | grep -i "breaking\|backward compat" -A 3 -B 1

# Check if tests actually test the new NAT placement features
echo -e "\n=== Checking if tests cover NAT Gateway placement ==="
rg "nat_gateway_public_subnet" test/ --type go -C 3

Length of output: 9152

---

@aknysh, I've completed a comprehensive review of PR #70. Here's my detailed analysis:

🔴 Critical Issues

1. Invalid Go VersionMy apologies - I was wrong! Go 1.25 was released in August 2025, so the version is actually correct. The upgrade from Go 1.23 to 1.25 is valid.

2. AWS Provider Version - BREAKING CHANGE ⚠️

The change from >= 4.9.0, < 6.0.0 to >= 5.0.0 IS a breaking change and contradicts the "100% backward compatible" claim in the PR description:

• ❌ Drops AWS Provider v4.x support - Users currently on AWS Provider v4.9.0 - v4.99.x cannot upgrade
• ⚠️ Removes upper bound - Now allows v5.x, v6.x, v7.x, v8.x... indefinitely, which could cause issues with future breaking changes
• 📝 The PRD states: "Support for AWS Provider v6.x" but the constraint >= 5.0.0 has no upper bound

Recommendation: Either:

• Change to >= 5.0.0, < 7.0.0 to match CloudPosse's typical pattern
• Update the PR description and documentation to clearly state this is a breaking change requiring AWS Provider v5.0+

3. Missing Variable Validation

The variables nat_gateway_public_subnet_indices and nat_gateway_public_subnet_names are documented as mutually exclusive, but there's no Terraform validation to enforce this:

variable "nat_gateway_public_subnet_indices" {
  description = "...Conflicts with `nat_gateway_public_subnet_names`..."
  # MISSING: validation block to ensure mutual exclusivity
}

Recommendation: Add validation:

variable "nat_gateway_public_subnet_indices" {
  type        = list(number)
  description = <<-EOT
    Indices (0-based) of public subnets where NAT Gateways should be placed.
    Use this for index-based NAT Gateway placement (e.g., [0, 1] to place NATs in first 2 public subnets per AZ).
    Conflicts with `nat_gateway_public_subnet_names`.
    If both are null, NAT Gateways are placed in all public subnets by default.
  EOT
  default     = null
  nullable    = true
  
  validation {
    condition     = var.nat_gateway_public_subnet_indices == null || var.nat_gateway_public_subnet_names == null
    error_message = "Cannot specify both nat_gateway_public_subnet_indices and nat_gateway_public_subnet_names. Choose one placement method."
  }
}

⚠️ Missing Test Coverage

I found no test coverage for the new NAT Gateway placement features:

• No tests using nat_gateway_public_subnet_indices
• No tests using nat_gateway_public_subnet_names
• No tests verifying separate public_subnets_per_az_count / private_subnets_per_az_count

Recommendation: Add test cases in test/component_test.go:

• Test NAT placement by index
• Test NAT placement by name
• Test separate public/private subnet counts
• Test mutual exclusivity validation (should fail gracefully)

✅ Strengths

1. Excellent Documentation: The PRD, README updates, and usage examples are comprehensive and well-written
2. Proper Backward Compatibility (Variables): All new variables default to null with proper fallback logic
3. Clean Module Integration: Module call properly passes all new and legacy variables
4. Test Refactoring: Good helper functions (validateVPCProperties, setupS3Cleanup) improve test maintainability
5. Enhanced Outputs: New NAT Gateway ID outputs are valuable additions

📝 Minor Issues

1. Outputs Documentation: The new outputs named_private_subnets_stats_map and named_public_subnets_stats_map now include NAT Gateway IDs. Consider adding an example showing how to use these new fields.

2. Cost Calculation: The README claims "$135/month savings" but this assumes specific NAT Gateway pricing. Consider adding a note that pricing varies by region.

3. Module Version Consistency: Double-check that all references to cloudposse/dynamic-subnets/aws use version 3.0.0 consistently.

🎯 Summary

This is a well-executed upgrade with excellent documentation, but it has one critical breaking change that needs to be addressed:

Must Fix Before Merge:

1. ❌ AWS Provider constraint breaking change - Either restore upper bound or document as breaking
2. ⚠️ Add validation for mutually exclusive NAT variables
3. ⚠️ Add test coverage for new features

Recommendations:

• Update PR description to clarify AWS Provider v5.0+ requirement (breaking change)
• Consider changing version constraint to >= 5.0.0, < 7.0.0
• Add Terraform validation for mutual exclusivity
• Add integration tests for NAT placement features
• Add usage examples for new output fields

Overall Assessment: High-quality work with thorough documentation, but the AWS Provider constraint change is a breaking change that conflicts with the "100% backward compatible" claim. This needs to be either fixed or properly documented.

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fd35423 and f575601.

📒 Files selected for processing (3)

• README.md (8 hunks)
• docs/prd/upgrade-to-dynamic-subnets-v3.md (1 hunks)
• src/README.md (6 hunks)

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

• src/README.md

🧰 Additional context used

📓 Path-based instructions (2)

**/*.{yml,yaml,md}

📄 CodeRabbit inference engine (AGENTS.md)

Use 2-space indentation for YAML and Markdown files

Files:

• docs/prd/upgrade-to-dynamic-subnets-v3.md
• README.md

{README.yaml,README.md,src/README.md}

📄 CodeRabbit inference engine (AGENTS.md)

Generate README.md (and src/README.md) from README.yaml via atmos/terraform-docs; do not edit generated READMEs manually

Files:

• README.md

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Summary

🔇 Additional comments (8)

docs/prd/upgrade-to-dynamic-subnets-v3.md (5)

607-621: Note that variable conflict is documented but may not be enforced in code.

Per the PR objectives, the mutual exclusivity of nat_gateway_public_subnet_indices and nat_gateway_public_subnet_names is documented in this PRD (lines 613–621) but lacks Terraform validation enforcement in the code (src/variables.tf, src/main.tf). The documentation correctly describes the constraint, but users may encounter runtime failures rather than plan-time validation errors. Consider adding a note that this is currently a documentation-only constraint pending code-level validation.

---

683-697: Cost savings example is clear and well-documented.

The cost example includes appropriate caveats (line 697: "Actual costs vary by region and usage") and pricing methodology is transparent. The math appears sound and the format is helpful for decision-making.

---

230-309: Usage examples are clear, well-structured, and demonstrate both legacy and new features.

The progression from cost-optimized, to named placement, to HA, to legacy configuration, and finally to a complete production example effectively educates users on the upgrade path and new capabilities. Examples correctly show both old (subnets_per_az_count) and new (public/private_subnets_per_az_*) variable usage.

---

3-5: Verify PRD status is accurate for review context.

The PRD header shows Status: "Implemented" (line 5), but per PR #70 status, the PR is still "open" and awaiting review/merge. If this PRD was finalized before the review, consider updating to reflect that the merge is still pending (status could be "In Review" or "Pending Merge").

---

19-20: Remove or substantially revise this review comment — it contains factual inaccuracies and references a non-existent PR objectives document.

The review identifies three problems with evidence issues:

1. Line number error: The backward compatibility claim is at line 593, not line 591.
2. Missing PR objectives document: The review references PR objectives that "require fix to restore upper bound or document as breaking," but no such document exists in the repository. This assertion cannot be verified.
3. Mischaracterization of backward compatibility claim: The "100% backward compatible" statement (line 593) specifically addresses variable and configuration compatibility, not provider versions. The PRD explicitly states at line 203 that "Dynamic-subnets v3.0.0 requires AWS Provider v5.0+."

While the PRD could arguably be clearer about the scope of backward compatibility or the implications of removing v4.x support, the review comment's basis for concern relies on non-existent documentation and contains verifiable errors.

Likely an incorrect or invalid review comment.

README.md (3)

352-361: Input documentation is complete but validation enforcement is missing.

The new input variables are well-documented with descriptions and types:

• Lines 352–353: nat_gateway_public_subnet_indices and nat_gateway_public_subnet_names (conflict documented)
• Lines 357–358: private_subnets_per_az_count and private_subnets_per_az_names
• Lines 360–361: public_subnets_per_az_count and public_subnets_per_az_names

However, per PR objectives, the mutual exclusivity constraint for NAT variables is documented but not enforced via Terraform validation blocks in src/variables.tf. This means users will encounter runtime errors rather than plan-time validation. The documentation is accurate, but the code gap remains.

---

381-432: Output documentation is comprehensive and includes new NAT Gateway details.

New outputs for NAT Gateway IDs (lines 397–398, 401, 407) and per-AZ subnet/route table maps (lines 385–390) are well-documented. Descriptions clearly indicate what each output provides (e.g., line 397–398 explicitly mention NAT Gateway ID presence in stats maps). No issues identified.

---

33-49: Usage examples are clear, comprehensive, and progressively educate users.

The README provides four complementary examples:

1. Basic/legacy configuration (lines 73–122)
2. Cost-optimized NAT (lines 124–149)
3. Named NAT placement (lines 151–165)
4. HA configuration (lines 167–179)
5. Separate public/private architecture (lines 180–200)
6. Complete production example (lines 224–268)

Examples correctly demonstrate the new per-AZ and NAT placement features and include cost-savings guidance. The progression from simple to complex is pedagogically sound.

Also applies to: 73-200, 224-268

[aknysh]

@coderabbitai please full review

[coderabbitai]

@aknysh, I'll perform a full review of all the changes in this PR.

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

docs/prd/upgrade-to-dynamic-subnets-v3.md (1)

628-653: Terraform validation blocks are not implemented, but runtime module validation is in place.

The variables nat_gateway_public_subnet_indices and nat_gateway_public_subnet_names (src/variables.tf, lines 320–342) lack Terraform validation blocks to enforce mutual exclusivity at plan time. However, runtime module validation catches conflicts at apply time, and the limitation is documented in the PRD as "Expected behavior."

Validation blocks do exist elsewhere in variables.tf (e.g., for ipv4_cidrs and subnets_per_az_count), establishing a precedent in the codebase. Adding similar validation blocks to enforce mutual exclusivity would move error detection earlier in the workflow (plan vs. apply) and improve developer experience. No explicit PR requirement for this validation was found in the PRD.

Recommendation: Consider adding validation blocks to nat_gateway_public_subnet_indices and nat_gateway_public_subnet_names as a future enhancement to align with the codebase pattern, but this is not blocking—the current runtime validation approach is functional and adequately documented.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f575601 and 91c1f2c.

📒 Files selected for processing (4)

• README.md (8 hunks)
• README.yaml (4 hunks)
• docs/prd/upgrade-to-dynamic-subnets-v3.md (1 hunks)
• src/README.md (6 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

{README.yaml,README.md,src/README.md}

📄 CodeRabbit inference engine (AGENTS.md)

Generate README.md (and src/README.md) from README.yaml via atmos/terraform-docs; do not edit generated READMEs manually

Files:

• README.md
• src/README.md
• README.yaml

**/*.{yml,yaml,md}

📄 CodeRabbit inference engine (AGENTS.md)

Use 2-space indentation for YAML and Markdown files

Files:

• README.md
• src/README.md
• docs/prd/upgrade-to-dynamic-subnets-v3.md
• README.yaml

{README.yaml,src/{variables,outputs}.tf}

📄 CodeRabbit inference engine (AGENTS.md)

When Terraform inputs/outputs change, update README.yaml and regenerate READMEs

Files:

• README.yaml

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Summary

🔇 Additional comments (10)

README.yaml (2)

5-48: Breaking change documentation is clear and well-structured.

The README.yaml effectively communicates the v3.0.0 upgrade with prominent breaking-change warnings, migration guidance, and comprehensive usage examples. The AWS Provider v5.0+ requirement is clearly stated at line 22 and detailed in lines 29–39 with explicit migration steps for v4.x users. Documentation quality is solid.

---

1-290: Indentation and structure comply with coding guidelines.

YAML indentation follows the 2-space convention per coding guidelines. File structure is well-organized with clear sections: description, usage, examples, references, and metadata.

README.md (3)

33-49: Breaking change prominently documented in auto-generated README.

The README.md correctly reflects the README.yaml source, with clear communication of AWS Provider v5.0+ requirement and v4.x breaking change. This appears in three places: feature summary (lines 33–49), dedicated breaking-change section (lines 73–83), and requirements table (line 301). Migration path is explicit.

---

301-307: Generated documentation accurately reflects module and provider versions.

Requirements table (line 301), Providers section (line 307), and Modules table (line 315) all correctly show AWS Provider ">= 5.0.0" and dynamic-subnets "3.0.0". Content is consistent with source.

Also applies to: 315-315

---

12-31: Auto-generated file notice is clear.

The "DO NOT EDIT THIS FILE" comment (lines 12–31) is comprehensive and directs users to edit README.yaml and regenerate via atmos/terraform-docs. This aligns with coding guidelines.

docs/prd/upgrade-to-dynamic-subnets-v3.md (3)

591-624: Breaking change documentation is comprehensive and well-qualified.

The Backward Compatibility section effectively separates the breaking AWS Provider change (lines 591–610) from configuration compatibility (lines 611–624). Clear distinction is made between:

• ⚠️ Breaking change: AWS Provider v5.0+ required (v4.x users must upgrade first)
• ✅ Configuration compatible: Terraform configs work unchanged on AWS Provider v5.0+

Migration path is explicit (lines 600–604), and rationale is provided (lines 606–609).

---

656-684: Success Criteria clearly marked and documented.

Lines 656–670 list completed criteria with ✅ checkmarks, including breaking change clearly marked as "⚠️ BREAKING CHANGE: Now requires AWS Provider v5.0+ (drops v4.x support)." Future Enhancements (lines 671–684) are appropriately separated but should be re-categorized per the test coverage concern (see prior comment).

Ensure that tests listed in "Future Enhancements" (lines 671–684) are moved to "In Progress" or marked as blocking once the test coverage requirement is clarified.

---

472-545: The review comment is incorrect. The file correctly documents tests as recommended but not blocking for merge.

The original review comment asserts that PR objectives require test coverage as a mandatory merge condition. However, the verification reveals:

1. Deliberate Design Choice: Commit 91c1f2c (Nov 2, 2025) explicitly changed the status to "Recommended but not blocking for merge" (line 474). The commit message states this was to "clarify recommendations vs requirements."

2. Success Criteria Alignment: The Success Criteria section (lines 656-686) lists 10 completed items (marked ✅) without test coverage. The recommended tests appear under "Future Enhancements" as unchecked items—not as merge blockers.

3. No PR Objective Stating Tests Are Required: Searching the file reveals only AWS Provider v5.0+ as a documented requirement. No section establishes test coverage as a prerequisite for merge.

4. Consistent Documentation: The file maintains clear separation between:

   • Current Coverage: 5 implemented tests (✅)
   • Recommended Coverage: Tests marked as "not blocking for merge" (lines 472–545)
   • Future Work: Same tests listed in Future Enhancements section

The code and documentation are correctly aligned with the author's intended design.

Likely an incorrect or invalid review comment.

src/README.md (2)

1-31: YAML frontmatter and description align with coding guidelines and documentation standards.

Lines 1–31 follow 2-space YAML indentation. Breaking change is documented prominently at lines 38–48, mirroring README.md. Component description and features (lines 15–31) are clear and comprehensive.

---

254-268: Generated component README correctly reflects module and provider versions.

Requirements and Modules tables (lines 254–268) show AWS Provider ">= 5.0.0" and dynamic-subnets "3.0.0". Content is consistent with README.md and README.yaml.

[aknysh]

@coderabbitai full review please, check if all your comments have been addressed

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

echo "=== 1. Check current AWS Provider constraint in src/versions.tf ==="
cat src/versions.tf

echo -e "\n=== 2. Check for validation blocks in src/variables.tf for NAT variables ==="
cat src/variables.tf | grep -A 20 "variable \"nat_gateway_public_subnet_indices\"" | head -30
cat src/variables.tf | grep -A 20 "variable \"nat_gateway_public_subnet_names\"" | head -30

echo -e "\n=== 3. Check Go version in test/go.mod ==="
head -5 test/go.mod

echo -e "\n=== 4. Search for new test cases in test/component_test.go ==="
grep "func (s \*ComponentSuite) Test" test/component_test.go

echo -e "\n=== 5. Check if tests use new NAT placement variables ==="
rg "nat_gateway_public_subnet_indices|nat_gateway_public_subnet_names" test/ --type go -C 2

echo -e "\n=== 6. Check if tests use separate public/private subnet counts ==="
rg "public_subnets_per_az_count|private_subnets_per_az_count" test/ --type yaml -C 2

Length of output: 2909

---

🌐 Web query:

Go programming language 1.25 release date

💡 Result:

Go 1.25.0 was released on August 12, 2025. [1][2]

Sources:
[1] Go Release History (go1.25.0 released 2025-08-12).
[2] Go 1.25 Release Notes (August 2025).

---

🏁 Script executed:

#!/bin/bash

echo "=== Search for mutual exclusivity validation anywhere in the codebase ==="
rg "nat_gateway_public_subnet_indices.*nat_gateway_public_subnet_names|nat_gateway_public_subnet_names.*nat_gateway_public_subnet_indices" src/ --type tf -C 3

echo -e "\n=== Check for precondition blocks in main.tf ==="
rg "precondition" src/main.tf -C 5

echo -e "\n=== Check the full variables.tf for any cross-variable validation ==="
cat src/variables.tf | grep -B 5 -A 15 "validation" | grep -A 15 -B 5 "nat_gateway"

echo -e "\n=== Look for any lifecycle blocks or checks ==="
rg "lifecycle|check" src/main.tf -C 3

Length of output: 2734

---

🏁 Script executed:

#!/bin/bash

echo "=== Check local.nat_placement_conflict definition in main.tf ==="
cat src/main.tf | grep -B 10 -A 5 "nat_placement_conflict"

echo -e "\n=== Check all locals in main.tf ==="
cat src/main.tf | grep -A 30 "locals {"

echo -e "\n=== Verify the check block is properly formatted ==="
cat src/main.tf | grep -B 2 -A 8 "check \"nat_placement"

Length of output: 3035

---

@aknysh, I've completed a thorough review to verify the status of all previous comments. Here's what I found:

✅ Issues That Were Addressed

1. Mutual Exclusivity Validation - FIXED ✓

Excellent work! The mutual exclusivity validation was properly implemented using a Terraform check block in src/main.tf:

locals {
  nat_placement_conflict = (
    var.nat_gateway_public_subnet_indices != null &&
    var.nat_gateway_public_subnet_names != null
  )
}

check "nat_placement_mutual_exclusivity" {
  assert {
    condition     = !local.nat_placement_conflict
    error_message = "Cannot specify both nat_gateway_public_subnet_indices and nat_gateway_public_subnet_names. Choose one NAT placement method or leave both null for default behavior (NAT in all public subnets)."
  }
}

This is actually a better approach than variable-level validation since it checks at plan time and provides clear error messaging.

2. Go Version 1.25 - No Issue ✓

My initial concern was incorrect. Go 1.25.0 is valid (released August 12, 2025).

❌ Critical Issues Still Unresolved

1. AWS Provider Constraint - Breaking Change Not Addressed

Status: The constraint remains >= 5.0.0 with no upper bound.

Impact:

• ❌ Drops support for AWS Provider v4.x (breaking change for users on v4.9.0 - v4.99.x)
• ⚠️ No upper bound means v5.x, v6.x, v7.x, v8.x... are all allowed indefinitely
• This contradicts typical CloudPosse patterns like >= 5.0.0, < 7.0.0

Recommendation: Either:

• Add upper bound: version = ">= 5.0.0, < 7.0.0"
• OR clearly document in PR description and README that AWS Provider v5.0+ is required (breaking change)

2. Missing Test Coverage - Not Addressed

Status: No test coverage found for:

• ❌ nat_gateway_public_subnet_indices (index-based NAT placement)
• ❌ nat_gateway_public_subnet_names (name-based NAT placement)
• ❌ public_subnets_per_az_count / private_subnets_per_az_count (separate subnet counts)
• ❌ Mutual exclusivity validation behavior (should fail when both NAT vars are set)

Current tests:

func (s *ComponentSuite) TestPrivateVPC()
func (s *ComponentSuite) TestPublicVPC()
func (s *ComponentSuite) TestVPCFlowLogs()
func (s *ComponentSuite) TestVPCWithEndpoints()
func (s *ComponentSuite) TestEnabledFlag()

Recommendation: Add test cases to validate:

1. NAT placement by index (e.g., nat_gateway_public_subnet_indices = [0])
2. NAT placement by name (e.g., nat_gateway_public_subnet_names = ["public"])
3. Separate public/private subnet counts
4. Validation failure when both NAT placement methods are specified

📊 Summary

Progress: 2/4 critical issues resolved

Issue	Status	Notes
Mutual exclusivity validation	✅ Fixed	Check block added in main.tf
Go 1.25 version	✅ Valid	Released August 2025
AWS Provider constraint	❌ Not fixed	Still breaking change, no upper bound
Test coverage	❌ Not added	New features untested

🎯 Remaining Action Items

Before merge, please address:

1. AWS Provider Version Constraint (Critical)

   • Add upper bound: >= 5.0.0, < 7.0.0
   • OR document this as a breaking change requiring AWS Provider v5.0+

2. Test Coverage (Important)

   • Add integration tests for NAT placement features
   • Add test for separate public/private subnet configurations
   • Add test verifying mutual exclusivity validation fails appropriately

Would you like me to help generate test case examples for the new features?

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

docs/prd/upgrade-to-dynamic-subnets-v3.md (1)

689-703: Clarify test coverage status in "Future Enhancements" section.

Lines 689–703 list test cases (TestSeparateSubnetCounts, TestNATPlacementByIndex, TestNATPlacementByName, TestNATGatewayIDOutputs) as "Future Enhancements." Given the PR objectives note that test coverage for new features is currently absent and should be addressed, consider clarifying whether these are:

1. Required before merge → Move to "Completed" or "In Progress" section, or
2. Truly deferred → Add a rationale explaining why new feature coverage is acceptable to defer for this release.

This is related to the prior test coverage comment and may be consolidated.

🧹 Nitpick comments (1)

src/versions.tf (1)

7-7: Clarify rationale for removing AWS Provider upper bound.

The constraint changed from ">= 4.9.0, < 6.0.0" to ">= 5.0.0", removing the upper bound entirely. This allows any future AWS Provider major version without validation. Consider either:

1. Restoring an upper bound (e.g., ">= 5.0.0, < 7.0.0") to safeguard against unvalidated major version incompatibilities, or
2. Explicitly documenting that future major versions are untested and users should validate compatibility independently.

The breaking change to require v5.0+ is well-documented; this note is about future-proofing the constraint.

Would you like me to suggest a versioning strategy that balances flexibility with safety?

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6dcfe89 and aed76d9.

⛔ Files ignored due to path filters (1)

• test/go.sum is excluded by !**/*.sum

📒 Files selected for processing (10)

• README.md (8 hunks)
• README.yaml (4 hunks)
• docs/prd/upgrade-to-dynamic-subnets-v3.md (1 hunks)
• src/README.md (6 hunks)
• src/main.tf (4 hunks)
• src/outputs.tf (5 hunks)
• src/variables.tf (1 hunks)
• src/versions.tf (1 hunks)
• test/component_test.go (1 hunks)
• test/go.mod (3 hunks)

🧰 Additional context used

📓 Path-based instructions (6)

{README.yaml,README.md,src/README.md}

📄 CodeRabbit inference engine (AGENTS.md)

Generate README.md (and src/README.md) from README.yaml via atmos/terraform-docs; do not edit generated READMEs manually

Files:

• src/README.md
• README.yaml
• README.md

**/*.{yml,yaml,md}

📄 CodeRabbit inference engine (AGENTS.md)

Use 2-space indentation for YAML and Markdown files

Files:

• src/README.md
• README.yaml
• docs/prd/upgrade-to-dynamic-subnets-v3.md
• README.md

{README.yaml,src/{variables,outputs}.tf}

📄 CodeRabbit inference engine (AGENTS.md)

When Terraform inputs/outputs change, update README.yaml and regenerate READMEs

Files:

• README.yaml
• src/outputs.tf
• src/variables.tf

test/**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

Place Go Terratest files under test/ and name them *_test.go using cloudposse/test-helpers and atmos fixtures

Files:

• test/component_test.go

src/{main,variables,outputs,providers,versions,context}.tf

📄 CodeRabbit inference engine (AGENTS.md)

Keep the Terraform component as the single source of truth under src/ with files: main.tf, variables.tf, outputs.tf, providers.tf, versions.tf, context.tf

Files:

• src/outputs.tf
• src/versions.tf
• src/main.tf
• src/variables.tf

**/*.tf

📄 CodeRabbit inference engine (AGENTS.md)

**/*.tf: Use 2-space indentation for Terraform files
In Terraform, prefer lower_snake_case for variables and locals; keep resource and data source names descriptive and aligned with Cloud Posse null-label patterns
Run terraform fmt and do not commit formatting violations

Files:

• src/outputs.tf
• src/versions.tf
• src/main.tf
• src/variables.tf

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Summary

🔇 Additional comments (29)

test/component_test.go (4)

17-23: LGTM: Centralized test constants improve maintainability.

Extracting common test values into constants improves consistency across the test suite and makes future updates easier.

---

29-43: LGTM: Well-structured validation helper with proper error reporting.

The helper correctly uses s.T().Helper() and provides clear assertion messages. Good refactor to reduce duplication across tests.

---

45-56: LGTM: Proper cleanup order prevents S3 deletion failures.

Emptying the S3 bucket before component destruction is correct and prevents common cleanup errors.

---

281-290: LGTM: Test suite wiring follows cloudposse/test-helpers pattern.

The suite correctly declares the VPC flow logs bucket dependency, ensuring proper test ordering.

docs/prd/upgrade-to-dynamic-subnets-v3.md (2)

587-610: Approve: Breaking change clearly documented.

The "⚠️ Breaking Change: AWS Provider Version Requirement" section (lines 589–610) now clearly documents that v5.0+ is required and provides migration guidance for v4.x users. This addresses the prior review concern about clarifying the provider version boundary in the backward compatibility section. ✅

---

472-545: Clarify PR acceptance criteria before acting on this concern.

The current PRD explicitly states: "Status: Recommended but not blocking for merge" (line 474). Your review assumes that PR objectives require these tests as blocking, but I could not locate documented PR acceptance criteria or objectives in the repository that would override this framing.

Before proceeding with changes, confirm: What are the actual PR acceptance criteria—do they require test cases as blocking prerequisites, or are they currently planned as post-merge enhancements? If the former, the PRD status should be updated to reflect that. If the latter, the current framing is correct and this review concern should be closed.

README.yaml (1)

1-296: Approve: Comprehensive v3.0.0 documentation.

README.yaml has been updated comprehensively with:

• Clear v3.0.0 feature descriptions (lines 8–22)
• Breaking change notice and migration path (lines 29–39)
• Multiple production-ready examples including cost optimization, named placement, and HA configurations (lines 41–236)
• Updated references to v3.0.0 modules (lines 238–296)

The breaking change is prominently flagged and the migration guidance is actionable. Content aligns with implementation changes across src/ files.

test/go.mod (3)

3-5: Approve: Go version and toolchain updates.

Go version bumped to 1.25 with toolchain directive (previously 1.23.0). Per prior review, Go 1.25 is a valid release. The toolchain directive ensures reproducibility across environments. ✅

---

10-10: Approve: Terratest version update.

terratest updated from v0.48.1 to v0.52.0, aligning with improved AWS SDK v2 support as noted in the PRD. This is a stable, recent version suitable for infrastructure testing. ✅

---

13-13: Approve: testify version update.

testify updated from v1.9.0 to v1.11.1, a recent stable version for Go testing assertions. No compatibility concerns. ✅

src/outputs.tf (5)

6-9: Approve: Subnet ARN and IPv6 CIDR outputs.

New outputs for public/private subnet ARNs and IPv6 CIDR blocks (lines 6–39) align with v3.0.0 feature set and expand output coverage for users needing subnet metadata beyond IDs and CIDR blocks. Descriptions are clear. ✅

Also applies to: 16-19, 26-29, 36-39

---

110-113: Approve: Network ACL ID outputs.

New outputs for public and private network ACL IDs (lines 110–118) provide additional network infrastructure metadata. Useful for users configuring network access rules. ✅

Also applies to: 115-118

---

157-170: Approve: NAT-related outputs.

New outputs for NAT instance AMI ID, NAT IPs, and NAT EIP allocation IDs (lines 157–170) expose additional NAT configuration details, supporting the v3.0.0 NAT placement flexibility. ✅

---

227-250: Approve: AZ-aware route table and availability zone outputs.

New outputs for availability zone IDs and AZ-to-route-table mappings (lines 227–250) support per-AZ infrastructure queries and improve observability of distributed subnet configurations. ✅

---

257-265: Approve: Named subnet stats with NAT Gateway IDs.

New outputs for named subnet stats maps (lines 257–265) now include NAT Gateway ID associations per subnet, enabling users to query which NAT Gateways serve each subnet. This is a key feature of v3.0.0 and critical for cost-optimized NAT placement verification. ✅

src/README.md (4)

33-49: Approve: Component description and v3.0.0 features.

The README header (lines 33–49) clearly describes the component and highlights v3.0.0 features including separate public/private subnet control, flexible NAT placement, and the breaking AWS Provider v5.0+ requirement. ✅

---

71-83: Approve: Breaking change documentation.

The "⚠️ Breaking Change: AWS Provider v5.0+ Required" section (lines 71–83) provides clear, actionable migration guidance for v4.x users. Addresses prior review concern comprehensively. ✅

---

85-281: Approve: Production-ready configuration examples.

The README includes six comprehensive configuration examples (Basic, Cost-Optimized NAT, Named NAT Placement, High-Availability NAT, Separate Public/Private Architecture, VPC Endpoints, Complete Production) covering all major v3.0.0 use cases. Descriptions and YAML are clear and actionable. ✅

---

317-373: Approve: Inputs and outputs tables comprehensive.

The Inputs table documents all new v3.0.0 variables (public_subnets_per_az_count/names, private_subnets_per_az_count/names, nat_gateway_public_subnet_indices/names) with descriptions of mutual exclusivity and backward compatibility. Outputs table includes all 14 new outputs (ARNs, IPv6 CIDRs, NAT-related, AZ mappings, subnet stats). ✅

src/main.tf (4)

7-11: Approve: NAT placement conflict detection logic.

Lines 7–11 introduce a local variable nat_placement_conflict that detects simultaneous specification of both NAT placement methods. The logic is correct and forms the basis for the plan-time check. ✅

---

99-105: Approve: Plan-time mutual exclusivity validation.

Lines 99–105 implement a Terraform check block that validates NAT Gateway placement variable mutual exclusivity at plan time (before resource creation). The error message is clear and actionable. This prevents configuration conflicts from reaching apply. ✅

---

156-156: Approve: Subnets module version upgraded to 3.0.0.

Line 156 correctly updates the subnets module version from 2.4.2 to 3.0.0, aligning with the PR objective. ✅

---

176-188: Approve: New v3.0.0 variables correctly passed to module.

Lines 176–188 pass all six new v3.0.0 input variables to the subnets module:

• Legacy variables (subnets_per_az_count/names) retained for backward compatibility
• New separate public/private variables (public/private_subnets_per_az_count/names)
• New NAT placement variables (nat_gateway_public_subnet_indices/names)

All variables correctly reference corresponding Terraform variables. ✅

README.md (3)

33-49: Approve: README description aligns with v3.0.0 scope.

Top-level README (lines 33–49) describes component with v3.0.0 features, breaking change notice, and architectural capabilities. Consistent with src/README.md. ✅

---

538-565: Approve: Testing guidance updated.

Testing section (lines 538–565) updated to reference Go 1.24+ (updated from prior version), Atmos, and Terratest. Guidance is clear and actionable. ✅

---

452-459: Approve: References include v3.0.0 resources.

Related projects and references sections (lines 452–459 and beyond) include links to terraform-aws-dynamic-subnets v3.0.0 release notes and module documentation, supporting users seeking detailed upgrade information. ✅

src/variables.tf (3)

274-316: Variables for per-AZ public/private subnet control are well-documented and backward-compatible.

All four new per-AZ variables follow the established pattern: nullable, defaulting to null, with clear descriptions of fallback to legacy subnets_per_az_* variables. Naming is consistent with Terraform conventions (lower_snake_case), type definitions are appropriate, and indentation is correct.

---

342-362: Name-based NAT placement variable is well-structured with appropriate regex validation.

The nat_gateway_public_subnet_names variable includes clear validation: non-empty list and regex pattern ^[a-z0-9-]+$ to enforce lowercase alphanumeric names with hyphens. This is consistent with the existing subnets_per_az_names variable (line 244: default ["common"]), which matches the same pattern.

---

320-340: Index bounds are validated by the upstream module—no action needed.

The terraform-aws-dynamic-subnets v3.0.0 module validates that provided indices are checked against the number/positions of public subnets per-AZ, which prevents out-of-range/invalid index usage. The variable validation in src/variables.tf correctly enforces non-negative indices, and the upstream module handles upper-bound enforcement during plan/apply.

[aknysh]

@coderabbitai full review please

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

echo "=== 1. Current AWS Provider constraint in src/versions.tf ==="
cat src/versions.tf

echo -e "\n=== 2. Check mutual exclusivity validation in src/main.tf ==="
cat src/main.tf | grep -B 5 -A 10 "nat_placement"

echo -e "\n=== 3. List all test functions in test/component_test.go ==="
grep "^func (s \*ComponentSuite) Test" test/component_test.go

echo -e "\n=== 4. Search for usage of new NAT variables in test files ==="
rg "nat_gateway_public_subnet_indices|nat_gateway_public_subnet_names" test/ --type yaml --type go -C 1 2>/dev/null || echo "No matches found in test files"

echo -e "\n=== 5. Search for usage of separate subnet count variables in test files ==="
rg "public_subnets_per_az_count|private_subnets_per_az_count" test/ --type yaml --type go -C 1 2>/dev/null || echo "No matches found in test files"

echo -e "\n=== 6. Check test stack catalog files for new variables ==="
find test/stacks -name "*.yaml" -exec echo "File: {}" \; -exec cat {} \; 2>/dev/null | head -100 || echo "No test stack files found"

echo -e "\n=== 7. Check Go version in go.mod ==="
head -5 test/go.mod

echo -e "\n=== 8. Check if there's any documentation about the AWS Provider requirement ==="
cat docs/prd/upgrade-to-dynamic-subnets-v3.md | grep -i "aws provider" -A 5 -B 2 | head -30

Length of output: 6136

---

@aknysh, excellent work! I've completed a comprehensive review and I'm pleased to report significant progress has been made. Here's the updated status:

✅ Outstanding Progress on Test Coverage!

Status: FULLY RESOLVED 🎉

Four new comprehensive test functions have been added to test/component_test.go:

1. ✅ TestNATPlacementByIndex() - Tests index-based NAT Gateway placement
2. ✅ TestNATPlacementByName() - Tests name-based NAT Gateway placement
3. ✅ TestSeparateSubnetCounts() - Tests separate public/private subnet counts
4. ✅ TestValidationMutualExclusivity() - Tests mutual exclusivity validation failure

Plus four new test fixture stack files in test/fixtures/stacks/catalog/usecase/:

• nat-by-index.yaml - Uses nat_gateway_public_subnet_indices: [0]
• nat-by-name.yaml - Uses nat_gateway_public_subnet_names: ["nat"]
• separate-counts.yaml - Tests different public/private counts (2 public, 3 private)
• validation-conflict.yaml - Intentionally specifies both NAT placement methods to test validation

This provides comprehensive coverage of all new features!

✅ Previously Resolved Issues (Still Valid)

1. Mutual Exclusivity Validation - Properly implemented with check block in main.tf ✓
2. Go 1.25 - Valid version (released August 2025) ✓

⚠️ AWS Provider Constraint - Final Assessment

Status: Remains >= 5.0.0 with no upper bound

After reviewing the PRD documentation, I can see this appears to be an intentional design choice:

• PRD explicitly states "AWS Provider v5.0+" requirement
• PRD mentions "Support for AWS Provider v6.x"
• Breaking change is documented throughout (PRD, README)

My recommendation: While CloudPosse often uses bounded constraints like >= 5.0.0, < 7.0.0, the current unbounded approach >= 5.0.0 provides flexibility for future AWS Provider versions. Given that:

• The breaking change is well-documented
• The PRD explicitly addresses AWS Provider v5.0+ and v6.x support
• No upper bound allows automatic compatibility with future provider versions

This is acceptable, provided the README and PR description clearly state:

• ⚠️ Breaking Change: Requires AWS Provider v5.0 or higher (v4.x is no longer supported)

If you prefer the more conservative bounded approach typical of CloudPosse patterns, consider: >= 5.0.0, < 7.0.0

📊 Final Summary

Status: 3 of 4 issues fully resolved ✅✅✅

Issue	Previous Status	Current Status
Mutual exclusivity validation	✅ Resolved	✅ Still resolved
Go 1.25 version	✅ Valid	✅ Still valid
Test coverage	❌ Missing	✅ Fully resolved!
AWS Provider constraint	❌ No upper bound	⚠️ Intentional (documented)

🎯 Final Recommendation

This PR is now in excellent shape and ready for merge! The comprehensive test coverage addresses all critical functionality.

Optional enhancement: If you want to be extra cautious, consider adding an upper bound to the AWS Provider constraint: >= 5.0.0, < 7.0.0. However, the current approach is acceptable given the thorough documentation.

Approval: ✅ This PR demonstrates high-quality work with:

• Comprehensive test coverage for all new features
• Proper validation of mutual exclusivity
• Excellent documentation (PRD, README, examples)
• Well-structured test fixtures
• Clear migration guidance

Great job on addressing the test coverage! 🐰✨

✅ 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

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 08a390f and 8c417df.

⛔ Files ignored due to path filters (1)

• test/go.sum is excluded by !**/*.sum

📒 Files selected for processing (6)

• README.md (8 hunks)
• README.yaml (4 hunks)
• docs/prd/upgrade-to-dynamic-subnets-v3.md (1 hunks)
• src/README.md (6 hunks)
• src/main.tf (4 hunks)
• test/go.mod (3 hunks)

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

• README.yaml

🧰 Additional context used

📓 Path-based instructions (4)

**/*.{yml,yaml,md}

📄 CodeRabbit inference engine (AGENTS.md)

Use 2-space indentation for YAML and Markdown files

Files:

• docs/prd/upgrade-to-dynamic-subnets-v3.md
• src/README.md
• README.md

{README.yaml,README.md,src/README.md}

📄 CodeRabbit inference engine (AGENTS.md)

Generate README.md (and src/README.md) from README.yaml via atmos/terraform-docs; do not edit generated READMEs manually

Files:

• src/README.md
• README.md

src/{main,variables,outputs,providers,versions,context}.tf

📄 CodeRabbit inference engine (AGENTS.md)

Keep the Terraform component as the single source of truth under src/ with files: main.tf, variables.tf, outputs.tf, providers.tf, versions.tf, context.tf

Files:

• src/main.tf

**/*.tf

📄 CodeRabbit inference engine (AGENTS.md)

**/*.tf: Use 2-space indentation for Terraform files
In Terraform, prefer lower_snake_case for variables and locals; keep resource and data source names descriptive and aligned with Cloud Posse null-label patterns
Run terraform fmt and do not commit formatting violations

Files:

• src/main.tf

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Summary

🔇 Additional comments (15)

src/main.tf (3)

7-11: Mutual exclusivity validation logic is correctly implemented.

The nat_placement_conflict local (lines 7–11) correctly detects when both nat_gateway_public_subnet_indices and nat_gateway_public_subnet_names are non-null, enabling the plan-time check to catch configuration errors early.

---

99-105: Plan-time validation block enforces mutual exclusivity with clear messaging.

The check "nat_placement_mutual_exclusivity" block (lines 99–105) correctly asserts that the conflict flag is false and provides a helpful error message guiding users to choose one NAT placement method. This design ensures errors are caught during plan rather than at apply time.

---

154-191: Module version and new variables passed correctly to terraform-aws-dynamic-subnets v3.0.1.

The module call correctly updates the version to 3.0.1 (line 156) and passes all six new variables for per-AZ subnet configuration and NAT placement (lines 176–189). Legacy variables are retained for backward compatibility, ensuring existing configs continue to work unchanged.

src/README.md (2)

33-50: Generated README properly documents v3.0.1 features and breaking changes.

The auto-generated README clearly highlights new v3.0.1 capabilities (per-AZ controls, flexible NAT placement, named subnets) and explicitly calls out the AWS Provider v5.0+ requirement and the critical NAT routing bug fix. The "DO NOT EDIT" header confirms this is auto-generated from README.yaml.

Also applies to: 44-50

---

72-267: Usage examples are comprehensive, clear, and production-ready.

The added configuration examples (Basic, Cost-Optimized, Named NAT, High-Availability, Separate Public/Private, VPC Endpoints, Complete Production) effectively demonstrate the new v3.0.1 features with realistic scenarios and tangible cost savings ($1,620/year example). Backward compatibility is noted for legacy configurations.

docs/prd/upgrade-to-dynamic-subnets-v3.md (4)

10-31: Executive summary clearly communicates upgrade scope and breaking changes.

The v1.2 PRD effectively summarizes the module upgrade to v3.0.1, new capabilities (separate per-AZ subnet config, flexible NAT placement), and the breaking AWS Provider v5.0+ requirement. The benefits and key changes are front-and-center.

---

592-628: Backward compatibility section clearly distinguishes provider breaking change from config compatibility.

The PRD correctly separates AWS Provider version requirements (breaking for v4.x users; requires v5.0+) from Terraform configuration compatibility (100% backward compatible on v5.0+). Migration guidance is provided for v4.x users. This clarity aligns with earlier review feedback and avoids user confusion.

---

467-550: Test coverage for v3.0.0 features is comprehensive and well-documented.

All four new feature tests are documented with stack configurations, validation steps, and rationale:

• TestNATPlacementByIndex: Validates index-based NAT placement with fixture nat-by-index.yaml
• TestNATPlacementByName: Validates name-based NAT placement with fixture nat-by-name.yaml
• TestSeparateSubnetCounts: Validates separate public/private counts with fixture separate-counts.yaml
• TestValidationMutualExclusivity: Validates mutual exclusivity validation failure with fixture validation-conflict.yaml

This directly addresses the prior review concern about test coverage for new features. Each test includes clear validation steps and expected outcomes.

---

674-697: Success criteria shows all 15 items completed, including new tests and validation.

The PRD tracks completion of all required work: module version upgrade, new variables, AWS Provider update, test infrastructure, mutual exclusivity validation, comprehensive documentation, and v3.0.1 patch inclusion. This aligns with PR objectives and demonstrates comprehensive coverage of the upgrade scope.

README.md (3)

33-50: Root README properly reflects v3.0.1 features and breaking changes.

The auto-generated README clearly documents the component's advanced capabilities (per-AZ controls, flexible NAT placement, cost optimization, VPC Endpoints, Shield protection) and explicitly calls out the AWS Provider v5.0+ requirement and NAT routing bug fix in the "What's New" section.

Also applies to: 44-50

---

442-455: References and Related Projects sections are comprehensive and version-accurate.

The updated References section includes links to:

• CloudPosse VPC Module v3.0.0
• CloudPosse Dynamic Subnets Module v3.0.1 with description of enhanced configuration
• v3.0.1 release notes (NAT routing bug fix)

Related Projects now include AWS VPC docs and NAT Gateway pricing for cost planning. This provides users with all necessary documentation and context for the upgrade.

---

283-294: Requirement tables correctly document AWS Provider v5.0+ and module versions.

Both Requirements (line 288) and Providers (line 294) tables show >= 5.0.0, aligning with the breaking change documentation. The dynamic-subnets module is correctly listed as v3.0.1.

test/go.mod (3)

3-3: Go 1.25 upgrade is valid and well-timed.

The Go version bump to 1.25 and toolchain declaration are appropriate. Per prior review comments, Go 1.25 was released on August 12, 2025, and is a stable choice for this component upgrade.

Also applies to: 5-5

---

165-175: Kubernetes dependencies are correctly managed as transitive.

Verified that k8s.io/* and sigs.k8s.io/* packages in test/go.mod are properly marked as indirect dependencies. The dependency chain shows these are pulled in by terratest v0.52.0 (along with test-helpers and go-commons), not directly imported. The test code contains no direct k8s imports. Version v0.34.0 is correctly selected by Go's module resolver as the highest compatible version across multiple transitive dependencies. No issues identified.

---

8-8: No issues identified with dependency upgrades.

The verified versions are terratest v0.52.0 and testify v1.11.1, with no documented breaking changes between the prior versions (v0.48.1 and v1.9.0 respectively). The v1.11.1 patch is stable—it fixed a regression from v1.11.0 regarding mock stringer behavior.

All four new test functions (TestNATPlacementByIndex, TestNATPlacementByName, TestSeparateSubnetCounts, TestValidationMutualExclusivity) use standard, stable testify assertion APIs (require/assert) and terratest AWS module APIs that remain compatible across the upgraded versions. No deprecated or removed APIs are present in the implementations.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

README.yaml (1)

22-22: AWS Provider v5.0+ requirement is documented but could be more prominent.

The breaking change (v4.x incompatibility) is mentioned in "What's New in v3.0.1" (line 22) but not prominently highlighted as a breaking change. Users scanning the README quickly may miss this. Consider adding a note in the Requirements section or using a callout (e.g., > [!WARNING] in Markdown) to ensure v4.x users see the requirement upfront.

This is a minor refinement since the requirement is documented; upgrading to `` given the "Chill" review setting, as per PR objectives confirm this is an intentional, documented decision.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8c417df and 720243e.

📒 Files selected for processing (6)

• README.md (9 hunks)
• README.yaml (4 hunks)
• src/README.md (7 hunks)
• src/main.tf (3 hunks)
• src/versions.tf (1 hunks)
• test/fixtures/stacks/catalog/usecase/nat-by-name.yaml (1 hunks)

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

• test/fixtures/stacks/catalog/usecase/nat-by-name.yaml
• src/main.tf
• src/versions.tf

🧰 Additional context used

📓 Path-based instructions (3)

{README.yaml,README.md,src/README.md}

📄 CodeRabbit inference engine (AGENTS.md)

Generate README.md (and src/README.md) from README.yaml via atmos/terraform-docs; do not edit generated READMEs manually

Files:

• README.yaml
• README.md
• src/README.md

**/*.{yml,yaml,md}

📄 CodeRabbit inference engine (AGENTS.md)

Use 2-space indentation for YAML and Markdown files

Files:

• README.yaml
• README.md
• src/README.md

{README.yaml,src/{variables,outputs}.tf}

📄 CodeRabbit inference engine (AGENTS.md)

When Terraform inputs/outputs change, update README.yaml and regenerate READMEs

Files:

• README.yaml

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Summary

🔇 Additional comments (6)

README.yaml (2)

5-50: README.yaml content is comprehensive and accurate.

The source YAML correctly documents v3.0.1 features, including per-AZ public/private subnet controls, NAT Gateway placement options (index and name-based), backward compatibility, and the AWS Provider v5.0+ requirement. Cost savings example and usage scenarios are clear and actionable.

---

106-107: Clear mutual-exclusivity guidance for NAT placement methods.

The documentation explicitly states that nat_gateway_public_subnet_indices and nat_gateway_public_subnet_names cannot be used together, aligning with the Terraform check block validation mentioned in the PR objectives.

README.md (2)

12-31: README.md is correctly auto-generated from README.yaml.

The file maintains auto-generation markers and comments (per build-harness). Content aligns with README.yaml source. No manual edits detected. As per coding guidelines, any updates should be made to README.yaml and regenerated via atmos readme.

---

288-288: AWS Provider v5.0+ requirement is clearly stated.

Both the Requirements and Providers sections correctly document the AWS Provider >= 5.0.0 constraint, consistent with the v3.0.1 upgrade and breaking change noted in the PR objectives.

Also applies to: 295-295

src/README.md (2)

1-11: src/README.md is correctly auto-generated from README.yaml.

Tags, metadata, and all content sections align with the YAML source. File structure and formatting are consistent with standard auto-generation. As per coding guidelines, updates must be made to README.yaml and regenerated.

---

241-241: AWS Provider v5.0+ requirement clearly documented in src/README.md.

Both Requirements and Providers sections correctly reflect the AWS Provider >= 5.0.0 constraint, consistent with v3.0.1 upgrade and breaking change for v4.x users.

Also applies to: 248-248

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/README.md (1)

110-113: Cost savings example is concrete but should cite pricing source or timestamp.

The example shows ~$135/month savings (3 AZ setup, ~$1,620/year), which is helpful. However, NAT Gateway pricing varies by region (e.g., $0.045/hour in us-east-1 vs. $0.045+ in other regions) and changes over time. Consider adding a note like "Based on us-east-1 pricing as of [date]" or linking to AWS NAT Gateway pricing page to set expectations.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 720243e and c4c96b0.

📒 Files selected for processing (2)

• README.md (9 hunks)
• src/README.md (7 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

{README.yaml,README.md,src/README.md}

📄 CodeRabbit inference engine (AGENTS.md)

Generate README.md (and src/README.md) from README.yaml via atmos/terraform-docs; do not edit generated READMEs manually

Files:

• src/README.md
• README.md

**/*.{yml,yaml,md}

📄 CodeRabbit inference engine (AGENTS.md)

Use 2-space indentation for YAML and Markdown files

Files:

• src/README.md
• README.md

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Summary

🔇 Additional comments (6)

src/README.md (2)

1-120: Comprehensive upgrade documentation with clear breaking change communication. ✓

The expanded documentation effectively covers the v3.0.1 upgrade, new features, and migration path. The "What's New in v3.0.1" section (line 49) and Requirements table (line 248) clearly communicate the AWS Provider ≥5.0.0 requirement, addressing the prior concern about v4.x breaking changes.

The six new variables are well documented with clear mutual-exclusivity constraints (line 115: "You can use EITHER... but not both. The plan will fail if both are specified"). Configuration examples progress logically from legacy/basic through cost-optimized, named placement, HA, and production patterns—each addressing real-world use cases.

---

279-330: Variable documentation covers new per-AZ controls, but verify null defaults against src/variables.tf.

All six new variables are documented with clear descriptions and default values (null for backward compatibility). However, ensure that:

• Descriptions of fallback behavior ("defaults to subnets_per_az_count for backward compatibility") match the actual Terraform code
• Default values shown (null) are correct in src/variables.tf
• Validation constraints (e.g., mutual exclusivity) are accurately described

README.md (4)

33-50: Clear feature summary with prominent v5.0.0 requirement and breaking-change language. ✓

The component introduction now covers key features (per-AZ control, flexible NAT placement, named subnets, cost optimization, VPC Flow Logs, VPC Endpoints, Shield protection) and "What's New in v3.0.1" section explicitly states "Requires AWS Provider v5.0+". This addresses prior concerns about communicating the breaking change for v4.x users, though phrasing could be more explicit (e.g., "Users on AWS Provider v4.x must upgrade to v5.0+").

---

545-545: Verify Go version requirement (1.24+ vs 1.25).

The testing setup section mentions "Install Go [1.24+ or newer]" but PR objectives indicate Go was upgraded to 1.25. Confirm whether the requirement is truly 1.24+ (which would include 1.25) or if it should explicitly state 1.25+. If Go 1.25 is the minimum supported version, update to reflect that.

---

361-362: New NAT placement variables clearly document mutual exclusivity and conflicts. ✓

Input descriptions for nat_gateway_public_subnet_indices and nat_gateway_public_subnet_names clearly state:

• Purpose: index-based vs. name-based NAT placement
• Conflict: "Conflicts with..." each other
• Fallback: "If both are null, NAT Gateways are placed in all public subnets by default"

Per-AZ count and name variables (public_subnets_per_az_count, private_subnets_per_az_count, etc.) are properly documented with backward-compatibility defaults.

Also applies to: 366-370

---

452-455: References updated with dynamic-subnets v3.0.1 and upstream release notes. ✓

The References section now includes links to:

• terraform-aws-vpc v3.0.0 (line 463)
• terraform-aws-dynamic-subnets v3.0.1 with description "Enhanced subnet configuration with separate public/private control" (line 464)
• Patch release notes "fixing NAT routing bug when max_nats < num_azs" (line 465)

This provides helpful context for users upgrading from v2.x to v3.x.

Also applies to: 463-465

[aknysh]

/terratest

[github-actions]

These changes were released in v2.0.0.