Compare commits
8 Commits
c66502e590
...
fix/openba
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
7b5136491f | ||
| eb3e558a44 | |||
|
|
545e7745da | ||
|
|
fe9987cefd | ||
|
|
0fbfc72d68 | ||
|
|
bd3e1134c9 | ||
|
|
5fb1154190 | ||
|
|
b022f322f0 |
@@ -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
|
||||
key), not equivalent to all API keys in the system.
|
||||
|
||||
Follow-up work (not shipped yet) replaces static token auth with Kubernetes
|
||||
ServiceAccount auth so no bootstrap token is needed at all.
|
||||
#### Kubernetes ServiceAccount auth (no bootstrap token)
|
||||
|
||||
`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`
|
||||
|
||||
|
||||
@@ -50,6 +50,7 @@ interface ServerLlm {
|
||||
* of the last credential-rotation failure (e.g. a dead OpenBao token).
|
||||
*/
|
||||
interface SecretBackendInfo {
|
||||
id: string;
|
||||
name: string;
|
||||
type: string;
|
||||
isDefault?: boolean;
|
||||
@@ -60,6 +61,22 @@ interface SecretBackendInfo {
|
||||
} | null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Live probe result from GET /api/v1/secretbackends/:id/health.
|
||||
*
|
||||
* `live` and `ready` are deliberately separate: a backend that is reachable but
|
||||
* whose credentials no longer grant anything is the failure mode that hid an
|
||||
* OpenBao re-init for four days. `null` means the probe itself failed, which we
|
||||
* report as unknown rather than pretending it means healthy.
|
||||
*/
|
||||
interface SecretBackendHealth {
|
||||
live: boolean;
|
||||
liveDetail?: string;
|
||||
ready: boolean;
|
||||
readyDetail?: string;
|
||||
cache?: { entries: number; servingStale: number; oldestStaleSince?: number | null } | null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Result of a live "say hi" probe against a server LLM. `ok` says we got a
|
||||
* 200 + non-empty content back; `say` is the trimmed first 16 chars of the
|
||||
@@ -99,6 +116,7 @@ export interface StatusCommandDeps {
|
||||
probeServerLlm: (mcpdUrl: string, name: string, token: string | null) => Promise<ServerLlmHealth>;
|
||||
/** Fetch SecretBackends from mcpd to surface backend health. Null on error. */
|
||||
fetchSecretBackends: (mcpdUrl: string, token: string | null) => Promise<SecretBackendInfo[] | null>;
|
||||
probeSecretBackend: (mcpdUrl: string, id: string, token: string | null) => Promise<SecretBackendHealth | null>;
|
||||
isTTY: boolean;
|
||||
}
|
||||
|
||||
@@ -275,6 +293,38 @@ function defaultFetchSecretBackends(mcpdUrl: string, token: string | null): Prom
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Live-probe one SecretBackend. Resolves to null on any unhappy path — same
|
||||
* never-throw discipline as the other probes here, and `null` renders as
|
||||
* "unknown", never as healthy.
|
||||
*/
|
||||
function defaultProbeSecretBackend(mcpdUrl: string, id: string, token: string | null): Promise<SecretBackendHealth | null> {
|
||||
return new Promise((resolve) => {
|
||||
let req: http.ClientRequest;
|
||||
const headers: Record<string, string> = { Accept: 'application/json' };
|
||||
if (token !== null) headers['Authorization'] = `Bearer ${token}`;
|
||||
try {
|
||||
req = httpDriverFor(mcpdUrl).get(`${mcpdUrl}/api/v1/secretbackends/${id}/health`, { timeout: 5000, headers }, (res) => {
|
||||
if (res.statusCode !== 200) { resolve(null); res.resume(); return; }
|
||||
const chunks: Buffer[] = [];
|
||||
res.on('data', (chunk: Buffer) => chunks.push(chunk));
|
||||
res.on('end', () => {
|
||||
try {
|
||||
resolve(JSON.parse(Buffer.concat(chunks).toString('utf-8')) as SecretBackendHealth);
|
||||
} catch {
|
||||
resolve(null);
|
||||
}
|
||||
});
|
||||
});
|
||||
} catch {
|
||||
resolve(null);
|
||||
return;
|
||||
}
|
||||
req.on('error', () => resolve(null));
|
||||
req.on('timeout', () => { req.destroy(); resolve(null); });
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* POST a tiny "say hi" prompt to /api/v1/llms/<name>/infer and decide if
|
||||
* the LLM actually serves inference. Returns ok=true when the response is
|
||||
@@ -386,6 +436,7 @@ const defaultDeps: StatusCommandDeps = {
|
||||
fetchProviders: defaultFetchProviders,
|
||||
fetchServerLlms: defaultFetchServerLlms,
|
||||
fetchSecretBackends: defaultFetchSecretBackends,
|
||||
probeSecretBackend: defaultProbeSecretBackend,
|
||||
probeServerLlm: defaultProbeServerLlm,
|
||||
isTTY: process.stdout.isTTY ?? false,
|
||||
};
|
||||
@@ -448,7 +499,7 @@ function formatProviderStatus(name: string, info: ProvidersInfo, ansi: boolean):
|
||||
}
|
||||
|
||||
export function createStatusCommand(deps?: Partial<StatusCommandDeps>): Command {
|
||||
const { configDeps, credentialsDeps, log, write, checkHealth, checkLlm, fetchModels, fetchProviders, fetchServerLlms, probeServerLlm, fetchSecretBackends, isTTY } = { ...defaultDeps, ...deps };
|
||||
const { configDeps, credentialsDeps, log, write, checkHealth, checkLlm, fetchModels, fetchProviders, fetchServerLlms, probeServerLlm, fetchSecretBackends, probeSecretBackend, isTTY } = { ...defaultDeps, ...deps };
|
||||
|
||||
return new Command('status')
|
||||
.description('Show mcpctl status and connectivity')
|
||||
@@ -482,6 +533,30 @@ export function createStatusCommand(deps?: Partial<StatusCommandDeps>): Command
|
||||
})))
|
||||
: null;
|
||||
|
||||
// Same live probe the table view uses. `healthy` is derived from the
|
||||
// probe, NOT from tokenMeta.lastRotationError — that field is only ever
|
||||
// written for token-auth backends, so scripts consuming it were told
|
||||
// every kubernetes-auth backend was healthy unconditionally.
|
||||
const secretBackendsWithHealth = secretBackends !== null
|
||||
? await Promise.all(secretBackends.map(async (b) => {
|
||||
const health = await probeSecretBackend(config.mcpdUrl, b.id, token);
|
||||
return {
|
||||
name: b.name,
|
||||
type: b.type,
|
||||
healthy: health !== null && health.live && health.ready,
|
||||
live: health?.live ?? null,
|
||||
ready: health?.ready ?? null,
|
||||
servingStale: health?.cache?.servingStale ?? 0,
|
||||
error: health === null
|
||||
? 'health probe failed'
|
||||
: !health.live ? (health.liveDetail ?? 'unreachable')
|
||||
: !health.ready ? (health.readyDetail ?? 'auth failed')
|
||||
: null,
|
||||
rotationError: b.tokenMeta?.lastRotationError ?? null,
|
||||
};
|
||||
}))
|
||||
: null;
|
||||
|
||||
const llm = llmLabel
|
||||
? llmStatus === 'ok' ? llmLabel : `${llmLabel} (${llmStatus})`
|
||||
: null;
|
||||
@@ -499,7 +574,7 @@ export function createStatusCommand(deps?: Partial<StatusCommandDeps>): Command
|
||||
llmStatus,
|
||||
...(providersInfo ? { providers: providersInfo } : {}),
|
||||
...(serverLlmsWithHealth !== null ? { serverLlms: serverLlmsWithHealth } : {}),
|
||||
...(secretBackends !== null ? { secretBackends: secretBackends.map((b) => ({ name: b.name, type: b.type, healthy: !b.tokenMeta?.lastRotationError, error: b.tokenMeta?.lastRotationError ?? null })) } : {}),
|
||||
...(secretBackends !== null ? { secretBackends: secretBackendsWithHealth } : {}),
|
||||
};
|
||||
|
||||
log(opts.output === 'json' ? formatJson(status) : formatYaml(status));
|
||||
@@ -530,7 +605,7 @@ export function createStatusCommand(deps?: Partial<StatusCommandDeps>): Command
|
||||
|
||||
if (!llmLabel) {
|
||||
log(`LLM: not configured (run 'mcpctl config setup')`);
|
||||
await renderSecretBackendsSection(secretBackendsPromise, isTTY);
|
||||
await renderSecretBackendsSection(secretBackendsPromise, isTTY, config.mcpdUrl, token);
|
||||
await renderServerLlmsSection(serverLlmsPromise, config.mcpdUrl, token, isTTY);
|
||||
return;
|
||||
}
|
||||
@@ -595,7 +670,7 @@ export function createStatusCommand(deps?: Partial<StatusCommandDeps>): Command
|
||||
}
|
||||
}
|
||||
|
||||
await renderSecretBackendsSection(secretBackendsPromise, isTTY);
|
||||
await renderSecretBackendsSection(secretBackendsPromise, isTTY, config.mcpdUrl, token);
|
||||
await renderServerLlmsSection(serverLlmsPromise, config.mcpdUrl, token, isTTY);
|
||||
});
|
||||
|
||||
@@ -609,21 +684,50 @@ export function createStatusCommand(deps?: Partial<StatusCommandDeps>): Command
|
||||
async function renderSecretBackendsSection(
|
||||
backendsPromise: Promise<SecretBackendInfo[] | null>,
|
||||
ansi: boolean,
|
||||
mcpdUrl: string,
|
||||
token: string | null,
|
||||
): Promise<void> {
|
||||
const backends = await backendsPromise;
|
||||
if (backends === null || backends.length === 0) return;
|
||||
const parts = backends.map((b) => {
|
||||
const err = b.tokenMeta?.lastRotationError;
|
||||
const tag = b.isDefault ? `${b.name}*` : b.name;
|
||||
if (err) {
|
||||
const short = err.split('\n')[0]?.slice(0, 80) ?? 'error';
|
||||
return ansi ? `${tag} ${RED}✗ ${short}${RESET}` : `${tag} ✗ ${short}`;
|
||||
}
|
||||
return ansi ? `${tag} ${GREEN}✓${RESET}` : `${tag} ✓`;
|
||||
});
|
||||
const healths = await Promise.all(backends.map((b) => probeSecretBackend(mcpdUrl, b.id, token)));
|
||||
const parts = backends.map((b, i) => renderOneBackend(b, healths[i] ?? null, ansi));
|
||||
log(`Secrets: ${parts.join(', ')}`);
|
||||
}
|
||||
|
||||
/**
|
||||
* Render one backend's status line.
|
||||
*
|
||||
* This used to be `tokenMeta.lastRotationError ? red : green`, which was a
|
||||
* hard-coded green tick for every `auth: kubernetes` backend — the rotator
|
||||
* only writes that field for token-auth backends, so it was never set and
|
||||
* `mcpctl status` reported OpenBao healthy even when it was unreachable.
|
||||
* Rotation state is now one clause among several, not the only signal.
|
||||
*/
|
||||
function renderOneBackend(b: SecretBackendInfo, health: SecretBackendHealth | null, ansi: boolean): string {
|
||||
const tag = b.isDefault === true ? `${b.name}*` : b.name;
|
||||
const paint = (colour: string, text: string): string => (ansi ? `${colour}${text}${RESET}` : text);
|
||||
const rotationErr = b.tokenMeta?.lastRotationError ?? '';
|
||||
const rotationClause = rotationErr === ''
|
||||
? ''
|
||||
: ` (rotation: ${rotationErr.split('\n')[0]?.slice(0, 60) ?? 'error'})`;
|
||||
|
||||
if (health === null) {
|
||||
// The probe itself failed. Unknown is not healthy — say so.
|
||||
return `${tag} ${paint(YELLOW, '? unknown')}${rotationClause}`;
|
||||
}
|
||||
if (!health.live) {
|
||||
return `${tag} ${paint(RED, `✗ unreachable: ${health.liveDetail ?? 'no detail'}`)}${rotationClause}`;
|
||||
}
|
||||
if (!health.ready) {
|
||||
return `${tag} ${paint(RED, `✗ auth failed: ${(health.readyDetail ?? 'no detail').slice(0, 60)}`)}${rotationClause}`;
|
||||
}
|
||||
const stale = health.cache?.servingStale ?? 0;
|
||||
if (stale > 0) {
|
||||
return `${tag} ${paint(YELLOW, `⚠ degraded — serving ${String(stale)} cached secret(s)`)}${rotationClause}`;
|
||||
}
|
||||
return `${tag} ${paint(GREEN, '✓ reachable')}${rotationClause}`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Print a "Server LLMs:" section listing mcpd-managed Llm rows by tier
|
||||
* with a per-LLM "say hi" liveness probe. Distinct from the mcplocal-side
|
||||
|
||||
@@ -30,6 +30,7 @@ function baseDeps(overrides?: Partial<StatusCommandDeps>): Partial<StatusCommand
|
||||
fetchServerLlms: async () => null,
|
||||
probeServerLlm: async () => ({ ok: true, ms: 12, say: 'hi' }),
|
||||
fetchSecretBackends: async () => null,
|
||||
probeSecretBackend: async () => ({ live: true, ready: true, cache: { entries: 0, servingStale: 0 } }),
|
||||
isTTY: false,
|
||||
...overrides,
|
||||
};
|
||||
@@ -46,33 +47,73 @@ afterEach(() => {
|
||||
});
|
||||
|
||||
describe('status command', () => {
|
||||
const BAO = { id: 'b1', name: 'bao', type: 'openbao', isDefault: true, tokenMeta: { lastRotationError: null } };
|
||||
|
||||
it('shows a healthy secret backend in the Secrets line', async () => {
|
||||
const cmd = createStatusCommand(baseDeps({
|
||||
fetchSecretBackends: async () => [
|
||||
{ name: 'bao', type: 'openbao', isDefault: true, tokenMeta: { lastRotationError: null } },
|
||||
{ name: 'default', type: 'plaintext' },
|
||||
],
|
||||
fetchSecretBackends: async () => [BAO, { id: 'b2', name: 'default', type: 'plaintext' }],
|
||||
}));
|
||||
await cmd.parseAsync([], { from: 'user' });
|
||||
const out = output.join('\n');
|
||||
expect(out).toContain('Secrets:');
|
||||
expect(out).toContain('bao* ✓');
|
||||
expect(out).toContain('default ✓');
|
||||
expect(out).toContain('bao* ✓ reachable');
|
||||
expect(out).toContain('default ✓ reachable');
|
||||
});
|
||||
|
||||
it('flags a dead secret-backend token in the Secrets line', async () => {
|
||||
const cmd = createStatusCommand(baseDeps({
|
||||
fetchSecretBackends: async () => [
|
||||
{ name: 'bao', type: 'openbao', isDefault: true, tokenMeta: { lastRotationError: 'BACKEND_TOKEN_DEAD: rejected the stored token\nmore detail' } },
|
||||
{ ...BAO, tokenMeta: { lastRotationError: 'BACKEND_TOKEN_DEAD: rejected the stored token\nmore detail' } },
|
||||
],
|
||||
}));
|
||||
await cmd.parseAsync([], { from: 'user' });
|
||||
const out = output.join('\n');
|
||||
expect(out).toContain('bao* ✗');
|
||||
expect(out).toContain('BACKEND_TOKEN_DEAD');
|
||||
expect(out).not.toContain('more detail'); // only first line, truncated
|
||||
});
|
||||
|
||||
it('reports an unreachable backend even when rotation never errored', async () => {
|
||||
// THE bug: a kubernetes-auth backend never writes tokenMeta.lastRotationError,
|
||||
// so this line used to render a green tick with OpenBao completely down.
|
||||
const cmd = createStatusCommand(baseDeps({
|
||||
fetchSecretBackends: async () => [BAO],
|
||||
probeSecretBackend: async () => ({ live: false, liveDetail: 'sealed', ready: false }),
|
||||
}));
|
||||
await cmd.parseAsync([], { from: 'user' });
|
||||
const out = output.join('\n');
|
||||
expect(out).toContain('bao* ✗ unreachable: sealed');
|
||||
expect(out).not.toContain('✓');
|
||||
});
|
||||
|
||||
it('distinguishes reachable-but-unusable from unreachable', async () => {
|
||||
const cmd = createStatusCommand(baseDeps({
|
||||
fetchSecretBackends: async () => [BAO],
|
||||
probeSecretBackend: async () => ({ live: true, ready: false, readyDetail: 'HTTP 403 permission denied' }),
|
||||
}));
|
||||
await cmd.parseAsync([], { from: 'user' });
|
||||
expect(output.join('\n')).toContain('bao* ✗ auth failed: HTTP 403 permission denied');
|
||||
});
|
||||
|
||||
it('reports degraded while serving cached secrets', async () => {
|
||||
const cmd = createStatusCommand(baseDeps({
|
||||
fetchSecretBackends: async () => [BAO],
|
||||
probeSecretBackend: async () => ({ live: true, ready: true, cache: { entries: 9, servingStale: 7 } }),
|
||||
}));
|
||||
await cmd.parseAsync([], { from: 'user' });
|
||||
expect(output.join('\n')).toContain('bao* ⚠ degraded — serving 7 cached secret(s)');
|
||||
});
|
||||
|
||||
it('reports unknown — never healthy — when the probe itself fails', async () => {
|
||||
const cmd = createStatusCommand(baseDeps({
|
||||
fetchSecretBackends: async () => [BAO],
|
||||
probeSecretBackend: async () => null,
|
||||
}));
|
||||
await cmd.parseAsync([], { from: 'user' });
|
||||
const out = output.join('\n');
|
||||
expect(out).toContain('bao* ? unknown');
|
||||
expect(out).not.toContain('✓');
|
||||
});
|
||||
|
||||
it('omits the Secrets line when mcpd returns no backends', async () => {
|
||||
const cmd = createStatusCommand(baseDeps({ fetchSecretBackends: async () => null }));
|
||||
await cmd.parseAsync([], { from: 'user' });
|
||||
|
||||
67
src/mcpd/src/bootstrap/warm-secret-cache.ts
Normal file
67
src/mcpd/src/bootstrap/warm-secret-cache.ts
Normal file
@@ -0,0 +1,67 @@
|
||||
/**
|
||||
* One-shot: resolve every secret that a running server depends on, so the
|
||||
* value cache holds a last-known-good copy before anything needs it.
|
||||
*
|
||||
* The caching driver absorbs a backend outage by serving the last value it saw
|
||||
* — but only for secrets it has actually seen. Without this, a cold mcpd (fresh
|
||||
* deploy, pod reschedule, crash-restart) has an empty cache, and if the backend
|
||||
* is unreachable at that moment every secret-bearing server fails to start.
|
||||
*
|
||||
* This is the honest mitigation, and it is deliberately partial: if the backend
|
||||
* is ALSO down at boot, this changes nothing and instances fail loudly, which
|
||||
* is correct. The alternatives — persisting last-known-good to Postgres or to
|
||||
* disk — are just "plaintext secrets at rest" wearing a hat, which is the thing
|
||||
* we are trying to move away from.
|
||||
*
|
||||
* Best-effort by construction: a failure here must never block startup, and the
|
||||
* warm is per-secret so one bad reference doesn't abandon the rest.
|
||||
*/
|
||||
import type { PrismaClient } from '@prisma/client';
|
||||
import type { SecretService } from '../services/secret.service.js';
|
||||
import type { ServerEnvEntry } from '../validation/mcp-server.schema.js';
|
||||
|
||||
export interface WarmLog {
|
||||
info: (msg: string) => void;
|
||||
warn: (msg: string) => void;
|
||||
}
|
||||
|
||||
export async function warmSecretCache(
|
||||
prisma: PrismaClient,
|
||||
secrets: SecretService,
|
||||
log: WarmLog,
|
||||
): Promise<{ warmed: number; failed: number }> {
|
||||
const servers = await prisma.mcpServer.findMany({
|
||||
where: { replicas: { gt: 0 } },
|
||||
select: { name: true, env: true },
|
||||
});
|
||||
|
||||
// Distinct (secret, key) pairs — several servers commonly share one secret,
|
||||
// and there is no point paying for the same read more than once.
|
||||
const refs = new Map<string, { name: string; key: string }>();
|
||||
for (const server of servers) {
|
||||
for (const entry of (server.env ?? []) as ServerEnvEntry[]) {
|
||||
const ref = entry.valueFrom?.secretRef;
|
||||
if (ref === undefined) continue;
|
||||
refs.set(`${ref.name}/${ref.key}`, { name: ref.name, key: ref.key });
|
||||
}
|
||||
}
|
||||
if (refs.size === 0) return { warmed: 0, failed: 0 };
|
||||
|
||||
let warmed = 0;
|
||||
let failed = 0;
|
||||
for (const ref of refs.values()) {
|
||||
try {
|
||||
// Value deliberately discarded — we only want it in the cache.
|
||||
await secrets.resolve(ref.name, ref.key);
|
||||
warmed++;
|
||||
} catch {
|
||||
// Expected when the backend is down, or when a server references a
|
||||
// secret that no longer exists. Neither should block startup, and both
|
||||
// surface loudly at instance-start time anyway.
|
||||
failed++;
|
||||
}
|
||||
}
|
||||
|
||||
log.info(`secret cache warm: ${String(warmed)} resolved, ${String(failed)} unavailable`);
|
||||
return { warmed, failed };
|
||||
}
|
||||
@@ -25,11 +25,13 @@ import { SecretBackendService } from './services/secret-backend.service.js';
|
||||
import { SecretMigrateService } from './services/secret-migrate.service.js';
|
||||
import { bootstrapSecretBackends } from './bootstrap/secret-backends.js';
|
||||
import { backfillSecretKeyNames } from './bootstrap/secret-key-names.js';
|
||||
import { warmSecretCache } from './bootstrap/warm-secret-cache.js';
|
||||
import { registerSecretBackendRoutes } from './routes/secret-backends.js';
|
||||
import { registerSecretMigrateRoutes } from './routes/secret-migrate.js';
|
||||
import { SecretBackendRotator } from './services/secret-backend-rotator.service.js';
|
||||
import { SecretBackendRotatorLoop } from './services/secret-backend-rotator-loop.js';
|
||||
import { registerSecretBackendRotateRoutes } from './routes/secret-backend-rotate.js';
|
||||
import { registerSecretBackendHealthRoutes } from './routes/secret-backend-health.js';
|
||||
import { LlmRepository } from './repositories/llm.repository.js';
|
||||
import { LlmService } from './services/llm.service.js';
|
||||
import { InferenceTaskRepository } from './repositories/inference-task.repository.js';
|
||||
@@ -474,16 +476,32 @@ async function main(): Promise<void> {
|
||||
},
|
||||
},
|
||||
secretRefResolver: secretResolverBridge,
|
||||
}, {
|
||||
// Cache-transition events go through pino so BACKEND_UNREACHABLE /
|
||||
// BACKEND_RECOVERED land in ErrorLogBuffer and `mcpctl errors`.
|
||||
log: {
|
||||
warn: (obj: Record<string, unknown>, msg: string): void => { app.log.warn(obj, msg); },
|
||||
info: (obj: Record<string, unknown>, msg: string): void => { app.log.info(obj, msg); },
|
||||
},
|
||||
});
|
||||
const secretService = new SecretService(secretRepo, secretBackendService);
|
||||
const secretMigrateService = new SecretMigrateService(secretRepo, secretBackendService);
|
||||
const secretBackendRotator = new SecretBackendRotator({
|
||||
backends: secretBackendService,
|
||||
secrets: secretService,
|
||||
log: {
|
||||
error: (obj: Record<string, unknown>, msg: string): void => { app.log.error(obj, msg); },
|
||||
warn: (msg: string): void => { app.log.warn(msg); },
|
||||
},
|
||||
});
|
||||
const secretBackendRotatorLoop = new SecretBackendRotatorLoop({
|
||||
backends: secretBackendService,
|
||||
rotator: secretBackendRotator,
|
||||
log: {
|
||||
info: (msg: string): void => { app.log.info(`[rotator] ${msg}`); },
|
||||
warn: (msg: string): void => { app.log.warn(`[rotator] ${msg}`); },
|
||||
error: (obj: Record<string, unknown>, msg: string): void => { app.log.error(obj, msg); },
|
||||
},
|
||||
});
|
||||
const llmAdapters = new LlmAdapterRegistry();
|
||||
// LlmService takes the adapter registry so create()/update() can run an
|
||||
@@ -511,6 +529,10 @@ async function main(): Promise<void> {
|
||||
const authService = new AuthService(prisma);
|
||||
const templateService = new TemplateService(templateRepo);
|
||||
const mcpProxyService = new McpProxyService(instanceRepo, serverRepo, orchestrator);
|
||||
// When syncStatus observes a container restart/replacement, the cached
|
||||
// STDIO pipe under that containerId is dead — evict it so the next call
|
||||
// redials (mcpctl#114). Setter injection, same as setInstanceService above.
|
||||
instanceService.setStdioInvalidator((cid) => mcpProxyService.removeClient(cid));
|
||||
const rbacDefinitionService = new RbacDefinitionService(rbacDefinitionRepo);
|
||||
const rbacService = new RbacService(rbacDefinitionRepo, prisma);
|
||||
const mcpTokenService = new McpTokenService(mcpTokenRepo, projectRepo, rbacDefinitionRepo, rbacService);
|
||||
@@ -673,6 +695,7 @@ async function main(): Promise<void> {
|
||||
registerSecretRoutes(app, secretService);
|
||||
registerSecretBackendRoutes(app, secretBackendService);
|
||||
registerSecretBackendRotateRoutes(app, secretBackendRotator);
|
||||
registerSecretBackendHealthRoutes(app, secretBackendService);
|
||||
registerSecretMigrateRoutes(app, secretMigrateService);
|
||||
registerLlmRoutes(app, llmService);
|
||||
registerAgentRoutes(app, agentService);
|
||||
@@ -960,6 +983,18 @@ async function main(): Promise<void> {
|
||||
app.log.error({ err }, 'secret keyNames backfill failed');
|
||||
});
|
||||
|
||||
// One-shot: pre-populate the secret value cache so a later backend outage is
|
||||
// absorbed rather than cascading into instance ERROR loops. Best-effort — if
|
||||
// the backend is already down at boot this is a no-op and instances fail
|
||||
// honestly. See bootstrap/warm-secret-cache.ts.
|
||||
warmSecretCache(
|
||||
prisma,
|
||||
secretService,
|
||||
{ info: (m: string): void => { app.log.info(m); }, warn: (m: string): void => { app.log.warn(m); } },
|
||||
).catch((err: unknown) => {
|
||||
app.log.warn({ err }, 'secret cache warm failed (non-fatal)');
|
||||
});
|
||||
|
||||
// Graceful shutdown
|
||||
setupGracefulShutdown(app, {
|
||||
disconnectDb: async () => {
|
||||
@@ -969,6 +1004,7 @@ async function main(): Promise<void> {
|
||||
healthProbeRunner.stop();
|
||||
secretBackendRotatorLoop.stop();
|
||||
gitBackup.stop();
|
||||
mcpProxyService.closeAll();
|
||||
await prisma.$disconnect();
|
||||
},
|
||||
});
|
||||
|
||||
76
src/mcpd/src/routes/secret-backend-health.ts
Normal file
76
src/mcpd/src/routes/secret-backend-health.ts
Normal file
@@ -0,0 +1,76 @@
|
||||
/**
|
||||
* GET /api/v1/secretbackends/:id/health — a live probe of a secret backend.
|
||||
*
|
||||
* Exists because the only health signal we had was `tokenMeta.lastRotationError`,
|
||||
* and the rotator writes that field ONLY for `auth: 'token'` backends
|
||||
* (`SecretBackendRotator.isRotatable()`). A `kubernetes`-auth backend therefore
|
||||
* never wrote it and rendered a hard-coded green tick in `mcpctl status` — even
|
||||
* with OpenBao completely unreachable.
|
||||
*
|
||||
* Two signals, deliberately separate, mirroring the liveness/readiness split
|
||||
* that instance health probes already use:
|
||||
*
|
||||
* live — is the backend reachable at all? (unauthenticated)
|
||||
* ready — can we actually read through it? (uses our credentials)
|
||||
*
|
||||
* A backend that is `live` but not `ready` is the exact shape of a re-initialised
|
||||
* OpenBao that left us holding valid-looking tokens granting nothing. Collapsing
|
||||
* the two into one boolean is what hid that for four days.
|
||||
*
|
||||
* RBAC: no special mapping needed — `mapUrlToPermission` falls through to the
|
||||
* generic `secretbackends` resource, so a GET requires `view:secretbackends`.
|
||||
*/
|
||||
import type { FastifyInstance } from 'fastify';
|
||||
import type { SecretBackendService } from '../services/secret-backend.service.js';
|
||||
import { NotFoundError } from '../services/mcp-server.service.js';
|
||||
|
||||
interface TokenMetaShape {
|
||||
lastRotationAt?: string;
|
||||
lastRotationError?: string | null;
|
||||
rotatable?: boolean;
|
||||
}
|
||||
|
||||
export function registerSecretBackendHealthRoutes(
|
||||
app: FastifyInstance,
|
||||
backends: SecretBackendService,
|
||||
): void {
|
||||
app.get<{ Params: { id: string } }>('/api/v1/secretbackends/:id/health', async (request, reply) => {
|
||||
try {
|
||||
const backend = await backends.getById(request.params.id);
|
||||
const driver = backends.driverFor(backend);
|
||||
|
||||
const live = await driver.healthCheck?.() ?? { ok: true, detail: 'no probe' };
|
||||
// Only probe readiness if the backend answered at all — otherwise the
|
||||
// auth check just re-reports the same outage with a confusing message.
|
||||
const ready = live.ok
|
||||
? await driver.authCheck?.() ?? { ok: true, detail: 'no probe' }
|
||||
: { ok: false, detail: 'not probed (backend unreachable)' };
|
||||
|
||||
const meta = (backend.tokenMeta ?? {}) as TokenMetaShape;
|
||||
const cache = backends.cacheStatsFor(backend);
|
||||
|
||||
return {
|
||||
backend: backend.name,
|
||||
type: backend.type,
|
||||
live: live.ok,
|
||||
liveDetail: live.detail,
|
||||
ready: ready.ok,
|
||||
readyDetail: ready.detail,
|
||||
// Present only for cached (remote) backends; plaintext has no cache.
|
||||
cache: cache ?? null,
|
||||
rotation: {
|
||||
rotatable: meta.rotatable ?? false,
|
||||
lastRotationAt: meta.lastRotationAt ?? null,
|
||||
lastRotationError: meta.lastRotationError ?? null,
|
||||
},
|
||||
};
|
||||
} catch (err) {
|
||||
if (err instanceof NotFoundError) {
|
||||
reply.code(404);
|
||||
return { error: err.message };
|
||||
}
|
||||
reply.code(502);
|
||||
return { error: err instanceof Error ? err.message : String(err) };
|
||||
}
|
||||
});
|
||||
}
|
||||
@@ -265,6 +265,16 @@ export class DockerContainerManager implements McpOrchestrator {
|
||||
const stderr = new PassThrough();
|
||||
this.docker.modem.demuxStream(stream, stdout, stderr);
|
||||
|
||||
// demuxStream never propagates end/close/error to the demuxed streams, so
|
||||
// a dead container left consumers waiting on a silent pipe — same funnel
|
||||
// as the one-shot execInContainer path uses (mcpctl#114).
|
||||
const endStdout = () => {
|
||||
if (!stdout.destroyed && !stdout.writableEnded) stdout.end();
|
||||
};
|
||||
stream.on('end', endStdout);
|
||||
stream.on('close', endStdout);
|
||||
stream.on('error', endStdout);
|
||||
|
||||
return {
|
||||
stdout,
|
||||
write(data: string) {
|
||||
|
||||
@@ -193,6 +193,13 @@ export class HealthProbeRunner {
|
||||
? 'unhealthy'
|
||||
: 'degraded';
|
||||
|
||||
// Crossing the threshold means every probe rode the same cached STDIO
|
||||
// pipe — evict it once so the next probe/call redials instead of failing
|
||||
// forever against a connection the other side already dropped.
|
||||
if (!result.healthy && state.consecutiveFailures === failureThreshold && instance.containerId) {
|
||||
this.mcpProxyService?.removeClient(instance.containerId);
|
||||
}
|
||||
|
||||
// Build event
|
||||
const probeLabel = probeKind === 'readiness'
|
||||
? `Readiness check (${healthCheck.tool})`
|
||||
|
||||
@@ -32,6 +32,9 @@ interface RetryMetadata {
|
||||
attemptCount?: number;
|
||||
lastAttemptAt?: string;
|
||||
nextRetryAt?: string;
|
||||
/** containerStatuses[0].restartCount at last sync — a bump means every
|
||||
* cached STDIO pipe to this (unchanged) containerId is dead. */
|
||||
lastRestartCount?: number;
|
||||
[k: string]: unknown;
|
||||
}
|
||||
|
||||
@@ -52,6 +55,8 @@ export class InvalidStateError extends Error {
|
||||
}
|
||||
|
||||
export class InstanceService {
|
||||
private stdioInvalidator?: (containerId: string) => void;
|
||||
|
||||
constructor(
|
||||
private instanceRepo: IMcpInstanceRepository,
|
||||
private serverRepo: IMcpServerRepository,
|
||||
@@ -59,6 +64,25 @@ export class InstanceService {
|
||||
private secretResolver?: SecretResolver,
|
||||
) {}
|
||||
|
||||
/**
|
||||
* Hook for evicting cached STDIO clients (McpProxyService.removeClient).
|
||||
* Setter injection, matching serverService.setInstanceService in main.ts —
|
||||
* McpProxyService is constructed after this service and already imports
|
||||
* from this file, so a constructor arg would be a circular import.
|
||||
*/
|
||||
setStdioInvalidator(fn: (containerId: string) => void): void {
|
||||
this.stdioInvalidator = fn;
|
||||
}
|
||||
|
||||
private invalidateStdio(containerId: string | null | undefined): void {
|
||||
if (!containerId) return;
|
||||
try {
|
||||
this.stdioInvalidator?.(containerId);
|
||||
} catch {
|
||||
/* best-effort */
|
||||
}
|
||||
}
|
||||
|
||||
async list(serverId?: string): Promise<McpInstance[]> {
|
||||
return this.instanceRepo.findAll(serverId);
|
||||
}
|
||||
@@ -71,37 +95,96 @@ export class InstanceService {
|
||||
|
||||
/**
|
||||
* Sync instance statuses with actual container state.
|
||||
* Detects crashed/stopped containers and marks them ERROR.
|
||||
*
|
||||
* Beyond marking crashed containers ERROR, this is the recovery path for
|
||||
* mcpctl#114: ERROR rows whose pod came back are re-adopted (the pod has
|
||||
* restartPolicy Always, so kubelet restarts the container in place and the
|
||||
* row must follow it back), and an in-place restart under an unchanged
|
||||
* containerId — visible only as a restartCount bump — invalidates any
|
||||
* cached STDIO pipe, which is dead by definition.
|
||||
*
|
||||
* Every metadata write here MERGES via readRetryMeta: updateStatus replaces
|
||||
* the JSON column wholesale, and clobbering nextRetryAt is what used to
|
||||
* make ERROR rows instantly dueForRetry and hot-loop against a 409.
|
||||
*/
|
||||
async syncStatus(): Promise<void> {
|
||||
const instances = await this.instanceRepo.findAll();
|
||||
for (const inst of instances) {
|
||||
if ((inst.status === 'RUNNING' || inst.status === 'STARTING') && inst.containerId) {
|
||||
try {
|
||||
const info = await this.orchestrator.inspectContainer(inst.containerId);
|
||||
if (!inst.containerId) continue;
|
||||
if (inst.status !== 'RUNNING' && inst.status !== 'STARTING' && inst.status !== 'ERROR') {
|
||||
continue;
|
||||
}
|
||||
|
||||
if (info.state === 'stopped' || info.state === 'error') {
|
||||
// Container died — get last logs for error context
|
||||
let errorMsg = `Container ${info.state}`;
|
||||
try {
|
||||
const logs = await this.orchestrator.getContainerLogs(inst.containerId, { tail: 5 });
|
||||
const lastLog = (logs.stdout || logs.stderr).trim().split('\n').pop();
|
||||
if (lastLog) errorMsg = lastLog;
|
||||
} catch { /* best-effort */ }
|
||||
await this.instanceRepo.updateStatus(inst.id, 'ERROR', {
|
||||
metadata: { error: errorMsg },
|
||||
});
|
||||
} else if (info.state === 'starting' && inst.status === 'RUNNING') {
|
||||
// Pod went back to starting (e.g. CrashLoopBackOff restart)
|
||||
await this.instanceRepo.updateStatus(inst.id, 'STARTING', {});
|
||||
} else if (info.state === 'running' && inst.status === 'STARTING') {
|
||||
// Pod became ready — promote to RUNNING
|
||||
await this.instanceRepo.updateStatus(inst.id, 'RUNNING', {});
|
||||
}
|
||||
} catch {
|
||||
// Container gone entirely
|
||||
let info: ContainerInfo;
|
||||
try {
|
||||
info = await this.orchestrator.inspectContainer(inst.containerId);
|
||||
} catch {
|
||||
// Container gone entirely. ERROR rows with a missing pod stay as they
|
||||
// are — the retry/backoff path owns recreating them.
|
||||
if (inst.status !== 'ERROR') {
|
||||
await this.instanceRepo.updateStatus(inst.id, 'ERROR', {
|
||||
metadata: { error: 'Container not found' },
|
||||
metadata: { ...readRetryMeta(inst), error: 'Container not found' },
|
||||
});
|
||||
this.invalidateStdio(inst.containerId);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
||||
const meta = readRetryMeta(inst);
|
||||
|
||||
if (inst.status === 'ERROR') {
|
||||
// The pod outlived the ERROR verdict (kubelet restarted the container
|
||||
// in place). Re-adopt instead of leaving the row stuck forever.
|
||||
if (info.state === 'running') {
|
||||
const { error: _e, attemptCount: _a, lastAttemptAt: _l, nextRetryAt: _n, ...rest } = meta;
|
||||
await this.instanceRepo.updateStatus(inst.id, 'RUNNING', {
|
||||
metadata: { ...rest, lastRestartCount: info.restartCount ?? 0 },
|
||||
});
|
||||
this.invalidateStdio(inst.containerId);
|
||||
} else if (info.state === 'starting') {
|
||||
// Keep retry metadata until it is actually running.
|
||||
await this.instanceRepo.updateStatus(inst.id, 'STARTING', { metadata: meta });
|
||||
this.invalidateStdio(inst.containerId);
|
||||
}
|
||||
// stopped/error: leave for the backoff/retry path.
|
||||
continue;
|
||||
}
|
||||
|
||||
if (info.state === 'stopped' || info.state === 'error') {
|
||||
// Container died — get last logs for error context
|
||||
let errorMsg = `Container ${info.state}`;
|
||||
try {
|
||||
const logs = await this.orchestrator.getContainerLogs(inst.containerId, { tail: 5 });
|
||||
const lastLog = (logs.stdout || logs.stderr).trim().split('\n').pop();
|
||||
if (lastLog) errorMsg = lastLog;
|
||||
} catch { /* best-effort */ }
|
||||
await this.instanceRepo.updateStatus(inst.id, 'ERROR', {
|
||||
metadata: { ...meta, error: errorMsg },
|
||||
});
|
||||
this.invalidateStdio(inst.containerId);
|
||||
} else if (info.state === 'starting' && inst.status === 'RUNNING') {
|
||||
// Pod went back to starting (e.g. CrashLoopBackOff restart)
|
||||
await this.instanceRepo.updateStatus(inst.id, 'STARTING', { metadata: meta });
|
||||
this.invalidateStdio(inst.containerId);
|
||||
} else if (info.state === 'running' && inst.status === 'STARTING') {
|
||||
// Pod became ready — promote to RUNNING and clear retry state.
|
||||
const { error: _e, attemptCount: _a, lastAttemptAt: _l, nextRetryAt: _n, ...rest } = meta;
|
||||
await this.instanceRepo.updateStatus(inst.id, 'RUNNING', {
|
||||
metadata: { ...rest, lastRestartCount: info.restartCount ?? 0 },
|
||||
});
|
||||
// A fresh/restarted pod under this name invalidates any cached pipe.
|
||||
this.invalidateStdio(inst.containerId);
|
||||
} else if (info.state === 'running' && info.restartCount !== undefined) {
|
||||
if (typeof meta.lastRestartCount === 'number' && info.restartCount > meta.lastRestartCount) {
|
||||
// In-place restart between polls: same pod name, dead pipes.
|
||||
await this.instanceRepo.updateStatus(inst.id, 'RUNNING', {
|
||||
metadata: { ...meta, lastRestartCount: info.restartCount },
|
||||
});
|
||||
this.invalidateStdio(inst.containerId);
|
||||
} else if (meta.lastRestartCount === undefined) {
|
||||
// First sighting: record the baseline without invalidating.
|
||||
await this.instanceRepo.updateStatus(inst.id, 'RUNNING', {
|
||||
metadata: { ...meta, lastRestartCount: info.restartCount },
|
||||
});
|
||||
}
|
||||
}
|
||||
@@ -232,6 +315,7 @@ export class InstanceService {
|
||||
} catch {
|
||||
// Container may already be gone
|
||||
}
|
||||
this.invalidateStdio(instance.containerId);
|
||||
}
|
||||
|
||||
await this.instanceRepo.delete(id);
|
||||
@@ -256,6 +340,7 @@ export class InstanceService {
|
||||
} catch {
|
||||
// best-effort
|
||||
}
|
||||
this.invalidateStdio(inst.containerId);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -489,6 +574,7 @@ export class InstanceService {
|
||||
try {
|
||||
await this.orchestrator.removeContainer(instance.containerId, true);
|
||||
} catch { /* best-effort */ }
|
||||
this.invalidateStdio(instance.containerId);
|
||||
}
|
||||
await this.instanceRepo.delete(instance.id);
|
||||
}
|
||||
|
||||
@@ -81,6 +81,18 @@ function podToContainerInfo(pod: V1Pod): ContainerInfo {
|
||||
info.port = ports[0].containerPort;
|
||||
}
|
||||
|
||||
// Restart visibility: a container that crashes and restarts IN PLACE keeps
|
||||
// the same pod name and reports state=running again — restartCount is the
|
||||
// only durable evidence, and syncStatus uses it to invalidate stale STDIO
|
||||
// pipes (mcpctl#114).
|
||||
const cs = pod.status?.containerStatuses?.[0];
|
||||
if (cs?.restartCount !== undefined) {
|
||||
info.restartCount = cs.restartCount;
|
||||
}
|
||||
if (cs?.state?.running?.startedAt) {
|
||||
info.startedAt = new Date(cs.state.running.startedAt as unknown as string);
|
||||
}
|
||||
|
||||
return info;
|
||||
}
|
||||
|
||||
@@ -116,10 +128,39 @@ export class KubernetesOrchestrator implements McpOrchestrator {
|
||||
}
|
||||
|
||||
const manifest = generatePodSpec(spec, this.namespace);
|
||||
const pod = await this.client.core.createNamespacedPod({
|
||||
namespace: this.namespace,
|
||||
body: manifest as V1Pod,
|
||||
});
|
||||
let pod;
|
||||
try {
|
||||
pod = await this.client.core.createNamespacedPod({
|
||||
namespace: this.namespace,
|
||||
body: manifest as V1Pod,
|
||||
});
|
||||
} catch (err) {
|
||||
if (httpStatusOf(err) !== 409) throw err;
|
||||
// AlreadyExists: a retry raced a pod that is still there. If it is
|
||||
// alive, ADOPT it — recreating under the same name can never succeed
|
||||
// and used to loop the instance in ERROR forever (mcpctl#114). Only a
|
||||
// genuinely dead pod is replaced.
|
||||
const podName = (manifest as V1Pod).metadata!.name!;
|
||||
const existing = await this.inspectContainer(podName);
|
||||
if (existing.state === 'running' || existing.state === 'starting') {
|
||||
return existing;
|
||||
}
|
||||
await this.removeContainer(podName, true);
|
||||
// deleteNamespacedPod returns before the pod is gone (grace period);
|
||||
// recreating while it is Terminating would 409 again. Bounded wait.
|
||||
for (let i = 0; i < 15; i++) {
|
||||
await new Promise((resolve) => setTimeout(resolve, 1000));
|
||||
try {
|
||||
await this.inspectContainer(podName);
|
||||
} catch {
|
||||
break; // 404 — pod is gone
|
||||
}
|
||||
}
|
||||
pod = await this.client.core.createNamespacedPod({
|
||||
namespace: this.namespace,
|
||||
body: manifest as V1Pod,
|
||||
});
|
||||
}
|
||||
|
||||
// Wait briefly for pod to start scheduling
|
||||
await new Promise((resolve) => setTimeout(resolve, 500));
|
||||
@@ -276,6 +317,17 @@ export class KubernetesOrchestrator implements McpOrchestrator {
|
||||
// Wait for WebSocket connection to establish
|
||||
const ws = await wsPromise;
|
||||
|
||||
// client-node's WebSocketHandler only ends stdout on an explicit
|
||||
// CloseStream/Status frame from the apiserver. An abnormal socket death
|
||||
// (container OOMKilled, node reboot) emits neither — without these
|
||||
// handlers the PassThrough stays open and the consumer never learns the
|
||||
// pipe is dead (mcpctl#114).
|
||||
const endStdout = () => {
|
||||
if (!stdout.destroyed && !stdout.writableEnded) stdout.end();
|
||||
};
|
||||
ws.on('close', endStdout);
|
||||
ws.on('error', endStdout);
|
||||
|
||||
return {
|
||||
stdout,
|
||||
write(data: string) {
|
||||
@@ -316,6 +368,13 @@ export class KubernetesOrchestrator implements McpOrchestrator {
|
||||
false, // tty
|
||||
);
|
||||
|
||||
// Same abnormal-death funnel as execInteractive — see comment there.
|
||||
const endStdout = () => {
|
||||
if (!stdout.destroyed && !stdout.writableEnded) stdout.end();
|
||||
};
|
||||
ws.on('close', endStdout);
|
||||
ws.on('error', endStdout);
|
||||
|
||||
return {
|
||||
stdout,
|
||||
write(data: string) {
|
||||
|
||||
@@ -180,20 +180,28 @@ export class McpProxyService {
|
||||
} catch (err) {
|
||||
this.removeClient(instance.containerId);
|
||||
// Fall back to one-shot exec when we have a command to run.
|
||||
// Attach mode has no equivalent one-shot fallback — surface the error.
|
||||
if (mode.kind === 'exec') {
|
||||
return sendViaStdio(this.orchestrator, instance.containerId, packageName, method, params, 120_000, command, runtime);
|
||||
}
|
||||
const detail = formatError(err);
|
||||
console.error(`[mcp-proxy] attach to ${instance.containerId} failed:`, err);
|
||||
return {
|
||||
jsonrpc: '2.0',
|
||||
id: 1,
|
||||
error: {
|
||||
code: -32000,
|
||||
message: `STDIO attach to '${instance.containerId}' failed: ${detail}`,
|
||||
},
|
||||
};
|
||||
// Attach mode has no one-shot equivalent, but the failure is usually
|
||||
// a stale pipe from an in-place container restart — retry once
|
||||
// through a fresh client (which redials) before surfacing the error,
|
||||
// so the FIRST call after a detected death succeeds (mcpctl#114).
|
||||
try {
|
||||
return await this.sendViaPersistentStdio(instance.containerId, mode, method, params);
|
||||
} catch (retryErr) {
|
||||
this.removeClient(instance.containerId);
|
||||
const detail = formatError(retryErr);
|
||||
console.error(`[mcp-proxy] attach to ${instance.containerId} failed (after retry):`, retryErr);
|
||||
return {
|
||||
jsonrpc: '2.0',
|
||||
id: 1,
|
||||
error: {
|
||||
code: -32000,
|
||||
message: `STDIO attach to '${instance.containerId}' failed: ${detail}`,
|
||||
},
|
||||
};
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -69,6 +69,14 @@ export interface ContainerInfo {
|
||||
/** Container IP on the first non-default network (for internal communication) */
|
||||
ip?: string;
|
||||
createdAt: Date;
|
||||
/**
|
||||
* Times the container restarted in place (k8s containerStatuses[0]).
|
||||
* A bump with an unchanged containerId means every cached STDIO pipe to it
|
||||
* is dead — syncStatus uses this to invalidate them (mcpctl#114).
|
||||
*/
|
||||
restartCount?: number;
|
||||
/** When the current container process started (k8s state.running.startedAt). */
|
||||
startedAt?: Date;
|
||||
}
|
||||
|
||||
export interface ContainerLogs {
|
||||
|
||||
@@ -26,7 +26,11 @@ export interface SecretBackendRotatorLoopDeps {
|
||||
/** Override in tests. */
|
||||
setTimeout?: (cb: () => void, ms: number) => NodeJS.Timeout;
|
||||
clearTimeout?: (t: NodeJS.Timeout) => void;
|
||||
log?: { info: (msg: string) => void; warn: (msg: string) => void };
|
||||
log?: {
|
||||
info: (msg: string) => void;
|
||||
warn: (msg: string) => void;
|
||||
error: (obj: Record<string, unknown>, msg: string) => void;
|
||||
};
|
||||
}
|
||||
|
||||
const DEFAULT_INTERVAL_MS = 24 * 3600 * 1000;
|
||||
@@ -36,7 +40,7 @@ export class SecretBackendRotatorLoop {
|
||||
private readonly timers = new Map<string, NodeJS.Timeout>();
|
||||
private readonly setT: (cb: () => void, ms: number) => NodeJS.Timeout;
|
||||
private readonly clearT: (t: NodeJS.Timeout) => void;
|
||||
private readonly log: { info: (msg: string) => void; warn: (msg: string) => void };
|
||||
private readonly log: NonNullable<SecretBackendRotatorLoopDeps['log']>;
|
||||
private stopped = false;
|
||||
|
||||
constructor(private readonly deps: SecretBackendRotatorLoopDeps) {
|
||||
@@ -44,9 +48,11 @@ export class SecretBackendRotatorLoop {
|
||||
this.clearT = deps.clearTimeout ?? ((t) => global.clearTimeout(t));
|
||||
this.log = deps.log ?? {
|
||||
// eslint-disable-next-line no-console
|
||||
info: (m) => console.log(`[rotator] ${m}`),
|
||||
info: (m: string): void => { console.log(`[rotator] ${m}`); },
|
||||
// eslint-disable-next-line no-console
|
||||
warn: (m) => console.warn(`[rotator] ${m}`),
|
||||
warn: (m: string): void => { console.warn(`[rotator] ${m}`); },
|
||||
// eslint-disable-next-line no-console
|
||||
error: (obj: Record<string, unknown>, m: string): void => { console.error(JSON.stringify({ level: 'fatal', ...obj, message: m })); },
|
||||
};
|
||||
}
|
||||
|
||||
@@ -70,13 +76,10 @@ export class SecretBackendRotatorLoop {
|
||||
this.deps.rotator.healthCheck(b.id)
|
||||
.then((res) => {
|
||||
if (!res.ok) {
|
||||
// eslint-disable-next-line no-console
|
||||
console.error(JSON.stringify({
|
||||
level: 'fatal',
|
||||
kind: 'BACKEND_TOKEN_DEAD',
|
||||
backend: b.name,
|
||||
message: res.message ?? 'unknown',
|
||||
}));
|
||||
this.log.error(
|
||||
{ kind: 'BACKEND_TOKEN_DEAD', backend: b.name },
|
||||
res.message ?? 'unknown',
|
||||
);
|
||||
this.log.warn(`backend '${b.name}' health check failed: ${res.message ?? 'unknown'}`);
|
||||
}
|
||||
})
|
||||
|
||||
@@ -53,18 +53,37 @@ export interface TokenMeta {
|
||||
rotatable?: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
* Structured logger. Must be a real pino-shaped logger in production: the
|
||||
* `BACKEND_TOKEN_DEAD` fatals below used to go out via bare `console.error`,
|
||||
* which bypasses the pino multistream feeding `ErrorLogBuffer` — so the one
|
||||
* failure `mcpctl errors` exists to surface was the one it never saw.
|
||||
*/
|
||||
export interface RotatorLog {
|
||||
error(obj: Record<string, unknown>, msg: string): void;
|
||||
warn(msg: string): void;
|
||||
}
|
||||
|
||||
export interface SecretBackendRotatorDeps {
|
||||
backends: SecretBackendService;
|
||||
secrets: SecretService;
|
||||
fetch?: typeof globalThis.fetch;
|
||||
now?: () => Date;
|
||||
log?: RotatorLog;
|
||||
}
|
||||
|
||||
export class SecretBackendRotator {
|
||||
private readonly now: () => Date;
|
||||
private readonly log: RotatorLog;
|
||||
|
||||
constructor(private readonly deps: SecretBackendRotatorDeps) {
|
||||
this.now = deps.now ?? (() => new Date());
|
||||
this.log = deps.log ?? {
|
||||
// eslint-disable-next-line no-console
|
||||
error: (obj: Record<string, unknown>, msg: string): void => { console.error(JSON.stringify({ level: 'fatal', ...obj, message: msg })); },
|
||||
// eslint-disable-next-line no-console
|
||||
warn: (msg: string): void => { console.warn(msg); },
|
||||
};
|
||||
}
|
||||
|
||||
/** True iff this backend is a wizard-provisioned token-auth openbao with rotation enabled. */
|
||||
@@ -144,15 +163,16 @@ export class SecretBackendRotator {
|
||||
: err;
|
||||
const wrappedMsg = wrapped instanceof Error ? wrapped.message : String(wrapped);
|
||||
await this.recordError(backendId, meta, wrappedMsg);
|
||||
// Loud, structured log so the operator sees it in `kubectl logs deploy/mcpd`.
|
||||
// eslint-disable-next-line no-console
|
||||
console.error(JSON.stringify({
|
||||
level: 'fatal',
|
||||
kind: tokenDead ? 'BACKEND_TOKEN_DEAD' : 'BACKEND_ROTATION_FAILED',
|
||||
backend: backend.name,
|
||||
url: cfg.url,
|
||||
message: wrappedMsg,
|
||||
}));
|
||||
// Loud and structured, through pino so it also lands in ErrorLogBuffer
|
||||
// and therefore in `mcpctl errors` — not just in `kubectl logs`.
|
||||
this.log.error(
|
||||
{
|
||||
kind: tokenDead ? 'BACKEND_TOKEN_DEAD' : 'BACKEND_ROTATION_FAILED',
|
||||
backend: backend.name,
|
||||
url: cfg.url,
|
||||
},
|
||||
wrappedMsg,
|
||||
);
|
||||
throw wrapped;
|
||||
}
|
||||
|
||||
@@ -164,7 +184,7 @@ export class SecretBackendRotator {
|
||||
// Log but don't fail the rotation — the new token is already live.
|
||||
const msg = err instanceof Error ? err.message : String(err);
|
||||
// eslint-disable-next-line no-console
|
||||
console.warn(`rotation: revoke old accessor '${oldAccessor}' on backend '${backend.name}' failed (continuing): ${msg}`);
|
||||
this.log.warn(`rotation: revoke old accessor '${oldAccessor}' on backend '${backend.name}' failed (continuing): ${msg}`);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -249,7 +269,7 @@ export class SecretBackendRotator {
|
||||
} catch (inner) {
|
||||
// Don't mask the original error — just log the DB failure.
|
||||
// eslint-disable-next-line no-console
|
||||
console.warn(`rotation: failed to persist lastRotationError (${message}): ${inner instanceof Error ? inner.message : String(inner)}`);
|
||||
this.log.warn(`rotation: failed to persist lastRotationError (${message}): ${inner instanceof Error ? inner.message : String(inner)}`);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2,6 +2,7 @@ import type { SecretBackend } from '@prisma/client';
|
||||
import type { ISecretBackendRepository } from '../repositories/secret-backend.repository.js';
|
||||
import type { SecretBackendDriver } from './secret-backends/types.js';
|
||||
import { createDriver, type DriverFactoryDeps } from './secret-backends/factory.js';
|
||||
import { CachingSecretBackendDriver, type CachingDriverOptions, type CacheStats } from './secret-backends/caching.js';
|
||||
import { NotFoundError, ConflictError } from './mcp-server.service.js';
|
||||
|
||||
export class SecretBackendInUseError extends Error {
|
||||
@@ -17,6 +18,7 @@ export class SecretBackendService {
|
||||
constructor(
|
||||
private readonly repo: ISecretBackendRepository,
|
||||
private readonly driverDeps: DriverFactoryDeps,
|
||||
private readonly cacheOpts: CachingDriverOptions = {},
|
||||
) {}
|
||||
|
||||
async list(): Promise<SecretBackend[]> {
|
||||
@@ -87,12 +89,34 @@ export class SecretBackendService {
|
||||
this.driverCache.delete(id);
|
||||
}
|
||||
|
||||
/** Get the driver for a given backend id, creating + caching on first call. */
|
||||
/**
|
||||
* Get the driver for a given backend id, creating + caching on first call.
|
||||
*
|
||||
* Remote backends are wrapped in `CachingSecretBackendDriver` so a backend
|
||||
* outage degrades to "serving last known-good" instead of failing every
|
||||
* caller. `plaintext` is deliberately NOT wrapped: its `read()` is an
|
||||
* identity function over the DB row passed in by the caller, so a value cache
|
||||
* there would serve pre-update data with nothing to invalidate it.
|
||||
*
|
||||
* Config changes invalidate for free — `update()` and `delete()` drop this
|
||||
* map, and the value cache lives inside the driver instance, which is right:
|
||||
* if `url`/`mount`/`pathPrefix` change, the cached names now mean something
|
||||
* different.
|
||||
*/
|
||||
driverFor(backend: SecretBackend): SecretBackendDriver {
|
||||
const cached = this.driverCache.get(backend.id);
|
||||
if (cached) return cached;
|
||||
const driver = createDriver(backend, this.driverDeps);
|
||||
const base = createDriver(backend, this.driverDeps);
|
||||
const driver = backend.type === 'plaintext'
|
||||
? base
|
||||
: new CachingSecretBackendDriver(base, { ...this.cacheOpts, backendName: backend.name });
|
||||
this.driverCache.set(backend.id, driver);
|
||||
return driver;
|
||||
}
|
||||
|
||||
/** Cache state for a backend, for the health endpoint. Never exposes values. */
|
||||
cacheStatsFor(backend: SecretBackend): CacheStats | undefined {
|
||||
const driver = this.driverFor(backend);
|
||||
return driver instanceof CachingSecretBackendDriver ? driver.stats() : undefined;
|
||||
}
|
||||
}
|
||||
|
||||
215
src/mcpd/src/services/secret-backends/caching.ts
Normal file
215
src/mcpd/src/services/secret-backends/caching.ts
Normal file
@@ -0,0 +1,215 @@
|
||||
/**
|
||||
* Caching + stale-while-error decorator for any `SecretBackendDriver`.
|
||||
*
|
||||
* ## Why this exists
|
||||
*
|
||||
* `SecretService.resolveData()` calls `driver.read()` on *every* use, and every
|
||||
* consumer funnels through it: server env resolution, LLM api keys, chat, git
|
||||
* providers, code repos, webhooks. With a remote backend that means one network
|
||||
* round-trip per secret per call, and — worse — any OpenBao blip propagates
|
||||
* straight through. An instance that restarts during a blip fails env
|
||||
* resolution, gets marked ERROR, and enters a 30s×5-then-5min backoff
|
||||
* (`instance.service.ts`), so a few seconds of backend unavailability turns
|
||||
* into minutes of degraded service.
|
||||
*
|
||||
* ## Semantics
|
||||
*
|
||||
* - **Fresh** (age < ttlMs): served from memory, no network.
|
||||
* - **Stale-while-error**: past the TTL we always try the backend first. If it
|
||||
* answers, we refresh. If it fails *as a transport failure*
|
||||
* (`SecretBackendUnavailableError`), we serve the last known-good value
|
||||
* instead of throwing. This is the part that actually stops the ERROR storm.
|
||||
* - **`SecretNotFoundError` evicts and rethrows.** Never served stale — that
|
||||
* would resurrect a deliberately deleted or revoked credential, which is
|
||||
* strictly worse than an outage.
|
||||
* - **Any other error rethrows, without stale.** A 403 that survives a token
|
||||
* refresh means our grants were revoked; papering over it with cached data is
|
||||
* exactly how an upstream OpenBao re-init once went unnoticed for four days.
|
||||
* - **No negative caching.** A miss must re-check; retry/backoff already lives
|
||||
* in the driver.
|
||||
*
|
||||
* The stale window is deliberately unbounded. A cap would mean a long outage
|
||||
* eventually takes mcpd down anyway, which defeats the purpose, and the
|
||||
* revoked-credential case is already handled definitively by `SecretNotFound`.
|
||||
*
|
||||
* ## What this does NOT fix
|
||||
*
|
||||
* A cold cache during an outage. If mcpd restarts while the backend is
|
||||
* unreachable, nothing has a last-known-good value and secret-bearing servers
|
||||
* fail to start — honestly, with a loud error. That is the correct behaviour:
|
||||
* booting a server with an empty credential is the failure mode that had
|
||||
* gitea-mcp reporting healthy while every authed call failed. The mitigation is
|
||||
* to warm this cache at boot, not to invent a value.
|
||||
*
|
||||
* Values live in heap in cleartext for the TTL, so the map is bounded (LRU) and
|
||||
* values are never logged.
|
||||
*/
|
||||
import type { SecretBackendDriver, SecretData, ExternalRef } from './types.js';
|
||||
import { SecretNotFoundError, SecretBackendUnavailableError } from './types.js';
|
||||
|
||||
export interface CachingDriverLog {
|
||||
warn(obj: Record<string, unknown>, msg: string): void;
|
||||
info(obj: Record<string, unknown>, msg: string): void;
|
||||
}
|
||||
|
||||
export interface CachingDriverOptions {
|
||||
/** How long a value is served without consulting the backend. */
|
||||
ttlMs?: number;
|
||||
/** LRU bound — these are plaintext credentials held in memory. */
|
||||
maxEntries?: number;
|
||||
/** Backend name, for log context only. */
|
||||
backendName?: string;
|
||||
now?: () => number;
|
||||
log?: CachingDriverLog;
|
||||
}
|
||||
|
||||
interface CacheEntry {
|
||||
data: SecretData;
|
||||
fetchedAt: number;
|
||||
/** Set when we last served this past its TTL because the backend was down. */
|
||||
staleSince: number | undefined;
|
||||
}
|
||||
|
||||
export const DEFAULT_CACHE_TTL_MS = 300_000;
|
||||
export const DEFAULT_CACHE_MAX_ENTRIES = 500;
|
||||
|
||||
const NOOP_LOG: CachingDriverLog = { warn: () => undefined, info: () => undefined };
|
||||
|
||||
export interface CacheStats {
|
||||
entries: number;
|
||||
servingStale: number;
|
||||
oldestStaleSince: number | undefined;
|
||||
}
|
||||
|
||||
export class CachingSecretBackendDriver implements SecretBackendDriver {
|
||||
readonly kind: string;
|
||||
|
||||
private readonly entries = new Map<string, CacheEntry>();
|
||||
private readonly ttlMs: number;
|
||||
private readonly maxEntries: number;
|
||||
private readonly backendName: string;
|
||||
private readonly nowFn: () => number;
|
||||
private readonly log: CachingDriverLog;
|
||||
|
||||
constructor(private readonly inner: SecretBackendDriver, opts: CachingDriverOptions = {}) {
|
||||
this.kind = `cached:${inner.kind}`;
|
||||
this.ttlMs = opts.ttlMs ?? DEFAULT_CACHE_TTL_MS;
|
||||
this.maxEntries = opts.maxEntries ?? DEFAULT_CACHE_MAX_ENTRIES;
|
||||
this.backendName = opts.backendName ?? inner.kind;
|
||||
this.nowFn = opts.now ?? ((): number => Date.now());
|
||||
this.log = opts.log ?? NOOP_LOG;
|
||||
}
|
||||
|
||||
async read(input: { name: string; externalRef: ExternalRef; data: SecretData }): Promise<SecretData> {
|
||||
const now = this.nowFn();
|
||||
const cached = this.entries.get(input.name);
|
||||
|
||||
if (cached !== undefined && now - cached.fetchedAt < this.ttlMs) {
|
||||
this.touch(input.name, cached);
|
||||
return cached.data;
|
||||
}
|
||||
|
||||
try {
|
||||
const data = await this.inner.read(input);
|
||||
if (cached?.staleSince !== undefined) {
|
||||
// Edge-triggered: only on the transition back to healthy.
|
||||
this.log.info(
|
||||
{ kind: 'BACKEND_RECOVERED', backend: this.backendName, secret: input.name,
|
||||
staleForMs: now - cached.staleSince },
|
||||
`secret backend '${this.backendName}' recovered; '${input.name}' is live again`,
|
||||
);
|
||||
}
|
||||
this.store(input.name, { data, fetchedAt: now, staleSince: undefined });
|
||||
return data;
|
||||
} catch (err) {
|
||||
if (err instanceof SecretNotFoundError) {
|
||||
// Definitive. Drop the stale copy so we can never hand it out later.
|
||||
this.entries.delete(input.name);
|
||||
throw err;
|
||||
}
|
||||
if (!(err instanceof SecretBackendUnavailableError) || cached === undefined) {
|
||||
throw err;
|
||||
}
|
||||
if (cached.staleSince === undefined) {
|
||||
cached.staleSince = now;
|
||||
this.log.warn(
|
||||
{ kind: 'BACKEND_UNREACHABLE', backend: this.backendName, secret: input.name,
|
||||
ageMs: now - cached.fetchedAt, reason: err.message },
|
||||
`secret backend '${this.backendName}' unreachable; serving cached '${input.name}'`,
|
||||
);
|
||||
}
|
||||
this.touch(input.name, cached);
|
||||
return cached.data;
|
||||
}
|
||||
}
|
||||
|
||||
async write(input: { name: string; data: SecretData }): Promise<{ externalRef: ExternalRef; storedData: SecretData }> {
|
||||
const result = await this.inner.write(input);
|
||||
// Cache what a subsequent read() would return — the values just written —
|
||||
// not `storedData`, which remote drivers deliberately leave empty.
|
||||
this.store(input.name, { data: input.data, fetchedAt: this.nowFn(), staleSince: undefined });
|
||||
return result;
|
||||
}
|
||||
|
||||
async delete(input: { name: string; externalRef: ExternalRef }): Promise<void> {
|
||||
await this.inner.delete(input);
|
||||
this.entries.delete(input.name);
|
||||
}
|
||||
|
||||
async list(): Promise<Array<{ name: string; externalRef: ExternalRef }>> {
|
||||
return this.inner.list();
|
||||
}
|
||||
|
||||
async healthCheck(): Promise<{ ok: boolean; detail?: string }> {
|
||||
return this.inner.healthCheck?.() ?? { ok: true, detail: 'no probe' };
|
||||
}
|
||||
|
||||
/**
|
||||
* Identity provisioning is a control-plane operation on the backend itself,
|
||||
* not a value read — nothing to cache, so pass straight through. Throwing
|
||||
* when unsupported (rather than silently succeeding) keeps a misconfigured
|
||||
* backend from looking like it granted access it never did.
|
||||
*/
|
||||
async ensureServerIdentity(input: { name: string; namespace: string; secretNames: string[] }): Promise<void> {
|
||||
if (this.inner.ensureServerIdentity === undefined) {
|
||||
throw new Error(`backend '${this.backendName}' (${this.inner.kind}) does not support per-server identities`);
|
||||
}
|
||||
await this.inner.ensureServerIdentity(input);
|
||||
}
|
||||
|
||||
async removeServerIdentity(input: { name: string }): Promise<void> {
|
||||
await this.inner.removeServerIdentity?.(input);
|
||||
}
|
||||
|
||||
async authCheck(): Promise<{ ok: boolean; detail?: string }> {
|
||||
return this.inner.authCheck?.() ?? { ok: true, detail: 'no probe' };
|
||||
}
|
||||
|
||||
/** Cache state for the backend health endpoint. Never exposes values. */
|
||||
stats(): CacheStats {
|
||||
let servingStale = 0;
|
||||
let oldestStaleSince: number | undefined;
|
||||
for (const e of this.entries.values()) {
|
||||
if (e.staleSince === undefined) continue;
|
||||
servingStale++;
|
||||
if (oldestStaleSince === undefined || e.staleSince < oldestStaleSince) oldestStaleSince = e.staleSince;
|
||||
}
|
||||
return { entries: this.entries.size, servingStale, oldestStaleSince };
|
||||
}
|
||||
|
||||
/** Move an entry to the MRU end of the insertion-ordered Map. */
|
||||
private touch(name: string, entry: CacheEntry): void {
|
||||
this.entries.delete(name);
|
||||
this.entries.set(name, entry);
|
||||
}
|
||||
|
||||
private store(name: string, entry: CacheEntry): void {
|
||||
this.entries.delete(name);
|
||||
this.entries.set(name, entry);
|
||||
while (this.entries.size > this.maxEntries) {
|
||||
const oldest = this.entries.keys().next();
|
||||
if (oldest.done === true) break;
|
||||
this.entries.delete(oldest.value);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -7,7 +7,7 @@
|
||||
* POST <url>/v1/<mount>/data/<path> -- write
|
||||
* GET <url>/v1/<mount>/data/<path> -- read latest
|
||||
* DELETE <url>/v1/<mount>/metadata/<path> -- full delete (all versions)
|
||||
* LIST <url>/v1/<mount>/metadata/ -- for migration
|
||||
* GET <url>/v1/<mount>/metadata/?list=true -- for migration (see list())
|
||||
* POST <url>/v1/auth/<mount>/login -- kubernetes auth
|
||||
*
|
||||
* Auth strategies (`config.auth`):
|
||||
@@ -28,6 +28,14 @@
|
||||
*/
|
||||
import { readFile } from 'node:fs/promises';
|
||||
import type { SecretBackendDriver, SecretData, ExternalRef, SecretRefResolver } from './types.js';
|
||||
import { SecretNotFoundError, SecretBackendUnavailableError } from './types.js';
|
||||
import {
|
||||
buildServerSecretPolicyHcl,
|
||||
writePolicy,
|
||||
deletePolicy,
|
||||
ensureKubernetesAuthRole,
|
||||
deleteKubernetesAuthRole,
|
||||
} from '@mcpctl/shared';
|
||||
|
||||
/** Best-effort read of a response body for error messages. Empty on parse failure. */
|
||||
async function bodyText(res: Response): Promise<string> {
|
||||
@@ -77,10 +85,23 @@ export interface OpenBaoDriverDeps {
|
||||
readServiceAccountToken?: (path: string) => Promise<string>;
|
||||
/** Clock for cache TTL — overridable in tests. */
|
||||
now?: () => number;
|
||||
/** Per-request timeout. Without one, an unreachable OpenBao hangs every caller. */
|
||||
timeoutMs?: number;
|
||||
/** Total attempts for retryable failures (network / 5xx / 429). 1 disables retry. */
|
||||
maxAttempts?: number;
|
||||
/** Base for exponential backoff between retries; full jitter is applied. */
|
||||
backoffBaseMs?: number;
|
||||
/** Test seam — real sleeps would make the retry tests take seconds. */
|
||||
sleep?: (ms: number) => Promise<void>;
|
||||
}
|
||||
|
||||
const SA_TOKEN_DEFAULT_PATH = '/var/run/secrets/kubernetes.io/serviceaccount/token';
|
||||
const TOKEN_RENEW_GRACE_MS = 60_000;
|
||||
const DEFAULT_TIMEOUT_MS = 5_000;
|
||||
const DEFAULT_MAX_ATTEMPTS = 3;
|
||||
const DEFAULT_BACKOFF_BASE_MS = 200;
|
||||
/** Statuses worth retrying: the backend is up but cannot answer right now. */
|
||||
const RETRYABLE_STATUS = new Set([429, 500, 502, 503, 504]);
|
||||
|
||||
export class OpenBaoDriver implements SecretBackendDriver {
|
||||
readonly kind = 'openbao';
|
||||
@@ -98,6 +119,10 @@ export class OpenBaoDriver implements SecretBackendDriver {
|
||||
private readonly resolver: SecretRefResolver | undefined;
|
||||
private readonly readSaToken: (path: string) => Promise<string>;
|
||||
private readonly nowFn: () => number;
|
||||
private readonly timeoutMs: number;
|
||||
private readonly maxAttempts: number;
|
||||
private readonly backoffBaseMs: number;
|
||||
private readonly sleep: (ms: number) => Promise<void>;
|
||||
|
||||
// Cached vault token + when (epoch ms) it should be considered expired and refetched.
|
||||
private cachedToken: string | undefined;
|
||||
@@ -131,13 +156,19 @@ export class OpenBaoDriver implements SecretBackendDriver {
|
||||
if (deps.secretRefResolver !== undefined) this.resolver = deps.secretRefResolver;
|
||||
this.readSaToken = deps.readServiceAccountToken ?? ((path) => readFile(path, 'utf-8').then((s) => s.trim()));
|
||||
this.nowFn = deps.now ?? (() => Date.now());
|
||||
this.timeoutMs = deps.timeoutMs ?? DEFAULT_TIMEOUT_MS;
|
||||
this.maxAttempts = deps.maxAttempts ?? DEFAULT_MAX_ATTEMPTS;
|
||||
this.backoffBaseMs = deps.backoffBaseMs ?? DEFAULT_BACKOFF_BASE_MS;
|
||||
this.sleep = deps.sleep ?? ((ms: number): Promise<void> => new Promise((r) => { setTimeout(r, ms); }));
|
||||
}
|
||||
|
||||
async read(input: { name: string; externalRef: ExternalRef; data: SecretData }): Promise<SecretData> {
|
||||
const path = this.pathFor(input.name);
|
||||
const res = await this.request('GET', `/v1/${this.mount}/data/${path}`);
|
||||
if (res.status === 404) {
|
||||
throw new Error(`OpenBao: secret '${input.name}' not found at ${path}`);
|
||||
// Definitive answer, not a transport failure — the caching decorator
|
||||
// must evict rather than serve a stale value here.
|
||||
throw new SecretNotFoundError(`OpenBao: secret '${input.name}' not found at ${path}`);
|
||||
}
|
||||
if (!res.ok) throw new Error(`OpenBao read ${path}: HTTP ${res.status} ${await bodyText(res)}`);
|
||||
const body = await res.json() as { data?: { data?: SecretData } };
|
||||
@@ -161,7 +192,13 @@ export class OpenBaoDriver implements SecretBackendDriver {
|
||||
|
||||
async list(): Promise<Array<{ name: string; externalRef: ExternalRef }>> {
|
||||
const listPath = this.pathPrefix === '' ? '' : `${this.pathPrefix}/`;
|
||||
const res = await this.request('LIST', `/v1/${this.mount}/metadata/${listPath}`);
|
||||
// `GET ?list=true`, not the LIST verb. OpenBao accepts both, but LIST is a
|
||||
// non-standard HTTP method and proxies drop it: through Cilium's ingress
|
||||
// Envoy (which is how `bao-k8s` is reached) LIST returns a bare
|
||||
// `400 Bad Request` while the GET form returns normally. Verified live
|
||||
// 2026-08-20 — this silently broke `mcpctl migrate secrets` against any
|
||||
// ingress-fronted backend.
|
||||
const res = await this.request('GET', `/v1/${this.mount}/metadata/${listPath}?list=true`);
|
||||
if (res.status === 404) return [];
|
||||
if (!res.ok) throw new Error(`OpenBao list: HTTP ${res.status} ${await bodyText(res)}`);
|
||||
const body = await res.json() as { data?: { keys?: string[] } };
|
||||
@@ -174,15 +211,104 @@ export class OpenBaoDriver implements SecretBackendDriver {
|
||||
}));
|
||||
}
|
||||
|
||||
/**
|
||||
* LIVENESS. Deliberately unauthenticated: `sys/health` needs no token, and
|
||||
* routing it through `request()` (as this used to) took a login first — so an
|
||||
* expired role reported as "OpenBao is down", and every probe cost a login.
|
||||
*
|
||||
* OpenBao encodes its state in the status code, so map it rather than
|
||||
* collapsing everything to ok/not-ok.
|
||||
*/
|
||||
async healthCheck(): Promise<{ ok: boolean; detail?: string }> {
|
||||
try {
|
||||
const res = await this.request('GET', '/v1/sys/health');
|
||||
return { ok: res.ok, detail: `HTTP ${res.status}` };
|
||||
const headers: Record<string, string> = {};
|
||||
if (this.namespace !== undefined) headers['X-Vault-Namespace'] = this.namespace;
|
||||
const res = await this.fetchImpl(`${this.url}/v1/sys/health`, {
|
||||
method: 'GET',
|
||||
headers,
|
||||
signal: AbortSignal.timeout(this.timeoutMs),
|
||||
});
|
||||
switch (res.status) {
|
||||
case 200: return { ok: true, detail: 'active' };
|
||||
case 429: return { ok: true, detail: 'standby' };
|
||||
case 472: case 473: return { ok: true, detail: 'replication secondary' };
|
||||
case 501: return { ok: false, detail: 'not initialized' };
|
||||
case 503: return { ok: false, detail: 'sealed' };
|
||||
default: return { ok: res.ok, detail: `HTTP ${String(res.status)}` };
|
||||
}
|
||||
} catch (err) {
|
||||
return { ok: false, detail: err instanceof Error ? err.message : String(err) };
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* READINESS. Exercises the capability we actually depend on — read/list under
|
||||
* `<mount>/<pathPrefix>/` — using the credentials we hold.
|
||||
*
|
||||
* `list()` rather than `auth/token/lookup-self` on purpose: lookup-self only
|
||||
* proves the token exists, not that its policy still grants anything. The
|
||||
* four-day outage in e51b924 was exactly a live token whose grants had been
|
||||
* dropped by an upstream re-init. The existing read policy already permits
|
||||
* this call, so it needs no bao-side change.
|
||||
*/
|
||||
async authCheck(): Promise<{ ok: boolean; detail?: string }> {
|
||||
try {
|
||||
await this.list();
|
||||
return { ok: true, detail: `readable at ${this.mount}/${this.pathPrefix}` };
|
||||
} catch (err) {
|
||||
return { ok: false, detail: err instanceof Error ? err.message : String(err) };
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Create/refresh a per-server OpenBao identity: a policy naming exactly this
|
||||
* server's secrets, and a Kubernetes auth role bound to exactly its
|
||||
* ServiceAccount.
|
||||
*
|
||||
* Requires mcpd's own token to hold the provisioning grant (see
|
||||
* `buildServerProvisioningPolicyHcl`) — deliberately confined to the
|
||||
* `mcpctl-server-` name prefix, so this cannot mint an identity that reads
|
||||
* anything mcpd itself could not already read.
|
||||
*
|
||||
* Only meaningful with `auth: 'kubernetes'`: the role has to live on the same
|
||||
* auth mount that validates this cluster's ServiceAccount tokens.
|
||||
*/
|
||||
async ensureServerIdentity(input: { name: string; namespace: string; secretNames: string[] }): Promise<void> {
|
||||
if (this.authStrategy !== 'kubernetes') {
|
||||
throw new Error(
|
||||
`OpenBao: per-server identities require auth: 'kubernetes' (this backend uses '${this.authStrategy}') — ` +
|
||||
'a token-auth backend has no ServiceAccount auth mount to bind a role on',
|
||||
);
|
||||
}
|
||||
const token = await this.getToken();
|
||||
const deps = { fetch: this.fetchImpl, ...(this.namespace !== undefined ? { namespace: this.namespace } : {}) };
|
||||
|
||||
// Policy first, then the role that references it. The reverse order would
|
||||
// briefly leave a role pointing at a non-existent policy, which OpenBao
|
||||
// resolves as "no capabilities" — a confusing transient 403 for the pod.
|
||||
await writePolicy(
|
||||
this.url,
|
||||
token,
|
||||
input.name,
|
||||
buildServerSecretPolicyHcl({ mount: this.mount, pathPrefix: this.pathPrefix, secretNames: input.secretNames }),
|
||||
deps,
|
||||
);
|
||||
await ensureKubernetesAuthRole(this.url, token, this.k8sAuthMount, input.name, {
|
||||
boundServiceAccountNames: [input.name],
|
||||
boundServiceAccountNamespaces: [input.namespace],
|
||||
tokenPolicies: [input.name],
|
||||
}, deps);
|
||||
}
|
||||
|
||||
/** Role before policy: revoke the ability to log in before the grants vanish. */
|
||||
async removeServerIdentity(input: { name: string }): Promise<void> {
|
||||
if (this.authStrategy !== 'kubernetes') return;
|
||||
const token = await this.getToken();
|
||||
const deps = { fetch: this.fetchImpl, ...(this.namespace !== undefined ? { namespace: this.namespace } : {}) };
|
||||
await deleteKubernetesAuthRole(this.url, token, this.k8sAuthMount, input.name, deps);
|
||||
await deletePolicy(this.url, token, input.name, deps);
|
||||
}
|
||||
|
||||
private pathFor(name: string): string {
|
||||
const safe = encodeURIComponent(name);
|
||||
return this.pathPrefix === '' ? safe : `${this.pathPrefix}/${safe}`;
|
||||
@@ -206,11 +332,28 @@ export class OpenBaoDriver implements SecretBackendDriver {
|
||||
const loginUrl = `${this.url}/v1/auth/${this.k8sAuthMount}/login`;
|
||||
const headers: Record<string, string> = { 'Content-Type': 'application/json' };
|
||||
if (this.namespace !== undefined) headers['X-Vault-Namespace'] = this.namespace;
|
||||
const res = await this.fetchImpl(loginUrl, {
|
||||
method: 'POST',
|
||||
headers,
|
||||
body: JSON.stringify({ role: this.k8sRole, jwt }),
|
||||
});
|
||||
// Bounded like every other call: a hung login is indistinguishable from a
|
||||
// hung read to the caller, and this one used to have no timeout at all.
|
||||
let res: Response;
|
||||
try {
|
||||
res = await this.fetchImpl(loginUrl, {
|
||||
method: 'POST',
|
||||
headers,
|
||||
body: JSON.stringify({ role: this.k8sRole, jwt }),
|
||||
signal: AbortSignal.timeout(this.timeoutMs),
|
||||
});
|
||||
} catch (err) {
|
||||
throw new SecretBackendUnavailableError(
|
||||
`OpenBao kubernetes login (role=${this.k8sRole!}): ${err instanceof Error ? err.message : String(err)}`,
|
||||
{ cause: err },
|
||||
);
|
||||
}
|
||||
if (RETRYABLE_STATUS.has(res.status)) {
|
||||
throw new SecretBackendUnavailableError(
|
||||
`OpenBao kubernetes login (role=${this.k8sRole!}): HTTP ${String(res.status)}`,
|
||||
{ lastStatus: res.status },
|
||||
);
|
||||
}
|
||||
if (!res.ok) {
|
||||
const text = await res.text().catch(() => '');
|
||||
throw new Error(`OpenBao kubernetes login (role=${this.k8sRole!}): HTTP ${String(res.status)} ${text}`);
|
||||
@@ -229,30 +372,77 @@ export class OpenBaoDriver implements SecretBackendDriver {
|
||||
return clientToken;
|
||||
}
|
||||
|
||||
private async request(method: string, path: string, body?: unknown): Promise<Response> {
|
||||
const token = await this.getToken();
|
||||
/** Build a fresh RequestInit — headers must not be shared across attempts. */
|
||||
private buildInit(method: string, token: string, body?: unknown): RequestInit {
|
||||
const headers: Record<string, string> = { 'X-Vault-Token': token };
|
||||
if (this.namespace !== undefined) headers['X-Vault-Namespace'] = this.namespace;
|
||||
if (body !== undefined) headers['Content-Type'] = 'application/json';
|
||||
|
||||
const init: RequestInit = { method, headers };
|
||||
const init: RequestInit = { method, headers, signal: AbortSignal.timeout(this.timeoutMs) };
|
||||
if (body !== undefined) init.body = JSON.stringify(body);
|
||||
return init;
|
||||
}
|
||||
|
||||
const res = await this.fetchImpl(`${this.url}${path}`, init);
|
||||
/** Full-jitter exponential backoff, so concurrent callers don't resonate. */
|
||||
private backoffFor(attempt: number): number {
|
||||
return Math.random() * this.backoffBaseMs * Math.pow(2, attempt - 1);
|
||||
}
|
||||
|
||||
// If the cached token expired between cache-check and request (k8s clock
|
||||
// skew, server-side revocation, etc.), purge cache and retry once.
|
||||
if (res.status === 403 && this.cachedToken !== undefined) {
|
||||
this.cachedToken = undefined;
|
||||
this.cachedTokenExpiresAt = 0;
|
||||
const fresh = await this.getToken();
|
||||
const retryHeaders: Record<string, string> = { 'X-Vault-Token': fresh };
|
||||
if (this.namespace !== undefined) retryHeaders['X-Vault-Namespace'] = this.namespace;
|
||||
if (body !== undefined) retryHeaders['Content-Type'] = 'application/json';
|
||||
const retryInit: RequestInit = { method, headers: retryHeaders };
|
||||
if (body !== undefined) retryInit.body = JSON.stringify(body);
|
||||
return this.fetchImpl(`${this.url}${path}`, retryInit);
|
||||
private async request(method: string, path: string, body?: unknown): Promise<Response> {
|
||||
const url = `${this.url}${path}`;
|
||||
let lastStatus: number | undefined;
|
||||
let lastErr: unknown;
|
||||
|
||||
for (let attempt = 1; attempt <= this.maxAttempts; attempt++) {
|
||||
let res: Response;
|
||||
try {
|
||||
const token = await this.getToken();
|
||||
res = await this.fetchImpl(url, this.buildInit(method, token, body));
|
||||
} catch (err) {
|
||||
// Network failure, DNS failure, or our own AbortSignal firing.
|
||||
lastErr = err;
|
||||
if (attempt < this.maxAttempts) {
|
||||
await this.sleep(this.backoffFor(attempt));
|
||||
continue;
|
||||
}
|
||||
throw new SecretBackendUnavailableError(
|
||||
`OpenBao ${method} ${path}: ${err instanceof Error ? err.message : String(err)} (after ${String(attempt)} attempt(s))`,
|
||||
{ cause: err },
|
||||
);
|
||||
}
|
||||
|
||||
// If the cached token expired between cache-check and request (k8s clock
|
||||
// skew, server-side revocation, etc.), purge cache and retry once. This
|
||||
// is deliberately OUTSIDE the retry budget: it is a credential refresh,
|
||||
// not a backend-unavailable condition, and it must stay single-shot so a
|
||||
// genuinely revoked grant fails loudly instead of looping.
|
||||
if (res.status === 403 && this.cachedToken !== undefined) {
|
||||
this.cachedToken = undefined;
|
||||
this.cachedTokenExpiresAt = 0;
|
||||
const fresh = await this.getToken();
|
||||
return this.fetchImpl(url, this.buildInit(method, fresh, body));
|
||||
}
|
||||
|
||||
// The backend is up but cannot answer right now — 503 is also what a
|
||||
// sealed OpenBao returns, which used to be an immediate hard failure.
|
||||
if (RETRYABLE_STATUS.has(res.status) && attempt < this.maxAttempts) {
|
||||
lastStatus = res.status;
|
||||
await this.sleep(this.backoffFor(attempt));
|
||||
continue;
|
||||
}
|
||||
if (RETRYABLE_STATUS.has(res.status)) {
|
||||
throw new SecretBackendUnavailableError(
|
||||
`OpenBao ${method} ${path}: HTTP ${String(res.status)} after ${String(attempt)} attempt(s)`,
|
||||
{ lastStatus: res.status },
|
||||
);
|
||||
}
|
||||
|
||||
return res;
|
||||
}
|
||||
return res;
|
||||
|
||||
/* c8 ignore next 5 -- unreachable: every loop exit above returns or throws */
|
||||
throw new SecretBackendUnavailableError(
|
||||
`OpenBao ${method} ${path}: exhausted ${String(this.maxAttempts)} attempt(s)`,
|
||||
lastErr !== undefined ? { cause: lastErr, ...(lastStatus !== undefined ? { lastStatus } : {}) } : (lastStatus !== undefined ? { lastStatus } : {}),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -46,8 +46,49 @@ export interface SecretBackendDriver {
|
||||
/** List everything the backend knows about. Used for migration + drift detection. */
|
||||
list(): Promise<Array<{ name: string; externalRef: ExternalRef }>>;
|
||||
|
||||
/** Optional: health probe. Used by `mcpctl describe secretbackend`. */
|
||||
/**
|
||||
* Optional LIVENESS probe: is the backend reachable at all?
|
||||
*
|
||||
* Must NOT require authentication — the whole point is to separate "the
|
||||
* backend is down/sealed" from "our credentials stopped working". Compare
|
||||
* `authCheck()`, which is the readiness half.
|
||||
*/
|
||||
healthCheck?(): Promise<{ ok: boolean; detail?: string }>;
|
||||
|
||||
/**
|
||||
* Optional: provision a per-server identity in the backend, so an MCP server
|
||||
* pod can fetch its OWN secrets directly (Agent Injector) without mcpd ever
|
||||
* materialising the value into the pod spec.
|
||||
*
|
||||
* Scoping is the entire point. Each server gets a policy naming only the
|
||||
* secrets it declares, bound to its own ServiceAccount. A single shared role
|
||||
* would let any opted-in pod — including third-party images we do not
|
||||
* control — read every secret under the prefix, which is a worse position
|
||||
* than leaving values in the pod spec.
|
||||
*
|
||||
* Idempotent: called on every reconcile, must converge rather than conflict.
|
||||
*/
|
||||
ensureServerIdentity?(input: {
|
||||
/** Identity name — also the policy name and the k8s ServiceAccount name. */
|
||||
name: string;
|
||||
/** Namespace the bound ServiceAccount lives in. */
|
||||
namespace: string;
|
||||
/** Secrets this server may read. Empty means "revoke everything". */
|
||||
secretNames: string[];
|
||||
}): Promise<void>;
|
||||
|
||||
/** Optional: tear down what `ensureServerIdentity` created. Idempotent. */
|
||||
removeServerIdentity?(input: { name: string }): Promise<void>;
|
||||
|
||||
/**
|
||||
* Optional READINESS probe: can we actually read through this backend with
|
||||
* the credentials we hold?
|
||||
*
|
||||
* A backend that answers `healthCheck()` but fails here is the exact shape of
|
||||
* the incident where a re-initialised OpenBao left mcpd holding valid-looking
|
||||
* tokens that granted nothing. Reporting one signal for both hides it.
|
||||
*/
|
||||
authCheck?(): Promise<{ ok: boolean; detail?: string }>;
|
||||
}
|
||||
|
||||
/** Stored config for a SecretBackend row; dispatched on `type`. */
|
||||
@@ -66,3 +107,40 @@ export interface BackendRow {
|
||||
export interface SecretRefResolver {
|
||||
resolve(secretName: string, key: string): Promise<string>;
|
||||
}
|
||||
|
||||
/**
|
||||
* The backend gave a definitive answer: this secret (or key) does not exist.
|
||||
*
|
||||
* Callers may treat this as final. The caching decorator EVICTS on this and
|
||||
* never serves a stale value for it — serving stale here would resurrect a
|
||||
* deliberately deleted or revoked credential, which is strictly worse than an
|
||||
* outage.
|
||||
*/
|
||||
export class SecretNotFoundError extends Error {
|
||||
constructor(message: string, options?: { cause?: unknown }) {
|
||||
super(message, options);
|
||||
this.name = 'SecretNotFoundError';
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The backend could not be reached or did not answer: DNS/TCP failure, request
|
||||
* timeout, or an exhausted retry budget against 5xx/429.
|
||||
*
|
||||
* This is the ONLY error the caching decorator will serve a stale value for.
|
||||
* The distinction has to be typed rather than string-matched: a mis-classified
|
||||
* "not found" would resurrect deleted secrets, and a mis-classified auth
|
||||
* failure would silently paper over a backend whose grants were revoked — the
|
||||
* failure mode that let an OpenBao re-init break every secret write for four
|
||||
* days (commit e51b924).
|
||||
*/
|
||||
export class SecretBackendUnavailableError extends Error {
|
||||
/** HTTP status of the last attempt, when the failure was an HTTP response. */
|
||||
readonly lastStatus: number | undefined;
|
||||
|
||||
constructor(message: string, options?: { cause?: unknown; lastStatus?: number }) {
|
||||
super(message, options?.cause !== undefined ? { cause: options.cause } : undefined);
|
||||
this.name = 'SecretBackendUnavailableError';
|
||||
this.lastStatus = options?.lastStatus;
|
||||
}
|
||||
}
|
||||
|
||||
142
src/mcpd/src/services/server-identity.service.ts
Normal file
142
src/mcpd/src/services/server-identity.service.ts
Normal file
@@ -0,0 +1,142 @@
|
||||
/**
|
||||
* Per-server OpenBao identities, so an MCP server pod can fetch its own secrets
|
||||
* without mcpd ever writing the value into the pod spec.
|
||||
*
|
||||
* ## Why per-server, and not one shared role
|
||||
*
|
||||
* The obvious implementation gives every injected pod one shared ServiceAccount
|
||||
* and one role granting `secret/data/mcpctl/*`. That would be a downgrade, not
|
||||
* an improvement: today's exposure is "4 credentials readable by anyone with
|
||||
* `get pod` in this namespace", and the shared-role version is "10 credentials
|
||||
* readable by any process inside a third-party MCP server image". We do not
|
||||
* control `gitea-mcp-server` or `ha-mcp`; they should not be able to read the
|
||||
* Grafana token.
|
||||
*
|
||||
* So each server gets:
|
||||
* - its own Kubernetes ServiceAccount `mcpctl-server-<server>`
|
||||
* - its own OpenBao policy `mcpctl-server-<server>` (names only
|
||||
* the secrets that server declares — no wildcards)
|
||||
* - its own OpenBao k8s auth role `mcpctl-server-<server>` (bound to
|
||||
* that one ServiceAccount in that one namespace)
|
||||
*
|
||||
* ## Convergence
|
||||
*
|
||||
* `ensureFor` is idempotent and is safe to call on every start. The generated
|
||||
* policy is sorted and deduped, so re-writing an unchanged server produces a
|
||||
* byte-identical body — an unstable policy would churn the OpenBao audit log
|
||||
* and make real changes invisible.
|
||||
*
|
||||
* Removing a secret from a server's env and restarting it narrows the policy on
|
||||
* the next reconcile. Removing the server entirely revokes the identity.
|
||||
*/
|
||||
import type { McpServer } from '@prisma/client';
|
||||
import type { SecretBackendService } from './secret-backend.service.js';
|
||||
import type { ServerEnvEntry } from '../validation/mcp-server.schema.js';
|
||||
|
||||
/** Minimal Kubernetes surface this needs — keeps the service testable. */
|
||||
export interface ServiceAccountPort {
|
||||
/** Namespace MCP server pods run in. */
|
||||
readonly namespace: string;
|
||||
/** Create the ServiceAccount if absent. Must tolerate "already exists". */
|
||||
ensure(name: string): Promise<void>;
|
||||
/** Delete it. Must tolerate "not found". */
|
||||
remove(name: string): Promise<void>;
|
||||
}
|
||||
|
||||
export interface ServerIdentityLog {
|
||||
info(obj: Record<string, unknown>, msg: string): void;
|
||||
warn(obj: Record<string, unknown>, msg: string): void;
|
||||
}
|
||||
|
||||
const NOOP_LOG: ServerIdentityLog = { info: () => undefined, warn: () => undefined };
|
||||
|
||||
/** Shared prefix. mcpd's OpenBao grant is confined to exactly this prefix. */
|
||||
export const IDENTITY_PREFIX = 'mcpctl-server-';
|
||||
|
||||
export class ServerIdentityService {
|
||||
private readonly log: ServerIdentityLog;
|
||||
|
||||
constructor(
|
||||
private readonly backends: SecretBackendService,
|
||||
private readonly serviceAccounts: ServiceAccountPort,
|
||||
log?: ServerIdentityLog,
|
||||
) {
|
||||
this.log = log ?? NOOP_LOG;
|
||||
}
|
||||
|
||||
/** Identity name for a server — also the SA, policy and role name. */
|
||||
identityNameFor(serverName: string): string {
|
||||
return `${IDENTITY_PREFIX}${serverName}`;
|
||||
}
|
||||
|
||||
/**
|
||||
* The distinct secrets a server declares. Inline `value` entries are not
|
||||
* secrets and must not widen the policy.
|
||||
*/
|
||||
secretNamesFor(server: Pick<McpServer, 'env'>): string[] {
|
||||
const entries = (server.env ?? []) as ServerEnvEntry[];
|
||||
const names = new Set<string>();
|
||||
for (const entry of entries) {
|
||||
const ref = entry.valueFrom?.secretRef;
|
||||
if (ref !== undefined) names.add(ref.name);
|
||||
}
|
||||
return [...names].sort((a, b) => a.localeCompare(b));
|
||||
}
|
||||
|
||||
/**
|
||||
* Converge the identity for one server. Returns the identity name so the
|
||||
* caller can stamp it onto the pod spec.
|
||||
*
|
||||
* Order matters: the ServiceAccount must exist before the role that binds it,
|
||||
* or the pod can start, fail to log in, and crashloop while the role is still
|
||||
* being written.
|
||||
*/
|
||||
async ensureFor(server: Pick<McpServer, 'name' | 'env'>): Promise<string> {
|
||||
const name = this.identityNameFor(server.name);
|
||||
const secretNames = this.secretNamesFor(server);
|
||||
|
||||
const backend = await this.backends.getDefault();
|
||||
const driver = this.backends.driverFor(backend);
|
||||
if (driver.ensureServerIdentity === undefined) {
|
||||
throw new Error(
|
||||
`secret backend '${backend.name}' (${backend.type}) cannot provision per-server identities — ` +
|
||||
'per-server secret delivery requires an openbao backend using kubernetes auth',
|
||||
);
|
||||
}
|
||||
|
||||
await this.serviceAccounts.ensure(name);
|
||||
await driver.ensureServerIdentity({
|
||||
name,
|
||||
namespace: this.serviceAccounts.namespace,
|
||||
secretNames,
|
||||
});
|
||||
|
||||
this.log.info(
|
||||
{ identity: name, namespace: this.serviceAccounts.namespace, secrets: secretNames },
|
||||
`provisioned scoped OpenBao identity for server '${server.name}' (${String(secretNames.length)} secret(s))`,
|
||||
);
|
||||
return name;
|
||||
}
|
||||
|
||||
/**
|
||||
* Revoke a server's identity. Best-effort per step: a half-removed identity
|
||||
* grants nothing useful, and failing the whole teardown because a
|
||||
* ServiceAccount was already gone would leave the OpenBao side orphaned.
|
||||
*/
|
||||
async removeFor(serverName: string): Promise<void> {
|
||||
const name = this.identityNameFor(serverName);
|
||||
const backend = await this.backends.getDefault();
|
||||
const driver = this.backends.driverFor(backend);
|
||||
|
||||
try {
|
||||
await driver.removeServerIdentity?.({ name });
|
||||
} catch (err) {
|
||||
this.log.warn({ identity: name, err: String(err) }, `could not remove OpenBao identity '${name}'`);
|
||||
}
|
||||
try {
|
||||
await this.serviceAccounts.remove(name);
|
||||
} catch (err) {
|
||||
this.log.warn({ identity: name, err: String(err) }, `could not remove ServiceAccount '${name}'`);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -121,15 +121,15 @@ export class PersistentStdioClient {
|
||||
this.processBuffer();
|
||||
});
|
||||
|
||||
exec.stdout.on('end', () => {
|
||||
this.initialized = false;
|
||||
this.exec = null;
|
||||
for (const [, pending] of this.pendingRequests) {
|
||||
clearTimeout(pending.timer);
|
||||
pending.reject(new Error('STDIO process exited'));
|
||||
}
|
||||
this.pendingRequests.clear();
|
||||
});
|
||||
// All three events funnel into the same teardown: 'end' is the graceful
|
||||
// path, but an abnormal websocket death may only surface as 'close' or
|
||||
// 'error' on the stream — before this, isConnected stayed true and every
|
||||
// request rode out the full timeout against a dead pipe (mcpctl#114).
|
||||
exec.stdout.on('end', () => this.teardown(exec, 'STDIO process exited'));
|
||||
exec.stdout.on('close', () => this.teardown(exec, 'STDIO stream closed'));
|
||||
exec.stdout.on('error', (err: Error) =>
|
||||
this.teardown(exec, `STDIO stream error: ${err.message}`),
|
||||
);
|
||||
|
||||
// Run MCP init handshake
|
||||
const initId = this.nextId++;
|
||||
@@ -174,6 +174,23 @@ export class PersistentStdioClient {
|
||||
this.initialized = true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Tear down a dead session: reject in-flight requests and mark the client
|
||||
* disconnected so the next send() redials via ensureReady(). The identity
|
||||
* guard is load-bearing: after a reconnect, a LATE 'close'/'end' from the
|
||||
* previous session's stream must not clobber the new session.
|
||||
*/
|
||||
private teardown(exec: InteractiveExec, reason: string): void {
|
||||
if (this.exec !== exec) return;
|
||||
this.initialized = false;
|
||||
this.exec = null;
|
||||
for (const [, pending] of this.pendingRequests) {
|
||||
clearTimeout(pending.timer);
|
||||
pending.reject(new Error(reason));
|
||||
}
|
||||
this.pendingRequests.clear();
|
||||
}
|
||||
|
||||
private write(msg: Record<string, unknown>): void {
|
||||
if (!this.exec) throw new Error('Not connected');
|
||||
this.exec.write(JSON.stringify(msg) + '\n');
|
||||
|
||||
@@ -511,3 +511,130 @@ describe('InstanceService', () => {
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// ── syncStatus (mcpctl#114) — first-ever coverage of this method ──
|
||||
|
||||
describe('syncStatus', () => {
|
||||
let instanceRepo: IMcpInstanceRepository;
|
||||
let serverRepo: IMcpServerRepository;
|
||||
let orchestrator: McpOrchestrator;
|
||||
let service: InstanceService;
|
||||
let invalidator: ReturnType<typeof vi.fn>;
|
||||
|
||||
function inspect(info: Partial<{ state: string; restartCount: number }>) {
|
||||
vi.mocked(orchestrator.inspectContainer).mockResolvedValue({
|
||||
containerId: 'ctr-abc',
|
||||
name: 'test',
|
||||
state: (info.state ?? 'running') as 'running',
|
||||
createdAt: new Date(),
|
||||
...(info.restartCount !== undefined ? { restartCount: info.restartCount } : {}),
|
||||
});
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
instanceRepo = mockInstanceRepo();
|
||||
serverRepo = mockServerRepo();
|
||||
orchestrator = mockOrchestrator();
|
||||
service = new InstanceService(instanceRepo, serverRepo, orchestrator);
|
||||
invalidator = vi.fn();
|
||||
service.setStdioInvalidator(invalidator);
|
||||
});
|
||||
|
||||
it('marks a stopped container ERROR and MERGES retry metadata instead of clobbering it', async () => {
|
||||
const future = new Date(Date.now() + 60_000).toISOString();
|
||||
vi.mocked(instanceRepo.findAll).mockResolvedValue([
|
||||
makeInstance({ status: 'RUNNING', metadata: { attemptCount: 3, nextRetryAt: future } }),
|
||||
]);
|
||||
inspect({ state: 'stopped' });
|
||||
|
||||
await service.syncStatus();
|
||||
|
||||
const [, status, fields] = vi.mocked(instanceRepo.updateStatus).mock.calls[0]!;
|
||||
expect(status).toBe('ERROR');
|
||||
const meta = fields!.metadata as Record<string, unknown>;
|
||||
expect(meta['error']).toBe('log output'); // last log line
|
||||
expect(meta['attemptCount']).toBe(3); // preserved — was clobbered before
|
||||
expect(meta['nextRetryAt']).toBe(future); // preserved — hot-loop killer
|
||||
expect(invalidator).toHaveBeenCalledWith('ctr-abc');
|
||||
});
|
||||
|
||||
it('recovers an ERROR instance whose pod is running again (the stuck state)', async () => {
|
||||
vi.mocked(instanceRepo.findAll).mockResolvedValue([
|
||||
makeInstance({
|
||||
status: 'ERROR',
|
||||
metadata: { error: 'Container stopped', attemptCount: 4, nextRetryAt: 'x', lastAttemptAt: 'y' },
|
||||
}),
|
||||
]);
|
||||
inspect({ state: 'running', restartCount: 2 });
|
||||
|
||||
await service.syncStatus();
|
||||
|
||||
const [, status, fields] = vi.mocked(instanceRepo.updateStatus).mock.calls[0]!;
|
||||
expect(status).toBe('RUNNING');
|
||||
const meta = fields!.metadata as Record<string, unknown>;
|
||||
expect(meta['error']).toBeUndefined();
|
||||
expect(meta['attemptCount']).toBeUndefined();
|
||||
expect(meta['nextRetryAt']).toBeUndefined();
|
||||
expect(meta['lastRestartCount']).toBe(2);
|
||||
expect(invalidator).toHaveBeenCalledWith('ctr-abc');
|
||||
});
|
||||
|
||||
it('leaves an ERROR instance alone when its pod is genuinely gone', async () => {
|
||||
vi.mocked(instanceRepo.findAll).mockResolvedValue([
|
||||
makeInstance({ status: 'ERROR', metadata: { error: 'x' } }),
|
||||
]);
|
||||
vi.mocked(orchestrator.inspectContainer).mockRejectedValue(new Error('not found'));
|
||||
|
||||
await service.syncStatus();
|
||||
|
||||
expect(instanceRepo.updateStatus).not.toHaveBeenCalled();
|
||||
expect(invalidator).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('detects an in-place restart via restartCount and invalidates the stdio pipe', async () => {
|
||||
vi.mocked(instanceRepo.findAll).mockResolvedValue([
|
||||
makeInstance({ status: 'RUNNING', metadata: { lastRestartCount: 1 } }),
|
||||
]);
|
||||
inspect({ state: 'running', restartCount: 2 });
|
||||
|
||||
await service.syncStatus();
|
||||
|
||||
const [, status, fields] = vi.mocked(instanceRepo.updateStatus).mock.calls[0]!;
|
||||
expect(status).toBe('RUNNING');
|
||||
expect((fields!.metadata as Record<string, unknown>)['lastRestartCount']).toBe(2);
|
||||
expect(invalidator).toHaveBeenCalledWith('ctr-abc');
|
||||
});
|
||||
|
||||
it('baselines a first-seen restartCount without invalidating', async () => {
|
||||
vi.mocked(instanceRepo.findAll).mockResolvedValue([
|
||||
makeInstance({ status: 'RUNNING', metadata: {} }),
|
||||
]);
|
||||
inspect({ state: 'running', restartCount: 0 });
|
||||
|
||||
await service.syncStatus();
|
||||
|
||||
const [, , fields] = vi.mocked(instanceRepo.updateStatus).mock.calls[0]!;
|
||||
expect((fields!.metadata as Record<string, unknown>)['lastRestartCount']).toBe(0);
|
||||
expect(invalidator).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('does nothing for a steady RUNNING instance with an unchanged restartCount', async () => {
|
||||
vi.mocked(instanceRepo.findAll).mockResolvedValue([
|
||||
makeInstance({ status: 'RUNNING', metadata: { lastRestartCount: 2 } }),
|
||||
]);
|
||||
inspect({ state: 'running', restartCount: 2 });
|
||||
|
||||
await service.syncStatus();
|
||||
|
||||
expect(instanceRepo.updateStatus).not.toHaveBeenCalled();
|
||||
expect(invalidator).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('remove() evicts the cached stdio client', async () => {
|
||||
vi.mocked(instanceRepo.findById).mockResolvedValue(makeInstance({ status: 'RUNNING' }));
|
||||
|
||||
await service.remove('inst-1');
|
||||
|
||||
expect(invalidator).toHaveBeenCalledWith('ctr-abc');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -63,12 +63,22 @@ vi.mock('@kubernetes/client-node', () => {
|
||||
makeApiClient = vi.fn(() => mockCore);
|
||||
}
|
||||
|
||||
// Track the live Exec/Attach instances so tests can program their vi.fn()s
|
||||
// (each K8sOfficialClient constructs fresh ones).
|
||||
const instances: { exec?: { exec: ReturnType<typeof vi.fn> }; attach?: { attach: ReturnType<typeof vi.fn> } } = {};
|
||||
|
||||
class MockExec {
|
||||
exec = vi.fn();
|
||||
constructor() {
|
||||
instances.exec = this;
|
||||
}
|
||||
}
|
||||
|
||||
class MockAttach {
|
||||
attach = vi.fn();
|
||||
constructor() {
|
||||
instances.attach = this;
|
||||
}
|
||||
}
|
||||
|
||||
class MockLog {
|
||||
@@ -82,7 +92,7 @@ vi.mock('@kubernetes/client-node', () => {
|
||||
Attach: MockAttach,
|
||||
Log: MockLog,
|
||||
// Export test helpers
|
||||
__testHelpers: { setHandler, getHandler, clearHandlers, mockCore },
|
||||
__testHelpers: { setHandler, getHandler, clearHandlers, mockCore, instances },
|
||||
};
|
||||
});
|
||||
|
||||
@@ -379,3 +389,134 @@ describe('httpStatusOf', () => {
|
||||
expect(httpStatusOf({ code: 'ECONNREFUSED' })).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
// ── mcpctl#114: interactive-session death signals, restartCount, 409-adopt ──
|
||||
|
||||
import { EventEmitter } from 'node:events';
|
||||
|
||||
function makeFakeWs(): EventEmitter & { close: ReturnType<typeof vi.fn> } {
|
||||
const ws = new EventEmitter() as EventEmitter & { close: ReturnType<typeof vi.fn> };
|
||||
ws.close = vi.fn();
|
||||
return ws;
|
||||
}
|
||||
|
||||
const podRestarted = {
|
||||
...podRunning,
|
||||
status: {
|
||||
...podRunning.status,
|
||||
containerStatuses: [{
|
||||
state: { running: { startedAt: '2026-01-02T00:00:00Z' } },
|
||||
restartCount: 3,
|
||||
lastState: { terminated: { reason: 'OOMKilled', exitCode: 137 } },
|
||||
}],
|
||||
},
|
||||
};
|
||||
|
||||
const podTerminated = {
|
||||
...podRunning,
|
||||
status: {
|
||||
phase: 'Failed',
|
||||
containerStatuses: [{
|
||||
state: { terminated: { reason: 'Error', exitCode: 1 } },
|
||||
restartCount: 1,
|
||||
}],
|
||||
},
|
||||
};
|
||||
|
||||
describe('KubernetesOrchestrator interactive sessions (mcpctl#114)', () => {
|
||||
let orch: KubernetesOrchestrator;
|
||||
|
||||
beforeEach(() => {
|
||||
clearHandlers();
|
||||
vi.clearAllMocks();
|
||||
orch = new KubernetesOrchestrator({ serversNamespace: 'mcpctl-servers' });
|
||||
});
|
||||
|
||||
it("execInteractive: ws 'close' ends stdout so the consumer sees the death", async () => {
|
||||
setHandler('readNamespacedPod:my-server', podRunning);
|
||||
const ws = makeFakeWs();
|
||||
k8sMock.__testHelpers.instances.exec!.exec.mockResolvedValue(ws);
|
||||
|
||||
const iexec = await orch.execInteractive!('my-server', ['node', 'index.js']);
|
||||
iexec.stdout.resume(); // PassThrough only emits 'end' once it is being read
|
||||
const ended = new Promise<void>((resolve) => iexec.stdout.on('end', resolve));
|
||||
ws.emit('close');
|
||||
await expect(ended).resolves.toBeUndefined();
|
||||
});
|
||||
|
||||
it("execInteractive: ws 'error' also ends stdout, and close() closes the ws", async () => {
|
||||
setHandler('readNamespacedPod:my-server', podRunning);
|
||||
const ws = makeFakeWs();
|
||||
k8sMock.__testHelpers.instances.exec!.exec.mockResolvedValue(ws);
|
||||
|
||||
const iexec = await orch.execInteractive!('my-server', ['node']);
|
||||
iexec.stdout.resume();
|
||||
const ended = new Promise<void>((resolve) => iexec.stdout.on('end', resolve));
|
||||
ws.emit('error', new Error('abnormal closure'));
|
||||
await expect(ended).resolves.toBeUndefined();
|
||||
|
||||
iexec.close();
|
||||
expect(ws.close).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("attachInteractive: ws 'close' ends stdout (first-ever attach coverage)", async () => {
|
||||
setHandler('readNamespacedPod:my-server', podRunning);
|
||||
const ws = makeFakeWs();
|
||||
k8sMock.__testHelpers.instances.attach!.attach.mockResolvedValue(ws);
|
||||
|
||||
const iexec = await orch.attachInteractive!('my-server');
|
||||
iexec.stdout.resume();
|
||||
const ended = new Promise<void>((resolve) => iexec.stdout.on('end', resolve));
|
||||
ws.emit('close');
|
||||
await expect(ended).resolves.toBeUndefined();
|
||||
expect(k8sMock.__testHelpers.instances.attach!.attach).toHaveBeenCalledOnce();
|
||||
});
|
||||
|
||||
it('inspectContainer surfaces restartCount and startedAt', async () => {
|
||||
setHandler('readNamespacedPod:my-server', podRestarted);
|
||||
const info = await orch.inspectContainer('my-server');
|
||||
expect(info.state).toBe('running');
|
||||
expect(info.restartCount).toBe(3);
|
||||
expect(info.startedAt).toEqual(new Date('2026-01-02T00:00:00Z'));
|
||||
});
|
||||
|
||||
it('createContainer ADOPTS an alive pod on 409 instead of throwing', async () => {
|
||||
setHandler('readNamespace:mcpctl-servers', {});
|
||||
setHandler('createNamespacedPod', undefined, { code: 409 });
|
||||
setHandler('readNamespacedPod:my-server', podRestarted);
|
||||
|
||||
const info = await orch.createContainer(testSpec);
|
||||
expect(info.containerId).toBe('my-server');
|
||||
expect(info.state).toBe('running');
|
||||
expect(mockCore.deleteNamespacedPod).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('createContainer replaces a genuinely dead pod on 409', async () => {
|
||||
setHandler('readNamespace:mcpctl-servers', {});
|
||||
setHandler('createNamespacedPod', undefined, { code: 409 });
|
||||
setHandler('readNamespacedPod:my-server', podTerminated);
|
||||
setHandler('deleteNamespacedPod:my-server', {});
|
||||
|
||||
const promise = orch.createContainer(testSpec);
|
||||
// Give it a tick to inspect + delete, then flip the world: pod is gone,
|
||||
// create succeeds.
|
||||
await new Promise((r) => setTimeout(r, 50));
|
||||
// The gone-wait loop polls once per second: serve 404 so it breaks, then
|
||||
// restore the pod before the post-create inspect (~500ms later) runs.
|
||||
setHandler('readNamespacedPod:my-server', undefined, { code: 404 });
|
||||
setHandler('createNamespacedPod', podRunning);
|
||||
setTimeout(() => setHandler('readNamespacedPod:my-server', podRunning), 1200);
|
||||
|
||||
const info = await promise;
|
||||
expect(info.state).toBe('running');
|
||||
expect(mockCore.deleteNamespacedPod).toHaveBeenCalled();
|
||||
expect(mockCore.createNamespacedPod).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it('createContainer still throws on non-409 errors', async () => {
|
||||
setHandler('readNamespace:mcpctl-servers', {});
|
||||
setHandler('createNamespacedPod', undefined, { code: 500 });
|
||||
await expect(orch.createContainer(testSpec)).rejects.toBeTruthy();
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -725,3 +725,103 @@ describe('MCP server full flow', () => {
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// ── mcpctl#114: attach server survives an in-place container restart ──
|
||||
|
||||
import { PassThrough } from 'node:stream';
|
||||
import type { InteractiveExec } from '../src/services/orchestrator.js';
|
||||
|
||||
/**
|
||||
* A scripted MCP server on the other end of an attach session: answers
|
||||
* initialize and every request with a result carrying its session number,
|
||||
* so the test can prove which session served which call.
|
||||
*/
|
||||
function makeScriptedAttach(session: number): InteractiveExec {
|
||||
const stdout = new PassThrough();
|
||||
return {
|
||||
stdout,
|
||||
write(data: string) {
|
||||
for (const line of data.split('\n')) {
|
||||
if (!line.trim()) continue;
|
||||
const msg = JSON.parse(line) as { id?: number; method?: string };
|
||||
if (msg.id === undefined) continue; // notifications
|
||||
const result = msg.method === 'initialize'
|
||||
? { capabilities: {} }
|
||||
: { ok: true, session };
|
||||
stdout.write(JSON.stringify({ jsonrpc: '2.0', id: msg.id, result }) + '\n');
|
||||
}
|
||||
},
|
||||
close() {
|
||||
stdout.destroy();
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
describe('attach-mode STDIO restart recovery (mcpctl#114)', () => {
|
||||
it('survives an in-place container restart: syncStatus invalidates, next call rides a new session', async () => {
|
||||
const serverRepo = createInMemoryServerRepo();
|
||||
const instanceRepo = createInMemoryInstanceRepo();
|
||||
|
||||
const server = await serverRepo.create({
|
||||
name: 'gitea',
|
||||
transport: 'STDIO',
|
||||
dockerImage: 'ghcr.io/gitea-mcp:latest',
|
||||
replicas: 1,
|
||||
} as never);
|
||||
const instance = await instanceRepo.create({
|
||||
serverId: server.id,
|
||||
containerId: 'pod-gitea',
|
||||
status: 'RUNNING',
|
||||
metadata: { lastRestartCount: 0 },
|
||||
} as never);
|
||||
|
||||
let restartCount = 0;
|
||||
const sessions: InteractiveExec[] = [];
|
||||
const orchestrator = {
|
||||
ping: vi.fn(async () => true),
|
||||
pullImage: vi.fn(async () => {}),
|
||||
createContainer: vi.fn(),
|
||||
stopContainer: vi.fn(async () => {}),
|
||||
removeContainer: vi.fn(async () => {}),
|
||||
inspectContainer: vi.fn(async () => ({
|
||||
containerId: 'pod-gitea',
|
||||
name: 'pod-gitea',
|
||||
state: 'running' as const,
|
||||
createdAt: new Date(),
|
||||
restartCount,
|
||||
})),
|
||||
getContainerLogs: vi.fn(async () => ({ stdout: '', stderr: '' })),
|
||||
execInContainer: vi.fn(),
|
||||
attachInteractive: vi.fn(async () => {
|
||||
const s = makeScriptedAttach(sessions.length + 1);
|
||||
sessions.push(s);
|
||||
return s;
|
||||
}),
|
||||
} as unknown as McpOrchestrator;
|
||||
|
||||
const proxyService = new McpProxyService(instanceRepo, serverRepo, orchestrator);
|
||||
const instanceService = new InstanceService(instanceRepo, serverRepo, orchestrator);
|
||||
instanceService.setStdioInvalidator((cid) => proxyService.removeClient(cid));
|
||||
|
||||
// 1. A call round-trips on session 1.
|
||||
const first = await proxyService.execute({ serverId: server.id, method: 'tools/list' });
|
||||
expect((first.result as { session: number }).session).toBe(1);
|
||||
|
||||
// 2. The container crashes and kubelet restarts it in place: same pod
|
||||
// name, restartCount bumps, the old session's pipe dies.
|
||||
sessions[0]!.stdout.end();
|
||||
restartCount = 1;
|
||||
|
||||
// 3. One reconcile tick detects the bump, keeps the row RUNNING, and
|
||||
// evicts the stale client.
|
||||
await instanceService.syncStatus();
|
||||
const after = await instanceRepo.findById(instance.id);
|
||||
expect(after!.status).toBe('RUNNING');
|
||||
expect((after!.metadata as { lastRestartCount: number }).lastRestartCount).toBe(1);
|
||||
|
||||
// 4. The next call succeeds first-try on a fresh attach session.
|
||||
const second = await proxyService.execute({ serverId: server.id, method: 'tools/list' });
|
||||
expect((second.result as { session: number }).session).toBe(2);
|
||||
expect(orchestrator.attachInteractive).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -109,3 +109,110 @@ describe('PersistentStdioClient', () => {
|
||||
await expect(client.send('tools/list')).rejects.toThrow(/interactive exec/i);
|
||||
});
|
||||
});
|
||||
|
||||
// Drive a fake session through the init handshake so send() can round-trip.
|
||||
async function completeHandshake(fake: ReturnType<typeof makeFakeExec>): Promise<void> {
|
||||
await new Promise((r) => setTimeout(r, 10));
|
||||
const init = JSON.parse(fake.written[0]!);
|
||||
fake.emit({ jsonrpc: '2.0', id: init.id, result: { capabilities: {} } });
|
||||
await new Promise((r) => setTimeout(r, 150));
|
||||
}
|
||||
|
||||
describe('PersistentStdioClient teardown & reconnect (mcpctl#114)', () => {
|
||||
it("stdout 'end' rejects in-flight requests and the next send() redials", async () => {
|
||||
const fakes = [makeFakeExec(), makeFakeExec()];
|
||||
let call = 0;
|
||||
const execInteractive = vi.fn(async () => fakes[call++]!.iexec);
|
||||
const client = new PersistentStdioClient(
|
||||
makeOrchestrator({ execInteractive }),
|
||||
'c1',
|
||||
{ kind: 'exec', command: ['node'] },
|
||||
);
|
||||
|
||||
const first = client.send('tools/list');
|
||||
await completeHandshake(fakes[0]!);
|
||||
// Kill the session while tools/list is in flight.
|
||||
fakes[0]!.iexec.stdout.end();
|
||||
await expect(first).rejects.toThrow('STDIO process exited');
|
||||
expect(client.isConnected).toBe(false);
|
||||
|
||||
// Next send must redial through a fresh interactive session.
|
||||
const second = client.send('tools/list');
|
||||
await completeHandshake(fakes[1]!);
|
||||
const req = JSON.parse(fakes[1]!.written[2]!);
|
||||
fakes[1]!.emit({ jsonrpc: '2.0', id: req.id, result: { tools: [] } });
|
||||
const res = await second;
|
||||
expect(res.result).toEqual({ tools: [] });
|
||||
expect(execInteractive).toHaveBeenCalledTimes(2);
|
||||
client.close();
|
||||
});
|
||||
|
||||
it("stdout 'error' tears down with the error message", async () => {
|
||||
const fake = makeFakeExec();
|
||||
const client = new PersistentStdioClient(
|
||||
makeOrchestrator({ execInteractive: vi.fn(async () => fake.iexec) }),
|
||||
'c1',
|
||||
{ kind: 'exec', command: ['node'] },
|
||||
);
|
||||
const pending = client.send('tools/list');
|
||||
await completeHandshake(fake);
|
||||
fake.iexec.stdout.emit('error', new Error('socket hang up'));
|
||||
await expect(pending).rejects.toThrow('STDIO stream error: socket hang up');
|
||||
expect(client.isConnected).toBe(false);
|
||||
client.close();
|
||||
});
|
||||
|
||||
it("a LATE 'end' from the previous session does not clobber the reconnected one", async () => {
|
||||
const fakes = [makeFakeExec(), makeFakeExec()];
|
||||
let call = 0;
|
||||
const client = new PersistentStdioClient(
|
||||
makeOrchestrator({ attachInteractive: vi.fn(async () => fakes[call++]!.iexec) }),
|
||||
'c1',
|
||||
{ kind: 'attach' },
|
||||
);
|
||||
|
||||
const first = client.send('tools/list');
|
||||
await completeHandshake(fakes[0]!);
|
||||
fakes[0]!.iexec.stdout.end();
|
||||
await expect(first).rejects.toThrow('STDIO process exited');
|
||||
|
||||
// Reconnect on a fresh session.
|
||||
const second = client.send('tools/list');
|
||||
await completeHandshake(fakes[1]!);
|
||||
|
||||
// The old session's stream fires a stale 'close' AFTER the reconnect —
|
||||
// the identity guard must ignore it.
|
||||
fakes[0]!.iexec.stdout.emit('close');
|
||||
expect(client.isConnected).toBe(true);
|
||||
|
||||
const req = JSON.parse(fakes[1]!.written[2]!);
|
||||
fakes[1]!.emit({ jsonrpc: '2.0', id: req.id, result: { tools: [{ name: 't' }] } });
|
||||
const res = await second;
|
||||
expect((res.result as { tools: unknown[] }).tools).toHaveLength(1);
|
||||
client.close();
|
||||
});
|
||||
|
||||
it('attach mode also redials after teardown', async () => {
|
||||
const fakes = [makeFakeExec(), makeFakeExec()];
|
||||
let call = 0;
|
||||
const attachInteractive = vi.fn(async () => fakes[call++]!.iexec);
|
||||
const client = new PersistentStdioClient(
|
||||
makeOrchestrator({ attachInteractive }),
|
||||
'c-gitea',
|
||||
{ kind: 'attach' },
|
||||
);
|
||||
|
||||
const first = client.send('tools/list');
|
||||
await completeHandshake(fakes[0]!);
|
||||
fakes[0]!.iexec.stdout.emit('close');
|
||||
await expect(first).rejects.toThrow('STDIO stream closed');
|
||||
|
||||
const second = client.send('tools/list');
|
||||
await completeHandshake(fakes[1]!);
|
||||
const req = JSON.parse(fakes[1]!.written[2]!);
|
||||
fakes[1]!.emit({ jsonrpc: '2.0', id: req.id, result: { tools: [] } });
|
||||
await expect(second).resolves.toMatchObject({ result: { tools: [] } });
|
||||
expect(attachInteractive).toHaveBeenCalledTimes(2);
|
||||
client.close();
|
||||
});
|
||||
});
|
||||
|
||||
98
src/mcpd/tests/secret-backend-health-route.test.ts
Normal file
98
src/mcpd/tests/secret-backend-health-route.test.ts
Normal file
@@ -0,0 +1,98 @@
|
||||
import { describe, it, expect, vi, afterEach } from 'vitest';
|
||||
import Fastify from 'fastify';
|
||||
import type { FastifyInstance } from 'fastify';
|
||||
import type { SecretBackend } from '@prisma/client';
|
||||
import { registerSecretBackendHealthRoutes } from '../src/routes/secret-backend-health.js';
|
||||
import { SecretBackendService } from '../src/services/secret-backend.service.js';
|
||||
import type { ISecretBackendRepository } from '../src/repositories/secret-backend.repository.js';
|
||||
import type { SecretBackendDriver } from '../src/services/secret-backends/types.js';
|
||||
|
||||
let app: FastifyInstance;
|
||||
afterEach(async () => { await app?.close(); });
|
||||
|
||||
function backendRow(overrides: Partial<SecretBackend> = {}): SecretBackend {
|
||||
return {
|
||||
id: 'b1', name: 'bao-k8s', type: 'openbao',
|
||||
config: { url: 'http://bao.example:8200', auth: 'kubernetes', role: 'mcpctl' },
|
||||
isDefault: true, description: '', version: 1,
|
||||
createdAt: new Date(), updatedAt: new Date(),
|
||||
...overrides,
|
||||
} as SecretBackend;
|
||||
}
|
||||
|
||||
/**
|
||||
* Build the route over a service whose driver is stubbed. We override
|
||||
* `driverFor` rather than the factory so the test drives the two probes
|
||||
* directly — the point here is the route's reporting, not driver internals.
|
||||
*/
|
||||
async function buildApp(
|
||||
probes: Pick<SecretBackendDriver, 'healthCheck' | 'authCheck'>,
|
||||
row: SecretBackend = backendRow(),
|
||||
): Promise<FastifyInstance> {
|
||||
const repo = {
|
||||
findById: vi.fn(async (id: string) => (id === row.id ? row : null)),
|
||||
} as unknown as ISecretBackendRepository;
|
||||
const svc = new SecretBackendService(repo, {
|
||||
plaintext: { listAllPlaintext: async () => [] },
|
||||
secretRefResolver: { resolve: async () => 'tok' },
|
||||
});
|
||||
vi.spyOn(svc, 'driverFor').mockReturnValue({ kind: 'openbao', ...probes } as SecretBackendDriver);
|
||||
vi.spyOn(svc, 'cacheStatsFor').mockReturnValue({ entries: 3, servingStale: 0, oldestStaleSince: undefined });
|
||||
|
||||
const a = Fastify();
|
||||
registerSecretBackendHealthRoutes(a, svc);
|
||||
await a.ready();
|
||||
return a;
|
||||
}
|
||||
|
||||
describe('GET /api/v1/secretbackends/:id/health', () => {
|
||||
it('reports live+ready when the backend is fully working', async () => {
|
||||
app = await buildApp({
|
||||
healthCheck: async () => ({ ok: true, detail: 'active' }),
|
||||
authCheck: async () => ({ ok: true, detail: 'readable at secret/mcpctl' }),
|
||||
});
|
||||
const res = await app.inject({ method: 'GET', url: '/api/v1/secretbackends/b1/health' });
|
||||
expect(res.statusCode).toBe(200);
|
||||
expect(res.json()).toMatchObject({ backend: 'bao-k8s', live: true, ready: true });
|
||||
});
|
||||
|
||||
it('reports NOT live when OpenBao is sealed — regardless of rotation state', async () => {
|
||||
// The bug this endpoint exists for: a kubernetes-auth backend never writes
|
||||
// tokenMeta.lastRotationError, so the old status line stayed green here.
|
||||
app = await buildApp({
|
||||
healthCheck: async () => ({ ok: false, detail: 'sealed' }),
|
||||
authCheck: async () => ({ ok: true, detail: 'should not be consulted' }),
|
||||
});
|
||||
const body = (await app.inject({ method: 'GET', url: '/api/v1/secretbackends/b1/health' })).json();
|
||||
expect(body.live).toBe(false);
|
||||
expect(body.liveDetail).toBe('sealed');
|
||||
expect(body.ready).toBe(false);
|
||||
expect(body.readyDetail).toMatch(/not probed/);
|
||||
expect(body.rotation.lastRotationError).toBeNull();
|
||||
});
|
||||
|
||||
it('distinguishes reachable-but-unusable (revoked grants) from unreachable', async () => {
|
||||
app = await buildApp({
|
||||
healthCheck: async () => ({ ok: true, detail: 'active' }),
|
||||
authCheck: async () => ({ ok: false, detail: 'OpenBao list: HTTP 403 permission denied' }),
|
||||
});
|
||||
const body = (await app.inject({ method: 'GET', url: '/api/v1/secretbackends/b1/health' })).json();
|
||||
expect(body).toMatchObject({ live: true, ready: false });
|
||||
expect(body.readyDetail).toMatch(/403/);
|
||||
});
|
||||
|
||||
it('surfaces cache state so degraded serving is visible', async () => {
|
||||
app = await buildApp({
|
||||
healthCheck: async () => ({ ok: true }),
|
||||
authCheck: async () => ({ ok: true }),
|
||||
});
|
||||
const body = (await app.inject({ method: 'GET', url: '/api/v1/secretbackends/b1/health' })).json();
|
||||
expect(body.cache).toMatchObject({ entries: 3, servingStale: 0 });
|
||||
});
|
||||
|
||||
it('404s for an unknown backend', async () => {
|
||||
app = await buildApp({ healthCheck: async () => ({ ok: true }), authCheck: async () => ({ ok: true }) });
|
||||
const res = await app.inject({ method: 'GET', url: '/api/v1/secretbackends/nope/health' });
|
||||
expect(res.statusCode).toBe(404);
|
||||
});
|
||||
});
|
||||
155
src/mcpd/tests/secret-backend-rotator-loop.test.ts
Normal file
155
src/mcpd/tests/secret-backend-rotator-loop.test.ts
Normal file
@@ -0,0 +1,155 @@
|
||||
/**
|
||||
* SecretBackendRotatorLoop had no coverage at all, despite being the boot-time
|
||||
* detector added after an upstream OpenBao re-init silently broke every secret
|
||||
* write for four days (e51b924). These pin the behaviours that matter when that
|
||||
* recurs: the boot health check fires, it reports through the injected logger
|
||||
* (so `mcpctl errors` sees it), and stop() genuinely stops.
|
||||
*/
|
||||
import { describe, it, expect, vi } from 'vitest';
|
||||
import type { SecretBackend } from '@prisma/client';
|
||||
import { SecretBackendRotatorLoop } from '../src/services/secret-backend-rotator-loop.js';
|
||||
import type { SecretBackendService } from '../src/services/secret-backend.service.js';
|
||||
import type { SecretBackendRotator } from '../src/services/secret-backend-rotator.service.js';
|
||||
|
||||
function backend(overrides: Partial<SecretBackend> = {}): SecretBackend {
|
||||
return {
|
||||
id: 'b1', name: 'bao', type: 'openbao',
|
||||
config: { url: 'http://bao.example:8200', rotation: { enabled: true, tokenRole: 'r', intervalHours: 24 } },
|
||||
isDefault: true, description: '', version: 1,
|
||||
createdAt: new Date(), updatedAt: new Date(),
|
||||
...overrides,
|
||||
} as SecretBackend;
|
||||
}
|
||||
|
||||
interface Harness {
|
||||
loop: SecretBackendRotatorLoop;
|
||||
rotator: { isRotatable: ReturnType<typeof vi.fn>; isOverdue: ReturnType<typeof vi.fn>; healthCheck: ReturnType<typeof vi.fn>; rotateOne: ReturnType<typeof vi.fn> };
|
||||
logs: { info: string[]; warn: string[]; error: Array<{ obj: Record<string, unknown>; msg: string }> };
|
||||
timers: Array<{ cb: () => void; ms: number }>;
|
||||
cleared: number;
|
||||
}
|
||||
|
||||
function harness(opts: {
|
||||
rows?: SecretBackend[];
|
||||
rotatable?: boolean;
|
||||
overdue?: boolean;
|
||||
health?: { ok: boolean; message?: string } | Error;
|
||||
} = {}): Harness {
|
||||
const rows = opts.rows ?? [backend()];
|
||||
const logs: Harness['logs'] = { info: [], warn: [], error: [] };
|
||||
const timers: Harness['timers'] = [];
|
||||
const state = { cleared: 0 };
|
||||
|
||||
const rotator = {
|
||||
isRotatable: vi.fn(() => opts.rotatable ?? true),
|
||||
isOverdue: vi.fn(() => opts.overdue ?? false),
|
||||
healthCheck: vi.fn(async () => {
|
||||
if (opts.health instanceof Error) throw opts.health;
|
||||
return opts.health ?? { ok: true };
|
||||
}),
|
||||
rotateOne: vi.fn(async () => ({})),
|
||||
};
|
||||
|
||||
const loop = new SecretBackendRotatorLoop({
|
||||
backends: {
|
||||
list: async () => rows,
|
||||
getById: async (id: string) => rows.find((r) => r.id === id) ?? rows[0]!,
|
||||
} as unknown as SecretBackendService,
|
||||
rotator: rotator as unknown as SecretBackendRotator,
|
||||
setTimeout: ((cb: () => void, ms: number) => { timers.push({ cb, ms }); return { id: timers.length } as unknown as NodeJS.Timeout; }),
|
||||
clearTimeout: (() => { state.cleared++; }),
|
||||
log: {
|
||||
info: (m) => { logs.info.push(m); },
|
||||
warn: (m) => { logs.warn.push(m); },
|
||||
error: (obj, msg) => { logs.error.push({ obj, msg }); },
|
||||
},
|
||||
});
|
||||
|
||||
return { loop, rotator, logs, timers, get cleared() { return state.cleared; } } as Harness;
|
||||
}
|
||||
|
||||
/** The boot health check is fire-and-forget; let its microtasks settle. */
|
||||
const settle = async (): Promise<void> => { await Promise.resolve(); await Promise.resolve(); await Promise.resolve(); };
|
||||
|
||||
describe('SecretBackendRotatorLoop', () => {
|
||||
it('stays idle when nothing is rotatable', async () => {
|
||||
const h = harness({ rotatable: false });
|
||||
await h.loop.start();
|
||||
expect(h.logs.info.join(' ')).toMatch(/no rotatable backends/);
|
||||
expect(h.timers).toHaveLength(0);
|
||||
expect(h.rotator.healthCheck).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('runs a boot-time health check for every rotatable backend', async () => {
|
||||
const h = harness({ rows: [backend(), backend({ id: 'b2', name: 'bao2' })] });
|
||||
await h.loop.start();
|
||||
await settle();
|
||||
expect(h.rotator.healthCheck).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it('emits BACKEND_TOKEN_DEAD through the logger, not console', async () => {
|
||||
// The regression that made `mcpctl errors` blind to it: this used to be a
|
||||
// bare console.error, which bypasses the pino stream feeding ErrorLogBuffer.
|
||||
const h = harness({ health: { ok: false, message: 'token rejected' } });
|
||||
await h.loop.start();
|
||||
await settle();
|
||||
expect(h.logs.error).toHaveLength(1);
|
||||
expect(h.logs.error[0]?.obj).toMatchObject({ kind: 'BACKEND_TOKEN_DEAD', backend: 'bao' });
|
||||
expect(h.logs.error[0]?.msg).toBe('token rejected');
|
||||
});
|
||||
|
||||
it('does not log a fatal when the backend is healthy', async () => {
|
||||
const h = harness({ health: { ok: true } });
|
||||
await h.loop.start();
|
||||
await settle();
|
||||
expect(h.logs.error).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('survives a health check that throws', async () => {
|
||||
const h = harness({ health: new Error('network down') });
|
||||
await expect(h.loop.start()).resolves.toBeUndefined();
|
||||
await settle();
|
||||
expect(h.logs.warn.join(' ')).toMatch(/health check threw: network down/);
|
||||
});
|
||||
|
||||
it('rotates immediately when a backend is overdue, and still schedules', async () => {
|
||||
const h = harness({ overdue: true });
|
||||
await h.loop.start();
|
||||
await settle();
|
||||
expect(h.rotator.rotateOne).toHaveBeenCalledWith('b1');
|
||||
expect(h.timers).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('does not rotate on boot when not overdue', async () => {
|
||||
const h = harness({ overdue: false });
|
||||
await h.loop.start();
|
||||
await settle();
|
||||
expect(h.rotator.rotateOne).not.toHaveBeenCalled();
|
||||
expect(h.timers).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('never schedules sooner than the 60s floor, even with adversarial jitter', async () => {
|
||||
// intervalHours tiny + default jitter would otherwise produce a negative delay.
|
||||
const rows = [backend({ config: { url: 'u', rotation: { enabled: true, tokenRole: 'r', intervalHours: 0.0001 } } } as Partial<SecretBackend>)];
|
||||
for (let i = 0; i < 50; i++) {
|
||||
const h = harness({ rows });
|
||||
await h.loop.start();
|
||||
expect(h.timers[0]?.ms).toBeGreaterThanOrEqual(60_000);
|
||||
}
|
||||
});
|
||||
|
||||
it('stop() clears timers and suppresses further scheduling', async () => {
|
||||
const h = harness();
|
||||
await h.loop.start();
|
||||
expect(h.timers).toHaveLength(1);
|
||||
|
||||
h.loop.stop();
|
||||
expect(h.cleared).toBeGreaterThan(0);
|
||||
|
||||
// The `stopped` guard has never been exercised: a firing timer must not
|
||||
// reschedule after stop().
|
||||
const before = h.timers.length;
|
||||
await h.loop.rotateNow('b1').catch(() => undefined);
|
||||
expect(h.timers).toHaveLength(before);
|
||||
});
|
||||
});
|
||||
@@ -1,6 +1,7 @@
|
||||
import { describe, it, expect, vi } from 'vitest';
|
||||
import { PlaintextDriver } from '../src/services/secret-backends/plaintext.js';
|
||||
import { OpenBaoDriver } from '../src/services/secret-backends/openbao.js';
|
||||
import { SecretNotFoundError, SecretBackendUnavailableError } from '../src/services/secret-backends/types.js';
|
||||
|
||||
describe('PlaintextDriver', () => {
|
||||
const driver = new PlaintextDriver({ listAllPlaintext: async () => [{ name: 'a', data: { k: 'v' } }] });
|
||||
@@ -86,9 +87,9 @@ describe('OpenBaoDriver', () => {
|
||||
await expect(driver.delete({ name: 'gone', externalRef: '' })).resolves.toBeUndefined();
|
||||
});
|
||||
|
||||
it('list returns names from the metadata LIST call', async () => {
|
||||
it('list returns names from the metadata listing', async () => {
|
||||
const fetchFn = makeFetch([{
|
||||
url: /\/v1\/secret\/metadata\/mcpctl\/$/,
|
||||
url: /\/v1\/secret\/metadata\/mcpctl\/\?list=true$/,
|
||||
status: 200,
|
||||
body: { data: { keys: ['token1', 'token2', 'sub-folder/'] } },
|
||||
}]);
|
||||
@@ -97,6 +98,10 @@ describe('OpenBaoDriver', () => {
|
||||
{ fetch: fetchFn as unknown as typeof fetch, secretRefResolver: resolver },
|
||||
);
|
||||
const result = await driver.list();
|
||||
// GET ?list=true rather than the LIST verb: proxies (Cilium's ingress
|
||||
// Envoy among them) answer a bare 400 to the non-standard method.
|
||||
const [, listInit] = fetchFn.mock.calls[0] as [unknown, RequestInit];
|
||||
expect(listInit.method).toBe('GET');
|
||||
// Sub-folders (trailing slash) are excluded; only leaf keys are returned.
|
||||
expect(result).toEqual([
|
||||
{ name: 'token1', externalRef: 'secret/mcpctl/token1' },
|
||||
@@ -242,3 +247,91 @@ describe('OpenBaoDriver', () => {
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('OpenBaoDriver: resilience', () => {
|
||||
const resolver = { resolve: vi.fn(async () => 'test-vault-token') };
|
||||
/** No real sleeping — otherwise the backoff tests take seconds. */
|
||||
const noSleep = async (): Promise<void> => undefined;
|
||||
|
||||
function driverWith(fetchFn: ReturnType<typeof vi.fn>, opts: Record<string, unknown> = {}): OpenBaoDriver {
|
||||
return new OpenBaoDriver(
|
||||
{ url: 'http://bao.example:8200', tokenSecretRef: { name: 'bao', key: 'token' } },
|
||||
{ fetch: fetchFn as unknown as typeof fetch, secretRefResolver: resolver, sleep: noSleep, ...opts },
|
||||
);
|
||||
}
|
||||
|
||||
it('maps a 404 read to SecretNotFoundError', async () => {
|
||||
const fetchFn = vi.fn(async () => new Response('', { status: 404 }));
|
||||
await expect(driverWith(fetchFn).read({ name: 'gone', externalRef: '', data: {} }))
|
||||
.rejects.toThrow(SecretNotFoundError);
|
||||
});
|
||||
|
||||
it('purges the token cache and retries once on 403 — outside the retry budget', async () => {
|
||||
// This path existed but was never covered; it is the revocation/re-init case.
|
||||
let n = 0;
|
||||
const fetchFn = vi.fn(async () => {
|
||||
n++;
|
||||
if (n === 1) return new Response('', { status: 403 });
|
||||
return new Response(JSON.stringify({ data: { data: { token: 'ok' } } }), { status: 200 });
|
||||
});
|
||||
const d = driverWith(fetchFn);
|
||||
await expect(d.read({ name: 's', externalRef: '', data: {} })).resolves.toEqual({ token: 'ok' });
|
||||
expect(fetchFn).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it('retries a 503 (sealed) and succeeds', async () => {
|
||||
let n = 0;
|
||||
const fetchFn = vi.fn(async () => {
|
||||
n++;
|
||||
if (n < 3) return new Response('', { status: 503 });
|
||||
return new Response(JSON.stringify({ data: { data: { token: 'ok' } } }), { status: 200 });
|
||||
});
|
||||
await expect(driverWith(fetchFn).read({ name: 's', externalRef: '', data: {} }))
|
||||
.resolves.toEqual({ token: 'ok' });
|
||||
expect(fetchFn).toHaveBeenCalledTimes(3);
|
||||
});
|
||||
|
||||
it('throws SecretBackendUnavailableError once the retry budget is exhausted', async () => {
|
||||
const fetchFn = vi.fn(async () => new Response('', { status: 503 }));
|
||||
await expect(driverWith(fetchFn, { maxAttempts: 3 }).read({ name: 's', externalRef: '', data: {} }))
|
||||
.rejects.toThrow(SecretBackendUnavailableError);
|
||||
expect(fetchFn).toHaveBeenCalledTimes(3);
|
||||
});
|
||||
|
||||
it('classifies a network/abort failure as SecretBackendUnavailableError', async () => {
|
||||
const fetchFn = vi.fn(async () => { throw new DOMException('timed out', 'TimeoutError'); });
|
||||
await expect(driverWith(fetchFn, { maxAttempts: 2 }).read({ name: 's', externalRef: '', data: {} }))
|
||||
.rejects.toThrow(SecretBackendUnavailableError);
|
||||
expect(fetchFn).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it('passes an AbortSignal on every request', async () => {
|
||||
const fetchFn = vi.fn(async () => new Response(JSON.stringify({ data: { data: {} } }), { status: 200 }));
|
||||
await driverWith(fetchFn, { timeoutMs: 1234 }).read({ name: 's', externalRef: '', data: {} });
|
||||
const [, init] = fetchFn.mock.calls[0] as [unknown, RequestInit];
|
||||
expect(init.signal).toBeInstanceOf(AbortSignal);
|
||||
});
|
||||
|
||||
it('healthCheck is unauthenticated and maps OpenBao status codes', async () => {
|
||||
const cases: Array<[number, boolean, string]> = [
|
||||
[200, true, 'active'],
|
||||
[429, true, 'standby'],
|
||||
[501, false, 'not initialized'],
|
||||
[503, false, 'sealed'],
|
||||
];
|
||||
for (const [status, ok, detail] of cases) {
|
||||
const fetchFn = vi.fn(async () => new Response('', { status }));
|
||||
const result = await driverWith(fetchFn).healthCheck();
|
||||
expect(result).toEqual({ ok, detail });
|
||||
// The whole point of the split: no token is minted for a liveness probe.
|
||||
const [, init] = fetchFn.mock.calls[0] as [unknown, RequestInit];
|
||||
expect((init.headers as Record<string, string>)['X-Vault-Token']).toBeUndefined();
|
||||
}
|
||||
});
|
||||
|
||||
it('authCheck reports false when the token can no longer list', async () => {
|
||||
const fetchFn = vi.fn(async () => new Response('', { status: 403 }));
|
||||
const result = await driverWith(fetchFn).authCheck();
|
||||
expect(result.ok).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
198
src/mcpd/tests/secret-cache.test.ts
Normal file
198
src/mcpd/tests/secret-cache.test.ts
Normal file
@@ -0,0 +1,198 @@
|
||||
import { describe, it, expect, vi } from 'vitest';
|
||||
import {
|
||||
CachingSecretBackendDriver,
|
||||
type CachingDriverLog,
|
||||
} from '../src/services/secret-backends/caching.js';
|
||||
import {
|
||||
SecretNotFoundError,
|
||||
SecretBackendUnavailableError,
|
||||
type SecretBackendDriver,
|
||||
type SecretData,
|
||||
} from '../src/services/secret-backends/types.js';
|
||||
|
||||
/** Minimal fake backing driver whose read() behaviour the tests drive. */
|
||||
function makeInner(overrides: Partial<SecretBackendDriver> = {}): SecretBackendDriver & {
|
||||
read: ReturnType<typeof vi.fn>;
|
||||
write: ReturnType<typeof vi.fn>;
|
||||
delete: ReturnType<typeof vi.fn>;
|
||||
} {
|
||||
return {
|
||||
kind: 'fake',
|
||||
read: vi.fn(async () => ({ token: 'live' } as SecretData)),
|
||||
write: vi.fn(async () => ({ externalRef: 'ref', storedData: {} as SecretData })),
|
||||
delete: vi.fn(async () => undefined),
|
||||
list: vi.fn(async () => []),
|
||||
...overrides,
|
||||
} as never;
|
||||
}
|
||||
|
||||
function makeLog(): CachingDriverLog & { warns: Array<Record<string, unknown>>; infos: Array<Record<string, unknown>> } {
|
||||
const warns: Array<Record<string, unknown>> = [];
|
||||
const infos: Array<Record<string, unknown>> = [];
|
||||
return { warns, infos, warn: (o) => { warns.push(o); }, info: (o) => { infos.push(o); } };
|
||||
}
|
||||
|
||||
const REQ = { name: 'gitea-creds', externalRef: 'secret/mcpctl/gitea-creds', data: {} };
|
||||
|
||||
describe('CachingSecretBackendDriver', () => {
|
||||
it('serves from cache within the TTL without touching the backend', async () => {
|
||||
const inner = makeInner();
|
||||
let now = 1_000;
|
||||
const d = new CachingSecretBackendDriver(inner, { ttlMs: 5_000, now: () => now });
|
||||
|
||||
expect(await d.read(REQ)).toEqual({ token: 'live' });
|
||||
now += 4_999;
|
||||
expect(await d.read(REQ)).toEqual({ token: 'live' });
|
||||
|
||||
expect(inner.read).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('refetches once the TTL has elapsed', async () => {
|
||||
const inner = makeInner();
|
||||
let now = 1_000;
|
||||
const d = new CachingSecretBackendDriver(inner, { ttlMs: 5_000, now: () => now });
|
||||
|
||||
await d.read(REQ);
|
||||
now += 5_001;
|
||||
await d.read(REQ);
|
||||
|
||||
expect(inner.read).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it('serves the stale value when the backend is unavailable', async () => {
|
||||
const inner = makeInner();
|
||||
let now = 1_000;
|
||||
const log = makeLog();
|
||||
const d = new CachingSecretBackendDriver(inner, { ttlMs: 1_000, now: () => now, log, backendName: 'bao' });
|
||||
|
||||
await d.read(REQ);
|
||||
inner.read.mockRejectedValue(new SecretBackendUnavailableError('bao down'));
|
||||
now += 10_000;
|
||||
|
||||
// This is the whole point: no throw, so instance.service never marks ERROR.
|
||||
expect(await d.read(REQ)).toEqual({ token: 'live' });
|
||||
expect(log.warns[0]?.kind).toBe('BACKEND_UNREACHABLE');
|
||||
});
|
||||
|
||||
it('logs BACKEND_UNREACHABLE only on the transition, not on every stale read', async () => {
|
||||
const inner = makeInner();
|
||||
let now = 1_000;
|
||||
const log = makeLog();
|
||||
const d = new CachingSecretBackendDriver(inner, { ttlMs: 1_000, now: () => now, log });
|
||||
|
||||
await d.read(REQ);
|
||||
inner.read.mockRejectedValue(new SecretBackendUnavailableError('bao down'));
|
||||
for (let i = 0; i < 5; i++) { now += 2_000; await d.read(REQ); }
|
||||
|
||||
expect(log.warns.filter((w) => w.kind === 'BACKEND_UNREACHABLE')).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('logs BACKEND_RECOVERED once the backend answers again', async () => {
|
||||
const inner = makeInner();
|
||||
let now = 1_000;
|
||||
const log = makeLog();
|
||||
const d = new CachingSecretBackendDriver(inner, { ttlMs: 1_000, now: () => now, log });
|
||||
|
||||
await d.read(REQ);
|
||||
inner.read.mockRejectedValue(new SecretBackendUnavailableError('bao down'));
|
||||
now += 2_000;
|
||||
await d.read(REQ);
|
||||
|
||||
inner.read.mockResolvedValue({ token: 'rotated' });
|
||||
now += 2_000;
|
||||
expect(await d.read(REQ)).toEqual({ token: 'rotated' });
|
||||
expect(log.infos.filter((i) => i.kind === 'BACKEND_RECOVERED')).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('NEVER serves stale for a deleted secret — evicts and rethrows', async () => {
|
||||
// Regression guard. Serving stale here would resurrect a revoked
|
||||
// credential, which is strictly worse than an outage.
|
||||
const inner = makeInner();
|
||||
let now = 1_000;
|
||||
const d = new CachingSecretBackendDriver(inner, { ttlMs: 1_000, now: () => now });
|
||||
|
||||
await d.read(REQ);
|
||||
inner.read.mockRejectedValue(new SecretNotFoundError('gone'));
|
||||
now += 2_000;
|
||||
|
||||
await expect(d.read(REQ)).rejects.toThrow(SecretNotFoundError);
|
||||
expect(d.stats().entries).toBe(0);
|
||||
|
||||
// And the entry really is gone — a later unavailable error has nothing to serve.
|
||||
inner.read.mockRejectedValue(new SecretBackendUnavailableError('bao down'));
|
||||
await expect(d.read(REQ)).rejects.toThrow(SecretBackendUnavailableError);
|
||||
});
|
||||
|
||||
it('does not serve stale for a non-transport error (e.g. revoked grants)', async () => {
|
||||
const inner = makeInner();
|
||||
let now = 1_000;
|
||||
const d = new CachingSecretBackendDriver(inner, { ttlMs: 1_000, now: () => now });
|
||||
|
||||
await d.read(REQ);
|
||||
inner.read.mockRejectedValue(new Error('OpenBao read: HTTP 403 permission denied'));
|
||||
now += 2_000;
|
||||
|
||||
await expect(d.read(REQ)).rejects.toThrow(/403/);
|
||||
});
|
||||
|
||||
it('rethrows on a cold cache even when the backend is unavailable', async () => {
|
||||
const inner = makeInner({ read: vi.fn(async () => { throw new SecretBackendUnavailableError('bao down'); }) as never });
|
||||
const d = new CachingSecretBackendDriver(inner);
|
||||
|
||||
await expect(d.read(REQ)).rejects.toThrow(SecretBackendUnavailableError);
|
||||
});
|
||||
|
||||
it('write() refreshes the cache so a read-after-write does not lag', async () => {
|
||||
const inner = makeInner();
|
||||
const d = new CachingSecretBackendDriver(inner, { ttlMs: 60_000 });
|
||||
|
||||
await d.read(REQ);
|
||||
await d.write({ name: REQ.name, data: { token: 'brand-new' } });
|
||||
|
||||
expect(await d.read(REQ)).toEqual({ token: 'brand-new' });
|
||||
expect(inner.read).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('delete() evicts', async () => {
|
||||
const inner = makeInner();
|
||||
const d = new CachingSecretBackendDriver(inner, { ttlMs: 60_000 });
|
||||
|
||||
await d.read(REQ);
|
||||
await d.delete({ name: REQ.name, externalRef: REQ.externalRef });
|
||||
|
||||
expect(d.stats().entries).toBe(0);
|
||||
});
|
||||
|
||||
it('bounds the map with an LRU eviction', async () => {
|
||||
const inner = makeInner();
|
||||
const d = new CachingSecretBackendDriver(inner, { ttlMs: 60_000, maxEntries: 2 });
|
||||
|
||||
await d.read({ ...REQ, name: 'a' });
|
||||
await d.read({ ...REQ, name: 'b' });
|
||||
await d.read({ ...REQ, name: 'a' }); // 'a' becomes most-recently-used
|
||||
await d.read({ ...REQ, name: 'c' }); // evicts 'b'
|
||||
|
||||
expect(d.stats().entries).toBe(2);
|
||||
inner.read.mockClear();
|
||||
await d.read({ ...REQ, name: 'a' });
|
||||
expect(inner.read).not.toHaveBeenCalled(); // 'a' survived
|
||||
await d.read({ ...REQ, name: 'b' });
|
||||
expect(inner.read).toHaveBeenCalledTimes(1); // 'b' was evicted
|
||||
});
|
||||
|
||||
it('reports stale count and age via stats()', async () => {
|
||||
const inner = makeInner();
|
||||
let now = 1_000;
|
||||
const d = new CachingSecretBackendDriver(inner, { ttlMs: 1_000, now: () => now });
|
||||
|
||||
await d.read({ ...REQ, name: 'a' });
|
||||
await d.read({ ...REQ, name: 'b' });
|
||||
expect(d.stats()).toMatchObject({ entries: 2, servingStale: 0 });
|
||||
|
||||
inner.read.mockRejectedValue(new SecretBackendUnavailableError('down'));
|
||||
now += 2_000;
|
||||
await d.read({ ...REQ, name: 'a' });
|
||||
|
||||
expect(d.stats()).toMatchObject({ entries: 2, servingStale: 1, oldestStaleSince: 3_000 });
|
||||
});
|
||||
});
|
||||
@@ -544,3 +544,32 @@ describe('HealthProbeRunner', () => {
|
||||
expect(result.message).toBe('ECONNREFUSED 10.0.0.1:3000');
|
||||
});
|
||||
});
|
||||
|
||||
describe('HealthProbeRunner stale-pipe eviction (mcpctl#114)', () => {
|
||||
it('evicts the cached stdio client exactly once, when failures cross the threshold', async () => {
|
||||
const instanceRepo = mockInstanceRepo();
|
||||
const serverRepo = mockServerRepo();
|
||||
const orchestrator = mockOrchestrator();
|
||||
const mcpProxyService = mockMcpProxyService();
|
||||
const runner = new HealthProbeRunner(instanceRepo, serverRepo, orchestrator, undefined, mcpProxyService);
|
||||
|
||||
const instance = makeInstance();
|
||||
// intervalSeconds: 0 so every tick actually probes; failureThreshold: 3.
|
||||
const server = makeServer({
|
||||
healthCheck: { tool: 'list_datasources', arguments: {}, intervalSeconds: 0, timeoutSeconds: 10, failureThreshold: 3 },
|
||||
} as Partial<McpServer>);
|
||||
vi.mocked(instanceRepo.findAll).mockResolvedValue([instance]);
|
||||
vi.mocked(serverRepo.findById).mockResolvedValue(server);
|
||||
vi.mocked(mcpProxyService.execute).mockRejectedValue(new Error('pipe is dead'));
|
||||
|
||||
await runner.tick(); // failure 1 — degraded
|
||||
await runner.tick(); // failure 2 — degraded
|
||||
expect(mcpProxyService.removeClient).not.toHaveBeenCalled();
|
||||
|
||||
await runner.tick(); // failure 3 — crosses threshold → evict once
|
||||
expect(mcpProxyService.removeClient).toHaveBeenCalledExactlyOnceWith('container-abc');
|
||||
|
||||
await runner.tick(); // failure 4 — already past threshold, no re-evict
|
||||
expect(mcpProxyService.removeClient).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
|
||||
66
src/mcpd/tests/warm-secret-cache.test.ts
Normal file
66
src/mcpd/tests/warm-secret-cache.test.ts
Normal file
@@ -0,0 +1,66 @@
|
||||
import { describe, it, expect, vi } from 'vitest';
|
||||
import { warmSecretCache } from '../src/bootstrap/warm-secret-cache.js';
|
||||
import type { PrismaClient } from '@prisma/client';
|
||||
import type { SecretService } from '../src/services/secret.service.js';
|
||||
|
||||
function prismaWith(servers: Array<{ name: string; env: unknown }>): PrismaClient {
|
||||
return { mcpServer: { findMany: vi.fn(async () => servers) } } as unknown as PrismaClient;
|
||||
}
|
||||
const noLog = { info: (): void => undefined, warn: (): void => undefined };
|
||||
|
||||
const envRef = (name: string, secret: string, key: string): unknown =>
|
||||
({ name, valueFrom: { secretRef: { name: secret, key } } });
|
||||
|
||||
describe('warmSecretCache', () => {
|
||||
it('resolves every distinct secret ref exactly once', async () => {
|
||||
const prisma = prismaWith([
|
||||
{ name: 'gitea', env: [envRef('GITEA_ACCESS_TOKEN', 'gitea-creds', 'GITEA_ACCESS_TOKEN')] },
|
||||
// Two servers sharing one secret must not cost two reads.
|
||||
{ name: 'a', env: [envRef('T', 'shared', 'TOKEN')] },
|
||||
{ name: 'b', env: [envRef('T', 'shared', 'TOKEN')] },
|
||||
]);
|
||||
const resolve = vi.fn(async () => 'value');
|
||||
const result = await warmSecretCache(prisma, { resolve } as unknown as SecretService, noLog);
|
||||
|
||||
expect(resolve).toHaveBeenCalledTimes(2);
|
||||
expect(result).toEqual({ warmed: 2, failed: 0 });
|
||||
});
|
||||
|
||||
it('ignores inline env values', async () => {
|
||||
const prisma = prismaWith([{ name: 's', env: [{ name: 'PLAIN', value: 'x' }] }]);
|
||||
const resolve = vi.fn(async () => 'v');
|
||||
expect(await warmSecretCache(prisma, { resolve } as unknown as SecretService, noLog))
|
||||
.toEqual({ warmed: 0, failed: 0 });
|
||||
expect(resolve).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('never throws when the backend is down — startup must not block', async () => {
|
||||
const prisma = prismaWith([
|
||||
{ name: 'a', env: [envRef('T', 's1', 'K')] },
|
||||
{ name: 'b', env: [envRef('T', 's2', 'K')] },
|
||||
]);
|
||||
const resolve = vi.fn(async () => { throw new Error('bao unreachable'); });
|
||||
await expect(warmSecretCache(prisma, { resolve } as unknown as SecretService, noLog))
|
||||
.resolves.toEqual({ warmed: 0, failed: 2 });
|
||||
});
|
||||
|
||||
it('keeps going after one bad reference', async () => {
|
||||
const prisma = prismaWith([
|
||||
{ name: 'a', env: [envRef('T', 'missing', 'K')] },
|
||||
{ name: 'b', env: [envRef('T', 'present', 'K')] },
|
||||
]);
|
||||
const resolve = vi.fn(async (n: string) => {
|
||||
if (n === 'missing') throw new Error('no such secret');
|
||||
return 'v';
|
||||
});
|
||||
expect(await warmSecretCache(prisma, { resolve } as unknown as SecretService, noLog))
|
||||
.toEqual({ warmed: 1, failed: 1 });
|
||||
});
|
||||
|
||||
it('only considers servers with replicas > 0', async () => {
|
||||
const prisma = prismaWith([]);
|
||||
await warmSecretCache(prisma, { resolve: vi.fn() } as unknown as SecretService, noLog);
|
||||
const findMany = (prisma.mcpServer.findMany as unknown as ReturnType<typeof vi.fn>);
|
||||
expect(findMany.mock.calls[0]?.[0]).toMatchObject({ where: { replicas: { gt: 0 } } });
|
||||
});
|
||||
});
|
||||
133
src/mcplocal/tests/smoke/secret-resilience.smoke.test.ts
Normal file
133
src/mcplocal/tests/smoke/secret-resilience.smoke.test.ts
Normal 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);
|
||||
});
|
||||
});
|
||||
@@ -306,3 +306,83 @@ export async function testWriteReadDelete(
|
||||
throw new Error(`vault smoke delete ${relPath}: HTTP ${String(delRes.status)} ${await readError(delRes)}`);
|
||||
}
|
||||
}
|
||||
|
||||
export interface KubernetesAuthRoleConfig {
|
||||
/** ServiceAccount names permitted to log in as this role. */
|
||||
boundServiceAccountNames: string[];
|
||||
/** Namespaces those ServiceAccounts must live in. */
|
||||
boundServiceAccountNamespaces: string[];
|
||||
/** Policies attached to tokens issued for this role. */
|
||||
tokenPolicies: string[];
|
||||
/** Token TTL in seconds. Default 3600 — pods only need it at startup. */
|
||||
tokenTtlSeconds?: number;
|
||||
}
|
||||
|
||||
/**
|
||||
* POST /v1/auth/<authMount>/role/<role>. Idempotent: upserts the role.
|
||||
*
|
||||
* Used to give each MCP server pod its own OpenBao identity, bound to its own
|
||||
* ServiceAccount, so the injector can fetch only that server's secrets. Note
|
||||
* `bound_service_account_names` is a list but we pass exactly one — a role
|
||||
* shared between ServiceAccounts would defeat the point.
|
||||
*/
|
||||
export async function ensureKubernetesAuthRole(
|
||||
url: string,
|
||||
token: string,
|
||||
authMount: string,
|
||||
role: string,
|
||||
cfg: KubernetesAuthRoleConfig,
|
||||
deps: VaultDeps = {},
|
||||
): Promise<void> {
|
||||
const fetchImpl = deps.fetch ?? globalThis.fetch;
|
||||
const mount = authMount.replace(/^\/|\/$/g, '');
|
||||
const res = await fetchImpl(`${baseUrl(url)}/v1/auth/${mount}/role/${encodeURIComponent(role)}`, {
|
||||
method: 'POST',
|
||||
headers: headers(token, deps.namespace, true),
|
||||
body: JSON.stringify({
|
||||
bound_service_account_names: cfg.boundServiceAccountNames,
|
||||
bound_service_account_namespaces: cfg.boundServiceAccountNamespaces,
|
||||
token_policies: cfg.tokenPolicies,
|
||||
token_ttl: cfg.tokenTtlSeconds ?? 3600,
|
||||
}),
|
||||
});
|
||||
if (!res.ok) {
|
||||
throw new Error(`vault ensure k8s auth role ${role}: HTTP ${String(res.status)} ${await readError(res)}`);
|
||||
}
|
||||
}
|
||||
|
||||
/** DELETE /v1/auth/<authMount>/role/<role>. Idempotent — 404 is success. */
|
||||
export async function deleteKubernetesAuthRole(
|
||||
url: string,
|
||||
token: string,
|
||||
authMount: string,
|
||||
role: string,
|
||||
deps: VaultDeps = {},
|
||||
): Promise<void> {
|
||||
const fetchImpl = deps.fetch ?? globalThis.fetch;
|
||||
const mount = authMount.replace(/^\/|\/$/g, '');
|
||||
const res = await fetchImpl(`${baseUrl(url)}/v1/auth/${mount}/role/${encodeURIComponent(role)}`, {
|
||||
method: 'DELETE',
|
||||
headers: headers(token, deps.namespace, false),
|
||||
});
|
||||
if (!res.ok && res.status !== 404) {
|
||||
throw new Error(`vault delete k8s auth role ${role}: HTTP ${String(res.status)} ${await readError(res)}`);
|
||||
}
|
||||
}
|
||||
|
||||
/** DELETE /v1/sys/policies/acl/<name>. Idempotent — 404 is success. */
|
||||
export async function deletePolicy(
|
||||
url: string,
|
||||
token: string,
|
||||
name: string,
|
||||
deps: VaultDeps = {},
|
||||
): Promise<void> {
|
||||
const fetchImpl = deps.fetch ?? globalThis.fetch;
|
||||
const res = await fetchImpl(`${baseUrl(url)}/v1/sys/policies/acl/${encodeURIComponent(name)}`, {
|
||||
method: 'DELETE',
|
||||
headers: headers(token, deps.namespace, false),
|
||||
});
|
||||
if (!res.ok && res.status !== 404) {
|
||||
throw new Error(`vault delete policy ${name}: HTTP ${String(res.status)} ${await readError(res)}`);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -33,3 +33,70 @@ export function buildAppMcpdPolicyHcl(cfg: AppMcpdPolicyConfig): string {
|
||||
'',
|
||||
].join('\n');
|
||||
}
|
||||
|
||||
/**
|
||||
* Per-server read policy for the OpenBao Agent Injector.
|
||||
*
|
||||
* Each MCP server pod authenticates to OpenBao as its OWN ServiceAccount and
|
||||
* gets a policy naming only the secrets that server actually declares. That is
|
||||
* the difference between "the injector moved the credential out of the pod
|
||||
* spec" and "the injector made things worse": with one shared role, every
|
||||
* opted-in pod — including third-party images we do not control — could read
|
||||
* every secret under the prefix. Here, `gitea` can read `gitea-creds` and
|
||||
* nothing else.
|
||||
*
|
||||
* No wildcards. Each secret is named explicitly; adding a secret to a server
|
||||
* means regenerating and re-writing this policy, which is exactly the audit
|
||||
* trail we want.
|
||||
*/
|
||||
export interface ServerSecretPolicyConfig {
|
||||
/** KV v2 mount name, e.g. 'secret'. */
|
||||
mount: string;
|
||||
/** Path prefix under the mount, e.g. 'mcpctl'. */
|
||||
pathPrefix: string;
|
||||
/** Secret names this server may read. Order-insensitive; deduped + sorted. */
|
||||
secretNames: string[];
|
||||
}
|
||||
|
||||
export function buildServerSecretPolicyHcl(cfg: ServerSecretPolicyConfig): string {
|
||||
const { mount } = cfg;
|
||||
const prefix = cfg.pathPrefix.replace(/^\/|\/$/g, '');
|
||||
// Sort + dedupe so the generated HCL is stable: an unstable policy body would
|
||||
// rewrite on every reconcile and make real changes invisible in the audit log.
|
||||
const names = [...new Set(cfg.secretNames)].sort((a, b) => a.localeCompare(b));
|
||||
const lines: string[] = [];
|
||||
for (const name of names) {
|
||||
const path = prefix === '' ? name : `${prefix}/${name}`;
|
||||
lines.push(`path "${mount}/data/${path}" { capabilities = ["read"] }`);
|
||||
lines.push(`path "${mount}/metadata/${path}" { capabilities = ["read"] }`);
|
||||
}
|
||||
lines.push('');
|
||||
return lines.join('\n');
|
||||
}
|
||||
|
||||
/**
|
||||
* Grants mcpd needs in order to provision the per-server identities above:
|
||||
* write its own scoped policies and the matching Kubernetes auth roles.
|
||||
*
|
||||
* Deliberately confined by name prefix. This is a real privilege increase for
|
||||
* mcpd, but a much narrower one than the obvious alternative of granting it
|
||||
* Kubernetes `secrets` verbs — that would let it read every Secret in the
|
||||
* cluster, not just the ones it already owns.
|
||||
*/
|
||||
export interface ServerProvisioningPolicyConfig {
|
||||
/** Kubernetes auth mount, e.g. 'kubernetes-worker0'. */
|
||||
authMount: string;
|
||||
/** Shared name prefix for generated policies + roles, e.g. 'mcpctl-server-'. */
|
||||
namePrefix: string;
|
||||
}
|
||||
|
||||
export function buildServerProvisioningPolicyHcl(cfg: ServerProvisioningPolicyConfig): string {
|
||||
const authMount = cfg.authMount.replace(/^\/|\/$/g, '');
|
||||
const prefix = cfg.namePrefix;
|
||||
return [
|
||||
`path "sys/policies/acl/${prefix}*" { capabilities = ["create", "read", "update", "delete"] }`,
|
||||
`path "auth/${authMount}/role/${prefix}*" { capabilities = ["create", "read", "update", "delete"] }`,
|
||||
`path "auth/${authMount}/role" { capabilities = ["list"] }`,
|
||||
'',
|
||||
].join('\n');
|
||||
}
|
||||
|
||||
138
src/shared/tests/vault-server-scoping.test.ts
Normal file
138
src/shared/tests/vault-server-scoping.test.ts
Normal file
@@ -0,0 +1,138 @@
|
||||
/**
|
||||
* Per-server OpenBao scoping: each MCP server pod gets its own identity and a
|
||||
* policy naming only its own secrets.
|
||||
*
|
||||
* The property under test is containment. A shared role would let any opted-in
|
||||
* pod — including third-party images we don't control — read every secret under
|
||||
* the prefix, which is a worse position than leaving values in the pod spec.
|
||||
*/
|
||||
import { describe, it, expect, vi } from 'vitest';
|
||||
import {
|
||||
buildServerSecretPolicyHcl,
|
||||
buildServerProvisioningPolicyHcl,
|
||||
ensureKubernetesAuthRole,
|
||||
deleteKubernetesAuthRole,
|
||||
deletePolicy,
|
||||
} from '../src/vault/index.js';
|
||||
|
||||
describe('buildServerSecretPolicyHcl', () => {
|
||||
const cfg = { mount: 'secret', pathPrefix: 'mcpctl' };
|
||||
|
||||
it('grants read on exactly the named secrets and nothing else', () => {
|
||||
const hcl = buildServerSecretPolicyHcl({ ...cfg, secretNames: ['gitea-creds'] });
|
||||
expect(hcl).toContain('path "secret/data/mcpctl/gitea-creds"');
|
||||
expect(hcl).toContain('path "secret/metadata/mcpctl/gitea-creds"');
|
||||
expect(hcl).toContain('capabilities = ["read"]');
|
||||
});
|
||||
|
||||
it('never emits a wildcard — that is the whole point', () => {
|
||||
const hcl = buildServerSecretPolicyHcl({ ...cfg, secretNames: ['gitea-creds', 'unifi-creds'] });
|
||||
expect(hcl).not.toContain('*');
|
||||
});
|
||||
|
||||
it('grants no write capability anywhere', () => {
|
||||
const hcl = buildServerSecretPolicyHcl({ ...cfg, secretNames: ['a', 'b'] });
|
||||
for (const verb of ['create', 'update', 'delete', 'list', 'sudo']) {
|
||||
expect(hcl, `must not grant ${verb}`).not.toContain(verb);
|
||||
}
|
||||
});
|
||||
|
||||
it('does not reach other servers\' secrets', () => {
|
||||
const gitea = buildServerSecretPolicyHcl({ ...cfg, secretNames: ['gitea-creds'] });
|
||||
expect(gitea).not.toContain('unifi-creds');
|
||||
expect(gitea).not.toContain('anthropic-key');
|
||||
expect(gitea).not.toContain('litellm-key');
|
||||
});
|
||||
|
||||
it('is stable under reordering and duplication', () => {
|
||||
// An unstable body would rewrite on every reconcile, drowning real changes
|
||||
// in the OpenBao audit log.
|
||||
const a = buildServerSecretPolicyHcl({ ...cfg, secretNames: ['b', 'a', 'b'] });
|
||||
const b = buildServerSecretPolicyHcl({ ...cfg, secretNames: ['a', 'b'] });
|
||||
expect(a).toBe(b);
|
||||
});
|
||||
|
||||
it('handles an empty prefix without producing a double slash', () => {
|
||||
const hcl = buildServerSecretPolicyHcl({ mount: 'secret', pathPrefix: '', secretNames: ['x'] });
|
||||
expect(hcl).toContain('path "secret/data/x"');
|
||||
expect(hcl).not.toContain('//');
|
||||
});
|
||||
|
||||
it('emits nothing but a trailing newline for a server with no secrets', () => {
|
||||
expect(buildServerSecretPolicyHcl({ ...cfg, secretNames: [] })).toBe('');
|
||||
});
|
||||
});
|
||||
|
||||
describe('buildServerProvisioningPolicyHcl', () => {
|
||||
const cfg = { authMount: 'kubernetes-worker0', namePrefix: 'mcpctl-server-' };
|
||||
|
||||
it('confines mcpd to the generated name prefix', () => {
|
||||
const hcl = buildServerProvisioningPolicyHcl(cfg);
|
||||
expect(hcl).toContain('path "sys/policies/acl/mcpctl-server-*"');
|
||||
expect(hcl).toContain('path "auth/kubernetes-worker0/role/mcpctl-server-*"');
|
||||
});
|
||||
|
||||
it('does not grant blanket policy or auth administration', () => {
|
||||
const hcl = buildServerProvisioningPolicyHcl(cfg);
|
||||
expect(hcl).not.toContain('path "sys/policies/acl/*"');
|
||||
expect(hcl).not.toContain('path "auth/*"');
|
||||
expect(hcl).not.toContain('path "sys/*"');
|
||||
});
|
||||
|
||||
it('grants no access to secret data at all', () => {
|
||||
// mcpd reads secrets through its OWN policy; the provisioning grant must
|
||||
// not widen that surface.
|
||||
expect(buildServerProvisioningPolicyHcl(cfg)).not.toContain('secret/data');
|
||||
});
|
||||
});
|
||||
|
||||
describe('ensureKubernetesAuthRole', () => {
|
||||
it('binds exactly one ServiceAccount in one namespace', async () => {
|
||||
const fetchFn = vi.fn(async () => new Response(null, { status: 204 }));
|
||||
await ensureKubernetesAuthRole(
|
||||
'http://bao.example:8200', 'tok', 'kubernetes-worker0', 'mcpctl-server-gitea',
|
||||
{
|
||||
boundServiceAccountNames: ['mcpctl-server-gitea'],
|
||||
boundServiceAccountNamespaces: ['mcpctl-servers'],
|
||||
tokenPolicies: ['mcpctl-server-gitea'],
|
||||
},
|
||||
{ fetch: fetchFn as unknown as typeof fetch },
|
||||
);
|
||||
const [url, init] = fetchFn.mock.calls[0] as [string, RequestInit];
|
||||
expect(url).toBe('http://bao.example:8200/v1/auth/kubernetes-worker0/role/mcpctl-server-gitea');
|
||||
const body = JSON.parse(init.body as string) as Record<string, unknown>;
|
||||
expect(body.bound_service_account_names).toEqual(['mcpctl-server-gitea']);
|
||||
expect(body.bound_service_account_namespaces).toEqual(['mcpctl-servers']);
|
||||
expect(body.token_policies).toEqual(['mcpctl-server-gitea']);
|
||||
expect(body.token_ttl).toBe(3600);
|
||||
});
|
||||
|
||||
it('surfaces the OpenBao error body on failure', async () => {
|
||||
const fetchFn = vi.fn(async () => new Response(JSON.stringify({ errors: ['permission denied'] }), { status: 403 }));
|
||||
await expect(ensureKubernetesAuthRole(
|
||||
'http://bao.example:8200', 'tok', 'kubernetes-worker0', 'r',
|
||||
{ boundServiceAccountNames: ['a'], boundServiceAccountNamespaces: ['n'], tokenPolicies: ['p'] },
|
||||
{ fetch: fetchFn as unknown as typeof fetch },
|
||||
)).rejects.toThrow(/permission denied/);
|
||||
});
|
||||
});
|
||||
|
||||
describe('cleanup helpers are idempotent', () => {
|
||||
it('treats a missing role as already deleted', async () => {
|
||||
const fetchFn = vi.fn(async () => new Response('', { status: 404 }));
|
||||
await expect(deleteKubernetesAuthRole('http://b', 't', 'kubernetes-worker0', 'gone', { fetch: fetchFn as unknown as typeof fetch }))
|
||||
.resolves.toBeUndefined();
|
||||
});
|
||||
|
||||
it('treats a missing policy as already deleted', async () => {
|
||||
const fetchFn = vi.fn(async () => new Response('', { status: 404 }));
|
||||
await expect(deletePolicy('http://b', 't', 'gone', { fetch: fetchFn as unknown as typeof fetch }))
|
||||
.resolves.toBeUndefined();
|
||||
});
|
||||
|
||||
it('still raises a real failure', async () => {
|
||||
const fetchFn = vi.fn(async () => new Response(JSON.stringify({ errors: ['denied'] }), { status: 403 }));
|
||||
await expect(deletePolicy('http://b', 't', 'p', { fetch: fetchFn as unknown as typeof fetch }))
|
||||
.rejects.toThrow(/denied/);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user