- Notifications
You must be signed in to change notification settings - Fork947
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?
Conversation
| @@ -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 comment
The 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 usestfparse to use thisfs.FS as well. That would be a small first step in getting rid of tfparse.
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.
tfparse will eventually be removed, likely in a release or two.
Maybe we should only do preset validation if using dynamic parameters. It is overloading the feature a little bit, as calling itterraform-preview or something would be more accurate.
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.
I'd like to update the classic template case that uses tfparse to use this fs.FS as well.
There is atfconfig.LoadModuleFromFilesystem but IIRC I tried using this before and ran into issues.
Maybe we should only do preset validation if using dynamic parameters.
@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.
Uh oh!
There was an error while loading.Please reload this page.
| 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 comment
The 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.
| @@ -483,7 +483,7 @@ require ( | |||
| require ( | |||
| github.com/coder/agentapi-sdk-gov0.0.0-20250505131810-560d1d88d225 | |||
| github.com/coder/aisdk-gov0.0.9 | |||
| github.com/coder/previewv1.0.3-0.20250701142654-c3d6e86b9393 | |||
| github.com/coder/previewv1.0.3-0.20250713201143-17616ecf763a | |||
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.
NB: We must not merge this PR until we've mergedcoder/preview#149 and updated this dependency in kind.
| @@ -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 comment
The reason will be displayed to describe this comment to others.Learn more.
tfparse will eventually be removed, likely in a release or two.
Maybe we should only do preset validation if using dynamic parameters. It is overloading the feature a little bit, as calling itterraform-preview or something would be more accurate.
| 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), | ||
| } | ||
| 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) | ||
| ifpresetErr!=nil { | ||
| code,resp:=presetErr.Response() | ||
| httpapi.Write(ctx,rw,code,resp) | ||
| return | ||
| } | ||
| varparsedTagsmap[string]string | ||
| varokbool | ||
| ifdynamicTemplate { | ||
| parsedTags,ok=api.dynamicTemplateVersionTags(ctx,rw,organization.ID,apiKey.UserID,file) | ||
| parsedTags,ok=api.dynamicTemplateVersionTags(ctx,rw,previewOutput,previewDiags) |
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.
Could we leave this extracted to its own function and just rename it with different outputs?
I ask because thispostTemplateVersionsByOrganization is kind of long already. And ideally, we only usepreview ifDynamicParameters is enabled on the template. Since the usage ofpreview is still in Beta.
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 comment
The reason will be displayed to describe this comment to others.Learn more.
do you feel strongly that the only public interface of preview should bepreview.Preview()? Some of this discomfort that you're describing here is imo a result of the preview API being too course. If we split Params, Tags and Presets into three separate functions in the preview project then we have the granular control we need to be able to decouple things and leave most of this logic down where it was.
A few customers and users are already running into preset validation issues and it breaks their workspaces. I'd prefer to get preset validation in for classic templates if we can. |
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.
LGTM 🎉
Just some small nits.
| @@ -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 comment
The reason will be displayed to describe this comment to others.Learn more.
nit:
| Message:"Unable toparse presets", | |
| Message:"Unable tovalidate presets", |
| // Validate presets on template version import to avoid errors that would | ||
| // have caused workspace creation to fail: |
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.
small nit:
| // 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 |
| 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 comment
The reason will be displayed to describe this comment to others.Learn more.
Should we check that no provisioner job was created?
| // Validate presets on template version import to avoid errors that would | ||
| // have caused workspace creation to fail: | ||
| presetErr:=dynamicparameters.CheckPresets(previewOutput,nil) |
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.
Is there any case where we callCheckPresets with ahcl.Diagnostics? Should we maybe remove this parameter?
@SasSwart I just fear a bug in preview could then break existing templates. We had some parsing bugs that prevented some templates from working. |
| rejectMu sync.RWMutex | ||
| rejectbool | ||
| } | ||
| // SetReject toggles whether GetGitSSHKey should return an error or passthrough to the underlying store. | ||
| func (d*dbRejectGitSSHKey)SetReject(rejectbool) { | ||
| d.rejectMu.Lock() | ||
| deferd.rejectMu.Unlock() | ||
| d.reject=reject |
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: Is this a fix for a separate flake? If so, might be no harm to separate in its own PR.
| @@ -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 comment
The reason will be displayed to describe this comment to others.Learn more.
I'd like to update the classic template case that uses tfparse to use this fs.FS as well.
There is atfconfig.LoadModuleFromFilesystem but IIRC I tried using this before and ran into issues.
Maybe we should only do preset validation if using dynamic parameters.
@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.
Uh oh!
There was an error while loading.Please reload this page.
Typos and other errors often result in invalid presets in a template. Coder would import these broken templates and present them to users when they create workspaces. An unsuspecting user who chooses a broken preset would then experience a failed workspace build with no obvious error message.
This PR adds additional validation beyond what is possible in the Terraform provider schema. Coder will now present a more helpful error message to template authors when they upload a new template version:
The frontend warning is less helpful right now, but I'd like to address that in a follow-up since I need frontend help:
closes#17333