Skip to content

Test the version commands against a KV v2 mount - #18

Merged
wayneeseguin merged 3 commits into
developfrom
test-kv-v2-fake
Jul 29, 2026
Merged

Test the version commands against a KV v2 mount#18
wayneeseguin merged 3 commits into
developfrom
test-kv-v2-fake

Conversation

@wayneeseguin

Copy link
Copy Markdown
Contributor

safe versions, safe undelete --all, and safe get path^N had no test of
their success paths — only of the arguments they reject. The reason is that the
internal/cli fake Vault spoke KV version 1, and version history only exists on
a version 2 mount. Real vault server -dev defaults to version 2, so the
commands most dependent on versions were the ones tests could not reach.

I flagged this gap in #15, where the two version-command fixes had to rest on
live runs rather than tests. This closes it.

The fake

newCLIFakeV2 gives a version 2 mount; newCLIFake is untouched and still
gives version 1. Version 2 splits a secret across data, metadata, delete,
undelete, and destroy, and keeps a per-path version list where a version can
be alive, deleted (reversible), or destroyed (not). Seeding helpers — setV2,
deleteV2, destroyV2 — and a versionStates accessor for assertions.

The endpoint shapes are taken from the vaultkv client source rather than guessed,
including the detail that a deleted version returns 404 with its metadata still
attached.

Mount discovery was also wrong, and the second commit fixes it. The client
reads the mount out of data.secret; the fake served it one level up, so the
struct decoded to an empty map and the client reported version 1 by falling
through the nil check
rather than by being told. The old behaviour was correct
by accident. All 17 existing newCLIFake callers still pass.

The tests

Nine cases: every version and state listed by versions; undelete --all
reviving every deleted version while leaving destroyed ones alone; get reading
a specific version, with ^0 and a bare path both meaning latest; and a deleted
version refusing to be read.

Two of them pin #15 directly. I verified they fail without it by reverting
those two fixes and re-running:

cmdVersions on an escaped path: no secret exists at path `secret/od\:d`
version states = [deleted deleted], want [alive alive]

The second is the important one — that bug was a silent no-op that still exited
zero, which is exactly the kind that a test has to catch because a human running
the command will not.

I mutation-tested the rest too: making Vault.Read ignore the requested version
fails the get test with = "two", want "one", so it is genuinely exercising
version selection rather than passing by construction.

make check and go test -race ./internal/cli/ both pass.

Scope

Tests only — no production code changes.

This covers the internal/cli fake. The pkg/vault fake is still version 1
only; extending it the same way would benefit the tree/paths/get unit
tests there, and is a reasonable follow-up rather than something to fold in here.

The fake spoke version 1 only, while real Vault defaults to version 2,
so nothing behind a version history could be tested. Add a second
constructor selecting a version 2 mount, backed by a per-path version
list and the data, metadata, delete, undelete and destroy endpoints.

Correct the mount discovery body while here. The client reads the
mount out of data.secret, so the old shape decoded to an empty map
and reported version 1 by falling through rather than by saying so.
versions, undelete --all and a versioned get had no test of their
success paths, only of the arguments they reject.
The version tests seeded history directly, so the write half of the
v2 mount went unexercised: set appending a version rather than
replacing a value, and revert writing an old version forward.
@wayneeseguin

Copy link
Copy Markdown
Contributor Author

Added a third commit covering the write half of the v2 mount.

The original tests seeded version history directly through the helpers, so
nothing ever wrote through the CLI — set appending a version rather than
replacing a value, and revert writing an old version forward, were both
unexercised. Three cases now cover that, and I confirmed they can fail: making
the fake replace the latest version instead of appending one gives

expected a second version to be created, states = [alive]
revert should have appended a version, states = [alive alive]

One finding worth recording, since it contradicts a plausible-sounding concern
about this fake. The vaultkv client only decodes a response body on exactly
200, so a write answering 204 yields meta.Version == 0 — which suggests the
fake must answer 200-with-body or tests would silently misbehave. I checked
rather than assuming: switching the fake's write to 204 leaves every test
passing, because safe never reads the version metadata a write returns. The
fake answers 200 with the metadata anyway, since that is what real Vault does
and future tests may want it, but it is fidelity rather than a load-bearing
detail. Worth knowing before someone "simplifies" it.

make check still passes.

@wayneeseguin
wayneeseguin merged commit 196acf1 into develop Jul 29, 2026
@wayneeseguin
wayneeseguin deleted the test-kv-v2-fake branch July 29, 2026 16:39
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.

1 participant