mirror of
https://github.com/coder/coder.git
synced 2026-09-21 20:51:01 +08:00
fix: render base variables into templates instead of tfvars (DEVEX-287) (#26436)
Part of the Template Builder wizard PR stack.
## Problem
The kubernetes base template used Terraform `variable` blocks and
`var.*` references for `use_kubeconfig` and `namespace`, but the
composed tar bundle never included a `.tfvars` file. This caused
`terraform plan` to fail with "required template variables need values:
namespace".
## Fix
Base templates now use Go template variables (`{{ .Variables.* }}`) just
like module templates do. Values are validated, HCL-quoted, and rendered
directly into the output HCL.
Also adds explicit "variable is required" validation to both
`mergeBaseVariables` and `mergeModuleVariables`, replacing the previous
reliance on `missingkey=error` at render time for clearer error
messages.
---
> [!NOTE]
> Generated by Coder Agents on behalf of @jeremyruppel
This commit is contained in:
@@ -174,9 +174,24 @@ func DefaultBaseRenderContext(exampleID string) BaseRenderContext {
|
||||
if err != nil || bases[exampleID] == nil {
|
||||
return BaseRenderContext{}
|
||||
}
|
||||
dc := bases[exampleID].Manifest.DefaultContext
|
||||
base := bases[exampleID]
|
||||
dc := base.Manifest.DefaultContext
|
||||
|
||||
// Populate Variables from manifest defaults so that Go template
|
||||
// rendering succeeds even without caller-supplied values.
|
||||
vars := make(map[string]string, len(base.Manifest.Variables))
|
||||
for _, v := range base.Manifest.Variables {
|
||||
if v.Computed || v.Sensitive {
|
||||
continue
|
||||
}
|
||||
if len(v.Default) > 0 && isSimpleJSONValue(v.Default) {
|
||||
vars[v.Name] = string(v.Default)
|
||||
}
|
||||
}
|
||||
|
||||
return BaseRenderContext{
|
||||
ContainerImage: dc.ContainerImage,
|
||||
Variables: vars,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -12,23 +12,9 @@ terraform {
|
||||
provider "coder" {
|
||||
}
|
||||
|
||||
variable "use_kubeconfig" {
|
||||
type = bool
|
||||
description = <<-EOF
|
||||
Use host kubeconfig? (true/false)
|
||||
|
||||
Set this to false if the Coder host is itself running as a Pod on the same
|
||||
Kubernetes cluster as you are deploying workspaces to.
|
||||
|
||||
Set this to true if the Coder host is running outside the Kubernetes cluster
|
||||
for workspaces. A valid "~/.kube/config" must be present on the Coder host.
|
||||
EOF
|
||||
default = false
|
||||
}
|
||||
|
||||
variable "namespace" {
|
||||
type = string
|
||||
description = "The Kubernetes namespace to create workspaces in (must exist prior to creating workspaces). If the Coder host is itself running as a Pod on the same Kubernetes cluster as you are deploying workspaces to, set this to the same namespace."
|
||||
provider "kubernetes" {
|
||||
# Authenticate via ~/.kube/config or a Coder-specific ServiceAccount, depending on admin preferences
|
||||
config_path = {{ .Variables.use_kubeconfig }} == true ? "~/.kube/config" : null
|
||||
}
|
||||
|
||||
data "coder_parameter" "cpu" {
|
||||
@@ -95,11 +81,6 @@ data "coder_parameter" "home_disk_size" {
|
||||
}
|
||||
}
|
||||
|
||||
provider "kubernetes" {
|
||||
# Authenticate via ~/.kube/config or a Coder-specific ServiceAccount, depending on admin preferences
|
||||
config_path = var.use_kubeconfig == true ? "~/.kube/config" : null
|
||||
}
|
||||
|
||||
data "coder_workspace" "me" {}
|
||||
data "coder_workspace_owner" "me" {}
|
||||
{{ if .ImageOptions }}
|
||||
@@ -185,7 +166,7 @@ resource "coder_agent" "main" {
|
||||
resource "kubernetes_persistent_volume_claim_v1" "home" {
|
||||
metadata {
|
||||
name = "coder-${data.coder_workspace.me.id}-home"
|
||||
namespace = var.namespace
|
||||
namespace = {{ .Variables.namespace }}
|
||||
labels = {
|
||||
"app.kubernetes.io/name" = "coder-pvc"
|
||||
"app.kubernetes.io/instance" = "coder-pvc-${data.coder_workspace.me.id}"
|
||||
@@ -220,7 +201,7 @@ resource "kubernetes_deployment_v1" "main" {
|
||||
wait_for_rollout = false
|
||||
metadata {
|
||||
name = "coder-${data.coder_workspace.me.id}"
|
||||
namespace = var.namespace
|
||||
namespace = {{ .Variables.namespace }}
|
||||
labels = {
|
||||
"app.kubernetes.io/name" = "coder-workspace"
|
||||
"app.kubernetes.io/instance" = "coder-workspace-${data.coder_workspace.me.id}"
|
||||
|
||||
@@ -104,7 +104,13 @@ func renderBase(baseTemplateID string, baseVars map[string]string) ([]byte, erro
|
||||
if renderCtx.Variables == nil {
|
||||
renderCtx.Variables = make(map[string]string)
|
||||
}
|
||||
maps.Copy(renderCtx.Variables, baseVars)
|
||||
|
||||
vars, err := mergeBaseVariables(baseTemplateID, baseVars)
|
||||
if err != nil {
|
||||
return nil, xerrors.Errorf("base %q: %w", baseTemplateID, err)
|
||||
}
|
||||
maps.Copy(renderCtx.Variables, vars)
|
||||
|
||||
mainTF, err := RenderBaseTemplate(baseTemplateID, "main.tf.tmpl", renderCtx)
|
||||
if err != nil {
|
||||
return nil, xerrors.Errorf("render base template: %w", err)
|
||||
@@ -112,6 +118,63 @@ func renderBase(baseTemplateID string, baseVars map[string]string) ([]byte, erro
|
||||
return mainTF, nil
|
||||
}
|
||||
|
||||
// mergeBaseVariables builds the final Variables map for a base template.
|
||||
// It starts with manifest defaults, overlays caller-supplied values,
|
||||
// validates types, and converts to HCL literals.
|
||||
func mergeBaseVariables(baseTemplateID string, callerVars map[string]string) (map[string]string, error) {
|
||||
allVars := BaseVariables(baseTemplateID)
|
||||
if len(allVars) == 0 && len(callerVars) == 0 {
|
||||
return make(map[string]string), nil
|
||||
}
|
||||
|
||||
allowedVars := make(map[string]ModuleVariable, len(allVars))
|
||||
for _, v := range allVars {
|
||||
if v.Computed || v.Sensitive {
|
||||
continue
|
||||
}
|
||||
allowedVars[v.Name] = v
|
||||
}
|
||||
|
||||
// Validate caller-supplied keys and values.
|
||||
for k, val := range callerVars {
|
||||
v, ok := allowedVars[k]
|
||||
if !ok {
|
||||
return nil, xerrors.Errorf("unknown variable %q", k)
|
||||
}
|
||||
if err := validateVariableValue(v, val); err != nil {
|
||||
return nil, xerrors.Errorf("variable %q: %w", k, err)
|
||||
}
|
||||
}
|
||||
|
||||
// Build merged map from manifest defaults.
|
||||
merged := make(map[string]string, len(allVars))
|
||||
for _, v := range allVars {
|
||||
if v.Computed || v.Sensitive {
|
||||
continue
|
||||
}
|
||||
if len(v.Default) > 0 && isSimpleJSONValue(v.Default) {
|
||||
merged[v.Name] = string(v.Default)
|
||||
}
|
||||
}
|
||||
|
||||
// Overlay validated caller values, converting to HCL literals.
|
||||
for k, val := range callerVars {
|
||||
merged[k] = toHCLLiteral(allowedVars[k], val)
|
||||
}
|
||||
|
||||
// Ensure all required variables without defaults have a value.
|
||||
for _, v := range allVars {
|
||||
if v.Computed || v.Sensitive {
|
||||
continue
|
||||
}
|
||||
if v.Required && merged[v.Name] == "" {
|
||||
return nil, xerrors.Errorf("variable %q is required", v.Name)
|
||||
}
|
||||
}
|
||||
|
||||
return merged, nil
|
||||
}
|
||||
|
||||
// loadCatalogMap loads the module catalog and returns it as a map keyed
|
||||
// by module ID.
|
||||
func loadCatalogMap() (map[string]ModuleManifest, error) {
|
||||
@@ -248,6 +311,17 @@ func mergeModuleVariables(manifest ModuleManifest, callerVars map[string]string)
|
||||
for k, val := range callerVars {
|
||||
merged[k] = toHCLLiteral(allowedVars[k], val)
|
||||
}
|
||||
|
||||
// Ensure all required variables without defaults have a value.
|
||||
for _, v := range manifest.Variables {
|
||||
if v.Computed || v.Sensitive {
|
||||
continue
|
||||
}
|
||||
if v.Required && merged[v.Name] == "" {
|
||||
return nil, xerrors.Errorf("variable %q is required", v.Name)
|
||||
}
|
||||
}
|
||||
|
||||
return merged, nil
|
||||
}
|
||||
|
||||
|
||||
@@ -55,9 +55,13 @@ func TestMergeModuleVariables(t *testing.T) {
|
||||
},
|
||||
}
|
||||
|
||||
requiredVars := map[string]string{
|
||||
"required_no_default": "value",
|
||||
}
|
||||
|
||||
t.Run("DefaultsApplied", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
merged, err := mergeModuleVariables(manifest, nil)
|
||||
merged, err := mergeModuleVariables(manifest, requiredVars)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, "13337", merged["port"])
|
||||
require.Equal(t, "false", merged["enabled"])
|
||||
@@ -65,7 +69,7 @@ func TestMergeModuleVariables(t *testing.T) {
|
||||
|
||||
t.Run("ComputedAndSensitiveSkipped", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
merged, err := mergeModuleVariables(manifest, nil)
|
||||
merged, err := mergeModuleVariables(manifest, requiredVars)
|
||||
require.NoError(t, err)
|
||||
require.NotContains(t, merged, "agent_id")
|
||||
require.NotContains(t, merged, "api_key")
|
||||
@@ -73,22 +77,24 @@ func TestMergeModuleVariables(t *testing.T) {
|
||||
|
||||
t.Run("NonRequiredWithoutDefaultGetsNull", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
merged, err := mergeModuleVariables(manifest, nil)
|
||||
merged, err := mergeModuleVariables(manifest, requiredVars)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, "null", merged["optional_no_default"])
|
||||
})
|
||||
|
||||
t.Run("RequiredWithoutDefaultOmitted", func(t *testing.T) {
|
||||
t.Run("RequiredWithoutDefaultIsRequired", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
merged, err := mergeModuleVariables(manifest, nil)
|
||||
require.NoError(t, err)
|
||||
require.NotContains(t, merged, "required_no_default")
|
||||
_, err := mergeModuleVariables(manifest, nil)
|
||||
require.Error(t, err)
|
||||
require.Contains(t, err.Error(), `variable "required_no_default"`)
|
||||
require.Contains(t, err.Error(), "is required")
|
||||
})
|
||||
|
||||
t.Run("CallerOverridesDefault", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
merged, err := mergeModuleVariables(manifest, map[string]string{
|
||||
"port": "9999",
|
||||
"port": "9999",
|
||||
"required_no_default": "value",
|
||||
})
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, "9999", merged["port"])
|
||||
@@ -166,6 +172,7 @@ func TestMergeModuleVariables(t *testing.T) {
|
||||
"port": "null",
|
||||
"enabled": "null",
|
||||
"optional_no_default": "null",
|
||||
"required_no_default": "null",
|
||||
})
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, "null", merged["port"])
|
||||
@@ -173,9 +180,11 @@ func TestMergeModuleVariables(t *testing.T) {
|
||||
require.Equal(t, "null", merged["optional_no_default"])
|
||||
})
|
||||
|
||||
t.Run("EmptyCallerVarsNoError", func(t *testing.T) {
|
||||
t.Run("EmptyCallerVarsUsesDefaults", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
merged, err := mergeModuleVariables(manifest, map[string]string{})
|
||||
merged, err := mergeModuleVariables(manifest, map[string]string{
|
||||
"required_no_default": "value",
|
||||
})
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, "13337", merged["port"])
|
||||
})
|
||||
|
||||
@@ -237,7 +237,8 @@ func TestCompose(t *testing.T) {
|
||||
},
|
||||
})
|
||||
require.Error(t, err)
|
||||
require.Contains(t, err.Error(), "render module")
|
||||
require.Contains(t, err.Error(), `variable "url"`)
|
||||
require.Contains(t, err.Error(), "is required")
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -58,6 +58,10 @@ func TestRenderBaseTemplate(t *testing.T) {
|
||||
renderCtx := templatebuilder.BaseRenderContext{
|
||||
ContainerImage: "custom/image:latest",
|
||||
ImageOptions: imageOpts,
|
||||
Variables: map[string]string{
|
||||
"namespace": `"test-ns"`,
|
||||
"use_kubeconfig": "false",
|
||||
},
|
||||
}
|
||||
out, err := templatebuilder.RenderBaseTemplate("kubernetes", "main.tf.tmpl", renderCtx)
|
||||
require.NoError(t, err)
|
||||
|
||||
+5
-24
@@ -12,23 +12,9 @@ terraform {
|
||||
provider "coder" {
|
||||
}
|
||||
|
||||
variable "use_kubeconfig" {
|
||||
type = bool
|
||||
description = <<-EOF
|
||||
Use host kubeconfig? (true/false)
|
||||
|
||||
Set this to false if the Coder host is itself running as a Pod on the same
|
||||
Kubernetes cluster as you are deploying workspaces to.
|
||||
|
||||
Set this to true if the Coder host is running outside the Kubernetes cluster
|
||||
for workspaces. A valid "~/.kube/config" must be present on the Coder host.
|
||||
EOF
|
||||
default = false
|
||||
}
|
||||
|
||||
variable "namespace" {
|
||||
type = string
|
||||
description = "The Kubernetes namespace to create workspaces in (must exist prior to creating workspaces). If the Coder host is itself running as a Pod on the same Kubernetes cluster as you are deploying workspaces to, set this to the same namespace."
|
||||
provider "kubernetes" {
|
||||
# Authenticate via ~/.kube/config or a Coder-specific ServiceAccount, depending on admin preferences
|
||||
config_path = false == true ? "~/.kube/config" : null
|
||||
}
|
||||
|
||||
data "coder_parameter" "cpu" {
|
||||
@@ -95,11 +81,6 @@ data "coder_parameter" "home_disk_size" {
|
||||
}
|
||||
}
|
||||
|
||||
provider "kubernetes" {
|
||||
# Authenticate via ~/.kube/config or a Coder-specific ServiceAccount, depending on admin preferences
|
||||
config_path = var.use_kubeconfig == true ? "~/.kube/config" : null
|
||||
}
|
||||
|
||||
data "coder_workspace" "me" {}
|
||||
data "coder_workspace_owner" "me" {}
|
||||
|
||||
@@ -172,7 +153,7 @@ resource "coder_agent" "main" {
|
||||
resource "kubernetes_persistent_volume_claim_v1" "home" {
|
||||
metadata {
|
||||
name = "coder-${data.coder_workspace.me.id}-home"
|
||||
namespace = var.namespace
|
||||
namespace = <no value>
|
||||
labels = {
|
||||
"app.kubernetes.io/name" = "coder-pvc"
|
||||
"app.kubernetes.io/instance" = "coder-pvc-${data.coder_workspace.me.id}"
|
||||
@@ -207,7 +188,7 @@ resource "kubernetes_deployment_v1" "main" {
|
||||
wait_for_rollout = false
|
||||
metadata {
|
||||
name = "coder-${data.coder_workspace.me.id}"
|
||||
namespace = var.namespace
|
||||
namespace = <no value>
|
||||
labels = {
|
||||
"app.kubernetes.io/name" = "coder-workspace"
|
||||
"app.kubernetes.io/instance" = "coder-workspace-${data.coder_workspace.me.id}"
|
||||
|
||||
Reference in New Issue
Block a user