Skip to content

Commit 7437e49

Browse files
johnstcnjohnstcn
committed
address PR comments
1 parent 56538a9 commit 7437e49

File tree

8 files changed

+88
-57
lines changed

8 files changed

+88
-57
lines changed

‎agent/agent.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,8 @@ type Options struct {
9191
Execer agentexec.Execer
9292
ContainerLister agentcontainers.Lister
9393

94-
ExperimentalContainersEnabled bool
95-
ExperimentalConnectionReports bool
94+
ExperimentalConnectionReports bool
95+
ExperimentalDevcontainersEnabled bool
9696
}
9797

9898
type Client interface {
@@ -156,7 +156,7 @@ func New(options Options) Agent {
156156
options.Execer = agentexec.DefaultExecer
157157
}
158158
if options.ContainerLister == nil {
159-
options.ContainerLister = agentcontainers.NewDocker(options.Execer)
159+
options.ContainerLister = agentcontainers.NoopLister{}
160160
}
161161

162162
hardCtx, hardCancel := context.WithCancel(context.Background())
@@ -195,7 +195,7 @@ func New(options Options) Agent {
195195
execer: options.Execer,
196196
lister: options.ContainerLister,
197197

198-
experimentalDevcontainersEnabled: options.ExperimentalContainersEnabled,
198+
experimentalDevcontainersEnabled: options.ExperimentalDevcontainersEnabled,
199199
experimentalConnectionReports: options.ExperimentalConnectionReports,
200200
}
201201
// Initially, we have a closed channel, reflecting the fact that we are not initially connected.
@@ -308,7 +308,7 @@ func (a *agent) init() {
308308
return a.reportConnection(id, connectionType, ip)
309309
},
310310

311-
ExperimentalContainersEnabled: a.experimentalDevcontainersEnabled,
311+
ExperimentalDevContainersEnabled: a.experimentalDevcontainersEnabled,
312312
})
313313
if err != nil {
314314
panic(err)
@@ -337,7 +337,7 @@ func (a *agent) init() {
337337
a.metrics.connectionsTotal, a.metrics.reconnectingPTYErrors,
338338
a.reconnectingPTYTimeout,
339339
func(s *reconnectingpty.Server) {
340-
s.ExperimentalContainersEnabled = a.experimentalDevcontainersEnabled
340+
s.ExperimentalDevcontainersEnabled = a.experimentalDevcontainersEnabled
341341
},
342342
)
343343
go a.runLoop()

‎agent/agent_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1841,7 +1841,7 @@ func TestAgent_ReconnectingPTYContainer(t *testing.T) {
18411841

18421842
// nolint: dogsled
18431843
conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{}, 0, func(_ *agenttest.Client, o *agent.Options) {
1844-
o.ExperimentalContainersEnabled = true
1844+
o.ExperimentalDevcontainersEnabled = true
18451845
})
18461846
ac, err := conn.ReconnectingPTY(ctx, uuid.New(), 80, 80, "/bin/sh", func(arp *workspacesdk.AgentReconnectingPTYInit) {
18471847
arp.Container = ct.Container.ID

‎agent/agentssh/agentssh.go

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ type Config struct {
115115
ReportConnection reportConnectionFunc
116116
// Experimental: allow connecting to running containers if
117117
// CODER_AGENT_DEVCONTAINERS_ENABLE=true.
118-
ExperimentalContainersEnabled bool
118+
ExperimentalDevContainersEnabled bool
119119
}
120120

121121
type Server struct {
@@ -425,16 +425,19 @@ func (s *Server) sessionHandler(session ssh.Session) {
425425
}
426426

427427
container, containerUser, env := extractContainerInfo(env)
428-
s.logger.Debug(ctx, "container info",
429-
slog.F("container", container),
430-
slog.F("container_user", containerUser),
431-
)
428+
if container != "" {
429+
s.logger.Debug(ctx, "container info",
430+
slog.F("container", container),
431+
slog.F("container_user", containerUser),
432+
)
433+
}
432434

433435
switch ss := session.Subsystem(); ss {
434436
case "":
435437
case "sftp":
436-
if s.config.ExperimentalContainersEnabled && container != "" {
438+
if s.config.ExperimentalDevContainersEnabled && container != "" {
437439
closeCause("sftp not yet supported with containers")
440+
_ = session.Exit(1)
438441
return
439442
}
440443
err := s.sftpHandler(logger, session)
@@ -546,7 +549,7 @@ func (s *Server) sessionStart(logger slog.Logger, session ssh.Session, env []str
546549

547550
var ei usershell.EnvInfoer
548551
var err error
549-
if s.config.ExperimentalContainersEnabled && container != "" {
552+
if s.config.ExperimentalDevContainersEnabled && container != "" {
550553
ei, err = agentcontainers.EnvInfo(ctx, s.Execer, container, containerUser)
551554
if err != nil {
552555
s.metrics.sessionErrors.WithLabelValues(magicTypeLabel, ptyLabel, "container_env_info").Add(1)

‎agent/reconnectingpty/server.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ type Server struct {
3232
reconnectingPTYs sync.Map
3333
timeout time.Duration
3434

35-
ExperimentalContainersEnabled bool
35+
ExperimentalDevcontainersEnabled bool
3636
}
3737

3838
// NewServer returns a new ReconnectingPTY server
@@ -187,7 +187,7 @@ func (s *Server) handleConn(ctx context.Context, logger slog.Logger, conn net.Co
187187
}()
188188

189189
var ei usershell.EnvInfoer
190-
if s.ExperimentalContainersEnabled && msg.Container != "" {
190+
if s.ExperimentalDevcontainersEnabled && msg.Container != "" {
191191
dei, err := agentcontainers.EnvInfo(ctx, s.commandCreator.Execer, msg.Container, msg.ContainerUser)
192192
if err != nil {
193193
return xerrors.Errorf("get container env info: %w", err)

‎cli/agent.go

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -38,24 +38,24 @@ import (
3838

3939
func (r *RootCmd) workspaceAgent() *serpent.Command {
4040
var (
41-
auth string
42-
logDir string
43-
scriptDataDir string
44-
pprofAddress string
45-
noReap bool
46-
sshMaxTimeout time.Duration
47-
tailnetListenPort int64
48-
prometheusAddress string
49-
debugAddress string
50-
slogHumanPath string
51-
slogJSONPath string
52-
slogStackdriverPath string
53-
blockFileTransfer bool
54-
agentHeaderCommand string
55-
agentHeader []string
56-
devcontainersEnabled bool
57-
58-
experimentalConnectionReports bool
41+
auth string
42+
logDir string
43+
scriptDataDir string
44+
pprofAddress string
45+
noReap bool
46+
sshMaxTimeout time.Duration
47+
tailnetListenPort int64
48+
prometheusAddress string
49+
debugAddress string
50+
slogHumanPath string
51+
slogJSONPath string
52+
slogStackdriverPath string
53+
blockFileTransfer bool
54+
agentHeaderCommand string
55+
agentHeader []string
56+
57+
experimentalConnectionReports bool
58+
experimentalDevcontainersEnabled bool
5959
)
6060
cmd := &serpent.Command{
6161
Use: "agent",
@@ -319,7 +319,7 @@ func (r *RootCmd) workspaceAgent() *serpent.Command {
319319
}
320320

321321
var containerLister agentcontainers.Lister
322-
if !devcontainersEnabled {
322+
if !experimentalDevcontainersEnabled {
323323
logger.Info(ctx, "agent devcontainer detection not enabled")
324324
containerLister = &agentcontainers.NoopLister{}
325325
} else {
@@ -358,8 +358,8 @@ func (r *RootCmd) workspaceAgent() *serpent.Command {
358358
Execer: execer,
359359
ContainerLister: containerLister,
360360

361-
ExperimentalContainersEnabled: devcontainersEnabled,
362-
ExperimentalConnectionReports: experimentalConnectionReports,
361+
ExperimentalDevcontainersEnabled: experimentalDevcontainersEnabled,
362+
ExperimentalConnectionReports: experimentalConnectionReports,
363363
})
364364

365365
promHandler := agent.PrometheusMetricsHandler(prometheusRegistry, logger)
@@ -487,7 +487,7 @@ func (r *RootCmd) workspaceAgent() *serpent.Command {
487487
Default: "false",
488488
Env: "CODER_AGENT_DEVCONTAINERS_ENABLE",
489489
Description: "Allow the agent to automatically detect running devcontainers.",
490-
Value: serpent.BoolOf(&devcontainersEnabled),
490+
Value: serpent.BoolOf(&experimentalDevcontainersEnabled),
491491
},
492492
{
493493
Flag: "experimental-connection-reports-enable",

‎cli/exp_rpty_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ func TestExpRpty(t *testing.T) {
8888
})
8989

9090
_ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) {
91-
o.ExperimentalContainersEnabled = true
91+
o.ExperimentalDevcontainersEnabled = true
9292
})
9393
_ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait()
9494

‎cli/ssh.go

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ func (r *RootCmd) ssh() *serpent.Command {
7878
networkInfoDir string
7979
networkInfoInterval time.Duration
8080

81-
container string
81+
containerName string
8282
containerUser string
8383
)
8484
client := new(codersdk.Client)
@@ -286,20 +286,31 @@ func (r *RootCmd) ssh() *serpent.Command {
286286
}
287287
conn.AwaitReachable(ctx)
288288

289-
if container != "" {
289+
if containerName != "" {
290290
cts, err := client.WorkspaceAgentListContainers(ctx, workspaceAgent.ID, nil)
291291
if err != nil {
292292
return xerrors.Errorf("list containers: %w", err)
293293
}
294+
if len(cts.Containers) == 0 {
295+
cliui.Info(inv.Stderr, "No containers found!")
296+
cliui.Info(inv.Stderr, "Tip: Agent container integration is experimental and not enabled by default.")
297+
cliui.Info(inv.Stderr, " To enable it, set CODER_AGENT_DEVCONTAINERS_ENABLE=true in your template.")
298+
return nil
299+
}
294300
var found bool
295301
for _, c := range cts.Containers {
296-
if c.FriendlyName == container || c.ID == container {
302+
if c.FriendlyName == containerName || c.ID == containerName {
297303
found = true
298304
break
299305
}
300306
}
301307
if !found {
302-
return xerrors.Errorf("container not found: %q", container)
308+
availableContainers := make([]string, len(cts.Containers))
309+
for i, c := range cts.Containers {
310+
availableContainers[i] = c.FriendlyName
311+
}
312+
cliui.Errorf(inv.Stderr, "Container not found: %q\nAvailable containers: %v", containerName, availableContainers)
313+
return nil
303314
}
304315
}
305316

@@ -475,9 +486,9 @@ func (r *RootCmd) ssh() *serpent.Command {
475486
}
476487
}
477488

478-
if container != "" {
489+
if containerName != "" {
479490
for k, v := range map[string]string{
480-
agentssh.ContainerEnvironmentVariable: container,
491+
agentssh.ContainerEnvironmentVariable: containerName,
481492
agentssh.ContainerUserEnvironmentVariable: containerUser,
482493
} {
483494
if err := sshSession.Setenv(k, v); err != nil {
@@ -630,15 +641,14 @@ func (r *RootCmd) ssh() *serpent.Command {
630641
Flag: "container",
631642
FlagShorthand: "c",
632643
Description: "Specifies a container inside the workspace to connect to.",
633-
Value: serpent.StringOf(&container),
644+
Value: serpent.StringOf(&containerName),
634645
Hidden: true, // Hidden until this features is at least in beta.
635646
},
636647
{
637-
Flag: "container-user",
638-
FlagShorthand: "u",
639-
Description: "When connecting to a container, specifies the user to connect as.",
640-
Value: serpent.StringOf(&containerUser),
641-
Hidden: true, // Hidden until this features is at least in beta.
648+
Flag: "container-user",
649+
Description: "When connecting to a container, specifies the user to connect as.",
650+
Value: serpent.StringOf(&containerUser),
651+
Hidden: true, // Hidden until this features is at least in beta.
642652
},
643653
sshDisableAutostartOption(serpent.BoolOf(&disableAutostart)),
644654
}

‎cli/ssh_test.go

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import (
3535
"golang.org/x/xerrors"
3636

3737
"github.com/coder/coder/v2/agent"
38+
"github.com/coder/coder/v2/agent/agentcontainers"
3839
"github.com/coder/coder/v2/agent/agentssh"
3940
"github.com/coder/coder/v2/agent/agenttest"
4041
agentproto "github.com/coder/coder/v2/agent/proto"
@@ -1959,7 +1960,8 @@ func TestSSH_Container(t *testing.T) {
19591960
})
19601961

19611962
_ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) {
1962-
o.ExperimentalContainersEnabled = true
1963+
o.ExperimentalDevcontainersEnabled = true
1964+
o.ContainerLister = agentcontainers.NewDocker(o.Execer)
19631965
})
19641966
_ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait()
19651967

@@ -1985,14 +1987,22 @@ func TestSSH_Container(t *testing.T) {
19851987
ctx := testutil.Context(t, testutil.WaitShort)
19861988
client, workspace, agentToken := setupWorkspaceForAgent(t)
19871989
_ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) {
1988-
o.ExperimentalContainersEnabled = true
1990+
o.ExperimentalDevcontainersEnabled = true
1991+
o.ContainerLister = agentcontainers.NewDocker(o.Execer)
19891992
})
19901993
_ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait()
19911994

19921995
inv, root := clitest.New(t, "ssh", workspace.Name, "-c", uuid.NewString())
19931996
clitest.SetupConfig(t, client, root)
1994-
err := inv.WithContext(ctx).Run()
1995-
require.ErrorContains(t, err, "container not found:")
1997+
ptty := ptytest.New(t).Attach(inv)
1998+
1999+
cmdDone := tGo(t, func() {
2000+
err := inv.WithContext(ctx).Run()
2001+
assert.NoError(t, err)
2002+
})
2003+
2004+
ptty.ExpectMatch("Container not found:")
2005+
<-cmdDone
19962006
})
19972007

19982008
t.Run("NotEnabled", func(t *testing.T) {
@@ -2005,8 +2015,16 @@ func TestSSH_Container(t *testing.T) {
20052015

20062016
inv, root := clitest.New(t, "ssh", workspace.Name, "-c", uuid.NewString())
20072017
clitest.SetupConfig(t, client, root)
2008-
err := inv.WithContext(ctx).Run()
2009-
require.ErrorContains(t, err, "container not found:")
2018+
ptty := ptytest.New(t).Attach(inv)
2019+
2020+
cmdDone := tGo(t, func() {
2021+
err := inv.WithContext(ctx).Run()
2022+
assert.NoError(t, err)
2023+
})
2024+
2025+
ptty.ExpectMatch("No containers found!")
2026+
ptty.ExpectMatch("Tip: Agent container integration is experimental and not enabled by default.")
2027+
<-cmdDone
20102028
})
20112029
}
20122030

0 commit comments

Comments
 (0)