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

Fix S3 path-style URL for host with dots for buckets that are placed in other regions than us-east-1 #2439

Conversation

Bonias
Copy link
Contributor

@Bonias Bonias commented Nov 25, 2019

#2359 added support for bucket names with period, but this only works for us-east-1 region. For other regions s3.{aws-region}.amazonaws.com needs to be used (see https://docs.aws.amazon.com/AmazonS3/latest/dev/UsingBucket.html#access-bucket-intro).

This PR adds support for bucket names with period that are placed in regions other than us-east-1.


Also, I'm wondering about one other thing regarding virtual-hosted–style URL. https://docs.aws.amazon.com/AmazonS3/latest/dev/UsingBucket.html#access-bucket-intro says:

Buckets created in Regions launched after March 20, 2019 are not reachable via the https://bucket.s3.amazonaws.com naming scheme.

Should carrierwave be updated to use http://{bucket}.s3.{aws-region}.amazonaws.com? If so should it:

  • use http://{bucket}.s3.{aws-region}.amazonaws.com for all regions (this is what both aws-sdk-s3, carrierwave-aws do and kind of what fog-aws does; fog-aws uses http://{bucket}.s3-{aws-region}.amazonaws.com),
  • use http://{bucket}.s3.{aws-region}.amazonaws.com only for regions created after March 20, 2019? (in that case carrierwave would require to have a list of regions created before March 20, 2019)

?

..for buckets that are placed in other regions that us-east-1.

According to https://docs.aws.amazon.com/AmazonS3/latest/dev/UsingBucket.html#access-bucket-intro,
`s3.amazonaws.com` can be used only for `us-east-1` region. For all
other regions `s3.aws-region.amazonaws.com` needs to be used.
@mshibuya mshibuya merged commit 21a0061 into carrierwaveuploader:master Dec 24, 2019
@mshibuya
Copy link
Member

Awesome, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants