From 34de4f05dea409e3825f6cf50576f509a2a725c2 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 14 May 2025 11:54:45 -0500 Subject: [PATCH 01/22] chore: static params as dynmaic --- coderd/parameters.go | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/coderd/parameters.go b/coderd/parameters.go index 6b6f4db531533..63bc50c23e4ea 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" "encoding/json" + "io/fs" "net/http" "time" @@ -37,6 +38,8 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http user := httpmw.UserParam(r) templateVersion := httpmw.TemplateVersionParam(r) + dynamicPreview := preview.Preview + // Check that the job has completed successfully job, err := api.Database.GetProvisionerJobByID(ctx, templateVersion.JobID) if httpapi.Is404Error(err) { @@ -190,6 +193,45 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http } } +func staticPreview(ctx context.Context, db database.Store, version uuid.UUID) func(ctx context.Context, input preview.Input, fs fs.FS) preview.Output { + dbTemplateVersionParameters, err := db.GetTemplateVersionParameters(ctx, version) + if err != nil { + return nil + } + + params := make([]previewtypes.Parameter, 0, len(dbTemplateVersionParameters)) + for _, it := range dbTemplateVersionParameters { + params = append(params, 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: nil, + Validations: nil, + Required: false, + Order: 0, + Ephemeral: false, + Source: nil, + }, + Value: previewtypes.NullString(), + Diagnostics: nil, + }) + } + + return func(_ context.Context, in preview.Input, _ fs.FS) preview.Output { + + return preview.Output{ + Parameters: nil, + } + } +} + func (api *API) getWorkspaceOwnerData( ctx context.Context, user database.User, From 005a08480b535b15b30473841cff48be605398bc Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 14 May 2025 16:22:58 -0500 Subject: [PATCH 02/22] chore: refactor preview code to be swappable --- coderd/coderd.go | 2 +- coderd/files/cache.go | 27 +++-- coderd/parameters.go | 182 ++++++++++++++++------------- coderd/parameters_internal_test.go | 95 +++++++++++++++ 4 files changed, 220 insertions(+), 86 deletions(-) create mode 100644 coderd/parameters_internal_test.go diff --git a/coderd/coderd.go b/coderd/coderd.go index 98ae7a8ede413..1158e55a8cca9 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -1595,7 +1595,7 @@ type API struct { // passed to dbauthz. AccessControlStore *atomic.Pointer[dbauthz.AccessControlStore] PortSharer atomic.Pointer[portsharing.PortSharer] - FileCache files.Cache + FileCache *files.Cache PrebuildsClaimer atomic.Pointer[prebuilds.Claimer] PrebuildsReconciler atomic.Pointer[prebuilds.ReconciliationOrchestrator] diff --git a/coderd/files/cache.go b/coderd/files/cache.go index ccdf9abff2ebb..28df15b8fe690 100644 --- a/coderd/files/cache.go +++ b/coderd/files/cache.go @@ -14,10 +14,18 @@ import ( "github.com/coder/coder/v2/coderd/util/lazy" ) +func New(fetch fetcher) *Cache { + return &Cache{ + lock: sync.Mutex{}, + data: make(map[uuid.UUID]*cacheEntry), + fetcher: fetch, + } +} + // NewFromStore returns a file cache that will fetch files from the provided // database. -func NewFromStore(store database.Store) Cache { - fetcher := func(ctx context.Context, fileID uuid.UUID) (fs.FS, error) { +func NewFromStore(store database.Store) *Cache { + fetch := func(ctx context.Context, fileID uuid.UUID) (fs.FS, error) { file, err := store.GetFileByID(ctx, fileID) if err != nil { return nil, xerrors.Errorf("failed to read file from database: %w", err) @@ -27,11 +35,7 @@ func NewFromStore(store database.Store) Cache { return archivefs.FromTarReader(content), nil } - return Cache{ - lock: sync.Mutex{}, - data: make(map[uuid.UUID]*cacheEntry), - fetcher: fetcher, - } + return New(fetch) } // Cache persists the files for template versions, and is used by dynamic @@ -112,3 +116,12 @@ func (c *Cache) Release(fileID uuid.UUID) { delete(c.data, fileID) } + +// Count returns the number of files currently in the cache. +// Mainly used for unit testing assertions. +func (c *Cache) Count() int { + c.lock.Lock() + defer c.lock.Unlock() + + return len(c.data) +} diff --git a/coderd/parameters.go b/coderd/parameters.go index 63bc50c23e4ea..716addbeed4a7 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -9,6 +9,7 @@ import ( "time" "github.com/google/uuid" + "github.com/hashicorp/hcl/v2" "golang.org/x/sync/errgroup" "golang.org/x/xerrors" @@ -38,8 +39,6 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http user := httpmw.UserParam(r) templateVersion := httpmw.TemplateVersionParam(r) - dynamicPreview := preview.Preview - // Check that the job has completed successfully job, err := api.Database.GetProvisionerJobByID(ctx, templateVersion.JobID) if httpapi.Is404Error(err) { @@ -60,77 +59,11 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http return } - // nolint:gocritic // We need to fetch the templates files for the Terraform - // evaluator, and the user likely does not have permission. - fileCtx := dbauthz.AsProvisionerd(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 - } - - 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(), - }) + render, closer, success := prepareDynamicPreview(ctx, rw, api.Database, api.FileCache, templateVersion, user) + if !success { 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("{}") - tf, err := api.Database.GetTemplateVersionTerraformValues(ctx, templateVersion.ID) - if err == nil { - plan = tf.CachedPlan - - if tf.CachedModuleFiles.Valid { - moduleFilesFS, err := api.FileCache.Acquire(fileCtx, tf.CachedModuleFiles.UUID) - if err != nil { - httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ - Message: "Internal error fetching Terraform modules.", - Detail: err.Error(), - }) - return - } - defer api.FileCache.Release(tf.CachedModuleFiles.UUID) - templateFS, err = files.NewOverlayFS(templateFS, []files.Overlay{{Path: ".terraform/modules", FS: moduleFilesFS}}) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error creating overlay filesystem.", - Detail: err.Error(), - }) - return - } - } - } else if !xerrors.Is(err, sql.ErrNoRows) { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Failed to retrieve Terraform values for template version", - Detail: err.Error(), - }) - return - } - - owner, err := api.getWorkspaceOwnerData(ctx, user, 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, - } + defer closer() conn, err := websocket.Accept(rw, r, nil) if err != nil { @@ -148,7 +81,7 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http ) // Send an initial form state, computed without any user input. - result, diagnostics := preview.Preview(ctx, input, templateFS) + result, diagnostics := render(ctx, map[string]string{}) response := codersdk.DynamicParametersResponse{ ID: -1, Diagnostics: previewtypes.Diagnostics(diagnostics), @@ -175,8 +108,7 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http // The connection has been closed, so there is no one to write to return } - input.ParameterValues = update.Inputs - result, diagnostics := preview.Preview(ctx, input, templateFS) + result, diagnostics := render(ctx, update.Inputs) response := codersdk.DynamicParametersResponse{ ID: update.ID, Diagnostics: previewtypes.Diagnostics(diagnostics), @@ -193,6 +125,99 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http } } +type previewFunction func(ctx context.Context, values map[string]string) (*preview.Output, hcl.Diagnostics) + +func prepareDynamicPreview(ctx context.Context, rw http.ResponseWriter, db database.Store, fc *files.Cache, templateVersion database.TemplateVersion, user database.User) (render previewFunction, closer func(), success bool) { + openFiles := make([]uuid.UUID, 0) + closeFiles := func() { + for _, it := range openFiles { + fc.Release(it) + } + } + defer func() { + if !success { + closeFiles() + } + }() + + // nolint:gocritic // We need to fetch the templates files for the Terraform + // evaluator, and the user likely does not have permission. + fileCtx := dbauthz.AsProvisionerd(ctx) + fileID, err := db.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 nil, nil, false + } + + templateFS, err := fc.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 nil, nil, false + } + openFiles = append(openFiles, 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("{}") + tf, err := db.GetTemplateVersionTerraformValues(ctx, templateVersion.ID) + if err == nil { + plan = tf.CachedPlan + + if tf.CachedModuleFiles.Valid { + moduleFilesFS, err := fc.Acquire(fileCtx, tf.CachedModuleFiles.UUID) + if err != nil { + httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ + Message: "Internal error fetching Terraform modules.", + Detail: err.Error(), + }) + return nil, nil, false + } + openFiles = append(openFiles, tf.CachedModuleFiles.UUID) + + templateFS, err = files.NewOverlayFS(templateFS, []files.Overlay{{Path: ".terraform/modules", FS: moduleFilesFS}}) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error creating overlay filesystem.", + Detail: err.Error(), + }) + return nil, nil, false + } + } + } else if !xerrors.Is(err, sql.ErrNoRows) { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Failed to retrieve Terraform values for template version", + Detail: err.Error(), + }) + return nil, nil, false + } + + owner, err := getWorkspaceOwnerData(ctx, db, user, templateVersion.OrganizationID) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching workspace owner.", + Detail: err.Error(), + }) + return nil, nil, false + } + + input := preview.Input{ + PlanJSON: plan, + ParameterValues: map[string]string{}, + Owner: owner, + } + + return func(ctx context.Context, values map[string]string) (*preview.Output, hcl.Diagnostics) { + return preview.Preview(ctx, input, templateFS) + }, closeFiles, true +} + func staticPreview(ctx context.Context, db database.Store, version uuid.UUID) func(ctx context.Context, input preview.Input, fs fs.FS) preview.Output { dbTemplateVersionParameters, err := db.GetTemplateVersionParameters(ctx, version) if err != nil { @@ -232,8 +257,9 @@ func staticPreview(ctx context.Context, db database.Store, version uuid.UUID) fu } } -func (api *API) getWorkspaceOwnerData( +func getWorkspaceOwnerData( ctx context.Context, + db database.Store, user database.User, organizationID uuid.UUID, ) (previewtypes.WorkspaceOwner, error) { @@ -244,7 +270,7 @@ func (api *API) getWorkspaceOwnerData( // 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 := api.Database.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), user.ID) + row, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), user.ID) if err != nil { return err } @@ -271,7 +297,7 @@ func (api *API) getWorkspaceOwnerData( var publicKey string g.Go(func() error { - key, err := api.Database.GetGitSSHKey(ctx, user.ID) + key, err := db.GetGitSSHKey(ctx, user.ID) if err != nil { return err } @@ -281,7 +307,7 @@ func (api *API) getWorkspaceOwnerData( var groupNames []string g.Go(func() error { - groups, err := api.Database.GetGroups(ctx, database.GetGroupsParams{ + groups, err := db.GetGroups(ctx, database.GetGroupsParams{ OrganizationID: organizationID, HasMemberID: user.ID, }) diff --git a/coderd/parameters_internal_test.go b/coderd/parameters_internal_test.go new file mode 100644 index 0000000000000..fcbc09e904f54 --- /dev/null +++ b/coderd/parameters_internal_test.go @@ -0,0 +1,95 @@ +package coderd + +import ( + "context" + "database/sql" + "io/fs" + "net/http/httptest" + "testing" + "time" + + "github.com/google/uuid" + "github.com/spf13/afero" + "github.com/stretchr/testify/require" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbgen" + "github.com/coder/coder/v2/coderd/database/dbmem" + "github.com/coder/coder/v2/coderd/files" + "github.com/coder/coder/v2/coderd/util/ptr" +) + +func Test_prepareDynamicPreview(t *testing.T) { + t.Parallel() + + t.Run("Success", func(t *testing.T) { + db, fc, ver, user := setupPrepareDynamicPreview(t, ``, ptr.Ref("{}")) + rec := httptest.NewRecorder() + _, closer, success := prepareDynamicPreview(context.Background(), rec, db, fc, ver, user) + require.True(t, success) + + require.Equal(t, fc.Count(), 3) + closer() + + require.Equal(t, fc.Count(), 0) + }) +} + +func setupPrepareDynamicPreview(t *testing.T, tf string, modules *string) (database.Store, *files.Cache, database.TemplateVersion, database.User) { + db := dbmem.New() + + versionFile := dbgen.File(t, db, database.File{}) + + user := dbgen.User(t, db, database.User{}) + job := dbgen.ProvisionerJob(t, db, nil, database.ProvisionerJob{ + FileID: versionFile.ID, + }) + + ver := dbgen.TemplateVersion(t, db, database.TemplateVersion{ + JobID: job.ID, + }) + + modulesFile := uuid.NullUUID{} + if modules != nil { + modulesFile = uuid.NullUUID{ + UUID: uuid.New(), + Valid: true, + } + } + + dbgen.TemplateVersionTerraformValues(t, db, database.InsertTemplateVersionTerraformValuesByJobIDParams{ + JobID: job.ID, + UpdatedAt: time.Time{}, + CachedPlan: nil, + CachedModuleFiles: modulesFile, + }) + + fc := files.New(func(ctx context.Context, u uuid.UUID) (fs.FS, error) { + mem := afero.NewMemMapFs() + + if u == versionFile.ID { + f, err := mem.Create("main.tf") + if err != nil { + return nil, xerrors.Errorf("create file: %w", err) + } + _, _ = f.WriteString(tf) + _ = f.Close() + return afero.NewIOFS(mem), nil + } + + if modulesFile.Valid && u == modulesFile.UUID { + f, err := mem.Create("modules.json") + if err != nil { + return nil, xerrors.Errorf("create file: %w", err) + } + _, _ = f.WriteString(*modules) + _ = f.Close() + return afero.NewIOFS(mem), nil + } + + return nil, sql.ErrNoRows + }) + + return db, fc, ver, user +} From 2f5f25184ecd769fc8148ad7ffb3b974a641205b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 14 May 2025 17:09:55 -0500 Subject: [PATCH 03/22] wip --- coderd/database/dbauthz/dbauthz_test.go | 4 +- coderd/database/dbgen/dbgen.go | 17 ++- coderd/parameters.go | 157 +++++++++++++++++------- coderd/parameters_internal_test.go | 12 +- coderd/parameters_test.go | 124 ++++++++++++++----- 5 files changed, 225 insertions(+), 89 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 9936208ae04c1..d97079b7f6b19 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -1214,8 +1214,8 @@ func (s *MethodTestSuite) TestTemplate() { JobID: job.ID, TemplateID: uuid.NullUUID{UUID: t.ID, Valid: true}, }) - dbgen.TemplateVersionTerraformValues(s.T(), db, database.InsertTemplateVersionTerraformValuesByJobIDParams{ - JobID: job.ID, + dbgen.TemplateVersionTerraformValues(s.T(), db, database.TemplateVersionTerraformValue{ + TemplateVersionID: tv.ID, }) check.Args(tv.ID).Asserts(t, policy.ActionRead) })) diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go index 8a345fa0fd6e7..e19a3f8c08af1 100644 --- a/coderd/database/dbgen/dbgen.go +++ b/coderd/database/dbgen/dbgen.go @@ -997,11 +997,19 @@ func TemplateVersionParameter(t testing.TB, db database.Store, orig database.Tem return version } -func TemplateVersionTerraformValues(t testing.TB, db database.Store, orig database.InsertTemplateVersionTerraformValuesByJobIDParams) { +func TemplateVersionTerraformValues(t testing.TB, db database.Store, orig database.TemplateVersionTerraformValue) database.TemplateVersionTerraformValue { t.Helper() + jobID := uuid.New() + if orig.TemplateVersionID != uuid.Nil { + v, err := db.GetTemplateVersionByID(genCtx, orig.TemplateVersionID) + if err == nil { + jobID = v.JobID + } + } + params := database.InsertTemplateVersionTerraformValuesByJobIDParams{ - JobID: takeFirst(orig.JobID, uuid.New()), + JobID: jobID, CachedPlan: takeFirstSlice(orig.CachedPlan, []byte("{}")), CachedModuleFiles: orig.CachedModuleFiles, UpdatedAt: takeFirst(orig.UpdatedAt, dbtime.Now()), @@ -1010,6 +1018,11 @@ func TemplateVersionTerraformValues(t testing.TB, db database.Store, orig databa err := db.InsertTemplateVersionTerraformValuesByJobID(genCtx, params) require.NoError(t, err, "insert template version parameter") + + v, err := db.GetTemplateVersionTerraformValues(genCtx, orig.TemplateVersionID) + require.NoError(t, err, "get template version values") + + return v } func WorkspaceAgentStat(t testing.TB, db database.Store, orig database.WorkspaceAgentStat) database.WorkspaceAgentStat { diff --git a/coderd/parameters.go b/coderd/parameters.go index 270749ccb0a0c..1374a43dcc3e4 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -4,7 +4,6 @@ import ( "context" "database/sql" "encoding/json" - "io/fs" "net/http" "time" @@ -19,8 +18,10 @@ import ( "github.com/coder/coder/v2/coderd/files" "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/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/websocket" @@ -60,11 +61,38 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http return } - render, closer, success := prepareDynamicPreview(ctx, rw, api.Database, api.FileCache, templateVersion, user) - if !success { + tf, err := api.Database.GetTemplateVersionTerraformValues(ctx, templateVersion.ID) + if err != nil && !xerrors.Is(err, sql.ErrNoRows) { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Failed to retrieve Terraform values for template version", + Detail: err.Error(), + }) return } - defer closer() + + staticDiagnostics := parameterProvisionerVersionDiagnostic(tf) + + var render previewFunction + major, minor, err := apiversion.Parse(tf.ProvisionerdVersion) + if err != nil || major < 1 || (major == 1 && minor < 5) { + staticRender, err := prepareStaticPreview(ctx, api.Database, templateVersion.ID) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Failed to setup static rendering", + Detail: err.Error(), + }) + return + } + render = staticRender + } else { + // If the major version is 1.5+, we can use the dynamic preview + dynamicRender, closer, success := prepareDynamicPreview(ctx, rw, api.Database, api.FileCache, tf, templateVersion, user) + if !success { + return + } + defer closer() + render = dynamicRender + } conn, err := websocket.Accept(rw, r, nil) if err != nil { @@ -128,7 +156,7 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http type previewFunction func(ctx context.Context, values map[string]string) (*preview.Output, hcl.Diagnostics) -func prepareDynamicPreview(ctx context.Context, rw http.ResponseWriter, db database.Store, fc *files.Cache, templateVersion database.TemplateVersion, user database.User) (render previewFunction, closer func(), success bool) { +func prepareDynamicPreview(ctx context.Context, rw http.ResponseWriter, db database.Store, fc *files.Cache, tf database.TemplateVersionTerraformValue, templateVersion database.TemplateVersion, user database.User) (render previewFunction, closer func(), success bool) { openFiles := make([]uuid.UUID, 0) closeFiles := func() { for _, it := range openFiles { @@ -167,36 +195,20 @@ func prepareDynamicPreview(ctx context.Context, rw http.ResponseWriter, db datab // 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("{}") - tf, err := db.GetTemplateVersionTerraformValues(ctx, templateVersion.ID) - if err == nil { - plan = tf.CachedPlan - - if tf.CachedModuleFiles.Valid { - moduleFilesFS, err := fc.Acquire(fileCtx, tf.CachedModuleFiles.UUID) - if err != nil { - httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ - Message: "Internal error fetching Terraform modules.", - Detail: err.Error(), - }) - return nil, nil, false - } - openFiles = append(openFiles, tf.CachedModuleFiles.UUID) + plan = tf.CachedPlan - templateFS, err = files.NewOverlayFS(templateFS, []files.Overlay{{Path: ".terraform/modules", FS: moduleFilesFS}}) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error creating overlay filesystem.", - Detail: err.Error(), - }) - return nil, nil, false - } + if tf.CachedModuleFiles.Valid { + moduleFilesFS, err := fc.Acquire(fileCtx, tf.CachedModuleFiles.UUID) + if err != nil { + httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ + Message: "Internal error fetching Terraform modules.", + Detail: err.Error(), + }) + return nil, nil, false } - } else if !xerrors.Is(err, sql.ErrNoRows) { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Failed to retrieve Terraform values for template version", - Detail: err.Error(), - }) - return nil, nil, false + openFiles = append(openFiles, tf.CachedModuleFiles.UUID) + + templateFS = files.NewOverlayFS(templateFS, []files.Overlay{{Path: ".terraform/modules", FS: moduleFilesFS}}) } owner, err := getWorkspaceOwnerData(ctx, db, user, templateVersion.OrganizationID) @@ -219,15 +231,15 @@ func prepareDynamicPreview(ctx context.Context, rw http.ResponseWriter, db datab }, closeFiles, true } -func staticPreview(ctx context.Context, db database.Store, version uuid.UUID) func(ctx context.Context, input preview.Input, fs fs.FS) preview.Output { +func prepareStaticPreview(ctx context.Context, db database.Store, version uuid.UUID) (previewFunction, error) { dbTemplateVersionParameters, err := db.GetTemplateVersionParameters(ctx, version) if err != nil { - return nil + return nil, xerrors.Errorf("error fetching template version parameters: %w", err) } params := make([]previewtypes.Parameter, 0, len(dbTemplateVersionParameters)) for _, it := range dbTemplateVersionParameters { - params = append(params, previewtypes.Parameter{ + param := previewtypes.Parameter{ ParameterData: previewtypes.ParameterData{ Name: it.Name, DisplayName: it.DisplayName, @@ -240,22 +252,77 @@ func staticPreview(ctx context.Context, db database.Store, version uuid.UUID) fu Icon: it.Icon, Options: nil, Validations: nil, - Required: false, - Order: 0, - Ephemeral: false, + Required: it.Required, + Order: int64(it.DisplayOrder), + Ephemeral: it.Ephemeral, Source: nil, }, - Value: previewtypes.NullString(), + // Always use the default, since we used to assume the empty string + Value: previewtypes.StringLiteral(it.DefaultValue), Diagnostics: nil, - }) - } + } - return func(_ context.Context, in preview.Input, _ fs.FS) preview.Output { + 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)) + } - return preview.Output{ - Parameters: nil, + 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, + }) } + + param.Diagnostics = previewtypes.Diagnostics(param.Valid(param.Value)) + params = append(params, param) } + + return func(ctx context.Context, 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 { + paramValue = param.DefaultValue.AsString() + } + param.Diagnostics = previewtypes.Diagnostics(param.Valid(param.Value)) + } + + return &preview.Output{ + Parameters: params, + }, nil + }, nil } func getWorkspaceOwnerData( diff --git a/coderd/parameters_internal_test.go b/coderd/parameters_internal_test.go index 99a919eb3e423..060d1851835f9 100644 --- a/coderd/parameters_internal_test.go +++ b/coderd/parameters_internal_test.go @@ -24,9 +24,9 @@ func Test_prepareDynamicPreview(t *testing.T) { t.Parallel() t.Run("Success", func(t *testing.T) { - db, fc, ver, user := setupPrepareDynamicPreview(t, ``, ptr.Ref("{}")) + db, fc, tf, ver, user := setupPrepareDynamicPreview(t, ``, ptr.Ref("{}")) rec := httptest.NewRecorder() - _, closer, success := prepareDynamicPreview(context.Background(), rec, db, fc, ver, user) + _, closer, success := prepareDynamicPreview(context.Background(), rec, db, fc, tf, ver, user) require.True(t, success) require.Equal(t, fc.Count(), 3) @@ -36,7 +36,7 @@ func Test_prepareDynamicPreview(t *testing.T) { }) } -func setupPrepareDynamicPreview(t *testing.T, tf string, modules *string) (database.Store, *files.Cache, database.TemplateVersion, database.User) { +func setupPrepareDynamicPreview(t *testing.T, tf string, modules *string) (database.Store, *files.Cache, database.TemplateVersionTerraformValue, database.TemplateVersion, database.User) { db := dbmem.New() versionFile := dbgen.File(t, db, database.File{}) @@ -58,8 +58,8 @@ func setupPrepareDynamicPreview(t *testing.T, tf string, modules *string) (datab } } - dbgen.TemplateVersionTerraformValues(t, db, database.InsertTemplateVersionTerraformValuesByJobIDParams{ - JobID: job.ID, + tfVals := dbgen.TemplateVersionTerraformValues(t, db, database.TemplateVersionTerraformValue{ + TemplateVersionID: ver.ID, UpdatedAt: time.Time{}, CachedPlan: nil, CachedModuleFiles: modulesFile, @@ -91,7 +91,7 @@ func setupPrepareDynamicPreview(t *testing.T, tf string, modules *string) (datab return nil, sql.ErrNoRows }) - return db, fc, ver, user + return db, fc, tfVals, ver, user } func Test_parameterProvisionerVersionDiagnostic(t *testing.T) { diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go index f335f60f2b8cf..fe2abb96f8d4a 100644 --- a/coderd/parameters_test.go +++ b/coderd/parameters_test.go @@ -137,47 +137,103 @@ func TestDynamicParametersOwnerSSHPublicKey(t *testing.T) { func TestDynamicParametersWithTerraformModules(t *testing.T) { t.Parallel() - cfg := coderdtest.DeploymentValues(t) - cfg.Experiments = []string{string(codersdk.ExperimentDynamicParameters)} - ownerClient := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true, DeploymentValues: cfg}) - owner := coderdtest.CreateFirstUser(t, ownerClient) - templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.RoleTemplateAdmin()) + t.Run("OK", func(t *testing.T) { + cfg := coderdtest.DeploymentValues(t) + cfg.Experiments = []string{string(codersdk.ExperimentDynamicParameters)} + ownerClient, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{IncludeProvisionerDaemon: true, DeploymentValues: cfg}) + defer func() { + require.Equal(t, 0, api.FileCache.Count(), "file cache released all files") + }() + owner := coderdtest.CreateFirstUser(t, ownerClient) + templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.RoleTemplateAdmin()) + + dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/modules/main.tf") + require.NoError(t, err) + modulesArchive, err := terraform.GetModulesArchive(os.DirFS("testdata/parameters/modules")) + require.NoError(t, err) + + files := echo.WithExtraFiles(map[string][]byte{ + "main.tf": dynamicParametersTerraformSource, + }) + files.ProvisionPlan = []*proto.Response{{ + Type: &proto.Response_Plan{ + Plan: &proto.PlanComplete{ + Plan: []byte("{}"), + ModuleFiles: modulesArchive, + }, + }, + }} - dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/modules/main.tf") - require.NoError(t, err) - modulesArchive, err := terraform.GetModulesArchive(os.DirFS("testdata/parameters/modules")) - require.NoError(t, err) + version := coderdtest.CreateTemplateVersion(t, templateAdmin, owner.OrganizationID, files) + coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, version.ID) + _ = coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, version.ID) - files := echo.WithExtraFiles(map[string][]byte{ - "main.tf": dynamicParametersTerraformSource, + ctx := testutil.Context(t, testutil.WaitShort) + stream, err := templateAdmin.TemplateVersionDynamicParameters(ctx, templateAdminUser.ID, version.ID) + require.NoError(t, err) + defer stream.Close(websocket.StatusGoingAway) + + previews := stream.Chan() + + // Should see the output of the module represented + preview := testutil.RequireReceive(ctx, t, previews) + require.Equal(t, -1, preview.ID) + require.Empty(t, preview.Diagnostics) + + require.Len(t, preview.Parameters, 1) + require.Equal(t, "jetbrains_ide", preview.Parameters[0].Name) + require.True(t, preview.Parameters[0].Value.Valid()) + require.Equal(t, "CL", preview.Parameters[0].Value.AsString()) }) - files.ProvisionPlan = []*proto.Response{{ - Type: &proto.Response_Plan{ - Plan: &proto.PlanComplete{ - Plan: []byte("{}"), - ModuleFiles: modulesArchive, + + t.Run("OldProvisioner", func(t *testing.T) { + cfg := coderdtest.DeploymentValues(t) + cfg.Experiments = []string{string(codersdk.ExperimentDynamicParameters)} + ownerClient, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{IncludeProvisionerDaemon: true, DeploymentValues: cfg}) + defer func() { + require.Equal(t, 0, api.FileCache.Count(), "file cache released all files") + }() + + owner := coderdtest.CreateFirstUser(t, ownerClient) + templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.RoleTemplateAdmin()) + + dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/modules/main.tf") + require.NoError(t, err) + modulesArchive, err := terraform.GetModulesArchive(os.DirFS("testdata/parameters/modules")) + require.NoError(t, err) + + files := echo.WithExtraFiles(map[string][]byte{ + "main.tf": dynamicParametersTerraformSource, + }) + files.ProvisionPlan = []*proto.Response{{ + Type: &proto.Response_Plan{ + Plan: &proto.PlanComplete{ + Plan: []byte("{}"), + ModuleFiles: modulesArchive, + }, }, - }, - }} + }} - version := coderdtest.CreateTemplateVersion(t, templateAdmin, owner.OrganizationID, files) - coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, version.ID) - _ = coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, version.ID) + version := coderdtest.CreateTemplateVersion(t, templateAdmin, owner.OrganizationID, files) + coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, version.ID) + _ = coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, version.ID) - ctx := testutil.Context(t, testutil.WaitShort) - stream, err := templateAdmin.TemplateVersionDynamicParameters(ctx, templateAdminUser.ID, version.ID) - require.NoError(t, err) - defer stream.Close(websocket.StatusGoingAway) + ctx := testutil.Context(t, testutil.WaitShort) + stream, err := templateAdmin.TemplateVersionDynamicParameters(ctx, templateAdminUser.ID, version.ID) + require.NoError(t, err) + defer stream.Close(websocket.StatusGoingAway) - previews := stream.Chan() + previews := stream.Chan() - // Should see the output of the module represented - preview := testutil.RequireReceive(ctx, t, previews) - require.Equal(t, -1, preview.ID) - require.Empty(t, preview.Diagnostics) + // Should see the output of the module represented + preview := testutil.RequireReceive(ctx, t, previews) + require.Equal(t, -1, preview.ID) + require.Empty(t, preview.Diagnostics) + + require.Len(t, preview.Parameters, 1) + require.Equal(t, "jetbrains_ide", preview.Parameters[0].Name) + require.True(t, preview.Parameters[0].Value.Valid()) + require.Equal(t, "CL", preview.Parameters[0].Value.AsString()) + }) - require.Len(t, preview.Parameters, 1) - require.Equal(t, "jetbrains_ide", preview.Parameters[0].Name) - require.True(t, preview.Parameters[0].Value.Valid()) - require.Equal(t, "CL", preview.Parameters[0].Value.AsString()) } From d9c5ce58df84196cd0e18c2beb88b8d05bdf6b4e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 14 May 2025 17:35:17 -0500 Subject: [PATCH 04/22] Working prototype --- coderd/parameters.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/coderd/parameters.go b/coderd/parameters.go index 1374a43dcc3e4..e1a26abbbb84d 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -24,6 +24,7 @@ import ( 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" ) @@ -250,8 +251,8 @@ func prepareStaticPreview(ctx context.Context, db database.Store, version uuid.U Mutable: it.Mutable, DefaultValue: previewtypes.StringLiteral(it.DefaultValue), Icon: it.Icon, - Options: nil, - Validations: nil, + Options: make([]*previewtypes.ParameterOption, 0), + Validations: make([]*previewtypes.ParameterValidation, 0), Required: it.Required, Order: int64(it.DisplayOrder), Ephemeral: it.Ephemeral, @@ -303,6 +304,11 @@ func prepareStaticPreview(ctx context.Context, db database.Store, version uuid.U }) } + // 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) } From 6299f436e2d3fa30b7c3a81519a4d05a919071ee Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 14 May 2025 19:50:00 -0500 Subject: [PATCH 05/22] work on unit testing --- cli/provisionerjobs_test.go | 2 +- cli/provisioners_test.go | 2 +- cli/templatepush_test.go | 2 +- coderd/coderd.go | 12 +++- coderd/coderdtest/coderdtest.go | 9 +-- coderd/parameters_test.go | 97 +++++++++++++++++++++++++++++-- coderd/provisionerdaemons_test.go | 2 +- 7 files changed, 111 insertions(+), 15 deletions(-) diff --git a/cli/provisionerjobs_test.go b/cli/provisionerjobs_test.go index 1566147c5311d..6192473fce61c 100644 --- a/cli/provisionerjobs_test.go +++ b/cli/provisionerjobs_test.go @@ -37,7 +37,7 @@ func TestProvisionerJobs(t *testing.T) { memberClient, member := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) // Create initial resources with a running provisioner. - firstProvisioner := coderdtest.NewTaggedProvisionerDaemon(t, coderdAPI, "default-provisioner", map[string]string{"owner": "", "scope": "organization"}) + firstProvisioner := coderdtest.NewTaggedProvisionerDaemon(t, coderdAPI, "default-provisioner", "", map[string]string{"owner": "", "scope": "organization"}) t.Cleanup(func() { _ = firstProvisioner.Close() }) version := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, completeWithAgent()) coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) diff --git a/cli/provisioners_test.go b/cli/provisioners_test.go index 30a89714ff57f..58de1513a9b6b 100644 --- a/cli/provisioners_test.go +++ b/cli/provisioners_test.go @@ -74,7 +74,7 @@ func TestProvisioners_Golden(t *testing.T) { _, member := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) // Create initial resources with a running provisioner. - firstProvisioner := coderdtest.NewTaggedProvisionerDaemon(t, coderdAPI, "default-provisioner", map[string]string{"owner": "", "scope": "organization"}) + firstProvisioner := coderdtest.NewTaggedProvisionerDaemon(t, coderdAPI, "default-provisioner", "", map[string]string{"owner": "", "scope": "organization"}) t.Cleanup(func() { _ = firstProvisioner.Close() }) version := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, completeWithAgent()) coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) diff --git a/cli/templatepush_test.go b/cli/templatepush_test.go index b8e4147e6bab4..492971fe29ac2 100644 --- a/cli/templatepush_test.go +++ b/cli/templatepush_test.go @@ -585,7 +585,7 @@ func TestTemplatePush(t *testing.T) { defer provisionerDocker.Close() // Start the second provisioner - provisionerFoobar := coderdtest.NewTaggedProvisionerDaemon(t, api, "provisioner-foobar", map[string]string{ + provisionerFoobar := coderdtest.NewTaggedProvisionerDaemon(t, api, "provisioner-foobar", "", map[string]string{ "foobar": "foobaz", }) defer provisionerFoobar.Close() diff --git a/coderd/coderd.go b/coderd/coderd.go index 0e7354fe3d073..81f3a32d5ca16 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -1723,10 +1723,10 @@ func compressHandler(h http.Handler) http.Handler { // CreateInMemoryProvisionerDaemon is an in-memory connection to a provisionerd. // Useful when starting coderd and provisionerd in the same process. func (api *API) CreateInMemoryProvisionerDaemon(dialCtx context.Context, name string, provisionerTypes []codersdk.ProvisionerType) (client proto.DRPCProvisionerDaemonClient, err error) { - return api.CreateInMemoryTaggedProvisionerDaemon(dialCtx, name, provisionerTypes, nil) + return api.CreateInMemoryTaggedProvisionerDaemon(dialCtx, name, "", provisionerTypes, nil) } -func (api *API) CreateInMemoryTaggedProvisionerDaemon(dialCtx context.Context, name string, provisionerTypes []codersdk.ProvisionerType, provisionerTags map[string]string) (client proto.DRPCProvisionerDaemonClient, err error) { +func (api *API) CreateInMemoryTaggedProvisionerDaemon(dialCtx context.Context, name string, versionOverride string, provisionerTypes []codersdk.ProvisionerType, provisionerTags map[string]string) (client proto.DRPCProvisionerDaemonClient, err error) { tracer := api.TracerProvider.Tracer(tracing.TracerName) clientSession, serverSession := drpcsdk.MemTransportPipe() defer func() { @@ -1753,6 +1753,12 @@ func (api *API) CreateInMemoryTaggedProvisionerDaemon(dialCtx context.Context, n return nil, xerrors.Errorf("failed to parse built-in provisioner key ID: %w", err) } + apiVersion := proto.CurrentVersion.String() + if versionOverride != "" && flag.Lookup("test.v") != nil { + // This should only be usable for unit testing. To fake a different provisioner version + apiVersion = versionOverride + } + //nolint:gocritic // in-memory provisioners are owned by system daemon, err := api.Database.UpsertProvisionerDaemon(dbauthz.AsSystemRestricted(dialCtx), database.UpsertProvisionerDaemonParams{ Name: name, @@ -1762,7 +1768,7 @@ func (api *API) CreateInMemoryTaggedProvisionerDaemon(dialCtx context.Context, n Tags: provisionersdk.MutateTags(uuid.Nil, provisionerTags), LastSeenAt: sql.NullTime{Time: dbtime.Now(), Valid: true}, Version: buildinfo.Version(), - APIVersion: proto.CurrentVersion.String(), + APIVersion: apiVersion, KeyID: keyID, }) if err != nil { diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 85f000939d75e..8a056bb884721 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -135,6 +135,7 @@ type Options struct { // IncludeProvisionerDaemon when true means to start an in-memory provisionerD IncludeProvisionerDaemon bool + ProvisionerDaemonVersion string ProvisionerDaemonTags map[string]string MetricsCacheRefreshInterval time.Duration AgentStatsRefreshInterval time.Duration @@ -601,7 +602,7 @@ func NewWithAPI(t testing.TB, options *Options) (*codersdk.Client, io.Closer, *c setHandler(rootHandler) var provisionerCloser io.Closer = nopcloser{} if options.IncludeProvisionerDaemon { - provisionerCloser = NewTaggedProvisionerDaemon(t, coderAPI, "test", options.ProvisionerDaemonTags) + provisionerCloser = NewTaggedProvisionerDaemon(t, coderAPI, "test", options.ProvisionerDaemonVersion, options.ProvisionerDaemonTags) } client := codersdk.New(serverURL) t.Cleanup(func() { @@ -645,10 +646,10 @@ func (c *ProvisionerdCloser) Close() error { // well with coderd testing. It registers the "echo" provisioner for // quick testing. func NewProvisionerDaemon(t testing.TB, coderAPI *coderd.API) io.Closer { - return NewTaggedProvisionerDaemon(t, coderAPI, "test", nil) + return NewTaggedProvisionerDaemon(t, coderAPI, "test", "", nil) } -func NewTaggedProvisionerDaemon(t testing.TB, coderAPI *coderd.API, name string, provisionerTags map[string]string) io.Closer { +func NewTaggedProvisionerDaemon(t testing.TB, coderAPI *coderd.API, name string, versionOverride string, provisionerTags map[string]string) io.Closer { t.Helper() // t.Cleanup runs in last added, first called order. t.TempDir() will delete @@ -676,7 +677,7 @@ func NewTaggedProvisionerDaemon(t testing.TB, coderAPI *coderd.API, name string, connectedCh := make(chan struct{}) daemon := provisionerd.New(func(dialCtx context.Context) (provisionerdproto.DRPCProvisionerDaemonClient, error) { - return coderAPI.CreateInMemoryTaggedProvisionerDaemon(dialCtx, name, []codersdk.ProvisionerType{codersdk.ProvisionerTypeEcho}, provisionerTags) + return coderAPI.CreateInMemoryTaggedProvisionerDaemon(dialCtx, name, versionOverride, []codersdk.ProvisionerType{codersdk.ProvisionerTypeEcho}, provisionerTags) }, &provisionerd.Options{ Logger: coderAPI.Logger.Named("provisionerd").Leveled(slog.LevelDebug), UpdateInterval: 250 * time.Millisecond, diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go index fe2abb96f8d4a..965a3193b01e1 100644 --- a/coderd/parameters_test.go +++ b/coderd/parameters_test.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/require" + "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" @@ -189,7 +190,11 @@ func TestDynamicParametersWithTerraformModules(t *testing.T) { t.Run("OldProvisioner", func(t *testing.T) { cfg := coderdtest.DeploymentValues(t) cfg.Experiments = []string{string(codersdk.ExperimentDynamicParameters)} - ownerClient, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{IncludeProvisionerDaemon: true, DeploymentValues: cfg}) + ownerClient, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{ + ProvisionerDaemonVersion: "1.4", // Old provisioner daemon + IncludeProvisionerDaemon: true, + DeploymentValues: cfg, + }) defer func() { require.Equal(t, 0, api.FileCache.Count(), "file cache released all files") }() @@ -199,8 +204,6 @@ func TestDynamicParametersWithTerraformModules(t *testing.T) { dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/modules/main.tf") require.NoError(t, err) - modulesArchive, err := terraform.GetModulesArchive(os.DirFS("testdata/parameters/modules")) - require.NoError(t, err) files := echo.WithExtraFiles(map[string][]byte{ "main.tf": dynamicParametersTerraformSource, @@ -209,7 +212,31 @@ func TestDynamicParametersWithTerraformModules(t *testing.T) { Type: &proto.Response_Plan{ Plan: &proto.PlanComplete{ Plan: []byte("{}"), - ModuleFiles: modulesArchive, + ModuleFiles: nil, + Parameters: []*proto.RichParameter{ + { + Name: "jetbrains_ide", + Type: "string", + DefaultValue: "CL", + Icon: "", + Options: []*proto.RichParameterOption{ + { + Name: "Clion", + Description: "", + Value: "CL", + Icon: "", + }, + { + Name: "Golang", + Description: "", + Value: "GO", + Icon: "", + }, + }, + ValidationRegex: "[CG][LO]", + ValidationError: "Regex check", + }, + }, }, }, }} @@ -237,3 +264,65 @@ func TestDynamicParametersWithTerraformModules(t *testing.T) { }) } + +type dynamicParamsTestSetupParams struct { +} + +type dynamicParamsTest struct { + client *codersdk.Client + api *coderd.API +} + +func dynamicParamsTestSetup(t *testing.T) dynamicParamsTest { + cfg := coderdtest.DeploymentValues(t) + cfg.Experiments = []string{string(codersdk.ExperimentDynamicParameters)} + ownerClient, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{IncludeProvisionerDaemon: true, DeploymentValues: cfg}) + defer func() { + require.Equal(t, 0, api.FileCache.Count(), "file cache released all files") + }() + owner := coderdtest.CreateFirstUser(t, ownerClient) + templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.RoleTemplateAdmin()) + + dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/modules/main.tf") + require.NoError(t, err) + modulesArchive, err := terraform.GetModulesArchive(os.DirFS("testdata/parameters/modules")) + require.NoError(t, err) + + files := echo.WithExtraFiles(map[string][]byte{ + "main.tf": dynamicParametersTerraformSource, + }) + files.ProvisionPlan = []*proto.Response{{ + Type: &proto.Response_Plan{ + Plan: &proto.PlanComplete{ + Plan: []byte("{}"), + ModuleFiles: modulesArchive, + }, + }, + }} + + version := coderdtest.CreateTemplateVersion(t, templateAdmin, owner.OrganizationID, files) + coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, version.ID) + _ = coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, version.ID) + + ctx := testutil.Context(t, testutil.WaitShort) + stream, err := templateAdmin.TemplateVersionDynamicParameters(ctx, templateAdminUser.ID, version.ID) + require.NoError(t, err) + defer stream.Close(websocket.StatusGoingAway) + + previews := stream.Chan() + + // Should see the output of the module represented + preview := testutil.RequireReceive(ctx, t, previews) + require.Equal(t, -1, preview.ID) + require.Empty(t, preview.Diagnostics) + + require.Len(t, preview.Parameters, 1) + require.Equal(t, "jetbrains_ide", preview.Parameters[0].Name) + require.True(t, preview.Parameters[0].Value.Valid()) + require.Equal(t, "CL", preview.Parameters[0].Value.AsString()) + + return dynamicParamsTest{ + client: ownerClient, + api: api, + } +} diff --git a/coderd/provisionerdaemons_test.go b/coderd/provisionerdaemons_test.go index 249da9d6bc922..fe3edc2fec0c8 100644 --- a/coderd/provisionerdaemons_test.go +++ b/coderd/provisionerdaemons_test.go @@ -38,7 +38,7 @@ func TestProvisionerDaemons(t *testing.T) { memberClient, member := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) // Create initial resources with a running provisioner. - firstProvisioner := coderdtest.NewTaggedProvisionerDaemon(t, coderdAPI, "default-provisioner", map[string]string{"owner": "", "scope": "organization"}) + firstProvisioner := coderdtest.NewTaggedProvisionerDaemon(t, coderdAPI, "default-provisioner", "", map[string]string{"owner": "", "scope": "organization"}) t.Cleanup(func() { _ = firstProvisioner.Close() }) version := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, nil) coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) From f80d8387d38beae6ed50403a46c071bbdf9d8014 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 14 May 2025 20:35:54 -0500 Subject: [PATCH 06/22] chore: add unit test for static values --- coderd/parameters_test.go | 214 +++++++++++++++++--------------------- go.mod | 2 +- go.sum | 4 +- 3 files changed, 101 insertions(+), 119 deletions(-) diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go index 965a3193b01e1..6497e769e2d33 100644 --- a/coderd/parameters_test.go +++ b/coderd/parameters_test.go @@ -10,8 +10,10 @@ import ( "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/codersdk/wsjson" "github.com/coder/coder/v2/provisioner/echo" "github.com/coder/coder/v2/provisioner/terraform" + provProto "github.com/coder/coder/v2/provisionerd/proto" "github.com/coder/coder/v2/provisionersdk/proto" "github.com/coder/coder/v2/testutil" "github.com/coder/websocket" @@ -135,45 +137,26 @@ func TestDynamicParametersOwnerSSHPublicKey(t *testing.T) { require.Equal(t, sshKey.PublicKey, preview.Parameters[0].Value.Value.AsString()) } -func TestDynamicParametersWithTerraformModules(t *testing.T) { +func TestDynamicParametersWithTerraformValues(t *testing.T) { t.Parallel() - t.Run("OK", func(t *testing.T) { - cfg := coderdtest.DeploymentValues(t) - cfg.Experiments = []string{string(codersdk.ExperimentDynamicParameters)} - ownerClient, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{IncludeProvisionerDaemon: true, DeploymentValues: cfg}) - defer func() { - require.Equal(t, 0, api.FileCache.Count(), "file cache released all files") - }() - owner := coderdtest.CreateFirstUser(t, ownerClient) - templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.RoleTemplateAdmin()) - + t.Run("OK_Modules", func(t *testing.T) { dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/modules/main.tf") require.NoError(t, err) + modulesArchive, err := terraform.GetModulesArchive(os.DirFS("testdata/parameters/modules")) require.NoError(t, err) - files := echo.WithExtraFiles(map[string][]byte{ - "main.tf": dynamicParametersTerraformSource, + setup := setupDynamicParamsTest(t, setupDynamicParamsTestParams{ + provisionerDaemonVersion: provProto.CurrentVersion.String(), + mainTF: dynamicParametersTerraformSource, + modulesArchive: modulesArchive, + plan: nil, + static: nil, }) - files.ProvisionPlan = []*proto.Response{{ - Type: &proto.Response_Plan{ - Plan: &proto.PlanComplete{ - Plan: []byte("{}"), - ModuleFiles: modulesArchive, - }, - }, - }} - - version := coderdtest.CreateTemplateVersion(t, templateAdmin, owner.OrganizationID, files) - coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, version.ID) - _ = coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, version.ID) ctx := testutil.Context(t, testutil.WaitShort) - stream, err := templateAdmin.TemplateVersionDynamicParameters(ctx, templateAdminUser.ID, version.ID) - require.NoError(t, err) - defer stream.Close(websocket.StatusGoingAway) - + stream := setup.stream previews := stream.Chan() // Should see the output of the module represented @@ -187,115 +170,118 @@ func TestDynamicParametersWithTerraformModules(t *testing.T) { require.Equal(t, "CL", preview.Parameters[0].Value.AsString()) }) + // OldProvisioners use the static parameters in the dynamic param flow t.Run("OldProvisioner", func(t *testing.T) { - cfg := coderdtest.DeploymentValues(t) - cfg.Experiments = []string{string(codersdk.ExperimentDynamicParameters)} - ownerClient, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{ - ProvisionerDaemonVersion: "1.4", // Old provisioner daemon - IncludeProvisionerDaemon: true, - DeploymentValues: cfg, - }) - defer func() { - require.Equal(t, 0, api.FileCache.Count(), "file cache released all files") - }() - - owner := coderdtest.CreateFirstUser(t, ownerClient) - templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.RoleTemplateAdmin()) - - dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/modules/main.tf") - require.NoError(t, err) - - files := echo.WithExtraFiles(map[string][]byte{ - "main.tf": dynamicParametersTerraformSource, - }) - files.ProvisionPlan = []*proto.Response{{ - Type: &proto.Response_Plan{ - Plan: &proto.PlanComplete{ - Plan: []byte("{}"), - ModuleFiles: nil, - Parameters: []*proto.RichParameter{ + setup := setupDynamicParamsTest(t, setupDynamicParamsTestParams{ + provisionerDaemonVersion: "1.4", + mainTF: nil, + modulesArchive: nil, + plan: nil, + static: []*proto.RichParameter{ + { + Name: "jetbrains_ide", + Type: "string", + DefaultValue: "PS", + Icon: "", + Options: []*proto.RichParameterOption{ { - Name: "jetbrains_ide", - Type: "string", - DefaultValue: "CL", - Icon: "", - Options: []*proto.RichParameterOption{ - { - Name: "Clion", - Description: "", - Value: "CL", - Icon: "", - }, - { - Name: "Golang", - Description: "", - Value: "GO", - Icon: "", - }, - }, - ValidationRegex: "[CG][LO]", - ValidationError: "Regex check", + Name: "PHPStorm", + Description: "", + Value: "PS", + Icon: "", + }, + { + Name: "Golang", + Description: "", + Value: "GO", + Icon: "", }, }, + ValidationRegex: "[PG][SO]", + ValidationError: "Regex check", }, }, - }} - - version := coderdtest.CreateTemplateVersion(t, templateAdmin, owner.OrganizationID, files) - coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, version.ID) - _ = coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, version.ID) + }) ctx := testutil.Context(t, testutil.WaitShort) - stream, err := templateAdmin.TemplateVersionDynamicParameters(ctx, templateAdminUser.ID, version.ID) - require.NoError(t, err) - defer stream.Close(websocket.StatusGoingAway) - + stream := setup.stream previews := stream.Chan() - // Should see the output of the module represented + // Assert the initial state preview := testutil.RequireReceive(ctx, t, previews) - require.Equal(t, -1, preview.ID) - require.Empty(t, preview.Diagnostics) - + diagCount := len(preview.Diagnostics) + require.Equal(t, 1, diagCount) + require.Contains(t, preview.Diagnostics[0].Summary, "classic creation flow") require.Len(t, preview.Parameters, 1) require.Equal(t, "jetbrains_ide", preview.Parameters[0].Name) require.True(t, preview.Parameters[0].Value.Valid()) - require.Equal(t, "CL", preview.Parameters[0].Value.AsString()) - }) + require.Equal(t, "PS", preview.Parameters[0].Value.AsString()) + + // Test some inputs + for _, exp := range []string{"PS", "GO", "Invalid"} { + err := stream.Send(codersdk.DynamicParametersRequest{ + ID: 1, + Inputs: map[string]string{ + "jetbrains_ide": exp, + }, + }) + require.NoError(t, err) + + preview := testutil.RequireReceive(ctx, t, previews) + diagCount := len(preview.Diagnostics) + require.Equal(t, 1, diagCount) + require.Contains(t, preview.Diagnostics[0].Summary, "classic creation flow") + + require.Len(t, preview.Parameters, 1) + if exp == "Invalid" { // Try an invalid option + require.Len(t, preview.Parameters[0].Diagnostics, 1) + } else { + require.Len(t, preview.Parameters[0].Diagnostics, 0) + } + require.Equal(t, "jetbrains_ide", preview.Parameters[0].Name) + require.True(t, preview.Parameters[0].Value.Valid()) + require.Equal(t, exp, preview.Parameters[0].Value.AsString()) + } + }) } -type dynamicParamsTestSetupParams struct { +type setupDynamicParamsTestParams struct { + provisionerDaemonVersion string + mainTF []byte + modulesArchive []byte + plan []byte + + static []*proto.RichParameter } type dynamicParamsTest struct { client *codersdk.Client api *coderd.API + stream *wsjson.Stream[codersdk.DynamicParametersResponse, codersdk.DynamicParametersRequest] } -func dynamicParamsTestSetup(t *testing.T) dynamicParamsTest { +func setupDynamicParamsTest(t *testing.T, args setupDynamicParamsTestParams) dynamicParamsTest { cfg := coderdtest.DeploymentValues(t) cfg.Experiments = []string{string(codersdk.ExperimentDynamicParameters)} - ownerClient, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{IncludeProvisionerDaemon: true, DeploymentValues: cfg}) - defer func() { - require.Equal(t, 0, api.FileCache.Count(), "file cache released all files") - }() + ownerClient, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{ + IncludeProvisionerDaemon: true, + ProvisionerDaemonVersion: args.provisionerDaemonVersion, + DeploymentValues: cfg, + }) + owner := coderdtest.CreateFirstUser(t, ownerClient) templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.RoleTemplateAdmin()) - dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/modules/main.tf") - require.NoError(t, err) - modulesArchive, err := terraform.GetModulesArchive(os.DirFS("testdata/parameters/modules")) - require.NoError(t, err) - files := echo.WithExtraFiles(map[string][]byte{ - "main.tf": dynamicParametersTerraformSource, + "main.tf": args.mainTF, }) files.ProvisionPlan = []*proto.Response{{ Type: &proto.Response_Plan{ Plan: &proto.PlanComplete{ - Plan: []byte("{}"), - ModuleFiles: modulesArchive, + Plan: args.plan, + ModuleFiles: args.modulesArchive, + Parameters: args.static, }, }, }} @@ -307,22 +293,18 @@ func dynamicParamsTestSetup(t *testing.T) dynamicParamsTest { ctx := testutil.Context(t, testutil.WaitShort) stream, err := templateAdmin.TemplateVersionDynamicParameters(ctx, templateAdminUser.ID, version.ID) require.NoError(t, err) - defer stream.Close(websocket.StatusGoingAway) - previews := stream.Chan() - - // Should see the output of the module represented - preview := testutil.RequireReceive(ctx, t, previews) - require.Equal(t, -1, preview.ID) - require.Empty(t, preview.Diagnostics) - - require.Len(t, preview.Parameters, 1) - require.Equal(t, "jetbrains_ide", preview.Parameters[0].Name) - require.True(t, preview.Parameters[0].Value.Valid()) - require.Equal(t, "CL", preview.Parameters[0].Value.AsString()) + t.Cleanup(func() { + _ = stream.Close(websocket.StatusGoingAway) + // Cache should always have 0 files when the only stream is closed + require.Eventually(t, func() bool { + return api.FileCache.Count() == 0 + }, testutil.WaitShort/5, testutil.IntervalMedium) + }) return dynamicParamsTest{ client: ownerClient, + stream: stream, api: api, } } diff --git a/go.mod b/go.mod index 9dfa9a5ab7adf..cdbd61574066f 100644 --- a/go.mod +++ b/go.mod @@ -485,7 +485,7 @@ require ( require ( github.com/anthropics/anthropic-sdk-go v0.2.0-beta.3 - github.com/coder/preview v0.0.2-0.20250509141204-fc9484dbe506 + github.com/coder/preview v0.0.2-0.20250510235314-66630669ff6f github.com/fsnotify/fsnotify v1.9.0 github.com/kylecarbs/aisdk-go v0.0.8 github.com/mark3labs/mcp-go v0.27.0 diff --git a/go.sum b/go.sum index 3a6d9d92cfb93..3d635991b7abe 100644 --- a/go.sum +++ b/go.sum @@ -911,8 +911,8 @@ github.com/coder/pq v1.10.5-0.20240813183442-0c420cb5a048 h1:3jzYUlGH7ZELIH4XggX github.com/coder/pq v1.10.5-0.20240813183442-0c420cb5a048/go.mod h1:AlVN5x4E4T544tWzH6hKfbfQvm3HdbOxrmggDNAPY9o= github.com/coder/pretty v0.0.0-20230908205945-e89ba86370e0 h1:3A0ES21Ke+FxEM8CXx9n47SZOKOpgSE1bbJzlE4qPVs= github.com/coder/pretty v0.0.0-20230908205945-e89ba86370e0/go.mod h1:5UuS2Ts+nTToAMeOjNlnHFkPahrtDkmpydBen/3wgZc= -github.com/coder/preview v0.0.2-0.20250509141204-fc9484dbe506 h1:rQ7Queq1IZwEBjEIk9EJsVx7XHQ+Rvo2h72/A88BnPg= -github.com/coder/preview v0.0.2-0.20250509141204-fc9484dbe506/go.mod h1:wXVvHiSmZv/7Q+Ug5I0B45TGM2U+YAjY4K3aB/6+KKo= +github.com/coder/preview v0.0.2-0.20250510235314-66630669ff6f h1:Q1AcLBpRRR8rf/H+GxcJaZ5Ox4UeIRkDgc6wvvmOJAo= +github.com/coder/preview v0.0.2-0.20250510235314-66630669ff6f/go.mod h1:GfkwIv5gQLpL01qeGU1/YoxoFtt5trzCqnWZLo77clU= github.com/coder/quartz v0.1.3 h1:hA2nI8uUA2fNN9uhXv2I4xZD4aHkA7oH3g2t03v4xf8= github.com/coder/quartz v0.1.3/go.mod h1:vsiCc+AHViMKH2CQpGIpFgdHIEQsxwm8yCscqKmzbRA= github.com/coder/retry v1.5.1 h1:iWu8YnD8YqHs3XwqrqsjoBTAVqT9ml6z9ViJ2wlMiqc= From a5ee3747dff2e51cf0052bc57385bfb9c5ba78cd Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 14 May 2025 20:38:46 -0500 Subject: [PATCH 07/22] revert changes to cache --- coderd/files/cache.go | 18 ++-- coderd/parameters_internal_test.go | 163 ----------------------------- 2 files changed, 7 insertions(+), 174 deletions(-) delete mode 100644 coderd/parameters_internal_test.go diff --git a/coderd/files/cache.go b/coderd/files/cache.go index 28df15b8fe690..2f105f71f09a5 100644 --- a/coderd/files/cache.go +++ b/coderd/files/cache.go @@ -14,18 +14,10 @@ import ( "github.com/coder/coder/v2/coderd/util/lazy" ) -func New(fetch fetcher) *Cache { - return &Cache{ - lock: sync.Mutex{}, - data: make(map[uuid.UUID]*cacheEntry), - fetcher: fetch, - } -} - // NewFromStore returns a file cache that will fetch files from the provided // database. -func NewFromStore(store database.Store) *Cache { - fetch := func(ctx context.Context, fileID uuid.UUID) (fs.FS, error) { +func NewFromStore(store database.Store) Cache { + fetcher := func(ctx context.Context, fileID uuid.UUID) (fs.FS, error) { file, err := store.GetFileByID(ctx, fileID) if err != nil { return nil, xerrors.Errorf("failed to read file from database: %w", err) @@ -35,7 +27,11 @@ func NewFromStore(store database.Store) *Cache { return archivefs.FromTarReader(content), nil } - return New(fetch) + return Cache{ + lock: sync.Mutex{}, + data: make(map[uuid.UUID]*cacheEntry), + fetcher: fetcher, + } } // Cache persists the files for template versions, and is used by dynamic diff --git a/coderd/parameters_internal_test.go b/coderd/parameters_internal_test.go deleted file mode 100644 index 060d1851835f9..0000000000000 --- a/coderd/parameters_internal_test.go +++ /dev/null @@ -1,163 +0,0 @@ -package coderd - -import ( - "context" - "database/sql" - "io/fs" - "net/http/httptest" - "testing" - "time" - - "github.com/google/uuid" - "github.com/spf13/afero" - "github.com/stretchr/testify/require" - "golang.org/x/xerrors" - - "github.com/coder/coder/v2/coderd/database" - "github.com/coder/coder/v2/coderd/database/dbgen" - "github.com/coder/coder/v2/coderd/database/dbmem" - "github.com/coder/coder/v2/coderd/files" - "github.com/coder/coder/v2/coderd/util/ptr" -) - -func Test_prepareDynamicPreview(t *testing.T) { - t.Parallel() - - t.Run("Success", func(t *testing.T) { - db, fc, tf, ver, user := setupPrepareDynamicPreview(t, ``, ptr.Ref("{}")) - rec := httptest.NewRecorder() - _, closer, success := prepareDynamicPreview(context.Background(), rec, db, fc, tf, ver, user) - require.True(t, success) - - require.Equal(t, fc.Count(), 3) - closer() - - require.Equal(t, fc.Count(), 0) - }) -} - -func setupPrepareDynamicPreview(t *testing.T, tf string, modules *string) (database.Store, *files.Cache, database.TemplateVersionTerraformValue, database.TemplateVersion, database.User) { - db := dbmem.New() - - versionFile := dbgen.File(t, db, database.File{}) - - user := dbgen.User(t, db, database.User{}) - job := dbgen.ProvisionerJob(t, db, nil, database.ProvisionerJob{ - FileID: versionFile.ID, - }) - - ver := dbgen.TemplateVersion(t, db, database.TemplateVersion{ - JobID: job.ID, - }) - - modulesFile := uuid.NullUUID{} - if modules != nil { - modulesFile = uuid.NullUUID{ - UUID: uuid.New(), - Valid: true, - } - } - - tfVals := dbgen.TemplateVersionTerraformValues(t, db, database.TemplateVersionTerraformValue{ - TemplateVersionID: ver.ID, - UpdatedAt: time.Time{}, - CachedPlan: nil, - CachedModuleFiles: modulesFile, - }) - - fc := files.New(func(ctx context.Context, u uuid.UUID) (fs.FS, error) { - mem := afero.NewMemMapFs() - - if u == versionFile.ID { - f, err := mem.Create("main.tf") - if err != nil { - return nil, xerrors.Errorf("create file: %w", err) - } - _, _ = f.WriteString(tf) - _ = f.Close() - return afero.NewIOFS(mem), nil - } - - if modulesFile.Valid && u == modulesFile.UUID { - f, err := mem.Create("modules.json") - if err != nil { - return nil, xerrors.Errorf("create file: %w", err) - } - _, _ = f.WriteString(*modules) - _ = f.Close() - return afero.NewIOFS(mem), nil - } - - return nil, sql.ErrNoRows - }) - - return db, fc, tfVals, ver, user -} - -func Test_parameterProvisionerVersionDiagnostic(t *testing.T) { - t.Parallel() - - testCases := []struct { - version string - warning bool - }{ - { - version: "", - warning: true, - }, - { - version: "invalid", - warning: true, - }, - { - version: "0.4", - warning: true, - }, - { - version: "0.5", - warning: true, - }, - { - version: "0.6", - warning: true, - }, - { - version: "1.4", - warning: true, - }, - { - version: "1.5", - warning: false, - }, - { - version: "1.6", - warning: false, - }, - { - version: "2.0", - warning: false, - }, - { - version: "2.5", - warning: false, - }, - { - version: "2.6", - warning: false, - }, - } - - for _, tc := range testCases { - t.Run("Version_"+tc.version, func(t *testing.T) { - t.Parallel() - diags := parameterProvisionerVersionDiagnostic(database.TemplateVersionTerraformValue{ - ProvisionerdVersion: tc.version, - }) - if tc.warning { - require.Len(t, diags, 1, "expected warning") - } else { - require.Len(t, diags, 0, "expected no warning") - } - }) - } -} From 3cb39eaad2a19637d263cc296e49fa2522cb531f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 14 May 2025 21:10:39 -0500 Subject: [PATCH 08/22] test: add unit test for closing files --- coderd/files/cache.go | 4 +-- coderd/parameters.go | 35 ++++++++++++++++++---- coderd/parameters_test.go | 61 +++++++++++++++++++++++++++++++++++++-- 3 files changed, 90 insertions(+), 10 deletions(-) diff --git a/coderd/files/cache.go b/coderd/files/cache.go index 2f105f71f09a5..56e9a715de189 100644 --- a/coderd/files/cache.go +++ b/coderd/files/cache.go @@ -16,7 +16,7 @@ import ( // NewFromStore returns a file cache that will fetch files from the provided // database. -func NewFromStore(store database.Store) Cache { +func NewFromStore(store database.Store) *Cache { fetcher := func(ctx context.Context, fileID uuid.UUID) (fs.FS, error) { file, err := store.GetFileByID(ctx, fileID) if err != nil { @@ -27,7 +27,7 @@ func NewFromStore(store database.Store) Cache { return archivefs.FromTarReader(content), nil } - return Cache{ + return &Cache{ lock: sync.Mutex{}, data: make(map[uuid.UUID]*cacheEntry), fetcher: fetcher, diff --git a/coderd/parameters.go b/coderd/parameters.go index e1a26abbbb84d..a162957a4923e 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -71,11 +71,23 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http return } + // staticDiagnostics is a set of diagnostics to be applied to all rendered results. staticDiagnostics := parameterProvisionerVersionDiagnostic(tf) + // render is the function that given a set of input values, will return the + // parameter state. There is 2 rendering functions. + // + // prepareStaticPreview uses the static set of parameters saved from the template + // import. These parameters are returned on every request, and have no dynamic + // functionality. This exists for backwards compatibility with older template versions + // which have not uploaded their plan & module files. + // + // prepareDynamicPreview uses the dynamic preview engine. var render previewFunction major, minor, err := apiversion.Parse(tf.ProvisionerdVersion) if err != nil || major < 1 || (major == 1 && minor < 5) { + // Versions < 1.5 do not upload the required files. + // Versions == "" are < 1.5, but we don't know the exact version. staticRender, err := prepareStaticPreview(ctx, api.Database, templateVersion.ID) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ @@ -113,7 +125,7 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http // Send an initial form state, computed without any user input. result, diagnostics := render(ctx, map[string]string{}) response := codersdk.DynamicParametersResponse{ - ID: -1, + ID: -1, // Always start with -1. Diagnostics: previewtypes.Diagnostics(diagnostics.Extend(staticDiagnostics)), } if result != nil { @@ -138,6 +150,7 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http // The connection has been closed, so there is no one to write to return } + result, diagnostics := render(ctx, update.Inputs) response := codersdk.DynamicParametersResponse{ ID: update.ID, @@ -158,12 +171,16 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http type previewFunction func(ctx context.Context, values map[string]string) (*preview.Output, hcl.Diagnostics) func prepareDynamicPreview(ctx context.Context, rw http.ResponseWriter, db database.Store, fc *files.Cache, tf database.TemplateVersionTerraformValue, templateVersion database.TemplateVersion, user database.User) (render previewFunction, closer func(), success bool) { + // keep track of all files opened openFiles := make([]uuid.UUID, 0) closeFiles := func() { for _, it := range openFiles { fc.Release(it) } } + + // This defer will close the files if the function exits early without success. + // Closing the files is important to avoid having a memory leak. defer func() { if !success { closeFiles() @@ -182,6 +199,8 @@ func prepareDynamicPreview(ctx context.Context, rw http.ResponseWriter, db datab return nil, nil, false } + // Add the file first. Calling `Release` if it fails is a no-op, so this is safe. + openFiles = append(openFiles, fileID) templateFS, err := fc.Acquire(fileCtx, fileID) if err != nil { httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ @@ -190,7 +209,6 @@ func prepareDynamicPreview(ctx context.Context, rw http.ResponseWriter, db datab }) return nil, nil, false } - openFiles = append(openFiles, fileID) // Having the Terraform plan available for the evaluation engine is helpful // for populating values from data blocks, but isn't strictly required. If @@ -198,6 +216,7 @@ func prepareDynamicPreview(ctx context.Context, rw http.ResponseWriter, db datab plan := json.RawMessage("{}") plan = tf.CachedPlan + openFiles = append(openFiles, tf.CachedModuleFiles.UUID) if tf.CachedModuleFiles.Valid { moduleFilesFS, err := fc.Acquire(fileCtx, tf.CachedModuleFiles.UUID) if err != nil { @@ -207,7 +226,6 @@ func prepareDynamicPreview(ctx context.Context, rw http.ResponseWriter, db datab }) return nil, nil, false } - openFiles = append(openFiles, tf.CachedModuleFiles.UUID) templateFS = files.NewOverlayFS(templateFS, []files.Overlay{{Path: ".terraform/modules", FS: moduleFilesFS}}) } @@ -371,7 +389,10 @@ func getWorkspaceOwnerData( var publicKey string g.Go(func() error { - key, err := db.GetGitSSHKey(ctx, user.ID) + // 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), user.ID) if err != nil { return err } @@ -381,7 +402,11 @@ func getWorkspaceOwnerData( var groupNames []string g.Go(func() error { - groups, err := db.GetGroups(ctx, database.GetGroupsParams{ + // 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: user.ID, }) diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go index 6497e769e2d33..0ff64291ccc5b 100644 --- a/coderd/parameters_test.go +++ b/coderd/parameters_test.go @@ -1,13 +1,19 @@ package coderd_test import ( + "context" "os" "testing" + "github.com/google/uuid" "github.com/stretchr/testify/require" + "golang.org/x/xerrors" "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/coderd/database/pubsub" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/wsjson" @@ -141,6 +147,8 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { t.Parallel() t.Run("OK_Modules", func(t *testing.T) { + t.Parallel() + dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/modules/main.tf") require.NoError(t, err) @@ -172,6 +180,8 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { // OldProvisioners use the static parameters in the dynamic param flow t.Run("OldProvisioner", func(t *testing.T) { + t.Parallel() + setup := setupDynamicParamsTest(t, setupDynamicParamsTestParams{ provisionerDaemonVersion: "1.4", mainTF: nil, @@ -244,15 +254,42 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { } }) + + t.Run("FileError", func(t *testing.T) { + // Verify files close even if the websocket terminates from an error + t.Parallel() + + db, ps := dbtestutil.NewDB(t) + dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/modules/main.tf") + require.NoError(t, err) + + modulesArchive, err := terraform.GetModulesArchive(os.DirFS("testdata/parameters/modules")) + require.NoError(t, err) + + setup := setupDynamicParamsTest(t, setupDynamicParamsTestParams{ + db: &dbRejectGitSSHKey{Store: db}, + ps: ps, + 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()) + }) } type setupDynamicParamsTestParams struct { + db database.Store + ps pubsub.Pubsub provisionerDaemonVersion string mainTF []byte modulesArchive []byte plan []byte - static []*proto.RichParameter + static []*proto.RichParameter + expectWebsocketError bool } type dynamicParamsTest struct { @@ -265,6 +302,8 @@ func setupDynamicParamsTest(t *testing.T, args setupDynamicParamsTestParams) dyn cfg := coderdtest.DeploymentValues(t) cfg.Experiments = []string{string(codersdk.ExperimentDynamicParameters)} ownerClient, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{ + Database: args.db, + Pubsub: args.ps, IncludeProvisionerDaemon: true, ProvisionerDaemonVersion: args.provisionerDaemonVersion, DeploymentValues: cfg, @@ -292,10 +331,16 @@ func setupDynamicParamsTest(t *testing.T, args setupDynamicParamsTestParams) dyn ctx := testutil.Context(t, testutil.WaitShort) stream, err := templateAdmin.TemplateVersionDynamicParameters(ctx, templateAdminUser.ID, version.ID) - require.NoError(t, err) + if args.expectWebsocketError { + require.Errorf(t, err, "expected error forming websocket") + } else { + require.NoError(t, err) + } t.Cleanup(func() { - _ = stream.Close(websocket.StatusGoingAway) + if stream != nil { + _ = stream.Close(websocket.StatusGoingAway) + } // Cache should always have 0 files when the only stream is closed require.Eventually(t, func() bool { return api.FileCache.Count() == 0 @@ -308,3 +353,13 @@ func setupDynamicParamsTest(t *testing.T, args setupDynamicParamsTestParams) dyn api: api, } } + +// dbRejectGitSSHKey is a cheeky way to force an error to occur in a place +// that is generally impossible to force an error. +type dbRejectGitSSHKey struct { + database.Store +} + +func (d *dbRejectGitSSHKey) GetGitSSHKey(_ context.Context, _ uuid.UUID) (database.GitSSHKey, error) { + return database.GitSSHKey{}, xerrors.New("forcing a fake error") +} From d5a68dd6db3dff4c8c64576764891e8495c9a4d3 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 May 2025 11:23:02 -0500 Subject: [PATCH 09/22] linting --- coderd/parameters.go | 6 ++++-- coderd/parameters_test.go | 21 ++++++++++++--------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/coderd/parameters.go b/coderd/parameters.go index a162957a4923e..20c41e260a55b 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -214,7 +214,9 @@ func prepareDynamicPreview(ctx context.Context, rw http.ResponseWriter, db datab // 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("{}") - plan = tf.CachedPlan + if len(tf.CachedPlan) > 0 { + plan = tf.CachedPlan + } openFiles = append(openFiles, tf.CachedModuleFiles.UUID) if tf.CachedModuleFiles.Valid { @@ -338,7 +340,7 @@ func prepareStaticPreview(ctx context.Context, db database.Store, version uuid.U if ok { param.Value = previewtypes.StringLiteral(paramValue) } else { - paramValue = param.DefaultValue.AsString() + param.Value = param.DefaultValue } param.Diagnostics = previewtypes.Diagnostics(param.Valid(param.Value)) } diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go index 0ff64291ccc5b..bf9515a50c5bf 100644 --- a/coderd/parameters_test.go +++ b/coderd/parameters_test.go @@ -182,6 +182,7 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { t.Run("OldProvisioner", func(t *testing.T) { t.Parallel() + const defaultValue = "PS" setup := setupDynamicParamsTest(t, setupDynamicParamsTestParams{ provisionerDaemonVersion: "1.4", mainTF: nil, @@ -191,13 +192,13 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { { Name: "jetbrains_ide", Type: "string", - DefaultValue: "PS", + DefaultValue: defaultValue, Icon: "", Options: []*proto.RichParameterOption{ { Name: "PHPStorm", Description: "", - Value: "PS", + Value: defaultValue, Icon: "", }, { @@ -225,15 +226,18 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { require.Len(t, preview.Parameters, 1) require.Equal(t, "jetbrains_ide", preview.Parameters[0].Name) require.True(t, preview.Parameters[0].Value.Valid()) - require.Equal(t, "PS", preview.Parameters[0].Value.AsString()) + require.Equal(t, defaultValue, preview.Parameters[0].Value.AsString()) // Test some inputs - for _, exp := range []string{"PS", "GO", "Invalid"} { + for _, exp := range []string{defaultValue, "GO", "Invalid", defaultValue} { + inputs := map[string]string{} + if exp != defaultValue { + // Let the default value be the default without being explicitly set + inputs["jetbrains_ide"] = exp + } err := stream.Send(codersdk.DynamicParametersRequest{ - ID: 1, - Inputs: map[string]string{ - "jetbrains_ide": exp, - }, + ID: 1, + Inputs: inputs, }) require.NoError(t, err) @@ -252,7 +256,6 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { require.True(t, preview.Parameters[0].Value.Valid()) require.Equal(t, exp, preview.Parameters[0].Value.AsString()) } - }) t.Run("FileError", func(t *testing.T) { From bbd08039887ce11deaaa9efb3d8b90667526f29d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 May 2025 11:37:14 -0500 Subject: [PATCH 10/22] chore: move group test to enterprise --- coderd/parameters.go | 3 + coderd/parameters_test.go | 71 ------------ enterprise/coderd/parameters_test.go | 101 ++++++++++++++++++ .../testdata/parameters/groups/main.tf | 2 +- .../testdata/parameters/groups/plan.json | 0 5 files changed, 105 insertions(+), 72 deletions(-) create mode 100644 enterprise/coderd/parameters_test.go rename {coderd => enterprise/coderd}/testdata/parameters/groups/main.tf (83%) rename {coderd => enterprise/coderd}/testdata/parameters/groups/plan.json (100%) diff --git a/coderd/parameters.go b/coderd/parameters.go index 20c41e260a55b..1cf909da6e3ac 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -248,6 +248,9 @@ func prepareDynamicPreview(ctx context.Context, rw http.ResponseWriter, db datab } return func(ctx context.Context, values map[string]string) (*preview.Output, hcl.Diagnostics) { + // Update the input values with the new values. + // The rest of the input is unchanged. + input.ParameterValues = values return preview.Preview(ctx, input, templateFS) }, closeFiles, true } diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go index bf9515a50c5bf..d038722761c34 100644 --- a/coderd/parameters_test.go +++ b/coderd/parameters_test.go @@ -25,77 +25,6 @@ import ( "github.com/coder/websocket" ) -func TestDynamicParametersOwnerGroups(t *testing.T) { - t.Parallel() - - cfg := coderdtest.DeploymentValues(t) - cfg.Experiments = []string{string(codersdk.ExperimentDynamicParameters)} - ownerClient := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true, DeploymentValues: cfg}) - owner := coderdtest.CreateFirstUser(t, ownerClient) - templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.RoleTemplateAdmin()) - - dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/groups/main.tf") - require.NoError(t, err) - dynamicParametersTerraformPlan, err := os.ReadFile("testdata/parameters/groups/plan.json") - require.NoError(t, err) - - files := echo.WithExtraFiles(map[string][]byte{ - "main.tf": dynamicParametersTerraformSource, - }) - files.ProvisionPlan = []*proto.Response{{ - Type: &proto.Response_Plan{ - Plan: &proto.PlanComplete{ - Plan: dynamicParametersTerraformPlan, - }, - }, - }} - - version := coderdtest.CreateTemplateVersion(t, templateAdmin, owner.OrganizationID, files) - coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, version.ID) - _ = coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, version.ID) - - ctx := testutil.Context(t, testutil.WaitShort) - stream, err := templateAdmin.TemplateVersionDynamicParameters(ctx, templateAdminUser.ID, version.ID) - require.NoError(t, err) - defer stream.Close(websocket.StatusGoingAway) - - previews := stream.Chan() - - // Should automatically send a form state with all defaulted/empty values - 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) - require.True(t, preview.Parameters[0].Value.Valid()) - require.Equal(t, "Everyone", preview.Parameters[0].Value.Value.AsString()) - - // Send a new value, and see it reflected - err = stream.Send(codersdk.DynamicParametersRequest{ - ID: 1, - Inputs: map[string]string{"group": "Bloob"}, - }) - 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) - require.True(t, preview.Parameters[0].Value.Valid()) - require.Equal(t, "Bloob", preview.Parameters[0].Value.Value.AsString()) - - // Back to default - err = stream.Send(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) - require.True(t, preview.Parameters[0].Value.Valid()) - require.Equal(t, "Everyone", preview.Parameters[0].Value.Value.AsString()) -} - func TestDynamicParametersOwnerSSHPublicKey(t *testing.T) { t.Parallel() diff --git a/enterprise/coderd/parameters_test.go b/enterprise/coderd/parameters_test.go new file mode 100644 index 0000000000000..e6bc564e43da2 --- /dev/null +++ b/enterprise/coderd/parameters_test.go @@ -0,0 +1,101 @@ +package coderd_test + +import ( + "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/provisioner/echo" + "github.com/coder/coder/v2/provisionersdk/proto" + "github.com/coder/coder/v2/testutil" + "github.com/coder/websocket" +) + +func TestDynamicParametersOwnerGroups(t *testing.T) { + t.Parallel() + + cfg := coderdtest.DeploymentValues(t) + cfg.Experiments = []string{string(codersdk.ExperimentDynamicParameters)} + ownerClient, owner := coderdenttest.New(t, + &coderdenttest.Options{ + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureTemplateRBAC: 1, + }, + }, + Options: &coderdtest.Options{IncludeProvisionerDaemon: true, DeploymentValues: cfg}, + }, + ) + templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.RoleTemplateAdmin()) + + // Create the group to be asserted + group := coderdtest.CreateGroup(t, ownerClient, owner.OrganizationID, "bloob", templateAdminUser) + + dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/groups/main.tf") + require.NoError(t, err) + dynamicParametersTerraformPlan, err := os.ReadFile("testdata/parameters/groups/plan.json") + require.NoError(t, err) + + files := echo.WithExtraFiles(map[string][]byte{ + "main.tf": dynamicParametersTerraformSource, + }) + files.ProvisionPlan = []*proto.Response{{ + Type: &proto.Response_Plan{ + Plan: &proto.PlanComplete{ + Plan: dynamicParametersTerraformPlan, + }, + }, + }} + + version := coderdtest.CreateTemplateVersion(t, templateAdmin, owner.OrganizationID, files) + coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, version.ID) + _ = coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, version.ID) + + ctx := testutil.Context(t, testutil.WaitShort) + stream, err := templateAdmin.TemplateVersionDynamicParameters(ctx, templateAdminUser.ID, version.ID) + require.NoError(t, err) + defer stream.Close(websocket.StatusGoingAway) + + previews := stream.Chan() + + // Should automatically send a form state with all defaulted/empty values + 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) + require.True(t, preview.Parameters[0].Value.Valid()) + require.Equal(t, database.EveryoneGroup, preview.Parameters[0].Value.Value.AsString()) + + // Send a new value, and see it reflected + err = stream.Send(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) + require.True(t, preview.Parameters[0].Value.Valid()) + require.Equal(t, group.Name, preview.Parameters[0].Value.Value.AsString()) + + // Back to default + err = stream.Send(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) + require.True(t, preview.Parameters[0].Value.Valid()) + require.Equal(t, database.EveryoneGroup, preview.Parameters[0].Value.Value.AsString()) +} diff --git a/coderd/testdata/parameters/groups/main.tf b/enterprise/coderd/testdata/parameters/groups/main.tf similarity index 83% rename from coderd/testdata/parameters/groups/main.tf rename to enterprise/coderd/testdata/parameters/groups/main.tf index 8bed301f33ce5..9356cc2840e91 100644 --- a/coderd/testdata/parameters/groups/main.tf +++ b/enterprise/coderd/testdata/parameters/groups/main.tf @@ -12,7 +12,7 @@ data "coder_parameter" "group" { name = "group" default = try(data.coder_workspace_owner.me.groups[0], "") dynamic "option" { - for_each = concat(data.coder_workspace_owner.me.groups, "bloob") + for_each = data.coder_workspace_owner.me.groups content { name = option.value value = option.value diff --git a/coderd/testdata/parameters/groups/plan.json b/enterprise/coderd/testdata/parameters/groups/plan.json similarity index 100% rename from coderd/testdata/parameters/groups/plan.json rename to enterprise/coderd/testdata/parameters/groups/plan.json From f5fe4b7de23ee2baf74c038693de7ce06dd10b8f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 May 2025 12:17:52 -0500 Subject: [PATCH 11/22] linting --- coderd/parameters.go | 2 +- coderd/parameters_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/parameters.go b/coderd/parameters.go index 1cf909da6e3ac..472b83efeeb3f 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -336,7 +336,7 @@ func prepareStaticPreview(ctx context.Context, db database.Store, version uuid.U params = append(params, param) } - return func(ctx context.Context, values map[string]string) (*preview.Output, hcl.Diagnostics) { + return func(_ context.Context, values map[string]string) (*preview.Output, hcl.Diagnostics) { for i := range params { param := ¶ms[i] paramValue, ok := values[param.Name] diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go index d038722761c34..ca8cbc4cfe7d3 100644 --- a/coderd/parameters_test.go +++ b/coderd/parameters_test.go @@ -292,6 +292,6 @@ type dbRejectGitSSHKey struct { database.Store } -func (d *dbRejectGitSSHKey) GetGitSSHKey(_ context.Context, _ uuid.UUID) (database.GitSSHKey, error) { +func (_ *dbRejectGitSSHKey) GetGitSSHKey(_ context.Context, _ uuid.UUID) (database.GitSSHKey, error) { return database.GitSSHKey{}, xerrors.New("forcing a fake error") } From dfd0e24253a7efc12921d9b02479cc59945e2d60 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 May 2025 13:08:54 -0500 Subject: [PATCH 12/22] linting --- coderd/parameters_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go index ca8cbc4cfe7d3..25f6058e60a7a 100644 --- a/coderd/parameters_test.go +++ b/coderd/parameters_test.go @@ -292,6 +292,6 @@ type dbRejectGitSSHKey struct { database.Store } -func (_ *dbRejectGitSSHKey) GetGitSSHKey(_ context.Context, _ uuid.UUID) (database.GitSSHKey, error) { +func (*dbRejectGitSSHKey) GetGitSSHKey(_ context.Context, _ uuid.UUID) (database.GitSSHKey, error) { return database.GitSSHKey{}, xerrors.New("forcing a fake error") } From 137561964c1fe8a90848ac85150c290cc34530f3 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 May 2025 13:21:27 -0500 Subject: [PATCH 13/22] revert deleted coderd/parameters_internal_test.go --- coderd/parameters_internal_test.go | 77 ++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 coderd/parameters_internal_test.go diff --git a/coderd/parameters_internal_test.go b/coderd/parameters_internal_test.go new file mode 100644 index 0000000000000..a02baeae380b6 --- /dev/null +++ b/coderd/parameters_internal_test.go @@ -0,0 +1,77 @@ +package coderd + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/database" +) + +func Test_parameterProvisionerVersionDiagnostic(t *testing.T) { + t.Parallel() + + testCases := []struct { + version string + warning bool + }{ + { + version: "", + warning: true, + }, + { + version: "invalid", + warning: true, + }, + { + version: "0.4", + warning: true, + }, + { + version: "0.5", + warning: true, + }, + { + version: "0.6", + warning: true, + }, + { + version: "1.4", + warning: true, + }, + { + version: "1.5", + warning: false, + }, + { + version: "1.6", + warning: false, + }, + { + version: "2.0", + warning: false, + }, + { + version: "2.5", + warning: false, + }, + { + version: "2.6", + warning: false, + }, + } + + for _, tc := range testCases { + t.Run("Version_"+tc.version, func(t *testing.T) { + t.Parallel() + diags := parameterProvisionerVersionDiagnostic(database.TemplateVersionTerraformValue{ + ProvisionerdVersion: tc.version, + }) + if tc.warning { + require.Len(t, diags, 1, "expected warning") + } else { + require.Len(t, diags, 0, "expected no warning") + } + }) + } +} From 4dd41495618b44c6de5422eeba5f8b0f58750ae5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 May 2025 14:20:37 -0500 Subject: [PATCH 14/22] chore: refactor websocket code into own function Dynamic & static code flow is now independent --- coderd/parameters.go | 263 ++++++++++++----------------- coderd/parameters_internal_test.go | 77 --------- 2 files changed, 106 insertions(+), 234 deletions(-) delete mode 100644 coderd/parameters_internal_test.go diff --git a/coderd/parameters.go b/coderd/parameters.go index 472b83efeeb3f..46b85ad0dc754 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -37,8 +37,7 @@ import ( // @Success 101 // @Router /users/{user}/templateversions/{templateversion}/parameters [get] func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http.Request) { - ctx, cancel := context.WithTimeout(r.Context(), 30*time.Minute) - defer cancel() + ctx := r.Context() user := httpmw.UserParam(r) templateVersion := httpmw.TemplateVersionParam(r) @@ -71,143 +70,45 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http return } - // staticDiagnostics is a set of diagnostics to be applied to all rendered results. - staticDiagnostics := parameterProvisionerVersionDiagnostic(tf) - - // render is the function that given a set of input values, will return the - // parameter state. There is 2 rendering functions. - // - // prepareStaticPreview uses the static set of parameters saved from the template - // import. These parameters are returned on every request, and have no dynamic - // functionality. This exists for backwards compatibility with older template versions - // which have not uploaded their plan & module files. - // - // prepareDynamicPreview uses the dynamic preview engine. - var render previewFunction major, minor, err := apiversion.Parse(tf.ProvisionerdVersion) - if err != nil || major < 1 || (major == 1 && minor < 5) { - // Versions < 1.5 do not upload the required files. - // Versions == "" are < 1.5, but we don't know the exact version. - staticRender, err := prepareStaticPreview(ctx, api.Database, templateVersion.ID) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Failed to setup static rendering", - Detail: err.Error(), - }) - return - } - render = staticRender + // If the api version is not valid or less than 1.5, we need to use the static parameters + useStaticParams := err != nil || major < 1 || (major == 1 && minor < 5) + if useStaticParams { + api.handleStaticParameters(rw, r, templateVersion.ID) } else { - // If the major version is 1.5+, we can use the dynamic preview - dynamicRender, closer, success := prepareDynamicPreview(ctx, rw, api.Database, api.FileCache, tf, templateVersion, user) - if !success { - return - } - defer closer() - render = dynamicRender - } - - conn, err := websocket.Accept(rw, r, nil) - if err != nil { - httpapi.Write(ctx, rw, http.StatusUpgradeRequired, codersdk.Response{ - Message: "Failed to accept WebSocket.", - Detail: err.Error(), - }) - return - } - stream := wsjson.NewStream[codersdk.DynamicParametersRequest, codersdk.DynamicParametersResponse]( - conn, - websocket.MessageText, - websocket.MessageText, - api.Logger, - ) - - // Send an initial form state, computed without any user input. - result, diagnostics := render(ctx, map[string]string{}) - response := codersdk.DynamicParametersResponse{ - ID: -1, // Always start with -1. - Diagnostics: previewtypes.Diagnostics(diagnostics.Extend(staticDiagnostics)), - } - if result != nil { - response.Parameters = result.Parameters - } - err = stream.Send(response) - if err != nil { - stream.Drop() - return - } - - // As the user types into the form, reprocess the state using their input, - // and respond with updates. - updates := stream.Chan() - for { - select { - case <-ctx.Done(): - stream.Close(websocket.StatusGoingAway) - return - case update, ok := <-updates: - if !ok { - // The connection has been closed, so there is no one to write to - return - } - - result, diagnostics := render(ctx, update.Inputs) - response := codersdk.DynamicParametersResponse{ - ID: update.ID, - Diagnostics: previewtypes.Diagnostics(diagnostics.Extend(staticDiagnostics)), - } - if result != nil { - response.Parameters = result.Parameters - } - err = stream.Send(response) - if err != nil { - stream.Drop() - return - } - } + api.handleDynamicParameters(rw, r, tf, templateVersion) } } type previewFunction func(ctx context.Context, values map[string]string) (*preview.Output, hcl.Diagnostics) -func prepareDynamicPreview(ctx context.Context, rw http.ResponseWriter, db database.Store, fc *files.Cache, tf database.TemplateVersionTerraformValue, templateVersion database.TemplateVersion, user database.User) (render previewFunction, closer func(), success bool) { - // keep track of all files opened - openFiles := make([]uuid.UUID, 0) - closeFiles := func() { - for _, it := range openFiles { - fc.Release(it) - } - } - - // This defer will close the files if the function exits early without success. - // Closing the files is important to avoid having a memory leak. - defer func() { - if !success { - closeFiles() - } - }() +func (api *API) handleDynamicParameters(rw http.ResponseWriter, r *http.Request, tf database.TemplateVersionTerraformValue, templateVersion database.TemplateVersion) { + var ( + ctx = r.Context() + user = httpmw.UserParam(r) + ) // nolint:gocritic // We need to fetch the templates files for the Terraform // evaluator, and the user likely does not have permission. fileCtx := dbauthz.AsProvisionerd(ctx) - fileID, err := db.GetFileIDByTemplateVersionID(fileCtx, templateVersion.ID) + 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 nil, nil, false + return } // Add the file first. Calling `Release` if it fails is a no-op, so this is safe. - openFiles = append(openFiles, fileID) - templateFS, err := fc.Acquire(fileCtx, fileID) + templateFS, err := api.FileCache.Acquire(fileCtx, fileID) + defer api.FileCache.Release(fileID) if err != nil { httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ Message: "Internal error fetching template version Terraform.", Detail: err.Error(), }) - return nil, nil, false + return } // Having the Terraform plan available for the evaluation engine is helpful @@ -218,27 +119,27 @@ func prepareDynamicPreview(ctx context.Context, rw http.ResponseWriter, db datab plan = tf.CachedPlan } - openFiles = append(openFiles, tf.CachedModuleFiles.UUID) if tf.CachedModuleFiles.Valid { - moduleFilesFS, err := fc.Acquire(fileCtx, tf.CachedModuleFiles.UUID) + moduleFilesFS, err := api.FileCache.Acquire(fileCtx, tf.CachedModuleFiles.UUID) + defer api.FileCache.Release(fileID) if err != nil { httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ Message: "Internal error fetching Terraform modules.", Detail: err.Error(), }) - return nil, nil, false + return } templateFS = files.NewOverlayFS(templateFS, []files.Overlay{{Path: ".terraform/modules", FS: moduleFilesFS}}) } - owner, err := getWorkspaceOwnerData(ctx, db, user, templateVersion.OrganizationID) + owner, err := getWorkspaceOwnerData(ctx, api.Database, user, templateVersion.OrganizationID) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching workspace owner.", Detail: err.Error(), }) - return nil, nil, false + return } input := preview.Input{ @@ -247,18 +148,23 @@ func prepareDynamicPreview(ctx context.Context, rw http.ResponseWriter, db datab Owner: owner, } - return func(ctx context.Context, values map[string]string) (*preview.Output, hcl.Diagnostics) { + api.handleParameterWebsocket(rw, r, func(ctx context.Context, values map[string]string) (*preview.Output, hcl.Diagnostics) { // Update the input values with the new values. // The rest of the input is unchanged. input.ParameterValues = values return preview.Preview(ctx, input, templateFS) - }, closeFiles, true + }) } -func prepareStaticPreview(ctx context.Context, db database.Store, version uuid.UUID) (previewFunction, error) { - dbTemplateVersionParameters, err := db.GetTemplateVersionParameters(ctx, version) +func (api *API) handleStaticParameters(rw http.ResponseWriter, r *http.Request, version uuid.UUID) { + ctx := r.Context() + dbTemplateVersionParameters, err := api.Database.GetTemplateVersionParameters(ctx, version) if err != nil { - return nil, xerrors.Errorf("error fetching template version parameters: %w", err) + 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)) @@ -336,7 +242,7 @@ func prepareStaticPreview(ctx context.Context, db database.Store, version uuid.U params = append(params, param) } - return func(_ context.Context, values map[string]string) (*preview.Output, hcl.Diagnostics) { + api.handleParameterWebsocket(rw, r, func(ctx context.Context, values map[string]string) (*preview.Output, hcl.Diagnostics) { for i := range params { param := ¶ms[i] paramValue, ok := values[param.Name] @@ -349,9 +255,80 @@ func prepareStaticPreview(ctx context.Context, db database.Store, version uuid.U } return &preview.Output{ - Parameters: params, - }, nil - }, nil + Parameters: params, + }, hcl.Diagnostics{ + { + Severity: hcl.DiagError, + Summary: "This template version is missing required metadata to support dynamic parameters. Go back to the classic creation flow.", + Detail: "To restore full functionality, please re-import the terraform as a new template version.", + }, + } + }) +} + +func (api *API) handleParameterWebsocket(rw http.ResponseWriter, r *http.Request, render previewFunction) { + ctx, cancel := context.WithTimeout(r.Context(), 30*time.Minute) + defer cancel() + + conn, err := websocket.Accept(rw, r, nil) + if err != nil { + httpapi.Write(ctx, rw, http.StatusUpgradeRequired, codersdk.Response{ + Message: "Failed to accept WebSocket.", + Detail: err.Error(), + }) + return + } + stream := wsjson.NewStream[codersdk.DynamicParametersRequest, codersdk.DynamicParametersResponse]( + conn, + websocket.MessageText, + websocket.MessageText, + api.Logger, + ) + + // Send an initial form state, computed without any user input. + result, diagnostics := render(ctx, map[string]string{}) + response := codersdk.DynamicParametersResponse{ + ID: -1, // Always start with -1. + Diagnostics: previewtypes.Diagnostics(diagnostics), + } + if result != nil { + response.Parameters = result.Parameters + } + err = stream.Send(response) + if err != nil { + stream.Drop() + return + } + + // As the user types into the form, reprocess the state using their input, + // and respond with updates. + updates := stream.Chan() + for { + select { + case <-ctx.Done(): + stream.Close(websocket.StatusGoingAway) + return + case update, ok := <-updates: + if !ok { + // The connection has been closed, so there is no one to write to + return + } + + result, diagnostics := render(ctx, update.Inputs) + response := codersdk.DynamicParametersResponse{ + ID: update.ID, + Diagnostics: previewtypes.Diagnostics(diagnostics), + } + if result != nil { + response.Parameters = result.Parameters + } + err = stream.Send(response) + if err != nil { + stream.Drop() + return + } + } + } } func getWorkspaceOwnerData( @@ -441,31 +418,3 @@ func getWorkspaceOwnerData( Groups: groupNames, }, nil } - -// parameterProvisionerVersionDiagnostic checks the version of the provisioner -// used to create the template version. If the version is less than 1.5, it -// returns a warning diagnostic. Only versions 1.5+ return the module & plan data -// required. -func parameterProvisionerVersionDiagnostic(tf database.TemplateVersionTerraformValue) hcl.Diagnostics { - missingMetadata := hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "This template version is missing required metadata to support dynamic parameters. Go back to the classic creation flow.", - Detail: "To restore full functionality, please re-import the terraform as a new template version.", - } - - if tf.ProvisionerdVersion == "" { - return hcl.Diagnostics{&missingMetadata} - } - - major, minor, err := apiversion.Parse(tf.ProvisionerdVersion) - if err != nil || tf.ProvisionerdVersion == "" { - return hcl.Diagnostics{&missingMetadata} - } else if major < 1 || (major == 1 && minor < 5) { - missingMetadata.Detail = "This template version does not support dynamic parameters. " + - "Some options may be missing or incorrect. " + - "Please contact an administrator to update the provisioner and re-import the template version." - return hcl.Diagnostics{&missingMetadata} - } - - return nil -} diff --git a/coderd/parameters_internal_test.go b/coderd/parameters_internal_test.go deleted file mode 100644 index a02baeae380b6..0000000000000 --- a/coderd/parameters_internal_test.go +++ /dev/null @@ -1,77 +0,0 @@ -package coderd - -import ( - "testing" - - "github.com/stretchr/testify/require" - - "github.com/coder/coder/v2/coderd/database" -) - -func Test_parameterProvisionerVersionDiagnostic(t *testing.T) { - t.Parallel() - - testCases := []struct { - version string - warning bool - }{ - { - version: "", - warning: true, - }, - { - version: "invalid", - warning: true, - }, - { - version: "0.4", - warning: true, - }, - { - version: "0.5", - warning: true, - }, - { - version: "0.6", - warning: true, - }, - { - version: "1.4", - warning: true, - }, - { - version: "1.5", - warning: false, - }, - { - version: "1.6", - warning: false, - }, - { - version: "2.0", - warning: false, - }, - { - version: "2.5", - warning: false, - }, - { - version: "2.6", - warning: false, - }, - } - - for _, tc := range testCases { - t.Run("Version_"+tc.version, func(t *testing.T) { - t.Parallel() - diags := parameterProvisionerVersionDiagnostic(database.TemplateVersionTerraformValue{ - ProvisionerdVersion: tc.version, - }) - if tc.warning { - require.Len(t, diags, 1, "expected warning") - } else { - require.Len(t, diags, 0, "expected no warning") - } - }) - } -} From f3cff03c5936a74df05ca0d044d7ae876af11a9b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 May 2025 14:21:16 -0500 Subject: [PATCH 15/22] fixup --- coderd/parameters.go | 1 - 1 file changed, 1 deletion(-) diff --git a/coderd/parameters.go b/coderd/parameters.go index 46b85ad0dc754..4d9b610adf1f1 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -38,7 +38,6 @@ import ( // @Router /users/{user}/templateversions/{templateversion}/parameters [get] func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - user := httpmw.UserParam(r) templateVersion := httpmw.TemplateVersionParam(r) // Check that the job has completed successfully From 13215d621fb2bfcd60cecc273daa8afbd3d640a7 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 May 2025 14:28:19 -0500 Subject: [PATCH 16/22] fix leaked files --- coderd/parameters.go | 2 +- codersdk/client.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/parameters.go b/coderd/parameters.go index 4d9b610adf1f1..24a382c5b7bf9 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -120,7 +120,7 @@ func (api *API) handleDynamicParameters(rw http.ResponseWriter, r *http.Request, if tf.CachedModuleFiles.Valid { moduleFilesFS, err := api.FileCache.Acquire(fileCtx, tf.CachedModuleFiles.UUID) - defer api.FileCache.Release(fileID) + defer api.FileCache.Release(tf.CachedModuleFiles.UUID) if err != nil { httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ Message: "Internal error fetching Terraform modules.", diff --git a/codersdk/client.go b/codersdk/client.go index 4492066785d6f..b0fb4d9764b3c 100644 --- a/codersdk/client.go +++ b/codersdk/client.go @@ -359,7 +359,7 @@ func (c *Client) Dial(ctx context.Context, path string, opts *websocket.DialOpti } conn, resp, err := websocket.Dial(ctx, u.String(), opts) - if resp.Body != nil { + if resp != nil && resp.Body != nil { resp.Body.Close() } if err != nil { From 72f2234eedef2b2c7232d68f973e80044b41a778 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 May 2025 14:35:02 -0500 Subject: [PATCH 17/22] fix up the error message for static params for now --- coderd/parameters.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/coderd/parameters.go b/coderd/parameters.go index 24a382c5b7bf9..733a724fcaa3c 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -257,8 +257,9 @@ func (api *API) handleStaticParameters(rw http.ResponseWriter, r *http.Request, Parameters: params, }, hcl.Diagnostics{ { - Severity: hcl.DiagError, - Summary: "This template version is missing required metadata to support dynamic parameters. Go back to the classic creation flow.", + // 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.", }, } From 4ea01657690a2a7987172b308b0c43f06f7794c6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 May 2025 14:40:04 -0500 Subject: [PATCH 18/22] linting --- coderd/parameters.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/parameters.go b/coderd/parameters.go index 733a724fcaa3c..3449ca41f1701 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -241,7 +241,7 @@ func (api *API) handleStaticParameters(rw http.ResponseWriter, r *http.Request, params = append(params, param) } - api.handleParameterWebsocket(rw, r, func(ctx context.Context, values map[string]string) (*preview.Output, hcl.Diagnostics) { + api.handleParameterWebsocket(rw, r, func(_ context.Context, values map[string]string) (*preview.Output, hcl.Diagnostics) { for i := range params { param := ¶ms[i] paramValue, ok := values[param.Name] From ef15df1170b5520b8f8d9d89e05622dedeb6c34a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 May 2025 16:50:52 -0500 Subject: [PATCH 19/22] move release to only if err == nil --- coderd/parameters.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/parameters.go b/coderd/parameters.go index 3449ca41f1701..a452843371c3a 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -101,7 +101,6 @@ func (api *API) handleDynamicParameters(rw http.ResponseWriter, r *http.Request, // Add the file first. Calling `Release` if it fails is a no-op, so this is safe. templateFS, err := api.FileCache.Acquire(fileCtx, fileID) - defer api.FileCache.Release(fileID) if err != nil { httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ Message: "Internal error fetching template version Terraform.", @@ -109,6 +108,7 @@ func (api *API) handleDynamicParameters(rw http.ResponseWriter, r *http.Request, }) 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 @@ -120,7 +120,6 @@ func (api *API) handleDynamicParameters(rw http.ResponseWriter, r *http.Request, if tf.CachedModuleFiles.Valid { moduleFilesFS, err := api.FileCache.Acquire(fileCtx, tf.CachedModuleFiles.UUID) - defer api.FileCache.Release(tf.CachedModuleFiles.UUID) if err != nil { httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ Message: "Internal error fetching Terraform modules.", @@ -128,6 +127,7 @@ func (api *API) handleDynamicParameters(rw http.ResponseWriter, r *http.Request, }) return } + defer api.FileCache.Release(tf.CachedModuleFiles.UUID) templateFS = files.NewOverlayFS(templateFS, []files.Overlay{{Path: ".terraform/modules", FS: moduleFilesFS}}) } From df65b926a09b6c4e800a4261209fea5daa70e550 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 15 May 2025 18:48:02 -0500 Subject: [PATCH 20/22] use options pattern for provisioner version override --- cli/provisionerjobs_test.go | 2 +- cli/provisioners_test.go | 2 +- cli/templatepush_test.go | 2 +- coderd/coderd.go | 25 +++++++++++++++++++++---- coderd/coderdtest/coderdtest.go | 8 ++++---- coderd/provisionerdaemons_test.go | 2 +- 6 files changed, 29 insertions(+), 12 deletions(-) diff --git a/cli/provisionerjobs_test.go b/cli/provisionerjobs_test.go index 6192473fce61c..1566147c5311d 100644 --- a/cli/provisionerjobs_test.go +++ b/cli/provisionerjobs_test.go @@ -37,7 +37,7 @@ func TestProvisionerJobs(t *testing.T) { memberClient, member := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) // Create initial resources with a running provisioner. - firstProvisioner := coderdtest.NewTaggedProvisionerDaemon(t, coderdAPI, "default-provisioner", "", map[string]string{"owner": "", "scope": "organization"}) + firstProvisioner := coderdtest.NewTaggedProvisionerDaemon(t, coderdAPI, "default-provisioner", map[string]string{"owner": "", "scope": "organization"}) t.Cleanup(func() { _ = firstProvisioner.Close() }) version := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, completeWithAgent()) coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) diff --git a/cli/provisioners_test.go b/cli/provisioners_test.go index 58de1513a9b6b..30a89714ff57f 100644 --- a/cli/provisioners_test.go +++ b/cli/provisioners_test.go @@ -74,7 +74,7 @@ func TestProvisioners_Golden(t *testing.T) { _, member := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) // Create initial resources with a running provisioner. - firstProvisioner := coderdtest.NewTaggedProvisionerDaemon(t, coderdAPI, "default-provisioner", "", map[string]string{"owner": "", "scope": "organization"}) + firstProvisioner := coderdtest.NewTaggedProvisionerDaemon(t, coderdAPI, "default-provisioner", map[string]string{"owner": "", "scope": "organization"}) t.Cleanup(func() { _ = firstProvisioner.Close() }) version := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, completeWithAgent()) coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) diff --git a/cli/templatepush_test.go b/cli/templatepush_test.go index 492971fe29ac2..b8e4147e6bab4 100644 --- a/cli/templatepush_test.go +++ b/cli/templatepush_test.go @@ -585,7 +585,7 @@ func TestTemplatePush(t *testing.T) { defer provisionerDocker.Close() // Start the second provisioner - provisionerFoobar := coderdtest.NewTaggedProvisionerDaemon(t, api, "provisioner-foobar", "", map[string]string{ + provisionerFoobar := coderdtest.NewTaggedProvisionerDaemon(t, api, "provisioner-foobar", map[string]string{ "foobar": "foobaz", }) defer provisionerFoobar.Close() diff --git a/coderd/coderd.go b/coderd/coderd.go index 81f3a32d5ca16..a1ac250a2502b 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -1720,13 +1720,30 @@ func compressHandler(h http.Handler) http.Handler { return cmp.Handler(h) } +type MemoryProvisionerDaemonOption func(*memoryProvisionerDaemonOptions) + +func MemoryProvisionerWithVersionOverride(version string) MemoryProvisionerDaemonOption { + return func(opts *memoryProvisionerDaemonOptions) { + opts.versionOverride = version + } +} + +type memoryProvisionerDaemonOptions struct { + versionOverride string +} + // CreateInMemoryProvisionerDaemon is an in-memory connection to a provisionerd. // Useful when starting coderd and provisionerd in the same process. func (api *API) CreateInMemoryProvisionerDaemon(dialCtx context.Context, name string, provisionerTypes []codersdk.ProvisionerType) (client proto.DRPCProvisionerDaemonClient, err error) { - return api.CreateInMemoryTaggedProvisionerDaemon(dialCtx, name, "", provisionerTypes, nil) + return api.CreateInMemoryTaggedProvisionerDaemon(dialCtx, name, provisionerTypes, nil) } -func (api *API) CreateInMemoryTaggedProvisionerDaemon(dialCtx context.Context, name string, versionOverride string, provisionerTypes []codersdk.ProvisionerType, provisionerTags map[string]string) (client proto.DRPCProvisionerDaemonClient, err error) { +func (api *API) CreateInMemoryTaggedProvisionerDaemon(dialCtx context.Context, name string, provisionerTypes []codersdk.ProvisionerType, provisionerTags map[string]string, opts ...MemoryProvisionerDaemonOption) (client proto.DRPCProvisionerDaemonClient, err error) { + options := &memoryProvisionerDaemonOptions{} + for _, opt := range opts { + opt(options) + } + tracer := api.TracerProvider.Tracer(tracing.TracerName) clientSession, serverSession := drpcsdk.MemTransportPipe() defer func() { @@ -1754,9 +1771,9 @@ func (api *API) CreateInMemoryTaggedProvisionerDaemon(dialCtx context.Context, n } apiVersion := proto.CurrentVersion.String() - if versionOverride != "" && flag.Lookup("test.v") != nil { + if options.versionOverride != "" && flag.Lookup("test.v") != nil { // This should only be usable for unit testing. To fake a different provisioner version - apiVersion = versionOverride + apiVersion = options.versionOverride } //nolint:gocritic // in-memory provisioners are owned by system diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 8a056bb884721..a25f0576e76be 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -602,7 +602,7 @@ func NewWithAPI(t testing.TB, options *Options) (*codersdk.Client, io.Closer, *c setHandler(rootHandler) var provisionerCloser io.Closer = nopcloser{} if options.IncludeProvisionerDaemon { - provisionerCloser = NewTaggedProvisionerDaemon(t, coderAPI, "test", options.ProvisionerDaemonVersion, options.ProvisionerDaemonTags) + provisionerCloser = NewTaggedProvisionerDaemon(t, coderAPI, "test", options.ProvisionerDaemonTags, coderd.MemoryProvisionerWithVersionOverride(options.ProvisionerDaemonVersion)) } client := codersdk.New(serverURL) t.Cleanup(func() { @@ -646,10 +646,10 @@ func (c *ProvisionerdCloser) Close() error { // well with coderd testing. It registers the "echo" provisioner for // quick testing. func NewProvisionerDaemon(t testing.TB, coderAPI *coderd.API) io.Closer { - return NewTaggedProvisionerDaemon(t, coderAPI, "test", "", nil) + return NewTaggedProvisionerDaemon(t, coderAPI, "test", nil) } -func NewTaggedProvisionerDaemon(t testing.TB, coderAPI *coderd.API, name string, versionOverride string, provisionerTags map[string]string) io.Closer { +func NewTaggedProvisionerDaemon(t testing.TB, coderAPI *coderd.API, name string, provisionerTags map[string]string, opts ...coderd.MemoryProvisionerDaemonOption) io.Closer { t.Helper() // t.Cleanup runs in last added, first called order. t.TempDir() will delete @@ -677,7 +677,7 @@ func NewTaggedProvisionerDaemon(t testing.TB, coderAPI *coderd.API, name string, connectedCh := make(chan struct{}) daemon := provisionerd.New(func(dialCtx context.Context) (provisionerdproto.DRPCProvisionerDaemonClient, error) { - return coderAPI.CreateInMemoryTaggedProvisionerDaemon(dialCtx, name, versionOverride, []codersdk.ProvisionerType{codersdk.ProvisionerTypeEcho}, provisionerTags) + return coderAPI.CreateInMemoryTaggedProvisionerDaemon(dialCtx, name, []codersdk.ProvisionerType{codersdk.ProvisionerTypeEcho}, provisionerTags, opts...) }, &provisionerd.Options{ Logger: coderAPI.Logger.Named("provisionerd").Leveled(slog.LevelDebug), UpdateInterval: 250 * time.Millisecond, diff --git a/coderd/provisionerdaemons_test.go b/coderd/provisionerdaemons_test.go index fe3edc2fec0c8..249da9d6bc922 100644 --- a/coderd/provisionerdaemons_test.go +++ b/coderd/provisionerdaemons_test.go @@ -38,7 +38,7 @@ func TestProvisionerDaemons(t *testing.T) { memberClient, member := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) // Create initial resources with a running provisioner. - firstProvisioner := coderdtest.NewTaggedProvisionerDaemon(t, coderdAPI, "default-provisioner", "", map[string]string{"owner": "", "scope": "organization"}) + firstProvisioner := coderdtest.NewTaggedProvisionerDaemon(t, coderdAPI, "default-provisioner", map[string]string{"owner": "", "scope": "organization"}) t.Cleanup(func() { _ = firstProvisioner.Close() }) version := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, nil) coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) From 5957960369043502126f3988ae7dc788cdac4a1e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 16 May 2025 08:10:38 -0500 Subject: [PATCH 21/22] fixup error message assert --- coderd/parameters_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go index 25f6058e60a7a..e7fc77f141efc 100644 --- a/coderd/parameters_test.go +++ b/coderd/parameters_test.go @@ -151,7 +151,7 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { preview := testutil.RequireReceive(ctx, t, previews) diagCount := len(preview.Diagnostics) require.Equal(t, 1, diagCount) - require.Contains(t, preview.Diagnostics[0].Summary, "classic creation flow") + require.Contains(t, preview.Diagnostics[0].Summary, "required metadata to support dynamic parameters") require.Len(t, preview.Parameters, 1) require.Equal(t, "jetbrains_ide", preview.Parameters[0].Name) require.True(t, preview.Parameters[0].Value.Valid()) @@ -173,7 +173,7 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { preview := testutil.RequireReceive(ctx, t, previews) diagCount := len(preview.Diagnostics) require.Equal(t, 1, diagCount) - require.Contains(t, preview.Diagnostics[0].Summary, "classic creation flow") + require.Contains(t, preview.Diagnostics[0].Summary, "required metadata to support dynamic parameters") require.Len(t, preview.Parameters, 1) if exp == "Invalid" { // Try an invalid option From 62db9dde0119c1c5e7d51f37aaf46ee69d14f21a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 16 May 2025 11:32:06 -0500 Subject: [PATCH 22/22] 1.5 -> 1.6 --- coderd/parameters.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/parameters.go b/coderd/parameters.go index a452843371c3a..c3fc4ffdeeede 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -71,7 +71,7 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http major, minor, err := apiversion.Parse(tf.ProvisionerdVersion) // If the api version is not valid or less than 1.5, we need to use the static parameters - useStaticParams := err != nil || major < 1 || (major == 1 && minor < 5) + useStaticParams := err != nil || major < 1 || (major == 1 && minor < 6) if useStaticParams { api.handleStaticParameters(rw, r, templateVersion.ID) } else {