Skip to content

Commit

Permalink
fix(core): write Metadata resource in core framework (#10306)
Browse files Browse the repository at this point in the history
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*
  • Loading branch information
rix0rrr committed Sep 28, 2020
1 parent d3df6c7 commit fb39803
Show file tree
Hide file tree
Showing 19 changed files with 309 additions and 79 deletions.
@@ -1 +1 @@
{"version":"6.0.0"}
{"version":"6.0.0"}
19 changes: 15 additions & 4 deletions packages/@aws-cdk/core/lib/app.ts
Expand Up @@ -35,11 +35,20 @@ export interface AppProps {
readonly stackTraces?: boolean;

/**
* Include runtime versioning information in cloud assembly manifest
* @default true runtime info is included unless `aws:cdk:disable-runtime-info` is set in the context.
* Include runtime versioning information in the Stacks of this app
*
* @deprecated use `versionReporting` instead
* @default Value of 'aws:cdk:version-reporting' context key
*/
readonly runtimeInfo?: boolean;

/**
* Include runtime versioning information in the Stacks of this app
*
* @default Value of 'aws:cdk:version-reporting' context key
*/
readonly analyticsReporting?: boolean;

/**
* Additional context values for the application.
*
Expand Down Expand Up @@ -101,8 +110,10 @@ export class App extends Stage {
this.node.setContext(cxapi.DISABLE_METADATA_STACK_TRACE, true);
}

if (props.runtimeInfo === false) {
this.node.setContext(cxapi.DISABLE_VERSION_REPORTING, true);
const analyticsReporting = props.analyticsReporting ?? props.runtimeInfo;

if (analyticsReporting !== undefined) {
this.node.setContext(cxapi.ANALYTICS_REPORTING_ENABLED_CONTEXT, analyticsReporting);
}

const autoSynth = props.autoSynth !== undefined ? props.autoSynth : cxapi.OUTDIR_ENV in process.env;
Expand Down
28 changes: 24 additions & 4 deletions packages/@aws-cdk/core/lib/cfn-fn.ts
Expand Up @@ -196,12 +196,18 @@ export class Fn {
* Returns true if all the specified conditions evaluate to true, or returns
* false if any one of the conditions evaluates to false. ``Fn::And`` acts as
* an AND operator. The minimum number of conditions that you can include is
* 2, and the maximum is 10.
* 1.
* @param conditions conditions to AND
* @returns an FnCondition token
*/
public static conditionAnd(...conditions: ICfnConditionExpression[]): ICfnConditionExpression {
return new FnAnd(...conditions);
if (conditions.length === 0) {
throw new Error('Fn.conditionAnd() needs at least one argument');
}
if (conditions.length === 1) {
return conditions[0];
}
return Fn.conditionAnd(..._inGroupsOf(conditions, 10).map(group => new FnAnd(...group)));
}

/**
Expand Down Expand Up @@ -249,12 +255,18 @@ export class Fn {
* Returns true if any one of the specified conditions evaluate to true, or
* returns false if all of the conditions evaluates to false. ``Fn::Or`` acts
* as an OR operator. The minimum number of conditions that you can include is
* 2, and the maximum is 10.
* 1.
* @param conditions conditions that evaluates to true or false.
* @returns an FnCondition token
*/
public static conditionOr(...conditions: ICfnConditionExpression[]): ICfnConditionExpression {
return new FnOr(...conditions);
if (conditions.length === 0) {
throw new Error('Fn.conditionOr() needs at least one argument');
}
if (conditions.length === 1) {
return conditions[0];
}
return Fn.conditionOr(..._inGroupsOf(conditions, 10).map(group => new FnOr(...group)));
}

/**
Expand Down Expand Up @@ -748,3 +760,11 @@ class FnJoin implements IResolvable {
return minimalCloudFormationJoin(this.delimiter, resolvedValues);
}
}

function _inGroupsOf<T>(array: T[], maxGroup: number): T[][] {
const result = new Array<T[]>();
for (let i = 0; i < array.length; i += maxGroup) {
result.push(array.slice(i, i + maxGroup));
}
return result;
}
92 changes: 92 additions & 0 deletions packages/@aws-cdk/core/lib/private/metadata-resource.ts
@@ -0,0 +1,92 @@
import * as cxapi from '@aws-cdk/cx-api';
import { RegionInfo } from '@aws-cdk/region-info';
import { CfnCondition } from '../cfn-condition';
import { Fn } from '../cfn-fn';
import { Aws } from '../cfn-pseudo';
import { CfnResource } from '../cfn-resource';
import { Construct } from '../construct-compat';
import { Lazy } from '../lazy';
import { Stack } from '../stack';
import { Token } from '../token';
import { collectRuntimeInformation } from './runtime-info';

/**
* Construct that will render the metadata resource
*/
export class MetadataResource extends Construct {
/**
* Clear the modules cache
*
* The next time the MetadataResource is rendered, it will do a lookup of the
* modules from the NodeJS module cache again.
*
* Used only for unit tests.
*/
public static clearModulesCache() {
this._modulesPropertyCache = undefined;
}

/**
* Cached version of the _modulesProperty() accessor
*
* No point in calculating this fairly expensive list more than once.
*/
private static _modulesPropertyCache?: string;

/**
* Calculate the modules property
*/
private static modulesProperty(): string {
if (this._modulesPropertyCache === undefined) {
this._modulesPropertyCache = formatModules(collectRuntimeInformation());
}
return this._modulesPropertyCache;
}

constructor(scope: Stack, id: string) {
super(scope, id);

const metadataServiceExists = Token.isUnresolved(scope.region) || RegionInfo.get(scope.region).cdkMetadataResourceAvailable;
if (metadataServiceExists) {
const resource = new CfnResource(this, 'Default', {
type: 'AWS::CDK::Metadata',
properties: {
Modules: Lazy.stringValue({ produce: () => MetadataResource.modulesProperty() }),
},
});

// In case we don't actually know the region, add a condition to determine it at deploy time
if (Token.isUnresolved(scope.region)) {
const condition = new CfnCondition(this, 'Condition', {
expression: makeCdkMetadataAvailableCondition(),
});

// To not cause undue template changes
condition.overrideLogicalId('CDKMetadataAvailable');

resource.cfnOptions.condition = condition;
}
}
}
}

function makeCdkMetadataAvailableCondition() {
return Fn.conditionOr(...RegionInfo.regions
.filter(ri => ri.cdkMetadataResourceAvailable)
.map(ri => Fn.conditionEquals(Aws.REGION, ri.name)));
}

function formatModules(runtime: cxapi.RuntimeInfo): string {
const modules = new Array<string>();

// inject toolkit version to list of modules
const cliVersion = process.env[cxapi.CLI_VERSION_ENV];
if (cliVersion) {
modules.push(`aws-cdk=${cliVersion}`);
}

for (const key of Object.keys(runtime.libraries).sort()) {
modules.push(`${key}=${runtime.libraries[key]}`);
}
return modules.join(',');
}
37 changes: 34 additions & 3 deletions packages/@aws-cdk/core/lib/private/synthesis.ts
Expand Up @@ -5,6 +5,7 @@ import { Aspects, IAspect } from '../aspect';
import { Construct, IConstruct, SynthesisOptions, ValidationError } from '../construct-compat';
import { Stack } from '../stack';
import { Stage, StageSynthesisOptions } from '../stage';
import { MetadataResource } from './metadata-resource';
import { prepareApp } from './prepare-app';
import { TreeMetadata } from './tree-metadata';

Expand All @@ -14,6 +15,8 @@ export function synthesize(root: IConstruct, options: SynthesisOptions = { }): c

invokeAspects(root);

injectMetadataResources(root);

// This is mostly here for legacy purposes as the framework itself does not use prepare anymore.
prepareTree(root);

Expand All @@ -35,9 +38,7 @@ export function synthesize(root: IConstruct, options: SynthesisOptions = { }): c
// stacks to add themselves to the synthesized cloud assembly.
synthesizeTree(root, builder);

return builder.buildAssembly({
runtimeInfo: options.runtimeInfo,
});
return builder.buildAssembly();
}

/**
Expand Down Expand Up @@ -110,6 +111,36 @@ function prepareTree(root: IConstruct) {
visit(root, 'post', construct => construct.onPrepare());
}

/**
* Find all stacks and add Metadata Resources to all of them
*
* There is no good generic place to do this. Can't do it in the constructor
* (because adding a child construct makes it impossible to set context on the
* node), and the generic prepare phase is deprecated.
*
* Only do this on [parent] stacks (not nested stacks), don't do this when
* disabled by the user.
*
* Also, only when running via the CLI. If we do it unconditionally,
* all unit tests everywhere are going to break massively. I've spent a day
* fixing our own, but downstream users would be affected just as badly.
*
* Stop at Assembly boundaries.
*/
function injectMetadataResources(root: IConstruct) {
visit(root, 'post', construct => {
if (!Stack.isStack(construct) || !construct._versionReportingEnabled) { return; }

// 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.
const CDKMetadata = 'CDKMetadata';
if (construct.node.tryFindChild(CDKMetadata)) { return; }

new MetadataResource(construct, CDKMetadata);
});
}

/**
* Synthesize children in post-order into the given builder
*
Expand Down
26 changes: 24 additions & 2 deletions packages/@aws-cdk/core/lib/stack.ts
Expand Up @@ -127,6 +127,14 @@ export interface StackProps {
* @default false
*/
readonly terminationProtection?: boolean;

/**
* Include runtime versioning information in this Stack
*
* @default `analyticsReporting` setting of containing `App`, or value of
* 'aws:cdk:version-reporting' context key
*/
readonly analyticsReporting?: boolean;
}

/**
Expand Down Expand Up @@ -281,6 +289,15 @@ export class Stack extends Construct implements ITaggable {
*/
public readonly synthesizer: IStackSynthesizer;

/**
* Whether version reporting is enabled for this stack
*
* Controls whether the CDK Metadata resource is injected
*
* @internal
*/
public readonly _versionReportingEnabled: boolean;

/**
* Logical ID generation strategy
*/
Expand Down Expand Up @@ -370,6 +387,10 @@ export class Stack extends Construct implements ITaggable {

this.templateFile = `${this.artifactId}.template.json`;

// Not for nested stacks
this._versionReportingEnabled = (props.analyticsReporting ?? this.node.tryGetContext(cxapi.ANALYTICS_REPORTING_ENABLED_CONTEXT))
&& !this.nestedStackParent;

this.synthesizer = props.synthesizer ?? (newStyleSynthesisContext
? new DefaultStackSynthesizer()
: new LegacyStackSynthesizer());
Expand Down Expand Up @@ -722,10 +743,11 @@ export class Stack extends Construct implements ITaggable {
// this right now, so some parts still happen here.
const builder = session.assembly;

const template = this._toCloudFormation();

// write the CloudFormation template as a JSON file
const outPath = path.join(builder.outdir, this.templateFile);
const text = JSON.stringify(this._toCloudFormation(), undefined, 2);
fs.writeFileSync(outPath, text);
fs.writeFileSync(outPath, JSON.stringify(template, undefined, 2));

for (const ctx of this._missingContext) {
builder.addMissing(ctx);
Expand Down
3 changes: 0 additions & 3 deletions packages/@aws-cdk/core/lib/stage.ts
Expand Up @@ -2,7 +2,6 @@ import * as cxapi from '@aws-cdk/cx-api';
import { IConstruct, Node } from 'constructs';
import { Construct } from './construct-compat';
import { Environment } from './environment';
import { collectRuntimeInformation } from './private/runtime-info';
import { synthesize } from './private/synthesis';

/**
Expand Down Expand Up @@ -172,10 +171,8 @@ export class Stage extends Construct {
*/
public synth(options: StageSynthesisOptions = { }): cxapi.CloudAssembly {
if (!this.assembly || options.force) {
const runtimeInfo = this.node.tryGetContext(cxapi.DISABLE_VERSION_REPORTING) ? undefined : collectRuntimeInformation();
this.assembly = synthesize(this, {
skipValidation: options.skipValidation,
runtimeInfo,
});
}

Expand Down
4 changes: 3 additions & 1 deletion packages/@aws-cdk/core/package.json
Expand Up @@ -178,6 +178,7 @@
"dependencies": {
"@aws-cdk/cloud-assembly-schema": "0.0.0",
"@aws-cdk/cx-api": "0.0.0",
"@aws-cdk/region-info": "0.0.0",
"constructs": "^3.0.4",
"fs-extra": "^9.0.1",
"minimatch": "^3.0.4"
Expand All @@ -190,7 +191,8 @@
"peerDependencies": {
"@aws-cdk/cloud-assembly-schema": "0.0.0",
"@aws-cdk/cx-api": "0.0.0",
"constructs": "^3.0.4"
"constructs": "^3.0.4",
"@aws-cdk/region-info": "0.0.0"
},
"engines": {
"node": ">= 10.13.0 <13 || >=13.7.0"
Expand Down

0 comments on commit fb39803

Please sign in to comment.