From ecabeede25d5bcb67a5da8d6dfdc80b4c63061a9 Mon Sep 17 00:00:00 2001 From: Kelvin Liu Date: Thu, 25 May 2023 16:06:05 +1000 Subject: [PATCH] Add tests for backend refCount fixes; --- state/migration_import_tasks.go | 1 - state/migration_import_test.go | 13 ++++++++++++ state/package_test.go | 10 +++++++++ state/secrets.go | 2 -- state/secrets_test.go | 28 ++++++++++++++++++++++--- state/state.go | 3 --- worker/secretdrainworker/worker.go | 3 +-- worker/secretdrainworker/worker_test.go | 9 +++++--- 8 files changed, 55 insertions(+), 14 deletions(-) diff --git a/state/migration_import_tasks.go b/state/migration_import_tasks.go index e38537a11daf..36009d1ebd1d 100644 --- a/state/migration_import_tasks.go +++ b/state/migration_import_tasks.go @@ -617,7 +617,6 @@ func (ImportSecrets) Execute(src SecretsInput, runner TransactionRunner, knownSe } if !utils.IsValidUUIDString(valueRef.BackendID) && !seenBackendIds.Contains(valueRef.BackendID) { if !knownSecretBackends.Contains(valueRef.BackendID) { - logger.Criticalf("target controller does not have secret backend %q, seenBackendIds %v", valueRef.BackendID, seenBackendIds.Values()) return errors.New("target controller does not have all required secret backends set up") } ops = append(ops, txn.Op{ diff --git a/state/migration_import_test.go b/state/migration_import_test.go index b073f7ec9ca9..d8d31c9ea976 100644 --- a/state/migration_import_test.go +++ b/state/migration_import_test.go @@ -3111,6 +3111,7 @@ func (s *MigrationImportSuite) TestSecrets(c *gc.C) { }, }) c.Assert(err, jc.ErrorIsNil) + err = s.State.GrantSecretAccess(uri, state.SecretAccessParams{ LeaderToken: &fakeToken{}, Scope: owner.Tag(), @@ -3139,8 +3140,16 @@ func (s *MigrationImportSuite) TestSecrets(c *gc.C) { }) c.Assert(err, jc.ErrorIsNil) + backendRefCount, err := s.State.ReadBackendRevisionCount(backendID) + c.Assert(err, jc.ErrorIsNil) + c.Assert(backendRefCount, gc.Equals, 1) + _, newSt := s.importModel(c, s.State) + backendRefCount, err = s.State.ReadBackendRevisionCount(backendID) + c.Assert(err, jc.ErrorIsNil) + c.Assert(backendRefCount, gc.Equals, 2) + store = state.NewSecrets(newSt) all, err := store.ListSecrets(state.SecretsFilter{}) c.Assert(err, jc.ErrorIsNil) @@ -3187,6 +3196,10 @@ func (s *MigrationImportSuite) TestSecrets(c *gc.C) { CurrentRevision: 667, LatestRevision: 2, }) + + backendRefCount, err = newSt.ReadBackendRevisionCount(backendID) + c.Assert(err, jc.ErrorIsNil) + c.Assert(backendRefCount, gc.Equals, 2) } func (s *MigrationImportSuite) TestSecretsMissingBackend(c *gc.C) { diff --git a/state/package_test.go b/state/package_test.go index da9afefbbb0d..f618f97b959f 100644 --- a/state/package_test.go +++ b/state/package_test.go @@ -6,6 +6,7 @@ package state import ( "testing" + "github.com/juju/errors" "github.com/juju/mgo/v3/bson" "github.com/juju/mgo/v3/txn" jc "github.com/juju/testing/checkers" @@ -105,3 +106,12 @@ func MustCloseUnitPortRanges(c *gc.C, st *State, machine *Machine, unitName, end } c.Assert(st.ApplyOperation(machPortRanges.Changes()), jc.ErrorIsNil) } + +func (st *State) ReadBackendRevisionCount(backendID string) (int, error) { + refCountCollection, ccloser := st.db().GetCollection(globalRefcountsC) + defer ccloser() + + key := secretBackendRefCountKey(backendID) + _, count, err := nsRefcounts.CurrentOp(refCountCollection, key) + return count, errors.Trace(err) +} diff --git a/state/secrets.go b/state/secrets.go index 87ac1a2c32f0..9a7d5da069c5 100644 --- a/state/secrets.go +++ b/state/secrets.go @@ -807,8 +807,6 @@ func (s *secretsStore) ChangeSecretBackend(arg ChangeSecretBackendParams) error } buildTxn := func(attempt int) ([]txn.Op, error) { - // !!!!!! - var ops []txn.Op if doc.ValueRef != nil { op, err := s.st.decSecretBackendRefCountOp(doc.ValueRef.BackendID) diff --git a/state/secrets_test.go b/state/secrets_test.go index 2be43597785f..d1711e0e7b2a 100644 --- a/state/secrets_test.go +++ b/state/secrets_test.go @@ -934,32 +934,54 @@ func (s *SecretsSuite) TestChangeSecretBackend(c *gc.C) { UpdateSecretParams: state.UpdateSecretParams{ LeaderToken: &fakeToken{}, Data: map[string]string{"foo": "bar"}, + ValueRef: &secrets.ValueRef{ + BackendID: "old-backend-id", + RevisionID: "rev-id", + }, }, } _, err := s.store.CreateSecret(uri, p) c.Assert(err, jc.ErrorIsNil) + backendRefCount, err := s.State.ReadBackendRevisionCount("old-backend-id") + c.Assert(err, jc.ErrorIsNil) + c.Assert(backendRefCount, gc.Equals, 1) + backendRefCount, err = s.State.ReadBackendRevisionCount("new-backend-id") + c.Assert(err, jc.ErrorIsNil) + c.Assert(backendRefCount, gc.Equals, 0) + val, valRef, err := s.store.GetSecretValue(uri, 1) c.Assert(err, jc.ErrorIsNil) c.Assert(val, jc.DeepEquals, secrets.NewSecretValue(map[string]string{"foo": "bar"})) - c.Assert(valRef, gc.IsNil) + c.Assert(valRef, gc.DeepEquals, &secrets.ValueRef{ + BackendID: "old-backend-id", + RevisionID: "rev-id", + }) err = s.store.ChangeSecretBackend(state.ChangeSecretBackendParams{ URI: uri, Token: &fakeToken{}, Revision: 1, ValueRef: &secrets.ValueRef{ - BackendID: "backend-id", + BackendID: "new-backend-id", RevisionID: "rev-id", }, }) c.Assert(err, jc.ErrorIsNil) + backendRefCount, err = s.State.ReadBackendRevisionCount("old-backend-id") + c.Assert(err, jc.ErrorIsNil) + c.Assert(backendRefCount, gc.Equals, 0) + + backendRefCount, err = s.State.ReadBackendRevisionCount("new-backend-id") + c.Assert(err, jc.ErrorIsNil) + c.Assert(backendRefCount, gc.Equals, 1) + val, valRef, err = s.store.GetSecretValue(uri, 1) c.Assert(err, jc.ErrorIsNil) c.Assert(val.IsEmpty(), jc.IsTrue) c.Assert(valRef, jc.DeepEquals, &secrets.ValueRef{ - BackendID: "backend-id", + BackendID: "new-backend-id", RevisionID: "rev-id", }) } diff --git a/state/state.go b/state/state.go index e68312d2b44e..3f4ee259e224 100644 --- a/state/state.go +++ b/state/state.go @@ -24,7 +24,6 @@ import ( jujutxn "github.com/juju/txn/v3" "github.com/juju/utils/v3" "github.com/juju/version/v2" - "github.com/kr/pretty" "github.com/juju/juju/core/arch" corecharm "github.com/juju/juju/core/charm" @@ -268,7 +267,6 @@ func cleanupSecretBackendRefCountAfterModelMigrationDone(st *State) error { } var ops []txn.Op - logger.Criticalf("cleanupSecretBackendRefCountAfterModelMigrationDone: %s", pretty.Sprint(result)) for _, r := range result { for i := r.Count; i > 0; i-- { refCountOps, err := st.decSecretBackendRefCountOp(r.ID) @@ -278,7 +276,6 @@ func cleanupSecretBackendRefCountAfterModelMigrationDone(st *State) error { ops = append(ops, refCountOps) } } - logger.Criticalf("cleanupSecretBackendRefCountAfterModelMigrationDone: ops %s", pretty.Sprint(ops)) return st.db().RunTransaction(ops) } diff --git a/worker/secretdrainworker/worker.go b/worker/secretdrainworker/worker.go index 3fd3eaff1d4e..f70f9e9e3406 100644 --- a/worker/secretdrainworker/worker.go +++ b/worker/secretdrainworker/worker.go @@ -177,11 +177,10 @@ func (w *Worker) drainSecret(md coresecrets.SecretMetadataForDrain, client jujus if err != nil { return errors.Trace(err) } - // revID := rev.ValueRef.RevisionID cleanUpInExternalBackend = func() error { w.config.Logger.Debugf("cleanup secret %s/%d from old backend %q", md.Metadata.URI.ID, rev.Revision, rev.ValueRef.BackendID) if valueRef.BackendID == rev.ValueRef.BackendID { - // Ideally, We should have done all these drain steps in the controller via transaction but we decided to only allow + // Ideally, We should have done all these drain steps in the controller via transaction, but by design, we only allow // uniters to be able to access secret content. So we have to do these extra checks to avoid // secret gets deleted wrongly when the model's secret backend is changed back to // the old backend while the secret is being drained. diff --git a/worker/secretdrainworker/worker_test.go b/worker/secretdrainworker/worker_test.go index 18879883e3c4..5a57a501ff59 100644 --- a/worker/secretdrainworker/worker_test.go +++ b/worker/secretdrainworker/worker_test.go @@ -277,16 +277,19 @@ func (s *workerSuite) TestDrainPartiallyFailed(c *gc.C) { }, }, }, nil), + + // revision 1 s.backendClient.EXPECT().GetBackend(nil).DoAndReturn(func(_ *string) (*provider.SecretsBackend, string, error) { return nil, "backend-3", nil }), - - // revision 1 s.backendClient.EXPECT().GetRevisionContent(uri, 1).Return(secretValue, nil), s.backendClient.EXPECT().SaveContent(uri, 1, secretValue).Return(coresecrets.ValueRef{}, errors.NotSupportedf("")), s.backendClient.EXPECT().GetBackend(ptr("backend-1")).Return(oldBackend, "", nil), // revision 2 + s.backendClient.EXPECT().GetBackend(nil).DoAndReturn(func(_ *string) (*provider.SecretsBackend, string, error) { + return nil, "backend-3", nil + }), s.backendClient.EXPECT().GetRevisionContent(uri, 2).Return(secretValue, nil), s.backendClient.EXPECT().SaveContent(uri, 2, secretValue).Return(coresecrets.ValueRef{}, errors.NotSupportedf("")), s.backendClient.EXPECT().GetBackend(ptr("backend-2")).Return(oldBackend, "", nil), @@ -314,7 +317,7 @@ func (s *workerSuite) TestDrainPartiallyFailed(c *gc.C) { return nil }), ) - start(`failed to drain secret revisions for "secret:.*" to the active backend "backend-3"`) + start(`failed to drain secret revisions for "secret:.*" to the active backend`) } func ptr[T any](v T) *T {