From 3fddfef879f2c53f8bf2e8ec0d56c1f4b42bea28 Mon Sep 17 00:00:00 2001 From: Ethan <39577870+ethanndickson@users.noreply.github.com> Date: Thu, 20 Feb 2025 12:51:25 +1100 Subject: [PATCH] fix!: enforce agent names be case-insensitive-unique per-workspace (#16614) Relates to https://github.com/coder/coder-desktop-macos/issues/54 Currently, it's possible to have two agents within the same workspace whose names only differ in capitalization: This leads to an ambiguity in two cases: - For CoderVPN, we'd like to allow support to workspaces with a hostname of the form: `agent.workspace.username.coder`. - Workspace apps (`coder_app`s) currently use subdomains of the form: `------(--)?`. Of note is that DNS hosts must be strictly lower case, hence the ambiguity. This fix is technically a breaking change, but only for the incredibly rare use case where a user has: - A workspace with two agents - Those agent names differ only in capitalization. Those templates & workspaces will now fail to build. This can be fixed by choosing wholly unique names for the agents. --- .../provisionerdserver/provisionerdserver.go | 6 ++-- .../provisionerdserver_test.go | 26 +++++++++++++++ provisioner/terraform/resources.go | 6 ++-- provisioner/terraform/resources_test.go | 33 +++++++++++++++++++ 4 files changed, 67 insertions(+), 4 deletions(-) diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index b928be1b52..1734ea53f0 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -1891,10 +1891,12 @@ func InsertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid. appSlugs = make(map[string]struct{}) ) for _, prAgent := range protoResource.Agents { - if _, ok := agentNames[prAgent.Name]; ok { + // Agent names must be case-insensitive-unique, to be unambiguous in + // `coder_app`s and CoderVPN DNS names. + if _, ok := agentNames[strings.ToLower(prAgent.Name)]; ok { return xerrors.Errorf("duplicate agent name %q", prAgent.Name) } - agentNames[prAgent.Name] = struct{}{} + agentNames[strings.ToLower(prAgent.Name)] = struct{}{} var instanceID sql.NullString if prAgent.GetInstanceId() != "" { diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index 21ba8c6fad..9f18260745 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -1905,6 +1905,32 @@ func TestInsertWorkspaceResource(t *testing.T) { }) require.ErrorContains(t, err, "duplicate app slug") }) + t.Run("DuplicateAgentNames", func(t *testing.T) { + t.Parallel() + db := dbmem.New() + job := uuid.New() + // case-insensitive-unique + err := insert(db, job, &sdkproto.Resource{ + Name: "something", + Type: "aws_instance", + Agents: []*sdkproto.Agent{{ + Name: "dev", + }, { + Name: "Dev", + }}, + }) + require.ErrorContains(t, err, "duplicate agent name") + err = insert(db, job, &sdkproto.Resource{ + Name: "something", + Type: "aws_instance", + Agents: []*sdkproto.Agent{{ + Name: "dev", + }, { + Name: "dev", + }}, + }) + require.ErrorContains(t, err, "duplicate agent name") + }) t.Run("Success", func(t *testing.T) { t.Parallel() db := dbmem.New() diff --git a/provisioner/terraform/resources.go b/provisioner/terraform/resources.go index 77c92da87b..65a0a1a988 100644 --- a/provisioner/terraform/resources.go +++ b/provisioner/terraform/resources.go @@ -215,10 +215,12 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s return nil, xerrors.Errorf("decode agent attributes: %w", err) } - if _, ok := agentNames[tfResource.Name]; ok { + // Agent names must be case-insensitive-unique, to be unambiguous in + // `coder_app`s and CoderVPN DNS names. + if _, ok := agentNames[strings.ToLower(tfResource.Name)]; ok { return nil, xerrors.Errorf("duplicate agent name: %s", tfResource.Name) } - agentNames[tfResource.Name] = struct{}{} + agentNames[strings.ToLower(tfResource.Name)] = struct{}{} // Handling for deprecated attributes. login_before_ready was replaced // by startup_script_behavior, but we still need to support it for diff --git a/provisioner/terraform/resources_test.go b/provisioner/terraform/resources_test.go index 1f1a03dfae..7527ba6dac 100644 --- a/provisioner/terraform/resources_test.go +++ b/provisioner/terraform/resources_test.go @@ -1026,6 +1026,39 @@ func TestAppSlugValidation(t *testing.T) { require.ErrorContains(t, err, "duplicate app slug") } +func TestAgentNameDuplicate(t *testing.T) { + t.Parallel() + ctx, logger := ctxAndLogger(t) + + // nolint:dogsled + _, filename, _, _ := runtime.Caller(0) + + dir := filepath.Join(filepath.Dir(filename), "testdata", "multiple-agents") + tfPlanRaw, err := os.ReadFile(filepath.Join(dir, "multiple-agents.tfplan.json")) + require.NoError(t, err) + var tfPlan tfjson.Plan + err = json.Unmarshal(tfPlanRaw, &tfPlan) + require.NoError(t, err) + tfPlanGraph, err := os.ReadFile(filepath.Join(dir, "multiple-agents.tfplan.dot")) + require.NoError(t, err) + + for _, resource := range tfPlan.PlannedValues.RootModule.Resources { + if resource.Type == "coder_agent" { + switch resource.Name { + case "dev1": + resource.Name = "dev" + case "dev2": + resource.Name = "Dev" + } + } + } + + state, err := terraform.ConvertState(ctx, []*tfjson.StateModule{tfPlan.PlannedValues.RootModule}, string(tfPlanGraph), logger) + require.Nil(t, state) + require.Error(t, err) + require.ErrorContains(t, err, "duplicate agent name") +} + func TestMetadataResourceDuplicate(t *testing.T) { t.Parallel() ctx, logger := ctxAndLogger(t)