Skip to content

Commit ca1b910

Browse files
Improve template manager/generation
This change parses the runner install template on creation or update. It does not "execute" the template, as that requires full template context which may vary depending on extra_specs, but the syntax is checked. Additionally, when a template is rendered (requested by a runner), the wrapper template will now catch the error and phone it home, marking the runner as failed. This will at least surface the error instead of leaving the runner in "installing" forever and only printing the error message to the garm log. Changes have been made to the CLI as well to notify the user that the template has an error, dropping them back into the editor. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
1 parent 9fd4d93 commit ca1b910

43 files changed

Lines changed: 15828 additions & 31 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

cmd/garm-cli/cmd/templates.go

Lines changed: 62 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
package cmd
1515

1616
import (
17+
"bufio"
18+
"errors"
1719
"fmt"
1820
"os"
1921
"os/exec"
@@ -23,6 +25,7 @@ import (
2325
"github.com/spf13/cobra"
2426

2527
commonParams "github.com/cloudbase/garm-provider-common/params"
28+
apiserverParams "github.com/cloudbase/garm/apiserver/params"
2629
apiTemplates "github.com/cloudbase/garm/client/templates"
2730
"github.com/cloudbase/garm/cmd/garm-cli/common"
2831
"github.com/cloudbase/garm/cmd/garm-cli/editor"
@@ -446,24 +449,40 @@ is opened in $EDITOR instead.`,
446449
updateReq := apiTemplates.NewUpdateTemplateParams()
447450
updateReq.TemplateID = float64(response.Payload.ID)
448451
updateReq.Body.Data = []byte(text)
449-
_, err := apiCli.Templates.UpdateTemplate(updateReq, authToken)
450-
return err
452+
if _, err := apiCli.Templates.UpdateTemplate(updateReq, authToken); err != nil {
453+
// Surface the API's error detail (e.g. the template parse error)
454+
// rather than the raw go-swagger response blob. The built-in
455+
// editor reports this in place, keeping the editor open to fix.
456+
return errors.New(templateAPIErrorMessage(err))
457+
}
458+
return nil
451459
}
452460

453461
if templateEditExternal {
454-
newContent, changed, err := editWithExternalEditor(original, string(response.Payload.OSType))
455-
if err != nil {
456-
return err
457-
}
458-
if !changed {
459-
fmt.Println("no changes made")
462+
// The external editor closes after each pass, so on a failed save we
463+
// show the error and offer to reopen with the user's edits intact.
464+
toEdit := original
465+
for {
466+
newContent, _, err := editWithExternalEditor(toEdit, string(response.Payload.OSType))
467+
if err != nil {
468+
return err
469+
}
470+
if newContent == original {
471+
fmt.Println("no changes made")
472+
return nil
473+
}
474+
if err := updateTemplate(newContent); err != nil {
475+
fmt.Fprintf(os.Stderr, "\ntemplate not saved: %s\nHit enter to retry, ctrl-c to cancel edit ", err)
476+
if _, rerr := bufio.NewReader(os.Stdin).ReadString('\n'); rerr != nil {
477+
// stdin closed (e.g. non-interactive) — nothing to retry with.
478+
return errors.New("template not saved")
479+
}
480+
toEdit = newContent
481+
continue
482+
}
483+
fmt.Println("changes saved successfully")
460484
return nil
461485
}
462-
if err := updateTemplate(newContent); err != nil {
463-
return fmt.Errorf("failed to update template: %s", err)
464-
}
465-
fmt.Println("changes saved successfully")
466-
return nil
467486
}
468487

469488
ed := editor.NewEditor()
@@ -541,6 +560,36 @@ func editWithExternalEditor(content, osType string) (string, bool, error) {
541560
return string(edited), string(edited) != content, nil
542561
}
543562

563+
// templateAPIErrorMessage extracts the human-readable message from a template
564+
// API error, preferring the structured APIErrorResponse detail (already
565+
// unmarshaled by the client) over the raw go-swagger response string.
566+
func templateAPIErrorMessage(err error) string {
567+
var updateErr *apiTemplates.UpdateTemplateDefault
568+
if errors.As(err, &updateErr) {
569+
if msg := apiErrorResponseMessage(updateErr.Payload); msg != "" {
570+
return msg
571+
}
572+
}
573+
var createErr *apiTemplates.CreateTemplateDefault
574+
if errors.As(err, &createErr) {
575+
if msg := apiErrorResponseMessage(createErr.Payload); msg != "" {
576+
return msg
577+
}
578+
}
579+
return err.Error()
580+
}
581+
582+
func apiErrorResponseMessage(p apiserverParams.APIErrorResponse) string {
583+
switch {
584+
case p.Details != "":
585+
return p.Details
586+
case p.Error != "":
587+
return p.Error
588+
default:
589+
return ""
590+
}
591+
}
592+
544593
func init() {
545594
templateCreateCmd.Flags().StringVar(&templateName, "name", "", "Name of the template.")
546595
templateCreateCmd.Flags().StringVar(&templateDescription, "description", "", "Template description.")

go.mod

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ require (
66
github.com/BurntSushi/toml v1.6.0
77
github.com/alecthomas/chroma/v2 v2.27.0
88
github.com/bradleyfalzon/ghinstallation/v2 v2.19.0
9-
github.com/cloudbase/garm-provider-common v0.1.10-0.20260627162627-e00d7529cc6f
9+
github.com/cloudbase/garm-provider-common v0.1.10-0.20260707095006-33396540b57b
1010
github.com/felixge/httpsnoop v1.1.0
1111
github.com/gdamore/tcell/v2 v2.13.10
1212
github.com/go-gormigrate/gormigrate/v2 v2.1.6
@@ -106,6 +106,7 @@ require (
106106
go.opentelemetry.io/otel/metric v1.44.0 // indirect
107107
go.opentelemetry.io/otel/trace v1.44.0 // indirect
108108
go.yaml.in/yaml/v3 v3.0.4 // indirect
109+
go.yaml.in/yaml/v4 v4.0.0-rc.6 // indirect
109110
golang.org/x/net v0.56.0 // indirect
110111
golang.org/x/sys v0.46.0 // indirect
111112
golang.org/x/text v0.38.0 // indirect

go.sum

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,8 @@ github.com/chzyer/test v1.0.0 h1:p3BQDXSxOhOG0P9z6/hGnII4LGiEPOYBhs8asl/fC04=
2525
github.com/chzyer/test v1.0.0/go.mod h1:2JlltgoNkt4TW/z9V/IzDdFaMTM2JPIi26O1pF38GC8=
2626
github.com/clipperhouse/uax29/v2 v2.7.0 h1:+gs4oBZ2gPfVrKPthwbMzWZDaAFPGYK72F0NJv2v7Vk=
2727
github.com/clipperhouse/uax29/v2 v2.7.0/go.mod h1:EFJ2TJMRUaplDxHKj1qAEhCtQPW2tJSwu5BF98AuoVM=
28-
github.com/cloudbase/garm-provider-common v0.1.10-0.20260627162627-e00d7529cc6f h1:IalGGcSKBGUd+KFFn2OXO6SMcODyCkSO/SMXtm4nwy8=
29-
github.com/cloudbase/garm-provider-common v0.1.10-0.20260627162627-e00d7529cc6f/go.mod h1:i1KXJVzi5ouzbdu5BXjuA+rqk4nxIBVBEYrKReSERIU=
28+
github.com/cloudbase/garm-provider-common v0.1.10-0.20260707095006-33396540b57b h1:70W/+KXIfnNXy8KcT17CdnDG5U7yflS3rxvIgfIKzDw=
29+
github.com/cloudbase/garm-provider-common v0.1.10-0.20260707095006-33396540b57b/go.mod h1:9UvQ/bHlwgSAeLFV94by9M7kRg3d0H3kemkjcFzJMmc=
3030
github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g=
3131
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
3232
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=
226226
go.yaml.in/yaml/v2 v2.4.4/go.mod h1:gMZqIpDtDqOfM0uNfy0SkpRhvUryYH0Z6wdMYcacYXQ=
227227
go.yaml.in/yaml/v3 v3.0.4 h1:tfq32ie2Jv2UxXFdLJdh3jXuOzWiL1fo0bu/FbuKpbc=
228228
go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg=
229+
go.yaml.in/yaml/v4 v4.0.0-rc.6 h1:1h7H1ohdUh93/FyE4YaDa1Zh64K6VVbjF4K6WUxMtH4=
230+
go.yaml.in/yaml/v4 v4.0.0-rc.6/go.mod h1:aZqd9kCMsGL7AuUv/m/PvWLdg5sjJsZ4oHDEnfPPfY0=
229231
golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
230232
golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc=
231233
golang.org/x/crypto v0.53.0 h1:QZ4Muo8THX6CizN2vPPd5fBGHyogrdK9fG4wLPFUsto=

internal/templates/templates.go

Lines changed: 59 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"log/slog"
1212
"os"
1313
"path"
14+
"regexp"
1415
"text/template"
1516

1617
"github.com/cloudbase/garm-provider-common/cloudconfig"
@@ -26,6 +27,7 @@ var Userdata embed.FS
2627

2728
type WrapperContext struct {
2829
CallbackToken string
30+
CallbackURL string
2931
MetadataURL string
3032
CACertBundle string
3133
}
@@ -74,8 +76,62 @@ func GetTemplateContent(osType commonParams.OSType, forge params.EndpointType) (
7476
return data, nil
7577
}
7678

79+
// parseTemplate is the single entrypoint used to parse a runner install
80+
// template. Both rendering and validation go through it so that a template
81+
// which validates successfully is guaranteed to parse identically at render
82+
// time (same delimiters, same absence of a custom FuncMap). If the render path
83+
// ever gains custom functions or options, adding them here keeps validation
84+
// faithful automatically.
85+
func parseTemplate(tpl string) (*template.Template, error) {
86+
return template.New("").Parse(tpl)
87+
}
88+
89+
// tmplParseErrRe matches the prefix text/template adds to parse errors, e.g.
90+
// `template: :3: unexpected "and"`. Our templates are unnamed, so the name
91+
// segment between the two colons is empty.
92+
var tmplParseErrRe = regexp.MustCompile(`^template: [^:]*:(\d+): *(.*)$`)
93+
94+
// cleanTemplateParseError rewrites the noisy `template: :<line>: <msg>` prefix
95+
// that text/template emits into a friendlier `line <n>: <msg>` form while
96+
// preserving the line number so callers (e.g. the web UI editor) can anchor a
97+
// diagnostic to it. Errors that don't match the expected shape are returned
98+
// verbatim.
99+
func cleanTemplateParseError(err error) string {
100+
msg := err.Error()
101+
if m := tmplParseErrRe.FindStringSubmatch(msg); m != nil {
102+
return fmt.Sprintf("line %s: %s", m[1], m[2])
103+
}
104+
return msg
105+
}
106+
107+
// ValidateTemplate performs parse-time validation of a runner install template.
108+
//
109+
// It only parses the template; it deliberately does NOT execute it. Parsing
110+
// validates the grammar (delimiter balance, pipelines, block/end matching and
111+
// references to known functions) without ever inspecting the render context, so
112+
// it is completely independent of the variable data a template is rendered
113+
// against — most notably the per-pool ExtraSpecs/ExtraContext map. A reference
114+
// such as `{{ index .ExtraContext "gitea_cache_address" }}` is valid regardless
115+
// of which keys exist at render time. Executing to validate would be both
116+
// unsound (the context is unknown and variable at save time) and unnecessary
117+
// (a missing map key yields "" rather than an error), which is why parse-time
118+
// is exactly the right level.
119+
func ValidateTemplate(data []byte) error {
120+
// Return typed bad-request errors (not plain fmt.Errorf) so that callers
121+
// which surface this directly get an HTTP 400 with the message in the
122+
// response body. A plain error would fall through handleError's default
123+
// case to an opaque 500 with the details stripped.
124+
if len(data) == 0 {
125+
return runnerErrors.NewBadRequestError("template data is empty")
126+
}
127+
if _, err := parseTemplate(string(data)); err != nil {
128+
return runnerErrors.NewBadRequestError("invalid template: %s", cleanTemplateParseError(err))
129+
}
130+
return nil
131+
}
132+
77133
func RenderRunnerInstallScript(tpl string, context any) ([]byte, error) {
78-
t, err := template.New("").Parse(tpl)
134+
t, err := parseTemplate(tpl)
79135
if err != nil {
80136
return nil, fmt.Errorf("failed to parse template: %w", err)
81137
}
@@ -88,14 +144,15 @@ func RenderRunnerInstallScript(tpl string, context any) ([]byte, error) {
88144
return []byte(data), nil
89145
}
90146

91-
func RenderRunnerInstallWrapper(ctx context.Context, osType commonParams.OSType, metadataURL, token string) ([]byte, error) {
147+
func RenderRunnerInstallWrapper(ctx context.Context, osType commonParams.OSType, metadataURL, callbackURL, token string) ([]byte, error) {
92148
tmpl, err := template.ParseFS(Userdata, "userdata/*_wrapper.tmpl")
93149
if err != nil {
94150
return nil, fmt.Errorf("failed to parse templates: %w", err)
95151
}
96152

97153
templateCtx := WrapperContext{
98154
MetadataURL: metadataURL,
155+
CallbackURL: callbackURL,
99156
CallbackToken: token,
100157
}
101158

internal/templates/userdata/linux_wrapper.tmpl

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,31 @@
33
set -ex
44
set -o pipefail
55

6+
CALLBACK_URL="{{ .CallbackURL }}"
67
METADATA_URL="{{ .MetadataURL }}"
78
BEARER_TOKEN="{{ .CallbackToken }}"
89

9-
curl -H "Authorization: Bearer $BEARER_TOKEN" --retry 5 --retry-delay 5 --retry-connrefused --fail $METADATA_URL/install-script/ -o /tmp/real-install.sh
10-
chmod +x /tmp/real-install.sh
10+
function call() {
11+
PAYLOAD="$1"
12+
[[ $CALLBACK_URL =~ ^(.*)/status(/)?$ ]] || CALLBACK_URL="${CALLBACK_URL}/status"
13+
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 ($?)"
14+
}
15+
16+
function fail() {
17+
MSG="$1"
18+
call "{\"status\": \"failed\", \"message\": \"$MSG\"}"
19+
exit 1
20+
}
1121

22+
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")
23+
if [ "$HTTP_CODE" != "200" ]; then
24+
BODY=$(cat /tmp/real-install.sh 2>/dev/null || true)
25+
rm -f /tmp/real-install.sh
26+
ERR=$(printf '%s' "$BODY" | sed -n 's/.*"details":"\(.*\)"}[[:space:]]*$/\1/p')
27+
[ -n "$ERR" ] || ERR=$(printf '%s' "$BODY" | sed -n 's/.*"error":"\([^"]*\)".*/\1/p')
28+
fail "failed to fetch install script (HTTP ${HTTP_CODE}): ${ERR}"
29+
fi
30+
31+
chmod +x /tmp/real-install.sh
1232
/tmp/real-install.sh
1333
rm -f /tmp/real-install.sh

internal/templates/userdata/windows_wrapper.tmpl

Lines changed: 49 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,10 @@
33
Set-ExecutionPolicy RemoteSigned -Force -ErrorAction SilentlyContinue
44
$ErrorActionPreference="Stop"
55

6+
$MetadataUrl = "{{ .MetadataURL }}"
7+
$CallbackUrl = "{{ .CallbackURL }}"
8+
$CallbackToken = "{{ .CallbackToken }}"
9+
610
function Import-Certificate() {
711
[CmdletBinding()]
812
param (
@@ -72,6 +76,24 @@ function Start-ExecuteWithRetry {
7276
}
7377
}
7478

79+
function Send-Failure() {
80+
[CmdletBinding()]
81+
param(
82+
[Parameter(Mandatory=$true)]
83+
[string]$Message
84+
)
85+
$statusUrl = $CallbackUrl
86+
if ($statusUrl -notmatch '/status/?$') {
87+
$statusUrl = "$statusUrl/status"
88+
}
89+
$payload = @{ status = "failed"; message = $Message } | ConvertTo-Json -Compress
90+
try {
91+
Invoke-WebRequest -UseBasicParsing -Method Post -Uri $statusUrl -Headers @{"Accept"="application/json"; "Authorization"="Bearer $CallbackToken"} -Body $payload | Out-Null
92+
} catch {
93+
Write-Output "failed to report failure status: $($_.Exception.Message)"
94+
}
95+
}
96+
7597
{{- if .CACertBundle }}
7698
$caBundleJson = '{{.CACertBundle}}'
7799
$converted = ConvertFrom-Json $caBundleJson
@@ -82,8 +104,31 @@ foreach ($i in $converted.psobject.Properties){
82104
{{- end }}
83105

84106
$installScript = (Join-Path $env:TMP "garm-install.ps1")
85-
Start-ExecuteWithRetry -ScriptBlock {
86-
wget -UseBasicParsing -Headers @{"Accept"="application/json"; "Authorization"="Bearer {{ .CallbackToken }}"} -Uri {{ .MetadataURL }}/install-script/ -OutFile $installScript
87-
} -MaxRetryCount 5 -RetryInterval 5 -RetryMessage "Retrying download of runner install script..."
107+
try {
108+
Start-ExecuteWithRetry -ScriptBlock {
109+
wget -UseBasicParsing -Headers @{"Accept"="application/json"; "Authorization"="Bearer $CallbackToken"} -Uri "$MetadataUrl/install-script/" -OutFile $installScript
110+
} -MaxRetryCount 5 -RetryInterval 5 -RetryMessage "Retrying download of runner install script..."
111+
} catch {
112+
$detail = $_.Exception.Message
113+
$resp = $_.Exception.Response
114+
if ($resp -ne $null) {
115+
try {
116+
$reader = New-Object System.IO.StreamReader($resp.GetResponseStream())
117+
$bodyText = $reader.ReadToEnd()
118+
if (![string]::IsNullOrWhiteSpace($bodyText)) {
119+
try {
120+
$parsed = $bodyText | ConvertFrom-Json
121+
if ($parsed.details) { $detail = $parsed.details }
122+
elseif ($parsed.error) { $detail = $parsed.error }
123+
else { $detail = $bodyText }
124+
} catch {
125+
$detail = $bodyText
126+
}
127+
}
128+
} catch { }
129+
}
130+
Send-Failure "failed to fetch install script: $detail"
131+
exit 1
132+
}
88133

89-
powershell.exe -Sta -NonInteractive -ExecutionPolicy RemoteSigned -File $installScript
134+
powershell.exe -Sta -NonInteractive -ExecutionPolicy RemoteSigned -File $installScript

runner/metadata.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -470,7 +470,11 @@ func (r *Runner) GetRunnerInstallScript(ctx context.Context) ([]byte, error) {
470470

471471
installScript, err := templates.RenderRunnerInstallScript(string(tplBytes), tplCtx)
472472
if err != nil {
473-
return nil, fmt.Errorf("failed to get runner install script: %w", err)
473+
// A render failure is deterministic (the template is broken), so return
474+
// a bad-request rather than a generic 500 error. A 500 error is opaque
475+
// to the caller. The install wrapper surfaces this detail as the runner's
476+
// "failed" status so the instance doesn't hang in "installing" forever.
477+
return nil, runnerErrors.NewBadRequestError("failed to render runner install script: %s", err)
474478
}
475479
return installScript, nil
476480
}

runner/templates.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,13 @@ func (r *Runner) CreateTemplate(ctx context.Context, param params.CreateTemplate
3434
return params.Template{}, runnerErrors.NewBadRequestError("invalid create params %q", err)
3535
}
3636

37+
// ValidateTemplate returns a typed bad-request error carrying the parse
38+
// error message. Wrap with %w to add call-site context for tracking while
39+
// preserving the bad-request type (so it still maps to HTTP 400).
40+
if err := templates.ValidateTemplate(param.Data); err != nil {
41+
return params.Template{}, fmt.Errorf("failed to validate template: %w", err)
42+
}
43+
3744
template, err := r.store.CreateTemplate(ctx, param)
3845
if err != nil {
3946
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
172179
return params.Template{}, runnerErrors.NewBadRequestError("invalid update params: %q", err)
173180
}
174181

182+
// Data is optional on update (name/description-only updates leave it nil,
183+
// and the store only writes it when non-empty), so only validate the
184+
// template body when the caller is actually changing it.
185+
if len(param.Data) > 0 {
186+
if err := templates.ValidateTemplate(param.Data); err != nil {
187+
return params.Template{}, fmt.Errorf("failed to validate template: %w", err)
188+
}
189+
}
190+
175191
template, err := r.store.UpdateTemplate(ctx, id, param)
176192
if err != nil {
177193
return params.Template{}, fmt.Errorf("failed to update template: %w", err)

0 commit comments

Comments
 (0)