Skip to content

Commit c7c66e2

Browse files
aledbfclaude
andcommitted
refactor(lifecycle): run hooks via the Docker exec API (drop docker exec shell-out)
The lifecycle shell server no longer shells out to `docker exec`. It runs each command as an independent `/bin/sh -c` exec through the moby client exec API, added as EngineClient.ExecInContainer (ExecCreate → ExecAttach → stdcopy demux → ExecInspect for the exit code). The DockerAPI interface gains ExecCreate/ ExecAttach/ExecInspect; the mock and a focused unit test (stdcopy framing) cover it. This drops the hand-rolled EOT framing protocol and the persistent-shell management from internal/lifecycle/shell.go. It is behavior-preserving: no lifecycle hook or userEnvProbe command depends on shell state carried across commands (each probe runs its own login/interactive shell; hooks are independent), so one-exec-per-command is equivalent while providing user/env per exec natively. NewShellServer now takes (ctx, ContainerExecutor) instead of a docker path; all nine call sites pass the engine client. Validated: 35 run-user-commands + exec runtime parity cases match against the TS oracle (lifecycle hooks, userEnvProbe, secrets, dotfiles, subfolder config). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 70ee1e2 commit c7c66e2

11 files changed

Lines changed: 298 additions & 371 deletions

File tree

‎docs/migration/REMAINING-WORK.md‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -426,6 +426,30 @@ with the TS oracle except where indicated; they are sequenced by value/risk.
426426
and a real-daemon smoke (`up` + `up --remove-existing-container`) exercises the
427427
reshaped `RemoveContainer`/`Events`/`ListContainers`/`PullImage` paths.
428428

429+
## Self-containment (fewer external commands)
430+
431+
The CLI drives containers through Go libraries where it is parity-neutral to do
432+
so, keeping the big shell-outs only where a library would regress behavior or
433+
bloat the binary.
434+
435+
- **Docker context + git root — ✅ DONE (zero new deps).** `docker context inspect`
436+
is replaced by a direct read of the context store (`internal/docker/context.go`);
437+
`git rev-parse --show-cdup` by a lexical `.git` parent walk. Removes the last hard
438+
runtime deps on the external `docker`/`git` binaries in the connect/config paths.
439+
- **Lifecycle shell → Docker exec API — ✅ DONE.** The lifecycle shell server (hooks
440+
+ userEnvProbe) no longer shells out to `docker exec`; it runs each command as an
441+
independent `/bin/sh -c` via the moby client exec API (`EngineClient.ExecInContainer`
442+
— ExecCreate/Attach/Inspect + stdcopy demux), dropping the EOT framing protocol.
443+
Safe because no hook/probe relies on shell state carried across commands (each
444+
probe command spawns its own login/interactive shell). Validated: 35
445+
run-user-commands + exec runtime cases match against the oracle.
446+
- **Kept as shell-outs (deliberate):** `docker buildx build` (buildx feature breadth
447+
+ the user's builder/context; a library would regress buildx or pull the heavy
448+
buildkit client), `docker compose` (its output is not in the compared stream, so a
449+
library adds huge deps for zero parity gain), interactive `docker exec -it` (the
450+
inherited-TTY design avoids re-adding PTY/SIGWINCH/128+N complexity), and the
451+
credential-helper protocol (external executables by design).
452+
429453
## Decisions that must be made explicit
430454

431455
Decisions already taken (firm):

‎internal/cli/exec.go‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -225,11 +225,7 @@ func runExec(ctx context.Context, opts *execOpts, cmdArgs []string) error {
225225

226226
// Probed env (lowest priority)
227227
if probeStrategy != lifecycle.ProbeNone {
228-
dockerPath := opts.dockerPath
229-
if dockerPath == "" {
230-
dockerPath = "docker"
231-
}
232-
probeServer, probeErr := lifecycle.NewShellServer(dockerPath, containerID, remoteUser, logger)
228+
probeServer, probeErr := lifecycle.NewShellServer(ctx, engine, containerID, remoteUser, logger)
233229
if probeErr == nil {
234230
probedEnv, _ := lifecycle.ProbeRemoteEnv(logger, probeServer, probeStrategy, remoteUser)
235231
probeServer.Close()

‎internal/cli/run_user_commands.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,7 @@ func runUserCommands(ctx context.Context, out Output, opts *runUserCommandsOpts)
229229

230230
var mergedRemoteEnv []string
231231
if probeStrategy != lifecycle.ProbeNone {
232-
probeServer, probeErr := lifecycle.NewShellServer(opts.dockerPath, containerID, remoteUser, logger)
232+
probeServer, probeErr := lifecycle.NewShellServer(ctx, engine, containerID, remoteUser, logger)
233233
if probeErr == nil {
234234
probedEnv, _ := lifecycle.ProbeRemoteEnv(logger, probeServer, probeStrategy, remoteUser, opts.containerSessionDataFolder)
235235
probeServer.Close()
@@ -252,7 +252,7 @@ func runUserCommands(ctx context.Context, out Output, opts *runUserCommandsOpts)
252252
}
253253
}
254254

255-
shellServer, err := lifecycle.NewShellServer(opts.dockerPath, containerID, remoteUser, logger, mergedRemoteEnv...)
255+
shellServer, err := lifecycle.NewShellServer(ctx, engine, containerID, remoteUser, logger, mergedRemoteEnv...)
256256
if err != nil {
257257
return writeErrorResult(out, fmt.Sprintf("Failed to start shell server: %v", err))
258258
}

‎internal/cli/setup.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ func newSetUpCmd() *cobra.Command {
129129

130130
var mergedRemoteEnv []string
131131
if probeStrategy != lifecycle.ProbeNone {
132-
probeServer, probeErr := lifecycle.NewShellServer(dockerPath, containerID, remoteUser, logger)
132+
probeServer, probeErr := lifecycle.NewShellServer(ctx, engine, containerID, remoteUser, logger)
133133
if probeErr == nil {
134134
probedEnv, _ := lifecycle.ProbeRemoteEnv(logger, probeServer, probeStrategy, remoteUser, containerSessionDataFolder)
135135
probeServer.Close()
@@ -147,7 +147,7 @@ func newSetUpCmd() *cobra.Command {
147147
}
148148
}
149149

150-
shellServer, err := lifecycle.NewShellServer(dockerPath, containerID, remoteUser, logger, mergedRemoteEnv...)
150+
shellServer, err := lifecycle.NewShellServer(ctx, engine, containerID, remoteUser, logger, mergedRemoteEnv...)
151151
if err != nil {
152152
return writeErrorResult(out, fmt.Sprintf("Failed to start shell: %v", err))
153153
}

‎internal/cli/up.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -615,7 +615,7 @@ func (r *upRunner) runReattachLifecycle(containerID string) {
615615
probeStrategy = lifecycle.UserEnvProbeStrategy(merged.UserEnvProbe)
616616
}
617617
if probeStrategy != lifecycle.ProbeNone {
618-
if probeServer, probeErr := lifecycle.NewShellServer(opts.dockerPath, containerID, remoteUser, logger); probeErr == nil {
618+
if probeServer, probeErr := lifecycle.NewShellServer(ctx, engine, containerID, remoteUser, logger); probeErr == nil {
619619
probedEnv, _ := lifecycle.ProbeRemoteEnv(logger, probeServer, probeStrategy, remoteUser, opts.containerSessionDataFolder)
620620
probeServer.Close()
621621
for k, v := range probedEnv {
@@ -635,7 +635,7 @@ func (r *upRunner) runReattachLifecycle(containerID string) {
635635
}
636636
}
637637

638-
shellServer, err := lifecycle.NewShellServer(opts.dockerPath, containerID, remoteUser, logger, mergedRemoteEnv...)
638+
shellServer, err := lifecycle.NewShellServer(ctx, engine, containerID, remoteUser, logger, mergedRemoteEnv...)
639639
if err != nil {
640640
logger.Write(fmt.Sprintf("Could not start shell server for reattach hooks: %v", err), log.LevelWarning)
641641
return
@@ -1566,7 +1566,7 @@ func (r *upRunner) fromCompose(cfg *config.DevContainerConfig, loadResult *confi
15661566
// a non-nil error (a *coreerrors.ContainerError with a TS-compatible description)
15671567
// only when a lifecycle hook fails; probe/dotfiles issues stay non-fatal warnings.
15681568
func (r *upRunner) runLifecycleForUp(containerID string, cfg *config.DevContainerConfig, workDir string, remoteUser string) error {
1569-
ctx, logger, dockerClient, engine, opts := r.ctx, r.log, r.docker, r.engine, r.opts
1569+
ctx, logger, engine, opts := r.ctx, r.log, r.engine, r.opts
15701570
// Build merged remote env: probed shell env → CLI --remote-env → config remoteEnv
15711571
probeStrategy := lifecycle.UserEnvProbeStrategy(opts.defaultUserEnvProbe)
15721572
if cfg.UserEnvProbe != "" {
@@ -1577,7 +1577,7 @@ func (r *upRunner) runLifecycleForUp(containerID string, cfg *config.DevContaine
15771577

15781578
// Probe user environment inside the container (with caching)
15791579
if probeStrategy != lifecycle.ProbeNone {
1580-
probeServer, probeErr := lifecycle.NewShellServer(dockerClient.DockerPath, containerID, remoteUser, logger)
1580+
probeServer, probeErr := lifecycle.NewShellServer(ctx, engine, containerID, remoteUser, logger)
15811581
if probeErr == nil {
15821582
probedEnv, _ := lifecycle.ProbeRemoteEnv(logger, probeServer, probeStrategy, remoteUser, opts.containerSessionDataFolder)
15831583
probeServer.Close()
@@ -1605,7 +1605,7 @@ func (r *upRunner) runLifecycleForUp(containerID string, cfg *config.DevContaine
16051605
}
16061606
}
16071607

1608-
shellServer, err := lifecycle.NewShellServer(dockerClient.DockerPath, containerID, remoteUser, logger, mergedRemoteEnv...)
1608+
shellServer, err := lifecycle.NewShellServer(ctx, engine, containerID, remoteUser, logger, mergedRemoteEnv...)
16091609
if err != nil {
16101610
logger.Write(fmt.Sprintf("Could not start shell server for lifecycle hooks: %v", err), log.LevelWarning)
16111611
return nil

‎internal/docker/engine.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,9 @@ type DockerAPI interface {
2929
ContainerRemove(ctx context.Context, containerID string, options mobyclient.ContainerRemoveOptions) (mobyclient.ContainerRemoveResult, error)
3030
ContainerStart(ctx context.Context, containerID string, options mobyclient.ContainerStartOptions) (mobyclient.ContainerStartResult, error)
3131
Events(ctx context.Context, options mobyclient.EventsListOptions) mobyclient.EventsResult
32+
ExecCreate(ctx context.Context, containerID string, options mobyclient.ExecCreateOptions) (mobyclient.ExecCreateResult, error)
33+
ExecAttach(ctx context.Context, execID string, options mobyclient.ExecAttachOptions) (mobyclient.ExecAttachResult, error)
34+
ExecInspect(ctx context.Context, execID string, options mobyclient.ExecInspectOptions) (mobyclient.ExecInspectResult, error)
3235
ImageInspect(ctx context.Context, imageID string, options ...mobyclient.ImageInspectOption) (mobyclient.ImageInspectResult, error)
3336
ImagePull(ctx context.Context, refStr string, options mobyclient.ImagePullOptions) (mobyclient.ImagePullResponse, error)
3437
ImageTag(ctx context.Context, options mobyclient.ImageTagOptions) (mobyclient.ImageTagResult, error)

‎internal/docker/engine_test.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,30 @@ type mockAPI struct {
3030
imagePullFn func(ctx context.Context, ref string, opts mobyclient.ImagePullOptions) (mobyclient.ImagePullResponse, error)
3131
imageTagFn func(ctx context.Context, opts mobyclient.ImageTagOptions) error
3232
serverVersionFn func(ctx context.Context, opts mobyclient.ServerVersionOptions) (mobyclient.ServerVersionResult, error)
33+
execCreateFn func(ctx context.Context, containerID string, opts mobyclient.ExecCreateOptions) (mobyclient.ExecCreateResult, error)
34+
execAttachFn func(ctx context.Context, execID string, opts mobyclient.ExecAttachOptions) (mobyclient.ExecAttachResult, error)
35+
execInspectFn func(ctx context.Context, execID string) (mobyclient.ExecInspectResult, error)
36+
}
37+
38+
func (m *mockAPI) ExecInspect(ctx context.Context, execID string, _ mobyclient.ExecInspectOptions) (mobyclient.ExecInspectResult, error) {
39+
if m.execInspectFn != nil {
40+
return m.execInspectFn(ctx, execID)
41+
}
42+
return mobyclient.ExecInspectResult{}, errors.New("not implemented")
43+
}
44+
45+
func (m *mockAPI) ExecCreate(ctx context.Context, containerID string, opts mobyclient.ExecCreateOptions) (mobyclient.ExecCreateResult, error) {
46+
if m.execCreateFn != nil {
47+
return m.execCreateFn(ctx, containerID, opts)
48+
}
49+
return mobyclient.ExecCreateResult{}, errors.New("not implemented")
50+
}
51+
52+
func (m *mockAPI) ExecAttach(ctx context.Context, execID string, opts mobyclient.ExecAttachOptions) (mobyclient.ExecAttachResult, error) {
53+
if m.execAttachFn != nil {
54+
return m.execAttachFn(ctx, execID, opts)
55+
}
56+
return mobyclient.ExecAttachResult{}, errors.New("not implemented")
3357
}
3458

3559
func (m *mockAPI) Close() error { return nil }

‎internal/docker/exec.go‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
package docker
2+
3+
import (
4+
"bytes"
5+
"context"
6+
7+
"github.com/moby/moby/api/pkg/stdcopy"
8+
mobyclient "github.com/moby/moby/client"
9+
)
10+
11+
// ExecInContainer runs `/bin/sh -c command` inside a container via the Docker
12+
// exec API (no `docker exec` subprocess) and returns its stdout, stderr and exit
13+
// code. It runs non-interactively (no TTY): stdout/stderr are demultiplexed with
14+
// stdcopy, and the exit code is read from the exec inspect after the stream ends.
15+
//
16+
// user (docker exec -u) and env ("KEY=VALUE", docker exec -e) mirror how the
17+
// lifecycle shell runs hooks and the userEnvProbe as the remote user.
18+
func (e *EngineClient) ExecInContainer(ctx context.Context, containerID, user string, env []string, command string) (stdout, stderr string, exitCode int, err error) {
19+
created, err := e.API.ExecCreate(ctx, containerID, mobyclient.ExecCreateOptions{
20+
User: user,
21+
Env: env,
22+
Cmd: []string{"/bin/sh", "-c", command},
23+
AttachStdout: true,
24+
AttachStderr: true,
25+
})
26+
if err != nil {
27+
return "", "", -1, err
28+
}
29+
30+
attach, err := e.API.ExecAttach(ctx, created.ID, mobyclient.ExecAttachOptions{})
31+
if err != nil {
32+
return "", "", -1, err
33+
}
34+
defer attach.Close()
35+
36+
var outBuf, errBuf bytes.Buffer
37+
// Non-TTY exec output is a multiplexed stdout/stderr frame stream.
38+
if _, copyErr := stdcopy.StdCopy(&outBuf, &errBuf, attach.Reader); copyErr != nil {
39+
return outBuf.String(), errBuf.String(), -1, copyErr
40+
}
41+
42+
inspect, err := e.API.ExecInspect(ctx, created.ID, mobyclient.ExecInspectOptions{})
43+
if err != nil {
44+
return outBuf.String(), errBuf.String(), -1, err
45+
}
46+
return outBuf.String(), errBuf.String(), inspect.ExitCode, nil
47+
}

‎internal/docker/exec_test.go‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
package docker
2+
3+
import (
4+
"bufio"
5+
"bytes"
6+
"context"
7+
"encoding/binary"
8+
"net"
9+
"testing"
10+
11+
mobyclient "github.com/moby/moby/client"
12+
13+
"github.com/devcontainers/cli/internal/log"
14+
)
15+
16+
// nopConn is a net.Conn whose only real method is Close (HijackedResponse.Close
17+
// calls Conn.Close); the embedded nil Conn is never otherwise used in the test.
18+
type nopConn struct{ net.Conn }
19+
20+
func (nopConn) Close() error { return nil }
21+
22+
// stdcopyFrame writes one stdcopy frame (8-byte header: stream type + big-endian
23+
// size, then payload) — the multiplexed format the non-TTY exec stream uses.
24+
func stdcopyFrame(buf *bytes.Buffer, streamType byte, payload string) {
25+
var hdr [8]byte
26+
hdr[0] = streamType // 1=stdout, 2=stderr
27+
binary.BigEndian.PutUint32(hdr[4:], uint32(len(payload)))
28+
buf.Write(hdr[:])
29+
buf.WriteString(payload)
30+
}
31+
32+
// framedExecStream builds a stdcopy-multiplexed stream carrying the given stdout
33+
// and stderr payloads, as the non-TTY exec attach endpoint would return.
34+
func framedExecStream(_ *testing.T, out, errText string) *bufio.Reader {
35+
var buf bytes.Buffer
36+
stdcopyFrame(&buf, 1, out)
37+
stdcopyFrame(&buf, 2, errText)
38+
return bufio.NewReader(&buf)
39+
}
40+
41+
func TestExecInContainer(t *testing.T) {
42+
var gotOpts mobyclient.ExecCreateOptions
43+
api := &mockAPI{
44+
execCreateFn: func(_ context.Context, id string, opts mobyclient.ExecCreateOptions) (mobyclient.ExecCreateResult, error) {
45+
if id != "cid" {
46+
t.Errorf("containerID = %q", id)
47+
}
48+
gotOpts = opts
49+
return mobyclient.ExecCreateResult{ID: "exec1"}, nil
50+
},
51+
execAttachFn: func(_ context.Context, execID string, _ mobyclient.ExecAttachOptions) (mobyclient.ExecAttachResult, error) {
52+
if execID != "exec1" {
53+
t.Errorf("execID = %q", execID)
54+
}
55+
return mobyclient.ExecAttachResult{HijackedResponse: mobyclient.HijackedResponse{
56+
Conn: nopConn{},
57+
Reader: framedExecStream(t, "hello\n", "oops\n"),
58+
}}, nil
59+
},
60+
execInspectFn: func(_ context.Context, _ string) (mobyclient.ExecInspectResult, error) {
61+
return mobyclient.ExecInspectResult{ExitCode: 3}, nil
62+
},
63+
}
64+
e := &EngineClient{API: api, Log: log.Null}
65+
66+
stdout, stderr, code, err := e.ExecInContainer(context.Background(), "cid", "vscode", []string{"FOO=bar"}, "echo hello")
67+
if err != nil {
68+
t.Fatal(err)
69+
}
70+
if stdout != "hello\n" {
71+
t.Errorf("stdout = %q, want hello", stdout)
72+
}
73+
if stderr != "oops\n" {
74+
t.Errorf("stderr = %q, want oops", stderr)
75+
}
76+
if code != 3 {
77+
t.Errorf("exitCode = %d, want 3", code)
78+
}
79+
// The command is run as `/bin/sh -c <command>` with the given user/env.
80+
if gotOpts.User != "vscode" || len(gotOpts.Env) != 1 || gotOpts.Env[0] != "FOO=bar" {
81+
t.Errorf("opts user/env = %q/%v", gotOpts.User, gotOpts.Env)
82+
}
83+
if len(gotOpts.Cmd) != 3 || gotOpts.Cmd[0] != "/bin/sh" || gotOpts.Cmd[1] != "-c" || gotOpts.Cmd[2] != "echo hello" {
84+
t.Errorf("cmd = %v", gotOpts.Cmd)
85+
}
86+
if gotOpts.TTY {
87+
t.Error("exec must be non-TTY for stdcopy demux")
88+
}
89+
}

0 commit comments

Comments
 (0)