feat(cloudfront): define lambda@edge as resolvable resource #2861

KnisterPeter · 2019-06-13T15:12:39Z

This declaration is required for deploying custom resources as lamdba
association, which by itself is required to deploy a lambda@edge for a
stack which is in a different region as 'us-east-1'.

Relates to #1575

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
- Design: For significant features, design document added to design folder
Title and Description
- Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

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

KnisterPeter · 2019-06-14T07:01:06Z

@rix0rrr Would you mind to review the second part of the lambda@edge support?

packages/@aws-cdk/aws-cloudfront/test/test.basic.ts

packages/@aws-cdk/aws-cloudfront/lib/web_distribution.ts

packages/@aws-cdk/aws-cloudfront/test/test.basic.ts

KnisterPeter · 2019-06-17T13:37:01Z

@rix0rrr I've integrated all your required changes

KnisterPeter · 2019-06-18T09:06:34Z

@rix0rrr At least everything compiles now and the test look fine. I'm not sure if this is working, since using this in our project with the current CDK build and link-all.sh script result in compile error in our project from the CDK. I'm not able to create a working example right now.

packages/@aws-cdk/aws-lambda/lib/lambda-version.ts

packages/@aws-cdk/aws-cloudfront/lib/web_distribution.ts

packages/@aws-cdk/aws-lambda/lib/lambda-version.ts

rix0rrr · 2019-06-18T13:08:03Z

packages/@aws-cdk/aws-lambda/lib/lambda-version.ts

+   * @param versionArn The version ARN to create this version from
+   */
+  public static fromVersionArn(scope: Construct, id: string, versionArn: string): IVersion {
+    return Version.fromVersionAttributes(scope, id, {


What you did here is correct, but the VersionArn now renders to something super silly, as you can tell from the test output.

It renders to Join(":", [Select(0, Split(":", Arn)), Select(1, Split(":", Arn)), ..., Select(6, Split(":", Arn))]) + ":" + Select(7, Split(":", Arn)), which is just a very roundabout way of getting the same value again.

Could you do something similar to the class Import block defined in the function below, and obtain functionName and functionArn in a more direct way (with fewer splits)?

I can only assume that the version ARN is the function ARN with appended :version and split it with typescript code. That will fail if one puts in a reference here, but that might be okay?

Otherwise I cannot assume anything about the content of versionArn. Therefore I used the cdk Fn helper to split and concat parts 0-6 (not 0-7), but jepp - thats kind of silly.

@rix0rrr I think it is not a valid assumtion that the value of versionArn is a literal string. The case described in the unit test by itself already fails that assumtion. So maybe we should keep the silly thing? At least it splits the version from the given ARN.

I think it is not a valid assumtion that the value of versionArn is a literal string.

No, but the passed versionArn could go directly into functionArn.

The only thing that then would need splitting is functionName, which can be obtained by Fn.select(6, Fn.split(':', versionArn)).

I've updated the implementation in addition with a rebase.
Unfortunately I couldn't compile the actual master so I haven't run (or updated) the tests.

KnisterPeter · 2019-06-19T12:07:34Z

@rix0rrr Do you have an option to get around the open issue/change? I have no idea to solve this. Could you please rereview?

KnisterPeter · 2019-06-20T07:41:03Z

@rix0rrr I've rebased this to the latest release.

This declaration is required for deploying custom resources as lamdba association, which by itself is required to deploy a lambda@edge for a stack which is in a different region as 'us-east-1'. Relates to aws#1575

This commit allows to reference a lambda from a different region and use that as function association.

mergify · 2019-08-21T09:19:19Z

Pull Request Checklist

Testing

Unit test added (prefer to add a new test rather than modify existing tests)
CLI change? Re-run/add CLI integration tests

Documentation

Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
README: update module README
Design: for significant features, follow the design process

Title uses the format type(scope): text

Type: fix, feat, refactor go into CHANGELOG, chore is hidden
Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
Style: use all lower-case, do not end with a period

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>

Sensitive Modules (requires 2 PR approvers)

IAM document library (in @aws-cdk/aws-iam)
EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
Grant APIs (if not based on official documentation with a reference)

Pull request has been modified.

mergify · 2019-08-21T09:19:22Z

Pull Request Checklist

Testing

Unit test added (prefer to add a new test rather than modify existing tests)
CLI change? Re-run/add CLI integration tests

Documentation

Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
README: update module README
Design: for significant features, follow the design process

Title uses the format type(scope): text

Type: fix, feat, refactor go into CHANGELOG, chore is hidden
Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
Style: use all lower-case, do not end with a period

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>

Sensitive Modules (requires 2 PR approvers)

IAM document library (in @aws-cdk/aws-iam)
EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
Grant APIs (if not based on official documentation with a reference)

mergify · 2019-08-21T09:49:57Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

rix0rrr · 2019-08-21T10:15:28Z

Temporarily not merging while we are releasing.

mergify · 2019-08-21T11:53:51Z

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

Pull request has been modified.

mergify · 2019-08-21T13:44:34Z

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

mergify · 2019-08-21T14:36:54Z

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

KnisterPeter requested a review from a team as a code owner June 13, 2019 15:12

rix0rrr previously requested changes Jun 17, 2019

View reviewed changes

packages/@aws-cdk/aws-cloudfront/test/test.basic.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-cloudfront/lib/web_distribution.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-cloudfront/test/test.basic.ts Outdated Show resolved Hide resolved

KnisterPeter force-pushed the lambda-edge-as-iresolvable branch from c11b70c to c1f2e76 Compare June 17, 2019 10:15

KnisterPeter force-pushed the lambda-edge-as-iresolvable branch from 4c4e3a8 to 2dc398b Compare June 18, 2019 09:04

rix0rrr reviewed Jun 18, 2019

View reviewed changes

KnisterPeter force-pushed the lambda-edge-as-iresolvable branch from 5292fa9 to 0171ac9 Compare June 20, 2019 07:40

KnisterPeter added 6 commits June 21, 2019 11:29

feat(cloudfront): define lambda@edge as resolvable resource

2108654

This declaration is required for deploying custom resources as lamdba association, which by itself is required to deploy a lambda@edge for a stack which is in a different region as 'us-east-1'. Relates to aws#1575

test: simplified test case by just test resources required for the case

d7f9f52

feat: allow to create a version from an arn

ad18695

This commit allows to reference a lambda from a different region and use that as function association.

refactor: resolve conflicts

769c77d

refactor: update based on review

0e2a023

refactor: use version arn as function arn

a819402

KnisterPeter force-pushed the lambda-edge-as-iresolvable branch from 0171ac9 to a819402 Compare June 21, 2019 09:42

NGL321 assigned RomainMuller, skinny85, rix0rrr, eladb and shivlaks Aug 1, 2019

eladb unassigned RomainMuller, skinny85, rix0rrr, eladb and shivlaks Aug 5, 2019

NGL321 assigned eladb Aug 6, 2019

eladb assigned rix0rrr Aug 13, 2019

eladb removed their assignment Aug 13, 2019

Merge remote-tracking branch 'origin/master' into pr/KnisterPeter/2861

b0f5c89

mergify bot added the automerge-enabled label Aug 21, 2019

Fixing package.json

df767c4

rix0rrr added 2 commits August 21, 2019 11:57

Merge remote-tracking branch 'origin/master' into pr/KnisterPeter/2861

0eb2108

Fix tests

ab5293a

rix0rrr previously approved these changes Aug 21, 2019

View reviewed changes

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Aug 21, 2019

rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Aug 21, 2019

Merge branch 'master' into lambda-edge-as-iresolvable

93ea85e

rix0rrr approved these changes Aug 21, 2019

View reviewed changes

Merge branch 'master' into lambda-edge-as-iresolvable

80793b3

mergify bot merged commit c39d659 into aws:master Aug 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cloudfront): define lambda@edge as resolvable resource #2861

feat(cloudfront): define lambda@edge as resolvable resource #2861

KnisterPeter commented Jun 13, 2019

KnisterPeter commented Jun 14, 2019

KnisterPeter commented Jun 17, 2019

KnisterPeter commented Jun 18, 2019

rix0rrr Jun 18, 2019

KnisterPeter Jun 18, 2019

KnisterPeter Jun 18, 2019

rix0rrr Jun 21, 2019

KnisterPeter Jun 21, 2019

KnisterPeter commented Jun 19, 2019

KnisterPeter commented Jun 20, 2019

mergify bot commented Aug 21, 2019

mergify bot commented Aug 21, 2019

mergify bot commented Aug 21, 2019

rix0rrr commented Aug 21, 2019

mergify bot commented Aug 21, 2019

mergify bot commented Aug 21, 2019

mergify bot commented Aug 21, 2019

feat(cloudfront): define lambda@edge as resolvable resource #2861

feat(cloudfront): define lambda@edge as resolvable resource #2861

Conversation

KnisterPeter commented Jun 13, 2019

Pull Request Checklist

KnisterPeter commented Jun 14, 2019

KnisterPeter commented Jun 17, 2019

KnisterPeter commented Jun 18, 2019

rix0rrr Jun 18, 2019

Choose a reason for hiding this comment

KnisterPeter Jun 18, 2019

Choose a reason for hiding this comment

KnisterPeter Jun 18, 2019

Choose a reason for hiding this comment

rix0rrr Jun 21, 2019

Choose a reason for hiding this comment

KnisterPeter Jun 21, 2019

Choose a reason for hiding this comment

KnisterPeter commented Jun 19, 2019

KnisterPeter commented Jun 20, 2019

mergify bot commented Aug 21, 2019

Pull Request Checklist

mergify bot commented Aug 21, 2019

Pull Request Checklist

mergify bot commented Aug 21, 2019

rix0rrr commented Aug 21, 2019

mergify bot commented Aug 21, 2019

mergify bot commented Aug 21, 2019

mergify bot commented Aug 21, 2019