feat: validate presets on template import #18844
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -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 | ||||||||
|
There was a problem hiding this comment. This was previously only necessary for dynamic parameters, but we now need it in both dynamic and classic templates because we validate presets in both cases. It feels a tad wasteful if considered in isolation, but I'd like to update the classic template case that uses There was a problem hiding this comment.
Maybe we should only do preset validation if using dynamic parameters. It is overloading the feature a little bit, as calling it There was a problem hiding this comment.
There is a
@Emyrk what potential issues do we avoid by doing this? Tying a behaviour relating to a 'GA' feature to a per-template 'Beta' feature feels like it would be unexpected. There was a problem hiding this comment. @johnstcn If there is a bug in |
||||||||
| 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), | ||||||||
SasSwart marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| } | ||||||||
| previewOutput, previewDiags := preview.Preview(ctx, previewInput, files) | ||||||||
|
|
||||||||
| // Validate presets on template version import to avoid errors that would | ||||||||
| // have caused workspace creation to fail: | ||||||||
|
Comment on lines
+1630
to
+1631
There was a problem hiding this comment. small nit:
Suggested change
|
||||||||
| presetErr := dynamicparameters.CheckPresets(previewOutput, nil) | ||||||||
|
There was a problem hiding this comment. Is there any case where we call |
||||||||
| 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, previewOutput, previewDiags) | ||||||||
|
Comment on lines
+1587
to
+1642
There was a problem hiding this comment. Could we leave this extracted to its own function and just rename it with different outputs? I ask because this func (api *API) templatePreview(ctx context.Context, rw http.ResponseWriter, orgID uuid.UUID, owner uuid.UUID, file database.File) (ParsedOutput, bool)There was a problem hiding this comment. do you feel strongly that the only public interface of preview should be There was a problem hiding this comment. @SasSwart We might want to split it. If we can keep it as a single |
||||||||
| if !ok { | ||||||||
| return | ||||||||
| } | ||||||||
|
|
@@ -1762,50 +1816,7 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht | |||||||
| warnings)) | ||||||||
| } | ||||||||
|
|
||||||||
| 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() | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 { | ||
|
There was a problem hiding this comment. The different failure cases are more exhaustively tested in coder/preview#149, so I'm only doing a happy and sad path here. |
||
| 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) | ||
|
There was a problem hiding this comment. Should we check that no provisioner job was created? |
||
| } | ||
| }) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestPatchCancelTemplateVersion(t *testing.T) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -483,7 +483,7 @@ 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.20250713201143-17616ecf763a | ||
|
There was a problem hiding this comment. NB: We must not merge this PR until we've merged coder/preview#149 and updated this dependency in kind. |
||
| github.com/fsnotify/fsnotify v1.9.0 | ||
| github.com/mark3labs/mcp-go v0.32.0 | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: