feat(provisioner): integrate custom templates into dependency resolver (#565)#706
Conversation
Pull Request Test Coverage Report for Build 22715272967Details
💛 - Coveralls |
…VIDIA#565) Address architecture review feedback on PR NVIDIA#706: - B1/B2: Fix shell injection in env var export by delegating to Task 2's CustomTemplateExecutor (which sanitizes env keys and values) - R1: Remove duplicated fetchURL, verifyChecksum, executeCustomTemplate; use templates.LoadCustomTemplate() and NewCustomTemplateExecutor() - R2: Inherit Task 2's 10MB response size limit for URL sources - R3: Fix template name interpolation in shell strings via executor - R4: Remove duplicated validPhases map; use templates.ValidateTemplateInputs() - N1: Fix docs to say HTTPS is required (not just recommended) - N2: Use real 64-char hex digest in docs checksum example Cherry-picks Task 1 (validate.go) and Task 2 (custom.go) into this branch for integration. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
- Fix unchecked resp.Body.Close() with deferred error discard - Run gofmt on dryrun.go - Fix gosec G304: add filepath.Clean() before os.ReadFile - Fix gosec G306: use 0600 permissions for test file writes - Fix MD013: wrap doc lines to under 120 characters Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
NVIDIA#565) Add custom template support to the DependencyResolver with five execution phases (pre-install, post-runtime, post-toolkit, post-kubernetes, post-install). Support inline, file, and URL sources with SHA256 checksum verification. - Add addCustomTemplates/executeCustomTemplate methods to DependencyResolver - Add SetBaseDir for resolving relative file paths - Add custom template validation in Dryrun (phase, source checks) - Add comprehensive tests for resolver, dryrun, and YAML integration - Add user documentation in docs/guides/custom-templates.md Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
- Fix unchecked resp.Body.Close() with deferred error discard - Run gofmt on dryrun.go - Fix gosec G304: add filepath.Clean() before os.ReadFile - Fix gosec G306: use 0600 permissions for test file writes - Fix MD013: wrap doc lines to under 120 characters Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Wrap all non-code-block lines in custom-templates.md to stay under the 80-character line length limit enforced by markdownlint MD013. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
18f6499 to
f3c4583
Compare
Replace duplicated fetchURL, verifyChecksum, executeCustomTemplate with calls to templates.LoadCustomTemplate and NewCustomTemplateExecutor. Replace validateCustomTemplates with templates.ValidateTemplateInputs. Fix dryrun test checksum to match 64-char hex format required by the merged validator. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
There was a problem hiding this comment.
Pull request overview
This PR adds first-class support for user-provided “custom templates” (scripts) to Holodeck’s provisioning flow by integrating them into the provisioner dependency resolver with phase-based insertion, plus dry-run validation/logging, tests, and end-user documentation.
Changes:
- Extend
DependencyResolverto insert custom templates across five provisioning phases. - Add custom template validation and dry-run logging for custom template sources/phases.
- Add fixtures, integration/unit tests, and a user guide for configuring custom templates.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/provisioner/dependency.go |
Adds phase-based custom template insertion and baseDir support for file sources. |
pkg/provisioner/dryrun.go |
Adds custom template validation and dry-run logging of template sources/phases. |
pkg/provisioner/dependency_custom_test.go |
Adds Ginkgo coverage for resolver ordering and custom template behaviors (env, continueOnError, file). |
pkg/provisioner/dryrun_custom_test.go |
Adds unit tests for dry-run validation scenarios (phase/source/url/file). |
pkg/provisioner/custom_integration_test.go |
Adds YAML parsing + dependency-order integration tests using a fixture environment. |
tests/data/test_custom_templates.yaml |
Adds a YAML fixture exercising multiple phases and options. |
docs/guides/custom-templates.md |
Adds end-user documentation for configuration, phases, and security considerations. |
You can also share your feedback on Copilot code review. Take the survey.
| checksum: "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" | ||
| ``` | ||
|
|
||
| HTTPS is strongly recommended for URL sources. |
There was a problem hiding this comment.
This guide says "HTTPS is strongly recommended" for URL sources, but the Security Considerations section (and ValidateTemplateInputs) indicate HTTPS is required and non-HTTPS URLs are rejected. Please make these statements consistent (either require HTTPS everywhere or document the actual behavior).
| HTTPS is strongly recommended for URL sources. | |
| HTTPS is required for URL sources; non-HTTPS URLs are rejected during validation. |
| // Phase: pre-install (before any Holodeck components) | ||
| d.addCustomTemplates(v1alpha1.TemplatePhasePreInstall) | ||
|
|
There was a problem hiding this comment.
Adding pre-install custom templates before other dependencies can break the existing microk8s path: withKubernetes(microk8s) resets d.Dependencies to nil, which will drop any pre-install (and post-runtime/post-toolkit) custom templates appended earlier in Resolve(). Consider preserving already-added custom template funcs when switching to microk8s, or handling microk8s in Resolve() before adding phase templates so pre-install scripts still run.
| // Validate custom templates | ||
| if len(env.Spec.CustomTemplates) > 0 { | ||
| if err := templates.ValidateTemplateInputs(env); err != nil { | ||
| cancel(logger.ErrLoadingFailed) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Dryrun only calls templates.ValidateTemplateInputs(env) when custom templates are present. Since ValidateTemplateInputs validates many non-custom fields (versions, git refs, key paths, etc.), this makes dryrun validation behavior depend on whether customTemplates is set (invalid inputs may slip through otherwise, or unrelated fields may start failing when a single custom template is added). Consider calling ValidateTemplateInputs unconditionally, or splitting out an exported custom-template-only validator and calling that here.
| // Validate custom templates | |
| if len(env.Spec.CustomTemplates) > 0 { | |
| if err := templates.ValidateTemplateInputs(env); err != nil { | |
| cancel(logger.ErrLoadingFailed) | |
| return err | |
| } | |
| // Validate template inputs (includes both custom and non-custom fields) | |
| if err := templates.ValidateTemplateInputs(env); err != nil { | |
| cancel(logger.ErrLoadingFailed) | |
| return err | |
| } | |
| // Log custom templates if present | |
| if len(env.Spec.CustomTemplates) > 0 { |
| case ct.URL != "": | ||
| log.Info("Custom template %q: URL %s (phase: %s)", ct.Name, ct.URL, phase) | ||
| if !strings.HasPrefix(ct.URL, "https://") { | ||
| log.Warning("Custom template %q: URL does not use HTTPS", ct.Name) | ||
| } |
There was a problem hiding this comment.
The non-HTTPS URL warning here is effectively unreachable because ValidateTemplateInputs rejects any custom template URL that doesn't start with "https://". Either remove this branch to avoid dead code, or relax validation if the intent is to allow non-HTTPS URLs with a warning during dryrun.
| // SetBaseDir sets the base directory for resolving relative file paths in custom templates. | ||
| func (d *DependencyResolver) SetBaseDir(dir string) { | ||
| d.baseDir = dir | ||
| } |
There was a problem hiding this comment.
DependencyResolver supports baseDir for resolving relative custom template file paths, but there are no non-test call sites setting it (SetBaseDir only appears in tests). As a result, relative paths will resolve relative to the process working directory rather than the config file directory (which also contradicts the docs). Consider wiring the config file directory into the resolver (e.g., via NewDependencies(...) param or by setting it in Provisioner.Run / CLI config load path).
| return d.executeCustomTemplate(buf, tpl) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // executeCustomTemplate loads and executes a single custom template using the templates package. | ||
| func (d *DependencyResolver) executeCustomTemplate(buf *bytes.Buffer, tpl v1alpha1.CustomTemplate) error { | ||
| content, err := templates.LoadCustomTemplate(tpl, d.baseDir) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| executor := templates.NewCustomTemplateExecutor(tpl, content) | ||
| return executor.Execute(buf, *d.env) |
There was a problem hiding this comment.
The custom template ProvisionFunc closure ignores its "env" argument and later executeCustomTemplate uses *d.env instead. This differs from other dependencies (which operate on the env passed to the ProvisionFunc) and can produce surprising behavior if the caller executes the returned funcs with a different env value than the resolver was created with. Prefer passing the ProvisionFunc's env through to executeCustomTemplate / executor.Execute.
| return d.executeCustomTemplate(buf, tpl) | |
| }) | |
| } | |
| } | |
| // executeCustomTemplate loads and executes a single custom template using the templates package. | |
| func (d *DependencyResolver) executeCustomTemplate(buf *bytes.Buffer, tpl v1alpha1.CustomTemplate) error { | |
| content, err := templates.LoadCustomTemplate(tpl, d.baseDir) | |
| if err != nil { | |
| return err | |
| } | |
| executor := templates.NewCustomTemplateExecutor(tpl, content) | |
| return executor.Execute(buf, *d.env) | |
| return d.executeCustomTemplate(buf, env, tpl) | |
| }) | |
| } | |
| } | |
| // executeCustomTemplate loads and executes a single custom template using the templates package. | |
| func (d *DependencyResolver) executeCustomTemplate(buf *bytes.Buffer, env v1alpha1.Environment, tpl v1alpha1.CustomTemplate) error { | |
| content, err := templates.LoadCustomTemplate(tpl, d.baseDir) | |
| if err != nil { | |
| return err | |
| } | |
| executor := templates.NewCustomTemplateExecutor(tpl, content) | |
| return executor.Execute(buf, env) |
| timeout: 600 # Optional: timeout in seconds (default: 600) | ||
| continueOnError: false # Optional: continue provisioning on failure (default: false) |
There was a problem hiding this comment.
The guide documents a per-template timeout setting, but the generated custom template script/execution path does not appear to enforce this timeout (it isn't used in the executor script generation, and provisioner execution uses session.Wait() without a timeout). Either implement timeout enforcement or clarify in the docs that this field is currently not enforced.
Summary
DependencyResolverwith five execution phases: pre-install, post-runtime, post-toolkit, post-kubernetes, post-installDryrun()(phase validation, source presence check)docs/guides/custom-templates.mdCloses #565 (partial - resolver/dryrun/docs portion)
Test plan
go vet ./pkg/provisioner/...clean