[core] Make programmatic App.synth()'s behavior more similar to CLI app synth's

liamfd commented 3 years ago

I have been attempting to use app.synth(), figuring it would be more or less a drop-in replacement for calling app synth on the CLI. My code looks something like:

const app = new cdk.App({
    outdir: 'cdk.out',
});
app.synth();

However, I've noticed that they produce pretty different output. In part because app.synth() doesn't appear to read from cdk.json out of the box the way the CLI does. My cdk.json looks something like:

{
  "app": "yarn babel-node node/cdk/bin/foo.ts",
  "context": {
    "@aws-cdk/core:enableStackNameDuplicates": "true",
    "aws-cdk:enableDiffNoFail": "true",
    "@aws-cdk/core:stackRelativeExports": "true",
    "@aws-cdk/core:newStyleStackSynthesis": "true"
  }
}

So, for example, core:newStyleStackSynthesis is not getting applied with the programmatic synth.

Additionally, the flags like --version-reporting --path-metadata and --asset-metadata, which default to true on the CLI, don't get set with a raw app.synth(). It seems we can get around that by setting the aws:cdk:version-reporting, aws:cdk:enable-path-metadata and aws:cdk:enable-asset-metadata context vars in cdk.json. We can also seem to set version-reporting by passing analyticsReporting: true to the App constructor, though I didn't see an equivalent for the other two flags.

Note that, even with all of those set, I still see slightly different output in the CDK::Metadata resource. The CLI generates:

"Properties": {
        "Modules": "aws-cdk=1.73.0,@aws-cdk/assets=1.73.0,@aws-cdk/aws-apigateway=1.73.0,@aws-cdk/aws-applicationautoscaling=1.73.0,@aws-cdk/aws-autoscaling=1.73.0,@aws-cdk/aws-autoscaling-common=1.73.0,@aws-cdk/aws-autoscaling-hooktargets=1.73.0,@aws-cdk/aws-cloudformation=1.73.0,@aws-cdk/aws-cloudwatch=1.73.0,@aws-cdk/aws-codebuild=1.73.0,@aws-cdk/aws-codeguruprofiler=1.73.0,@aws-cdk/aws-codepipeline=1.73.0,@aws-cdk/aws-codepipeline-actions=1.73.0,@aws-cdk/aws-ec2=1.73.0,@aws-cdk/aws-ecr=1.73.0,@aws-cdk/aws-ecr-assets=1.73.0,@aws-cdk/aws-ecs=1.73.0,@aws-cdk/aws-elasticloadbalancingv2=1.73.0,@aws-cdk/aws-events=1.73.0,@aws-cdk/aws-events-targets=1.73.0,@aws-cdk/aws-iam=1.73.0,@aws-cdk/aws-kms=1.73.0,@aws-cdk/aws-lambda=1.73.0,@aws-cdk/aws-logs=1.73.0,@aws-cdk/aws-s3=1.73.0,@aws-cdk/aws-s3-assets=1.73.0,@aws-cdk/aws-servicediscovery=1.73.0,@aws-cdk/aws-sns=1.73.0,@aws-cdk/aws-sns-subscriptions=1.73.0,@aws-cdk/aws-sqs=1.73.0,@aws-cdk/aws-ssm=1.73.0,@aws-cdk/cfnspec=1.73.0,@aws-cdk/cloud-assembly-schema=1.73.0,@aws-cdk/cloudformation-diff=1.73.0,@aws-cdk/cloudformation-include=1.73.0,@aws-cdk/core=1.73.0,@aws-cdk/custom-resources=1.73.0,@aws-cdk/cx-api=1.73.0,@aws-cdk/region-info=1.73.0,@aws-cdk/yaml-cfn=1.73.0,jsii-runtime=node.js/v10.18.0"
      },

But a programmatic invocation generates:

"Properties": {
        "Modules": "@aws-cdk/assets=1.73.0,@aws-cdk/aws-apigateway=1.73.0,@aws-cdk/aws-applicationautoscaling=1.73.0,@aws-cdk/aws-autoscaling-common=1.73.0,@aws-cdk/aws-cloudwatch=1.73.0,@aws-cdk/aws-codeguruprofiler=1.73.0,@aws-cdk/aws-ec2=1.73.0,@aws-cdk/aws-events=1.73.0,@aws-cdk/aws-iam=1.73.0,@aws-cdk/aws-kms=1.73.0,@aws-cdk/aws-lambda=1.73.0,@aws-cdk/aws-logs=1.73.0,@aws-cdk/aws-s3=1.73.0,@aws-cdk/aws-s3-assets=1.73.0,@aws-cdk/aws-sqs=1.73.0,@aws-cdk/aws-ssm=1.73.0,@aws-cdk/cfnspec=1.73.0,@aws-cdk/cloud-assembly-schema=1.73.0,@aws-cdk/cloudformation-diff=1.73.0,@aws-cdk/cloudformation-include=1.73.0,@aws-cdk/core=1.73.0,@aws-cdk/cx-api=1.73.0,@aws-cdk/region-info=1.73.0,@aws-cdk/yaml-cfn=1.73.0,jsii-runtime=node.js/v10.18.0"
      },

Additionally, the CLI doesn't seem to respect at least the aws:cdk:enable-asset-metadata and aws:cdk:enable-path-metadata flags. And setting any of those three flags in the cdk.json causes the cdk to print warnings like:

A reserved context key ('context.aws:') key was found in /path/to/cdk.json, it might cause surprising behavior and should be removed.

I think the intention is that you set those on the top level object in the cdk.json, but I can't see how to pass them to the App's synth process.

Use Case

Judging by https://github.com/aws/aws-cdk/issues/601, app deploy has no publicly supported programmatic interface, but the docs seem to suggest that App.synth() is fine to use. So I'm performing a programmatic App.synth and then later a CLI cdk deploy.

This was exhibiting some surprising behavior because of some deprecated code in the deploy command, which was causing it to inject the Metadata resource that the app.synth() was not injecting (because, unlike the CLI synth, versionReporting is not defaulted to true). That behavior goes away if I use one of the version-reporting workarounds mentioned above, but it was a little surprising.

Proposed Solution

It's entirely possible that I've either missed something obvious, or am misunderstanding how this is supposed to work. Assuming that's not the case, I think there are a few potential improvements worth considering:

Read from the cdk.json by default

I was somewhat surprised this wasn't happening.

I did find a partial workaround for this:

import { Configuration } from 'aws-cdk/lib/settings';

// ...

  const configuration = new Configuration();
  await configuration.load();

  const app = new cdk.App({
    context: configuration.context.all,
    outdir: 'cdk.out',
  });
  app.synth();

However, this only passes the context, other flags that seem to translate to settings (e.g. versionReporting) aren't carried through like this.

Allow the "App" constructor to take equivalent params to the CLI

I don't have a ton of familiarity with the design intentions here, but I think allowing the App constructor to take a Settings or Configuration prop would be the best way to accomplish this, given that right now you can only specify parts of those objects through context and the analyticsReporting props. Right now the Settings generated by a cli synth look like:

{
      "versionReporting": true,
      "pathMetadata": true,
      "output": "cdk.out",
      "app": "yarn babel-node node/cdk/bin/foo.ts",
      "context": {
        "@aws-cdk/core:enableStackNameDuplicates": "true",
        "aws-cdk:enableDiffNoFail": "true",
        "@aws-cdk/core:stackRelativeExports": "true",
        "@aws-cdk/core:newStyleStackSynthesis": "true"
      },
      "debug": false,
      "assetMetadata": true,
      "toolkitBucket": {},
      "staging": true,
      "bundlingStacks": [
        "*"
      ]
}

Perhaps not all of those are applicable, but as it stands its a little hard to see how to replicate cli synth behavior programmatically. As far as I can tell it seems like you need to use the context flags I mentioned above, but as I said, those print warnings. I think the intention is that you use settings in the cdk.json instead of those context values, but as far as I know there's still no way to pass those settings to the App constructor.

If this isn't viable, it seems like making App's prop signature symmetrical to the CLI's flags would be a win, as right now we've got analyticsReporting which defaults to a false-y value in one but true in the other, path-metadata and asset-metadata which can't be directly set in one and are true in the other, and probably some other flags I'm not considering.

Apply the same defaults as the CLI, or at least make them accessible

Additionally, I think it would be less surprising if programmatic and cli synths either defaulted to the same behavior, or if there was a simple way to opt in to the defaults in the programmatic command. A high level option for this (perhaps with a Settings / Configuration builder whose result we could pass to the App constructor) would help handle unknown unknowns to those new to the CDK, and hopefully be a bit more future proof as new defaults get added down the road.

Document Differences

If those options aren't viable (or maybe while they're in development), I think it would be good to add some warnings to the docs about the asymmetry between programmatic and CLI synth calls, because as it stands the programmatic call seems much lower level and less feature rich.

Other

Workarounds

[x] :wave: I may be able to implement this feature request
- I could take a crack at writing the documentation on the current state of things, and might be able to contribute to some of the changes described, it depends what they are.
[ ] :warning: This feature might incur a breaking change

This is a :rocket: Feature Request

Thanks for reading, and thanks to the maintainers for their work on this project!

rix0rrr commented 3 years ago

The separation between CLI and framework are on purpose.

Taking this as a duplicate of https://github.com/aws/aws-cdk/issues/601

liamfd commented 3 years ago

@rix0rrr thanks for taking a look.

Just to clarify, I understand that it would be beneficial to keep the CLI and framework logic separate, but do you mean that the behavior / set of options is intentionally different as well?

In either case, if it seems unlikely that any changes would be prioritized in the near term, do you think it would make sense to add a line to the App#synth docs like:

Note that this method only offers a subset of the options that cdk app synth command does, and has different default behavior. For most applications, we recommend that you use the CLI instead of invoking this method directly.

I'm happy to take a shot at a PR for the docs, I just want to make sure I understand the situation.

normtown commented 2 years ago

@rix0rrr said:

The separation between CLI and framework are on purpose.

I'm sure there are good reasons for separation, but I don't think this addresses the original observation. Nor does it give the customer a model for what to expect to be in the framework vs the CLI.

What are the criteria that drive decisions for that separation? i.e. Given a choice between logic X going into CLI code vs framework code, what determines where X belongs?

For example (and going along with the theme of inconsistency between cdk synth and app.synth()), why does the logic that provides AZs for a region/account during synthesis of a VPC belong in a plugin that only gets loaded by the CLI? Why isn't that part of calling app.synth() in a CDK app? If one were to call app.synth() and inspect the stack template, they'd see dummy AZs ('dummy1a', 'dummy1b', and 'dummy1c').

github-actions[bot] commented 1 year ago

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.

aws / aws-cdk