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

Cannot find right method to import existing vpc with ec2.Vpc.fromLookup or ec2.Vpc.fromVpcAttributes #12160

Closed
pandrel opened this issue Dec 19, 2020 · 27 comments · Fixed by #12569
Labels
documentation This is a problem with documentation. feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged.

Comments

@pandrel
Copy link

pandrel commented Dec 19, 2020

Issue:
I am trying to import an existing vpc to deploy few lambda, ecs and albs in private subnets. Running into issues when trying to import the vpc using ec2.Vpc.fromLookup or ec2.Vpc.fromVpcAttributes.

The existing vpcid, azs, privatesubnets are exported as
dev-vpcId. - value: 'vpc-xxxxx'
dev-azs - value: 'us-east-1a,us-east-1b'
dev-privateSubnets - 'subnet-xxxx,subnet-yyyy'

    const vpc = ec2.Vpc.fromVpcAttributes(this,'vpc',{
      vpcId: Fn.importValue(envName+'-vpcId'),
      availabilityZones: Fn.split(',',Fn.importValue(envName+'-azs')),
      privateSubnetIds: Fn.split(',',Fn.importValue(envName+'-privateSubnets')),
    })

Error

...../node_modules/@aws-cdk/core/lib/private/resolve.ts:94
      throw new Error('Found an encoded list token string in a scalar string context. Use \'Fn.select(0, list)\' (not \'list[0]\') to extract elements from token lists.');
            ^
Error: Found an encoded list token string in a scalar string context. Use 'Fn.select(0, list)' (not 'list[0]') to extract elements from token lists.

with ec2.Vpc.fromLookup() we cannot use import values

    const vpc = ec2.Vpc.fromLookup(this, 'vpc',{
      vpcId: Fn.importValue(envName+'-vpcId')
    })

Error

..../node_modules/@aws-cdk/aws-ec2/lib/vpc.ts:1040
      routerId: natGatewayId,
            ^
Error: All arguments to Vpc.fromLookup() must be concrete (no Tokens)

Please advise on the right method to import an existing vpc with details from the export values.


This is a 📕 documentation issue

@pandrel pandrel added documentation This is a problem with documentation. feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Dec 19, 2020
@redbaron
Copy link
Contributor

Fn.importValue executed during CloudFormation run , but ec2.Vpc.fromLookup runs before CF stack is submitted.

I'd try to give up Fn.importValue and hardcode map { dev: 'vpc-xxx', prod: 'vpc-yyyy' } and use these directly in ec2.Vpc.fromLookup

@pandrel
Copy link
Author

pandrel commented Dec 21, 2020

Hardcoded values for vpc and subnets do work, but i cannot use them for automation. The values of the subnets and vpc's are in the export values i need a way to import them in my current cdk stack. Please advise ?

@michaelwiles
Copy link
Contributor

Is the vpc you want to use also created by cdk? It is possible if this the case, do reference the vpc from the separate stack directly.

From the docs for from_lookup:

This function only needs to be used to use VPCs not defined in your CDK application. If you are looking to share a VPC between stacks, you can pass the Vpc object between stacks and use it as normal.

@pandrel
Copy link
Author

pandrel commented Dec 21, 2020

Yes, the vpc is created by CDK but by a totally different codecommmit repo.

My main requirement is to import a vpc with private subnets specifically via exported values.

The below appears to be working, but i am assuming here there are always two AZs. If the scenario changes in future based on environment, this code will no longer work.

    const envName = props.envName;
    const vpcId = Fn.importValue(envName+"-vpcId");
    const availabilityZones = Fn.split(",",Fn.importValue(envName+"-azs"));
    const privateSubnetIds = Fn.split(",", Fn.importValue(envName+"-privateSubnets"));

    const vpc = ec2.Vpc.fromVpcAttributes(this, "importedVpc", {
      vpcId: vpcId,
      availabilityZones: availabilityZones,
      privateSubnetIds: [ Fn.select(0,privateSubnetIds), Fn.select(1,privateSubnetIds)],
    }); 

I am not sure why Fn.split(",", Fn.importValue(envName+"-privateSubnets")) is not acceptable to privateSubnetIds in the ec2.Vpc.fromVpcAttributes even though its a string[] type

@pandrel
Copy link
Author

pandrel commented Dec 31, 2020

Please advise if there is any update on this issue / recommendation on importing an existing vpc without hardcoding the vpcid and subnetid's.

@adriantaut
Copy link
Contributor

adriantaut commented Jan 5, 2021

struggling with the same problem with the newer versions of CDK (1.82.0), when attempting to spin up an EKS cluster. It might be related with #11945 since this worked with 1.76.0. Maybe it's worth mentioning, the VPC has both public/private subnets, thus public/private Routing Tables. Please find my code below:

const vpc = Vpc.fromVpcAttributes(scope, 'DefaultVpc', {
    vpcId: ssm.StringParameter.valueForStringParameter(scope, `${basePath}/VpcId`),
    availabilityZones: ssm.StringParameter.valueForStringParameter(scope, `${basePath}/AvailabilityZones`),
    privateSubnetIds: ssm.StringParameter.valueForStringParameter(scope, `${basePath}/PrivateSubnetIds`),
    privateSubnetRouteTableIds: ssm.StringParameter.valueForStringParameter(scope, `${basePath}/PrivateSubnetRouteTableIds`),
    publicSubnetIds: ssm.StringParameter.valueForStringParameter(scope, `${basePath}/PublicSubnetIds`),
    publicSubnetRouteTableIds: ssm.StringParameter.valueForStringParameter(scope, `${basePath}/PublicSubnetRouteTableIds`),
  });

const cluster = new eks.Cluster(this, 'DetestadrCluster', {
     vpc: vpc,
     defaultCapacity: 0,
     version: eks.KubernetesVersion.V1_18
});

Throwing the same error when resolving the template:

/Users/tauta/gitlab/aws/2.0aws/cdk/detestadr-configrepo-aws-ppb/cdk/node_modules/@aws-cdk/core/lib/private/resolve.ts:94
      throw new Error('Found an encoded list token string in a scalar string context. Use \'Fn.select(0, list)\' (not \'list[0]\') to extract elements from token lists.');

In addition, spinning an ECS cluster has no issues:

        const ecsCluster = new ecs.Cluster(this, 'EcsCluster', {
            clusterName: `blablablaCluster`,
            vpc: vpc,
        });

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 18, 2021

Please advise if there is any update on this issue / recommendation on importing an existing vpc without hardcoding the vpcid and subnetid's.

Tag your VPC and use fromLookup to look up by tag.

@adriantaut
Copy link
Contributor

@rix0rrr the fromLookup() method does not work for cross-account deployments...at least in my use case, we are trying to deploy from a tooling account to dev/prod accounts

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 18, 2021

For now, that's unfortunately true.

Preload the right credentials into your shell while you run cdk synth, and be sure to commit cdk.context.json. That'll unblock you.

@adriantaut
Copy link
Contributor

adriantaut commented Jan 18, 2021

we are using a CodePipeline-based CI/CD workflow...and committing the credentials in github is not a solution. I can see this was addressed and partially fixed in #11945 , unfortunately I cannot figure it out what is still missing

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 18, 2021

You're not committing the credentials, you are committing the result of the lookups.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 18, 2021

Generally, using ec2.Vpc.fromVpcAttributes with deploy-time values for the subnet IDs (parameters and Fn.importValues) is not safe and cannot work.

We don't go ahead and error out when you try to do this, because it accidentally used to work for some special cases (notably, AutoScalingGroups) and we didn't want to break those existing cases.

In all other cases, the only correct way to do this is to use Vpc.fromLookup, and committing cdk.context.json.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 18, 2021

@adriantaut I am unsure why this would even compile:

    privateSubnetIds: ssm.StringParameter.valueForStringParameter(scope, `${basePath}/PrivateSubnetIds`),

Given that ssm.StringParameter.valueForStringParameter returns a string, and privateSubnetIds expects a string[]. Are you using JavaScript by any chance?

mergify bot pushed a commit that referenced this issue Jan 18, 2021
Many people don't commit or want to commit `cdk.context.json`, and
are subsequently annoyed that they can't use `Vpc.fromLookup()` in a
pipeline.

Make it very clear that this is the way CDK is designed to work and
they must do it.

Relates to #12160

----

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

@rix0rrr my mistake, was just trying to make the code as easy as possible, the actual implementation relies on a getVpc() method we have and it looks like:

/**
 * Return the default VPC in any account, relying on SSM parameters created by the Cloud Foundations VPC stack (/platform/vpc/*)
 */
export function getVpc(scope: Construct, vpcName?: string, usePublic?: boolean) {
  const vpc = vpcName ?? 'default';
  const basePath = `/platform/vpc/${vpc}`;

  let publicSubnetIds = undefined;
  let publicSubnetRouteTableIds = undefined;

  const getStringListParam = (name: string) => {
    return Fn.split(',', ssm.StringParameter.valueForStringParameter(scope, `${basePath}/${name}`));
  };

  if (usePublic ?? false) {
    publicSubnetIds = getStringListParam('PublicSubnetIds');
    publicSubnetRouteTableIds = getStringListParam('PublicSubnetRouteTableIds');
  }

  return Vpc.fromVpcAttributes(scope, 'DefaultVpc', {
    vpcId: ssm.StringParameter.valueForStringParameter(scope, `${basePath}/VpcId`),
    // TODO: this will only work if there is a subnet in each AZ within the region
    availabilityZones: getStringListParam('AvailabilityZones'),
    privateSubnetIds: getStringListParam('PrivateSubnetIds'),
    privateSubnetRouteTableIds: getStringListParam('PrivateSubnetRouteTableIds'),
    publicSubnetIds,
    publicSubnetRouteTableIds,
  });
}

And this was very appreciated with CDK and helpful having the ability to synthesize account-agnostic CFN templates.

@adriantaut
Copy link
Contributor

during the AWS account creation we are creating some VPCs and pushing it's parameters to SSM, then people with this method can get their VPC by name. This was developed having in mind the CDK Pipelines limitation https://docs.aws.amazon.com/cdk/api/latest/docs/pipelines-readme.html#current-limitations.

Having this there is no need for local permissions or the need to assume cross-account roles for fetching all the VPC info in dev/prod accounts during local synth time, eventually there is no need at all to synth locally since this happens in the Synth stage of the CDK Pipelines also.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 18, 2021

I am seriously confused about why the code you're showing me would ever have worked.

The following line of code would make it so that it never could have, and it has been in there for 6 months.

https://github.com/aws/aws-cdk/blame/master/packages/@aws-cdk/aws-eks/lib/cluster.ts#L962

That is, unless:

  • You've been using it with an imported VPC that only had public OR private subnets (not both)
  • You've been constructing eks.Cluster with a vpcSubnets parameter that only selects public OR private subnets (not more than one).

As soon as that line is trying to select more than one tokenized list of subnets, the code cannot have worked.

@adriantaut
Copy link
Contributor

adriantaut commented Jan 18, 2021

now I'm also confused, indeed, we were only using VPCs with only private subnets and we never had the chance to test it with public subnets since the VPCs are all private

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 18, 2021

Okay, that gives me something to go on. Thanks.

rix0rrr added a commit that referenced this issue Jan 18, 2021
This PR has a fix similar to #12040, and introduces more explanations
and safeguards around the mechanism of importing a VPC using
`fromVpcAttributes()`. This is not the recommended way of importing
VPCs, but many users really don't want to use lookups so we'd better
make it a little safer.

This PR contains:

* A fix in the EKS library to have it stop logging subnet IDs to
  metadata if it looks like the subnet ID will lead to the
  aforementioned synthesis error (similar to the fix in the linked
  PR).
* A validation in the EKS library to stop a similar-but-different
  error from occurring if people select multiple VPC subnet groups
  from a VPC imported from token lists; this can never work and we
  might as well tell them directly.
* A metadata warning added to VPCs imported using
  `Vpc.fromVpcAttributes()`, to inform users that their VPC imported
  in this way has a good chance of not working in all cases they
  expect it to.
* A mechanism to specify the length of deploy-time lists at synthesis
  time, by passing an `assumedLength` parameter to `Fn.split()`. This
  will produce a list that is safe for manipulation.
* A note on how to use `Vpc.fromVpcAttributes()` in the `ec2` README.

Fixes #12160.
@mergify mergify bot closed this as completed in #12569 Jan 19, 2021
mergify bot pushed a commit that referenced this issue Jan 19, 2021
This PR has a fix similar to #12040, and introduces more explanations
and safeguards around the mechanism of importing a VPC using
`fromVpcAttributes()`. This is not the recommended way of importing
VPCs, but many users really don't want to use lookups so we'd better
make it a little safer.

This PR contains:

* A fix in the EKS library to have it stop logging subnet IDs to
  metadata if it looks like the subnet ID will lead to the
  aforementioned synthesis error (similar to the fix in the linked
  PR).
* A validation in the EKS library to stop a similar-but-different
  error from occurring if people select multiple VPC subnet groups
  from a VPC imported from token lists; this can never work and we
  might as well tell them directly.
* A metadata warning added to VPCs imported using
  `Vpc.fromVpcAttributes()`, to inform users that their VPC imported
  in this way has a good chance of not working in all cases they
  expect it to.
* A mechanism to specify the length of deploy-time lists at synthesis
  time, by passing an `assumedLength` parameter to `Fn.split()`. This
  will produce a list that is safe for manipulation, and removes the limitations
  addressed by the previous bullets.
* A note on how to use `Vpc.fromVpcAttributes()` in the `ec2` README.

Fixes #12160.


----

*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.

@pandrel
Copy link
Author

pandrel commented Jan 21, 2021

@rix0rrr, my original question was not related to EKS at all, it is in general how to import an already existing vpc without hardcoding the vpcid in the code. Specifically either passing the values of vpcid, private subnet id's via CFN Export values or SSM params.

It appears the issue was closed incorrectly related to a different issue about EKS.

mohanrajendran pushed a commit to mohanrajendran/aws-cdk that referenced this issue Jan 24, 2021
Many people don't commit or want to commit `cdk.context.json`, and
are subsequently annoyed that they can't use `Vpc.fromLookup()` in a
pipeline.

Make it very clear that this is the way CDK is designed to work and
they must do it.

Relates to aws#12160

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mohanrajendran pushed a commit to mohanrajendran/aws-cdk that referenced this issue Jan 24, 2021
This PR has a fix similar to aws#12040, and introduces more explanations
and safeguards around the mechanism of importing a VPC using
`fromVpcAttributes()`. This is not the recommended way of importing
VPCs, but many users really don't want to use lookups so we'd better
make it a little safer.

This PR contains:

* A fix in the EKS library to have it stop logging subnet IDs to
  metadata if it looks like the subnet ID will lead to the
  aforementioned synthesis error (similar to the fix in the linked
  PR).
* A validation in the EKS library to stop a similar-but-different
  error from occurring if people select multiple VPC subnet groups
  from a VPC imported from token lists; this can never work and we
  might as well tell them directly.
* A metadata warning added to VPCs imported using
  `Vpc.fromVpcAttributes()`, to inform users that their VPC imported
  in this way has a good chance of not working in all cases they
  expect it to.
* A mechanism to specify the length of deploy-time lists at synthesis
  time, by passing an `assumedLength` parameter to `Fn.split()`. This
  will produce a list that is safe for manipulation, and removes the limitations
  addressed by the previous bullets.
* A note on how to use `Vpc.fromVpcAttributes()` in the `ec2` README.

Fixes aws#12160.


----

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

@pandrel Have you found the fix for this problem. I am currently facing the same issue.

@namedgraph
Copy link

namedgraph commented Sep 6, 2021

@rix0rrr I'm building a CF template with CDK that I want to use in the AWS Marketplace, therefore I need to get a VPC from user input.
Are you saying the recommended method is to ask the user (make a CfnParameter) for a tag value and use it in Vpc.fromLookup? But a tag value is not strongly typed unlike AWS::EC2::VPC::Id, so it seems to me like an inferior approach in terms of usability that leaves much more room for user error?

I was hoping for this code to work, but I get An exception occured while executing the Java class. Resolution error: Resolution error: Found an encoded list token string in a scalar string context. Use 'Fn.select(0, list)' (not 'list[0]') to extract elements from token lists... error instead:

CfnParameter vpcId = new CfnParameter(this, "VPC_ID");
vpcId.setType("AWS::EC2::VPC::Id");
CfnParameter availabilityZones = new CfnParameter(this, "AVAILABILITY_ZONES");
availabilityZones.setType("List<AWS::EC2::AvailabilityZone::Name>");
CfnParameter publicSubnetIds = new CfnParameter(this, "PUBLIC_SUBNET_IDS");
publicSubnetIds.setType("List<AWS::EC2::Subnet::Id>");

IVpc vpc = Vpc.fromVpcAttributes(this, "LDHVPC", VpcAttributes.builder().
    vpcId(vpcId.getValueAsString()).
    availabilityZones(availabilityZones.getValueAsList()).
    publicSubnetIds(publicSubnetIds.getValueAsList()).
    build());

@pandrel-aws
Copy link

pandrel-aws commented Sep 7, 2021

@pandrel Have you found the fix for this problem. I am currently facing the same issue.

I am using the ec2.Vpc.fromLookup() with tags option to import an existing vpc.

example:

    const envName = 'dev' // passed from construct stack props but using fixed value for example
    const vpc = ec2.Vpc.fromLookup(this, 'vpc', {
      tags: { 'envName': envName}
    })

@pandrel-aws
Copy link

@rix0rrr I'm building a CF template with CDK that I want to use in the AWS Marketplace, therefore I need to get a VPC from user input.
Are you saying the recommended method is to ask the user (make a CfnParameter) for a tag value and use it in Vpc.fromLookup? But a tag value is not strongly typed unlike AWS::EC2::VPC::Id, so it seems to me like an inferior approach in terms of usability that leaves much more room for user error?

I was hoping for this code to work, but I get An exception occured while executing the Java class. Resolution error: Resolution error: Found an encoded list token string in a scalar string context. Use 'Fn.select(0, list)' (not 'list[0]') to extract elements from token lists... error instead:

CfnParameter vpcId = new CfnParameter(this, "VPC_ID");
vpcId.setType("AWS::EC2::VPC::Id");
CfnParameter availabilityZones = new CfnParameter(this, "AVAILABILITY_ZONES");
availabilityZones.setType("List<AWS::EC2::AvailabilityZone::Name>");
CfnParameter publicSubnetIds = new CfnParameter(this, "PUBLIC_SUBNET_IDS");
publicSubnetIds.setType("List<AWS::EC2::Subnet::Id>");

IVpc vpc = Vpc.fromVpcAttributes(this, "LDHVPC", VpcAttributes.builder().
    vpcId(vpcId.getValueAsString()).
    availabilityZones(availabilityZones.getValueAsList()).
    publicSubnetIds(publicSubnetIds.getValueAsList()).
    build());

If you are already using CfnParameter to provide vpcId, then you can use ec2.Vpc.fromlookup() with explicit vpcid option.

    const vpcId = 'vpc-xxxxx'
    const vpc = ec2.Vpc.fromLookup(this, 'vpc', {
      vpcId: vpcId
    })

Any specific reason you are adding the extra attributes ?

@namedgraph
Copy link

namedgraph commented Sep 7, 2021

@pandrel that was the first thing I tried:

CfnParameter vpcId = new CfnParameter(this, "VPC_ID");
vpcId.setType("AWS::EC2::VPC::Id");
IVpc vpc = Vpc.fromLookup(this, "LDHVPC", VpcLookupOptions.builder().
    vpcId(vpcId.getValueAsString()).
    build());

That gives an error Vpc.fromLookup() must be concrete (no Tokens). Issue #3600 is about that.

@pandrel-aws
Copy link

My mistake, repeated the scenario and tested again .. got the same error as mentioned by you.

@drock
Copy link

drock commented Oct 26, 2022

@rix0rrr the fromLookup() method does not work for cross-account deployments...at least in my use case, we are trying to deploy from a tooling account to dev/prod accounts

You can get around this by synthesizing the stack from your local environment. The lookup will be triggered and the results cached in the cdk.context.json file. Commit that file. Then, in your pipeline runs, they will use that cached value instead of actually performing the lookup into AWS and thus you won't need to worry about the cross account permissions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This is a problem with documentation. feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants