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

Unable to use deploy-time parameters for privateSubnetIds for Vpc.fromVpcAttributes #4118

Open
amwill04 opened this issue Sep 17, 2019 · 13 comments
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@amwill04
Copy link

Unable to pass CommaDelimitedList CfnParameter to VPC.fromVpcAttributes privateSubnetIds

Reproduction Steps

When trying to use existing VPC for a lambda function

const vpcSubnetId = new CfnParameter(this, 'VPCSubnetId', {
            description: 'VPC Subnet Id',
            type: 'CommaDelimitedList',
});
const vpc = Vpc.fromVpcAttributes(this, 'GeoVPC', {
       vpcId: vpcId.valueAsString,
       availabilityZones: ['eu-west-2a', 'eu-west-2b', 'eu-west-2c'],
       privateSubnetIds: vpcSubnetId.valueAsList,
});

When runing npn run build npx synth for a stack with the above in it it will declare the following error:

Number of privateSubnetIds (1) must be a multiple of availability zones (3).

Environment

  • **CLI Version :**1.8.0
  • **Framework Version:**1.8.0
  • **OS :**MacOs 10.14.6
  • **Language :**typescript
@amwill04 amwill04 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 17, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 18, 2019

It would work for now if you made the AZs an argument as well.

@rix0rrr rix0rrr removed the needs-triage This issue or PR still needs to be triaged. label Sep 18, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 18, 2019

The solution is for the VPC construct to not validate anything about subnets if the input is a token.

@amwill04
Copy link
Author

amwill04 commented Sep 18, 2019

@rix0rrr - thanks.

I actually stumbled by accident across doing that last night and the following works.

const mapping = new CfnMapping(this, 'StackMapping', {
    mapping: {
        VPC: {
            vpcAvailabilityZones: ['eu-west-2a', 'eu-west-2b', 'eu-west-2c'],
        },
    },
});

const vpc = Vpc.fromVpcAttributes(this, 'GeoVPC', {
    vpcId: vpcId.valueAsString,
     availabilityZones: [...Token.asList(mapping.findInMap('VPC', 'vpcAvailabilityZones'))],
     privateSubnetIds: [...Token.asList(vpcSubnetId.valueAsList)],
 });

@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 18, 2019

Ha, that works as well. The [...list] part shouldn't be necessary btw.

@rix0rrr rix0rrr added the p2 label Oct 24, 2019
@john-tipper
Copy link

I see the same error for publicSubnetIds:

IVpc vpc = Vpc.fromVpcAttributes(this, "VPC", VpcAttributes.builder()
                .vpcId(vpcId.getValueAsString())
                .publicSubnetIds(publicSubnetIds.getValueAsList())
                .availabilityZones(List.of("eu-west-2a", "eu-west-2b", "eu-west-2c"))
                .build());

@TiltedTower
Copy link

I have gotten this to work by using the cdk.Fn.getAzs() function instead of creating the mapping

@SomayaB SomayaB added @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud and removed package/vpc labels May 27, 2020
@rix0rrr rix0rrr added the effort/medium Medium work item – several days of effort label Aug 12, 2020
@rix0rrr rix0rrr changed the title Unable to use Parameter .valueAsList for privateSubnetIds for Vpc.fromVpcAttributes Unable to use deploy-time parameters for privateSubnetIds for Vpc.fromVpcAttributes Dec 11, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 11, 2020

This does not work, and the reason it does not work is that a lot of the CDK makes assumptions that subnets can be represented an Subnet[] (that is: an array of Subnet objects), and that afterwards we can do things like:

subnets.map(s => s.subnetId)
subnets.map(s => s.routeTableId)

And so on.

Obviously the function that will have to translate:

importSubnets(Fn.importValue('subnetIds')) // => ISubnet[] ????

Is going to be hard to write.


It may accidentally work today. That is because the token:

// Encoded list token
const subnetIds = Token.asList(Fn.importValue('someValue'))
// [ '#{Token[1234]}' ]

// Gets parsed to a singleton `Subnet` list:
const subnets = subnetIds.map(s => Subnet.fromSubnetAttributes({ subnetId: s }));
// [ Subnet({ subnetId: '#{Token[1234]}' }) ]

// This 'subnetId' is illegal by itself, and if yould try to use it for,
// say, an ec2.Instance it would fail. However, if you treat this single
// subnet as a GROUP of subnets:
new CfnAutoScalingGroup({ subnetIds: subnets.map(s => s.subnetId) })
// [ '#{Token[1234]}' ]

// And this resolves back to:
resolve(cfnSubnetIds)
// SubnetIds: { Fn::ImportValue: 'someValue' }

// Again! This is not a feature but an accident.

This needs a lot of hacky code to make work in the current design, but more properly we should take it up as part of the Vpc redesign and introduce a class represent Subnets. This class can have two different implementaions: a ConcreteSubnets class that actually has a list of Subnet objects as today, or a DeployTimeSubnets class that only holds on to the CFN parameter and spits it back out again.

Tackle as part of #5927

rix0rrr added a commit that referenced this issue Dec 12, 2020
…ime lists

Even though using `Vpc.fromVpcAttributes()` using deploy-time lists like
from `Fn.importValue()`s and `CfnParameters` was never really supposed
to work, it accidentally did.

The reason for that is:

```ts

// Encoded list token
const subnetIds = Token.asList(Fn.importValue('someValue'))
// [ '#{Token[1234]}' ]

// Gets parsed to a singleton `Subnet` list:
const subnets = subnetIds.map(s => Subnet.fromSubnetAttributes({ subnetId: s }));
// [ Subnet({ subnetId: '#{Token[1234]}' }) ]

// This 'subnetId' is illegal by itself, and if yould try to use it for,
// say, an ec2.Instance it would fail. However, if you treat this single
// subnet as a GROUP of subnets:
new CfnAutoScalingGroup({ subnetIds: subnets.map(s => s.subnetId) })
// [ '#{Token[1234]}' ]

// And this resolves back to:
resolve(cfnSubnetIds)
// SubnetIds: { Fn::ImportValue: 'someValue' }
```

--------

We introduced an additional check in #11899 to make sure that the
list-element token that represents an encoded list (`'#{Token[1234]}'`)
never occurs in a non-list context, because it's illegal there.

However, because:

* `Subnet.fromSubnetAttributes()` logs the subnetId as a *warning*
  to its own metadata (which will log a string like `"there's something
  wrong with '#{Token[1234]}' ..."`).
* All metadata is resolved just the same as the template expressions
  are.

The `resolve()` function encounters that orphaned list token in the
metadata and throws.

--------

The *proper* solution would be to handle unparseable list tokens
specially to never get into this situation, but doing that requires
introducing classes and new concepts that will be a large effort and
not be backwards compatible. Tracking in #4118.

Another possible solution is to stop resolving metadata. I don't
know if we usefully use this feature; I think we don't. However,
we have tests enforcing that it is being done, and I'm scared
to break something there.

The quick solution around this for now is to have
`Subnet.fromSubnetAttributes()` recognize when it's about to log
a problematic identifier to metadata, and don't do it.

Fixes #11945.
mergify bot pushed a commit that referenced this issue Dec 14, 2020
…ime lists (#12040)

Even though using `Vpc.fromVpcAttributes()` using deploy-time lists like
from `Fn.importValue()`s and `CfnParameters` was never really supposed
to work, it accidentally did.

The reason for that is:

```ts

// Encoded list token
const subnetIds = Token.asList(Fn.importValue('someValue'))
// [ '#{Token[1234]}' ]

// Gets parsed to a singleton `Subnet` list:
const subnets = subnetIds.map(s => Subnet.fromSubnetAttributes({ subnetId: s }));
// [ Subnet({ subnetId: '#{Token[1234]}' }) ]

// This 'subnetId' is illegal by itself, and if yould try to use it for,
// say, an ec2.Instance it would fail. However, if you treat this single
// subnet as a GROUP of subnets:
new CfnAutoScalingGroup({ subnetIds: subnets.map(s => s.subnetId) })
// [ '#{Token[1234]}' ]

// And this resolves back to:
resolve(cfnSubnetIds)
// SubnetIds: { Fn::ImportValue: 'someValue' }
```

--------

We introduced an additional check in #11899 to make sure that the
list-element token that represents an encoded list (`'#{Token[1234]}'`)
never occurs in a non-list context, because it's illegal there.

However, because:

* `Subnet.fromSubnetAttributes()` logs the subnetId as a *warning*
  to its own metadata (which will log a string like `"there's something
  wrong with '#{Token[1234]}' ..."`).
* All metadata is resolved just the same as the template expressions
  are.

The `resolve()` function encounters that orphaned list token in the
metadata and throws.

--------

The *proper* solution would be to handle unparseable list tokens
specially to never get into this situation, but doing that requires
introducing classes and new concepts that will be a large effort and
not be backwards compatible. Tracking in #4118.

Another possible solution is to stop resolving metadata. I don't
know if we usefully use this feature; I think we don't. However,
we have tests enforcing that it is being done, and I'm scared
to break something there.

The quick solution around this for now is to have
`Subnet.fromSubnetAttributes()` recognize when it's about to log
a problematic identifier to metadata, and don't do it.

Fixes #11945.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
flochaz pushed a commit to flochaz/aws-cdk that referenced this issue Jan 5, 2021
…ime lists (aws#12040)

Even though using `Vpc.fromVpcAttributes()` using deploy-time lists like
from `Fn.importValue()`s and `CfnParameters` was never really supposed
to work, it accidentally did.

The reason for that is:

```ts

// Encoded list token
const subnetIds = Token.asList(Fn.importValue('someValue'))
// [ '#{Token[1234]}' ]

// Gets parsed to a singleton `Subnet` list:
const subnets = subnetIds.map(s => Subnet.fromSubnetAttributes({ subnetId: s }));
// [ Subnet({ subnetId: '#{Token[1234]}' }) ]

// This 'subnetId' is illegal by itself, and if yould try to use it for,
// say, an ec2.Instance it would fail. However, if you treat this single
// subnet as a GROUP of subnets:
new CfnAutoScalingGroup({ subnetIds: subnets.map(s => s.subnetId) })
// [ '#{Token[1234]}' ]

// And this resolves back to:
resolve(cfnSubnetIds)
// SubnetIds: { Fn::ImportValue: 'someValue' }
```

--------

We introduced an additional check in aws#11899 to make sure that the
list-element token that represents an encoded list (`'#{Token[1234]}'`)
never occurs in a non-list context, because it's illegal there.

However, because:

* `Subnet.fromSubnetAttributes()` logs the subnetId as a *warning*
  to its own metadata (which will log a string like `"there's something
  wrong with '#{Token[1234]}' ..."`).
* All metadata is resolved just the same as the template expressions
  are.

The `resolve()` function encounters that orphaned list token in the
metadata and throws.

--------

The *proper* solution would be to handle unparseable list tokens
specially to never get into this situation, but doing that requires
introducing classes and new concepts that will be a large effort and
not be backwards compatible. Tracking in aws#4118.

Another possible solution is to stop resolving metadata. I don't
know if we usefully use this feature; I think we don't. However,
we have tests enforcing that it is being done, and I'm scared
to break something there.

The quick solution around this for now is to have
`Subnet.fromSubnetAttributes()` recognize when it's about to log
a problematic identifier to metadata, and don't do it.

Fixes aws#11945.

----

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

eruvanos commented Jan 20, 2021

Hi,

we faced this issue multiple times, as a workaround we wrap the subnetIds with select statements:


const vpcSubnetIdsRef = Token.asList(Fn.Split(",", Fn.importValue('VPCSubnetId')))
// or
// const vpcSubnetIdsRef = new CfnParameter(this, 'VPCSubnetId', {
//            description: 'VPC Subnet Id',
//            type: 'CommaDelimitedList',
// });

const azs = ['eu-west-2a', 'eu-west-2b', 'eu-west-2c']

const subnetIds = [];
azs.forEach((_, index) => {
        subnetIds.push(Fn.select(index, vpcSubnetIdsRef))
});

const vpc = Vpc.fromVpcAttributes(this, 'GeoVPC', {
       vpcId: vpcId.valueAsString,
       availabilityZones: azs,
       privateSubnetIds: subnetIds,
});

This will create a list with three tokens, which will be resolved in cfn later.

@HannesHasselbring
Copy link

and it can also be used with higher level constructs like rds.ServerlessCluster

@TiltedTower
Copy link

Hi,

we faced this issue multiple times, as a workaround we wrap the subnetIds with select statements:


const vpcSubnetIdsRef = Token.asList(Fn.Split(",", Fn.importValue('VPCSubnetId')))
// or
// const vpcSubnetIdsRef = new CfnParameter(this, 'VPCSubnetId', {
//            description: 'VPC Subnet Id',
//            type: 'CommaDelimitedList',
// });

const azs = ['eu-west-2a', 'eu-west-2b', 'eu-west-2c']

const subnetIds = [];
azs.forEach((_, index) => {
        subnetIds.push(Fn.select(index, vpcSubnetIdsRef))
});

const vpc = Vpc.fromVpcAttributes(this, 'GeoVPC', {
       vpcId: vpcId.valueAsString,
       availabilityZones: azs,
       privateSubnetIds: subnetIds,
});

This will create a list with three tokens, which will be resolved in cfn later.

This worked beautifully. A real lifesaver, thank you!

@wchaws
Copy link
Contributor

wchaws commented Jan 31, 2021

Here is another nice workaround

const vpcIdParam = new CfnParameter(this, 'YourVpcId', { type: 'AWS::EC2::VPC::Id' });
const pubSubnetsParam = new CfnParameter(this, 'PubSubnets', { type: 'List<AWS::EC2::Subnet::Id>' });
const privSubnetsParam = new CfnParameter(this, 'PrivSubnets', { type: 'List<AWS::EC2::Subnet::Id>' });

const azs = ['eu-west-2a', 'eu-west-2b', 'eu-west-2c'];

const vpc = ec2.Vpc.fromVpcAttributes(this, 'VpcAttr', {
  vpcId: vpcIdParam.valueAsString,
  vpcCidrBlock: Aws.NO_VALUE,
  availabilityZones: azs,
  publicSubnetIds: azs.map((_, index) => Fn.select(index, pubSubnetsParam.valueAsList)),
  privateSubnetIds: azs.map((_, index) => Fn.select(index, privSubnetsParam.valueAsList)),
});

@rix0rrr rix0rrr removed their assignment Jun 3, 2021
@github-actions
Copy link

github-actions bot commented Jun 3, 2022

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jun 3, 2022
@namdevmrathod
Copy link

I got resolved this issue by using below code in cdk for lambda function in vpc

import * as iam from 'aws-cdk-lib/aws-iam';
import * as lambda from 'aws-cdk-lib/aws-lambda';
import * as cdk from 'aws-cdk-lib';
import * as path from 'path';
import * as ec2 from '@aws-cdk/aws-ec2'
import { Vpc } from 'aws-cdk-lib/aws-ec2';

export class VpcStack extends cdk.Stack {
constructor(scope: cdk.App, id: string, props?: cdk.StackProps) {
super(scope, id, props);

//get VPC Info form AWS account, FYI we are not rebuilding we are referencing
const DefaultVpc = Vpc.fromVpcAttributes(this, 'vpcdev', {
vpcId: 'vpc-0e0e20c7600fd9b59',
availabilityZones: ['ap-south-1a', 'ap-south-1b'],
privateSubnetIds: ['subnet-0148d982d214587dd', 'subnet-0a68e97507e0c7fae'],
});

//  lambda function definition

const lambdaFunction = new lambda.Function(this, 'lambda-function', {
functionName: 'lambda-function',
runtime: lambda.Runtime.NODEJS_14_X,
vpc: DefaultVpc,
memorySize: 512,
timeout: cdk.Duration.seconds(10),
handler: 'index.handler',
code: lambda.Code.fromAsset(path.join(__dirname, '/../src/')),
environment: {},
});
}
}

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jun 8, 2022
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/medium Medium work item – several days of effort p2
Projects
None yet
Development

No branches or pull requests

9 participants