- Notifications
You must be signed in to change notification settings - Fork1k
fix(coderd/wsbuilder): correctly evaluate dynamic workspace tag values#15897
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
github-actionsbot commentedDec 17, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Awesome, thanks for adding this functionality!
| } | ||
| func (b*Builder)getTemplateVersionVariables() ([]database.TemplateVersionVariable,error) { | ||
| ifb.templateVersionVariables!=nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we use a mutex/atomic here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yea, we do this pattern everywhere. It's all single threaded
| // 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. | ||
| funcNewExternalProvisionerDaemon(t testing.TB,client*codersdk.Client,org uuid.UUID,tagsmap[string]string) io.Closer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Suggestion:NewExternalEchoProvisionerDaemon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I can rename in a follow-up! It would be a big diff :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yea, I think I wrote the original name. I don't think I even considered expected non-echo type in tests 😄
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM!
| // 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. | ||
| funcNewExternalProvisionerDaemon(t testing.TB,client*codersdk.Client,org uuid.UUID,tagsmap[string]string) io.Closer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So this runs real terraform? Does this mean tests can accidentally provision resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 wereally 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. | ||
| funcTestWorkspaceTagsTerraform(t*testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I could add a hacky check for the patternresource "[^"]+" "[^"]+" 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 aHTTP_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah I think we may need to think about the best way to prevent misuse of this.
dcf5153 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Relates to#15894:
coderdenttest.NewExternalProvisionerDaemonTerraformcoder_workspace_tagsspecified inmain.tfcoderd/wsbuilderto fetch template version variables and include them in eval context for evaluatingcoder_workspace_tags