Skip to content

fix(coderd): change the order of precedence between coder_workspace_tags and request tags #16119

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 14, 2025
Merged

fix(coderd): change the order of precedence between coder_workspace_tags and request tags #16119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
79bc366
fix(coderd): swap order of precedence of coder_workspace_tags and tag…
johnstcn Jan 13, 2025
11c3db7
fmt
johnstcn Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions coderd/templateversions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
55 changes: 52 additions & 3 deletions coderd/templateversions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Member

@mafredri mafredri Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is clobbering actually involved here? Since there's no two entries of the same tag the result is just a union.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a test below that shows the actual clobbering behaviour. I guess "not clobbering" and "union" are similar enough in result but different enough in semantics to be confusing. But the tags are not a union, there is an order of precedence here that I'm trying to codify and demonstrate.

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"
Expand All @@ -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{
Expand Down
26 changes: 16 additions & 10 deletions enterprise/coderd/workspaces_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -1520,15 +1521,18 @@ 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" {
tags = {
"foo" = "bar"
}
}`,
// When we go to create the workspace, there won't be any provisioner
// matching tag foo=bar.
skipCreateWorkspace: true,
},
} {
tc := tc
Expand Down Expand Up @@ -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)
}
})
}
}
Expand Down
Loading