From 1105f16d35983a364f73f066de06bc126ac75973 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 3 Mar 2025 13:01:46 -0500 Subject: [PATCH 1/4] add marks.RemoveAll RemoveAll filters marks from a set of cty.PathValueMarks, removing all instances of the remove parameter in all paths. If a path has no remaining marks, it is removed entirely. --- internal/lang/marks/paths.go | 22 ++++++++++++++++++++ internal/lang/marks/paths_test.go | 34 +++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/internal/lang/marks/paths.go b/internal/lang/marks/paths.go index 6489b9a524..97120602a5 100644 --- a/internal/lang/marks/paths.go +++ b/internal/lang/marks/paths.go @@ -41,6 +41,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 { diff --git a/internal/lang/marks/paths_test.go b/internal/lang/marks/paths_test.go index 536d8d01db..ed4db941c7 100644 --- a/internal/lang/marks/paths_test.go +++ b/internal/lang/marks/paths_test.go @@ -58,6 +58,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"), From ad6cbd6d7b4c34b1337ff820a9b72dd01415a506 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 3 Mar 2025 13:05:09 -0500 Subject: [PATCH 2/4] use marks.RemoveAll to filter ephemeral marks PathsWithMark was incorrectly used to try and filter out ephemeral marks, but that doesn't work when a path has multiple marks. --- .../terraform/context_plan_ephemeral_test.go | 18 ++++++++++++++++++ .../node_resource_abstract_instance.go | 9 +++++---- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/internal/terraform/context_plan_ephemeral_test.go b/internal/terraform/context_plan_ephemeral_test.go index ecc40399ab..464943524e 100644 --- a/internal/terraform/context_plan_ephemeral_test.go +++ b/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) diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index 975764ab14..04823b72d4 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/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()) { From 64c68a0a45ee90aaad26915b4214c00a75782cec Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 3 Mar 2025 13:22:12 -0500 Subject: [PATCH 3/4] don't add paths multiple times in PathsWithMark PathsWithMark was adding the same path multiple times if it had multiple extra marks. --- internal/lang/marks/paths.go | 3 +++ internal/lang/marks/paths_test.go | 16 ++++++++++++---- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/internal/lang/marks/paths.go b/internal/lang/marks/paths.go index 97120602a5..0bb81ae43b 100644 --- a/internal/lang/marks/paths.go +++ b/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 } } } diff --git a/internal/lang/marks/paths_test.go b/internal/lang/marks/paths_test.go index ed4db941c7..f6adf437e3 100644 --- a/internal/lang/marks/paths_test.go +++ b/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 != "" { From fdf25b60eda2e1e3bfe94125d93136d103ec5cd3 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 3 Mar 2025 14:42:18 -0500 Subject: [PATCH 4/4] changelog --- .changes/v1.11/BUG FIXES-20250303-144020.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changes/v1.11/BUG FIXES-20250303-144020.yaml diff --git a/.changes/v1.11/BUG FIXES-20250303-144020.yaml b/.changes/v1.11/BUG FIXES-20250303-144020.yaml new file mode 100644 index 0000000000..0a1be9fa3c --- /dev/null +++ b/.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"