Fix some more test failures and recursive locking (#6549)

command/kv_test.go

@@ -4,6 +4,7 @@ import (
 	"io"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/hashicorp/vault/api"
 	"github.com/mitchellh/cli"
@@ -401,6 +402,9 @@ func TestKVGetCommand(t *testing.T) {
 					t.Fatal(err)
 				}
 
+				// Give time for the upgrade code to run/finish
+				time.Sleep(time.Second)
+
 				if _, err := client.Logical().Write("secret/read/foo", map[string]interface{}{
 					"foo": "bar",
 				}); err != nil {
@@ -496,6 +500,9 @@ func TestKVMetadataGetCommand(t *testing.T) {
 					t.Fatal(err)
 				}
 
+				// Give time for the upgrade code to run/finish
+				time.Sleep(time.Second)
+
 				if _, err := client.Logical().Write("kv/data/foo", map[string]interface{}{
 					"data": map[string]interface{}{
 						"foo": "bar",

vault/plugin_catalog.go

@@ -180,8 +180,12 @@ func (c *PluginCatalog) UpgradePlugins(ctx context.Context, logger log.Logger) e
 // It returns a PluginRunner or an error if no plugin was found.
 func (c *PluginCatalog) Get(ctx context.Context, name string, pluginType consts.PluginType) (*pluginutil.PluginRunner, error) {
 	c.lock.RLock()
-	defer c.lock.RUnlock()
+	runner, err := c.get(ctx, name, pluginType)
+	c.lock.RUnlock()
+	return runner, err
+}
 
+func (c *PluginCatalog) get(ctx context.Context, name string, pluginType consts.PluginType) (*pluginutil.PluginRunner, error) {
 	// If the directory isn't set only look for builtin plugins.
 	if c.directory != "" {
 		// Look for external plugins in the barrier
@@ -348,7 +352,7 @@ func (c *PluginCatalog) List(ctx context.Context, pluginType consts.PluginType)
 	for _, plugin := range keys {
 
 		// Only list user-added plugins if they're of the given type.
-		if entry, err := c.Get(ctx, plugin, pluginType); err == nil && entry != nil {
+		if entry, err := c.get(ctx, plugin, pluginType); err == nil && entry != nil {
 
 			// Some keys will be prepended with the plugin type, but other ones won't.
 			// Users don't expect to see the plugin type, so we need to strip that here.

vault/rekey.go

@@ -908,8 +908,6 @@ func (c *Core) RekeyVerifyRestart(recovery bool) logical.HTTPCodedError {
 // RekeyRetrieveBackup is used to retrieve any backed-up PGP-encrypted unseal
 // keys
 func (c *Core) RekeyRetrieveBackup(ctx context.Context, recovery bool) (*RekeyBackup, logical.HTTPCodedError) {
-	c.stateLock.RLock()
-	defer c.stateLock.RUnlock()
 	if c.Sealed() {
 		return nil, logical.CodedError(http.StatusServiceUnavailable, consts.ErrSealed.Error())
 	}
@@ -945,8 +943,6 @@ func (c *Core) RekeyRetrieveBackup(ctx context.Context, recovery bool) (*RekeyBa
 
 // RekeyDeleteBackup is used to delete any backed-up PGP-encrypted unseal keys
 func (c *Core) RekeyDeleteBackup(ctx context.Context, recovery bool) logical.HTTPCodedError {
-	c.stateLock.RLock()
-	defer c.stateLock.RUnlock()
 	if c.Sealed() {
 		return logical.CodedError(http.StatusServiceUnavailable, consts.ErrSealed.Error())
 	}

vault/router.go

@@ -498,9 +498,14 @@ func (r *Router) routeCommon(ctx context.Context, req *logical.Request, existenc
 	re := raw.(*routeEntry)
 
 	// Grab a read lock on the route entry, this protects against the backend
-	// being reloaded during a request.
-	re.l.RLock()
-	defer re.l.RUnlock()
+	// being reloaded during a request. The exception is a renew request on the
+	// token store; such a request will have already been routed through the
+	// token store -> exp manager -> here so we need to not grab the lock again
+	// or we'll be recursively grabbing it.
+	if !(req.Operation == logical.RenewOperation && strings.HasPrefix(req.Path, "auth/token")) {
+		re.l.RLock()
+		defer re.l.RUnlock()
+	}
 
 	// Filtered mounts will have a nil backend
 	if re.backend == nil {