Improve template manager/generation

cmd/garm-cli/cmd/templates.go

@@ -14,6 +14,8 @@
 package cmd
 
 import (
+	"bufio"
+	"errors"
 	"fmt"
 	"os"
 	"os/exec"
@@ -23,6 +25,7 @@ import (
 	"github.com/spf13/cobra"
 
 	commonParams "github.com/cloudbase/garm-provider-common/params"
+	apiserverParams "github.com/cloudbase/garm/apiserver/params"
 	apiTemplates "github.com/cloudbase/garm/client/templates"
 	"github.com/cloudbase/garm/cmd/garm-cli/common"
 	"github.com/cloudbase/garm/cmd/garm-cli/editor"
@@ -446,24 +449,40 @@ is opened in $EDITOR instead.`,
 			updateReq := apiTemplates.NewUpdateTemplateParams()
 			updateReq.TemplateID = float64(response.Payload.ID)
 			updateReq.Body.Data = []byte(text)
-			_, err := apiCli.Templates.UpdateTemplate(updateReq, authToken)
-			return err
+			if _, err := apiCli.Templates.UpdateTemplate(updateReq, authToken); err != nil {
+				// Surface the API's error detail (e.g. the template parse error)
+				// rather than the raw go-swagger response blob. The built-in
+				// editor reports this in place, keeping the editor open to fix.
+				return errors.New(templateAPIErrorMessage(err))
+			}
+			return nil
 		}
 
 		if templateEditExternal {
-			newContent, changed, err := editWithExternalEditor(original, string(response.Payload.OSType))
-			if err != nil {
-				return err
-			}
-			if !changed {
-				fmt.Println("no changes made")
+			// The external editor closes after each pass, so on a failed save we
+			// show the error and offer to reopen with the user's edits intact.
+			toEdit := original
+			for {
+				newContent, _, err := editWithExternalEditor(toEdit, string(response.Payload.OSType))
+				if err != nil {
+					return err
+				}
+				if newContent == original {
+					fmt.Println("no changes made")
+					return nil
+				}
+				if err := updateTemplate(newContent); err != nil {
+					fmt.Fprintf(os.Stderr, "\ntemplate not saved: %s\nHit enter to retry, ctrl-c to cancel edit ", err)
+					if _, rerr := bufio.NewReader(os.Stdin).ReadString('\n'); rerr != nil {
+						// stdin closed (e.g. non-interactive) — nothing to retry with.
+						return errors.New("template not saved")
+					}
+					toEdit = newContent
+					continue
+				}
+				fmt.Println("changes saved successfully")
 				return nil
 			}
-			if err := updateTemplate(newContent); err != nil {
-				return fmt.Errorf("failed to update template: %s", err)
-			}
-			fmt.Println("changes saved successfully")
-			return nil
 		}
 
 		ed := editor.NewEditor()
@@ -541,6 +560,36 @@ func editWithExternalEditor(content, osType string) (string, bool, error) {
 	return string(edited), string(edited) != content, nil
 }
 
+// templateAPIErrorMessage extracts the human-readable message from a template
+// API error, preferring the structured APIErrorResponse detail (already
+// unmarshaled by the client) over the raw go-swagger response string.
+func templateAPIErrorMessage(err error) string {
+	var updateErr *apiTemplates.UpdateTemplateDefault
+	if errors.As(err, &updateErr) {
+		if msg := apiErrorResponseMessage(updateErr.Payload); msg != "" {
+			return msg
+		}
+	}
+	var createErr *apiTemplates.CreateTemplateDefault
+	if errors.As(err, &createErr) {
+		if msg := apiErrorResponseMessage(createErr.Payload); msg != "" {
+			return msg
+		}
+	}
+	return err.Error()
+}
+
+func apiErrorResponseMessage(p apiserverParams.APIErrorResponse) string {
+	switch {
+	case p.Details != "":
+		return p.Details
+	case p.Error != "":
+		return p.Error
+	default:
+		return ""
+	}
+}
+
 func init() {
 	templateCreateCmd.Flags().StringVar(&templateName, "name", "", "Name of the template.")
 	templateCreateCmd.Flags().StringVar(&templateDescription, "description", "", "Template description.")

go.mod

@@ -6,7 +6,7 @@ require (
 	github.com/BurntSushi/toml v1.6.0
 	github.com/alecthomas/chroma/v2 v2.27.0
 	github.com/bradleyfalzon/ghinstallation/v2 v2.19.0
-	github.com/cloudbase/garm-provider-common v0.1.10-0.20260627162627-e00d7529cc6f
+	github.com/cloudbase/garm-provider-common v0.1.10-0.20260707095006-33396540b57b
 	github.com/felixge/httpsnoop v1.1.0
 	github.com/gdamore/tcell/v2 v2.13.10
 	github.com/go-gormigrate/gormigrate/v2 v2.1.6
@@ -106,6 +106,7 @@ require (
 	go.opentelemetry.io/otel/metric v1.44.0 // indirect
 	go.opentelemetry.io/otel/trace v1.44.0 // indirect
 	go.yaml.in/yaml/v3 v3.0.4 // indirect
+	go.yaml.in/yaml/v4 v4.0.0-rc.6 // indirect
 	golang.org/x/net v0.56.0 // indirect
 	golang.org/x/sys v0.46.0 // indirect
 	golang.org/x/text v0.38.0 // indirect

go.sum

@@ -25,8 +25,8 @@ github.com/chzyer/test v1.0.0 h1:p3BQDXSxOhOG0P9z6/hGnII4LGiEPOYBhs8asl/fC04=
 github.com/chzyer/test v1.0.0/go.mod h1:2JlltgoNkt4TW/z9V/IzDdFaMTM2JPIi26O1pF38GC8=
 github.com/clipperhouse/uax29/v2 v2.7.0 h1:+gs4oBZ2gPfVrKPthwbMzWZDaAFPGYK72F0NJv2v7Vk=
 github.com/clipperhouse/uax29/v2 v2.7.0/go.mod h1:EFJ2TJMRUaplDxHKj1qAEhCtQPW2tJSwu5BF98AuoVM=
-github.com/cloudbase/garm-provider-common v0.1.10-0.20260627162627-e00d7529cc6f h1:IalGGcSKBGUd+KFFn2OXO6SMcODyCkSO/SMXtm4nwy8=
-github.com/cloudbase/garm-provider-common v0.1.10-0.20260627162627-e00d7529cc6f/go.mod h1:i1KXJVzi5ouzbdu5BXjuA+rqk4nxIBVBEYrKReSERIU=
+github.com/cloudbase/garm-provider-common v0.1.10-0.20260707095006-33396540b57b h1:70W/+KXIfnNXy8KcT17CdnDG5U7yflS3rxvIgfIKzDw=
+github.com/cloudbase/garm-provider-common v0.1.10-0.20260707095006-33396540b57b/go.mod h1:9UvQ/bHlwgSAeLFV94by9M7kRg3d0H3kemkjcFzJMmc=
 github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
@@ -226,6 +226,8 @@ go.yaml.in/yaml/v2 v2.4.4 h1:tuyd0P+2Ont/d6e2rl3be67goVK4R6deVxCUX5vyPaQ=
 go.yaml.in/yaml/v2 v2.4.4/go.mod h1:gMZqIpDtDqOfM0uNfy0SkpRhvUryYH0Z6wdMYcacYXQ=
 go.yaml.in/yaml/v3 v3.0.4 h1:tfq32ie2Jv2UxXFdLJdh3jXuOzWiL1fo0bu/FbuKpbc=
 go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg=
+go.yaml.in/yaml/v4 v4.0.0-rc.6 h1:1h7H1ohdUh93/FyE4YaDa1Zh64K6VVbjF4K6WUxMtH4=
+go.yaml.in/yaml/v4 v4.0.0-rc.6/go.mod h1:aZqd9kCMsGL7AuUv/m/PvWLdg5sjJsZ4oHDEnfPPfY0=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
 golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc=
 golang.org/x/crypto v0.53.0 h1:QZ4Muo8THX6CizN2vPPd5fBGHyogrdK9fG4wLPFUsto=

internal/templates/templates.go

@@ -11,6 +11,7 @@ import (
 	"log/slog"
 	"os"
 	"path"
+	"regexp"
 	"text/template"
 
 	"github.com/cloudbase/garm-provider-common/cloudconfig"
@@ -26,6 +27,7 @@ var Userdata embed.FS
 
 type WrapperContext struct {
 	CallbackToken string
+	CallbackURL   string
 	MetadataURL   string
 	CACertBundle  string
 }
@@ -74,8 +76,62 @@ func GetTemplateContent(osType commonParams.OSType, forge params.EndpointType) (
 	return data, nil
 }
 
+// parseTemplate is the single entrypoint used to parse a runner install
+// template. Both rendering and validation go through it so that a template
+// which validates successfully is guaranteed to parse identically at render
+// time (same delimiters, same absence of a custom FuncMap). If the render path
+// ever gains custom functions or options, adding them here keeps validation
+// faithful automatically.
+func parseTemplate(tpl string) (*template.Template, error) {
+	return template.New("").Parse(tpl)
+}
+
+// tmplParseErrRe matches the prefix text/template adds to parse errors, e.g.
+// `template: :3: unexpected "and"`. Our templates are unnamed, so the name
+// segment between the two colons is empty.
+var tmplParseErrRe = regexp.MustCompile(`^template: [^:]*:(\d+): *(.*)$`)
+
+// cleanTemplateParseError rewrites the noisy `template: :<line>: <msg>` prefix
+// that text/template emits into a friendlier `line <n>: <msg>` form while
+// preserving the line number so callers (e.g. the web UI editor) can anchor a
+// diagnostic to it. Errors that don't match the expected shape are returned
+// verbatim.
+func cleanTemplateParseError(err error) string {
+	msg := err.Error()
+	if m := tmplParseErrRe.FindStringSubmatch(msg); m != nil {
+		return fmt.Sprintf("line %s: %s", m[1], m[2])
+	}
+	return msg
+}
+
+// ValidateTemplate performs parse-time validation of a runner install template.
+//
+// It only parses the template; it deliberately does NOT execute it. Parsing
+// validates the grammar (delimiter balance, pipelines, block/end matching and
+// references to known functions) without ever inspecting the render context, so
+// it is completely independent of the variable data a template is rendered
+// against — most notably the per-pool ExtraSpecs/ExtraContext map. A reference
+// such as `{{ index .ExtraContext "gitea_cache_address" }}` is valid regardless
+// of which keys exist at render time. Executing to validate would be both
+// unsound (the context is unknown and variable at save time) and unnecessary
+// (a missing map key yields "" rather than an error), which is why parse-time
+// is exactly the right level.
+func ValidateTemplate(data []byte) error {
+	// Return typed bad-request errors (not plain fmt.Errorf) so that callers
+	// which surface this directly get an HTTP 400 with the message in the
+	// response body. A plain error would fall through handleError's default
+	// case to an opaque 500 with the details stripped.
+	if len(data) == 0 {
+		return runnerErrors.NewBadRequestError("template data is empty")
+	}
+	if _, err := parseTemplate(string(data)); err != nil {
+		return runnerErrors.NewBadRequestError("invalid template: %s", cleanTemplateParseError(err))
+	}
+	return nil
+}
+
 func RenderRunnerInstallScript(tpl string, context any) ([]byte, error) {
-	t, err := template.New("").Parse(tpl)
+	t, err := parseTemplate(tpl)
 	if err != nil {
 		return nil, fmt.Errorf("failed to parse template: %w", err)
 	}
@@ -88,14 +144,15 @@ func RenderRunnerInstallScript(tpl string, context any) ([]byte, error) {
 	return []byte(data), nil
 }
 
-func RenderRunnerInstallWrapper(ctx context.Context, osType commonParams.OSType, metadataURL, token string) ([]byte, error) {
+func RenderRunnerInstallWrapper(ctx context.Context, osType commonParams.OSType, metadataURL, callbackURL, token string) ([]byte, error) {
 	tmpl, err := template.ParseFS(Userdata, "userdata/*_wrapper.tmpl")
 	if err != nil {
 		return nil, fmt.Errorf("failed to parse templates: %w", err)
 	}
 
 	templateCtx := WrapperContext{
 		MetadataURL:   metadataURL,
+		CallbackURL:   callbackURL,
 		CallbackToken: token,
 	}
 

internal/templates/userdata/linux_wrapper.tmpl

@@ -3,11 +3,31 @@
 set -ex
 set -o pipefail
 
+CALLBACK_URL="{{ .CallbackURL }}"
 METADATA_URL="{{ .MetadataURL }}"
 BEARER_TOKEN="{{ .CallbackToken }}"
 
-curl -H "Authorization: Bearer $BEARER_TOKEN" --retry 5 --retry-delay 5 --retry-connrefused --fail $METADATA_URL/install-script/ -o /tmp/real-install.sh
-chmod +x /tmp/real-install.sh
+function call() {
+	PAYLOAD="$1"
+	[[ $CALLBACK_URL =~ ^(.*)/status(/)?$ ]] || CALLBACK_URL="${CALLBACK_URL}/status"
+	curl --retry 5 --retry-delay 5 --retry-connrefused --fail -s -X POST -d "${PAYLOAD}" -H 'Accept: application/json' -H "Authorization: Bearer ${BEARER_TOKEN}" "${CALLBACK_URL}" || echo "failed to call home: exit code ($?)"
+}
+
+function fail() {
+	MSG="$1"
+	call "{\"status\": \"failed\", \"message\": \"$MSG\"}"
+	exit 1
+}
 
+HTTP_CODE=$(curl -H "Authorization: Bearer $BEARER_TOKEN" --retry 5 --retry-delay 5 --retry-connrefused -s -w "%{http_code}" -o /tmp/real-install.sh "$METADATA_URL/install-script/" || echo "000")
+if [ "$HTTP_CODE" != "200" ]; then
+	BODY=$(cat /tmp/real-install.sh 2>/dev/null || true)
+	rm -f /tmp/real-install.sh
+	ERR=$(printf '%s' "$BODY" | sed -n 's/.*"details":"\(.*\)"}[[:space:]]*$/\1/p')
+	[ -n "$ERR" ] || ERR=$(printf '%s' "$BODY" | sed -n 's/.*"error":"\([^"]*\)".*/\1/p')
+	fail "failed to fetch install script (HTTP ${HTTP_CODE}): ${ERR}"
+fi
+
+chmod +x /tmp/real-install.sh
 /tmp/real-install.sh
 rm -f /tmp/real-install.sh

internal/templates/userdata/windows_wrapper.tmpl

@@ -3,6 +3,10 @@
 Set-ExecutionPolicy RemoteSigned -Force -ErrorAction SilentlyContinue
 $ErrorActionPreference="Stop"
 
+$MetadataUrl = "{{ .MetadataURL }}"
+$CallbackUrl = "{{ .CallbackURL }}"
+$CallbackToken = "{{ .CallbackToken }}"
+
 function Import-Certificate() {
 	[CmdletBinding()]
 	param (
@@ -72,6 +76,24 @@ function Start-ExecuteWithRetry {
     }
 }
 
+function Send-Failure() {
+    [CmdletBinding()]
+    param(
+        [Parameter(Mandatory=$true)]
+        [string]$Message
+    )
+    $statusUrl = $CallbackUrl
+    if ($statusUrl -notmatch '/status/?$') {
+        $statusUrl = "$statusUrl/status"
+    }
+    $payload = @{ status = "failed"; message = $Message } | ConvertTo-Json -Compress
+    try {
+        Invoke-WebRequest -UseBasicParsing -Method Post -Uri $statusUrl -Headers @{"Accept"="application/json"; "Authorization"="Bearer $CallbackToken"} -Body $payload | Out-Null
+    } catch {
+        Write-Output "failed to report failure status: $($_.Exception.Message)"
+    }
+}
+
 {{- if .CACertBundle }}
 $caBundleJson = '{{.CACertBundle}}'
 $converted = ConvertFrom-Json $caBundleJson
@@ -82,8 +104,31 @@ foreach ($i in $converted.psobject.Properties){
 {{- end }}
 
 $installScript = (Join-Path $env:TMP "garm-install.ps1")
-Start-ExecuteWithRetry -ScriptBlock {
-	wget -UseBasicParsing -Headers @{"Accept"="application/json"; "Authorization"="Bearer {{ .CallbackToken }}"} -Uri {{ .MetadataURL }}/install-script/ -OutFile $installScript
-} -MaxRetryCount 5 -RetryInterval 5 -RetryMessage "Retrying download of runner install script..."
+try {
+    Start-ExecuteWithRetry -ScriptBlock {
+        wget -UseBasicParsing -Headers @{"Accept"="application/json"; "Authorization"="Bearer $CallbackToken"} -Uri "$MetadataUrl/install-script/" -OutFile $installScript
+    } -MaxRetryCount 5 -RetryInterval 5 -RetryMessage "Retrying download of runner install script..."
+} catch {
+    $detail = $_.Exception.Message
+    $resp = $_.Exception.Response
+    if ($resp -ne $null) {
+        try {
+            $reader = New-Object System.IO.StreamReader($resp.GetResponseStream())
+            $bodyText = $reader.ReadToEnd()
+            if (![string]::IsNullOrWhiteSpace($bodyText)) {
+                try {
+                    $parsed = $bodyText | ConvertFrom-Json
+                    if ($parsed.details) { $detail = $parsed.details }
+                    elseif ($parsed.error) { $detail = $parsed.error }
+                    else { $detail = $bodyText }
+                } catch {
+                    $detail = $bodyText
+                }
+            }
+        } catch { }
+    }
+    Send-Failure "failed to fetch install script: $detail"
+    exit 1
+}
 
-powershell.exe -Sta -NonInteractive -ExecutionPolicy RemoteSigned -File $installScript
+powershell.exe -Sta -NonInteractive -ExecutionPolicy RemoteSigned -File $installScript

runner/metadata.go

@@ -470,7 +470,11 @@ func (r *Runner) GetRunnerInstallScript(ctx context.Context) ([]byte, error) {
 
 	installScript, err := templates.RenderRunnerInstallScript(string(tplBytes), tplCtx)
 	if err != nil {
-		return nil, fmt.Errorf("failed to get runner install script: %w", err)
+		// A render failure is deterministic (the template is broken), so return
+		// a bad-request rather than a generic 500 error. A 500 error is opaque
+		// to the caller. The install wrapper surfaces this detail as the runner's
+		// "failed" status so the instance doesn't hang in "installing" forever.
+		return nil, runnerErrors.NewBadRequestError("failed to render runner install script: %s", err)
 	}
 	return installScript, nil
 }

runner/templates.go

@@ -34,6 +34,13 @@ func (r *Runner) CreateTemplate(ctx context.Context, param params.CreateTemplate
 		return params.Template{}, runnerErrors.NewBadRequestError("invalid create params %q", err)
 	}
 
+	// ValidateTemplate returns a typed bad-request error carrying the parse
+	// error message. Wrap with %w to add call-site context for tracking while
+	// preserving the bad-request type (so it still maps to HTTP 400).
+	if err := templates.ValidateTemplate(param.Data); err != nil {
+		return params.Template{}, fmt.Errorf("failed to validate template: %w", err)
+	}
+
 	template, err := r.store.CreateTemplate(ctx, param)
 	if err != nil {
 		return params.Template{}, fmt.Errorf("failed to create template: %w", err)
@@ -172,6 +179,15 @@ func (r *Runner) UpdateTemplate(ctx context.Context, id uint, param params.Updat
 		return params.Template{}, runnerErrors.NewBadRequestError("invalid update params: %q", err)
 	}
 
+	// Data is optional on update (name/description-only updates leave it nil,
+	// and the store only writes it when non-empty), so only validate the
+	// template body when the caller is actually changing it.
+	if len(param.Data) > 0 {
+		if err := templates.ValidateTemplate(param.Data); err != nil {
+			return params.Template{}, fmt.Errorf("failed to validate template: %w", err)
+		}
+	}
+
 	template, err := r.store.UpdateTemplate(ctx, id, param)
 	if err != nil {
 		return params.Template{}, fmt.Errorf("failed to update template: %w", err)