Skip to content

fix(route53-targets): only use dualstack prefix for dual-stack NLBs #34641

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

pahud
Copy link
Contributor

@pahud pahud commented Jun 5, 2025

Issue # (if applicable)

Closes #16987.

Reason for this change

LoadBalancerTarget was unconditionally adding the dualstack. prefix to all load balancer DNS names, causing IPv4-only Network Load Balancers to become unreachable via Route53 alias records. This breaks a fundamental use case where users expect Route53 aliases to work with their default (IPv4-only) NLBs.

Description of changes

Core Fix:

  • Modified LoadBalancerTarget.getDnsName() to conditionally apply the dualstack prefix based on load balancer type and IP address configuration
  • Added isNetworkLoadBalancer() and isIpv4Only() helper methods for type detection

Logic Changes:

  • IPv4-only NLBs: Use plain DNS name (fixes the broken behavior)
  • Dual-stack NLBs: Use dualstack prefix (maintains existing behavior)
  • ALBs: Continue using dualstack prefix (preserves backward compatibility)

Why this approach:

  • Fixes the bug while maintaining backward compatibility for ALBs
  • Uses constructor name checking for reliable NLB detection
  • Defaults to dualstack prefix when in doubt (safer fallback)

Alternatives considered:

  • Interface-based type checking (rejected due to complexity)
  • Breaking change for all load balancers (rejected for backward compatibility)
  • Runtime DNS validation (rejected as too complex)

Describe any new or updated permissions being added

No new IAM permissions required. This is a client-side logic change that affects how CDK generates CloudFormation templates.

Description of how you validated changes

Unit Tests:

  • Added comprehensive test cases covering IPv4-only NLB, dual-stack NLB, default NLB, and ALB scenarios
  • Verified existing ALB tests continue to pass (backward compatibility)

Integration Tests:

  • Created integ.nlb-alias-target.ts with real CloudFormation deployment
  • Generated snapshots proving correct DNS name generation:
    • IPv4-only NLB: {"Fn::GetAtt": ["IPv4NLB", "DNSName"]} (no prefix)
    • Dual-stack NLB: {"Fn::Join": ["", ["dualstack.", {"Fn::GetAtt": [...]}]]} (with prefix)
  • Successfully deployed and validated in AWS environment

Manual Validation:

  • Verified CloudFormation template generation produces expected outputs
  • Confirmed Route53 record syntax matches AWS requirements

Checklist


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

This follows the exact AWS CDK PR template format and provides comprehensive details about the fix, testing, and impact.

@pahud pahud marked this pull request as draft June 5, 2025 18:25
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jun 5, 2025
…y and dual-stack NLBs with appropriate DNS naming
pahud added 8 commits June 5, 2025 15:48
- Created snapshot files for integration tests related to NLB alias target.
- Added manifest and asset files to support the new integration test.
- Included CloudFormation template for the NLB integration test.
- Updated tree structure to reflect the new resources and dependencies.
- Modified the TestBucketDeployment class to set the postCliContext option for disabling the use of CDK managed log groups.

refactor: improve load balancer target detection logic

- Enhanced the isNetworkLoadBalancer and isIpv4Only methods for better readability and maintainability by using clearer variable names and comments.

chore: clean up yarn.lock by removing unused dependencies

- Removed references to unused packages and updated existing dependencies to streamline the project.
…y and dual-stack NLBs with appropriate DNS naming
- Add explanatory comment for constructor name checking approach
- Improve variable naming and formatting in isIpv4Only method
- Better explain why NLB vs ALB detection is needed for DNS requirements
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

(This review is outdated)

pahud added 2 commits June 5, 2025 22:52
- Created snapshot files for integration test of NLB alias target.
- Added CloudFormation template and asset manifest for the NLB integration.
- Implemented VPC, subnets, and load balancers in the test setup.
- Included assertions for bootstrap version checks in the CloudFormation template.
@aws-cdk-automation aws-cdk-automation dismissed their stale review June 6, 2025 03:07

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

pahud added 6 commits June 5, 2025 23:10
…support

- Updated VPC configuration to support dual-stack with IPv6 CIDR blocks.
- Enabled auto-assign of IPv6 addresses for public subnets.
- Created dual-stack NLB and added corresponding A and AAAA records for DNS.
- Ensured IPv4-only NLB records do not have dualstack prefix.
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 1a8d531
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@pahud pahud marked this pull request as ready for review June 6, 2025 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-route53-targets: LoadBalancerTarget always appends the dualstack prefix even when not a valid option
2 participants