test(secrets): smoke-cover backend health honesty + cache correctness

Two of these four assertions currently FAIL against the live cluster, which
is the point: mcpd there still renders `Secrets: bao-k8s* ✓` from the
rotation field, and its JSON status carries no live/ready pair. Per the
project rule the fix is to deploy, not to relax the assertion.

The status check specifically rejects a bare "name ✓" with no qualifier —
that string is the old rendering and proves the probe was never consulted.

Deliberately does not take the real OpenBao down. Simulating an outage
against shared infrastructure to satisfy a test would be worse than the
bug it covers; the outage paths are unit-tested with an injected clock.

Docs: adds a Reliability section (request hardening, the stale-while-error
table, the cold-cache gap and why persisting values is not the answer,
live-vs-ready) and replaces the stale "Kubernetes ServiceAccount auth is
not shipped yet" note — it shipped in 5152066, and it is what the live
backend has used since June.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018vybEitX4FykeMatKe5Xki
This commit is contained in:
Michal
2026-08-20 22:16:04 +01:00
parent 0fbfc72d68
commit fe9987cefd
2 changed files with 237 additions and 2 deletions

View File

@@ -118,8 +118,110 @@ That's the whole point of keeping plaintext around — it's the trust root:
token itself. DB access is now equivalent to OpenBao token access (a single token itself. DB access is now equivalent to OpenBao token access (a single
key), not equivalent to all API keys in the system. key), not equivalent to all API keys in the system.
Follow-up work (not shipped yet) replaces static token auth with Kubernetes #### Kubernetes ServiceAccount auth (no bootstrap token)
ServiceAccount auth so no bootstrap token is needed at all.
`auth: kubernetes` removes the chicken-and-egg entirely: mcpd exchanges its
projected ServiceAccount JWT for an OpenBao token at
`auth/<authMount>/role/<role>`, so there is no static credential in the database
at all. The token is cached for its lease and re-minted lazily with a 60s grace
window.
```yaml
kind: secretbackend
name: bao-k8s
type: openbao
isDefault: true
config:
url: https://bao.example
auth: kubernetes
role: mcpctl
authMount: kubernetes-worker0 # defaults to `kubernetes`
```
Note that the daily **rotator does not apply** to these backends — there is no
stored token to rotate. That has a consequence for monitoring, see below.
## Reliability
Remote backends are network dependencies on the critical path of nearly
everything: server env resolution, LLM api keys, chat, git providers, code
repos, webhooks. Three mechanisms keep an outage from cascading.
### Request hardening
Every call carries a timeout (default 5s) and retries `5xx`/`429`/network
failures with full-jitter exponential backoff (3 attempts). A **sealed** OpenBao
answers `503`, so this covers unseal windows and failovers.
The `403` path is separate and deliberately single-shot: the driver purges its
cached token, re-authenticates and retries **once**. That is a credential
refresh, not a backend-unavailable condition — looping on it would hide a
genuinely revoked grant.
### Value cache with stale-while-error
Resolved values are cached per backend (default TTL 5 minutes, LRU-bounded).
Past the TTL the backend is always consulted; if it fails *as a transport
failure*, the last known-good value is served instead of throwing.
| Failure | Behaviour |
|---|---|
| Backend unreachable / timeout / exhausted 5xx | Serve last known-good, mark degraded, log `BACKEND_UNREACHABLE` once |
| Secret deleted (404) | **Evict and throw.** Never served stale — that would resurrect a revoked credential |
| 403 after a token refresh | Throw. Revoked grants must stay loud |
| Nothing cached yet | Throw |
The stale window is unbounded on purpose: a cap would mean a long outage
eventually takes mcpd down anyway.
`plaintext` backends are not cached — their `read()` is an identity function
over the row the caller already supplied.
**Cold cache is the known gap.** If mcpd restarts *while* the backend is
unreachable, nothing has a last-known-good value and secret-bearing servers fail
to start. That is deliberate: booting a server with an empty credential is worse
(gitea-mcp once ran for weeks with an empty `GITEA_ACCESS_TOKEN`, answering
`tools/list` and reporting healthy while every authenticated call failed). mcpd
mitigates it by warming the cache at boot — one read per referenced secret — so
an outage that starts *after* startup is fully absorbed.
### Health: `live` vs `ready`
```bash
curl $MCPD/api/v1/secretbackends/<id>/health
```
```json
{ "live": true, "liveDetail": "active",
"ready": false, "readyDetail": "OpenBao list: HTTP 403 permission denied",
"cache": { "entries": 9, "servingStale": 0 },
"rotation": { "rotatable": false, "lastRotationError": null } }
```
- **`live`** — unauthenticated `sys/health`. Distinguishes *down* from *sealed*
from *standby*.
- **`ready`** — a real read with our credentials.
The two are separate because `live && !ready` is a distinct, important state: a
re-initialised OpenBao hands back valid-looking tokens that grant nothing.
Collapsing them into one boolean is what let that go unnoticed for four days.
`mcpctl status` renders the probe directly:
```
Secrets: bao-k8s* ✓ reachable, default ✓ reachable
Secrets: bao-k8s* ⚠ degraded — serving 7 cached secret(s)
Secrets: bao-k8s* ✗ unreachable: sealed
Secrets: bao-k8s* ✗ auth failed: HTTP 403 permission denied
Secrets: bao-k8s* ? unknown
```
> **Historical note.** This verdict used to come solely from
> `tokenMeta.lastRotationError`, which only the rotator writes — and the rotator
> skips `auth: kubernetes` backends. The Secrets line was therefore *incapable*
> of going red for a k8s-auth backend, and reported OpenBao healthy while it was
> unreachable. `?` (probe failed) renders yellow, never green: not knowing is not
> health.
## Migration — `mcpctl migrate secrets` ## Migration — `mcpctl migrate secrets`

View File

@@ -0,0 +1,133 @@
/**
* Smoke tests: secret-backend health honesty + value caching, against live mcpd.
*
* Covers the two behaviours that unit tests cannot prove, because both are
* about what the REAL backend and the REAL CLI do together:
*
* 1. `mcpctl status` reports a probed verdict, not a hard-coded tick. The bug
* being guarded is that the verdict used to come from
* `tokenMeta.lastRotationError`, which a `kubernetes`-auth backend never
* writes — so the line was structurally incapable of going red.
* 2. The value cache does not corrupt reads, and a delete really evicts.
*
* Deliberately does NOT take the real backend down. Simulating an outage
* against shared infrastructure to satisfy a test would be worse than the bug.
*
* Target: mcpd direct (`--direct`), same skip-if-unreachable discipline as the
* other smokes here.
*
* Run with: pnpm test:smoke
*/
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
import http from 'node:http';
import https from 'node:https';
import { execSync } from 'node:child_process';
const MCPD_URL = process.env.MCPD_URL ?? 'https://mcpctl.ad.itaz.eu';
const SECRET_NAME = `smoke-cache-${Date.now().toString(36)}`;
interface CliResult { code: number; stdout: string; stderr: string }
function run(args: string): CliResult {
try {
return { code: 0, stdout: execSync(`mcpctl --direct ${args}`, { encoding: 'utf-8', timeout: 30_000, stdio: ['ignore', 'pipe', 'pipe'] }).trim(), stderr: '' };
} catch (err) {
const e = err as { status?: number; stdout?: Buffer | string; stderr?: Buffer | string };
return {
code: e.status ?? 1,
stdout: e.stdout ? String(e.stdout) : '',
stderr: e.stderr ? String(e.stderr) : '',
};
}
}
function healthz(url: string, timeoutMs = 5000): Promise<boolean> {
return new Promise((resolve) => {
const parsed = new URL(`${url.replace(/\/$/, '')}/healthz`);
const driver = parsed.protocol === 'https:' ? https : http;
const req = driver.get(
{ hostname: parsed.hostname, port: parsed.port || (parsed.protocol === 'https:' ? 443 : 80), path: parsed.pathname, timeout: timeoutMs },
(res) => { resolve((res.statusCode ?? 500) < 500); res.resume(); },
);
req.on('error', () => resolve(false));
req.on('timeout', () => { req.destroy(); resolve(false); });
});
}
let mcpdUp = false;
describe('secret resilience smoke', () => {
beforeAll(async () => {
mcpdUp = await healthz(MCPD_URL);
if (!mcpdUp) {
// eslint-disable-next-line no-console
console.warn(`\n ○ secret resilience smoke: skipped — ${MCPD_URL}/healthz unreachable. Set MCPD_URL to override.\n`);
}
}, 20_000);
afterAll(() => {
if (!mcpdUp) return;
run(`delete secret ${SECRET_NAME}`);
});
it('status reports a probed backend verdict, not an unconditional tick', () => {
if (!mcpdUp) return;
const result = run('status');
expect(result.code, result.stderr).toBe(0);
const line = result.stdout.split('\n').find((l) => l.startsWith('Secrets:'));
expect(line, 'status must include a Secrets: line').toBeDefined();
// The verdict must be one the live probe can produce. A bare "name ✓" with
// no qualifier is the OLD rendering and means the probe was not consulted.
expect(line).toMatch(/reachable|degraded|unreachable|auth failed|unknown/);
});
it('reports live and ready separately per backend in JSON output', () => {
if (!mcpdUp) return;
const result = run('status -o json');
expect(result.code, result.stderr).toBe(0);
const parsed = JSON.parse(result.stdout) as {
secretBackends?: Array<{ name: string; healthy: boolean; live: boolean | null; ready: boolean | null }>;
};
expect(parsed.secretBackends, 'JSON status must carry secretBackends').toBeDefined();
for (const b of parsed.secretBackends ?? []) {
// Both signals present and independent — not one boolean copied twice.
expect(b, `backend ${b.name}`).toHaveProperty('live');
expect(b, `backend ${b.name}`).toHaveProperty('ready');
expect(b.healthy).toBe(b.live === true && b.ready === true);
}
});
it('caching does not corrupt repeated reads, and delete evicts', () => {
if (!mcpdUp) return;
const created = run(`create secret ${SECRET_NAME} --data TOKEN=cache-probe-value`);
expect(created.code, created.stderr).toBe(0);
// Two reads back-to-back: the second is a cache hit. Both must agree.
const first = run(`describe secret ${SECRET_NAME} --show-values`);
const second = run(`describe secret ${SECRET_NAME} --show-values`);
expect(first.code, first.stderr).toBe(0);
expect(second.code, second.stderr).toBe(0);
expect(first.stdout).toContain('cache-probe-value');
expect(second.stdout).toContain('cache-probe-value');
// Delete must evict — a cached value surviving a delete is exactly the
// "resurrected revoked credential" failure the cache guards against.
const deleted = run(`delete secret ${SECRET_NAME}`);
expect(deleted.code, deleted.stderr).toBe(0);
const after = run(`describe secret ${SECRET_NAME} --show-values`);
expect(after.code, 'reading a deleted secret must fail, not serve cache').not.toBe(0);
});
it('exposes the per-backend health endpoint used by status', () => {
if (!mcpdUp) return;
const backends = run('get secretbackends -o json');
expect(backends.code, backends.stderr).toBe(0);
const rows = JSON.parse(backends.stdout) as Array<{ id: string; name: string }>;
expect(rows.length).toBeGreaterThan(0);
// describe must surface the same probe, for every backend type — the old
// Token health block was gated on tokenMeta.rotatable and so rendered
// nothing at all for kubernetes-auth backends.
const described = run(`describe secretbackend ${rows[0]?.name ?? ''}`);
expect(described.code, described.stderr).toBe(0);
});
});