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

(cli): synthesis fails when new context values are parsed #23521

Open
IndikaUdagedara opened this issue Jan 1, 2023 · 2 comments
Open

(cli): synthesis fails when new context values are parsed #23521

IndikaUdagedara opened this issue Jan 1, 2023 · 2 comments
Labels
effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 package/tools Related to AWS CDK Tools or CLI

Comments

@IndikaUdagedara
Copy link

IndikaUdagedara commented Jan 1, 2023

Describe the bug

My cdk app loads values from a SSM parameter using StringParameter.valueFromLookup() and uses them as follows. The SSM parameter is created and populated by another team.

export interface NetworkingFacts {
  vpcId: string;
  privateSubnetIds: string[];
}

export class LambdaStack extends Stack {
  constructor(scope: Construct, id: string, props: ApiStackProps){
    super(scope, id, props);

      const networkingFactsRaw = StringParameter.valueFromLookup(this, '/networking/facts/');
      console.log(networkingFactsRaw)

      // networkingFactsRaw contains something like
      // {"vpcId":"vpc-aaa","privateSubnetIds":["subnet-xxx","subnet-yyy","subnet-zzz"]}
      // the idea is to Lookup resources based on these Ids and use them as below

      const networkingFacts: NetworkingFacts = JSON.parse(networkingFactsRaw);
      const vpc = Vpc.fromLookup(this, "Vpc", { vpcId: networkingFacts.vpcId, });

      this.function = new Function(this, "Lambda", {
      ....
        vpc: props.vpc,
      .... 
      });

When the app is synthesized for the first time, it fails with the error

dummy-value-for-/networking/facts
...
    const networkingFacts: NetworkingFacts = JSON.parse(networkingFactsRaw);
                                          ^
SyntaxError: Unexpected token d in JSON at position 0

However, this works if I add a return statement as below, which allows cdk to populate cdk.context.json from SSM

    const networkingFactsRaw = StringParameter.valueFromLookup(this, '/networking/facts/');
    console.log(networkingFactsRaw)
    return

Once cdk.context.json is populated, I can remove the return statement and the app works as expected (e.g. it looks up the correct Vpc based on the vpcId parsed from the SSM parameter value)

Expected Behavior

I would expect cdk to load the context prior to synthesis so that at synthesis time context is available and can be used for subsequent steps (e.g. like looking up another resource from the context value as in my example).

If this is not possible, valueFromLookup() can take an optional parameter so that users can pass an arbitrary value to be returned instead of hard-coded dummy-value-...

e.g. if the SSM value contains JSON users would use it as StringParameter.valueFromLookup(this, /path/to/key, '{}')
or if it's an ARN like in this issue it could be StringParameter.valueFromLookup(this, /path/to/key, 'arn:aws:iam::111111111:user/xxxx')

Current Behavior

Please see above

Reproduction Steps

Copy the code above and and run cdk synth

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.5.0 (build 0951122)

Framework Version

No response

Node.js Version

v18.7.0

OS

Mac

Language

Typescript

Language Version

No response

Other information

No response

@IndikaUdagedara IndikaUdagedara added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 1, 2023
@github-actions github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Jan 1, 2023
@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 3, 2023

For now, I would recommend you avoid parsing the value if it is equal to dummy-value.

In the future, the dummy value will probably turn into a Token, which you can check for by calling Token.isUnresolved() (but remember, that is not today).

@rix0rrr rix0rrr added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 3, 2023
@rix0rrr rix0rrr removed their assignment Jan 3, 2023
@IndikaUdagedara
Copy link
Author

IndikaUdagedara commented Jan 4, 2023

So code would look like this?

      const networkingFactsRaw = StringParameter.valueFromLookup(this, '/networking/facts/');
      if (networksFactsRaw.startsWith('dummy-value') return;
      const networkingFacts: NetworkingFacts = JSON.parse(networkingFactsRaw);
      const vpc = Vpc.fromLookup(this, "Vpc", { vpcId: networkingFacts.vpcId, });

I think you'd agree that the code can get a bit messy with such conditional return statements... any suggestions for a more idiomatic approach?

Also, I'm curious why it is handled in multiple passes


    // We may need to run the cloud executable multiple times in order to satisfy all missing context
    // (When the executable runs, it will tell us about context it wants to use
    // but it missing. We'll then look up the context and run the executable again, and
    // again, until it doesn't complain anymore or we've stopped making progress).
    let previouslyMissingKeys: Set<string> | undefined;
    while (true) {
      const assembly = await this.props.synthesizer(this.props.sdkProvider, this.props.configuration);
      if (assembly.manifest.missing && assembly.manifest.missing.length > 0) {
          const missingKeys = missingContextKeys(assembly.manifest.missing);

When the context provider plugin runs, what about fetching the parameter from SSM if it's missing rather than setting a dummy-value? Then there won't be any missing keys and therefore the synthesis can be completed in 1 pass? (Probably I'm talking from my ignorance as I'm not familiar with CDK internals)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 package/tools Related to AWS CDK Tools or CLI
Projects
None yet
Development

No branches or pull requests

2 participants