diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 4e8773792b7e5..ddf98e38bdb48 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -1147,18 +1147,49 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c } var appsWithPossibleDuplicates []SubAgentApp - var possibleAgentName string - - if config, err := api.dccli.ReadConfig(ctx, dc.WorkspaceFolder, dc.ConfigPath, - []string{ - fmt.Sprintf("CODER_WORKSPACE_AGENT_NAME=%s", dc.Name), - fmt.Sprintf("CODER_WORKSPACE_OWNER_NAME=%s", api.ownerName), - fmt.Sprintf("CODER_WORKSPACE_NAME=%s", api.workspaceName), - fmt.Sprintf("CODER_URL=%s", api.subAgentURL), - }, - ); err != nil { - api.logger.Error(ctx, "unable to read devcontainer config", slog.Error(err)) - } else { + + if err := func() error { + var ( + config DevcontainerConfig + configOutdated bool + ) + + readConfig := func() (DevcontainerConfig, error) { + return api.dccli.ReadConfig(ctx, dc.WorkspaceFolder, dc.ConfigPath, []string{ + fmt.Sprintf("CODER_WORKSPACE_AGENT_NAME=%s", subAgentConfig.Name), + fmt.Sprintf("CODER_WORKSPACE_OWNER_NAME=%s", api.ownerName), + fmt.Sprintf("CODER_WORKSPACE_NAME=%s", api.workspaceName), + fmt.Sprintf("CODER_URL=%s", api.subAgentURL), + }) + } + + if config, err = readConfig(); err != nil { + return err + } + + // NOTE(DanielleMaywood): + // We only want to take an agent name specified in the root customization layer. + // This restricts the ability for a feature to specify the agent name. We may revisit + // this in the future, but for now we want to restrict this behavior. + if name := config.Configuration.Customizations.Coder.Name; name != "" { + // We only want to pick this name if it is a valid name. + if provisioner.AgentNameRegex.Match([]byte(name)) { + subAgentConfig.Name = name + configOutdated = true + } else { + logger.Warn(ctx, "invalid name in devcontainer customization, ignoring", + slog.F("name", name), + slog.F("regex", provisioner.AgentNameRegex.String()), + ) + } + } + + if configOutdated { + if config, err = readConfig(); err != nil { + return err + } + } + coderCustomization := config.MergedConfiguration.Customizations.Coder for _, customization := range coderCustomization { @@ -1176,18 +1207,9 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c appsWithPossibleDuplicates = append(appsWithPossibleDuplicates, customization.Apps...) } - // NOTE(DanielleMaywood): - // We only want to take an agent name specified in the root customization layer. - // This restricts the ability for a feature to specify the agent name. We may revisit - // this in the future, but for now we want to restrict this behavior. - if name := config.Configuration.Customizations.Coder.Name; name != "" { - // We only want to pick this name if it is a valid name. - if provisioner.AgentNameRegex.Match([]byte(name)) { - possibleAgentName = name - } else { - logger.Warn(ctx, "invalid agent name in devcontainer customization, ignoring", slog.F("name", name)) - } - } + return nil + }(); err != nil { + api.logger.Error(ctx, "unable to read devcontainer config", slog.Error(err)) } displayApps := make([]codersdk.DisplayApp, 0, len(displayAppsMap)) @@ -1219,10 +1241,6 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c subAgentConfig.DisplayApps = displayApps subAgentConfig.Apps = apps - - if possibleAgentName != "" { - subAgentConfig.Name = possibleAgentName - } } deleteSubAgent := proc.agent.ID != uuid.Nil && maybeRecreateSubAgent && !proc.agent.EqualConfig(subAgentConfig) diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index bcd76c658a717..d0141ea590826 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -1884,6 +1884,111 @@ func TestAPI(t *testing.T) { }) } }) + + t.Run("CreateReadsConfigTwice", 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)") + } + + var ( + ctx = testutil.Context(t, testutil.WaitMedium) + logger = testutil.Logger(t) + mClock = quartz.NewMock(t) + mCCLI = acmock.NewMockContainerCLI(gomock.NewController(t)) + fSAC = &fakeSubAgentClient{ + logger: logger.Named("fakeSubAgentClient"), + createErrC: make(chan error, 1), + } + fDCCLI = &fakeDevcontainerCLI{ + readConfig: agentcontainers.DevcontainerConfig{ + Configuration: agentcontainers.DevcontainerConfiguration{ + Customizations: agentcontainers.DevcontainerCustomizations{ + Coder: agentcontainers.CoderCustomization{ + // We want to specify a custom name for this agent. + Name: "custom-name", + }, + }, + }, + }, + readConfigErrC: make(chan func(envs []string) error, 2), + execErrC: make(chan func(cmd string, args ...string) error, 1), + } + + testContainer = codersdk.WorkspaceAgentContainer{ + ID: "test-container-id", + FriendlyName: "test-container", + Image: "test-image", + Running: true, + CreatedAt: time.Now(), + Labels: map[string]string{ + agentcontainers.DevcontainerLocalFolderLabel: "/workspaces", + agentcontainers.DevcontainerConfigFileLabel: "/workspace/.devcontainer/devcontainer.json", + }, + } + ) + + coderBin, err := os.Executable() + require.NoError(t, err) + + // Mock the `List` function to always return out test container. + mCCLI.EXPECT().List(gomock.Any()).Return(codersdk.WorkspaceAgentListContainersResponse{ + Containers: []codersdk.WorkspaceAgentContainer{testContainer}, + }, nil).AnyTimes() + + // Mock the steps used for injecting the coder agent. + gomock.InOrder( + mCCLI.EXPECT().DetectArchitecture(gomock.Any(), testContainer.ID).Return(runtime.GOARCH, nil), + mCCLI.EXPECT().ExecAs(gomock.Any(), testContainer.ID, "root", "mkdir", "-p", "/.coder-agent").Return(nil, nil), + mCCLI.EXPECT().Copy(gomock.Any(), testContainer.ID, coderBin, "/.coder-agent/coder").Return(nil), + mCCLI.EXPECT().ExecAs(gomock.Any(), testContainer.ID, "root", "chmod", "0755", "/.coder-agent", "/.coder-agent/coder").Return(nil, nil), + ) + + mClock.Set(time.Now()).MustWait(ctx) + tickerTrap := mClock.Trap().TickerFunc("updaterLoop") + + api := agentcontainers.NewAPI(logger, + agentcontainers.WithClock(mClock), + agentcontainers.WithContainerCLI(mCCLI), + agentcontainers.WithDevcontainerCLI(fDCCLI), + agentcontainers.WithSubAgentClient(fSAC), + agentcontainers.WithSubAgentURL("test-subagent-url"), + agentcontainers.WithWatcher(watcher.NewNoop()), + ) + defer api.Close() + + // Close before api.Close() defer to avoid deadlock after test. + defer close(fSAC.createErrC) + defer close(fDCCLI.execErrC) + defer close(fDCCLI.readConfigErrC) + + // Given: We allow agent creation and injection to succeed. + testutil.RequireSend(ctx, t, fSAC.createErrC, nil) + testutil.RequireSend(ctx, t, fDCCLI.execErrC, func(cmd string, args ...string) error { + assert.Equal(t, "pwd", cmd) + assert.Empty(t, args) + return nil + }) + testutil.RequireSend(ctx, t, fDCCLI.readConfigErrC, func(env []string) error { + // We expect the wrong workspace agent name passed in first. + assert.Contains(t, env, "CODER_WORKSPACE_AGENT_NAME=test-container") + return nil + }) + testutil.RequireSend(ctx, t, fDCCLI.readConfigErrC, func(env []string) error { + // We then expect the agent name passed here to have been read from the config. + assert.Contains(t, env, "CODER_WORKSPACE_AGENT_NAME=custom-name") + assert.NotContains(t, env, "CODER_WORKSPACE_AGENT_NAME=test-container") + return nil + }) + + // Wait until the ticker has been registered. + tickerTrap.MustWait(ctx).MustRelease(ctx) + tickerTrap.Close() + + // Then: We expected it to succeed + require.Len(t, fSAC.created, 1) + }) } // mustFindDevcontainerByPath returns the devcontainer with the given workspace