Skip to content

Commit 911d734

Browse files
fix: avoid re-using AuthInstanceID for sub agents (#22196)
Parent agents were re-using AuthInstanceID when spawning child agents. This caused GetWorkspaceAgentByInstanceID to return the most recently created sub agent instead of the parent when the parent tried to refetch its own manifest. Fix by not reusing AuthInstanceID for sub agents, and updating GetWorkspaceAgentByInstanceID to filter them out entirely.
1 parent 0f6fbe7 commit 911d734

File tree

7 files changed

+150
-2
lines changed

7 files changed

+150
-2
lines changed

coderd/agentapi/subagent.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ func (a *SubAgentAPI) CreateSubAgent(ctx context.Context, req *agentproto.Create
128128
Name: agentName,
129129
ResourceID: parentAgent.ResourceID,
130130
AuthToken: uuid.New(),
131-
AuthInstanceID: parentAgent.AuthInstanceID,
131+
AuthInstanceID: sql.NullString{},
132132
Architecture: req.Architecture,
133133
EnvironmentVariables: pqtype.NullRawMessage{},
134134
OperatingSystem: req.OperatingSystem,

coderd/agentapi/subagent_test.go

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,52 @@ func TestSubAgentAPI(t *testing.T) {
175175
}
176176
})
177177

178+
// Context: https://github.com/coder/coder/pull/22196
179+
t.Run("CreateSubAgentDoesNotInheritAuthInstanceID", func(t *testing.T) {
180+
t.Parallel()
181+
182+
var (
183+
log = testutil.Logger(t)
184+
clock = quartz.NewMock(t)
185+
186+
db, org = newDatabaseWithOrg(t)
187+
user, agent = newUserWithWorkspaceAgent(t, db, org)
188+
)
189+
190+
// Given: The parent agent has an AuthInstanceID set
191+
ctx := testutil.Context(t, testutil.WaitShort)
192+
parentAgent, err := db.GetWorkspaceAgentByID(dbauthz.AsSystemRestricted(ctx), agent.ID)
193+
require.NoError(t, err)
194+
require.True(t, parentAgent.AuthInstanceID.Valid, "parent agent should have an AuthInstanceID")
195+
require.NotEmpty(t, parentAgent.AuthInstanceID.String)
196+
197+
api := newAgentAPI(t, log, db, clock, user, org, agent)
198+
199+
// When: We create a sub agent
200+
createResp, err := api.CreateSubAgent(ctx, &proto.CreateSubAgentRequest{
201+
Name: "sub-agent",
202+
Directory: "/workspaces/test",
203+
Architecture: "amd64",
204+
OperatingSystem: "linux",
205+
})
206+
require.NoError(t, err)
207+
208+
subAgentID, err := uuid.FromBytes(createResp.Agent.Id)
209+
require.NoError(t, err)
210+
211+
// Then: The sub-agent must NOT re-use the parent's AuthInstanceID.
212+
subAgent, err := db.GetWorkspaceAgentByID(dbauthz.AsSystemRestricted(ctx), subAgentID)
213+
require.NoError(t, err)
214+
assert.False(t, subAgent.AuthInstanceID.Valid, "sub-agent should not have an AuthInstanceID")
215+
assert.Empty(t, subAgent.AuthInstanceID.String, "sub-agent AuthInstanceID string should be empty")
216+
217+
// Double-check: looking up by the parent's instance ID must
218+
// still return the parent, not the sub-agent.
219+
lookedUp, err := db.GetWorkspaceAgentByInstanceID(dbauthz.AsSystemRestricted(ctx), parentAgent.AuthInstanceID.String)
220+
require.NoError(t, err)
221+
assert.Equal(t, parentAgent.ID, lookedUp.ID, "instance ID lookup should still return the parent agent")
222+
})
223+
178224
type expectedAppError struct {
179225
index int32
180226
field string

coderd/database/querier_test.go

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6319,6 +6319,56 @@ func TestGetWorkspaceAgentsByParentID(t *testing.T) {
63196319
})
63206320
}
63216321

6322+
func TestGetWorkspaceAgentByInstanceID(t *testing.T) {
6323+
t.Parallel()
6324+
6325+
// Context: https://github.com/coder/coder/pull/22196
6326+
t.Run("DoesNotReturnSubAgents", func(t *testing.T) {
6327+
t.Parallel()
6328+
6329+
// Given: A parent workspace agent with an AuthInstanceID and a
6330+
// sub-agent that shares the same AuthInstanceID.
6331+
db, _ := dbtestutil.NewDB(t)
6332+
org := dbgen.Organization(t, db, database.Organization{})
6333+
job := dbgen.ProvisionerJob(t, db, nil, database.ProvisionerJob{
6334+
Type: database.ProvisionerJobTypeTemplateVersionImport,
6335+
OrganizationID: org.ID,
6336+
})
6337+
resource := dbgen.WorkspaceResource(t, db, database.WorkspaceResource{
6338+
JobID: job.ID,
6339+
})
6340+
6341+
authInstanceID := fmt.Sprintf("instance-%s-%d", t.Name(), time.Now().UnixNano())
6342+
parentAgent := dbgen.WorkspaceAgent(t, db, database.WorkspaceAgent{
6343+
ResourceID: resource.ID,
6344+
AuthInstanceID: sql.NullString{
6345+
String: authInstanceID,
6346+
Valid: true,
6347+
},
6348+
})
6349+
// Create a sub-agent with the same AuthInstanceID (simulating
6350+
// the old behavior before the fix).
6351+
_ = dbgen.WorkspaceAgent(t, db, database.WorkspaceAgent{
6352+
ParentID: uuid.NullUUID{UUID: parentAgent.ID, Valid: true},
6353+
ResourceID: resource.ID,
6354+
AuthInstanceID: sql.NullString{
6355+
String: authInstanceID,
6356+
Valid: true,
6357+
},
6358+
})
6359+
6360+
ctx := testutil.Context(t, testutil.WaitShort)
6361+
6362+
// When: We look up the agent by instance ID.
6363+
agent, err := db.GetWorkspaceAgentByInstanceID(ctx, authInstanceID)
6364+
require.NoError(t, err)
6365+
6366+
// Then: The result must be the parent agent, not the sub-agent.
6367+
assert.Equal(t, parentAgent.ID, agent.ID, "instance ID lookup should return the parent agent, not a sub-agent")
6368+
assert.False(t, agent.ParentID.Valid, "returned agent should not have a parent (should be the parent itself)")
6369+
})
6370+
}
6371+
63226372
func requireUsersMatch(t testing.TB, expected []database.User, found []database.GetUsersRow, msg string) {
63236373
t.Helper()
63246374
require.ElementsMatch(t, expected, database.ConvertUserRows(found), msg)

coderd/database/queries.sql.go

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

coderd/database/queries/workspaceagents.sql

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ WHERE
1717
auth_instance_id = @auth_instance_id :: TEXT
1818
-- Filter out deleted sub agents.
1919
AND deleted = FALSE
20+
-- Filter out sub agents, they do not authenticate with auth_instance_id.
21+
AND parent_id IS NULL
2022
ORDER BY
2123
created_at DESC;
2224

coderd/provisionerdserver/provisionerdserver.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3319,7 +3319,7 @@ func insertDevcontainerSubagent(
33193319
ResourceID: resourceID,
33203320
Name: dc.GetName(),
33213321
AuthToken: uuid.New(),
3322-
AuthInstanceID: parentAgent.AuthInstanceID,
3322+
AuthInstanceID: sql.NullString{},
33233323
Architecture: parentAgent.Architecture,
33243324
EnvironmentVariables: envJSON,
33253325
Directory: dc.GetWorkspaceFolder(),

coderd/provisionerdserver/provisionerdserver_test.go

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4072,6 +4072,54 @@ func TestInsertWorkspaceResource(t *testing.T) {
40724072
}},
40734073
},
40744074
},
4075+
{
4076+
// This test verifies that subagents created via
4077+
// devcontainers do not inherit the parent agent's
4078+
// AuthInstanceID.
4079+
// Context: https://github.com/coder/coder/pull/22196
4080+
name: "SubAgentDoesNotInheritAuthInstanceID",
4081+
resource: &sdkproto.Resource{
4082+
Name: "something",
4083+
Type: "aws_instance",
4084+
Agents: []*sdkproto.Agent{{
4085+
Id: agentID.String(),
4086+
Name: "dev",
4087+
Architecture: "amd64",
4088+
OperatingSystem: "linux",
4089+
Auth: &sdkproto.Agent_InstanceId{
4090+
InstanceId: "parent-instance-id",
4091+
},
4092+
Devcontainers: []*sdkproto.Devcontainer{{
4093+
Id: devcontainerID.String(),
4094+
Name: "sub",
4095+
WorkspaceFolder: "/workspace",
4096+
SubagentId: subAgentID.String(),
4097+
Apps: []*sdkproto.App{
4098+
{Slug: "code-server", DisplayName: "VS Code", Url: "http://localhost:8080"},
4099+
},
4100+
}},
4101+
}},
4102+
},
4103+
expectSubAgentCount: 1,
4104+
check: func(t *testing.T, db database.Store, parentAgent database.WorkspaceAgent, subAgents []database.WorkspaceAgent, _ bool) {
4105+
// Parent should have the AuthInstanceID set.
4106+
require.True(t, parentAgent.AuthInstanceID.Valid, "parent agent should have an AuthInstanceID")
4107+
require.Equal(t, "parent-instance-id", parentAgent.AuthInstanceID.String)
4108+
4109+
require.Len(t, subAgents, 1)
4110+
subAgent := subAgents[0]
4111+
4112+
// Sub-agent must NOT inherit the parent's AuthInstanceID.
4113+
assert.False(t, subAgent.AuthInstanceID.Valid, "sub-agent should not have an AuthInstanceID")
4114+
assert.Empty(t, subAgent.AuthInstanceID.String, "sub-agent AuthInstanceID string should be empty")
4115+
4116+
// Looking up by the parent's instance ID must still
4117+
// return the parent, not the sub-agent.
4118+
lookedUp, err := db.GetWorkspaceAgentByInstanceID(ctx, parentAgent.AuthInstanceID.String)
4119+
require.NoError(t, err)
4120+
assert.Equal(t, parentAgent.ID, lookedUp.ID, "instance ID lookup should still return the parent agent")
4121+
},
4122+
},
40754123
{
40764124
// This test verifies the backward-compatibility behavior where a
40774125
// devcontainer with a SubagentId but no apps, scripts, or envs does

0 commit comments

Comments
 (0)