- Notifications
You must be signed in to change notification settings - Fork948
feat: validate presets on template import#18844
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -26,6 +26,14 @@ func tagValidationError(diags hcl.Diagnostics) *DiagnosticError { | ||||||
| } | ||||||
| } | ||||||
| func presetValidationError(diags hcl.Diagnostics) *DiagnosticError { | ||||||
| return &DiagnosticError{ | ||||||
| Message: "Unable to parse presets", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. nit: Suggested change
| ||||||
| 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 | ||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more.
Maybe we should only do preset validation if using dynamic parameters. It is overloading the feature a little bit, as calling it | ||||||||
| 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 resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||||||||
| } | ||||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. small nit: Suggested change
| ||||||||
| presetErr := dynamicparameters.CheckPresets(previewOutput, nil) | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. do you feel strongly that the only public interface of preview should be | ||||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. The different failure cases are more exhaustively tested incoder/preview#149, so I'm only doing a happy and sad path here. | ||
| namestring | ||
| filesmap[string]string | ||
| expectErrorstring | ||
| }{ | ||
| { | ||
| 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, | ||
| }) | ||
| iftt.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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Should we check that no provisioner job was created? | ||
| } | ||
| }) | ||
| } | ||
| }) | ||
| } | ||
| funcTestPatchCancelTemplateVersion(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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. NB: We must not merge this PR until we've mergedcoder/preview#149 and updated this dependency in kind. | ||
| github.com/fsnotify/fsnotify v1.9.0 | ||
| github.com/mark3labs/mcp-go v0.32.0 | ||
| ) | ||
Uh oh!
There was an error while loading.Please reload this page.