From 9c2c83a1311fbb4c7a4a794f418efaecdb5bea6d Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 23 Jan 2025 04:53:18 +0000 Subject: [PATCH] fix(coderd): change the order of precedence between coder_workspace_tags and request tags (#16119) This PR switches the order of precedence of workspace tags when posting a template version. Previously, user-specified tags in the request could not override those detected from our parsing of the template file. Now, they can do. This addresses a customer issue where were attempting to set a workspace tag via variable. Note: there is a possible follow-up item here where we could pass in the workspace tag values from the request into `tfparse` and let it take those user-specified values into account. This is covered in a separate test. --- coderd/templateversions.go | 8 ++-- coderd/templateversions_test.go | 55 ++++++++++++++++++++++++++-- enterprise/coderd/workspaces_test.go | 26 ++++++++----- 3 files changed, 71 insertions(+), 18 deletions(-) diff --git a/coderd/templateversions.go b/coderd/templateversions.go index e9297d02e2a55..d47a3f96cefc1 100644 --- a/coderd/templateversions.go +++ b/coderd/templateversions.go @@ -1597,11 +1597,9 @@ 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 - // 2 may clobber 1. - tags := provisionersdk.MutateTags(apiKey.UserID, req.ProvisionerTags, parsedTags) + // 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 var provisionerJob database.ProvisionerJob diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index 7d386988fe16d..75c05c6af76b2 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 cce93dcc3a8fc..d074444bee10a 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -1217,7 +1217,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", @@ -1304,8 +1305,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" { @@ -1313,6 +1314,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 @@ -1352,13 +1356,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) + } }) } }