fix(coderd/wsbuilder): correctly evaluate dynamic workspace tag values #15897
Conversation
| @@ -617,6 +618,22 @@ func (b *Builder) getTemplateVersionParameters() ([]database.TemplateVersionPara | |||
| return tvp, nil | |||
| } | |||
|
|
|||
| func (b *Builder) getTemplateVersionVariables() ([]database.TemplateVersionVariable, error) { | |||
| if b.templateVersionVariables != nil { | |||
There was a problem hiding this comment.
Should we use a mutex/atomic here?
There was a problem hiding this comment.
I don't think the builder is meant to be safe for concurrent use.
Maybe good for a follow-up though, as it could trip someone else up in future.
There was a problem hiding this comment.
Yea, we do this pattern everywhere. It's all single threaded
| // goroutine and returns a closer to stop it. The echo provisioner is used | ||
| // here. This is the default provisioner for tests and should be fine for | ||
| // most use cases. If you need to test terraform-specific behaviors, use | ||
| // NewExternalProvisionerDaemonTerraform instead. | ||
| func NewExternalProvisionerDaemon(t testing.TB, client *codersdk.Client, org uuid.UUID, tags map[string]string) io.Closer { |
There was a problem hiding this comment.
Suggestion: NewExternalEchoProvisionerDaemon?
There was a problem hiding this comment.
I can rename in a follow-up! It would be a big diff :D
There was a problem hiding this comment.
Yea, I think I wrote the original name. I don't think I even considered expected non-echo type in tests 😄
| // goroutine and returns a closer to stop it. The echo provisioner is used | ||
| // here. This is the default provisioner for tests and should be fine for | ||
| // most use cases. If you need to test terraform-specific behaviors, use | ||
| // NewExternalProvisionerDaemonTerraform instead. | ||
| func NewExternalProvisionerDaemon(t testing.TB, client *codersdk.Client, org uuid.UUID, tags map[string]string) io.Closer { |
There was a problem hiding this comment.
Yea, I think I wrote the original name. I don't think I even considered expected non-echo type in tests 😄
| assert.NoError(t, terraform.Serve(ctx, &terraform.ServeOptions{ | ||
| BinaryPath: terraformPath, | ||
| CachePath: t.TempDir(), | ||
| ServeOptions: &provisionersdk.ServeOptions{ | ||
| Listener: provisionerSrv, | ||
| WorkDirectory: t.TempDir(), | ||
| }, |
There was a problem hiding this comment.
So this runs real terraform? Does this mean tests can accidentally provision resources?
There was a problem hiding this comment.
Yeah, that's the tricky thing if you're not careful. There's no other real way to test this apart from spinning up a Docker container or manual testing, and we really need some kind of integration test here.
| // TestWorkspaceTagsTerraform tests that a workspace can be created with tags. | ||
| // This is an end-to-end-style test, meaning that we actually run the | ||
| // real Terraform provisioner and validate that the workspace is created | ||
| // successfully. The workspace itself does not specify any resources, and | ||
| // this is fine. | ||
| func TestWorkspaceTagsTerraform(t *testing.T) { |
There was a problem hiding this comment.
Woah, he did it 🤯
This is a super nice test. Slightly concerned with terraform being executed in tests, would be nice if we could lock it down somehow to prevent it allocating any resources.
There was a problem hiding this comment.
I could add a hacky check for the pattern resource "[^"]+" "[^"]+" and fail. That would at least make folks think twice before hacking it further to allow certain resources.
EDIT: Actually I'm not even sure I could do that right now due to how provisionerd works. Needs some more thinking. For now... ruleguard? 🤷
There was a problem hiding this comment.
I don't think ruleguard would be sufficient. I wonder if we can do something to the executable. Run it another userspace with no perms? Configure a HTTP_PROXY that is broken so any external requests fail? I'm not sure 🤷♂️
I'm ok if we let this slide for now, since I don't know a good way. Just an unfortunate consequence.
There was a problem hiding this comment.
Yeah I think we may need to think about the best way to prevent misuse of this.
Relates to #15894:
coderdenttest.NewExternalProvisionerDaemonTerraformcoder_workspace_tagsspecified inmain.tfcoderd/wsbuilderto fetch template version variables and include them in eval context for evaluatingcoder_workspace_tags