fix(eks): looked up vpc causing premature validation errors for private subnets #33786

shikha372 · 2025-03-15T01:02:54Z

Issue # (if applicable)

Related to one of the related issue for EKS and VPC in #22025 .

Reason for this change

Currently EKS cluster with private endpoint access fails during cdk synth operation for a lookedup VPC.
investigating further into lookup implementation, the VPC id is first populated through some dummy values including the one for private subnets :

    {
      name: 'Private',
      type: cxapi.VpcSubnetGroupType.PRIVATE,
      subnets: [
        {
          availabilityZone: 'dummy1a',
          subnetId: 'p-12345',
          routeTableId: 'rtb-12345p',
          cidr: '1.2.3.4/5',
        },

But there are no dummy values defined for the case of privatesubnetIds :

privateSubnetIds: undefined,

which results in return of filtering by IDs option as a null object until the values are fully resolved.
Reference code: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-ec2/lib/vpc.ts#L2705

Description of changes

using existing field isPendingLookup in the SubnetSelection which is being set on the basis of this.incompleteSubnetDefinition = isIncomplete; where isIncomplete is set to false during first pass of cdk synth.
So during first synth operation, validation will be skipped.

Describe any new or updated permissions being added

NA

Description of how you validated changes

Added unit test and integration test

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

codecov · 2025-03-15T01:11:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.38%. Comparing base (8b23b5d) to head (89ecdf6).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #33786   +/-   ##
=======================================
  Coverage   82.38%   82.38%           
=======================================
  Files         120      120           
  Lines        6955     6955           
  Branches     1173     1173           
=======================================
  Hits         5730     5730           
  Misses       1120     1120           
  Partials      105      105

Flag	Coverage Δ
suite.unit	`82.38% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`∅ <ø> (∅)`
packages/aws-cdk-lib/core	`82.38% <ø> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

packages/aws-cdk-lib/aws-ec2/lib/subnet.ts

iliapolo · 2025-03-19T10:17:08Z

packages/aws-cdk-lib/aws-eks/lib/cluster.ts

-    if (privateSubnets.length === 0 && publicAccessDisabled) {
-      // no private subnets and no public access at all, no good.
-      throw new Error('Vpc must contain private subnets when public endpoint access is disabled');
+    if (!isLookedUpVPC && !isSubnetIdFilter) {


Note that LookedUpVpc is always the type when using Vpc.fromLookup. Its just that on the first synth, its values are dummy ones. The condition you are proposing here will make us skip the validation entirely, but actually we just want to skip it on the first synth, right?

So the condition needs to be based on the state of the VPC, not on its type.

There a couple other options to address this issue I think:

Subnet Selection

We already have a flag on SubnetSelection that tells us whether a lookup is pending:

aws-cdk/packages/aws-cdk-lib/aws-ec2/lib/vpc.ts

Line 405 in 2623e00

readonly isPendingLookup?: boolean;

SubnetSelection is used extensively in the EKS module. I don't have it all laid out, but can we leverage this to determine we don't need this validation?

incompleteSubnetDefinition

VpcBase has a field indicating whether we should validate on subnets or not:

aws-cdk/packages/aws-cdk-lib/aws-ec2/lib/vpc.ts

Lines 459 to 462 in 2623e00

/**

* If this is set to true, don't error out on trying to select subnets

*/

protected incompleteSubnetDefinition: boolean = false;

Its not currently public though, but worst case, we can discuss making it so.

didn't know about it, its pretty useful and solves this problem, refactored code to use isPendingLookup, which returns false during dummyValue which i believe is due to dummy value being undefined under getValue (please correct me if the understanding is not correct), which results in LookedUpVpc with incompleteSubnetDefinition to true

const attributes: cxapi.VpcContextResponse = ContextProvider.getValue(scope, { provider: cxschema.ContextProvider.VPC_PROVIDER, props: { ...overrides, filter, returnAsymmetricSubnets: true, returnVpnGateways: options.returnVpnGateways, subnetGroupNameTag: options.subnetGroupNameTag, } as cxschema.VpcContextQuery, dummyValue: undefined, }).value; new LookedUpVpc(scope, id, attributes ?? DUMMY_VPC_PROPS, attributes === undefined); constructor(scope: Construct, id: string, props: cxapi.VpcContextResponse, isIncomplete: boolean) this.incompleteSubnetDefinition = isIncomplete;

We don't need to make incompleteSubnetDefinition public as we can fetch the corresponding isPendingLookup from selectSubnets method

packages/aws-cdk-lib/aws-eks/lib/cluster.ts

iliapolo · 2025-03-19T10:29:20Z

@shikha372 Please also amend the PR title to briefly describe what the issue is. Avoid generic terms like "fixing".

For example: fix(eks): looked up vpc causing premature validation errors.

packages/aws-cdk-lib/aws-eks/lib/cluster.ts

packages/aws-cdk-lib/aws-eks/test/cluster.test.ts

iliapolo · 2025-03-24T15:31:17Z

...ges/@aws-cdk-testing/framework-integ/test/aws-eks/test/integ.eks-cluster-private-endpoint.ts

  diffAssets: false,
 });
-
-app.synth();


I guess this was intentional, but a note would be good to make your intention clear.

actually this was not intentional, but i don't think its an issue as we don't need it anymore for integration tests.

iliapolo · 2025-03-24T15:43:51Z

...esting/framework-integ/test/aws-eks/test/integ.eks-cluster-private-endpoint-subnet-filter.ts

+const stack = new cdk.Stack(app, 'StackWithVpc', {
+  env: {
+    region: 'eu-west-1',
+    account: '123456',


Can you explain how this successfully deploys? sorry I might just not be familiar with the magic that replaces/overrides this at deploy time, and why is this needed?

iliapolo · 2025-03-24T15:55:01Z

...esting/framework-integ/test/aws-eks/test/integ.eks-cluster-private-endpoint-subnet-filter.ts

+});
+
+// Look up an existing VPC
+const vpc = ec2.Vpc.fromLookup(stack, 'Vpc', {


I don't really understand how this test works...the VPC you are looking up gets deployed in this same stack, so during lookup (synthesis), the VPC does not exist.

My guess is that the CLI just gives up and proceeds without fetching lookup values:

https://github.com/aws/aws-cdk-cli/blob/2dc5a564d8225f99063afd80bf4067770b0b0291/packages/aws-cdk/lib/api/cxapp/cloud-executable.ts#L81-L83

Can you confirm? You probably need to separate stacks here. Though I actually think an integ test is not really required here, the unit tests seem sufficient - can you elaborate on what additional value the integ tests give you?

actually i was referring for existing VPC lookup test here , I tried to model it with two stacks but seems when we set enableLookups in integ test it is not honoring the stack dependency and fails with the value not found.

// Add explicit dependency to ensure VPC stack is deployed first

clusterStack.addDependency(vpcStack); new integ.IntegTest(app, 'aws-cdk-eks-cluster-private-endpoint-subnet-filter', { testCases: [vpcStack], // Test includes assets that are updated weekly. If not disabled, the upgrade PR will fail. diffAssets: false, enableLookups: true, stackUpdateWorkflow: false, });

But i;ve tested the validation with a sample CDK stack application linked to update aws-cdk module with fix;

export class CdkVpcAppStack extends cdk.Stack { constructor(scope: Construct, id: string, props?: cdk.StackProps) { super(scope, id, props); const vpc = ec2.Vpc.fromLookup(this, 'Vpc', { vpcName: 'aws-cdk-eks-cluster-private-endpoint-test', isDefault: false }); new eks.Cluster(this, 'eks-cluster', { vpc, version: eks.KubernetesVersion.V1_31, kubectlLayer: new KubectlV31Layer(this, 'KubectlLayer'), endpointAccess: eks.EndpointAccess.PRIVATE, vpcSubnets: [ { // subnetType: ec2.SubnetType.PRIVATE_WITH_EGRESS, subnetFilters: [ ec2.SubnetFilter.byIds(['subnet-XXXXX']), ], } ] }); } }

aws-cdk-automation

(This review is outdated)

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

godwingrs22 · 2025-03-26T19:52:51Z

packages/aws-cdk-lib/aws-eks/lib/cluster.ts

Wondering if we should also perform below validation here only if there is no pending lookups? Is it applicable?

if (placeClusterHandlerInVpc && privateSubnets.length === 0) { throw new Error('Cannot place cluster handler in the VPC since no private subnets could be selected'); }

yep its added for this use case https://github.com/shikha372/aws-cdk/blob/shilkagg/fix-eks/packages/aws-cdk-lib/aws-eks/lib/cluster.ts#L1679

godwingrs22

LGTM

addressed requested changes and reviewed with abstractions team for approval

mergify · 2025-03-27T17:31:25Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

shikha372 · 2025-03-27T17:31:26Z

@Mergifyio update

mergify · 2025-03-27T17:31:36Z

update

☑️ Nothing to do

#commits-behind > 0 [📌 update requirement]
queue-position = -1 [📌 update requirement]
-closed [📌 update requirement]
-conflict [📌 update requirement]

aws-cdk-automation · 2025-03-27T18:07:00Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 89ecdf6
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2025-03-27T18:07:23Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions · 2025-03-27T18:07:41Z

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 bot added the p2 label Mar 15, 2025

aws-cdk-automation requested a review from a team March 15, 2025 01:03

mergify bot added the contribution/core This is a PR that came from AWS. label Mar 15, 2025

shikha372 force-pushed the shilkagg/fix-eks branch 2 times, most recently from 51d0172 to 83c35a0 Compare March 17, 2025 21:44

iliapolo previously requested changes Mar 19, 2025

View reviewed changes

shikha372 force-pushed the shilkagg/fix-eks branch 2 times, most recently from c207f2e to e112c7f Compare March 19, 2025 22:52

shikha372 changed the title ~~fix(eks): fixing the filtering by subnetIDs option~~ fix(eks): looked up vpc causing premature validation errors for private subnets Mar 19, 2025

shikha372 marked this pull request as ready for review March 20, 2025 00:04

iliapolo reviewed Mar 24, 2025

View reviewed changes

fix(eks): fixing the filtering by subnetIDs option

5ef64d0

shikha372 force-pushed the shilkagg/fix-eks branch 2 times, most recently from aa1175e to dababbd Compare March 24, 2025 23:08

aws-cdk-automation previously requested changes Mar 26, 2025

View reviewed changes

fix(eks): looked up vpc causing premature validation errors

b2dc787

shikha372 force-pushed the shilkagg/fix-eks branch from c93a02e to b2dc787 Compare March 26, 2025 05:41

shikha372 added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Mar 26, 2025

Merge branch 'main' into shilkagg/fix-eks

aa50690

godwingrs22 self-assigned this Mar 26, 2025

godwingrs22 reviewed Mar 26, 2025

View reviewed changes

godwingrs22 approved these changes Mar 26, 2025

View reviewed changes

godwingrs22 and others added 2 commits March 26, 2025 13:40

Merge branch 'main' into shilkagg/fix-eks

58b0e46

Merge branch 'main' into shilkagg/fix-eks

474770e

shikha372 requested a review from iliapolo March 26, 2025 21:14

Merge branch 'main' into shilkagg/fix-eks

89ecdf6

mergify bot merged commit 73744b4 into aws:main Mar 27, 2025
20 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Mar 27, 2025

	/**
	* If this is set to true, don't error out on trying to select subnets
	*/
	protected incompleteSubnetDefinition: boolean = false;

fix(eks): looked up vpc causing premature validation errors for private subnets #33786

fix(eks): looked up vpc causing premature validation errors for private subnets #33786

Uh oh!

Conversation

shikha372 commented Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue # (if applicable)

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

Uh oh!

codecov bot commented Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

iliapolo Mar 19, 2025

Choose a reason for hiding this comment

Subnet Selection

incompleteSubnetDefinition

Uh oh!

shikha372 Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iliapolo commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iliapolo Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

shikha372 Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

iliapolo Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

iliapolo Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

shikha372 Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

godwingrs22 Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

shikha372 Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

godwingrs22 left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Mar 27, 2025

Uh oh!

shikha372 commented Mar 27, 2025

Uh oh!

mergify bot commented Mar 27, 2025

☑️ Nothing to do

Uh oh!

aws-cdk-automation commented Mar 27, 2025

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Mar 27, 2025

Uh oh!

Uh oh!

github-actions bot commented Mar 27, 2025

Uh oh!

Reviewers

Assignees

shikha372 commented Mar 15, 2025 •

edited

Loading

codecov bot commented Mar 15, 2025 •

edited

Loading

shikha372 Mar 19, 2025 •

edited

Loading

iliapolo commented Mar 19, 2025 •

edited

Loading

shikha372 Mar 24, 2025 •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading