feat: validate presets on template import #18844

SasSwart · 2025-07-14T10:29:28Z

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

SasSwart · 2025-07-14T10:45:15Z

coderd/templateversions.go

@@ -1582,10 +1583,63 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht
 		}
 	}

+	var files fs.FS


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 tfparse to use this fs.FS as well. That would be a small first step in getting rid of tfparse.

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 it terraform-preview or something would be more accurate.

I'd like to update the classic template case that uses tfparse to use this fs.FS as well.

There is a tfconfig.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.

@johnstcn preview is a large part of the dynamic parameters feature. It just feels premature to push preview to GA for this parsing, while the rest of its use is still getting tested in Beta.

If there is a bug in preview, we will know when Beta is in the wild

coderd/templateversions.go

SasSwart · 2025-07-14T10:46:50Z

coderd/templateversions_test.go

+		owner := coderdtest.CreateFirstUser(t, client)
+		templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin())
+
+		for _, tt := range []struct {


The different failure cases are more exhaustively tested in coder/preview#149, so I'm only doing a happy and sad path here.

SasSwart · 2025-07-14T10:47:54Z

go.mod

@@ -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.20250701142654-c3d6e86b9393
+	github.com/coder/preview v1.0.3-0.20250713201143-17616ecf763a


NB: We must not merge this PR until we've merged coder/preview#149 and updated this dependency in kind.

Emyrk · 2025-07-14T15:51:37Z

coderd/templateversions.go

@@ -1582,10 +1583,63 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht
 		}
 	}

+	var files fs.FS


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 it terraform-preview or something would be more accurate.

Emyrk · 2025-07-14T15:58:57Z

coderd/templateversions.go

+	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),
+	}
+	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)
+	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, organization.ID, apiKey.UserID, file)
+		parsedTags, ok = api.dynamicTemplateVersionTags(ctx, rw, previewOutput, previewDiags)


Could we leave this extracted to its own function and just rename it with different outputs?

I ask because this postTemplateVersionsByOrganization is kind of long already. And ideally, we only use preview if DynamicParameters is enabled on the template. Since the usage of preview 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)

do you feel strongly that the only public interface of preview should be preview.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.

@SasSwart We might want to split it. If we can keep it as a single preview.Preview, that is ideal. But I understand what you are saying.

SasSwart · 2025-07-14T17:14:45Z

Maybe we should only do preset validation if using dynamic parameters

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.

ssncferreira

LGTM 🎉
Just some small nits.

ssncferreira · 2025-07-15T11:12:55Z

coderd/dynamicparameters/error.go

@@ -26,6 +26,14 @@ func tagValidationError(diags hcl.Diagnostics) *DiagnosticError {
 	}
 }

+func presetValidationError(diags hcl.Diagnostics) *DiagnosticError {
+	return &DiagnosticError{
+		Message:          "Unable to parse presets",


nit:

Suggested change

Message: "Unable to parse presets",

Message: "Unable to validate presets",

ssncferreira · 2025-07-15T11:19:29Z

coderd/templateversions.go

+	// Validate presets on template version import to avoid errors that would
+	// have caused workspace creation to fail:


small nit:

Suggested change

// 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

ssncferreira · 2025-07-15T11:24:58Z

coderd/templateversions_test.go

+					require.Zero(t, tv.MatchedProvisioners.Available)
+					require.Zero(t, tv.MatchedProvisioners.MostRecentlySeen.Time)
+				} else {
+					require.ErrorContains(t, err, tt.expectError)


Should we check that no provisioner job was created?

ssncferreira · 2025-07-15T11:29:30Z

coderd/templateversions.go

+
+	// Validate presets on template version import to avoid errors that would
+	// have caused workspace creation to fail:
+	presetErr := dynamicparameters.CheckPresets(previewOutput, nil)


Is there any case where we call CheckPresets with a hcl.Diagnostics? Should we maybe remove this parameter?

Emyrk · 2025-07-15T17:17:32Z

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.

@SasSwart I just fear a bug in preview could then break existing templates. We had some parsing bugs that prevented some templates from working.

johnstcn · 2025-07-20T16:18:23Z

coderd/parameters_test.go

+	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


nit: Is this a fix for a separate flake? If so, might be no harm to separate in its own PR.

johnstcn · 2025-07-20T16:33:10Z

coderd/templateversions.go

@@ -1582,10 +1583,63 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht
 		}
 	}

+	var files fs.FS


I'd like to update the classic template case that uses tfparse to use this fs.FS as well.

There is a tfconfig.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.

feat: validate presets on template import

ac7af5e

github-actions bot assigned SasSwart Jul 14, 2025

remove replace directive and update preview dependency

945a675

SasSwart commented Jul 14, 2025

View reviewed changes

SasSwart requested review from johnstcn, Emyrk and ssncferreira July 14, 2025 10:48

SasSwart marked this pull request as ready for review July 14, 2025 10:51

Emyrk reviewed Jul 14, 2025

View reviewed changes

ssncferreira approved these changes Jul 15, 2025

View reviewed changes

johnstcn reviewed Jul 20, 2025

View reviewed changes

	Message: "Unable to parse presets",
	Message: "Unable to validate presets",

		// Validate presets on template version import to avoid errors that would
		// have caused workspace creation to fail:

	// 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

feat: validate presets on template import #18844

Are you sure you want to change the base?

feat: validate presets on template import #18844

Conversation

SasSwart commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SasSwart commented Jul 14, 2025

Uh oh!

ssncferreira left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Emyrk commented Jul 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SasSwart commented Jul 14, 2025 •

edited

Loading