fix(core): parsing an ARN with a slash after a colon in the resource part fails

[skinny85]

New-style ARNs are of the form 'arn:aws:s4:us-west-1:12345:/resource-type/resource-name'.
We didn't handle that correctly in parseArn(), and instead returned an undefined resource,
which funnily enough should never happen according to our types.
Introduce the concept of ARN formats,
represented by an enum in core,
and replace the Stack.parseArn() method by a new one Stack.splitArn(),
taking that enum as a required second argument.

Spotted in https://github.com/aws/aws-cdk/pull/15140/files#r653112073

---

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

[gitpod-io]

[rix0rrr]

Two things:

• ARNs need to be roundtrippable: we need to be able to formatArn() the information we got out of parseArn() and get the same ARN back. We might need to add modernArn: boolean to ArnComponents or something (or maybe typeSep = ':' | ':/').
• We need parseArnIfToken() to produce CloudFormation expressions which, when evaluated, would produce the same value as parseArn() would do on a literal string.

For the latter, given eval-cfn.ts we could probably have each test exercise both forms of parsing, so that might be a worthwhile thing to add: https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/core/test/evaluate-cfn.ts/#L8

[rix0rrr]

The double test would look somewhat like:

function assertArnParsesProperlyTo(arn: string, expectedComponents: ArnComponents, parseOptions: ...) {
  const components = stack.parseArn(arn);
  expect(components).toEqual(expectedComponents);

  const tokenExprs = stack.parseArn(new Intrinsic({ Ref: TheArn }), ...parseOptions);
  const tokenComponents = evaluateCFN(tokenExprs, { TheArn: arn });
  expect(tokenComponents).toEqual(expectedComponents);
}

(or something)

[skinny85]

@rix0rrr included your comments. Please take another look!

[rix0rrr]

Provisional approval provided you come up with a good name for the new parameter :)

[skinny85]

This is definitely the most interesting test case. I can be convinced that it should return arnFormat : ArnFormat.SLASH_RESOURCE_NAME, resource: 'my_corporate_bucket', sep: '/', and resourceName: 'object.zip' instead of what it's returning right now.

[rix0rrr]

I think the current interpretation is correct.

arn:aws:s3:::my_corporate_bucket/object.zip is really addressing a file in a bucket, with a fully qualified name. It makes sense that my_corporate_bucket/object.zip is the value of resource.

What I personally don't like is that resource varyingly means the TYPE of the resource, or the NAME of the resource itself (depending on ARN format), but... c'est la vie, not something we can reasonably change anymore...

[skinny85]

@rix0rrr I've decided to go with the option of introducing the ArnFormat concept. I'm really happy with how it turned out - I think the API of parsing ARNs is now much cleaner than it was before.

Let me know what you think!

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: e37d4dc
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[skinny85]

@Mergifyio refresh

[mergify]

Command refresh: success

Pull request refreshed

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).