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 AWS S3 options #1541

Merged
merged 1 commit into from Jan 21, 2020
Merged

Fix AWS S3 options #1541

merged 1 commit into from Jan 21, 2020

Conversation

guicassolato
Copy link
Contributor

@guicassolato guicassolato commented Jan 17, 2020

Related to THREESCALE-4052


This PR ensures Paperclip will consider the new configuration options introduced by #1522 while uploading attachments to S3, passing them along to the Aws::S3::Resource instance.

  • Sets s3_options with the endpoint value (matching s3_protocol and s3_host_name) if configured by the user
  • Sets force_path_style: true by default

AWS S3
Screenshot 2020-01-17 at 19 32 35

Minio.io
Screenshot 2020-01-17 at 19 32 10

@@ -8,6 +8,7 @@ s3: &s3
region: "<%= ENV['AWS_REGION'] %>"
hostname: "<%= ENV['AWS_HOSTNAME'] %>"
protocol: "<%= ENV['AWS_PROTOCOL'] %>"
force_path_style: <%= ENV['AWS_PATH_STYLE'].presence || false %>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eguzki , I'm afraid we'll need to introduce one more env var, AWS_PATH_STYLE.

We cannot enforce it by default because path-style endpoints will lose support on 30 Sep 2020. Other "API-compatible" services like minio.io, however, may still require it (at least, http://play.minio.io does).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we decide not to implement this new env var, a workaround is including a dot in the name of the bucket. This will enforce path-style endpoints despite the value of the force_path_style configuration option. (Only valid when the protocol is https.)

https://github.com/aws/aws-sdk-ruby/blob/9409865777f15c7db2a8b6937fdd571082d12cce/aws-sdk-resources/lib/aws-sdk-resources/services/s3/bucket.rb#L111

Copy link
Member

Choose a reason for hiding this comment

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

AWS_PATH_STYLE will be there

Copy link
Member

Choose a reason for hiding this comment

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

PR with the new env var: 3scale/3scale-operator#306

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure to understand correctly this force_path_style configuration.

Why do we need it? I can't see any discussion in the issue or its comments about this
I just want to understand the need of this, or lets document what is missing in the issue (for future archeologist)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because https://github.com/aws/aws-sdk-ruby tests if the name of the bucket is "dns-compatible". Whenever it sees a bucket named, for example, "mybucket", it will build an endpoint using the subdomain style: i.e. "https://mybucket.host". On the other hand, if the bucket is named "my.bucket" (i.e. not "dns-compatible"), the endpoint will be built using path style: i.e. "https://host/my.bucket".

http://play.minio.io requires path style always, no matter if the bucket name is "dns compatible" or not. Not having the option to set force_path_style would make 3scale incompatible with use cases where the name of the bucket is "dns-compatible" but the endpoint must still be in path-style.

@guicassolato guicassolato requested a review from a team January 20, 2020 08:42
@guicassolato guicassolato merged commit fcf392b into master Jan 21, 2020
@guicassolato guicassolato deleted the fix-s3-api-compatibles branch January 21, 2020 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants