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

Add Region in config.json file (#2570) #2571

Closed
wants to merge 1 commit into from
Closed

Add Region in config.json file (#2570) #2571

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 15, 2018

If not set in config.json, default is still us-east-1.

Default is still `us-east-1`
@codecov
Copy link

codecov bot commented Oct 15, 2018

Codecov Report

Merging #2571 into master will decrease coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2571      +/-   ##
==========================================
- Coverage   10.49%   10.49%   -0.01%     
==========================================
  Files         107      107              
  Lines       10828    10829       +1     
==========================================
  Hits         1136     1136              
- Misses       9542     9543       +1     
  Partials      150      150
Impacted Files Coverage Δ
cmd/mb-main.go 0% <ø> (ø) ⬆️
cmd/config-v9.go 0% <ø> (ø) ⬆️
cmd/utils.go 29.65% <0%> (-0.18%) ⬇️
cmd/client-s3.go 13.38% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7bbc81f...0f739b3. Read the comment docs.

@vadmeste
Copy link
Member

Hey @quentinselle, thanks for this PR.

The purpose of this PR is not clear for me yet, mc is supposed to calculate a bucket region automatically when applicable, are you targeting any specific use case ?

@ghost
Copy link
Author

ghost commented Oct 22, 2018

This PR is related to this issue #2570. If you have any questions, please feel free to ask.

@vadmeste
Copy link
Member

This PR is related to this issue #2570. If you have any questions, please feel free to ask.

Listing buckets does not depend on region as @kannappanr mentioned, the region depends on the location of a bucket.

You can enable --debug flag in aws to see that it uses 'us-east-1' by default when region doesn't make sense in the current operation, (see StringToSign)

$ aws --debug s3 ls s3://
....
2018-10-22 17:07:39,629 - MainThread - botocore.auth - DEBUG - StringToSign:
AWS4-HMAC-SHA256                 
20181022T160739Z                
20181022/us-east-1/s3/aws4_request
....

@ghost
Copy link
Author

ghost commented Oct 22, 2018

Indeed, I discussed with @kannappanr on issue #2570 and we concluded that this Pull Request is useless. We found a better way to fix my problem (-> raise AuthorizationMalformed).

That's why I linked you the issue in the hope you'll read it and give a update to it before reply here (no reply on the issue since last week).

I now return a AuthorizationMalformed http error with the valid bucket region (<Region>$REGION</Region>) and I'm waiting for your reply (minio).

How do you suggest to implement the retry in minio-go ?
Could you please answer in the issue and close this Pull Request.

Thank you.

@poornas
Copy link
Contributor

poornas commented Oct 22, 2018

@quentinselle, minio-go already retries with region when response code is InvalidRegion. response message for AuthorizationHeaderMalformed needs to be parsed to get the correct region and set as errResp.region in https://github.com/minio/minio-go/blob/master/api-error-response.go#L179 and
https://github.com/minio/minio-go/blob/master/api.go#L630 to use the errResp.Region in case of AuthorizationHeaderMalformed

@kannappanr
Copy link
Collaborator

Closing this PR as suggested by @quentinselle

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.

3 participants