s3: add documentation and test cases for upload part size

devopsmakers · Mar 13, 2020 · de3e69f · de3e69f
1 parent cdf1233
commit de3e69f
Show file tree

Hide file tree

Showing 11 changed files with 88 additions and 27 deletions.
diff --git a/docs/account.md b/docs/account.md
@@ -46,6 +46,7 @@ For each account, the following properties can be configured:
 - `s3_endpoint`, specifies a S3 endpoint (server) different from AWS. It is not required if you are connecting to AWS
 - `s3_storage_class`, leave blank to use the default or specify a valid AWS [storage class](https://docs.aws.amazon.com/AmazonS3/latest/dev/storage-class-intro.html)
 - `s3_key_prefix`, allows to restrict access to the virtual folder identified by this prefix and its contents
+- `s3_upload_part_size`, the buffer size for multipart uploads (MB). Zero means the default (5 MB). Minimum is 5
 - `gcs_bucket`, required for GCS filesystem
 - `gcs_credentials`, Google Cloud Storage JSON credentials base64 encoded
 - `gcs_automatic_credentials`, integer. Set to 1 to use Application Default Credentials strategy or set to 0 to use explicit credentials via `gcs_credentials`

diff --git a/httpd/api_utils.go b/httpd/api_utils.go
@@ -453,6 +453,16 @@ func compareUserFsConfig(expected *dataprovider.User, actual *dataprovider.User)
 	if expected.FsConfig.Provider != actual.FsConfig.Provider {
 		return errors.New("Fs provider mismatch")
 	}
+	if err := compareS3Config(expected, actual); err != nil {
+		return err
+	}
+	if err := compareGCSConfig(expected, actual); err != nil {
+		return err
+	}
+	return nil
+}
+
+func compareS3Config(expected *dataprovider.User, actual *dataprovider.User) error {
 	if expected.FsConfig.S3Config.Bucket != actual.FsConfig.S3Config.Bucket {
 		return errors.New("S3 bucket mismatch")
 	}
@@ -471,10 +481,17 @@ func compareUserFsConfig(expected *dataprovider.User, actual *dataprovider.User)
 	if expected.FsConfig.S3Config.StorageClass != actual.FsConfig.S3Config.StorageClass {
 		return errors.New("S3 storage class mismatch")
 	}
+	if expected.FsConfig.S3Config.UploadPartSize != actual.FsConfig.S3Config.UploadPartSize {
+		return errors.New("S3 upload part size class mismatch")
+	}
 	if expected.FsConfig.S3Config.KeyPrefix != actual.FsConfig.S3Config.KeyPrefix &&
 		expected.FsConfig.S3Config.KeyPrefix+"/" != actual.FsConfig.S3Config.KeyPrefix {
 		return errors.New("S3 key prefix mismatch")
 	}
+	return nil
+}
+
+func compareGCSConfig(expected *dataprovider.User, actual *dataprovider.User) error {
 	if expected.FsConfig.GCSConfig.Bucket != actual.FsConfig.GCSConfig.Bucket {
 		return errors.New("GCS bucket mismatch")
 	}

diff --git a/httpd/httpd_test.go b/httpd/httpd_test.go
@@ -394,6 +394,12 @@ func TestAddUserInvalidFsConfig(t *testing.T) {
 	if err != nil {
 		t.Errorf("unexpected error adding user with invalid fs config: %v", err)
 	}
+	u.FsConfig.S3Config.KeyPrefix = ""
+	u.FsConfig.S3Config.UploadPartSize = 3
+	_, _, err = httpd.AddUser(u, http.StatusBadRequest)
+	if err != nil {
+		t.Errorf("unexpected error adding user with invalid fs config: %v", err)
+	}
 	u = getTestUser()
 	u.FsConfig.Provider = 2
 	u.FsConfig.GCSConfig.Bucket = ""
@@ -2010,7 +2016,7 @@ func TestWebUserS3Mock(t *testing.T) {
 	user.FsConfig.S3Config.Endpoint = "http://127.0.0.1:9000/path?a=b"
 	user.FsConfig.S3Config.StorageClass = "Standard"
 	user.FsConfig.S3Config.KeyPrefix = "somedir/subdir/"
-	user.FsConfig.S3Config.PartSize = 5
+	user.FsConfig.S3Config.UploadPartSize = 5
 	form := make(url.Values)
 	form.Set("username", user.Username)
 	form.Set("home_dir", user.HomeDir)
@@ -2035,13 +2041,21 @@ func TestWebUserS3Mock(t *testing.T) {
 	form.Set("s3_storage_class", user.FsConfig.S3Config.StorageClass)
 	form.Set("s3_endpoint", user.FsConfig.S3Config.Endpoint)
 	form.Set("s3_key_prefix", user.FsConfig.S3Config.KeyPrefix)
-	form.Set("s3_part_size", strconv.FormatInt(int64(user.FsConfig.S3Config.PartSize), 10))
 	form.Set("allowed_extensions", "/dir1::.jpg,.png")
 	form.Set("denied_extensions", "/dir2::.zip")
+	// test invalid s3_upload_part_size
+	form.Set("s3_upload_part_size", "a")
 	b, contentType, _ := getMultipartFormData(form, "", "")
 	req, _ = http.NewRequest(http.MethodPost, webUserPath+"/"+strconv.FormatInt(user.ID, 10), &b)
 	req.Header.Set("Content-Type", contentType)
 	rr = executeRequest(req)
+	checkResponseCode(t, http.StatusOK, rr.Code)
+	// now add the user
+	form.Set("s3_upload_part_size", strconv.FormatInt(user.FsConfig.S3Config.UploadPartSize, 10))
+	b, contentType, _ = getMultipartFormData(form, "", "")
+	req, _ = http.NewRequest(http.MethodPost, webUserPath+"/"+strconv.FormatInt(user.ID, 10), &b)
+	req.Header.Set("Content-Type", contentType)
+	rr = executeRequest(req)
 	checkResponseCode(t, http.StatusSeeOther, rr.Code)
 	req, _ = http.NewRequest(http.MethodGet, userPath+"?limit=1&offset=0&order=ASC&username="+user.Username, nil)
 	rr = executeRequest(req)
@@ -2082,6 +2096,9 @@ func TestWebUserS3Mock(t *testing.T) {
 	if updateUser.FsConfig.S3Config.KeyPrefix != user.FsConfig.S3Config.KeyPrefix {
 		t.Error("s3 key prefix mismatch")
 	}
+	if updateUser.FsConfig.S3Config.UploadPartSize != user.FsConfig.S3Config.UploadPartSize {
+		t.Error("s3 upload part size mismatch")
+	}
 	if len(updateUser.Filters.FileExtensions) != 2 {
 		t.Errorf("unexpected extensions filter: %+v", updateUser.Filters.FileExtensions)
 	}

diff --git a/httpd/internal_test.go b/httpd/internal_test.go
@@ -363,6 +363,12 @@ func TestCompareUserFsConfig(t *testing.T) {
 		t.Errorf("S3 key prefix does not match")
 	}
 	expected.FsConfig.S3Config.KeyPrefix = ""
+	expected.FsConfig.S3Config.UploadPartSize = 10
+	err = compareUserFsConfig(expected, actual)
+	if err == nil {
+		t.Errorf("S3 upload part size does not match")
+	}
+	expected.FsConfig.S3Config.UploadPartSize = 0
 }
 
 func TestCompareUserGCSConfig(t *testing.T) {

diff --git a/httpd/schema/openapi.yaml b/httpd/schema/openapi.yaml
@@ -2,7 +2,7 @@ openapi: 3.0.1
 info:
   title: SFTPGo
   description: 'SFTPGo REST API'
-  version: 1.8.3
+  version: 1.8.4
 
 servers:
 - url: /api/v1
@@ -1004,6 +1004,12 @@ components:
           description: optional endpoint
         storage_class:
           type: string
+        upload_part_size:
+          type: integer
+          description: the buffer size (in MB) to use for multipart uploads. The minimum allowed part size is 5MB, and if this value is set to zero, the default value (5MB) for the AWS SDK will be used. The minimum allowed value is 5.
+        upload_concurrency:
+          type: integer
+          description: the number of parts to upload in parallel. If this value is set to zero, 2 will be used
         key_prefix:
           type: string
           description: key_prefix is similar to a chroot directory for a local filesystem. If specified the SFTP user will only see contents that starts with this prefix and so you can restrict access to a specific virtual folder. The prefix, if not empty, must not start with "/" and must end with "/". If empty the whole bucket contents will be available

diff --git a/httpd/web.go b/httpd/web.go
@@ -329,7 +329,7 @@ func getFsConfigFromUserPostFields(r *http.Request) (dataprovider.Filesystem, er
 		fs.S3Config.Endpoint = r.Form.Get("s3_endpoint")
 		fs.S3Config.StorageClass = r.Form.Get("s3_storage_class")
 		fs.S3Config.KeyPrefix = r.Form.Get("s3_key_prefix")
-		fs.S3Config.PartSize, err = strconv.ParseInt(r.Form.Get("s3_part_size"), 10, 64)
+		fs.S3Config.UploadPartSize, err = strconv.ParseInt(r.Form.Get("s3_upload_part_size"), 10, 64)
 		if err != nil {
 			return fs, err
 		}

diff --git a/scripts/README.md b/scripts/README.md
@@ -44,7 +44,7 @@ Let's see a sample usage for each REST API.
 Command:
 
 ```
-python sftpgo_api_cli.py add-user test_username --password "test_pwd" --home-dir="/tmp/test_home_dir" --uid 33 --gid 1000 --max-sessions 2 --quota-size 0 --quota-files 3 --permissions "list" "download" "upload" "delete" "rename" "create_dirs" "overwrite" --subdirs-permissions "/dir1::list,download" "/dir2::*" --upload-bandwidth 100 --download-bandwidth 60 --status 0 --expiration-date 2019-01-01 --allowed-ip "192.168.1.1/32" --fs S3 --s3-bucket test --s3-region eu-west-1 --s3-access-key accesskey --s3-access-secret secret --s3-endpoint "http://127.0.0.1:9000" --s3-storage-class Standard --s3-key-prefix "vfolder/" --denied-login-methods "password" "keyboard-interactive" --allowed-extensions "/dir1::.jpg,.png" "/dir2::.rar,.png" --denied-extensions "/dir3::.zip,.rar"
+python sftpgo_api_cli.py add-user test_username --password "test_pwd" --home-dir="/tmp/test_home_dir" --uid 33 --gid 1000 --max-sessions 2 --quota-size 0 --quota-files 3 --permissions "list" "download" "upload" "delete" "rename" "create_dirs" "overwrite" --subdirs-permissions "/dir1::list,download" "/dir2::*" --upload-bandwidth 100 --download-bandwidth 60 --status 0 --expiration-date 2019-01-01 --allowed-ip "192.168.1.1/32" --fs S3 --s3-bucket test --s3-region eu-west-1 --s3-access-key accesskey --s3-access-secret secret --s3-endpoint "http://127.0.0.1:9000" --s3-storage-class Standard --s3-key-prefix "vfolder/" --s3-upload-part-size 10 --denied-login-methods "password" "keyboard-interactive" --allowed-extensions "/dir1::.jpg,.png" "/dir2::.rar,.png" --denied-extensions "/dir3::.zip,.rar"
 ```
 
 Output:
@@ -63,7 +63,8 @@ Output:
       "endpoint": "http://127.0.0.1:9000",
       "key_prefix": "vfolder/",
       "region": "eu-west-1",
-      "storage_class": "Standard"
+      "storage_class": "Standard",
+      "upload_part_size": 10
     }
   },
   "filters": {

diff --git a/scripts/sftpgo_api_cli.py b/scripts/sftpgo_api_cli.py
@@ -77,7 +77,7 @@ def buildUserObject(self, user_id=0, username='', password='', public_keys=[], h
 					s3_region='', s3_access_key='', s3_access_secret='', s3_endpoint='', s3_storage_class='',
 					s3_key_prefix='', gcs_bucket='', gcs_key_prefix='', gcs_storage_class='', gcs_credentials_file='',
 					gcs_automatic_credentials='automatic', denied_login_methods=[], virtual_folders=[],
-					denied_extensions=[], allowed_extensions=[]):
+					denied_extensions=[], allowed_extensions=[], s3_upload_part_size=0):
 		user = {'id':user_id, 'username':username, 'uid':uid, 'gid':gid,
 			'max_sessions':max_sessions, 'quota_size':quota_size, 'quota_files':quota_files,
 			'upload_bandwidth':upload_bandwidth, 'download_bandwidth':download_bandwidth,
@@ -101,7 +101,7 @@ def buildUserObject(self, user_id=0, username='', password='', public_keys=[], h
 		user.update({'filesystem':self.buildFsConfig(fs_provider, s3_bucket, s3_region, s3_access_key, s3_access_secret,
 													s3_endpoint, s3_storage_class, s3_key_prefix, gcs_bucket,
 													gcs_key_prefix, gcs_storage_class, gcs_credentials_file,
-													gcs_automatic_credentials)})
+													gcs_automatic_credentials, s3_upload_part_size)})
 		return user
 
 	def buildVirtualFolders(self, vfolders):
@@ -204,12 +204,12 @@ def buildFilters(self, allowed_ip, denied_ip, denied_login_methods, denied_exten
 
 	def buildFsConfig(self, fs_provider, s3_bucket, s3_region, s3_access_key, s3_access_secret, s3_endpoint,
 					s3_storage_class, s3_key_prefix, gcs_bucket, gcs_key_prefix, gcs_storage_class,
-					gcs_credentials_file, gcs_automatic_credentials):
+					gcs_credentials_file, gcs_automatic_credentials, s3_upload_part_size):
 		fs_config = {'provider':0}
 		if fs_provider == 'S3':
 			s3config = {'bucket':s3_bucket, 'region':s3_region, 'access_key':s3_access_key, 'access_secret':
 					s3_access_secret, 'endpoint':s3_endpoint, 'storage_class':s3_storage_class, 'key_prefix':
-					s3_key_prefix}
+					s3_key_prefix, 'upload_part_size':s3_upload_part_size}
 			fs_config.update({'provider':1, 's3config':s3config})
 		elif fs_provider == 'GCS':
 			gcsconfig = {'bucket':gcs_bucket, 'key_prefix':gcs_key_prefix, 'storage_class':gcs_storage_class}
@@ -238,13 +238,14 @@ def addUser(self, username='', password='', public_keys='', home_dir='', uid=0,
 			subdirs_permissions=[], allowed_ip=[], denied_ip=[], fs_provider='local', s3_bucket='', s3_region='',
 			s3_access_key='', s3_access_secret='', s3_endpoint='', s3_storage_class='', s3_key_prefix='', gcs_bucket='',
 			gcs_key_prefix='', gcs_storage_class='', gcs_credentials_file='', gcs_automatic_credentials='automatic',
-			denied_login_methods=[], virtual_folders=[], denied_extensions=[], allowed_extensions=[]):
+			denied_login_methods=[], virtual_folders=[], denied_extensions=[], allowed_extensions=[],
+			s3_upload_part_size=0):
 		u = self.buildUserObject(0, username, password, public_keys, home_dir, uid, gid, max_sessions,
 			quota_size, quota_files, self.buildPermissions(perms, subdirs_permissions), upload_bandwidth, download_bandwidth,
 			status, expiration_date, allowed_ip, denied_ip, fs_provider, s3_bucket, s3_region, s3_access_key,
 			s3_access_secret, s3_endpoint, s3_storage_class, s3_key_prefix, gcs_bucket, gcs_key_prefix, gcs_storage_class,
 			gcs_credentials_file, gcs_automatic_credentials, denied_login_methods, virtual_folders, denied_extensions,
-			allowed_extensions)
+			allowed_extensions, s3_upload_part_size)
 		r = requests.post(self.userPath, json=u, auth=self.auth, verify=self.verify)
 		self.printResponse(r)
 
@@ -254,13 +255,13 @@ def updateUser(self, user_id, username='', password='', public_keys='', home_dir
 				s3_bucket='', s3_region='', s3_access_key='', s3_access_secret='', s3_endpoint='', s3_storage_class='',
 				s3_key_prefix='', gcs_bucket='', gcs_key_prefix='', gcs_storage_class='', gcs_credentials_file='',
 				gcs_automatic_credentials='automatic', denied_login_methods=[], virtual_folders=[], denied_extensions=[],
-				allowed_extensions=[]):
+				allowed_extensions=[], s3_upload_part_size=0):
 		u = self.buildUserObject(user_id, username, password, public_keys, home_dir, uid, gid, max_sessions,
 			quota_size, quota_files, self.buildPermissions(perms, subdirs_permissions), upload_bandwidth, download_bandwidth,
 			status, expiration_date, allowed_ip, denied_ip, fs_provider, s3_bucket, s3_region, s3_access_key,
 			s3_access_secret, s3_endpoint, s3_storage_class, s3_key_prefix, gcs_bucket, gcs_key_prefix, gcs_storage_class,
 			gcs_credentials_file, gcs_automatic_credentials, denied_login_methods, virtual_folders, denied_extensions,
-			allowed_extensions)
+			allowed_extensions, s3_upload_part_size)
 		r = requests.put(urlparse.urljoin(self.userPath, 'user/' + str(user_id)), json=u, auth=self.auth, verify=self.verify)
 		self.printResponse(r)
 
@@ -537,6 +538,8 @@ def addCommonUserArguments(parser):
 	parser.add_argument('--s3-access-secret', type=str, default='', help='Default: %(default)s')
 	parser.add_argument('--s3-endpoint', type=str, default='', help='Default: %(default)s')
 	parser.add_argument('--s3-storage-class', type=str, default='', help='Default: %(default)s')
+	parser.add_argument('--s3-upload-part-size', type=int, default=0, help='The buffer size for multipart uploads (MB). ' +
+					'Zero means the default (5 MB). Minimum is 5. Default: %(default)s')
 	parser.add_argument('--gcs-bucket', type=str, default='', help='Default: %(default)s')
 	parser.add_argument('--gcs-key-prefix', type=str, default='', help='Virtual root directory. If non empty only this ' +
 					'directory and its contents will be available. Cannot start with "/". For example "folder/subfolder/".' +
@@ -656,7 +659,8 @@ def addCommonUserArguments(parser):
 				args.denied_ip, args.fs, args.s3_bucket, args.s3_region, args.s3_access_key, args.s3_access_secret,
 				args.s3_endpoint, args.s3_storage_class, args.s3_key_prefix, args.gcs_bucket, args.gcs_key_prefix,
 				args.gcs_storage_class, args.gcs_credentials_file, args.gcs_automatic_credentials,
-				args.denied_login_methods, args.virtual_folders, args.denied_extensions, args.allowed_extensions)
+				args.denied_login_methods, args.virtual_folders, args.denied_extensions, args.allowed_extensions,
+				args.s3_upload_part_size)
 	elif args.command == 'update-user':
 		api.updateUser(args.id, args.username, args.password, args.public_keys, args.home_dir, args.uid, args.gid,
 					args.max_sessions, args.quota_size, args.quota_files, args.permissions, args.upload_bandwidth,
@@ -665,7 +669,7 @@ def addCommonUserArguments(parser):
 					args.s3_access_key, args.s3_access_secret, args.s3_endpoint, args.s3_storage_class,
 					args.s3_key_prefix, args.gcs_bucket, args.gcs_key_prefix, args.gcs_storage_class,
 					args.gcs_credentials_file, args.gcs_automatic_credentials, args.denied_login_methods,
-					args.virtual_folders, args.denied_extensions, args.allowed_extensions)
+					args.virtual_folders, args.denied_extensions, args.allowed_extensions, args.s3_upload_part_size)
 	elif args.command == 'delete-user':
 		api.deleteUser(args.id)
 	elif args.command == 'get-users':

diff --git a/templates/user.html b/templates/user.html
@@ -301,10 +301,13 @@ <h1 class="h5 mb-4 text-gray-800">{{if .IsAdd}}Add a new user{{else}}Edit user{{
     </div>
 
     <div class="form-group row s3">
-        <label for="idS3PartSize" class="col-sm-2 col-form-label">Part Size (MB)</label>
+        <label for="idS3PartSize" class="col-sm-2 col-form-label">UL Part Size (MB)</label>
         <div class="col-sm-3">
-            <input type="number" class="form-control" id="idS3PartSoze" name="s3_part_size" placeholder=""
-                value="{{.User.FsConfig.S3Config.PartSize}}" min="5">
+            <input type="number" class="form-control" id="idS3PartSize" name="s3_upload_part_size" placeholder=""
+                value="{{.User.FsConfig.S3Config.UploadPartSize}}" aria-describedby="S3PartSizeHelpBlock">
+            <small id="S3PartSizeHelpBlock" class="form-text text-muted">
+                The buffer size for multipart uploads. Zero means the default (5 MB). Minimum is 5
+            </small>
         </div>
         <div class="col-sm-2"></div>
     </div>

diff --git a/vfs/s3fs.go b/vfs/s3fs.go
@@ -36,7 +36,13 @@ type S3FsConfig struct {
 	AccessSecret string `json:"access_secret,omitempty"`
 	Endpoint     string `json:"endpoint,omitempty"`
 	StorageClass string `json:"storage_class,omitempty"`
-	PartSize     int64  `json:"partsize,omitempty"`
+	// The buffer size (in MB) to use for multipart uploads. The minimum allowed part size is 5MB,
+	// and if this value is set to zero, the default value (5MB) for the AWS SDK will be used.
+	// The minimum allowed value is 5.
+	// Please note that if the upload bandwidth between the SFTP client and SFTPGo is greater than the
+	// upload bandwidth between SFTPGo and S3 then the SFTP client will idle wait for the uploads of
+	// the last parts, and it could timeout. Keep this in mind if you customize these parameters.
+	UploadPartSize int64 `json:"upload_part_size,omitempty"`
 }
 
 // S3Fs is a Fs implementation for Amazon S3 compatible object storage.
@@ -82,10 +88,10 @@ func NewS3Fs(connectionID, localTempDir string, config S3FsConfig) (Fs, error) {
 		awsConfig.S3ForcePathStyle = aws.Bool(true)
 	}
 
-	if fs.config.PartSize == 0 {
-		fs.config.PartSize = s3manager.DefaultUploadPartSize
+	if fs.config.UploadPartSize == 0 {
+		fs.config.UploadPartSize = s3manager.DefaultUploadPartSize
 	} else {
-		fs.config.PartSize *= 1024 * 1024
+		fs.config.UploadPartSize *= 1024 * 1024
 	}
 
 	sessOpts := session.Options{
@@ -208,10 +214,10 @@ func (fs S3Fs) Create(name string, flag int) (*os.File, *pipeat.PipeWriterAt, fu
 			StorageClass: utils.NilIfEmpty(fs.config.StorageClass),
 		}, func(u *s3manager.Uploader) {
 			u.Concurrency = 2
-			u.PartSize = fs.config.PartSize
+			u.PartSize = fs.config.UploadPartSize
 		})
 		r.CloseWithError(err)
-		fsLog(fs, logger.LevelDebug, "upload completed, path: %#v, response: %v, readed bytes: %v, err: %v",
+		fsLog(fs, logger.LevelDebug, "upload completed, path: %#v, response: %v, readed bytes: %v, err: %+v",
 			name, response, r.GetReadedBytes(), err)
 		metrics.S3TransferCompleted(r.GetReadedBytes(), 0, err)
 	}()

diff --git a/vfs/vfs.go b/vfs/vfs.go
@@ -103,8 +103,8 @@ func ValidateS3FsConfig(config *S3FsConfig) error {
 			config.KeyPrefix += "/"
 		}
 	}
-	if config.PartSize != 0 && config.PartSize < 5 {
-		return errors.New("part_size ret cannot be lower than 5MB")
+	if config.UploadPartSize != 0 && config.UploadPartSize < 5 {
+		return errors.New("upload_part_size cannot be != 0 and lower than 5 (MB)")
 	}
 	return nil
 }