Merge pull request #36619 from hashicorp/jbardin/filter-ephemeral-marks

Filter ephemeral marks from planned value

.changes/v1.11/BUG FIXES-20250303-144020.yaml

@@ -0,0 +1,5 @@
+kind: BUG FIXES
+body: Combining ephemeral and sensitive marks could fail when serializing planned changes
+time: 2025-03-03T14:40:20.606817-05:00
+custom:
+    Issue: "36619"

internal/lang/marks/paths.go

@@ -31,9 +31,12 @@ func PathsWithMark(pvms []cty.PathValueMarks, wantMark any) (withWanted []cty.Pa
 		if _, ok := pvm.Marks[wantMark]; ok {
 			withWanted = append(withWanted, pvm.Path)
 		}
+
 		for mark := range pvm.Marks {
 			if mark != wantMark {
 				withOthers = append(withOthers, pvm)
+				// only add a path with unwanted marks a single time
+				break
 			}
 		}
 	}
@@ -41,6 +44,28 @@ func PathsWithMark(pvms []cty.PathValueMarks, wantMark any) (withWanted []cty.Pa
 	return withWanted, withOthers
 }
 
+// RemoveAll take a series of PathValueMarks and removes the unwanted mark from
+// all paths. Paths with no remaining marks will be removed entirely. The
+// PathValuesMarks passed in are not cloned, and RemoveAll will modify the
+// original values, so the prior set of marks should not be retained for use.
+func RemoveAll(pvms []cty.PathValueMarks, remove any) []cty.PathValueMarks {
+	if len(pvms) == 0 {
+		// No-allocations path for the common case where there are no marks at all.
+		return nil
+	}
+
+	var res []cty.PathValueMarks
+
+	for _, pvm := range pvms {
+		delete(pvm.Marks, remove)
+		if len(pvm.Marks) > 0 {
+			res = append(res, pvm)
+		}
+	}
+
+	return res
+}
+
 // MarkPaths transforms the given value by marking each of the given paths
 // with the given mark value.
 func MarkPaths(val cty.Value, mark any, paths []cty.Path) cty.Value {

internal/lang/marks/paths_test.go

@@ -16,7 +16,7 @@ func TestPathsWithMark(t *testing.T) {
 	input := []cty.PathValueMarks{
 		{
 			Path:  cty.GetAttrPath("sensitive"),
-			Marks: cty.NewValueMarks(Sensitive),
+			Marks: cty.NewValueMarks("sensitive"),
 		},
 		{
 			Path:  cty.GetAttrPath("other"),
@@ -24,11 +24,15 @@ func TestPathsWithMark(t *testing.T) {
 		},
 		{
 			Path:  cty.GetAttrPath("both"),
-			Marks: cty.NewValueMarks(Sensitive, "other"),
+			Marks: cty.NewValueMarks("sensitive", "other"),
+		},
+		{
+			Path:  cty.GetAttrPath("neither"),
+			Marks: cty.NewValueMarks("x", "y"),
 		},
 	}
 
-	gotPaths, gotOthers := PathsWithMark(input, Sensitive)
+	gotPaths, gotOthers := PathsWithMark(input, "sensitive")
 	wantPaths := []cty.Path{
 		cty.GetAttrPath("sensitive"),
 		cty.GetAttrPath("both"),
@@ -40,7 +44,7 @@ func TestPathsWithMark(t *testing.T) {
 		},
 		{
 			Path:  cty.GetAttrPath("both"),
-			Marks: cty.NewValueMarks(Sensitive, "other"),
+			Marks: cty.NewValueMarks("sensitive", "other"),
 			// Note that this intentionally preserves the fact that the
 			// attribute was both sensitive _and_ had another mark, since
 			// that gives the caller the most possible information to
@@ -48,6 +52,10 @@ func TestPathsWithMark(t *testing.T) {
 			// an error message, or whatever. It also conveniently avoids
 			// allocating a new mark set, which is nice.
 		},
+		{
+			Path:  cty.GetAttrPath("neither"),
+			Marks: cty.NewValueMarks("x", "y"),
+		},
 	}
 
 	if diff := cmp.Diff(wantPaths, gotPaths, ctydebug.CmpOptions); diff != "" {
@@ -58,6 +66,40 @@ func TestPathsWithMark(t *testing.T) {
 	}
 }
 
+func TestRemoveAll(t *testing.T) {
+	input := []cty.PathValueMarks{
+		{
+			Path:  cty.GetAttrPath("sensitive"),
+			Marks: cty.NewValueMarks("sensitive"),
+		},
+		{
+			Path:  cty.GetAttrPath("other"),
+			Marks: cty.NewValueMarks("other"),
+		},
+		{
+			Path:  cty.GetAttrPath("both"),
+			Marks: cty.NewValueMarks("sensitive", "other"),
+		},
+	}
+
+	want := []cty.PathValueMarks{
+		{
+			Path:  cty.GetAttrPath("sensitive"),
+			Marks: cty.NewValueMarks("sensitive"),
+		},
+		{
+			Path:  cty.GetAttrPath("both"),
+			Marks: cty.NewValueMarks("sensitive"),
+		},
+	}
+
+	got := RemoveAll(input, "other")
+
+	if diff := cmp.Diff(want, got, ctydebug.CmpOptions); diff != "" {
+		t.Errorf("wrong matched paths\n%s", diff)
+	}
+}
+
 func TestMarkPaths(t *testing.T) {
 	value := cty.ObjectVal(map[string]cty.Value{
 		"s": cty.StringVal(".s"),

internal/terraform/context_plan_ephemeral_test.go

@@ -565,6 +565,24 @@ resource "ephem_write_only" "test" {
 			expectValidateEphemeralResourceConfigCalled: true,
 			expectCloseEphemeralResourceCalled:          true,
 		},
+		"write_only_sensitive_and_ephem": {
+			module: map[string]string{
+				"main.tf": `
+variable "in" {
+  sensitive = true
+  ephemeral = true
+}
+resource "ephem_write_only" "test" {
+    write_only = var.in
+}
+`,
+			},
+			inputs: InputValues{
+				"in": &InputValue{
+					Value: cty.StringVal("test"),
+				},
+			},
+		},
 	} {
 		t.Run(name, func(t *testing.T) {
 			m := testModuleInline(t, tc.module)

internal/terraform/node_resource_abstract_instance.go

@@ -1066,8 +1066,9 @@ func (n *NodeAbstractResourceInstance) plan(
 	// for write-only attributes (the only place where ephemeral values are allowed).
 	// This is verified in objchange.AssertPlanValid already.
 	unmarkedPlannedNewVal := plannedNewVal
-	_, nonEphemeralMarks := marks.PathsWithMark(unmarkedPaths, marks.Ephemeral)
-	plannedNewVal = plannedNewVal.MarkWithPaths(nonEphemeralMarks)
+	unmarkedPaths = marks.RemoveAll(unmarkedPaths, marks.Ephemeral)
+
+	plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths)
 	if sensitivePaths := schema.SensitivePaths(plannedNewVal, nil); len(sensitivePaths) != 0 {
 		plannedNewVal = marks.MarkPaths(plannedNewVal, marks.Sensitive, sensitivePaths)
 	}
@@ -1153,8 +1154,8 @@ func (n *NodeAbstractResourceInstance) plan(
 		plannedNewVal = resp.PlannedState
 		plannedPrivate = resp.PlannedPrivate
 
-		if len(nonEphemeralMarks) > 0 {
-			plannedNewVal = plannedNewVal.MarkWithPaths(nonEphemeralMarks)
+		if len(unmarkedPaths) > 0 {
+			plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths)
 		}
 
 		for _, err := range plannedNewVal.Type().TestConformance(schema.ImpliedType()) {