Fix S3 MoveFile/CopyFile failing on files larger than 5GiB (#37035) (#37057)

Automatic Merge

server/platform/shared/filestore/mocks/s3CopyClient.go

@@ -0,0 +1,122 @@
+// Code generated by mockery v2.53.4. DO NOT EDIT.
+
+// Regenerate this file using `make filestore-mocks`.
+
+package mocks
+
+import (
+	context "context"
+
+	minio "github.com/minio/minio-go/v7"
+	mock "github.com/stretchr/testify/mock"
+)
+
+// s3CopyClient is an autogenerated mock type for the s3CopyClient type
+type s3CopyClient struct {
+	mock.Mock
+}
+
+// ComposeObject provides a mock function with given fields: ctx, dst, srcs
+func (_m *s3CopyClient) ComposeObject(ctx context.Context, dst minio.CopyDestOptions, srcs ...minio.CopySrcOptions) (minio.UploadInfo, error) {
+	_va := make([]interface{}, len(srcs))
+	for _i := range srcs {
+		_va[_i] = srcs[_i]
+	}
+	var _ca []interface{}
+	_ca = append(_ca, ctx, dst)
+	_ca = append(_ca, _va...)
+	ret := _m.Called(_ca...)
+
+	if len(ret) == 0 {
+		panic("no return value specified for ComposeObject")
+	}
+
+	var r0 minio.UploadInfo
+	var r1 error
+	if rf, ok := ret.Get(0).(func(context.Context, minio.CopyDestOptions, ...minio.CopySrcOptions) (minio.UploadInfo, error)); ok {
+		return rf(ctx, dst, srcs...)
+	}
+	if rf, ok := ret.Get(0).(func(context.Context, minio.CopyDestOptions, ...minio.CopySrcOptions) minio.UploadInfo); ok {
+		r0 = rf(ctx, dst, srcs...)
+	} else {
+		r0 = ret.Get(0).(minio.UploadInfo)
+	}
+
+	if rf, ok := ret.Get(1).(func(context.Context, minio.CopyDestOptions, ...minio.CopySrcOptions) error); ok {
+		r1 = rf(ctx, dst, srcs...)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
+// CopyObject provides a mock function with given fields: ctx, dst, src
+func (_m *s3CopyClient) CopyObject(ctx context.Context, dst minio.CopyDestOptions, src minio.CopySrcOptions) (minio.UploadInfo, error) {
+	ret := _m.Called(ctx, dst, src)
+
+	if len(ret) == 0 {
+		panic("no return value specified for CopyObject")
+	}
+
+	var r0 minio.UploadInfo
+	var r1 error
+	if rf, ok := ret.Get(0).(func(context.Context, minio.CopyDestOptions, minio.CopySrcOptions) (minio.UploadInfo, error)); ok {
+		return rf(ctx, dst, src)
+	}
+	if rf, ok := ret.Get(0).(func(context.Context, minio.CopyDestOptions, minio.CopySrcOptions) minio.UploadInfo); ok {
+		r0 = rf(ctx, dst, src)
+	} else {
+		r0 = ret.Get(0).(minio.UploadInfo)
+	}
+
+	if rf, ok := ret.Get(1).(func(context.Context, minio.CopyDestOptions, minio.CopySrcOptions) error); ok {
+		r1 = rf(ctx, dst, src)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
+// StatObject provides a mock function with given fields: ctx, bucketName, objectName, opts
+func (_m *s3CopyClient) StatObject(ctx context.Context, bucketName string, objectName string, opts minio.GetObjectOptions) (minio.ObjectInfo, error) {
+	ret := _m.Called(ctx, bucketName, objectName, opts)
+
+	if len(ret) == 0 {
+		panic("no return value specified for StatObject")
+	}
+
+	var r0 minio.ObjectInfo
+	var r1 error
+	if rf, ok := ret.Get(0).(func(context.Context, string, string, minio.GetObjectOptions) (minio.ObjectInfo, error)); ok {
+		return rf(ctx, bucketName, objectName, opts)
+	}
+	if rf, ok := ret.Get(0).(func(context.Context, string, string, minio.GetObjectOptions) minio.ObjectInfo); ok {
+		r0 = rf(ctx, bucketName, objectName, opts)
+	} else {
+		r0 = ret.Get(0).(minio.ObjectInfo)
+	}
+
+	if rf, ok := ret.Get(1).(func(context.Context, string, string, minio.GetObjectOptions) error); ok {
+		r1 = rf(ctx, bucketName, objectName, opts)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
+// newS3CopyClient creates a new instance of s3CopyClient. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations.
+// The first argument is typically a *testing.T value.
+func newS3CopyClient(t interface {
+	mock.TestingT
+	Cleanup(func())
+}) *s3CopyClient {
+	mock := &s3CopyClient{}
+	mock.Mock.Test(t)
+
+	t.Cleanup(func() { mock.AssertExpectations(t) })
+
+	return mock
+}

server/platform/shared/filestore/s3store.go

@@ -366,6 +366,48 @@ func (b *S3FileBackend) FileModTime(path string) (time.Time, error) {
 	return info.LastModified, nil
 }
 
+// maxS3SingleCopySize is the largest source object (5GiB) that S3 allows for a
+// single server-side copy (CopyObject). Sources larger than this must be copied
+// with a server-side multipart copy (UploadPartCopy).
+const maxS3SingleCopySize = 5 * 1024 * 1024 * 1024
+
+// s3CopyClient is the subset of the S3 client used to route a server-side copy.
+// It exists so copyObjectWithClient can be unit tested with a mock client.
+type s3CopyClient interface {
+	StatObject(ctx context.Context, bucketName, objectName string, opts s3.StatObjectOptions) (s3.ObjectInfo, error)
+	CopyObject(ctx context.Context, dst s3.CopyDestOptions, src s3.CopySrcOptions) (s3.UploadInfo, error)
+	ComposeObject(ctx context.Context, dst s3.CopyDestOptions, srcs ...s3.CopySrcOptions) (s3.UploadInfo, error)
+}
+
+// copyObject performs a server-side copy of a single object from srcOpts to
+// dstOpts, using the backend's request timeout.
+func (b *S3FileBackend) copyObject(srcOpts s3.CopySrcOptions, dstOpts s3.CopyDestOptions) error {
+	ctx, cancel := context.WithTimeout(context.Background(), b.timeout)
+	defer cancel()
+
+	return copyObjectWithClient(ctx, b.client, srcOpts, dstOpts)
+}
+
+// copyObjectWithClient uses CopyObject for sources up to 5GiB, and falls back to
+// ComposeObject — which performs a server-side multipart copy — for larger
+// sources, which S3's single-operation CopyObject rejects. The size is checked
+// explicitly rather than relying on ComposeObject's own single-copy fast path,
+// which is only taken when the source range Start is -1 (a value its input
+// validation rejects, so it is unreachable here).
+func copyObjectWithClient(ctx context.Context, client s3CopyClient, srcOpts s3.CopySrcOptions, dstOpts s3.CopyDestOptions) error {
+	stat, err := client.StatObject(ctx, srcOpts.Bucket, srcOpts.Object, s3.StatObjectOptions{})
+	if err != nil {
+		return errors.Wrapf(err, "unable to stat source file %s", srcOpts.Object)
+	}
+
+	if stat.Size > maxS3SingleCopySize {
+		_, err = client.ComposeObject(ctx, dstOpts, srcOpts)
+	} else {
+		_, err = client.CopyObject(ctx, dstOpts, srcOpts)
+	}
+	return err
+}
+
 func (b *S3FileBackend) CopyFile(oldPath, newPath string) error {
 	oldPath, err := b.prefixedPath(oldPath)
 	if err != nil {
@@ -388,9 +430,7 @@ func (b *S3FileBackend) CopyFile(oldPath, newPath string) error {
 		dstOpts.Encryption = encrypt.NewSSE()
 	}
 
-	ctx, cancel := context.WithTimeout(context.Background(), b.timeout)
-	defer cancel()
-	if _, err := b.client.CopyObject(ctx, dstOpts, srcOpts); err != nil {
+	if err := b.copyObject(srcOpts, dstOpts); err != nil {
 		return errors.Wrapf(err, "unable to copy file from %s to %s", oldPath, newPath)
 	}
 
@@ -438,9 +478,7 @@ func (b *S3FileBackend) DecodeFilePathIfNeeded(path string) error {
 		dstOpts.Encryption = encrypt.NewSSE()
 	}
 
-	ctx, cancel := context.WithTimeout(context.Background(), b.timeout)
-	defer cancel()
-	if _, err := b.client.CopyObject(ctx, dstOpts, srcOpts); err != nil {
+	if err := b.copyObject(srcOpts, dstOpts); err != nil {
 		return errors.Wrapf(err, "unable to copy the file to %s to the new destination", newPath)
 	}
 
@@ -475,9 +513,7 @@ func (b *S3FileBackend) MoveFile(oldPath, newPath string) error {
 		dstOpts.Encryption = encrypt.NewSSE()
 	}
 
-	ctx, cancel := context.WithTimeout(context.Background(), b.timeout)
-	defer cancel()
-	if _, err := b.client.CopyObject(ctx, dstOpts, srcOpts); err != nil {
+	if err := b.copyObject(srcOpts, dstOpts); err != nil {
 		return errors.Wrapf(err, "unable to copy the file to %s to the new destination", newPath)
 	}
 

server/platform/shared/filestore/s3store_copyobject_test.go

@@ -0,0 +1,79 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package filestore
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	s3 "github.com/minio/minio-go/v7"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// ensure the real S3 client satisfies the interface used by copyObjectWithClient.
+var _ s3CopyClient = (*s3.Client)(nil)
+
+// mockS3CopyClient is a minimal s3CopyClient that records which copy operation
+// was invoked and lets StatObject's result be controlled per test.
+type mockS3CopyClient struct {
+	statInfo s3.ObjectInfo
+	statErr  error
+
+	copyCalled    bool
+	composeCalled bool
+}
+
+func (m *mockS3CopyClient) StatObject(_ context.Context, _, _ string, _ s3.StatObjectOptions) (s3.ObjectInfo, error) {
+	return m.statInfo, m.statErr
+}
+
+func (m *mockS3CopyClient) CopyObject(_ context.Context, _ s3.CopyDestOptions, _ s3.CopySrcOptions) (s3.UploadInfo, error) {
+	m.copyCalled = true
+	return s3.UploadInfo{}, nil
+}
+
+func (m *mockS3CopyClient) ComposeObject(_ context.Context, _ s3.CopyDestOptions, _ ...s3.CopySrcOptions) (s3.UploadInfo, error) {
+	m.composeCalled = true
+	return s3.UploadInfo{}, nil
+}
+
+var (
+	testCopySrc = s3.CopySrcOptions{Bucket: "bucket", Object: "src"}
+	testCopyDst = s3.CopyDestOptions{Bucket: "bucket", Object: "dst"}
+)
+
+func Test_copyObject_UsesCompose_ForLarge(t *testing.T) {
+	mock := &mockS3CopyClient{statInfo: s3.ObjectInfo{Size: maxS3SingleCopySize + 1}}
+
+	err := copyObjectWithClient(context.Background(), mock, testCopySrc, testCopyDst)
+
+	require.NoError(t, err)
+	assert.True(t, mock.composeCalled, "expected ComposeObject (multipart copy) for sources larger than 5GiB")
+	assert.False(t, mock.copyCalled, "expected CopyObject not to be used for sources larger than 5GiB")
+}
+
+func Test_copyObject_UsesCopy_ForSmall(t *testing.T) {
+	// A source exactly at the 5GiB limit must still use the single CopyObject.
+	mock := &mockS3CopyClient{statInfo: s3.ObjectInfo{Size: maxS3SingleCopySize}}
+
+	err := copyObjectWithClient(context.Background(), mock, testCopySrc, testCopyDst)
+
+	require.NoError(t, err)
+	assert.True(t, mock.copyCalled, "expected CopyObject for sources up to 5GiB")
+	assert.False(t, mock.composeCalled, "expected ComposeObject not to be used for sources up to 5GiB")
+}
+
+func Test_copyObject_PropagatesStatError(t *testing.T) {
+	statErr := errors.New("stat failed")
+	mock := &mockS3CopyClient{statErr: statErr}
+
+	err := copyObjectWithClient(context.Background(), mock, testCopySrc, testCopyDst)
+
+	require.Error(t, err)
+	assert.ErrorIs(t, err, statErr, "StatObject error must be propagated")
+	assert.False(t, mock.copyCalled, "no copy should be attempted when StatObject fails")
+	assert.False(t, mock.composeCalled, "no compose should be attempted when StatObject fails")
+}