Skip to content

Commit

Permalink
This is an automated cherry-pick of pingcap#41889
Browse files Browse the repository at this point in the history
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
  • Loading branch information
3pointer authored and ti-chi-bot committed Apr 12, 2023
1 parent b5701ee commit 02953c7
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 10 deletions.
36 changes: 26 additions & 10 deletions br/pkg/storage/s3.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,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
}

Expand Down Expand Up @@ -354,22 +355,37 @@ func NewS3Storage(backend *backuppb.S3, opts *ExternalStorageOptions) (obj *S3St
)
}
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)
}
<<<<<<< HEAD
}
region, err := s3manager.GetBucketRegionWithClient(context.Background(), 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
>>>>>>> 0a08f64082e (br: skip automatically get bucket region with other s3 compatible provider. (#41889))
}

if qs.Region != region {
Expand Down
75 changes: 75 additions & 0 deletions br/pkg/storage/s3_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ func TestApplyUpdate(t *testing.T) {
ForcePathStyle: true,
Bucket: "bucket",
Prefix: "prefix",
Provider: "ceph",
},
},
{
Expand All @@ -224,6 +225,7 @@ func TestApplyUpdate(t *testing.T) {
ForcePathStyle: false,
Bucket: "bucket",
Prefix: "prefix",
Provider: "alibaba",
},
},
{
Expand All @@ -238,6 +240,7 @@ func TestApplyUpdate(t *testing.T) {
ForcePathStyle: false,
Bucket: "bucket",
Prefix: "prefix",
Provider: "netease",
},
},
{
Expand Down Expand Up @@ -1193,3 +1196,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)
}
}

0 comments on commit 02953c7

Please sign in to comment.