Implement ExecTask and ExecTaskStreaming - #102
ritesh-harihar wants to merge 14 commits into
Conversation
Current PR includes Dropping |
tgross
left a comment
There was a problem hiding this comment.
Made a first pass review but there's a lot of editing to do here before this is really ready
| ptmMu.Lock() | ||
| ptmMu.Unlock() // fence: ensures stdin goroutine is not inside Setsize/Write when defer fires |
There was a problem hiding this comment.
I rechecked the logic, it didn't actually guarantee that.
The resize goroutine keeps running until the client stream ends, so the Lock/Unlock only confirmed no Setsize was in progress at that exact instant, and nothing stopped a new Setsize from starting right after. The earlier code only caught the in-progress case at that instant, it did nothing to stop a new Setsize from starting afterward.
Added a closed flag under the same lock. Once that's set, the stdin goroutine sees it and backs off before calling Setsize, so nothing touches the PTY while it's being closed.
| func stderrDataMsg(b []byte) *drivers.ExecTaskStreamingResponseMsg { | ||
| return &drivers.ExecTaskStreamingResponseMsg{ | ||
| Stderr: &dproto.ExecTaskStreamingIOOperation{Data: b}, | ||
| } | ||
| } | ||
|
|
||
| func stderrCloseMsg() *drivers.ExecTaskStreamingResponseMsg { | ||
| return &drivers.ExecTaskStreamingResponseMsg{ | ||
| Stderr: &dproto.ExecTaskStreamingIOOperation{Close: true}, | ||
| } | ||
| } |
There was a problem hiding this comment.
How do these differ from the stdoutDataMsg/stdoutCloseMsg functions? Does the close message even need to be a function rather than var declaration, given that it returns a literal?
There was a problem hiding this comment.
There is not much difference (only by the field) these can be refactored, yeah the close message didn't need to be a function.
Added two wrappers, wrapStdout/wrapStderr for these.
| // Mark ptm closed under the lock, this waits for any in-flight Write/Setsize | ||
| // and blocks the stdin goroutine from touching ptm before the deferred Close. | ||
| ptmMu.Lock() | ||
| ptmClosed = true | ||
| ptmMu.Unlock() |
There was a problem hiding this comment.
I keep coming back to this because it's not clear to me why we want the mutex to guard ptm in the first place. Isn't *os.File safe for concurrent use? drivers.ExecTaskStream isn't, but I thought the pty itself was?
There was a problem hiding this comment.
Yes *os.File is safe for concurrent use, so the mutex was never needed for the PTY itself. The one exception was pty.Setsize: creack/pty implements it as an ioctl on the raw fd from File.Fd(), which bypasses the poller and can race Close. So the mutex wasn't guarding the PTY, it was just a workaround for Setsize's unsafe raw-fd path.
Removed the mutex(which was confusing) and added new setPTYSize(), which keeps the fd valid for the call and is safe against a concurrent Close.
this is the exact race the pipeline caught before the fix: here.
Fixes: #5
Summary
Before
ExecTaskandExecTaskStreamingwere stubs that returned "not yet implemented". This meant:This PR implements all three exec entry-points —
ExecTask,ExecTaskStreaming, andExecTaskStreamingRaw— so the driver fully supports both script checks and nomad alloc exec, including interactive PTY sessions.How it works — namespace entry
Every exec path enters the running task's Linux namespaces by invoking
nsentertargeting the shim's PID. The namespaces entered are:The resulting command prefix built by nsenterArgs():
nsenter --no-fork --target=<PID> --mount --pid --ipc [--net=<netns>] -- <command>Three exec entry-points
Nomad calls different interfaces depending on context:
Request flow — before this PR
Request flow — after this PR
Path A — interactive shell (nomad alloc exec -t)
Details
Path B — non-interactive command (nomad alloc exec without -t)
Details
Path C — script checks (ExecTask)
Details
Testing
Details
Result Before:
Result After:
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.