Add disableHTTPS and usePathStyle s3v2.Options as query param#3491
Add disableHTTPS and usePathStyle s3v2.Options as query param#3491vangent merged 1 commit intogoogle:masterfrom
Conversation
|
/assign @vangent |
aws/aws.go
Outdated
| // V2s3OptionsFromURLParams. | ||
| // | ||
| // The following query options are supported: | ||
| // - usePathStyle: A value of "true" forces the request to use path-style addressing; sets s3v2.Options.UsePathStyle. |
There was a problem hiding this comment.
Just so I understand -- do these options make sense for all AWS services, or just for S3?
If they make sense for all AWS services, let's update the existing V2ConfigFromURLParams to return an awsv2.Config and a []func(*s3v2.Options), instead of creating a new one.
There was a problem hiding this comment.
s3v2.Options options are used by s3 only.
There was a problem hiding this comment.
So I think we need to have a separate function.
There was a problem hiding this comment.
If it's only used by S3, then let's move this code into blob/s3blob. I.e., see how the "accelerate" option is parsed and used.
There was a problem hiding this comment.
Done that. Please review again.
aws/aws.go
Outdated
| // | ||
| // The following query options are supported: | ||
| // - usePathStyle: A value of "true" forces the request to use path-style addressing; sets s3v2.Options.UsePathStyle. | ||
| // - disableHTTPS: A value of "true" disables SSL when sending requests; sets s3v2.Options.EndpointOptions.DisableHTTPS. |
There was a problem hiding this comment.
Please follow the existing style for URL parameters, e.g., "use_path_style" and "disable_https".
Let's add support for "no_accelerate" too? (leave the default as true, but allow people to turn it off).
There was a problem hiding this comment.
Aren't we following camelcase everywhere else?
Reference:
disableSSL
https://github.com/google/go-cloud/pull/3491/files#diff-6fe59643f73b478394f267669c70f00520f5902ea6d880ea1806169a2e687296L102
s3ForcePathStyle
https://github.com/google/go-cloud/pull/3491/files#diff-6fe59643f73b478394f267669c70f00520f5902ea6d880ea1806169a2e687296L96
There was a problem hiding this comment.
Hmm, you're right. But most existing URL parameters are not camelcase, so let's follow that.
I can send a separate PR to fix those.
There was a problem hiding this comment.
Ignore my comment about "no_accelerate" -- that's already supported.
blob/s3blob/s3blob.go
Outdated
| @@ -199,10 +199,16 @@ func (o *URLOpener) OpenBucketURL(ctx context.Context, u *url.URL) (*blob.Bucket | |||
| if err != nil { | |||
There was a problem hiding this comment.
Do the tests pass? I would expect V2ConfigFromURLParams to fail if you passed it a URL with these new parameters in it.
There was a problem hiding this comment.
I have added the following tests to catch this issue:
https://github.com/google/go-cloud/pull/3491/files#diff-0520a4d136ddd4348403e11bf2af92e43dd4dae87d9d970370c795cb966614f6R311
Similarly, in other test:
https://github.com/google/go-cloud/pull/3491/files#diff-0520a4d136ddd4348403e11bf2af92e43dd4dae87d9d970370c795cb966614f6R222
Now, if someone add a new query param in any of the place, this test will be able to catch this issue.
khrm
force-pushed
the
s3Options
branch
5 times, most recently
from
ba017d3 to
32a9faa
Compare
|
@vangent Can you review this again? |
This ensures that we can use blob/blob with s3 compatible storage like minio. Pass use_path_style=true to force PathStyle for s3. Pass disable_https=true to disable tls for endpoint.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3491 +/- ##
=======================================
Coverage 73.23% 73.24%
=======================================
Files 113 113
Lines 15018 15037 +19
=======================================
+ Hits 10999 11014 +15
- Misses 3257 3259 +2
- Partials 762 764 +2 ☔ View full report in Codecov by Sentry. |
2 participants
This ensures that we can use blob/blob with s3 compatible storage like minio.
Pass
usePathStyle=trueto force PathStyle for s3. PassdisableHTTPS=trueto disable tls for endpoint.Please use a title starting with the name of the affected package, or "all",
followed by a colon, followed by a short summary of the issue. Example:
blob/gcsblob: fix typo in documentation.Fixes: #3490