diff --git a/weed/remote_storage/s3/aliyun.go b/weed/remote_storage/s3/aliyun.go index 667ef72a5..6954c2669 100644 --- a/weed/remote_storage/s3/aliyun.go +++ b/weed/remote_storage/s3/aliyun.go @@ -25,8 +25,7 @@ func (s AliyunRemoteStorageMaker) HasBucket() bool { func (s AliyunRemoteStorageMaker) Make(conf *remote_pb.RemoteConf) (remote_storage.RemoteStorageClient, error) { client := &s3RemoteStorageClient{ - supportTagging: true, - conf: conf, + conf: conf, } accessKey := util.Nvl(conf.AliyunAccessKey, os.Getenv("ALICLOUD_ACCESS_KEY_ID")) secretKey := util.Nvl(conf.AliyunSecretKey, os.Getenv("ALICLOUD_ACCESS_KEY_SECRET")) diff --git a/weed/remote_storage/s3/backblaze.go b/weed/remote_storage/s3/backblaze.go index 487826838..d8cfbeb15 100644 --- a/weed/remote_storage/s3/backblaze.go +++ b/weed/remote_storage/s3/backblaze.go @@ -23,8 +23,7 @@ func (s BackBlazeRemoteStorageMaker) HasBucket() bool { func (s BackBlazeRemoteStorageMaker) Make(conf *remote_pb.RemoteConf) (remote_storage.RemoteStorageClient, error) { client := &s3RemoteStorageClient{ - supportTagging: false, - conf: conf, + conf: conf, } config := &aws.Config{ Endpoint: aws.String(conf.BackblazeEndpoint), diff --git a/weed/remote_storage/s3/baidu.go b/weed/remote_storage/s3/baidu.go index 5c175e74b..285e095c3 100644 --- a/weed/remote_storage/s3/baidu.go +++ b/weed/remote_storage/s3/baidu.go @@ -26,8 +26,7 @@ func (s BaiduRemoteStorageMaker) HasBucket() bool { func (s BaiduRemoteStorageMaker) Make(conf *remote_pb.RemoteConf) (remote_storage.RemoteStorageClient, error) { client := &s3RemoteStorageClient{ - supportTagging: true, - conf: conf, + conf: conf, } accessKey := util.Nvl(conf.BaiduAccessKey, os.Getenv("BDCLOUD_ACCESS_KEY")) secretKey := util.Nvl(conf.BaiduSecretKey, os.Getenv("BDCLOUD_SECRET_KEY")) diff --git a/weed/remote_storage/s3/contabo.go b/weed/remote_storage/s3/contabo.go index 8147350e9..d20a8ec0a 100644 --- a/weed/remote_storage/s3/contabo.go +++ b/weed/remote_storage/s3/contabo.go @@ -25,8 +25,7 @@ func (s ContaboRemoteStorageMaker) HasBucket() bool { func (s ContaboRemoteStorageMaker) Make(conf *remote_pb.RemoteConf) (remote_storage.RemoteStorageClient, error) { client := &s3RemoteStorageClient{ - supportTagging: true, - conf: conf, + conf: conf, } accessKey := util.Nvl(conf.ContaboAccessKey, os.Getenv("ACCESS_KEY")) secretKey := util.Nvl(conf.ContaboSecretKey, os.Getenv("SECRET_KEY")) diff --git a/weed/remote_storage/s3/filebase.go b/weed/remote_storage/s3/filebase.go index 6640e2a14..89b96e385 100644 --- a/weed/remote_storage/s3/filebase.go +++ b/weed/remote_storage/s3/filebase.go @@ -26,8 +26,7 @@ func (s FilebaseRemoteStorageMaker) HasBucket() bool { func (s FilebaseRemoteStorageMaker) Make(conf *remote_pb.RemoteConf) (remote_storage.RemoteStorageClient, error) { client := &s3RemoteStorageClient{ - supportTagging: true, - conf: conf, + conf: conf, } accessKey := util.Nvl(conf.FilebaseAccessKey, os.Getenv("AWS_ACCESS_KEY_ID")) secretKey := util.Nvl(conf.FilebaseSecretKey, os.Getenv("AWS_SECRET_ACCESS_KEY")) diff --git a/weed/remote_storage/s3/s3_storage_client.go b/weed/remote_storage/s3/s3_storage_client.go index 1132b36d3..1ca606633 100644 --- a/weed/remote_storage/s3/s3_storage_client.go +++ b/weed/remote_storage/s3/s3_storage_client.go @@ -47,8 +47,7 @@ func (s s3RemoteStorageMaker) Make(conf *remote_pb.RemoteConf) (remote_storage.R // a guarded DialContext. func MakeWithHTTPClient(conf *remote_pb.RemoteConf, httpClient *http.Client) (remote_storage.RemoteStorageClient, error) { client := &s3RemoteStorageClient{ - supportTagging: true, - conf: conf, + conf: conf, } config := &aws.Config{ Region: aws.String(conf.S3Region), @@ -82,12 +81,11 @@ func MakeWithHTTPClient(conf *remote_pb.RemoteConf, httpClient *http.Client) (re } type s3RemoteStorageClient struct { - conf *remote_pb.RemoteConf - conn s3iface.S3API - supportTagging bool + conf *remote_pb.RemoteConf + conn s3iface.S3API } -var _ = remote_storage.RemoteStorageClient(&s3RemoteStorageClient{supportTagging: true}) +var _ = remote_storage.RemoteStorageClient(&s3RemoteStorageClient{}) func (s *s3RemoteStorageClient) Traverse(remote *remote_pb.RemoteStorageLocation, visitFn remote_storage.VisitFunc) (err error) { @@ -470,12 +468,18 @@ func (s *s3RemoteStorageClient) UpdateFileMetadata(loc *remote_pb.RemoteStorageL } } + // same as the write path: a remote without tagging support rejects both + // PutObjectTagging and DeleteObjectTagging + if !s.conf.S3SupportTagging { + return + } + tagging := toTagging(newEntry.Extended) if len(tagging.TagSet) > 0 { _, err = s.conn.PutObjectTagging(&s3.PutObjectTaggingInput{ Bucket: aws.String(loc.Bucket), Key: aws.String(loc.Path[1:]), - Tagging: toTagging(newEntry.Extended), + Tagging: tagging, }) } else { _, err = s.conn.DeleteObjectTagging(&s3.DeleteObjectTaggingInput{ diff --git a/weed/remote_storage/s3/s3_storage_client_test.go b/weed/remote_storage/s3/s3_storage_client_test.go index 1ac666063..40bc56fb0 100644 --- a/weed/remote_storage/s3/s3_storage_client_test.go +++ b/weed/remote_storage/s3/s3_storage_client_test.go @@ -258,6 +258,73 @@ func TestS3WriteFilePassesMimeAsContentType(t *testing.T) { require.Equal(t, "text/html", rt.uploadContentType(), "Content-Type should match entry.Attributes.Mime") } +// recordingRoundTripper records every request and answers all of them with a 200. +type recordingRoundTripper struct { + requests []*http.Request +} + +func (c *recordingRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + c.requests = append(c.requests, req.Clone(req.Context())) + if req.Body != nil { + _, _ = io.Copy(io.Discard, req.Body) + _ = req.Body.Close() + } + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader("")), + Header: http.Header{}, + Request: req, + }, nil +} + +func newRecordingS3Client(t *testing.T, supportTagging bool) (*s3RemoteStorageClient, *recordingRoundTripper) { + t.Helper() + rt := &recordingRoundTripper{} + conf := &remote_pb.RemoteConf{ + Name: "test", + S3Region: "us-east-1", + S3Endpoint: "https://example.invalid", + S3ForcePathStyle: true, + S3AccessKey: "test-key", + S3SecretKey: "test-secret", + S3SupportTagging: supportTagging, + } + rs, err := MakeWithHTTPClient(conf, &http.Client{Transport: rt}) + require.NoError(t, err) + return rs.(*s3RemoteStorageClient), rt +} + +func TestS3UpdateFileMetadataSkipsTaggingWhenUnsupported(t *testing.T) { + client, rt := newRecordingS3Client(t, false) + loc := &remote_pb.RemoteStorageLocation{Name: "test", Bucket: "bucket", Path: "/dir/file.bin"} + plain := &filer_pb.Entry{} + tagged := &filer_pb.Entry{Extended: map[string][]byte{"k1": []byte("v1")}} + + require.NoError(t, client.UpdateFileMetadata(loc, plain, tagged)) + require.Empty(t, rt.requests, "adding attributes must not send PutObjectTagging") + + require.NoError(t, client.UpdateFileMetadata(loc, tagged, plain)) + require.Empty(t, rt.requests, "removing attributes must not send DeleteObjectTagging") +} + +func TestS3UpdateFileMetadataSendsTaggingWhenSupported(t *testing.T) { + client, rt := newRecordingS3Client(t, true) + loc := &remote_pb.RemoteStorageLocation{Name: "test", Bucket: "bucket", Path: "/dir/file.bin"} + plain := &filer_pb.Entry{} + tagged := &filer_pb.Entry{Extended: map[string][]byte{"k1": []byte("v1")}} + + require.NoError(t, client.UpdateFileMetadata(loc, plain, tagged)) + require.Len(t, rt.requests, 1) + require.Equal(t, http.MethodPut, rt.requests[0].Method) + require.Contains(t, rt.requests[0].URL.RawQuery, "tagging") + + rt.requests = nil + require.NoError(t, client.UpdateFileMetadata(loc, tagged, plain)) + require.Len(t, rt.requests, 1) + require.Equal(t, http.MethodDelete, rt.requests[0].Method) + require.Contains(t, rt.requests[0].URL.RawQuery, "tagging") +} + func TestS3WriteFileOmitsContentTypeWhenMimeMissing(t *testing.T) { client, rt := newCapturingS3Client(t) loc := &remote_pb.RemoteStorageLocation{ diff --git a/weed/remote_storage/s3/storj.go b/weed/remote_storage/s3/storj.go index c58ebf7c6..9ec403d61 100644 --- a/weed/remote_storage/s3/storj.go +++ b/weed/remote_storage/s3/storj.go @@ -25,8 +25,7 @@ func (s StorjRemoteStorageMaker) HasBucket() bool { func (s StorjRemoteStorageMaker) Make(conf *remote_pb.RemoteConf) (remote_storage.RemoteStorageClient, error) { client := &s3RemoteStorageClient{ - supportTagging: true, - conf: conf, + conf: conf, } accessKey := util.Nvl(conf.StorjAccessKey, os.Getenv("AWS_ACCESS_KEY_ID")) secretKey := util.Nvl(conf.StorjSecretKey, os.Getenv("AWS_SECRET_ACCESS_KEY")) diff --git a/weed/remote_storage/s3/tencent.go b/weed/remote_storage/s3/tencent.go index c0da1fa05..184a5b372 100644 --- a/weed/remote_storage/s3/tencent.go +++ b/weed/remote_storage/s3/tencent.go @@ -25,8 +25,7 @@ func (s TencentRemoteStorageMaker) HasBucket() bool { func (s TencentRemoteStorageMaker) Make(conf *remote_pb.RemoteConf) (remote_storage.RemoteStorageClient, error) { client := &s3RemoteStorageClient{ - supportTagging: true, - conf: conf, + conf: conf, } accessKey := util.Nvl(conf.TencentSecretId, os.Getenv("COS_SECRETID")) secretKey := util.Nvl(conf.TencentSecretKey, os.Getenv("COS_SECRETKEY")) diff --git a/weed/remote_storage/s3/wasabi.go b/weed/remote_storage/s3/wasabi.go index 08e5c951b..5b98485ca 100644 --- a/weed/remote_storage/s3/wasabi.go +++ b/weed/remote_storage/s3/wasabi.go @@ -25,8 +25,7 @@ func (s WasabiRemoteStorageMaker) HasBucket() bool { func (s WasabiRemoteStorageMaker) Make(conf *remote_pb.RemoteConf) (remote_storage.RemoteStorageClient, error) { client := &s3RemoteStorageClient{ - supportTagging: true, - conf: conf, + conf: conf, } accessKey := util.Nvl(conf.WasabiAccessKey) secretKey := util.Nvl(conf.WasabiSecretKey)