From c3841577cf24ea27d21ad6ae788fa07a475f3d83 Mon Sep 17 00:00:00 2001 From: Elad Ben-Israel Date: Mon, 24 Feb 2020 16:41:47 +0200 Subject: [PATCH 1/2] fix(core): top-level resources cannot use long logical ids When allocating a logical ID for a CloudFormation resource we normally render the id by taking in the full construct path, trim it down to 240 characters and append a hash to it to make sure its unique. This technically allows construct IDs to be of any length. However, when a resource is defined as a top-level resource (i.e. the child of a stack), it won't get this treatment and we will just use the construct id as it's logical id. In this case, synthesis will fail with an error indicating the logical ID is invalid (too long). This might work ok if the logical ID is human provided, but in some cases (like #6190), the id is actually created automatically and in some cases can be longer than 255. This fix will only apply the special treatment to top-level resources if their construct ID is shorter than the maximum allowed length (255). Otherwise, it will use the same mechanism to trim them and append the hash. Fixes #6190. --- packages/@aws-cdk/core/lib/private/uniqueid.ts | 3 ++- packages/@aws-cdk/core/test/test.logical-id.ts | 12 ++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/core/lib/private/uniqueid.ts b/packages/@aws-cdk/core/lib/private/uniqueid.ts index ef0798d0e2da7..e3ad6a2d02fa4 100644 --- a/packages/@aws-cdk/core/lib/private/uniqueid.ts +++ b/packages/@aws-cdk/core/lib/private/uniqueid.ts @@ -19,6 +19,7 @@ const PATH_SEP = '/'; const HASH_LEN = 8; const MAX_HUMAN_LEN = 240; // max ID len is 255 +const MAX_ID_LEN = 255; /** * Calculates a unique ID for a set of textual components. @@ -45,7 +46,7 @@ export function makeUniqueId(components: string[]) { // top-level resources will simply use the `name` as-is in order to support // transparent migration of cloudformation templates to the CDK without the // need to rename all resources. - if (components.length === 1) { + if (components.length === 1 && components[0].length < MAX_ID_LEN) { return removeNonAlphanumeric(components[0]); } diff --git a/packages/@aws-cdk/core/test/test.logical-id.ts b/packages/@aws-cdk/core/test/test.logical-id.ts index 25a6219ee86a8..d0ef8735c54f9 100644 --- a/packages/@aws-cdk/core/test/test.logical-id.ts +++ b/packages/@aws-cdk/core/test/test.logical-id.ts @@ -35,6 +35,18 @@ export = { test.done(); }, + 'if resource is top-level and logical id is longer than allowed, it is trimmed with a hash'(test: Test) { + // GIVEN + const stack = new Stack(undefined, 'TestStack'); + + // WHEN + const r = new CfnResource(stack, 'x'.repeat(256), { type: 'Resource' }); + + // THEN + test.equals(stack.resolve(r.logicalId), 'x'.repeat(240) + `C7A139A2`); + test.done(); + }, + 'Logical IDs can be renamed at the stack level'(test: Test) { // GIVEN const stack = new Stack(); From d18f105bfaa7f63d1cd54d5a23bc107a6b18cd65 Mon Sep 17 00:00:00 2001 From: Elad Ben-Israel Date: Mon, 24 Feb 2020 17:18:54 +0200 Subject: [PATCH 2/2] check length after trimming non-alpha --- packages/@aws-cdk/core/lib/private/uniqueid.ts | 15 +++++++++++++-- packages/@aws-cdk/core/test/test.logical-id.ts | 4 ++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/core/lib/private/uniqueid.ts b/packages/@aws-cdk/core/lib/private/uniqueid.ts index e3ad6a2d02fa4..be04dcf7cf0ed 100644 --- a/packages/@aws-cdk/core/lib/private/uniqueid.ts +++ b/packages/@aws-cdk/core/lib/private/uniqueid.ts @@ -46,8 +46,19 @@ export function makeUniqueId(components: string[]) { // top-level resources will simply use the `name` as-is in order to support // transparent migration of cloudformation templates to the CDK without the // need to rename all resources. - if (components.length === 1 && components[0].length < MAX_ID_LEN) { - return removeNonAlphanumeric(components[0]); + if (components.length === 1) { + // we filter out non-alpha characters but that is actually a bad idea + // because it could create conflicts ("A-B" and "AB" will render the same + // logical ID). sadly, changing it in the 1.x version line is impossible + // because it will be a breaking change. we should consider for v2.0. + // https://github.com/aws/aws-cdk/issues/6421 + const candidate = removeNonAlphanumeric(components[0]); + + // if our candidate is short enough, use it as is. otherwise, fall back to + // the normal mode. + if (candidate.length <= MAX_ID_LEN) { + return candidate; + } } const hash = pathHash(components); diff --git a/packages/@aws-cdk/core/test/test.logical-id.ts b/packages/@aws-cdk/core/test/test.logical-id.ts index d0ef8735c54f9..b2f9a096f347a 100644 --- a/packages/@aws-cdk/core/test/test.logical-id.ts +++ b/packages/@aws-cdk/core/test/test.logical-id.ts @@ -28,9 +28,13 @@ export = { // WHEN const r = new CfnResource(stack, 'MyAwesomeness', { type: 'Resource' }); + const r2 = new CfnResource(stack, 'x'.repeat(255), { type: 'Resource' }); // max length + const r3 = new CfnResource(stack, '*y-'.repeat(255), { type: 'Resource' }); // non-alpha are filtered out (yes, I know it might conflict) // THEN test.equal(stack.resolve(r.logicalId), 'MyAwesomeness'); + test.equal(stack.resolve(r2.logicalId), 'x'.repeat(255)); + test.equal(stack.resolve(r3.logicalId), 'y'.repeat(255)); test.done(); },