feat(templates): implement custom template loader and executor (#565)#702
Conversation
…A#565) LoadCustomTemplate supports inline, file, and URL sources with SHA256 checksum verification. CustomTemplateExecutor generates bash script with env vars, logging, and continueOnError support. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
…xecutor (NVIDIA#565) - B1: Use single-quote shell quoting (shellQuote) for env var values to prevent command substitution injection via $(...) - B2: Validate env var keys match ^[a-zA-Z_][a-zA-Z0-9_]*$ to prevent key injection - R1: Move || true outside holodeck_log string so it acts as a shell operator, not part of the log message - R2: Sanitize template name/phase before shell interpolation using sanitizeName() which strips non-alphanumeric characters - N1: Detect 10MB URL response truncation instead of silently truncating - N2: Update ContinueOnError test assertion to match corrected output Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Pull Request Test Coverage Report for Build 22713988417Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
…NVIDIA#565) Revert escaped quotes inside Go backtick raw strings where \" was literal backslash-quote instead of plain double quote. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
…IDIA#565) - Wrap resp.Body.Close() to check error (errcheck) - Use filepath.Clean before os.ReadFile (gosec G304) - Use 0600 permissions for test WriteFile calls (gosec G306) Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
There was a problem hiding this comment.
Pull request overview
Adds initial support in the provisioner template layer for user-provided “custom templates” by loading script content from inline/file/URL sources and generating shell script blocks to execute them during provisioning.
Changes:
- Introduces
LoadCustomTemplate()with optional SHA256 verification and relative file resolution. - Adds
fetchURL()with a 60s timeout and a 10MB response-size cap. - Implements
CustomTemplateExecutorto emit bash script content with[CUSTOM]logging, env var exports, andcontinueOnErrorbehavior, plus accompanying unit tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| pkg/provisioner/templates/custom.go | Adds custom template loading (inline/file/url), checksum verification, URL fetching, and script generation/execution wrapper. |
| pkg/provisioner/templates/custom_test.go | Adds unit tests for loading sources/checksums and for executor output (logging, env quoting, sanitization, continueOnError). |
Comments suppressed due to low confidence (1)
pkg/provisioner/templates/custom_test.go:325
- The file ends with an incomplete comment about fetchURL truncation detection. If this is a TODO for additional tests, consider either turning it into an actionable TODO with a tracking issue/reference or removing it to keep the test file clean.
// N1: fetchURL truncation detection (tested via direct call would need HTTP server,
// so we test the limit constant indirectly -- the implementation test is structural)
You can also share your feedback on Copilot code review. Take the survey.
| switch { | ||
| case tpl.Inline != "": | ||
| content = []byte(tpl.Inline) | ||
| case tpl.File != "": | ||
| path := tpl.File | ||
| if !filepath.IsAbs(path) { | ||
| path = filepath.Join(baseDir, path) | ||
| } | ||
| content, err = os.ReadFile(filepath.Clean(path)) //nolint:gosec // path is validated by caller | ||
| if err != nil { | ||
| return nil, fmt.Errorf("custom template %q: failed to read file %q: %w", tpl.Name, path, err) | ||
| } | ||
| case tpl.URL != "": | ||
| content, err = fetchURL(tpl.URL, tpl.Name) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| default: | ||
| return nil, fmt.Errorf("custom template %q: no source specified", tpl.Name) | ||
| } |
There was a problem hiding this comment.
LoadCustomTemplate silently prefers Inline over File over URL when multiple sources are set. This makes configs ambiguous and can hide user mistakes; it should instead validate that exactly one of inline/file/url is specified and return an error when none or more than one are provided.
| // Verify checksum if provided | ||
| if tpl.Checksum != "" { | ||
| expected := strings.TrimPrefix(tpl.Checksum, "sha256:") | ||
| actual := sha256Hex(content) | ||
| if actual != expected { | ||
| return nil, fmt.Errorf("custom template %q: checksum mismatch: expected %s, got %s", tpl.Name, expected, actual) | ||
| } | ||
| } |
There was a problem hiding this comment.
Checksum parsing uses strings.TrimPrefix("sha256:") without validating the prefix/format. If the checksum is missing the prefix or uses a different algorithm label, the error becomes a confusing mismatch instead of a clear "invalid checksum format". Consider requiring the "sha256:" prefix and validating the digest is 64 hex chars before comparing.
| // fetchURL downloads content from a URL with a timeout. | ||
| func fetchURL(url, name string) ([]byte, error) { | ||
| client := &http.Client{Timeout: 60 * time.Second} | ||
| resp, err := client.Get(url) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("custom template %q: failed to fetch %q: %w", name, url, err) | ||
| } | ||
| defer func() { _ = resp.Body.Close() }() |
There was a problem hiding this comment.
fetchURL accepts arbitrary URL strings and does not enforce the API contract in v1alpha1.CustomTemplate ("remote HTTPS location"). As written it will happily fetch over plain http:// and will follow redirects (potentially downgrading to http). Parse the URL and require https scheme (and optionally reject redirects to non-https) before issuing the request.
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("custom template %q: URL %q returned status %d", name, url, resp.StatusCode) | ||
| } | ||
|
|
||
| // Read one extra byte to detect truncation | ||
| content, err := io.ReadAll(io.LimitReader(resp.Body, maxURLResponseBytes+1)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("custom template %q: failed to read response: %w", name, err) | ||
| } | ||
| if len(content) > maxURLResponseBytes { | ||
| return nil, fmt.Errorf("custom template %q: URL response exceeds 10MB limit", name) | ||
| } |
There was a problem hiding this comment.
The new error strings here use "URL" capitalized (e.g., "URL ... returned status" / "URL response exceeds ..."). The repo's Go error-message convention is lowercase messages without punctuation; please make these consistent (e.g., "url ... returned status" / "url response exceeds ...").
| fmt.Fprintf(tpl, "set +e\n") | ||
| tpl.Write(ct.Content) | ||
| fmt.Fprintf(tpl, "\n_custom_rc=$?\nset -e\n") |
There was a problem hiding this comment.
continueOnError currently wraps the template by toggling set +e and checking $?, but a template that calls exit will still terminate the whole provisioning script, bypassing this handler. To reliably honor ContinueOnError, run the custom content in a subshell (e.g., ( ... )) or bash -c and capture that command's exit code instead of executing the content directly in the main shell.
| fmt.Fprintf(tpl, "set +e\n") | |
| tpl.Write(ct.Content) | |
| fmt.Fprintf(tpl, "\n_custom_rc=$?\nset -e\n") | |
| // Run the custom content in a subshell so that 'exit' only terminates | |
| // the subshell and not the main provisioning script. | |
| fmt.Fprintf(tpl, "(\n") | |
| fmt.Fprintf(tpl, " set +e\n") | |
| tpl.Write(ct.Content) | |
| fmt.Fprintf(tpl, "\n)\n_custom_rc=$?\n") |
| // shellQuote produces a single-quoted shell string, escaping embedded single quotes. | ||
| func shellQuote(s string) string { | ||
| return "'" + strings.ReplaceAll(s, "'", "'\\''") + "'" | ||
| } |
There was a problem hiding this comment.
PR description says env var values are exported using shell-quoting with %q, but the implementation uses a custom single-quote escaping function (shellQuote). Either update the PR description to match, or switch to the described quoting approach so the documentation and code stay aligned.
| // B2: Shell injection via env var keys -- invalid keys must be rejected | ||
| func TestNewCustomTemplateExecutor_InvalidEnvKey(t *testing.T) { | ||
| tpl := v1alpha1.CustomTemplate{ | ||
| Name: "bad-key", | ||
| Inline: "echo safe", | ||
| Env: map[string]string{ | ||
| "FOO; rm -rf /": "value", | ||
| }, | ||
| } | ||
|
|
||
| ct := NewCustomTemplateExecutor(tpl, []byte("echo safe")) | ||
|
|
||
| var buf bytes.Buffer | ||
| err := ct.Execute(&buf, v1alpha1.Environment{}) | ||
| if err == nil { | ||
| t.Fatal("expected error for invalid env var key") | ||
| } | ||
| if !strings.Contains(err.Error(), "invalid environment variable name") { | ||
| t.Errorf("unexpected error: %v", err) | ||
| } |
There was a problem hiding this comment.
Test name implies NewCustomTemplateExecutor rejects invalid env keys, but the error is actually produced by ct.Execute(). Rename the test to reflect where validation happens (or move the validation into the constructor if that's the intent) to avoid misleading future readers.
Summary
LoadCustomTemplate()supporting inline, file, and URL sources with SHA256 checksum verificationCustomTemplateExecutorthat generates bash scripts with[CUSTOM]log prefix, environment variable exports (shell-quoted with%q), andcontinueOnErrorhandlingfetchURL()with 60s timeout and 10MB response limit for URL-sourced templatesTest plan
go build ./...passesgo vet ./...passesRelated