Skip to content

Commit b49e62f

Browse files
DanielleMaywoodDanielleMaywood
authored
fix(agent/agentcontainers): ensure agent name env var is correct (#18457)
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).
1 parent 9e7b7f2 commit b49e62f

File tree

2 files changed

+151
-28
lines changed

2 files changed

+151
-28
lines changed

‎agent/agentcontainers/api.go

Lines changed: 46 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1147,18 +1147,49 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c
11471147
}
11481148

11491149
var appsWithPossibleDuplicates []SubAgentApp
1150-
var possibleAgentName string
1151-
1152-
if config, err := api.dccli.ReadConfig(ctx, dc.WorkspaceFolder, dc.ConfigPath,
1153-
[]string{
1154-
fmt.Sprintf("CODER_WORKSPACE_AGENT_NAME=%s", dc.Name),
1155-
fmt.Sprintf("CODER_WORKSPACE_OWNER_NAME=%s", api.ownerName),
1156-
fmt.Sprintf("CODER_WORKSPACE_NAME=%s", api.workspaceName),
1157-
fmt.Sprintf("CODER_URL=%s", api.subAgentURL),
1158-
},
1159-
); err != nil {
1160-
api.logger.Error(ctx, "unable to read devcontainer config", slog.Error(err))
1161-
} else {
1150+
1151+
if err := func() error {
1152+
var (
1153+
config DevcontainerConfig
1154+
configOutdated bool
1155+
)
1156+
1157+
readConfig := func() (DevcontainerConfig, error) {
1158+
return api.dccli.ReadConfig(ctx, dc.WorkspaceFolder, dc.ConfigPath, []string{
1159+
fmt.Sprintf("CODER_WORKSPACE_AGENT_NAME=%s", subAgentConfig.Name),
1160+
fmt.Sprintf("CODER_WORKSPACE_OWNER_NAME=%s", api.ownerName),
1161+
fmt.Sprintf("CODER_WORKSPACE_NAME=%s", api.workspaceName),
1162+
fmt.Sprintf("CODER_URL=%s", api.subAgentURL),
1163+
})
1164+
}
1165+
1166+
if config, err = readConfig(); err != nil {
1167+
return err
1168+
}
1169+
1170+
// NOTE(DanielleMaywood):
1171+
// We only want to take an agent name specified in the root customization layer.
1172+
// This restricts the ability for a feature to specify the agent name. We may revisit
1173+
// this in the future, but for now we want to restrict this behavior.
1174+
if name := config.Configuration.Customizations.Coder.Name; name != "" {
1175+
// We only want to pick this name if it is a valid name.
1176+
if provisioner.AgentNameRegex.Match([]byte(name)) {
1177+
subAgentConfig.Name = name
1178+
configOutdated = true
1179+
} else {
1180+
logger.Warn(ctx, "invalid name in devcontainer customization, ignoring",
1181+
slog.F("name", name),
1182+
slog.F("regex", provisioner.AgentNameRegex.String()),
1183+
)
1184+
}
1185+
}
1186+
1187+
if configOutdated {
1188+
if config, err = readConfig(); err != nil {
1189+
return err
1190+
}
1191+
}
1192+
11621193
coderCustomization := config.MergedConfiguration.Customizations.Coder
11631194

11641195
for _, customization := range coderCustomization {
@@ -1176,18 +1207,9 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c
11761207
appsWithPossibleDuplicates = append(appsWithPossibleDuplicates, customization.Apps...)
11771208
}
11781209

1179-
// NOTE(DanielleMaywood):
1180-
// We only want to take an agent name specified in the root customization layer.
1181-
// This restricts the ability for a feature to specify the agent name. We may revisit
1182-
// this in the future, but for now we want to restrict this behavior.
1183-
if name := config.Configuration.Customizations.Coder.Name; name != "" {
1184-
// We only want to pick this name if it is a valid name.
1185-
if provisioner.AgentNameRegex.Match([]byte(name)) {
1186-
possibleAgentName = name
1187-
} else {
1188-
logger.Warn(ctx, "invalid agent name in devcontainer customization, ignoring", slog.F("name", name))
1189-
}
1190-
}
1210+
return nil
1211+
}(); err != nil {
1212+
api.logger.Error(ctx, "unable to read devcontainer config", slog.Error(err))
11911213
}
11921214

11931215
displayApps := make([]codersdk.DisplayApp, 0, len(displayAppsMap))
@@ -1219,10 +1241,6 @@ func (api *API) maybeInjectSubAgentIntoContainerLocked(ctx context.Context, dc c
12191241

12201242
subAgentConfig.DisplayApps = displayApps
12211243
subAgentConfig.Apps = apps
1222-
1223-
if possibleAgentName != "" {
1224-
subAgentConfig.Name = possibleAgentName
1225-
}
12261244
}
12271245

12281246
deleteSubAgent := proc.agent.ID != uuid.Nil && maybeRecreateSubAgent && !proc.agent.EqualConfig(subAgentConfig)

‎agent/agentcontainers/api_test.go

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1884,6 +1884,111 @@ func TestAPI(t *testing.T) {
18841884
})
18851885
}
18861886
})
1887+
1888+
t.Run("CreateReadsConfigTwice", func(t *testing.T) {
1889+
t.Parallel()
1890+
1891+
if runtime.GOOS == "windows" {
1892+
t.Skip("Dev Container tests are not supported on Windows (this test uses mocks but fails due to Windows paths)")
1893+
}
1894+
1895+
var (
1896+
ctx = testutil.Context(t, testutil.WaitMedium)
1897+
logger = testutil.Logger(t)
1898+
mClock = quartz.NewMock(t)
1899+
mCCLI = acmock.NewMockContainerCLI(gomock.NewController(t))
1900+
fSAC = &fakeSubAgentClient{
1901+
logger: logger.Named("fakeSubAgentClient"),
1902+
createErrC: make(chan error, 1),
1903+
}
1904+
fDCCLI = &fakeDevcontainerCLI{
1905+
readConfig: agentcontainers.DevcontainerConfig{
1906+
Configuration: agentcontainers.DevcontainerConfiguration{
1907+
Customizations: agentcontainers.DevcontainerCustomizations{
1908+
Coder: agentcontainers.CoderCustomization{
1909+
// We want to specify a custom name for this agent.
1910+
Name: "custom-name",
1911+
},
1912+
},
1913+
},
1914+
},
1915+
readConfigErrC: make(chan func(envs []string) error, 2),
1916+
execErrC: make(chan func(cmd string, args ...string) error, 1),
1917+
}
1918+
1919+
testContainer = codersdk.WorkspaceAgentContainer{
1920+
ID: "test-container-id",
1921+
FriendlyName: "test-container",
1922+
Image: "test-image",
1923+
Running: true,
1924+
CreatedAt: time.Now(),
1925+
Labels: map[string]string{
1926+
agentcontainers.DevcontainerLocalFolderLabel: "/workspaces",
1927+
agentcontainers.DevcontainerConfigFileLabel: "/workspace/.devcontainer/devcontainer.json",
1928+
},
1929+
}
1930+
)
1931+
1932+
coderBin, err := os.Executable()
1933+
require.NoError(t, err)
1934+
1935+
// Mock the `List` function to always return out test container.
1936+
mCCLI.EXPECT().List(gomock.Any()).Return(codersdk.WorkspaceAgentListContainersResponse{
1937+
Containers: []codersdk.WorkspaceAgentContainer{testContainer},
1938+
}, nil).AnyTimes()
1939+
1940+
// Mock the steps used for injecting the coder agent.
1941+
gomock.InOrder(
1942+
mCCLI.EXPECT().DetectArchitecture(gomock.Any(), testContainer.ID).Return(runtime.GOARCH, nil),
1943+
mCCLI.EXPECT().ExecAs(gomock.Any(), testContainer.ID, "root", "mkdir", "-p", "/.coder-agent").Return(nil, nil),
1944+
mCCLI.EXPECT().Copy(gomock.Any(), testContainer.ID, coderBin, "/.coder-agent/coder").Return(nil),
1945+
mCCLI.EXPECT().ExecAs(gomock.Any(), testContainer.ID, "root", "chmod", "0755", "/.coder-agent", "/.coder-agent/coder").Return(nil, nil),
1946+
)
1947+
1948+
mClock.Set(time.Now()).MustWait(ctx)
1949+
tickerTrap := mClock.Trap().TickerFunc("updaterLoop")
1950+
1951+
api := agentcontainers.NewAPI(logger,
1952+
agentcontainers.WithClock(mClock),
1953+
agentcontainers.WithContainerCLI(mCCLI),
1954+
agentcontainers.WithDevcontainerCLI(fDCCLI),
1955+
agentcontainers.WithSubAgentClient(fSAC),
1956+
agentcontainers.WithSubAgentURL("test-subagent-url"),
1957+
agentcontainers.WithWatcher(watcher.NewNoop()),
1958+
)
1959+
defer api.Close()
1960+
1961+
// Close before api.Close() defer to avoid deadlock after test.
1962+
defer close(fSAC.createErrC)
1963+
defer close(fDCCLI.execErrC)
1964+
defer close(fDCCLI.readConfigErrC)
1965+
1966+
// Given: We allow agent creation and injection to succeed.
1967+
testutil.RequireSend(ctx, t, fSAC.createErrC, nil)
1968+
testutil.RequireSend(ctx, t, fDCCLI.execErrC, func(cmd string, args ...string) error {
1969+
assert.Equal(t, "pwd", cmd)
1970+
assert.Empty(t, args)
1971+
return nil
1972+
})
1973+
testutil.RequireSend(ctx, t, fDCCLI.readConfigErrC, func(env []string) error {
1974+
// We expect the wrong workspace agent name passed in first.
1975+
assert.Contains(t, env, "CODER_WORKSPACE_AGENT_NAME=test-container")
1976+
return nil
1977+
})
1978+
testutil.RequireSend(ctx, t, fDCCLI.readConfigErrC, func(env []string) error {
1979+
// We then expect the agent name passed here to have been read from the config.
1980+
assert.Contains(t, env, "CODER_WORKSPACE_AGENT_NAME=custom-name")
1981+
assert.NotContains(t, env, "CODER_WORKSPACE_AGENT_NAME=test-container")
1982+
return nil
1983+
})
1984+
1985+
// Wait until the ticker has been registered.
1986+
tickerTrap.MustWait(ctx).MustRelease(ctx)
1987+
tickerTrap.Close()
1988+
1989+
// Then: We expected it to succeed
1990+
require.Len(t, fSAC.created, 1)
1991+
})
18871992
}
18881993

18891994
// mustFindDevcontainerByPath returns the devcontainer with the given workspace

0 commit comments

Comments
 (0)