fix(secrets): injector wrapper must replace the entrypoint #119
@@ -564,8 +564,13 @@ export class InstanceService {
|
|||||||
spec.serviceAccountName = identity;
|
spec.serviceAccountName = identity;
|
||||||
spec.automountServiceAccountToken = true;
|
spec.automountServiceAccountToken = true;
|
||||||
spec.annotations = this.serverIdentity!.annotationsFor(identity, spec.envFromSecret);
|
spec.annotations = this.serverIdentity!.annotationsFor(identity, spec.envFromSecret);
|
||||||
|
// Replaces the image entrypoint (see wrapCommand); the original argv
|
||||||
|
// is folded in, so spec.command must not also be emitted as args.
|
||||||
const wrapped = this.serverIdentity!.wrapCommand(server, spec.command);
|
const wrapped = this.serverIdentity!.wrapCommand(server, spec.command);
|
||||||
if (wrapped !== undefined) spec.command = wrapped;
|
if (wrapped !== undefined) {
|
||||||
|
spec.entrypoint = wrapped;
|
||||||
|
delete spec.command;
|
||||||
|
}
|
||||||
} catch (idErr) {
|
} catch (idErr) {
|
||||||
const msg = idErr instanceof Error ? idErr.message : String(idErr);
|
const msg = idErr instanceof Error ? idErr.message : String(idErr);
|
||||||
return this.markInstanceError(instance, `secret identity provisioning failed: ${msg}`);
|
return this.markInstanceError(instance, `secret identity provisioning failed: ${msg}`);
|
||||||
|
|||||||
@@ -259,7 +259,12 @@ function buildContainerSpec(spec: ContainerSpec) {
|
|||||||
// In Docker, spec.command maps to Cmd (args to entrypoint).
|
// In Docker, spec.command maps to Cmd (args to entrypoint).
|
||||||
// In k8s, we use `args` to pass arguments to the image's entrypoint,
|
// In k8s, we use `args` to pass arguments to the image's entrypoint,
|
||||||
// preserving the runner image's entrypoint (uvx, npx -y, etc.)
|
// preserving the runner image's entrypoint (uvx, npx -y, etc.)
|
||||||
if (spec.command && spec.command.length > 0) {
|
//
|
||||||
|
// `entrypoint` is the exception: it REPLACES the entrypoint (k8s `command`),
|
||||||
|
// which injected secret delivery needs so the sourcing shell can be PID 1.
|
||||||
|
if (spec.entrypoint && spec.entrypoint.length > 0) {
|
||||||
|
container.command = spec.entrypoint;
|
||||||
|
} else if (spec.command && spec.command.length > 0) {
|
||||||
container.args = spec.command;
|
container.args = spec.command;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -62,6 +62,17 @@ export interface ContainerSpec {
|
|||||||
serviceAccountName?: string;
|
serviceAccountName?: string;
|
||||||
/** Injected agents need the projected SA token; plain servers do not. */
|
/** Injected agents need the projected SA token; plain servers do not. */
|
||||||
automountServiceAccountToken?: boolean;
|
automountServiceAccountToken?: boolean;
|
||||||
|
/**
|
||||||
|
* REPLACES the image's ENTRYPOINT (k8s `command`), unlike `command`, which is
|
||||||
|
* appended to it as `args`.
|
||||||
|
*
|
||||||
|
* Needed only for injected secret delivery: the container has to run a shell
|
||||||
|
* that sources the rendered file before exec'ing the real process, and that
|
||||||
|
* shell must BE the entrypoint. Because it replaces the entrypoint, the argv
|
||||||
|
* here has to include whatever the image's entrypoint would have contributed
|
||||||
|
* (`npx -y`, `uvx`, ...).
|
||||||
|
*/
|
||||||
|
entrypoint?: string[];
|
||||||
/** Host port to bind (null = auto-assign) */
|
/** Host port to bind (null = auto-assign) */
|
||||||
hostPort?: number | null;
|
hostPort?: number | null;
|
||||||
/** Container port to expose */
|
/** Container port to expose */
|
||||||
|
|||||||
@@ -96,26 +96,6 @@ export class ServerIdentityService {
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* Rewrite argv so the rendered secrets are sourced before the server runs.
|
|
||||||
*
|
|
||||||
* `command` is what mcpd already computed: for package-based servers that is
|
|
||||||
* the runner image's entrypoint plus the package, which mcpd owns. For a
|
|
||||||
* dockerImage server it may be absent — the image's own ENTRYPOINT would run,
|
|
||||||
* and mcpd cannot introspect it, which is why `entrypoint` is required on the
|
|
||||||
* server row in that case (enforced at validation).
|
|
||||||
*/
|
|
||||||
wrapCommand(
|
|
||||||
server: Pick<McpServer, 'env' | 'entrypoint'>,
|
|
||||||
command: string[] | undefined,
|
|
||||||
): string[] | undefined {
|
|
||||||
const secretNames = this.secretNamesFor(server);
|
|
||||||
if (secretNames.length === 0) return command;
|
|
||||||
const argv = command ?? (server.entrypoint as string[] | null) ?? undefined;
|
|
||||||
if (argv === undefined || argv.length === 0) return command;
|
|
||||||
return wrapCommandForInjector(argv, secretNames);
|
|
||||||
}
|
|
||||||
|
|
||||||
/** Identity name for a server — also the SA, policy and role name. */
|
/** Identity name for a server — also the SA, policy and role name. */
|
||||||
identityNameFor(serverName: string): string {
|
identityNameFor(serverName: string): string {
|
||||||
return `${IDENTITY_PREFIX}${serverName}`;
|
return `${IDENTITY_PREFIX}${serverName}`;
|
||||||
@@ -135,6 +115,39 @@ export class ServerIdentityService {
|
|||||||
return [...names].sort((a, b) => a.localeCompare(b));
|
return [...names].sort((a, b) => a.localeCompare(b));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Build the container ENTRYPOINT that sources the rendered secrets and then
|
||||||
|
* execs the real server.
|
||||||
|
*
|
||||||
|
* This must REPLACE the image's entrypoint, not extend it. mcpd normally puts
|
||||||
|
* a package server's argv into k8s `args` so the runner image's `npx -y` /
|
||||||
|
* `uvx` entrypoint still runs; a wrapper placed there would be executed BY
|
||||||
|
* npx (`npx -y /bin/sh -c ...`) and fail. So the argv returned here folds in
|
||||||
|
* whatever the image's entrypoint would have contributed.
|
||||||
|
*
|
||||||
|
* Returns undefined when there is nothing to wrap, leaving the pod on the
|
||||||
|
* normal entrypoint+args path.
|
||||||
|
*/
|
||||||
|
wrapCommand(
|
||||||
|
server: Pick<McpServer, 'env' | 'entrypoint' | 'packageName' | 'runtime'>,
|
||||||
|
command: string[] | undefined,
|
||||||
|
): string[] | undefined {
|
||||||
|
const secretNames = this.secretNamesFor(server);
|
||||||
|
if (secretNames.length === 0) return undefined;
|
||||||
|
|
||||||
|
// mcpd owns the runner images, so their entrypoints are known. A
|
||||||
|
// dockerImage server's is not introspectable — hence `entrypoint` being
|
||||||
|
// required on the row at validation time.
|
||||||
|
const imageEntrypoint = server.packageName
|
||||||
|
? server.runtime === 'python'
|
||||||
|
? ['uvx']
|
||||||
|
: ['npx', '-y']
|
||||||
|
: ((server.entrypoint as string[] | null) ?? undefined);
|
||||||
|
if (imageEntrypoint === undefined || imageEntrypoint.length === 0) return undefined;
|
||||||
|
|
||||||
|
return wrapCommandForInjector([...imageEntrypoint, ...(command ?? [])], secretNames);
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Converge the identity for one server. Returns the identity name so the
|
* Converge the identity for one server. Returns the identity name so the
|
||||||
* caller can stamp it onto the pod spec.
|
* caller can stamp it onto the pod spec.
|
||||||
|
|||||||
@@ -136,3 +136,23 @@ describe('shell quoting survives adversarial values', () => {
|
|||||||
expect(out).toBe('');
|
expect(out).toBe('');
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('the wrapper must REPLACE the image entrypoint, not extend it', () => {
|
||||||
|
// Regression: mcpd maps ContainerSpec.command -> k8s `args` so the runner
|
||||||
|
// image's `npx -y` / `uvx` entrypoint still runs. Emitting the wrapper there
|
||||||
|
// meant the pod actually ran `npx -y /bin/sh -c '...'`, which crashlooped.
|
||||||
|
it('emits container.command (entrypoint) and no args', () => {
|
||||||
|
const pod = generatePodSpec({
|
||||||
|
name: 'g', image: 'runner',
|
||||||
|
entrypoint: ['/bin/sh', '-c', '. /vault/secrets/s; exec "$0" "$@"', 'npx', '-y', '@leval/mcp-grafana'],
|
||||||
|
} as ContainerSpec, 'mcpctl-servers');
|
||||||
|
expect(pod.spec.containers[0]?.command?.[0]).toBe('/bin/sh');
|
||||||
|
expect(pod.spec.containers[0]?.args).toBeUndefined();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('leaves the normal entrypoint+args path alone when not wrapping', () => {
|
||||||
|
const pod = generatePodSpec({ name: 'g', image: 'runner', command: ['@leval/mcp-grafana'] } as ContainerSpec, 'mcpctl-servers');
|
||||||
|
expect(pod.spec.containers[0]?.command).toBeUndefined();
|
||||||
|
expect(pod.spec.containers[0]?.args).toEqual(['@leval/mcp-grafana']);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user