From 944841a237ad9286ac9c8a2e7661872fa2044dc2 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Thu, 15 Oct 2020 08:54:57 +0530 Subject: [PATCH] MM-29573: Reuse s3 client amongst all requests (#15948) * MM-29573: Reuse s3 client amongst all requests An http.Client is meant as a long-term object to be reused between consecutive requests. This reduces burden on HTTP servers by reusing TCP connections and also makes things faster by avoiding the TCP handshake for every single request. https://mattermost.atlassian.net/browse/MM-29573 * Trigger CI * Fix i18n Co-authored-by: Mattermod --- go.tools.mod | 2 +- go.tools.sum | 4 + i18n/en.json | 8 +- services/filesstore/filesstore.go | 17 ++--- services/filesstore/s3store.go | 117 +++++++++++------------------- 5 files changed, 58 insertions(+), 90 deletions(-) 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)