From 222e227261f1435315e98cacf9d0bfcd9f1a2585 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Thu, 19 Jun 2025 13:35:19 +0000 Subject: [PATCH 1/6] fix(agent/agentcontainers): ensure agent name env var is correct Previously, `CODER_WORKSPACE_AGENT_NAME` would always be passed as the dev container name. This is invalid for the following scenarios: - The dev container is specified in terraform - The dev container has a name customization This change now runs `ReadConfig` twice. The first read is to extract a name (if present), from the `devcontainer.json`. The second read will then use the name we have stored for the dev container (so this could be either the customization, terraform resource name, or container name). --- agent/agentcontainers/api.go | 39 ++++++----- agent/agentcontainers/api_test.go | 111 +++++++++++++++++++++++++++++- 2 files changed, 130 insertions(+), 20 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 4e8773792b7e5..6dcbda90fc148 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -1146,12 +1146,30 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c codersdk.DisplayAppPortForward: true, } + // We need to read the config twice. This first time is so that we can extract the + // agent's name. + if config, err := api.dccli.ReadConfig(ctx, dc.WorkspaceFolder, dc.ConfigPath, []string{}); err != nil { + api.logger.Error(ctx, "unable to read devcontainer config", slog.Error(err)) + } else { + // 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 + } else { + logger.Warn(ctx, "invalid agent name in devcontainer customization, ignoring", slog.F("name", name)) + } + } + } + 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_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), @@ -1175,19 +1193,6 @@ 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)) - } - } } displayApps := make([]codersdk.DisplayApp, 0, len(displayAppsMap)) @@ -1219,10 +1224,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..d48f1d014e794 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -1263,7 +1263,7 @@ func TestAPI(t *testing.T) { } fakeDCCLI = &fakeDevcontainerCLI{ execErrC: make(chan func(cmd string, args ...string) error, 1), - readConfigErrC: make(chan func(envs []string) error, 1), + readConfigErrC: make(chan func(envs []string) error, 2), } testContainer = codersdk.WorkspaceAgentContainer{ @@ -1325,6 +1325,10 @@ func TestAPI(t *testing.T) { assert.Empty(t, args) return nil }) // Exec pwd. + testutil.RequireSend(ctx, t, fakeDCCLI.readConfigErrC, func(envs []string) error { + assert.Empty(t, envs) + return nil + }) // We run 'ReadConfig' twice, once to get the agent name, second to read the rest. testutil.RequireSend(ctx, t, fakeDCCLI.readConfigErrC, func(envs []string) error { assert.Contains(t, envs, "CODER_WORKSPACE_AGENT_NAME=test-container") assert.Contains(t, envs, "CODER_WORKSPACE_NAME=test-workspace") @@ -1472,6 +1476,10 @@ func TestAPI(t *testing.T) { assert.Empty(t, args) return nil }) // Exec pwd. + testutil.RequireSend(ctx, t, fakeDCCLI.readConfigErrC, func(envs []string) error { + assert.Empty(t, envs) + return nil + }) // We run 'ReadConfig' twice, once to get the agent name, second to read the rest. testutil.RequireSend(ctx, t, fakeDCCLI.readConfigErrC, func(envs []string) error { assert.Contains(t, envs, "CODER_WORKSPACE_AGENT_NAME=test-container") assert.Contains(t, envs, "CODER_WORKSPACE_NAME=test-workspace") @@ -1884,6 +1892,107 @@ func TestAPI(t *testing.T) { }) } }) + + t.Run("CreateReadsConfigTwice", func(t *testing.T) { + t.Parallel() + + 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 no env args to be passed into the first read. + assert.Empty(t, env) + return nil + }) + testutil.RequireSend(ctx, t, fDCCLI.readConfigErrC, func(env []string) error { + // We 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 From e29d0d05ae9fff414975ca30506cafe8d40f2f74 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Thu, 19 Jun 2025 13:46:06 +0000 Subject: [PATCH 2/6] fix: skip test on windows --- agent/agentcontainers/api_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index d48f1d014e794..039f64f9749c7 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -1896,6 +1896,10 @@ 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) From 94865d14c903e0449c2461f6ae7cdbfecb0b63ee Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Thu, 19 Jun 2025 14:42:56 +0000 Subject: [PATCH 3/6] chore: feedback on log --- agent/agentcontainers/api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 6dcbda90fc148..4e8eea47ccfb1 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -1160,7 +1160,7 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c if provisioner.AgentNameRegex.Match([]byte(name)) { subAgentConfig.Name = name } else { - logger.Warn(ctx, "invalid agent name in devcontainer customization, ignoring", slog.F("name", name)) + logger.Warn(ctx, "invalid name in devcontainer customization, ignoring", slog.F("name", name)) } } } From b1b3fd40d3cce9a0b3250175f5fb83e82889efc2 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Thu, 19 Jun 2025 14:43:11 +0000 Subject: [PATCH 4/6] chore: only read config twice if agent name is changed --- agent/agentcontainers/api.go | 48 ++++++++++++++++++++----------- agent/agentcontainers/api_test.go | 14 ++------- 2 files changed, 34 insertions(+), 28 deletions(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 4e8eea47ccfb1..af3ca01d76b9a 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -1146,11 +1146,27 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c codersdk.DisplayAppPortForward: true, } - // We need to read the config twice. This first time is so that we can extract the - // agent's name. - if config, err := api.dccli.ReadConfig(ctx, dc.WorkspaceFolder, dc.ConfigPath, []string{}); err != nil { - api.logger.Error(ctx, "unable to read devcontainer config", slog.Error(err)) - } else { + var appsWithPossibleDuplicates []SubAgentApp + + 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 @@ -1159,24 +1175,18 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c // 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)) } } - } - var appsWithPossibleDuplicates []SubAgentApp + if configOutdated { + if config, err = readConfig(); err != nil { + return err + } + } - if config, err := 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), - }, - ); err != nil { - api.logger.Error(ctx, "unable to read devcontainer config", slog.Error(err)) - } else { coderCustomization := config.MergedConfiguration.Customizations.Coder for _, customization := range coderCustomization { @@ -1193,6 +1203,10 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c appsWithPossibleDuplicates = append(appsWithPossibleDuplicates, customization.Apps...) } + + return nil + }(); err != nil { + api.logger.Error(ctx, "unable to read devcontainer config", slog.Error(err)) } displayApps := make([]codersdk.DisplayApp, 0, len(displayAppsMap)) diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 039f64f9749c7..b1b88a7248af6 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -1325,10 +1325,6 @@ func TestAPI(t *testing.T) { assert.Empty(t, args) return nil }) // Exec pwd. - testutil.RequireSend(ctx, t, fakeDCCLI.readConfigErrC, func(envs []string) error { - assert.Empty(t, envs) - return nil - }) // We run 'ReadConfig' twice, once to get the agent name, second to read the rest. testutil.RequireSend(ctx, t, fakeDCCLI.readConfigErrC, func(envs []string) error { assert.Contains(t, envs, "CODER_WORKSPACE_AGENT_NAME=test-container") assert.Contains(t, envs, "CODER_WORKSPACE_NAME=test-workspace") @@ -1476,10 +1472,6 @@ func TestAPI(t *testing.T) { assert.Empty(t, args) return nil }) // Exec pwd. - testutil.RequireSend(ctx, t, fakeDCCLI.readConfigErrC, func(envs []string) error { - assert.Empty(t, envs) - return nil - }) // We run 'ReadConfig' twice, once to get the agent name, second to read the rest. testutil.RequireSend(ctx, t, fakeDCCLI.readConfigErrC, func(envs []string) error { assert.Contains(t, envs, "CODER_WORKSPACE_AGENT_NAME=test-container") assert.Contains(t, envs, "CODER_WORKSPACE_NAME=test-workspace") @@ -1979,12 +1971,12 @@ func TestAPI(t *testing.T) { return nil }) testutil.RequireSend(ctx, t, fDCCLI.readConfigErrC, func(env []string) error { - // We expect no env args to be passed into the first read. - assert.Empty(t, env) + // 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 expect the agent name passed here to have been read from the config. + // 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 From be4a45be770b3a8f5ff6fa9b027f08d5a496e95d Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Thu, 19 Jun 2025 14:44:32 +0000 Subject: [PATCH 5/6] chore: add regex to log --- agent/agentcontainers/api.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index af3ca01d76b9a..ddf98e38bdb48 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -1177,7 +1177,10 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c subAgentConfig.Name = name configOutdated = true } else { - logger.Warn(ctx, "invalid name in devcontainer customization, ignoring", slog.F("name", name)) + logger.Warn(ctx, "invalid name in devcontainer customization, ignoring", + slog.F("name", name), + slog.F("regex", provisioner.AgentNameRegex.String()), + ) } } From fa55d0c71dd439373760010f3349960c8e7a7c45 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Thu, 19 Jun 2025 14:53:28 +0000 Subject: [PATCH 6/6] chore: channel size 2 -> 1 --- agent/agentcontainers/api_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index b1b88a7248af6..d0141ea590826 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -1263,7 +1263,7 @@ func TestAPI(t *testing.T) { } fakeDCCLI = &fakeDevcontainerCLI{ execErrC: make(chan func(cmd string, args ...string) error, 1), - readConfigErrC: make(chan func(envs []string) error, 2), + readConfigErrC: make(chan func(envs []string) error, 1), } testContainer = codersdk.WorkspaceAgentContainer{