From 0a08f64082e59493306b87ad94fa2902b054f4ec Mon Sep 17 00:00:00 2001
From: 3pointer <luancheng@pingcap.com>
Date: Thu, 30 Mar 2023 14:42:55 +0800
Subject: [PATCH] br: skip automatically get bucket region with other s3
 compatible provider. (#41889)

close pingcap/tidb#41916, close pingcap/tidb#42033
---
 br/pkg/storage/s3.go      | 37 +++++++++++--------
 br/pkg/storage/s3_test.go | 75 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+), 14 deletions(-)

diff --git a/br/pkg/storage/s3.go b/br/pkg/storage/s3.go
index 7abbd9d0e814b..97b9b09a69272 100644
--- a/br/pkg/storage/s3.go
+++ b/br/pkg/storage/s3.go
@@ -189,6 +189,7 @@ func (options *S3BackendOptions) Apply(s3 *backuppb.S3) error {
 	s3.ForcePathStyle = options.ForcePathStyle
 	s3.RoleArn = options.RoleARN
 	s3.ExternalId = options.ExternalID
+	s3.Provider = options.Provider
 	return nil
 }
 
@@ -358,22 +359,30 @@ func NewS3Storage(ctx context.Context, backend *backuppb.S3, opts *ExternalStora
 		)
 	}
 	c := s3.New(ses, s3CliConfigs...)
-	confCred := ses.Config.Credentials
-	setCredOpt := func(req *request.Request) {
-		// s3manager.GetBucketRegionWithClient will set credential anonymous, which works with s3.
-		// we need reassign credential to be compatible with minio authentication.
-		if confCred != nil {
-			req.Config.Credentials = confCred
+
+	var region string
+	if len(qs.Provider) == 0 || qs.Provider == "aws" {
+		confCred := ses.Config.Credentials
+		setCredOpt := func(req *request.Request) {
+			// s3manager.GetBucketRegionWithClient will set credential anonymous, which works with s3.
+			// we need reassign credential to be compatible with minio authentication.
+			if confCred != nil {
+				req.Config.Credentials = confCred
+			}
+			// s3manager.GetBucketRegionWithClient use path style addressing default.
+			// we need set S3ForcePathStyle by our config if we set endpoint.
+			if qs.Endpoint != "" {
+				req.Config.S3ForcePathStyle = ses.Config.S3ForcePathStyle
+			}
 		}
-		// s3manager.GetBucketRegionWithClient use path style addressing default.
-		// we need set S3ForcePathStyle by our config if we set endpoint.
-		if qs.Endpoint != "" {
-			req.Config.S3ForcePathStyle = ses.Config.S3ForcePathStyle
+		region, err = s3manager.GetBucketRegionWithClient(ctx, c, qs.Bucket, setCredOpt)
+		if err != nil {
+			return nil, errors.Annotatef(err, "failed to get region of bucket %s", qs.Bucket)
 		}
-	}
-	region, err := s3manager.GetBucketRegionWithClient(ctx, c, qs.Bucket, setCredOpt)
-	if err != nil {
-		return nil, errors.Annotatef(err, "failed to get region of bucket %s", qs.Bucket)
+	} else {
+		// for other s3 compatible provider like ovh storage didn't return the region correctlly
+		// so we cannot automatically get the bucket region. just fallback to manually region setting.
+		region = qs.Region
 	}
 
 	if qs.Region != region {
diff --git a/br/pkg/storage/s3_test.go b/br/pkg/storage/s3_test.go
index 865c1e726e708..0541e66b8617f 100644
--- a/br/pkg/storage/s3_test.go
+++ b/br/pkg/storage/s3_test.go
@@ -211,6 +211,7 @@ func TestApplyUpdate(t *testing.T) {
 				ForcePathStyle: true,
 				Bucket:         "bucket",
 				Prefix:         "prefix",
+				Provider:       "ceph",
 			},
 		},
 		{
@@ -225,6 +226,7 @@ func TestApplyUpdate(t *testing.T) {
 				ForcePathStyle: false,
 				Bucket:         "bucket",
 				Prefix:         "prefix",
+				Provider:       "alibaba",
 			},
 		},
 		{
@@ -239,6 +241,7 @@ func TestApplyUpdate(t *testing.T) {
 				ForcePathStyle: false,
 				Bucket:         "bucket",
 				Prefix:         "prefix",
+				Provider:       "netease",
 			},
 		},
 		{
@@ -1217,3 +1220,75 @@ func TestObjectLock(t *testing.T) {
 	)
 	require.Equal(t, true, s.storage.IsObjectLockEnabled())
 }
+
+func TestS3StorageBucketRegion(t *testing.T) {
+	type testcase struct {
+		name         string
+		expectRegion string
+		s3           *backuppb.S3
+	}
+
+	require.NoError(t, os.Setenv("AWS_ACCESS_KEY_ID", "ab"))
+	require.NoError(t, os.Setenv("AWS_SECRET_ACCESS_KEY", "cd"))
+	require.NoError(t, os.Setenv("AWS_SESSION_TOKEN", "ef"))
+
+	cases := []testcase{
+		{
+			"empty region from aws",
+			"us-east-1",
+			&backuppb.S3{
+				Region:   "",
+				Bucket:   "bucket",
+				Prefix:   "prefix",
+				Provider: "aws",
+			},
+		},
+		{
+			"region from different provider",
+			"sdg",
+			&backuppb.S3{
+				Region:   "sdg",
+				Bucket:   "bucket",
+				Prefix:   "prefix",
+				Provider: "ovh",
+			},
+		},
+		{
+			"empty region from different provider",
+			"",
+			&backuppb.S3{
+				Region:   "",
+				Bucket:   "bucket",
+				Prefix:   "prefix",
+				Provider: "ovh",
+			},
+		},
+		{
+			"region from aws",
+			"us-west-2",
+			&backuppb.S3{
+				Region:   "us-west-2",
+				Bucket:   "bucket",
+				Prefix:   "prefix",
+				Provider: "aws",
+			},
+		},
+	}
+	for _, ca := range cases {
+		func(name string, region string, s3 *backuppb.S3) {
+			s := createGetBucketRegionServer(region, 200, true)
+			defer s.Close()
+			s3.ForcePathStyle = true
+			s3.Endpoint = s.URL
+
+			t.Log(name)
+			es, err := New(context.Background(),
+				&backuppb.StorageBackend{Backend: &backuppb.StorageBackend_S3{S3: s3}},
+				&ExternalStorageOptions{})
+			require.NoError(t, err)
+			ss, ok := es.(*S3Storage)
+			require.True(t, ok)
+			require.Equal(t, region, ss.GetOptions().Region)
+		}(ca.name, ca.expectRegion, ca.s3)
+	}
+}