From 81b74cdb22b33590baa7f15dcfdffb9d464c4608 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Mon, 20 Mar 2023 16:23:52 -0400
Subject: [PATCH] don't compare plan marks for missing values

If a resource has a change in marks from the prior state, we need to
notify the user that an update is going to be necessary to at least
store that new value in the state. If the provider however returns the
prior state value in lieu of a new config value, we need to be sure to
filter any new marks for comparison as well. The comparison of the prior
marks and new marks must take into account whether those new marks could
even be applied, because if the value is unchanged the new marks may be
completely irrelevant.
---
 internal/terraform/context_plan2_test.go      | 63 +++++++++++++++++++
 internal/terraform/marks.go                   | 16 +++++
 .../node_resource_abstract_instance.go        | 11 +++-
 3 files changed, 88 insertions(+), 2 deletions(-)

diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go
index 4e3d52f079..230390bb87 100644
--- a/internal/terraform/context_plan2_test.go
+++ b/internal/terraform/context_plan2_test.go
@@ -4035,3 +4035,66 @@ func TestContext2Plan_dataSourceReadPlanError(t *testing.T) {
 		t.Fatalf("failed to round-trip through planfile: %s", err)
 	}
 }
+
+func TestContext2Plan_ignoredMarkedValue(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+resource "test_object" "a" {
+  map = {
+    prior = "value"
+    new   = sensitive("ignored")
+  }
+}
+`})
+
+	testProvider := &MockProvider{
+		GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{
+			ResourceTypes: map[string]providers.Schema{
+				"test_object": providers.Schema{
+					Block: &configschema.Block{
+						Attributes: map[string]*configschema.Attribute{
+							"map": {
+								Type:     cty.Map(cty.String),
+								Optional: true,
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	testProvider.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) (resp providers.PlanResourceChangeResponse) {
+		// We're going to ignore any changes here and return the prior state.
+		resp.PlannedState = req.PriorState
+		return resp
+	}
+
+	state := states.NewState()
+	root := state.RootModule()
+	root.SetResourceInstanceCurrent(
+		mustResourceInstanceAddr("test_object.a").Resource,
+		&states.ResourceInstanceObjectSrc{
+			Status:       states.ObjectReady,
+			AttrsJSON:    []byte(`{"map":{"prior":"value"}}`),
+			Dependencies: []addrs.ConfigResource{},
+		},
+		mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
+	)
+	ctx := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(testProvider),
+		},
+	})
+
+	// plan+apply to create the initial state
+	opts := SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))
+	plan, diags := ctx.Plan(m, state, opts)
+	assertNoErrors(t, diags)
+
+	for _, c := range plan.Changes.Resources {
+		if c.Action != plans.NoOp {
+			t.Errorf("unexpected %s change for %s", c.Action, c.Addr)
+		}
+	}
+}
diff --git a/internal/terraform/marks.go b/internal/terraform/marks.go
index 8e2a326072..8dd14cd538 100644
--- a/internal/terraform/marks.go
+++ b/internal/terraform/marks.go
@@ -7,6 +7,22 @@ import (
 	"github.com/zclconf/go-cty/cty"
 )
 
+// filterMarks removes any PathValueMarks from marks which cannot be applied to
+// the given value. When comparing existing marks to those from a map or other
+// dynamic value, we may not have values at the same paths and need to strip
+// out irrelevant marks.
+func filterMarks(val cty.Value, marks []cty.PathValueMarks) []cty.PathValueMarks {
+	var res []cty.PathValueMarks
+	for _, mark := range marks {
+		// any error here just means the path cannot apply to this value, so we
+		// don't need this mark for comparison.
+		if _, err := mark.Path.Apply(val); err == nil {
+			res = append(res, mark)
+		}
+	}
+	return res
+}
+
 // marksEqual compares 2 unordered sets of PathValue marks for equality, with
 // the comparison using the cty.PathValueMarks.Equal method.
 func marksEqual(a, b []cty.PathValueMarks) bool {
diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go
index 0d458fd29c..01a8cc76bf 100644
--- a/internal/terraform/node_resource_abstract_instance.go
+++ b/internal/terraform/node_resource_abstract_instance.go
@@ -1074,8 +1074,15 @@ func (n *NodeAbstractResourceInstance) plan(
 	}
 
 	// If we plan to write or delete sensitive paths from state,
-	// this is an Update action
-	if action == plans.NoOp && !marksEqual(unmarkedPaths, priorPaths) {
+	// this is an Update action.
+	//
+	// We need to filter out any marks which may not apply to the new planned
+	// value before comparison. The one case where a provider is allowed to
+	// return a different value from the configuration is when a config change
+	// is not functionally significant and the prior state can be returned. If a
+	// new mark was also discarded from that config change, it needs to be
+	// ignored here to prevent an errant update action.
+	if action == plans.NoOp && !marksEqual(filterMarks(plannedNewVal, unmarkedPaths), priorPaths) {
 		action = plans.Update
 	}