diff --git a/go.tools.mod b/go.tools.mod index 188f6715aa..e54084086f 100644 --- a/go.tools.mod +++ b/go.tools.mod @@ -4,7 +4,7 @@ go 1.14 require ( github.com/jstemmer/go-junit-report v0.9.1 // indirect - github.com/mattermost/mattermost-utilities/mmgotool v0.0.0-20200915114419-f4421bc07461 // indirect + github.com/mattermost/mattermost-utilities/mmgotool v0.0.0-20201013204121-e463fcd00ba8 // indirect github.com/philhofer/fwd v1.0.0 // indirect github.com/reflog/struct2interface v0.6.1 // indirect github.com/tinylib/msgp v1.1.2 // indirect diff --git a/go.tools.sum b/go.tools.sum index c33467c6b5..2e5d316406 100644 --- a/go.tools.sum +++ b/go.tools.sum @@ -70,6 +70,10 @@ github.com/mattermost/mattermost-utilities/mmgotool v0.0.0-20200915114419-f4421b github.com/mattermost/mattermost-utilities/mmgotool v0.0.0-20200915114419-f4421bc07461/go.mod h1:3gKozJI8n2Y/vW37GfnFWAdehGXe5yZlt+HykK6Y3DM= github.com/mattermost/mattermost-utilities/mmgotool v0.0.0-20200926180007-fd1b679200e5 h1:1fVtMi+1XPAtxffRZiMSiy7Zm4Od3fyHWnC2BydfYHY= github.com/mattermost/mattermost-utilities/mmgotool v0.0.0-20200926180007-fd1b679200e5/go.mod h1:3gKozJI8n2Y/vW37GfnFWAdehGXe5yZlt+HykK6Y3DM= +github.com/mattermost/mattermost-utilities/mmgotool v0.0.0-20201001222518-66764584f346 h1:ByexaM3lb5ZdHGtcB1QwRkDXDxt/2MUTvI8+LjwhDXg= +github.com/mattermost/mattermost-utilities/mmgotool v0.0.0-20201001222518-66764584f346/go.mod h1:3gKozJI8n2Y/vW37GfnFWAdehGXe5yZlt+HykK6Y3DM= +github.com/mattermost/mattermost-utilities/mmgotool v0.0.0-20201013204121-e463fcd00ba8 h1:6wiKDdS/MMY0Jht12lRZJq1zv5eHEd4fK60SwkUVGjA= +github.com/mattermost/mattermost-utilities/mmgotool v0.0.0-20201013204121-e463fcd00ba8/go.mod h1:3gKozJI8n2Y/vW37GfnFWAdehGXe5yZlt+HykK6Y3DM= github.com/matttproud/golang_protobuf_extensions v1.0.1/go.mod h1:D8He9yQNgCq6Z5Ld7szi9bcBfOoFv/3dc6xSMkL2PC0= github.com/mitchellh/go-homedir v1.1.0/go.mod h1:SfyaCUpYCn1Vlf4IUYiD9fPX4A5wJrkLzIz1N1q0pr0= github.com/mitchellh/mapstructure v1.1.2/go.mod h1:FVVH3fgwuzCH5S8UJGiWEs2h04kUh9fWfEaFds41c1Y= diff --git a/i18n/en.json b/i18n/en.json index 9e8038b152..187933ef2c 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -1384,6 +1384,10 @@ "id": "api.file.move_file.rename.app_error", "translation": "Unable to move file locally." }, + { + "id": "api.file.new_backend.s3.app_error", + "translation": "Encountered an error opening a connection to S3." + }, { "id": "api.file.no_driver.app_error", "translation": "No file driver selected." @@ -1416,10 +1420,6 @@ "id": "api.file.test_connection.s3.bucket_exists.app_error", "translation": "Error checking if bucket exists." }, - { - "id": "api.file.test_connection.s3.connection.app_error", - "translation": "Bad connection to S3 or minio." - }, { "id": "api.file.test_connection.s3.list_objects.app_error", "translation": "Error trying to list objects." diff --git a/services/filesstore/filesstore.go b/services/filesstore/filesstore.go index 8bc2d9e8cf..7ca4c1c2d8 100644 --- a/services/filesstore/filesstore.go +++ b/services/filesstore/filesstore.go @@ -34,18 +34,11 @@ type FileBackend interface { func NewFileBackend(settings *model.FileSettings, enableComplianceFeatures bool) (FileBackend, *model.AppError) { switch *settings.DriverName { case model.IMAGE_DRIVER_S3: - return &S3FileBackend{ - endpoint: *settings.AmazonS3Endpoint, - accessKey: *settings.AmazonS3AccessKeyId, - secretKey: *settings.AmazonS3SecretAccessKey, - secure: settings.AmazonS3SSL == nil || *settings.AmazonS3SSL, - signV2: settings.AmazonS3SignV2 != nil && *settings.AmazonS3SignV2, - region: *settings.AmazonS3Region, - bucket: *settings.AmazonS3Bucket, - pathPrefix: *settings.AmazonS3PathPrefix, - encrypt: settings.AmazonS3SSE != nil && *settings.AmazonS3SSE && enableComplianceFeatures, - trace: settings.AmazonS3Trace != nil && *settings.AmazonS3Trace, - }, nil + backend, err := NewS3FileBackend(settings, enableComplianceFeatures) + if err != nil { + return nil, model.NewAppError("NewFileBackend", "api.file.new_backend.s3.app_error", nil, err.Error(), http.StatusInternalServerError) + } + return backend, nil case model.IMAGE_DRIVER_LOCAL: return &LocalFileBackend{ directory: *settings.Directory, diff --git a/services/filesstore/s3store.go b/services/filesstore/s3store.go index 4f12006554..03f59c8595 100644 --- a/services/filesstore/s3store.go +++ b/services/filesstore/s3store.go @@ -21,6 +21,8 @@ import ( "github.com/mattermost/mattermost-server/v5/model" ) +// S3FileBackend contains all necessary information to communicate with +// an AWS S3 compatible API backend. type S3FileBackend struct { endpoint string accessKey string @@ -32,6 +34,7 @@ type S3FileBackend struct { pathPrefix string encrypt bool trace bool + client *s3.Client } const ( @@ -39,6 +42,28 @@ const ( bucketNotFound = "NoSuchBucket" ) +// NewS3FileBackend returns an instance of an S3FileBackend. +func NewS3FileBackend(settings *model.FileSettings, enableComplianceFeatures bool) (*S3FileBackend, error) { + backend := &S3FileBackend{ + endpoint: *settings.AmazonS3Endpoint, + accessKey: *settings.AmazonS3AccessKeyId, + secretKey: *settings.AmazonS3SecretAccessKey, + secure: settings.AmazonS3SSL == nil || *settings.AmazonS3SSL, + signV2: settings.AmazonS3SignV2 != nil && *settings.AmazonS3SignV2, + region: *settings.AmazonS3Region, + bucket: *settings.AmazonS3Bucket, + pathPrefix: *settings.AmazonS3PathPrefix, + encrypt: settings.AmazonS3SSE != nil && *settings.AmazonS3SSE && enableComplianceFeatures, + trace: settings.AmazonS3Trace != nil && *settings.AmazonS3Trace, + } + cli, err := backend.s3New() + if err != nil { + return nil, err + } + backend.client = cli + return backend, nil +} + // Similar to s3.New() but allows initialization of signature v2 or signature v4 client. // If signV2 input is false, function always returns signature v4. // @@ -73,17 +98,13 @@ func (b *S3FileBackend) s3New() (*s3.Client, error) { } func (b *S3FileBackend) TestConnection() *model.AppError { - s3Clnt, err := b.s3New() - if err != nil { - return model.NewAppError("TestFileConnection", "api.file.test_connection.s3.connection.app_error", nil, err.Error(), http.StatusInternalServerError) - } - exists := true + var err error // If a path prefix is present, we attempt to test the bucket by listing objects under the path // and just checking the first response. This is because the BucketExists call is only at a bucket level // and sometimes the user might only be allowed access to the specified path prefix. if b.pathPrefix != "" { - obj := <-s3Clnt.ListObjects(context.Background(), b.bucket, s3.ListObjectsOptions{Prefix: b.pathPrefix}) + obj := <-b.client.ListObjects(context.Background(), b.bucket, s3.ListObjectsOptions{Prefix: b.pathPrefix}) if obj.Err != nil { typedErr := s3.ToErrorResponse(obj.Err) if typedErr.Code != bucketNotFound { @@ -92,7 +113,7 @@ func (b *S3FileBackend) TestConnection() *model.AppError { exists = false } } else { - exists, err = s3Clnt.BucketExists(context.Background(), b.bucket) + exists, err = b.client.BucketExists(context.Background(), b.bucket) if err != nil { return model.NewAppError("TestFileConnection", "api.file.test_connection.s3.bucket_exists.app_error", nil, err.Error(), http.StatusInternalServerError) } @@ -102,7 +123,7 @@ func (b *S3FileBackend) TestConnection() *model.AppError { mlog.Debug("Connection to S3 or minio is good. Bucket exists.") } else { mlog.Warn("Bucket specified does not exist. Attempting to create...") - err := s3Clnt.MakeBucket(context.Background(), b.bucket, s3.MakeBucketOptions{Region: b.region}) + err := b.client.MakeBucket(context.Background(), b.bucket, s3.MakeBucketOptions{Region: b.region}) if err != nil { return model.NewAppError("TestFileConnection", "api.file.test_connection.s3.bucket_create.app_error", nil, err.Error(), http.StatusInternalServerError) } @@ -113,13 +134,8 @@ func (b *S3FileBackend) TestConnection() *model.AppError { // Caller must close the first return value func (b *S3FileBackend) Reader(path string) (ReadCloseSeeker, *model.AppError) { - s3Clnt, err := b.s3New() - if err != nil { - return nil, model.NewAppError("Reader", "api.file.reader.s3.app_error", nil, err.Error(), http.StatusInternalServerError) - } - path = filepath.Join(b.pathPrefix, path) - minioObject, err := s3Clnt.GetObject(context.Background(), b.bucket, path, s3.GetObjectOptions{}) + minioObject, err := b.client.GetObject(context.Background(), b.bucket, path, s3.GetObjectOptions{}) if err != nil { return nil, model.NewAppError("Reader", "api.file.reader.s3.app_error", nil, err.Error(), http.StatusInternalServerError) } @@ -128,13 +144,8 @@ func (b *S3FileBackend) Reader(path string) (ReadCloseSeeker, *model.AppError) { } func (b *S3FileBackend) ReadFile(path string) ([]byte, *model.AppError) { - s3Clnt, err := b.s3New() - if err != nil { - return nil, model.NewAppError("ReadFile", "api.file.read_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) - } - path = filepath.Join(b.pathPrefix, path) - minioObject, err := s3Clnt.GetObject(context.Background(), b.bucket, path, s3.GetObjectOptions{}) + minioObject, err := b.client.GetObject(context.Background(), b.bucket, path, s3.GetObjectOptions{}) if err != nil { return nil, model.NewAppError("ReadFile", "api.file.read_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) } @@ -148,15 +159,9 @@ func (b *S3FileBackend) ReadFile(path string) ([]byte, *model.AppError) { } func (b *S3FileBackend) FileExists(path string) (bool, *model.AppError) { - s3Clnt, err := b.s3New() - - if err != nil { - return false, model.NewAppError("FileExists", "api.file.file_exists.s3.app_error", nil, err.Error(), http.StatusInternalServerError) - } - path = filepath.Join(b.pathPrefix, path) - _, err = s3Clnt.StatObject(context.Background(), b.bucket, path, s3.StatObjectOptions{}) + _, err := b.client.StatObject(context.Background(), b.bucket, path, s3.StatObjectOptions{}) if err == nil { return true, nil } @@ -170,11 +175,6 @@ func (b *S3FileBackend) FileExists(path string) (bool, *model.AppError) { } func (b *S3FileBackend) CopyFile(oldPath, newPath string) *model.AppError { - s3Clnt, err := b.s3New() - if err != nil { - return model.NewAppError("copyFile", "api.file.write_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) - } - oldPath = filepath.Join(b.pathPrefix, oldPath) newPath = filepath.Join(b.pathPrefix, newPath) srcOpts := s3.CopySrcOptions{ @@ -187,18 +187,13 @@ func (b *S3FileBackend) CopyFile(oldPath, newPath string) *model.AppError { Object: newPath, Encryption: encrypt.NewSSE(), } - if _, err = s3Clnt.CopyObject(context.Background(), dstOpts, srcOpts); err != nil { + if _, err := b.client.CopyObject(context.Background(), dstOpts, srcOpts); err != nil { return model.NewAppError("copyFile", "api.file.move_file.copy_within_s3.app_error", nil, err.Error(), http.StatusInternalServerError) } return nil } func (b *S3FileBackend) MoveFile(oldPath, newPath string) *model.AppError { - s3Clnt, err := b.s3New() - if err != nil { - return model.NewAppError("moveFile", "api.file.write_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) - } - oldPath = filepath.Join(b.pathPrefix, oldPath) newPath = filepath.Join(b.pathPrefix, newPath) srcOpts := s3.CopySrcOptions{ @@ -212,11 +207,11 @@ func (b *S3FileBackend) MoveFile(oldPath, newPath string) *model.AppError { Encryption: encrypt.NewSSE(), } - if _, err = s3Clnt.CopyObject(context.Background(), dstOpts, srcOpts); err != nil { + if _, err := b.client.CopyObject(context.Background(), dstOpts, srcOpts); err != nil { return model.NewAppError("moveFile", "api.file.move_file.copy_within_s3.app_error", nil, err.Error(), http.StatusInternalServerError) } - if err = s3Clnt.RemoveObject(context.Background(), b.bucket, oldPath, s3.RemoveObjectOptions{}); err != nil { + if err := b.client.RemoveObject(context.Background(), b.bucket, oldPath, s3.RemoveObjectOptions{}); err != nil { return model.NewAppError("moveFile", "api.file.move_file.delete_from_s3.app_error", nil, err.Error(), http.StatusInternalServerError) } @@ -224,11 +219,6 @@ func (b *S3FileBackend) MoveFile(oldPath, newPath string) *model.AppError { } func (b *S3FileBackend) WriteFile(fr io.Reader, path string) (int64, *model.AppError) { - s3Clnt, err := b.s3New() - if err != nil { - return 0, model.NewAppError("WriteFile", "api.file.write_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) - } - var contentType string path = filepath.Join(b.pathPrefix, path) if ext := filepath.Ext(path); model.IsFileExtImage(ext) { @@ -238,7 +228,7 @@ func (b *S3FileBackend) WriteFile(fr io.Reader, path string) (int64, *model.AppE } options := s3PutOptions(b.encrypt, contentType) - info, err := s3Clnt.PutObject(context.Background(), b.bucket, path, fr, -1, options) + info, err := b.client.PutObject(context.Background(), b.bucket, path, fr, -1, options) if err != nil { return info.Size, model.NewAppError("WriteFile", "api.file.write_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) } @@ -247,13 +237,8 @@ func (b *S3FileBackend) WriteFile(fr io.Reader, path string) (int64, *model.AppE } func (b *S3FileBackend) AppendFile(fr io.Reader, path string) (int64, *model.AppError) { - s3Clnt, err := b.s3New() - if err != nil { - return 0, model.NewAppError("AppendFile", "api.file.append_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) - } - fp := filepath.Join(b.pathPrefix, path) - if _, err = s3Clnt.StatObject(context.Background(), b.bucket, fp, s3.StatObjectOptions{}); err != nil { + if _, err := b.client.StatObject(context.Background(), b.bucket, fp, s3.StatObjectOptions{}); err != nil { return 0, model.NewAppError("AppendFile", "api.file.append_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) } @@ -267,8 +252,8 @@ func (b *S3FileBackend) AppendFile(fr io.Reader, path string) (int64, *model.App options := s3PutOptions(b.encrypt, contentType) sse := options.ServerSideEncryption partName := fp + ".part" - info, err := s3Clnt.PutObject(context.Background(), b.bucket, partName, fr, -1, options) - defer s3Clnt.RemoveObject(context.Background(), b.bucket, partName, s3.RemoveObjectOptions{}) + info, err := b.client.PutObject(context.Background(), b.bucket, partName, fr, -1, options) + defer b.client.RemoveObject(context.Background(), b.bucket, partName, s3.RemoveObjectOptions{}) if info.Size > 0 { src1Opts := s3.CopySrcOptions{ Bucket: b.bucket, @@ -283,7 +268,7 @@ func (b *S3FileBackend) AppendFile(fr io.Reader, path string) (int64, *model.App Object: fp, Encryption: sse, } - _, err = s3Clnt.ComposeObject(context.Background(), dstOpts, src1Opts, src2Opts) + _, err = b.client.ComposeObject(context.Background(), dstOpts, src1Opts, src2Opts) if err != nil { return 0, model.NewAppError("AppendFile", "api.file.append_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) } @@ -299,13 +284,8 @@ func (b *S3FileBackend) AppendFile(fr io.Reader, path string) (int64, *model.App } func (b *S3FileBackend) RemoveFile(path string) *model.AppError { - s3Clnt, err := b.s3New() - if err != nil { - return model.NewAppError("RemoveFile", "utils.file.remove_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) - } - path = filepath.Join(b.pathPrefix, path) - if err := s3Clnt.RemoveObject(context.Background(), b.bucket, path, s3.RemoveObjectOptions{}); err != nil { + if err := b.client.RemoveObject(context.Background(), b.bucket, path, s3.RemoveObjectOptions{}); err != nil { return model.NewAppError("RemoveFile", "utils.file.remove_file.s3.app_error", nil, err.Error(), http.StatusInternalServerError) } @@ -335,11 +315,6 @@ func getPathsFromObjectInfos(in <-chan s3.ObjectInfo) <-chan s3.ObjectInfo { func (b *S3FileBackend) ListDirectory(path string) (*[]string, *model.AppError) { var paths []string - s3Clnt, err := b.s3New() - if err != nil { - return nil, model.NewAppError("ListDirectory", "utils.file.list_directory.s3.app_error", nil, err.Error(), http.StatusInternalServerError) - } - path = filepath.Join(b.pathPrefix, path) if !strings.HasSuffix(path, "/") && len(path) > 0 { // s3Clnt returns only the path itself when "/" is not present @@ -350,7 +325,7 @@ func (b *S3FileBackend) ListDirectory(path string) (*[]string, *model.AppError) opts := s3.ListObjectsOptions{ Prefix: path, } - for object := range s3Clnt.ListObjects(context.Background(), b.bucket, opts) { + for object := range b.client.ListObjects(context.Background(), b.bucket, opts) { if object.Err != nil { return nil, model.NewAppError("ListDirectory", "utils.file.list_directory.s3.app_error", nil, object.Err.Error(), http.StatusInternalServerError) } @@ -361,16 +336,12 @@ func (b *S3FileBackend) ListDirectory(path string) (*[]string, *model.AppError) } func (b *S3FileBackend) RemoveDirectory(path string) *model.AppError { - s3Clnt, err := b.s3New() - if err != nil { - return model.NewAppError("RemoveDirectory", "utils.file.remove_directory.s3.app_error", nil, err.Error(), http.StatusInternalServerError) - } opts := s3.ListObjectsOptions{ Prefix: filepath.Join(b.pathPrefix, path), Recursive: true, } - list := s3Clnt.ListObjects(context.Background(), b.bucket, opts) - objectsCh := s3Clnt.RemoveObjects(context.Background(), b.bucket, getPathsFromObjectInfos(list), s3.RemoveObjectsOptions{}) + list := b.client.ListObjects(context.Background(), b.bucket, opts) + objectsCh := b.client.RemoveObjects(context.Background(), b.bucket, getPathsFromObjectInfos(list), s3.RemoveObjectsOptions{}) for err := range objectsCh { if err.Err != nil { return model.NewAppError("RemoveDirectory", "utils.file.remove_directory.s3.app_error", nil, err.Err.Error(), http.StatusInternalServerError)