Skip to content

Commit 1fc74f6

Browse files
mafredrimafredri
authored
refactor(agent): update agentcontainers api initialization (#17600)
There were too many ways to configure the agentcontainers API resulting in inconsistent behavior or features not being enabled. This refactor introduces a control flag for enabling or disabling the containers API. When disabled, all implementations are no-op and explicit endpoint behaviors are defined. When enabled, concrete implementations are used by default but can be overridden by passing options.
1 parent 22b932a commit 1fc74f6

File tree

12 files changed

+118
-67
lines changed

12 files changed

+118
-67
lines changed

‎agent/agent.go

+10-6
Original file line numberDiff line numberDiff line change
@@ -89,9 +89,9 @@ type Options struct {
8989
ServiceBannerRefreshInterval time.Duration
9090
BlockFileTransfer bool
9191
Execer agentexec.Execer
92-
ContainerLister agentcontainers.Lister
9392

9493
ExperimentalDevcontainersEnabled bool
94+
ContainerAPIOptions []agentcontainers.Option // Enable ExperimentalDevcontainersEnabled for these to be effective.
9595
}
9696

9797
type Client interface {
@@ -154,9 +154,6 @@ func New(options Options) Agent {
154154
if options.Execer == nil {
155155
options.Execer = agentexec.DefaultExecer
156156
}
157-
if options.ContainerLister == nil {
158-
options.ContainerLister = agentcontainers.NoopLister{}
159-
}
160157

161158
hardCtx, hardCancel := context.WithCancel(context.Background())
162159
gracefulCtx, gracefulCancel := context.WithCancel(hardCtx)
@@ -192,9 +189,9 @@ func New(options Options) Agent {
192189
prometheusRegistry: prometheusRegistry,
193190
metrics: newAgentMetrics(prometheusRegistry),
194191
execer: options.Execer,
195-
lister: options.ContainerLister,
196192

197193
experimentalDevcontainersEnabled: options.ExperimentalDevcontainersEnabled,
194+
containerAPIOptions: options.ContainerAPIOptions,
198195
}
199196
// Initially, we have a closed channel, reflecting the fact that we are not initially connected.
200197
// Each time we connect we replace the channel (while holding the closeMutex) with a new one
@@ -274,9 +271,10 @@ type agent struct {
274271
// labeled in Coder with the agent + workspace.
275272
metrics *agentMetrics
276273
execer agentexec.Execer
277-
lister agentcontainers.Lister
278274

279275
experimentalDevcontainersEnabled bool
276+
containerAPIOptions []agentcontainers.Option
277+
containerAPI atomic.Pointer[agentcontainers.API] // Set by apiHandler.
280278
}
281279

282280
func (a *agent) TailnetConn() *tailnet.Conn {
@@ -1170,6 +1168,12 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context,
11701168
}
11711169
a.metrics.startupScriptSeconds.WithLabelValues(label).Set(dur)
11721170
a.scriptRunner.StartCron()
1171+
if containerAPI := a.containerAPI.Load(); containerAPI != nil {
1172+
// Inform the container API that the agent is ready.
1173+
// This allows us to start watching for changes to
1174+
// the devcontainer configuration files.
1175+
containerAPI.SignalReady()
1176+
}
11731177
})
11741178
if err != nil {
11751179
return xerrors.Errorf("track conn goroutine: %w", err)

‎agent/agentcontainers/api.go

+47-20
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ type API struct {
3939
watcher watcher.Watcher
4040

4141
cacheDuration time.Duration
42+
execer agentexec.Execer
4243
cl Lister
4344
dccli DevcontainerCLI
4445
clock quartz.Clock
@@ -56,14 +57,6 @@ type API struct {
5657
// Option is a functional option for API.
5758
type Option func(*API)
5859

59-
// WithLister sets the agentcontainers.Lister implementation to use.
60-
// The default implementation uses the Docker CLI to list containers.
61-
func WithLister(cl Lister) Option {
62-
return func(api *API) {
63-
api.cl = cl
64-
}
65-
}
66-
6760
// WithClock sets the quartz.Clock implementation to use.
6861
// This is primarily used for testing to control time.
6962
func WithClock(clock quartz.Clock) Option {
@@ -72,6 +65,21 @@ func WithClock(clock quartz.Clock) Option {
7265
}
7366
}
7467

68+
// WithExecer sets the agentexec.Execer implementation to use.
69+
func WithExecer(execer agentexec.Execer) Option {
70+
return func(api *API) {
71+
api.execer = execer
72+
}
73+
}
74+
75+
// WithLister sets the agentcontainers.Lister implementation to use.
76+
// The default implementation uses the Docker CLI to list containers.
77+
func WithLister(cl Lister) Option {
78+
return func(api *API) {
79+
api.cl = cl
80+
}
81+
}
82+
7583
// WithDevcontainerCLI sets the DevcontainerCLI implementation to use.
7684
// This can be used in tests to modify @devcontainer/cli behavior.
7785
func WithDevcontainerCLI(dccli DevcontainerCLI) Option {
@@ -113,6 +121,7 @@ func NewAPI(logger slog.Logger, options ...Option) *API {
113121
done: make(chan struct{}),
114122
logger: logger,
115123
clock: quartz.NewReal(),
124+
execer: agentexec.DefaultExecer,
116125
cacheDuration: defaultGetContainersCacheDuration,
117126
lockCh: make(chan struct{}, 1),
118127
devcontainerNames: make(map[string]struct{}),
@@ -123,30 +132,46 @@ func NewAPI(logger slog.Logger, options ...Option) *API {
123132
opt(api)
124133
}
125134
if api.cl == nil {
126-
api.cl = &DockerCLILister{}
135+
api.cl = NewDocker(api.execer)
127136
}
128137
if api.dccli == nil {
129-
api.dccli = NewDevcontainerCLI(logger.Named("devcontainer-cli"), agentexec.DefaultExecer)
138+
api.dccli = NewDevcontainerCLI(logger.Named("devcontainer-cli"), api.execer)
130139
}
131140
if api.watcher == nil {
132-
api.watcher = watcher.NewNoop()
141+
var err error
142+
api.watcher, err = watcher.NewFSNotify()
143+
if err != nil {
144+
logger.Error(ctx, "create file watcher service failed", slog.Error(err))
145+
api.watcher = watcher.NewNoop()
146+
}
133147
}
134148

149+
go api.loop()
150+
151+
return api
152+
}
153+
154+
// SignalReady signals the API that we are ready to begin watching for
155+
// file changes. This is used to prime the cache with the current list
156+
// of containers and to start watching the devcontainer config files for
157+
// changes. It should be called after the agent ready.
158+
func (api *API) SignalReady() {
159+
// Prime the cache with the current list of containers.
160+
_, _ = api.cl.List(api.ctx)
161+
135162
// Make sure we watch the devcontainer config files for changes.
136163
for _, devcontainer := range api.knownDevcontainers {
137-
if devcontainer.ConfigPath != "" {
138-
if err := api.watcher.Add(devcontainer.ConfigPath); err != nil {
139-
api.logger.Error(ctx, "watch devcontainer config file failed", slog.Error(err), slog.F("file", devcontainer.ConfigPath))
140-
}
164+
if devcontainer.ConfigPath == "" {
165+
continue
141166
}
142-
}
143167

144-
go api.start()
145-
146-
return api
168+
if err := api.watcher.Add(devcontainer.ConfigPath); err != nil {
169+
api.logger.Error(api.ctx, "watch devcontainer config file failed", slog.Error(err), slog.F("file", devcontainer.ConfigPath))
170+
}
171+
}
147172
}
148173

149-
func (api *API) start() {
174+
func (api *API) loop() {
150175
defer close(api.done)
151176

152177
for {
@@ -187,9 +212,11 @@ func (api *API) start() {
187212
// Routes returns the HTTP handler for container-related routes.
188213
func (api *API) Routes() http.Handler {
189214
r := chi.NewRouter()
215+
190216
r.Get("/", api.handleList)
191217
r.Get("/devcontainers", api.handleListDevcontainers)
192218
r.Post("/{id}/recreate", api.handleRecreate)
219+
193220
return r
194221
}
195222

‎agent/agentcontainers/api_test.go

+5
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"cdr.dev/slog"
1919
"cdr.dev/slog/sloggers/slogtest"
2020
"github.com/coder/coder/v2/agent/agentcontainers"
21+
"github.com/coder/coder/v2/agent/agentcontainers/watcher"
2122
"github.com/coder/coder/v2/codersdk"
2223
"github.com/coder/coder/v2/testutil"
2324
"github.com/coder/quartz"
@@ -253,6 +254,7 @@ func TestAPI(t *testing.T) {
253254
logger,
254255
agentcontainers.WithLister(tt.lister),
255256
agentcontainers.WithDevcontainerCLI(tt.devcontainerCLI),
257+
agentcontainers.WithWatcher(watcher.NewNoop()),
256258
)
257259
defer api.Close()
258260
r.Mount("/", api.Routes())
@@ -558,6 +560,7 @@ func TestAPI(t *testing.T) {
558560
r := chi.NewRouter()
559561
apiOptions := []agentcontainers.Option{
560562
agentcontainers.WithLister(tt.lister),
563+
agentcontainers.WithWatcher(watcher.NewNoop()),
561564
}
562565

563566
if len(tt.knownDevcontainers) > 0 {
@@ -631,6 +634,8 @@ func TestAPI(t *testing.T) {
631634
)
632635
defer api.Close()
633636

637+
api.SignalReady()
638+
634639
r := chi.NewRouter()
635640
r.Mount("/", api.Routes())
636641

‎agent/api.go

+21-6
Original file line numberDiff line numberDiff line change
@@ -37,23 +37,35 @@ func (a *agent) apiHandler() (http.Handler, func() error) {
3737
cacheDuration: cacheDuration,
3838
}
3939

40-
containerAPIOpts := []agentcontainers.Option{
41-
agentcontainers.WithLister(a.lister),
42-
}
4340
if a.experimentalDevcontainersEnabled {
41+
containerAPIOpts := []agentcontainers.Option{
42+
agentcontainers.WithExecer(a.execer),
43+
}
4444
manifest := a.manifest.Load()
4545
if manifest != nil && len(manifest.Devcontainers) > 0 {
4646
containerAPIOpts = append(
4747
containerAPIOpts,
4848
agentcontainers.WithDevcontainers(manifest.Devcontainers),
4949
)
5050
}
51+
52+
// Append after to allow the agent options to override the default options.
53+
containerAPIOpts = append(containerAPIOpts, a.containerAPIOptions...)
54+
55+
containerAPI := agentcontainers.NewAPI(a.logger.Named("containers"), containerAPIOpts...)
56+
r.Mount("/api/v0/containers", containerAPI.Routes())
57+
a.containerAPI.Store(containerAPI)
58+
} else {
59+
r.HandleFunc("/api/v0/containers", func(w http.ResponseWriter, r *http.Request) {
60+
httpapi.Write(r.Context(), w, http.StatusForbidden, codersdk.Response{
61+
Message: "The agent dev containers feature is experimental and not enabled by default.",
62+
Detail: "To enable this feature, set CODER_AGENT_DEVCONTAINERS_ENABLE=true in your template.",
63+
})
64+
})
5165
}
52-
containerAPI := agentcontainers.NewAPI(a.logger.Named("containers"), containerAPIOpts...)
5366

5467
promHandler := PrometheusMetricsHandler(a.prometheusRegistry, a.logger)
5568

56-
r.Mount("/api/v0/containers", containerAPI.Routes())
5769
r.Get("/api/v0/listening-ports", lp.handler)
5870
r.Get("/api/v0/netcheck", a.HandleNetcheck)
5971
r.Post("/api/v0/list-directory", a.HandleLS)
@@ -64,7 +76,10 @@ func (a *agent) apiHandler() (http.Handler, func() error) {
6476
r.Get("/debug/prometheus", promHandler.ServeHTTP)
6577

6678
return r, func() error {
67-
return containerAPI.Close()
79+
if containerAPI := a.containerAPI.Load(); containerAPI != nil {
80+
return containerAPI.Close()
81+
}
82+
return nil
6883
}
6984
}
7085

‎cli/agent.go

+3-8
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import (
2626
"cdr.dev/slog/sloggers/slogjson"
2727
"cdr.dev/slog/sloggers/slogstackdriver"
2828
"github.com/coder/coder/v2/agent"
29-
"github.com/coder/coder/v2/agent/agentcontainers"
3029
"github.com/coder/coder/v2/agent/agentexec"
3130
"github.com/coder/coder/v2/agent/agentssh"
3231
"github.com/coder/coder/v2/agent/reaper"
@@ -319,13 +318,10 @@ func (r *RootCmd) workspaceAgent() *serpent.Command {
319318
return xerrors.Errorf("create agent execer: %w", err)
320319
}
321320

322-
var containerLister agentcontainers.Lister
323-
if !experimentalDevcontainersEnabled {
324-
logger.Info(ctx, "agent devcontainer detection not enabled")
325-
containerLister = &agentcontainers.NoopLister{}
326-
} else {
321+
if experimentalDevcontainersEnabled {
327322
logger.Info(ctx, "agent devcontainer detection enabled")
328-
containerLister = agentcontainers.NewDocker(execer)
323+
} else {
324+
logger.Info(ctx, "agent devcontainer detection not enabled")
329325
}
330326

331327
agnt := agent.New(agent.Options{
@@ -354,7 +350,6 @@ func (r *RootCmd) workspaceAgent() *serpent.Command {
354350
PrometheusRegistry: prometheusRegistry,
355351
BlockFileTransfer: blockFileTransfer,
356352
Execer: execer,
357-
ContainerLister: containerLister,
358353

359354
ExperimentalDevcontainersEnabled: experimentalDevcontainersEnabled,
360355
})

‎cli/exp_rpty_test.go

-2
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import (
99
"github.com/ory/dockertest/v3/docker"
1010

1111
"github.com/coder/coder/v2/agent"
12-
"github.com/coder/coder/v2/agent/agentcontainers"
1312
"github.com/coder/coder/v2/agent/agenttest"
1413
"github.com/coder/coder/v2/cli/clitest"
1514
"github.com/coder/coder/v2/coderd/coderdtest"
@@ -112,7 +111,6 @@ func TestExpRpty(t *testing.T) {
112111

113112
_ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) {
114113
o.ExperimentalDevcontainersEnabled = true
115-
o.ContainerLister = agentcontainers.NewDocker(o.Execer)
116114
})
117115
_ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait()
118116

‎cli/open_test.go

+5-2
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"go.uber.org/mock/gomock"
1515

1616
"github.com/coder/coder/v2/agent"
17+
"github.com/coder/coder/v2/agent/agentcontainers"
1718
"github.com/coder/coder/v2/agent/agentcontainers/acmock"
1819
"github.com/coder/coder/v2/agent/agenttest"
1920
"github.com/coder/coder/v2/cli/clitest"
@@ -335,7 +336,8 @@ func TestOpenVSCodeDevContainer(t *testing.T) {
335336
})
336337

337338
_ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) {
338-
o.ContainerLister = mcl
339+
o.ExperimentalDevcontainersEnabled = true
340+
o.ContainerAPIOptions = append(o.ContainerAPIOptions, agentcontainers.WithLister(mcl))
339341
})
340342
_ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait()
341343

@@ -508,7 +510,8 @@ func TestOpenVSCodeDevContainer_NoAgentDirectory(t *testing.T) {
508510
})
509511

510512
_ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) {
511-
o.ContainerLister = mcl
513+
o.ExperimentalDevcontainersEnabled = true
514+
o.ContainerAPIOptions = append(o.ContainerAPIOptions, agentcontainers.WithLister(mcl))
512515
})
513516
_ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait()
514517

‎cli/ssh.go

-2
Original file line numberDiff line numberDiff line change
@@ -299,8 +299,6 @@ func (r *RootCmd) ssh() *serpent.Command {
299299
}
300300
if len(cts.Containers) == 0 {
301301
cliui.Info(inv.Stderr, "No containers found!")
302-
cliui.Info(inv.Stderr, "Tip: Agent container integration is experimental and not enabled by default.")
303-
cliui.Info(inv.Stderr, " To enable it, set CODER_AGENT_DEVCONTAINERS_ENABLE=true in your template.")
304302
return nil
305303
}
306304
var found bool

‎cli/ssh_test.go

+3-11
Original file line numberDiff line numberDiff line change
@@ -2029,7 +2029,6 @@ func TestSSH_Container(t *testing.T) {
20292029

20302030
_ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) {
20312031
o.ExperimentalDevcontainersEnabled = true
2032-
o.ContainerLister = agentcontainers.NewDocker(o.Execer)
20332032
})
20342033
_ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait()
20352034

@@ -2058,7 +2057,7 @@ func TestSSH_Container(t *testing.T) {
20582057
mLister := acmock.NewMockLister(ctrl)
20592058
_ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) {
20602059
o.ExperimentalDevcontainersEnabled = true
2061-
o.ContainerLister = mLister
2060+
o.ContainerAPIOptions = append(o.ContainerAPIOptions, agentcontainers.WithLister(mLister))
20622061
})
20632062
_ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait()
20642063

@@ -2097,16 +2096,9 @@ func TestSSH_Container(t *testing.T) {
20972096

20982097
inv, root := clitest.New(t, "ssh", workspace.Name, "-c", uuid.NewString())
20992098
clitest.SetupConfig(t, client, root)
2100-
ptty := ptytest.New(t).Attach(inv)
2101-
2102-
cmdDone := tGo(t, func() {
2103-
err := inv.WithContext(ctx).Run()
2104-
assert.NoError(t, err)
2105-
})
21062099

2107-
ptty.ExpectMatch("No containers found!")
2108-
ptty.ExpectMatch("Tip: Agent container integration is experimental and not enabled by default.")
2109-
<-cmdDone
2100+
err := inv.WithContext(ctx).Run()
2101+
require.ErrorContains(t, err, "The agent dev containers feature is experimental and not enabled by default.")
21102102
})
21112103
}
21122104

0 commit comments

Comments
 (0)