fix(core): write Metadata resource in core framework #10306

rix0rrr · 2020-09-11T07:56:10Z

The Metadata resource used to be added by the CLI, which led to a bug. The better, less error-prone way to do it is to have the framework add the metadata resource to the stack template upon synthesis.

The resources need to be added just-in-time (before synthesis), because if we do it in the constructor node.setContext() will stop working (for the Stack already having children).

We only add the Metadata resource if we're running via the CLI. If we did not do this, all unit tests everywhere that use toMatchTemplate()/toExactlyMatchTemplate()/toMatch() will break. There are hundreds alone in our codebase, nevermind however many other ones are out there. The consequences of this are that we [still] will not record users who are doing in-memory synthesis.

The CLI only does the work when the runtimeInfo field of the assembly is filled, which we just never do anymore. However, the code cannot be removed from the CLI because old versions of the framework might still set that field and expect the resource to be added to the template.

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.

Better for new style synthesis

…ws-cdk into huijbers/metadata-in-framework

…n-framework2

njlynch

Should we update this comment to point to this new code?

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

Lines 94 to 99 in 2b3e14c

    
           // @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.

packages/@aws-cdk/core/lib/private/metadata-resource.ts

packages/@aws-cdk/core/test/test.app.ts

packages/@aws-cdk/core/test/util.ts

packages/@aws-cdk/core/lib/private/metadata-resource.ts

eladb · 2020-09-11T11:47:33Z

packages/@aws-cdk/core/lib/private/synthesis.ts

+    if (!Stack.isStack(construct)
+      || construct.parentStack
+      // Unless disabled
+      || construct.node.tryGetContext(cxapi.DISABLE_VERSION_REPORTING)


Should this be a property of Stack such as stack.versionReportingEnabled which will take into account context and the nested stack posture instead of leaking the fact that this can he set through context?

I was also thinking that this should be an opt-in switch ("enable") and not opt-out ("disable"). This way, we (and our users) won't need to fix all their unit tests because the metadata resource will only be attached if the CLI sets that context flag.

We could do that, but since we already have an existing opt-out flag: DISABLE_VERSION_REPORTING (which we need to support for backwards compatibility anyway) I didn't really want to add another ENABLE_VERSION_REPORTING flag as well.

The Stack property probably makes sense although it would need to be exposed as a public property or a (ew!) public @internal one so that this function external to the class can use it.

It fights with the App's { runtimeInfo?: @default true } property as well...

DISABLE_VERSION_REPORTING is not really public API, so I would recommend removing it in favor of ENABLE_VERSION_REPORTING and deprecate runtimeInfo in favor of versionReporting (in props and at the Stack level as public API.

eladb · 2020-09-11T11:48:28Z

packages/@aws-cdk/core/lib/private/synthesis.ts

+      // Unless disabled
+      || construct.node.tryGetContext(cxapi.DISABLE_VERSION_REPORTING)
+      // While running via CLI
+      || !process.env[cxapi.CLI_VERSION_ENV]) { return; }


I believe I described that twice.

Won't it make sense to just not include the CLI version in case this environment variable in not defined?

It's not actually about the CLI_VERSION_ENV itself. I'm using CLI_VERSION_ENV as a trigger to determine whether we're running under the CLI (and hence emitting the metadata)

eladb · 2020-09-11T11:49:21Z

packages/@aws-cdk/core/lib/private/synthesis.ts

+    // Because of https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/assert/lib/synth-utils.ts#L74
+    // synthesize() may be called more than once on a stack in unit tests, and the below would break
+    // if we execute it a second time. Guard against the constructs already existing.
+    if (construct.node.tryFindChild('CDKMetadata')) { return; }


Make 'CDKMetadata' a const

eladb · 2020-09-11T11:52:43Z

packages/@aws-cdk/cx-api/lib/cloud-assembly.ts

@@ -369,6 +369,8 @@ export interface AssemblyBuildOptions {
  /**
   * Include the specified runtime information (module versions) in manifest.
   * @default - if this option is not specified, runtime info will not be included
+   * @deprecated All template modifications that should result from this should
+   * have already been inserted into the template.
   */
  readonly runtimeInfo?: RuntimeInfo;


Do we really want to continue to emit this info into the assembly? I think this should now be always undefined

Yes. But I don't think we can remove it from this API due to backwards compatibility guarantees.

eladb

Let's try to clean up all the flags and switches and stuff. I don't think that's going to be a major breaking change

rix0rrr · 2020-09-22T08:08:46Z

Let's try to clean up all the flags and switches and stuff. I don't think that's going to be a major breaking change

Nevertheless, we're going to have to break the cx-api protocol version for this, because we're flipping a default.

If we don't break the protocol version, people will be free to use an old CLI with the new framework. The old CLI won't emit DISABLE_VERSION_REPORTING (because the default is true), and the framework won't see ENABLE_VERSION_REPORTING (because the default is false) and not do anything.

njlynch

Looks good to me, minus some docstring changes.

packages/@aws-cdk/core/lib/app.ts

packages/@aws-cdk/core/lib/stack.ts

packages/@aws-cdk/cx-api/lib/app.ts

Co-authored-by: Nick Lynch <nlynch@amazon.com>

rix0rrr · 2020-09-22T10:59:34Z

Thanks @njlynch. Waiting for @eladb to weigh in on the protocol change as well

eladb

Looks good. See a few questions/clarifications

eladb · 2020-09-28T11:47:04Z

packages/@aws-cdk/core/lib/app.ts

+   *
+   * @default Value of 'aws:cdk:version-reporting' context key
+   */
+  readonly versionReporting?: boolean;


I am wondering if the correct terminology should be analytics: boolean--broaden the scope a little bit.

eladb · 2020-09-28T11:50:37Z

packages/@aws-cdk/core/lib/private/synthesis.ts

+    const CDKMetadata = 'CDKMetadata';
+    if (construct.node.tryFindChild(CDKMetadata)) { return; }
+
+    new MetadataResource(construct, CDKMetadata);


Remind me what's the rationale for not just adding this in the Stack's constructor based on the prop?

Can't set context anymore

And why do we need to set context? My view is that the context is just used to determine the default for analyticsReporting when new stacks are created. From that point forward we shouldn't care about the context key, no?

eladb · 2020-09-28T11:52:28Z

packages/aws-cdk/lib/api/cxapp/exec.ts

-  }
+  if (versionReporting) { context[cxapi.VERSION_REPORTING_ENABLED_CONTEXT] = true; }
+  // We need to keep on doing this for framework version from before this flag was deprecated.
+  if (!versionReporting) { context[cxapi.DISABLE_VERSION_REPORTING] = true; }


I wonder if we can remove this constant from cxapi and just plug the explicit value here for backwards compat

Oh please yes

eladb · 2020-09-28T11:53:03Z

Added do not merge

aws-cdk-automation · 2020-09-28T16:33:24Z

AWS CodeBuild CI Report

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

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

mergify · 2020-09-28T16:33:49Z

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).

rix0rrr added 8 commits September 4, 2020 15:50

Lint

250fecb

Remove console statement

e8d1673

fix(core): generate metadata resource in framework

2986948

Better for new style synthesis

fix(core): generate metadata resource in framework

d34489e

Better for new style synthesis

Merge branch 'huijbers/metadata-in-framework' of github.com:awslabs/a…

8bc1979

…ws-cdk into huijbers/metadata-in-framework

WIP

c62f0bc

Fix tests

80773c8

rix0rrr requested review from njlynch, nija-at and a team September 11, 2020 07:56

rix0rrr self-assigned this Sep 11, 2020

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

rix0rrr added 2 commits September 11, 2020 09:57

Merge remote-tracking branch 'origin/master' into huijbers/metadata-i…

906b927

…n-framework2

Humans can't mere

30ae434

njlynch reviewed Sep 11, 2020

View reviewed changes

eladb suggested changes Sep 11, 2020

View reviewed changes

eladb reviewed Sep 14, 2020

View reviewed changes

rix0rrr mentioned this pull request Sep 21, 2020

[core] Slow execution at SynthUtils.toCloudFormation on v1.57.0 #10213

Closed

Reverse default value of flags

b815752

rix0rrr requested review from eladb, njlynch and a team September 22, 2020 09:14

Tiny changes

96c1879

njlynch approved these changes Sep 22, 2020

View reviewed changes

packages/@aws-cdk/core/lib/app.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/core/lib/stack.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/cx-api/lib/app.ts Outdated Show resolved Hide resolved

rix0rrr and others added 3 commits September 22, 2020 12:58

Update packages/@aws-cdk/core/lib/app.ts

1eadb6d

Co-authored-by: Nick Lynch <nlynch@amazon.com>

Update packages/@aws-cdk/core/lib/stack.ts

6c0119c

Co-authored-by: Nick Lynch <nlynch@amazon.com>

Update packages/@aws-cdk/cx-api/lib/app.ts

fc7229d

Co-authored-by: Nick Lynch <nlynch@amazon.com>

eladb approved these changes Sep 28, 2020

View reviewed changes

eladb added the pr/do-not-merge This PR should not be merged at this time. label Sep 28, 2020

rix0rrr added 3 commits September 28, 2020 15:37

Rename "version reporting" -> "analytics reporting"

183026f

Carry rename through in more places

71b8536

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

a3fc956

rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Sep 28, 2020

rix0rrr changed the title ~~fix(core): add Metadata resource in core framework~~ fix(core): write Metadata resource in core framework Sep 28, 2020

mergify bot merged commit fb39803 into master Sep 28, 2020

mergify bot deleted the huijbers/metadata-in-framework2 branch September 28, 2020 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): write Metadata resource in core framework #10306

fix(core): write Metadata resource in core framework #10306

rix0rrr commented Sep 11, 2020

njlynch left a comment

eladb Sep 11, 2020

eladb Sep 11, 2020

rix0rrr Sep 11, 2020

eladb Sep 14, 2020

eladb Sep 11, 2020

rix0rrr Sep 11, 2020

eladb Sep 14, 2020

rix0rrr Sep 22, 2020

eladb Sep 11, 2020

eladb Sep 11, 2020

rix0rrr Sep 11, 2020

eladb left a comment

rix0rrr commented Sep 22, 2020

njlynch left a comment

rix0rrr commented Sep 22, 2020

eladb left a comment

eladb Sep 28, 2020

eladb Sep 28, 2020

rix0rrr Sep 28, 2020

eladb Sep 29, 2020

eladb Sep 28, 2020

rix0rrr Sep 28, 2020

eladb commented Sep 28, 2020

aws-cdk-automation commented Sep 28, 2020

mergify bot commented Sep 28, 2020

	// @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.

fix(core): write Metadata resource in core framework #10306

fix(core): write Metadata resource in core framework #10306

Conversation

rix0rrr commented Sep 11, 2020

njlynch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

rix0rrr commented Sep 22, 2020

njlynch left a comment

Choose a reason for hiding this comment

rix0rrr commented Sep 22, 2020

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb commented Sep 28, 2020

aws-cdk-automation commented Sep 28, 2020

AWS CodeBuild CI Report

mergify bot commented Sep 28, 2020