(aws-ec2): Vpc.from_lookup does not work out of the box on init templates

[hornetmadness]

This sample code that shows the env settings seem to be ignored when the VPCImport2 class is ran. If I comment out the import_vpc_stack line, it runs fine and creates the VPC. But when using the vpc.from_lookup function is always fails. This code is just a representation of trying to use vpc.from_import and showing that the env actually works.

Reproduction Steps

from aws_cdk import core, aws_ec2 as ec2

app = core.App()
vpc_id = "vpc-08c949884e65e3b6f"
#env = core.Environment(region="us-west-1",account="000")
env = {"region": "us-west-1", "account": "000"}


class addvpc(core.Stack):
    def __init__(
        self,
        scope: core.Construct,
        id: str,
        env: dict,
        **kwargs,
    ) -> None:
        super().__init__(scope, id, **kwargs)
        self.vpc = ec2.Vpc(
            self,
            id=f"Just-a-test-vpc",
            max_azs=2,
            cidr="192.168.0.0/16",
        )


class VPCImport2(core.Stack):
    def __init__(
        self, scope: core.Construct, id: str, env: dict, vpc_id: str, **kwargs
    ) -> None:
        super().__init__(scope, id, **kwargs)
        print(env)
        vpc = ec2.Vpc.from_lookup(
            self,
            "vpc_imported",
            is_default=False,
            vpc_id=vpc_id,
        )


vpc_stack = addvpc(scope=app, id="Addfakevpc", env=env)
import_vpc_stack = VPCImport2(scope=app, id="vpc-import", env=env, vpc_id=vpc_id)

app.synth()

What did you expect to happen?

The VPC that was just created would get imported,

What actually happened?

Error: Cannot retrieve value from context provider vpc-provider since account/region are not specified at the stack level. Either configure "env" with explicit account and region when you define your stack, or use the environment variables "CDK_DEFAULT_ACCOUNT" and "CDK_DEFAULT_REGION" to inherit environment information from the CLI (not recommended for production stacks)

cdk list -v
...
Setting "CDK_DEFAULT_REGION" environment variable to us-west-1
Resolving default credentials
Retrieved account ID 000 from disk cache
Setting "CDK_DEFAULT_ACCOUNT" environment variable to 000
context: {
  'availability-zones:account=000:region=us-west-1': [ 'us-west-1a', 'us-west-1c' ],
....

Environment

• CDK CLI Version : 1.8.1
• Framework Version:
• Node.js Version: v14.15.1
• OS : Debian Buster
• Language (Version): Python (3.9.1)

Other

I feel like the error that is being thrown is not the real error but is some other underlyaing problem as I cant believe that this would stop working just for one certain function.

---

This is 🐛 Bug Report

[webdog]

I am experiencing the same issue as @hornetmadness on a typescript CDK project

CDK: 1.83.0
OS: Ubuntu 20.04 LTS
Node: 12.19.1
Typescript: 3.9.7

Determining if we're on an EC2 instance.
Does not look like an EC2 instance.
Toolkit stack: CDKToolkit
Setting "CDK_DEFAULT_REGION" environment variable to us-east-1
Resolving default credentials
Retrieved account ID REDACTED from disk cache
Setting "CDK_DEFAULT_ACCOUNT" environment variable to REDACTED
context: {
  '@aws-cdk/core:enableStackNameDuplicates': 'true',
  'aws-cdk:enableDiffNoFail': 'true',
  '@aws-cdk/core:stackRelativeExports': 'true',
  '@aws-cdk/aws-ecr-assets:dockerIgnoreSupport': true,
  '@aws-cdk/aws-secretsmanager:parseOwnedSecretName': true,
  '@aws-cdk/aws-kms:defaultKeyPolicies': true,
  'aws:cdk:enable-path-metadata': true,
  'aws:cdk:enable-asset-metadata': true,
  'aws:cdk:version-reporting': true,
  'aws:cdk:bundling-stacks': []
}
outdir: cdk.out
env: {
  CDK_DEFAULT_REGION: 'us-east-1',
  CDK_DEFAULT_ACCOUNT: 'REDACTED',
  CDK_CONTEXT_JSON: '{"@aws-cdk/core:enableStackNameDuplicates":"true","aws-cdk:enableDiffNoFail":"true","@aws-cdk/core:stackRelativeExports":"true","@aws-cdk/aws-ecr-assets:dockerIgnoreSupport":true,"@aws-cdk/aws-secretsmanager:parseOwnedSecretName":true,"@aws-cdk/aws-kms:defaultKeyPolicies":true,"aws:cdk:enable-path-metadata":true,"aws:cdk:enable-asset-metadata":true,"aws:cdk:version-reporting":true,"aws:cdk:bundling-stacks":[]}',
  CDK_OUTDIR: 'cdk.out',
  CDK_CLI_ASM_VERSION: '7.0.0',
  CDK_CLI_VERSION: '1.83.0'
}
Cannot retrieve value from context provider vpc-provider since account/region are not specified at the stack level. Either configure "env" with explicit account and region when you define your stack, or use the environment variables "CDK_DEFAULT_ACCOUNT" and "CDK_DEFAULT_REGION" to inherit environment information from the CLI (not recommended for production stacks)
Subprocess exited with error 1

According to the Environment Guide

but to determine the target at synthesis time, your stack can use two environment variables provided by the AWS CDK CLI: CDK_DEFAULT_ACCOUNT and CDK_DEFAULT_REGION. These variables are set based on the AWS profile specified using the --profile option, or the default AWS profile if you don't specify one.

It appears that we advise users to explicitly import environment variables inside of the Stack invocation, and this works.

new MyDevStack(app, 'dev', { 
  env: { 
    account: process.env.CDK_DEFAULT_ACCOUNT, 
    region: process.env.CDK_DEFAULT_REGION 
}});

Output of cdk list -v:

env: {
  CDK_DEFAULT_REGION: 'us-east-1',
  CDK_DEFAULT_ACCOUNT: 'REDACTED',
  CDK_CONTEXT_JSON: '{"vpc-provider:account=REDACTED:filter.isDefault=true:region=us-east-1:returnAsymmetricSubnets=true":{"vpcId":"REDACTED","vpcCidrBlock":"REDACTED","availabilityZones":
...
  CDK_OUTDIR: 'cdk.out',
  CDK_CLI_ASM_VERSION: '7.0.0',
  CDK_CLI_VERSION: '1.83.0'
}

To me, there is a subtle, possibly opaque issues here: What is the difference between the CDK using an inherited CLI value (As per the docs), and the consumer/user setting these environment variables for CDK? If CDK "inherits" values from the CLI only when the user retrieves them inside the code, that doesn't feel like inheritance to me. That feels like a property being set by the user, from a retrieved value.

It seems we've had this discussion before and updated our guidance, from this feedback, but I don't think we've resolved the issue with clarity here.

I see a couple of resolution paths here:

• Update the error messaging to remove the references to setting environment variables, and instead refer to the Environment documentation which defines the expected behavior of explicit definition inside the stack invocation.
• Update the CDK behavior to utilize the environment variables it has implicitly discovered as reported in the debug log.(Personally in favor of this approach, it's convenient, and a common practice across many other types of shell tools and utilities). The user then still has the capability to use other named accounts and regions by passing along additional parameters in the stack invocation.

[hoegertn]

@hornetmadness I think you are not transferring the env value to the super constructor call in your VPCImport2 class. This needs to be sent upstream to the stack. super().__init__(scope, id, env=env, **kwargs)

@webdog I advise against using the CDK_DEFAULT_XXX as a fallback when no env is set. When doing lookups I think we want to make sure that users are specifying the target environment exactly and not using whatever credentials they have active at the moment of synth.

[rix0rrr]

@webdog your main issue seems to be with the word "inherit", is that right?

It's unlikely we'll change the behavior to do case analysis. The current behaviors are:

• Nothing specified -> environment agnostic stack
• Env specified in code -> stack targeting that env
• Env specified using environment variables -> stack targeting whatever env is the "current" env

Muddying the waters by having "nothing specified" mean EITHER "environment agnostic" or "use current env" depending on the phase of the moon (not really, but the decision would be based on state that is not super obvious for users to observe, so that's a potential footgun right there -- the behavior will be hard to explain/predict) does not seem appealing.

Two things we could address:

• Is the current API clear enough to distinguish the 3 cases? Arguably it could be more explicit what the defaults are.
• We actually want people to make a choice in this regard, but our cdk init templates generate stacks that default to environment agnostic, and work fine until they don't (so users may not be aware they were supposed to have made a choice).

As for the first, would:

new DemoStack(app, 'Stack', {
  env: StackEnvironment.agnostic_environment(),
});

new DemoStack(app, 'Stack', {
  env: StackEnvironment.current_awscli_environment(),
});

Help?

As for the second, I guess we can make the error message EVEN LONGER to point them to the stack invocations, or have the default generated Stack throw an error until you pass an env in app.py/app.ts or something... is that still useful?

We could potentially generate commented-out code that shows the 3 alternatives side-by-side and encourages users to uncomment one of them.

[rix0rrr]

Adding @jerry-aws to see if he has some ideas on how to phrase this even more clearly.

[webdog]

@webdog your main issue seems to be with the word "inherit", is that right?

👋 More or less, yeah (Can't speak for the original author of this issue). I started diving into the codebase last week, but I couldn't find a definitive spot where we could inject logic to except another error message when a lookup is present, so I empathize with the challenge, especially Point 1 in your comment about API clarity.

As for the first, would:
...
Help?

It would, but I don't know if there's a need to be that explicit, and IIRC, the current stack.environment property is read-only, yeah? So I would still need to load my environment somewhere else. (Though, selfishly, making an environment property accessible like that would be neat, but I'd like to be respectful of y'alls time)

As for the second, I guess we can make the error message EVEN LONGER to point them to the stack invocations, or have the default generated Stack throw an error until you pass an env in app.py/app.ts or something... is that still useful?

👎 On longer error messages, it might muddy the clarity as you've said. If I'm in the situation where I'm using a lookup, just let me know that when I'm using a lookup, I have to explicitly define the account and region, even when I've set defaults, or support the defaults in a non-production setup (Maybe this is something as simple as cdk [synth|deploy] --production as an identifier)

[ghost]

We can always improve documentation and error messages, but that might just be a bandage on the problem. Taking a step back, maybe there's some way to make this choice clearer to customers.

To me, a significant difference in behavior like we have in the Stack class suggests that we ought to be using different types, rather than keying on a property value. We might instead provide EnvironmentAgnosticStack and ConcreteEnvironmentStack classes, the latter requiring the env prop,. Then the intended behavior is obvious and explicitly chosen by the customer. Customers might need to look up what "agnostic" and "concrete" mean, but they don't need to remember that the env prop is magic.

You can then deprecate Stack, or maybe just make it an alias for EnvironmentAgnosticStack (you would need a feature flag for that, I guess, until v2). You lose the ability to make a given stack behave either way based on its parameters. Is that important?

[rix0rrr]

You lose the ability to make a given stack behave either way based on its parameters. Is that important?

It is actually; it's not atypical to deploy the same stack to a fixed prod account, as well as a "floating" dev account.

Which is why inheritance is often not the answer, because it limits choices.

[ghost]

I see. I still think overloading env to change this behavior isn't obvious enough. It's something we end up having to explain a fair bit. Possibly it's an inevitable part of the learning curve. But I'll see what I can come up with for a better error message.

[github-actions]

⚠️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.