-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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 support for response headers policy #17359
Conversation
@njlynch Post the cfnspec bump merge, this PR is now ready for review. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Aside from the two spots where it looks like enums could be used, I think this looks good, but I want to chew on it for a bit. Some of the modeling (where it's copying the L1) feels like there's maybe just a bit of room for either flattening or simplifying the structure of CloudFormation.
packages/@aws-cdk/aws-cloudfront/lib/response-headers-policy.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-cloudfront/lib/response-headers-policy.ts
Outdated
Show resolved
Hide resolved
* | ||
* @default - no content type options | ||
*/ | ||
readonly contentTypeOptions?: ResponseHeadersContentTypeOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this is an instance where we'd prefer to flatten this structure, or otherwise model it a bit differently.
This is an example usage when modeled as-is:
{
...,
contentTypeOptions: { override: false },
}
I'm not sure that's intuitively clear that means "set X-Content-Type-Options header to nosniff
unless the origin also includes the header". Although I guess since that's the only valid value for the header, maybe it does intuitively mean that. Let me chew on this for a (business) day or so and see if anything else springs to mind as an alternative model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that was my understanding as well. The override flag would signify if the behavior should override the header if it is already a part of the response from the origin. Therefore I kept it nested.
More often than not, the customers might use a managed policy for this. But in cases where they would want to customize the behavior, we should give them the full plethora of controls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After giving it some thought (sorry -- much more than a business day or so!) I think this is fine. There are a few alternatives to how it could be modeled, but many try to anticipate usage and simplify for the "normal" case, and I don't (yet) know what that is here. This is functional and reasonably straightforward, so let's roll with it!
Pull request has been modified.
@njlynch Did you get a chance to look into this? |
packages/@aws-cdk/aws-cloudfront/lib/response-headers-policy.ts
Outdated
Show resolved
Hide resolved
Pull request has been modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
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). |
Pull request has been modified.
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). |
Excellent work @ayush987goyal Thanks for shipping this feature. 😃 🎉 |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
feat(cloudfront): Add support for response headers policy closes aws#17290 Notes: ~1. Currently the CFNSpec is not up-to-date with the latest available cloudformation changes for `ResponseHeadersPolicyId` in `AWS::CloudFront::Distribution CacheBehavior`. Some aspects of the same are added to the PR but are left commented. Would update the PR once the spec is updated.~ Refs: 1. https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/adding-response-headers.html 2. https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-cloudfront-responseheaderspolicy.html 3. https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudfront-distribution-cachebehavior.html#cfn-cloudfront-distribution-cachebehavior-responseheaderspolicyid ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Hi, 156 responseHeadersPolicy: cfnResponseHeadersPolicy, node_modules/@aws-cdk/aws-cloudfront/lib/distribution.d.ts:72:14 |
@samarkumar1 You can't really mix and match cdk1 and cdk2. ckd2 is now a monolithic library going forward (it contains all cdk packages). Also, cdk2 has this new cloudfront feature available where as 1.134.0 does not (it will be present in 1.135.0). Hope this helps. |
Hi @robertd, |
@samarkumar1 You're absolutely right.... For some reason I thought this feature snuck into v2. Given that re:invent was last week I'm expecting that v2.1.0 and v1.135.0 will get released this week. 🤞 |
Hi Ben, Hmmm… It should be there. I wonder if something got stale. I’d
suggest deleting node_modules and running `npm i` again.
…On Tue, Jan 4, 2022 at 6:42 AM Ben Stickley ***@***.***> wrote:
@robertd <https://github.com/robertd>, I still don't see this feature in
v2.3.0. Am I missing something?
—
Reply to this email directly, view it on GitHub
<#17359 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAHWN54NRXR3HJIG64B5YTUUL2LZANCNFSM5HNVNSCA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
feat(cloudfront): Add support for response headers policy closes aws#17290 Notes: ~1. Currently the CFNSpec is not up-to-date with the latest available cloudformation changes for `ResponseHeadersPolicyId` in `AWS::CloudFront::Distribution CacheBehavior`. Some aspects of the same are added to the PR but are left commented. Would update the PR once the spec is updated.~ Refs: 1. https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/adding-response-headers.html 2. https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-cloudfront-responseheaderspolicy.html 3. https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudfront-distribution-cachebehavior.html#cfn-cloudfront-distribution-cachebehavior-responseheaderspolicyid ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
feat(cloudfront): Add support for response headers policy
closes #17290
Notes:
1. Currently the CFNSpec is not up-to-date with the latest available cloudformation changes forResponseHeadersPolicyId
inAWS::CloudFront::Distribution CacheBehavior
. Some aspects of the same are added to the PR but are left commented. Would update the PR once the spec is updated.Refs:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license