EC2: SecurityGroup.fromLookupByName fails even without tokens

[mmieluch]

Describe the bug

This piece of code will fail:

    const sg = SecurityGroup.fromLookupByName(
      this,
      "MySecurityGroup",
      "foo-security-group",
      props.vpc,
    );

with error stating that

All arguments to look up a security group must be concrete (no Tokens)

The group name in the call is not a token, however, but a hard-coded string.

Regression Issue

• Select this option if this issue appears to be a regression.

Last Known Working CDK Version

No response

Expected Behavior

The snippet from description should correctly resolve the security group if it exists.

Current Behavior

cdk deploy fails with error:
All arguments to look up a security group must be concrete (no Tokens)

Reproduction Steps

interface FooStackProps {
  env: Environment;
  vpc: IVpc;
}

class FooStack extends Stack {
  constructor(scope: Construct, id: string, props: FooStackProps) {
    super(scope, id, props);

    const sg1 = new SecurityGroup(this, "SG1", {
      vpc: network.mainVpc,
      securityGroupName: "sg-1",
    });

    const sg2 = SecurityGroup.fromLookupByName(
      this,
      "TestSecurityGroup",
      "sg-1",
      network.mainVpc,
    );
  }
}
new FooStack(app, "FooStack", {
  env: params.env,
  vpc: network.mainVpc,
});

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.159.1

Framework Version

No response

Node.js Version

v20.17.0

OS

Linux arch 6.6.52-1-lts x86_64

Language

TypeScript

Language Version

5.4.5

Other information

No response

[khushail]

Hi @mmieluch , thanks for reaching out.

This error is reproducible -

However after looking at the code, here is my analysis -

1. The error is generated after checking Token -

https://github.com/aws/aws-cdk/blob/182804a3b3116924e2f7a8e50a22e2e7d99c71ae/packages/aws-cdk-lib/aws-ec2/lib/security-group.ts#L436C1-L439C6

2. Tokens represent values that can only be resolved at a later time in the app lifecycle. As seen in the above code block, the token value is evaluated to null (bacause VPC id is not available till deployment) which is causing the error.

3. In the Readme, its mentioned that .fromSecurityGroupByName(), result will be written to cdk.context.json and read from there.-

So in that scenario, I think one workaround would be to store this value in SSMParameter store , import in in other stack and then pass it on to fromSecurityGroupByName() to get the desired group. You could refer to this article for more detailed example.

Let me know if this helps!

[mmieluch]

OK, thank you for the explanation! In that case, would it be possible to improve the error message to indicate which argument the method has a problem with? I spent quite some time trying to debug, file an issue, etc., and I'm sure I'm not the only one that attempted to tackle this. The error message makes sense in hindsight, but even now I think it's a bit cryptic. It wouldn't be a bugfix, but a quality of life improvement.

Thank you for taking your time to investigate this ticket.

[pahud]

fromLookupByName() actually calls fromLookupByAttributes()

aws-cdk/packages/aws-cdk-lib/aws-ec2/lib/security-group.ts

Lines 436 to 439 in 3c92012

	private static fromLookupAttributes(scope: Construct, id: string, options: SecurityGroupLookupOptions) {
	if (Token.isUnresolved(options.securityGroupId) || Token.isUnresolved(options.securityGroupName) || Token.isUnresolved(options.vpc?.vpcId)) {
	throw new Error('All arguments to look up a security group must be concrete (no Tokens)');
	}

I believe it's actually your VPC is an unresolved token

Token.isUnresolved(options.vpc?.vpcId)

In your original provided code:

   const sg2 = SecurityGroup.fromLookupByName(
      this,
      "TestSecurityGroup",
      "sg-1",
      network.mainVpc,
    );

It's not clear to me where is network.mainVpc defined so I guess it's network.mainVpc that is unresolvable.

Why this has to be resolvable? It's because when you just provide the name of the SG, CDK would not be able to lookup the SG ID only with that name. It requires a concrete VPC ID as well. If you scroll down a little bit

aws-cdk/packages/aws-cdk-lib/aws-ec2/lib/security-group.ts

Lines 442 to 447 in 3c92012

	provider: cxschema.ContextProvider.SECURITY_GROUP_PROVIDER,
	props: {
	securityGroupId: options.securityGroupId,
	securityGroupName: options.securityGroupName,
	vpcId: options.vpc?.vpcId,
	},

CDK would need that VPC ID for the context provider to lookup the real SG ID. What's happening here is that a real SDK call would be invoked. If this VPC ID can't be resolved then the lookup won't succeed with the context provider.

In many cases it would be unresolvable for example:

1. You import that using Fn.Import() from another stack.
2. You reference it from a template parameter.
3. Any other cases when it would only be resolved when CloudFormation deploy it, which happens after cdk synth.

Hope it clarifies.

[mmieluch]

Hi @pahud, massive thanks for the explanation! Like I said earlier, it absolutely makes sense in hindsight. I'm only proposing maybe improving upon the error message given in that situation, because it was not clear from the error I posted that it was in fact the VPC ID (which existed prior to me trying to reference the SG in another stack) that was causing the issue. Not sure if that's even possible or timeworthy. Maybe I was just too hung up on the security group name part of the equasion.

Whatever you decide to do with this ticket and my plea, you have my huge thanks for responding and explaining in such great detail!

[pahud]

@mmieluch No, you are absolutely right and I agree with you. CDK is an open source project and if you found anything that deserve the improvement, you don't need to ask for approval. Just submit a PR for that and it could be reviewed just in 1-2 weeks and hopefully get merged. You would make CDK the way you love.

I guess we could increase the verbosity here

aws-cdk/packages/aws-cdk-lib/aws-ec2/lib/security-group.ts

Lines 437 to 439 in 3c92012

	if (Token.isUnresolved(options.securityGroupId) || Token.isUnresolved(options.securityGroupName) || Token.isUnresolved(options.vpc?.vpcId)) {
	throw new Error('All arguments to look up a security group must be concrete (no Tokens)');
	}

If you haven't contributed to any CDK pull requests yet, we encourage you to check out this blog post and give it a try.

[mmieluch]

I'll absolutely give it a shot! Thank you for your guidance.