mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
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.
This commit is contained in:
@@ -128,7 +128,7 @@ func (a *SubAgentAPI) CreateSubAgent(ctx context.Context, req *agentproto.Create
|
||||
Name: agentName,
|
||||
ResourceID: parentAgent.ResourceID,
|
||||
AuthToken: uuid.New(),
|
||||
AuthInstanceID: parentAgent.AuthInstanceID,
|
||||
AuthInstanceID: sql.NullString{},
|
||||
Architecture: req.Architecture,
|
||||
EnvironmentVariables: pqtype.NullRawMessage{},
|
||||
OperatingSystem: req.OperatingSystem,
|
||||
|
||||
@@ -175,6 +175,52 @@ func TestSubAgentAPI(t *testing.T) {
|
||||
}
|
||||
})
|
||||
|
||||
// Context: https://github.com/coder/coder/pull/22196
|
||||
t.Run("CreateSubAgentDoesNotInheritAuthInstanceID", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
var (
|
||||
log = testutil.Logger(t)
|
||||
clock = quartz.NewMock(t)
|
||||
|
||||
db, org = newDatabaseWithOrg(t)
|
||||
user, agent = newUserWithWorkspaceAgent(t, db, org)
|
||||
)
|
||||
|
||||
// Given: The parent agent has an AuthInstanceID set
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
parentAgent, err := db.GetWorkspaceAgentByID(dbauthz.AsSystemRestricted(ctx), agent.ID)
|
||||
require.NoError(t, err)
|
||||
require.True(t, parentAgent.AuthInstanceID.Valid, "parent agent should have an AuthInstanceID")
|
||||
require.NotEmpty(t, parentAgent.AuthInstanceID.String)
|
||||
|
||||
api := newAgentAPI(t, log, db, clock, user, org, agent)
|
||||
|
||||
// When: We create a sub agent
|
||||
createResp, err := api.CreateSubAgent(ctx, &proto.CreateSubAgentRequest{
|
||||
Name: "sub-agent",
|
||||
Directory: "/workspaces/test",
|
||||
Architecture: "amd64",
|
||||
OperatingSystem: "linux",
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
subAgentID, err := uuid.FromBytes(createResp.Agent.Id)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Then: The sub-agent must NOT re-use the parent's AuthInstanceID.
|
||||
subAgent, err := db.GetWorkspaceAgentByID(dbauthz.AsSystemRestricted(ctx), subAgentID)
|
||||
require.NoError(t, err)
|
||||
assert.False(t, subAgent.AuthInstanceID.Valid, "sub-agent should not have an AuthInstanceID")
|
||||
assert.Empty(t, subAgent.AuthInstanceID.String, "sub-agent AuthInstanceID string should be empty")
|
||||
|
||||
// Double-check: looking up by the parent's instance ID must
|
||||
// still return the parent, not the sub-agent.
|
||||
lookedUp, err := db.GetWorkspaceAgentByInstanceID(dbauthz.AsSystemRestricted(ctx), parentAgent.AuthInstanceID.String)
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, parentAgent.ID, lookedUp.ID, "instance ID lookup should still return the parent agent")
|
||||
})
|
||||
|
||||
type expectedAppError struct {
|
||||
index int32
|
||||
field string
|
||||
|
||||
@@ -6319,6 +6319,56 @@ func TestGetWorkspaceAgentsByParentID(t *testing.T) {
|
||||
})
|
||||
}
|
||||
|
||||
func TestGetWorkspaceAgentByInstanceID(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// Context: https://github.com/coder/coder/pull/22196
|
||||
t.Run("DoesNotReturnSubAgents", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// Given: A parent workspace agent with an AuthInstanceID and a
|
||||
// sub-agent that shares the same AuthInstanceID.
|
||||
db, _ := dbtestutil.NewDB(t)
|
||||
org := dbgen.Organization(t, db, database.Organization{})
|
||||
job := dbgen.ProvisionerJob(t, db, nil, database.ProvisionerJob{
|
||||
Type: database.ProvisionerJobTypeTemplateVersionImport,
|
||||
OrganizationID: org.ID,
|
||||
})
|
||||
resource := dbgen.WorkspaceResource(t, db, database.WorkspaceResource{
|
||||
JobID: job.ID,
|
||||
})
|
||||
|
||||
authInstanceID := fmt.Sprintf("instance-%s-%d", t.Name(), time.Now().UnixNano())
|
||||
parentAgent := dbgen.WorkspaceAgent(t, db, database.WorkspaceAgent{
|
||||
ResourceID: resource.ID,
|
||||
AuthInstanceID: sql.NullString{
|
||||
String: authInstanceID,
|
||||
Valid: true,
|
||||
},
|
||||
})
|
||||
// Create a sub-agent with the same AuthInstanceID (simulating
|
||||
// the old behavior before the fix).
|
||||
_ = dbgen.WorkspaceAgent(t, db, database.WorkspaceAgent{
|
||||
ParentID: uuid.NullUUID{UUID: parentAgent.ID, Valid: true},
|
||||
ResourceID: resource.ID,
|
||||
AuthInstanceID: sql.NullString{
|
||||
String: authInstanceID,
|
||||
Valid: true,
|
||||
},
|
||||
})
|
||||
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
|
||||
// When: We look up the agent by instance ID.
|
||||
agent, err := db.GetWorkspaceAgentByInstanceID(ctx, authInstanceID)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Then: The result must be the parent agent, not the sub-agent.
|
||||
assert.Equal(t, parentAgent.ID, agent.ID, "instance ID lookup should return the parent agent, not a sub-agent")
|
||||
assert.False(t, agent.ParentID.Valid, "returned agent should not have a parent (should be the parent itself)")
|
||||
})
|
||||
}
|
||||
|
||||
func requireUsersMatch(t testing.TB, expected []database.User, found []database.GetUsersRow, msg string) {
|
||||
t.Helper()
|
||||
require.ElementsMatch(t, expected, database.ConvertUserRows(found), msg)
|
||||
|
||||
@@ -18251,6 +18251,8 @@ WHERE
|
||||
auth_instance_id = $1 :: TEXT
|
||||
-- Filter out deleted sub agents.
|
||||
AND deleted = FALSE
|
||||
-- Filter out sub agents, they do not authenticate with auth_instance_id.
|
||||
AND parent_id IS NULL
|
||||
ORDER BY
|
||||
created_at DESC
|
||||
`
|
||||
|
||||
@@ -17,6 +17,8 @@ WHERE
|
||||
auth_instance_id = @auth_instance_id :: TEXT
|
||||
-- Filter out deleted sub agents.
|
||||
AND deleted = FALSE
|
||||
-- Filter out sub agents, they do not authenticate with auth_instance_id.
|
||||
AND parent_id IS NULL
|
||||
ORDER BY
|
||||
created_at DESC;
|
||||
|
||||
|
||||
@@ -3319,7 +3319,7 @@ func insertDevcontainerSubagent(
|
||||
ResourceID: resourceID,
|
||||
Name: dc.GetName(),
|
||||
AuthToken: uuid.New(),
|
||||
AuthInstanceID: parentAgent.AuthInstanceID,
|
||||
AuthInstanceID: sql.NullString{},
|
||||
Architecture: parentAgent.Architecture,
|
||||
EnvironmentVariables: envJSON,
|
||||
Directory: dc.GetWorkspaceFolder(),
|
||||
|
||||
@@ -4072,6 +4072,54 @@ func TestInsertWorkspaceResource(t *testing.T) {
|
||||
}},
|
||||
},
|
||||
},
|
||||
{
|
||||
// This test verifies that subagents created via
|
||||
// devcontainers do not inherit the parent agent's
|
||||
// AuthInstanceID.
|
||||
// Context: https://github.com/coder/coder/pull/22196
|
||||
name: "SubAgentDoesNotInheritAuthInstanceID",
|
||||
resource: &sdkproto.Resource{
|
||||
Name: "something",
|
||||
Type: "aws_instance",
|
||||
Agents: []*sdkproto.Agent{{
|
||||
Id: agentID.String(),
|
||||
Name: "dev",
|
||||
Architecture: "amd64",
|
||||
OperatingSystem: "linux",
|
||||
Auth: &sdkproto.Agent_InstanceId{
|
||||
InstanceId: "parent-instance-id",
|
||||
},
|
||||
Devcontainers: []*sdkproto.Devcontainer{{
|
||||
Id: devcontainerID.String(),
|
||||
Name: "sub",
|
||||
WorkspaceFolder: "/workspace",
|
||||
SubagentId: subAgentID.String(),
|
||||
Apps: []*sdkproto.App{
|
||||
{Slug: "code-server", DisplayName: "VS Code", Url: "http://localhost:8080"},
|
||||
},
|
||||
}},
|
||||
}},
|
||||
},
|
||||
expectSubAgentCount: 1,
|
||||
check: func(t *testing.T, db database.Store, parentAgent database.WorkspaceAgent, subAgents []database.WorkspaceAgent, _ bool) {
|
||||
// Parent should have the AuthInstanceID set.
|
||||
require.True(t, parentAgent.AuthInstanceID.Valid, "parent agent should have an AuthInstanceID")
|
||||
require.Equal(t, "parent-instance-id", parentAgent.AuthInstanceID.String)
|
||||
|
||||
require.Len(t, subAgents, 1)
|
||||
subAgent := subAgents[0]
|
||||
|
||||
// Sub-agent must NOT inherit the parent's AuthInstanceID.
|
||||
assert.False(t, subAgent.AuthInstanceID.Valid, "sub-agent should not have an AuthInstanceID")
|
||||
assert.Empty(t, subAgent.AuthInstanceID.String, "sub-agent AuthInstanceID string should be empty")
|
||||
|
||||
// Looking up by the parent's instance ID must still
|
||||
// return the parent, not the sub-agent.
|
||||
lookedUp, err := db.GetWorkspaceAgentByInstanceID(ctx, parentAgent.AuthInstanceID.String)
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, parentAgent.ID, lookedUp.ID, "instance ID lookup should still return the parent agent")
|
||||
},
|
||||
},
|
||||
{
|
||||
// This test verifies the backward-compatibility behavior where a
|
||||
// devcontainer with a SubagentId but no apps, scripts, or envs does
|
||||
|
||||
Reference in New Issue
Block a user