From ca3ca60ba4d9a4e507c0cbfba79bc62e41e82e66 Mon Sep 17 00:00:00 2001 From: ashmrtn <3891298+ashmrtn@users.noreply.github.com> Date: Tue, 30 Jan 2024 10:27:35 -0800 Subject: [PATCH] Update repo init for partial init case (#5060) Unclear if this is precisely the path we want to take to handle possible partial repo initialization so feel free to leave suggestions if a different behavior would be better Update the repo init logic to continue if the repo is marked as already existing. If it already exists then the process will try to update persistent config info in the repo. If the repo already exists but some connection credential is incorrect then stack errors such that both the reason for the connect failure and the fact that the repo already exists are both visible. Manually tested attempting to initialize a repo that was already initialized but using a different passphrase and got the output: ```text Error: initializing repository: a repository was already initialized with that configuration: connecting to kopia repo: unable to create format manager: invalid repository password: repo already exists: repository already initialized exit status 1 ``` Attempting to initialize a repo again using the same passphrase resulted in a success message with no errors reported Unfortunately I can't really add unit tests to ensure the config ends up the way we expect because I have no way to trigger an error partway through kopia's init logic (or even just after it) without some non-trivial code refactoring --- #### Does this PR need a docs update or release note? - [x] :white_check_mark: Yes, it's included - [ ] :clock1: Yes, but in a later PR - [ ] :no_entry: No #### Type of change - [ ] :sunflower: Feature - [x] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup #### Test Plan - [x] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- CHANGELOG.md | 2 ++ src/cli/flags/filesystem.go | 7 ------ src/cli/flags/repo.go | 6 ++--- src/cli/flags/s3.go | 5 ---- src/cli/repo/filesystem.go | 5 ---- src/cli/repo/s3.go | 5 ---- src/cli/utils/flags_test.go | 4 --- src/internal/kopia/conn.go | 15 ++++++++--- src/internal/kopia/conn_test.go | 44 ++++++++++++++++++++++++++++++++- 9 files changed, 58 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0c4cc2cc..67446bda5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,10 +19,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Groups and Teams backups no longer fail when a resource has no display name. - Contacts in-place restore failed if the restore destination was empty. - Link shares with external users are now backed up and restored as expected +- Ensure persistent repo config is populated on repo init if repo init failed partway through during the previous init attempt. ### Changed - When running `backup details` on an empty backup returns a more helpful error message. - Backup List additionally shows the data category for each backup. +- Remove hidden `--succeed-if-exists` flag for repo init. Repo init will now succeed without error if run on an existing repo with the same passphrase. ### Known issues - Backing up a group mailbox item may fail if it has a very large number of attachments (500+). diff --git a/src/cli/flags/filesystem.go b/src/cli/flags/filesystem.go index 8ba330d02..be2cf9016 100644 --- a/src/cli/flags/filesystem.go +++ b/src/cli/flags/filesystem.go @@ -28,13 +28,6 @@ func AddFilesystemFlags(cmd *cobra.Command) { "", "path to local or network storage") cobra.CheckErr(cmd.MarkFlagRequired(FilesystemPathFN)) - - fs.BoolVar( - &SucceedIfExistsFV, - SucceedIfExistsFN, - false, - "Exit with success if the repo has already been initialized.") - cobra.CheckErr(fs.MarkHidden("succeed-if-exists")) } func FilesystemFlagOverrides(cmd *cobra.Command) map[string]string { diff --git a/src/cli/flags/repo.go b/src/cli/flags/repo.go index ac84488b6..0344d2fbc 100644 --- a/src/cli/flags/repo.go +++ b/src/cli/flags/repo.go @@ -12,9 +12,8 @@ const ( AWSSessionTokenFN = "aws-session-token" // Corso Flags - PassphraseFN = "passphrase" - NewPassphraseFN = "new-passphrase" - SucceedIfExistsFN = "succeed-if-exists" + PassphraseFN = "passphrase" + NewPassphraseFN = "new-passphrase" ) var ( @@ -25,7 +24,6 @@ var ( AWSSessionTokenFV string PassphraseFV string NewPhasephraseFV string - SucceedIfExistsFV bool ) // AddMultipleBackupIDsFlag adds the --backups flag. diff --git a/src/cli/flags/s3.go b/src/cli/flags/s3.go index d0b97bbc8..8a807cc55 100644 --- a/src/cli/flags/s3.go +++ b/src/cli/flags/s3.go @@ -38,11 +38,6 @@ func AddS3BucketFlags(cmd *cobra.Command) { fs.StringVar(&EndpointFV, EndpointFN, "", "S3 service endpoint.") fs.BoolVar(&DoNotUseTLSFV, DoNotUseTLSFN, false, "Disable TLS (HTTPS)") fs.BoolVar(&DoNotVerifyTLSFV, DoNotVerifyTLSFN, false, "Disable TLS (HTTPS) certificate verification.") - - // In general, we don't want to expose this flag to users and have them mistake it - // for a broad-scale idempotency solution. We can un-hide it later the need arises. - fs.BoolVar(&SucceedIfExistsFV, SucceedIfExistsFN, false, "Exit with success if the repo has already been initialized.") - cobra.CheckErr(fs.MarkHidden("succeed-if-exists")) } func S3FlagOverrides(cmd *cobra.Command) map[string]string { diff --git a/src/cli/repo/filesystem.go b/src/cli/repo/filesystem.go index 37824a25e..9a2ad1d30 100644 --- a/src/cli/repo/filesystem.go +++ b/src/cli/repo/filesystem.go @@ -2,7 +2,6 @@ package repo import ( "github.com/alcionai/clues" - "github.com/pkg/errors" "github.com/spf13/cobra" "github.com/alcionai/corso/src/cli/flags" @@ -110,10 +109,6 @@ func initFilesystemCmd(cmd *cobra.Command, args []string) error { ric := repository.InitConfig{RetentionOpts: retentionOpts} if err = r.Initialize(ctx, ric); err != nil { - if flags.SucceedIfExistsFV && errors.Is(err, repository.ErrorRepoAlreadyExists) { - return nil - } - return Only(ctx, clues.Stack(ErrInitializingRepo, err)) } diff --git a/src/cli/repo/s3.go b/src/cli/repo/s3.go index 412de3ea6..4cfc00753 100644 --- a/src/cli/repo/s3.go +++ b/src/cli/repo/s3.go @@ -4,7 +4,6 @@ import ( "strings" "github.com/alcionai/clues" - "github.com/pkg/errors" "github.com/spf13/cobra" "github.com/alcionai/corso/src/cli/flags" @@ -132,10 +131,6 @@ func initS3Cmd(cmd *cobra.Command, args []string) error { ric := repository.InitConfig{RetentionOpts: retentionOpts} if err = r.Initialize(ctx, ric); err != nil { - if flags.SucceedIfExistsFV && errors.Is(err, repository.ErrorRepoAlreadyExists) { - return nil - } - return Only(ctx, clues.Stack(ErrInitializingRepo, err)) } diff --git a/src/cli/utils/flags_test.go b/src/cli/utils/flags_test.go index 1cf1c826d..436de81a4 100644 --- a/src/cli/utils/flags_test.go +++ b/src/cli/utils/flags_test.go @@ -103,7 +103,6 @@ func (suite *FlagUnitSuite) TestAddS3BucketFlags() { assert.Equal(t, "prefix1", flags.PrefixFV, flags.PrefixFN) assert.True(t, flags.DoNotUseTLSFV, flags.DoNotUseTLSFN) assert.True(t, flags.DoNotVerifyTLSFV, flags.DoNotVerifyTLSFN) - assert.True(t, flags.SucceedIfExistsFV, flags.SucceedIfExistsFN) }, } @@ -116,7 +115,6 @@ func (suite *FlagUnitSuite) TestAddS3BucketFlags() { "--" + flags.PrefixFN, "prefix1", "--" + flags.DoNotUseTLSFN, "--" + flags.DoNotVerifyTLSFN, - "--" + flags.SucceedIfExistsFN, }) err := cmd.Execute() @@ -130,7 +128,6 @@ func (suite *FlagUnitSuite) TestFilesystemFlags() { Use: "test", Run: func(cmd *cobra.Command, args []string) { assert.Equal(t, "/tmp/test", flags.FilesystemPathFV, flags.FilesystemPathFN) - assert.True(t, flags.SucceedIfExistsFV, flags.SucceedIfExistsFN) assert.Equal(t, "tenantID", flags.AzureClientTenantFV, flags.AzureClientTenantFN) assert.Equal(t, "clientID", flags.AzureClientIDFV, flags.AzureClientIDFN) assert.Equal(t, "secret", flags.AzureClientSecretFV, flags.AzureClientSecretFN) @@ -143,7 +140,6 @@ func (suite *FlagUnitSuite) TestFilesystemFlags() { cmd.SetArgs([]string{ "test", "--" + flags.FilesystemPathFN, "/tmp/test", - "--" + flags.SucceedIfExistsFN, "--" + flags.AzureClientIDFN, "clientID", "--" + flags.AzureClientTenantFN, "tenantID", "--" + flags.AzureClientSecretFN, "secret", diff --git a/src/internal/kopia/conn.go b/src/internal/kopia/conn.go index d79d37f41..436c865e9 100644 --- a/src/internal/kopia/conn.go +++ b/src/internal/kopia/conn.go @@ -149,12 +149,16 @@ func (w *conn) Initialize( RetentionPeriod: blobCfg.RetentionPeriod, } + var initErr error + if err = repo.Initialize(ctx, bst, &kopiaOpts, cfg.CorsoPassphrase); err != nil { - if errors.Is(err, repo.ErrAlreadyInitialized) { - return clues.StackWC(ctx, ErrorRepoAlreadyExists, err) + if !errors.Is(err, repo.ErrAlreadyInitialized) { + return clues.WrapWC(ctx, err, "initializing repo") } - return clues.WrapWC(ctx, err, "initializing repo") + logger.Ctx(ctx).Info("repo already exists, verifying repo config") + + initErr = clues.StackWC(ctx, ErrorRepoAlreadyExists, err) } err = w.commonConnect( @@ -166,7 +170,10 @@ func (w *conn) Initialize( cfg.CorsoPassphrase, defaultCompressor) if err != nil { - return err + // If the repo already exists then give some indication to that to help the + // user debug. For example, they could have called init again on a repo that + // already exists but accidentally used a different passphrase. + return clues.Stack(err, initErr) } if err := w.setDefaultConfigValues(ctx); err != nil { diff --git a/src/internal/kopia/conn_test.go b/src/internal/kopia/conn_test.go index 4513160a0..93c98479d 100644 --- a/src/internal/kopia/conn_test.go +++ b/src/internal/kopia/conn_test.go @@ -3,6 +3,7 @@ package kopia import ( "context" "math" + "strings" "testing" "time" @@ -15,6 +16,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" + "golang.org/x/exp/maps" "github.com/alcionai/corso/src/internal/common/ptr" strTD "github.com/alcionai/corso/src/internal/common/str/testdata" @@ -94,7 +96,7 @@ func TestWrapperIntegrationSuite(t *testing.T) { }) } -func (suite *WrapperIntegrationSuite) TestRepoExistsError() { +func (suite *WrapperIntegrationSuite) TestInitialize_SamePassphrase() { t := suite.T() repoNameHash := strTD.NewHashForRepoConfigName() @@ -110,6 +112,46 @@ func (suite *WrapperIntegrationSuite) TestRepoExistsError() { err = k.Close(ctx) require.NoError(t, err, clues.ToCore(err)) + err = k.Initialize(ctx, repository.Options{}, repository.Retention{}, repoNameHash) + assert.NoError(t, err, clues.ToCore(err)) +} + +func (suite *WrapperIntegrationSuite) TestInitialize_IncorrectPassphrase() { + t := suite.T() + repoNameHash := strTD.NewHashForRepoConfigName() + + ctx, flush := tester.NewContext(t) + defer flush() + + st1 := storeTD.NewFilesystemStorage(t) + k := NewConn(st1) + + err := k.Initialize(ctx, repository.Options{}, repository.Retention{}, repoNameHash) + require.NoError(t, err, clues.ToCore(err)) + + err = k.Close(ctx) + require.NoError(t, err, clues.ToCore(err)) + + // Hacky way to edit the existing passphrase for the repo so we can check that + // we get a sensible error back. + st2 := st1 + st2.Config = maps.Clone(st1.Config) + + var found bool + + for k, v := range st2.Config { + if strings.Contains(strings.ToLower(k), "passphrase") { + st2.Config[k] = v + "1" + found = true + + break + } + } + + require.True(t, found, "unable to update passphrase for test") + + k = NewConn(st2) + err = k.Initialize(ctx, repository.Options{}, repository.Retention{}, repoNameHash) assert.Error(t, err, clues.ToCore(err)) assert.ErrorIs(t, err, ErrorRepoAlreadyExists)