Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cloudfront): add PublicKey and KeyGroup L2 constructs #12743

Merged

Conversation

robertd
Copy link
Contributor

@robertd robertd commented Jan 28, 2021

@njlynch This is my humble start on creating L2 constructs for PublicKey and KeyGroup for CloudFront module. I'm going to need some guidance/mentorship as this is my first L2 construct from the scratch. I'll convert this PR to draft and I'll post some of my thoughts and ideas around this feature tomorrow. I'm trying to address feature requests in #11791. I've decided to lump PublicKey and KeyGroup features together as they seem to depend on each other.

All in the good spirits of learning how to extend CDK 🍻 .

Any ideas and/or constructive criticism is more than welcome... that's the best way to learn.✌️


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

@gitpod-io
Copy link

gitpod-io bot commented Jan 28, 2021

@github-actions github-actions bot added the @aws-cdk/aws-cloudfront Related to Amazon CloudFront label Jan 28, 2021
@njlynch njlynch self-requested a review January 28, 2021 13:12
Copy link
Contributor

@njlynch njlynch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution! Nice work.

It's already looking pretty good, just a few minor comments on what you have here.
Next step(s) would be tests, some awslint exclusions in the package.json file for some of the missing properties (LastModifiedTime and CreatedTime, which I think are safe to omit), and then (preferably here, but maybe in a separate PR) integration with the Distribution(s).

packages/@aws-cdk/aws-cloudfront/lib/index.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cloudfront/lib/key-group.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cloudfront/lib/key-group.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cloudfront/lib/public-key.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cloudfront/lib/public-key.ts Outdated Show resolved Hide resolved
@robertd
Copy link
Contributor Author

robertd commented Jan 28, 2021

@njlynch Thank you so much for your mentorship and guidance. Much appreciated.

It's already looking pretty good, just a few minor comments on what you have here.
Next step(s) would be tests, some awslint exclusions in the package.json file for some of the missing properties (LastModifiedTime and CreatedTime, which I think are safe to omit), and then (preferably here, but maybe in a separate PR) integration with the Distribution(s).

Can you elaborate a bit on awslint exclusions. Are those autogenerated or I need to manually add them in package.json?

Something like this?

"docs-public-apis:@aws-cdk/aws-cloudfront.PublicKey.CreatedTime",
"docs-public-apis:@aws-cdk/aws-cloudfront.KeyGroup.LastModifiedTime",

@njlynch
Copy link
Contributor

njlynch commented Jan 28, 2021

Can you elaborate a bit on awslint exclusions. Are those autogenerated or I need to manually add them in package.json?

They need to be manually added. When you run the build (e.g., lb from https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md#partial-build-tools) or separately run the linter (yarn awslint), you should see errors pop up. For these specific attributes, simply add the errors to the exclusion list already present in the package.json.

@robertd
Copy link
Contributor Author

robertd commented Jan 28, 2021

@njlynch I have lots of work to do 😄 . I see some awslint errors outside of the scope of this PR... I should probably address those in a separate PR.

~ yarn awslint
error: [awslint:construct-ctor:@aws-cdk/aws-cloudfront.CachePolicy.<initializer>.params[0]] signature of all construct constructors should be "scope, id, props". If the construct is using the "constructs" module, set the environment variable "AWSLINT_BASE_CONSTRUCT" and re-run  (expected="@aws-cdk/core.Construct",actual="constructs.Construct")
error: [awslint:construct-ctor:@aws-cdk/aws-cloudfront.CloudFrontWebDistribution.<initializer>.params[0]] signature of all construct constructors should be "scope, id, props". If the construct is using the "constructs" module, set the environment variable "AWSLINT_BASE_CONSTRUCT" and re-run  (expected="@aws-cdk/core.Construct",actual="constructs.Construct")
error: [awslint:construct-ctor:@aws-cdk/aws-cloudfront.Distribution.<initializer>.params[0]] signature of all construct constructors should be "scope, id, props". If the construct is using the "constructs" module, set the environment variable "AWSLINT_BASE_CONSTRUCT" and re-run  (expected="@aws-cdk/core.Construct",actual="constructs.Construct")
error: [awslint:construct-ctor:@aws-cdk/aws-cloudfront.KeyGroup.<initializer>.params[0]] signature of all construct constructors should be "scope, id, props". If the construct is using the "constructs" module, set the environment variable "AWSLINT_BASE_CONSTRUCT" and re-run  (expected="@aws-cdk/core.Construct",actual="constructs.Construct")
error: [awslint:construct-interface-extends-iconstruct:@aws-cdk/aws-cloudfront.IKeyGroup] construct interface must extend core.IConstruct
error: [awslint:construct-ctor:@aws-cdk/aws-cloudfront.OriginAccessIdentity.<initializer>.params[0]] signature of all construct constructors should be "scope, id, props". If the construct is using the "constructs" module, set the environment variable "AWSLINT_BASE_CONSTRUCT" and re-run  (expected="@aws-cdk/core.Construct",actual="constructs.Construct")
error: [awslint:construct-ctor:@aws-cdk/aws-cloudfront.OriginRequestPolicy.<initializer>.params[0]] signature of all construct constructors should be "scope, id, props". If the construct is using the "constructs" module, set the environment variable "AWSLINT_BASE_CONSTRUCT" and re-run  (expected="@aws-cdk/core.Construct",actual="constructs.Construct")
error: [awslint:construct-ctor:@aws-cdk/aws-cloudfront.PublicKey.<initializer>.params[0]] signature of all construct constructors should be "scope, id, props". If the construct is using the "constructs" module, set the environment variable "AWSLINT_BASE_CONSTRUCT" and re-run  (expected="@aws-cdk/core.Construct",actual="constructs.Construct")
error: [awslint:construct-interface-extends-iconstruct:@aws-cdk/aws-cloudfront.IPublicKey] construct interface must extend core.IConstruct
error: [awslint:resource-interface-extends-resource:@aws-cdk/aws-cloudfront.IKeyGroup] construct interfaces of AWS resources must extend cdk.IResource
error: [awslint:resource-interface-extends-resource:@aws-cdk/aws-cloudfront.IPublicKey] construct interfaces of AWS resources must extend cdk.IResource
error: [awslint:resource-attribute:@aws-cdk/aws-cloudfront.PublicKey.keyGroupId] resources must represent all cloudformation attributes as attribute properties. "@attribute ATTR[,ATTR]" can be used to tag non-standard attribute names. missing property: keyGroupId
error: [awslint:props-physical-name:@aws-cdk/aws-cloudfront.PublicKeyProps] Every Resource must have a single physical name construction property, with a name that is an ending substring of <cfnResource>Name
error: [awslint:from-signature:@aws-cdk/aws-cloudfront.CachePolicy.fromCachePolicyId.params[0]] invalid method signature for fromXxx method. If the construct is using the "constructs" module, set the environment variable "AWSLINT_BASE_CONSTRUCT" and re-run  (expected="@aws-cdk/core.Construct",actual="constructs.Construct")
error: [awslint:from-attributes:@aws-cdk/aws-cloudfront.Distribution.fromDistributionAttributes.params[0]] static fromXxxAttributes is a factory of IXxx from its primitive attributes. If the construct is using the "constructs" module, set the environment variable "AWSLINT_BASE_CONSTRUCT" and re-run  (expected="@aws-cdk/core.Construct",actual="constructs.Construct")
error: [awslint:from-signature:@aws-cdk/aws-cloudfront.KeyGroup.fromKeyGroupId.params[0]] invalid method signature for fromXxx method. If the construct is using the "constructs" module, set the environment variable "AWSLINT_BASE_CONSTRUCT" and re-run  (expected="@aws-cdk/core.Construct",actual="constructs.Construct")
error: [awslint:from-signature:@aws-cdk/aws-cloudfront.OriginAccessIdentity.fromOriginAccessIdentityName.params[0]] invalid method signature for fromXxx method. If the construct is using the "constructs" module, set the environment variable "AWSLINT_BASE_CONSTRUCT" and re-run  (expected="@aws-cdk/core.Construct",actual="constructs.Construct")
error: [awslint:from-signature:@aws-cdk/aws-cloudfront.OriginRequestPolicy.fromOriginRequestPolicyId.params[0]] invalid method signature for fromXxx method. If the construct is using the "constructs" module, set the environment variable "AWSLINT_BASE_CONSTRUCT" and re-run  (expected="@aws-cdk/core.Construct",actual="constructs.Construct")
error: [awslint:from-signature:@aws-cdk/aws-cloudfront.PublicKey.fromPublicKeyId.params[0]] invalid method signature for fromXxx method. If the construct is using the "constructs" module, set the environment variable "AWSLINT_BASE_CONSTRUCT" and re-run  (expected="@aws-cdk/core.Construct",actual="constructs.Construct")
error: [awslint:docs-public-apis:@aws-cdk/aws-cloudfront.KeyGroup.fromKeyGroupId] Public API element must have a docstring
error: [awslint:docs-public-apis:@aws-cdk/aws-cloudfront.PublicKey.fromPublicKeyId] Public API element must have a docstring
error: [awslint:props-default-doc:@aws-cdk/aws-cloudfront.KeyGroupProps.items] Optional property must have @default documentation
error Command failed with exit code 1.

@njlynch
Copy link
Contributor

njlynch commented Jan 28, 2021

I see some awslint errors outside of the scope of this PR... I should probably address those in a separate PR.

See the error message: [...] If the construct is using the "constructs" module, set the environment variable "AWSLINT_BASE_CONSTRUCT" and re-run [...] .

If you set that and re-run, it looks like all of the errors are related to the new constructs and probably relevant to fix (or explicitly ignore).

@robertd
Copy link
Contributor Author

robertd commented Jan 28, 2021

@njlynch Interestingly... awslint is reporting these two params to be added to the package.json file...

      "resource-attribute:@aws-cdk/aws-cloudfront.PublicKey.keyGroupLastModifiedTime",
      "resource-attribute:@aws-cdk/aws-cloudfront.KeyGroup.keyGroupLastModifiedTime"

...while AWS docs for PublicKey are showing CreatedDate parameter: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-cloudfront-publickey.html

Do you think AWS docs are out of date perhaps?

@robertd
Copy link
Contributor Author

robertd commented Jan 28, 2021

See the error message: [...] If the construct is using the "constructs" module, set the environment variable "AWSLINT_BASE_CONSTRUCT" and re-run [...] .

If you set that and re-run, it looks like all of the errors are related to the new constructs and probably relevant to fix (or explicitly ignore).

Yep... looks better now...

AWSLINT_BASE_CONSTRUCT="1" yarn awslint
yarn run v1.22.10
$ cdk-awslint
error: [awslint:construct-interface-extends-iconstruct:@aws-cdk/aws-cloudfront.IKeyGroup] construct interface must extend core.IConstruct
error: [awslint:construct-interface-extends-iconstruct:@aws-cdk/aws-cloudfront.IPublicKey] construct interface must extend core.IConstruct
error: [awslint:resource-interface-extends-resource:@aws-cdk/aws-cloudfront.IKeyGroup] construct interfaces of AWS resources must extend cdk.IResource
error: [awslint:resource-interface-extends-resource:@aws-cdk/aws-cloudfront.IPublicKey] construct interfaces of AWS resources must extend cdk.IResource
error: [awslint:resource-attribute:@aws-cdk/aws-cloudfront.PublicKey.keyGroupId] resources must represent all cloudformation attributes as attribute properties. "@attribute ATTR[,ATTR]" can be used to tag non-standard attribute names. missing property: keyGroupId
error: [awslint:props-physical-name:@aws-cdk/aws-cloudfront.PublicKeyProps] Every Resource must have a single physical name construction property, with a name that is an ending substring of <cfnResource>Name
error: [awslint:docs-public-apis:@aws-cdk/aws-cloudfront.KeyGroup.fromKeyGroupId] Public API element must have a docstring
error: [awslint:docs-public-apis:@aws-cdk/aws-cloudfront.PublicKey.fromPublicKeyId] Public API element must have a docstring
error: [awslint:props-default-doc:@aws-cdk/aws-cloudfront.KeyGroupProps.items] Optional property must have @default documentation
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@robertd robertd force-pushed the robertd/cloudfront-public-keys-and-key-groups branch from 9beb550 to e0a1c2b Compare January 29, 2021 05:59
@robertd
Copy link
Contributor Author

robertd commented Jan 29, 2021

@njlynch I've added some basic tests...

Any advice on how to tackle these errors?

@aws-cdk/aws-cloudfront: error: [awslint:construct-interface-extends-iconstruct:@aws-cdk/aws-cloudfront.IKeyGroup] construct interface must extend core.IConstruct
@aws-cdk/aws-cloudfront: error: [awslint:construct-interface-extends-iconstruct:@aws-cdk/aws-cloudfront.IPublicKey] construct interface must extend core.IConstruct
@aws-cdk/aws-cloudfront: error: [awslint:resource-interface-extends-resource:@aws-cdk/aws-cloudfront.IKeyGroup] construct interfaces of AWS resources must extend cdk.IResource
@aws-cdk/aws-cloudfront: error: [awslint:resource-interface-extends-resource:@aws-cdk/aws-cloudfront.IPublicKey] construct interfaces of AWS resources must extend cdk.IResource
@aws-cdk/aws-cloudfront: error: [awslint:resource-attribute:@aws-cdk/aws-cloudfront.PublicKey.keyGroupId] resources must represent all cloudformation attributes as attribute properties. "@attribute ATTR[,ATTR]" can be used to tag non-standard attribute names. missing property: keyGroupId
@aws-cdk/aws-cloudfront: error: [awslint:props-physical-name:@aws-cdk/aws-cloudfront.PublicKeyProps] Every Resource must have a single physical name construction property, with a name that is an ending substring of <cfnResource>Name

@njlynch
Copy link
Contributor

njlynch commented Jan 29, 2021

Any advice on how to tackle these errors?

The first four are (hopefully) somewhat self-explanatory; the interfaces need to extend IResource (which extends IConstruct).

The second two took me a minute to spot, but the issue is here: :)
https://github.com/aws/aws-cdk/pull/12743/files#diff-27ff5a6e4e43880801ce1ec71a27263d12a410bac2cbf7be4ed1b8cc59f07329R43-R45

@robertd robertd force-pushed the robertd/cloudfront-public-keys-and-key-groups branch from 800ce3b to 3242e7b Compare January 29, 2021 15:20
@robertd
Copy link
Contributor Author

robertd commented Jan 29, 2021

@njlynch

The first four are (hopefully) somewhat self-explanatory; the interfaces need to extend IResource (which extends IConstruct).

I've been using https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-cloudfront/lib/origin-request-policy.ts as a template and that one doesn't extend IResource for the interfaces... Do you know why?

The second two took me a minute to spot, but the issue is here: :)
https://github.com/aws/aws-cdk/pull/12743/files#diff-27ff5a6e4e43880801ce1ec71a27263d12a410bac2cbf7be4ed1b8cc59f07329R43-R45

Ah... copy paste error and I wasn't even paying attention to comments area :)

@njlynch
Copy link
Contributor

njlynch commented Jan 29, 2021

I've been using [Origin Request Policies] as a template and that one doesn't extend IResource for the interfaces.

That was a conscious design decision to enable the static managed policies API to be as friendly as possible. There's not a huge downside to not extending IResource in that specific case -- or frankly, this one -- as the resources can't be referenced or shared cross-account (due to not having ARNs). That being said, whereas Origin Request & Cache Policies had the excuse of the managed policies, I don't think IPublicKey and IKeyGroup have a good reason not to extend IResource.

@robertd robertd force-pushed the robertd/cloudfront-public-keys-and-key-groups branch from 8860a46 to 500ee4e Compare January 29, 2021 20:48
@robertd robertd marked this pull request as ready for review January 30, 2021 00:11
@robertd
Copy link
Contributor Author

robertd commented Jan 30, 2021

@njlynch I think this is ready for your review. I think this will do it for the MVP. Let me know if you need anything else addressed and then I'll rebase this into a single commit. 👍

In the meantime I'll start working on hooking this up in cloudfront.Distribution in a separate PR.

Btw... I have a few (far fetched) ideas for the future. :)

  • Loading of public keys inline or uploading them through cdk-bucket... something similar to lambda.Code.fromInline and lambda.Code.fromAsset.
  • Key Rotation feature would be nice... using ECS or Lambda to generate private & public keys that would be stored in AWS Secrets Manager and encrypted with KMS.

Thoughts?

@robertd
Copy link
Contributor Author

robertd commented Jan 30, 2021

Anything against adding throwaway public key to the repo? ... It's needed for integ tests to work.

@robertd robertd force-pushed the robertd/cloudfront-public-keys-and-key-groups branch from 2867788 to 291fbd4 Compare January 30, 2021 16:37
packages/@aws-cdk/aws-cloudfront/lib/key-group.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cloudfront/lib/key-group.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cloudfront/lib/key-group.ts Outdated Show resolved Hide resolved

private generateName(): string {
const name = Names.uniqueId(this);
if (name.length > 80) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you find this documented somewhere, or find it via trial & error, or is this just a conservative guess?

Copy link
Contributor Author

@robertd robertd Feb 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main point was more around the length. If the key group can have a 255-character (or 1024-character) name, should we artificially restrict to 80? I suppose this is fine for a first take; Names.uniqueId will rarely return a name this long anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these helper functions (shorteners) be part of Names perhaps?

packages/@aws-cdk/aws-cloudfront/README.md Show resolved Hide resolved
packages/@aws-cdk/aws-cloudfront/README.md Outdated Show resolved Hide resolved
@njlynch
Copy link
Contributor

njlynch commented Feb 1, 2021

Btw... I have a few (far fetched) ideas for the future. :)

  • Loading of public keys inline or uploading them through cdk-bucket... something similar to lambda.Code.fromInline and lambda.Code.fromAsset.
  • Key Rotation feature would be nice... using ECS or Lambda to generate private & public keys that would be stored in AWS Secrets Manager and encrypted with KMS.

Loading keys from file (inline) would be a useful extension. It seems like we could add that in later by adding an encodedKeyFile property, checking that it's exclusive with encodedKey, and then reading from the file to populate the encodedKey. Might even be worth adding in from Day 1 here, or we can wait for more feedback from the community.

Key rotation would be interesting. I wonder about the use cases for that, and the potential complexity with cross-environment/account usage. Ideally, you'd create a new key, add the new key to CloudFront, transition the usage to the new key, and then remove the old key(s) from CloudFront. I imagine that would be a multi-step, multi-deployment activity across multiple accounts in many cases.

Anything against adding throwaway public key to the repo?

No, this is fine. As you said, it's needed for the tests to work.

@njlynch
Copy link
Contributor

njlynch commented Feb 1, 2021

... and then I'll rebase this into a single commit.

Oh btw, don't worry about that. Our automation will automatically update from HEAD, build, squash + commit when it's approved. In the meantime, rebasing makes reviewing incremental diffs on GitHub difficult (or impossible). If you want/need to keep up to date, merge commits are fine (e.g., git merge origin/master), but I have a preference for avoiding rebasing unless it is necessary.

Co-authored-by: Nick Lynch <nlynch@amazon.com>
@robertd
Copy link
Contributor Author

robertd commented Feb 1, 2021

... and then I'll rebase this into a single commit.

Oh btw, don't worry about that. Our automation will automatically update from HEAD, build, squash + commit when it's approved. In the meantime, rebasing makes reviewing incremental diffs on GitHub difficult (or impossible). If you want/need to keep up to date, merge commits are fine (e.g., git merge origin/master), but I have a preference for avoiding rebasing unless it is necessary.

Good to know... I've been rebasing/force pushing to make things look like a single commit ... but I'll stop doing that 👍

@robertd
Copy link
Contributor Author

robertd commented Feb 2, 2021

Loading keys from file (inline) would be a useful extension. It seems like we could add that in later by adding an encodedKeyFile property, checking that it's exclusive with encodedKey, and then reading from the file to populate the encodedKey. Might even be worth adding in from Day 1 here, or we can wait for more feedback from the community.

We can wait for more feedback.

Key rotation would be interesting. I wonder about the use cases for that, and the potential complexity with cross-environment/account usage. Ideally, you'd create a new key, add the new key to CloudFront, transition the usage to the new key, and then remove the old key(s) from CloudFront. I imagine that would be a multi-step, multi-deployment activity across multiple accounts in many cases.

It's a long shot... so we can put this on a wish list.

@robertd
Copy link
Contributor Author

robertd commented Feb 2, 2021

@njlynch I'm going to play with adding ability to point to a .pem key. I'll definitely have few questions for you in the future :)

Until then I think we can release this as MVP unless there are any other issues you want me to address first.

@njlynch
Copy link
Contributor

njlynch commented Feb 2, 2021

Just one more tweak (see above + the change to the test to support it), and this looks good to go!

@robertd
Copy link
Contributor Author

robertd commented Feb 2, 2021

@njlynch Ready for your review! 👍 Many thanks for all your assistance.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 7c7e041
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Copy link
Contributor

@njlynch njlynch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent!

@mergify
Copy link
Contributor

mergify bot commented Feb 3, 2021

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).

@mergify mergify bot merged commit 59cb6d0 into aws:master Feb 3, 2021
@robertd robertd deleted the robertd/cloudfront-public-keys-and-key-groups branch February 4, 2021 17:37
NovakGu pushed a commit to NovakGu/aws-cdk that referenced this pull request Feb 18, 2021
@njlynch This is my humble start on creating L2 constructs for `PublicKey` and `KeyGroup` for CloudFront module. I'm going to need some guidance/mentorship as this is my first L2 construct from the scratch. I'll convert this PR to draft and I'll post some of my thoughts and ideas around this feature tomorrow. I'm trying to address feature requests in aws#11791. I've decided to lump `PublicKey` and `KeyGroup` features together as they seem to depend on each other.

All in the good spirits of learning how to extend CDK 🍻 .

Any ideas and/or constructive criticism is more than welcome... that's the best way to learn.✌️ 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-cloudfront Related to Amazon CloudFront
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants