fix(k8s): read ApiException.code when classifying client-node errors
@kubernetes/client-node v1 raises ApiException, which carries the HTTP status on `.code`. Every check in the orchestrator read only `.statusCode` — the pre-1.0 HttpError field — so `status` came back undefined and each "expected" 404/409 was rethrown instead of handled. Found in production: the first start of a server with a volume read a not-yet-existing PVC, got a 404 that should have meant "create it", and failed the instance instead. The same latent bug sat in removeContainer, where a missing pod failed the delete rather than being treated as already gone. httpStatusOf() checks .code, .statusCode and .response.statusCode, and falls back to parsing the rendered "HTTP-Code: NNN" message for wrappers that keep none of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017BMXdb2qZbPSh8Q7XpTyjB
This commit is contained in:
@@ -13,6 +13,32 @@ import type { K8sOfficialClientConfig } from './k8s-client-official.js';
|
|||||||
import { generatePodSpec, generatePvcSpec, sanitizeName } from './manifest-generator.js';
|
import { generatePodSpec, generatePvcSpec, sanitizeName } from './manifest-generator.js';
|
||||||
import type { V1Pod, V1PersistentVolumeClaim } from '@kubernetes/client-node';
|
import type { V1Pod, V1PersistentVolumeClaim } from '@kubernetes/client-node';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* HTTP status from a client-node error, across the shapes it actually throws.
|
||||||
|
*
|
||||||
|
* @kubernetes/client-node v1 raises `ApiException`, which carries the status on
|
||||||
|
* `.code` — not `.statusCode` as the pre-1.0 `HttpError` did. Reading only
|
||||||
|
* `.statusCode` yields undefined and turns every "expected" 404/409 into a
|
||||||
|
* rethrow, so a missing pod fails a delete and a missing PVC fails a create.
|
||||||
|
* The message-parsing fallback covers wrappers that preserve neither field.
|
||||||
|
*/
|
||||||
|
export function httpStatusOf(err: unknown): number | undefined {
|
||||||
|
const e = err as {
|
||||||
|
code?: unknown;
|
||||||
|
statusCode?: unknown;
|
||||||
|
response?: { statusCode?: unknown };
|
||||||
|
message?: unknown;
|
||||||
|
};
|
||||||
|
for (const candidate of [e.code, e.statusCode, e.response?.statusCode]) {
|
||||||
|
if (typeof candidate === 'number') return candidate;
|
||||||
|
}
|
||||||
|
if (typeof e.message === 'string') {
|
||||||
|
const match = /HTTP-Code:\s*(\d{3})/.exec(e.message);
|
||||||
|
if (match?.[1] !== undefined) return Number(match[1]);
|
||||||
|
}
|
||||||
|
return undefined;
|
||||||
|
}
|
||||||
|
|
||||||
function mapPodState(pod: V1Pod): ContainerInfo['state'] {
|
function mapPodState(pod: V1Pod): ContainerInfo['state'] {
|
||||||
const cs = pod.status?.containerStatuses?.[0];
|
const cs = pod.status?.containerStatuses?.[0];
|
||||||
if (cs) {
|
if (cs) {
|
||||||
@@ -113,8 +139,7 @@ export class KubernetesOrchestrator implements McpOrchestrator {
|
|||||||
gracePeriodSeconds: 5,
|
gracePeriodSeconds: 5,
|
||||||
});
|
});
|
||||||
} catch (err: unknown) {
|
} catch (err: unknown) {
|
||||||
const status = (err as { statusCode?: number }).statusCode
|
const status = httpStatusOf(err);
|
||||||
?? (err as { response?: { statusCode?: number } }).response?.statusCode;
|
|
||||||
if (status !== 404) throw err;
|
if (status !== 404) throw err;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -323,8 +348,7 @@ export class KubernetesOrchestrator implements McpOrchestrator {
|
|||||||
body: { apiVersion: 'v1', kind: 'Namespace', metadata: { name } },
|
body: { apiVersion: 'v1', kind: 'Namespace', metadata: { name } },
|
||||||
});
|
});
|
||||||
} catch (createErr: unknown) {
|
} catch (createErr: unknown) {
|
||||||
const status = (createErr as { statusCode?: number }).statusCode
|
const status = httpStatusOf(createErr);
|
||||||
?? (createErr as { response?: { statusCode?: number } }).response?.statusCode;
|
|
||||||
if (status !== 409) throw createErr; // Already exists is fine
|
if (status !== 409) throw createErr; // Already exists is fine
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -350,8 +374,7 @@ export class KubernetesOrchestrator implements McpOrchestrator {
|
|||||||
});
|
});
|
||||||
return; // Already there — reuse it, data and all.
|
return; // Already there — reuse it, data and all.
|
||||||
} catch (err: unknown) {
|
} catch (err: unknown) {
|
||||||
const status = (err as { statusCode?: number }).statusCode
|
const status = httpStatusOf(err);
|
||||||
?? (err as { response?: { statusCode?: number } }).response?.statusCode;
|
|
||||||
if (status !== 404) throw err;
|
if (status !== 404) throw err;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -361,8 +384,7 @@ export class KubernetesOrchestrator implements McpOrchestrator {
|
|||||||
body: generatePvcSpec(volume, this.namespace, labels) as V1PersistentVolumeClaim,
|
body: generatePvcSpec(volume, this.namespace, labels) as V1PersistentVolumeClaim,
|
||||||
});
|
});
|
||||||
} catch (createErr: unknown) {
|
} catch (createErr: unknown) {
|
||||||
const status = (createErr as { statusCode?: number }).statusCode
|
const status = httpStatusOf(createErr);
|
||||||
?? (createErr as { response?: { statusCode?: number } }).response?.statusCode;
|
|
||||||
if (status !== 409) throw createErr; // Lost a create race — fine.
|
if (status !== 409) throw createErr; // Lost a create race — fine.
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -87,7 +87,7 @@ vi.mock('@kubernetes/client-node', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
// Import after mock
|
// Import after mock
|
||||||
import { KubernetesOrchestrator } from '../src/services/k8s/kubernetes-orchestrator.js';
|
import { KubernetesOrchestrator, httpStatusOf } from '../src/services/k8s/kubernetes-orchestrator.js';
|
||||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||||
const k8sMock = await import('@kubernetes/client-node') as any;
|
const k8sMock = await import('@kubernetes/client-node') as any;
|
||||||
const { setHandler, clearHandlers, mockCore } = k8sMock.__testHelpers;
|
const { setHandler, clearHandlers, mockCore } = k8sMock.__testHelpers;
|
||||||
@@ -356,3 +356,26 @@ describe('KubernetesOrchestrator', () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('httpStatusOf', () => {
|
||||||
|
it('reads .code, which is what client-node v1 ApiException actually sets', () => {
|
||||||
|
// The bug this exists for: reading only .statusCode returned undefined, so
|
||||||
|
// an expected 404 was rethrown and a first-start PVC create never happened.
|
||||||
|
expect(httpStatusOf({ code: 404 })).toBe(404);
|
||||||
|
expect(httpStatusOf({ code: 409 })).toBe(409);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('still reads the pre-1.0 HttpError shapes', () => {
|
||||||
|
expect(httpStatusOf({ statusCode: 404 })).toBe(404);
|
||||||
|
expect(httpStatusOf({ response: { statusCode: 409 } })).toBe(409);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('falls back to parsing the rendered message', () => {
|
||||||
|
expect(httpStatusOf(new Error('HTTP-Code: 403\nMessage: Unknown API Status Code!'))).toBe(403);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns undefined when there is no status to find', () => {
|
||||||
|
expect(httpStatusOf(new Error('socket hang up'))).toBeUndefined();
|
||||||
|
expect(httpStatusOf({ code: 'ECONNREFUSED' })).toBeUndefined();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user