- 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 from1 commit
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
- Loading branch information
Uh oh!
There was an error while loading.Please reload this page.
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 { | ||||||
| } | ||||||
| } | ||||||
| funcpresetValidationError(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), | ||||||
| } | ||||||
| } | ||||||
| typeDiagnosticErrorstruct { | ||||||
| // Message is the human-readable message that will be returned to the user. | ||||||
| Messagestring | ||||||
| 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 |
|---|---|---|
| @@ -3,6 +3,7 @@ package coderd_test | ||
| import ( | ||
| "context" | ||
| "os" | ||
| "sync" | ||
| "testing" | ||
| "github.com/google/uuid" | ||
| @@ -193,20 +194,23 @@ 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) | ||
| modulesArchive, err := terraform.GetModulesArchive(os.DirFS("testdata/parameters/modules")) | ||
| require.NoError(t, err) | ||
| setup := setupDynamicParamsTest(t, setupDynamicParamsTestParams{ | ||
| 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 | ||
Comment on lines +419 to +427 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: Is this a fix for a separate flake? If so, might be no harm to separate in its own PR. | ||
| } | ||
| 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) | ||
| } | ||
| 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 | ||||||||
| } | ||||||||
| } | ||||||||
| varfiles 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 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.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @johnstcn If there is a bug in | ||||||||
| switchfile.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))) | ||||||||
| iferr!=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) | ||||||||
| iferr!=nil { | ||||||||
| ifhttpapi.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 | ||||||||
| ifpresetErr!=nil { | ||||||||
| code,resp:=presetErr.Response() | ||||||||
| httpapi.Write(ctx,rw,code,resp) | ||||||||
| return | ||||||||
| } | ||||||||
| varparsedTagsmap[string]string | ||||||||
| varokbool | ||||||||
| ifdynamicTemplate { | ||||||||
| 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 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. @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) | ||||||||
| iftagErr!=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. | ||
| 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. 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? | ||
| } | ||
| }) | ||
| } | ||
| }) | ||
| } | ||
| func TestPatchCancelTemplateVersion(t *testing.T) { | ||
Uh oh!
There was an error while loading.Please reload this page.