From 1ca9483a02fb8e1e29d915379087c13a8c55afc6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 17 Jun 2025 16:24:21 -0500 Subject: [PATCH 01/22] wip --- coderd/dynamicparameters/render.go | 115 +++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) create mode 100644 coderd/dynamicparameters/render.go diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go new file mode 100644 index 0000000000000..4bdc754668666 --- /dev/null +++ b/coderd/dynamicparameters/render.go @@ -0,0 +1,115 @@ +package dynamicparameters + +import ( + "context" + + "github.com/google/uuid" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/apiversion" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/files" + "github.com/coder/preview" + + "github.com/hashicorp/hcl/v2" +) + +type Loader struct { + templateVersionID uuid.UUID + + // cache of objects + templateVersion *database.TemplateVersion + job *database.ProvisionerJob + terraformValues *database.TemplateVersionTerraformValue +} + +func New(ctx context.Context, versionID uuid.UUID) *Loader { + return &Loader{ + templateVersionID: versionID, + } +} + +func (r *Loader) WithTemplateVersion(tv database.TemplateVersion) *Loader { + if tv.ID == r.templateVersionID { + r.templateVersion = &tv + } + + return r +} + +func (r *Loader) WithProvisionerJob(job database.ProvisionerJob) *Loader { + r.job = &job + + return r +} + +func (r *Loader) WithTerraformValues(values database.TemplateVersionTerraformValue) *Loader { + if values.TemplateVersionID == r.templateVersionID { + r.terraformValues = &values + } + + return r +} + +func (r *Loader) Load(ctx context.Context, db database.Store) error { + if r.templateVersion == nil { + tv, err := db.GetTemplateVersionByID(ctx, r.templateVersionID) + if err != nil { + return xerrors.Errorf("template version: %w", err) + } + r.templateVersion = &tv + } + + if r.job == nil { + job, err := db.GetProvisionerJobByID(ctx, r.templateVersion.JobID) + if err != nil { + return xerrors.Errorf("provisioner job: %w", err) + } + r.job = &job + } + + if !r.job.CompletedAt.Valid { + return xerrors.Errorf("job has not completed") + } + + if r.terraformValues == nil { + values, err := db.GetTemplateVersionTerraformValues(ctx, r.templateVersion.ID) + if err != nil { + return xerrors.Errorf("template version terraform values: %w", err) + } + r.terraformValues = &values + } + + return nil +} + +func (r *Loader) loaded() bool { + return r.templateVersion != nil && r.job != nil && r.terraformValues != nil +} + +func (r *Loader) Renderer(ctx context.Context, cache *files.Cache) (any, error) { + if !r.loaded() { + return nil, xerrors.New("Load() must be called before Renderer()") + } + + // If they can read the template version, then they can read the file. + fileCtx := dbauthz.AsFileReader(ctx) + templateFS, err := cache.Acquire(fileCtx, r.job.FileID) + if err != nil { + return nil, xerrors.Errorf("acquire template file: %w", err) + } + +} + +func (r *Loader) Render(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { + + return nil, nil +} + +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 +} From d4dab77f7d80e3e94f56add3c99a7e29bf92ec8b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 17 Jun 2025 17:15:35 -0500 Subject: [PATCH 02/22] refactor and move dynamic param rendering --- coderd/dynamicparameters/render.go | 201 ++++++++++++++- coderd/dynamicparameters/static.go | 134 ++++++++++ coderd/parameters.go | 396 +++-------------------------- 3 files changed, 360 insertions(+), 371 deletions(-) create mode 100644 coderd/dynamicparameters/static.go diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index 4bdc754668666..ac8141e4335cd 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -2,8 +2,12 @@ package dynamicparameters import ( "context" + "encoding/json" + "io/fs" + "sync" "github.com/google/uuid" + "golang.org/x/sync/errgroup" "golang.org/x/xerrors" "github.com/coder/coder/v2/apiversion" @@ -11,10 +15,24 @@ import ( "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/files" "github.com/coder/preview" + previewtypes "github.com/coder/preview/types" "github.com/hashicorp/hcl/v2" ) +type Renderer interface { + Render(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) + Close() +} + +var ( + ErrorTemplateVersionNotReady error = xerrors.New("template version job not finished") +) + +// Loader is used to load the necessary coder objects for rendering a template +// version's parameters. The output is a Renderer, which is the object that uses +// the cached objects to render the template version's parameters. Closing the +// Renderer will release the cached files. type Loader struct { templateVersionID uuid.UUID @@ -24,7 +42,7 @@ type Loader struct { terraformValues *database.TemplateVersionTerraformValue } -func New(ctx context.Context, versionID uuid.UUID) *Loader { +func New(versionID uuid.UUID) *Loader { return &Loader{ templateVersionID: versionID, } @@ -70,7 +88,7 @@ func (r *Loader) Load(ctx context.Context, db database.Store) error { } if !r.job.CompletedAt.Valid { - return xerrors.Errorf("job has not completed") + return ErrorTemplateVersionNotReady } if r.terraformValues == nil { @@ -88,11 +106,21 @@ func (r *Loader) loaded() bool { return r.templateVersion != nil && r.job != nil && r.terraformValues != nil } -func (r *Loader) Renderer(ctx context.Context, cache *files.Cache) (any, error) { +func (r *Loader) Renderer(ctx context.Context, db database.Store, cache *files.Cache) (Renderer, error) { if !r.loaded() { return nil, xerrors.New("Load() must be called before Renderer()") } + if !ProvisionerVersionSupportsDynamicParameters(r.terraformValues.ProvisionerdVersion) { + return r.staticRender(ctx, db) + } + + return r.dynamicRenderer(ctx, db, cache) +} + +// Renderer caches all the necessary files when rendering a template version's +// parameters. It must be closed after use to release the cached files. +func (r *Loader) dynamicRenderer(ctx context.Context, db database.Store, cache *files.Cache) (*DynamicRenderer, error) { // If they can read the template version, then they can read the file. fileCtx := dbauthz.AsFileReader(ctx) templateFS, err := cache.Acquire(fileCtx, r.job.FileID) @@ -100,11 +128,174 @@ func (r *Loader) Renderer(ctx context.Context, cache *files.Cache) (any, error) return nil, xerrors.Errorf("acquire template file: %w", err) } + var moduleFilesFS fs.FS + if r.terraformValues.CachedModuleFiles.Valid { + moduleFilesFS, err = cache.Acquire(fileCtx, r.terraformValues.CachedModuleFiles.UUID) + if err != nil { + cache.Release(r.job.FileID) + return nil, xerrors.Errorf("acquire module files: %w", err) + } + templateFS = files.NewOverlayFS(templateFS, []files.Overlay{{Path: ".terraform/modules", FS: moduleFilesFS}}) + } + + plan := json.RawMessage("{}") + if len(r.terraformValues.CachedPlan) > 0 { + plan = r.terraformValues.CachedPlan + } + + return &DynamicRenderer{ + data: r, + templateFS: templateFS, + db: db, + plan: plan, + close: func() { + cache.Release(r.job.FileID) + if moduleFilesFS != nil { + cache.Release(r.terraformValues.CachedModuleFiles.UUID) + } + }, + }, nil } -func (r *Loader) Render(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { +type DynamicRenderer struct { + db database.Store + data *Loader + templateFS fs.FS + plan json.RawMessage + + failedOwners map[uuid.UUID]error + currentOwner *previewtypes.WorkspaceOwner + + once sync.Once + close func() +} + +func (r *DynamicRenderer) Render(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { + err := r.getWorkspaceOwnerData(ctx, ownerID) + if err != nil || r.currentOwner == nil { + return nil, hcl.Diagnostics{ + { + Severity: hcl.DiagError, + Summary: "Failed to fetch workspace owner", + Detail: "Please check your permissions or the user may not exist.", + Extra: previewtypes.DiagnosticExtra{ + Code: "owner_not_found", + }, + }, + } + } + + input := preview.Input{ + PlanJSON: r.data.terraformValues.CachedPlan, + ParameterValues: map[string]string{}, + Owner: *r.currentOwner, + } + + return preview.Preview(ctx, input, r.templateFS) +} + +func (r *DynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uuid.UUID) error { + if r.currentOwner != nil && r.currentOwner.ID == ownerID.String() { + return nil // already fetched + } + + if r.failedOwners[ownerID] != nil { + // previously failed, do not try again + return r.failedOwners[ownerID] + } + + var g errgroup.Group + + // TODO: @emyrk we should only need read access on the org member, not the + // site wide user object. Figure out a better way to handle this. + user, err := r.db.GetUserByID(ctx, ownerID) + if err != nil { + return xerrors.Errorf("fetch user: %w", err) + } + + var ownerRoles []previewtypes.WorkspaceOwnerRBACRole + g.Go(func() error { + // nolint:gocritic // This is kind of the wrong query to use here, but it + // matches how the provisioner currently works. We should figure out + // something that needs less escalation but has the correct behavior. + row, err := r.db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), ownerID) + if err != nil { + return err + } + roles, err := row.RoleNames() + if err != nil { + return err + } + ownerRoles = make([]previewtypes.WorkspaceOwnerRBACRole, 0, len(roles)) + for _, it := range roles { + if it.OrganizationID != uuid.Nil && it.OrganizationID != r.data.templateVersion.OrganizationID { + continue + } + var orgID string + if it.OrganizationID != uuid.Nil { + orgID = it.OrganizationID.String() + } + ownerRoles = append(ownerRoles, previewtypes.WorkspaceOwnerRBACRole{ + Name: it.Name, + OrgID: orgID, + }) + } + return nil + }) + + var publicKey string + g.Go(func() error { + // The correct public key has to be sent. This will not be leaked + // unless the template leaks it. + // nolint:gocritic + key, err := r.db.GetGitSSHKey(dbauthz.AsSystemRestricted(ctx), ownerID) + if err != nil { + return err + } + publicKey = key.PublicKey + return nil + }) + + var groupNames []string + g.Go(func() error { + // The groups need to be sent to preview. These groups are not exposed to the + // user, unless the template does it through the parameters. Regardless, we need + // the correct groups, and a user might not have read access. + // nolint:gocritic + groups, err := r.db.GetGroups(dbauthz.AsSystemRestricted(ctx), database.GetGroupsParams{ + OrganizationID: r.data.templateVersion.OrganizationID, + HasMemberID: ownerID, + }) + if err != nil { + return err + } + groupNames = make([]string, 0, len(groups)) + for _, it := range groups { + groupNames = append(groupNames, it.Group.Name) + } + return nil + }) + + err = g.Wait() + if err != nil { + return err + } + + r.currentOwner = &previewtypes.WorkspaceOwner{ + ID: user.ID.String(), + Name: user.Username, + FullName: user.Name, + Email: user.Email, + LoginType: string(user.LoginType), + RBACRoles: ownerRoles, + SSHPublicKey: publicKey, + Groups: groupNames, + } + return nil +} - return nil, nil +func (r *DynamicRenderer) Close() { + r.once.Do(r.close) } func ProvisionerVersionSupportsDynamicParameters(version string) bool { diff --git a/coderd/dynamicparameters/static.go b/coderd/dynamicparameters/static.go new file mode 100644 index 0000000000000..c945f510718ea --- /dev/null +++ b/coderd/dynamicparameters/static.go @@ -0,0 +1,134 @@ +package dynamicparameters + +import ( + "context" + "encoding/json" + + "github.com/google/uuid" + "github.com/hashicorp/hcl/v2" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/util/ptr" + sdkproto "github.com/coder/coder/v2/provisionersdk/proto" + "github.com/coder/preview" + previewtypes "github.com/coder/preview/types" + "github.com/coder/terraform-provider-coder/v2/provider" +) + +type StaticRender struct { + staticParams []previewtypes.Parameter +} + +func (r *Loader) staticRender(ctx context.Context, db database.Store) (*StaticRender, error) { + dbTemplateVersionParameters, err := db.GetTemplateVersionParameters(ctx, r.templateVersionID) + if err != nil { + return nil, xerrors.Errorf("template version parameters: %w", err) + } + + params := make([]previewtypes.Parameter, 0, len(dbTemplateVersionParameters)) + for _, it := range dbTemplateVersionParameters { + param := previewtypes.Parameter{ + ParameterData: previewtypes.ParameterData{ + Name: it.Name, + DisplayName: it.DisplayName, + Description: it.Description, + Type: previewtypes.ParameterType(it.Type), + FormType: provider.ParameterFormType(it.FormType), + Styling: previewtypes.ParameterStyling{}, + Mutable: it.Mutable, + DefaultValue: previewtypes.StringLiteral(it.DefaultValue), + Icon: it.Icon, + Options: make([]*previewtypes.ParameterOption, 0), + Validations: make([]*previewtypes.ParameterValidation, 0), + Required: it.Required, + Order: int64(it.DisplayOrder), + Ephemeral: it.Ephemeral, + Source: nil, + }, + // Always use the default, since we used to assume the empty string + Value: previewtypes.StringLiteral(it.DefaultValue), + Diagnostics: nil, + } + + if it.ValidationError != "" || it.ValidationRegex != "" || it.ValidationMonotonic != "" { + var reg *string + if it.ValidationRegex != "" { + reg = ptr.Ref(it.ValidationRegex) + } + + var vMin *int64 + if it.ValidationMin.Valid { + vMin = ptr.Ref(int64(it.ValidationMin.Int32)) + } + + var vMax *int64 + if it.ValidationMax.Valid { + vMin = ptr.Ref(int64(it.ValidationMax.Int32)) + } + + var monotonic *string + if it.ValidationMonotonic != "" { + monotonic = ptr.Ref(it.ValidationMonotonic) + } + + param.Validations = append(param.Validations, &previewtypes.ParameterValidation{ + Error: it.ValidationError, + Regex: reg, + Min: vMin, + Max: vMax, + Monotonic: monotonic, + }) + } + + var protoOptions []*sdkproto.RichParameterOption + _ = json.Unmarshal(it.Options, &protoOptions) // Not going to make this fatal + for _, opt := range protoOptions { + param.Options = append(param.Options, &previewtypes.ParameterOption{ + Name: opt.Name, + Description: opt.Description, + Value: previewtypes.StringLiteral(opt.Value), + Icon: opt.Icon, + }) + } + + // Take the form type from the ValidateFormType function. This is a bit + // unfortunate we have to do this, but it will return the default form_type + // for a given set of conditions. + _, param.FormType, _ = provider.ValidateFormType(provider.OptionType(param.Type), len(param.Options), param.FormType) + + param.Diagnostics = previewtypes.Diagnostics(param.Valid(param.Value)) + params = append(params, param) + } + + return &StaticRender{ + staticParams: params, + }, nil +} + +func (r *StaticRender) Render(_ context.Context, _ uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { + params := r.staticParams + for i := range params { + param := ¶ms[i] + paramValue, ok := values[param.Name] + if ok { + param.Value = previewtypes.StringLiteral(paramValue) + } else { + param.Value = param.DefaultValue + } + param.Diagnostics = previewtypes.Diagnostics(param.Valid(param.Value)) + } + + return &preview.Output{ + Parameters: params, + }, hcl.Diagnostics{ + { + // Only a warning because the form does still work. + Severity: hcl.DiagWarning, + Summary: "This template version is missing required metadata to support dynamic parameters.", + Detail: "To restore full functionality, please re-import the terraform as a new template version.", + }, + } +} + +func (r *StaticRender) Close() {} diff --git a/coderd/parameters.go b/coderd/parameters.go index c88199956392d..5bb60c9f05627 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -2,30 +2,18 @@ package coderd import ( "context" - "database/sql" - "encoding/json" "net/http" "time" "github.com/google/uuid" - "github.com/hashicorp/hcl/v2" - "golang.org/x/sync/errgroup" "golang.org/x/xerrors" - "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" - "github.com/coder/coder/v2/coderd/database/dbauthz" - "github.com/coder/coder/v2/coderd/files" + "github.com/coder/coder/v2/coderd/dynamicparameters" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/httpmw" - "github.com/coder/coder/v2/coderd/util/ptr" - "github.com/coder/coder/v2/coderd/wsbuilder" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/wsjson" - sdkproto "github.com/coder/coder/v2/provisionersdk/proto" - "github.com/coder/preview" - previewtypes "github.com/coder/preview/types" - "github.com/coder/terraform-provider-coder/v2/provider" "github.com/coder/websocket" ) @@ -85,283 +73,54 @@ func (api *API) templateVersionDynamicParameters(listen bool, initial codersdk.D ctx := r.Context() templateVersion := httpmw.TemplateVersionParam(r) - // Check that the job has completed successfully - job, err := api.Database.GetProvisionerJobByID(ctx, templateVersion.JobID) - if httpapi.Is404Error(err) { - httpapi.ResourceNotFound(rw) - return - } + loader := dynamicparameters.New(templateVersion.ID). + WithTemplateVersion(templateVersion) + + err := loader.Load(ctx, api.Database) if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching provisioner job.", - Detail: err.Error(), - }) - return - } - if !job.CompletedAt.Valid { - httpapi.Write(ctx, rw, http.StatusTooEarly, codersdk.Response{ - Message: "Template version job has not finished", - }) - return - } - tf, err := api.Database.GetTemplateVersionTerraformValues(ctx, templateVersion.ID) - if err != nil && !xerrors.Is(err, sql.ErrNoRows) { + if httpapi.Is404Error(err) { + httpapi.ResourceNotFound(rw) + return + } + + if xerrors.Is(err, dynamicparameters.ErrorTemplateVersionNotReady) { + httpapi.Write(ctx, rw, http.StatusTooEarly, codersdk.Response{ + Message: "Template version job has not finished", + }) + } + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Failed to retrieve Terraform values for template version", + Message: "Internal error fetching template version data.", Detail: err.Error(), }) return } - if wsbuilder.ProvisionerVersionSupportsDynamicParameters(tf.ProvisionerdVersion) { - api.handleDynamicParameters(listen, rw, r, tf, templateVersion, initial) - } else { - api.handleStaticParameters(listen, rw, r, templateVersion.ID, initial) - } - } -} - -type previewFunction func(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) - -// nolint:revive -func (api *API) handleDynamicParameters(listen bool, rw http.ResponseWriter, r *http.Request, tf database.TemplateVersionTerraformValue, templateVersion database.TemplateVersion, initial codersdk.DynamicParametersRequest) { - var ( - ctx = r.Context() - apikey = httpmw.APIKey(r) - ) - - // nolint:gocritic // We need to fetch the templates files for the Terraform - // evaluator, and the user likely does not have permission. - fileCtx := dbauthz.AsFileReader(ctx) - fileID, err := api.Database.GetFileIDByTemplateVersionID(fileCtx, templateVersion.ID) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error finding template version Terraform.", - Detail: err.Error(), - }) - return - } - - // Add the file first. Calling `Release` if it fails is a no-op, so this is safe. - templateFS, err := api.FileCache.Acquire(fileCtx, fileID) - if err != nil { - httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ - Message: "Internal error fetching template version Terraform.", - Detail: err.Error(), - }) - return - } - defer api.FileCache.Release(fileID) - - // Having the Terraform plan available for the evaluation engine is helpful - // for populating values from data blocks, but isn't strictly required. If - // we don't have a cached plan available, we just use an empty one instead. - plan := json.RawMessage("{}") - if len(tf.CachedPlan) > 0 { - plan = tf.CachedPlan - } - - if tf.CachedModuleFiles.Valid { - moduleFilesFS, err := api.FileCache.Acquire(fileCtx, tf.CachedModuleFiles.UUID) + renderer, err := loader.Renderer(ctx, api.Database, api.FileCache) if err != nil { - httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ - Message: "Internal error fetching Terraform modules.", + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error creating renderer for template version.", Detail: err.Error(), }) return } - defer api.FileCache.Release(tf.CachedModuleFiles.UUID) - - templateFS = files.NewOverlayFS(templateFS, []files.Overlay{{Path: ".terraform/modules", FS: moduleFilesFS}}) - } - - owner, err := getWorkspaceOwnerData(ctx, api.Database, apikey.UserID, templateVersion.OrganizationID) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching workspace owner.", - Detail: err.Error(), - }) - return - } - - input := preview.Input{ - PlanJSON: plan, - ParameterValues: map[string]string{}, - Owner: owner, - } - - // failedOwners keeps track of which owners failed to fetch from the database. - // This prevents db spam on repeated requests for the same failed owner. - failedOwners := make(map[uuid.UUID]error) - failedOwnerDiag := hcl.Diagnostics{ - { - Severity: hcl.DiagError, - Summary: "Failed to fetch workspace owner", - Detail: "Please check your permissions or the user may not exist.", - Extra: previewtypes.DiagnosticExtra{ - Code: "owner_not_found", - }, - }, - } - - dynamicRender := func(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { - if ownerID == uuid.Nil { - // Default to the authenticated user - // Nice for testing - ownerID = apikey.UserID - } - - if _, ok := failedOwners[ownerID]; ok { - // If it has failed once, assume it will fail always. - // Re-open the websocket to try again. - return nil, failedOwnerDiag - } - - // Update the input values with the new values. - input.ParameterValues = values - - // Update the owner if there is a change - if input.Owner.ID != ownerID.String() { - owner, err = getWorkspaceOwnerData(ctx, api.Database, ownerID, templateVersion.OrganizationID) - if err != nil { - failedOwners[ownerID] = err - return nil, failedOwnerDiag - } - input.Owner = owner - } + defer renderer.Close() - return preview.Preview(ctx, input, templateFS) - } - if listen { - api.handleParameterWebsocket(rw, r, initial, dynamicRender) - } else { - api.handleParameterEvaluate(rw, r, initial, dynamicRender) - } -} - -// nolint:revive -func (api *API) handleStaticParameters(listen bool, rw http.ResponseWriter, r *http.Request, version uuid.UUID, initial codersdk.DynamicParametersRequest) { - ctx := r.Context() - dbTemplateVersionParameters, err := api.Database.GetTemplateVersionParameters(ctx, version) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Failed to retrieve template version parameters", - Detail: err.Error(), - }) - return - } - - params := make([]previewtypes.Parameter, 0, len(dbTemplateVersionParameters)) - for _, it := range dbTemplateVersionParameters { - param := previewtypes.Parameter{ - ParameterData: previewtypes.ParameterData{ - Name: it.Name, - DisplayName: it.DisplayName, - Description: it.Description, - Type: previewtypes.ParameterType(it.Type), - FormType: "", // ooooof - Styling: previewtypes.ParameterStyling{}, - Mutable: it.Mutable, - DefaultValue: previewtypes.StringLiteral(it.DefaultValue), - Icon: it.Icon, - Options: make([]*previewtypes.ParameterOption, 0), - Validations: make([]*previewtypes.ParameterValidation, 0), - Required: it.Required, - Order: int64(it.DisplayOrder), - Ephemeral: it.Ephemeral, - Source: nil, - }, - // Always use the default, since we used to assume the empty string - Value: previewtypes.StringLiteral(it.DefaultValue), - Diagnostics: nil, - } - - if it.ValidationError != "" || it.ValidationRegex != "" || it.ValidationMonotonic != "" { - var reg *string - if it.ValidationRegex != "" { - reg = ptr.Ref(it.ValidationRegex) - } - - var vMin *int64 - if it.ValidationMin.Valid { - vMin = ptr.Ref(int64(it.ValidationMin.Int32)) - } - - var vMax *int64 - if it.ValidationMax.Valid { - vMin = ptr.Ref(int64(it.ValidationMax.Int32)) - } - - var monotonic *string - if it.ValidationMonotonic != "" { - monotonic = ptr.Ref(it.ValidationMonotonic) - } - - param.Validations = append(param.Validations, &previewtypes.ParameterValidation{ - Error: it.ValidationError, - Regex: reg, - Min: vMin, - Max: vMax, - Monotonic: monotonic, - }) - } - - var protoOptions []*sdkproto.RichParameterOption - _ = json.Unmarshal(it.Options, &protoOptions) // Not going to make this fatal - for _, opt := range protoOptions { - param.Options = append(param.Options, &previewtypes.ParameterOption{ - Name: opt.Name, - Description: opt.Description, - Value: previewtypes.StringLiteral(opt.Value), - Icon: opt.Icon, - }) - } - - // Take the form type from the ValidateFormType function. This is a bit - // unfortunate we have to do this, but it will return the default form_type - // for a given set of conditions. - _, param.FormType, _ = provider.ValidateFormType(provider.OptionType(param.Type), len(param.Options), param.FormType) - - param.Diagnostics = previewtypes.Diagnostics(param.Valid(param.Value)) - params = append(params, param) - } - - staticRender := func(_ context.Context, _ uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { - for i := range params { - param := ¶ms[i] - paramValue, ok := values[param.Name] - if ok { - param.Value = previewtypes.StringLiteral(paramValue) - } else { - param.Value = param.DefaultValue - } - param.Diagnostics = previewtypes.Diagnostics(param.Valid(param.Value)) + if listen { + api.handleParameterWebsocket(rw, r, initial, renderer) + } else { + api.handleParameterEvaluate(rw, r, initial, renderer) } - return &preview.Output{ - Parameters: params, - }, hcl.Diagnostics{ - { - // Only a warning because the form does still work. - Severity: hcl.DiagWarning, - Summary: "This template version is missing required metadata to support dynamic parameters.", - Detail: "To restore full functionality, please re-import the terraform as a new template version.", - }, - } - } - if listen { - api.handleParameterWebsocket(rw, r, initial, staticRender) - } else { - api.handleParameterEvaluate(rw, r, initial, staticRender) } } -func (*API) handleParameterEvaluate(rw http.ResponseWriter, r *http.Request, initial codersdk.DynamicParametersRequest, render previewFunction) { +func (*API) handleParameterEvaluate(rw http.ResponseWriter, r *http.Request, initial codersdk.DynamicParametersRequest, render dynamicparameters.Renderer) { ctx := r.Context() // Send an initial form state, computed without any user input. - result, diagnostics := render(ctx, initial.OwnerID, initial.Inputs) + result, diagnostics := render.Render(ctx, initial.OwnerID, initial.Inputs) response := codersdk.DynamicParametersResponse{ ID: 0, Diagnostics: db2sdk.HCLDiagnostics(diagnostics), @@ -373,7 +132,7 @@ func (*API) handleParameterEvaluate(rw http.ResponseWriter, r *http.Request, ini httpapi.Write(ctx, rw, http.StatusOK, response) } -func (api *API) handleParameterWebsocket(rw http.ResponseWriter, r *http.Request, initial codersdk.DynamicParametersRequest, render previewFunction) { +func (api *API) handleParameterWebsocket(rw http.ResponseWriter, r *http.Request, initial codersdk.DynamicParametersRequest, render dynamicparameters.Renderer) { ctx, cancel := context.WithTimeout(r.Context(), 30*time.Minute) defer cancel() @@ -393,7 +152,7 @@ func (api *API) handleParameterWebsocket(rw http.ResponseWriter, r *http.Request ) // Send an initial form state, computed without any user input. - result, diagnostics := render(ctx, initial.OwnerID, initial.Inputs) + result, diagnostics := render.Render(ctx, initial.OwnerID, initial.Inputs) response := codersdk.DynamicParametersResponse{ ID: -1, // Always start with -1. Diagnostics: db2sdk.HCLDiagnostics(diagnostics), @@ -421,7 +180,7 @@ func (api *API) handleParameterWebsocket(rw http.ResponseWriter, r *http.Request return } - result, diagnostics := render(ctx, update.OwnerID, update.Inputs) + result, diagnostics := render.Render(ctx, update.OwnerID, update.Inputs) response := codersdk.DynamicParametersResponse{ ID: update.ID, Diagnostics: db2sdk.HCLDiagnostics(diagnostics), @@ -437,98 +196,3 @@ func (api *API) handleParameterWebsocket(rw http.ResponseWriter, r *http.Request } } } - -func getWorkspaceOwnerData( - ctx context.Context, - db database.Store, - ownerID uuid.UUID, - organizationID uuid.UUID, -) (previewtypes.WorkspaceOwner, error) { - var g errgroup.Group - - // TODO: @emyrk we should only need read access on the org member, not the - // site wide user object. Figure out a better way to handle this. - user, err := db.GetUserByID(ctx, ownerID) - if err != nil { - return previewtypes.WorkspaceOwner{}, xerrors.Errorf("fetch user: %w", err) - } - - var ownerRoles []previewtypes.WorkspaceOwnerRBACRole - g.Go(func() error { - // nolint:gocritic // This is kind of the wrong query to use here, but it - // matches how the provisioner currently works. We should figure out - // something that needs less escalation but has the correct behavior. - row, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), ownerID) - if err != nil { - return err - } - roles, err := row.RoleNames() - if err != nil { - return err - } - ownerRoles = make([]previewtypes.WorkspaceOwnerRBACRole, 0, len(roles)) - for _, it := range roles { - if it.OrganizationID != uuid.Nil && it.OrganizationID != organizationID { - continue - } - var orgID string - if it.OrganizationID != uuid.Nil { - orgID = it.OrganizationID.String() - } - ownerRoles = append(ownerRoles, previewtypes.WorkspaceOwnerRBACRole{ - Name: it.Name, - OrgID: orgID, - }) - } - return nil - }) - - var publicKey string - g.Go(func() error { - // The correct public key has to be sent. This will not be leaked - // unless the template leaks it. - // nolint:gocritic - key, err := db.GetGitSSHKey(dbauthz.AsSystemRestricted(ctx), ownerID) - if err != nil { - return err - } - publicKey = key.PublicKey - return nil - }) - - var groupNames []string - g.Go(func() error { - // The groups need to be sent to preview. These groups are not exposed to the - // user, unless the template does it through the parameters. Regardless, we need - // the correct groups, and a user might not have read access. - // nolint:gocritic - groups, err := db.GetGroups(dbauthz.AsSystemRestricted(ctx), database.GetGroupsParams{ - OrganizationID: organizationID, - HasMemberID: ownerID, - }) - if err != nil { - return err - } - groupNames = make([]string, 0, len(groups)) - for _, it := range groups { - groupNames = append(groupNames, it.Group.Name) - } - return nil - }) - - err = g.Wait() - if err != nil { - return previewtypes.WorkspaceOwner{}, err - } - - return previewtypes.WorkspaceOwner{ - ID: user.ID.String(), - Name: user.Username, - FullName: user.Name, - Email: user.Email, - LoginType: string(user.LoginType), - RBACRoles: ownerRoles, - SSHPublicKey: publicKey, - Groups: groupNames, - }, nil -} From c50ee0898c8c0a97e1848f4b9fd70aa58f70204c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 17 Jun 2025 17:44:20 -0500 Subject: [PATCH 03/22] fixup FileError test --- coderd/dynamicparameters/render.go | 59 +++++++++++++++++++----------- coderd/parameters.go | 1 - coderd/parameters_test.go | 13 +++++-- 3 files changed, 46 insertions(+), 27 deletions(-) diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index ac8141e4335cd..469098cf11651 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -26,7 +26,7 @@ type Renderer interface { } var ( - ErrorTemplateVersionNotReady error = xerrors.New("template version job not finished") + ErrorTemplateVersionNotReady = xerrors.New("template version job not finished") ) // Loader is used to load the necessary coder objects for rendering a template @@ -144,10 +144,11 @@ func (r *Loader) dynamicRenderer(ctx context.Context, db database.Store, cache * } return &DynamicRenderer{ - data: r, - templateFS: templateFS, - db: db, - plan: plan, + data: r, + templateFS: templateFS, + db: db, + plan: plan, + failedOwners: make(map[uuid.UUID]error), close: func() { cache.Release(r.job.FileID) if moduleFilesFS != nil { @@ -171,8 +172,14 @@ type DynamicRenderer struct { } func (r *DynamicRenderer) Render(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { - err := r.getWorkspaceOwnerData(ctx, ownerID) - if err != nil || r.currentOwner == nil { + // Always start with the cached error, if we have one. + ownerErr := r.failedOwners[ownerID] + if ownerErr == nil { + ownerErr = r.getWorkspaceOwnerData(ctx, ownerID) + } + + if ownerErr != nil || r.currentOwner == nil { + r.failedOwners[ownerID] = ownerErr return nil, hcl.Diagnostics{ { Severity: hcl.DiagError, @@ -199,16 +206,24 @@ func (r *DynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uui return nil // already fetched } - if r.failedOwners[ownerID] != nil { - // previously failed, do not try again - return r.failedOwners[ownerID] - } - var g errgroup.Group - // TODO: @emyrk we should only need read access on the org member, not the - // site wide user object. Figure out a better way to handle this. - user, err := r.db.GetUserByID(ctx, ownerID) + // You only need to be able to read the organization member to get the owner + // data. Only the terraform files can therefore leak more information than the + // caller should have access to. All this info should be public assuming you can + // read the user though. + mem, err := database.ExpectOne(r.db.OrganizationMembers(ctx, database.OrganizationMembersParams{ + OrganizationID: r.data.templateVersion.OrganizationID, + UserID: ownerID, + IncludeSystem: false, + })) + if err != nil { + return err + } + + // User data is required for the form. Org member is checked above + // nolint:gocritic + user, err := r.db.GetUserByID(dbauthz.AsProvisionerd(ctx), mem.OrganizationMember.UserID) if err != nil { return xerrors.Errorf("fetch user: %w", err) } @@ -218,7 +233,7 @@ func (r *DynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uui // nolint:gocritic // This is kind of the wrong query to use here, but it // matches how the provisioner currently works. We should figure out // something that needs less escalation but has the correct behavior. - row, err := r.db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), ownerID) + row, err := r.db.GetAuthorizationUserRoles(dbauthz.AsProvisionerd(ctx), ownerID) if err != nil { return err } @@ -248,7 +263,7 @@ func (r *DynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uui // The correct public key has to be sent. This will not be leaked // unless the template leaks it. // nolint:gocritic - key, err := r.db.GetGitSSHKey(dbauthz.AsSystemRestricted(ctx), ownerID) + key, err := r.db.GetGitSSHKey(dbauthz.AsProvisionerd(ctx), ownerID) if err != nil { return err } @@ -262,7 +277,7 @@ func (r *DynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uui // user, unless the template does it through the parameters. Regardless, we need // the correct groups, and a user might not have read access. // nolint:gocritic - groups, err := r.db.GetGroups(dbauthz.AsSystemRestricted(ctx), database.GetGroupsParams{ + groups, err := r.db.GetGroups(dbauthz.AsProvisionerd(ctx), database.GetGroupsParams{ OrganizationID: r.data.templateVersion.OrganizationID, HasMemberID: ownerID, }) @@ -282,10 +297,10 @@ func (r *DynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uui } r.currentOwner = &previewtypes.WorkspaceOwner{ - ID: user.ID.String(), - Name: user.Username, - FullName: user.Name, - Email: user.Email, + ID: mem.OrganizationMember.UserID.String(), + Name: mem.Username, + FullName: mem.Name, + Email: mem.Email, LoginType: string(user.LoginType), RBACRoles: ownerRoles, SSHPublicKey: publicKey, diff --git a/coderd/parameters.go b/coderd/parameters.go index 5bb60c9f05627..1a51f6620573f 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -112,7 +112,6 @@ func (api *API) templateVersionDynamicParameters(listen bool, initial codersdk.D } else { api.handleParameterEvaluate(rw, r, initial, renderer) } - } } diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go index 3c792c2ce9a7a..3ea117d09178e 100644 --- a/coderd/parameters_test.go +++ b/coderd/parameters_test.go @@ -203,11 +203,16 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { provisionerDaemonVersion: provProto.CurrentVersion.String(), mainTF: dynamicParametersTerraformSource, modulesArchive: modulesArchive, - expectWebsocketError: true, }) - // This is checked in setupDynamicParamsTest. Just doing this in the - // test to make it obvious what this test is doing. - require.Zero(t, setup.api.FileCache.Count()) + + stream := setup.stream + previews := stream.Chan() + + // Assert the failed owner + ctx := testutil.Context(t, testutil.WaitShort) + preview := testutil.RequireReceive(ctx, t, previews) + require.Len(t, preview.Diagnostics, 1) + require.Equal(t, preview.Diagnostics[0].Summary, "Failed to fetch workspace owner") }) t.Run("RebuildParameters", func(t *testing.T) { From 8152bb747cc435b47127cf8ea254184d723b6bad Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 17 Jun 2025 17:51:12 -0500 Subject: [PATCH 04/22] more wip --- coderd/dynamicparameter_test.go | 21 +++++++++++++++++++++ coderd/dynamicparameters/render.go | 2 +- coderd/testdata/parameters/dynamic/main.tf | 22 ++++++++++++++++++++++ 3 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 coderd/dynamicparameter_test.go create mode 100644 coderd/testdata/parameters/dynamic/main.tf diff --git a/coderd/dynamicparameter_test.go b/coderd/dynamicparameter_test.go new file mode 100644 index 0000000000000..203a597e30fc2 --- /dev/null +++ b/coderd/dynamicparameter_test.go @@ -0,0 +1,21 @@ +package coderd_test + +import ( + _ "embed" + "os" + "testing" + + "github.com/stretchr/testify/require" + + provProto "github.com/coder/coder/v2/provisionerd/proto" +) + +func TestDynamicParameterTemplate(t *testing.T) { + dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/dynamic/main.tf") + require.NoError(t, err) + + setupDynamicParamsTest(t, setupDynamicParamsTestParams{ + provisionerDaemonVersion: provProto.CurrentVersion.String(), + mainTF: dynamicParametersTerraformSource, + }) +} diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index 469098cf11651..b542703365252 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -194,7 +194,7 @@ func (r *DynamicRenderer) Render(ctx context.Context, ownerID uuid.UUID, values input := preview.Input{ PlanJSON: r.data.terraformValues.CachedPlan, - ParameterValues: map[string]string{}, + ParameterValues: values, Owner: *r.currentOwner, } diff --git a/coderd/testdata/parameters/dynamic/main.tf b/coderd/testdata/parameters/dynamic/main.tf new file mode 100644 index 0000000000000..fe587e14b3ede --- /dev/null +++ b/coderd/testdata/parameters/dynamic/main.tf @@ -0,0 +1,22 @@ +terraform { + required_providers { + coder = { + source = "coder/coder" + version = "2.5.3" + } + } +} + +data "coder_workspace_owner" "me" {} + +locals { + isAdmin = contains(data.coder_workspace_owner.me.groups, "admin") +} + +data "coder_parameter" "isAdmin" { + name = "isAdmin" + type = "bool" + form_type = "switch" + default = local.isAdmin + order = 1 +} From 247470ee1f0b4b6c77054cf58061ec8d30b98502 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 18 Jun 2025 10:47:40 -0500 Subject: [PATCH 05/22] chore: add dynamic parameter unit test --- coderd/coderdtest/dynamicparameters.go | 129 ++++++++++++++++++ coderd/coderdtest/stream.go | 25 ++++ coderd/dynamicparameter_test.go | 21 --- coderd/parameters_test.go | 26 +--- coderd/testdata/parameters/dynamic/main.tf | 22 --- coderd/util/slice/slice.go | 13 ++ enterprise/coderd/dynamicparameters_test.go | 124 +++++++++++++++++ .../testdata/parameters/dynamic/main.tf | 93 +++++++++++++ 8 files changed, 389 insertions(+), 64 deletions(-) create mode 100644 coderd/coderdtest/dynamicparameters.go create mode 100644 coderd/coderdtest/stream.go delete mode 100644 coderd/dynamicparameter_test.go delete mode 100644 coderd/testdata/parameters/dynamic/main.tf create mode 100644 enterprise/coderd/dynamicparameters_test.go create mode 100644 enterprise/coderd/testdata/parameters/dynamic/main.tf diff --git a/coderd/coderdtest/dynamicparameters.go b/coderd/coderdtest/dynamicparameters.go new file mode 100644 index 0000000000000..b5bb34a0e3468 --- /dev/null +++ b/coderd/coderdtest/dynamicparameters.go @@ -0,0 +1,129 @@ +package coderdtest + +import ( + "encoding/json" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/util/ptr" + "github.com/coder/coder/v2/coderd/util/slice" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/provisioner/echo" + "github.com/coder/coder/v2/provisionersdk/proto" +) + +type DynamicParameterTemplateParams struct { + MainTF string + Plan json.RawMessage + ModulesArchive []byte + + // StaticParams is used if the provisioner daemon version does not support dynamic parameters. + StaticParams []*proto.RichParameter +} + +func DynamicParameterTemplate(t *testing.T, client *codersdk.Client, org uuid.UUID, args DynamicParameterTemplateParams) (codersdk.Template, codersdk.TemplateVersion) { + t.Helper() + + files := echo.WithExtraFiles(map[string][]byte{ + "main.tf": []byte(args.MainTF), + }) + files.ProvisionPlan = []*proto.Response{{ + Type: &proto.Response_Plan{ + Plan: &proto.PlanComplete{ + Plan: args.Plan, + ModuleFiles: args.ModulesArchive, + Parameters: args.StaticParams, + }, + }, + }} + + version := CreateTemplateVersion(t, client, org, files) + AwaitTemplateVersionJobCompleted(t, client, version.ID) + tpl := CreateTemplate(t, client, org, version.ID) + + var err error + tpl, err = client.UpdateTemplateMeta(t.Context(), tpl.ID, codersdk.UpdateTemplateMeta{ + UseClassicParameterFlow: ptr.Ref(false), + }) + require.NoError(t, err) + + return tpl, version +} + +type ParameterAsserter struct { + Name string + Params []codersdk.PreviewParameter + t *testing.T +} + +func AssertParameter(t *testing.T, name string, params []codersdk.PreviewParameter) *ParameterAsserter { + return &ParameterAsserter{ + Name: name, + Params: params, + t: t, + } +} + +func (a *ParameterAsserter) find(name string) *codersdk.PreviewParameter { + a.t.Helper() + for _, p := range a.Params { + if p.Name == name { + return &p + } + } + + assert.Fail(a.t, "parameter not found", "expected parameter %q to exist", a.Name) + return nil +} + +func (a *ParameterAsserter) NotExists() *ParameterAsserter { + a.t.Helper() + + names := slice.Convert(a.Params, func(p codersdk.PreviewParameter) string { + return p.Name + }) + + assert.NotContains(a.t, names, a.Name) + return a +} + +func (a *ParameterAsserter) Exists() *ParameterAsserter { + a.t.Helper() + + names := slice.Convert(a.Params, func(p codersdk.PreviewParameter) string { + return p.Name + }) + + assert.Contains(a.t, names, a.Name) + return a +} + +func (a *ParameterAsserter) Value(expected string) *ParameterAsserter { + a.t.Helper() + + p := a.find(a.Name) + if p == nil { + return a + } + + assert.Equal(a.t, expected, p.Value.Value) + return a +} + +func (a *ParameterAsserter) Options(expected ...string) *ParameterAsserter { + a.t.Helper() + + p := a.find(a.Name) + if p == nil { + return a + } + + optValues := slice.Convert(p.Options, func(p codersdk.PreviewParameterOption) string { + return p.Value.Value + }) + assert.ElementsMatch(a.t, expected, optValues, "parameter %q options", a.Name) + return a +} diff --git a/coderd/coderdtest/stream.go b/coderd/coderdtest/stream.go new file mode 100644 index 0000000000000..83bcce2ed29db --- /dev/null +++ b/coderd/coderdtest/stream.go @@ -0,0 +1,25 @@ +package coderdtest + +import "github.com/coder/coder/v2/codersdk/wsjson" + +// SynchronousStream returns a function that assumes the stream is synchronous. +// Meaning each request sent assumes exactly one response will be received. +// The function will block until the response is received or an error occurs. +// +// This should not be used in production code, as it does not handle edge cases. +// The second function `pop` can be used to retrieve the next response from the +// stream without sending a new request. This is useful for dynamic parameters +func SynchronousStream[R any, W any](stream *wsjson.Stream[R, W]) (do func(W) (R, error), pop func() R) { + rec := stream.Chan() + + return func(req W) (R, error) { + err := stream.Send(req) + if err != nil { + return *new(R), err + } + + return <-rec, nil + }, func() R { + return <-rec + } +} diff --git a/coderd/dynamicparameter_test.go b/coderd/dynamicparameter_test.go deleted file mode 100644 index 203a597e30fc2..0000000000000 --- a/coderd/dynamicparameter_test.go +++ /dev/null @@ -1,21 +0,0 @@ -package coderd_test - -import ( - _ "embed" - "os" - "testing" - - "github.com/stretchr/testify/require" - - provProto "github.com/coder/coder/v2/provisionerd/proto" -) - -func TestDynamicParameterTemplate(t *testing.T) { - dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/dynamic/main.tf") - require.NoError(t, err) - - setupDynamicParamsTest(t, setupDynamicParamsTestParams{ - provisionerDaemonVersion: provProto.CurrentVersion.String(), - mainTF: dynamicParametersTerraformSource, - }) -} diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go index 3ea117d09178e..794ff8db3354d 100644 --- a/coderd/parameters_test.go +++ b/coderd/parameters_test.go @@ -368,28 +368,12 @@ func setupDynamicParamsTest(t *testing.T, args setupDynamicParamsTestParams) dyn owner := coderdtest.CreateFirstUser(t, ownerClient) templateAdmin, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.RoleTemplateAdmin()) - files := echo.WithExtraFiles(map[string][]byte{ - "main.tf": args.mainTF, + tpl, version := coderdtest.DynamicParameterTemplate(t, templateAdmin, owner.OrganizationID, coderdtest.DynamicParameterTemplateParams{ + MainTF: string(args.mainTF), + Plan: args.plan, + ModulesArchive: args.modulesArchive, + StaticParams: args.static, }) - files.ProvisionPlan = []*proto.Response{{ - Type: &proto.Response_Plan{ - Plan: &proto.PlanComplete{ - Plan: args.plan, - ModuleFiles: args.modulesArchive, - Parameters: args.static, - }, - }, - }} - - version := coderdtest.CreateTemplateVersion(t, templateAdmin, owner.OrganizationID, files) - coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, version.ID) - tpl := coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, version.ID) - - var err error - tpl, err = templateAdmin.UpdateTemplateMeta(t.Context(), tpl.ID, codersdk.UpdateTemplateMeta{ - UseClassicParameterFlow: ptr.Ref(false), - }) - require.NoError(t, err) ctx := testutil.Context(t, testutil.WaitShort) stream, err := templateAdmin.TemplateVersionDynamicParameters(ctx, codersdk.Me, version.ID) diff --git a/coderd/testdata/parameters/dynamic/main.tf b/coderd/testdata/parameters/dynamic/main.tf deleted file mode 100644 index fe587e14b3ede..0000000000000 --- a/coderd/testdata/parameters/dynamic/main.tf +++ /dev/null @@ -1,22 +0,0 @@ -terraform { - required_providers { - coder = { - source = "coder/coder" - version = "2.5.3" - } - } -} - -data "coder_workspace_owner" "me" {} - -locals { - isAdmin = contains(data.coder_workspace_owner.me.groups, "admin") -} - -data "coder_parameter" "isAdmin" { - name = "isAdmin" - type = "bool" - form_type = "switch" - default = local.isAdmin - order = 1 -} diff --git a/coderd/util/slice/slice.go b/coderd/util/slice/slice.go index f3811650786b7..67d4a939713c0 100644 --- a/coderd/util/slice/slice.go +++ b/coderd/util/slice/slice.go @@ -217,3 +217,16 @@ func CountConsecutive[T comparable](needle T, haystack ...T) int { return max(maxLength, curLength) } + +// Convert converts a slice of type F to a slice of type T using the provided function f. +func Convert[F any, T any](a []F, f func(F) T) []T { + if a == nil { + return nil + } + + tmp := make([]T, 0, len(a)) + for _, v := range a { + tmp = append(tmp, f(v)) + } + return tmp +} diff --git a/enterprise/coderd/dynamicparameters_test.go b/enterprise/coderd/dynamicparameters_test.go new file mode 100644 index 0000000000000..b659b41105e44 --- /dev/null +++ b/enterprise/coderd/dynamicparameters_test.go @@ -0,0 +1,124 @@ +package coderd_test + +import ( + _ "embed" + "os" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/enterprise/coderd/coderdenttest" + "github.com/coder/coder/v2/enterprise/coderd/license" + "github.com/coder/coder/v2/testutil" + "github.com/coder/websocket" +) + +// TestDynamicParameterTemplate uses a template with some dynamic elements, and +// tests the parameters, values, etc are all as expected. +func TestDynamicParameterTemplate(t *testing.T) { + t.Parallel() + + owner, _, api, first := coderdenttest.NewWithAPI(t, &coderdenttest.Options{ + Options: &coderdtest.Options{IncludeProvisionerDaemon: true}, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }, + }) + + orgID := first.OrganizationID + + _, userData := coderdtest.CreateAnotherUser(t, owner, orgID) + templateAdmin, templateAdminData := coderdtest.CreateAnotherUser(t, owner, orgID, rbac.ScopedRoleOrgTemplateAdmin(orgID)) + userAdmin, userAdminData := coderdtest.CreateAnotherUser(t, owner, orgID, rbac.ScopedRoleOrgUserAdmin(orgID)) + _, auditorData := coderdtest.CreateAnotherUser(t, owner, orgID, rbac.ScopedRoleOrgAuditor(orgID)) + + coderdtest.CreateGroup(t, owner, orgID, "developer", auditorData, userData) + coderdtest.CreateGroup(t, owner, orgID, "admin", templateAdminData, userAdminData) + coderdtest.CreateGroup(t, owner, orgID, "auditor", auditorData, templateAdminData, userAdminData) + + dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/dynamic/main.tf") + require.NoError(t, err) + + _, version := coderdtest.DynamicParameterTemplate(t, templateAdmin, orgID, coderdtest.DynamicParameterTemplateParams{ + MainTF: string(dynamicParametersTerraformSource), + Plan: nil, + ModulesArchive: nil, + StaticParams: nil, + }) + + var _ = userAdmin + + ctx := testutil.Context(t, testutil.WaitLong) + + stream, err := templateAdmin.TemplateVersionDynamicParameters(ctx, userData.ID.String(), version.ID) + require.NoError(t, err) + defer func() { + _ = stream.Close(websocket.StatusNormalClosure) + + // Wait until the cache ends up empty. This verifies the cache does not + // leak any files. + require.Eventually(t, func() bool { + return api.AGPL.FileCache.Count() == 0 + }, testutil.WaitShort, testutil.IntervalFast, "file cache should be empty after the test") + }() + + // Initial response + preview, pop := coderdtest.SynchronousStream(stream) + init := pop() + coderdtest.AssertParameter(t, "isAdmin", init.Parameters). + Exists().Value("false") + coderdtest.AssertParameter(t, "adminonly", init.Parameters). + NotExists() + coderdtest.AssertParameter(t, "groups", init.Parameters). + Exists().Options(database.EveryoneGroup, "developer") + require.Len(t, init.Diagnostics, 0, "no top level diags") + + // Switch to an admin + resp, err := preview(codersdk.DynamicParametersRequest{ + ID: 1, + Inputs: map[string]string{ + "colors": `["red"]`, + "thing": "apple", + }, + OwnerID: userAdminData.ID, + }) + require.NoError(t, err) + require.Equal(t, resp.ID, 1) + + coderdtest.AssertParameter(t, "isAdmin", resp.Parameters). + Exists().Value("true") + coderdtest.AssertParameter(t, "adminonly", resp.Parameters). + Exists() + coderdtest.AssertParameter(t, "groups", resp.Parameters). + Exists().Options(database.EveryoneGroup, "admin", "auditor") + coderdtest.AssertParameter(t, "colors", resp.Parameters). + Exists().Value(`["red"]`) + coderdtest.AssertParameter(t, "thing", resp.Parameters). + Exists().Value("apple").Options("apple", "ruby") + require.Len(t, init.Diagnostics, 0, "no top level diags") + + // Try some other colors + resp, err = preview(codersdk.DynamicParametersRequest{ + ID: 2, + Inputs: map[string]string{ + "colors": `["yellow", "blue"]`, + "thing": "banana", + }, + OwnerID: userAdminData.ID, + }) + require.NoError(t, err) + require.Equal(t, resp.ID, 2) + + coderdtest.AssertParameter(t, "isAdmin", resp.Parameters). + Exists().Value("true") + coderdtest.AssertParameter(t, "colors", resp.Parameters). + Exists().Value(`["yellow", "blue"]`) + coderdtest.AssertParameter(t, "thing", resp.Parameters). + Exists().Value("banana").Options("banana", "ocean", "sky") +} diff --git a/enterprise/coderd/testdata/parameters/dynamic/main.tf b/enterprise/coderd/testdata/parameters/dynamic/main.tf new file mode 100644 index 0000000000000..3fd3e7e69c766 --- /dev/null +++ b/enterprise/coderd/testdata/parameters/dynamic/main.tf @@ -0,0 +1,93 @@ +terraform { + required_providers { + coder = { + source = "coder/coder" + version = "2.5.3" + } + } +} + +data "coder_workspace_owner" "me" {} + +locals { + isAdmin = contains(data.coder_workspace_owner.me.groups, "admin") +} + +data "coder_parameter" "isAdmin" { + name = "isAdmin" + type = "bool" + form_type = "switch" + default = local.isAdmin + order = 1 +} + +data "coder_parameter" "adminonly" { + count = local.isAdmin ? 1 : 0 + name = "adminonly" + form_type = "input" + type = "string" + default = "I am an admin!" + order = 2 +} + + +data "coder_parameter" "groups" { + name = "groups" + type = "list(string)" + form_type = "multi-select" + default = jsonencode([data.coder_workspace_owner.me.groups[0]]) + order = 50 + + dynamic "option" { + for_each = data.coder_workspace_owner.me.groups + content { + name = option.value + value = option.value + } + } +} + +locals { + colors = { + "red": ["apple", "ruby"] + "yellow": ["banana"] + "blue": ["ocean", "sky"] + } +} + +data "coder_parameter" "colors" { + name = "colors" + type = "list(string)" + form_type = "multi-select" + order = 100 + + dynamic "option" { + for_each = keys(local.colors) + content { + name = option.value + value = option.value + } + } +} + +locals { + selected = jsondecode(data.coder_parameter.colors.value) + things = flatten([ + for color in local.selected : local.colors[color] + ]) +} + +data "coder_parameter" "thing" { + name = "thing" + type = "string" + form_type = "dropdown" + order = 101 + + dynamic "option" { + for_each = local.things + content { + name = option.value + value = option.value + } + } +} From 6799896e67c4e732ea83aa5237463cac06f581de Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 18 Jun 2025 10:50:36 -0500 Subject: [PATCH 06/22] add another dynamic param --- enterprise/coderd/dynamicparameters_test.go | 9 +++++++-- enterprise/coderd/testdata/parameters/dynamic/main.tf | 10 ++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/enterprise/coderd/dynamicparameters_test.go b/enterprise/coderd/dynamicparameters_test.go index b659b41105e44..e3ac4ed280dea 100644 --- a/enterprise/coderd/dynamicparameters_test.go +++ b/enterprise/coderd/dynamicparameters_test.go @@ -71,13 +71,13 @@ func TestDynamicParameterTemplate(t *testing.T) { // Initial response preview, pop := coderdtest.SynchronousStream(stream) init := pop() + require.Len(t, init.Diagnostics, 0, "no top level diags") coderdtest.AssertParameter(t, "isAdmin", init.Parameters). Exists().Value("false") coderdtest.AssertParameter(t, "adminonly", init.Parameters). NotExists() coderdtest.AssertParameter(t, "groups", init.Parameters). Exists().Options(database.EveryoneGroup, "developer") - require.Len(t, init.Diagnostics, 0, "no top level diags") // Switch to an admin resp, err := preview(codersdk.DynamicParametersRequest{ @@ -90,6 +90,7 @@ func TestDynamicParameterTemplate(t *testing.T) { }) require.NoError(t, err) require.Equal(t, resp.ID, 1) + require.Len(t, resp.Diagnostics, 0, "no top level diags") coderdtest.AssertParameter(t, "isAdmin", resp.Parameters). Exists().Value("true") @@ -101,7 +102,8 @@ func TestDynamicParameterTemplate(t *testing.T) { Exists().Value(`["red"]`) coderdtest.AssertParameter(t, "thing", resp.Parameters). Exists().Value("apple").Options("apple", "ruby") - require.Len(t, init.Diagnostics, 0, "no top level diags") + coderdtest.AssertParameter(t, "cool", resp.Parameters). + NotExists() // Try some other colors resp, err = preview(codersdk.DynamicParametersRequest{ @@ -114,7 +116,10 @@ func TestDynamicParameterTemplate(t *testing.T) { }) require.NoError(t, err) require.Equal(t, resp.ID, 2) + require.Len(t, resp.Diagnostics, 0, "no top level diags") + coderdtest.AssertParameter(t, "cool", resp.Parameters). + Exists() coderdtest.AssertParameter(t, "isAdmin", resp.Parameters). Exists().Value("true") coderdtest.AssertParameter(t, "colors", resp.Parameters). diff --git a/enterprise/coderd/testdata/parameters/dynamic/main.tf b/enterprise/coderd/testdata/parameters/dynamic/main.tf index 3fd3e7e69c766..1ffc95f2b6863 100644 --- a/enterprise/coderd/testdata/parameters/dynamic/main.tf +++ b/enterprise/coderd/testdata/parameters/dynamic/main.tf @@ -91,3 +91,13 @@ data "coder_parameter" "thing" { } } } + +// Cool people like blue. Idk what to tell you. +data "coder_parameter" "cool" { + count = contains(local.selected, "blue") ? 1 : 0 + name = "cool" + type = "bool" + form_type = "switch" + order = 102 + default = "true" +} From 08bffe2c9aa750fc31cfd53a3e2bab505be21ab5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 18 Jun 2025 11:02:32 -0500 Subject: [PATCH 07/22] add some comments --- coderd/dynamicparameters/render.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index b542703365252..493b96414c548 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -20,6 +20,12 @@ import ( "github.com/hashicorp/hcl/v2" ) +// Renderer is able to execute and evaluate terraform with the given inputs. +// It may use the database to fetch additional state, such as a user's groups, +// roles, etc. Therefore, it requires an authenticated `ctx`. +// +// 'Close()' **must** be called once the renderer is no longer needed. +// Forgetting to do so will result in a memory leak. type Renderer interface { Render(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) Close() @@ -31,8 +37,7 @@ var ( // Loader is used to load the necessary coder objects for rendering a template // version's parameters. The output is a Renderer, which is the object that uses -// the cached objects to render the template version's parameters. Closing the -// Renderer will release the cached files. +// the cached objects to render the template version's parameters. type Loader struct { templateVersionID uuid.UUID @@ -106,6 +111,13 @@ func (r *Loader) loaded() bool { return r.templateVersion != nil && r.job != nil && r.terraformValues != nil } +// Renderer returns a Renderer that can be used to render the template version's +// parameters. It automatically determines whether to use a static or dynamic +// renderer based on the template version's state. +// +// Static parameter rendering is required to support older template versions that +// do not have the database state to support dynamic parameters. A constant +// warning will be displayed for these template versions. func (r *Loader) Renderer(ctx context.Context, db database.Store, cache *files.Cache) (Renderer, error) { if !r.loaded() { return nil, xerrors.New("Load() must be called before Renderer()") From f651edc773663a37aa0e1ddab2970e4d0df0ee5b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 18 Jun 2025 13:42:44 -0500 Subject: [PATCH 08/22] fmt --- coderd/dynamicparameters/render.go | 4 +- enterprise/coderd/dynamicparameters_test.go | 2 +- .../testdata/parameters/dynamic/main.tf | 62 +++++++++---------- 3 files changed, 33 insertions(+), 35 deletions(-) diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index 493b96414c548..a66fc9e9d4b25 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -31,9 +31,7 @@ type Renderer interface { Close() } -var ( - ErrorTemplateVersionNotReady = xerrors.New("template version job not finished") -) +var ErrorTemplateVersionNotReady = xerrors.New("template version job not finished") // Loader is used to load the necessary coder objects for rendering a template // version's parameters. The output is a Renderer, which is the object that uses diff --git a/enterprise/coderd/dynamicparameters_test.go b/enterprise/coderd/dynamicparameters_test.go index e3ac4ed280dea..60d68fecd87d1 100644 --- a/enterprise/coderd/dynamicparameters_test.go +++ b/enterprise/coderd/dynamicparameters_test.go @@ -52,7 +52,7 @@ func TestDynamicParameterTemplate(t *testing.T) { StaticParams: nil, }) - var _ = userAdmin + _ = userAdmin ctx := testutil.Context(t, testutil.WaitLong) diff --git a/enterprise/coderd/testdata/parameters/dynamic/main.tf b/enterprise/coderd/testdata/parameters/dynamic/main.tf index 1ffc95f2b6863..615f57dc9c074 100644 --- a/enterprise/coderd/testdata/parameters/dynamic/main.tf +++ b/enterprise/coderd/testdata/parameters/dynamic/main.tf @@ -1,7 +1,7 @@ terraform { required_providers { coder = { - source = "coder/coder" + source = "coder/coder" version = "2.5.3" } } @@ -14,29 +14,29 @@ locals { } data "coder_parameter" "isAdmin" { - name = "isAdmin" - type = "bool" - form_type = "switch" - default = local.isAdmin - order = 1 + name = "isAdmin" + type = "bool" + form_type = "switch" + default = local.isAdmin + order = 1 } data "coder_parameter" "adminonly" { - count = local.isAdmin ? 1 : 0 - name = "adminonly" + count = local.isAdmin ? 1 : 0 + name = "adminonly" form_type = "input" type = "string" default = "I am an admin!" - order = 2 + order = 2 } data "coder_parameter" "groups" { - name = "groups" - type = "list(string)" - form_type = "multi-select" - default = jsonencode([data.coder_workspace_owner.me.groups[0]]) - order = 50 + name = "groups" + type = "list(string)" + form_type = "multi-select" + default = jsonencode([data.coder_workspace_owner.me.groups[0]]) + order = 50 dynamic "option" { for_each = data.coder_workspace_owner.me.groups @@ -49,17 +49,17 @@ data "coder_parameter" "groups" { locals { colors = { - "red": ["apple", "ruby"] - "yellow": ["banana"] - "blue": ["ocean", "sky"] + "red" : ["apple", "ruby"] + "yellow" : ["banana"] + "blue" : ["ocean", "sky"] } } data "coder_parameter" "colors" { - name = "colors" - type = "list(string)" - form_type = "multi-select" - order = 100 + name = "colors" + type = "list(string)" + form_type = "multi-select" + order = 100 dynamic "option" { for_each = keys(local.colors) @@ -78,10 +78,10 @@ locals { } data "coder_parameter" "thing" { - name = "thing" - type = "string" - form_type = "dropdown" - order = 101 + name = "thing" + type = "string" + form_type = "dropdown" + order = 101 dynamic "option" { for_each = local.things @@ -94,10 +94,10 @@ data "coder_parameter" "thing" { // Cool people like blue. Idk what to tell you. data "coder_parameter" "cool" { - count = contains(local.selected, "blue") ? 1 : 0 - name = "cool" - type = "bool" - form_type = "switch" - order = 102 - default = "true" + count = contains(local.selected, "blue") ? 1 : 0 + name = "cool" + type = "bool" + form_type = "switch" + order = 102 + default = "true" } From 67644cc99cc41e285224f3efb0c07032636572cf Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 18 Jun 2025 13:49:54 -0500 Subject: [PATCH 09/22] remove need to constantly send owner id --- coderd/dynamicparameters/render.go | 23 ++++++++++++----------- coderd/parameters.go | 9 +++++++++ enterprise/coderd/parameters_test.go | 12 +++++------- 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index a66fc9e9d4b25..5a47214c22e7b 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -105,7 +105,7 @@ func (r *Loader) Load(ctx context.Context, db database.Store) error { return nil } -func (r *Loader) loaded() bool { +func (r *Loader) isReady() bool { return r.templateVersion != nil && r.job != nil && r.terraformValues != nil } @@ -117,7 +117,7 @@ func (r *Loader) loaded() bool { // do not have the database state to support dynamic parameters. A constant // warning will be displayed for these template versions. func (r *Loader) Renderer(ctx context.Context, db database.Store, cache *files.Cache) (Renderer, error) { - if !r.loaded() { + if !r.isReady() { return nil, xerrors.New("Load() must be called before Renderer()") } @@ -154,11 +154,11 @@ func (r *Loader) dynamicRenderer(ctx context.Context, db database.Store, cache * } return &DynamicRenderer{ - data: r, - templateFS: templateFS, - db: db, - plan: plan, - failedOwners: make(map[uuid.UUID]error), + data: r, + templateFS: templateFS, + db: db, + plan: plan, + ownerErrors: make(map[uuid.UUID]error), close: func() { cache.Release(r.job.FileID) if moduleFilesFS != nil { @@ -174,8 +174,9 @@ type DynamicRenderer struct { templateFS fs.FS plan json.RawMessage - failedOwners map[uuid.UUID]error - currentOwner *previewtypes.WorkspaceOwner + ownerErrors map[uuid.UUID]error + currentOwner *previewtypes.WorkspaceOwner + currentOwnerID uuid.UUID once sync.Once close func() @@ -183,13 +184,13 @@ type DynamicRenderer struct { func (r *DynamicRenderer) Render(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { // Always start with the cached error, if we have one. - ownerErr := r.failedOwners[ownerID] + ownerErr := r.ownerErrors[ownerID] if ownerErr == nil { ownerErr = r.getWorkspaceOwnerData(ctx, ownerID) } if ownerErr != nil || r.currentOwner == nil { - r.failedOwners[ownerID] = ownerErr + r.ownerErrors[ownerID] = ownerErr return nil, hcl.Diagnostics{ { Severity: hcl.DiagError, diff --git a/coderd/parameters.go b/coderd/parameters.go index 1a51f6620573f..cf2ce2c7162d3 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -168,6 +168,7 @@ func (api *API) handleParameterWebsocket(rw http.ResponseWriter, r *http.Request // As the user types into the form, reprocess the state using their input, // and respond with updates. updates := stream.Chan() + ownerID := initial.OwnerID for { select { case <-ctx.Done(): @@ -179,6 +180,14 @@ func (api *API) handleParameterWebsocket(rw http.ResponseWriter, r *http.Request return } + // Take a nil uuid to mean the previous owner ID. + // This just removes the need to constantly send who you are. + if update.OwnerID == uuid.Nil { + update.OwnerID = ownerID + } + + ownerID = update.OwnerID + result, diagnostics := render.Render(ctx, update.OwnerID, update.Inputs) response := codersdk.DynamicParametersResponse{ ID: update.ID, diff --git a/enterprise/coderd/parameters_test.go b/enterprise/coderd/parameters_test.go index 93f5057206527..bda9e3c59e021 100644 --- a/enterprise/coderd/parameters_test.go +++ b/enterprise/coderd/parameters_test.go @@ -31,7 +31,7 @@ func TestDynamicParametersOwnerGroups(t *testing.T) { Options: &coderdtest.Options{IncludeProvisionerDaemon: true}, }, ) - templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.RoleTemplateAdmin()) + templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.ScopedRoleOrgTemplateAdmin(owner.OrganizationID)) _, noGroupUser := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID) // Create the group to be asserted @@ -79,10 +79,10 @@ func TestDynamicParametersOwnerGroups(t *testing.T) { require.NoError(t, err) defer stream.Close(websocket.StatusGoingAway) - previews := stream.Chan() + previews, pop := coderdtest.SynchronousStream(stream) // Should automatically send a form state with all defaulted/empty values - preview := testutil.RequireReceive(ctx, t, previews) + preview := pop() require.Equal(t, -1, preview.ID) require.Empty(t, preview.Diagnostics) require.Equal(t, "group", preview.Parameters[0].Name) @@ -90,12 +90,11 @@ func TestDynamicParametersOwnerGroups(t *testing.T) { require.Equal(t, database.EveryoneGroup, preview.Parameters[0].Value.Value) // Send a new value, and see it reflected - err = stream.Send(codersdk.DynamicParametersRequest{ + preview, err = previews(codersdk.DynamicParametersRequest{ ID: 1, Inputs: map[string]string{"group": group.Name}, }) require.NoError(t, err) - preview = testutil.RequireReceive(ctx, t, previews) require.Equal(t, 1, preview.ID) require.Empty(t, preview.Diagnostics) require.Equal(t, "group", preview.Parameters[0].Name) @@ -103,12 +102,11 @@ func TestDynamicParametersOwnerGroups(t *testing.T) { require.Equal(t, group.Name, preview.Parameters[0].Value.Value) // Back to default - err = stream.Send(codersdk.DynamicParametersRequest{ + preview, err = previews(codersdk.DynamicParametersRequest{ ID: 3, Inputs: map[string]string{}, }) require.NoError(t, err) - preview = testutil.RequireReceive(ctx, t, previews) require.Equal(t, 3, preview.ID) require.Empty(t, preview.Diagnostics) require.Equal(t, "group", preview.Parameters[0].Name) From ea4aa7dc57c409c6966a15cffdd2f8c2813195d5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 18 Jun 2025 14:02:10 -0500 Subject: [PATCH 10/22] linting --- coderd/dynamicparameters/render.go | 13 +++++++------ coderd/dynamicparameters/static.go | 2 +- coderd/parameters.go | 5 +++-- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index 5a47214c22e7b..cf2f617df141d 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -31,7 +31,7 @@ type Renderer interface { Close() } -var ErrorTemplateVersionNotReady = xerrors.New("template version job not finished") +var ErrTemplateVersionNotReady = xerrors.New("template version job not finished") // Loader is used to load the necessary coder objects for rendering a template // version's parameters. The output is a Renderer, which is the object that uses @@ -91,7 +91,7 @@ func (r *Loader) Load(ctx context.Context, db database.Store) error { } if !r.job.CompletedAt.Valid { - return ErrorTemplateVersionNotReady + return ErrTemplateVersionNotReady } if r.terraformValues == nil { @@ -131,7 +131,9 @@ func (r *Loader) Renderer(ctx context.Context, db database.Store, cache *files.C // Renderer caches all the necessary files when rendering a template version's // parameters. It must be closed after use to release the cached files. func (r *Loader) dynamicRenderer(ctx context.Context, db database.Store, cache *files.Cache) (*DynamicRenderer, error) { - // If they can read the template version, then they can read the file. + // If they can read the template version, then they can read the file for + // parameter loading purposes. + //nolint:gocritic fileCtx := dbauthz.AsFileReader(ctx) templateFS, err := cache.Acquire(fileCtx, r.job.FileID) if err != nil { @@ -174,9 +176,8 @@ type DynamicRenderer struct { templateFS fs.FS plan json.RawMessage - ownerErrors map[uuid.UUID]error - currentOwner *previewtypes.WorkspaceOwner - currentOwnerID uuid.UUID + ownerErrors map[uuid.UUID]error + currentOwner *previewtypes.WorkspaceOwner once sync.Once close func() diff --git a/coderd/dynamicparameters/static.go b/coderd/dynamicparameters/static.go index c945f510718ea..7c6c3099ca8f4 100644 --- a/coderd/dynamicparameters/static.go +++ b/coderd/dynamicparameters/static.go @@ -131,4 +131,4 @@ func (r *StaticRender) Render(_ context.Context, _ uuid.UUID, values map[string] } } -func (r *StaticRender) Close() {} +func (*StaticRender) Close() {} diff --git a/coderd/parameters.go b/coderd/parameters.go index cf2ce2c7162d3..dcf66617f7e1a 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -68,6 +68,7 @@ func (api *API) templateVersionDynamicParametersWebsocket(rw http.ResponseWriter })(rw, r) } +//nolint:revive // listen is a control flag func (api *API) templateVersionDynamicParameters(listen bool, initial codersdk.DynamicParametersRequest) func(rw http.ResponseWriter, r *http.Request) { return func(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -78,16 +79,16 @@ func (api *API) templateVersionDynamicParameters(listen bool, initial codersdk.D err := loader.Load(ctx, api.Database) if err != nil { - if httpapi.Is404Error(err) { httpapi.ResourceNotFound(rw) return } - if xerrors.Is(err, dynamicparameters.ErrorTemplateVersionNotReady) { + if xerrors.Is(err, dynamicparameters.ErrTemplateVersionNotReady) { httpapi.Write(ctx, rw, http.StatusTooEarly, codersdk.Response{ Message: "Template version job has not finished", }) + return } httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ From 758cd2653685a878edbdf0f3a292f788ea5d2153 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 18 Jun 2025 14:08:45 -0500 Subject: [PATCH 11/22] refactor loader --- coderd/dynamicparameters/render.go | 13 +++++-------- coderd/parameters.go | 15 +++------------ 2 files changed, 8 insertions(+), 20 deletions(-) diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index cf2f617df141d..a912fef79c8ab 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -45,7 +45,7 @@ type Loader struct { terraformValues *database.TemplateVersionTerraformValue } -func New(versionID uuid.UUID) *Loader { +func Prepare(versionID uuid.UUID) *Loader { return &Loader{ templateVersionID: versionID, } @@ -73,7 +73,7 @@ func (r *Loader) WithTerraformValues(values database.TemplateVersionTerraformVal return r } -func (r *Loader) Load(ctx context.Context, db database.Store) error { +func (r *Loader) loadData(ctx context.Context, db database.Store) error { if r.templateVersion == nil { tv, err := db.GetTemplateVersionByID(ctx, r.templateVersionID) if err != nil { @@ -105,10 +105,6 @@ func (r *Loader) Load(ctx context.Context, db database.Store) error { return nil } -func (r *Loader) isReady() bool { - return r.templateVersion != nil && r.job != nil && r.terraformValues != nil -} - // Renderer returns a Renderer that can be used to render the template version's // parameters. It automatically determines whether to use a static or dynamic // renderer based on the template version's state. @@ -117,8 +113,9 @@ func (r *Loader) isReady() bool { // do not have the database state to support dynamic parameters. A constant // warning will be displayed for these template versions. func (r *Loader) Renderer(ctx context.Context, db database.Store, cache *files.Cache) (Renderer, error) { - if !r.isReady() { - return nil, xerrors.New("Load() must be called before Renderer()") + err := r.loadData(ctx, db) + if err != nil { + return nil, xerrors.Errorf("load data: %w", err) } if !ProvisionerVersionSupportsDynamicParameters(r.terraformValues.ProvisionerdVersion) { diff --git a/coderd/parameters.go b/coderd/parameters.go index dcf66617f7e1a..fc719044ede26 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -74,10 +74,10 @@ func (api *API) templateVersionDynamicParameters(listen bool, initial codersdk.D ctx := r.Context() templateVersion := httpmw.TemplateVersionParam(r) - loader := dynamicparameters.New(templateVersion.ID). - WithTemplateVersion(templateVersion) + renderer, err := dynamicparameters.Prepare(templateVersion.ID). + WithTemplateVersion(templateVersion). + Renderer(ctx, api.Database, api.FileCache) - err := loader.Load(ctx, api.Database) if err != nil { if httpapi.Is404Error(err) { httpapi.ResourceNotFound(rw) @@ -97,15 +97,6 @@ func (api *API) templateVersionDynamicParameters(listen bool, initial codersdk.D }) return } - - renderer, err := loader.Renderer(ctx, api.Database, api.FileCache) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error creating renderer for template version.", - Detail: err.Error(), - }) - return - } defer renderer.Close() if listen { From e8f1d2dbe14fe7035b5aa0b623079203f23aee5b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 18 Jun 2025 14:12:23 -0500 Subject: [PATCH 12/22] comments --- coderd/dynamicparameters/render.go | 16 ++++++++++------ coderd/dynamicparameters/static.go | 10 +++++----- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index a912fef79c8ab..683f97310d593 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -45,6 +45,10 @@ type Loader struct { terraformValues *database.TemplateVersionTerraformValue } +// Prepare is the entrypoint for this package. It is broken into 2 steps to allow +// prepopulating some of the existing data and saving some database queries. +// +// Usage: dynamicparameters.Prepare(...).Renderer(...) func Prepare(versionID uuid.UUID) *Loader { return &Loader{ templateVersionID: versionID, @@ -127,7 +131,7 @@ func (r *Loader) Renderer(ctx context.Context, db database.Store, cache *files.C // Renderer caches all the necessary files when rendering a template version's // parameters. It must be closed after use to release the cached files. -func (r *Loader) dynamicRenderer(ctx context.Context, db database.Store, cache *files.Cache) (*DynamicRenderer, error) { +func (r *Loader) dynamicRenderer(ctx context.Context, db database.Store, cache *files.Cache) (*dynamicRenderer, error) { // If they can read the template version, then they can read the file for // parameter loading purposes. //nolint:gocritic @@ -152,7 +156,7 @@ func (r *Loader) dynamicRenderer(ctx context.Context, db database.Store, cache * plan = r.terraformValues.CachedPlan } - return &DynamicRenderer{ + return &dynamicRenderer{ data: r, templateFS: templateFS, db: db, @@ -167,7 +171,7 @@ func (r *Loader) dynamicRenderer(ctx context.Context, db database.Store, cache * }, nil } -type DynamicRenderer struct { +type dynamicRenderer struct { db database.Store data *Loader templateFS fs.FS @@ -180,7 +184,7 @@ type DynamicRenderer struct { close func() } -func (r *DynamicRenderer) Render(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { +func (r *dynamicRenderer) Render(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { // Always start with the cached error, if we have one. ownerErr := r.ownerErrors[ownerID] if ownerErr == nil { @@ -210,7 +214,7 @@ func (r *DynamicRenderer) Render(ctx context.Context, ownerID uuid.UUID, values return preview.Preview(ctx, input, r.templateFS) } -func (r *DynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uuid.UUID) error { +func (r *dynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uuid.UUID) error { if r.currentOwner != nil && r.currentOwner.ID == ownerID.String() { return nil // already fetched } @@ -318,7 +322,7 @@ func (r *DynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uui return nil } -func (r *DynamicRenderer) Close() { +func (r *dynamicRenderer) Close() { r.once.Do(r.close) } diff --git a/coderd/dynamicparameters/static.go b/coderd/dynamicparameters/static.go index 7c6c3099ca8f4..5bebf1443a0b5 100644 --- a/coderd/dynamicparameters/static.go +++ b/coderd/dynamicparameters/static.go @@ -16,11 +16,11 @@ import ( "github.com/coder/terraform-provider-coder/v2/provider" ) -type StaticRender struct { +type staticRender struct { staticParams []previewtypes.Parameter } -func (r *Loader) staticRender(ctx context.Context, db database.Store) (*StaticRender, error) { +func (r *Loader) staticRender(ctx context.Context, db database.Store) (*staticRender, error) { dbTemplateVersionParameters, err := db.GetTemplateVersionParameters(ctx, r.templateVersionID) if err != nil { return nil, xerrors.Errorf("template version parameters: %w", err) @@ -101,12 +101,12 @@ func (r *Loader) staticRender(ctx context.Context, db database.Store) (*StaticRe params = append(params, param) } - return &StaticRender{ + return &staticRender{ staticParams: params, }, nil } -func (r *StaticRender) Render(_ context.Context, _ uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { +func (r *staticRender) Render(_ context.Context, _ uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) { params := r.staticParams for i := range params { param := ¶ms[i] @@ -131,4 +131,4 @@ func (r *StaticRender) Render(_ context.Context, _ uuid.UUID, values map[string] } } -func (*StaticRender) Close() {} +func (*staticRender) Close() {} From 23840d925f3a0f9da04d116f646a1129b87e1bf3 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 18 Jun 2025 14:14:39 -0500 Subject: [PATCH 13/22] linting --- coderd/parameters.go | 1 - 1 file changed, 1 deletion(-) diff --git a/coderd/parameters.go b/coderd/parameters.go index fc719044ede26..d49b69d27e736 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -77,7 +77,6 @@ func (api *API) templateVersionDynamicParameters(listen bool, initial codersdk.D renderer, err := dynamicparameters.Prepare(templateVersion.ID). WithTemplateVersion(templateVersion). Renderer(ctx, api.Database, api.FileCache) - if err != nil { if httpapi.Is404Error(err) { httpapi.ResourceNotFound(rw) From 6328899d18b9166f910af42864e547e84c8bfe02 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 18 Jun 2025 14:20:22 -0500 Subject: [PATCH 14/22] add idea --- coderd/dynamicparameters/render.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index 683f97310d593..518de6c7b4574 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -163,6 +163,9 @@ func (r *Loader) dynamicRenderer(ctx context.Context, db database.Store, cache * plan: plan, ownerErrors: make(map[uuid.UUID]error), close: func() { + // Up to 2 files are cached, and must be released when rendering is complete. + // TODO: Might be smart to always call release when the context is + // cancelled. cache.Release(r.job.FileID) if moduleFilesFS != nil { cache.Release(r.terraformValues.CachedModuleFiles.UUID) From 21b24d2f1ce90fe87707490a9c9b715dff9fb596 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 19 Jun 2025 19:40:31 -0500 Subject: [PATCH 15/22] spelling --- coderd/dynamicparameters/render.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index 69c1db6f32a62..4556e267363a4 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -166,7 +166,7 @@ func (r *Loader) dynamicRenderer(ctx context.Context, db database.Store, cache * close: func() { // Up to 2 files are cached, and must be released when rendering is complete. // TODO: Might be smart to always call release when the context is - // cancelled. + // canceled. templateFS.Close() if moduleFilesFS != nil { moduleFilesFS.Close() From 7a1622d711617eea55804322cf363466e85fcd70 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 20 Jun 2025 06:49:17 -0500 Subject: [PATCH 16/22] add comment, remove unused field --- coderd/dynamicparameters/render.go | 13 +++++-------- coderd/parameters.go | 5 +++++ 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index 4556e267363a4..78454f5d3c3de 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -2,8 +2,8 @@ package dynamicparameters import ( "context" - "encoding/json" "io/fs" + "log/slog" "sync" "github.com/google/uuid" @@ -152,16 +152,10 @@ func (r *Loader) dynamicRenderer(ctx context.Context, db database.Store, cache * terraformFS = files.NewOverlayFS(templateFS, []files.Overlay{{Path: ".terraform/modules", FS: moduleFilesFS}}) } - plan := json.RawMessage("{}") - if len(r.terraformValues.CachedPlan) > 0 { - plan = r.terraformValues.CachedPlan - } - return &dynamicRenderer{ data: r, templateFS: terraformFS, db: db, - plan: plan, ownerErrors: make(map[uuid.UUID]error), close: func() { // Up to 2 files are cached, and must be released when rendering is complete. @@ -179,7 +173,6 @@ type dynamicRenderer struct { db database.Store data *Loader templateFS fs.FS - plan json.RawMessage ownerErrors map[uuid.UUID]error currentOwner *previewtypes.WorkspaceOwner @@ -213,6 +206,10 @@ func (r *dynamicRenderer) Render(ctx context.Context, ownerID uuid.UUID, values PlanJSON: r.data.terraformValues.CachedPlan, ParameterValues: values, Owner: *r.currentOwner, + // Do not emit parser logs to coderd output logs. + // TODO: Returning this logs in the output would benefit the caller. + // Unsure how large the logs can be, so for now we just discard them. + Logger: slog.New(slog.DiscardHandler), } return preview.Preview(ctx, input, r.templateFS) diff --git a/coderd/parameters.go b/coderd/parameters.go index d49b69d27e736..76045310f32bb 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -68,6 +68,11 @@ func (api *API) templateVersionDynamicParametersWebsocket(rw http.ResponseWriter })(rw, r) } +// The `listen` control flag determines whether to open a websocket connection to +// handle the request or not. This same function is used to 'evaluate' a template +// as a single invocation, or to 'listen' for a back and forth interaction with +// the user to update the form as they type. +// //nolint:revive // listen is a control flag func (api *API) templateVersionDynamicParameters(listen bool, initial codersdk.DynamicParametersRequest) func(rw http.ResponseWriter, r *http.Request) { return func(rw http.ResponseWriter, r *http.Request) { From db612b3a426fb5d9b8d48f04123bfeef77c36e49 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 20 Jun 2025 06:59:41 -0500 Subject: [PATCH 17/22] chore: reprot json error as param diagnostic --- coderd/dynamicparameters/static.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/coderd/dynamicparameters/static.go b/coderd/dynamicparameters/static.go index 5bebf1443a0b5..eadb304e852e1 100644 --- a/coderd/dynamicparameters/static.go +++ b/coderd/dynamicparameters/static.go @@ -48,7 +48,7 @@ func (r *Loader) staticRender(ctx context.Context, db database.Store) (*staticRe }, // Always use the default, since we used to assume the empty string Value: previewtypes.StringLiteral(it.DefaultValue), - Diagnostics: nil, + Diagnostics: make(previewtypes.Diagnostics, 0), } if it.ValidationError != "" || it.ValidationRegex != "" || it.ValidationMonotonic != "" { @@ -82,7 +82,15 @@ func (r *Loader) staticRender(ctx context.Context, db database.Store) (*staticRe } var protoOptions []*sdkproto.RichParameterOption - _ = json.Unmarshal(it.Options, &protoOptions) // Not going to make this fatal + err := json.Unmarshal(it.Options, &protoOptions) + if err != nil { + param.Diagnostics = append(param.Diagnostics, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Failed to parse json parameter options", + Detail: err.Error(), + }) + } + for _, opt := range protoOptions { param.Options = append(param.Options, &previewtypes.ParameterOption{ Name: opt.Name, @@ -97,7 +105,7 @@ func (r *Loader) staticRender(ctx context.Context, db database.Store) (*staticRe // for a given set of conditions. _, param.FormType, _ = provider.ValidateFormType(provider.OptionType(param.Type), len(param.Options), param.FormType) - param.Diagnostics = previewtypes.Diagnostics(param.Valid(param.Value)) + param.Diagnostics = append(param.Diagnostics, previewtypes.Diagnostics(param.Valid(param.Value))...) params = append(params, param) } From 000f72247a9c6377fd90e71c7eddfb783ac2e7e9 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 20 Jun 2025 08:19:28 -0500 Subject: [PATCH 18/22] minor fixes --- coderd/dynamicparameters/static.go | 2 +- coderd/util/slice/slice.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/dynamicparameters/static.go b/coderd/dynamicparameters/static.go index eadb304e852e1..bd23a9a497344 100644 --- a/coderd/dynamicparameters/static.go +++ b/coderd/dynamicparameters/static.go @@ -64,7 +64,7 @@ func (r *Loader) staticRender(ctx context.Context, db database.Store) (*staticRe var vMax *int64 if it.ValidationMax.Valid { - vMin = ptr.Ref(int64(it.ValidationMax.Int32)) + vMax = ptr.Ref(int64(it.ValidationMax.Int32)) } var monotonic *string diff --git a/coderd/util/slice/slice.go b/coderd/util/slice/slice.go index 67d4a939713c0..2a510e24d2b53 100644 --- a/coderd/util/slice/slice.go +++ b/coderd/util/slice/slice.go @@ -221,7 +221,7 @@ func CountConsecutive[T comparable](needle T, haystack ...T) int { // Convert converts a slice of type F to a slice of type T using the provided function f. func Convert[F any, T any](a []F, f func(F) T) []T { if a == nil { - return nil + return []T{} } tmp := make([]T, 0, len(a)) From 7b8013880725a91e5140243187e098a21b35a505 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 20 Jun 2025 08:48:59 -0500 Subject: [PATCH 19/22] only use 1 function, not a 2 step --- coderd/dynamicparameters/render.go | 55 ++++++++++++++++-------------- coderd/dynamicparameters/static.go | 2 +- coderd/parameters.go | 6 ++-- 3 files changed, 34 insertions(+), 29 deletions(-) diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index 78454f5d3c3de..9c4c73f87e5bc 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -33,10 +33,10 @@ type Renderer interface { var ErrTemplateVersionNotReady = xerrors.New("template version job not finished") -// Loader is used to load the necessary coder objects for rendering a template +// loader is used to load the necessary coder objects for rendering a template // version's parameters. The output is a Renderer, which is the object that uses // the cached objects to render the template version's parameters. -type Loader struct { +type loader struct { templateVersionID uuid.UUID // cache of objects @@ -45,39 +45,44 @@ type Loader struct { terraformValues *database.TemplateVersionTerraformValue } -// Prepare is the entrypoint for this package. It is broken into 2 steps to allow -// prepopulating some of the existing data and saving some database queries. -// -// Usage: dynamicparameters.Prepare(...).Renderer(...) -func Prepare(versionID uuid.UUID) *Loader { - return &Loader{ +// Prepare is the entrypoint for this package. It loads the necessary objects & +// files from the database and returns a Renderer that can be used to render the +// template version's parameters. +func Prepare(ctx context.Context, db database.Store, cache *files.Cache, versionID uuid.UUID, options ...func(r *loader)) (Renderer, error) { + l := &loader{ templateVersionID: versionID, } -} -func (r *Loader) WithTemplateVersion(tv database.TemplateVersion) *Loader { - if tv.ID == r.templateVersionID { - r.templateVersion = &tv + for _, opt := range options { + opt(l) } - return r + return l.Renderer(ctx, db, cache) } -func (r *Loader) WithProvisionerJob(job database.ProvisionerJob) *Loader { - r.job = &job - - return r +func WithTemplateVersion(tv database.TemplateVersion) func(r *loader) { + return func(r *loader) { + if tv.ID == r.templateVersionID { + r.templateVersion = &tv + } + } } -func (r *Loader) WithTerraformValues(values database.TemplateVersionTerraformValue) *Loader { - if values.TemplateVersionID == r.templateVersionID { - r.terraformValues = &values +func WithProvisionerJob(job database.ProvisionerJob) func(r *loader) { + return func(r *loader) { + r.job = &job } +} - return r +func WithTerraformValues(values database.TemplateVersionTerraformValue) func(r *loader) { + return func(r *loader) { + if values.TemplateVersionID == r.templateVersionID { + r.terraformValues = &values + } + } } -func (r *Loader) loadData(ctx context.Context, db database.Store) error { +func (r *loader) loadData(ctx context.Context, db database.Store) error { if r.templateVersion == nil { tv, err := db.GetTemplateVersionByID(ctx, r.templateVersionID) if err != nil { @@ -116,7 +121,7 @@ func (r *Loader) loadData(ctx context.Context, db database.Store) error { // Static parameter rendering is required to support older template versions that // do not have the database state to support dynamic parameters. A constant // warning will be displayed for these template versions. -func (r *Loader) Renderer(ctx context.Context, db database.Store, cache *files.Cache) (Renderer, error) { +func (r *loader) Renderer(ctx context.Context, db database.Store, cache *files.Cache) (Renderer, error) { err := r.loadData(ctx, db) if err != nil { return nil, xerrors.Errorf("load data: %w", err) @@ -131,7 +136,7 @@ func (r *Loader) Renderer(ctx context.Context, db database.Store, cache *files.C // Renderer caches all the necessary files when rendering a template version's // parameters. It must be closed after use to release the cached files. -func (r *Loader) dynamicRenderer(ctx context.Context, db database.Store, cache *files.Cache) (*dynamicRenderer, error) { +func (r *loader) dynamicRenderer(ctx context.Context, db database.Store, cache *files.Cache) (*dynamicRenderer, error) { // If they can read the template version, then they can read the file for // parameter loading purposes. //nolint:gocritic @@ -171,7 +176,7 @@ func (r *Loader) dynamicRenderer(ctx context.Context, db database.Store, cache * type dynamicRenderer struct { db database.Store - data *Loader + data *loader templateFS fs.FS ownerErrors map[uuid.UUID]error diff --git a/coderd/dynamicparameters/static.go b/coderd/dynamicparameters/static.go index bd23a9a497344..64ecbc3af29c1 100644 --- a/coderd/dynamicparameters/static.go +++ b/coderd/dynamicparameters/static.go @@ -20,7 +20,7 @@ type staticRender struct { staticParams []previewtypes.Parameter } -func (r *Loader) staticRender(ctx context.Context, db database.Store) (*staticRender, error) { +func (r *loader) staticRender(ctx context.Context, db database.Store) (*staticRender, error) { dbTemplateVersionParameters, err := db.GetTemplateVersionParameters(ctx, r.templateVersionID) if err != nil { return nil, xerrors.Errorf("template version parameters: %w", err) diff --git a/coderd/parameters.go b/coderd/parameters.go index 76045310f32bb..4b8b13486934f 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -79,9 +79,9 @@ func (api *API) templateVersionDynamicParameters(listen bool, initial codersdk.D ctx := r.Context() templateVersion := httpmw.TemplateVersionParam(r) - renderer, err := dynamicparameters.Prepare(templateVersion.ID). - WithTemplateVersion(templateVersion). - Renderer(ctx, api.Database, api.FileCache) + renderer, err := dynamicparameters.Prepare(ctx, api.Database, api.FileCache, templateVersion.ID, + dynamicparameters.WithTemplateVersion(templateVersion), + ) if err != nil { if httpapi.Is404Error(err) { httpapi.ResourceNotFound(rw) From c1902a77528ec1d6074e066a123c1fa62940b4ea Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 20 Jun 2025 08:49:51 -0500 Subject: [PATCH 20/22] Revert "only use 1 function, not a 2 step" This reverts commit 7b8013880725a91e5140243187e098a21b35a505. --- coderd/dynamicparameters/render.go | 55 ++++++++++++++---------------- coderd/dynamicparameters/static.go | 2 +- coderd/parameters.go | 6 ++-- 3 files changed, 29 insertions(+), 34 deletions(-) diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index 9c4c73f87e5bc..78454f5d3c3de 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -33,10 +33,10 @@ type Renderer interface { var ErrTemplateVersionNotReady = xerrors.New("template version job not finished") -// loader is used to load the necessary coder objects for rendering a template +// Loader is used to load the necessary coder objects for rendering a template // version's parameters. The output is a Renderer, which is the object that uses // the cached objects to render the template version's parameters. -type loader struct { +type Loader struct { templateVersionID uuid.UUID // cache of objects @@ -45,44 +45,39 @@ type loader struct { terraformValues *database.TemplateVersionTerraformValue } -// Prepare is the entrypoint for this package. It loads the necessary objects & -// files from the database and returns a Renderer that can be used to render the -// template version's parameters. -func Prepare(ctx context.Context, db database.Store, cache *files.Cache, versionID uuid.UUID, options ...func(r *loader)) (Renderer, error) { - l := &loader{ +// Prepare is the entrypoint for this package. It is broken into 2 steps to allow +// prepopulating some of the existing data and saving some database queries. +// +// Usage: dynamicparameters.Prepare(...).Renderer(...) +func Prepare(versionID uuid.UUID) *Loader { + return &Loader{ templateVersionID: versionID, } +} - for _, opt := range options { - opt(l) +func (r *Loader) WithTemplateVersion(tv database.TemplateVersion) *Loader { + if tv.ID == r.templateVersionID { + r.templateVersion = &tv } - return l.Renderer(ctx, db, cache) + return r } -func WithTemplateVersion(tv database.TemplateVersion) func(r *loader) { - return func(r *loader) { - if tv.ID == r.templateVersionID { - r.templateVersion = &tv - } - } -} +func (r *Loader) WithProvisionerJob(job database.ProvisionerJob) *Loader { + r.job = &job -func WithProvisionerJob(job database.ProvisionerJob) func(r *loader) { - return func(r *loader) { - r.job = &job - } + return r } -func WithTerraformValues(values database.TemplateVersionTerraformValue) func(r *loader) { - return func(r *loader) { - if values.TemplateVersionID == r.templateVersionID { - r.terraformValues = &values - } +func (r *Loader) WithTerraformValues(values database.TemplateVersionTerraformValue) *Loader { + if values.TemplateVersionID == r.templateVersionID { + r.terraformValues = &values } + + return r } -func (r *loader) loadData(ctx context.Context, db database.Store) error { +func (r *Loader) loadData(ctx context.Context, db database.Store) error { if r.templateVersion == nil { tv, err := db.GetTemplateVersionByID(ctx, r.templateVersionID) if err != nil { @@ -121,7 +116,7 @@ func (r *loader) loadData(ctx context.Context, db database.Store) error { // Static parameter rendering is required to support older template versions that // do not have the database state to support dynamic parameters. A constant // warning will be displayed for these template versions. -func (r *loader) Renderer(ctx context.Context, db database.Store, cache *files.Cache) (Renderer, error) { +func (r *Loader) Renderer(ctx context.Context, db database.Store, cache *files.Cache) (Renderer, error) { err := r.loadData(ctx, db) if err != nil { return nil, xerrors.Errorf("load data: %w", err) @@ -136,7 +131,7 @@ func (r *loader) Renderer(ctx context.Context, db database.Store, cache *files.C // Renderer caches all the necessary files when rendering a template version's // parameters. It must be closed after use to release the cached files. -func (r *loader) dynamicRenderer(ctx context.Context, db database.Store, cache *files.Cache) (*dynamicRenderer, error) { +func (r *Loader) dynamicRenderer(ctx context.Context, db database.Store, cache *files.Cache) (*dynamicRenderer, error) { // If they can read the template version, then they can read the file for // parameter loading purposes. //nolint:gocritic @@ -176,7 +171,7 @@ func (r *loader) dynamicRenderer(ctx context.Context, db database.Store, cache * type dynamicRenderer struct { db database.Store - data *loader + data *Loader templateFS fs.FS ownerErrors map[uuid.UUID]error diff --git a/coderd/dynamicparameters/static.go b/coderd/dynamicparameters/static.go index 64ecbc3af29c1..bd23a9a497344 100644 --- a/coderd/dynamicparameters/static.go +++ b/coderd/dynamicparameters/static.go @@ -20,7 +20,7 @@ type staticRender struct { staticParams []previewtypes.Parameter } -func (r *loader) staticRender(ctx context.Context, db database.Store) (*staticRender, error) { +func (r *Loader) staticRender(ctx context.Context, db database.Store) (*staticRender, error) { dbTemplateVersionParameters, err := db.GetTemplateVersionParameters(ctx, r.templateVersionID) if err != nil { return nil, xerrors.Errorf("template version parameters: %w", err) diff --git a/coderd/parameters.go b/coderd/parameters.go index 4b8b13486934f..76045310f32bb 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -79,9 +79,9 @@ func (api *API) templateVersionDynamicParameters(listen bool, initial codersdk.D ctx := r.Context() templateVersion := httpmw.TemplateVersionParam(r) - renderer, err := dynamicparameters.Prepare(ctx, api.Database, api.FileCache, templateVersion.ID, - dynamicparameters.WithTemplateVersion(templateVersion), - ) + renderer, err := dynamicparameters.Prepare(templateVersion.ID). + WithTemplateVersion(templateVersion). + Renderer(ctx, api.Database, api.FileCache) if err != nil { if httpapi.Is404Error(err) { httpapi.ResourceNotFound(rw) From d2634e21a1dff15c2c49e1c0c0581b3ec9f6d2c6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 20 Jun 2025 08:48:59 -0500 Subject: [PATCH 21/22] only use 1 function, not a 2 step --- coderd/dynamicparameters/render.go | 55 ++++++++++++++++-------------- coderd/dynamicparameters/static.go | 2 +- coderd/parameters.go | 6 ++-- 3 files changed, 34 insertions(+), 29 deletions(-) diff --git a/coderd/dynamicparameters/render.go b/coderd/dynamicparameters/render.go index 78454f5d3c3de..9c4c73f87e5bc 100644 --- a/coderd/dynamicparameters/render.go +++ b/coderd/dynamicparameters/render.go @@ -33,10 +33,10 @@ type Renderer interface { var ErrTemplateVersionNotReady = xerrors.New("template version job not finished") -// Loader is used to load the necessary coder objects for rendering a template +// loader is used to load the necessary coder objects for rendering a template // version's parameters. The output is a Renderer, which is the object that uses // the cached objects to render the template version's parameters. -type Loader struct { +type loader struct { templateVersionID uuid.UUID // cache of objects @@ -45,39 +45,44 @@ type Loader struct { terraformValues *database.TemplateVersionTerraformValue } -// Prepare is the entrypoint for this package. It is broken into 2 steps to allow -// prepopulating some of the existing data and saving some database queries. -// -// Usage: dynamicparameters.Prepare(...).Renderer(...) -func Prepare(versionID uuid.UUID) *Loader { - return &Loader{ +// Prepare is the entrypoint for this package. It loads the necessary objects & +// files from the database and returns a Renderer that can be used to render the +// template version's parameters. +func Prepare(ctx context.Context, db database.Store, cache *files.Cache, versionID uuid.UUID, options ...func(r *loader)) (Renderer, error) { + l := &loader{ templateVersionID: versionID, } -} -func (r *Loader) WithTemplateVersion(tv database.TemplateVersion) *Loader { - if tv.ID == r.templateVersionID { - r.templateVersion = &tv + for _, opt := range options { + opt(l) } - return r + return l.Renderer(ctx, db, cache) } -func (r *Loader) WithProvisionerJob(job database.ProvisionerJob) *Loader { - r.job = &job - - return r +func WithTemplateVersion(tv database.TemplateVersion) func(r *loader) { + return func(r *loader) { + if tv.ID == r.templateVersionID { + r.templateVersion = &tv + } + } } -func (r *Loader) WithTerraformValues(values database.TemplateVersionTerraformValue) *Loader { - if values.TemplateVersionID == r.templateVersionID { - r.terraformValues = &values +func WithProvisionerJob(job database.ProvisionerJob) func(r *loader) { + return func(r *loader) { + r.job = &job } +} - return r +func WithTerraformValues(values database.TemplateVersionTerraformValue) func(r *loader) { + return func(r *loader) { + if values.TemplateVersionID == r.templateVersionID { + r.terraformValues = &values + } + } } -func (r *Loader) loadData(ctx context.Context, db database.Store) error { +func (r *loader) loadData(ctx context.Context, db database.Store) error { if r.templateVersion == nil { tv, err := db.GetTemplateVersionByID(ctx, r.templateVersionID) if err != nil { @@ -116,7 +121,7 @@ func (r *Loader) loadData(ctx context.Context, db database.Store) error { // Static parameter rendering is required to support older template versions that // do not have the database state to support dynamic parameters. A constant // warning will be displayed for these template versions. -func (r *Loader) Renderer(ctx context.Context, db database.Store, cache *files.Cache) (Renderer, error) { +func (r *loader) Renderer(ctx context.Context, db database.Store, cache *files.Cache) (Renderer, error) { err := r.loadData(ctx, db) if err != nil { return nil, xerrors.Errorf("load data: %w", err) @@ -131,7 +136,7 @@ func (r *Loader) Renderer(ctx context.Context, db database.Store, cache *files.C // Renderer caches all the necessary files when rendering a template version's // parameters. It must be closed after use to release the cached files. -func (r *Loader) dynamicRenderer(ctx context.Context, db database.Store, cache *files.Cache) (*dynamicRenderer, error) { +func (r *loader) dynamicRenderer(ctx context.Context, db database.Store, cache *files.Cache) (*dynamicRenderer, error) { // If they can read the template version, then they can read the file for // parameter loading purposes. //nolint:gocritic @@ -171,7 +176,7 @@ func (r *Loader) dynamicRenderer(ctx context.Context, db database.Store, cache * type dynamicRenderer struct { db database.Store - data *Loader + data *loader templateFS fs.FS ownerErrors map[uuid.UUID]error diff --git a/coderd/dynamicparameters/static.go b/coderd/dynamicparameters/static.go index bd23a9a497344..64ecbc3af29c1 100644 --- a/coderd/dynamicparameters/static.go +++ b/coderd/dynamicparameters/static.go @@ -20,7 +20,7 @@ type staticRender struct { staticParams []previewtypes.Parameter } -func (r *Loader) staticRender(ctx context.Context, db database.Store) (*staticRender, error) { +func (r *loader) staticRender(ctx context.Context, db database.Store) (*staticRender, error) { dbTemplateVersionParameters, err := db.GetTemplateVersionParameters(ctx, r.templateVersionID) if err != nil { return nil, xerrors.Errorf("template version parameters: %w", err) diff --git a/coderd/parameters.go b/coderd/parameters.go index 76045310f32bb..4b8b13486934f 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -79,9 +79,9 @@ func (api *API) templateVersionDynamicParameters(listen bool, initial codersdk.D ctx := r.Context() templateVersion := httpmw.TemplateVersionParam(r) - renderer, err := dynamicparameters.Prepare(templateVersion.ID). - WithTemplateVersion(templateVersion). - Renderer(ctx, api.Database, api.FileCache) + renderer, err := dynamicparameters.Prepare(ctx, api.Database, api.FileCache, templateVersion.ID, + dynamicparameters.WithTemplateVersion(templateVersion), + ) if err != nil { if httpapi.Is404Error(err) { httpapi.ResourceNotFound(rw) From 21ce54b4a06679adfbe99a437705ab1e65721af5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 20 Jun 2025 10:29:38 -0500 Subject: [PATCH 22/22] refactor TemplateVersionParameter to be a converter func --- coderd/dynamicparameters/static.go | 167 +++++++++++++++-------------- 1 file changed, 84 insertions(+), 83 deletions(-) diff --git a/coderd/dynamicparameters/static.go b/coderd/dynamicparameters/static.go index 64ecbc3af29c1..14988a2d162c0 100644 --- a/coderd/dynamicparameters/static.go +++ b/coderd/dynamicparameters/static.go @@ -9,6 +9,7 @@ import ( "golang.org/x/xerrors" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/db2sdk" "github.com/coder/coder/v2/coderd/util/ptr" sdkproto "github.com/coder/coder/v2/provisionersdk/proto" "github.com/coder/preview" @@ -26,89 +27,7 @@ func (r *loader) staticRender(ctx context.Context, db database.Store) (*staticRe return nil, xerrors.Errorf("template version parameters: %w", err) } - params := make([]previewtypes.Parameter, 0, len(dbTemplateVersionParameters)) - for _, it := range dbTemplateVersionParameters { - param := previewtypes.Parameter{ - ParameterData: previewtypes.ParameterData{ - Name: it.Name, - DisplayName: it.DisplayName, - Description: it.Description, - Type: previewtypes.ParameterType(it.Type), - FormType: provider.ParameterFormType(it.FormType), - Styling: previewtypes.ParameterStyling{}, - Mutable: it.Mutable, - DefaultValue: previewtypes.StringLiteral(it.DefaultValue), - Icon: it.Icon, - Options: make([]*previewtypes.ParameterOption, 0), - Validations: make([]*previewtypes.ParameterValidation, 0), - Required: it.Required, - Order: int64(it.DisplayOrder), - Ephemeral: it.Ephemeral, - Source: nil, - }, - // Always use the default, since we used to assume the empty string - Value: previewtypes.StringLiteral(it.DefaultValue), - Diagnostics: make(previewtypes.Diagnostics, 0), - } - - if it.ValidationError != "" || it.ValidationRegex != "" || it.ValidationMonotonic != "" { - var reg *string - if it.ValidationRegex != "" { - reg = ptr.Ref(it.ValidationRegex) - } - - var vMin *int64 - if it.ValidationMin.Valid { - vMin = ptr.Ref(int64(it.ValidationMin.Int32)) - } - - var vMax *int64 - if it.ValidationMax.Valid { - vMax = ptr.Ref(int64(it.ValidationMax.Int32)) - } - - var monotonic *string - if it.ValidationMonotonic != "" { - monotonic = ptr.Ref(it.ValidationMonotonic) - } - - param.Validations = append(param.Validations, &previewtypes.ParameterValidation{ - Error: it.ValidationError, - Regex: reg, - Min: vMin, - Max: vMax, - Monotonic: monotonic, - }) - } - - var protoOptions []*sdkproto.RichParameterOption - err := json.Unmarshal(it.Options, &protoOptions) - if err != nil { - param.Diagnostics = append(param.Diagnostics, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Failed to parse json parameter options", - Detail: err.Error(), - }) - } - - for _, opt := range protoOptions { - param.Options = append(param.Options, &previewtypes.ParameterOption{ - Name: opt.Name, - Description: opt.Description, - Value: previewtypes.StringLiteral(opt.Value), - Icon: opt.Icon, - }) - } - - // Take the form type from the ValidateFormType function. This is a bit - // unfortunate we have to do this, but it will return the default form_type - // for a given set of conditions. - _, param.FormType, _ = provider.ValidateFormType(provider.OptionType(param.Type), len(param.Options), param.FormType) - - param.Diagnostics = append(param.Diagnostics, previewtypes.Diagnostics(param.Valid(param.Value))...) - params = append(params, param) - } - + params := db2sdk.List(dbTemplateVersionParameters, TemplateVersionParameter) return &staticRender{ staticParams: params, }, nil @@ -140,3 +59,85 @@ func (r *staticRender) Render(_ context.Context, _ uuid.UUID, values map[string] } func (*staticRender) Close() {} + +func TemplateVersionParameter(it database.TemplateVersionParameter) previewtypes.Parameter { + param := previewtypes.Parameter{ + ParameterData: previewtypes.ParameterData{ + Name: it.Name, + DisplayName: it.DisplayName, + Description: it.Description, + Type: previewtypes.ParameterType(it.Type), + FormType: provider.ParameterFormType(it.FormType), + Styling: previewtypes.ParameterStyling{}, + Mutable: it.Mutable, + DefaultValue: previewtypes.StringLiteral(it.DefaultValue), + Icon: it.Icon, + Options: make([]*previewtypes.ParameterOption, 0), + Validations: make([]*previewtypes.ParameterValidation, 0), + Required: it.Required, + Order: int64(it.DisplayOrder), + Ephemeral: it.Ephemeral, + Source: nil, + }, + // Always use the default, since we used to assume the empty string + Value: previewtypes.StringLiteral(it.DefaultValue), + Diagnostics: make(previewtypes.Diagnostics, 0), + } + + if it.ValidationError != "" || it.ValidationRegex != "" || it.ValidationMonotonic != "" { + var reg *string + if it.ValidationRegex != "" { + reg = ptr.Ref(it.ValidationRegex) + } + + var vMin *int64 + if it.ValidationMin.Valid { + vMin = ptr.Ref(int64(it.ValidationMin.Int32)) + } + + var vMax *int64 + if it.ValidationMax.Valid { + vMax = ptr.Ref(int64(it.ValidationMax.Int32)) + } + + var monotonic *string + if it.ValidationMonotonic != "" { + monotonic = ptr.Ref(it.ValidationMonotonic) + } + + param.Validations = append(param.Validations, &previewtypes.ParameterValidation{ + Error: it.ValidationError, + Regex: reg, + Min: vMin, + Max: vMax, + Monotonic: monotonic, + }) + } + + var protoOptions []*sdkproto.RichParameterOption + err := json.Unmarshal(it.Options, &protoOptions) + if err != nil { + param.Diagnostics = append(param.Diagnostics, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Failed to parse json parameter options", + Detail: err.Error(), + }) + } + + for _, opt := range protoOptions { + param.Options = append(param.Options, &previewtypes.ParameterOption{ + Name: opt.Name, + Description: opt.Description, + Value: previewtypes.StringLiteral(opt.Value), + Icon: opt.Icon, + }) + } + + // Take the form type from the ValidateFormType function. This is a bit + // unfortunate we have to do this, but it will return the default form_type + // for a given set of conditions. + _, param.FormType, _ = provider.ValidateFormType(provider.OptionType(param.Type), len(param.Options), param.FormType) + + param.Diagnostics = append(param.Diagnostics, previewtypes.Diagnostics(param.Valid(param.Value))...) + return param +}