config: default S3v2 for google endpoints #2179
Conversation
cmd/config-host-add.go
Outdated
| @@ -128,6 +128,9 @@ func mainConfigHostAdd(ctx *cli.Context) error { | |||
| api := args.Get(4) | |||
| if api == "" { | |||
| api = "S3v4" | |||
| if url == "https://storage.googleapis.com" { | |||
There was a problem hiding this comment.
We have to do this little elaborate check to cater for these conditions.
- URL is
storage.googleapis.comand then no signature type was specified default tos3v2which this patch addresses. - URL is
storage.googleapis.comand then signature type was specified by it was specified wrong ass3v4where we need to error out indicating that user should uses3v2instead in the error message.
To check if the url is google we should just use the helper
if isGoogle(url) {
api = "S3v2"
}
Codecov Report
@@ Coverage Diff @@
## master #2179 +/- ##
=========================================
- Coverage 9.15% 9.15% -0.01%
=========================================
Files 93 93
Lines 6901 6906 +5
=========================================
Hits 632 632
- Misses 6135 6140 +5
Partials 134 134
Continue to review full report at Codecov.
|
cmd/client-s3.go
Outdated
| @@ -940,7 +940,11 @@ func isAmazonAccelerated(host string) bool { | |||
| } | |||
|
|
|||
| func isGoogle(host string) bool { | |||
| return s3utils.IsGoogleEndpoint(url.URL{Host: host}) | |||
| url, err := url.Parse(host) | |||
There was a problem hiding this comment.
@CarterMcClellan thanks for this PR.
isGoogle() is used to receive only a host address without scheme and path.
Now the problem is that, if we go with this change and then move to go 1.8, we will have the following (unexpected, I know) result:
isGoogle("storage.googleapis.com") = false
That's because with go 1.8, url.Parse("storage.googleapis.com") returns url.Host = "" and url.Path = "storage.googleapis.com"
So I really suggest moving url.Parse() code in config-host-add.go with the change that you already did.
cmd/config-host-add.go
Outdated
| @@ -126,6 +128,16 @@ func mainConfigHostAdd(ctx *cli.Context) error { | |||
| accessKey := args.Get(2) | |||
| secretKey := args.Get(3) | |||
| api := args.Get(4) | |||
|
|
|||
| urlHost, _ := urlpkg.Parse(url) | |||
There was a problem hiding this comment.
Could you rename urlHost to parsedURL ? that would be more meaningful.
cmd/config-host-add.go
Outdated
|
|
||
| if isGoogle(urlHost.Host) { | ||
| if api == "S3v4" { | ||
| fatalIf(errInvalidArgument().Trace(api), "Unsupported API signature for url. Please use `mc config host add "+alias+" "+url+" "+accessKey+" "+secretKey+" S3v2` instead") |
There was a problem hiding this comment.
A missing dot at the end of the error message.
|
One final comment is, can you reword your commit message like:
|
harshavardhana
changed the title
config: Force S3v2 for google endpoints