Skip to content

Commit 04bebbc

Browse files
bussyjdOisinKyne
authored andcommitted
fix(agent): gate kubectl -t on both stdin and stdout being TTYs
obol's in-pod exec requested `kubectl exec -t` whenever stdin was a terminal. Under command substitution — e.g. a release-smoke `out=$(obol buy inference …)` invoked from a tmux pane — stdin is a pty but stdout is a pipe. Allocating a TTY there corrupts the captured output and, on kubectl < 1.36, panics with a nil-pointer dereference in the terminal-resize path (terminalSizeQueueAdapter.Next on a nil receiver). Mirror kubectl's own setupTTY: request -t only when BOTH stdin and stdout are character devices. Factor the decision into the pure streamsSupportTTY predicate and cover the regression (terminal stdin + pipe stdout → no -t) plus an end-to-end "no -t under go test" guarantee.
1 parent 47a012f commit 04bebbc

2 files changed

Lines changed: 114 additions & 4 deletions

File tree

internal/agentruntime/exec.go

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import (
1616
// no side effects. argv is the full in-pod command vector; argv[0] is the
1717
// binary (e.g. "python3" or a runtime CLI path), argv[1:] its args.
1818
//
19-
// withTTY toggles the `-t` flag; callers usually pass stdinIsTerminal().
19+
// withTTY toggles the `-t` flag; callers usually pass shouldRequestTTY().
2020
func BuildExecArgs(runtime Runtime, id string, argv []string, withTTY bool) []string {
2121
svc := Describe(runtime).ServiceName
2222
out := []string{"exec", "-i"}
@@ -50,7 +50,7 @@ func ExecInPod(cfg *config.Config, runtime Runtime, id string, argv []string) er
5050

5151
kubectlBinary := filepath.Join(cfg.BinDir, "kubectl")
5252

53-
cmd := exec.Command(kubectlBinary, BuildExecArgs(runtime, id, argv, stdinIsTerminal())...)
53+
cmd := exec.Command(kubectlBinary, BuildExecArgs(runtime, id, argv, shouldRequestTTY())...)
5454
cmd.Env = append(os.Environ(), "KUBECONFIG="+kubeconfigPath)
5555
cmd.Stdin = os.Stdin
5656
cmd.Stdout = os.Stdout
@@ -68,7 +68,35 @@ func ExecInPod(cfg *config.Config, runtime Runtime, id string, argv []string) er
6868
return nil
6969
}
7070

71-
func stdinIsTerminal() bool {
72-
info, err := os.Stdin.Stat()
71+
// shouldRequestTTY reports whether `kubectl exec` should be invoked with -t.
72+
// It mirrors kubectl's own setupTTY decision: a TTY is allocated only when BOTH
73+
// the process stdin AND stdout are terminals.
74+
//
75+
// Gating on stdin alone is wrong. When obol is invoked under command
76+
// substitution — e.g. the release smoke's `buy_output=$(obol buy inference …)`
77+
// run from a tmux pane — stdin is a pty but stdout is a pipe. Requesting -t in
78+
// that case is harmful on two counts:
79+
// - it corrupts captured output with TTY line-ending/escape semantics, and
80+
// - kubectl < 1.36 panics with a nil-pointer dereference in its
81+
// terminal-resize path (terminalSizeQueueAdapter.Next on a nil receiver)
82+
// when -t is set but stdout is not a terminal.
83+
//
84+
// Requiring both streams to be terminals avoids both and matches kubectl.
85+
func shouldRequestTTY() bool {
86+
return streamsSupportTTY(os.Stdin, os.Stdout)
87+
}
88+
89+
// streamsSupportTTY is the pure predicate behind shouldRequestTTY: both streams
90+
// must be character devices (terminals). Factored out so it is unit-testable
91+
// without touching the process-wide os.Stdin/os.Stdout.
92+
func streamsSupportTTY(in, out *os.File) bool {
93+
return isCharDevice(in) && isCharDevice(out)
94+
}
95+
96+
func isCharDevice(f *os.File) bool {
97+
if f == nil {
98+
return false
99+
}
100+
info, err := f.Stat()
73101
return err == nil && info.Mode()&os.ModeCharDevice != 0
74102
}

internal/agentruntime/exec_test.go

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package agentruntime
22

33
import (
4+
"os"
45
"reflect"
56
"testing"
67

@@ -77,6 +78,87 @@ func TestBuildExecArgs(t *testing.T) {
7778
}
7879
}
7980

81+
// TestStreamsSupportTTY locks in the kubectl-mirroring gate: -t is requested
82+
// only when BOTH stdin and stdout are terminals. The decisive regression case
83+
// is "stdin is a terminal but stdout is a pipe" (command substitution like the
84+
// release smoke's `$(obol buy inference …)` from a tmux pane), which previously
85+
// added -t and crashed kubectl < 1.36 in its terminal-resize path.
86+
func TestStreamsSupportTTY(t *testing.T) {
87+
// A regular file is not a character device.
88+
regular, err := os.CreateTemp(t.TempDir(), "stream")
89+
if err != nil {
90+
t.Fatalf("CreateTemp: %v", err)
91+
}
92+
t.Cleanup(func() { regular.Close() })
93+
94+
// Pipe ends are not character devices either (they model the captured-output
95+
// case: stdout wired to a pipe under `$(...)`).
96+
pr, pw, err := os.Pipe()
97+
if err != nil {
98+
t.Fatalf("os.Pipe: %v", err)
99+
}
100+
t.Cleanup(func() { pr.Close(); pw.Close() })
101+
102+
// /dev/null IS a character device — a portable stand-in for a terminal here.
103+
// The "char-device stdin, pipe stdout" case is the exact regression: the old
104+
// stdin-only gate returned true (→ -t → kubectl < 1.36 panic); the two-stream
105+
// gate must return false. Guarded so the table degrades gracefully if /dev/null
106+
// is somehow unavailable.
107+
devNull, dnErr := os.Open(os.DevNull)
108+
if dnErr == nil {
109+
t.Cleanup(func() { devNull.Close() })
110+
if !isCharDevice(devNull) {
111+
t.Fatalf("expected %s to be a character device", os.DevNull)
112+
}
113+
}
114+
115+
type ttyCase struct {
116+
name string
117+
in *os.File
118+
out *os.File
119+
want bool
120+
}
121+
tests := []ttyCase{
122+
{"both pipes", pr, pw, false},
123+
{"stdin pipe, stdout regular file", pr, regular, false},
124+
{"stdin regular file, stdout pipe", regular, pw, false},
125+
{"nil stdin", nil, pw, false},
126+
{"nil stdout", pr, nil, false},
127+
{"both nil", nil, nil, false},
128+
}
129+
if devNull != nil {
130+
tests = append(tests,
131+
// Regression: terminal stdin + pipe stdout must NOT request a TTY.
132+
ttyCase{"char-device stdin, pipe stdout (regression)", devNull, pw, false},
133+
// Both terminals is the only true case.
134+
ttyCase{"both char devices", devNull, devNull, true},
135+
)
136+
}
137+
138+
for _, tc := range tests {
139+
t.Run(tc.name, func(t *testing.T) {
140+
if got := streamsSupportTTY(tc.in, tc.out); got != tc.want {
141+
t.Fatalf("streamsSupportTTY(%s) = %v, want %v", tc.name, got, tc.want)
142+
}
143+
})
144+
}
145+
}
146+
147+
// TestExecInPod_NoTTYWhenStdoutPiped is the end-to-end guarantee: when this test
148+
// process runs (under `go test`, stdout is captured, not a terminal),
149+
// shouldRequestTTY must be false so BuildExecArgs omits -t.
150+
func TestExecInPod_NoTTYWhenStdoutPiped(t *testing.T) {
151+
if shouldRequestTTY() {
152+
t.Skip("test stdout is a terminal; the piped-output gate cannot be exercised here")
153+
}
154+
args := BuildExecArgs(Hermes, DefaultInstanceID, []string{"true"}, shouldRequestTTY())
155+
for _, a := range args {
156+
if a == "-t" {
157+
t.Fatalf("BuildExecArgs unexpectedly contains -t when stdout is not a terminal: %#v", args)
158+
}
159+
}
160+
}
161+
80162
func TestExecInPod_EmptyArgvRejected(t *testing.T) {
81163
cfg := &config.Config{ConfigDir: t.TempDir(), BinDir: t.TempDir()}
82164
err := ExecInPod(cfg, Hermes, DefaultInstanceID, nil)

0 commit comments

Comments
 (0)