-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
pahud
wants to merge
17
commits into
aws:main
Choose a base branch
from
pahud:fix-16987
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
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
…y and dual-stack NLBs with appropriate DNS naming
- 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
aws-cdk-automation
previously requested changes
Jun 6, 2025
There was a problem hiding this 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)
- 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.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
…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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Issue # (if applicable)
Closes #16987.
Reason for this change
LoadBalancerTarget
was unconditionally adding thedualstack.
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:
LoadBalancerTarget.getDnsName()
to conditionally apply thedualstack
prefix based on load balancer type and IP address configurationisNetworkLoadBalancer()
andisIpv4Only()
helper methods for type detectionLogic Changes:
Why this approach:
Alternatives considered:
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:
Integration Tests:
integ.nlb-alias-target.ts
with real CloudFormation deployment{"Fn::GetAtt": ["IPv4NLB", "DNSName"]}
(no prefix){"Fn::Join": ["", ["dualstack.", {"Fn::GetAtt": [...]}]]}
(with prefix)Manual Validation:
Checklist
GUIDELINES
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.