From 79bc366797f8abfe58e8a81e46734b08167f536a Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 13 Jan 2025 17:11:16 +0000 Subject: [PATCH 1/2] fix(coderd): swap order of precedence of coder_workspace_tags and tags from request --- coderd/templateversions.go | 6 +-- coderd/templateversions_test.go | 55 ++++++++++++++++++++++++++-- enterprise/coderd/workspaces_test.go | 26 ++++++++----- 3 files changed, 71 insertions(+), 16 deletions(-) diff --git a/coderd/templateversions.go b/coderd/templateversions.go index e9297d02e2a55..aedf091ee4711 100644 --- a/coderd/templateversions.go +++ b/coderd/templateversions.go @@ -1598,10 +1598,10 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht // Ensure the "owner" tag is properly applied in addition to request tags and coder_workspace_tags. // Tag order precedence: - // 1) User-specified tags in the request - // 2) Tags parsed from coder_workspace_tags data source in template file + // 1) Tags parsed from coder_workspace_tags data source in template file + // 2) User-specified tags in the request // 2 may clobber 1. - tags := provisionersdk.MutateTags(apiKey.UserID, req.ProvisionerTags, parsedTags) + tags := provisionersdk.MutateTags(apiKey.UserID, parsedTags, req.ProvisionerTags) var templateVersion database.TemplateVersion var provisionerJob database.ProvisionerJob diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index b2ec822f998bc..af26fddecff95 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -355,10 +355,11 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { wantTags: map[string]string{"owner": "", "scope": "organization", "foo": "bar", "a": "1", "b": "2"}, }, { - name: "main.tf with workspace tags and request tags", + name: "main.tf with request tags not clobbering workspace tags", files: map[string]string{ `main.tf`: ` - // This file is the same as the above, except for this comment. + // This file is, once again, the same as the above, except + // for a slightly different comment. variable "a" { type = string default = "1" @@ -381,9 +382,57 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { } }`, }, - reqTags: map[string]string{"baz": "zap", "foo": "noclobber"}, + reqTags: map[string]string{"baz": "zap"}, wantTags: map[string]string{"owner": "", "scope": "organization", "foo": "bar", "baz": "zap", "a": "1", "b": "2"}, }, + { + name: "main.tf with request tags clobbering workspace tags", + files: map[string]string{ + `main.tf`: ` + // This file is the same as the above, except for this comment. + variable "a" { + type = string + default = "1" + } + data "coder_parameter" "b" { + type = string + default = "2" + } + data "coder_parameter" "unrelated" { + name = "unrelated" + type = "list(string)" + default = jsonencode(["a", "b"]) + } + resource "null_resource" "test" {} + data "coder_workspace_tags" "tags" { + tags = { + "foo": "bar", + "a": var.a, + "b": data.coder_parameter.b.value, + } + }`, + }, + reqTags: map[string]string{"baz": "zap", "foo": "clobbered"}, + wantTags: map[string]string{"owner": "", "scope": "organization", "foo": "clobbered", "baz": "zap", "a": "1", "b": "2"}, + }, + // FIXME(cian): we should skip evaluating tags for which values have already been provided. + { + name: "main.tf with variable missing default value but value is passed in request", + files: map[string]string{ + `main.tf`: ` + variable "a" { + type = string + } + data "coder_workspace_tags" "tags" { + tags = { + "a": var.a, + } + }`, + }, + reqTags: map[string]string{"a": "b"}, + //wantTags: map[string]string{"owner": "", "scope": "organization", "a": "b"}, + expectError: `provisioner tag "a" evaluated to an empty value`, + }, { name: "main.tf with disallowed workspace tag value", files: map[string]string{ diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index 17685df83b387..f2e3ff503b950 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -1433,7 +1433,8 @@ func TestWorkspaceTagsTerraform(t *testing.T) { createTemplateVersionRequestTags map[string]string // the coder_workspace_tags bit of main.tf. // you can add more stuff here if you need - tfWorkspaceTags string + tfWorkspaceTags string + skipCreateWorkspace bool }{ { name: "no tags", @@ -1520,8 +1521,8 @@ func TestWorkspaceTagsTerraform(t *testing.T) { }`, }, { - name: "does not override static tag", - provisionerTags: map[string]string{"foo": "bar"}, + name: "overrides static tag from request", + provisionerTags: map[string]string{"foo": "baz"}, createTemplateVersionRequestTags: map[string]string{"foo": "baz"}, tfWorkspaceTags: ` data "coder_workspace_tags" "tags" { @@ -1529,6 +1530,9 @@ func TestWorkspaceTagsTerraform(t *testing.T) { "foo" = "bar" } }`, + // When we go to create the workspace, there won't be any provisioner + // matching tag foo=bar. + skipCreateWorkspace: true, }, } { tc := tc @@ -1570,13 +1574,15 @@ func TestWorkspaceTagsTerraform(t *testing.T) { coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, tv.ID) tpl := coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, tv.ID) - // Creating a workspace as a non-privileged user must succeed - ws, err := member.CreateUserWorkspace(ctx, memberUser.Username, codersdk.CreateWorkspaceRequest{ - TemplateID: tpl.ID, - Name: coderdtest.RandomUsername(t), - }) - require.NoError(t, err, "failed to create workspace") - coderdtest.AwaitWorkspaceBuildJobCompleted(t, member, ws.LatestBuild.ID) + if !tc.skipCreateWorkspace { + // Creating a workspace as a non-privileged user must succeed + ws, err := member.CreateUserWorkspace(ctx, memberUser.Username, codersdk.CreateWorkspaceRequest{ + TemplateID: tpl.ID, + Name: coderdtest.RandomUsername(t), + }) + require.NoError(t, err, "failed to create workspace") + coderdtest.AwaitWorkspaceBuildJobCompleted(t, member, ws.LatestBuild.ID) + } }) } } From 11c3db77829bb8ffc08b3c01732f48c734b2cf15 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 13 Jan 2025 17:35:21 +0000 Subject: [PATCH 2/2] fmt --- coderd/templateversions.go | 6 ++---- coderd/templateversions_test.go | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/coderd/templateversions.go b/coderd/templateversions.go index aedf091ee4711..d47a3f96cefc1 100644 --- a/coderd/templateversions.go +++ b/coderd/templateversions.go @@ -1597,10 +1597,8 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht } // Ensure the "owner" tag is properly applied in addition to request tags and coder_workspace_tags. - // Tag order precedence: - // 1) Tags parsed from coder_workspace_tags data source in template file - // 2) User-specified tags in the request - // 2 may clobber 1. + // User-specified tags in the request will take precedence over tags parsed from `coder_workspace_tags` + // data sources defined in the template file. tags := provisionersdk.MutateTags(apiKey.UserID, parsedTags, req.ProvisionerTags) var templateVersion database.TemplateVersion diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index af26fddecff95..c470f409c3748 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -430,7 +430,7 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { }`, }, reqTags: map[string]string{"a": "b"}, - //wantTags: map[string]string{"owner": "", "scope": "organization", "a": "b"}, + // wantTags: map[string]string{"owner": "", "scope": "organization", "a": "b"}, expectError: `provisioner tag "a" evaluated to an empty value`, }, {