Skip to content

Conversation

@ramonskie
Copy link
Contributor

Summary

  • Fix terraform resource reference error when using --lb-type nlb with --dualstack=true
  • Split DNS templates so each LB type references the correct resource type: cf_dns.tf for Classic ELBs (type "cf"), cf_nlb_dns.tf for Network LBs (type "nlb")

Problem

PR #644 introduced a bug where cf_dns.tf conditionally referenced aws_lb.* resources based on var.dualstack, but these resources only exist when state.LB.Type="nlb". This caused terraform to fail with "Reference to undeclared resource" errors when users set --lb-type cf --dualstack=true.

Solution

Split the DNS configuration into separate templates:

  • cf_dns.tf - References aws_elb.* resources for type "cf"
  • cf_nlb_dns.tf - References aws_lb.* resources for type "nlb"

The template generator now selects the correct DNS template based on state.LB.Type, eliminating the conditional logic mismatch.

Adds test coverage for NLB load balancer type to verify:
- NLB without domain does not include DNS template
- NLB with domain includes cf_nlb_dns template that correctly references aws_lb resources
@ramonskie
Copy link
Contributor Author

the pr that introduced the dual stack feature has been reverted in #663
so we can close this

@rkoster rkoster requested review from a team, benjaminguttmann-avtq, Copilot and lnguyen and removed request for a team November 6, 2025 16:37
@rkoster rkoster moved this from Inbox to Pending Review | Discussion in Foundational Infrastructure Working Group Nov 6, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for Network Load Balancer (NLB) DNS configuration by creating a new template file and updating the template generator to use it when NLB type is specified. The change separates DNS configuration for NLB resources (aws_lb) from classic ELB resources (aws_elb).

  • Created cf_nlb_dns.tf template that references aws_lb resources instead of aws_elb resources
  • Updated cf_dns.tf to remove conditional logic for dualstack, now only references aws_elb resources
  • Added NLB type to the list of supported load balancer types for LB commands

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
terraform/aws/templates/cf_nlb_dns.tf New DNS template file for NLB that references aws_lb resources
terraform/aws/templates/cf_dns.tf Simplified to remove dualstack conditionals, now only uses aws_elb resources
terraform/aws/template_generator_test.go Added test coverage for NLB with and without system domain
terraform/aws/template_generator.go Updated to load and use the new cf_nlb_dns template for NLB type
commands/aws_lbs.go Extended LB type switch to include "nlb" alongside "cf"

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

type = "CNAME"
ttl = 300

records = [one(aws_elb.iso_router_lb[*].dns_name)]
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The isolation segment DNS record references aws_elb.iso_router_lb but should reference the NLB resource instead. Based on the pattern in iso_segments.tf, this should be one(aws_lb.iso_router_nlb[*].dns_name) to match the NLB infrastructure that this template is designed for.

Suggested change
records = [one(aws_elb.iso_router_lb[*].dns_name)]
records = [one(aws_lb.iso_router_nlb[*].dns_name)]

Copilot uses AI. Check for mistakes.
@peanball
Copy link
Contributor

peanball commented Nov 7, 2025

Please close this. I will prepare another one to ensure that the desired semantics are correct.

I also want to introduce a proper CLI flag for dual stack, not just a TF variable.

@ramonskie ramonskie closed this Nov 7, 2025
@github-project-automation github-project-automation bot moved this from Pending Review | Discussion to Done in Foundational Infrastructure Working Group Nov 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

2 participants