Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions api/v1/container_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,15 @@ type ContainerSpec struct {
// PEM formatted public certificates to be created in the container
// +optional
PemCertificates *ContainerPemCertificates `json:"pemCertificates,omitempty"`

// Optional terminal/PTY configuration. When set and Enabled is true, the
// container's primary process is started under a host pseudo-terminal
// and its stdin/stdout/stderr are bridged to the configured UDS via
// HMP v1, instead of the container being run detached with separate log
// capture. Terminal mode requires Windows on the host (ConPTY) for the
// initial slice; non-Windows hosts return ErrTerminalNotSupported at
// startup.
Terminal *TerminalSpec `json:"terminal,omitempty"`
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be included in the validation (for both initial object creation and object update) and in the lifecycle key calculation.

}

func (cs *ContainerSpec) Equal(other *ContainerSpec) bool {
Expand Down Expand Up @@ -791,6 +800,10 @@ func (cs *ContainerSpec) Equal(other *ContainerSpec) bool {
return false
}

if !cs.Terminal.Equal(other.Terminal) {
return false
}

return true
}

Expand Down
13 changes: 13 additions & 0 deletions api/v1/executable_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,13 @@ type ExecutableSpec struct {
// PEM formatted certificates to be written for the Executable
// +optional
PemCertificates *ExecutablePemCertificates `json:"pemCertificates,omitempty"`

// Terminal, when non-nil and Enabled, allocates a pseudo-terminal for the
// Executable's process and exposes an HMP v1 producer endpoint that the
// Aspire terminal host connects to as a client. See TerminalSpec for
// details.
// +optional
Terminal *TerminalSpec `json:"terminal,omitempty"`
}

func (es ExecutableSpec) Equal(other ExecutableSpec) bool {
Expand Down Expand Up @@ -314,6 +321,10 @@ func (es ExecutableSpec) Equal(other ExecutableSpec) bool {
return false
}

if !es.Terminal.Equal(other.Terminal) {
return false
}

return true
}

Expand Down Expand Up @@ -355,6 +366,8 @@ func (es ExecutableSpec) Validate(specPath *field.Path) field.ErrorList {

errorList = append(errorList, es.PemCertificates.Validate(specPath.Child("pemCertificates"))...)

errorList = append(errorList, es.Terminal.Validate(specPath.Child("terminal"))...)

return errorList
}

Expand Down
94 changes: 94 additions & 0 deletions api/v1/terminal_types.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See LICENSE in the project root for license information.
*--------------------------------------------------------------------------------------------*/

package v1

import (
"k8s.io/apimachinery/pkg/util/validation/field"
)

// TerminalSpec configures pseudo-terminal allocation for an Executable or
// Container replica and the HMP v1 producer endpoint that the Aspire terminal
// host connects to as a client.
//
// When Enabled is true, DCP allocates a PTY for the underlying process and
// listens on UDSPath (a Unix Domain Socket path on Linux/macOS, or a named pipe
// path on Windows in a follow-up). When the terminal host opens an HMP v1
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not Unix domain sockets on Windows? We use them for pumping DCP logs into Aspire dashboard already...

// connection, DCP starts an HMP v1 server on the connection and bridges:
//
// - PTY output (from the process's tty) -> HMP v1 Output frames
// - HMP v1 Input frames -> PTY input (process stdin)
// - HMP v1 Resize frames -> PTY resize (TIOCSWINSZ / ResizePseudoConsole)
// - Process exit -> HMP v1 Exit frame, then close
//
// The HMP v1 wire format is defined by the Aspire dashboard's terminal host
// (see Hex1b's Hmp1Protocol). DCP's responsibility is limited to PTY
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a URL to the HMP spec here.

// allocation, the listener, and frame translation.
type TerminalSpec struct {
// Enabled controls whether DCP allocates a PTY for the process and exposes
// an HMP v1 producer endpoint at UDSPath.
Enabled bool `json:"enabled,omitempty"`
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this flag? What does it mean to have TerminalSpec present in an Executable or Controller spec (*Terminal is not nil) but Enabled is false? Is this a valid spec and if so, what is the semantics?


// UDSPath is the Unix Domain Socket path that DCP listens on for the
// terminal host's HMP v1 client connection. Required when Enabled is true.
UDSPath string `json:"udsPath,omitempty"`
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to support changing UDSPath for existing Container or Executable? If yes, we should handle it in the controller(s). If not we should prevent that change via validation.


// Cols is the initial width of the pseudo-terminal in character columns.
// If zero, a sensible default (80) is used.
// +kubebuilder:default:=0
Cols int32 `json:"cols,omitempty"`

// Rows is the initial height of the pseudo-terminal in character rows.
// If zero, a sensible default (24) is used.
// +kubebuilder:default:=0
Rows int32 `json:"rows,omitempty"`
}

// Equal reports whether two TerminalSpec values are equal.
func (ts *TerminalSpec) Equal(other *TerminalSpec) bool {
if ts == other {
return true
}
if ts == nil || other == nil {
return false
}
return ts.Enabled == other.Enabled &&
ts.UDSPath == other.UDSPath &&
ts.Cols == other.Cols &&
ts.Rows == other.Rows
}

// Validate verifies the TerminalSpec content.
func (ts *TerminalSpec) Validate(specPath *field.Path) field.ErrorList {
errorList := field.ErrorList{}
if ts == nil {
return errorList
}
if ts.Enabled && ts.UDSPath == "" {
errorList = append(errorList, field.Invalid(specPath.Child("udsPath"), ts.UDSPath, "udsPath is required when Enabled is true."))
}
if ts.Cols < 0 {
errorList = append(errorList, field.Invalid(specPath.Child("cols"), ts.Cols, "cols must be non-negative."))
}
if ts.Rows < 0 {
errorList = append(errorList, field.Invalid(specPath.Child("rows"), ts.Rows, "rows must be non-negative."))
}
return errorList
}

// DeepCopyInto copies the receiver, writing into out.
func (in *TerminalSpec) DeepCopyInto(out *TerminalSpec) {
*out = *in
}

// DeepCopy returns a deep copy of the TerminalSpec.
func (in *TerminalSpec) DeepCopy() *TerminalSpec {
if in == nil {
return nil
}
out := new(TerminalSpec)
in.DeepCopyInto(out)
return out
}
10 changes: 10 additions & 0 deletions api/v1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

59 changes: 59 additions & 0 deletions controllers/container_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,10 @@
rcd.containerState = apiv1.ContainerStateRunning
rcd.startAttemptFinishedAt = metav1.NowMicro()
change |= statusChanged

if termErr := r.ensureContainerTerminalSession(ctx, rcd, log); termErr != nil {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No this is both not the right place and unnecessary to do the terminal hookup here. It should be only done as part of startContainerWithOrchestrator() (which the PR already does). Just delete this chunk.

log.Error(termErr, "Failed to attach terminal session to running container; container is running but interactive terminal will be unavailable")
}
case inspected != nil && inspected.Status == containers.ContainerStatusExited:
rcd.containerState = apiv1.ContainerStateExited
rcd.startAttemptFinishedAt = metav1.NowMicro()
Expand Down Expand Up @@ -1481,6 +1485,13 @@
if inspected.Status == containers.ContainerStatusRunning {
log.V(1).Info("Container started")
rcd.containerState = apiv1.ContainerStateRunning

if err := r.ensureContainerTerminalSession(startupCtx, rcd, log); err != nil {

Check failure on line 1489 in controllers/container_controller.go

View workflow job for this annotation

GitHub Actions / Lint ubuntu-latest

shadow: declaration of "err" shadows declaration at line 1121 (govet)
// Terminal attach failure is non-fatal: the container is
// running and observable via the orchestrator, but no
// interactive PTY is available. We log and continue.
log.Error(err, "Failed to attach terminal session to running container; container is running but interactive terminal will be unavailable")
}
} else {
log.V(1).Info("Container started and exited shortly after", "ContainerStatus", inspected.Status)
rcd.containerState = apiv1.ContainerStateExited
Expand Down Expand Up @@ -1557,6 +1568,7 @@
// or if the container has already finished starting/stopping and we know the outcome of either.

defer rcd.deleteStartupLogFiles(log)
defer rcd.closeTerminalSession(log)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the terminal supposed to be working if the container is stopped/exited? If not, this probably belongs to stopContainerIfNecessary()


if container.Spec.Persistent {
log.V(1).Info("Container is not using Managed mode, leaving underlying resources")
Expand Down Expand Up @@ -1588,6 +1600,53 @@
r.ReleaseContainerWatchForResource(container.UID, log)
}

// ensureContainerTerminalSession attaches a host-side PTY to the running
// container and starts the HMP v1 listener at the configured UDS path,
// storing the resulting session on rcd. No-op if the container does not
// have terminal enabled or a session is already active.
//
// The container must have been created with `-t -i` for the attach to
// deliver a usable terminal; that is handled by applyCreateContainerOptions
// in the docker/podman orchestrator when ContainerSpec.Terminal is set.
//
// Errors here are non-fatal to the container lifecycle: the container is
// already running by the time this is called. The caller is expected to
// log the error and move on.
func (r *ContainerReconciler) ensureContainerTerminalSession(
ctx context.Context,
rcd *runningContainerData,
log logr.Logger,
) error {
if rcd == nil || rcd.runSpec == nil {
return nil
}

terminal := rcd.runSpec.Terminal
if terminal == nil || !terminal.Enabled {
return nil
}

if rcd.terminalSession != nil {
return nil
}

if !rcd.hasValidContainerID() {
return errors.New("ensureContainerTerminalSession called without a valid container ID")
}

runner, ok := r.orchestrator.(containers.CLICommandRunner)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just make CLICommandRunner part of ContainerOrchestrator interface so this kind of casting is not necessary.

if !ok {
return fmt.Errorf("container orchestrator %T does not implement containers.CLICommandRunner; terminal attach not supported", r.orchestrator)
}

session, err := startContainerTerminalSession(ctx, runner, string(rcd.containerID), terminal, log)
if err != nil {
return err
}
rcd.terminalSession = session
return nil
}

func (r *ContainerReconciler) startContainerWithTimeout(
parentCtx context.Context,
containerName string,
Expand Down
95 changes: 95 additions & 0 deletions controllers/container_terminal.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See LICENSE in the project root for license information.
*--------------------------------------------------------------------------------------------*/

package controllers

import (
"context"
"fmt"

"github.com/go-logr/logr"

apiv1 "github.com/microsoft/dcp/api/v1"
"github.com/microsoft/dcp/internal/containers"
"github.com/microsoft/dcp/internal/termpty"
)

// startContainerTerminalSession runs the container runtime CLI's `attach`
// command against an already-running container under a host PTY, then
// stands up an HMP v1 listener at spec.UDSPath that bridges viewer
// connections to that PTY. The returned Session owns the lifetime of both
// the CLI process and the listener; callers must Close it during teardown.
//
// We use `<runtime> attach` (not `<runtime> start --attach --interactive`)
// because the container is already started by the time this is called via
// the reconciler's normal `docker container start <id>` path. Running
// `docker start --attach --interactive` against a running container is a
// no-op and would leave the container's primary process with no host-side
// stdin/stdout connection.
//
// `--sig-proxy=false` prevents the attach process from forwarding signals
// (e.g. SIGINT from the dashboard) to the container; signals are delivered
// in-band via the HMP v1 input channel as keystrokes (Ctrl-C → 0x03 byte).
//
// The container must have been created with `-t -i` (allocate TTY + keep
// stdin open) for the attach to deliver a usable terminal; this is handled
// automatically when ContainerSpec.Terminal != nil && Enabled by the docker
// and podman orchestrators' applyCreateContainerOptions helper.
//
// On hosts where DCP does not yet implement PTY allocation (currently
// non-Windows) this returns termpty.ErrTerminalNotSupported.
func startContainerTerminalSession(
ctx context.Context,
runner containers.CLICommandRunner,
containerID string,
spec *apiv1.TerminalSpec,
log logr.Logger,
) (*termpty.Session, error) {
if spec == nil || !spec.Enabled {
return nil, fmt.Errorf("startContainerTerminalSession: spec must be non-nil and Enabled")
}

// Use MakeCommand to extract the configured CLI path (e.g. "docker" or
// "podman", possibly resolved against PATH); we don't actually start the
// command via the orchestrator's process.Executor because we need direct
// ConPTY semantics.
//
// `--detach-keys=""` disables the default Ctrl-P,Ctrl-Q detach sequence
// so those keystrokes are forwarded into the container as plain bytes
// (matching an interactive terminal's expectation that all keys reach
// the application). HMP v1 viewers manage detach themselves.
cmd := runner.MakeCommand("attach", "--sig-proxy=false", "--detach-keys=", containerID)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functionality to create a command to attach to a container should probably go into containers package


commandLine := termpty.BuildWindowsCommandLine(cmd.Path, cmd.Args[1:])
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ehm, not just "Windows" 😄


startLog := log.WithValues(
"Cmd", cmd.Path,
"ContainerID", containerID,
"Terminal", true,
"UDSPath", spec.UDSPath,
)
startLog.Info("Attaching container under PTY...")

tp, err := termpty.StartProcess(ctx, termpty.CommandSpec{
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to put console support into process package. @danegsta has already added some related stuff there.

CommandLine: commandLine,
Cols: int(spec.Cols),
Rows: int(spec.Rows),
})
if err != nil {
return nil, fmt.Errorf("starting container attach under PTY: %w", err)
}

session, err := termpty.StartSession(ctx, termpty.SessionConfig{
UDSPath: spec.UDSPath,
Cols: int(spec.Cols),
Rows: int(spec.Rows),
}, tp, startLog)
if err != nil {
// StartSession is responsible for closing tp.PTY when it fails.
return nil, fmt.Errorf("starting container terminal session listener: %w", err)
}

return session, nil
}
Loading
Loading