Add some more tests around deletion and fix upsert status returning

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.

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])