chore: disable parameter validatation for dynamic params for all transitions #17926
Conversation
Validation drift prevents builds from being submitted, such as deleting
…_workspace_params
coderd/workspacebuilds.go
Outdated
| // Only defer to dynamic parameters if the experiment is enabled. | ||
| if api.Experiments.Enabled(codersdk.ExperimentDynamicParameters) && | ||
| createBuild.EnableDynamicParameters != nil { | ||
| // Explicit opt-in | ||
| builder = builder.DynamicParameters(*createBuild.EnableDynamicParameters) | ||
| } |
There was a problem hiding this comment.
I provide the explicit opt in as a way to force through transitions such as DELETE if there an issue. It would be unfortunate to have to change a template wide setting to fix a single workspace if we get into a strange state.
There was a problem hiding this comment.
Can we drop a WARN or something if the build requests dynamic parameters but the experiment is not enabled?
coderd/workspacebuilds.go
Outdated
| // Only defer to dynamic parameters if the experiment is enabled. | ||
| if api.Experiments.Enabled(codersdk.ExperimentDynamicParameters) && | ||
| createBuild.EnableDynamicParameters != nil { | ||
| // Explicit opt-in | ||
| builder = builder.DynamicParameters(*createBuild.EnableDynamicParameters) | ||
| } |
There was a problem hiding this comment.
Can we drop a WARN or something if the build requests dynamic parameters but the experiment is not enabled?
| func ProvisionerVersionSupportsDynamicParameters(version string) bool { | ||
| major, minor, err := apiversion.Parse(version) | ||
| // If the api version is not valid or less than 1.6, we need to use the static parameters | ||
| useStaticParams := err != nil || major < 1 || (major == 1 && minor < 6) | ||
| return !useStaticParams | ||
| } |
There was a problem hiding this comment.
nit: this feels like something that might want to be in provisionersdk
There was a problem hiding this comment.
Hmm. Really tough to know where to place it. I'm going to keep it with wsbuilder right now, since it only affects workspace builds.
I think I'll create a new package for dynamic params more generally as more code is added to support the feature, and I can move it there.
What this does
Dynamic params currently has relaxed parameter validation in coder/coder. This is because conditional parameters cannot be validated with the static parameters in the database.
We might want to tighten this up later based on the
coderprovider version in the terraform. As version >=2.4.0 all have the strict validation built into the provider. And the UI form relies on the preview engine for validation.Unit test
A unit test of validation drift is present. Without this dynamic params option in wsbuilder, workspace transactions fail to validate, and will never succeed.
This puts the workspace in a locked position where you cannot change its state.
The error is:
This would also break updates, auto-starts, auto-stop, deletes, etc.
Future work
Before we exit experimental, we should revisit if parameter validation is critical in coder/coder (for provider versions >2.4.0).
Validation in coder/coder is for raising errors early, rather than waiting for the terraform logs. For any action without the ability to change params, this does not really provide much help, as there is no way to resolve the issue.
We can always hook up dynamic params to more endpoints, the cost is just a bit of memory loading in all the files.
#17942
Note
This currently only applies to deployments that enable the experiment. Historical versions enforce the previous validation always.
Templates can additionally be opt-in or opt-out of dynamic params. Meaning a deployment with the experiment could have some templates testing the new params, and some on the old.