From d2c3a144b9cf454a37feedddb9c44d079ff2922f Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Thu, 20 Feb 2025 04:14:24 +0000 Subject: [PATCH 1/2] fix!: enforce regex for agent names Underscores and double hyphens are now blocked. The regex is almost the exact same as the `coder_app` `slug` regex, but uppercase characters are still permitted. --- coderd/database/dbauthz/dbauthz_test.go | 3 +- coderd/database/dbfake/dbfake.go | 3 +- .../provisionerdserver/provisionerdserver.go | 16 +++ .../provisionerdserver_test.go | 90 +++++++++++++- coderd/templateversions_test.go | 4 +- coderd/workspaceagents_test.go | 3 +- coderd/workspacebuilds_test.go | 1 + coderd/workspaceresourceauth_test.go | 3 + coderd/workspaces_test.go | 11 +- enterprise/coderd/templates_test.go | 4 +- provisioner/appslug.go | 13 --- provisioner/appslug_test.go | 64 ---------- provisioner/regexes.go | 31 +++++ provisioner/regexes_test.go | 88 ++++++++++++++ provisioner/terraform/resources.go | 18 ++- provisioner/terraform/resources_test.go | 110 +++++++++++++++--- site/site.go | 2 +- 17 files changed, 363 insertions(+), 101 deletions(-) delete mode 100644 provisioner/appslug.go delete mode 100644 provisioner/appslug_test.go create mode 100644 provisioner/regexes.go create mode 100644 provisioner/regexes_test.go diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 3bf63c3300f13..c960f06c65f1b 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -3918,7 +3918,8 @@ func (s *MethodTestSuite) TestSystemFunctions() { s.Run("InsertWorkspaceAgent", s.Subtest(func(db database.Store, check *expects) { dbtestutil.DisableForeignKeysAndTriggers(s.T(), db) check.Args(database.InsertWorkspaceAgentParams{ - ID: uuid.New(), + ID: uuid.New(), + Name: "dev", }).Asserts(rbac.ResourceSystem, policy.ActionCreate) })) s.Run("InsertWorkspaceApp", s.Subtest(func(db database.Store, check *expects) { diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 9c5a09f40ff65..197502ebac42c 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -91,7 +91,8 @@ func (b WorkspaceBuildBuilder) WithAgent(mutations ...func([]*sdkproto.Agent) [] //nolint: revive // returns modified struct b.agentToken = uuid.NewString() agents := []*sdkproto.Agent{{ - Id: uuid.NewString(), + Id: uuid.NewString(), + Name: "dev", Auth: &sdkproto.Agent_Token{ Token: b.agentToken, }, diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 1734ea53f0782..f431805a350a1 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -1891,6 +1891,19 @@ func InsertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid. appSlugs = make(map[string]struct{}) ) for _, prAgent := range protoResource.Agents { + // Similar logic is duplicated in terraform/resources.go. + if prAgent.Name == "" { + return xerrors.Errorf("agent name cannot be empty") + } + // In 2025-02 we removed support for underscores in agent names. To + // provide a nicer error message, we check the regex first and check + // for underscores if it fails. + if !provisioner.AgentNameRegex.MatchString(prAgent.Name) { + if strings.Contains(prAgent.Name, "_") { + return xerrors.Errorf("agent name %q contains underscores which are no longer supported, please use hyphens instead (regex: %q)", prAgent.Name, provisioner.AgentNameRegex.String()) + } + return xerrors.Errorf("agent name %q does not match regex %q", prAgent.Name, provisioner.AgentNameRegex.String()) + } // 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 { @@ -2070,10 +2083,13 @@ func InsertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid. } for _, app := range prAgent.Apps { + // Similar logic is duplicated in terraform/resources.go. slug := app.Slug if slug == "" { return xerrors.Errorf("app must have a slug or name set") } + // Contrary to agent names above, app slugs were never permitted to + // contain uppercase letters or underscores. if !provisioner.AppSlugRegex.MatchString(slug) { return xerrors.Errorf("app slug %q does not match regex %q", slug, provisioner.AppSlugRegex.String()) } diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index 9f18260745d5a..cc73089e82b63 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -1883,6 +1883,7 @@ func TestInsertWorkspaceResource(t *testing.T) { Name: "something", Type: "aws_instance", Agents: []*sdkproto.Agent{{ + Name: "dev", Auth: &sdkproto.Agent_Token{ Token: "bananas", }, @@ -1896,6 +1897,7 @@ func TestInsertWorkspaceResource(t *testing.T) { Name: "something", Type: "aws_instance", Agents: []*sdkproto.Agent{{ + Name: "dev", Apps: []*sdkproto.App{{ Slug: "a", }, { @@ -1903,7 +1905,61 @@ func TestInsertWorkspaceResource(t *testing.T) { }}, }}, }) - require.ErrorContains(t, err, "duplicate app slug") + require.ErrorContains(t, err, `duplicate app slug, must be unique per template: "a"`) + err = insert(dbmem.New(), uuid.New(), &sdkproto.Resource{ + Name: "something", + Type: "aws_instance", + Agents: []*sdkproto.Agent{{ + Name: "dev1", + Apps: []*sdkproto.App{{ + Slug: "a", + }}, + }, { + Name: "dev2", + Apps: []*sdkproto.App{{ + Slug: "a", + }}, + }}, + }) + require.ErrorContains(t, err, `duplicate app slug, must be unique per template: "a"`) + }) + t.Run("AppSlugInvalid", func(t *testing.T) { + t.Parallel() + db := dbmem.New() + job := uuid.New() + err := insert(db, job, &sdkproto.Resource{ + Name: "something", + Type: "aws_instance", + Agents: []*sdkproto.Agent{{ + Name: "dev", + Apps: []*sdkproto.App{{ + Slug: "dev_1", + }}, + }}, + }) + require.ErrorContains(t, err, `app slug "dev_1" does not match regex`) + err = insert(db, job, &sdkproto.Resource{ + Name: "something", + Type: "aws_instance", + Agents: []*sdkproto.Agent{{ + Name: "dev", + Apps: []*sdkproto.App{{ + Slug: "dev--1", + }}, + }}, + }) + require.ErrorContains(t, err, `app slug "dev--1" does not match regex`) + err = insert(db, job, &sdkproto.Resource{ + Name: "something", + Type: "aws_instance", + Agents: []*sdkproto.Agent{{ + Name: "dev", + Apps: []*sdkproto.App{{ + Slug: "Dev", + }}, + }}, + }) + require.ErrorContains(t, err, `app slug "Dev" does not match regex`) }) t.Run("DuplicateAgentNames", func(t *testing.T) { t.Parallel() @@ -1931,6 +1987,35 @@ func TestInsertWorkspaceResource(t *testing.T) { }) require.ErrorContains(t, err, "duplicate agent name") }) + t.Run("AgentNameInvalid", func(t *testing.T) { + t.Parallel() + db := dbmem.New() + job := uuid.New() + err := insert(db, job, &sdkproto.Resource{ + Name: "something", + Type: "aws_instance", + Agents: []*sdkproto.Agent{{ + Name: "Dev", + }}, + }) + require.NoError(t, err) // uppercase is still allowed + err = insert(db, job, &sdkproto.Resource{ + Name: "something", + Type: "aws_instance", + Agents: []*sdkproto.Agent{{ + Name: "dev_1", + }}, + }) + require.ErrorContains(t, err, `agent name "dev_1" contains underscores`) // custom error for underscores + err = insert(db, job, &sdkproto.Resource{ + Name: "something", + Type: "aws_instance", + Agents: []*sdkproto.Agent{{ + Name: "dev--1", + }}, + }) + require.ErrorContains(t, err, `agent name "dev--1" does not match regex`) + }) t.Run("Success", func(t *testing.T) { t.Parallel() db := dbmem.New() @@ -2007,6 +2092,7 @@ func TestInsertWorkspaceResource(t *testing.T) { Name: "something", Type: "aws_instance", Agents: []*sdkproto.Agent{{ + Name: "dev", DisplayApps: &sdkproto.DisplayApps{ Vscode: true, VscodeInsiders: true, @@ -2035,6 +2121,7 @@ func TestInsertWorkspaceResource(t *testing.T) { Name: "something", Type: "aws_instance", Agents: []*sdkproto.Agent{{ + Name: "dev", DisplayApps: &sdkproto.DisplayApps{}, }}, }) @@ -2059,6 +2146,7 @@ func TestInsertWorkspaceResource(t *testing.T) { Name: "something", Type: "aws_instance", Agents: []*sdkproto.Agent{{ + Name: "dev", DisplayApps: &sdkproto.DisplayApps{}, ResourcesMonitoring: &sdkproto.ResourcesMonitoring{ Memory: &sdkproto.MemoryResourceMonitor{ diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index 9fd1bf6e2d830..4e3e3d2f7f2b0 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -829,6 +829,7 @@ func TestTemplateVersionResources(t *testing.T) { Type: "example", Agents: []*proto.Agent{{ Id: "something", + Name: "dev", Auth: &proto.Agent_Token{}, }}, }, { @@ -875,7 +876,8 @@ func TestTemplateVersionLogs(t *testing.T) { Name: "some", Type: "example", Agents: []*proto.Agent{{ - Id: "something", + Id: "something", + Name: "dev", Auth: &proto.Agent_Token{ Token: uuid.NewString(), }, diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index cdb33e08a54aa..69bba9d8baabd 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -393,7 +393,8 @@ func TestWorkspaceAgentConnectRPC(t *testing.T) { Name: "example", Type: "aws_instance", Agents: []*proto.Agent{{ - Id: uuid.NewString(), + Id: uuid.NewString(), + Name: "dev", Auth: &proto.Agent_Token{ Token: uuid.NewString(), }, diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index fc8961a8c74ac..f6bfcfd2ead28 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -720,6 +720,7 @@ func TestWorkspaceBuildLogs(t *testing.T) { Type: "example", Agents: []*proto.Agent{{ Id: "something", + Name: "dev", Auth: &proto.Agent_Token{}, }}, }, { diff --git a/coderd/workspaceresourceauth_test.go b/coderd/workspaceresourceauth_test.go index d653231ab90d6..8c1b64feaf59a 100644 --- a/coderd/workspaceresourceauth_test.go +++ b/coderd/workspaceresourceauth_test.go @@ -33,6 +33,7 @@ func TestPostWorkspaceAuthAzureInstanceIdentity(t *testing.T) { Name: "somename", Type: "someinstance", Agents: []*proto.Agent{{ + Name: "dev", Auth: &proto.Agent_InstanceId{ InstanceId: instanceID, }, @@ -78,6 +79,7 @@ func TestPostWorkspaceAuthAWSInstanceIdentity(t *testing.T) { Name: "somename", Type: "someinstance", Agents: []*proto.Agent{{ + Name: "dev", Auth: &proto.Agent_InstanceId{ InstanceId: instanceID, }, @@ -164,6 +166,7 @@ func TestPostWorkspaceAuthGoogleInstanceIdentity(t *testing.T) { Name: "somename", Type: "someinstance", Agents: []*proto.Agent{{ + Name: "dev", Auth: &proto.Agent_InstanceId{ InstanceId: instanceID, }, diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index b8bf71c3eb3ac..7a81d5192668f 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -219,6 +219,7 @@ func TestWorkspace(t *testing.T) { Type: "example", Agents: []*proto.Agent{{ Id: uuid.NewString(), + Name: "dev", Auth: &proto.Agent_Token{}, }}, }}, @@ -259,6 +260,7 @@ func TestWorkspace(t *testing.T) { Type: "example", Agents: []*proto.Agent{{ Id: uuid.NewString(), + Name: "dev", Auth: &proto.Agent_Token{}, ConnectionTimeoutSeconds: 1, }}, @@ -1722,7 +1724,8 @@ func TestWorkspaceFilterManual(t *testing.T) { Name: "example", Type: "aws_instance", Agents: []*proto.Agent{{ - Id: uuid.NewString(), + Id: uuid.NewString(), + Name: "dev", Auth: &proto.Agent_Token{ Token: authToken, }, @@ -2729,7 +2732,8 @@ func TestWorkspaceWatcher(t *testing.T) { Name: "example", Type: "aws_instance", Agents: []*proto.Agent{{ - Id: uuid.NewString(), + Id: uuid.NewString(), + Name: "dev", Auth: &proto.Agent_Token{ Token: authToken, }, @@ -2951,6 +2955,7 @@ func TestWorkspaceResource(t *testing.T) { Type: "example", Agents: []*proto.Agent{{ Id: "something", + Name: "dev", Auth: &proto.Agent_Token{}, Apps: apps, }}, @@ -3025,6 +3030,7 @@ func TestWorkspaceResource(t *testing.T) { Type: "example", Agents: []*proto.Agent{{ Id: "something", + Name: "dev", Auth: &proto.Agent_Token{}, Apps: apps, }}, @@ -3068,6 +3074,7 @@ func TestWorkspaceResource(t *testing.T) { Type: "example", Agents: []*proto.Agent{{ Id: "something", + Name: "dev", Auth: &proto.Agent_Token{}, }}, Metadata: []*proto.Resource_Metadata{{ diff --git a/enterprise/coderd/templates_test.go b/enterprise/coderd/templates_test.go index 30225ced30892..a40ed7b64a6db 100644 --- a/enterprise/coderd/templates_test.go +++ b/enterprise/coderd/templates_test.go @@ -161,11 +161,11 @@ func TestTemplates(t *testing.T) { Name: "some", Type: "example", Agents: []*proto.Agent{{ - Id: "something", + Id: "something", + Name: "test", Auth: &proto.Agent_Token{ Token: uuid.NewString(), }, - Name: "test", }}, }, { Name: "another", diff --git a/provisioner/appslug.go b/provisioner/appslug.go deleted file mode 100644 index a13fa4eb2dc9e..0000000000000 --- a/provisioner/appslug.go +++ /dev/null @@ -1,13 +0,0 @@ -package provisioner - -import "regexp" - -// AppSlugRegex is the regex used to validate the slug of a coder_app -// resource. It must be a valid hostname and cannot contain two consecutive -// hyphens or start/end with a hyphen. -// -// This regex is duplicated in the terraform provider code, so make sure to -// update it there as well. -// -// There are test cases for this regex in appslug_test.go. -var AppSlugRegex = regexp.MustCompile(`^[a-z0-9](-?[a-z0-9])*$`) diff --git a/provisioner/appslug_test.go b/provisioner/appslug_test.go deleted file mode 100644 index f13f220e9c63c..0000000000000 --- a/provisioner/appslug_test.go +++ /dev/null @@ -1,64 +0,0 @@ -package provisioner_test - -import ( - "testing" - - "github.com/stretchr/testify/require" - - "github.com/coder/coder/v2/provisioner" -) - -func TestValidAppSlugRegex(t *testing.T) { - t.Parallel() - - t.Run("Valid", func(t *testing.T) { - t.Parallel() - - validStrings := []string{ - "a", - "1", - "a1", - "1a", - "1a1", - "1-1", - "a-a", - "ab-cd", - "ab-cd-ef", - "abc-123", - "a-123", - "abc-1", - "ab-c", - "a-bc", - } - - for _, s := range validStrings { - require.True(t, provisioner.AppSlugRegex.MatchString(s), s) - } - }) - - t.Run("Invalid", func(t *testing.T) { - t.Parallel() - - invalidStrings := []string{ - "", - "-", - "-abc", - "abc-", - "ab--cd", - "a--bc", - "ab--c", - "_", - "ab_cd", - "_abc", - "abc_", - " ", - "abc ", - " abc", - "ab cd", - } - - for _, s := range invalidStrings { - require.False(t, provisioner.AppSlugRegex.MatchString(s), s) - } - }) -} diff --git a/provisioner/regexes.go b/provisioner/regexes.go new file mode 100644 index 0000000000000..fe4db3e9e9e6a --- /dev/null +++ b/provisioner/regexes.go @@ -0,0 +1,31 @@ +package provisioner + +import "regexp" + +var ( + // AgentNameRegex is the regex used to validate the name of a coder_agent + // resource. It must be a valid hostname and cannot contain two consecutive + // hyphens or start/end with a hyphen. Uppercase characters ARE permitted, + // although duplicate agent names with different casing will be rejected. + // + // Previously, underscores were permitted, but this was changed in 2025-02. + // App URLs never supported underscores, and proxy requests to apps on + // agents with underscores in the name always failed. + // + // Due to terraform limitations, this cannot be validated at the provider + // level as resource names cannot be read from the provider API, so this is + // not duplicated in the terraform provider code. + // + // There are test cases for this regex in regexes_test.go. + AgentNameRegex = regexp.MustCompile(`(?i)^[a-z0-9](-?[a-z0-9])*$`) + + // AppSlugRegex is the regex used to validate the slug of a coder_app + // resource. It must be a valid hostname and cannot contain two consecutive + // hyphens or start/end with a hyphen. + // + // This regex is duplicated in the terraform provider code, so make sure to + // update it there as well. + // + // There are test cases for this regex in regexes_test.go. + AppSlugRegex = regexp.MustCompile(`^[a-z0-9](-?[a-z0-9])*$`) +) diff --git a/provisioner/regexes_test.go b/provisioner/regexes_test.go new file mode 100644 index 0000000000000..d8c69f9b67156 --- /dev/null +++ b/provisioner/regexes_test.go @@ -0,0 +1,88 @@ +package provisioner_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/provisioner" +) + +var ( + validStrings = []string{ + "a", + "1", + "a1", + "1a", + "1a1", + "1-1", + "a-a", + "ab-cd", + "ab-cd-ef", + "abc-123", + "a-123", + "abc-1", + "ab-c", + "a-bc", + } + + invalidStrings = []string{ + "", + "-", + "-abc", + "abc-", + "ab--cd", + "a--bc", + "ab--c", + "_", + "ab_cd", + "_abc", + "abc_", + " ", + "abc ", + " abc", + "ab cd", + } + + uppercaseStrings = []string{ + "A", + "A1", + "1A", + } +) + +func TestAgentNameRegex(t *testing.T) { + t.Parallel() + + t.Run("Valid", func(t *testing.T) { + t.Parallel() + for _, s := range append(validStrings, uppercaseStrings...) { + require.True(t, provisioner.AgentNameRegex.MatchString(s), s) + } + }) + + t.Run("Invalid", func(t *testing.T) { + t.Parallel() + for _, s := range invalidStrings { + require.False(t, provisioner.AgentNameRegex.MatchString(s), s) + } + }) +} + +func TestAppSlugRegex(t *testing.T) { + t.Parallel() + + t.Run("Valid", func(t *testing.T) { + t.Parallel() + for _, s := range validStrings { + require.True(t, provisioner.AppSlugRegex.MatchString(s), s) + } + }) + + t.Run("Invalid", func(t *testing.T) { + t.Parallel() + for _, s := range append(invalidStrings, uppercaseStrings...) { + require.False(t, provisioner.AppSlugRegex.MatchString(s), s) + } + }) +} diff --git a/provisioner/terraform/resources.go b/provisioner/terraform/resources.go index 65a0a1a988752..b3e71d452d51a 100644 --- a/provisioner/terraform/resources.go +++ b/provisioner/terraform/resources.go @@ -215,6 +215,19 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s return nil, xerrors.Errorf("decode agent attributes: %w", err) } + // Similar logic is duplicated in terraform/resources.go. + if tfResource.Name == "" { + return nil, xerrors.Errorf("agent name cannot be empty") + } + // In 2025-02 we removed support for underscores in agent names. To + // provide a nicer error message, we check the regex first and check + // for underscores if it fails. + if !provisioner.AgentNameRegex.MatchString(tfResource.Name) { + if strings.Contains(tfResource.Name, "_") { + return nil, xerrors.Errorf("agent name %q contains underscores which are no longer supported, please use hyphens instead (regex: %q)", tfResource.Name, provisioner.AgentNameRegex.String()) + } + return nil, xerrors.Errorf("agent name %q does not match regex %q", tfResource.Name, provisioner.AgentNameRegex.String()) + } // 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 { @@ -443,6 +456,7 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s if attrs.Slug == "" { attrs.Slug = resource.Name } + // Similar logic is duplicated in terraform/resources.go. if attrs.DisplayName == "" { if attrs.Name != "" { // Name is deprecated but still accepted. @@ -452,8 +466,10 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s } } + // Contrary to agent names above, app slugs were never permitted to + // contain uppercase letters or underscores. if !provisioner.AppSlugRegex.MatchString(attrs.Slug) { - return nil, xerrors.Errorf("invalid app slug %q, please update your coder/coder provider to the latest version and specify the slug property on each coder_app", attrs.Slug) + return nil, xerrors.Errorf("app slug %q does not match regex %q", attrs.Slug, provisioner.AppSlugRegex.String()) } if _, exists := appSlugs[attrs.Slug]; exists { diff --git a/provisioner/terraform/resources_test.go b/provisioner/terraform/resources_test.go index 7527ba6dacd5a..0f6efb2e00fe4 100644 --- a/provisioner/terraform/resources_test.go +++ b/provisioner/terraform/resources_test.go @@ -1001,31 +1001,115 @@ func TestAppSlugValidation(t *testing.T) { tfPlanGraph, err := os.ReadFile(filepath.Join(dir, "multiple-apps.tfplan.dot")) require.NoError(t, err) - // Change all slugs to be invalid. - for _, resource := range tfPlan.PlannedValues.RootModule.Resources { - if resource.Type == "coder_app" { - resource.AttributeValues["slug"] = "$$$ invalid slug $$$" - } + cases := []struct { + slug string + errContains string + }{ + {slug: "$$$ invalid slug $$$", errContains: "does not match regex"}, + {slug: "invalid--slug", errContains: "does not match regex"}, + {slug: "invalid_slug", errContains: "does not match regex"}, + {slug: "Invalid-slug", errContains: "does not match regex"}, + {slug: "valid", errContains: ""}, } - 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, "invalid app slug") + //nolint:paralleltest + for i, c := range cases { + c := c + t.Run(fmt.Sprintf("case-%d", i), func(t *testing.T) { + // Change the first app slug to match the current case. + for _, resource := range tfPlan.PlannedValues.RootModule.Resources { + if resource.Type == "coder_app" { + resource.AttributeValues["slug"] = c.slug + break + } + } + + _, err := terraform.ConvertState(ctx, []*tfjson.StateModule{tfPlan.PlannedValues.RootModule}, string(tfPlanGraph), logger) + if c.errContains != "" { + require.ErrorContains(t, err, c.errContains) + } else { + require.NoError(t, err) + } + }) + } +} + +func TestAppSlugDuplicate(t *testing.T) { + t.Parallel() + ctx, logger := ctxAndLogger(t) + + // nolint:dogsled + _, filename, _, _ := runtime.Caller(0) + + dir := filepath.Join(filepath.Dir(filename), "testdata", "multiple-apps") + tfPlanRaw, err := os.ReadFile(filepath.Join(dir, "multiple-apps.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-apps.tfplan.dot")) + require.NoError(t, err) - // Change all slugs to be identical and valid. for _, resource := range tfPlan.PlannedValues.RootModule.Resources { if resource.Type == "coder_app" { - resource.AttributeValues["slug"] = "valid" + resource.AttributeValues["slug"] = "dev" } } - state, err = terraform.ConvertState(ctx, []*tfjson.StateModule{tfPlan.PlannedValues.RootModule}, string(tfPlanGraph), logger) - require.Nil(t, state) + _, err = terraform.ConvertState(ctx, []*tfjson.StateModule{tfPlan.PlannedValues.RootModule}, string(tfPlanGraph), logger) require.Error(t, err) require.ErrorContains(t, err, "duplicate app slug") } +func TestAgentNameInvalid(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) + + cases := []struct { + name string + errContains string + }{ + {name: "bad--name", errContains: "does not match regex"}, + {name: "bad_name", errContains: "contains underscores"}, // custom error for underscores + {name: "valid-name-123", errContains: ""}, + {name: "valid", errContains: ""}, + {name: "UppercaseValid", errContains: ""}, + } + + //nolint:paralleltest + for i, c := range cases { + c := c + t.Run(fmt.Sprintf("case-%d", i), func(t *testing.T) { + // Change the first agent name to match the current case. + for _, resource := range tfPlan.PlannedValues.RootModule.Resources { + if resource.Type == "coder_agent" { + resource.Name = c.name + break + } + } + + _, err := terraform.ConvertState(ctx, []*tfjson.StateModule{tfPlan.PlannedValues.RootModule}, string(tfPlanGraph), logger) + if c.errContains != "" { + require.ErrorContains(t, err, c.errContains) + } else { + require.NoError(t, err) + } + }) + } +} + func TestAgentNameDuplicate(t *testing.T) { t.Parallel() ctx, logger := ctxAndLogger(t) diff --git a/site/site.go b/site/site.go index 3a85f7b3963ad..e2209b4052929 100644 --- a/site/site.go +++ b/site/site.go @@ -166,7 +166,7 @@ func New(opts *Options) *Handler { handler.installScript, err = parseInstallScript(opts.SiteFS, opts.BuildInfo) if err != nil { - _, _ = fmt.Fprintf(os.Stderr, "install.sh will be unavailable: %v", err.Error()) + opts.Logger.Warn(context.Background(), "could not parse install.sh, it will be unavailable", slog.Error(err)) } return handler From d2470165461646ca69fa3febee286c4313f51e02 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Thu, 20 Feb 2025 04:55:34 +0000 Subject: [PATCH 2/2] fixup! fix!: enforce regex for agent names --- provisioner/terraform/resources_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/provisioner/terraform/resources_test.go b/provisioner/terraform/resources_test.go index 0f6efb2e00fe4..46ad49d01d476 100644 --- a/provisioner/terraform/resources_test.go +++ b/provisioner/terraform/resources_test.go @@ -984,6 +984,7 @@ func TestInvalidTerraformAddress(t *testing.T) { require.Equal(t, state.Resources[0].ModulePath, "invalid terraform address") } +//nolint:tparallel func TestAppSlugValidation(t *testing.T) { t.Parallel() ctx, logger := ctxAndLogger(t) @@ -1061,6 +1062,7 @@ func TestAppSlugDuplicate(t *testing.T) { require.ErrorContains(t, err, "duplicate app slug") } +//nolint:tparallel func TestAgentNameInvalid(t *testing.T) { t.Parallel() ctx, logger := ctxAndLogger(t)