fix(ssm): malformed ARNs for parameters with physical names that use path notation #4842

eladb · 2019-11-04T15:43:01Z

SSM parameter names can have one of two forms: “simpleName” or “/path/name”. This makes it impossible to render an ARN for the parameter is the name is an unresolvable token (such as a “Ref”) because we can’t decide whether a “/“ separator is required in the ARN.

The previous implementation assumed "Ref" always returns the name without a "/" prefix, and therefore did not use the "/" separator.

This fix will use the physical name itself (if possible) to determine the separator (and also assume that generated names will not use the path notation) and in the case where there is no concrete name to use, it requires that users explicitly specify the ARN separator through an attribute or a prop.

This change also adds a validation that verifies that if a physical name is provided and uses path notation, it must begin with a "/".

Fixes #4803
Fixes #2777

Misc: re-add install.sh to call npx yarn install

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

…path notation SSM parameter names can have one of two forms: “simpleName” or “/path/name”. This makes it tricky to render an ARN for the parameter is the name is an unresolvable token (such as a “Ref”) because we can’t decide whether a “/“ separator is required in the ARN. The previous implementation assumed "Ref" always returns the name without a "/" prefix, and therefore did not use the "/" separator. This fix will use the physical name itself (if possible) to determine the separator (and also assume that generated names will not use the path notation). The only case where this is impossible is if the physical name is a token (either created or imported), in which case we should be able to synthesize a CloudFormation condition which will parse the token during deployment. This test also adds a validation that verifies that if a physical name is provided and uses path notation, it must begin with a "/". Misc: re-add `install.sh` to call `npx yarn install`

mergify · 2019-11-04T15:43:39Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

packages/@aws-cdk/aws-ssm/lib/parameter.ts

rix0rrr · 2019-11-04T15:50:05Z

packages/@aws-cdk/aws-ssm/lib/util.ts

+      // parameterName is a token and physical name is not helping us (either missing or a token itself)
+      // in this use case we will need to synthesize a CloudFormation condition that will be used to determine
+      // if the name has a "/" prefix or not.
+      const startsWithSlash = startsWithCondition(scope as Construct, parameterName, "/");


Oh shit. After thinking about it some more, I'm not sure this is going to work. At least, this won't work for { Refs }s, as Conditions need to evaluate before Resources are.

Yes, you are right:

❌ integ-parameter-arns failed: ValidationError: Template format error: Unresolved dependencies [StringDeployTime8EC8E402]. Cannot reference resources in the Conditions block of the template

aws-cdk-automation · 2019-11-04T16:01:52Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

revert attempt to guess parameter name prefix if it's a token since we can't incorporate refs in conditions. Instead, if the parameter name if a token, we expect `parameterArnSeparator` to be explicitly defined and be one of "/" or "".

aws-cdk-automation · 2019-11-04T17:59:26Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

* fix test expectation * add public API doc

aws-cdk-automation · 2019-11-04T20:36:31Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr · 2019-11-05T08:31:01Z

install.sh

@@ -0,0 +1,3 @@
+#!/bin/bash
+set -euo pipefail
+exec npx yarn install


--frozen-lockfile

rix0rrr · 2019-11-05T08:31:22Z

packages/@aws-cdk/aws-ssm/lib/parameter.ts

+   * @default - automatically determined based on the value of `parameterName`
+   * unless it is a token, in which case this field is required.
+   */
+  readonly parameterArnSeparator?: string;


Why is this not simply a boolean startsWithSlash or not?

I don't know. I am dumb?

Please fix!! I am getting Unable to determine ARN separator for SSM parameter since the parameter name is an unresolved token. Use "fromAttributes" and specify "simpleName" explicitly

Can you please raise a new issue with a retro?

aws-cdk-automation · 2019-11-05T09:33:36Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2019-11-05T09:34:09Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2019-11-05T10:04:22Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr reviewed Nov 4, 2019

View reviewed changes

packages/@aws-cdk/aws-ssm/lib/parameter.ts Outdated Show resolved Hide resolved

rix0rrr reviewed Nov 4, 2019

View reviewed changes

SomayaB assigned eladb Nov 4, 2019

SomayaB added the contribution/core This is a PR that came from AWS. label Nov 4, 2019

explicit parameterArnSeparator

b0e5fa5

revert attempt to guess parameter name prefix if it's a token since we can't incorporate refs in conditions. Instead, if the parameter name if a token, we expect `parameterArnSeparator` to be explicitly defined and be one of "/" or "".

Elad Ben-Israel added 2 commits November 4, 2019 20:02

Merge remote-tracking branch 'origin/master' into benisrae/fix-ssm-arns

47a1661

misc

210d0b0

* fix test expectation * add public API doc

rix0rrr reviewed Nov 5, 2019

View reviewed changes

Elad Ben-Israel added 2 commits November 5, 2019 11:09

add --frozen-lockfile to install.sh

5494c88

rename "parameterArnSeparator: string" to "simpleName: boolean"

8d8ee14

rix0rrr approved these changes Nov 5, 2019

View reviewed changes

Merge branch 'master' into benisrae/fix-ssm-arns

31a866b

mergify bot merged commit 43f276a into master Nov 5, 2019

mergify bot deleted the benisrae/fix-ssm-arns branch November 5, 2019 10:07

fix(ssm): malformed ARNs for parameters with physical names that use path notation #4842

fix(ssm): malformed ARNs for parameters with physical names that use path notation #4842

Uh oh!

Conversation

eladb commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify bot commented Nov 4, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Nov 4, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Nov 4, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Nov 4, 2019

AWS CodeBuild CI Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Nov 5, 2019

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Nov 5, 2019

Uh oh!

aws-cdk-automation commented Nov 5, 2019

AWS CodeBuild CI Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eladb commented Nov 4, 2019 •

edited

Loading