Fix NAT routing when max_nats limits NATs to fewer AZs #227
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
**Problem:** When max_nats < number of AZs, NAT Gateways are only created in the first max_nats AZs, but the route table mapping formulas assumed NATs exist in all AZs, causing "Invalid index" errors. **Example scenario that failed:** - 2 AZs, max_nats=1 → Only 1 NAT created (in AZ0) - 2 private route tables (1 per AZ) - Mapping produced [0, 1] but only NAT[0] exists - Error: aws_nat_gateway.default[1] - invalid index **Root cause:** The private_route_table_to_nat_map and public_route_table_to_nat_map formulas calculated: az_index * nats_per_az + subnet_within_az This works when NATs exist in all AZs, but fails when max_nats limits NATs to fewer AZs. The formula could generate indices >= nat_count. **Fix:** Added modulo operation to clamp results to available NAT indices: (floor(i / subnets_per_az_count) * nats_per_az + (i % subnets_per_az_count) % nats_per_az) % nat_count Now route tables in AZs without NATs will wrap around to use NATs from other AZs (typically AZ0). **Why wasn't this caught by tests?** None of the module's examples use the max_nats feature. All examples either omit max_nats (defaulting to unlimited) or use max_nats >= number of AZs, so the bug was never triggered. **Testing:** This bug was discovered by the aws-vpc component test suite when using: - 2 AZs with max_nats: 1 - public_subnets_enabled: true - NAT Gateway enabled Fixes: #226
**New Example: limited-nat-gateways** Tests the critical max_nats < num_azs scenario that was previously untested and contained the bug fixed in commit 3681299. **What it tests:** - 3 AZs with max_nats=1 (only 1 NAT Gateway created) - 3 AZs with max_nats=2 (only 2 NAT Gateways created) - Verifies that private subnets in AZs without NATs can still route to the internet through NATs in other AZs - Ensures route table mapping correctly wraps around when NATs are limited to fewer AZs **Why this is important:** This test would have caught the bug before it reached production. The bug caused "Invalid index" errors when max_nats < num_azs because route tables in AZs without NATs tried to reference non-existent NAT Gateway indices. **Use case documented:** The example includes comprehensive README explaining when to use max_nats for cost optimization: - Dev/test environments: 1 NAT for 3 AZs saves ~$65/month (67%) - Trade-off: Lower availability vs. cost savings - When to use and when to avoid this pattern **Test coverage:** - TestExamplesLimitedNatGateways: 3 AZs, 1 NAT - TestExamplesLimitedNatGatewaysTwoNats: 3 AZs, 2 NATs - TestExamplesLimitedNatGatewaysDisabled: enabled=false check This brings total test coverage to: - 7 examples (was 6) - 11 test functions (was 8) - ✅ max_nats feature now tested Closes test coverage gap identified during bug investigation.
**Documentation Added:** 1. **Test Coverage Analysis** (docs/test-coverage-analysis.md) - Complete analysis of all 7 examples and 13 test functions - Feature coverage matrix showing what's tested vs untested - Identified critical gaps: NAT Instance, IPv6/NAT64, custom route tables - Recommendations for future test improvements - Test execution strategy and best practices 2. **PRD: Fix max_nats Routing Bug** (docs/prd/fix-max-nats-routing.md) - Executive summary of the critical bug - Detailed problem statement with error examples - Root cause analysis and why tests didn't catch it - Technical solution design with before/after code - Complete testing strategy with 3 new test functions - Cost analysis showing 67% savings with max_nats - Rollout plan, success metrics, and risk assessment - Lessons learned and process improvements **Key Findings:** - Bug existed because max_nats had ZERO test coverage - Module has good test coverage overall (13 tests, 7 examples) - Several features still lack tests: NAT Instance, IPv6, custom routes - New limited-nat-gateways example fills critical gap - Cost optimization: $65/month savings per environment with max_nats=1 **Impact:** This documentation ensures: - Future contributors understand test requirements - Test gaps are visible and prioritized - Cost optimization features are documented - Bug investigation process is recorded - Test coverage improvements have a roadmap
**Major Additions:** 1. **NAT Gateway Placement Behavior Section** - Explains the two configuration dimensions: subnet names and max_nats - Formula: Total NATs = min(num_azs, max_nats) × num_subnet_names - Clarifies that multiple subnet names = multiple NATs per AZ 2. **Visual ASCII Diagrams for 4 Placement Strategies** - Strategy 1: Standard (1 NAT per AZ) - $97.20/month - Strategy 2: Redundant (2 NATs per AZ) - $194.40/month - Strategy 3: Limited (1 NAT total with max_nats) - $32.40/month - Strategy 4: Hybrid (2 NATs in 1 AZ) - $64.80/month Each diagram shows: - Which AZs get NATs - Which subnet types get NATs - Routing arrows for cross-AZ traffic - Monthly cost calculations 3. **Configuration Calculation Table** - 6 common configuration scenarios - Shows AZs, subnet names, max_nats, total NATs, cost, and use case - Helps users quickly find the right configuration 4. **Decision Tree for Choosing NAT Configuration** - Production vs non-production paths - Maximum availability vs cost optimization - Special requirements (failover, single AZ, no NAT) - Recommendations with cost and availability trade-offs 5. **Routing Behavior Explanation** - Mathematical formula breakdown with examples - Shows how modulo operation prevents invalid indices - Demonstrates the bug fix in action 6. **Best Practices by Environment Type** - Production: NAT per AZ minimum, consider redundancy - Development: max_nats=1 for cost savings - Staging: Balanced approach with max_nats=2 - Cost optimization tips **Why This Matters:** The original PRD didn't clearly explain that: - nat_gateway_public_subnet_names can create MULTIPLE NATs per AZ - max_nats limits which AZs get NATs, not total NAT count - These two variables multiply together This caused confusion about cost implications. The new diagrams and decision tree make it crystal clear how to configure NATs for different scenarios and understand the cost/availability trade-offs. **Examples:** - 3 AZs + names=["lb","web"] + max_nats=3 = 6 NATs ($194/mo) - 3 AZs + names=["lb"] + max_nats=1 = 1 NAT ($32/mo) - Understanding this prevents costly misconfigurations
aknysh
requested review from
osterman
and removed request for
kevcube and
oycyc
Member
Author
|
/terratest |
Remove hardcoded `max_nats = 1` from fixtures.us-east-2.tfvars to allow tests to properly override the value via the Vars parameter. **Problem:** TestExamplesLimitedNatGatewaysTwoNats was failing because it tried to override max_nats=2 via Vars, but the fixtures file had max_nats=1 hardcoded. Depending on Terraform's variable precedence and how terratest constructs the command line, the -var-file value may take precedence over the -var flag. **Solution:** Remove max_nats from fixtures file. Both tests explicitly pass max_nats in their Vars map: - TestExamplesLimitedNatGateways passes max_nats: 1 - TestExamplesLimitedNatGatewaysTwoNats passes max_nats: 2 The example's variables.tf provides default = 1 as a fallback. Fixes the test failure where expected 2 NAT Gateways but got 1. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
/terratest |
Member
Author
|
/terratest |
Contributor
|
These changes were released in v3.0.1. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
what
max_natsis set to fewer than the number of Availability Zoneslimited-nat-gatewaysdemonstrating themax_natsfeaturemax_natsfeaturewhy
Critical Bug: When
max_nats < num_azs, Terraform failed with "Invalid index" error because route tables in AZs without NATs attempted to reference non-existent NAT Gateway indices.Example Failure:
Root Cause: The route table mapping formula calculated:
This produced indices [0, 1, 2] but only NAT[0] existed.
Fix: Added modulo operation to wrap indices to available NATs:
Now produces [0, 0, 0] - all route to the single NAT.
Test Coverage Gap: The
max_natsfeature had ZERO test coverage. None of the 6 existing examples tested this scenario. The bug was discovered by the aws-vpc component test suite, not by this module's own tests.Changes Include:
Bug Fix (
main.tf):private_route_table_to_nat_mapcalculationpublic_route_table_to_nat_mapcalculationNew Test Example (
examples/limited-nat-gateways):Test Coverage (
test/src/examples_limited_nat_gateways_test.go):TestExamplesLimitedNatGateways- Tests max_nats=1TestExamplesLimitedNatGatewaysTwoNats- Tests max_nats=2TestExamplesLimitedNatGatewaysDisabled- Tests enabled=falseDocumentation:
Cost Implications:
The
max_natsfeature enables significant cost savings in non-production environments:This bug blocked users from utilizing this cost optimization feature.
references
max_nats < num_azsfor cost optimization/docs/test-coverage-analysis.md/docs/prd/fix-max-nats-routing.md