fix(podshell): select a container so multi-container pods can open a shell - #581
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the pod shell manager to support selecting a specific container to exec into, mirroring kubectl's container resolution behavior (explicit choice, default-container annotation, or first container in spec order). It also returns the list of available containers in the response to enable container switching without additional API calls. The review feedback suggests optimizing the waitRunning function by performing an initial check before allocating a ticker, and adding a test case to explicitly verify that init containers are ignored during container resolution.
…shell
Opening a shell on a pod with more than one container failed immediately
with:
[stream closed: unable to upgrade connection: a container name must be
specified for pod app-dev-..., choose one of: [nextjs-static-init app nginx]]
`openSession` built PodExecOptions without a Container, so the API server
had nothing to attach to. Single-container pods worked only because the
API server can infer the target there. The sibling exec call sites in
pkg/podexec (executor.go, profiler.go) both already pass Container —
podshell was the outlier.
Resolve one the way kubectl does: an explicitly requested container wins,
then the kubectl.kubernetes.io/default-container annotation, then the first
container in spec order. A requested name is validated against the pod
first, since an unknown container otherwise fails deep inside the stream
with a message the user cannot act on. A stale annotation pointing at a
removed container falls back rather than breaking the shell.
Init containers are excluded — they have already terminated by the time the
pod is Running, so offering them only produces a confusing failure.
Request gains an optional `container` field, and start now reports both the
container it attached to and the full list. That lets the UI offer a
container switcher without another API call, and without a second agent
rollout — which is the slow part for on-prem installs. Omitting the field
keeps the existing behaviour, so older clients are unaffected.
waitRunning now returns the pod it already fetched, so resolution costs no
extra API round trip.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
749680e to
4750c7e
Compare
|
Thanks — one taken, one declined. Init-container test — added. Fair point: I asserted the exclusion in a comment and in the PR description but never locked it down.
for {
pod, err := m.cs.CoreV1().Pods(ns).Get(cctx, name, metav1.GetOptions{})
if err == nil && pod.Status.Phase == corev1.PodRunning {
return pod, nil
}
select { case <-cctx.Done(): ...; case <-t.C: }
}So an already-Running pod returns after exactly one API call with no scheduling delay — the happy path is not paying a tick. The only saving would be one
|
|
📦 Image Tags Updated |
|
📦 Image Tags Updated |
Summary
Opening the in-app shell on a pod with more than one container fails immediately:
openSessionbuiltPodExecOptionswith noContainer, so the API server had nothing to attach to and refused the SPDY upgrade. Single-container pods worked only because the API server can infer the target in that case — which is why this went unnoticed.Worth noting the sibling exec call sites,
pkg/podexec/executor.go:87andpkg/podexec/profiler.go:606, both already passContainer.podshellwas the one outlier, not a pattern the codebase gets wrong.What changed
resolveContainerpicks the target the waykubectldoes:kubectl.kubernetes.io/default-containerannotation, so the in-app shell lands wherekubectl execon the same pod would;A stale annotation naming a removed container falls back rather than breaking the shell.
Init containers are deliberately excluded. They have already terminated by the time a pod is
Running, so offering them would only produce a confusing failure — relevant here, since the reported pod'snextjs-static-initwould otherwise be a candidate.waitRunningnow returns the pod it already fetched, so resolution costs no extra API round trip.Wire changes (both backward compatible)
Requestgains optionalcontainer. Omitting it keeps today's behaviour, so existing clients are unaffected — this is also the compatibility floor: the deployed UI sends no container, so a fix that required one would break every current client.startresponses now carrycontainer(what it attached to) andcontainers(what else it could have). This is deliberately slightly ahead of the UI: it lets a container switcher ship later with no second agent rollout, which is the slow part for on-prem installs. Both fields areomitemptyand ignored by current clients. Happy to drop them if you'd rather keep the surface minimal.Type of change
Chart version
Test plan
go test ./pkg/podshell/...— passes, including 7 new table cases covering single-container, multi-container spec order, annotation precedence, stale annotation fallback, explicit request, unknown request (asserts the error lists the valid names), and the empty-container guard.make fmtclean,go vet ./pkg/podshell/...clean, fullmake testgreen across all packages.make lintcould not be run locally — the installedgolangci-lint2.8.0 is built with go1.25.5 while the module targets go1.26.3, so it refuses withcan't load config. This is a stale local toolchain, not a finding; CI runs the pinned linter.kubectl.kubernetes.io/default-containerlands in the annotated container.Related issues
Reported against the in-app pod terminal, alongside nudgebee-enterprise#36589 (terminal failing to open) and nudgebee-enterprise#36642 (typing latency).
🤖 Generated with Claude Code