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

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

Merged
merged 5 commits into from
Sep 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,19 @@ export class CloudFormationStackArtifact extends CloudArtifact {
this.originalName = this.stackName;
}

/**
* Full path to the template file
*/
public get templateFullPath() {
return path.join(this.assembly.directory, this.templateFile);
}

/**
* The CloudFormation template for this stack.
*/
public get template(): any {
if (this._template === undefined) {
this._template = JSON.parse(fs.readFileSync(path.join(this.assembly.directory, this.templateFile), 'utf-8'));
this._template = JSON.parse(fs.readFileSync(this.templateFullPath, 'utf-8'));
}
return this._template;
}
Expand Down
21 changes: 15 additions & 6 deletions packages/@aws-cdk/cx-api/lib/cloud-assembly.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,13 +172,22 @@ export class CloudAssembly {
* @returns all the CloudFormation stack artifacts that are included in this assembly.
*/
public get stacks(): CloudFormationStackArtifact[] {
const result = new Array<CloudFormationStackArtifact>();
for (const a of this.artifacts) {
if (a instanceof CloudFormationStackArtifact) {
result.push(a);
}
return this.artifacts.filter(isCloudFormationStackArtifact);

function isCloudFormationStackArtifact(x: any): x is CloudFormationStackArtifact {
return x instanceof CloudFormationStackArtifact;
}
}

/**
* The nested assembly artifacts in this assembly
*/
public get nestedAssemblies(): NestedCloudAssemblyArtifact[] {
return this.artifacts.filter(isNestedCloudAssemblyArtifact);

function isNestedCloudAssemblyArtifact(x: any): x is NestedCloudAssemblyArtifact {
return x instanceof NestedCloudAssemblyArtifact;
}
return result;
}

private validateDeps() {
Expand Down
119 changes: 78 additions & 41 deletions packages/aws-cdk/lib/api/cxapp/cloud-executable.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { promises as fs } from 'fs';
import * as cxapi from '@aws-cdk/cx-api';
import { RegionInfo } from '@aws-cdk/region-info';
import * as contextproviders from '../../context-providers';
Expand Down Expand Up @@ -89,54 +90,76 @@ export class CloudExecutable {
}
}

if (trackVersions && assembly.runtime) {
const modules = formatModules(assembly.runtime);
for (const stack of assembly.stacks) {
if (!stack.template.Resources) {
stack.template.Resources = {};
}
const resourcePresent = stack.environment.region === cxapi.UNKNOWN_REGION
|| RegionInfo.get(stack.environment.region).cdkMetadataResourceAvailable;
if (resourcePresent) {
if (!stack.template.Resources.CDKMetadata) {
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`);
}
}
} else {
warning(`The stack ${stack.id} already includes a CDKMetadata resource`);
}
}
}
if (trackVersions) {
// @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.
Comment on lines +94 to +99
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

await this.addMetadataResource(assembly);
}

return new CloudAssembly(assembly);
}
}

/**
* Modify the templates in the assembly in-place to add metadata resource declarations
*/
private async addMetadataResource(rootAssembly: cxapi.CloudAssembly) {
if (!rootAssembly.runtime) { return; }

const modules = formatModules(rootAssembly.runtime);
await processAssembly(rootAssembly);

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);
}
}

function formatModules(runtime: cxapi.RuntimeInfo): string {
const modules = new Array<string>();
async function processStack(stack: cxapi.CloudFormationStackArtifact) {
const resourcePresent = stack.environment.region === cxapi.UNKNOWN_REGION
|| RegionInfo.get(stack.environment.region).cdkMetadataResourceAvailable;
if (!resourcePresent) { return; }

// 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}`);
if (!stack.template.Resources) {
stack.template.Resources = {};
}
if (stack.template.Resources.CDKMetadata) {
warning(`The stack ${stack.id} already includes a CDKMetadata resource`);
return;
}

for (const key of Object.keys(runtime.libraries).sort()) {
modules.push(`${key}=${runtime.libraries[key]}`);
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(',');
}

// 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' });
Comment on lines +156 to +162
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
}
}
Expand Down Expand Up @@ -185,4 +208,18 @@ function _inGroupsOf<T>(array: T[], maxGroup: number): T[][] {
result.push(array.slice(i, i + maxGroup));
}
return result;
}
}

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

// 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}`);

for (const key of Object.keys(runtime.libraries).sort()) {
modules.push(`${key}=${runtime.libraries[key]}`);
}
return modules.join(',');
}
10 changes: 10 additions & 0 deletions packages/aws-cdk/test/integ/cli/app/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,14 @@ class ConditionalResourceStack extends cdk.Stack {
}
}

class SomeStage extends cdk.Stage {
constructor(parent, id, props) {
super(parent, id, props);

new YourStack(this, 'StackInStage');
}
}

const app = new cdk.App();

const defaultEnv = {
Expand Down Expand Up @@ -286,4 +294,6 @@ new YourStack(app, `${stackPrefix}-termination-protection`, {
terminationProtection: process.env.TERMINATION_PROTECTION !== 'FALSE' ? true : false,
});

new SomeStage(app, `${stackPrefix}-stage`);

app.synth();
19 changes: 19 additions & 0 deletions packages/aws-cdk/test/integ/cli/cli.integtest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,25 @@ integTest('generating and loading assembly', async () => {
await cdkDeploy('docker-with-custom-file', { options: ['-a', '.'], cwd: asmOutputDir });
});

integTest('templates on disk contain metadata resource, also in nested assemblies', async () => {
// Synth first, and switch on version reporting because cdk.json is disabling it
await cdk(['synth', '--version-reporting=true']);

// Load template from disk from root assembly
const templateContents = await shell(['cat', 'cdk.out/*-lambda.template.json'], {
cwd: INTEG_TEST_DIR,
});

expect(JSON.parse(templateContents).Resources.CDKMetadata).toBeTruthy();

// Load template from nested assembly
const nestedTemplateContents = await shell(['cat', 'cdk.out/assembly-*-stage/*-stage-StackInStage.template.json'], {
cwd: INTEG_TEST_DIR,
});

expect(JSON.parse(nestedTemplateContents).Resources.CDKMetadata).toBeTruthy();
});

async function listChildren(parent: string, pred: (x: string) => Promise<boolean>) {
const ret = new Array<string>();
for (const child of await fs.readdir(parent, { encoding: 'utf-8' })) {
Expand Down