Skip to content

Commit ac7af5e

Browse files
SasSwartSasSwart
committed
feat: validate presets on template import
1 parent 3126f21 commit ac7af5e

File tree

7 files changed

+226
-50
lines changed

7 files changed

+226
-50
lines changed

‎coderd/dynamicparameters/error.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,14 @@ func tagValidationError(diags hcl.Diagnostics) *DiagnosticError {
2626
}
2727
}
2828

29+
func presetValidationError(diags hcl.Diagnostics) *DiagnosticError {
30+
return &DiagnosticError{
31+
Message: "Unable to parse presets",
32+
Diagnostics: diags,
33+
KeyedDiagnostics: make(map[string]hcl.Diagnostics),
34+
}
35+
}
36+
2937
type DiagnosticError struct {
3038
// Message is the human-readable message that will be returned to the user.
3139
Message string

‎coderd/dynamicparameters/presets.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
package dynamicparameters
2+
3+
import (
4+
"github.com/hashicorp/hcl/v2"
5+
6+
"github.com/coder/preview"
7+
)
8+
9+
// CheckPresets extracts the preset related diagnostics from a template version preset
10+
func CheckPresets(output *preview.Output, diags hcl.Diagnostics) *DiagnosticError {
11+
de := presetValidationError(diags)
12+
presets := output.Presets
13+
for _, preset := range presets {
14+
if hcl.Diagnostics(preset.Diagnostics).HasErrors() {
15+
de.Extend(preset.Name, hcl.Diagnostics(preset.Diagnostics))
16+
}
17+
}
18+
19+
if de.HasError() {
20+
return de
21+
}
22+
23+
return nil
24+
}

‎coderd/parameters_test.go

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package coderd_test
33
import (
44
"context"
55
"os"
6+
"sync"
67
"testing"
78

89
"github.com/google/uuid"
@@ -193,20 +194,23 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) {
193194
t.Parallel()
194195

195196
db, ps := dbtestutil.NewDB(t)
197+
dbReject := &dbRejectGitSSHKey{Store: db}
196198
dynamicParametersTerraformSource, err := os.ReadFile("testdata/parameters/modules/main.tf")
197199
require.NoError(t, err)
198200

199201
modulesArchive, err := terraform.GetModulesArchive(os.DirFS("testdata/parameters/modules"))
200202
require.NoError(t, err)
201203

202204
setup := setupDynamicParamsTest(t, setupDynamicParamsTestParams{
203-
db: &dbRejectGitSSHKey{Store: db},
205+
db: dbReject,
204206
ps: ps,
205207
provisionerDaemonVersion: provProto.CurrentVersion.String(),
206208
mainTF: dynamicParametersTerraformSource,
207209
modulesArchive: modulesArchive,
208210
})
209211

212+
dbReject.SetReject(true)
213+
210214
stream := setup.stream
211215
previews := stream.Chan()
212216

@@ -412,8 +416,25 @@ func setupDynamicParamsTest(t *testing.T, args setupDynamicParamsTestParams) dyn
412416
// that is generally impossible to force an error.
413417
type dbRejectGitSSHKey struct {
414418
database.Store
419+
rejectMu sync.RWMutex
420+
reject bool
421+
}
422+
423+
// SetReject toggles whether GetGitSSHKey should return an error or passthrough to the underlying store.
424+
func (d *dbRejectGitSSHKey) SetReject(reject bool) {
425+
d.rejectMu.Lock()
426+
defer d.rejectMu.Unlock()
427+
d.reject = reject
415428
}
416429

417-
func (*dbRejectGitSSHKey) GetGitSSHKey(_ context.Context, _ uuid.UUID) (database.GitSSHKey, error) {
418-
return database.GitSSHKey{}, xerrors.New("forcing a fake error")
430+
func (d *dbRejectGitSSHKey) GetGitSSHKey(ctx context.Context, userID uuid.UUID) (database.GitSSHKey, error) {
431+
d.rejectMu.RLock()
432+
reject := d.reject
433+
d.rejectMu.RUnlock()
434+
435+
if reject {
436+
return database.GitSSHKey{}, xerrors.New("forcing a fake error")
437+
}
438+
439+
return d.Store.GetGitSSHKey(ctx, userID)
419440
}

‎coderd/templateversions.go

Lines changed: 56 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616

1717
"github.com/go-chi/chi/v5"
1818
"github.com/google/uuid"
19+
"github.com/hashicorp/hcl/v2"
1920
"github.com/moby/moby/pkg/namesgenerator"
2021
"github.com/sqlc-dev/pqtype"
2122
"golang.org/x/xerrors"
@@ -1582,10 +1583,63 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht
15821583
}
15831584
}
15841585

1586+
var files fs.FS
1587+
switch file.Mimetype {
1588+
case "application/x-tar":
1589+
files = archivefs.FromTarReader(bytes.NewBuffer(file.Data))
1590+
case "application/zip":
1591+
files, err = archivefs.FromZipReader(bytes.NewReader(file.Data), int64(len(file.Data)))
1592+
if err != nil {
1593+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
1594+
Message: "Internal error reading file",
1595+
Detail: "extract zip archive: " + err.Error(),
1596+
})
1597+
return
1598+
}
1599+
default:
1600+
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
1601+
Message: "Unsupported file type",
1602+
Detail: fmt.Sprintf("Mimetype %q is not supported", file.Mimetype),
1603+
})
1604+
return
1605+
}
1606+
ownerData, err := dynamicparameters.WorkspaceOwner(ctx, api.Database, organization.ID, apiKey.UserID)
1607+
if err != nil {
1608+
if httpapi.Is404Error(err) {
1609+
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
1610+
Message: "Internal error checking workspace tags",
1611+
Detail: fmt.Sprintf("Owner not found, uuid=%s", apiKey.UserID.String()),
1612+
})
1613+
return
1614+
}
1615+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
1616+
Message: "Internal error checking workspace tags",
1617+
Detail: "fetch owner data: " + err.Error(),
1618+
})
1619+
return
1620+
}
1621+
1622+
previewInput := preview.Input{
1623+
PlanJSON: nil, // Template versions are before `terraform plan`
1624+
ParameterValues: nil, // No user-specified parameters
1625+
Owner: *ownerData,
1626+
Logger: stdslog.New(stdslog.DiscardHandler),
1627+
}
1628+
previewOutput, previewDiags := preview.Preview(ctx, previewInput, files)
1629+
1630+
// Validate presets on template version import to avoid errors that would
1631+
// have caused workspace creation to fail:
1632+
presetErr := dynamicparameters.CheckPresets(previewOutput, nil)
1633+
if presetErr != nil {
1634+
code, resp := presetErr.Response()
1635+
httpapi.Write(ctx, rw, code, resp)
1636+
return
1637+
}
1638+
15851639
var parsedTags map[string]string
15861640
var ok bool
15871641
if dynamicTemplate {
1588-
parsedTags, ok = api.dynamicTemplateVersionTags(ctx, rw, organization.ID, apiKey.UserID, file)
1642+
parsedTags, ok = api.dynamicTemplateVersionTags(ctx, rw, previewOutput, previewDiags)
15891643
if !ok {
15901644
return
15911645
}
@@ -1762,50 +1816,7 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht
17621816
warnings))
17631817
}
17641818

1765-
func (api *API) dynamicTemplateVersionTags(ctx context.Context, rw http.ResponseWriter, orgID uuid.UUID, owner uuid.UUID, file database.File) (map[string]string, bool) {
1766-
ownerData, err := dynamicparameters.WorkspaceOwner(ctx, api.Database, orgID, owner)
1767-
if err != nil {
1768-
if httpapi.Is404Error(err) {
1769-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
1770-
Message: "Internal error checking workspace tags",
1771-
Detail: fmt.Sprintf("Owner not found, uuid=%s", owner.String()),
1772-
})
1773-
return nil, false
1774-
}
1775-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
1776-
Message: "Internal error checking workspace tags",
1777-
Detail: "fetch owner data: " + err.Error(),
1778-
})
1779-
return nil, false
1780-
}
1781-
1782-
var files fs.FS
1783-
switch file.Mimetype {
1784-
case "application/x-tar":
1785-
files = archivefs.FromTarReader(bytes.NewBuffer(file.Data))
1786-
case "application/zip":
1787-
files, err = archivefs.FromZipReader(bytes.NewReader(file.Data), int64(len(file.Data)))
1788-
if err != nil {
1789-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
1790-
Message: "Internal error checking workspace tags",
1791-
Detail: "extract zip archive: " + err.Error(),
1792-
})
1793-
return nil, false
1794-
}
1795-
default:
1796-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
1797-
Message: "Unsupported file type for dynamic template version tags",
1798-
Detail: fmt.Sprintf("Mimetype %q is not supported for dynamic template version tags", file.Mimetype),
1799-
})
1800-
return nil, false
1801-
}
1802-
1803-
output, diags := preview.Preview(ctx, preview.Input{
1804-
PlanJSON: nil, // Template versions are before `terraform plan`
1805-
ParameterValues: nil, // No user-specified parameters
1806-
Owner: *ownerData,
1807-
Logger: stdslog.New(stdslog.DiscardHandler),
1808-
}, files)
1819+
func (*API) dynamicTemplateVersionTags(ctx context.Context, rw http.ResponseWriter, output *preview.Output, diags hcl.Diagnostics) (map[string]string, bool) {
18091820
tagErr := dynamicparameters.CheckTags(output, diags)
18101821
if tagErr != nil {
18111822
code, resp := tagErr.Response()

‎coderd/templateversions_test.go

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -641,6 +641,118 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) {
641641
})
642642
}
643643
})
644+
645+
t.Run("Presets", func(t *testing.T) {
646+
t.Parallel()
647+
store, ps := dbtestutil.NewDB(t)
648+
client := coderdtest.New(t, &coderdtest.Options{
649+
Database: store,
650+
Pubsub: ps,
651+
})
652+
owner := coderdtest.CreateFirstUser(t, client)
653+
templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin())
654+
655+
for _, tt := range []struct {
656+
name string
657+
files map[string]string
658+
expectError string
659+
}{
660+
{
661+
name: "valid preset",
662+
files: map[string]string{
663+
`main.tf`: `
664+
terraform {
665+
required_providers {
666+
coder = {
667+
source = "coder/coder"
668+
version = "2.8.0"
669+
}
670+
}
671+
}
672+
data "coder_parameter" "valid_parameter" {
673+
name = "valid_parameter_name"
674+
default = "valid_option_value"
675+
option {
676+
name = "valid_option_name"
677+
value = "valid_option_value"
678+
}
679+
}
680+
data "coder_workspace_preset" "valid_preset" {
681+
name = "valid_preset"
682+
parameters = {
683+
"valid_parameter_name" = "valid_option_value"
684+
}
685+
}
686+
`,
687+
},
688+
},
689+
{
690+
name: "invalid preset",
691+
files: map[string]string{
692+
`main.tf`: `
693+
terraform {
694+
required_providers {
695+
coder = {
696+
source = "coder/coder"
697+
version = "2.8.0"
698+
}
699+
}
700+
}
701+
data "coder_parameter" "valid_parameter" {
702+
name = "valid_parameter_name"
703+
default = "valid_option_value"
704+
option {
705+
name = "valid_option_name"
706+
value = "valid_option_value"
707+
}
708+
}
709+
data "coder_workspace_preset" "invalid_parameter_name" {
710+
name = "invalid_parameter_name"
711+
parameters = {
712+
"invalid_parameter_name" = "irrelevant_value"
713+
}
714+
}
715+
`,
716+
},
717+
expectError: "Undefined Parameter",
718+
},
719+
} {
720+
t.Run(tt.name, func(t *testing.T) {
721+
t.Parallel()
722+
ctx := testutil.Context(t, testutil.WaitShort)
723+
724+
// Create an archive from the files provided in the test case.
725+
tarFile := testutil.CreateTar(t, tt.files)
726+
727+
// Post the archive file
728+
fi, err := templateAdmin.Upload(ctx, "application/x-tar", bytes.NewReader(tarFile))
729+
require.NoError(t, err)
730+
731+
// Create a template version from the archive
732+
tvName := testutil.GetRandomNameHyphenated(t)
733+
tv, err := templateAdmin.CreateTemplateVersion(ctx, owner.OrganizationID, codersdk.CreateTemplateVersionRequest{
734+
Name: tvName,
735+
StorageMethod: codersdk.ProvisionerStorageMethodFile,
736+
Provisioner: codersdk.ProvisionerTypeTerraform,
737+
FileID: fi.ID,
738+
})
739+
740+
if tt.expectError == "" {
741+
require.NoError(t, err)
742+
// Assert the expected provisioner job is created from the template version import
743+
pj, err := store.GetProvisionerJobByID(ctx, tv.Job.ID)
744+
require.NoError(t, err)
745+
require.NotNil(t, pj)
746+
// Also assert that we get the expected information back from the API endpoint
747+
require.Zero(t, tv.MatchedProvisioners.Count)
748+
require.Zero(t, tv.MatchedProvisioners.Available)
749+
require.Zero(t, tv.MatchedProvisioners.MostRecentlySeen.Time)
750+
} else {
751+
require.ErrorContains(t, err, tt.expectError)
752+
}
753+
})
754+
}
755+
})
644756
}
645757

646758
func TestPatchCancelTemplateVersion(t *testing.T) {

‎go.mod

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -488,6 +488,8 @@ require (
488488
github.com/mark3labs/mcp-go v0.32.0
489489
)
490490

491+
replace github.com/coder/preview => ../preview
492+
491493
require (
492494
cel.dev/expr v0.23.0 // indirect
493495
cloud.google.com/go v0.120.0 // indirect

‎go.sum

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -916,8 +916,6 @@ github.com/coder/pq v1.10.5-0.20250630052411-a259f96b6102 h1:ahTJlTRmTogsubgRVGO
916916
github.com/coder/pq v1.10.5-0.20250630052411-a259f96b6102/go.mod h1:AlVN5x4E4T544tWzH6hKfbfQvm3HdbOxrmggDNAPY9o=
917917
github.com/coder/pretty v0.0.0-20230908205945-e89ba86370e0 h1:3A0ES21Ke+FxEM8CXx9n47SZOKOpgSE1bbJzlE4qPVs=
918918
github.com/coder/pretty v0.0.0-20230908205945-e89ba86370e0/go.mod h1:5UuS2Ts+nTToAMeOjNlnHFkPahrtDkmpydBen/3wgZc=
919-
github.com/coder/preview v1.0.3-0.20250701142654-c3d6e86b9393 h1:l+m2liikn8JoEv6C22QIV4qseolUfvNsyUNA6JJsD6Y=
920-
github.com/coder/preview v1.0.3-0.20250701142654-c3d6e86b9393/go.mod h1:efDWGlO/PZPrvdt5QiDhMtTUTkPxejXo9c0wmYYLLjM=
921919
github.com/coder/quartz v0.2.1 h1:QgQ2Vc1+mvzewg2uD/nj8MJ9p9gE+QhGJm+Z+NGnrSE=
922920
github.com/coder/quartz v0.2.1/go.mod h1:vsiCc+AHViMKH2CQpGIpFgdHIEQsxwm8yCscqKmzbRA=
923921
github.com/coder/retry v1.5.1 h1:iWu8YnD8YqHs3XwqrqsjoBTAVqT9ml6z9ViJ2wlMiqc=

0 commit comments

Comments
 (0)