Skip to content

feat(ec2): improve subnet selection to support subnet group name and combined selection#35513

Closed
dwchiang wants to merge 4 commits intoaws:mainfrom
dwchiang:fix-35422-subnet-selection-type-and-name
Closed

feat(ec2): improve subnet selection to support subnet group name and combined selection#35513
dwchiang wants to merge 4 commits intoaws:mainfrom
dwchiang:fix-35422-subnet-selection-type-and-name

Conversation

@dwchiang
Copy link
Copy Markdown

Summary

This PR addresses issue #35422 by enhancing the SubnetSelection functionality to support more precise subnet selection when multiple public subnets with different purposes exist in a VPC.

Problem

Currently, when using SubnetSelection(subnet_type=SubnetType.PUBLIC), the CDK selects any public subnet without considering the specific subnet group name. This causes issues when users have multiple public subnet groups (e.g., "public" and "nat") and need to target a specific group.

Solution

This PR introduces the following enhancements:

  1. Enhanced SubnetSelection: Improved support for combining subnet_type and subnet_group_name for more precise selection
  2. Backward Compatibility: All existing subnet selection methods continue to work as before
  3. Better Documentation: Updated README with examples showing the new selection capabilities

Changes Made

  • packages/aws-cdk-lib/aws-ec2/lib/vpc.ts: Enhanced subnet selection logic
  • packages/aws-cdk-lib/aws-ec2/lib/instance.ts: Updated to support new selection methods
  • packages/aws-cdk-lib/aws-ec2/test/vpc.test.ts: Added comprehensive tests for new functionality
  • packages/aws-cdk-lib/aws-ec2/test/instance.test.ts: Added tests for instance subnet selection
  • packages/aws-cdk-lib/aws-ec2/test/integ.subnet-selection-combined.ts: New integration test
  • packages/aws-cdk-lib/aws-ec2/README.md: Updated documentation with examples

Usage Examples

# Select by subnet group name
subnets = ec2.SubnetSelection(subnet_group_name="public")

# Combine type and group name for precise selection
subnets = ec2.SubnetSelection(
    subnet_type=ec2.SubnetType.PUBLIC,
    subnet_group_name="public"
)

Testing

  • Unit tests added for new functionality
  • Integration tests added
  • Existing tests pass
  • Manual testing in real AWS environment (to be done during review)

Breaking Changes

None. This change is fully backward compatible.

Fixes #35422

@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. p2 labels Sep 17, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team September 17, 2025 12:41
Copy link
Copy Markdown
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
pahud previously requested changes Sep 17, 2025
Copy link
Copy Markdown
Contributor

@pahud pahud left a comment

Choose a reason for hiding this comment

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

Thank you for the PR.

packages/aws-cdk-lib/aws-ec2/test/integ.subnet-selection-combined.ts

let's move this integ test to packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test and

$ cd framework-integ
$ yarn build --fix
$ yarn integ test/aws-ec2/test/integ.subnet-selection-combined.js --update-on-failed

to generate the new snapshot.

and you may want to

$ yarn integ --directory test/aws-ec2/test/ --update-on-failed

to make sure all existing aws-ec2 integ tests stay up-to-date and refreshed if necessary.

dwchiang added a commit to dwchiang/aws-cdk that referenced this pull request Sep 18, 2025
…shots

- Move integ.subnet-selection-combined.ts from aws-cdk-lib to framework-integ
- Update imports to use proper framework-integ pattern
- Convert to use IntegTest construct as required by modern CDK testing
- Generate integration test snapshots

Addresses review feedback from PR aws#35513
@aws-cdk-automation aws-cdk-automation dismissed their stale review September 18, 2025 13:43

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

dwchiang and others added 2 commits September 18, 2025 22:09
…combined selection

This change enhances the SubnetSelection functionality to address the issue
where users cannot specify exact subnet groups by name when multiple public
subnets exist with different purposes.

Key improvements:
- Add support for subnet_group_name in SubnetSelection
- Enable combined subnet selection using both type and name
- Maintain backward compatibility with existing subnet selection methods

Fixes aws#35422

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
…shots

- Move integ.subnet-selection-combined.ts from aws-cdk-lib to framework-integ
- Update imports to use proper framework-integ pattern
- Convert to use IntegTest construct as required by modern CDK testing
- Generate integration test snapshots

Addresses review feedback from PR aws#35513
@dwchiang dwchiang force-pushed the fix-35422-subnet-selection-type-and-name branch from 62b7275 to 260af5f Compare September 18, 2025 14:09
@mergify mergify bot dismissed pahud’s stale review September 18, 2025 14:10

Pull request has been modified.

…r simpler alternatives

- Add comprehensive JSDoc documentation to selectSubnetObjectsByTypeAndName method
- Include guidance about when to use simpler alternatives (subnetGroupName alone, subnetType alone, or SubnetFilter.byIds)
- Update README with note about considering simpler alternatives
- Improve SubnetSelection interface documentation with usage guidance
@pahud pahud self-assigned this Sep 18, 2025
pahud
pahud previously requested changes Sep 18, 2025
Copy link
Copy Markdown
Contributor

@pahud pahud left a comment

Choose a reason for hiding this comment

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

Per CI error:

aws-cdk-lib: /codebuild/output/src1050340690/src/actions-runner/_work/aws-cdk/aws-cdk/packages/aws-cdk-lib/aws-ec2/lib/vpc.ts
aws-cdk-lib:   717:5  error  Trailing spaces not allowed  no-trailing-spaces
aws-cdk-lib:   722:5  error  Trailing spaces not allowed  no-trailing-spaces
aws-cdk-lib: ✖ 2 problems (2 errors, 0 warnings)
aws-cdk-lib:   2 errors and 0 warnings potentially fixable with the `--fix` option.
aws-cdk-lib: Error: /codebuild/output/src1050340690/src/actions-runner/_work/aws-cdk/aws-cdk/node_modules/eslint/bin/eslint.js . --ext=.ts --resolve-plugins-relative-to=/codebuild/output/src1050340690/src/actions-runner/_work/aws-cdk/aws-cdk/tools/@aws-cdk/cdk-build-tools/lib exited with error code 1

please run yarn lint --fix in aws-cdk-lib

Addresses PR review comment about ESLint trailing spaces errors on lines 717 and 722.
@mergify mergify bot dismissed pahud’s stale review September 19, 2025 23:44

Pull request has been modified.

@pahud pahud marked this pull request as ready for review September 22, 2025 15:43
Copy link
Copy Markdown
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Looking at the original issue, it's not clear to me how the recommended:

    subnet_group_name="public"  # Specify your desired group name

solution is not sufficient.

Subnet groups of a given name are already of a defined type.

In other words in the following example:

# Combine type and group name for precise selection
subnets = ec2.SubnetSelection(
    subnet_group_name="public"
    subnet_type=ec2.SubnetType.PUBLIC,
)

Once we know that subnet_group_name="public", it must already follow that subnet_type=PUBLIC.

If you passed in any other value than PUBLIC here, you would always get an empty set.

It might be that the original reporter actually had an unfixable problem because there is a bug in the code, but if they do this is not the solution to it.

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing

To prevent automatic closure:

  • Resume work on the PR
  • OR request an exemption by adding a comment containing 'Exemption Request' with justification e.x "Exemption Request: "
  • OR request clarification by adding a comment containing 'Clarification Request' with a question e.x "Clarification Request: "

This PR will automatically close in 14 days if no action is taken.

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Nov 5, 2025
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 5, 2025

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. p2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

aws-ec2: EC2 deploys to wrong subnet

4 participants