From 5b757e1784a5719902a7cb8be9edbd1feaacbd1f Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 3 Aug 2018 10:30:24 +0200 Subject: [PATCH] Framework: introducing "Default" construct id The "Default" construct identifier will make sure the name component does not get included in the logical ID in any way (neither in path nor in hash). This is useful when, during refactoring, wrapping one construct in another, to make sure logical IDs of already deployed resources don't change. This fixes #482. --- .../cdk/lib/cloudformation/logical-id.ts | 26 +++++++++++-- .../test/cloudformation/test.logical-id.ts | 27 ++++++++++++++ packages/aws-cdk-docs/src/logical-ids.rst | 29 +++++++++------ tools/cdk-build-tools/bin/cdk-test.ts | 2 +- tools/cdk-build-tools/lib/package-info.ts | 37 +++++++++++++++---- 5 files changed, 97 insertions(+), 24 deletions(-) diff --git a/packages/@aws-cdk/cdk/lib/cloudformation/logical-id.ts b/packages/@aws-cdk/cdk/lib/cloudformation/logical-id.ts index 5bdbc409ee314..4bc66fd13b4f9 100644 --- a/packages/@aws-cdk/cdk/lib/cloudformation/logical-id.ts +++ b/packages/@aws-cdk/cdk/lib/cloudformation/logical-id.ts @@ -46,12 +46,17 @@ export interface IAddressingScheme { * (i.e. `L1/L2/Pipeline/Pipeline`), they will be de-duplicated to make the * resulting human portion of the ID more pleasing: `L1L2Pipeline` * instead of `L1L2PipelinePipeline` - * - If a component is named "Resource" it will be omitted from the path. This - * allows L2 construct to use this convention to "hide" the wrapped L1 from - * the logical ID. + * - If a component is named "Default" it will be omitted from the path. This + * allows refactoring higher level abstractions around constructs without affecting + * the IDs of already deployed resources. + * - If a component is named "Resource" it will be omitted from the user-visible + * path, but included in the hash. This reduces visual noise in the human readable + * part of the identifier. */ export class HashedAddressingScheme implements IAddressingScheme { public allocateAddress(addressComponents: string[]): string { + addressComponents = addressComponents.filter(x => x !== HIDDEN_ID); + if (addressComponents.length === 0) { throw new Error('Construct has empty Logical ID'); } @@ -65,7 +70,7 @@ export class HashedAddressingScheme implements IAddressingScheme { const hash = pathHash(addressComponents); const human = removeDupes(addressComponents) - .filter(x => x !== 'Resource') + .filter(x => x !== HIDDEN_FROM_HUMAN_ID) .join('') .slice(0, MAX_HUMAN_LEN); @@ -73,6 +78,19 @@ export class HashedAddressingScheme implements IAddressingScheme { } } +/** + * Resources with this ID are hidden from humans + * + * They do not appear in the human-readable part of the logical ID, + * but they are included in the hash calculation. + */ +const HIDDEN_FROM_HUMAN_ID = 'Resource'; + +/** + * Resources with this ID are complete hidden from the logical ID calculation. + */ +const HIDDEN_ID = 'Default'; + /** * Class that keeps track of the logical IDs that are assigned to resources * diff --git a/packages/@aws-cdk/cdk/test/cloudformation/test.logical-id.ts b/packages/@aws-cdk/cdk/test/cloudformation/test.logical-id.ts index ff8f91e5c5cee..7f8348b71db68 100644 --- a/packages/@aws-cdk/cdk/test/cloudformation/test.logical-id.ts +++ b/packages/@aws-cdk/cdk/test/cloudformation/test.logical-id.ts @@ -104,6 +104,33 @@ const uniqueTests = { } }); + test.done(); + }, + + 'can transparently wrap constructs using "Default" id'(test: Test) { + // GIVEN + const stack1 = new Stack(); + const parent1 = new Construct(stack1, 'Parent'); + new Resource(parent1, 'HeyThere', { type: 'AWS::TAAS::Thing' }); + const template1 = stack1.toCloudFormation(); + + // AND + const theId1 = Object.keys(template1.Resources)[0]; + test.equal('AWS::TAAS::Thing', template1.Resources[theId1].Type); + + // WHEN + const stack2 = new Stack(); + const parent2 = new Construct(stack2, 'Parent'); + const invisibleWrapper = new Construct(parent2, 'Default'); + new Resource(invisibleWrapper, 'HeyThere', { type: 'AWS::TAAS::Thing' }); + const template2 = stack1.toCloudFormation(); + + const theId2 = Object.keys(template2.Resources)[0]; + test.equal('AWS::TAAS::Thing', template2.Resources[theId2].Type); + + // THEN: same ID, same object + test.equal(theId1, theId2); + test.done(); } }; diff --git a/packages/aws-cdk-docs/src/logical-ids.rst b/packages/aws-cdk-docs/src/logical-ids.rst index 7155b4120b9e6..afea5091bc471 100644 --- a/packages/aws-cdk-docs/src/logical-ids.rst +++ b/packages/aws-cdk-docs/src/logical-ids.rst @@ -32,13 +32,13 @@ Each resource in the construct tree has a unique path that represents its location within the tree. Since logical IDs can only use alphanumeric characters and also restricted in length, the CDK is unable to simply use a delimited path as the logical ID. -Instead, logical IDs are allocated by concatenating a human-friendly rendition -from the path (concatenation, de-duplicate, trim) with an eight-character MD5 -hash of the delimited path. -This final component is necessary since |CFN| logical IDs cannot include -the delimiting slash character (/), so simply concatenating the component -values does not work. For example, concatenating the components of the -path */a/b/c* produces **abc**, which is the same as concatenating the components of +Instead, logical IDs are allocated by concatenating a human-friendly rendition +from the path (concatenation, de-duplicate, trim) with an eight-character MD5 +hash of the delimited path. +This final component is necessary since |CFN| logical IDs cannot include +the delimiting slash character (/), so simply concatenating the component +values does not work. For example, concatenating the components of the +path */a/b/c* produces **abc**, which is the same as concatenating the components of the path */ab/c*. .. code-block:: text @@ -46,7 +46,7 @@ the path */ab/c*. VPCPrivateSubnet2RouteTable0A19E10E <-----------human---------><-hash-> -Low-level CloudFormation resources (from `@aws-cdk/resources`) +Low-level CloudFormation resources (from `@aws-cdk/resources`) that are direct children of the |stack-class| class use their name as their logical ID without modification. This makes it easier to port existing templates into a CDK app. @@ -67,8 +67,13 @@ Logical IDs remain unchanged across updates The |cdk| applies some heuristics to improve the human-friendliness of the prefix: -- If a path component is **Resource**, it is omitted. - This postfix does not normally contribute any additional useful information to the ID. +- If a path component is **Default**, is is hidden completely from the logical ID + computation. You will generally want to use this if you create a new construct + that wraps an existing one. By naming the inner construct **Default**, you + ensure that the logical identifiers of resources in already-deployed copy of + that construct do not change. +- If a path component is **Resource**, it is omitted from the human readable portion. + of the logical ID. This postfix does not normally contribute any additional useful information to the ID. - If two subsequent names in the path are the same, only one is retained. - If the prefix exceeds 240 characters, it is trimmed to 240 characters. This ensures that the total length of the logical ID does not exceed the 255 character @@ -92,7 +97,7 @@ logical IDs to certain resources, given either their full path or // a good practice would be to always put these at the top of your stack initializer. this.renameLogical('MyTableCD117FA1', 'MyTable'); this.renameLogical('MyQueueAB4432A3', 'MyAwesomeQueue'); - + new Table(this, 'MyTable'); new Queue(this, 'MyQueue'); } @@ -128,5 +133,5 @@ stacks. `cdk diff` will tell you which resources are about to be destroyed: [-] ☢️ Destroying MyTable (type: AWS::DynamoDB::Table) [+] 🆕 Creating MyTableCD117FA1 (type: AWS::DynamoDB::Table) -Now, you can add a :py:meth:`aws-cdk.Stack.renameLogical` call before the +Now, you can add a :py:meth:`aws-cdk.Stack.renameLogical` call before the table is defined to rename **MyTableCD117FA1** to **MyTable**. diff --git a/tools/cdk-build-tools/bin/cdk-test.ts b/tools/cdk-build-tools/bin/cdk-test.ts index c304c8034c3d7..b15f0cb30d41d 100644 --- a/tools/cdk-build-tools/bin/cdk-test.ts +++ b/tools/cdk-build-tools/bin/cdk-test.ts @@ -33,7 +33,7 @@ async function main() { testCommand.push(...['nyc', '--clean']); } testCommand.push('nodeunit'); - testCommand.push(...testFiles); + testCommand.push(...testFiles.map(f => f.path)); await shell(testCommand, timers); } diff --git a/tools/cdk-build-tools/lib/package-info.ts b/tools/cdk-build-tools/lib/package-info.ts index 5efd60e9945bd..f95fee34d6320 100644 --- a/tools/cdk-build-tools/lib/package-info.ts +++ b/tools/cdk-build-tools/lib/package-info.ts @@ -2,6 +2,9 @@ import fs = require('fs'); import path = require('path'); import util = require('util'); +const readdir = util.promisify(fs.readdir); +const stat = util.promisify(fs.stat); + /** * Return the package JSON for the current package */ @@ -27,9 +30,29 @@ export function isJsii(): boolean { return currentPackageJson().jsii !== undefined; } -export async function listFiles(dirName: string, predicate: (x: string) => boolean): Promise { +export interface File { + filename: string; + path: string; +} + +export async function listFiles(dirName: string, predicate: (x: File) => boolean): Promise { try { - return (await util.promisify(fs.readdir)(dirName)).filter(predicate).map(f => path.join(dirName, f)); + const files = (await readdir(dirName)).map(filename => ({ filename, path: path.join(dirName, filename) })); + + const ret: File[] = []; + for (const file of files) { + const s = await stat(file.path); + if (s.isDirectory()) { + // Recurse + ret.push(...await listFiles(file.path, predicate)); + } else { + if (predicate(file)) { + ret.push(file); + } + } + } + + return ret; } catch (e) { if (e.code === 'ENOENT') { return []; } throw e; @@ -39,8 +62,8 @@ export async function listFiles(dirName: string, predicate: (x: string) => boole /** * Return the unit test files for this package */ -export async function unitTestFiles(): Promise { - return listFiles('test', f => f.startsWith('test.') && f.endsWith('.js')); +export async function unitTestFiles(): Promise { + return listFiles('test', f => f.filename.startsWith('test.') && f.filename.endsWith('.js')); } /** @@ -56,12 +79,12 @@ export async function hasOnlyAutogeneratedTests(): Promise { const packageName = path.basename(process.cwd()).replace(/^aws-/, ''); return (tests.length === 1 - && tests[0] === `test/test.${packageName}.js` - && fs.readFileSync(tests[0], { encoding: 'utf-8' }).indexOf(AUTOGENERATED_TEST_MARKER) !== -1); + && tests[0].path === `test/test.${packageName}.js` + && fs.readFileSync(tests[0].path, { encoding: 'utf-8' }).indexOf(AUTOGENERATED_TEST_MARKER) !== -1); } export async function hasIntegTests(): Promise { - const files = await listFiles('test', f => f.startsWith('integ.') && f.endsWith('.js')); + const files = await listFiles('test', f => f.filename.startsWith('integ.') && f.filename.endsWith('.js')); return files.length > 0; }