Skip to content
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

aws-ec2: SubnetFilter byIds does not work as expected when using vpc.selectSubnets() #24427

Closed
marcandjulien opened this issue Mar 2, 2023 · 3 comments · Fixed by #24625
Closed
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@marcandjulien
Copy link

Describe the bug

When creating a subnet selection with only the SubnetFilter.byIds, the selection if empty if the subnets are not of type PRIVATE_WITH_EGRESS.

Expected Behavior

Being able to select subnet by ids in a subnet selection when using SubnetFilter.byIds.

Current Behavior

When creating a subnet selection with only the SubnetFilter.byIds, the selection if empty if the subnets are not of type PRIVATE_WITH_EGRESS.

Reproduction Steps

Try a subnet selection with a SubnetFilter.byIds on a vpc without subnet of type PRIVATE_WITH_EGRESS.

Possible Solution

Current code

  /**
   * From https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-ec2/lib/vpc.ts
   */

  /**
   * Returns IDs of selected subnets
   */
  public selectSubnets(selection: SubnetSelection = {}): SelectedSubnets {
    const subnets = this.selectSubnetObjects(selection);
    const pubs = new Set(this.publicSubnets);

    return {
      subnetIds: subnets.map(s => s.subnetId),
      get availabilityZones(): string[] { return subnets.map(s => s.availabilityZone); },
      internetConnectivityEstablished: tap(new CompositeDependable(), d => subnets.forEach(s => d.add(s.internetConnectivityEstablished))),
      subnets,
      hasPublic: subnets.some(s => pubs.has(s)),
      isPendingLookup: this.incompleteSubnetDefinition,
    };
  }

  protected selectSubnetObjects(selection: SubnetSelection = {}): ISubnet[] {
    selection = this.reifySelectionDefaults(selection);

    if (selection.subnets !== undefined) {
      return selection.subnets;
    }

    let subnets;

    if (selection.subnetGroupName !== undefined) { // Select by name
      subnets = this.selectSubnetObjectsByName(selection.subnetGroupName);

    } else { // Or specify by type
      const type = selection.subnetType || SubnetType.PRIVATE_WITH_EGRESS;
      subnets = this.selectSubnetObjectsByType(type);
    }

    // Apply all the filters
    subnets = this.applySubnetFilters(subnets, selection.subnetFilters ?? []);

    return subnets;
  }

I propose creating a new function selectAllSubnetObjects applying a patch on the selectSubnetObjects function:

  private selectAllSubnetObjects() {
    const allSubnets = [...this.publicSubnets, ...this.privateSubnets, ...this.isolatedSubnets];
    return allSubnets;
  }

  protected selectSubnetObjects(selection: SubnetSelection = {}): ISubnet[] {
    selection = this.reifySelectionDefaults(selection);

    if (selection.subnets !== undefined) {
      return selection.subnets;
    }

    let subnets;

    if (selection.subnetGroupName !== undefined) { // Select by name
      subnets = this.selectSubnetObjectsByName(selection.subnetGroupName);
    } else if (selection.subnetType) { // Or specify by type
      const type = selection.subnetType;
      subnets = this.selectSubnetObjectsByType(type);
    } else {
      subnets = this.selectAllSubnetObjects();
    }

    // Apply all the filters
    subnets = this.applySubnetFilters(subnets, selection.subnetFilters ?? []);

    return subnets;
  }

Additional Information/Context

No response

CDK CLI Version

2.60.0 (build 2d40d77)

Framework Version

No response

Node.js Version

v16.17.0

OS

Windows 11

Language

Typescript

Language Version

No response

Other information

No response

@marcandjulien marcandjulien added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 2, 2023
@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Mar 2, 2023
@khushail khushail added needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. labels Mar 2, 2023
@pahud
Copy link
Contributor

pahud commented Mar 2, 2023

Yes I can reproduce this bug.

    const vpc = ec2.Vpc.fromLookup(this, 'Vpc', { isDefault: true });
    const subnets = vpc.selectSubnets({
      // select a public subnet by IDs
      // subnetFilters: [ ec2.SubnetFilter.byIds(['subnet-050929f8c6efb7657'])]
      // select a private subnet by IDs
      subnetFilters: [ ec2.SubnetFilter.byIds(['subnet-0d0069776469b2e0d'])]
    })
    
    new cdk.CfnOutput(this, 'SubnetIds', { value: subnets.subnetIds.join(',') })

And I think we probably should fix here:

const type = selection.subnetType || SubnetType.PRIVATE_WITH_EGRESS;

@pahud pahud added p1 effort/small Small work item – less than a day of effort and removed needs-reproduction This issue needs reproduction. labels Mar 2, 2023
@ahmetsoykan
Copy link
Contributor

Hello,

i think subnet type selection is also happening in another function; and when tried with filtering byID it resulted with empty array with other vpc setups too.

if (placement.subnetType === undefined && placement.subnetGroupName === undefined && placement.subnets === undefined) {

by reproducing the case, i can not create outputs because subnets.subnetIds is empty array.

const vpc = new Vpc(this, "VPC", {
  subnetConfiguration: [
    { subnetType: SubnetType.PUBLIC, name: "BlaBla" },
    { subnetType: SubnetType.PRIVATE_ISOLATED, name: "DontTalkAtAll" },
  ],
});
const subnets = vpc.selectSubnets({});

console.log(subnets.subnetIds);

subnets.subnetIds.forEach((item, index) => {
  new cdk.CfnOutput(this, `SubnetID${index}`, {
    value: item,
  });
});

from my understanding, is it doing filtering right, but when the subnetIDs are tokenised, it resulted with empty because there was no matches. example subnetIds: [ '${Token[TOKEN.622]}', '${Token[TOKEN.638]}' ].

return subnets.filter(subnet => this.subnetIds.includes(subnet.subnetId));

Then tried this code block, it can be resolved as string.

 const vpc = new Vpc(this, "VPC", {
  subnetConfiguration: [
    { subnetType: SubnetType.PUBLIC, name: "BlaBla" },
    { subnetType: SubnetType.PRIVATE_ISOLATED, name: "DontTalkAtAll" },
  ],
});
const subnets = vpc.selectSubnets({});

console.log(cdk.Token.asString(subnets.subnets[0].subnetId));

new cdk.CfnOutput(this, `SubnetID1`, {
  value:  cdk.Token.asString(subnets.subnets[0].subnetId),
});

As this is my first conversation, can you please review my findings, @pahud . thank you.

@mergify mergify bot closed this as completed in #24625 Mar 29, 2023
mergify bot pushed a commit that referenced this issue Mar 29, 2023
…ter returns an empty array (#24625)

> Fix for SubnetFilter.byId()
>
> fix for returning an empty array after this SubnetFilter.byId() applied

Closes #24427.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
4 participants