From 39df45609a86a1f6871abaa8ec768d697c448eca Mon Sep 17 00:00:00 2001
From: Romain Marcadier-Muller <romain.muller@telecomnancy.net>
Date: Tue, 12 Feb 2019 15:47:28 +0100
Subject: [PATCH] fix(ssm): Generate correct SSM Parameter ARN (#1726)

The SSM parameter name (`resourceName` portion of it's ARN) is required
to start with a `/`, which caused the presence of a doubled `//` in the generated
ARN caused by the fact `sep` defaults to `/`.

This change allows `sep` to be set to an empty string, allowing SSM rendering
of correct SSM parameter ARNs.
---
 packages/@aws-cdk/aws-ssm/lib/parameter.ts    |  1 +
 .../test/integ.parameter.lit.expected.json    |  2 +-
 .../@aws-cdk/aws-ssm/test/test.parameter.ts   | 21 +++++++++++++++++++
 .../@aws-cdk/cdk/lib/cloudformation/arn.ts    | 10 ++++-----
 .../cdk/test/cloudformation/test.arn.ts       | 17 +++++++++++++++
 5 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/packages/@aws-cdk/aws-ssm/lib/parameter.ts b/packages/@aws-cdk/aws-ssm/lib/parameter.ts
index c45a74e3b82c0..f228ac84a5ab0 100644
--- a/packages/@aws-cdk/aws-ssm/lib/parameter.ts
+++ b/packages/@aws-cdk/aws-ssm/lib/parameter.ts
@@ -119,6 +119,7 @@ export abstract class ParameterBase extends cdk.Construct implements IParameter
     return cdk.Stack.find(this).formatArn({
       service: 'ssm',
       resource: 'parameter',
+      sep: '', // Sep is empty because this.parameterName starts with a / already!
       resourceName: this.parameterName,
     });
   }
diff --git a/packages/@aws-cdk/aws-ssm/test/integ.parameter.lit.expected.json b/packages/@aws-cdk/aws-ssm/test/integ.parameter.lit.expected.json
index 30cf55aa0f712..2985dd230d609 100644
--- a/packages/@aws-cdk/aws-ssm/test/integ.parameter.lit.expected.json
+++ b/packages/@aws-cdk/aws-ssm/test/integ.parameter.lit.expected.json
@@ -60,7 +60,7 @@
                     {
                       "Ref": "AWS::AccountId"
                     },
-                    ":parameter/",
+                    ":parameter",
                     {
                       "Ref": "StringParameter472EED0E"
                     }
diff --git a/packages/@aws-cdk/aws-ssm/test/test.parameter.ts b/packages/@aws-cdk/aws-ssm/test/test.parameter.ts
index fb985eea620de..35eacb136cad3 100644
--- a/packages/@aws-cdk/aws-ssm/test/test.parameter.ts
+++ b/packages/@aws-cdk/aws-ssm/test/test.parameter.ts
@@ -100,4 +100,25 @@ export = {
     }));
     test.done();
   },
+
+  'parameterArn is crafted correctly'(test: Test) {
+    // GIVEN
+    const stack = new cdk.Stack();
+    const param = new ssm.StringParameter(stack, 'Parameter', { value: 'Foo' });
+
+    // THEN
+    test.deepEqual(param.node.resolve(param.parameterArn), {
+      'Fn::Join': ['', [
+        'arn:',
+        { Ref: 'AWS::Partition' },
+        ':ssm:',
+        { Ref: 'AWS::Region' },
+        ':',
+        { Ref: 'AWS::AccountId' },
+        ':parameter',
+        { Ref: 'Parameter9E1B4FBA' }
+      ]]
+    });
+    test.done();
+  }
 };
diff --git a/packages/@aws-cdk/cdk/lib/cloudformation/arn.ts b/packages/@aws-cdk/cdk/lib/cloudformation/arn.ts
index e6b86166c3a01..7bffa08d2da3f 100644
--- a/packages/@aws-cdk/cdk/lib/cloudformation/arn.ts
+++ b/packages/@aws-cdk/cdk/lib/cloudformation/arn.ts
@@ -13,7 +13,7 @@ import { Stack } from './stack';
  *
  * The ARN will be formatted as follows:
  *
- *   arn:{partition}:{service}:{region}:{account}:{resource}{sep}}{resource-name}
+ *   arn:{partition}:{service}:{region}:{account}:{resource}{sep}{resource-name}
  *
  * The required ARN pieces that are omitted will be taken from the stack that
  * the 'scope' is attached to. If all ARN pieces are supplied, the supplied scope
@@ -23,12 +23,12 @@ export function arnFromComponents(components: ArnComponents, stack: Stack): stri
   const partition = components.partition !== undefined ? components.partition : stack.partition;
   const region = components.region !== undefined ? components.region : stack.region;
   const account = components.account !== undefined ? components.account : stack.accountId;
+  const sep = components.sep !== undefined ? components.sep : '/';
 
   const values = [ 'arn', ':', partition, ':', components.service, ':', region, ':', account, ':', components.resource ];
 
-  const sep = components.sep || '/';
-  if (sep !== '/' && sep !== ':') {
-    throw new Error('resourcePathSep may only be ":" or "/"');
+  if (sep !== '/' && sep !== ':' && sep !== '') {
+    throw new Error('resourcePathSep may only be ":", "/" or an empty string');
   }
 
   if (components.resourceName != null) {
@@ -257,7 +257,7 @@ export interface ArnComponents {
   /**
    * Separator between resource type and the resource.
    *
-   * Can be either '/' or ':'. Will only be used if path is defined.
+   * Can be either '/', ':' or an empty string. Will only be used if resourceName is defined.
    * @default '/'
    */
   sep?: string;
diff --git a/packages/@aws-cdk/cdk/test/cloudformation/test.arn.ts b/packages/@aws-cdk/cdk/test/cloudformation/test.arn.ts
index cbb3c3caa96de..6f2761d31fee7 100644
--- a/packages/@aws-cdk/cdk/test/cloudformation/test.arn.ts
+++ b/packages/@aws-cdk/cdk/test/cloudformation/test.arn.ts
@@ -68,6 +68,23 @@ export = {
     test.done();
   },
 
+  'resourcePathSep can be set to "" instead of the default "/"'(test: Test) {
+    const stack = new Stack();
+
+    const arn = stack.formatArn({
+      service: 'ssm',
+      resource: 'parameter',
+      sep: '',
+      resourceName: '/parameter-name'
+    });
+
+    const pseudo = new Aws(stack);
+
+    test.deepEqual(stack.node.resolve(arn),
+                   stack.node.resolve(`arn:${pseudo.partition}:ssm:${pseudo.region}:${pseudo.accountId}:parameter/parameter-name`));
+    test.done();
+  },
+
   'fails if resourcePathSep is neither ":" nor "/"'(test: Test) {
     const stack = new Stack();