From ac7af5e9600401892bf04cede5640871763071ae Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 14 Jul 2025 10:28:47 +0000 Subject: [PATCH 1/4] feat: validate presets on template import --- coderd/dynamicparameters/error.go | 8 ++ coderd/dynamicparameters/presets.go | 24 ++++++ coderd/parameters_test.go | 27 ++++++- coderd/templateversions.go | 101 ++++++++++++++----------- coderd/templateversions_test.go | 112 ++++++++++++++++++++++++++++ go.mod | 2 + go.sum | 2 - 7 files changed, 226 insertions(+), 50 deletions(-) create mode 100644 coderd/dynamicparameters/presets.go diff --git a/coderd/dynamicparameters/error.go b/coderd/dynamicparameters/error.go index 4c27905bfa832..aeac5d237ff01 100644 --- a/coderd/dynamicparameters/error.go +++ b/coderd/dynamicparameters/error.go @@ -26,6 +26,14 @@ func tagValidationError(diags hcl.Diagnostics) *DiagnosticError { } } +func presetValidationError(diags hcl.Diagnostics) *DiagnosticError { + return &DiagnosticError{ + Message: "Unable to parse presets", + Diagnostics: diags, + KeyedDiagnostics: make(map[string]hcl.Diagnostics), + } +} + type DiagnosticError struct { // Message is the human-readable message that will be returned to the user. Message string diff --git a/coderd/dynamicparameters/presets.go b/coderd/dynamicparameters/presets.go new file mode 100644 index 0000000000000..83715f2221585 --- /dev/null +++ b/coderd/dynamicparameters/presets.go @@ -0,0 +1,24 @@ +package dynamicparameters + +import ( + "github.com/hashicorp/hcl/v2" + + "github.com/coder/preview" +) + +// CheckPresets extracts the preset related diagnostics from a template version preset +func CheckPresets(output *preview.Output, diags hcl.Diagnostics) *DiagnosticError { + de := presetValidationError(diags) + presets := output.Presets + for _, preset := range presets { + if hcl.Diagnostics(preset.Diagnostics).HasErrors() { + de.Extend(preset.Name, hcl.Diagnostics(preset.Diagnostics)) + } + } + + if de.HasError() { + return de + } + + return nil +} diff --git a/coderd/parameters_test.go b/coderd/parameters_test.go index 855d95eb1de59..56438df456fd3 100644 --- a/coderd/parameters_test.go +++ b/coderd/parameters_test.go @@ -3,6 +3,7 @@ package coderd_test import ( "context" "os" + "sync" "testing" "github.com/google/uuid" @@ -193,6 +194,7 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { t.Parallel() db, ps := dbtestutil.NewDB(t) + dbReject := &dbRejectGitSSHKey{Store: db} dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/modules/main.tf") require.NoError(t, err) @@ -200,13 +202,15 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) { require.NoError(t, err) setup := setupDynamicParamsTest(t, setupDynamicParamsTestParams{ - db: &dbRejectGitSSHKey{Store: db}, + db: dbReject, ps: ps, provisionerDaemonVersion: provProto.CurrentVersion.String(), mainTF: dynamicParametersTerraformSource, modulesArchive: modulesArchive, }) + dbReject.SetReject(true) + stream := setup.stream previews := stream.Chan() @@ -412,8 +416,25 @@ func setupDynamicParamsTest(t *testing.T, args setupDynamicParamsTestParams) dyn // that is generally impossible to force an error. type dbRejectGitSSHKey struct { database.Store + rejectMu sync.RWMutex + reject bool +} + +// SetReject toggles whether GetGitSSHKey should return an error or passthrough to the underlying store. +func (d *dbRejectGitSSHKey) SetReject(reject bool) { + d.rejectMu.Lock() + defer d.rejectMu.Unlock() + d.reject = reject } -func (*dbRejectGitSSHKey) GetGitSSHKey(_ context.Context, _ uuid.UUID) (database.GitSSHKey, error) { - return database.GitSSHKey{}, xerrors.New("forcing a fake error") +func (d *dbRejectGitSSHKey) GetGitSSHKey(ctx context.Context, userID uuid.UUID) (database.GitSSHKey, error) { + d.rejectMu.RLock() + reject := d.reject + d.rejectMu.RUnlock() + + if reject { + return database.GitSSHKey{}, xerrors.New("forcing a fake error") + } + + return d.Store.GetGitSSHKey(ctx, userID) } diff --git a/coderd/templateversions.go b/coderd/templateversions.go index fa5a7ed1fe757..6b6b7b673f255 100644 --- a/coderd/templateversions.go +++ b/coderd/templateversions.go @@ -16,6 +16,7 @@ import ( "github.com/go-chi/chi/v5" "github.com/google/uuid" + "github.com/hashicorp/hcl/v2" "github.com/moby/moby/pkg/namesgenerator" "github.com/sqlc-dev/pqtype" "golang.org/x/xerrors" @@ -1582,10 +1583,63 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht } } + var files fs.FS + switch file.Mimetype { + case "application/x-tar": + files = archivefs.FromTarReader(bytes.NewBuffer(file.Data)) + case "application/zip": + files, err = archivefs.FromZipReader(bytes.NewReader(file.Data), int64(len(file.Data))) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error reading file", + Detail: "extract zip archive: " + err.Error(), + }) + return + } + default: + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Unsupported file type", + Detail: fmt.Sprintf("Mimetype %q is not supported", file.Mimetype), + }) + return + } + ownerData, err := dynamicparameters.WorkspaceOwner(ctx, api.Database, organization.ID, apiKey.UserID) + if err != nil { + if httpapi.Is404Error(err) { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Internal error checking workspace tags", + Detail: fmt.Sprintf("Owner not found, uuid=%s", apiKey.UserID.String()), + }) + return + } + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error checking workspace tags", + Detail: "fetch owner data: " + err.Error(), + }) + return + } + + previewInput := preview.Input{ + PlanJSON: nil, // Template versions are before `terraform plan` + ParameterValues: nil, // No user-specified parameters + Owner: *ownerData, + Logger: stdslog.New(stdslog.DiscardHandler), + } + previewOutput, previewDiags := preview.Preview(ctx, previewInput, files) + + // Validate presets on template version import to avoid errors that would + // have caused workspace creation to fail: + presetErr := dynamicparameters.CheckPresets(previewOutput, nil) + if presetErr != nil { + code, resp := presetErr.Response() + httpapi.Write(ctx, rw, code, resp) + return + } + var parsedTags map[string]string var ok bool if dynamicTemplate { - parsedTags, ok = api.dynamicTemplateVersionTags(ctx, rw, organization.ID, apiKey.UserID, file) + parsedTags, ok = api.dynamicTemplateVersionTags(ctx, rw, previewOutput, previewDiags) if !ok { return } @@ -1762,50 +1816,7 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht warnings)) } -func (api *API) dynamicTemplateVersionTags(ctx context.Context, rw http.ResponseWriter, orgID uuid.UUID, owner uuid.UUID, file database.File) (map[string]string, bool) { - ownerData, err := dynamicparameters.WorkspaceOwner(ctx, api.Database, orgID, owner) - if err != nil { - if httpapi.Is404Error(err) { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Internal error checking workspace tags", - Detail: fmt.Sprintf("Owner not found, uuid=%s", owner.String()), - }) - return nil, false - } - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error checking workspace tags", - Detail: "fetch owner data: " + err.Error(), - }) - return nil, false - } - - var files fs.FS - switch file.Mimetype { - case "application/x-tar": - files = archivefs.FromTarReader(bytes.NewBuffer(file.Data)) - case "application/zip": - files, err = archivefs.FromZipReader(bytes.NewReader(file.Data), int64(len(file.Data))) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error checking workspace tags", - Detail: "extract zip archive: " + err.Error(), - }) - return nil, false - } - default: - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Unsupported file type for dynamic template version tags", - Detail: fmt.Sprintf("Mimetype %q is not supported for dynamic template version tags", file.Mimetype), - }) - return nil, false - } - - output, diags := preview.Preview(ctx, preview.Input{ - PlanJSON: nil, // Template versions are before `terraform plan` - ParameterValues: nil, // No user-specified parameters - Owner: *ownerData, - Logger: stdslog.New(stdslog.DiscardHandler), - }, files) +func (*API) dynamicTemplateVersionTags(ctx context.Context, rw http.ResponseWriter, output *preview.Output, diags hcl.Diagnostics) (map[string]string, bool) { tagErr := dynamicparameters.CheckTags(output, diags) if tagErr != nil { code, resp := tagErr.Response() diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index 1ad06bae38aee..1ef079614e993 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -641,6 +641,118 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { }) } }) + + t.Run("Presets", func(t *testing.T) { + t.Parallel() + store, ps := dbtestutil.NewDB(t) + client := coderdtest.New(t, &coderdtest.Options{ + Database: store, + Pubsub: ps, + }) + owner := coderdtest.CreateFirstUser(t, client) + templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin()) + + for _, tt := range []struct { + name string + files map[string]string + expectError string + }{ + { + name: "valid preset", + files: map[string]string{ + `main.tf`: ` + terraform { + required_providers { + coder = { + source = "coder/coder" + version = "2.8.0" + } + } + } + data "coder_parameter" "valid_parameter" { + name = "valid_parameter_name" + default = "valid_option_value" + option { + name = "valid_option_name" + value = "valid_option_value" + } + } + data "coder_workspace_preset" "valid_preset" { + name = "valid_preset" + parameters = { + "valid_parameter_name" = "valid_option_value" + } + } + `, + }, + }, + { + name: "invalid preset", + files: map[string]string{ + `main.tf`: ` + terraform { + required_providers { + coder = { + source = "coder/coder" + version = "2.8.0" + } + } + } + data "coder_parameter" "valid_parameter" { + name = "valid_parameter_name" + default = "valid_option_value" + option { + name = "valid_option_name" + value = "valid_option_value" + } + } + data "coder_workspace_preset" "invalid_parameter_name" { + name = "invalid_parameter_name" + parameters = { + "invalid_parameter_name" = "irrelevant_value" + } + } + `, + }, + expectError: "Undefined Parameter", + }, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + + // Create an archive from the files provided in the test case. + tarFile := testutil.CreateTar(t, tt.files) + + // Post the archive file + fi, err := templateAdmin.Upload(ctx, "application/x-tar", bytes.NewReader(tarFile)) + require.NoError(t, err) + + // Create a template version from the archive + tvName := testutil.GetRandomNameHyphenated(t) + tv, err := templateAdmin.CreateTemplateVersion(ctx, owner.OrganizationID, codersdk.CreateTemplateVersionRequest{ + Name: tvName, + StorageMethod: codersdk.ProvisionerStorageMethodFile, + Provisioner: codersdk.ProvisionerTypeTerraform, + FileID: fi.ID, + }) + + if tt.expectError == "" { + require.NoError(t, err) + // Assert the expected provisioner job is created from the template version import + pj, err := store.GetProvisionerJobByID(ctx, tv.Job.ID) + require.NoError(t, err) + require.NotNil(t, pj) + // Also assert that we get the expected information back from the API endpoint + require.Zero(t, tv.MatchedProvisioners.Count) + require.Zero(t, tv.MatchedProvisioners.Available) + require.Zero(t, tv.MatchedProvisioners.MostRecentlySeen.Time) + } else { + require.ErrorContains(t, err, tt.expectError) + } + }) + } + }) } func TestPatchCancelTemplateVersion(t *testing.T) { diff --git a/go.mod b/go.mod index 886515cf29dbf..3de6787f3dd3f 100644 --- a/go.mod +++ b/go.mod @@ -488,6 +488,8 @@ require ( github.com/mark3labs/mcp-go v0.32.0 ) +replace github.com/coder/preview => ../preview + require ( cel.dev/expr v0.23.0 // indirect cloud.google.com/go v0.120.0 // indirect diff --git a/go.sum b/go.sum index ded3464d585b3..a2b0dc6aa4984 100644 --- a/go.sum +++ b/go.sum @@ -916,8 +916,6 @@ github.com/coder/pq v1.10.5-0.20250630052411-a259f96b6102 h1:ahTJlTRmTogsubgRVGO github.com/coder/pq v1.10.5-0.20250630052411-a259f96b6102/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 v1.0.3-0.20250701142654-c3d6e86b9393 h1:l+m2liikn8JoEv6C22QIV4qseolUfvNsyUNA6JJsD6Y= -github.com/coder/preview v1.0.3-0.20250701142654-c3d6e86b9393/go.mod h1:efDWGlO/PZPrvdt5QiDhMtTUTkPxejXo9c0wmYYLLjM= github.com/coder/quartz v0.2.1 h1:QgQ2Vc1+mvzewg2uD/nj8MJ9p9gE+QhGJm+Z+NGnrSE= github.com/coder/quartz v0.2.1/go.mod h1:vsiCc+AHViMKH2CQpGIpFgdHIEQsxwm8yCscqKmzbRA= github.com/coder/retry v1.5.1 h1:iWu8YnD8YqHs3XwqrqsjoBTAVqT9ml6z9ViJ2wlMiqc= From 945a675ccdbadae04ce9e02820edc6657e483c85 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 14 Jul 2025 10:38:29 +0000 Subject: [PATCH 2/4] remove replace directive and update preview dependency --- go.mod | 4 +--- go.sum | 2 ++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 3de6787f3dd3f..ee9ea82811fac 100644 --- a/go.mod +++ b/go.mod @@ -483,13 +483,11 @@ require ( require ( github.com/coder/agentapi-sdk-go v0.0.0-20250505131810-560d1d88d225 github.com/coder/aisdk-go v0.0.9 - github.com/coder/preview v1.0.3-0.20250701142654-c3d6e86b9393 + github.com/coder/preview v1.0.3-0.20250713201143-17616ecf763a github.com/fsnotify/fsnotify v1.9.0 github.com/mark3labs/mcp-go v0.32.0 ) -replace github.com/coder/preview => ../preview - require ( cel.dev/expr v0.23.0 // indirect cloud.google.com/go v0.120.0 // indirect diff --git a/go.sum b/go.sum index a2b0dc6aa4984..d456eb2ed1f7f 100644 --- a/go.sum +++ b/go.sum @@ -916,6 +916,8 @@ github.com/coder/pq v1.10.5-0.20250630052411-a259f96b6102 h1:ahTJlTRmTogsubgRVGO github.com/coder/pq v1.10.5-0.20250630052411-a259f96b6102/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 v1.0.3-0.20250713201143-17616ecf763a h1:SLAUUH/lIfa15B28wU8h6Alze500HQeNYoUMB/OGE8A= +github.com/coder/preview v1.0.3-0.20250713201143-17616ecf763a/go.mod h1:efDWGlO/PZPrvdt5QiDhMtTUTkPxejXo9c0wmYYLLjM= github.com/coder/quartz v0.2.1 h1:QgQ2Vc1+mvzewg2uD/nj8MJ9p9gE+QhGJm+Z+NGnrSE= github.com/coder/quartz v0.2.1/go.mod h1:vsiCc+AHViMKH2CQpGIpFgdHIEQsxwm8yCscqKmzbRA= github.com/coder/retry v1.5.1 h1:iWu8YnD8YqHs3XwqrqsjoBTAVqT9ml6z9ViJ2wlMiqc= From c5ab3b91be411b1da9ef9956de867ea3709ed5a2 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 30 Jul 2025 12:46:24 +0000 Subject: [PATCH 3/4] minor nits --- coderd/dynamicparameters/error.go | 2 +- coderd/templateversions.go | 4 ++-- coderd/templateversions_test.go | 1 + 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/coderd/dynamicparameters/error.go b/coderd/dynamicparameters/error.go index aeac5d237ff01..ae2217936b9dd 100644 --- a/coderd/dynamicparameters/error.go +++ b/coderd/dynamicparameters/error.go @@ -28,7 +28,7 @@ func tagValidationError(diags hcl.Diagnostics) *DiagnosticError { func presetValidationError(diags hcl.Diagnostics) *DiagnosticError { return &DiagnosticError{ - Message: "Unable to parse presets", + Message: "Unable to validate presets", Diagnostics: diags, KeyedDiagnostics: make(map[string]hcl.Diagnostics), } diff --git a/coderd/templateversions.go b/coderd/templateversions.go index 3e21334cb1472..2a6e09d94978e 100644 --- a/coderd/templateversions.go +++ b/coderd/templateversions.go @@ -1821,8 +1821,8 @@ func (api *API) dynamicTemplateVersionTags(ctx context.Context, rw http.Response httpapi.Write(ctx, rw, code, resp) return nil, false } - // Validate presets on template version import to avoid errors that would - // have caused workspace creation to fail: + + // Fails early if presets are invalid to prevent downstream workspace creation errors presetErr := dynamicparameters.CheckPresets(output, nil) if presetErr != nil { code, resp := presetErr.Response() diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index dd6c6608fb969..0b5bf6fcf2302 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -728,6 +728,7 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { require.Zero(t, tv.MatchedProvisioners.MostRecentlySeen.Time) } else { require.ErrorContains(t, err, tt.expectError) + require.Equal(t, tv.Job.ID, uuid.Nil) } }) } From c86701233536b93ecdd8ab48cee897ebc94028dc Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 30 Jul 2025 12:57:18 +0000 Subject: [PATCH 4/4] protect against nilpointer exceptions in dynamic parameter functions --- coderd/dynamicparameters/presets.go | 4 ++++ coderd/dynamicparameters/tags.go | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/coderd/dynamicparameters/presets.go b/coderd/dynamicparameters/presets.go index 83715f2221585..24974962e029f 100644 --- a/coderd/dynamicparameters/presets.go +++ b/coderd/dynamicparameters/presets.go @@ -9,6 +9,10 @@ import ( // CheckPresets extracts the preset related diagnostics from a template version preset func CheckPresets(output *preview.Output, diags hcl.Diagnostics) *DiagnosticError { de := presetValidationError(diags) + if output == nil { + return de + } + presets := output.Presets for _, preset := range presets { if hcl.Diagnostics(preset.Diagnostics).HasErrors() { diff --git a/coderd/dynamicparameters/tags.go b/coderd/dynamicparameters/tags.go index 38a9bf4691571..d9037db5dd909 100644 --- a/coderd/dynamicparameters/tags.go +++ b/coderd/dynamicparameters/tags.go @@ -11,6 +11,10 @@ import ( func CheckTags(output *preview.Output, diags hcl.Diagnostics) *DiagnosticError { de := tagValidationError(diags) + if output == nil { + return de + } + failedTags := output.WorkspaceTags.UnusableTags() if len(failedTags) == 0 && !de.HasError() { return nil // No errors, all is good!