Skip to content

Add support for Vault KV engine version 2#1

Merged
mumoshu merged 1 commit into
helmfile:masterfrom
klebediev:feature/vault-kv2-support
Oct 8, 2019
Merged

Add support for Vault KV engine version 2#1
mumoshu merged 1 commit into
helmfile:masterfrom
klebediev:feature/vault-kv2-support

Conversation

@klebediev

Copy link
Copy Markdown
Contributor

There are two versions of Vault KV engine.
Vault KV engine version 2 (which is enabled by default on vault cluster creation under secret/ path) has a bit different response format:
v1: https://www.vaultproject.io/api/secret/kv/kv-v1.html#sample-response
VS
v2: https://www.vaultproject.io/api/secret/kv/kv-v2.html#sample-response-1

Example:

# kv v1
$ vault secrets enable -path=kvv1 kv
Success! Enabled the kv secrets engine at: kvv1/

$ vault write kvv1/aaa/mysecret abc=xxx
Success! Data written to: kvv1/aaa/mysecret

$ vault write kvv1/data qwerty=123
Success! Data written to: kvv1/data

# kv v2
$ vault secrets enable -path=kvv2 -version=2 kv
Success! Enabled the kv secrets engine at: kvv2/

$ vault kv put kvv2/foo/bar abc=xyz bcd=123
Key              Value
---              -----
created_time     2019-10-08T05:21:37.003754Z
deletion_time    n/a
destroyed        false
version          1

# eval
echo 'values: 
- a: ref+vault://127.0.0.1:8200/kvv1/aaa/mysecret?proto=http#/abc
- b: ref+vault://127.0.0.1:8200/kvv1/data?proto=http#/qwerty
- c: ref+vault://127.0.0.1:8200/kvv2/data/foo/bar?proto=http#/abc' | bin/vals eval -f -

values:
  - a: xxx
  - b: "123"
  - c: xyz

@mumoshu mumoshu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks a lot for the info and the fix ☺️

@mumoshu mumoshu merged commit bd2d244 into helmfile:master Oct 8, 2019
@klebediev klebediev deleted the feature/vault-kv2-support branch October 8, 2019 08:50
yxxhero pushed a commit that referenced this pull request Jun 30, 2026
* feat(vault): cache KV version preflight and secret map reads

Without caching, each secret ref in creds.yaml costs two Vault API
calls: one preflight to /v1/sys/internal/ui/mounts/<path> to detect
KV v1/v2, and one ReadWithData for the secret itself. This exhausts
low use-limit tokens (e.g. -use-limit=1) immediately.

With both caches:
- kvVersionCache: one preflight per unique mount path per run
- secretMapCache: one read per unique secret path per run

For N secrets across M unique mounts under P unique paths, token
use-limit needed drops from 2N to M+P (typically P+1 for single mount).

Claude-Session: https://claude.ai/code/session_01RDWpvGwQQzRfWUYhohzhNk
Signed-off-by: Sergio Rua <[email protected]>

* fix(vault): remove secretMapCache, add mutex-protected kvVersionCache

secretMapCache is redundant with the runtime-level LRU cache in vals.go
(docCache/strCache) which is already mutex-protected and bounded. Keeping
it at the provider level caused two problems:
1. Stale data in long-running processes (vals-operator) — no eviction
2. Data race under concurrent access — plain map, no synchronisation

kvVersionCache is kept because it caches mount-level KV version preflight
calls, which the runtime cache does not cover. Mount KV versions are
immutable at runtime so the cache never needs clearing. A sync.Mutex
protects it for concurrent callers.

Claude-Session: https://claude.ai/code/session_01RDWpvGwQQzRfWUYhohzhNk
Signed-off-by: Sergio Rua <[email protected]>

* fix(vault): restore secretMapCache with mutex protection

The runtime-level docCache in vals.go does not prevent duplicate
GetStringMap calls at the provider level — e.g. when multiple keys
reference the same secret path. Without secretMapCache each such path
costs one Vault read per key, exhausting low use-limit tokens.

Both caches are now protected by a single sync.Mutex making them safe
for concurrent callers (e.g. vals-operator).

Claude-Session: https://claude.ai/code/session_01RDWpvGwQQzRfWUYhohzhNk
Signed-off-by: Sergio Rua <[email protected]>

* fix(vault): dedup concurrent reads via singleflight, drop secret cache

Address PR review (#1213):

- Replace the never-evicting secretMapCache with singleflight dedup of
  concurrent reads, so a burst of parallel callers (vals-operator) costs
  one Vault read/token-use instead of N, without ever serving stale
  secrets (#1, #2, #3).
- Return a per-caller copy of the map since singleflight shares one
  result by reference (#2).
- Key the KV-version cache by mount path so sibling secrets under the
  same mount share a single preflight (#4).
- Serialize lazy client creation/auth in ensureClient to remove the
  first-use race (#6).
- Add concurrency tests asserting preflight/read call counts and map
  isolation against a stubbed Vault server (#5).
- Drop the hand-wavy fieldalignment nolint; order fields cleanly (#7).

Signed-off-by: Sergio Rua <[email protected]>

* fix(vault): make mount-prefix cache lookup segment-safe

Normalize the kvVersionCache key to a trailing slash on insert and match
siblings with HasPrefix(key+"/", mount) so a cached mount can never
match a textually-prefixed but distinct mount (e.g. "secret/" vs
"secretv2/x"). Empty-mount fallbacks stay full secret paths and are
only ever matched exactly, never by prefix.

Also: drop the redundant inner map copy in readSecretMap (GetStringMap
owns the per-caller copy), note the per-caller copy is shallow, and
document that sfVersion does not dedupe a concurrent first-burst of
distinct sibling paths.

Add tests for the segment-safe prefix match and for mount-key
normalization when Vault reports a mount without a trailing slash.

Signed-off-by: Sergio Rua <[email protected]>

* test(vault): fix fieldalignment in lookup test cases struct

Signed-off-by: Sergio Rua <[email protected]>

---------

Signed-off-by: Sergio Rua <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants