synced/coder
1
0
Fork 0
mirror of https://github.com/coder/coder.git synced 2025-07-13 21:36:50 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(agent/agentcontainers): ensure agent name env var is correct (#18457)

Browse Source 
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).
This commit is contained in:
Danielle Maywood
2025-06-19 16:44:00 +01:00
committed by GitHub
parent 9e7b7f2b28
commit b49e62faad
2 changed files with 150 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files

72
agent/agentcontainers/api.go
View File

@ -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)

105
agent/agentcontainers/api_test.go
View File

@ -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