diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 6b6f391d238f2..6d2c46b961122 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -83,6 +83,7 @@ type API struct { recreateErrorTimes map[string]time.Time // By workspace folder. injectedSubAgentProcs map[string]subAgentProcess // By workspace folder. usingWorkspaceFolderName map[string]bool // By workspace folder. + ignoredDevcontainers map[string]bool // By workspace folder. Tracks three states (true, false and not checked). asyncWg sync.WaitGroup devcontainerLogSourceIDs map[string]uuid.UUID // By workspace folder. @@ -276,6 +277,7 @@ func NewAPI(logger slog.Logger, options ...Option) *API { devcontainerNames: make(map[string]bool), knownDevcontainers: make(map[string]codersdk.WorkspaceAgentDevcontainer), configFileModifiedTimes: make(map[string]time.Time), + ignoredDevcontainers: make(map[string]bool), recreateSuccessTimes: make(map[string]time.Time), recreateErrorTimes: make(map[string]time.Time), scriptLogger: func(uuid.UUID) ScriptLogger { return noopScriptLogger{} }, @@ -804,6 +806,10 @@ func (api *API) getContainers() (codersdk.WorkspaceAgentListContainersResponse, if len(api.knownDevcontainers) > 0 { devcontainers = make([]codersdk.WorkspaceAgentDevcontainer, 0, len(api.knownDevcontainers)) for _, dc := range api.knownDevcontainers { + if api.ignoredDevcontainers[dc.WorkspaceFolder] { + continue + } + // Include the agent if it's running (we're iterating over // copies, so mutating is fine). if proc := api.injectedSubAgentProcs[dc.WorkspaceFolder]; proc.agent.ID != uuid.Nil { @@ -1036,6 +1042,10 @@ func (api *API) markDevcontainerDirty(configPath string, modifiedAt time.Time) { logger.Info(api.ctx, "marking devcontainer as dirty") dc.Dirty = true } + if _, ok := api.ignoredDevcontainers[dc.WorkspaceFolder]; ok { + logger.Debug(api.ctx, "clearing devcontainer ignored state") + delete(api.ignoredDevcontainers, dc.WorkspaceFolder) // Allow re-reading config. + } api.knownDevcontainers[dc.WorkspaceFolder] = dc } @@ -1092,6 +1102,10 @@ func (api *API) cleanupSubAgents(ctx context.Context) error { // This method uses an internal timeout to prevent blocking indefinitely // if something goes wrong with the injection. func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc codersdk.WorkspaceAgentDevcontainer) (err error) { + if api.ignoredDevcontainers[dc.WorkspaceFolder] { + return nil + } + ctx, cancel := context.WithTimeout(ctx, defaultOperationTimeout) defer cancel() @@ -1113,6 +1127,42 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c maybeRecreateSubAgent := false proc, injected := api.injectedSubAgentProcs[dc.WorkspaceFolder] if injected { + if _, ignoreChecked := api.ignoredDevcontainers[dc.WorkspaceFolder]; !ignoreChecked { + // If ignore status has not yet been checked, or cleared by + // modifications to the devcontainer.json, we must read it + // to determine the current status. This can happen while + // the devcontainer subagent is already running or before + // we've had a chance to inject it. + // + // Note, for simplicity, we do not try to optimize to reduce + // ReadConfig calls here. + config, err := api.dccli.ReadConfig(ctx, dc.WorkspaceFolder, dc.ConfigPath, nil) + if err != nil { + return xerrors.Errorf("read devcontainer config: %w", err) + } + + dcIgnored := config.Configuration.Customizations.Coder.Ignore + if dcIgnored { + proc.stop() + if proc.agent.ID != uuid.Nil { + // Unlock while doing the delete operation. + api.mu.Unlock() + client := *api.subAgentClient.Load() + if err := client.Delete(ctx, proc.agent.ID); err != nil { + api.mu.Lock() + return xerrors.Errorf("delete subagent: %w", err) + } + api.mu.Lock() + } + // Reset agent and containerID to force config re-reading if ignore is toggled. + proc.agent = SubAgent{} + proc.containerID = "" + api.injectedSubAgentProcs[dc.WorkspaceFolder] = proc + api.ignoredDevcontainers[dc.WorkspaceFolder] = dcIgnored + return nil + } + } + if proc.containerID == container.ID && proc.ctx.Err() == nil { // Same container and running, no need to reinject. return nil @@ -1131,7 +1181,8 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c // Container ID changed or the subagent process is not running, // stop the existing subagent context to replace it. proc.stop() - } else { + } + if proc.agent.OperatingSystem == "" { // Set SubAgent defaults. proc.agent.OperatingSystem = "linux" // Assuming Linux for devcontainers. } @@ -1150,7 +1201,11 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c ranSubAgent := false // Clean up if injection fails. + var dcIgnored, setDCIgnored bool defer func() { + if setDCIgnored { + api.ignoredDevcontainers[dc.WorkspaceFolder] = dcIgnored + } if !ranSubAgent { proc.stop() if !api.closed { @@ -1188,48 +1243,6 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c proc.agent.Architecture = arch } - agentBinaryPath, err := os.Executable() - if err != nil { - return xerrors.Errorf("get agent binary path: %w", err) - } - agentBinaryPath, err = filepath.EvalSymlinks(agentBinaryPath) - if err != nil { - return xerrors.Errorf("resolve agent binary path: %w", err) - } - - // If we scripted this as a `/bin/sh` script, we could reduce these - // steps to one instruction, speeding up the injection process. - // - // Note: We use `path` instead of `filepath` here because we are - // working with Unix-style paths inside the container. - if _, err := api.ccli.ExecAs(ctx, container.ID, "root", "mkdir", "-p", path.Dir(coderPathInsideContainer)); err != nil { - return xerrors.Errorf("create agent directory in container: %w", err) - } - - if err := api.ccli.Copy(ctx, container.ID, agentBinaryPath, coderPathInsideContainer); err != nil { - return xerrors.Errorf("copy agent binary: %w", err) - } - - logger.Info(ctx, "copied agent binary to container") - - // Make sure the agent binary is executable so we can run it (the - // user doesn't matter since we're making it executable for all). - if _, err := api.ccli.ExecAs(ctx, container.ID, "root", "chmod", "0755", path.Dir(coderPathInsideContainer), coderPathInsideContainer); err != nil { - return xerrors.Errorf("set agent binary executable: %w", err) - } - - // Attempt to add CAP_NET_ADMIN to the binary to improve network - // performance (optional, allow to fail). See `bootstrap_linux.sh`. - // TODO(mafredri): Disable for now until we can figure out why this - // causes the following error on some images: - // - // Image: mcr.microsoft.com/devcontainers/base:ubuntu - // Error: /.coder-agent/coder: Operation not permitted - // - // if _, err := api.ccli.ExecAs(ctx, container.ID, "root", "setcap", "cap_net_admin+ep", coderPathInsideContainer); err != nil { - // logger.Warn(ctx, "set CAP_NET_ADMIN on agent binary failed", slog.Error(err)) - // } - subAgentConfig := proc.agent.CloneConfig(dc) if proc.agent.ID == uuid.Nil || maybeRecreateSubAgent { subAgentConfig.Architecture = arch @@ -1269,6 +1282,13 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c return err } + // We only allow ignore to be set in the root customization layer to + // prevent weird interactions with devcontainer features. + dcIgnored, setDCIgnored = config.Configuration.Customizations.Coder.Ignore, true + if dcIgnored { + return nil + } + workspaceFolder = config.Workspace.WorkspaceFolder // NOTE(DanielleMaywood): @@ -1317,6 +1337,22 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c api.logger.Error(ctx, "unable to read devcontainer config", slog.Error(err)) } + if dcIgnored { + proc.stop() + if proc.agent.ID != uuid.Nil { + // If we stop the subagent, we also need to delete it. + client := *api.subAgentClient.Load() + if err := client.Delete(ctx, proc.agent.ID); err != nil { + return xerrors.Errorf("delete subagent: %w", err) + } + } + // Reset agent and containerID to force config re-reading if + // ignore is toggled. + proc.agent = SubAgent{} + proc.containerID = "" + return nil + } + displayApps := make([]codersdk.DisplayApp, 0, len(displayAppsMap)) for app, enabled := range displayAppsMap { if enabled { @@ -1349,6 +1385,48 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c subAgentConfig.Directory = workspaceFolder } + agentBinaryPath, err := os.Executable() + if err != nil { + return xerrors.Errorf("get agent binary path: %w", err) + } + agentBinaryPath, err = filepath.EvalSymlinks(agentBinaryPath) + if err != nil { + return xerrors.Errorf("resolve agent binary path: %w", err) + } + + // If we scripted this as a `/bin/sh` script, we could reduce these + // steps to one instruction, speeding up the injection process. + // + // Note: We use `path` instead of `filepath` here because we are + // working with Unix-style paths inside the container. + if _, err := api.ccli.ExecAs(ctx, container.ID, "root", "mkdir", "-p", path.Dir(coderPathInsideContainer)); err != nil { + return xerrors.Errorf("create agent directory in container: %w", err) + } + + if err := api.ccli.Copy(ctx, container.ID, agentBinaryPath, coderPathInsideContainer); err != nil { + return xerrors.Errorf("copy agent binary: %w", err) + } + + logger.Info(ctx, "copied agent binary to container") + + // Make sure the agent binary is executable so we can run it (the + // user doesn't matter since we're making it executable for all). + if _, err := api.ccli.ExecAs(ctx, container.ID, "root", "chmod", "0755", path.Dir(coderPathInsideContainer), coderPathInsideContainer); err != nil { + return xerrors.Errorf("set agent binary executable: %w", err) + } + + // Attempt to add CAP_NET_ADMIN to the binary to improve network + // performance (optional, allow to fail). See `bootstrap_linux.sh`. + // TODO(mafredri): Disable for now until we can figure out why this + // causes the following error on some images: + // + // Image: mcr.microsoft.com/devcontainers/base:ubuntu + // Error: /.coder-agent/coder: Operation not permitted + // + // if _, err := api.ccli.ExecAs(ctx, container.ID, "root", "setcap", "cap_net_admin+ep", coderPathInsideContainer); err != nil { + // logger.Warn(ctx, "set CAP_NET_ADMIN on agent binary failed", slog.Error(err)) + // } + deleteSubAgent := proc.agent.ID != uuid.Nil && maybeRecreateSubAgent && !proc.agent.EqualConfig(subAgentConfig) if deleteSubAgent { logger.Debug(ctx, "deleting existing subagent for recreation", slog.F("agent_id", proc.agent.ID)) diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 3ee1c775087c2..b6bae46c835c9 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -2070,6 +2070,177 @@ func TestAPI(t *testing.T) { require.Equal(t, testDir, lastCmd.Dir, "custom directory should be used") require.Equal(t, testEnv, lastCmd.Env, "custom environment should be used") }) + + t.Run("IgnoreCustomization", func(t *testing.T) { + t.Parallel() + + if runtime.GOOS == "windows" { + t.Skip("Dev Container tests are not supported on Windows (this test uses mocks but fails due to Windows paths)") + } + + ctx := testutil.Context(t, testutil.WaitShort) + + startTime := time.Date(2025, 1, 1, 12, 0, 0, 0, time.UTC) + configPath := "/workspace/project/.devcontainer/devcontainer.json" + + container := codersdk.WorkspaceAgentContainer{ + ID: "container-id", + FriendlyName: "container-name", + Running: true, + CreatedAt: startTime.Add(-1 * time.Hour), + Labels: map[string]string{ + agentcontainers.DevcontainerLocalFolderLabel: "/workspace/project", + agentcontainers.DevcontainerConfigFileLabel: configPath, + }, + } + + fLister := &fakeContainerCLI{ + containers: codersdk.WorkspaceAgentListContainersResponse{ + Containers: []codersdk.WorkspaceAgentContainer{container}, + }, + arch: runtime.GOARCH, + } + + // Start with ignore=true + fDCCLI := &fakeDevcontainerCLI{ + execErrC: make(chan func(string, ...string) error, 1), + readConfig: agentcontainers.DevcontainerConfig{ + Configuration: agentcontainers.DevcontainerConfiguration{ + Customizations: agentcontainers.DevcontainerCustomizations{ + Coder: agentcontainers.CoderCustomization{Ignore: true}, + }, + }, + Workspace: agentcontainers.DevcontainerWorkspace{WorkspaceFolder: "/workspace/project"}, + }, + } + + fakeSAC := &fakeSubAgentClient{ + logger: slogtest.Make(t, nil).Named("fakeSubAgentClient"), + agents: make(map[uuid.UUID]agentcontainers.SubAgent), + createErrC: make(chan error, 1), + deleteErrC: make(chan error, 1), + } + + mClock := quartz.NewMock(t) + mClock.Set(startTime) + fWatcher := newFakeWatcher(t) + + logger := slogtest.Make(t, nil).Leveled(slog.LevelDebug) + api := agentcontainers.NewAPI( + logger, + agentcontainers.WithDevcontainerCLI(fDCCLI), + agentcontainers.WithContainerCLI(fLister), + agentcontainers.WithSubAgentClient(fakeSAC), + agentcontainers.WithWatcher(fWatcher), + agentcontainers.WithClock(mClock), + ) + defer func() { + close(fakeSAC.createErrC) + close(fakeSAC.deleteErrC) + api.Close() + }() + + r := chi.NewRouter() + r.Mount("/", api.Routes()) + + t.Log("Phase 1: Test ignore=true filters out devcontainer") + req := httptest.NewRequest(http.MethodGet, "/", nil).WithContext(ctx) + rec := httptest.NewRecorder() + r.ServeHTTP(rec, req) + require.Equal(t, http.StatusOK, rec.Code) + + var response codersdk.WorkspaceAgentListContainersResponse + err := json.NewDecoder(rec.Body).Decode(&response) + require.NoError(t, err) + + assert.Empty(t, response.Devcontainers, "ignored devcontainer should not be in response when ignore=true") + assert.Len(t, response.Containers, 1, "regular container should still be listed") + + t.Log("Phase 2: Change to ignore=false") + fDCCLI.readConfig.Configuration.Customizations.Coder.Ignore = false + var ( + exitSubAgent = make(chan struct{}) + subAgentExited = make(chan struct{}) + exitSubAgentOnce sync.Once + ) + defer func() { + exitSubAgentOnce.Do(func() { + close(exitSubAgent) + }) + }() + execSubAgent := func(cmd string, args ...string) error { + if len(args) != 1 || args[0] != "agent" { + t.Log("execSubAgent called with unexpected arguments", cmd, args) + return nil + } + defer close(subAgentExited) + select { + case <-exitSubAgent: + case <-ctx.Done(): + return ctx.Err() + } + return nil + } + testutil.RequireSend(ctx, t, fDCCLI.execErrC, execSubAgent) + testutil.RequireSend(ctx, t, fakeSAC.createErrC, nil) + + fWatcher.sendEventWaitNextCalled(ctx, fsnotify.Event{ + Name: configPath, + Op: fsnotify.Write, + }) + + err = api.RefreshContainers(ctx) + require.NoError(t, err) + + t.Log("Phase 2: Cont, waiting for sub agent to exit") + exitSubAgentOnce.Do(func() { + close(exitSubAgent) + }) + select { + case <-subAgentExited: + case <-ctx.Done(): + t.Fatal("timeout waiting for sub agent to exit") + } + + req = httptest.NewRequest(http.MethodGet, "/", nil).WithContext(ctx) + rec = httptest.NewRecorder() + r.ServeHTTP(rec, req) + require.Equal(t, http.StatusOK, rec.Code) + + err = json.NewDecoder(rec.Body).Decode(&response) + require.NoError(t, err) + + assert.Len(t, response.Devcontainers, 1, "devcontainer should be in response when ignore=false") + assert.Len(t, response.Containers, 1, "regular container should still be listed") + assert.Equal(t, "/workspace/project", response.Devcontainers[0].WorkspaceFolder) + require.Len(t, fakeSAC.created, 1, "sub agent should be created when ignore=false") + createdAgentID := fakeSAC.created[0].ID + + t.Log("Phase 3: Change back to ignore=true and test sub agent deletion") + fDCCLI.readConfig.Configuration.Customizations.Coder.Ignore = true + testutil.RequireSend(ctx, t, fakeSAC.deleteErrC, nil) + + fWatcher.sendEventWaitNextCalled(ctx, fsnotify.Event{ + Name: configPath, + Op: fsnotify.Write, + }) + + err = api.RefreshContainers(ctx) + require.NoError(t, err) + + req = httptest.NewRequest(http.MethodGet, "/", nil).WithContext(ctx) + rec = httptest.NewRecorder() + r.ServeHTTP(rec, req) + require.Equal(t, http.StatusOK, rec.Code) + + err = json.NewDecoder(rec.Body).Decode(&response) + require.NoError(t, err) + + assert.Empty(t, response.Devcontainers, "devcontainer should be filtered out when ignore=true again") + assert.Len(t, response.Containers, 1, "regular container should still be listed") + require.Len(t, fakeSAC.deleted, 1, "sub agent should be deleted when ignore=true") + assert.Equal(t, createdAgentID, fakeSAC.deleted[0], "the same sub agent that was created should be deleted") + }) } // mustFindDevcontainerByPath returns the devcontainer with the given workspace diff --git a/agent/agentcontainers/devcontainercli.go b/agent/agentcontainers/devcontainercli.go index 850dc84d5ac7d..e49c6900facdb 100644 --- a/agent/agentcontainers/devcontainercli.go +++ b/agent/agentcontainers/devcontainercli.go @@ -44,6 +44,7 @@ type CoderCustomization struct { DisplayApps map[codersdk.DisplayApp]bool `json:"displayApps,omitempty"` Apps []SubAgentApp `json:"apps,omitempty"` Name string `json:"name,omitempty"` + Ignore bool `json:"ignore,omitempty"` } type DevcontainerWorkspace struct { diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index b3fb53c228ef8..3154fb32bb3d2 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -1432,6 +1432,7 @@ func TestWorkspaceAgentRecreateDevcontainer(t *testing.T) { }, nil).AnyTimes() // DetectArchitecture always returns "" for this test to disable agent injection. mccli.EXPECT().DetectArchitecture(gomock.Any(), devContainer.ID).Return("", nil).AnyTimes() + mdccli.EXPECT().ReadConfig(gomock.Any(), workspaceFolder, configFile, gomock.Any()).Return(agentcontainers.DevcontainerConfig{}, nil).Times(1) mdccli.EXPECT().Up(gomock.Any(), workspaceFolder, configFile, gomock.Any()).Return("someid", nil).Times(1) return 0 },