fix(cli): metadata not recorded for templates >50k #10184

rix0rrr · 2020-09-04T14:00:26Z

The CDK Metadata Resource is added by the CLI, and used to be only added
to the in-memory representation of the template.

This in-memory representation is only used in the CreateChangeSet
call if the full template body is <=50kB.

If the template is larger than this size, the template file that's on
disk will be uploaded to the bootstrap S3 bucket and referenced.
However, that template will not have had the metadata resource
injected and so the stack will not be registered.

This broken behavior was introduced in v1.29.0, released in March 2020,
when we switched to uploading the on-disk file to S3 instead of a re-serialization
of the [modified] in-memory representation. See:

a75f711

This PR:

Provisionally fixes this issue by flushing the in-memory modifications
back out to disk.
Also adds the metadata resources to stacks found in nested Cloud
Assemblies, such as the stacks deployed via CDK Pipelines.

There is some jank around new-style synthesis stacks: the protocol
specifies that those stacks have synthesized their templates as assets,
and those will not (necessarily) be processed by the CLI, although
it might coincidentally work if the files are the same.

The proper fix to this whole mess is to have the framework inject the
metadata resource at synthesis time. That fix is forthcoming, but this
current one is a good stopgap until that time.

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

The CDK Metadata Resource is added by the CLI, and only added to the in-memory representation of the template. This in-memory representation is only used in the `CreateChangeSet` call if the full template body is <=50kB. If the template is larger than this size, the template file that's on disk will be uploaded to the bootstrap S3 bucket and referenced. However, that template will not have had the metadata resource injected and so the stack will not be registered. This PR: - Provisionally fixes this issue by flushing the in-memory modifications back out to disk. - Also adds the metadata resources to stacks found in nested Cloud Assemblies, such as the stacks deployed via CDK Pipelines. There is some jank around new-style synthesis stacks: the protocol specifies that those stacks have synthesized their templates as assets, and those will not (necessarily) be processed by the CLI, although it might coincidentally work if the files are the same. The proper fix to this whole mess is to have the framework inject the metadata resource at synthesis time. That fix is forthcoming, but this current one is a good stopgap until that time.

nija-at · 2020-09-04T15:34:29Z

packages/aws-cdk/lib/api/cxapp/cloud-executable.ts

+        // @deprecated(v2): this should honestly not be done here. The framework
+        // should (and will, shortly) synthesize this information directly into
+        // the template. However, in order to support old framework versions
+        // that don't synthesize this info yet, we can only remove this code
+        // once we break backwards compatibility.
+        await this.addMetadataResource(assembly);


Can we be super clear about what should happen as part of v2?

Should this if statement be dropped entirely? I'd like the engineer who is doing the v2 work not have to load a ton of context.

should (and will, shortly) synthesize this information directly into the template

I assume this has not bee done yet. Let's wait until so before we can add the @deprecated code.
We should be using the @deprecated annotation only when it's ready to be dropped.

Sorry to be a pita.

nija-at

requesting changes for the comment around @deprecated tag.

nija-at · 2020-09-04T16:12:08Z

packages/aws-cdk/lib/api/cxapp/cloud-executable.ts

+  private async addMetadataResource(rootAssembly: cxapi.CloudAssembly) {
+    if (!rootAssembly.runtime) { return; }
+
+    const modules = formatModules(rootAssembly.runtime);
+    await processAssembly(rootAssembly);

-      function formatModules(runtime: cxapi.RuntimeInfo): string {
-        const modules = new Array<string>();
+    async function processAssembly(assembly: cxapi.CloudAssembly) {
+      for (const stack of assembly.stacks) {
+        await processStack(stack);
+      }
+      for (const nested of assembly.nestedAssemblies) {
+        await processAssembly(nested.nestedAssembly);
+      }
+    }

-        // inject toolkit version to list of modules
-        // eslint-disable-next-line @typescript-eslint/no-require-imports
-        const toolkitVersion = require('../../../package.json').version;
-        modules.push(`aws-cdk=${toolkitVersion}`);
+    async function processStack(stack: cxapi.CloudFormationStackArtifact) {
+      const resourcePresent = stack.environment.region === cxapi.UNKNOWN_REGION
+        || RegionInfo.get(stack.environment.region).cdkMetadataResourceAvailable;
+      if (!resourcePresent) { return; }

-        for (const key of Object.keys(runtime.libraries).sort()) {
-          modules.push(`${key}=${runtime.libraries[key]}`);
+      if (!stack.template.Resources) {
+        stack.template.Resources = {};
+      }
+      if (stack.template.Resources.CDKMetadata) {
+        warning(`The stack ${stack.id} already includes a CDKMetadata resource`);
+        return;
+      }
+
+      stack.template.Resources.CDKMetadata = {
+        Type: 'AWS::CDK::Metadata',
+        Properties: {
+          Modules: modules,
+        },
+      };
+
+      if (stack.environment.region === cxapi.UNKNOWN_REGION) {
+        stack.template.Conditions = stack.template.Conditions || {};
+        const condName = 'CDKMetadataAvailable';
+        if (!stack.template.Conditions[condName]) {
+          stack.template.Conditions[condName] = _makeCdkMetadataAvailableCondition();
+          stack.template.Resources.CDKMetadata.Condition = condName;
+        } else {
+          warning(`The stack ${stack.id} already includes a ${condName} condition`);
        }
-        return modules.join(',');
      }


Has any logic changed here at all or is this all refactor? I hate non-trivial refactors in bug fix PRs.
I can't say if I should or not should not scrutinize this code.

I know, sorry. I was cleaning this up and then detected the bug. It shouldn't be fairly easy to parse once you know how to chunk it.

The part that injects the resource is the same (processStack is an extract method refactor), the path leading up to it (processAssembly) is different: we now recurse through assemblies.

nija-at · 2020-09-04T16:16:43Z

packages/aws-cdk/lib/api/cxapp/cloud-executable.ts

+      // The template has changed in-memory, but the file on disk remains unchanged so far.
+      // The CLI *might* later on deploy the in-memory version (if it's <50kB) or use the
+      // on-disk version (if it's >50kB).
+      //
+      // Be sure to flush the changes we just made back to disk. The on-disk format is always
+      // JSON.
+      await fs.writeFile(stack.templateFullPath, JSON.stringify(stack.template, undefined, 2), { encoding: 'utf-8' });


Mutate the synthesized template. Ewww...

I suppose the root problem is that the CLI shouldn't be injecting the metadata. Is it safe to assume this will not happen when the correct fix in put in place?

eladb

Wouldn't be much easier to simply move metadata collection to the framework now instead of more tweaks in the CLI?

rix0rrr · 2020-09-07T07:12:32Z

Clarified that the if block should be removed in case of v2.

Wouldn't be much easier to simply move metadata collection to the framework now instead of more tweaks in the CLI?

Yes, but not good enough. Doesn't help for old framework version -- new versions of the CLI should basically still support v1.0.0 framework and so must always keep the ability to inject the resource into a cloud assembly that doesn't currently have one.

I'll also direct your attention to the closing paragraph of the PR:

The proper fix to this whole mess is to have the framework inject the
metadata resource at synthesis time. That fix is forthcoming, but this
current one is a good stopgap until that time.

Addressed

mergify · 2020-09-07T12:42:22Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-09-07T13:09:35Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 2df648e
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-09-07T13:09:59Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

nija-at · 2020-09-07T13:10:19Z

packages/aws-cdk/lib/api/cxapp/cloud-executable.ts

+        // @deprecated(v2): remove this 'if' block and all code referenced by it.
+        // This should honestly not be done here. The framework
+        // should (and will, shortly) synthesize this information directly into
+        // the template. However, in order to support old framework versions
+        // that don't synthesize this info yet, we can only remove this code
+        // once we break backwards compatibility.


I still think we shouldn't add this notice until the change is done to the framework

polothy · 2020-09-14T21:33:01Z

Hello @rix0rrr !

When I run cdk deploy -a cdk.out STACK_NAME or cdk diff -a cdk.out STACK_NAME, I'm getting this warning about every stack in my project (which has many many stacks):

aws-cdk/packages/aws-cdk/lib/api/cxapp/cloud-executable.ts

Line 134 in a2174a4

warning(`The stack ${stack.id} already includes a CDKMetadata resource`);

I'm using the the "@aws-cdk/core:newStyleStackSynthesis": true in my CDK context.

Is there something I can do to remove the warning? Sorry if this isn't a good place to ask this question.

rix0rrr requested a review from a team September 4, 2020 14:00

rix0rrr self-assigned this Sep 4, 2020

rix0rrr changed the title ~~fix(cli): metadata not recorded in templates >50k~~ fix(cli): metadata not recorded for templates >50k Sep 4, 2020

mergify bot added the contribution/core This is a PR that came from AWS. label Sep 4, 2020

rix0rrr added 2 commits September 4, 2020 16:15

Lint

250fecb

Remove console statement

e8d1673

nija-at reviewed Sep 4, 2020

View reviewed changes

nija-at previously requested changes Sep 4, 2020

View reviewed changes

eladb reviewed Sep 6, 2020

View reviewed changes

Update cloud-executable.ts

d2e03e6

rix0rrr requested review from nija-at, eladb and a team September 7, 2020 07:12

iliapolo approved these changes Sep 7, 2020

View reviewed changes

Merge branch 'master' into huijbers/metadata-in-template

2df648e

mergify bot merged commit dfd2baf into master Sep 7, 2020

mergify bot deleted the huijbers/metadata-in-template branch September 7, 2020 13:10

nija-at reviewed Sep 7, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): metadata not recorded for templates >50k #10184

fix(cli): metadata not recorded for templates >50k #10184

rix0rrr commented Sep 4, 2020 •

edited

Loading

nija-at Sep 4, 2020 •

edited

Loading

nija-at Sep 4, 2020 •

edited

Loading

nija-at left a comment

nija-at Sep 4, 2020

rix0rrr Sep 7, 2020 •

edited

Loading

nija-at Sep 4, 2020

eladb left a comment

rix0rrr commented Sep 7, 2020

mergify bot commented Sep 7, 2020

aws-cdk-automation commented Sep 7, 2020

mergify bot commented Sep 7, 2020

nija-at Sep 7, 2020

polothy commented Sep 14, 2020

fix(cli): metadata not recorded for templates >50k #10184

fix(cli): metadata not recorded for templates >50k #10184

Conversation

rix0rrr commented Sep 4, 2020 • edited Loading

nija-at Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

nija-at Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

nija-at left a comment

Choose a reason for hiding this comment

nija-at Sep 4, 2020

Choose a reason for hiding this comment

rix0rrr Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

nija-at Sep 4, 2020

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

rix0rrr commented Sep 7, 2020

mergify bot commented Sep 7, 2020

aws-cdk-automation commented Sep 7, 2020

AWS CodeBuild CI Report

mergify bot commented Sep 7, 2020

nija-at Sep 7, 2020

Choose a reason for hiding this comment

polothy commented Sep 14, 2020

rix0rrr commented Sep 4, 2020 •

edited

Loading

nija-at Sep 4, 2020 •

edited

Loading

nija-at Sep 4, 2020 •

edited

Loading

rix0rrr Sep 7, 2020 •

edited

Loading