Skip to content

Commit e76d58f

Browse files
EmyrkEmyrk
authored
chore: disable parameter validatation for dynamic params for all transitions (coder#17926)
Dynamic params skip parameter validation in coder/coder. This is because conditional parameters cannot be validated with the static parameters in the database.
1 parent 93f17bc commit e76d58f

File tree

15 files changed

+258
-17
lines changed

15 files changed

+258
-17
lines changed

‎cli/server.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1124,7 +1124,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
11241124
autobuildTicker := time.NewTicker(vals.AutobuildPollInterval.Value())
11251125
defer autobuildTicker.Stop()
11261126
autobuildExecutor := autobuild.NewExecutor(
1127-
ctx, options.Database, options.Pubsub, options.PrometheusRegistry, coderAPI.TemplateScheduleStore, &coderAPI.Auditor, coderAPI.AccessControlStore, logger, autobuildTicker.C, options.NotificationsEnqueuer)
1127+
ctx, options.Database, options.Pubsub, options.PrometheusRegistry, coderAPI.TemplateScheduleStore, &coderAPI.Auditor, coderAPI.AccessControlStore, logger, autobuildTicker.C, options.NotificationsEnqueuer, coderAPI.Experiments)
11281128
autobuildExecutor.Run()
11291129

11301130
jobReaperTicker := time.NewTicker(vals.JobReaperDetectorInterval.Value())

‎coderd/apidoc/docs.go‎

Lines changed: 4 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apidoc/swagger.json‎

Lines changed: 4 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/autobuild/lifecycle_executor.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
"github.com/coder/coder/v2/coderd/notifications"
2828
"github.com/coder/coder/v2/coderd/schedule"
2929
"github.com/coder/coder/v2/coderd/wsbuilder"
30+
"github.com/coder/coder/v2/codersdk"
3031
)
3132

3233
// Executor automatically starts or stops workspaces.
@@ -43,6 +44,7 @@ type Executor struct {
4344
// NotificationsEnqueuer handles enqueueing notifications for delivery by SMTP, webhook, etc.
4445
notificationsEnqueuer notifications.Enqueuer
4546
reg prometheus.Registerer
47+
experiments codersdk.Experiments
4648

4749
metrics executorMetrics
4850
}
@@ -59,7 +61,7 @@ type Stats struct {
5961
}
6062

6163
// New returns a new wsactions executor.
62-
func NewExecutor(ctx context.Context, db database.Store, ps pubsub.Pubsub, reg prometheus.Registerer, tss *atomic.Pointer[schedule.TemplateScheduleStore], auditor *atomic.Pointer[audit.Auditor], acs *atomic.Pointer[dbauthz.AccessControlStore], log slog.Logger, tick <-chan time.Time, enqueuer notifications.Enqueuer) *Executor {
64+
func NewExecutor(ctx context.Context, db database.Store, ps pubsub.Pubsub, reg prometheus.Registerer, tss *atomic.Pointer[schedule.TemplateScheduleStore], auditor *atomic.Pointer[audit.Auditor], acs *atomic.Pointer[dbauthz.AccessControlStore], log slog.Logger, tick <-chan time.Time, enqueuer notifications.Enqueuer, exp codersdk.Experiments) *Executor {
6365
factory := promauto.With(reg)
6466
le := &Executor{
6567
//nolint:gocritic // Autostart has a limited set of permissions.
@@ -73,6 +75,7 @@ func NewExecutor(ctx context.Context, db database.Store, ps pubsub.Pubsub, reg p
7375
accessControlStore: acs,
7476
notificationsEnqueuer: enqueuer,
7577
reg: reg,
78+
experiments: exp,
7679
metrics: executorMetrics{
7780
autobuildExecutionDuration: factory.NewHistogram(prometheus.HistogramOpts{
7881
Namespace: "coderd",
@@ -258,6 +261,7 @@ func (e *Executor) runOnce(t time.Time) Stats {
258261
builder := wsbuilder.New(ws, nextTransition).
259262
SetLastWorkspaceBuildInTx(&latestBuild).
260263
SetLastWorkspaceBuildJobInTx(&latestJob).
264+
Experiments(e.experiments).
261265
Reason(reason)
262266
log.Debug(e.ctx, "auto building workspace", slog.F("transition", nextTransition))
263267
if nextTransition == database.WorkspaceTransitionStart &&

‎coderd/coderdtest/coderdtest.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can
354354
auditor.Store(&options.Auditor)
355355

356356
ctx, cancelFunc := context.WithCancel(context.Background())
357+
experiments := coderd.ReadExperiments(*options.Logger, options.DeploymentValues.Experiments)
357358
lifecycleExecutor := autobuild.NewExecutor(
358359
ctx,
359360
options.Database,
@@ -365,6 +366,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can
365366
*options.Logger,
366367
options.AutobuildTicker,
367368
options.NotificationsEnqueuer,
369+
experiments,
368370
).WithStatsChannel(options.AutobuildStats)
369371
lifecycleExecutor.Run()
370372

‎coderd/parameters.go‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,13 @@ import (
1212
"golang.org/x/sync/errgroup"
1313
"golang.org/x/xerrors"
1414

15-
"github.com/coder/coder/v2/apiversion"
1615
"github.com/coder/coder/v2/coderd/database"
1716
"github.com/coder/coder/v2/coderd/database/dbauthz"
1817
"github.com/coder/coder/v2/coderd/files"
1918
"github.com/coder/coder/v2/coderd/httpapi"
2019
"github.com/coder/coder/v2/coderd/httpmw"
2120
"github.com/coder/coder/v2/coderd/util/ptr"
21+
"github.com/coder/coder/v2/coderd/wsbuilder"
2222
"github.com/coder/coder/v2/codersdk"
2323
"github.com/coder/coder/v2/codersdk/wsjson"
2424
sdkproto "github.com/coder/coder/v2/provisionersdk/proto"
@@ -69,13 +69,10 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http
6969
return
7070
}
7171

72-
major, minor, err := apiversion.Parse(tf.ProvisionerdVersion)
73-
// If the api version is not valid or less than 1.5, we need to use the static parameters
74-
useStaticParams := err != nil || major < 1 || (major == 1 && minor < 6)
75-
if useStaticParams {
76-
api.handleStaticParameters(rw, r, templateVersion.ID)
77-
} else {
72+
if wsbuilder.ProvisionerVersionSupportsDynamicParameters(tf.ProvisionerdVersion) {
7873
api.handleDynamicParameters(rw, r, tf, templateVersion)
74+
} else {
75+
api.handleStaticParameters(rw, r, templateVersion.ID)
7976
}
8077
}
8178

‎coderd/workspacebuilds.go‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -338,6 +338,7 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) {
338338
RichParameterValues(createBuild.RichParameterValues).
339339
LogLevel(string(createBuild.LogLevel)).
340340
DeploymentValues(api.Options.DeploymentValues).
341+
Experiments(api.Experiments).
341342
TemplateVersionPresetID(createBuild.TemplateVersionPresetID)
342343

343344
var (
@@ -383,6 +384,22 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) {
383384
builder = builder.State(createBuild.ProvisionerState)
384385
}
385386

387+
// Only defer to dynamic parameters if the experiment is enabled.
388+
if api.Experiments.Enabled(codersdk.ExperimentDynamicParameters) {
389+
if createBuild.EnableDynamicParameters != nil {
390+
// Explicit opt-in
391+
builder = builder.DynamicParameters(*createBuild.EnableDynamicParameters)
392+
}
393+
} else {
394+
if createBuild.EnableDynamicParameters != nil {
395+
api.Logger.Warn(ctx, "ignoring dynamic parameter field sent by request, the experiment is not enabled",
396+
slog.F("field", *createBuild.EnableDynamicParameters),
397+
slog.F("user", apiKey.UserID.String()),
398+
slog.F("transition", string(createBuild.Transition)),
399+
)
400+
}
401+
}
402+
386403
workspaceBuild, provisionerJob, provisionerDaemons, err = builder.Build(
387404
ctx,
388405
tx,

‎coderd/workspaces.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -704,6 +704,8 @@ func createWorkspace(
704704
Reason(database.BuildReasonInitiator).
705705
Initiator(initiatorID).
706706
ActiveVersion().
707+
Experiments(api.Experiments).
708+
DeploymentValues(api.DeploymentValues).
707709
RichParameterValues(req.RichParameterValues)
708710
if req.TemplateVersionID != uuid.Nil {
709711
builder = builder.VersionID(req.TemplateVersionID)
@@ -716,7 +718,7 @@ func createWorkspace(
716718
}
717719

718720
if req.EnableDynamicParameters && api.Experiments.Enabled(codersdk.ExperimentDynamicParameters) {
719-
builder = builder.UsingDynamicParameters()
721+
builder = builder.DynamicParameters(req.EnableDynamicParameters)
720722
}
721723

722724
workspaceBuild, provisionerJob, provisionerDaemons, err = builder.Build(

‎coderd/wsbuilder/wsbuilder.go‎

Lines changed: 71 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@ import (
1313
"github.com/hashicorp/hcl/v2"
1414
"github.com/hashicorp/hcl/v2/hclsyntax"
1515

16+
"github.com/coder/coder/v2/apiversion"
1617
"github.com/coder/coder/v2/coderd/rbac/policy"
18+
"github.com/coder/coder/v2/coderd/util/ptr"
1719
"github.com/coder/coder/v2/provisioner/terraform/tfparse"
1820
"github.com/coder/coder/v2/provisionersdk"
1921
sdkproto "github.com/coder/coder/v2/provisionersdk/proto"
@@ -51,9 +53,11 @@ type Builder struct {
5153
state stateTarget
5254
logLevel string
5355
deploymentValues *codersdk.DeploymentValues
56+
experiments codersdk.Experiments
5457

55-
richParameterValues []codersdk.WorkspaceBuildParameter
56-
dynamicParametersEnabled bool
58+
richParameterValues []codersdk.WorkspaceBuildParameter
59+
// dynamicParametersEnabled is non-nil if set externally
60+
dynamicParametersEnabled *bool
5761
initiator uuid.UUID
5862
reason database.BuildReason
5963
templateVersionPresetID uuid.UUID
@@ -66,6 +70,7 @@ type Builder struct {
6670
template *database.Template
6771
templateVersion *database.TemplateVersion
6872
templateVersionJob *database.ProvisionerJob
73+
terraformValues *database.TemplateVersionTerraformValue
6974
templateVersionParameters *[]database.TemplateVersionParameter
7075
templateVersionVariables *[]database.TemplateVersionVariable
7176
templateVersionWorkspaceTags *[]database.TemplateVersionWorkspaceTag
@@ -155,6 +160,14 @@ func (b Builder) DeploymentValues(dv *codersdk.DeploymentValues) Builder {
155160
return b
156161
}
157162

163+
func (b Builder) Experiments(exp codersdk.Experiments) Builder {
164+
// nolint: revive
165+
cpy := make(codersdk.Experiments, len(exp))
166+
copy(cpy, exp)
167+
b.experiments = cpy
168+
return b
169+
}
170+
158171
func (b Builder) Initiator(u uuid.UUID) Builder {
159172
// nolint: revive
160173
b.initiator = u
@@ -187,8 +200,9 @@ func (b Builder) MarkPrebuiltWorkspaceClaim() Builder {
187200
return b
188201
}
189202

190-
func (b Builder) UsingDynamicParameters() Builder {
191-
b.dynamicParametersEnabled = true
203+
func (b Builder) DynamicParameters(using bool) Builder {
204+
// nolint: revive
205+
b.dynamicParametersEnabled = ptr.Ref(using)
192206
return b
193207
}
194208

@@ -516,6 +530,22 @@ func (b *Builder) getTemplateVersionID() (uuid.UUID, error) {
516530
return bld.TemplateVersionID, nil
517531
}
518532

533+
func (b *Builder) getTemplateTerraformValues() (*database.TemplateVersionTerraformValue, error) {
534+
if b.terraformValues != nil {
535+
return b.terraformValues, nil
536+
}
537+
v, err := b.getTemplateVersion()
538+
if err != nil {
539+
return nil, xerrors.Errorf("get template version so we can get terraform values: %w", err)
540+
}
541+
vals, err := b.store.GetTemplateVersionTerraformValues(b.ctx, v.ID)
542+
if err != nil {
543+
return nil, xerrors.Errorf("get template version terraform values %s: %w", v.JobID, err)
544+
}
545+
b.terraformValues = &vals
546+
return b.terraformValues, err
547+
}
548+
519549
func (b *Builder) getLastBuild() (*database.WorkspaceBuild, error) {
520550
if b.lastBuild != nil {
521551
return b.lastBuild, nil
@@ -593,9 +623,10 @@ func (b *Builder) getParameters() (names, values []string, err error) {
593623
return nil, nil, BuildError{http.StatusBadRequest, "Unable to build workspace with unsupported parameters", err}
594624
}
595625

596-
if b.dynamicParametersEnabled {
597-
// Dynamic parameters skip all parameter validation.
598-
// Pass the user's input as is.
626+
// Dynamic parameters skip all parameter validation.
627+
// Deleting a workspace also should skip parameter validation.
628+
// Pass the user's input as is.
629+
if b.usingDynamicParameters() {
599630
// TODO: The previous behavior was only to pass param values
600631
// for parameters that exist. Since dynamic params can have
601632
// conditional parameter existence, the static frame of reference
@@ -989,3 +1020,36 @@ func (b *Builder) checkRunningBuild() error {
9891020
}
9901021
return nil
9911022
}
1023+
1024+
func (b *Builder) usingDynamicParameters() bool {
1025+
if !b.experiments.Enabled(codersdk.ExperimentDynamicParameters) {
1026+
// Experiment required
1027+
return false
1028+
}
1029+
1030+
vals, err := b.getTemplateTerraformValues()
1031+
if err != nil {
1032+
return false
1033+
}
1034+
1035+
if !ProvisionerVersionSupportsDynamicParameters(vals.ProvisionerdVersion) {
1036+
return false
1037+
}
1038+
1039+
if b.dynamicParametersEnabled != nil {
1040+
return *b.dynamicParametersEnabled
1041+
}
1042+
1043+
tpl, err := b.getTemplate()
1044+
if err != nil {
1045+
return false // Let another part of the code get this error
1046+
}
1047+
return !tpl.UseClassicParameterFlow
1048+
}
1049+
1050+
func ProvisionerVersionSupportsDynamicParameters(version string) bool {
1051+
major, minor, err := apiversion.Parse(version)
1052+
// If the api version is not valid or less than 1.6, we need to use the static parameters
1053+
useStaticParams := err != nil || major < 1 || (major == 1 && minor < 6)
1054+
return !useStaticParams
1055+
}

‎coderd/wsbuilder/wsbuilder_test.go‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -839,6 +839,32 @@ func TestWorkspaceBuildWithPreset(t *testing.T) {
839839
req.NoError(err)
840840
}
841841

842+
func TestProvisionerVersionSupportsDynamicParameters(t *testing.T) {
843+
t.Parallel()
844+
845+
for v, dyn := range map[string]bool{
846+
"": false,
847+
"na": false,
848+
"0.0": false,
849+
"0.10": false,
850+
"1.4": false,
851+
"1.5": false,
852+
"1.6": true,
853+
"1.7": true,
854+
"1.8": true,
855+
"2.0": true,
856+
"2.17": true,
857+
"4.0": true,
858+
} {
859+
t.Run(v, func(t *testing.T) {
860+
t.Parallel()
861+
862+
does := wsbuilder.ProvisionerVersionSupportsDynamicParameters(v)
863+
require.Equal(t, dyn, does)
864+
})
865+
}
866+
}
867+
842868
type txExpect func(mTx *dbmock.MockStore)
843869

844870
func expectDB(t *testing.T, opts ...txExpect) *dbmock.MockStore {

0 commit comments

Comments
 (0)