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 <mattermod@users.noreply.github.com>
2020-10-15 08:54:57 +05:30
--- 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
--- 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=
--- 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."
--- 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,
--- 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)