From 7cb0922c622794e3cd19d86e98fda644caf3ec96 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Wed, 3 Jul 2019 11:08:29 +0200 Subject: [PATCH 1/3] fix(ssm): correctly deduplicate parameter names Parameter names with '/' in them would not be correctly deduplicated, since the '/' is also used to delimit identifiers in construct paths. Add protection in core to not allow construct identifiers to be created with '/' in them, as tryFindChild() would not be able to find it anymore. Fixes #3076. --- .../@aws-cdk/aws-ecs/test/test.ecs-cluster.ts | 16 ++++++++++++++++ packages/@aws-cdk/aws-ssm/lib/parameter.ts | 5 +++-- packages/@aws-cdk/aws-ssm/test/test.parameter.ts | 12 +++++++++++- packages/@aws-cdk/core/lib/construct.ts | 12 ++++++++++++ packages/@aws-cdk/core/test/test.construct.ts | 6 ++++++ 5 files changed, 48 insertions(+), 3 deletions(-) diff --git a/packages/@aws-cdk/aws-ecs/test/test.ecs-cluster.ts b/packages/@aws-cdk/aws-ecs/test/test.ecs-cluster.ts index 9c683245b8bea..961d93c38c2d8 100644 --- a/packages/@aws-cdk/aws-ecs/test/test.ecs-cluster.ts +++ b/packages/@aws-cdk/aws-ecs/test/test.ecs-cluster.ts @@ -155,6 +155,22 @@ export = { test.done(); }, + "multiple clusters with default capacity"(test: Test) { + // GIVEN + const stack = new cdk.Stack(); + const vpc = new ec2.Vpc(stack, 'MyVpc', {}); + + // WHEN + for (let i = 0; i < 2; i++) { + const cluster = new ecs.Cluster(stack, `EcsCluster${i}`, { vpc, }); + cluster.addCapacity('MyCapacity', { + instanceType: new ec2.InstanceType('m3.medium'), + }); + } + + test.done(); + }, + 'lifecycle hook is automatically added'(test: Test) { // GIVEN const stack = new cdk.Stack(); diff --git a/packages/@aws-cdk/aws-ssm/lib/parameter.ts b/packages/@aws-cdk/aws-ssm/lib/parameter.ts index a5605c4af01b8..fcb05a68709d0 100644 --- a/packages/@aws-cdk/aws-ssm/lib/parameter.ts +++ b/packages/@aws-cdk/aws-ssm/lib/parameter.ts @@ -1,7 +1,7 @@ import iam = require('@aws-cdk/aws-iam'); import { CfnDynamicReference, CfnDynamicReferenceService, CfnParameter, - Construct, ContextProvider, Fn, IConstruct, IResource, Resource, Stack, Token + Construct, ContextProvider, Fn, IConstruct, IResource, Resource, Stack, Token, ConstructNode } from '@aws-cdk/core'; import cxapi = require('@aws-cdk/cx-api'); import ssm = require('./ssm.generated'); @@ -248,6 +248,7 @@ export class StringParameter extends ParameterBase implements IStringParameter { const stack = Stack.of(scope); const id = makeIdentityForImportedValue(parameterName); const exists = stack.node.tryFindChild(id) as IStringParameter; + if (exists) { return exists.stringValue; } return this.fromStringParameterAttributes(stack, id, { parameterName, version }).stringValue; @@ -371,7 +372,7 @@ function _assertValidValue(value: string, allowedPattern: string): void { } function makeIdentityForImportedValue(parameterName: string) { - return `SsmParameterValue:${parameterName}:C96584B6-F00A-464E-AD19-53AFF4B05118`; + return ConstructNode.sanitizeId(`SsmParameterValue:${parameterName}:C96584B6-F00A-464E-AD19-53AFF4B05118`); } function arnForParameterName(scope: IConstruct, parameterName: string): string { diff --git a/packages/@aws-cdk/aws-ssm/test/test.parameter.ts b/packages/@aws-cdk/aws-ssm/test/test.parameter.ts index 8d3416010b87c..f6b8f4a22a13e 100644 --- a/packages/@aws-cdk/aws-ssm/test/test.parameter.ts +++ b/packages/@aws-cdk/aws-ssm/test/test.parameter.ts @@ -281,7 +281,17 @@ export = { } }); test.done(); - } + }, + + 'can query actual SSM Parameter Names, multiple times'(test: Test) { + // GIVEN + const stack = new Stack(); + // WHEN + ssm.StringParameter.valueForStringParameter(stack, '/my/param/name'); + ssm.StringParameter.valueForStringParameter(stack, '/my/param/name'); + + test.done(); + }, } }; diff --git a/packages/@aws-cdk/core/lib/construct.ts b/packages/@aws-cdk/core/lib/construct.ts index ece3dd4eb4fb1..f3ad18ebed224 100644 --- a/packages/@aws-cdk/core/lib/construct.ts +++ b/packages/@aws-cdk/core/lib/construct.ts @@ -27,6 +27,14 @@ export class ConstructNode { */ public static readonly PATH_SEP = '/'; + /** + * Return a sanitized version of an arbitrary string, so it can be used as an ID + */ + public static sanitizeId(id: string) { + // Cannot have PATH_SEPs in the ID + return id.replace(/\//g, '-'); + } + /** * Synthesizes a CloudAssembly from a construct tree. * @param root The root of the construct tree. @@ -139,6 +147,10 @@ export class ConstructNode { throw new Error('Only root constructs may have an empty name'); } + if (ConstructNode.sanitizeId(id) !== id) { + throw new Error('Invalid characters in id, use ConstructNode.sanitizeId(): ' + id); + } + // Has side effect so must be very last thing in constructor scope.node.addChild(host, this.id); } else { diff --git a/packages/@aws-cdk/core/test/test.construct.ts b/packages/@aws-cdk/core/test/test.construct.ts index 6aa8cfd793209..6362c142e998d 100644 --- a/packages/@aws-cdk/core/test/test.construct.ts +++ b/packages/@aws-cdk/core/test/test.construct.ts @@ -20,6 +20,12 @@ export = { test.done(); }, + 'constructs names cannot have a slash in them'(test: Test) { + const root = new Root(); + test.throws(() => new Construct(root, 'hello/bye')); + test.done(); + }, + 'construct.name returns the name of the construct'(test: Test) { const t = createTree(); From e171763f587f1810e9cac32a97c14b62a4a66c16 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Wed, 3 Jul 2019 11:38:52 +0200 Subject: [PATCH 2/3] Back to implicit sanitizing --- packages/@aws-cdk/aws-ssm/lib/parameter.ts | 4 +- packages/@aws-cdk/core/lib/construct.ts | 55 +++++-------------- packages/@aws-cdk/core/test/test.construct.ts | 6 -- 3 files changed, 16 insertions(+), 49 deletions(-) diff --git a/packages/@aws-cdk/aws-ssm/lib/parameter.ts b/packages/@aws-cdk/aws-ssm/lib/parameter.ts index fcb05a68709d0..08edec6e1d95a 100644 --- a/packages/@aws-cdk/aws-ssm/lib/parameter.ts +++ b/packages/@aws-cdk/aws-ssm/lib/parameter.ts @@ -1,7 +1,7 @@ import iam = require('@aws-cdk/aws-iam'); import { CfnDynamicReference, CfnDynamicReferenceService, CfnParameter, - Construct, ContextProvider, Fn, IConstruct, IResource, Resource, Stack, Token, ConstructNode + Construct, ContextProvider, Fn, IConstruct, IResource, Resource, Stack, Token } from '@aws-cdk/core'; import cxapi = require('@aws-cdk/cx-api'); import ssm = require('./ssm.generated'); @@ -372,7 +372,7 @@ function _assertValidValue(value: string, allowedPattern: string): void { } function makeIdentityForImportedValue(parameterName: string) { - return ConstructNode.sanitizeId(`SsmParameterValue:${parameterName}:C96584B6-F00A-464E-AD19-53AFF4B05118`); + return `SsmParameterValue:${parameterName}:C96584B6-F00A-464E-AD19-53AFF4B05118`; } function arnForParameterName(scope: IConstruct, parameterName: string): string { diff --git a/packages/@aws-cdk/core/lib/construct.ts b/packages/@aws-cdk/core/lib/construct.ts index f3ad18ebed224..12ec724a8c5c5 100644 --- a/packages/@aws-cdk/core/lib/construct.ts +++ b/packages/@aws-cdk/core/lib/construct.ts @@ -27,14 +27,6 @@ export class ConstructNode { */ public static readonly PATH_SEP = '/'; - /** - * Return a sanitized version of an arbitrary string, so it can be used as an ID - */ - public static sanitizeId(id: string) { - // Cannot have PATH_SEPs in the ID - return id.replace(/\//g, '-'); - } - /** * Synthesizes a CloudAssembly from a construct tree. * @param root The root of the construct tree. @@ -137,7 +129,7 @@ export class ConstructNode { constructor(private readonly host: Construct, scope: IConstruct, id: string) { id = id || ''; // if undefined, convert to empty string - this.id = id; + this.id = sanitizeId(id); this.scope = scope; // We say that scope is required, but root scopes will bypass the type @@ -147,10 +139,6 @@ export class ConstructNode { throw new Error('Only root constructs may have an empty name'); } - if (ConstructNode.sanitizeId(id) !== id) { - throw new Error('Invalid characters in id, use ConstructNode.sanitizeId(): ' + id); - } - // Has side effect so must be very last thing in constructor scope.node.addChild(host, this.id); } else { @@ -158,9 +146,6 @@ export class ConstructNode { this.id = id; } - // escape any path separators so they don't wreck havoc - this.id = this._escapePathSeparator(this.id); - if (Token.isUnresolved(id)) { throw new Error(`Cannot use tokens in construct ID: ${id}`); } @@ -186,25 +171,13 @@ export class ConstructNode { } /** - * Return a descendant by path, or undefined - * - * Note that if the original ID of the construct you are looking for contained - * a '/', then it would have been replaced by '--'. + * Return a direct child by id, or undefined * - * @param path Relative path of a direct or indirect child - * @returns a child by path or undefined if not found. + * @param id Identifier of direct child + * @returns the child if found, or undefined */ - public tryFindChild(path: string): IConstruct | undefined { - if (path.startsWith(ConstructNode.PATH_SEP)) { - throw new Error('Path must be relative'); - } - const parts = path.split(ConstructNode.PATH_SEP); - - let curr: IConstruct | undefined = this.host; - while (curr != null && parts.length > 0) { - curr = curr.node._children[parts.shift()!]; - } - return curr; + public tryFindChild(id: string): IConstruct | undefined { + return this._children[sanitizeId(id)]; } /** @@ -539,14 +512,6 @@ export class ConstructNode { this.invokedAspects.push(aspect); } } - - /** - * If the construct ID contains a path separator, it is replaced by double dash (`--`). - */ - private _escapePathSeparator(id: string) { - if (!id) { return id; } - return id.split(ConstructNode.PATH_SEP).join('--'); - } } /** @@ -727,3 +692,11 @@ function ignore(_x: any) { // Import this _after_ everything else to help node work the classes out in the correct order... import { Reference } from './reference'; + +/** + * Return a sanitized version of an arbitrary string, so it can be used as an ID + */ +function sanitizeId(id: string) { + // Escape path seps as double dashes + return id.replace(/\//g, '--'); +} diff --git a/packages/@aws-cdk/core/test/test.construct.ts b/packages/@aws-cdk/core/test/test.construct.ts index 6362c142e998d..6aa8cfd793209 100644 --- a/packages/@aws-cdk/core/test/test.construct.ts +++ b/packages/@aws-cdk/core/test/test.construct.ts @@ -20,12 +20,6 @@ export = { test.done(); }, - 'constructs names cannot have a slash in them'(test: Test) { - const root = new Root(); - test.throws(() => new Construct(root, 'hello/bye')); - test.done(); - }, - 'construct.name returns the name of the construct'(test: Test) { const t = createTree(); From ff4c200fa8b8ed1d7c9a126c6071401037484762 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Wed, 3 Jul 2019 12:41:34 +0200 Subject: [PATCH 3/3] Use PATH_SEP in regex --- packages/@aws-cdk/core/lib/construct.ts | 4 +++- packages/@aws-cdk/core/lib/private/encoding.ts | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/core/lib/construct.ts b/packages/@aws-cdk/core/lib/construct.ts index 12ec724a8c5c5..1fc8a85cd3299 100644 --- a/packages/@aws-cdk/core/lib/construct.ts +++ b/packages/@aws-cdk/core/lib/construct.ts @@ -693,10 +693,12 @@ function ignore(_x: any) { import { Reference } from './reference'; +const PATH_SEP_REGEX = new RegExp(`${ConstructNode.PATH_SEP}`, 'g'); + /** * Return a sanitized version of an arbitrary string, so it can be used as an ID */ function sanitizeId(id: string) { // Escape path seps as double dashes - return id.replace(/\//g, '--'); + return id.replace(PATH_SEP_REGEX, '--'); } diff --git a/packages/@aws-cdk/core/lib/private/encoding.ts b/packages/@aws-cdk/core/lib/private/encoding.ts index ef57fe0d06634..51b2c73d8246d 100644 --- a/packages/@aws-cdk/core/lib/private/encoding.ts +++ b/packages/@aws-cdk/core/lib/private/encoding.ts @@ -78,7 +78,7 @@ export class TokenString { /** * Quote a string for use in a regex */ -function regexQuote(s: string) { +export function regexQuote(s: string) { return s.replace(/[.?*+^$[\]\\(){}|-]/g, "\\$&"); }