Merge pull request #32892 from hashicorp/jbardin/update-missing-sensitive

don't compare plan marks for missing values

Author: jbardin

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)
+		}
+	}
+}

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 {

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
 	}