ephemeral: providers are responsible for setting write-only attributes to null

internal/plans/objchange/plan_valid.go

@@ -301,6 +301,16 @@ func assertPlannedValueValid(attrS *configschema.Attribute, priorV, configV, pla
 		return errs
 	}
 
+	if attrS.WriteOnly {
+		// The provider is not allowed to return non-null values for write-only attributes
+		if !plannedV.IsNull() {
+			errs = append(errs, path.NewErrorf("planned value for write-only attribute is not null"))
+		}
+
+		// We don't want to evaluate further if the attribute is write-only and null
+		return errs
+	}
+
 	switch {
 	// The provider can plan any value for a computed-only attribute. There may
 	// be a config value here in the case where a user used `ignore_changes` on

internal/plans/objchange/plan_valid_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/zclconf/go-cty/cty"
 
 	"github.com/hashicorp/terraform/internal/configs/configschema"
+	"github.com/hashicorp/terraform/internal/lang/marks"
 	"github.com/hashicorp/terraform/internal/tfdiags"
 )
 
@@ -1965,6 +1966,67 @@ func TestAssertPlanValid(t *testing.T) {
 			}),
 			[]string{},
 		},
+
+		"write-only attributes": {
+			&configschema.Block{
+				Attributes: map[string]*configschema.Attribute{
+					"foo": {
+						Type:      cty.String,
+						WriteOnly: true,
+					},
+				},
+			},
+			cty.ObjectVal(map[string]cty.Value{
+				"foo": cty.NullVal(cty.String),
+			}),
+			cty.ObjectVal(map[string]cty.Value{
+				"foo": cty.StringVal("write-only").Mark(marks.Ephemeral),
+			}),
+			cty.ObjectVal(map[string]cty.Value{
+				"foo": cty.NullVal(cty.String),
+			}),
+			[]string{},
+		},
+
+		"nested write-only attributes": {
+			&configschema.Block{
+				BlockTypes: map[string]*configschema.NestedBlock{
+					"nested": {
+						Nesting: configschema.NestingList,
+						Block: configschema.Block{
+							Attributes: map[string]*configschema.Attribute{
+								"foo": {
+									Type:      cty.String,
+									WriteOnly: true,
+								},
+							},
+						},
+					},
+				},
+			},
+			cty.ObjectVal(map[string]cty.Value{
+				"nested": cty.ListVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.NullVal(cty.String),
+					}),
+				}),
+			}),
+			cty.ObjectVal(map[string]cty.Value{
+				"nested": cty.ListVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.StringVal("write-only").Mark(marks.Ephemeral),
+					}),
+				}),
+			}),
+			cty.ObjectVal(map[string]cty.Value{
+				"nested": cty.ListVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"foo": cty.NullVal(cty.String),
+					}),
+				}),
+			}),
+			[]string{},
+		},
 	}
 
 	for name, test := range tests {

internal/providers/testing/provider_mock.go

@@ -422,6 +422,11 @@ func (p *MockProvider) PlanResourceChange(r providers.PlanResourceChangeRequest)
 			return v, nil
 		}
 
+		// Write-only attributes always return null
+		if attrSchema.WriteOnly {
+			return cty.NullVal(v.Type()), nil
+		}
+
 		// get the current configuration value, to detect when a
 		// computed+optional attributes has become unset
 		configVal, err := path.Apply(r.Config)

internal/terraform/context_apply_ephemeral_test.go

@@ -15,6 +15,7 @@ import (
 	"github.com/hashicorp/terraform/internal/providers"
 	testing_provider "github.com/hashicorp/terraform/internal/providers/testing"
 	"github.com/hashicorp/terraform/internal/states"
+	"github.com/hashicorp/terraform/internal/tfdiags"
 	"github.com/zclconf/go-cty/cty"
 )
 
@@ -729,3 +730,157 @@ resource "ephem_write_only" "wo" {
 		t.Fatalf("write_only attribute should be null")
 	}
 }
+
+func TestContext2Apply_write_only_attribute_provider_plans_with_non_null_value(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+variable "ephem" {
+  type        = string
+  ephemeral   = true
+}
+
+resource "ephem_write_only" "wo" {
+  normal     = "normal"
+  write_only = var.ephem
+}
+`,
+	})
+
+	ephem := &testing_provider.MockProvider{
+		GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{
+			ResourceTypes: map[string]providers.Schema{
+				"ephem_write_only": {
+					Block: &configschema.Block{
+						Attributes: map[string]*configschema.Attribute{
+							"normal": {
+								Type:     cty.String,
+								Required: true,
+							},
+							"write_only": {
+								Type:      cty.String,
+								WriteOnly: true,
+								Required:  true,
+							},
+						},
+					},
+				},
+			},
+		},
+		PlanResourceChangeResponse: &providers.PlanResourceChangeResponse{
+			PlannedState: cty.ObjectVal(map[string]cty.Value{
+				"normal":     cty.StringVal("normal"),
+				"write_only": cty.StringVal("the provider should have set this to null"),
+			}),
+		},
+	}
+
+	ctx := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("ephem"): testProviderFuncFixed(ephem),
+		},
+	})
+
+	ephemVar := &InputValue{
+		Value:      cty.StringVal("ephemeral_value"),
+		SourceType: ValueFromCLIArg,
+	}
+
+	_, diags := ctx.Plan(m, nil, &PlanOpts{
+		Mode: plans.NormalMode,
+		SetVariables: InputValues{
+			"ephem": ephemVar,
+		},
+	})
+
+	var expectedDiags tfdiags.Diagnostics
+
+	expectedDiags = append(expectedDiags, tfdiags.Sourceless(
+		tfdiags.Error,
+		"Provider produced invalid plan",
+		`Provider "registry.terraform.io/hashicorp/ephem" planned an invalid value for ephem_write_only.wo.write_only: planned value for write-only attribute is not null.
+
+This is a bug in the provider, which should be reported in the provider's own issue tracker.`,
+	))
+
+	assertDiagnosticsMatch(t, diags, expectedDiags)
+}
+
+func TestContext2Apply_write_only_attribute_provider_applies_with_non_null_value(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+variable "ephem" {
+  type        = string
+  ephemeral   = true
+}
+
+resource "ephem_write_only" "wo" {
+  normal     = "normal"
+  write_only = var.ephem
+}
+`,
+	})
+
+	ephem := &testing_provider.MockProvider{
+		GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{
+			ResourceTypes: map[string]providers.Schema{
+				"ephem_write_only": {
+					Block: &configschema.Block{
+						Attributes: map[string]*configschema.Attribute{
+							"normal": {
+								Type:     cty.String,
+								Required: true,
+							},
+							"write_only": {
+								Type:      cty.String,
+								WriteOnly: true,
+								Required:  true,
+							},
+						},
+					},
+				},
+			},
+		},
+		ApplyResourceChangeResponse: &providers.ApplyResourceChangeResponse{
+			NewState: cty.ObjectVal(map[string]cty.Value{
+				"normal":     cty.StringVal("normal"),
+				"write_only": cty.StringVal("the provider should have set this to null"),
+			}),
+		},
+	}
+
+	ctx := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("ephem"): testProviderFuncFixed(ephem),
+		},
+	})
+
+	ephemVar := &InputValue{
+		Value:      cty.StringVal("ephemeral_value"),
+		SourceType: ValueFromCLIArg,
+	}
+
+	plan, planDiags := ctx.Plan(m, nil, &PlanOpts{
+		Mode: plans.NormalMode,
+		SetVariables: InputValues{
+			"ephem": ephemVar,
+		},
+	})
+
+	assertNoDiagnostics(t, planDiags)
+
+	_, diags := ctx.Apply(plan, m, &ApplyOpts{
+		SetVariables: InputValues{
+			"ephem": ephemVar,
+		},
+	})
+
+	var expectedDiags tfdiags.Diagnostics
+
+	expectedDiags = append(expectedDiags, tfdiags.Sourceless(
+		tfdiags.Error,
+		"Write-only attribute set during apply",
+		`Provider "provider[\"registry.terraform.io/hashicorp/ephem\"]" set the write-only attribute "ephem_write_only.wo.write_only" during apply. Write-only attributes must not be set by the provider during apply, so this is a bug in the provider, which should be reported in the provider's own issue tracker.`,
+	))
+
+	assertDiagnosticsMatch(t, diags, expectedDiags)
+}

internal/terraform/node_resource_abstract_instance.go

@@ -17,7 +17,6 @@ import (
 	"github.com/hashicorp/terraform/internal/configs"
 	"github.com/hashicorp/terraform/internal/configs/configschema"
 	"github.com/hashicorp/terraform/internal/instances"
-	"github.com/hashicorp/terraform/internal/lang/ephemeral"
 	"github.com/hashicorp/terraform/internal/lang/marks"
 	"github.com/hashicorp/terraform/internal/moduletest/mocking"
 	"github.com/hashicorp/terraform/internal/plans"
@@ -1026,17 +1025,16 @@ func (n *NodeAbstractResourceInstance) plan(
 	// Add the marks back to the planned new value -- this must happen after
 	// ignore changes have been processed. We add in the schema marks as well,
 	// to ensure that provider defined private attributes are marked correctly
-	// here.
+	// here. We remove the ephemeral marks, the provider is expected to return null
+	// for write-only attributes (the only place where ephemeral values are allowed).
+	// This is verified in objchange.AssertPlanValid already.
 	unmarkedPlannedNewVal := plannedNewVal
-	plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths)
+	_, nonEphemeralMarks := marks.PathsWithMark(unmarkedPaths, marks.Ephemeral)
+	plannedNewVal = plannedNewVal.MarkWithPaths(nonEphemeralMarks)
 	if sensitivePaths := schema.SensitivePaths(plannedNewVal, nil); len(sensitivePaths) != 0 {
 		plannedNewVal = marks.MarkPaths(plannedNewVal, marks.Sensitive, sensitivePaths)
 	}
 
-	// From the point of view of the provider ephemeral value marks have been removed before plan
-	// and are reapplied now so we now need to set the values at these marks to null again.
-	plannedNewVal = ephemeral.RemoveEphemeralValues(plannedNewVal)
-
 	reqRep, reqRepDiags := getRequiredReplaces(unmarkedPriorVal, unmarkedPlannedNewVal, resp.RequiresReplace, n.ResolvedProvider.Provider, n.Addr)
 	diags = diags.Append(reqRepDiags)
 	if diags.HasErrors() {
@@ -1113,8 +1111,8 @@ func (n *NodeAbstractResourceInstance) plan(
 		plannedNewVal = resp.PlannedState
 		plannedPrivate = resp.PlannedPrivate
 
-		if len(unmarkedPaths) > 0 {
-			plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths)
+		if len(nonEphemeralMarks) > 0 {
+			plannedNewVal = plannedNewVal.MarkWithPaths(nonEphemeralMarks)
 		}
 
 		for _, err := range plannedNewVal.Type().TestConformance(schema.ImpliedType()) {
@@ -2549,6 +2547,25 @@ func (n *NodeAbstractResourceInstance) apply(
 		return nil, diags
 	}
 
+	// Providers are supposed to return null values for all write-only attributes
+	var writeOnlyDiags tfdiags.Diagnostics
+	if writeOnlyPaths := NonNullWriteOnlyPaths(newVal, schema, nil); len(writeOnlyPaths) != 0 {
+		for _, p := range writeOnlyPaths {
+			writeOnlyDiags = writeOnlyDiags.Append(tfdiags.Sourceless(
+				tfdiags.Error,
+				"Write-only attribute set during apply",
+				fmt.Sprintf(
+					"Provider %q set the write-only attribute \"%s%s\" during apply. Write-only attributes must not be set by the provider during apply, so this is a bug in the provider, which should be reported in the provider's own issue tracker.",
+					n.ResolvedProvider.String(), n.Addr.String(), tfdiags.FormatCtyPath(p),
+				),
+			))
+		}
+		diags = diags.Append(writeOnlyDiags)
+	}
+	if writeOnlyDiags.HasErrors() {
+		return nil, diags
+	}
+
 	// After this point we have a type-conforming result object and so we
 	// must always run to completion to ensure it can be saved. If n.Error
 	// is set then we must not return a non-nil error, in order to allow
@@ -2720,6 +2737,26 @@ func (n *NodeAbstractResourceInstance) prevRunAddr(ctx EvalContext) addrs.AbsRes
 	return resourceInstancePrevRunAddr(ctx, n.Addr)
 }
 
+// NonNullWriteOnlyPaths returns a list of paths to attributes that are write-only
+// and non-null in the given value.
+func NonNullWriteOnlyPaths(val cty.Value, schema *configschema.Block, p cty.Path) (paths []cty.Path) {
+	for name, attr := range schema.Attributes {
+		attrPath := append(p, cty.GetAttrStep{Name: name})
+		attrVal, _ := attrPath.Apply(val)
+		if attr.WriteOnly && !attrVal.IsNull() {
+			paths = append(paths, attrPath)
+		}
+	}
+
+	for name, blockS := range schema.BlockTypes {
+		blockPath := append(p, cty.GetAttrStep{Name: name})
+		x := NonNullWriteOnlyPaths(val, &blockS.Block, blockPath)
+		paths = append(paths, x...)
+	}
+
+	return paths
+}
+
 func resourceInstancePrevRunAddr(ctx EvalContext, currentAddr addrs.AbsResourceInstance) addrs.AbsResourceInstance {
 	table := ctx.MoveResults()
 	return table.OldAddr(currentAddr)

internal/terraform/node_resource_abstract_instance_test.go

@@ -250,3 +250,138 @@ func TestNodeAbstractResourceInstance_refresh_with_deferred_read(t *testing.T) {
 		t.Fatalf("expected deferral to be AbsentPrereq, got %s", deferred.Reason)
 	}
 }
+
+func TestNonNullWriteOnlyPaths(t *testing.T) {
+	for name, tc := range map[string]struct {
+		val    cty.Value
+		schema *configschema.Block
+
+		expectedPaths []cty.Path
+	}{
+		"no write-only attributes": {
+			val: cty.ObjectVal(map[string]cty.Value{
+				"id": cty.StringVal("i-abc123"),
+			}),
+			schema: &configschema.Block{
+				Attributes: map[string]*configschema.Attribute{
+					"id": {
+						Type: cty.String,
+					},
+				},
+			},
+		},
+
+		"write-only attribute with null value": {
+			val: cty.ObjectVal(map[string]cty.Value{
+				"id": cty.NullVal(cty.String),
+			}),
+			schema: &configschema.Block{
+				Attributes: map[string]*configschema.Attribute{
+					"id": {
+						Type:      cty.String,
+						Optional:  true,
+						WriteOnly: true,
+					},
+				},
+			},
+		},
+
+		"write-only attribute with non-null value": {
+			val: cty.ObjectVal(map[string]cty.Value{
+				"valid": cty.NullVal(cty.String),
+				"id":    cty.StringVal("i-abc123"),
+			}),
+			schema: &configschema.Block{
+				Attributes: map[string]*configschema.Attribute{
+					"valid": {
+						Type:      cty.String,
+						Optional:  true,
+						WriteOnly: true,
+					},
+					"id": {
+						Type:      cty.String,
+						Optional:  true,
+						WriteOnly: true,
+					},
+				},
+			},
+			expectedPaths: []cty.Path{cty.GetAttrPath("id")},
+		},
+
+		"write-only attributes in blocks": {
+			val: cty.ObjectVal(map[string]cty.Value{
+				"foo": cty.ObjectVal(map[string]cty.Value{
+					"valid-write-only": cty.NullVal(cty.String),
+					"valid":            cty.StringVal("valid"),
+					"id":               cty.StringVal("i-abc123"),
+					"bar": cty.ObjectVal(map[string]cty.Value{
+						"valid-write-only": cty.NullVal(cty.String),
+						"valid":            cty.StringVal("valid"),
+						"id":               cty.StringVal("i-abc123"),
+					}),
+				}),
+			}),
+			schema: &configschema.Block{
+				BlockTypes: map[string]*configschema.NestedBlock{
+					"foo": {
+						Block: configschema.Block{
+							Attributes: map[string]*configschema.Attribute{
+								"valid-write-only": {
+									Type:      cty.String,
+									Optional:  true,
+									WriteOnly: true,
+								},
+								"valid": {
+									Type:     cty.String,
+									Optional: true,
+								},
+								"id": {
+									Type:      cty.String,
+									Optional:  true,
+									WriteOnly: true,
+								},
+							},
+							BlockTypes: map[string]*configschema.NestedBlock{
+								"bar": {
+									Block: configschema.Block{
+										Attributes: map[string]*configschema.Attribute{
+											"valid-write-only": {
+												Type:      cty.String,
+												Optional:  true,
+												WriteOnly: true,
+											},
+											"valid": {
+												Type:     cty.String,
+												Optional: true,
+											},
+											"id": {
+												Type:      cty.String,
+												Optional:  true,
+												WriteOnly: true,
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			expectedPaths: []cty.Path{cty.GetAttrPath("foo").GetAttr("id"), cty.GetAttrPath("foo").GetAttr("bar").GetAttr("id")},
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			paths := NonNullWriteOnlyPaths(tc.val, tc.schema, nil)
+
+			if len(paths) != len(tc.expectedPaths) {
+				t.Fatalf("expected %d write-only paths, got %d", len(tc.expectedPaths), len(paths))
+			}
+
+			for i, path := range paths {
+				if !path.Equals(tc.expectedPaths[i]) {
+					t.Fatalf("expected path %#v, got %#v", tc.expectedPaths[i], path)
+				}
+			}
+		})
+	}
+}