Skip to content

feat(agent): wire up agentssh server to allow exec into container #16638

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Feb 26, 2025
9 changes: 9 additions & 0 deletions agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ type Options struct {
BlockFileTransfer bool
Execer agentexec.Execer
ContainerLister agentcontainers.Lister

ExperimentalContainersEnabled bool
}

type Client interface {
Expand Down Expand Up @@ -188,6 +190,8 @@ func New(options Options) Agent {
metrics: newAgentMetrics(prometheusRegistry),
execer: options.Execer,
lister: options.ContainerLister,

experimentalDevcontainersEnabled: options.ExperimentalContainersEnabled,
}
// Initially, we have a closed channel, reflecting the fact that we are not initially connected.
// Each time we connect we replace the channel (while holding the closeMutex) with a new one
Expand Down Expand Up @@ -258,6 +262,8 @@ type agent struct {
metrics *agentMetrics
execer agentexec.Execer
lister agentcontainers.Lister

experimentalDevcontainersEnabled bool
}

func (a *agent) TailnetConn() *tailnet.Conn {
Expand Down Expand Up @@ -297,6 +303,9 @@ func (a *agent) init() {
a.sshServer,
a.metrics.connectionsTotal, a.metrics.reconnectingPTYErrors,
a.reconnectingPTYTimeout,
func(s *reconnectingpty.Server) {
s.ExperimentalContainersEnabled = a.experimentalDevcontainersEnabled
},
)
go a.runLoop()
}
Expand Down
78 changes: 75 additions & 3 deletions agent/agent_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,24 +25,28 @@ import (
"testing"
"time"

"go.uber.org/goleak"
"tailscale.com/net/speedtest"
"tailscale.com/tailcfg"

"github.com/bramvdbogaerde/go-scp"
"github.com/google/uuid"
"github.com/ory/dockertest/v3"
"github.com/ory/dockertest/v3/docker"
"github.com/pion/udp"
"github.com/pkg/sftp"
"github.com/prometheus/client_golang/prometheus"
promgo "github.com/prometheus/client_model/go"
"github.com/spf13/afero"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/goleak"
"golang.org/x/crypto/ssh"
"golang.org/x/exp/slices"
"golang.org/x/xerrors"
"tailscale.com/net/speedtest"
"tailscale.com/tailcfg"

"cdr.dev/slog"
"cdr.dev/slog/sloggers/slogtest"

"github.com/coder/coder/v2/agent"
"github.com/coder/coder/v2/agent/agentssh"
"github.com/coder/coder/v2/agent/agenttest"
Expand Down Expand Up @@ -1761,6 +1765,74 @@ func TestAgent_ReconnectingPTY(t *testing.T) {
}
}

// This tests end-to-end functionality of connecting to a running container
// and executing a command. It creates a real Docker container and runs a
// command. As such, it does not run by default in CI.
// You can run it manually as follows:
//
// CODER_TEST_USE_DOCKER=1 go test -count=1 ./agent -run TestAgent_ReconnectingPTYContainer
func TestAgent_ReconnectingPTYContainer(t *testing.T) {
t.Parallel()
if os.Getenv("CODER_TEST_USE_DOCKER") != "1" {
t.Skip("Set CODER_TEST_USE_DOCKER=1 to run this test")
}

ctx := testutil.Context(t, testutil.WaitLong)

pool, err := dockertest.NewPool("")
require.NoError(t, err, "Could not connect to docker")
ct, err := pool.RunWithOptions(&dockertest.RunOptions{
Repository: "busybox",
Tag: "latest",
Cmd: []string{"sleep", "infnity"},
}, func(config *docker.HostConfig) {
config.AutoRemove = true
config.RestartPolicy = docker.RestartPolicy{Name: "no"}
})
require.NoError(t, err, "Could not start container")
t.Cleanup(func() {
err := pool.Purge(ct)
require.NoError(t, err, "Could not stop container")
})
// Wait for container to start
require.Eventually(t, func() bool {
ct, ok := pool.ContainerByName(ct.Container.Name)
return ok && ct.Container.State.Running
}, testutil.WaitShort, testutil.IntervalSlow, "Container did not start in time")

// nolint: dogsled
conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{}, 0, func(_ *agenttest.Client, o *agent.Options) {
o.ExperimentalContainersEnabled = true
})
ac, err := conn.ReconnectingPTY(ctx, uuid.New(), 80, 80, "/bin/sh", func(arp *workspacesdk.AgentReconnectingPTYInit) {
arp.Container = ct.Container.ID
})
require.NoError(t, err, "failed to create ReconnectingPTY")
defer ac.Close()
tr := testutil.NewTerminalReader(t, ac)

require.NoError(t, tr.ReadUntil(ctx, func(line string) bool {
return strings.Contains(line, "#") || strings.Contains(line, "$")
}), "find prompt")

require.NoError(t, json.NewEncoder(ac).Encode(workspacesdk.ReconnectingPTYRequest{
Data: "hostname\r",
}), "write hostname")
require.NoError(t, tr.ReadUntil(ctx, func(line string) bool {
return strings.Contains(line, "hostname")
}), "find hostname command")

require.NoError(t, tr.ReadUntil(ctx, func(line string) bool {
return strings.Contains(line, ct.Container.Config.Hostname)
}), "find hostname output")
require.NoError(t, json.NewEncoder(ac).Encode(workspacesdk.ReconnectingPTYRequest{
Data: "exit\r",
}), "write exit command")

// Wait for the connection to close.
require.ErrorIs(t, tr.ReadUntil(ctx, nil), io.EOF)
}

func TestAgent_Dial(t *testing.T) {
t.Parallel()

Expand Down
20 changes: 4 additions & 16 deletions agent/agentcontainers/containers_dockercli.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"context"
"encoding/json"
"fmt"
"os"
"os/user"
"slices"
"sort"
Expand All @@ -15,6 +14,7 @@ import (
"time"

"github.com/coder/coder/v2/agent/agentexec"
"github.com/coder/coder/v2/agent/usershell"
"github.com/coder/coder/v2/codersdk"

"golang.org/x/exp/maps"
Expand All @@ -37,6 +37,7 @@ func NewDocker(execer agentexec.Execer) Lister {
// DockerEnvInfoer is an implementation of agentssh.EnvInfoer that returns
// information about a container.
type DockerEnvInfoer struct {
usershell.SystemEnvInfo
container string
user *user.User
userShell string
Expand Down Expand Up @@ -122,26 +123,13 @@ func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerU
return &dei, nil
}

func (dei *DockerEnvInfoer) CurrentUser() (*user.User, error) {
func (dei *DockerEnvInfoer) User() (*user.User, error) {
// Clone the user so that the caller can't modify it
u := *dei.user
return &u, nil
}

func (*DockerEnvInfoer) Environ() []string {
// Return a clone of the environment so that the caller can't modify it
return os.Environ()
}

func (*DockerEnvInfoer) UserHomeDir() (string, error) {
// We default the working directory of the command to the user's home
// directory. Since this came from inside the container, we cannot guarantee
// that this exists on the host. Return the "real" home directory of the user
// instead.
return os.UserHomeDir()
}

Comment on lines -131 to -143
Copy link
Member Author

Choose a reason for hiding this comment

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

review: we now use the embedded usershell.SystemEnvInfo instead

func (dei *DockerEnvInfoer) UserShell(string) (string, error) {
func (dei *DockerEnvInfoer) Shell(string) (string, error) {
return dei.userShell, nil
}

Expand Down
6 changes: 3 additions & 3 deletions agent/agentcontainers/containers_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -502,15 +502,15 @@ func TestDockerEnvInfoer(t *testing.T) {
dei, err := EnvInfo(ctx, agentexec.DefaultExecer, ct.Container.ID, tt.containerUser)
require.NoError(t, err, "Expected no error from DockerEnvInfo()")

u, err := dei.CurrentUser()
u, err := dei.User()
require.NoError(t, err, "Expected no error from CurrentUser()")
require.Equal(t, tt.expectedUsername, u.Username, "Expected username to match")

hd, err := dei.UserHomeDir()
hd, err := dei.HomeDir()
require.NoError(t, err, "Expected no error from UserHomeDir()")
require.NotEmpty(t, hd, "Expected user homedir to be non-empty")

sh, err := dei.UserShell(tt.containerUser)
sh, err := dei.Shell(tt.containerUser)
require.NoError(t, err, "Expected no error from UserShell()")
require.Equal(t, tt.expectedUserShell, sh, "Expected user shell to match")

Expand Down
66 changes: 19 additions & 47 deletions agent/agentssh/agentssh.go
Original file line number Diff line number Diff line change
Expand Up @@ -698,63 +698,24 @@ func (s *Server) sftpHandler(logger slog.Logger, session ssh.Session) {
_ = session.Exit(1)
}

// EnvInfoer encapsulates external information required by CreateCommand.
type EnvInfoer interface {
// CurrentUser returns the current user.
CurrentUser() (*user.User, error)
// Environ returns the environment variables of the current process.
Environ() []string
// UserHomeDir returns the home directory of the current user.
UserHomeDir() (string, error)
// UserShell returns the shell of the given user.
UserShell(username string) (string, error)
}

type systemEnvInfoer struct{}

var defaultEnvInfoer EnvInfoer = &systemEnvInfoer{}

// DefaultEnvInfoer returns a default implementation of
// EnvInfoer. This reads information using the default Go
// implementations.
func DefaultEnvInfoer() EnvInfoer {
return defaultEnvInfoer
}

func (systemEnvInfoer) CurrentUser() (*user.User, error) {
return user.Current()
}

func (systemEnvInfoer) Environ() []string {
return os.Environ()
}

func (systemEnvInfoer) UserHomeDir() (string, error) {
return userHomeDir()
}

func (systemEnvInfoer) UserShell(username string) (string, error) {
return usershell.Get(username)
}

Comment on lines -701 to -739
Copy link
Member Author

Choose a reason for hiding this comment

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

review: moved to usershell package

// CreateCommand processes raw command input with OpenSSH-like behavior.
// If the script provided is empty, it will default to the users shell.
// This injects environment variables specified by the user at launch too.
// The final argument is an interface that allows the caller to provide
// alternative implementations for the dependencies of CreateCommand.
// This is useful when creating a command to be run in a separate environment
// (for example, a Docker container). Pass in nil to use the default.
func (s *Server) CreateCommand(ctx context.Context, script string, env []string, deps EnvInfoer) (*pty.Cmd, error) {
if deps == nil {
deps = DefaultEnvInfoer()
func (s *Server) CreateCommand(ctx context.Context, script string, env []string, ei usershell.EnvInfoer) (*pty.Cmd, error) {
if ei == nil {
ei = &usershell.SystemEnvInfo{}
}
currentUser, err := deps.CurrentUser()
currentUser, err := ei.User()
if err != nil {
return nil, xerrors.Errorf("get current user: %w", err)
}
username := currentUser.Username

shell, err := deps.UserShell(username)
shell, err := ei.Shell(username)
if err != nil {
return nil, xerrors.Errorf("get user shell: %w", err)
}
Expand Down Expand Up @@ -802,21 +763,32 @@ func (s *Server) CreateCommand(ctx context.Context, script string, env []string,
}
}

cmd := s.Execer.PTYCommandContext(ctx, name, args...)
// Modify command prior to execution. This will usually be a no-op, but not
// always. For example, to run a command in a Docker container, we need to
// modify the command to be `docker exec -it <container> <command>`.
modifiedName, modifiedArgs := ei.ModifyCommand(name, args...)
// Log if the command was modified.
if modifiedName != name && slices.Compare(modifiedArgs, args) != 0 {
s.logger.Debug(ctx, "modified command",
slog.F("before", append([]string{name}, args...)),
slog.F("after", append([]string{modifiedName}, modifiedArgs...)),
)
}
cmd := s.Execer.PTYCommandContext(ctx, modifiedName, modifiedArgs...)
Copy link
Member

Choose a reason for hiding this comment

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

Didn't we also have a docker execer? Did we end up not implementing that change, and are only relying on env infoer now? Or are we modifying commands from multiple angles? If former, all good, if latter, I'm wondering if we could unify the concepts (perhaps infoer can be applied on the execer level or vice-versa).

Copy link
Member Author

Choose a reason for hiding this comment

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

That's all in the envinfoer now.

cmd.Dir = s.config.WorkingDirectory()

// If the metadata directory doesn't exist, we run the command
// in the users home directory.
_, err = os.Stat(cmd.Dir)
if cmd.Dir == "" || err != nil {
// Default to user home if a directory is not set.
homedir, err := deps.UserHomeDir()
homedir, err := ei.HomeDir()
if err != nil {
return nil, xerrors.Errorf("get home dir: %w", err)
}
cmd.Dir = homedir
}
cmd.Env = append(deps.Environ(), env...)
cmd.Env = append(ei.Environ(), env...)
cmd.Env = append(cmd.Env, fmt.Sprintf("USER=%s", username))

// Set SSH connection environment variables (these are also set by OpenSSH
Expand Down
10 changes: 7 additions & 3 deletions agent/agentssh/agentssh_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,22 +124,26 @@ type fakeEnvInfoer struct {
UserShellFn func(string) (string, error)
}

func (f *fakeEnvInfoer) CurrentUser() (u *user.User, err error) {
func (f *fakeEnvInfoer) User() (u *user.User, err error) {
return f.CurrentUserFn()
}

func (f *fakeEnvInfoer) Environ() []string {
return f.EnvironFn()
}

func (f *fakeEnvInfoer) UserHomeDir() (string, error) {
func (f *fakeEnvInfoer) HomeDir() (string, error) {
return f.UserHomeDirFn()
}

func (f *fakeEnvInfoer) UserShell(u string) (string, error) {
func (f *fakeEnvInfoer) Shell(u string) (string, error) {
return f.UserShellFn(u)
}

func (*fakeEnvInfoer) ModifyCommand(cmd string, args ...string) (string, []string) {
return cmd, args
}

func TestNewServer_CloseActiveConnections(t *testing.T) {
t.Parallel()

Expand Down
Loading
Loading