From c9ddb461ea7aea95bf684f5e518a4d9015915e6c Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Thu, 16 Jul 2020 12:00:13 +0300 Subject: [PATCH 01/22] command/{app, cp}, storage/{s3, s3_test}: added cross-region transfer support --- .github/workflows/ci.yml | 9 +------ CHANGELOG.md | 3 +++ command/app.go | 13 +++++++++- command/cp.go | 6 +++++ storage/s3.go | 48 ++++++++++++++++++++++++++++--------- storage/s3_test.go | 52 ++++++++++++++++++++++++++++++++++++++++ 6 files changed, 111 insertions(+), 20 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1b2cc8600..640b3ea69 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -41,14 +41,7 @@ jobs: - run: make test qa: - strategy: - matrix: - os: - - macos - - ubuntu - - windows - - runs-on: ${{ matrix.os }}-latest + runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 diff --git a/CHANGELOG.md b/CHANGELOG.md index a2f90e2a2..cb0f2301c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## not released yet +#### Features +- Added cross-region transfer support using `source-region` and `region` flags on `s5cmd`. It can +be used with cp/mv operations where source and destination buckets reside in different regions. ([#155](https://github.com/peak/s5cmd/issues/155)) #### Bugfixes diff --git a/command/app.go b/command/app.go index 25184a4af..e31515bbb 100644 --- a/command/app.go +++ b/command/app.go @@ -55,6 +55,14 @@ var app = &cli.App{ Name: "install-completion", Usage: "install completion for your shell", }, + &cli.StringFlag{ + Name: "source-region", + Usage: "connect to a specific region of the remote object storage service", + }, + &cli.StringFlag{ + Name: "region", + Usage: "region of the destination bucket for cp/mv operations; default is source-region", + }, }, Before: func(c *cli.Context) error { noVerifySSL := c.Bool("no-verify-ssl") @@ -63,6 +71,8 @@ var app = &cli.App{ workerCount := c.Int("numworkers") printJSON := c.Bool("json") logLevel := c.String("log") + srcRegion := c.String("source-region") + dstRegion := c.String("region") log.Init(logLevel, printJSON) parallel.Init(workerCount) @@ -75,9 +85,10 @@ var app = &cli.App{ MaxRetries: retryCount, Endpoint: endpointURL, NoVerifySSL: noVerifySSL, + Region: srcRegion, } - return storage.Init(s3opts) + return storage.Init(s3opts, dstRegion) }, Action: func(c *cli.Context) error { if c.Bool("install-completion") { diff --git a/command/cp.go b/command/cp.go index 7c648d1fa..ccb96e8d3 100644 --- a/command/cp.go +++ b/command/cp.go @@ -64,6 +64,12 @@ Examples: 10. Mirror an S3 prefix to target S3 prefix > s5cmd {{.HelpName}} -n -s -u s3://bucket/source-prefix/* s3://bucket/target-prefix/ + + 13. Copy S3 objects to another bucket in different region with default source region + > s5cmd -region eu-east-2 {{.HelpName}} s3://bucket/object s3://target-bucket/prefix/object + + 14. Copy S3 objects to another bucket in different region with explicitly provided source region + > s5cmd -source-region us-west-1 -region eu-east-2 {{.HelpName}} s3://bucket/object s3://target-bucket/prefix/object ` var copyCommandFlags = []cli.Flag{ diff --git a/storage/s3.go b/storage/s3.go index 729549b76..0631b009d 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -47,8 +47,8 @@ const ( var cachedS3 *S3 // Init creates a new global S3 session. -func Init(opts S3Options) error { - s3, err := NewS3Storage(opts) +func Init(opts S3Options, destinationRegion string) error { + s3, err := NewS3Storage(opts, destinationRegion) cachedS3 = s3 return err } @@ -56,10 +56,11 @@ func Init(opts S3Options) error { // S3 is a storage type which interacts with S3API, DownloaderAPI and // UploaderAPI. type S3 struct { - api s3iface.S3API - downloader s3manageriface.DownloaderAPI - uploader s3manageriface.UploaderAPI - endpointURL urlpkg.URL + api s3iface.S3API + downloader s3manageriface.DownloaderAPI + uploader s3manageriface.UploaderAPI + endpointURL urlpkg.URL + destinationS3 *S3 } // S3Options stores configuration for S3 storage. @@ -88,7 +89,7 @@ func parseEndpoint(endpoint string) (urlpkg.URL, error) { } // NewS3Storage creates new S3 session. -func NewS3Storage(opts S3Options) (*S3, error) { +func NewS3Storage(opts S3Options, dstRegion string) (*S3, error) { endpointURL, err := parseEndpoint(opts.Endpoint) if err != nil { return nil, err @@ -99,12 +100,30 @@ func NewS3Storage(opts S3Options) (*S3, error) { return nil, err } - return &S3{ + s3Storage := &S3{ api: s3.New(awsSession), downloader: s3manager.NewDownloader(awsSession), uploader: s3manager.NewUploader(awsSession), endpointURL: endpointURL, - }, nil + } + if dstRegion == "" || dstRegion == aws.StringValue(awsSession.Config.Region) { + s3Storage.destinationS3 = s3Storage + return s3Storage, nil + } + + dstOpts := opts + dstOpts.Region = dstRegion + dstSession, err := newSession(dstOpts) + if err != nil { + return nil, err + } + s3Storage.destinationS3 = &S3{ + api: s3.New(dstSession), + downloader: s3manager.NewDownloader(dstSession), + uploader: s3manager.NewUploader(dstSession), + endpointURL: endpointURL, + } + return s3Storage, nil } // Stat retrieves metadata from S3 object without returning the object itself. @@ -311,7 +330,7 @@ func (s *S3) Copy(ctx context.Context, from, to *url.URL, metadata map[string]st input.StorageClass = aws.String(storageClass) } - _, err := s.api.CopyObject(input) + _, err := s.Api().CopyObject(input) return err } @@ -374,7 +393,7 @@ func (s *S3) Put( input.StorageClass = aws.String(storageClass) } - _, err := s.uploader.UploadWithContext(ctx, input, func(u *s3manager.Uploader) { + _, err := s.Uploader().UploadWithContext(ctx, input, func(u *s3manager.Uploader) { u.PartSize = partSize u.Concurrency = concurrency }) @@ -693,3 +712,10 @@ func (a *writeAtAdapter) Write(p []byte) (int, error) { a.offset += int64(n) return n, nil } +func (s *S3) Uploader() s3manageriface.UploaderAPI { + return s.destinationS3.uploader +} + +func (s *S3) Api() s3iface.S3API { + return s.destinationS3.api +} diff --git a/storage/s3_test.go b/storage/s3_test.go index c189422cb..c2396cb4c 100644 --- a/storage/s3_test.go +++ b/storage/s3_test.go @@ -398,3 +398,55 @@ func TestS3Retry(t *testing.T) { }) } } + +func TestInitWithDifferentSourceAndDestinationRegion(t *testing.T) { + + testcases := []struct { + name string + sourceRegion string + destinationRegion string + + expectedSessions int + }{ + { + name: "same source-region and region", + sourceRegion: "eu-west-1", + destinationRegion: "eu-west-1", + + expectedSessions: 1, + }, + { + name: "source-region and region not set", + expectedSessions: 1, + }, + { + name: "region set to default value of source-region", + destinationRegion: "us-east-1", + expectedSessions: 1, + }, + } + for _, tc := range testcases { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + + s3opts := S3Options{ + Region: tc.sourceRegion, + } + s3Storage, err := NewS3Storage(s3opts, tc.destinationRegion) + + if err != nil { + t.Error(err) + } + numOfSessions := 1 + if s3Storage.destinationS3 != s3Storage { + numOfSessions++ + } + + if numOfSessions != tc.expectedSessions { + t.Errorf("Expected %q, got %q", tc.expectedSessions, numOfSessions) + } + }) + } + +} From 424486775b907dfb1adf49d8947646b46d96438e Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Fri, 17 Jul 2020 22:22:45 +0300 Subject: [PATCH 02/22] command/{}, storage/{}, e2e/cp_test: batch run for cross-region transfer --- CHANGELOG.md | 2 +- command/app.go | 21 +++++---- command/cat.go | 7 ++- command/cp.go | 108 +++++++++++++++++++++++++++++++++------------ command/du.go | 5 ++- command/ls.go | 10 ++++- command/mb.go | 5 ++- command/mv.go | 21 +++++++++ command/rm.go | 5 ++- e2e/cp_test.go | 1 - storage/s3.go | 72 ++++++++++++++++++++---------- storage/s3_test.go | 46 +++++++++++++------ storage/storage.go | 16 +++++-- 13 files changed, 235 insertions(+), 84 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cb0f2301c..e22e05a9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ ## not released yet #### Features -- Added cross-region transfer support using `source-region` and `region` flags on `s5cmd`. It can +- Added cross-region transfer support using `source-region` and `region` flags. Ex, it can be used with cp/mv operations where source and destination buckets reside in different regions. ([#155](https://github.com/peak/s5cmd/issues/155)) #### Bugfixes diff --git a/command/app.go b/command/app.go index e31515bbb..d3e4bbafd 100644 --- a/command/app.go +++ b/command/app.go @@ -19,6 +19,10 @@ const ( appName = "s5cmd" ) +// AppStorageOptions will be overridden by inner command flags +// such as if provided `cp -region` will override s5cmd -region flag value. +var AppStorageOptions storage.StorageOptions + var app = &cli.App{ Name: appName, Usage: "Blazing fast S3 and local filesystem execution tool", @@ -61,7 +65,7 @@ var app = &cli.App{ }, &cli.StringFlag{ Name: "region", - Usage: "region of the destination bucket for cp/mv operations; default is source-region", + Usage: "(global) region of the destination bucket for cp/mv operations; default is source-region", }, }, Before: func(c *cli.Context) error { @@ -71,8 +75,6 @@ var app = &cli.App{ workerCount := c.Int("numworkers") printJSON := c.Bool("json") logLevel := c.String("log") - srcRegion := c.String("source-region") - dstRegion := c.String("region") log.Init(logLevel, printJSON) parallel.Init(workerCount) @@ -81,14 +83,15 @@ var app = &cli.App{ return fmt.Errorf("retry count cannot be a negative value") } - s3opts := storage.S3Options{ - MaxRetries: retryCount, - Endpoint: endpointURL, - NoVerifySSL: noVerifySSL, - Region: srcRegion, + AppStorageOptions = storage.StorageOptions{ + MaxRetries: retryCount, + Endpoint: endpointURL, + NoVerifySSL: noVerifySSL, + SourceRegion: c.String("source-region"), + DestinationRegion: c.String("region"), } - return storage.Init(s3opts, dstRegion) + return nil }, Action: func(c *cli.Context) error { if c.Bool("install-completion") { diff --git a/command/cat.go b/command/cat.go index f3c4c47bc..a229a47d1 100644 --- a/command/cat.go +++ b/command/cat.go @@ -67,11 +67,14 @@ var catCommand = &cli.Command{ // Cat prints content of given source to standard output. func Cat(ctx context.Context, src *url.URL) error { - client := storage.NewClient(src) + client, err := storage.NewClient(src, AppStorageOptions) + if err != nil { + return err + } // set concurrency to 1 for sequential write to 'stdout' and give a dummy 'partSize' since // `storage.S3.Get()` ignores 'partSize' if concurrency is set to 1. - _, err := client.Get(ctx, src, sequentialWriterAt{w: os.Stdout}, 1, -1) + _, err = client.Get(ctx, src, sequentialWriterAt{w: os.Stdout}, 1, -1) return err } diff --git a/command/cp.go b/command/cp.go index ccb96e8d3..544edbf8b 100644 --- a/command/cp.go +++ b/command/cp.go @@ -66,10 +66,10 @@ Examples: > s5cmd {{.HelpName}} -n -s -u s3://bucket/source-prefix/* s3://bucket/target-prefix/ 13. Copy S3 objects to another bucket in different region with default source region - > s5cmd -region eu-east-2 {{.HelpName}} s3://bucket/object s3://target-bucket/prefix/object + > s5cmd {{.HelpName}} -region eu-east-2 s3://bucket/object s3://target-bucket/prefix/object 14. Copy S3 objects to another bucket in different region with explicitly provided source region - > s5cmd -source-region us-west-1 -region eu-east-2 {{.HelpName}} s3://bucket/object s3://target-bucket/prefix/object + > s5cmd {{.HelpName}} -source-region us-west-1 -region eu-east-2 s3://bucket/object s3://target-bucket/prefix/object ` var copyCommandFlags = []cli.Flag{ @@ -113,6 +113,14 @@ var copyCommandFlags = []cli.Flag{ Value: defaultPartSize, Usage: "size of each part transferred between host and remote server, in MiB", }, + &cli.StringFlag{ + Name: "source-region", + Usage: "connect to a specific region of the remote object storage service", + }, + &cli.StringFlag{ + Name: "region", + Usage: "region of the destination bucket for cp/mv operations; default is source-region", + }, } var copyCommand = &cli.Command{ @@ -122,9 +130,20 @@ var copyCommand = &cli.Command{ Flags: copyCommandFlags, CustomHelpTemplate: copyHelpTemplate, Before: func(c *cli.Context) error { - return validate(c) + return validate(c, AppStorageOptions) }, Action: func(c *cli.Context) error { + + // get application level values for the flags if not provided + srcRegion := c.String("source-region") + if srcRegion == "" { + srcRegion = AppStorageOptions.SourceRegion + } + dstRegion := c.String("region") + if dstRegion == "" { + dstRegion = AppStorageOptions.DestinationRegion + } + return Copy{ src: c.Args().Get(0), dst: c.Args().Get(1), @@ -138,8 +157,16 @@ var copyCommand = &cli.Command{ flatten: c.Bool("flatten"), followSymlinks: !c.Bool("no-follow-symlinks"), storageClass: storage.StorageClass(c.String("storage-class")), - concurrency: c.Int("concurrency"), - partSize: c.Int64("part-size") * megabytes, + + StorageOptions: storage.StorageOptions{ + Concurrency: c.Int("concurrency"), + PartSize: c.Int64("part-size") * megabytes, + SourceRegion: srcRegion, + DestinationRegion: dstRegion, + MaxRetries: AppStorageOptions.MaxRetries, + NoVerifySSL: AppStorageOptions.NoVerifySSL, + Endpoint: AppStorageOptions.Endpoint, + }, }.Run(c.Context) }, } @@ -162,8 +189,7 @@ type Copy struct { storageClass storage.StorageClass // s3 options - concurrency int - partSize int64 + storage.StorageOptions } const fdlimitWarning = ` @@ -184,7 +210,10 @@ func (c Copy) Run(ctx context.Context) error { return err } - client := storage.NewClient(srcurl) + client, err := storage.NewClient(srcurl, c.StorageOptions) + if err != nil { + return err + } objch, err := expandSource(ctx, client, c.followSymlinks, srcurl) if err != nil { @@ -283,7 +312,7 @@ func (c Copy) prepareDownloadTask( isBatch bool, ) func() error { return func() error { - dsturl, err := prepareLocalDestination(ctx, srcurl, dsturl, c.flatten, isBatch) + dsturl, err := prepareLocalDestination(ctx, srcurl, dsturl, c.flatten, isBatch, c.StorageOptions) if err != nil { return err } @@ -324,10 +353,16 @@ func (c Copy) prepareUploadTask( // doDownload is used to fetch a remote object and save as a local object. func (c Copy) doDownload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) error { - srcClient := storage.NewClient(srcurl) - dstClient := storage.NewClient(dsturl) + srcClient, err := storage.NewClient(srcurl, c.StorageOptions) + if err != nil { + return err + } + dstClient, err := storage.NewClient(dsturl, c.StorageOptions) + if err != nil { + return err + } - err := c.shouldOverride(ctx, srcurl, dsturl) + err = c.shouldOverride(ctx, srcurl, dsturl) if err != nil { // FIXME(ig): rename if errorpkg.IsWarning(err) { @@ -343,7 +378,7 @@ func (c Copy) doDownload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) } defer f.Close() - size, err := srcClient.Get(ctx, srcurl, f, c.concurrency, c.partSize) + size, err := srcClient.Get(ctx, srcurl, f, c.Concurrency, c.PartSize) if err != nil { _ = dstClient.Delete(ctx, dsturl) return err @@ -383,19 +418,25 @@ func (c Copy) doUpload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) er return err } - dstClient := storage.NewClient(dsturl) + dstClient, err := storage.NewClient(dsturl, c.StorageOptions) + if err != nil { + return err + } metadata := map[string]string{ "StorageClass": string(c.storageClass), "ContentType": guessContentType(f), } - err = dstClient.Put(ctx, f, dsturl, metadata, c.concurrency, c.partSize) + err = dstClient.Put(ctx, f, dsturl, metadata, c.Concurrency, c.PartSize) if err != nil { return err } - srcClient := storage.NewClient(srcurl) + srcClient, err := storage.NewClient(srcurl, c.StorageOptions) + if err != nil { + return err + } obj, _ := srcClient.Stat(ctx, srcurl) size := obj.Size @@ -423,13 +464,16 @@ func (c Copy) doUpload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) er } func (c Copy) doCopy(ctx context.Context, srcurl *url.URL, dsturl *url.URL) error { - srcClient := storage.NewClient(srcurl) + srcClient, err := storage.NewClient(srcurl, c.StorageOptions) + if err != nil { + return err + } metadata := map[string]string{ "StorageClass": string(c.storageClass), } - err := c.shouldOverride(ctx, srcurl, dsturl) + err = c.shouldOverride(ctx, srcurl, dsturl) if err != nil { if errorpkg.IsWarning(err) { printDebug(c.op, srcurl, dsturl, err) @@ -474,12 +518,12 @@ func (c Copy) shouldOverride(ctx context.Context, srcurl *url.URL, dsturl *url.U return nil } - srcObj, err := getObject(ctx, srcurl) + srcObj, err := getObject(ctx, srcurl, c.StorageOptions) if err != nil { return err } - dstObj, err := getObject(ctx, dsturl) + dstObj, err := getObject(ctx, dsturl, c.StorageOptions) if err != nil { return err } @@ -542,6 +586,7 @@ func prepareLocalDestination( dsturl *url.URL, flatten bool, isBatch bool, + storageOpts storage.StorageOptions, ) (*url.URL, error) { objname := srcurl.Base() if isBatch && !flatten { @@ -554,7 +599,10 @@ func prepareLocalDestination( } } - client := storage.NewClient(dsturl) + client, err := storage.NewClient(dsturl, storageOpts) + if err != nil { + return nil, err + } obj, err := client.Stat(ctx, dsturl) if err != nil && err != storage.ErrGivenObjectNotFound { @@ -586,8 +634,11 @@ func prepareLocalDestination( // getObject checks if the object from given url exists. If no object is // found, error and returning object would be nil. -func getObject(ctx context.Context, url *url.URL) (*storage.Object, error) { - client := storage.NewClient(url) +func getObject(ctx context.Context, url *url.URL, storageOpts storage.StorageOptions) (*storage.Object, error) { + client, err := storage.NewClient(url, storageOpts) + if err != nil { + return nil, err + } obj, err := client.Stat(ctx, url) if err == storage.ErrGivenObjectNotFound { @@ -597,7 +648,7 @@ func getObject(ctx context.Context, url *url.URL) (*storage.Object, error) { return obj, err } -func validate(c *cli.Context) error { +func validate(c *cli.Context, storageOpts storage.StorageOptions) error { if c.Args().Len() != 2 { return fmt.Errorf("expected source and destination arguments") } @@ -636,7 +687,7 @@ func validate(c *cli.Context) error { case srcurl.Type == dsturl.Type: return validateCopy(srcurl, dsturl) case dsturl.IsRemote(): - return validateUpload(ctx, srcurl, dsturl) + return validateUpload(ctx, srcurl, dsturl, storageOpts) default: return nil } @@ -651,8 +702,11 @@ func validateCopy(srcurl, dsturl *url.URL) error { return fmt.Errorf("local->local copy operations are not permitted") } -func validateUpload(ctx context.Context, srcurl, dsturl *url.URL) error { - srcclient := storage.NewClient(srcurl) +func validateUpload(ctx context.Context, srcurl, dsturl *url.URL, storageOpts storage.StorageOptions) error { + srcclient, err := storage.NewClient(srcurl, storageOpts) + if err != nil { + return err + } if srcurl.HasGlob() { return nil diff --git a/command/du.go b/command/du.go index 5d4191ae1..3c834dbef 100644 --- a/command/du.go +++ b/command/du.go @@ -79,7 +79,10 @@ func Size( return err } - client := storage.NewClient(srcurl) + client, err := storage.NewClient(srcurl, AppStorageOptions) + if err != nil { + return err + } storageTotal := map[string]sizeAndCount{} total := sizeAndCount{} diff --git a/command/ls.go b/command/ls.go index 8d4c0f930..e5e9d8fad 100644 --- a/command/ls.go +++ b/command/ls.go @@ -81,7 +81,10 @@ var listCommand = &cli.Command{ func ListBuckets(ctx context.Context) error { // set as remote storage url := &url.URL{Type: 0} - client := storage.NewClient(url) + client, err := storage.NewClient(url, AppStorageOptions) + if err != nil { + return err + } buckets, err := client.ListBuckets(ctx, "") if err != nil { @@ -108,7 +111,10 @@ func List( return err } - client := storage.NewClient(srcurl) + client, err := storage.NewClient(srcurl, AppStorageOptions) + if err != nil { + return err + } var merror error diff --git a/command/mb.go b/command/mb.go index 6395347cf..32f583ef7 100644 --- a/command/mb.go +++ b/command/mb.go @@ -66,7 +66,10 @@ func MakeBucket( return err } - client := storage.NewClient(bucket) + client, err := storage.NewClient(bucket, AppStorageOptions) + if err != nil { + return err + } err = client.MakeBucket(ctx, bucket.Bucket) if err != nil { diff --git a/command/mv.go b/command/mv.go index 88c492494..cfbe39afb 100644 --- a/command/mv.go +++ b/command/mv.go @@ -41,6 +41,17 @@ var moveCommand = &cli.Command{ return copyCommand.Before(c) }, Action: func(c *cli.Context) error { + + // get application level values for the flags if not provided + srcRegion := c.String("source-region") + if srcRegion == "" { + srcRegion = AppStorageOptions.SourceRegion + } + dstRegion := c.String("region") + if dstRegion == "" { + dstRegion = AppStorageOptions.DestinationRegion + } + copyCommand := Copy{ src: c.Args().Get(0), dst: c.Args().Get(1), @@ -53,6 +64,16 @@ var moveCommand = &cli.Command{ ifSourceNewer: c.Bool("if-source-newer"), flatten: c.Bool("flatten"), storageClass: storage.StorageClass(c.String("storage-class")), + + StorageOptions: storage.StorageOptions{ + Concurrency: c.Int("concurrency"), + PartSize: c.Int64("part-size") * megabytes, + SourceRegion: srcRegion, + DestinationRegion: dstRegion, + MaxRetries: AppStorageOptions.MaxRetries, + NoVerifySSL: AppStorageOptions.NoVerifySSL, + Endpoint: AppStorageOptions.Endpoint, + }, } return copyCommand.Run(c.Context) diff --git a/command/rm.go b/command/rm.go index 2b195ddf0..f7c021390 100644 --- a/command/rm.go +++ b/command/rm.go @@ -71,7 +71,10 @@ func Delete( } srcurl := srcurls[0] - client := storage.NewClient(srcurl) + client, err := storage.NewClient(srcurl, AppStorageOptions) + if err != nil { + return err + } objChan := expandSources(ctx, client, false, srcurls...) diff --git a/e2e/cp_test.go b/e2e/cp_test.go index fe077e2a9..a7d06a4b0 100644 --- a/e2e/cp_test.go +++ b/e2e/cp_test.go @@ -838,7 +838,6 @@ func TestCopyDirBackslashedToS3(t *testing.T) { result.Assert(t, icmd.Success) - assertLines(t, result.Stdout(), map[int]compareFunc{ 0: equals(`cp %v/readme.md %vreadme.md`, srcpath, dstpath), 1: equals(`cp %v/t\est/filetest.txt %vt\est/filetest.txt`, srcpath, dstpath), diff --git a/storage/s3.go b/storage/s3.go index 0631b009d..976a909e7 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -31,6 +31,11 @@ var _ Storage = (*S3)(nil) var sentinelURL = urlpkg.URL{} +var ( + mutex = &sync.Mutex{} + mapS3optsSession = map[S3Options]*session.Session{} +) + const ( // deleteObjectsMax is the max allowed objects to be deleted on single HTTP // request. @@ -43,16 +48,6 @@ const ( gcsEndpoint = "storage.googleapis.com" ) -// Re-used AWS sessions dramatically improve performance. -var cachedS3 *S3 - -// Init creates a new global S3 session. -func Init(opts S3Options, destinationRegion string) error { - s3, err := NewS3Storage(opts, destinationRegion) - cachedS3 = s3 - return err -} - // S3 is a storage type which interacts with S3API, DownloaderAPI and // UploaderAPI. type S3 struct { @@ -89,13 +84,18 @@ func parseEndpoint(endpoint string) (urlpkg.URL, error) { } // NewS3Storage creates new S3 session. -func NewS3Storage(opts S3Options, dstRegion string) (*S3, error) { +func NewS3Storage(opts StorageOptions) (*S3, error) { endpointURL, err := parseEndpoint(opts.Endpoint) if err != nil { return nil, err } - awsSession, err := newSession(opts) + awsSession, err := newSession(S3Options{ + Region: opts.DestinationRegion, + Endpoint: opts.Endpoint, + MaxRetries: opts.MaxRetries, + NoVerifySSL: opts.NoVerifySSL, + }) if err != nil { return nil, err } @@ -106,24 +106,27 @@ func NewS3Storage(opts S3Options, dstRegion string) (*S3, error) { uploader: s3manager.NewUploader(awsSession), endpointURL: endpointURL, } - if dstRegion == "" || dstRegion == aws.StringValue(awsSession.Config.Region) { + if opts.SourceRegion == "" || opts.SourceRegion == aws.StringValue(awsSession.Config.Region) { s3Storage.destinationS3 = s3Storage return s3Storage, nil } - dstOpts := opts - dstOpts.Region = dstRegion - dstSession, err := newSession(dstOpts) + dstSession, err := newSession(S3Options{ + Region: opts.SourceRegion, + Endpoint: opts.Endpoint, + MaxRetries: opts.MaxRetries, + NoVerifySSL: opts.NoVerifySSL, + }) if err != nil { return nil, err } - s3Storage.destinationS3 = &S3{ - api: s3.New(dstSession), - downloader: s3manager.NewDownloader(dstSession), - uploader: s3manager.NewUploader(dstSession), - endpointURL: endpointURL, - } - return s3Storage, nil + return &S3{ + api: s3.New(dstSession), + downloader: s3manager.NewDownloader(dstSession), + uploader: s3manager.NewUploader(dstSession), + endpointURL: endpointURL, + destinationS3: s3Storage, + }, nil } // Stat retrieves metadata from S3 object without returning the object itself. @@ -561,6 +564,13 @@ func (s *S3) MakeBucket(ctx context.Context, name string) error { // NewAwsSession initializes a new AWS session with region fallback and custom // options. func newSession(opts S3Options) (*session.Session, error) { + mutex.Lock() + defer mutex.Unlock() + + if sess, ok := mapS3optsSession[opts]; ok { + return sess, nil + } + awsCfg := aws.NewConfig() endpointURL, err := parseEndpoint(opts.Endpoint) @@ -623,9 +633,25 @@ func newSession(opts S3Options) (*session.Session, error) { sess.Config.Region = aws.String(endpoints.UsEast1RegionID) } + mapS3optsSession[opts] = sess + return sess, nil } +// NumOfSessions returns number of sessions currently active. +func NumOfSessions() int { + mutex.Lock() + defer mutex.Unlock() + return len(mapS3optsSession) +} + +// RemoveAllSessions clears all existing sessions. +func RemoveAllSessions() { + mutex.Lock() + defer mutex.Unlock() + mapS3optsSession = map[S3Options]*session.Session{} +} + // customRetryer wraps the SDK's built in DefaultRetryer adding additional // error codes. Such as, retry for S3 InternalError code. type customRetryer struct { diff --git a/storage/s3_test.go b/storage/s3_test.go index c2396cb4c..cc036aeb8 100644 --- a/storage/s3_test.go +++ b/storage/s3_test.go @@ -14,6 +14,7 @@ import ( "github.com/aws/aws-sdk-go/awstesting/unit" "github.com/aws/aws-sdk-go/service/s3" "github.com/google/go-cmp/cmp" + "gotest.tools/v3/assert" "github.com/peak/s5cmd/storage/url" ) @@ -76,6 +77,10 @@ func TestNewSessionWithRegionSetViaEnv(t *testing.T) { const expectedRegion = "us-west-2" + if NumOfSessions() > 0 { + RemoveAllSessions() + } + os.Setenv("AWS_REGION", expectedRegion) defer os.Unsetenv("AWS_REGION") @@ -399,8 +404,10 @@ func TestS3Retry(t *testing.T) { } } -func TestInitWithDifferentSourceAndDestinationRegion(t *testing.T) { +func TestNumOfSessions(t *testing.T) { + RemoveAllSessions() + const defaultRegion = "us-east-1" testcases := []struct { name string sourceRegion string @@ -408,6 +415,13 @@ func TestInitWithDifferentSourceAndDestinationRegion(t *testing.T) { expectedSessions int }{ + { + name: "different source-region and region", + sourceRegion: "cn-north-1", + destinationRegion: "eu-central-1", + + expectedSessions: 2, + }, { name: "same source-region and region", sourceRegion: "eu-west-1", @@ -421,32 +435,38 @@ func TestInitWithDifferentSourceAndDestinationRegion(t *testing.T) { }, { name: "region set to default value of source-region", - destinationRegion: "us-east-1", + destinationRegion: defaultRegion, expectedSessions: 1, }, } + const expectedTotalNumOfSessions = 5 for _, tc := range testcases { tc := tc t.Run(tc.name, func(t *testing.T) { + defer RemoveAllSessions() - s3opts := S3Options{ - Region: tc.sourceRegion, + storageOpts := StorageOptions{ + SourceRegion: tc.sourceRegion, + DestinationRegion: tc.destinationRegion, } - s3Storage, err := NewS3Storage(s3opts, tc.destinationRegion) + _, err := NewS3Storage(storageOpts) if err != nil { t.Error(err) } - numOfSessions := 1 - if s3Storage.destinationS3 != s3Storage { - numOfSessions++ - } - if numOfSessions != tc.expectedSessions { - t.Errorf("Expected %q, got %q", tc.expectedSessions, numOfSessions) - } + assert.Equal(t, NumOfSessions(), tc.expectedSessions) }) } - + for _, tc := range testcases { + _, err := NewS3Storage(StorageOptions{ + SourceRegion: tc.sourceRegion, + DestinationRegion: tc.destinationRegion, + }) + if err != nil { + t.Error(err) + } + } + assert.Equal(t, NumOfSessions(), expectedTotalNumOfSessions) } diff --git a/storage/storage.go b/storage/storage.go index f8e9845aa..01c8a3545 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -57,12 +57,12 @@ type Storage interface { // NewClient returns new Storage client from given url. Storage implementation // is inferred from the url. -func NewClient(url *url.URL) Storage { +func NewClient(url *url.URL, storageOpts StorageOptions) (Storage, error) { if url.IsRemote() { - return cachedS3 + return NewS3Storage(storageOpts) } - return NewFilesystem() + return NewFilesystem(), nil } // Object is a generic type which contains metadata for storage items. @@ -199,3 +199,13 @@ type notImplemented struct { func (e notImplemented) Error() string { return fmt.Sprintf("%q is not supported on %q storage", e.method, e.apiType) } + +type StorageOptions struct { + NoVerifySSL bool + Concurrency int + MaxRetries int + PartSize int64 + SourceRegion string + DestinationRegion string + Endpoint string +} From b50bed95516ba5faf3bb16076e05ae86a1f0892d Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Sat, 25 Jul 2020 00:56:58 +0300 Subject: [PATCH 03/22] command/, storage/: updates based on pr review --- command/app.go | 23 ---------------- command/cat.go | 6 ++-- command/cp.go | 67 ++++++++++++++++++--------------------------- command/du.go | 4 ++- command/ls.go | 10 ++++--- command/mb.go | 4 ++- command/mv.go | 25 +++-------------- command/rm.go | 4 ++- command/run.go | 11 ++++++++ storage/s3.go | 68 +++++++++++++++++----------------------------- storage/s3_test.go | 38 ++++++++++---------------- storage/storage.go | 14 ++-------- 12 files changed, 101 insertions(+), 173 deletions(-) diff --git a/command/app.go b/command/app.go index d3e4bbafd..a068ae91c 100644 --- a/command/app.go +++ b/command/app.go @@ -9,7 +9,6 @@ import ( "github.com/peak/s5cmd/log" "github.com/peak/s5cmd/parallel" - "github.com/peak/s5cmd/storage" ) const ( @@ -19,10 +18,6 @@ const ( appName = "s5cmd" ) -// AppStorageOptions will be overridden by inner command flags -// such as if provided `cp -region` will override s5cmd -region flag value. -var AppStorageOptions storage.StorageOptions - var app = &cli.App{ Name: appName, Usage: "Blazing fast S3 and local filesystem execution tool", @@ -59,19 +54,9 @@ var app = &cli.App{ Name: "install-completion", Usage: "install completion for your shell", }, - &cli.StringFlag{ - Name: "source-region", - Usage: "connect to a specific region of the remote object storage service", - }, - &cli.StringFlag{ - Name: "region", - Usage: "(global) region of the destination bucket for cp/mv operations; default is source-region", - }, }, Before: func(c *cli.Context) error { - noVerifySSL := c.Bool("no-verify-ssl") retryCount := c.Int("retry-count") - endpointURL := c.String("endpoint-url") workerCount := c.Int("numworkers") printJSON := c.Bool("json") logLevel := c.String("log") @@ -83,14 +68,6 @@ var app = &cli.App{ return fmt.Errorf("retry count cannot be a negative value") } - AppStorageOptions = storage.StorageOptions{ - MaxRetries: retryCount, - Endpoint: endpointURL, - NoVerifySSL: noVerifySSL, - SourceRegion: c.String("source-region"), - DestinationRegion: c.String("region"), - } - return nil }, Action: func(c *cli.Context) error { diff --git a/command/cat.go b/command/cat.go index a229a47d1..503b42bcd 100644 --- a/command/cat.go +++ b/command/cat.go @@ -61,13 +61,13 @@ var catCommand = &cli.Command{ return err } - return Cat(c.Context, src) + return Cat(c.Context, src, storage.NewS3Options(c, true)) }, } // Cat prints content of given source to standard output. -func Cat(ctx context.Context, src *url.URL) error { - client, err := storage.NewClient(src, AppStorageOptions) +func Cat(ctx context.Context, src *url.URL, s3Opts storage.S3Options) error { + client, err := storage.NewClient(src, s3Opts) if err != nil { return err } diff --git a/command/cp.go b/command/cp.go index ec29870bc..23b4bb98e 100644 --- a/command/cp.go +++ b/command/cp.go @@ -148,20 +148,9 @@ var copyCommand = &cli.Command{ Flags: copyCommandFlags, CustomHelpTemplate: copyHelpTemplate, Before: func(c *cli.Context) error { - return validate(c, AppStorageOptions) + return validate(c, storage.NewS3Options(c, true)) }, Action: func(c *cli.Context) error { - - // get application level values for the flags if not provided - srcRegion := c.String("source-region") - if srcRegion == "" { - srcRegion = AppStorageOptions.SourceRegion - } - dstRegion := c.String("region") - if dstRegion == "" { - dstRegion = AppStorageOptions.DestinationRegion - } - return Copy{ src: c.Args().Get(0), dst: c.Args().Get(1), @@ -179,15 +168,8 @@ var copyCommand = &cli.Command{ encryptionKeyID: c.String("sse-kms-key-id"), acl: c.String("acl"), - StorageOptions: storage.StorageOptions{ - Concurrency: c.Int("concurrency"), - PartSize: c.Int64("part-size") * megabytes, - SourceRegion: srcRegion, - DestinationRegion: dstRegion, - MaxRetries: AppStorageOptions.MaxRetries, - NoVerifySSL: AppStorageOptions.NoVerifySSL, - Endpoint: AppStorageOptions.Endpoint, - }, + srcS3opts: storage.NewS3Options(c, true), + dstS3opts: storage.NewS3Options(c, false), }.Run(c.Context) }, } @@ -213,7 +195,10 @@ type Copy struct { acl string // s3 options - storage.StorageOptions + concurrency int + partSize int64 + srcS3opts storage.S3Options + dstS3opts storage.S3Options } const fdlimitWarning = ` @@ -234,7 +219,7 @@ func (c Copy) Run(ctx context.Context) error { return err } - client, err := storage.NewClient(srcurl, c.StorageOptions) + client, err := storage.NewClient(srcurl, c.srcS3opts) if err != nil { return err } @@ -336,7 +321,7 @@ func (c Copy) prepareDownloadTask( isBatch bool, ) func() error { return func() error { - dsturl, err := prepareLocalDestination(ctx, srcurl, dsturl, c.flatten, isBatch, c.StorageOptions) + dsturl, err := prepareLocalDestination(ctx, srcurl, dsturl, c.flatten, isBatch, c.dstS3opts) if err != nil { return err } @@ -377,11 +362,11 @@ func (c Copy) prepareUploadTask( // doDownload is used to fetch a remote object and save as a local object. func (c Copy) doDownload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) error { - srcClient, err := storage.NewClient(srcurl, c.StorageOptions) + srcClient, err := storage.NewClient(srcurl, c.srcS3opts) if err != nil { return err } - dstClient, err := storage.NewClient(dsturl, c.StorageOptions) + dstClient, err := storage.NewClient(dsturl, c.dstS3opts) if err != nil { return err } @@ -402,7 +387,7 @@ func (c Copy) doDownload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) } defer f.Close() - size, err := srcClient.Get(ctx, srcurl, f, c.Concurrency, c.PartSize) + size, err := srcClient.Get(ctx, srcurl, f, c.concurrency, c.partSize) if err != nil { _ = dstClient.Delete(ctx, dsturl) return err @@ -442,7 +427,7 @@ func (c Copy) doUpload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) er return err } - dstClient, err := storage.NewClient(dsturl, c.StorageOptions) + dstClient, err := storage.NewClient(dsturl, c.dstS3opts) if err != nil { return err } @@ -454,12 +439,12 @@ func (c Copy) doUpload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) er SetSSEKeyID(c.encryptionKeyID). SetACL(c.acl) - err = dstClient.Put(ctx, f, dsturl, metadata, c.Concurrency, c.PartSize) + err = dstClient.Put(ctx, f, dsturl, metadata, c.concurrency, c.partSize) if err != nil { return err } - srcClient, err := storage.NewClient(srcurl, c.StorageOptions) + srcClient, err := storage.NewClient(srcurl, c.srcS3opts) if err != nil { return err } @@ -490,7 +475,7 @@ func (c Copy) doUpload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) er } func (c Copy) doCopy(ctx context.Context, srcurl *url.URL, dsturl *url.URL) error { - srcClient, err := storage.NewClient(srcurl, c.StorageOptions) + srcClient, err := storage.NewClient(srcurl, c.srcS3opts) if err != nil { return err } @@ -546,12 +531,12 @@ func (c Copy) shouldOverride(ctx context.Context, srcurl *url.URL, dsturl *url.U return nil } - srcObj, err := getObject(ctx, srcurl, c.StorageOptions) + srcObj, err := getObject(ctx, srcurl, c.srcS3opts) if err != nil { return err } - dstObj, err := getObject(ctx, dsturl, c.StorageOptions) + dstObj, err := getObject(ctx, dsturl, c.dstS3opts) if err != nil { return err } @@ -614,7 +599,7 @@ func prepareLocalDestination( dsturl *url.URL, flatten bool, isBatch bool, - storageOpts storage.StorageOptions, + s3opts storage.S3Options, ) (*url.URL, error) { objname := srcurl.Base() if isBatch && !flatten { @@ -627,7 +612,7 @@ func prepareLocalDestination( } } - client, err := storage.NewClient(dsturl, storageOpts) + client, err := storage.NewClient(dsturl, s3opts) if err != nil { return nil, err } @@ -662,8 +647,8 @@ func prepareLocalDestination( // getObject checks if the object from given url exists. If no object is // found, error and returning object would be nil. -func getObject(ctx context.Context, url *url.URL, storageOpts storage.StorageOptions) (*storage.Object, error) { - client, err := storage.NewClient(url, storageOpts) +func getObject(ctx context.Context, url *url.URL, s3opts storage.S3Options) (*storage.Object, error) { + client, err := storage.NewClient(url, s3opts) if err != nil { return nil, err } @@ -676,7 +661,7 @@ func getObject(ctx context.Context, url *url.URL, storageOpts storage.StorageOpt return obj, err } -func validate(c *cli.Context, storageOpts storage.StorageOptions) error { +func validate(c *cli.Context, srcS3opts storage.S3Options) error { if c.Args().Len() != 2 { return fmt.Errorf("expected source and destination arguments") } @@ -715,7 +700,7 @@ func validate(c *cli.Context, storageOpts storage.StorageOptions) error { case srcurl.Type == dsturl.Type: return validateCopy(srcurl, dsturl) case dsturl.IsRemote(): - return validateUpload(ctx, srcurl, dsturl, storageOpts) + return validateUpload(ctx, srcurl, dsturl, srcS3opts) default: return nil } @@ -730,8 +715,8 @@ func validateCopy(srcurl, dsturl *url.URL) error { return fmt.Errorf("local->local copy operations are not permitted") } -func validateUpload(ctx context.Context, srcurl, dsturl *url.URL, storageOpts storage.StorageOptions) error { - srcclient, err := storage.NewClient(srcurl, storageOpts) +func validateUpload(ctx context.Context, srcurl, dsturl *url.URL, srcS3opts storage.S3Options) error { + srcclient, err := storage.NewClient(srcurl, srcS3opts) if err != nil { return err } diff --git a/command/du.go b/command/du.go index 3c834dbef..ba5b46f60 100644 --- a/command/du.go +++ b/command/du.go @@ -63,6 +63,7 @@ var sizeCommand = &cli.Command{ c.Args().First(), groupByClass, humanize, + storage.NewS3Options(c, true), ) }, } @@ -73,13 +74,14 @@ func Size( src string, groupByClass bool, humanize bool, + s3opts storage.S3Options, ) error { srcurl, err := url.New(src) if err != nil { return err } - client, err := storage.NewClient(srcurl, AppStorageOptions) + client, err := storage.NewClient(srcurl, s3opts) if err != nil { return err } diff --git a/command/ls.go b/command/ls.go index e7efa03a7..7cb0583f8 100644 --- a/command/ls.go +++ b/command/ls.go @@ -67,7 +67,7 @@ var listCommand = &cli.Command{ }, Action: func(c *cli.Context) error { if !c.Args().Present() { - return ListBuckets(c.Context) + return ListBuckets(c.Context, storage.NewS3Options(c, true)) } showEtag := c.Bool("etag") @@ -80,15 +80,16 @@ var listCommand = &cli.Command{ showEtag, humanize, showStorageClass, + storage.NewS3Options(c, true), ) }, } // ListBuckets prints all buckets. -func ListBuckets(ctx context.Context) error { +func ListBuckets(ctx context.Context, s3opts storage.S3Options) error { // set as remote storage url := &url.URL{Type: 0} - client, err := storage.NewClient(url, AppStorageOptions) + client, err := storage.NewClient(url, s3opts) if err != nil { return err } @@ -113,13 +114,14 @@ func List( showEtag bool, humanize bool, showStorageClass bool, + s3opts storage.S3Options, ) error { srcurl, err := url.New(src) if err != nil { return err } - client, err := storage.NewClient(srcurl, AppStorageOptions) + client, err := storage.NewClient(srcurl, s3opts) if err != nil { return err } diff --git a/command/mb.go b/command/mb.go index 32f583ef7..de9437600 100644 --- a/command/mb.go +++ b/command/mb.go @@ -51,6 +51,7 @@ var makeBucketCommand = &cli.Command{ c.Context, c.Command.Name, c.Args().First(), + storage.NewS3Options(c, true), ) }, } @@ -60,13 +61,14 @@ func MakeBucket( ctx context.Context, op string, src string, + s3opts storage.S3Options, ) error { bucket, err := url.New(src) if err != nil { return err } - client, err := storage.NewClient(bucket, AppStorageOptions) + client, err := storage.NewClient(bucket, s3opts) if err != nil { return err } diff --git a/command/mv.go b/command/mv.go index 76179b033..27d63a8fa 100644 --- a/command/mv.go +++ b/command/mv.go @@ -41,42 +41,25 @@ var moveCommand = &cli.Command{ return copyCommand.Before(c) }, Action: func(c *cli.Context) error { - - // get application level values for the flags if not provided - srcRegion := c.String("source-region") - if srcRegion == "" { - srcRegion = AppStorageOptions.SourceRegion - } - dstRegion := c.String("region") - if dstRegion == "" { - dstRegion = AppStorageOptions.DestinationRegion - } - copyCommand := Copy{ src: c.Args().Get(0), dst: c.Args().Get(1), op: c.Command.Name, fullCommand: givenCommand(c), - deleteSource: true, // delete source + deleteSource: true, // don't delete source // flags noClobber: c.Bool("no-clobber"), ifSizeDiffer: c.Bool("if-size-differ"), ifSourceNewer: c.Bool("if-source-newer"), flatten: c.Bool("flatten"), + followSymlinks: !c.Bool("no-follow-symlinks"), storageClass: storage.StorageClass(c.String("storage-class")), encryptionMethod: c.String("sse"), encryptionKeyID: c.String("sse-kms-key-id"), acl: c.String("acl"), - StorageOptions: storage.StorageOptions{ - Concurrency: c.Int("concurrency"), - PartSize: c.Int64("part-size") * megabytes, - SourceRegion: srcRegion, - DestinationRegion: dstRegion, - MaxRetries: AppStorageOptions.MaxRetries, - NoVerifySSL: AppStorageOptions.NoVerifySSL, - Endpoint: AppStorageOptions.Endpoint, - }, + srcS3opts: storage.NewS3Options(c, true), + dstS3opts: storage.NewS3Options(c, false), } return copyCommand.Run(c.Context) diff --git a/command/rm.go b/command/rm.go index f7c021390..10431ec8d 100644 --- a/command/rm.go +++ b/command/rm.go @@ -53,6 +53,7 @@ var deleteCommand = &cli.Command{ c.Context, c.Command.Name, givenCommand(c), + storage.NewS3Options(c, true), c.Args().Slice()..., ) }, @@ -63,6 +64,7 @@ func Delete( ctx context.Context, op string, fullCommand string, + s3opts storage.S3Options, src ...string, ) error { srcurls, err := newURLs(src...) @@ -71,7 +73,7 @@ func Delete( } srcurl := srcurls[0] - client, err := storage.NewClient(srcurl, AppStorageOptions) + client, err := storage.NewClient(srcurl, s3opts) if err != nil { return err } diff --git a/command/run.go b/command/run.go index 3afaf03f0..ae97346da 100644 --- a/command/run.go +++ b/command/run.go @@ -31,11 +31,22 @@ Examples: 2. Read commands from standard input and execute in parallel. > cat commands.txt | s5cmd {{.HelpName}} ` +var runCommandFlags = []cli.Flag{ + &cli.StringFlag{ + Name: "source-region", + Usage: "[default] connect to a specific region of the remote object storage service", + }, + &cli.StringFlag{ + Name: "region", + Usage: "[default] region of the destination bucket for cp/mv operations; defaulted to source-region", + }, +} var runCommand = &cli.Command{ Name: "run", HelpName: "run", Usage: "run commands in batch", + Flags: runCommandFlags, // to be passed to children subcommands CustomHelpTemplate: runHelpTemplate, Before: func(c *cli.Context) error { if c.Args().Len() > 1 { diff --git a/storage/s3.go b/storage/s3.go index fa8adf5bf..67da0d7bc 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -24,6 +24,7 @@ import ( "github.com/aws/aws-sdk-go/service/s3/s3iface" "github.com/aws/aws-sdk-go/service/s3/s3manager" "github.com/aws/aws-sdk-go/service/s3/s3manager/s3manageriface" + "github.com/urfave/cli/v2" "github.com/peak/s5cmd/storage/url" ) @@ -52,11 +53,10 @@ const ( // S3 is a storage type which interacts with S3API, DownloaderAPI and // UploaderAPI. type S3 struct { - api s3iface.S3API - downloader s3manageriface.DownloaderAPI - uploader s3manageriface.UploaderAPI - endpointURL urlpkg.URL - destinationS3 *S3 + api s3iface.S3API + downloader s3manageriface.DownloaderAPI + uploader s3manageriface.UploaderAPI + endpointURL urlpkg.URL } // S3Options stores configuration for S3 storage. @@ -67,6 +67,21 @@ type S3Options struct { NoVerifySSL bool } +// NewS3Options returns new S3Options object by extracting +// its fields from the provided context. +func NewS3Options(c *cli.Context, isSrc bool) S3Options { + region := c.String("source-region") + if !isSrc && c.String("region") != "" { + region = c.String("region") + } + return S3Options{ + MaxRetries: c.Int("retry-count"), + Endpoint: c.String("endpoint-url"), + NoVerifySSL: c.Bool("no-verify-ssl"), + Region: region, + } +} + func parseEndpoint(endpoint string) (urlpkg.URL, error) { if endpoint == "" { return sentinelURL, nil @@ -85,48 +100,22 @@ func parseEndpoint(endpoint string) (urlpkg.URL, error) { } // NewS3Storage creates new S3 session. -func NewS3Storage(opts StorageOptions) (*S3, error) { +func NewS3Storage(opts S3Options) (*S3, error) { endpointURL, err := parseEndpoint(opts.Endpoint) if err != nil { return nil, err } - awsSession, err := newSession(S3Options{ - Region: opts.DestinationRegion, - Endpoint: opts.Endpoint, - MaxRetries: opts.MaxRetries, - NoVerifySSL: opts.NoVerifySSL, - }) + awsSession, err := newSession(opts) if err != nil { return nil, err } - s3Storage := &S3{ + return &S3{ api: s3.New(awsSession), downloader: s3manager.NewDownloader(awsSession), uploader: s3manager.NewUploader(awsSession), endpointURL: endpointURL, - } - if opts.SourceRegion == "" || opts.SourceRegion == aws.StringValue(awsSession.Config.Region) { - s3Storage.destinationS3 = s3Storage - return s3Storage, nil - } - - dstSession, err := newSession(S3Options{ - Region: opts.SourceRegion, - Endpoint: opts.Endpoint, - MaxRetries: opts.MaxRetries, - NoVerifySSL: opts.NoVerifySSL, - }) - if err != nil { - return nil, err - } - return &S3{ - api: s3.New(dstSession), - downloader: s3manager.NewDownloader(dstSession), - uploader: s3manager.NewUploader(dstSession), - endpointURL: endpointURL, - destinationS3: s3Storage, }, nil } @@ -368,7 +357,7 @@ func (s *S3) Copy(ctx context.Context, from, to *url.URL, metadata Metadata) err input.ACL = aws.String(acl) } - _, err := s.Api().CopyObject(input) + _, err := s.api.CopyObject(input) return err } @@ -444,7 +433,7 @@ func (s *S3) Put( } } - _, err := s.Uploader().UploadWithContext(ctx, input, func(u *s3manager.Uploader) { + _, err := s.uploader.UploadWithContext(ctx, input, func(u *s3manager.Uploader) { u.PartSize = partSize u.Concurrency = concurrency }) @@ -806,10 +795,3 @@ func (a *writeAtAdapter) Write(p []byte) (int, error) { a.offset += int64(n) return n, nil } -func (s *S3) Uploader() s3manageriface.UploaderAPI { - return s.destinationS3.uploader -} - -func (s *S3) Api() s3iface.S3API { - return s.destinationS3.api -} diff --git a/storage/s3_test.go b/storage/s3_test.go index 3089acb08..7c5628961 100644 --- a/storage/s3_test.go +++ b/storage/s3_test.go @@ -420,7 +420,6 @@ func TestS3Retry(t *testing.T) { func TestNumOfSessions(t *testing.T) { RemoveAllSessions() - const defaultRegion = "us-east-1" testcases := []struct { name string sourceRegion string @@ -446,39 +445,34 @@ func TestNumOfSessions(t *testing.T) { name: "source-region and region not set", expectedSessions: 1, }, - { - name: "region set to default value of source-region", - destinationRegion: defaultRegion, - expectedSessions: 1, - }, } - const expectedTotalNumOfSessions = 5 + const expectedTotalNumOfSessions = 4 for _, tc := range testcases { tc := tc t.Run(tc.name, func(t *testing.T) { defer RemoveAllSessions() - storageOpts := StorageOptions{ - SourceRegion: tc.sourceRegion, - DestinationRegion: tc.destinationRegion, - } - _, err := NewS3Storage(storageOpts) - - if err != nil { - t.Error(err) + for _, r := range []string{tc.sourceRegion, tc.destinationRegion} { + _, err := NewS3Storage(S3Options{ + Region: r, + }) + if err != nil { + t.Error(err) + } } assert.Equal(t, NumOfSessions(), tc.expectedSessions) }) } for _, tc := range testcases { - _, err := NewS3Storage(StorageOptions{ - SourceRegion: tc.sourceRegion, - DestinationRegion: tc.destinationRegion, - }) - if err != nil { - t.Error(err) + for _, r := range []string{tc.sourceRegion, tc.destinationRegion} { + _, err := NewS3Storage(S3Options{ + Region: r, + }) + if err != nil { + t.Error(err) + } } } assert.Equal(t, NumOfSessions(), expectedTotalNumOfSessions) @@ -584,7 +578,6 @@ func TestS3CopyEncryptionRequest(t *testing.T) { mockS3 := &S3{ api: mockApi, } - mockS3.destinationS3 = mockS3 metadata := NewMetadata().SetSSE(tc.sse).SetSSEKeyID(tc.sseKeyID).SetACL(tc.acl) @@ -678,7 +671,6 @@ func TestS3PutEncryptionRequest(t *testing.T) { mockS3 := &S3{ uploader: s3manager.NewUploaderWithClient(mockApi), } - mockS3.destinationS3 = mockS3 metadata := NewMetadata().SetSSE(tc.sse).SetSSEKeyID(tc.sseKeyID).SetACL(tc.acl) diff --git a/storage/storage.go b/storage/storage.go index bbedc0d39..8924fa12a 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -57,9 +57,9 @@ type Storage interface { // NewClient returns new Storage client from given url. Storage implementation // is inferred from the url. -func NewClient(url *url.URL, storageOpts StorageOptions) (Storage, error) { +func NewClient(url *url.URL, opts S3Options) (Storage, error) { if url.IsRemote() { - return NewS3Storage(storageOpts) + return NewS3Storage(opts) } return NewFilesystem(), nil @@ -176,16 +176,6 @@ func (e notImplemented) Error() string { return fmt.Sprintf("%q is not supported on %q storage", e.method, e.apiType) } -type StorageOptions struct { - NoVerifySSL bool - Concurrency int - MaxRetries int - PartSize int64 - SourceRegion string - DestinationRegion string - Endpoint string -} - type Metadata map[string]string // NewMetadata will return an empty metadata object. From 7d1d18f504b559b9e7ff81a58366d015f0295f51 Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Sat, 25 Jul 2020 11:56:22 +0300 Subject: [PATCH 04/22] storage/{s3, s3_test}: updates based on pr review --- storage/s3.go | 51 ++++++++++++++++++++++++++++------------------ storage/s3_test.go | 10 ++++----- 2 files changed, 36 insertions(+), 25 deletions(-) diff --git a/storage/s3.go b/storage/s3.go index 67da0d7bc..fd0a5e31c 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -33,10 +33,7 @@ var _ Storage = (*S3)(nil) var sentinelURL = urlpkg.URL{} -var ( - mutex = &sync.Mutex{} - mapS3optsSession = map[S3Options]*session.Session{} -) +var allSessions = &Session{sessions: map[S3Options]*session.Session{}} const ( // deleteObjectsMax is the max allowed objects to be deleted on single HTTP @@ -106,7 +103,7 @@ func NewS3Storage(opts S3Options) (*S3, error) { return nil, err } - awsSession, err := newSession(opts) + awsSession, err := Sessions().newSession(opts) if err != nil { return nil, err } @@ -598,13 +595,25 @@ func (s *S3) MakeBucket(ctx context.Context, name string) error { return err } +// Session holds session.Session according to s3Opts +// and it synchronizes access/modification. +type Session struct { + sync.Mutex + sessions map[S3Options]*session.Session +} + +// Sessions returns allSessions singleton. +func Sessions() *Session { + return allSessions +} + // NewAwsSession initializes a new AWS session with region fallback and custom // options. -func newSession(opts S3Options) (*session.Session, error) { - mutex.Lock() - defer mutex.Unlock() +func (s *Session) newSession(opts S3Options) (*session.Session, error) { + s.Lock() + defer s.Unlock() - if sess, ok := mapS3optsSession[opts]; ok { + if sess, ok := s.sessions[opts]; ok { return sess, nil } @@ -670,23 +679,25 @@ func newSession(opts S3Options) (*session.Session, error) { sess.Config.Region = aws.String(endpoints.UsEast1RegionID) } - mapS3optsSession[opts] = sess + s.sessions[opts] = sess return sess, nil } // NumOfSessions returns number of sessions currently active. func NumOfSessions() int { - mutex.Lock() - defer mutex.Unlock() - return len(mapS3optsSession) -} - -// RemoveAllSessions clears all existing sessions. -func RemoveAllSessions() { - mutex.Lock() - defer mutex.Unlock() - mapS3optsSession = map[S3Options]*session.Session{} + s := Sessions() + s.Lock() + defer s.Unlock() + return len(s.sessions) +} + +// ClearSessions clears all existing sessions. +func ClearSessions() { + s := Sessions() + s.Lock() + defer s.Unlock() + s.sessions = map[S3Options]*session.Session{} } // customRetryer wraps the SDK's built in DefaultRetryer adding additional diff --git a/storage/s3_test.go b/storage/s3_test.go index 7c5628961..98c2d3756 100644 --- a/storage/s3_test.go +++ b/storage/s3_test.go @@ -66,7 +66,7 @@ func TestNewSessionPathStyle(t *testing.T) { t.Run(tc.name, func(t *testing.T) { opts := S3Options{Endpoint: tc.endpoint.Hostname()} - sess, err := newSession(opts) + sess, err := Sessions().newSession(opts) if err != nil { t.Fatal(err) } @@ -87,13 +87,13 @@ func TestNewSessionWithRegionSetViaEnv(t *testing.T) { const expectedRegion = "us-west-2" if NumOfSessions() > 0 { - RemoveAllSessions() + ClearSessions() } os.Setenv("AWS_REGION", expectedRegion) defer os.Unsetenv("AWS_REGION") - sess, err := newSession(opts) + sess, err := Sessions().newSession(opts) if err != nil { t.Fatal(err) } @@ -418,7 +418,7 @@ func TestS3Retry(t *testing.T) { } func TestNumOfSessions(t *testing.T) { - RemoveAllSessions() + ClearSessions() testcases := []struct { name string @@ -451,7 +451,7 @@ func TestNumOfSessions(t *testing.T) { tc := tc t.Run(tc.name, func(t *testing.T) { - defer RemoveAllSessions() + defer ClearSessions() for _, r := range []string{tc.sourceRegion, tc.destinationRegion} { _, err := NewS3Storage(S3Options{ From 4ad98e0f970d30909641a32c46ac8e556af8faa6 Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Tue, 28 Jul 2020 19:57:43 +0300 Subject: [PATCH 05/22] command/, readme: updates based on pr review --- README.md | 14 ++++++++++ command/app.go | 16 ++++++++++++ command/cat.go | 2 +- command/cp.go | 13 +++++++--- command/du.go | 2 +- command/ls.go | 4 +-- command/mb.go | 2 +- command/mv.go | 4 +-- command/rm.go | 2 +- e2e/cp_test.go | 56 ++++++++++++++++++++++++++++++++++++++++ e2e/run_test.go | 57 +++++++++++++++++++++++++++++++++++++++++ e2e/util_test.go | 66 +++++++++++++++++++++++++++++++++--------------- storage/s3.go | 27 ++++---------------- 13 files changed, 210 insertions(+), 55 deletions(-) diff --git a/README.md b/README.md index a787fc23c..b45b2e86d 100644 --- a/README.md +++ b/README.md @@ -179,6 +179,12 @@ they'll be deleted in a single request. Will copy all the matching objects to the given S3 prefix, respecting the source folder hierarchy. +If source and destination buckets reside on different regions: + + s5cmd cp --source-region=us-east-2 --region=eu-central-1 s3://bucket1/file.txt s3://bucket2/ + +`--region` flag is for the bucket destination region, if not set, it will default to `--source-region` + ⚠️ Copying objects (from S3 to S3) larger than 5GB is not supported yet. We have an [open ticket](https://github.com/peak/s5cmd/issues/29) to track the issue. @@ -219,6 +225,14 @@ mv s3://bucket/2020/03/18/file1.gz s3://bucket/2020/03/18/original/file.gz ls # inline comments are OK too ``` +For batch operations it is possible to set a generic source and destination regions for buckets. For instance, + + s5cmd run --source-region= --region= commands.txt + +For each subcommand in the `commands.txt` file, source and destionation regions will +default to the flags set in the `run` command; +subcommands can overwrite these global flags by providing of their own. + ### Specifying credentials `s5cmd` uses official AWS SDK to access S3. SDK requires credentials to sign diff --git a/command/app.go b/command/app.go index a068ae91c..4c4ce6b89 100644 --- a/command/app.go +++ b/command/app.go @@ -4,6 +4,7 @@ import ( "context" "fmt" + "github.com/peak/s5cmd/storage" cmpinstall "github.com/posener/complete/cmd/install" "github.com/urfave/cli/v2" @@ -113,3 +114,18 @@ func Main(ctx context.Context, args []string) error { return app.RunContext(ctx, args) } + +// s3opts returns new S3Options object by extracting +// its fields from the provided context. +func s3opts(c *cli.Context, isSrc bool) storage.S3Options { + region := c.String("source-region") + if !isSrc && c.String("region") != "" { + region = c.String("region") + } + return storage.S3Options{ + MaxRetries: c.Int("retry-count"), + Endpoint: c.String("endpoint-url"), + NoVerifySSL: c.Bool("no-verify-ssl"), + Region: region, + } +} diff --git a/command/cat.go b/command/cat.go index 503b42bcd..fbf80317d 100644 --- a/command/cat.go +++ b/command/cat.go @@ -61,7 +61,7 @@ var catCommand = &cli.Command{ return err } - return Cat(c.Context, src, storage.NewS3Options(c, true)) + return Cat(c.Context, src, s3opts(c, true)) }, } diff --git a/command/cp.go b/command/cp.go index 23b4bb98e..0a56be1fb 100644 --- a/command/cp.go +++ b/command/cp.go @@ -148,7 +148,7 @@ var copyCommand = &cli.Command{ Flags: copyCommandFlags, CustomHelpTemplate: copyHelpTemplate, Before: func(c *cli.Context) error { - return validate(c, storage.NewS3Options(c, true)) + return validate(c, s3opts(c, true)) }, Action: func(c *cli.Context) error { return Copy{ @@ -168,8 +168,8 @@ var copyCommand = &cli.Command{ encryptionKeyID: c.String("sse-kms-key-id"), acl: c.String("acl"), - srcS3opts: storage.NewS3Options(c, true), - dstS3opts: storage.NewS3Options(c, false), + srcS3opts: s3opts(c, true), + dstS3opts: s3opts(c, false), }.Run(c.Context) }, } @@ -480,6 +480,11 @@ func (c Copy) doCopy(ctx context.Context, srcurl *url.URL, dsturl *url.URL) erro return err } + dstClient, err := storage.NewClient(dsturl, c.dstS3opts) + if err != nil { + return err + } + metadata := storage.NewMetadata(). SetStorageClass(string(c.storageClass)). SetSSE(c.encryptionMethod). @@ -495,7 +500,7 @@ func (c Copy) doCopy(ctx context.Context, srcurl *url.URL, dsturl *url.URL) erro return err } - err = srcClient.Copy(ctx, srcurl, dsturl, metadata) + err = dstClient.Copy(ctx, srcurl, dsturl, metadata) if err != nil { return err } diff --git a/command/du.go b/command/du.go index ba5b46f60..ad443cb4a 100644 --- a/command/du.go +++ b/command/du.go @@ -63,7 +63,7 @@ var sizeCommand = &cli.Command{ c.Args().First(), groupByClass, humanize, - storage.NewS3Options(c, true), + s3opts(c, true), ) }, } diff --git a/command/ls.go b/command/ls.go index 7cb0583f8..c957f7f01 100644 --- a/command/ls.go +++ b/command/ls.go @@ -67,7 +67,7 @@ var listCommand = &cli.Command{ }, Action: func(c *cli.Context) error { if !c.Args().Present() { - return ListBuckets(c.Context, storage.NewS3Options(c, true)) + return ListBuckets(c.Context, s3opts(c, true)) } showEtag := c.Bool("etag") @@ -80,7 +80,7 @@ var listCommand = &cli.Command{ showEtag, humanize, showStorageClass, - storage.NewS3Options(c, true), + s3opts(c, true), ) }, } diff --git a/command/mb.go b/command/mb.go index de9437600..a74b78c6c 100644 --- a/command/mb.go +++ b/command/mb.go @@ -51,7 +51,7 @@ var makeBucketCommand = &cli.Command{ c.Context, c.Command.Name, c.Args().First(), - storage.NewS3Options(c, true), + s3opts(c, true), ) }, } diff --git a/command/mv.go b/command/mv.go index 27d63a8fa..f6140a648 100644 --- a/command/mv.go +++ b/command/mv.go @@ -58,8 +58,8 @@ var moveCommand = &cli.Command{ encryptionKeyID: c.String("sse-kms-key-id"), acl: c.String("acl"), - srcS3opts: storage.NewS3Options(c, true), - dstS3opts: storage.NewS3Options(c, false), + srcS3opts: s3opts(c, true), + dstS3opts: s3opts(c, false), } return copyCommand.Run(c.Context) diff --git a/command/rm.go b/command/rm.go index 10431ec8d..ea1bafda4 100644 --- a/command/rm.go +++ b/command/rm.go @@ -53,7 +53,7 @@ var deleteCommand = &cli.Command{ c.Context, c.Command.Name, givenCommand(c), - storage.NewS3Options(c, true), + s3opts(c, true), c.Args().Slice()..., ) }, diff --git a/e2e/cp_test.go b/e2e/cp_test.go index a7d06a4b0..d3251c0e8 100644 --- a/e2e/cp_test.go +++ b/e2e/cp_test.go @@ -29,6 +29,8 @@ import ( "testing" "time" + "github.com/peak/s5cmd/storage" + "gotest.tools/v3/assert" "gotest.tools/v3/fs" "gotest.tools/v3/icmd" @@ -1438,6 +1440,60 @@ func TestCopySingleS3ObjectToS3(t *testing.T) { assert.Assert(t, ensureS3Object(s3client, bucket, dstfilename, content)) } +// cp --source-region=us-east-2 --region=us-west-2 s3://bucket/object s3://bucket/object2 +func TestCopySingleS3ObjectToS3WithDifferentSourceAndDestinationRegion(t *testing.T) { + t.Parallel() + + srcBucket := randomString(30) + dstBucket := randomString(30) + + endpoint, workdir, cleanup := server(t, "bolt") + defer cleanup() + + s5cmd := s5cmd(workdir, endpoint) + + srcClient := s3client(t, storage.S3Options{ + Endpoint: endpoint, + Region: "us-east-2", + NoVerifySSL: true, + }) + + dstClient := s3client(t, storage.S3Options{ + Endpoint: endpoint, + Region: "us-west-2", + NoVerifySSL: true, + }) + + createBucket(t, srcClient, srcBucket) + createBucket(t, dstClient, dstBucket) + + const ( + filename = "testfile1.txt" + dstfilename = "copy_" + filename + content = "this is a file content" + ) + + putFile(t, srcClient, srcBucket, filename, content) + + src := fmt.Sprintf("s3://%v/%v", srcBucket, filename) + dst := fmt.Sprintf("s3://%v/%v", dstBucket, dstfilename) + + cmd := s5cmd("cp", "--source-region=us-east-2", "--region=us-west-2", src, dst) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Success) + + assertLines(t, result.Stdout(), map[int]compareFunc{ + 0: equals(`cp %v %v`, src, dst), + }) + + // assert s3 source object + assert.Assert(t, ensureS3Object(srcClient, srcBucket, filename, content)) + + // assert s3 destination object + assert.Assert(t, ensureS3Object(dstClient, dstBucket, dstfilename, content)) +} + // --json cp s3://bucket/object s3://bucket2/object func TestCopySingleS3ObjectToS3JSON(t *testing.T) { t.Parallel() diff --git a/e2e/run_test.go b/e2e/run_test.go index 750deb505..3db6dbdf9 100644 --- a/e2e/run_test.go +++ b/e2e/run_test.go @@ -6,6 +6,9 @@ import ( "testing" "time" + "github.com/aws/aws-sdk-go/service/s3" + "github.com/peak/s5cmd/storage" + "gotest.tools/v3/fs" "gotest.tools/v3/icmd" ) @@ -232,3 +235,57 @@ func TestRunSpecialCharactersInPrefix(t *testing.T) { assertLines(t, result.Stderr(), map[int]compareFunc{}) } + +func TestRunFromFileWithMultipleSourceAndDestinationRegions(t *testing.T) { + t.Parallel() + + const numOfRegions = 4 + + var buckets [numOfRegions]string + for i := range buckets { + buckets[i] = randomString(30) + } + + endpoint, workdir, cleanup := server(t, "bolt") + defer cleanup() + + s5cmd := s5cmd(workdir, endpoint) + + regions := [numOfRegions]string{"us-east-2", "eu-east-1", "eu-central-2", "us-west-1"} + clients := [5]*s3.S3{} + for i, r := range regions { + clients[i] = s3client(t, storage.S3Options{ + Endpoint: endpoint, + Region: r, + NoVerifySSL: true, + }) + } + for i := 0; i < numOfRegions; i++ { + createBucket(t, clients[i], buckets[i]) + + filename := fmt.Sprintf("file%d.txt", i) + putFile(t, clients[i], buckets[i], filename, "content") + } + + filecontent := strings.Join([]string{ + fmt.Sprintf("ls s3://%v/file0.txt", buckets[0]), + fmt.Sprintf("cp s3://%v/file1.txt s3://%v/", buckets[1], buckets[2]), + fmt.Sprintf("mv s3://%v/file2.txt s3://%v/", buckets[2], buckets[3]), + }, "\n") + + file := fs.NewFile(t, "prefix", fs.WithContent(filecontent)) + defer file.Remove() + + cmd := s5cmd("run", "--source-region=us-east-2", file.Path()) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Success) + + assertLines(t, result.Stdout(), map[int]compareFunc{ + 0: suffix("file0.txt"), + 1: equals("cp s3://%v/file1.txt s3://%v/file1.txt", buckets[1], buckets[2]), + 2: equals("mv s3://%v/file2.txt s3://%v/file2.txt", buckets[2], buckets[3]), + }, sortInput(true)) + + assertLines(t, result.Stderr(), map[int]compareFunc{}) +} diff --git a/e2e/util_test.go b/e2e/util_test.go index 8f0b3abb1..5480fe121 100644 --- a/e2e/util_test.go +++ b/e2e/util_test.go @@ -21,6 +21,8 @@ import ( "testing" "time" + "github.com/peak/s5cmd/storage" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/aws/credentials" @@ -73,6 +75,21 @@ func setup(t *testing.T, options ...option) (*s3.S3, func(...string) icmd.Cmd, f for _, option := range options { option(opts) } + + endpoint, workdir, cleanup := server(t, opts.s3backend) + + client := s3client(t, storage.S3Options{ + Endpoint: endpoint, + Region: "us-east-1", + NoVerifySSL: true, + }) + + return client, s5cmd(workdir, endpoint), cleanup +} + +func server(t *testing.T, s3backend string) (string, string, func()) { + t.Helper() + // testdir := fs.NewDir() tries to create a new directory which // has a prefix = [test function name][operation name] // e.g., prefix' = "TestCopySingleS3ObjectToLocal/cp_s3://bucket/object_file" @@ -86,25 +103,35 @@ func setup(t *testing.T, options ...option) (*s3.S3, func(...string) icmd.Cmd, f testdir := fs.NewDir(t, prefix, fs.WithDir("workdir", fs.WithMode(0700))) workdir := testdir.Join("workdir") - var ( - s3LogLevel = *flagTestLogLevel - awsLogLevel = aws.LogOff - ) + s3LogLevel := *flagTestLogLevel - switch *flagTestLogLevel { - case "debug": - s3LogLevel = "info" - // aws has no level other than 'debug' - awsLogLevel = aws.LogDebug + if *flagTestLogLevel == "debug" { + s3LogLevel = "info" // aws has no level other than 'debug' } - endpoint, dbcleanup := s3ServerEndpoint(t, testdir, s3LogLevel, opts.s3backend) + endpoint, dbcleanup := s3ServerEndpoint(t, testdir, s3LogLevel, s3backend) + + cleanup := func() { + testdir.Remove() + dbcleanup() + } + + return endpoint, workdir, cleanup +} + +func s3client(t *testing.T, options storage.S3Options) *s3.S3 { + t.Helper() + + awsLogLevel := aws.LogOff + if *flagTestLogLevel == "debug" { + awsLogLevel = aws.LogDebug + } s3Config := aws.NewConfig(). - WithEndpoint(endpoint). - WithRegion("us-east-1"). + WithEndpoint(options.Endpoint). + WithRegion(options.Region). WithCredentials(credentials.NewStaticCredentials(defaultAccessKeyID, defaultSecretAccessKey, "")). - WithDisableSSL(true). + WithDisableSSL(options.NoVerifySSL). WithS3ForcePathStyle(true). WithCredentialsChainVerboseErrors(true). WithLogLevel(awsLogLevel) @@ -112,7 +139,11 @@ func setup(t *testing.T, options ...option) (*s3.S3, func(...string) icmd.Cmd, f sess, err := session.NewSession(s3Config) assert.NilError(t, err) - s5cmd := func(args ...string) icmd.Cmd { + return s3.New(sess) +} + +func s5cmd(workdir, endpoint string) func(args ...string) icmd.Cmd { + return func(args ...string) icmd.Cmd { endpoint := []string{"--endpoint-url", endpoint} args = append(endpoint, args...) @@ -129,13 +160,6 @@ func setup(t *testing.T, options ...option) (*s3.S3, func(...string) icmd.Cmd, f cmd.Dir = workdir return cmd } - - cleanup := func() { - testdir.Remove() - dbcleanup() - } - - return s3.New(sess), s5cmd, cleanup } func goBuildS5cmd() func() { diff --git a/storage/s3.go b/storage/s3.go index fd0a5e31c..8f56f1b30 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -24,8 +24,6 @@ import ( "github.com/aws/aws-sdk-go/service/s3/s3iface" "github.com/aws/aws-sdk-go/service/s3/s3manager" "github.com/aws/aws-sdk-go/service/s3/s3manager/s3manageriface" - "github.com/urfave/cli/v2" - "github.com/peak/s5cmd/storage/url" ) @@ -33,7 +31,7 @@ var _ Storage = (*S3)(nil) var sentinelURL = urlpkg.URL{} -var allSessions = &Session{sessions: map[S3Options]*session.Session{}} +var allSessions = &s3Session{sessions: map[S3Options]*session.Session{}} const ( // deleteObjectsMax is the max allowed objects to be deleted on single HTTP @@ -64,21 +62,6 @@ type S3Options struct { NoVerifySSL bool } -// NewS3Options returns new S3Options object by extracting -// its fields from the provided context. -func NewS3Options(c *cli.Context, isSrc bool) S3Options { - region := c.String("source-region") - if !isSrc && c.String("region") != "" { - region = c.String("region") - } - return S3Options{ - MaxRetries: c.Int("retry-count"), - Endpoint: c.String("endpoint-url"), - NoVerifySSL: c.Bool("no-verify-ssl"), - Region: region, - } -} - func parseEndpoint(endpoint string) (urlpkg.URL, error) { if endpoint == "" { return sentinelURL, nil @@ -595,21 +578,21 @@ func (s *S3) MakeBucket(ctx context.Context, name string) error { return err } -// Session holds session.Session according to s3Opts +// s3Session holds session.Session according to s3Opts // and it synchronizes access/modification. -type Session struct { +type s3Session struct { sync.Mutex sessions map[S3Options]*session.Session } // Sessions returns allSessions singleton. -func Sessions() *Session { +func Sessions() *s3Session { return allSessions } // NewAwsSession initializes a new AWS session with region fallback and custom // options. -func (s *Session) newSession(opts S3Options) (*session.Session, error) { +func (s *s3Session) newSession(opts S3Options) (*session.Session, error) { s.Lock() defer s.Unlock() From 4ab4450aebf0cba684ca14b8a5236c338c240aee Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Tue, 28 Jul 2020 22:13:23 +0300 Subject: [PATCH 06/22] command/{app, cp, run}: updates based on pr review --- command/app.go | 14 ++++++++++++-- command/cp.go | 2 ++ command/run.go | 10 ++++++---- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/command/app.go b/command/app.go index 4c4ce6b89..45814dff2 100644 --- a/command/app.go +++ b/command/app.go @@ -119,8 +119,18 @@ func Main(ctx context.Context, args []string) error { // its fields from the provided context. func s3opts(c *cli.Context, isSrc bool) storage.S3Options { region := c.String("source-region") - if !isSrc && c.String("region") != "" { - region = c.String("region") + if region == "" { + region = c.String("default-source-region") + } + + if !isSrc { + dstRegion := c.String("region") + if dstRegion == "" { + region = c.String("default-region") + } + if dstRegion != "" { + region = dstRegion + } } return storage.S3Options{ MaxRetries: c.Int("retry-count"), diff --git a/command/cp.go b/command/cp.go index 0a56be1fb..0f3c7b5f1 100644 --- a/command/cp.go +++ b/command/cp.go @@ -164,6 +164,8 @@ var copyCommand = &cli.Command{ flatten: c.Bool("flatten"), followSymlinks: !c.Bool("no-follow-symlinks"), storageClass: storage.StorageClass(c.String("storage-class")), + concurrency: c.Int("concurrency"), + partSize: c.Int64("part-size") * megabytes, encryptionMethod: c.String("sse"), encryptionKeyID: c.String("sse-kms-key-id"), acl: c.String("acl"), diff --git a/command/run.go b/command/run.go index ae97346da..6a11d70ad 100644 --- a/command/run.go +++ b/command/run.go @@ -33,12 +33,14 @@ Examples: ` var runCommandFlags = []cli.Flag{ &cli.StringFlag{ - Name: "source-region", - Usage: "[default] connect to a specific region of the remote object storage service", + Name: "default-source-region", + Aliases: []string{"source-region"}, + Usage: "connect to a specific region of the remote object storage service", }, &cli.StringFlag{ - Name: "region", - Usage: "[default] region of the destination bucket for cp/mv operations; defaulted to source-region", + Name: "default-region", + Aliases: []string{"region"}, + Usage: "region of the destination bucket for cp/mv operations; defaulted to source-region", }, } From ab3dfb4cb36e7fd8b60377defd58b5645d8d3deb Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Wed, 29 Jul 2020 09:36:30 +0300 Subject: [PATCH 07/22] command/: updates based on pr review --- command/app.go | 29 ++++++++++++++++++----------- command/cat.go | 2 +- command/cp.go | 8 ++++---- command/du.go | 2 +- command/ls.go | 4 ++-- command/mb.go | 2 +- command/mv.go | 4 ++-- command/rm.go | 2 +- 8 files changed, 30 insertions(+), 23 deletions(-) diff --git a/command/app.go b/command/app.go index 45814dff2..315b1f14c 100644 --- a/command/app.go +++ b/command/app.go @@ -116,26 +116,33 @@ func Main(ctx context.Context, args []string) error { } // s3opts returns new S3Options object by extracting -// its fields from the provided context. -func s3opts(c *cli.Context, isSrc bool) storage.S3Options { +// its fields from the provided context. Region is +// taken as (default) source-region. +func s3opts(c *cli.Context) storage.S3Options { region := c.String("source-region") if region == "" { region = c.String("default-source-region") } + return storage.S3Options{ + MaxRetries: c.Int("retry-count"), + Endpoint: c.String("endpoint-url"), + NoVerifySSL: c.Bool("no-verify-ssl"), + Region: region, + } +} - if !isSrc { - dstRegion := c.String("region") - if dstRegion == "" { - region = c.String("default-region") - } - if dstRegion != "" { - region = dstRegion - } +// dstS3opts returns new S3Options object by extracting +// its fields from the provided context. Region is +// taken as (default) region, i.e., destination region. +func dstS3opts(c *cli.Context) storage.S3Options { + dstRegion := c.String("region") + if dstRegion == "" { + dstRegion = c.String("default-region") } return storage.S3Options{ MaxRetries: c.Int("retry-count"), Endpoint: c.String("endpoint-url"), NoVerifySSL: c.Bool("no-verify-ssl"), - Region: region, + Region: dstRegion, } } diff --git a/command/cat.go b/command/cat.go index fbf80317d..9194fccb0 100644 --- a/command/cat.go +++ b/command/cat.go @@ -61,7 +61,7 @@ var catCommand = &cli.Command{ return err } - return Cat(c.Context, src, s3opts(c, true)) + return Cat(c.Context, src, s3opts(c)) }, } diff --git a/command/cp.go b/command/cp.go index 0f3c7b5f1..ebed5ebc4 100644 --- a/command/cp.go +++ b/command/cp.go @@ -125,7 +125,7 @@ var copyCommandFlags = []cli.Flag{ }, &cli.StringFlag{ Name: "region", - Usage: "region of the destination bucket for cp/mv operations; default is source-region", + Usage: "region of the destination bucket for cp/mv operations", }, &cli.StringFlag{ Name: "sse", @@ -148,7 +148,7 @@ var copyCommand = &cli.Command{ Flags: copyCommandFlags, CustomHelpTemplate: copyHelpTemplate, Before: func(c *cli.Context) error { - return validate(c, s3opts(c, true)) + return validate(c, s3opts(c)) }, Action: func(c *cli.Context) error { return Copy{ @@ -170,8 +170,8 @@ var copyCommand = &cli.Command{ encryptionKeyID: c.String("sse-kms-key-id"), acl: c.String("acl"), - srcS3opts: s3opts(c, true), - dstS3opts: s3opts(c, false), + srcS3opts: s3opts(c), + dstS3opts: dstS3opts(c), }.Run(c.Context) }, } diff --git a/command/du.go b/command/du.go index ad443cb4a..ba4adc708 100644 --- a/command/du.go +++ b/command/du.go @@ -63,7 +63,7 @@ var sizeCommand = &cli.Command{ c.Args().First(), groupByClass, humanize, - s3opts(c, true), + s3opts(c), ) }, } diff --git a/command/ls.go b/command/ls.go index c957f7f01..295c4149e 100644 --- a/command/ls.go +++ b/command/ls.go @@ -67,7 +67,7 @@ var listCommand = &cli.Command{ }, Action: func(c *cli.Context) error { if !c.Args().Present() { - return ListBuckets(c.Context, s3opts(c, true)) + return ListBuckets(c.Context, s3opts(c)) } showEtag := c.Bool("etag") @@ -80,7 +80,7 @@ var listCommand = &cli.Command{ showEtag, humanize, showStorageClass, - s3opts(c, true), + s3opts(c), ) }, } diff --git a/command/mb.go b/command/mb.go index a74b78c6c..3329ca2cc 100644 --- a/command/mb.go +++ b/command/mb.go @@ -51,7 +51,7 @@ var makeBucketCommand = &cli.Command{ c.Context, c.Command.Name, c.Args().First(), - s3opts(c, true), + s3opts(c), ) }, } diff --git a/command/mv.go b/command/mv.go index f6140a648..ccd286cc3 100644 --- a/command/mv.go +++ b/command/mv.go @@ -58,8 +58,8 @@ var moveCommand = &cli.Command{ encryptionKeyID: c.String("sse-kms-key-id"), acl: c.String("acl"), - srcS3opts: s3opts(c, true), - dstS3opts: s3opts(c, false), + srcS3opts: s3opts(c), + dstS3opts: dstS3opts(c), } return copyCommand.Run(c.Context) diff --git a/command/rm.go b/command/rm.go index ea1bafda4..4ce644ca4 100644 --- a/command/rm.go +++ b/command/rm.go @@ -53,7 +53,7 @@ var deleteCommand = &cli.Command{ c.Context, c.Command.Name, givenCommand(c), - s3opts(c, true), + s3opts(c), c.Args().Slice()..., ) }, From cb13391d431ec310c2ebe4b27eb283183aad02f5 Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Wed, 29 Jul 2020 09:55:28 +0300 Subject: [PATCH 08/22] storage/{s3, s3_test}: updates based on pr review --- storage/s3.go | 13 ++++--------- storage/s3_test.go | 4 ++-- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/storage/s3.go b/storage/s3.go index 627641f10..f3e1e3a52 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -31,7 +31,7 @@ var _ Storage = (*S3)(nil) var sentinelURL = urlpkg.URL{} -var allSessions = &s3Session{sessions: map[S3Options]*session.Session{}} +var sessions = &s3Session{sessions: map[S3Options]*session.Session{}} const ( // deleteObjectsMax is the max allowed objects to be deleted on single HTTP @@ -86,7 +86,7 @@ func NewS3Storage(opts S3Options) (*S3, error) { return nil, err } - awsSession, err := Sessions().newSession(opts) + awsSession, err := sessions.newSession(opts) if err != nil { return nil, err } @@ -591,11 +591,6 @@ type s3Session struct { sessions map[S3Options]*session.Session } -// Sessions returns allSessions singleton. -func Sessions() *s3Session { - return allSessions -} - // NewAwsSession initializes a new AWS session with region fallback and custom // options. func (s *s3Session) newSession(opts S3Options) (*session.Session, error) { @@ -675,7 +670,7 @@ func (s *s3Session) newSession(opts S3Options) (*session.Session, error) { // NumOfSessions returns number of sessions currently active. func NumOfSessions() int { - s := Sessions() + s := sessions s.Lock() defer s.Unlock() return len(s.sessions) @@ -683,7 +678,7 @@ func NumOfSessions() int { // ClearSessions clears all existing sessions. func ClearSessions() { - s := Sessions() + s := sessions s.Lock() defer s.Unlock() s.sessions = map[S3Options]*session.Session{} diff --git a/storage/s3_test.go b/storage/s3_test.go index 98c2d3756..37600aa5d 100644 --- a/storage/s3_test.go +++ b/storage/s3_test.go @@ -66,7 +66,7 @@ func TestNewSessionPathStyle(t *testing.T) { t.Run(tc.name, func(t *testing.T) { opts := S3Options{Endpoint: tc.endpoint.Hostname()} - sess, err := Sessions().newSession(opts) + sess, err := sessions.newSession(opts) if err != nil { t.Fatal(err) } @@ -93,7 +93,7 @@ func TestNewSessionWithRegionSetViaEnv(t *testing.T) { os.Setenv("AWS_REGION", expectedRegion) defer os.Unsetenv("AWS_REGION") - sess, err := Sessions().newSession(opts) + sess, err := sessions.newSession(opts) if err != nil { t.Fatal(err) } From 377ed32bcb1bb4f9dd1e9a71e74e09bb82b349fc Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Wed, 29 Jul 2020 15:00:42 +0300 Subject: [PATCH 09/22] command/{app, cp, options}, storage/: updates based on pr review --- command/app.go | 33 ------------------ command/cp.go | 9 +++-- command/options.go | 38 ++++++++++++++++++++ storage/s3.go | 16 --------- storage/s3_test.go | 87 ---------------------------------------------- 5 files changed, 42 insertions(+), 141 deletions(-) create mode 100644 command/options.go diff --git a/command/app.go b/command/app.go index 315b1f14c..a068ae91c 100644 --- a/command/app.go +++ b/command/app.go @@ -4,7 +4,6 @@ import ( "context" "fmt" - "github.com/peak/s5cmd/storage" cmpinstall "github.com/posener/complete/cmd/install" "github.com/urfave/cli/v2" @@ -114,35 +113,3 @@ func Main(ctx context.Context, args []string) error { return app.RunContext(ctx, args) } - -// s3opts returns new S3Options object by extracting -// its fields from the provided context. Region is -// taken as (default) source-region. -func s3opts(c *cli.Context) storage.S3Options { - region := c.String("source-region") - if region == "" { - region = c.String("default-source-region") - } - return storage.S3Options{ - MaxRetries: c.Int("retry-count"), - Endpoint: c.String("endpoint-url"), - NoVerifySSL: c.Bool("no-verify-ssl"), - Region: region, - } -} - -// dstS3opts returns new S3Options object by extracting -// its fields from the provided context. Region is -// taken as (default) region, i.e., destination region. -func dstS3opts(c *cli.Context) storage.S3Options { - dstRegion := c.String("region") - if dstRegion == "" { - dstRegion = c.String("default-region") - } - return storage.S3Options{ - MaxRetries: c.Int("retry-count"), - Endpoint: c.String("endpoint-url"), - NoVerifySSL: c.Bool("no-verify-ssl"), - Region: dstRegion, - } -} diff --git a/command/cp.go b/command/cp.go index ebed5ebc4..83f50874f 100644 --- a/command/cp.go +++ b/command/cp.go @@ -477,11 +477,6 @@ func (c Copy) doUpload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) er } func (c Copy) doCopy(ctx context.Context, srcurl *url.URL, dsturl *url.URL) error { - srcClient, err := storage.NewClient(srcurl, c.srcS3opts) - if err != nil { - return err - } - dstClient, err := storage.NewClient(dsturl, c.dstS3opts) if err != nil { return err @@ -508,6 +503,10 @@ func (c Copy) doCopy(ctx context.Context, srcurl *url.URL, dsturl *url.URL) erro } if c.deleteSource { + srcClient, err := storage.NewClient(srcurl, c.srcS3opts) + if err != nil { + return err + } if err := srcClient.Delete(ctx, srcurl); err != nil { return err } diff --git a/command/options.go b/command/options.go new file mode 100644 index 000000000..b5c602c33 --- /dev/null +++ b/command/options.go @@ -0,0 +1,38 @@ +package command + +import ( + "github.com/peak/s5cmd/storage" + "github.com/urfave/cli/v2" +) + +// s3opts returns new S3Options object by extracting +// its fields from the provided context. Region is +// taken as (default) source-region. +func s3opts(c *cli.Context) storage.S3Options { + region := c.String("source-region") + if region == "" { + region = c.String("default-source-region") + } + return storage.S3Options{ + MaxRetries: c.Int("retry-count"), + Endpoint: c.String("endpoint-url"), + NoVerifySSL: c.Bool("no-verify-ssl"), + Region: region, + } +} + +// dstS3opts returns new S3Options object by extracting +// its fields from the provided context. Region is +// taken as (default) region, i.e., destination region. +func dstS3opts(c *cli.Context) storage.S3Options { + dstRegion := c.String("region") + if dstRegion == "" { + dstRegion = c.String("default-region") + } + return storage.S3Options{ + MaxRetries: c.Int("retry-count"), + Endpoint: c.String("endpoint-url"), + NoVerifySSL: c.Bool("no-verify-ssl"), + Region: dstRegion, + } +} diff --git a/storage/s3.go b/storage/s3.go index f3e1e3a52..224fb0cd7 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -668,22 +668,6 @@ func (s *s3Session) newSession(opts S3Options) (*session.Session, error) { return sess, nil } -// NumOfSessions returns number of sessions currently active. -func NumOfSessions() int { - s := sessions - s.Lock() - defer s.Unlock() - return len(s.sessions) -} - -// ClearSessions clears all existing sessions. -func ClearSessions() { - s := sessions - s.Lock() - defer s.Unlock() - s.sessions = map[S3Options]*session.Session{} -} - // customRetryer wraps the SDK's built in DefaultRetryer adding additional // error codes. Such as, retry for S3 InternalError code. type customRetryer struct { diff --git a/storage/s3_test.go b/storage/s3_test.go index 37600aa5d..3a80db2d3 100644 --- a/storage/s3_test.go +++ b/storage/s3_test.go @@ -9,7 +9,6 @@ import ( "math/rand" "net/http" urlpkg "net/url" - "os" "reflect" "strings" "testing" @@ -79,31 +78,6 @@ func TestNewSessionPathStyle(t *testing.T) { } } -func TestNewSessionWithRegionSetViaEnv(t *testing.T) { - opts := S3Options{ - Region: "", - } - - const expectedRegion = "us-west-2" - - if NumOfSessions() > 0 { - ClearSessions() - } - - os.Setenv("AWS_REGION", expectedRegion) - defer os.Unsetenv("AWS_REGION") - - sess, err := sessions.newSession(opts) - if err != nil { - t.Fatal(err) - } - - got := aws.StringValue(sess.Config.Region) - if got != expectedRegion { - t.Fatalf("expected %v, got %v", expectedRegion, got) - } -} - func TestS3ListSuccess(t *testing.T) { url, err := url.New("s3://bucket/key") if err != nil { @@ -417,67 +391,6 @@ func TestS3Retry(t *testing.T) { } } -func TestNumOfSessions(t *testing.T) { - ClearSessions() - - testcases := []struct { - name string - sourceRegion string - destinationRegion string - - expectedSessions int - }{ - { - name: "different source-region and region", - sourceRegion: "cn-north-1", - destinationRegion: "eu-central-1", - - expectedSessions: 2, - }, - { - name: "same source-region and region", - sourceRegion: "eu-west-1", - destinationRegion: "eu-west-1", - - expectedSessions: 1, - }, - { - name: "source-region and region not set", - expectedSessions: 1, - }, - } - const expectedTotalNumOfSessions = 4 - for _, tc := range testcases { - tc := tc - - t.Run(tc.name, func(t *testing.T) { - defer ClearSessions() - - for _, r := range []string{tc.sourceRegion, tc.destinationRegion} { - _, err := NewS3Storage(S3Options{ - Region: r, - }) - if err != nil { - t.Error(err) - } - } - - assert.Equal(t, NumOfSessions(), tc.expectedSessions) - }) - } - for _, tc := range testcases { - for _, r := range []string{tc.sourceRegion, tc.destinationRegion} { - _, err := NewS3Storage(S3Options{ - Region: r, - }) - if err != nil { - t.Error(err) - } - } - } - assert.Equal(t, NumOfSessions(), expectedTotalNumOfSessions) -} - // Credit to aws-sdk-go func val(i interface{}, s string) interface{} { v, err := awsutil.ValuesAtPath(i, s) From 1b19b3a7f89a42798732814ea81182e041e4d663 Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Tue, 4 Aug 2020 16:20:33 +0300 Subject: [PATCH 10/22] storage/s3_test: update based on feedback: more? testcase --- storage/s3_test.go | 77 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/storage/s3_test.go b/storage/s3_test.go index 3a80db2d3..8c561251a 100644 --- a/storage/s3_test.go +++ b/storage/s3_test.go @@ -18,6 +18,7 @@ import ( "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/aws/awsutil" "github.com/aws/aws-sdk-go/aws/request" + "github.com/aws/aws-sdk-go/aws/session" "github.com/aws/aws-sdk-go/awstesting/unit" "github.com/aws/aws-sdk-go/service/s3" "github.com/aws/aws-sdk-go/service/s3/s3manager" @@ -677,3 +678,79 @@ func TestS3listObjectsV2(t *testing.T) { } assert.Equal(t, len(mapReturnObjNameToModtime), 0) } + +func TestSessionCreateAndCachingWithDifferentRegions(t *testing.T) { + const bucketRegionPath = "CreateBucketConfiguration.LocationConstraint" + + testcases := []struct { + region string + expectedRegion string // to check if `create session` request with specific region was executed + + alreadyCreated bool // sessions should not be created again if they already have been created before + }{ + {}, + { + alreadyCreated: true, + }, + { + region: "eu-central-1", + expectedRegion: "eu-central-1", + }, + { + region: "eu-central-1", + expectedRegion: "eu-central-1", + + alreadyCreated: true, + }, + } + + sess := map[string]*session.Session{} + + for _, tc := range testcases { + awsSess, err := sessions.newSession(S3Options{ + Region: tc.region, + }) + + if err != nil { + t.Error(err) + } + + awsSess.Handlers.Unmarshal.Clear() + awsSess.Handlers.UnmarshalMeta.Clear() + awsSess.Handlers.UnmarshalError.Clear() + awsSess.Handlers.Send.Clear() + + awsSess.Handlers.Send.PushBack(func(r *request.Request) { + + r.HTTPResponse = &http.Response{ + StatusCode: http.StatusOK, + Body: ioutil.NopCloser(strings.NewReader("")), + } + + region := val(r.Params, bucketRegionPath) + if region != nil && tc.expectedRegion != "" { + assert.Equal(t, region, tc.expectedRegion) + } + + }) + + if tc.alreadyCreated { + _, ok := sess[tc.region] + assert.Check(t, ok, "session should not have been created again") + } else { + sess[tc.region] = awsSess + } + + mockApi := s3.New(awsSess) + + mockS3 := &S3{ + api: mockApi, + } + + err = mockS3.MakeBucket(context.Background(), "test") + + if err != nil { + t.Error(err) + } + } +} From 053de4f27b40deb94e150707c0ba44fb83786ae2 Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Tue, 4 Aug 2020 17:01:54 +0300 Subject: [PATCH 11/22] storage/s3_test: fixed an error from the last commit --- storage/s3_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/storage/s3_test.go b/storage/s3_test.go index 8c561251a..39d707597 100644 --- a/storage/s3_test.go +++ b/storage/s3_test.go @@ -731,7 +731,6 @@ func TestSessionCreateAndCachingWithDifferentRegions(t *testing.T) { if region != nil && tc.expectedRegion != "" { assert.Equal(t, region, tc.expectedRegion) } - }) if tc.alreadyCreated { @@ -750,6 +749,10 @@ func TestSessionCreateAndCachingWithDifferentRegions(t *testing.T) { err = mockS3.MakeBucket(context.Background(), "test") if err != nil { + if _, ok := err.(awserr.Error); ok { + // ignore aws response errors, we check request parameters only + continue + } t.Error(err) } } From 4a7730a5f6a514281e07dacfff67d94432dc57ca Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Sun, 9 Aug 2020 01:09:07 +0300 Subject: [PATCH 12/22] storage/ : updates based on pr review --- command/mv.go | 2 +- storage/s3.go | 6 ++++++ storage/s3_test.go | 23 +++++++++++++++++++++++ 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/command/mv.go b/command/mv.go index ccd286cc3..6e04c44c7 100644 --- a/command/mv.go +++ b/command/mv.go @@ -46,7 +46,7 @@ var moveCommand = &cli.Command{ dst: c.Args().Get(1), op: c.Command.Name, fullCommand: givenCommand(c), - deleteSource: true, // don't delete source + deleteSource: true, // delete source // flags noClobber: c.Bool("no-clobber"), ifSizeDiffer: c.Bool("if-size-differ"), diff --git a/storage/s3.go b/storage/s3.go index 224fb0cd7..103481b7a 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -668,6 +668,12 @@ func (s *s3Session) newSession(opts S3Options) (*session.Session, error) { return sess, nil } +func (s *s3Session) clear() { + s.Lock() + defer s.Unlock() + s.sessions = map[S3Options]*session.Session{} +} + // customRetryer wraps the SDK's built in DefaultRetryer adding additional // error codes. Such as, retry for S3 InternalError code. type customRetryer struct { diff --git a/storage/s3_test.go b/storage/s3_test.go index 39d707597..6054b7cba 100644 --- a/storage/s3_test.go +++ b/storage/s3_test.go @@ -9,6 +9,7 @@ import ( "math/rand" "net/http" urlpkg "net/url" + "os" "reflect" "strings" "testing" @@ -79,6 +80,28 @@ func TestNewSessionPathStyle(t *testing.T) { } } +func TestNewSessionWithRegionSetViaEnv(t *testing.T) { + opts := S3Options{ + Region: "", + } + sessions.clear() + + const expectedRegion = "us-west-2" + + os.Setenv("AWS_REGION", expectedRegion) + defer os.Unsetenv("AWS_REGION") + + sess, err := sessions.newSession(opts) + if err != nil { + t.Fatal(err) + } + + got := aws.StringValue(sess.Config.Region) + if got != expectedRegion { + t.Fatalf("expected %v, got %v", expectedRegion, got) + } +} + func TestS3ListSuccess(t *testing.T) { url, err := url.New("s3://bucket/key") if err != nil { From c77c2cececbd855cc7356b9c27c1cabf3880e7da Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Mon, 10 Aug 2020 18:54:33 +0300 Subject: [PATCH 13/22] command/: `infer` source and destination bucket regions --- CHANGELOG.md | 2 +- README.md | 14 --------- command/app.go | 10 +++++++ command/cat.go | 6 ++-- command/cp.go | 49 ++++++++++++++---------------- command/du.go | 7 ++--- command/ls.go | 13 ++++---- command/mb.go | 4 +-- command/mv.go | 10 +++---- command/options.go | 38 ------------------------ command/rm.go | 4 +-- e2e/cp_test.go | 56 ----------------------------------- e2e/run_test.go | 57 ----------------------------------- e2e/util_test.go | 4 +-- storage/s3.go | 56 +++++++++++++++++++++++++---------- storage/s3_test.go | 74 +++++++++++----------------------------------- storage/storage.go | 4 +-- 17 files changed, 116 insertions(+), 292 deletions(-) delete mode 100644 command/options.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 66552e62d..865e8b476 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ #### Features -- Added cross-region transfer support using `source-region` and `region` flags. It can be used for cp/mv operations where source and destination buckets reside in different regions. ([#155](https://github.com/peak/s5cmd/issues/155)) +- Added cross-region transfer support. It can be used for cp/mv operations where source and destination buckets reside in different regions. ([#155](https://github.com/peak/s5cmd/issues/155)) ## v1.1.0 - 22 Jul 2020 diff --git a/README.md b/README.md index b45b2e86d..a787fc23c 100644 --- a/README.md +++ b/README.md @@ -179,12 +179,6 @@ they'll be deleted in a single request. Will copy all the matching objects to the given S3 prefix, respecting the source folder hierarchy. -If source and destination buckets reside on different regions: - - s5cmd cp --source-region=us-east-2 --region=eu-central-1 s3://bucket1/file.txt s3://bucket2/ - -`--region` flag is for the bucket destination region, if not set, it will default to `--source-region` - ⚠️ Copying objects (from S3 to S3) larger than 5GB is not supported yet. We have an [open ticket](https://github.com/peak/s5cmd/issues/29) to track the issue. @@ -225,14 +219,6 @@ mv s3://bucket/2020/03/18/file1.gz s3://bucket/2020/03/18/original/file.gz ls # inline comments are OK too ``` -For batch operations it is possible to set a generic source and destination regions for buckets. For instance, - - s5cmd run --source-region= --region= commands.txt - -For each subcommand in the `commands.txt` file, source and destionation regions will -default to the flags set in the `run` command; -subcommands can overwrite these global flags by providing of their own. - ### Specifying credentials `s5cmd` uses official AWS SDK to access S3. SDK requires credentials to sign diff --git a/command/app.go b/command/app.go index a068ae91c..81fa1873f 100644 --- a/command/app.go +++ b/command/app.go @@ -4,6 +4,8 @@ import ( "context" "fmt" + "github.com/peak/s5cmd/storage" + cmpinstall "github.com/posener/complete/cmd/install" "github.com/urfave/cli/v2" @@ -56,7 +58,9 @@ var app = &cli.App{ }, }, Before: func(c *cli.Context) error { + noVerifySSL := c.Bool("no-verify-ssl") retryCount := c.Int("retry-count") + endpointURL := c.String("endpoint-url") workerCount := c.Int("numworkers") printJSON := c.Bool("json") logLevel := c.String("log") @@ -68,6 +72,12 @@ var app = &cli.App{ return fmt.Errorf("retry count cannot be a negative value") } + storage.Init(storage.S3Options{ + MaxRetries: retryCount, + Endpoint: endpointURL, + NoVerifySSL: noVerifySSL, + }) + return nil }, Action: func(c *cli.Context) error { diff --git a/command/cat.go b/command/cat.go index 9194fccb0..406939cdb 100644 --- a/command/cat.go +++ b/command/cat.go @@ -61,13 +61,13 @@ var catCommand = &cli.Command{ return err } - return Cat(c.Context, src, s3opts(c)) + return Cat(c.Context, src) }, } // Cat prints content of given source to standard output. -func Cat(ctx context.Context, src *url.URL, s3Opts storage.S3Options) error { - client, err := storage.NewClient(src, s3Opts) +func Cat(ctx context.Context, src *url.URL) error { + client, err := storage.NewClient(src) if err != nil { return err } diff --git a/command/cp.go b/command/cp.go index 83f50874f..65487ee8d 100644 --- a/command/cp.go +++ b/command/cp.go @@ -148,12 +148,15 @@ var copyCommand = &cli.Command{ Flags: copyCommandFlags, CustomHelpTemplate: copyHelpTemplate, Before: func(c *cli.Context) error { - return validate(c, s3opts(c)) + return validate(c) }, Action: func(c *cli.Context) error { + src := c.Args().Get(0) + dst := c.Args().Get(1) + return Copy{ - src: c.Args().Get(0), - dst: c.Args().Get(1), + src: src, + dst: dst, op: c.Command.Name, fullCommand: givenCommand(c), deleteSource: false, // don't delete source @@ -169,9 +172,6 @@ var copyCommand = &cli.Command{ encryptionMethod: c.String("sse"), encryptionKeyID: c.String("sse-kms-key-id"), acl: c.String("acl"), - - srcS3opts: s3opts(c), - dstS3opts: dstS3opts(c), }.Run(c.Context) }, } @@ -199,8 +199,6 @@ type Copy struct { // s3 options concurrency int partSize int64 - srcS3opts storage.S3Options - dstS3opts storage.S3Options } const fdlimitWarning = ` @@ -221,7 +219,7 @@ func (c Copy) Run(ctx context.Context) error { return err } - client, err := storage.NewClient(srcurl, c.srcS3opts) + client, err := storage.NewClient(srcurl) if err != nil { return err } @@ -323,7 +321,7 @@ func (c Copy) prepareDownloadTask( isBatch bool, ) func() error { return func() error { - dsturl, err := prepareLocalDestination(ctx, srcurl, dsturl, c.flatten, isBatch, c.dstS3opts) + dsturl, err := prepareLocalDestination(ctx, srcurl, dsturl, c.flatten, isBatch) if err != nil { return err } @@ -364,11 +362,11 @@ func (c Copy) prepareUploadTask( // doDownload is used to fetch a remote object and save as a local object. func (c Copy) doDownload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) error { - srcClient, err := storage.NewClient(srcurl, c.srcS3opts) + srcClient, err := storage.NewClient(srcurl) if err != nil { return err } - dstClient, err := storage.NewClient(dsturl, c.dstS3opts) + dstClient, err := storage.NewClient(dsturl) if err != nil { return err } @@ -429,7 +427,7 @@ func (c Copy) doUpload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) er return err } - dstClient, err := storage.NewClient(dsturl, c.dstS3opts) + dstClient, err := storage.NewClient(dsturl) if err != nil { return err } @@ -446,7 +444,7 @@ func (c Copy) doUpload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) er return err } - srcClient, err := storage.NewClient(srcurl, c.srcS3opts) + srcClient, err := storage.NewClient(srcurl) if err != nil { return err } @@ -477,7 +475,7 @@ func (c Copy) doUpload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) er } func (c Copy) doCopy(ctx context.Context, srcurl *url.URL, dsturl *url.URL) error { - dstClient, err := storage.NewClient(dsturl, c.dstS3opts) + dstClient, err := storage.NewClient(dsturl) if err != nil { return err } @@ -503,7 +501,7 @@ func (c Copy) doCopy(ctx context.Context, srcurl *url.URL, dsturl *url.URL) erro } if c.deleteSource { - srcClient, err := storage.NewClient(srcurl, c.srcS3opts) + srcClient, err := storage.NewClient(srcurl) if err != nil { return err } @@ -537,12 +535,12 @@ func (c Copy) shouldOverride(ctx context.Context, srcurl *url.URL, dsturl *url.U return nil } - srcObj, err := getObject(ctx, srcurl, c.srcS3opts) + srcObj, err := getObject(ctx, srcurl) if err != nil { return err } - dstObj, err := getObject(ctx, dsturl, c.dstS3opts) + dstObj, err := getObject(ctx, dsturl) if err != nil { return err } @@ -605,7 +603,6 @@ func prepareLocalDestination( dsturl *url.URL, flatten bool, isBatch bool, - s3opts storage.S3Options, ) (*url.URL, error) { objname := srcurl.Base() if isBatch && !flatten { @@ -618,7 +615,7 @@ func prepareLocalDestination( } } - client, err := storage.NewClient(dsturl, s3opts) + client, err := storage.NewClient(dsturl) if err != nil { return nil, err } @@ -653,8 +650,8 @@ func prepareLocalDestination( // getObject checks if the object from given url exists. If no object is // found, error and returning object would be nil. -func getObject(ctx context.Context, url *url.URL, s3opts storage.S3Options) (*storage.Object, error) { - client, err := storage.NewClient(url, s3opts) +func getObject(ctx context.Context, url *url.URL) (*storage.Object, error) { + client, err := storage.NewClient(url) if err != nil { return nil, err } @@ -667,7 +664,7 @@ func getObject(ctx context.Context, url *url.URL, s3opts storage.S3Options) (*st return obj, err } -func validate(c *cli.Context, srcS3opts storage.S3Options) error { +func validate(c *cli.Context) error { if c.Args().Len() != 2 { return fmt.Errorf("expected source and destination arguments") } @@ -706,7 +703,7 @@ func validate(c *cli.Context, srcS3opts storage.S3Options) error { case srcurl.Type == dsturl.Type: return validateCopy(srcurl, dsturl) case dsturl.IsRemote(): - return validateUpload(ctx, srcurl, dsturl, srcS3opts) + return validateUpload(ctx, srcurl, dsturl) default: return nil } @@ -721,8 +718,8 @@ func validateCopy(srcurl, dsturl *url.URL) error { return fmt.Errorf("local->local copy operations are not permitted") } -func validateUpload(ctx context.Context, srcurl, dsturl *url.URL, srcS3opts storage.S3Options) error { - srcclient, err := storage.NewClient(srcurl, srcS3opts) +func validateUpload(ctx context.Context, srcurl, dsturl *url.URL) error { + srcclient, err := storage.NewClient(srcurl) if err != nil { return err } diff --git a/command/du.go b/command/du.go index ba4adc708..22faab797 100644 --- a/command/du.go +++ b/command/du.go @@ -58,12 +58,12 @@ var sizeCommand = &cli.Command{ groupByClass := c.Bool("group") humanize := c.Bool("humanize") + src := c.Args().First() return Size( c.Context, - c.Args().First(), + src, groupByClass, humanize, - s3opts(c), ) }, } @@ -74,14 +74,13 @@ func Size( src string, groupByClass bool, humanize bool, - s3opts storage.S3Options, ) error { srcurl, err := url.New(src) if err != nil { return err } - client, err := storage.NewClient(srcurl, s3opts) + client, err := storage.NewClient(srcurl) if err != nil { return err } diff --git a/command/ls.go b/command/ls.go index 295c4149e..bfe99f063 100644 --- a/command/ls.go +++ b/command/ls.go @@ -67,29 +67,29 @@ var listCommand = &cli.Command{ }, Action: func(c *cli.Context) error { if !c.Args().Present() { - return ListBuckets(c.Context, s3opts(c)) + return ListBuckets(c.Context) } showEtag := c.Bool("etag") humanize := c.Bool("humanize") showStorageClass := c.Bool("storage-class") + src := c.Args().First() return List( c.Context, - c.Args().First(), + src, showEtag, humanize, showStorageClass, - s3opts(c), ) }, } // ListBuckets prints all buckets. -func ListBuckets(ctx context.Context, s3opts storage.S3Options) error { +func ListBuckets(ctx context.Context) error { // set as remote storage url := &url.URL{Type: 0} - client, err := storage.NewClient(url, s3opts) + client, err := storage.NewClient(url) if err != nil { return err } @@ -114,14 +114,13 @@ func List( showEtag bool, humanize bool, showStorageClass bool, - s3opts storage.S3Options, ) error { srcurl, err := url.New(src) if err != nil { return err } - client, err := storage.NewClient(srcurl, s3opts) + client, err := storage.NewClient(srcurl) if err != nil { return err } diff --git a/command/mb.go b/command/mb.go index 3329ca2cc..62cc0b4bc 100644 --- a/command/mb.go +++ b/command/mb.go @@ -51,7 +51,6 @@ var makeBucketCommand = &cli.Command{ c.Context, c.Command.Name, c.Args().First(), - s3opts(c), ) }, } @@ -61,14 +60,13 @@ func MakeBucket( ctx context.Context, op string, src string, - s3opts storage.S3Options, ) error { bucket, err := url.New(src) if err != nil { return err } - client, err := storage.NewClient(bucket, s3opts) + client, err := storage.NewClient(&url.URL{}) if err != nil { return err } diff --git a/command/mv.go b/command/mv.go index 6e04c44c7..6e46f9bc8 100644 --- a/command/mv.go +++ b/command/mv.go @@ -41,9 +41,12 @@ var moveCommand = &cli.Command{ return copyCommand.Before(c) }, Action: func(c *cli.Context) error { + src := c.Args().Get(0) + dst := c.Args().Get(1) + copyCommand := Copy{ - src: c.Args().Get(0), - dst: c.Args().Get(1), + src: src, + dst: dst, op: c.Command.Name, fullCommand: givenCommand(c), deleteSource: true, // delete source @@ -57,9 +60,6 @@ var moveCommand = &cli.Command{ encryptionMethod: c.String("sse"), encryptionKeyID: c.String("sse-kms-key-id"), acl: c.String("acl"), - - srcS3opts: s3opts(c), - dstS3opts: dstS3opts(c), } return copyCommand.Run(c.Context) diff --git a/command/options.go b/command/options.go deleted file mode 100644 index b5c602c33..000000000 --- a/command/options.go +++ /dev/null @@ -1,38 +0,0 @@ -package command - -import ( - "github.com/peak/s5cmd/storage" - "github.com/urfave/cli/v2" -) - -// s3opts returns new S3Options object by extracting -// its fields from the provided context. Region is -// taken as (default) source-region. -func s3opts(c *cli.Context) storage.S3Options { - region := c.String("source-region") - if region == "" { - region = c.String("default-source-region") - } - return storage.S3Options{ - MaxRetries: c.Int("retry-count"), - Endpoint: c.String("endpoint-url"), - NoVerifySSL: c.Bool("no-verify-ssl"), - Region: region, - } -} - -// dstS3opts returns new S3Options object by extracting -// its fields from the provided context. Region is -// taken as (default) region, i.e., destination region. -func dstS3opts(c *cli.Context) storage.S3Options { - dstRegion := c.String("region") - if dstRegion == "" { - dstRegion = c.String("default-region") - } - return storage.S3Options{ - MaxRetries: c.Int("retry-count"), - Endpoint: c.String("endpoint-url"), - NoVerifySSL: c.Bool("no-verify-ssl"), - Region: dstRegion, - } -} diff --git a/command/rm.go b/command/rm.go index 4ce644ca4..71dda4855 100644 --- a/command/rm.go +++ b/command/rm.go @@ -53,7 +53,6 @@ var deleteCommand = &cli.Command{ c.Context, c.Command.Name, givenCommand(c), - s3opts(c), c.Args().Slice()..., ) }, @@ -64,7 +63,6 @@ func Delete( ctx context.Context, op string, fullCommand string, - s3opts storage.S3Options, src ...string, ) error { srcurls, err := newURLs(src...) @@ -73,7 +71,7 @@ func Delete( } srcurl := srcurls[0] - client, err := storage.NewClient(srcurl, s3opts) + client, err := storage.NewClient(srcurl) if err != nil { return err } diff --git a/e2e/cp_test.go b/e2e/cp_test.go index d3251c0e8..a7d06a4b0 100644 --- a/e2e/cp_test.go +++ b/e2e/cp_test.go @@ -29,8 +29,6 @@ import ( "testing" "time" - "github.com/peak/s5cmd/storage" - "gotest.tools/v3/assert" "gotest.tools/v3/fs" "gotest.tools/v3/icmd" @@ -1440,60 +1438,6 @@ func TestCopySingleS3ObjectToS3(t *testing.T) { assert.Assert(t, ensureS3Object(s3client, bucket, dstfilename, content)) } -// cp --source-region=us-east-2 --region=us-west-2 s3://bucket/object s3://bucket/object2 -func TestCopySingleS3ObjectToS3WithDifferentSourceAndDestinationRegion(t *testing.T) { - t.Parallel() - - srcBucket := randomString(30) - dstBucket := randomString(30) - - endpoint, workdir, cleanup := server(t, "bolt") - defer cleanup() - - s5cmd := s5cmd(workdir, endpoint) - - srcClient := s3client(t, storage.S3Options{ - Endpoint: endpoint, - Region: "us-east-2", - NoVerifySSL: true, - }) - - dstClient := s3client(t, storage.S3Options{ - Endpoint: endpoint, - Region: "us-west-2", - NoVerifySSL: true, - }) - - createBucket(t, srcClient, srcBucket) - createBucket(t, dstClient, dstBucket) - - const ( - filename = "testfile1.txt" - dstfilename = "copy_" + filename - content = "this is a file content" - ) - - putFile(t, srcClient, srcBucket, filename, content) - - src := fmt.Sprintf("s3://%v/%v", srcBucket, filename) - dst := fmt.Sprintf("s3://%v/%v", dstBucket, dstfilename) - - cmd := s5cmd("cp", "--source-region=us-east-2", "--region=us-west-2", src, dst) - result := icmd.RunCmd(cmd) - - result.Assert(t, icmd.Success) - - assertLines(t, result.Stdout(), map[int]compareFunc{ - 0: equals(`cp %v %v`, src, dst), - }) - - // assert s3 source object - assert.Assert(t, ensureS3Object(srcClient, srcBucket, filename, content)) - - // assert s3 destination object - assert.Assert(t, ensureS3Object(dstClient, dstBucket, dstfilename, content)) -} - // --json cp s3://bucket/object s3://bucket2/object func TestCopySingleS3ObjectToS3JSON(t *testing.T) { t.Parallel() diff --git a/e2e/run_test.go b/e2e/run_test.go index 3db6dbdf9..750deb505 100644 --- a/e2e/run_test.go +++ b/e2e/run_test.go @@ -6,9 +6,6 @@ import ( "testing" "time" - "github.com/aws/aws-sdk-go/service/s3" - "github.com/peak/s5cmd/storage" - "gotest.tools/v3/fs" "gotest.tools/v3/icmd" ) @@ -235,57 +232,3 @@ func TestRunSpecialCharactersInPrefix(t *testing.T) { assertLines(t, result.Stderr(), map[int]compareFunc{}) } - -func TestRunFromFileWithMultipleSourceAndDestinationRegions(t *testing.T) { - t.Parallel() - - const numOfRegions = 4 - - var buckets [numOfRegions]string - for i := range buckets { - buckets[i] = randomString(30) - } - - endpoint, workdir, cleanup := server(t, "bolt") - defer cleanup() - - s5cmd := s5cmd(workdir, endpoint) - - regions := [numOfRegions]string{"us-east-2", "eu-east-1", "eu-central-2", "us-west-1"} - clients := [5]*s3.S3{} - for i, r := range regions { - clients[i] = s3client(t, storage.S3Options{ - Endpoint: endpoint, - Region: r, - NoVerifySSL: true, - }) - } - for i := 0; i < numOfRegions; i++ { - createBucket(t, clients[i], buckets[i]) - - filename := fmt.Sprintf("file%d.txt", i) - putFile(t, clients[i], buckets[i], filename, "content") - } - - filecontent := strings.Join([]string{ - fmt.Sprintf("ls s3://%v/file0.txt", buckets[0]), - fmt.Sprintf("cp s3://%v/file1.txt s3://%v/", buckets[1], buckets[2]), - fmt.Sprintf("mv s3://%v/file2.txt s3://%v/", buckets[2], buckets[3]), - }, "\n") - - file := fs.NewFile(t, "prefix", fs.WithContent(filecontent)) - defer file.Remove() - - cmd := s5cmd("run", "--source-region=us-east-2", file.Path()) - result := icmd.RunCmd(cmd) - - result.Assert(t, icmd.Success) - - assertLines(t, result.Stdout(), map[int]compareFunc{ - 0: suffix("file0.txt"), - 1: equals("cp s3://%v/file1.txt s3://%v/file1.txt", buckets[1], buckets[2]), - 2: equals("mv s3://%v/file2.txt s3://%v/file2.txt", buckets[2], buckets[3]), - }, sortInput(true)) - - assertLines(t, result.Stderr(), map[int]compareFunc{}) -} diff --git a/e2e/util_test.go b/e2e/util_test.go index 5480fe121..5d05a627c 100644 --- a/e2e/util_test.go +++ b/e2e/util_test.go @@ -26,6 +26,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/aws/credentials" + "github.com/aws/aws-sdk-go/aws/endpoints" "github.com/aws/aws-sdk-go/aws/session" "github.com/aws/aws-sdk-go/service/s3" "github.com/google/go-cmp/cmp" @@ -80,7 +81,6 @@ func setup(t *testing.T, options ...option) (*s3.S3, func(...string) icmd.Cmd, f client := s3client(t, storage.S3Options{ Endpoint: endpoint, - Region: "us-east-1", NoVerifySSL: true, }) @@ -129,7 +129,7 @@ func s3client(t *testing.T, options storage.S3Options) *s3.S3 { s3Config := aws.NewConfig(). WithEndpoint(options.Endpoint). - WithRegion(options.Region). + WithRegion(endpoints.UsEast1RegionID). WithCredentials(credentials.NewStaticCredentials(defaultAccessKeyID, defaultSecretAccessKey, "")). WithDisableSSL(options.NoVerifySSL). WithS3ForcePathStyle(true). diff --git a/storage/s3.go b/storage/s3.go index 103481b7a..3118b4642 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -31,8 +31,6 @@ var _ Storage = (*S3)(nil) var sentinelURL = urlpkg.URL{} -var sessions = &s3Session{sessions: map[S3Options]*session.Session{}} - const ( // deleteObjectsMax is the max allowed objects to be deleted on single HTTP // request. @@ -45,6 +43,19 @@ const ( gcsEndpoint = "storage.googleapis.com" ) +var ( + s3OptionSingle S3Options + sessionSingle s3Session +) + +func Init(opts S3Options) { + s3OptionSingle = opts + sessionSingle = s3Session{ + Mutex: sync.Mutex{}, + sessions: map[bucket]*session.Session{}, + } +} + // S3 is a storage type which interacts with S3API, DownloaderAPI and // UploaderAPI. type S3 struct { @@ -58,7 +69,6 @@ type S3 struct { type S3Options struct { MaxRetries int Endpoint string - Region string NoVerifySSL bool } @@ -80,13 +90,16 @@ func parseEndpoint(endpoint string) (urlpkg.URL, error) { } // NewS3Storage creates new S3 session. -func NewS3Storage(opts S3Options) (*S3, error) { - endpointURL, err := parseEndpoint(opts.Endpoint) +func NewS3Storage(url *url.URL) (*S3, error) { + endpointURL, err := parseEndpoint(s3OptionSingle.Endpoint) if err != nil { return nil, err } - awsSession, err := sessions.newSession(opts) + awsSession, err := sessionSingle.newSession(sessOptions{ + S3Options: s3OptionSingle, + bucket: bucket(url.Bucket), + }) if err != nil { return nil, err } @@ -584,20 +597,27 @@ func (s *S3) MakeBucket(ctx context.Context, name string) error { return err } +type bucket string + // s3Session holds session.Session according to s3Opts // and it synchronizes access/modification. type s3Session struct { sync.Mutex - sessions map[S3Options]*session.Session + sessions map[bucket]*session.Session +} + +type sessOptions struct { + S3Options + bucket } // NewAwsSession initializes a new AWS session with region fallback and custom // options. -func (s *s3Session) newSession(opts S3Options) (*session.Session, error) { +func (s *s3Session) newSession(opts sessOptions) (*session.Session, error) { s.Lock() defer s.Unlock() - if sess, ok := s.sessions[opts]; ok { + if sess, ok := s.sessions[opts.bucket]; ok { return sess, nil } @@ -631,10 +651,6 @@ func (s *s3Session) newSession(opts S3Options) (*session.Session, error) { WithS3UseAccelerate(useAccelerate). WithHTTPClient(httpClient) - if opts.Region != "" { - awsCfg.WithRegion(opts.Region) - } - awsCfg.Retryer = newCustomRetryer(opts.MaxRetries) useSharedConfig := session.SharedConfigEnable @@ -663,7 +679,17 @@ func (s *s3Session) newSession(opts S3Options) (*session.Session, error) { sess.Config.Region = aws.String(endpoints.UsEast1RegionID) } - s.sessions[opts] = sess + // get region of the bucket and create session accordingly + if opts.bucket != "" { + region, err := s3manager.GetBucketRegion(context.Background(), sess, string(opts.bucket), "") + if err != nil { + return nil, err + } + + sess.Config.Region = aws.String(region) + } + + s.sessions[opts.bucket] = sess return sess, nil } @@ -671,7 +697,7 @@ func (s *s3Session) newSession(opts S3Options) (*session.Session, error) { func (s *s3Session) clear() { s.Lock() defer s.Unlock() - s.sessions = map[S3Options]*session.Session{} + s.sessions = map[bucket]*session.Session{} } // customRetryer wraps the SDK's built in DefaultRetryer adding additional diff --git a/storage/s3_test.go b/storage/s3_test.go index 6054b7cba..17965e797 100644 --- a/storage/s3_test.go +++ b/storage/s3_test.go @@ -67,7 +67,10 @@ func TestNewSessionPathStyle(t *testing.T) { t.Run(tc.name, func(t *testing.T) { opts := S3Options{Endpoint: tc.endpoint.Hostname()} - sess, err := sessions.newSession(opts) + Init(opts) + sess, err := sessionSingle.newSession(sessOptions{ + S3Options: opts, + }) if err != nil { t.Fatal(err) } @@ -81,17 +84,16 @@ func TestNewSessionPathStyle(t *testing.T) { } func TestNewSessionWithRegionSetViaEnv(t *testing.T) { - opts := S3Options{ - Region: "", - } - sessions.clear() + opts := sessOptions{} + + sessionSingle.clear() const expectedRegion = "us-west-2" os.Setenv("AWS_REGION", expectedRegion) defer os.Unsetenv("AWS_REGION") - sess, err := sessions.newSession(opts) + sess, err := sessionSingle.newSession(opts) if err != nil { t.Fatal(err) } @@ -702,81 +704,41 @@ func TestS3listObjectsV2(t *testing.T) { assert.Equal(t, len(mapReturnObjNameToModtime), 0) } -func TestSessionCreateAndCachingWithDifferentRegions(t *testing.T) { +func TestSessionCreateAndCachingWithDifferentBuckets(t *testing.T) { const bucketRegionPath = "CreateBucketConfiguration.LocationConstraint" testcases := []struct { - region string - expectedRegion string // to check if `create session` request with specific region was executed - + bucket string alreadyCreated bool // sessions should not be created again if they already have been created before }{ - {}, { - alreadyCreated: true, + bucket: "bucket", }, { - region: "eu-central-1", - expectedRegion: "eu-central-1", + bucket: "bucket", + alreadyCreated: true, }, { - region: "eu-central-1", - expectedRegion: "eu-central-1", - - alreadyCreated: true, + bucket: "test-bucket", }, } sess := map[string]*session.Session{} for _, tc := range testcases { - awsSess, err := sessions.newSession(S3Options{ - Region: tc.region, + awsSess, err := sessionSingle.newSession(sessOptions{ + bucket: bucket(tc.bucket), }) if err != nil { t.Error(err) } - awsSess.Handlers.Unmarshal.Clear() - awsSess.Handlers.UnmarshalMeta.Clear() - awsSess.Handlers.UnmarshalError.Clear() - awsSess.Handlers.Send.Clear() - - awsSess.Handlers.Send.PushBack(func(r *request.Request) { - - r.HTTPResponse = &http.Response{ - StatusCode: http.StatusOK, - Body: ioutil.NopCloser(strings.NewReader("")), - } - - region := val(r.Params, bucketRegionPath) - if region != nil && tc.expectedRegion != "" { - assert.Equal(t, region, tc.expectedRegion) - } - }) - if tc.alreadyCreated { - _, ok := sess[tc.region] + _, ok := sess[tc.bucket] assert.Check(t, ok, "session should not have been created again") } else { - sess[tc.region] = awsSess - } - - mockApi := s3.New(awsSess) - - mockS3 := &S3{ - api: mockApi, - } - - err = mockS3.MakeBucket(context.Background(), "test") - - if err != nil { - if _, ok := err.(awserr.Error); ok { - // ignore aws response errors, we check request parameters only - continue - } - t.Error(err) + sess[tc.bucket] = awsSess } } } diff --git a/storage/storage.go b/storage/storage.go index 8924fa12a..34fbf1323 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -57,9 +57,9 @@ type Storage interface { // NewClient returns new Storage client from given url. Storage implementation // is inferred from the url. -func NewClient(url *url.URL, opts S3Options) (Storage, error) { +func NewClient(url *url.URL) (Storage, error) { if url.IsRemote() { - return NewS3Storage(opts) + return NewS3Storage(url) } return NewFilesystem(), nil From cf9d41e8c3d57559693335b6d08c8444661c8223 Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Mon, 10 Aug 2020 18:59:50 +0300 Subject: [PATCH 14/22] command/{cp, run}: removed unnecessary flags --- command/cp.go | 15 +-------------- command/run.go | 13 ------------- 2 files changed, 1 insertion(+), 27 deletions(-) diff --git a/command/cp.go b/command/cp.go index 65487ee8d..e1335934d 100644 --- a/command/cp.go +++ b/command/cp.go @@ -70,12 +70,7 @@ Examples: 12. Perform KMS-SSE of the object(s) at the destination using customer managed Customer Master Key (CMK) key id > s5cmd {{.HelpName}} -sse aws:kms -sse-kms-key-id s3://bucket/object s3://target-bucket/prefix/object - - 13. Copy S3 objects to another bucket in different region with default source region - > s5cmd {{.HelpName}} -region eu-east-2 s3://bucket/object s3://target-bucket/prefix/object - - 14. Copy S3 objects to another bucket in different region with explicitly provided source region - > s5cmd {{.HelpName}} -source-region us-west-1 -region eu-east-2 s3://bucket/object s3://target-bucket/prefix/object + ` var copyCommandFlags = []cli.Flag{ @@ -119,14 +114,6 @@ var copyCommandFlags = []cli.Flag{ Value: defaultPartSize, Usage: "size of each part transferred between host and remote server, in MiB", }, - &cli.StringFlag{ - Name: "source-region", - Usage: "connect to a specific region of the remote object storage service", - }, - &cli.StringFlag{ - Name: "region", - Usage: "region of the destination bucket for cp/mv operations", - }, &cli.StringFlag{ Name: "sse", Usage: "perform server side encryption of the data at its destination, e.g. aws:kms", diff --git a/command/run.go b/command/run.go index 6a11d70ad..3afaf03f0 100644 --- a/command/run.go +++ b/command/run.go @@ -31,24 +31,11 @@ Examples: 2. Read commands from standard input and execute in parallel. > cat commands.txt | s5cmd {{.HelpName}} ` -var runCommandFlags = []cli.Flag{ - &cli.StringFlag{ - Name: "default-source-region", - Aliases: []string{"source-region"}, - Usage: "connect to a specific region of the remote object storage service", - }, - &cli.StringFlag{ - Name: "default-region", - Aliases: []string{"region"}, - Usage: "region of the destination bucket for cp/mv operations; defaulted to source-region", - }, -} var runCommand = &cli.Command{ Name: "run", HelpName: "run", Usage: "run commands in batch", - Flags: runCommandFlags, // to be passed to children subcommands CustomHelpTemplate: runHelpTemplate, Before: func(c *cli.Context) error { if c.Args().Len() > 1 { From 26116108536ec42b08fd3ee6ad01fe7c3471b4c8 Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Tue, 11 Aug 2020 17:18:38 +0300 Subject: [PATCH 15/22] command/ : updates based on pr review --- command/app.go | 3 +-- command/cat.go | 2 +- command/cp.go | 21 ++++++++++----------- command/du.go | 2 +- command/ls.go | 4 ++-- command/mb.go | 2 +- command/rm.go | 2 +- storage/s3.go | 8 ++++---- storage/s3_test.go | 6 +++--- storage/storage.go | 4 ++-- 10 files changed, 26 insertions(+), 28 deletions(-) diff --git a/command/app.go b/command/app.go index 81fa1873f..28c079b4e 100644 --- a/command/app.go +++ b/command/app.go @@ -4,13 +4,12 @@ import ( "context" "fmt" - "github.com/peak/s5cmd/storage" - cmpinstall "github.com/posener/complete/cmd/install" "github.com/urfave/cli/v2" "github.com/peak/s5cmd/log" "github.com/peak/s5cmd/parallel" + "github.com/peak/s5cmd/storage" ) const ( diff --git a/command/cat.go b/command/cat.go index 406939cdb..aab5404c4 100644 --- a/command/cat.go +++ b/command/cat.go @@ -67,7 +67,7 @@ var catCommand = &cli.Command{ // Cat prints content of given source to standard output. func Cat(ctx context.Context, src *url.URL) error { - client, err := storage.NewClient(src) + client, err := storage.NewClient(ctx, src) if err != nil { return err } diff --git a/command/cp.go b/command/cp.go index e1335934d..3ddf058c5 100644 --- a/command/cp.go +++ b/command/cp.go @@ -70,7 +70,6 @@ Examples: 12. Perform KMS-SSE of the object(s) at the destination using customer managed Customer Master Key (CMK) key id > s5cmd {{.HelpName}} -sse aws:kms -sse-kms-key-id s3://bucket/object s3://target-bucket/prefix/object - ` var copyCommandFlags = []cli.Flag{ @@ -206,7 +205,7 @@ func (c Copy) Run(ctx context.Context) error { return err } - client, err := storage.NewClient(srcurl) + client, err := storage.NewClient(ctx, srcurl) if err != nil { return err } @@ -349,11 +348,11 @@ func (c Copy) prepareUploadTask( // doDownload is used to fetch a remote object and save as a local object. func (c Copy) doDownload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) error { - srcClient, err := storage.NewClient(srcurl) + srcClient, err := storage.NewClient(ctx, srcurl) if err != nil { return err } - dstClient, err := storage.NewClient(dsturl) + dstClient, err := storage.NewClient(ctx, dsturl) if err != nil { return err } @@ -414,7 +413,7 @@ func (c Copy) doUpload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) er return err } - dstClient, err := storage.NewClient(dsturl) + dstClient, err := storage.NewClient(ctx, dsturl) if err != nil { return err } @@ -431,7 +430,7 @@ func (c Copy) doUpload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) er return err } - srcClient, err := storage.NewClient(srcurl) + srcClient, err := storage.NewClient(ctx, srcurl) if err != nil { return err } @@ -462,7 +461,7 @@ func (c Copy) doUpload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) er } func (c Copy) doCopy(ctx context.Context, srcurl *url.URL, dsturl *url.URL) error { - dstClient, err := storage.NewClient(dsturl) + dstClient, err := storage.NewClient(ctx, dsturl) if err != nil { return err } @@ -488,7 +487,7 @@ func (c Copy) doCopy(ctx context.Context, srcurl *url.URL, dsturl *url.URL) erro } if c.deleteSource { - srcClient, err := storage.NewClient(srcurl) + srcClient, err := storage.NewClient(ctx, srcurl) if err != nil { return err } @@ -602,7 +601,7 @@ func prepareLocalDestination( } } - client, err := storage.NewClient(dsturl) + client, err := storage.NewClient(ctx, dsturl) if err != nil { return nil, err } @@ -638,7 +637,7 @@ func prepareLocalDestination( // getObject checks if the object from given url exists. If no object is // found, error and returning object would be nil. func getObject(ctx context.Context, url *url.URL) (*storage.Object, error) { - client, err := storage.NewClient(url) + client, err := storage.NewClient(ctx, url) if err != nil { return nil, err } @@ -706,7 +705,7 @@ func validateCopy(srcurl, dsturl *url.URL) error { } func validateUpload(ctx context.Context, srcurl, dsturl *url.URL) error { - srcclient, err := storage.NewClient(srcurl) + srcclient, err := storage.NewClient(ctx, srcurl) if err != nil { return err } diff --git a/command/du.go b/command/du.go index 22faab797..5ef7b6c35 100644 --- a/command/du.go +++ b/command/du.go @@ -80,7 +80,7 @@ func Size( return err } - client, err := storage.NewClient(srcurl) + client, err := storage.NewClient(ctx, srcurl) if err != nil { return err } diff --git a/command/ls.go b/command/ls.go index bfe99f063..3c5833380 100644 --- a/command/ls.go +++ b/command/ls.go @@ -89,7 +89,7 @@ var listCommand = &cli.Command{ func ListBuckets(ctx context.Context) error { // set as remote storage url := &url.URL{Type: 0} - client, err := storage.NewClient(url) + client, err := storage.NewClient(ctx, url) if err != nil { return err } @@ -120,7 +120,7 @@ func List( return err } - client, err := storage.NewClient(srcurl) + client, err := storage.NewClient(ctx, srcurl) if err != nil { return err } diff --git a/command/mb.go b/command/mb.go index 62cc0b4bc..91451caf4 100644 --- a/command/mb.go +++ b/command/mb.go @@ -66,7 +66,7 @@ func MakeBucket( return err } - client, err := storage.NewClient(&url.URL{}) + client, err := storage.NewClient(ctx, &url.URL{}) if err != nil { return err } diff --git a/command/rm.go b/command/rm.go index 71dda4855..95acf0ba4 100644 --- a/command/rm.go +++ b/command/rm.go @@ -71,7 +71,7 @@ func Delete( } srcurl := srcurls[0] - client, err := storage.NewClient(srcurl) + client, err := storage.NewClient(ctx, srcurl) if err != nil { return err } diff --git a/storage/s3.go b/storage/s3.go index 3118b4642..22e0fc58f 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -90,13 +90,13 @@ func parseEndpoint(endpoint string) (urlpkg.URL, error) { } // NewS3Storage creates new S3 session. -func NewS3Storage(url *url.URL) (*S3, error) { +func NewS3Storage(ctx context.Context, url *url.URL) (*S3, error) { endpointURL, err := parseEndpoint(s3OptionSingle.Endpoint) if err != nil { return nil, err } - awsSession, err := sessionSingle.newSession(sessOptions{ + awsSession, err := sessionSingle.newSession(ctx, sessOptions{ S3Options: s3OptionSingle, bucket: bucket(url.Bucket), }) @@ -613,7 +613,7 @@ type sessOptions struct { // NewAwsSession initializes a new AWS session with region fallback and custom // options. -func (s *s3Session) newSession(opts sessOptions) (*session.Session, error) { +func (s *s3Session) newSession(ctx context.Context, opts sessOptions) (*session.Session, error) { s.Lock() defer s.Unlock() @@ -681,7 +681,7 @@ func (s *s3Session) newSession(opts sessOptions) (*session.Session, error) { // get region of the bucket and create session accordingly if opts.bucket != "" { - region, err := s3manager.GetBucketRegion(context.Background(), sess, string(opts.bucket), "") + region, err := s3manager.GetBucketRegion(ctx, sess, string(opts.bucket), "") if err != nil { return nil, err } diff --git a/storage/s3_test.go b/storage/s3_test.go index 17965e797..68a3309b8 100644 --- a/storage/s3_test.go +++ b/storage/s3_test.go @@ -68,7 +68,7 @@ func TestNewSessionPathStyle(t *testing.T) { opts := S3Options{Endpoint: tc.endpoint.Hostname()} Init(opts) - sess, err := sessionSingle.newSession(sessOptions{ + sess, err := sessionSingle.newSession(context.Background(), sessOptions{ S3Options: opts, }) if err != nil { @@ -93,7 +93,7 @@ func TestNewSessionWithRegionSetViaEnv(t *testing.T) { os.Setenv("AWS_REGION", expectedRegion) defer os.Unsetenv("AWS_REGION") - sess, err := sessionSingle.newSession(opts) + sess, err := sessionSingle.newSession(context.Background(), opts) if err != nil { t.Fatal(err) } @@ -726,7 +726,7 @@ func TestSessionCreateAndCachingWithDifferentBuckets(t *testing.T) { sess := map[string]*session.Session{} for _, tc := range testcases { - awsSess, err := sessionSingle.newSession(sessOptions{ + awsSess, err := sessionSingle.newSession(context.Background(), sessOptions{ bucket: bucket(tc.bucket), }) diff --git a/storage/storage.go b/storage/storage.go index 34fbf1323..3d46079d1 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -57,9 +57,9 @@ type Storage interface { // NewClient returns new Storage client from given url. Storage implementation // is inferred from the url. -func NewClient(url *url.URL) (Storage, error) { +func NewClient(ctx context.Context, url *url.URL) (Storage, error) { if url.IsRemote() { - return NewS3Storage(url) + return NewS3Storage(ctx, url) } return NewFilesystem(), nil From 2692fe7bc4b6a26f1534081ae249e80c26350619 Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Tue, 11 Aug 2020 17:39:54 +0300 Subject: [PATCH 16/22] changelog, command/ : some roll-backs --- CHANGELOG.md | 3 +-- command/cp.go | 7 ++----- command/du.go | 3 +-- command/ls.go | 3 +-- command/mv.go | 7 ++----- 5 files changed, 7 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 865e8b476..ebc00f470 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,13 +2,12 @@ #### Features -- Added cross-region transfer support. It can be used for cp/mv operations where source and destination buckets reside in different regions. ([#155](https://github.com/peak/s5cmd/issues/155)) +- Added cross-region transfer support. Bucket regions are inferred, thus, supporting cross-region transfers and multiple regions in batch mode. ([#155](https://github.com/peak/s5cmd/issues/155)) ## v1.1.0 - 22 Jul 2020 With this release, Windows is supported. - #### Breaking changes - Dropped storage class short codes display from default behaviour of `ls` operation. Instead, use `-s` flag with `ls` to see full names of the storage classes when listing objects. diff --git a/command/cp.go b/command/cp.go index 3ddf058c5..b1e8b99ff 100644 --- a/command/cp.go +++ b/command/cp.go @@ -137,12 +137,9 @@ var copyCommand = &cli.Command{ return validate(c) }, Action: func(c *cli.Context) error { - src := c.Args().Get(0) - dst := c.Args().Get(1) - return Copy{ - src: src, - dst: dst, + src: c.Args().Get(0), + dst: c.Args().Get(1), op: c.Command.Name, fullCommand: givenCommand(c), deleteSource: false, // don't delete source diff --git a/command/du.go b/command/du.go index 5ef7b6c35..80cc8c556 100644 --- a/command/du.go +++ b/command/du.go @@ -58,10 +58,9 @@ var sizeCommand = &cli.Command{ groupByClass := c.Bool("group") humanize := c.Bool("humanize") - src := c.Args().First() return Size( c.Context, - src, + c.Args().First(), groupByClass, humanize, ) diff --git a/command/ls.go b/command/ls.go index 3c5833380..068b0fd8b 100644 --- a/command/ls.go +++ b/command/ls.go @@ -73,11 +73,10 @@ var listCommand = &cli.Command{ showEtag := c.Bool("etag") humanize := c.Bool("humanize") showStorageClass := c.Bool("storage-class") - src := c.Args().First() return List( c.Context, - src, + c.Args().First(), showEtag, humanize, showStorageClass, diff --git a/command/mv.go b/command/mv.go index 6e46f9bc8..e60513449 100644 --- a/command/mv.go +++ b/command/mv.go @@ -41,12 +41,9 @@ var moveCommand = &cli.Command{ return copyCommand.Before(c) }, Action: func(c *cli.Context) error { - src := c.Args().Get(0) - dst := c.Args().Get(1) - copyCommand := Copy{ - src: src, - dst: dst, + src: c.Args().Get(0), + dst: c.Args().Get(1), op: c.Command.Name, fullCommand: givenCommand(c), deleteSource: true, // delete source From 4e54be4b6b55057efe8f9ee77bb1ce103c3520bc Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Fri, 2 Oct 2020 01:01:56 +0300 Subject: [PATCH 17/22] storage/storage: removed region from options --- storage/storage.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/storage/storage.go b/storage/storage.go index dcd621ebb..c474afcfc 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -50,7 +50,6 @@ func NewRemoteClient(ctx context.Context, url *url.URL, opts Options) (*S3, erro newOpts := Options{ MaxRetries: opts.MaxRetries, Endpoint: opts.Endpoint, - Region: opts.Region, NoVerifySSL: opts.NoVerifySSL, DryRun: opts.DryRun, bucket: url.Bucket, @@ -69,7 +68,6 @@ func NewClient(ctx context.Context, url *url.URL, opts Options) (Storage, error) type Options struct { MaxRetries int Endpoint string - Region string NoVerifySSL bool DryRun bool bucket string From 080ce0ef30dae7af9c29c17adf1aff1d2ad05ca2 Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Fri, 2 Oct 2020 22:09:21 +0300 Subject: [PATCH 18/22] storage/s3: updates based on pr review --- CHANGELOG.md | 1 - storage/s3.go | 12 ++++-------- storage/s3_test.go | 8 ++++---- storage/storage.go | 2 +- 4 files changed, 9 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 918d1eef8..68514e0ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,6 @@ ## not released yet #### Features - - Added cross-region transfer support. Bucket regions are inferred, thus, supporting cross-region transfers and multiple regions in batch mode. ([#155](https://github.com/peak/s5cmd/issues/155)) #### Improvements diff --git a/storage/s3.go b/storage/s3.go index 9ec658168..0da1b869a 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -42,18 +42,14 @@ const ( ) // Re-used AWS sessions dramatically improve performance. -var cachedSessions func() *s3Session +var sessionProvider *s3Session // Init creates a new global S3 session. func init() { - s3Sess := &s3Session{ + sessionProvider = &s3Session{ Mutex: sync.Mutex{}, sessions: map[Options]*session.Session{}, } - - cachedSessions = func() *s3Session { - return s3Sess - } } // S3 is a storage type which interacts with S3API, DownloaderAPI and @@ -84,13 +80,13 @@ func parseEndpoint(endpoint string) (urlpkg.URL, error) { } // NewS3Storage creates new S3 session. -func newS3Storage(ctx context.Context, opts Options, s3sessProvider func() *s3Session) (*S3, error) { +func newS3Storage(ctx context.Context, opts Options, sessionProvider *s3Session) (*S3, error) { endpointURL, err := parseEndpoint(opts.Endpoint) if err != nil { return nil, err } - awsSession, err := s3sessProvider().newSession(ctx, opts) + awsSession, err := sessionProvider.newSession(ctx, opts) if err != nil { return nil, err } diff --git a/storage/s3_test.go b/storage/s3_test.go index 5813386af..6f8ad0da1 100644 --- a/storage/s3_test.go +++ b/storage/s3_test.go @@ -74,7 +74,7 @@ func TestNewSessionPathStyle(t *testing.T) { t.Run(tc.name, func(t *testing.T) { opts := Options{Endpoint: tc.endpoint.Hostname()} - sess, err := cachedSessions().newSession(context.Background(), opts) + sess, err := sessionProvider.newSession(context.Background(), opts) if err != nil { t.Fatal(err) } @@ -88,14 +88,14 @@ func TestNewSessionPathStyle(t *testing.T) { } func TestNewSessionWithRegionSetViaEnv(t *testing.T) { - cachedSessions().clear() + sessionProvider.clear() const expectedRegion = "us-west-2" os.Setenv("AWS_REGION", expectedRegion) defer os.Unsetenv("AWS_REGION") - sess, err := cachedSessions().newSession(context.Background(), Options{}) + sess, err := sessionProvider.newSession(context.Background(), Options{}) if err != nil { t.Fatal(err) } @@ -735,7 +735,7 @@ func TestSessionCreateAndCachingWithDifferentBuckets(t *testing.T) { sess := map[string]*session.Session{} for _, tc := range testcases { - awsSess, err := cachedSessions().newSession(context.Background(), Options{ + awsSess, err := sessionProvider.newSession(context.Background(), Options{ bucket: tc.bucket, }) diff --git a/storage/storage.go b/storage/storage.go index c474afcfc..2d27066c4 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -54,7 +54,7 @@ func NewRemoteClient(ctx context.Context, url *url.URL, opts Options) (*S3, erro DryRun: opts.DryRun, bucket: url.Bucket, } - return newS3Storage(ctx, newOpts, cachedSessions) + return newS3Storage(ctx, newOpts, sessionProvider) } func NewClient(ctx context.Context, url *url.URL, opts Options) (Storage, error) { From 868569c81434a59af149c85d0ceaacef0487c08e Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Mon, 5 Oct 2020 13:01:54 +0300 Subject: [PATCH 19/22] storage/s3: updates based on pr review --- storage/s3.go | 2 +- storage/storage.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/storage/s3.go b/storage/s3.go index 0da1b869a..c80ff4e88 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -80,7 +80,7 @@ func parseEndpoint(endpoint string) (urlpkg.URL, error) { } // NewS3Storage creates new S3 session. -func newS3Storage(ctx context.Context, opts Options, sessionProvider *s3Session) (*S3, error) { +func newS3Storage(ctx context.Context, opts Options) (*S3, error) { endpointURL, err := parseEndpoint(opts.Endpoint) if err != nil { return nil, err diff --git a/storage/storage.go b/storage/storage.go index 2d27066c4..c14bddcba 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -54,7 +54,7 @@ func NewRemoteClient(ctx context.Context, url *url.URL, opts Options) (*S3, erro DryRun: opts.DryRun, bucket: url.Bucket, } - return newS3Storage(ctx, newOpts, sessionProvider) + return newS3Storage(ctx, newOpts) } func NewClient(ctx context.Context, url *url.URL, opts Options) (Storage, error) { From 56a0963d18e6f4eda7e277687c58a4d4cd0b068c Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Sat, 17 Oct 2020 13:10:17 +0300 Subject: [PATCH 20/22] storage/s3: updates based on pr review --- go | 0 storage/s3.go | 11 +++-------- 2 files changed, 3 insertions(+), 8 deletions(-) create mode 100644 go diff --git a/go b/go new file mode 100644 index 000000000..e69de29bb diff --git a/storage/s3.go b/storage/s3.go index c80ff4e88..570d2c75d 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -42,14 +42,9 @@ const ( ) // Re-used AWS sessions dramatically improve performance. -var sessionProvider *s3Session - -// Init creates a new global S3 session. -func init() { - sessionProvider = &s3Session{ - Mutex: sync.Mutex{}, - sessions: map[Options]*session.Session{}, - } +var sessionProvider = &s3Session{ + Mutex: sync.Mutex{}, + sessions: map[Options]*session.Session{}, } // S3 is a storage type which interacts with S3API, DownloaderAPI and From 339ad77cc9e6e7c145a328d4e6fa45aa73263c15 Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Tue, 20 Oct 2020 14:28:17 +0300 Subject: [PATCH 21/22] storage/s3: updates based on pr review --- storage/s3.go | 1 - 1 file changed, 1 deletion(-) diff --git a/storage/s3.go b/storage/s3.go index 570d2c75d..0ef64a725 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -43,7 +43,6 @@ const ( // Re-used AWS sessions dramatically improve performance. var sessionProvider = &s3Session{ - Mutex: sync.Mutex{}, sessions: map[Options]*session.Session{}, } From cc3115dfcfb81f5540198871dc0d02d5c364d753 Mon Sep 17 00:00:00 2001 From: Farukhzhon Date: Wed, 21 Oct 2020 13:11:11 +0300 Subject: [PATCH 22/22] changelog: updates based on pr review --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68514e0ac..37f3d3b3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,11 +3,11 @@ ## not released yet #### Features +- Added global `--dry-run` option. It displays which command(s) will be executed without actually having a side effect. ([#90](https://github.com/peak/s5cmd/issues/90)) +- Added `--stat` option for `s5cmd` and it displays program execution statistics before the end of the program output. ([#148](https://github.com/peak/s5cmd/issues/148)) - Added cross-region transfer support. Bucket regions are inferred, thus, supporting cross-region transfers and multiple regions in batch mode. ([#155](https://github.com/peak/s5cmd/issues/155)) #### Improvements -- Added global `--dry-run` option. It displays which command(s) will be executed without actually having a side effect. ([#90](https://github.com/peak/s5cmd/issues/90)) -- Added `--stat` option for `s5cmd` and it displays program execution statistics before the end of the program output. ([#148](https://github.com/peak/s5cmd/issues/148)) - AWS S3 `RequestTimeTooSkewed` request error was not retryable before, it is now. ([205](https://github.com/peak/s5cmd/issues/205)) - For some operations errors were printed at the end of the program execution. Now, errors are displayed immediately after being detected. ([#136](https://github.com/peak/s5cmd/issues/136))