From bb979d089cd6c7b40e86a4c41e9918821f476acb Mon Sep 17 00:00:00 2001 From: Jarek Kowalski Date: Tue, 1 Feb 2022 23:03:56 -0800 Subject: [PATCH] fix(providers): fixed rclone connection in KopiaUI (#1712) (#1713) v0.10.3 introduced a regression where RClone connections did not work in KopiaUI but were ok in the CLI. The root cause was #1691 which caused storage context to be closed after opening the repository. Added test that verifies that storage does not rely on the context remaining open. --- repo/blob/azure/azure_storage_test.go | 13 +++++++++++-- repo/blob/b2/b2_storage_test.go | 7 ++++++- repo/blob/filesystem/filesystem_storage_test.go | 7 ++++++- repo/blob/gcs/gcs_storage_test.go | 7 ++++++- repo/blob/rclone/rclone_storage.go | 2 +- repo/blob/rclone/rclone_storage_test.go | 8 +++++++- repo/blob/s3/s3_storage_test.go | 8 +++++++- repo/blob/sftp/sftp_storage_test.go | 6 +++++- repo/blob/webdav/webdav_storage_test.go | 7 ++++++- 9 files changed, 55 insertions(+), 10 deletions(-) diff --git a/repo/blob/azure/azure_storage_test.go b/repo/blob/azure/azure_storage_test.go index 63547d109..4d7f932ff 100644 --- a/repo/blob/azure/azure_storage_test.go +++ b/repo/blob/azure/azure_storage_test.go @@ -110,12 +110,16 @@ func TestAzureStorage(t *testing.T) { ctx := testlogging.Context(t) - st, err := azure.New(ctx, &azure.Options{ + // use context that gets canceled after opening storage to ensure it's not used beyond New(). + newctx, cancel := context.WithCancel(ctx) + st, err := azure.New(newctx, &azure.Options{ Container: container, StorageAccount: storageAccount, StorageKey: storageKey, Prefix: fmt.Sprintf("test-%v-%x-", clock.Now().Unix(), data), }) + + cancel() require.NoError(t, err) defer st.Close(ctx) @@ -138,13 +142,18 @@ func TestAzureStorageSASToken(t *testing.T) { ctx := testlogging.Context(t) - st, err := azure.New(ctx, &azure.Options{ + // use context that gets canceled after storage is initialize, + // to verify we do not depend on the original context past initialization. + newctx, cancel := context.WithCancel(ctx) + st, err := azure.New(newctx, &azure.Options{ Container: container, StorageAccount: storageAccount, SASToken: sasToken, Prefix: fmt.Sprintf("sastest-%v-%x-", clock.Now().Unix(), data), }) + require.NoError(t, err) + cancel() defer st.Close(ctx) defer blobtesting.CleanupOldData(ctx, t, st, 0) diff --git a/repo/blob/b2/b2_storage_test.go b/repo/blob/b2/b2_storage_test.go index 652f55212..e78caf99f 100644 --- a/repo/blob/b2/b2_storage_test.go +++ b/repo/blob/b2/b2_storage_test.go @@ -73,7 +73,12 @@ func TestB2Storage(t *testing.T) { } ctx := testlogging.Context(t) - st, err := b2.New(ctx, opt) + + // use context that gets canceled after opening storage to ensure it's not used beyond New(). + newctx, cancel := context.WithCancel(ctx) + st, err := b2.New(newctx, opt) + + cancel() require.NoError(t, err) defer st.Close(ctx) diff --git a/repo/blob/filesystem/filesystem_storage_test.go b/repo/blob/filesystem/filesystem_storage_test.go index 1a8abfd7a..efc3aa31d 100644 --- a/repo/blob/filesystem/filesystem_storage_test.go +++ b/repo/blob/filesystem/filesystem_storage_test.go @@ -1,6 +1,7 @@ package filesystem import ( + "context" "path/filepath" "reflect" "sort" @@ -35,13 +36,17 @@ func TestFileStorage(t *testing.T) { } { path := testutil.TempDirectory(t) - r, err := New(ctx, &Options{ + newctx, cancel := context.WithCancel(ctx) + + // use context that gets canceled after opening storage to ensure it's not used beyond New(). + r, err := New(newctx, &Options{ Path: path, Options: sharded.Options{ DirectoryShards: shardSpec, }, }, true) + cancel() require.NoError(t, err) require.NotNil(t, r) diff --git a/repo/blob/gcs/gcs_storage_test.go b/repo/blob/gcs/gcs_storage_test.go index 9366be8e1..696574b26 100644 --- a/repo/blob/gcs/gcs_storage_test.go +++ b/repo/blob/gcs/gcs_storage_test.go @@ -3,6 +3,7 @@ import ( "bytes" "compress/gzip" + "context" "encoding/base64" "io" "os" @@ -38,7 +39,11 @@ func TestGCSStorage(t *testing.T) { ctx := testlogging.Context(t) - st, err := gcs.New(ctx, mustGetOptionsOrSkip(t, uuid.NewString())) + // use context that gets canceled after opening storage to ensure it's not used beyond New(). + newctx, cancel := context.WithCancel(ctx) + st, err := gcs.New(newctx, mustGetOptionsOrSkip(t, uuid.NewString())) + + cancel() require.NoError(t, err) defer st.Close(ctx) diff --git a/repo/blob/rclone/rclone_storage.go b/repo/blob/rclone/rclone_storage.go index 936c07803..18bf132a1 100644 --- a/repo/blob/rclone/rclone_storage.go +++ b/repo/blob/rclone/rclone_storage.go @@ -269,7 +269,7 @@ func New(ctx context.Context, opt *Options, isCreate bool) (blob.Storage, error) arguments = append(arguments, "--config", tmpConfigFile) } - r.cmd = exec.CommandContext(ctx, rcloneExe, arguments...) //nolint:gosec + r.cmd = exec.Command(rcloneExe, arguments...) //nolint:gosec r.cmd.Env = append(r.cmd.Env, opt.RCloneEnv...) startupTimeout := rcloneStartupTimeout diff --git a/repo/blob/rclone/rclone_storage_test.go b/repo/blob/rclone/rclone_storage_test.go index 68829859e..32eeedd6e 100644 --- a/repo/blob/rclone/rclone_storage_test.go +++ b/repo/blob/rclone/rclone_storage_test.go @@ -1,6 +1,7 @@ package rclone_test import ( + "context" "encoding/base64" "encoding/json" "os" @@ -64,11 +65,16 @@ func TestRCloneStorage(t *testing.T) { rcloneExe := mustGetRcloneExeOrSkip(t) dataDir := testutil.TempDirectory(t) - st, err := rclone.New(ctx, &rclone.Options{ + // use context that gets canceled after opening storage to ensure it's not used beyond New(). + newctx, cancel := context.WithCancel(ctx) + st, err := rclone.New(newctx, &rclone.Options{ // pass local file as remote path. RemotePath: dataDir, RCloneExe: rcloneExe, }, true) + + cancel() + if err != nil { t.Fatalf("unable to connect to rclone backend: %v", err) } diff --git a/repo/blob/s3/s3_storage_test.go b/repo/blob/s3/s3_storage_test.go index e70f4728d..2d45a1e63 100644 --- a/repo/blob/s3/s3_storage_test.go +++ b/repo/blob/s3/s3_storage_test.go @@ -1,6 +1,7 @@ package s3 import ( + "context" "crypto/tls" "encoding/json" "errors" @@ -407,6 +408,7 @@ func testStorage(t *testing.T, options *Options, runValidationTest bool, opts bl require.Equal(t, "", options.Prefix) st0, err := New(ctx, options) + require.NoError(t, err) defer st0.Close(ctx) @@ -415,7 +417,11 @@ func testStorage(t *testing.T, options *Options, runValidationTest bool, opts bl options.Prefix = uuid.NewString() - st, err := New(ctx, options) + // use context that gets canceled after opening storage to ensure it's not used beyond New(). + newctx, cancel := context.WithCancel(ctx) + st, err := New(newctx, options) + + cancel() require.NoError(t, err) defer st.Close(ctx) diff --git a/repo/blob/sftp/sftp_storage_test.go b/repo/blob/sftp/sftp_storage_test.go index ed120b1ba..2494a6a8b 100644 --- a/repo/blob/sftp/sftp_storage_test.go +++ b/repo/blob/sftp/sftp_storage_test.go @@ -182,7 +182,9 @@ func TestSFTPStorageValid(t *testing.T) { t.Run(fmt.Sprintf("Embed=%v", embedCreds), func(t *testing.T) { ctx := testlogging.Context(t) - st, err := createSFTPStorage(ctx, t, sftp.Options{ + // use context that gets canceled after opening storage to ensure it's not used beyond New(). + newctx, cancel := context.WithCancel(ctx) + st, err := createSFTPStorage(newctx, t, sftp.Options{ Path: "/upload", Host: host, Username: sftpUsernameWithKeyAuth, @@ -194,6 +196,8 @@ func TestSFTPStorageValid(t *testing.T) { t.Fatalf("unable to connect to SSH: %v", err) } + cancel() + deleteBlobs(ctx, t, st) blobtesting.VerifyStorage(ctx, t, st, blob.PutOptions{}) diff --git a/repo/blob/webdav/webdav_storage_test.go b/repo/blob/webdav/webdav_storage_test.go index 40ef6292f..120c2ab42 100644 --- a/repo/blob/webdav/webdav_storage_test.go +++ b/repo/blob/webdav/webdav_storage_test.go @@ -2,6 +2,7 @@ import ( "bytes" + "context" "fmt" "io" "net/http" @@ -148,7 +149,9 @@ func transformMissingPUTs(next http.Handler) http.HandlerFunc { func verifyWebDAVStorage(t *testing.T, url, username, password string, shardSpec []int) { ctx := testlogging.Context(t) - st, err := New(testlogging.Context(t), &Options{ + // use context that gets canceled after opening storage to ensure it's not used beyond New(). + newctx, cancel := context.WithCancel(ctx) + st, err := New(newctx, &Options{ URL: url, Options: sharded.Options{ DirectoryShards: shardSpec, @@ -157,6 +160,8 @@ func verifyWebDAVStorage(t *testing.T, url, username, password string, shardSpec Password: password, }, false) + cancel() + if st == nil || err != nil { t.Errorf("unexpected result: %v %v", st, err) }