Skip to content

Commit

Permalink
fix(ssm): correctly deduplicate parameter names (aws#3183)
Browse files Browse the repository at this point in the history
Parameter names with '/' in them would not be correctly deduplicated,
since the '/' is also used to delimit identifiers in construct paths.

Change findChild() so it no longer traverses into the tree; if we assume it
will only ever find one child deep, we can do the same kind of escaping
there as we apply to construct IDs.

Fixes aws#3076.

BREAKING CHANGE: `construct.findChild()` now only looks up direct children
  • Loading branch information
rix0rrr authored and Kaixiang-AWS committed Jul 3, 2019
1 parent bc567b6 commit 84e18a4
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 31 deletions.
16 changes: 16 additions & 0 deletions packages/@aws-cdk/aws-ecs/test/test.ecs-cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-ssm/lib/parameter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
12 changes: 11 additions & 1 deletion packages/@aws-cdk/aws-ssm/test/test.parameter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
},
}
};
45 changes: 16 additions & 29 deletions packages/@aws-cdk/core/lib/construct.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,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
Expand All @@ -146,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}`);
}
Expand All @@ -174,25 +171,13 @@ export class ConstructNode {
}

/**
* Return a descendant by path, or undefined
* Return a direct child by id, or undefined
*
* Note that if the original ID of the construct you are looking for contained
* a '/', then it would have been replaced by '--'.
*
* @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)];
}

/**
Expand Down Expand Up @@ -527,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('--');
}
}

/**
Expand Down Expand Up @@ -715,3 +692,13 @@ function ignore(_x: any) {
// Import this _after_ everything else to help node work the classes out in the correct order...

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(PATH_SEP_REGEX, '--');
}
2 changes: 1 addition & 1 deletion packages/@aws-cdk/core/lib/private/encoding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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, "\\$&");
}

Expand Down

0 comments on commit 84e18a4

Please sign in to comment.