storage/s3: fix precedence of region detection (peak#331)

Co-authored-by: Aykut Farsak <aykutfarsak@gmail.com> 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 peak#325, resolves peak#320. Closes peak#317.
lizzzcai · Sep 30, 2021 · 6fac3f1 · 6fac3f1
1 parent 541b5e1
commit 6fac3f1
Show file tree

Hide file tree

Showing 4 changed files with 117 additions and 2 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,10 @@
 # Changelog
 
+## not released yet
+
+#### Bugfixes
+- Fixed a bug about precedence of region detection, which auto region detection would always override region defined in environment or profile. ([#325](https://github.com/peak/s5cmd/issues/325))
+
 ## v1.4.0 - 21 Sep 2021
 
 #### Features

diff --git a/README.md b/README.md
@@ -307,6 +307,17 @@ requests to AWS. Credentials can be provided in a variety of ways:
 The SDK detects and uses the built-in providers automatically, without requiring
 manual configurations.
 
+### Region detection
+
+While executing the commands, `s5cmd` detects the region according to the following order of priority:
+
+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.
+
+
 ### Shell auto-completion
 
 Shell completion is supported for bash, zsh and fish.

diff --git a/storage/s3.go b/storage/s3.go
@@ -819,14 +819,20 @@ func (sc *SessionCache) clear() {
 }
 
 func setSessionRegion(ctx context.Context, sess *session.Session, bucket string) error {
-	if aws.StringValue(sess.Config.Region) == "" {
-		sess.Config.Region = aws.String(endpoints.UsEast1RegionID)
+	region := aws.StringValue(sess.Config.Region)
+
+	if region != "" {
+		return nil
 	}
 
+	// set default region
+	sess.Config.Region = aws.String(endpoints.UsEast1RegionID)
+
 	if bucket == "" {
 		return nil
 	}
 
+	// auto-detection
 	region, err := s3manager.GetBucketRegion(ctx, sess, bucket, "", func(r *request.Request) {
 		r.Config.Credentials = sess.Config.Credentials
 	})
@@ -842,6 +848,7 @@ func setSessionRegion(ctx context.Context, sess *session.Session, bucket string)
 	} else {
 		sess.Config.Region = aws.String(region)
 	}
+
 	return nil
 }
 

diff --git a/storage/s3_test.go b/storage/s3_test.go
@@ -8,6 +8,7 @@ import (
 	"io/ioutil"
 	"math/rand"
 	"net/http"
+	"net/http/httptest"
 	urlpkg "net/url"
 	"os"
 	"reflect"
@@ -783,6 +784,97 @@ func TestSessionCreateAndCachingWithDifferentBuckets(t *testing.T) {
 	}
 }
 
+func TestSessionRegionDetection(t *testing.T) {
+	bucketRegion := "sa-east-1"
+
+	testcases := []struct {
+		name           string
+		bucket         string
+		optsRegion     string
+		envRegion      string
+		expectedRegion string
+	}{
+		{
+			name:           "RegionWithSourceRegionParameter",
+			bucket:         "bucket",
+			optsRegion:     "ap-east-1",
+			envRegion:      "ca-central-1",
+			expectedRegion: "ap-east-1",
+		},
+		{
+			name:           "RegionWithEnvironmentVariable",
+			bucket:         "bucket",
+			optsRegion:     "",
+			envRegion:      "ca-central-1",
+			expectedRegion: "ca-central-1",
+		},
+		{
+			name:           "RegionWithBucketRegion",
+			bucket:         "bucket",
+			optsRegion:     "",
+			envRegion:      "",
+			expectedRegion: bucketRegion,
+		},
+		{
+			name:           "DefaultRegion",
+			bucket:         "",
+			optsRegion:     "",
+			envRegion:      "",
+			expectedRegion: "us-east-1",
+		},
+	}
+
+	// ignore local profile loading
+	os.Setenv("AWS_SDK_LOAD_CONFIG", "0")
+
+	// mock auto bucket detection
+	server := func() *httptest.Server {
+		return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Header().Set("X-Amz-Bucket-Region", bucketRegion)
+			w.WriteHeader(http.StatusOK)
+		}))
+	}()
+	defer server.Close()
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			opts := Options{
+				Endpoint: server.URL,
+
+				// since profile loading disabled above, we need to provide
+				// credentials to the session. NoSignRequest could be used
+				// for anonymous credentials.
+				NoSignRequest: true,
+			}
+
+			if tc.optsRegion != "" {
+				opts.region = tc.optsRegion
+			}
+
+			if tc.envRegion != "" {
+				os.Setenv("AWS_REGION", tc.envRegion)
+				defer os.Unsetenv("AWS_REGION")
+			}
+
+			if tc.bucket != "" {
+				opts.bucket = tc.bucket
+			}
+
+			globalSessionCache.clear()
+
+			sess, err := globalSessionCache.newSession(context.Background(), opts)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			got := aws.StringValue(sess.Config.Region)
+			if got != tc.expectedRegion {
+				t.Fatalf("expected %v, got %v", tc.expectedRegion, got)
+			}
+		})
+	}
+}
+
 func TestSessionAutoRegionValidateCredentials(t *testing.T) {
 	awsSess := unit.Session
 	awsSess.Handlers.Unmarshal.Clear()