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.

Sign up for GitHub

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

Jump to bottom

storage/s3: fix precedence of region detection #331

Merged
merged 11 commits into from
Sep 30, 2021
Merged

storage/s3: fix precedence of region detection #331

merged 11 commits into from
Sep 30, 2021

Conversation

seruman
Copy link
Member

@seruman seruman commented Jul 29, 2021 •
edited
Loading

Previously auto region detection would always override the region defined in SDK config which populated via AWS_REGION/AWS_DEFAULT_REGION or region in given profile with AWS_PROFILE.

s5cmd/storage/s3.go

Lines 822 to 832 in 02f63b3

if aws.StringValue(sess.Config.Region) == "" {
sess.Config.Region = aws.String(endpoints.UsEast1RegionID)
}
if bucket == "" {
return nil
}
region, err := s3manager.GetBucketRegion(ctx, sess, bucket, "", func(r *request.Request) {
r.Config.Credentials = sess.Config.Credentials
})

This was causing issues mainly on users of S3 compatible services returning empty response on HeadBucket request that's converted into us-east-1 in s3manager.GetBucketRegion()(1, 2) ; e.g #325, #354.

This PR makes changes to predecense of region detection as below;

  1. --source-region or --destination-region flags of cp command.
  2. AWS_REGION environment variable.
  3. Region section of AWS profile.
  4. Auto detection from bucket region (via HeadBucket).
  5. us-east-1 as default region.

Resolves #325, resolves #320.

Closes #317.

@seruman seruman marked this pull request as ready for review July 29, 2021 13:38
@seruman seruman requested a review from a team as a code owner July 29, 2021 13:38
@seruman seruman requested review from igungor, aykutfarsak and sonmezonur and removed request for a team and aykutfarsak July 29, 2021 13:38
@ilkinulas ilkinulas self-requested a review August 2, 2021 11:37
@sonmezonur sonmezonur mentioned this pull request Aug 24, 2021
Closed
@Niek Niek mentioned this pull request Sep 15, 2021
Closed
@Niek
Copy link

Niek commented Sep 15, 2021

@igungor @sonmezonur @ilkinulas Can this PR be reviewed?

@igungor igungor added this to the v2.0.0 milestone Sep 20, 2021
This was referenced Sep 20, 2021
Closed
Closed
@seruman
Copy link
Member Author

seruman commented Sep 20, 2021

Hi @Niek, as this change breaks the usage of auto-region detection we plan to release it on v2.0.0 in the following months.

@seruman seruman mentioned this pull request Sep 20, 2021
Closed
@igungor
Copy link
Member

igungor commented Sep 23, 2021

@seruman could you rebase and update the changelog?

@igungor igungor removed the request for review from sonmezonur September 24, 2021 08:47
@igungor
Copy link
Member

igungor commented Sep 24, 2021

I've removed Onur from the reviewers list. He is on "vacation" for a month 😄 . 🍸

@igungor igungor requested review from aykutfarsak and removed request for ilkinulas September 30, 2021 07:11
@igungor igungor merged commit 6fac3f1 into master Sep 30, 2021
@igungor igungor deleted the fix-region-detection branch September 30, 2021 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aykutfarsak aykutfarsak aykutfarsak approved these changes

@igungor igungor igungor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.

Error during uploads to Openstack SWIFT S3 Redesign region detection
4 participants
@seruman @Niek @igungor @aykutfarsak