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.

Sign up for GitHub

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

Jump to bottom

fix(cloudfront): Use regional endpoint for S3 bucket origins #2554

Merged
merged 2 commits into from May 21, 2019
Merged

fix(cloudfront): Use regional endpoint for S3 bucket origins #2554

merged 2 commits into from May 21, 2019

Conversation

RomainMuller
Copy link
Contributor

@RomainMuller RomainMuller commented May 15, 2019 •
edited

This contains 32 commits (if controversial, I can split into three PRs easily - but the two first ones are small enough):
1. fix(core): Module detection did not work in Node 12+
2. fix(s3): Make IBucket.arnForObject accept only (exactly) one key pattern
3. fix(cloudfront): Use regional endpoint for S3 bucket origins

This PR should be rebased, not squashed.

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.

@RomainMuller RomainMuller requested a review from a team as a code owner May 15, 2019 22:06
mindstorms6
mindstorms6 approved these changes May 16, 2019
View reviewed changes
Copy link
Contributor

@mindstorms6 mindstorms6 left a comment

Choose a reason for hiding this comment

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

Nice!
The RegionalBucketName is exactly what we want there 👍

@RomainMuller
fix(s3): Make IBucket.arnForObject accept only (exactly) one key pattern
e6b9aef 
The variadic signature allowed the method to be invoked with zero
arguments, resulting in an unusable ARN. Instead of accepting an array
of path elements that will be concatenated, accept only one pattern and
use it as-is.

BREAKING CHANGE: The `IBucket.arnForObject` method no longer
concatenates path fragments on your behalf. Pass the `/`-concatenated
key pattern instead.
@RomainMuller
fix(cloudfront): Use regional endpoint for S3 bucket origins
2b5f07e 
The regional endpoint has to be used for S3 bucket origins, otherwise
CloudFront will receive an HTTP 302 response (redirecting to the
regional endpoint), which it will cache. This will lead to users seeing
the actual bucket endpoint, instead of it being hidden behind the
CloudFront distribution.
@RomainMuller RomainMuller force-pushed the rmuller/fix-cloudfront-s3-origin-domainname branch from 1a2e018 to 2b5f07e Compare May 21, 2019 08:52
@RomainMuller RomainMuller merged commit 64c3c6b into master May 21, 2019
@RomainMuller RomainMuller deleted the rmuller/fix-cloudfront-s3-origin-domainname branch May 21, 2019 10:58
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
njlynch added a commit that referenced this pull request Sep 28, 2020
@njlynch
fix(cloudfront): logging bucket uses regional domain name
688d4d8 
According to the CloudFront docs, the logging bucket should be specified as the
bucket domain name. #2554 updated origin buckets to use the regional bucket
domain names -- which is correct -- but also incorrectly updated the logging
bucket specifications as well.

This has a minor impact of being unable to navigate to the logging bucket from
the CloudFront console, but otherwise the logs are stored correctly.

fixes #10512
@njlynch njlynch mentioned this pull request Sep 28, 2020
Merged
mergify bot pushed a commit that referenced this pull request Sep 29, 2020
@njlynch
fix(cloudfront): logging bucket uses regional domain name (#10570)
19dd454 
According to the CloudFront docs, the logging bucket should be specified as the
bucket domain name. #2554 updated origin buckets to use the regional bucket
domain names -- which is correct -- but also incorrectly updated the logging
bucket specifications as well.

This has a minor impact of being unable to navigate to the logging bucket from
the CloudFront console, but otherwise the logs are stored correctly.

fixes #10512


----

*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
Reviewers

@mindstorms6 mindstorms6 mindstorms6 approved these changes

@rix0rrr rix0rrr rix0rrr approved these changes

Assignees
No one assigned
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@RomainMuller @mindstorms6 @rix0rrr @NGL321