From e48cb2e840a1ba1e840834dccceb2f3e827a56d2 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 3 May 2016 00:19:18 -0400 Subject: [PATCH] Add some more tests around deletion and fix upsert status returning --- builtin/logical/transit/lock_manager.go | 11 ++-- builtin/logical/transit/policy_test.go | 83 ++++++++++++++++++++----- 2 files changed, 74 insertions(+), 20 deletions(-) diff --git a/builtin/logical/transit/lock_manager.go b/builtin/logical/transit/lock_manager.go index 9f20a423a5..515515726f 100644 --- a/builtin/logical/transit/lock_manager.go +++ b/builtin/logical/transit/lock_manager.go @@ -133,21 +133,24 @@ func (lm *lockManager) GetPolicyExclusive(storage logical.Storage, name string) // needed, retry. If successful, call one more time to get a read lock and // return the value. func (lm *lockManager) GetPolicyUpsert(storage logical.Storage, name string, derived bool) (*Policy, *sync.RWMutex, bool, error) { - p, lock, upserted, err := lm.getPolicyCommon(storage, name, true, derived, shared) + p, lock, _, err := lm.getPolicyCommon(storage, name, true, derived, shared) if err == nil || (err != nil && err != errNeedExclusiveLock) { - return p, lock, upserted, err + return p, lock, false, err } // Try again while asking for an exlusive lock - p, lock, upserted, err = lm.getPolicyCommon(storage, name, true, derived, exclusive) + p, lock, upserted, err := lm.getPolicyCommon(storage, name, true, derived, exclusive) if err != nil || p == nil || lock == nil { return p, lock, upserted, err } lock.Unlock() - return lm.getPolicyCommon(storage, name, true, derived, shared) + // Now get a shared lock for the return, but preserve the value of upsert + p, lock, _, err = lm.getPolicyCommon(storage, name, true, derived, shared) + + return p, lock, upserted, err } // When the function returns, a lock will be held on the policy if err == nil. diff --git a/builtin/logical/transit/policy_test.go b/builtin/logical/transit/policy_test.go index b6d0acf418..f326416208 100644 --- a/builtin/logical/transit/policy_test.go +++ b/builtin/logical/transit/policy_test.go @@ -22,21 +22,19 @@ func Test_KeyUpgrade(t *testing.T) { func testKeyUpgradeCommon(t *testing.T, lm *lockManager) { storage := &logical.InmemStorage{} - p, lockType, upserted, err := lm.GetPolicyUpsert(storage, "test", false) + p, lock, upserted, err := lm.GetPolicyUpsert(storage, "test", false) + if lock != nil { + defer lock.RUnlock() + } if err != nil { t.Fatal(err) } if p == nil { t.Fatal("nil policy") } - defer lm.UnlockPolicy("test", lockType) - if !upserted { t.Fatal("expected an upsert") } - if lockType != exclusive { - t.Fatal("expected an exclusive lock") - } testBytes := make([]byte, len(p.Keys[1].Key)) copy(testBytes, p.Keys[1].Key) @@ -70,14 +68,14 @@ func testArchivingUpgradeCommon(t *testing.T, lm *lockManager) { storage := &logical.InmemStorage{} - p, lockType, _, err := lm.GetPolicyUpsert(storage, "test", false) + p, lock, _, err := lm.GetPolicyUpsert(storage, "test", false) if err != nil { t.Fatal(err) } - if p == nil { - t.Fatal("nil policy") + if p == nil || lock == nil { + t.Fatal("nil policy or lock") } - lm.UnlockPolicy("test", lockType) + lock.RUnlock() // Store the initial key in the archive keysArchive = append(keysArchive, p.Keys[1]) @@ -122,16 +120,67 @@ func testArchivingUpgradeCommon(t *testing.T, lm *lockManager) { } // Now get the policy again; the upgrade should happen automatically - p, lockType, err = lm.GetPolicy(storage, "test") + p, lock, err = lm.GetPolicyShared(storage, "test") if err != nil { t.Fatal(err) } - if p == nil { - t.Fatal("nil lockingPolicy") + if p == nil || lock == nil { + t.Fatal("nil policy or lock") } - lm.UnlockPolicy("test", lockType) + lock.RUnlock() checkKeys(t, p, storage, "upgrade", 10, 10, 10) + + // Let's check some deletion logic while we're at it + + // The policy should be in there + if lm.CacheActive() && lm.cache["test"] == nil { + t.Fatal("nil policy in cache") + } + + // First we'll do this wrong, by not setting the deletion flag + err = lm.DeletePolicy(storage, "test") + if err == nil { + t.Fatal("got nil error, but should not have been able to delete since we didn't set the deletion flag on the policy") + } + + // The policy should still be in there + if lm.CacheActive() && lm.cache["test"] == nil { + t.Fatal("nil policy in cache") + } + + p, lock, err = lm.GetPolicyShared(storage, "test") + if err != nil { + t.Fatal(err) + } + if p == nil || lock == nil { + t.Fatal("policy or lock nil after bad delete") + } + lock.RUnlock() + + // Now do it properly + p.DeletionAllowed = true + err = p.Persist(storage) + if err != nil { + t.Fatal(err) + } + err = lm.DeletePolicy(storage, "test") + if err != nil { + t.Fatal(err) + } + + // The policy should *not* be in there + if lm.CacheActive() && lm.cache["test"] != nil { + t.Fatal("non-nil policy in cache") + } + + p, lock, err = lm.GetPolicyShared(storage, "test") + if err != nil { + t.Fatal(err) + } + if p != nil || lock != nil { + t.Fatal("policy or lock not nil after delete") + } } func Test_Archiving(t *testing.T) { @@ -149,14 +198,16 @@ func testArchivingCommon(t *testing.T, lm *lockManager) { storage := &logical.InmemStorage{} - p, lockType, _, err := lm.GetPolicyUpsert(storage, "test", false) + p, lock, _, err := lm.GetPolicyUpsert(storage, "test", false) + if lock != nil { + defer lock.RUnlock() + } if err != nil { t.Fatal(err) } if p == nil { t.Fatal("nil policy") } - defer lm.UnlockPolicy("test", lockType) // Store the initial key in the archive keysArchive = append(keysArchive, p.Keys[1])