From 8f559072d7817bd1fa12ebe77fc141fc12bb6baf Mon Sep 17 00:00:00 2001 From: Karl Fischer Date: Fri, 19 Feb 2021 16:25:12 +0100 Subject: [PATCH] Thread safe cache --- Makefile | 9 ++- client/cache.go | 58 +++++++++++++++++++ client/client.go | 11 +--- client/list.go | 2 +- client/traverse.go | 5 +- client/type.go | 50 ++++++++-------- test/run-all-tests.sh | 5 ++ test/suites/commands/append.bats | 3 +- test/suites/commands/cat.bats | 3 +- test/suites/commands/cd.bats | 3 +- test/suites/commands/cp.bats | 3 +- test/suites/commands/grep.bats | 3 +- test/suites/commands/ls.bats | 3 +- test/suites/commands/mv.bats | 3 +- test/suites/commands/replace.bats | 3 +- test/suites/commands/rm.bats | 3 +- test/suites/concurrency/mv.bats | 21 +++++++ test/suites/edge-cases/false-commands.bats | 3 +- .../edge-cases/kv-migrations-with-append.bats | 3 +- .../edge-cases/kv-migrations-with-cp.bats | 3 +- .../edge-cases/kv-migrations-with-mv.bats | 3 +- test/suites/edge-cases/logging.bats | 3 +- test/suites/edge-cases/params.bats | 3 +- test/suites/edge-cases/print-version.bats | 3 +- test/suites/past-issues/45_rm-bug.bats | 3 +- test/util/common.bash | 43 ++++++++++++++ test/util/concurrency-setup.bash | 37 ++++++++++++ test/util/{util.bash => standard-setup.bash} | 44 -------------- 28 files changed, 235 insertions(+), 101 deletions(-) create mode 100644 client/cache.go create mode 100644 test/suites/concurrency/mv.bats create mode 100755 test/util/common.bash create mode 100755 test/util/concurrency-setup.bash rename test/util/{util.bash => standard-setup.bash} (75%) diff --git a/Makefile b/Makefile index 579dfe07..96362123 100644 --- 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 new file mode 100644 index 00000000..4414e10c --- /dev/null +++ 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 index b18b49bb..17b77923 100644 --- 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 index 1db265c5..727b37c4 100644 --- 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 index c38e4654..556f0d94 100644 --- 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 index ccc41898..94c6dfdc 100644 --- a/client/type.go +++ b/client/type.go @@ -26,38 +26,37 @@ 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 { @@ -65,7 +64,8 @@ func (client *Client) lowLevelType(path string) (result PathKind) { } } 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 index 9eee2fde..e816c9b5 100755 --- 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 index f73b61fe..5a246a2a 100644 --- 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 index 49343996..1c18b216 100644 --- 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 index 840d3ecc..896eb2ba 100644 --- 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 index ca1f5fb9..176fa373 100644 --- 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 index ba058692..2c2a16c5 100644 --- 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 index b62b86f3..9c6549d7 100644 --- 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 index 483c4167..2d825042 100644 --- 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 index a1592c6b..02dca993 100644 --- 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 index b72be858..044c15c5 100644 --- 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 new file mode 100644 index 00000000..96c54a21 --- /dev/null +++ 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 index c5d1a152..d6d1198c 100644 --- 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 index eef94610..c622ae65 100644 --- 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 index af021e88..5c0898c7 100644 --- 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 index 28f475f4..ed894d2d 100644 --- 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 index aa4cad95..343712f5 100644 --- 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 index 3fdfed9d..22661111 100644 --- 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 index 16c339fd..9ee69903 100644 --- 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 index 312fe0c6..6cb60ab2 100644 --- 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 diff --git a/test/util/common.bash b/test/util/common.bash new file mode 100755 index 00000000..b6538d9b --- /dev/null +++ b/test/util/common.bash @@ -0,0 +1,43 @@ +#!/bin/bash + +export VAULT_VERSION=${VAULT_VERSION:-"1.6.1"} +export VAULT_CONTAINER_NAME="vsh-integration-test-vault" +export VAULT_HOST_PORT=${VAULT_HOST_PORT:-"8888"} + +export VAULT_TOKEN="root" +export VAULT_ADDR="http://localhost:${VAULT_HOST_PORT}" + +DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" +export DIR +UNAME=$(uname | tr '[:upper:]' '[:lower:]') +case "$(uname -m)" in + x86_64) + ARCH=amd64 ;; + arm64|aarch64|armv8b|armv8l) + ARCH=arm64 ;; + arm*) + ARCH=arm ;; + i386|i686) + ARCH=386 ;; + *) + ARCH=$(uname -m) ;; +esac +export ARCH +export APP_BIN="${DIR}/../../build/vsh_${UNAME}_${ARCH}" +export NO_VALUE_FOUND="No value found at" + +teardown() { + docker rm -f ${VAULT_CONTAINER_NAME} &> /dev/null +} + +vault_exec() { + vault_exec_output "$@" &> /dev/null +} + +vault_exec_output() { + docker exec ${VAULT_CONTAINER_NAME} /bin/sh -c "$1" +} + +get_vault_value() { + vault_exec_output "vault kv get -field=\"${1}\" \"${2}\" || true" +} diff --git a/test/util/concurrency-setup.bash b/test/util/concurrency-setup.bash new file mode 100755 index 00000000..b84cb11d --- /dev/null +++ b/test/util/concurrency-setup.bash @@ -0,0 +1,37 @@ +data_for_path() { + result="" + for i in $(seq 1 400); do + result="${result} vault kv put ${1}/${i} value=1;"; + done + echo $result +} + +setup() { + rm -f vsh_trace.log + docker run -d \ + --name=${VAULT_CONTAINER_NAME} \ + -p "${VAULT_HOST_PORT}:8200" \ + --cap-add=IPC_LOCK \ + -e "VAULT_ADDR=http://127.0.0.1:8200" \ + -e "VAULT_TOKEN=root" \ + -e "VAULT_DEV_ROOT_TOKEN_ID=root" \ + -e "VAULT_DEV_LISTEN_ADDRESS=0.0.0.0:8200" \ + "vault:${VAULT_VERSION}" &> /dev/null + docker cp "$DIR/policy-no-root.hcl" ${VAULT_CONTAINER_NAME}:. + docker cp "$DIR/policy-delete-only.hcl" ${VAULT_CONTAINER_NAME}:. + # need some time for GH Actions CI + sleep 3 + vault_exec "vault secrets disable secret; + vault policy write no-root policy-no-root.hcl; + vault token create -id=no-root -policy=no-root; + vault policy write delete-only policy-delete-only.hcl; + vault token create -id=delete-only -policy=delete-only; + vault secrets enable -version=1 -path=KV1 kv; + vault secrets enable -version=2 -path=KV2 kv" + + dirs="src src/a src/b src/1 src/a/a src/a/a/a src/b/a src/b/b src/b/b/a" + for dir in $dirs; do + vault_exec "$(data_for_path "/KV2/${dir}")" & + done + wait +} diff --git a/test/util/util.bash b/test/util/standard-setup.bash similarity index 75% rename from test/util/util.bash rename to test/util/standard-setup.bash index 4c96b932..c8023987 100755 --- a/test/util/util.bash +++ b/test/util/standard-setup.bash @@ -1,31 +1,3 @@ -#!/bin/bash - -export VAULT_VERSION=${VAULT_VERSION:-"1.6.1"} -export VAULT_CONTAINER_NAME="vsh-integration-test-vault" -export VAULT_HOST_PORT=${VAULT_HOST_PORT:-"8888"} - -export VAULT_TOKEN="root" -export VAULT_ADDR="http://localhost:${VAULT_HOST_PORT}" - -DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" -export DIR -UNAME=$(uname | tr '[:upper:]' '[:lower:]') -case "$(uname -m)" in - x86_64) - ARCH=amd64 ;; - arm64|aarch64|armv8b|armv8l) - ARCH=arm64 ;; - arm*) - ARCH=arm ;; - i386|i686) - ARCH=386 ;; - *) - ARCH=$(uname -m) ;; -esac -export ARCH -export APP_BIN="${DIR}/../../build/vsh_${UNAME}_${ARCH}" -export NO_VALUE_FOUND="No value found at" - setup() { rm -f vsh_trace.log docker run -d \ @@ -76,19 +48,3 @@ setup() { echo -n 'a \"quoted\" value' | vault kv put ${kv_backend}/src/quoted/foo bar=-" done } - -teardown() { - docker rm -f ${VAULT_CONTAINER_NAME} &> /dev/null -} - -vault_exec() { - vault_exec_output "$@" &> /dev/null -} - -vault_exec_output() { - docker exec ${VAULT_CONTAINER_NAME} /bin/sh -c "$1" -} - -get_vault_value() { - vault_exec_output "vault kv get -field=\"${1}\" \"${2}\" || true" -}