Thread safe cache

fishi0x01 · Feb 21, 2021 · 8f55907 · 8f55907
1 parent 37192ef
commit 8f55907
Show file tree

Hide file tree

Showing 28 changed files with 235 additions and 101 deletions.
diff --git a/Makefile b/Makefile
@@ -31,7 +31,7 @@ compile-releases: clean ## Compile vsh binaries for multiple platforms and archi
 	done
 	cd build/ && sha256sum * > SHA256SUM
 
-compile: clean ## Compile vsh for platform based on uname
+compile: ## Compile vsh for platform based on uname
 	go build -ldflags "-X main.vshVersion=$(VERSION)" -o build/${APP_NAME}_$(shell uname | tr '[:upper:]' '[:lower:]')_$(ARCH)
 
 get-bats: ## Download bats dependencies to test directory
@@ -51,8 +51,11 @@ integration-tests: ## Run integration test suites (requires bats - see get-bats)
 single-test: ## Run a single test suite, e.g., make single-test KV_BACKEND=KV2 VAULT_VERSION=1.6.1 TEST_SUITE=commands/cp
 	KV_BACKEND=$(KV_BACKEND) VAULT_VERSION=$(VAULT_VERSION) TEST_SUITE=$(TEST_SUITE) test/run-single-test.sh
 
-local-vault-test-instance: ## Start a local vault container with integration test provisioning
-	bash -c ". test/util/util.bash && setup"
+local-vault-standard-test-instance: ## Start a local vault container with standard setup provisioning
+	bash -c ". test/util/common.bash && . test/util/standard-setup.bash && setup"
+
+local-vault-concurrency-test-instance: ## Start a local vault container with concurrency setup provisioning
+	bash -c ". test/util/common.bash && . test/util/concurrency-setup.bash && setup"
 
 clean: ## Remove builds and vsh related docker containers
 	docker rm -f vsh-integration-test-vault || true

diff --git a/client/cache.go b/client/cache.go
@@ -0,0 +1,58 @@
+package client
+
+import (
+	"github.com/hashicorp/vault/api"
+	"sync"
+)
+
+// Cache is a thread-safe cache for vault queries
+type Cache struct {
+	vaultClient *api.Client
+	listQueries map[string]*cacheElement
+	mutex       sync.Mutex
+}
+
+type cacheElement struct {
+	Secret *api.Secret
+	Err    error
+}
+
+// NewCache create a new thread-safe cache object
+func NewCache(vaultClient *api.Client) *Cache {
+	return &Cache{
+		vaultClient: vaultClient,
+		listQueries: make(map[string]*cacheElement),
+	}
+}
+
+// Clear removes all entries from the cache
+func (cache *Cache) Clear() {
+	cache.mutex.Lock()
+	cache.listQueries = make(map[string]*cacheElement)
+	cache.mutex.Unlock()
+}
+
+// List tries to get path from cache.
+// If path is not available in cache, it uses the vault client to query and update cache.
+func (cache *Cache) List(path string) (result *api.Secret, err error) {
+	// try to get path from cache
+	cache.mutex.Lock()
+	value, hit := cache.listQueries[path]
+	cache.mutex.Unlock()
+	if hit {
+		return value.Secret, value.Err
+	}
+
+	// not found in cache -> query vault
+	// NOTE: this part is not mutexed
+	result, err = cache.vaultClient.Logical().List(path)
+
+	// update cache
+	cache.mutex.Lock()
+	cache.listQueries[path] = &cacheElement{
+		Secret: result,
+		Err:    err,
+	}
+	cache.mutex.Unlock()
+	return result, err
+}
diff --git a/client/client.go b/client/client.go
@@ -17,7 +17,7 @@ type Client struct {
 	Name       string
 	Pwd        string
 	KVBackends map[string]int
-	listCache  map[string][]string
+	cache      *Cache
 }
 
 // VaultConfig container to keep parameters for Client configuration
@@ -92,7 +92,7 @@ func NewClient(conf *VaultConfig) (*Client, error) {
 		Name:       conf.Addr,
 		Pwd:        conf.StartPath,
 		KVBackends: backends,
-		listCache:  make(map[string][]string),
+		cache:      NewCache(vault),
 	})
 }
 
@@ -134,16 +134,11 @@ func (client *Client) Delete(absolutePath string) (err error) {
 
 // List elements at the given absolutePath, using the given client
 func (client *Client) List(absolutePath string) (result []string, err error) {
-	if val, ok := client.listCache[absolutePath]; ok {
-		return val, nil
-	}
 	if client.isTopLevelPath(absolutePath) {
 		result = client.listTopLevel()
 	} else {
 		result, err = client.listLowLevel(normalizedVaultPath(absolutePath))
 	}
-	client.listCache[absolutePath] = result
-
 	return result, err
 }
 
@@ -186,5 +181,5 @@ func (client *Client) SubpathsForPath(path string) (filePaths []string, err erro
 
 // ClearCache clears the list cache
 func (client *Client) ClearCache() {
-	client.listCache = make(map[string][]string)
+	client.cache.Clear()
 }
diff --git a/client/list.go b/client/list.go
@@ -18,7 +18,7 @@ func (client *Client) listLowLevel(path string) (result []string, err error) {
 		return nil, errors.New("Not a directory: " + path)
 	}
 
-	s, err := client.Vault.Logical().List(client.getKVMetaDataPath(path))
+	s, err := client.cache.List(client.getKVMetaDataPath(path))
 	if err != nil {
 		log.AppTrace("%+v", err)
 		return result, err

diff --git a/client/traverse.go b/client/traverse.go
@@ -14,10 +14,10 @@ func (client *Client) topLevelTraverse() (result []string) {
 }
 
 func (client *Client) lowLevelTraverse(path string) (result []string) {
-	s, err := client.Vault.Logical().List(client.getKVMetaDataPath(path))
+	s, err := client.cache.List(client.getKVMetaDataPath(path))
 	if err != nil {
 		log.AppTrace("%+v", err)
-		return nil
+		return
 	}
 
 	if s != nil {
@@ -39,6 +39,5 @@ func (client *Client) lowLevelTraverse(path string) (result []string) {
 		leaf := strings.ReplaceAll("/"+path, "//", "/")
 		result = append(result, leaf)
 	}
-
 	return result
 }
diff --git a/client/type.go b/client/type.go
@@ -26,46 +26,46 @@ func (client *Client) topLevelType(path string) PathKind {
 	}
 }
 
-var cachedPath = ""
-var cachedDirFiles = make(map[string]int)
-
-func (client *Client) isAmbiguous(path string) (result bool) {
-	// get current directory content
-	if cachedPath != path {
-		pathTrim := strings.TrimSuffix(path, "/")
-		cachedDirFiles = make(map[string]int)
-		s, err := client.Vault.Logical().List(client.getKVMetaDataPath(filepath.Dir(pathTrim)))
-		if err == nil && s != nil {
-			if keysInterface, ok := s.Data["keys"]; ok {
-				for _, valInterface := range keysInterface.([]interface{}) {
-					val := valInterface.(string)
-					cachedDirFiles[val] = 1
-				}
-			}
-		}
-		cachedPath = path
-	}
-
+func (client *Client) isAmbiguous(path string, dirFiles map[string]int) (result bool) {
 	// check if path exists as file and directory
 	result = false
-	if _, ok := cachedDirFiles[filepath.Base(path)]; ok {
-		if _, ok := cachedDirFiles[filepath.Base(path)+"/"]; ok {
+	if _, ok := dirFiles[filepath.Base(path)]; ok {
+		if _, ok := dirFiles[filepath.Base(path)+"/"]; ok {
 			result = true
 		}
 	}
 	return result
 }
 
+func (client *Client) getDirFiles(path string) (result map[string]int) {
+	// get current directory content
+	result = make(map[string]int)
+	pathTrim := strings.TrimSuffix(path, "/")
+	lsPath := client.getKVMetaDataPath(filepath.Dir(pathTrim))
+	s, err := client.cache.List(lsPath)
+	if err == nil && s != nil {
+		if keysInterface, ok := s.Data["keys"]; ok {
+			for _, valInterface := range keysInterface.([]interface{}) {
+				val := valInterface.(string)
+				result[val] = 1
+			}
+		}
+	}
+	return result
+}
+
 func (client *Client) lowLevelType(path string) (result PathKind) {
-	if client.isAmbiguous(path) {
+	dirFiles := client.getDirFiles(path)
+	if client.isAmbiguous(path, dirFiles) {
 		if strings.HasSuffix(path, "/") {
 			result = NODE
 		} else {
 			result = LEAF
 		}
 	} else {
 		hasNode := false
-		s, err := client.Vault.Logical().List(client.getKVMetaDataPath(path + "/"))
+		kvPath := client.getKVMetaDataPath(path + "/")
+		s, err := client.cache.List(kvPath)
 		if err == nil && s != nil {
 			if _, ok := s.Data["keys"]; ok {
 				hasNode = true
@@ -74,7 +74,7 @@ func (client *Client) lowLevelType(path string) (result PathKind) {
 		if hasNode {
 			result = NODE
 		} else {
-			if _, ok := cachedDirFiles[filepath.Base(path)]; ok {
+			if _, ok := dirFiles[filepath.Base(path)]; ok {
 				result = LEAF
 			} else {
 				result = NONE

diff --git a/test/run-all-tests.sh b/test/run-all-tests.sh
@@ -18,3 +18,8 @@ do
         VAULT_VERSION=${vault_version} KV_BACKEND="${kv_backend}" ${BATS} "${DIR}/suites/commands/"
     done
 done
+
+# Concurrency tests are primarily designed to operate on a large set of files.
+# At this point, we only care about catching potential concurrency issues.
+# Testing different vault and KV versions has been done in prior tests.
+VAULT_VERSION=${VAULT_VERSIONS[0]} ${BATS} "${DIR}/suites/concurrency/"
diff --git a/test/suites/commands/append.bats b/test/suites/commands/append.bats
@@ -1,4 +1,5 @@
-load ../../util/util
+load ../../util/common
+load ../../util/standard-setup
 load ../../bin/plugins/bats-support/load
 load ../../bin/plugins/bats-assert/load
 

diff --git a/test/suites/commands/cat.bats b/test/suites/commands/cat.bats
@@ -1,4 +1,5 @@
-load ../../util/util
+load ../../util/common
+load ../../util/standard-setup
 load ../../bin/plugins/bats-support/load
 load ../../bin/plugins/bats-assert/load
 

diff --git a/test/suites/commands/cd.bats b/test/suites/commands/cd.bats
@@ -1,4 +1,5 @@
-load ../../util/util
+load ../../util/common
+load ../../util/standard-setup
 load ../../bin/plugins/bats-support/load
 load ../../bin/plugins/bats-assert/load
 

diff --git a/test/suites/commands/cp.bats b/test/suites/commands/cp.bats
@@ -1,4 +1,5 @@
-load ../../util/util
+load ../../util/common
+load ../../util/standard-setup
 load ../../bin/plugins/bats-support/load
 load ../../bin/plugins/bats-assert/load
 

diff --git a/test/suites/commands/grep.bats b/test/suites/commands/grep.bats
@@ -1,4 +1,5 @@
-load ../../util/util
+load ../../util/common
+load ../../util/standard-setup
 load ../../bin/plugins/bats-support/load
 load ../../bin/plugins/bats-assert/load
 

diff --git a/test/suites/commands/ls.bats b/test/suites/commands/ls.bats
@@ -1,4 +1,5 @@
-load ../../util/util
+load ../../util/common
+load ../../util/standard-setup
 load ../../bin/plugins/bats-support/load
 load ../../bin/plugins/bats-assert/load
 

diff --git a/test/suites/commands/mv.bats b/test/suites/commands/mv.bats
@@ -1,4 +1,5 @@
-load ../../util/util
+load ../../util/common
+load ../../util/standard-setup
 load ../../bin/plugins/bats-support/load
 load ../../bin/plugins/bats-assert/load
 

diff --git a/test/suites/commands/replace.bats b/test/suites/commands/replace.bats
@@ -1,4 +1,5 @@
-load ../../util/util
+load ../../util/common
+load ../../util/standard-setup
 load ../../bin/plugins/bats-support/load
 load ../../bin/plugins/bats-assert/load
 

diff --git a/test/suites/commands/rm.bats b/test/suites/commands/rm.bats
@@ -1,4 +1,5 @@
-load ../../util/util
+load ../../util/common
+load ../../util/standard-setup
 load ../../bin/plugins/bats-support/load
 load ../../bin/plugins/bats-assert/load
 

diff --git a/test/suites/concurrency/mv.bats b/test/suites/concurrency/mv.bats
@@ -0,0 +1,21 @@
+load ../../util/common
+load ../../util/concurrency-setup
+load ../../bin/plugins/bats-support/load
+load ../../bin/plugins/bats-assert/load
+
+@test "vault-${VAULT_VERSION} concurrency 'mv'" {
+  #######################################
+  echo "==== case: move large directory tree ===="
+  run ${APP_BIN} -c "mv /KV2/src/ /KV2/dest/"
+  assert_success
+
+  echo "ensure at least one file got moved to destination"
+  run get_vault_value "value" "/KV2/dest/a/a/50"
+  assert_success
+  assert_output "1"
+
+  echo "ensure at least one src file got removed"
+  run get_vault_value "value" "/KV2/src/a/a/50"
+  assert_success
+  assert_output --partial "${NO_VALUE_FOUND}"
+}
diff --git a/test/suites/edge-cases/false-commands.bats b/test/suites/edge-cases/false-commands.bats
@@ -1,4 +1,5 @@
-load ../../util/util
+load ../../util/common
+load ../../util/standard-setup
 load ../../bin/plugins/bats-support/load
 load ../../bin/plugins/bats-assert/load
 

diff --git a/test/suites/edge-cases/kv-migrations-with-append.bats b/test/suites/edge-cases/kv-migrations-with-append.bats
@@ -1,4 +1,5 @@
-load ../../util/util
+load ../../util/common
+load ../../util/standard-setup
 load ../../bin/plugins/bats-support/load
 load ../../bin/plugins/bats-assert/load
 

diff --git a/test/suites/edge-cases/kv-migrations-with-cp.bats b/test/suites/edge-cases/kv-migrations-with-cp.bats
@@ -1,4 +1,5 @@
-load ../../util/util
+load ../../util/common
+load ../../util/standard-setup
 load ../../bin/plugins/bats-support/load
 load ../../bin/plugins/bats-assert/load
 

diff --git a/test/suites/edge-cases/kv-migrations-with-mv.bats b/test/suites/edge-cases/kv-migrations-with-mv.bats
@@ -1,4 +1,5 @@
-load ../../util/util
+load ../../util/common
+load ../../util/standard-setup
 load ../../bin/plugins/bats-support/load
 load ../../bin/plugins/bats-assert/load
 

diff --git a/test/suites/edge-cases/logging.bats b/test/suites/edge-cases/logging.bats
@@ -1,4 +1,5 @@
-load ../../util/util
+load ../../util/common
+load ../../util/standard-setup
 load ../../bin/plugins/bats-support/load
 load ../../bin/plugins/bats-assert/load
 load ../../bin/plugins/bats-file/load

diff --git a/test/suites/edge-cases/params.bats b/test/suites/edge-cases/params.bats
@@ -1,4 +1,5 @@
-load ../../util/util
+load ../../util/common
+load ../../util/standard-setup
 load ../../bin/plugins/bats-support/load
 load ../../bin/plugins/bats-assert/load
 

diff --git a/test/suites/edge-cases/print-version.bats b/test/suites/edge-cases/print-version.bats
@@ -1,4 +1,5 @@
-load ../../util/util
+load ../../util/common
+load ../../util/standard-setup
 load ../../bin/plugins/bats-support/load
 load ../../bin/plugins/bats-assert/load
 

diff --git a/test/suites/past-issues/45_rm-bug.bats b/test/suites/past-issues/45_rm-bug.bats
@@ -1,4 +1,5 @@
-load ../../util/util
+load ../../util/common
+load ../../util/standard-setup
 load ../../bin/plugins/bats-support/load
 load ../../bin/plugins/bats-assert/load