Skip to content

Commit c331dca

Browse files
DanielleMaywoodDanielleMaywood
committed
fix: for child agents, ensure unique across provisioner job
Child agents are a new concept so there are very little existing tests for them. This means we can easily make the change for them without having to update a mountain of tests. So they get a special case for checking properly, whilst parent agents have less scrutiny due to the vast number of tests that appear to not work properly.
1 parent 2aadf9f commit c331dca

File tree

3 files changed

+128
-73
lines changed

3 files changed

+128
-73
lines changed

‎coderd/database/dump.sql

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

‎coderd/database/migrations/000330_workspace_agent_name_unique_trigger.up.sql

Lines changed: 24 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -4,47 +4,37 @@ DECLARE
44
provisioner_job_id uuid;
55
agents_with_name integer;
66
BEGIN
7-
-- Count how many agents in this resource already have
8-
-- the given agent ID.
9-
SELECT COUNT(*) INTO agents_with_name
10-
FROM workspace_agents
11-
WHERE workspace_agents.resource_id = NEW.resource_id
12-
AND workspace_agents.name = NEW.name
13-
AND workspace_agents.id != NEW.id;
7+
IF NEW.parent_id IS NULL THEN
8+
-- Count how many agents in this resource already have
9+
-- the given agent ID.
10+
SELECT COUNT(*) INTO agents_with_name
11+
FROM workspace_agents
12+
WHERE workspace_agents.resource_id = NEW.resource_id
13+
AND workspace_agents.name = NEW.name
14+
AND workspace_agents.id != NEW.id;
15+
ELSE
16+
SELECT provisioner_jobs.id INTO provisioner_job_id
17+
FROM workspace_resources
18+
JOIN provisioner_jobs ON provisioner_jobs.id = workspace_resources.job_id
19+
WHERE workspace_resources.id = NEW.resource_id;
20+
21+
-- Count how many agents in this provisioner job already have
22+
-- the given agent ID.
23+
SELECT COUNT(*) INTO agents_with_name
24+
FROM workspace_agents
25+
JOIN workspace_resources ON workspace_resources.id = workspace_agents.resource_id
26+
JOIN provisioner_jobs ON provisioner_jobs.id = workspace_resources.job_id
27+
WHERE provisioner_jobs.id = provisioner_job_id
28+
AND workspace_agents.name = NEW.name
29+
AND workspace_agents.id != NEW.id;
30+
END IF;
1431

1532
-- If there's already an agent with this name, raise an error
1633
IF agents_with_name > 0 THEN
1734
RAISE EXCEPTION 'workspace agent name "%" already exists in this workspace resource', NEW.name
1835
USING ERRCODE = 'unique_violation';
1936
END IF;
2037

21-
-- Get the provisioner_jobs.id for this agent by following the relationship chain:
22-
-- workspace_agents -> workspace_resources -> provisioner_jobs
23-
-- SELECT pj.id INTO provisioner_job_id
24-
-- FROM workspace_resources wr
25-
-- JOIN provisioner_jobs pj ON wr.job_id = pj.id
26-
-- WHERE wr.id = NEW.resource_id;
27-
28-
-- If we couldn't find a provisioner_job.id, allow the insert (might be a template import or other edge case)
29-
-- IF provisioner_job_id IS NULL THEN
30-
-- RETURN NEW;
31-
-- END IF;
32-
33-
-- Check if there's already an agent with this name for this provisioner job
34-
-- SELECT COUNT(*) INTO existing_count
35-
-- FROM workspace_agents wa
36-
-- JOIN workspace_resources wr ON wa.resource_id = wr.id
37-
-- JOIN provisioner_jobs pj ON wr.job_id = pj.id
38-
-- WHERE pj.id = provisioner_job_id
39-
-- AND wa.name = NEW.name
40-
-- AND wa.id != NEW.id;
41-
42-
-- If there's already an agent with this name, raise an error
43-
-- IF existing_count > 0 THEN
44-
-- RAISE EXCEPTION 'workspace agent name "%" already exists in this provisioner job', NEW.name
45-
-- USING ERRCODE = 'unique_violation';
46-
-- END IF;
47-
4838
RETURN NEW;
4939
END;
5040
$$ LANGUAGE plpgsql;

‎coderd/database/querier_test.go

Lines changed: 80 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4714,7 +4714,7 @@ func TestWorkspaceAgentNameUniqueTrigger(t *testing.T) {
47144714
t.Skip("This test makes use of a database trigger not implemented in dbmem")
47154715
}
47164716

4717-
createWorkspaceWithAgent := func(t *testing.T, db database.Store, org database.Organization, agentName string) (database.WorkspaceTable, database.WorkspaceResource, database.WorkspaceAgent) {
4717+
createWorkspaceWithAgent := func(t *testing.T, db database.Store, org database.Organization, agentName string) (database.WorkspaceBuild, database.WorkspaceResource, database.WorkspaceAgent) {
47184718
t.Helper()
47194719

47204720
user := dbgen.User(t, db, database.User{})
@@ -4750,19 +4750,18 @@ func TestWorkspaceAgentNameUniqueTrigger(t *testing.T) {
47504750
Name: agentName,
47514751
})
47524752

4753-
return workspace, resource, agent
4753+
return build, resource, agent
47544754
}
47554755

4756-
t.Run("DuplicateNamesInSameWorkspaceBuild", func(t *testing.T) {
4756+
t.Run("DuplicateNamesInSameWorkspaceResource", func(t *testing.T) {
47574757
t.Parallel()
47584758

47594759
db, _ := dbtestutil.NewDB(t)
47604760
org := dbgen.Organization(t, db, database.Organization{})
47614761
ctx := testutil.Context(t, testutil.WaitShort)
47624762

47634763
// Given: A workspace with an agent
4764-
_, resource, agent := createWorkspaceWithAgent(t, db, org, "duplicate-agent")
4765-
require.Equal(t, "duplicate-agent", agent.Name)
4764+
_, resource, _ := createWorkspaceWithAgent(t, db, org, "duplicate-agent")
47664765

47674766
// When: Another agent is created for that workspace with the same name.
47684767
_, err := db.InsertWorkspaceAgent(ctx, database.InsertWorkspaceAgentParams{
@@ -4785,6 +4784,82 @@ func TestWorkspaceAgentNameUniqueTrigger(t *testing.T) {
47854784
require.Contains(t, pqErr.Message, `workspace agent name "duplicate-agent" already exists in this workspace resource`)
47864785
})
47874786

4787+
t.Run("DuplicateNamesInSameProvisionerJob", func(t *testing.T) {
4788+
t.Parallel()
4789+
4790+
db, _ := dbtestutil.NewDB(t)
4791+
org := dbgen.Organization(t, db, database.Organization{})
4792+
ctx := testutil.Context(t, testutil.WaitShort)
4793+
4794+
// Given: A workspace with an agent
4795+
_, resource, agent := createWorkspaceWithAgent(t, db, org, "duplicate-agent")
4796+
4797+
// When: A child agent is created for that workspace with the same name.
4798+
_, err := db.InsertWorkspaceAgent(ctx, database.InsertWorkspaceAgentParams{
4799+
ID: uuid.New(),
4800+
CreatedAt: time.Now(),
4801+
UpdatedAt: time.Now(),
4802+
Name: agent.Name,
4803+
ResourceID: resource.ID,
4804+
AuthToken: uuid.New(),
4805+
Architecture: "amd64",
4806+
OperatingSystem: "linux",
4807+
APIKeyScope: database.AgentKeyScopeEnumAll,
4808+
})
4809+
4810+
// Then: We expect it to fail.
4811+
require.Error(t, err)
4812+
var pqErr *pq.Error
4813+
require.True(t, errors.As(err, &pqErr))
4814+
require.Equal(t, pq.ErrorCode("23505"), pqErr.Code) // unique_violation
4815+
require.Contains(t, pqErr.Message, `workspace agent name "duplicate-agent" already exists in this workspace resource`)
4816+
})
4817+
4818+
t.Run("DuplicateChildNamesOverMultipleResources", func(t *testing.T) {
4819+
t.Parallel()
4820+
4821+
db, _ := dbtestutil.NewDB(t)
4822+
org := dbgen.Organization(t, db, database.Organization{})
4823+
ctx := testutil.Context(t, testutil.WaitShort)
4824+
4825+
// Given: A workspace with two agents
4826+
_, resource1, agent1 := createWorkspaceWithAgent(t, db, org, "parent-agent-1")
4827+
4828+
resource2 := dbgen.WorkspaceResource(t, db, database.WorkspaceResource{JobID: resource1.JobID})
4829+
agent2 := dbgen.WorkspaceAgent(t, db, database.WorkspaceAgent{
4830+
ResourceID: resource2.ID,
4831+
Name: "parent-agent-2",
4832+
})
4833+
4834+
// Given: One agent has a child agent
4835+
agent1Child := dbgen.WorkspaceAgent(t, db, database.WorkspaceAgent{
4836+
ParentID: uuid.NullUUID{Valid: true, UUID: agent1.ID},
4837+
Name: "child-agent",
4838+
ResourceID: resource1.ID,
4839+
})
4840+
4841+
// When: A child agent is inserted for the other parent.
4842+
_, err := db.InsertWorkspaceAgent(ctx, database.InsertWorkspaceAgentParams{
4843+
ID: uuid.New(),
4844+
ParentID: uuid.NullUUID{Valid: true, UUID: agent2.ID},
4845+
CreatedAt: time.Now(),
4846+
UpdatedAt: time.Now(),
4847+
Name: agent1Child.Name,
4848+
ResourceID: resource2.ID,
4849+
AuthToken: uuid.New(),
4850+
Architecture: "amd64",
4851+
OperatingSystem: "linux",
4852+
APIKeyScope: database.AgentKeyScopeEnumAll,
4853+
})
4854+
4855+
// Then: We expect it to fail.
4856+
require.Error(t, err)
4857+
var pqErr *pq.Error
4858+
require.True(t, errors.As(err, &pqErr))
4859+
require.Equal(t, pq.ErrorCode("23505"), pqErr.Code) // unique_violation
4860+
require.Contains(t, pqErr.Message, `workspace agent name "child-agent" already exists in this workspace resource`)
4861+
})
4862+
47884863
t.Run("SameNamesInDifferentWorkspaces", func(t *testing.T) {
47894864
t.Parallel()
47904865

0 commit comments

Comments
 (0)