From 40a77ea74bab3eafedbedc28cb71e273d51e7885 Mon Sep 17 00:00:00 2001 From: Stephen Kirby Date: Fri, 1 May 2026 20:49:46 +0000 Subject: [PATCH 1/3] fix(internal/provider): handle unknown values in tf_vars and provisioner_tags Change TerraformVariables and ProvisionerTags fields in TemplateVersion from Go native slices ([]Variable) to types.Set, matching the existing SetNestedAttribute schema definition. Go slices cannot represent unknown Terraform values during plan, causing Value Conversion Error when these fields receive dynamic values through modules or variable interpolation. Add varsFromSet helper to centralize Set-to-slice conversion with proper null/unknown handling at all four call sites. Closes #305 --- .../provider/template_data_source_test.go | 4 +- internal/provider/template_resource.go | 75 +++++++-- internal/provider/template_resource_test.go | 158 ++++++++++++++---- 3 files changed, 194 insertions(+), 43 deletions(-) diff --git a/internal/provider/template_data_source_test.go b/internal/provider/template_data_source_test.go index 8e0e4dc..b7d2a7e 100644 --- a/internal/provider/template_data_source_test.go +++ b/internal/provider/template_data_source_test.go @@ -35,12 +35,12 @@ func TestAccTemplateDataSource(t *testing.T) { Name: types.StringValue("main"), Message: types.StringValue("Initial commit"), Directory: types.StringValue("../../integration/template-test/example-template/"), - TerraformVariables: []Variable{ + TerraformVariables: mustVariableSet([]Variable{ { Name: types.StringValue("name"), Value: types.StringValue("world"), }, - }, + }), }, }) require.NoError(t, err) diff --git a/internal/provider/template_resource.go b/internal/provider/template_resource.go index f053773..f35ff29 100644 --- a/internal/provider/template_resource.go +++ b/internal/provider/template_resource.go @@ -163,8 +163,8 @@ type TemplateVersion struct { Directory types.String `tfsdk:"directory"` DirectoryHash types.String `tfsdk:"directory_hash"` Active types.Bool `tfsdk:"active"` - TerraformVariables []Variable `tfsdk:"tf_vars"` - ProvisionerTags []Variable `tfsdk:"provisioner_tags"` + TerraformVariables types.Set `tfsdk:"tf_vars"` + ProvisionerTags types.Set `tfsdk:"provisioner_tags"` } type Versions []TemplateVersion @@ -183,6 +183,34 @@ type Variable struct { Value types.String `tfsdk:"value"` } +// variableAttrTypes returns the attribute type map for Variable objects +// used to construct types.Set values. +func variableAttrTypes() map[string]attr.Type { + return map[string]attr.Type{ + "name": types.StringType, + "value": types.StringType, + } +} + +// variableObjectType returns the object type used as the element type +// in Set attributes for Variable. +func variableObjectType() types.ObjectType { + return types.ObjectType{ + AttrTypes: variableAttrTypes(), + } +} + +// varsFromSet converts a types.Set to a slice of Variable structs. +// It returns nil with no error when the set is null or unknown. +func varsFromSet(ctx context.Context, varSet types.Set) ([]Variable, diag.Diagnostics) { + if varSet.IsNull() || varSet.IsUnknown() { + return nil, nil + } + var vars []Variable + diags := varSet.ElementsAs(ctx, &vars, false) + return vars, diags +} + var variableNestedObject = schema.NestedAttributeObject{ Attributes: map[string]schema.Attribute{ "name": schema.StringAttribute{ @@ -1043,7 +1071,7 @@ func (d *versionsPlanModifier) PlanModifyList(ctx context.Context, req planmodif } } - diag = planVersions.reconcileVersionIDs(lv, configVersions, hasActiveVersion) + diag = planVersions.reconcileVersionIDs(ctx, lv, configVersions, hasActiveVersion) if diag.HasError() { resp.Diagnostics.Append(diag...) return @@ -1199,14 +1227,22 @@ func newVersion(ctx context.Context, client *codersdk.Client, req newVersionRequ tflog.Info(ctx, "discovered and parsed vars files", map[string]any{ "vars": vars, }) - for _, variable := range req.Version.TerraformVariables { + tfVarSlice, varDiags := varsFromSet(ctx, req.Version.TerraformVariables) + if varDiags.HasError() { + return nil, logs, fmt.Errorf("failed to convert terraform variables: %s", varDiags.Errors()) + } + for _, variable := range tfVarSlice { vars = append(vars, codersdk.VariableValue{ Name: variable.Name.ValueString(), Value: variable.Value.ValueString(), }) } - provTags := make(map[string]string, len(req.Version.ProvisionerTags)) - for _, provisionerTag := range req.Version.ProvisionerTags { + provTagSlice, tagDiags := varsFromSet(ctx, req.Version.ProvisionerTags) + if tagDiags.HasError() { + return nil, logs, fmt.Errorf("failed to convert provisioner tags: %s", tagDiags.Errors()) + } + provTags := make(map[string]string, len(provTagSlice)) + for _, provisionerTag := range provTagSlice { provTags[provisionerTag.Name.ValueString()] = provisionerTag.Value.ValueString() } tmplVerReq := codersdk.CreateTemplateVersionRequest{ @@ -1444,8 +1480,13 @@ func (v Versions) setPrivateState(ctx context.Context, ps privateState) (diags d lv := make(LastVersionsByHash) for _, version := range v { vbh, ok := lv[version.DirectoryHash.ValueString()] - tfVars := make(map[string]string, len(version.TerraformVariables)) - for _, tfVar := range version.TerraformVariables { + tfVarSlice, varDiags := varsFromSet(ctx, version.TerraformVariables) + if varDiags.HasError() { + diags.Append(varDiags...) + return diags + } + tfVars := make(map[string]string, len(tfVarSlice)) + for _, tfVar := range tfVarSlice { tfVars[tfVar.Name.ValueString()] = tfVar.Value.ValueString() } // Store the IDs and names of all versions with the same directory hash, @@ -1476,7 +1517,7 @@ func (v Versions) setPrivateState(ctx context.Context, ps privateState) (diags d return ps.SetKey(ctx, LastVersionsKey, lvBytes) } -func (planVersions Versions) reconcileVersionIDs(lv LastVersionsByHash, configVersions Versions, hasOneActiveVersion bool) (diag diag.Diagnostics) { +func (planVersions Versions) reconcileVersionIDs(ctx context.Context, lv LastVersionsByHash, configVersions Versions, hasOneActiveVersion bool) (diag diag.Diagnostics) { // We remove versions that we've matched from `lv`, so make a copy for // resolving tfvar changes at the end. fullLv := make(LastVersionsByHash) @@ -1532,7 +1573,7 @@ func (planVersions Versions) reconcileVersionIDs(lv LastVersionsByHash, configVe if !ok { continue } - if tfVariablesChanged(prevs, &planVersions[i]) { + if tfVariablesChanged(ctx, prevs, &planVersions[i]) { planVersions[i].ID = NewUUIDUnknown() // We could always set the name to unknown here, to generate a // random one (this is what the Web UI currently does when @@ -1581,7 +1622,7 @@ func versionDeactivated(prevs []PreviousTemplateVersion, planned *TemplateVersio return false } -func tfVariablesChanged(prevs []PreviousTemplateVersion, planned *TemplateVersion) bool { +func tfVariablesChanged(ctx context.Context, prevs []PreviousTemplateVersion, planned *TemplateVersion) bool { for _, prev := range prevs { if prev.ID == planned.ID.ValueUUID() { // If the previous version has no TFVars, then it was created using @@ -1589,7 +1630,16 @@ func tfVariablesChanged(prevs []PreviousTemplateVersion, planned *TemplateVersio if prev.TFVars == nil { return true } - for _, tfVar := range planned.TerraformVariables { + plannedVars, diags := varsFromSet(ctx, planned.TerraformVariables) + if diags.HasError() { + return true + } + // If the set is unknown or null, we cannot compare and + // must treat it as changed. + if planned.TerraformVariables.IsUnknown() || planned.TerraformVariables.IsNull() { + return true + } + for _, tfVar := range plannedVars { if prev.TFVars[tfVar.Name.ValueString()] != tfVar.Value.ValueString() { return true } @@ -1598,7 +1648,6 @@ func tfVariablesChanged(prevs []PreviousTemplateVersion, planned *TemplateVersio } } return true - } func formatLogs(err error, logs []codersdk.ProvisionerJobLog) string { diff --git a/internal/provider/template_resource_test.go b/internal/provider/template_resource_test.go index 971de6e..c58d4c6 100644 --- a/internal/provider/template_resource_test.go +++ b/internal/provider/template_resource_test.go @@ -11,6 +11,7 @@ import ( "text/template" "github.com/google/uuid" + "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/hashicorp/terraform-plugin-testing/terraform" @@ -22,6 +23,22 @@ import ( "github.com/coder/terraform-provider-coderd/integration" ) +// mustVariableSet creates a types.Set from a slice of Variable structs +// for tests. +func mustVariableSet(vars []Variable) types.Set { + if len(vars) == 0 { + return types.SetValueMust(variableObjectType(), []attr.Value{}) + } + vals := make([]attr.Value, len(vars)) + for i, v := range vars { + vals[i] = types.ObjectValueMust(variableAttrTypes(), map[string]attr.Value{ + "name": v.Name, + "value": v.Value, + }) + } + return types.SetValueMust(variableObjectType(), vals) +} + func TestAccTemplateResource(t *testing.T) { t.Parallel() if os.Getenv("TF_ACC") == "" { @@ -1245,13 +1262,13 @@ func TestReconcileVersionIDs(t *testing.T) { Name: types.StringValue("foo"), DirectoryHash: types.StringValue("aaa"), ID: NewUUIDUnknown(), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), }, { Name: types.StringValue("bar"), DirectoryHash: types.StringValue("aaa"), ID: NewUUIDUnknown(), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), }, }, configVersions: []TemplateVersion{ @@ -1276,13 +1293,13 @@ func TestReconcileVersionIDs(t *testing.T) { Name: types.StringValue("foo"), DirectoryHash: types.StringValue("aaa"), ID: NewUUIDUnknown(), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), }, { Name: types.StringValue("bar"), DirectoryHash: types.StringValue("aaa"), ID: UUIDValue(aUUID), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), }, }, }, @@ -1293,13 +1310,13 @@ func TestReconcileVersionIDs(t *testing.T) { Name: types.StringValue("foo"), DirectoryHash: types.StringValue("aaa"), ID: NewUUIDUnknown(), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), }, { Name: types.StringValue("bar"), DirectoryHash: types.StringValue("aaa"), ID: NewUUIDUnknown(), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), }, }, configVersions: []TemplateVersion{ @@ -1324,13 +1341,13 @@ func TestReconcileVersionIDs(t *testing.T) { Name: types.StringValue("foo"), DirectoryHash: types.StringValue("aaa"), ID: UUIDValue(aUUID), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), }, { Name: types.StringValue("bar"), DirectoryHash: types.StringValue("aaa"), ID: NewUUIDUnknown(), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), }, }, }, @@ -1341,13 +1358,13 @@ func TestReconcileVersionIDs(t *testing.T) { Name: types.StringValue("foo"), DirectoryHash: types.StringValue("aaa"), ID: NewUUIDUnknown(), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), }, { Name: types.StringValue("bar"), DirectoryHash: types.StringValue("aaa"), ID: NewUUIDUnknown(), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), }, }, configVersions: []TemplateVersion{ @@ -1377,13 +1394,13 @@ func TestReconcileVersionIDs(t *testing.T) { Name: types.StringValue("foo"), DirectoryHash: types.StringValue("aaa"), ID: UUIDValue(aUUID), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), }, { Name: types.StringValue("bar"), DirectoryHash: types.StringValue("aaa"), ID: UUIDValue(bUUID), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), }, }, }, @@ -1394,13 +1411,13 @@ func TestReconcileVersionIDs(t *testing.T) { Name: types.StringValue("foo"), DirectoryHash: types.StringValue("aaa"), ID: NewUUIDUnknown(), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), }, { Name: types.StringUnknown(), DirectoryHash: types.StringValue("aaa"), ID: NewUUIDUnknown(), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), }, }, configVersions: []TemplateVersion{ @@ -1430,13 +1447,13 @@ func TestReconcileVersionIDs(t *testing.T) { Name: types.StringValue("foo"), DirectoryHash: types.StringValue("aaa"), ID: UUIDValue(aUUID), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), }, { Name: types.StringValue("baz"), DirectoryHash: types.StringValue("aaa"), ID: UUIDValue(bUUID), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), }, }, }, @@ -1447,7 +1464,7 @@ func TestReconcileVersionIDs(t *testing.T) { Name: types.StringValue("weird_draught12"), DirectoryHash: types.StringValue("bbb"), ID: UUIDValue(aUUID), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), }, }, configVersions: []TemplateVersion{ @@ -1469,7 +1486,7 @@ func TestReconcileVersionIDs(t *testing.T) { Name: types.StringUnknown(), DirectoryHash: types.StringValue("bbb"), ID: NewUUIDUnknown(), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), }, }, }, @@ -1480,12 +1497,12 @@ func TestReconcileVersionIDs(t *testing.T) { Name: types.StringValue("foo"), DirectoryHash: types.StringValue("aaa"), ID: UUIDValue(aUUID), - TerraformVariables: []Variable{ + TerraformVariables: mustVariableSet([]Variable{ { Name: types.StringValue("foo"), Value: types.StringValue("bar"), }, - }, + }), }, }, configVersions: []TemplateVersion{ @@ -1509,12 +1526,12 @@ func TestReconcileVersionIDs(t *testing.T) { Name: types.StringValue("foo"), DirectoryHash: types.StringValue("aaa"), ID: NewUUIDUnknown(), - TerraformVariables: []Variable{ + TerraformVariables: mustVariableSet([]Variable{ { Name: types.StringValue("foo"), Value: types.StringValue("bar"), }, - }, + }), }, }, }, @@ -1525,12 +1542,12 @@ func TestReconcileVersionIDs(t *testing.T) { Name: types.StringValue("foo"), DirectoryHash: types.StringValue("aaa"), ID: UUIDValue(aUUID), - TerraformVariables: []Variable{ + TerraformVariables: mustVariableSet([]Variable{ { Name: types.StringValue("foo"), Value: types.StringValue("bar"), }, - }, + }), }, }, configVersions: []TemplateVersion{ @@ -1554,12 +1571,12 @@ func TestReconcileVersionIDs(t *testing.T) { Name: types.StringValue("foo"), DirectoryHash: types.StringValue("aaa"), ID: UUIDValue(aUUID), - TerraformVariables: []Variable{ + TerraformVariables: mustVariableSet([]Variable{ { Name: types.StringValue("foo"), Value: types.StringValue("bar"), }, - }, + }), }, }, }, @@ -1570,7 +1587,7 @@ func TestReconcileVersionIDs(t *testing.T) { Name: types.StringValue("foo"), DirectoryHash: types.StringValue("aaa"), ID: NewUUIDUnknown(), - TerraformVariables: []Variable{}, + TerraformVariables: mustVariableSet([]Variable{}), Active: types.BoolValue(false), }, }, @@ -1599,7 +1616,7 @@ func TestReconcileVersionIDs(t *testing.T) { t.Run(c.Name, func(t *testing.T) { t.Parallel() - diag := c.planVersions.reconcileVersionIDs(c.inputState, c.configVersions, c.cfgHasActiveVersion) + diag := c.planVersions.reconcileVersionIDs(context.Background(), c.inputState, c.configVersions, c.cfgHasActiveVersion) if c.expectError { require.True(t, diag.HasError()) } else { @@ -1609,3 +1626,88 @@ func TestReconcileVersionIDs(t *testing.T) { } } + +// TestUnknownTerraformVariablesHandling verifies that types.Set fields +// for TerraformVariables and ProvisionerTags correctly handle unknown +// values during plan time, which is the fix for issue #305. +func TestUnknownTerraformVariablesHandling(t *testing.T) { + t.Parallel() + + // Construct a TemplateVersion with an unknown TerraformVariables set, + // simulating what happens when tf_vars references a variable that + // is not yet known at plan time. + unknownSet := types.SetUnknown(variableObjectType()) + nullSet := types.SetNull(variableObjectType()) + + // Verify varsFromSet handles unknown sets gracefully. + ctx := context.Background() + vars, diags := varsFromSet(ctx, unknownSet) + require.False(t, diags.HasError(), "varsFromSet should not error on unknown set") + require.Nil(t, vars, "varsFromSet should return nil for unknown set") + + // Verify varsFromSet handles null sets gracefully. + vars, diags = varsFromSet(ctx, nullSet) + require.False(t, diags.HasError(), "varsFromSet should not error on null set") + require.Nil(t, vars, "varsFromSet should return nil for null set") + + // Verify varsFromSet handles a populated set correctly. + populatedSet := mustVariableSet([]Variable{ + { + Name: types.StringValue("foo"), + Value: types.StringValue("bar"), + }, + { + Name: types.StringValue("baz"), + Value: types.StringValue("qux"), + }, + }) + vars, diags = varsFromSet(ctx, populatedSet) + require.False(t, diags.HasError(), "varsFromSet should not error on populated set") + require.Len(t, vars, 2, "varsFromSet should return 2 variables") + + // Verify that the TemplateVersion struct can hold unknown sets + // without causing panics (this is the core of issue #305). + tv := TemplateVersion{ + ID: NewUUIDUnknown(), + Name: types.StringValue("test"), + DirectoryHash: types.StringValue("abc"), + TerraformVariables: unknownSet, + ProvisionerTags: nullSet, + } + require.True(t, tv.TerraformVariables.IsUnknown()) + require.True(t, tv.ProvisionerTags.IsNull()) + + // Verify tfVariablesChanged handles unknown sets. + prevs := []PreviousTemplateVersion{ + { + ID: uuid.New(), + Name: "test", + TFVars: map[string]string{"foo": "bar"}, + }, + } + // Unknown set should be treated as changed. + tvWithUnknown := TemplateVersion{ + ID: UUIDValue(prevs[0].ID), + TerraformVariables: unknownSet, + } + require.True(t, tfVariablesChanged(ctx, prevs, &tvWithUnknown), + "unknown tf_vars should be treated as changed") + + // Null set should be treated as changed. + tvWithNull := TemplateVersion{ + ID: UUIDValue(prevs[0].ID), + TerraformVariables: nullSet, + } + require.True(t, tfVariablesChanged(ctx, prevs, &tvWithNull), + "null tf_vars should be treated as changed") + + // Set with same values should not be treated as changed. + tvWithSame := TemplateVersion{ + ID: UUIDValue(prevs[0].ID), + TerraformVariables: mustVariableSet([]Variable{ + {Name: types.StringValue("foo"), Value: types.StringValue("bar")}, + }), + } + require.False(t, tfVariablesChanged(ctx, prevs, &tvWithSame), + "identical tf_vars should not be treated as changed") +} From d047065fb4e0b52eaa71fdddc79a231dc499b0b7 Mon Sep 17 00:00:00 2001 From: Stephen Kirby Date: Fri, 1 May 2026 20:51:28 +0000 Subject: [PATCH 2/3] style: fix struct field alignment for gofmt --- internal/provider/template_resource.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/provider/template_resource.go b/internal/provider/template_resource.go index f35ff29..73b48f8 100644 --- a/internal/provider/template_resource.go +++ b/internal/provider/template_resource.go @@ -163,8 +163,8 @@ type TemplateVersion struct { Directory types.String `tfsdk:"directory"` DirectoryHash types.String `tfsdk:"directory_hash"` Active types.Bool `tfsdk:"active"` - TerraformVariables types.Set `tfsdk:"tf_vars"` - ProvisionerTags types.Set `tfsdk:"provisioner_tags"` + TerraformVariables types.Set `tfsdk:"tf_vars"` + ProvisionerTags types.Set `tfsdk:"provisioner_tags"` } type Versions []TemplateVersion From ac4d09708236074d9071f761ad8b6eab467fe232 Mon Sep 17 00:00:00 2001 From: ju-pe Date: Sun, 3 May 2026 21:59:36 +0000 Subject: [PATCH 3/3] fix: avoid stripping cty sensitivity marks during plan modification When coderd_template has sensitive tf_vars and a deferred dependency (like time_static) that gets replaced during apply, Terraform re-plans the resource. The previous plan modifier used types.ListValueFrom() to write the entire versions list back, which reconstructed tftypes values and stripped Terraform core's cty-level sensitivity marks. This caused: 'Provider produced inconsistent final plan: inconsistent values for sensitive attribute' Changes: - Replace attribute-level PlanModifyList with resource-level ModifyPlan that only writes directory_hash via SetAttribute (proven safe for sensitivity preservation) - Move version reconciliation logic (new vs reuse) from plan modifier into Update, re-deriving from private state instead of ID.IsUnknown() - Keep reconcileVersionIDs() for unit tests but no longer call from plan modifier Fixes #305 --- internal/provider/template_resource.go | 214 ++++++++++++-------- internal/provider/template_resource_test.go | 75 +++++++ 2 files changed, 201 insertions(+), 88 deletions(-) diff --git a/internal/provider/template_resource.go b/internal/provider/template_resource.go index 73b48f8..8a10254 100644 --- a/internal/provider/template_resource.go +++ b/internal/provider/template_resource.go @@ -43,6 +43,7 @@ import ( var _ resource.Resource = &TemplateResource{} var _ resource.ResourceWithImportState = &TemplateResource{} var _ resource.ResourceWithConfigValidators = &TemplateResource{} +var _ resource.ResourceWithModifyPlan = &TemplateResource{} func NewTemplateResource() resource.Resource { return &TemplateResource{} @@ -508,9 +509,6 @@ func (r *TemplateResource) Schema(ctx context.Context, req resource.SchemaReques }, }, }, - PlanModifiers: []planmodifier.List{ - NewVersionsPlanModifier(), - }, }, }, } @@ -821,8 +819,20 @@ func (r *TemplateResource) Update(ctx context.Context, req resource.UpdateReques tflog.Info(ctx, "successfully updated template ACL") } + // Read config to determine which version names are user-set vs computed. + var configVersions Versions + resp.Diagnostics.Append(req.Config.GetAttribute(ctx, path.Root("versions"), &configVersions)...) + if resp.Diagnostics.HasError() { + return + } + for idx := range newState.Versions { if newState.Versions[idx].ID.IsUnknown() { + // If the user didn't explicitly set a name in the config, + // clear it so that Coderd generates a fresh random name. + if idx < len(configVersions) && configVersions[idx].Name.IsNull() { + newState.Versions[idx].Name = types.StringValue("") + } tflog.Info(ctx, "discovered a new or modified template version") uploadResp, logs, err := newVersion(ctx, client, newVersionRequest{ Version: &newState.Versions[idx], @@ -946,6 +956,109 @@ func (r *TemplateResource) ConfigValidators(context.Context) []resource.ConfigVa return []resource.ConfigValidator{} } +// ModifyPlan implements resource.ResourceWithModifyPlan. +// It computes directory hashes for each version and validates version constraints. +// Unlike the previous attribute-level plan modifier, this method only writes +// directory_hash values via SetAttribute, avoiding reconstruction of the entire +// versions list which would strip cty-level sensitivity marks from tf_vars. +func (r *TemplateResource) ModifyPlan(ctx context.Context, req resource.ModifyPlanRequest, resp *resource.ModifyPlanResponse) { + // On destroy, the plan will be null. Nothing to do. + if req.Plan.Raw.IsNull() { + return + } + + var planVersions Versions + resp.Diagnostics.Append(req.Plan.GetAttribute(ctx, path.Root("versions"), &planVersions)...) + if resp.Diagnostics.HasError() { + return + } + + var configVersions Versions + resp.Diagnostics.Append(req.Config.GetAttribute(ctx, path.Root("versions"), &configVersions)...) + if resp.Diagnostics.HasError() { + return + } + + hasActiveVersion, diag := hasOneActiveVersion(configVersions) + if diag.HasError() { + resp.Diagnostics.Append(diag...) + return + } + + // Read previous versions from private state. + var lv LastVersionsByHash + lvBytes, diag := req.Private.GetKey(ctx, LastVersionsKey) + if diag.HasError() { + resp.Diagnostics.Append(diag...) + return + } + if lvBytes == nil { + lv = make(LastVersionsByHash) + // If there's no prior private state, this might be resource creation, + // in which case one version must be active. + if !hasActiveVersion { + resp.Diagnostics.AddError("Client Error", "At least one template version must be active when creating a"+ + " `coderd_template` resource.\n(Subsequent resource updates can be made without an active template in the list).") + return + } + } else { + err := json.Unmarshal(lvBytes, &lv) + if err != nil { + resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Failed to unmarshal private state when reading: %s", err)) + return + } + } + + // Compute directory hashes. + for i := range planVersions { + hash, err := computeDirectoryHash(planVersions[i].Directory.ValueString()) + if err != nil { + resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Failed to compute directory hash: %s", err)) + return + } + planVersions[i].DirectoryHash = types.StringValue(hash) + } + + // Reconcile version IDs using the shared function. + diag = planVersions.reconcileVersionIDs(ctx, lv, configVersions, hasActiveVersion) + if diag.HasError() { + resp.Diagnostics.Append(diag...) + return + } + + // Write reconciled values back to plan via SetAttribute to preserve sensitivity marks. + for i := range planVersions { + resp.Diagnostics.Append(resp.Plan.SetAttribute(ctx, + path.Root("versions").AtListIndex(i).AtName("directory_hash"), + planVersions[i].DirectoryHash, + )...) + if resp.Diagnostics.HasError() { + return + } + resp.Diagnostics.Append(resp.Plan.SetAttribute(ctx, + path.Root("versions").AtListIndex(i).AtName("id"), + planVersions[i].ID, + )...) + if resp.Diagnostics.HasError() { + return + } + // Only write name when the config name is null (auto-generated). + // When the user provides an explicit name expression, the plan already + // carries the correct cty value with any sensitivity marks from the + // config. reconcileVersionIDs only modifies name when config is null, + // so there's nothing new to write for user-set names. + if configVersions[i].Name.IsNull() { + resp.Diagnostics.Append(resp.Plan.SetAttribute(ctx, + path.Root("versions").AtListIndex(i).AtName("name"), + planVersions[i].Name, + )...) + if resp.Diagnostics.HasError() { + return + } + } + } +} + type versionsValidator struct{} func NewVersionsValidator() validator.List { @@ -1007,82 +1120,6 @@ func (a *versionsValidator) ValidateList(ctx context.Context, req validator.List var _ validator.List = &versionsValidator{} -type versionsPlanModifier struct{} - -// Description implements planmodifier.Object. -func (d *versionsPlanModifier) Description(ctx context.Context) string { - return d.MarkdownDescription(ctx) -} - -// MarkdownDescription implements planmodifier.Object. -func (d *versionsPlanModifier) MarkdownDescription(context.Context) string { - return "Compute the hash of a directory." -} - -// PlanModifyObject implements planmodifier.List. -func (d *versionsPlanModifier) PlanModifyList(ctx context.Context, req planmodifier.ListRequest, resp *planmodifier.ListResponse) { - var planVersions Versions - resp.Diagnostics.Append(req.PlanValue.ElementsAs(ctx, &planVersions, false)...) - if resp.Diagnostics.HasError() { - return - } - var configVersions Versions - resp.Diagnostics.Append(req.ConfigValue.ElementsAs(ctx, &configVersions, false)...) - if resp.Diagnostics.HasError() { - return - } - - hasActiveVersion, diag := hasOneActiveVersion(configVersions) - if diag.HasError() { - resp.Diagnostics.Append(diag...) - return - } - - for i := range planVersions { - hash, err := computeDirectoryHash(planVersions[i].Directory.ValueString()) - if err != nil { - resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Failed to compute directory hash: %s", err)) - return - } - planVersions[i].DirectoryHash = types.StringValue(hash) - } - - var lv LastVersionsByHash - lvBytes, diag := req.Private.GetKey(ctx, LastVersionsKey) - if diag.HasError() { - resp.Diagnostics.Append(diag...) - return - } - // If this is the first read, init the private state value - if lvBytes == nil { - lv = make(LastVersionsByHash) - // If there's no prior private state, this might be resource creation, - // in which case one version must be active. - if !hasActiveVersion { - resp.Diagnostics.AddError("Client Error", "At least one template version must be active when creating a"+ - " `coderd_template` resource.\n(Subsequent resource updates can be made without an active template in the list).") - return - } - } else { - err := json.Unmarshal(lvBytes, &lv) - if err != nil { - resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Failed to unmarshal private state when reading: %s", err)) - return - } - } - - diag = planVersions.reconcileVersionIDs(ctx, lv, configVersions, hasActiveVersion) - if diag.HasError() { - resp.Diagnostics.Append(diag...) - return - } - - resp.PlanValue, diag = types.ListValueFrom(ctx, req.PlanValue.ElementType(ctx), planVersions) - if diag.HasError() { - resp.Diagnostics.Append(diag...) - } -} - func hasOneActiveVersion(data Versions) (hasActiveVersion bool, diags diag.Diagnostics) { active := false for _, version := range data { @@ -1101,12 +1138,6 @@ func hasOneActiveVersion(data Versions) (hasActiveVersion bool, diags diag.Diagn return active, diags } -func NewVersionsPlanModifier() planmodifier.List { - return &versionsPlanModifier{} -} - -var _ planmodifier.List = &versionsPlanModifier{} - var weekValidator = setvalidator.ValueStringsAre( stringvalidator.OneOf("monday", "tuesday", "wednesday", "thursday", "friday", "saturday", "sunday"), ) @@ -1630,13 +1661,20 @@ func tfVariablesChanged(ctx context.Context, prevs []PreviousTemplateVersion, pl if prev.TFVars == nil { return true } + // If the set is unknown, we cannot compare and must treat it as changed. + if planned.TerraformVariables.IsUnknown() { + return true + } + // If the set is null (tf_vars not specified), treat as no variables. + // Only consider this a change if the previous version had variables. + if planned.TerraformVariables.IsNull() { + return len(prev.TFVars) > 0 + } plannedVars, diags := varsFromSet(ctx, planned.TerraformVariables) if diags.HasError() { return true } - // If the set is unknown or null, we cannot compare and - // must treat it as changed. - if planned.TerraformVariables.IsUnknown() || planned.TerraformVariables.IsNull() { + if len(plannedVars) != len(prev.TFVars) { return true } for _, tfVar := range plannedVars { diff --git a/internal/provider/template_resource_test.go b/internal/provider/template_resource_test.go index c58d4c6..92a9a40 100644 --- a/internal/provider/template_resource_test.go +++ b/internal/provider/template_resource_test.go @@ -11,10 +11,12 @@ import ( "text/template" "github.com/google/uuid" + "github.com/hashicorp/go-version" "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/hashicorp/terraform-plugin-testing/terraform" + "github.com/hashicorp/terraform-plugin-testing/tfversion" cp "github.com/otiai10/copy" "github.com/stretchr/testify/require" @@ -1036,6 +1038,79 @@ resource "coderd_template" "sample" { }) } +func TestAccTemplateResourceSensitiveTFVars(t *testing.T) { + t.Parallel() + cfg := ` +provider coderd { + url = %q + token = %q +} + +variable "secret" { + sensitive = true + default = %q +} + +resource "time_static" "trigger" { + triggers = { + secret = var.secret + } +} + +resource "coderd_template" "sample" { + name = "sensitive-tfvars-test" + versions = [ + { + name = "v_${time_static.trigger.unix}" + directory = %q + active = true + tf_vars = [ + { + name = "secret" + value = var.secret + } + ] + } + ] +} +` + + ctx := t.Context() + client := integration.StartCoder(ctx, t, "template_sensitive_tfvars_acc") + + exTemplate := t.TempDir() + err := cp.Copy("../../integration/template-test/example-template", exTemplate) + require.NoError(t, err) + + cfgStep1 := fmt.Sprintf(cfg, client.URL.String(), client.SessionToken(), "initial-secret", exTemplate) + cfgStep2 := fmt.Sprintf(cfg, client.URL.String(), client.SessionToken(), "changed-secret", exTemplate) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + IsUnitTest: true, + ProtoV6ProviderFactories: testAccProtoV6ProviderFactories, + // Terraform < 1.1 panics rendering plan diffs with sensitivity marks + // on nested attributes ("value is marked, so must be unmarked first"). + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.SkipBelow(version.Must(version.NewVersion("1.1.0"))), + }, + ExternalProviders: map[string]resource.ExternalProvider{ + "time": { + Source: "hashicorp/time", + VersionConstraint: ">=0.9.0", + }, + }, + Steps: []resource.TestStep{ + { + Config: cfgStep1, + }, + { + Config: cfgStep2, + }, + }, + }) +} + type testAccTemplateResourceConfig struct { URL string Token string