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): support for cache policies #10656

Merged
merged 11 commits into from
Oct 7, 2020
Merged

Conversation

njlynch
Copy link
Contributor

@njlynch njlynch commented Oct 2, 2020

Support for the new Cache Policy concept in CloudFront, which replaces the
existing ForwardedValues properties and specifies what parts of a request
make up the cache key, and what TTLs to use.

Implementation notes:

  • To achieve the API I wanted for the managed policies, I opted to not have
    ICachePolicy extend IResource. I admit to not understanding all of the
    implications of this, so welcome feedback.
  • I also opted to exclude the cachePolicyLastUpdatedTimestamp attribute from
    both ICachePolicy and CachePolicy, as the use case for them isn't clear,
    and handling the property for managed/imported cache policies was awkward.
  • Lastly, I opted to remove completely (rather than deprecate) the
    forwardedValues property, forcing users to create (or use managed) Cache
    Policies to achieve the same behavior.
  • Related to the above, ForwardedValues is marked as deprecated and optional
    in the CloudFormation docs, but if no CachePolicyId or ForwardedValues are
    specified, the deployment will fail claiming ForwardedValues is missing.
    That's why I opted to default the CachePolicyId to "CachingOptimized".

fixes #9644

BREAKING CHANGE: Distribution forwardQueryString and forwardQueryStringCacheKeys have been removed in favor of cachePolicy and the new CachePolicy construct.

  • cloudfront: Distributions now default to the "CachingOptimized" managed cache policy

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

Support for the new Cache Policy concept in CloudFront, which replaces the
existing `ForwardedValues` properties and specifies what parts of a request
make up the cache key, and what TTLs to use.

_Implementation notes:_
* To achieve the API I wanted for the managed policies, I opted to *not* have
  `ICachePolicy` extend `IResource`. I admit to not understanding all of the
  implications of this, so welcome feedback.
* I also opted to exclude the `cachePolicyLastUpdatedTimestamp` attribute from
  both `ICachePolicy` and `CachePolicy`, as the use case for them isn't clear,
  and handling the property for managed/imported cache policies was awkward.
* Lastly, I opted to remove completely (rather than deprecate) the
  `forwardedValues` property, forcing users to create (or use managed) Cache
  Policies to achieve the same behavior.
* Related to the above, `ForwardedValues` is marked as deprecated and optional
  in the CloudFormation docs, but if no `CachePolicyId` or `ForwardedValues` are
  specified, the deployment will fail claiming `ForwardedValues` is missing.
  That's why I opted to default the `CachePolicyId` to "CachingOptimized".

*--NOTE: This functionality is currently broken:--*
The CloudFormation support for creating new Cache Policies was released, but
subsequent issues have been found and it appears users have been unable to
create Cache Policies. See aws-cloudformation/cloudformation-coverage-roadmap#571
Marking this as draft until the above issue is resolved.

fixes #9644

BREAKING CHANGES: Distribution `forwardedValues` have been removed in favor of `cachePolicy` and the new CachePolicy construct.
* **cloudfront:** Distributions now default to the "CachingOptimized" managed cache policy
@njlynch njlynch requested a review from a team October 2, 2020 09:55
@njlynch njlynch self-assigned this Oct 2, 2020
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Oct 2, 2020
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Some small comments/suggestion.

Also, I think the "Breaking change" is wrong - the names of the properties that were removed are forwardQueryString and forwardQueryStringCacheKeys.

packages/@aws-cdk/aws-cloudfront/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cloudfront/lib/cache-policy.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cloudfront/lib/cache-policy.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cloudfront/lib/cache-policy.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cloudfront/lib/cache-policy.ts Outdated Show resolved Hide resolved
@njlynch njlynch marked this pull request as ready for review October 6, 2020 17:40
@njlynch njlynch requested review from a team and skinny85 October 6, 2020 17:40
@skinny85 skinny85 added the pr/do-not-merge This PR should not be merged at this time. label Oct 7, 2020
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Overall looks great! Couple of minor comments.

@njlynch njlynch removed the pr/do-not-merge This PR should not be merged at this time. label Oct 7, 2020
Copy link
Contributor

@shivlaks shivlaks left a comment

Choose a reason for hiding this comment

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

looks great! some minor suggestions for your consideration 🙂

packages/@aws-cdk/aws-cloudfront/lib/cache-policy.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cloudfront/lib/cache-policy.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cloudfront/lib/cache-policy.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cloudfront/lib/distribution.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-cloudfront/test/cache-policy.test.ts Outdated Show resolved Hide resolved
njlynch added a commit that referenced this pull request Oct 7, 2020
Support for the new Origin Request Policy concept in CloudFront, which provides
greater control to users over what values are forwarded to the origin from the
original viewer request.

_Implementation Notes:_
* *Heavily* influenced by #10656, including the same notes on not extending from
  `IResource` and excluding the `lastModifiedTimestamp`.
* Currently marked as a DRAFT, since this PR *requires* #10656;
  origin request policies cannot be used without cache policies.

fixes #9647
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

LGTM!

packages/@aws-cdk/aws-cloudfront/lib/cache-policy.ts Outdated Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Oct 7, 2020

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

njlynch added a commit that referenced this pull request Oct 7, 2020
Support for the new Origin Request Policy concept in CloudFront, which provides
greater control to users over what values are forwarded to the origin from the
original viewer request.

_Implementation Notes:_
* *Heavily* influenced by #10656, including the same notes on not extending from
  `IResource` and excluding the `lastModifiedTimestamp`.

fixes #9647
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify
Copy link
Contributor

mergify bot commented Oct 7, 2020

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 5a97d27 into master Oct 7, 2020
@mergify mergify bot deleted the njlynch/cf-cache-policy branch October 7, 2020 19:11
njlynch added a commit that referenced this pull request Oct 7, 2020
Support for the new Origin Request Policy concept in CloudFront, which provides
greater control to users over what values are forwarded to the origin from the
original viewer request.

_Implementation Notes:_
* *Heavily* influenced by #10656, including the same notes on not extending from
  `IResource` and excluding the `lastModifiedTimestamp`.

fixes #9647
mergify bot pushed a commit that referenced this pull request Oct 7, 2020
Support for the new Origin Request Policy concept in CloudFront, which provides
greater control to users over what values are forwarded to the origin from the
original viewer request.

_Implementation Notes:_
* *Heavily* influenced by #10656, including the same notes on not extending from
  `IResource` and excluding the `lastModifiedTimestamp`.
* Currently marked as a DRAFT, since this PR *requires* #10656;
  origin request policies cannot be used without cache policies.

fixes #9647

----

*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
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[cloudfront] Cache policies
5 participants