Skip to content

Commit

Permalink
fix(ssm): correctly deduplicate parameter names
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.

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.
  • Loading branch information
rix0rrr committed Jul 3, 2019
1 parent 6679e86 commit 7cb0922
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 3 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
5 changes: 3 additions & 2 deletions packages/@aws-cdk/aws-ssm/lib/parameter.ts
Original file line number Diff line number Diff line change
@@ -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');
Expand Down 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 Expand Up @@ -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 {
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();
},
}
};
12 changes: 12 additions & 0 deletions packages/@aws-cdk/core/lib/construct.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down
6 changes: 6 additions & 0 deletions packages/@aws-cdk/core/test/test.construct.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down

0 comments on commit 7cb0922

Please sign in to comment.