Skip to content

Commit cc236ab

Browse files
johnstcnjohnstcn
authored
Merge branch 'main' into cj/prebuild-template-upgrade
2 parents d4529c8 + 8e3022e commit cc236ab

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

56 files changed

+3129
-985
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

‎cli/server.go

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2312,19 +2312,20 @@ func ConnectToPostgres(ctx context.Context, logger slog.Logger, driver string, d
23122312

23132313
var err error
23142314
var sqlDB *sql.DB
2315+
dbNeedsClosing := true
23152316
// Try to connect for 30 seconds.
23162317
ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
23172318
defer cancel()
23182319

23192320
defer func() {
2320-
if err == nil {
2321+
if !dbNeedsClosing {
23212322
return
23222323
}
23232324
if sqlDB != nil {
23242325
_ = sqlDB.Close()
23252326
sqlDB = nil
2327+
logger.Debug(ctx, "closed db before returning from ConnectToPostgres")
23262328
}
2327-
logger.Error(ctx, "connect to postgres failed", slog.Error(err))
23282329
}()
23292330

23302331
var tries int
@@ -2361,12 +2362,7 @@ func ConnectToPostgres(ctx context.Context, logger slog.Logger, driver string, d
23612362
}
23622363
defer version.Close()
23632364
if !version.Next() {
2364-
// it's critical we assign to the err variable, otherwise the defer statement
2365-
// that runs db.Close() will not execute it
2366-
if err = version.Err(); err != nil {
2367-
return nil, xerrors.Errorf("no rows returned for version select: %w", err)
2368-
}
2369-
return nil, xerrors.Errorf("no rows returned for version select")
2365+
return nil, xerrors.Errorf("no rows returned for version select: %w", version.Err())
23702366
}
23712367
var versionNum int
23722368
err = version.Scan(&versionNum)
@@ -2408,6 +2404,7 @@ func ConnectToPostgres(ctx context.Context, logger slog.Logger, driver string, d
24082404
// of connection churn.
24092405
sqlDB.SetMaxIdleConns(3)
24102406

2407+
dbNeedsClosing = false
24112408
return sqlDB, nil
24122409
}
24132410

‎cli/ssh.go

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -925,36 +925,33 @@ func getWorkspaceAndAgent(ctx context.Context, inv *serpent.Invocation, client *
925925
func getWorkspaceAgent(workspace codersdk.Workspace, agentName string) (workspaceAgent codersdk.WorkspaceAgent, err error) {
926926
resources := workspace.LatestBuild.Resources
927927

928-
agents := make([]codersdk.WorkspaceAgent, 0)
928+
var (
929+
availableNames []string
930+
agents []codersdk.WorkspaceAgent
931+
)
929932
for _, resource := range resources {
930-
agents = append(agents, resource.Agents...)
933+
for _, agent := range resource.Agents {
934+
availableNames = append(availableNames, agent.Name)
935+
agents = append(agents, agent)
936+
}
931937
}
932938
if len(agents) == 0 {
933939
return codersdk.WorkspaceAgent{}, xerrors.Errorf("workspace %q has no agents", workspace.Name)
934940
}
941+
slices.Sort(availableNames)
935942
if agentName != "" {
936943
for _, otherAgent := range agents {
937944
if otherAgent.Name != agentName {
938945
continue
939946
}
940-
workspaceAgent = otherAgent
941-
break
942-
}
943-
if workspaceAgent.ID == uuid.Nil {
944-
return codersdk.WorkspaceAgent{}, xerrors.Errorf("agent not found by name %q", agentName)
947+
return otherAgent, nil
945948
}
949+
return codersdk.WorkspaceAgent{}, xerrors.Errorf("agent not found by name %q, available agents: %v", agentName, availableNames)
946950
}
947-
if workspaceAgent.ID == uuid.Nil {
948-
if len(agents) > 1 {
949-
workspaceAgent, err = cryptorand.Element(agents)
950-
if err != nil {
951-
return codersdk.WorkspaceAgent{}, err
952-
}
953-
} else {
954-
workspaceAgent = agents[0]
955-
}
951+
if len(agents) == 1 {
952+
return agents[0], nil
956953
}
957-
return workspaceAgent, nil
954+
return codersdk.WorkspaceAgent{}, xerrors.Errorf("multiple agents found, please specify the agent name, available agents: %v", availableNames)
958955
}
959956

960957
// Attempt to poll workspace autostop. We write a per-workspace lockfile to

0 commit comments

Comments
 (0)