Merge pull request #34942 from hashicorp/backport/jbardin/plan-compar…

…e-marks/nearly-accepted-colt

Backport of Always add all marks to plan and apply values, then compare marks from the values into v1.8

internal/providers/testing/provider_mock.go

@@ -379,7 +379,12 @@ func (p *MockProvider) PlanResourceChange(r providers.PlanResourceChangeRequest)
 		// computed+optional attributes has become unset
 		configVal, err := path.Apply(r.Config)
 		if err != nil {
-			return v, err
+			// cty can't currently apply some paths, so don't try to guess
+			// what's needed here and return the proposed part of the value.
+			// This is only a helper to create a default plan value, any tests
+			// relying on specific plan behavior will create their own
+			// PlanResourceChange responses.
+			return v, nil
 		}
 
 		switch {

internal/terraform/context_apply2_test.go

@@ -3304,3 +3304,76 @@ resource "test_resource" "a" {
 		t.Errorf("unexpected sensitive paths\ndiff:\n%s", diff)
 	}
 }
+
+func TestContext2Apply_sensitiveNestedComputedAttributes(t *testing.T) {
+	// Ensure we're not trying to double-mark values decoded from state
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+resource "test_object" "a" {
+}
+`,
+	})
+
+	p := new(testing_provider.MockProvider)
+	p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&providerSchema{
+		ResourceTypes: map[string]*configschema.Block{
+			"test_object": {
+				Attributes: map[string]*configschema.Attribute{
+					"id": {
+						Type:     cty.String,
+						Computed: true,
+					},
+					"list": {
+						Computed: true,
+						NestedType: &configschema.Object{
+							Nesting: configschema.NestingList,
+							Attributes: map[string]*configschema.Attribute{
+								"secret": {
+									Type:      cty.String,
+									Computed:  true,
+									Sensitive: true,
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	})
+	p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) (resp providers.ApplyResourceChangeResponse) {
+		obj := req.PlannedState.AsValueMap()
+		obj["list"] = cty.ListVal([]cty.Value{
+			cty.ObjectVal(map[string]cty.Value{
+				"secret": cty.StringVal("secret"),
+			}),
+		})
+		obj["id"] = cty.StringVal("id")
+		resp.NewState = cty.ObjectVal(obj)
+		return resp
+	}
+
+	ctx := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+	})
+
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
+	assertNoErrors(t, diags)
+
+	state, diags := ctx.Apply(plan, m, nil)
+	if diags.HasErrors() {
+		t.Fatal(diags.ErrWithWarnings())
+	}
+
+	if len(state.ResourceInstance(mustResourceInstanceAddr("test_object.a")).Current.AttrSensitivePaths) < 1 {
+		t.Fatal("no attributes marked as sensitive in state")
+	}
+
+	plan, diags = ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
+	assertNoErrors(t, diags)
+
+	if c := plan.Changes.ResourceInstance(mustResourceInstanceAddr("test_object.a")); c.Action != plans.NoOp {
+		t.Errorf("Unexpected %s change for %s", c.Action, c.Addr)
+	}
+}

internal/terraform/context_plan2_test.go

@@ -5428,3 +5428,68 @@ resource "test_object" "obj" {
 		t.Fatal("data.test_data_source.foo schema contains a sensitive attribute, should be marked in state")
 	}
 }
+
+// When a schema declares that attributes nested within sets are sensitive, the
+// resulting cty values will transfer those marks to the containing set. Verify
+// that this does not present a change in the plan.
+func TestContext2Plan_nestedSensitiveMarks(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+resource "test_object" "obj" {
+  set_block {
+    foo = "bar"
+  }
+}
+`,
+	})
+
+	p := new(testing_provider.MockProvider)
+	p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&providerSchema{
+		ResourceTypes: map[string]*configschema.Block{
+			"test_object": {
+				Attributes: map[string]*configschema.Attribute{
+					"id": {
+						Type:     cty.String,
+						Computed: true,
+					},
+				},
+				BlockTypes: map[string]*configschema.NestedBlock{
+					"set_block": {
+						Nesting: configschema.NestingSet,
+						Block: configschema.Block{
+							Attributes: map[string]*configschema.Attribute{
+								"foo": {
+									Type:      cty.String,
+									Sensitive: true,
+									Optional:  true,
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	})
+
+	state := states.BuildState(func(s *states.SyncState) {
+		s.SetResourceInstanceCurrent(mustResourceInstanceAddr("test_object.obj"), &states.ResourceInstanceObjectSrc{
+			AttrsJSON:          []byte(`{"id":"z","set_block":[{"foo":"bar"}]}`),
+			AttrSensitivePaths: []cty.PathValueMarks{{Path: cty.Path{cty.GetAttrStep{Name: "set_block"}}, Marks: cty.NewValueMarks(marks.Sensitive)}},
+			Status:             states.ObjectReady,
+		}, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`))
+	})
+
+	ctx := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+	})
+
+	plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
+	assertNoErrors(t, diags)
+
+	ch := plan.Changes.ResourceInstance(mustResourceInstanceAddr("test_object.obj"))
+	if ch.Action != plans.NoOp {
+		t.Fatal("expected no change in plan")
+	}
+}

internal/terraform/marks.go

@@ -10,20 +10,11 @@ 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
+// valueMarksEqual compares the marks of 2 cty.Values for equality.
+func valueMarksEqual(a, b cty.Value) bool {
+	_, aMarks := a.UnmarkDeepWithPaths()
+	_, bMarks := b.UnmarkDeepWithPaths()
+	return marksEqual(aMarks, bMarks)
 }
 
 // marksEqual compares 2 unordered sets of PathValue marks for equality, with
@@ -56,26 +47,3 @@ func marksEqual(a, b []cty.PathValueMarks) bool {
 
 	return true
 }
-
-// Remove duplicate PathValueMarks from the slice.
-// When adding marks from a resource schema to a value, most of the time there
-// will be duplicates from a prior value already in the list of marks. While
-// MarkwithPaths will accept duplicates, we need to be able to easily compare
-// the PathValueMarks within this package too.
-func dedupePathValueMarks(m []cty.PathValueMarks) []cty.PathValueMarks {
-	var res []cty.PathValueMarks
-	// we'll use a GoString format key to differentiate PathValueMarks, since
-	// the Path portion is not automagically comparable.
-	seen := make(map[string]bool)
-
-	for _, pvm := range m {
-		key := fmt.Sprintf("%#v", pvm)
-		if _, ok := seen[key]; ok {
-			continue
-		}
-		seen[key] = true
-		res = append(res, pvm)
-	}
-
-	return res
-}

internal/terraform/node_resource_abstract_instance.go

@@ -726,8 +726,7 @@ func (n *NodeAbstractResourceInstance) refresh(ctx EvalContext, deposedKey state
 	// configuration, as well as any marks from the schema which were not in
 	// the prior state. New marks may appear when the prior state was from an
 	// import operation, or if the provider added new marks to the schema.
-	if marks := dedupePathValueMarks(append(priorMarks, schema.ValueMarks(ret.Value, nil)...)); len(marks) > 0 {
-		//if marks := priorMarks; len(marks) > 0 {
+	if marks := append(priorMarks, schema.ValueMarks(ret.Value, nil)...); len(marks) > 0 {
 		ret.Value = ret.Value.MarkWithPaths(marks)
 	}
 
@@ -878,7 +877,7 @@ func (n *NodeAbstractResourceInstance) plan(
 	// Store the paths for the config val to re-mark after we've sent things
 	// over the wire.
 	unmarkedConfigVal, unmarkedPaths := configValIgnored.UnmarkDeepWithPaths()
-	unmarkedPriorVal, priorPaths := priorVal.UnmarkDeepWithPaths()
+	unmarkedPriorVal, _ := priorVal.UnmarkDeepWithPaths()
 
 	proposedNewVal := objchange.ProposedNew(schema, unmarkedPriorVal, unmarkedConfigVal)
 
@@ -1009,7 +1008,7 @@ func (n *NodeAbstractResourceInstance) plan(
 	// to ensure that provider defined private attributes are marked correctly
 	// here.
 	unmarkedPlannedNewVal := plannedNewVal
-	unmarkedPaths = dedupePathValueMarks(append(unmarkedPaths, schema.ValueMarks(plannedNewVal, nil)...))
+	unmarkedPaths = append(unmarkedPaths, schema.ValueMarks(plannedNewVal, nil)...)
 
 	if len(unmarkedPaths) > 0 {
 		plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths)
@@ -1212,16 +1211,15 @@ func (n *NodeAbstractResourceInstance) plan(
 		actionReason = plans.ResourceInstanceReplaceBecauseTainted
 	}
 
-	// If we plan to write or delete sensitive paths from state,
-	// this is an Update action.
+	// If we plan to change the sensitivity on some portion of the value, this
+	// is an Update action even when the values are otherwise equal.
 	//
-	// 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) {
+	// The marks should be normalized by being applied to the new value. We
+	// don't try to compare the marks we had collected from the config and
+	// schema, because the act of applying marks to a value may result in
+	// slightly different marks. For example marks within a set transfer to the
+	// entire set, and are not saved on the individual elements.
+	if action == plans.NoOp && !valueMarksEqual(plannedNewVal, priorVal) {
 		action = plans.Update
 	}
 
@@ -1647,7 +1645,7 @@ func (n *NodeAbstractResourceInstance) readDataSource(ctx EvalContext, configVal
 		newVal = cty.UnknownAsNull(newVal)
 	}
 
-	pvm = dedupePathValueMarks(append(pvm, schema.ValueMarks(newVal, nil)...))
+	pvm = append(pvm, schema.ValueMarks(newVal, nil)...)
 	if len(pvm) > 0 {
 		newVal = newVal.MarkWithPaths(pvm)
 	}
@@ -1801,7 +1799,7 @@ func (n *NodeAbstractResourceInstance) planDataSource(ctx EvalContext, checkRule
 		// even though we are only returning the config value because we can't
 		// yet read the data source, we need to incorporate the schema marks so
 		// that downstream consumers can detect them when planning.
-		unmarkedPaths = dedupePathValueMarks(append(unmarkedPaths, schema.ValueMarks(proposedNewVal, nil)...))
+		unmarkedPaths = append(unmarkedPaths, schema.ValueMarks(proposedNewVal, nil)...)
 		if len(unmarkedPaths) > 0 {
 			proposedNewVal = proposedNewVal.MarkWithPaths(unmarkedPaths)
 		}
@@ -1869,7 +1867,7 @@ func (n *NodeAbstractResourceInstance) planDataSource(ctx EvalContext, checkRule
 			// not only do we want to ensure this synthetic value has the marks,
 			// but since this is the value being returned from the data source
 			// we need to ensure the schema marks are added as well.
-			unmarkedPaths = dedupePathValueMarks(append(unmarkedPaths, schema.ValueMarks(newVal, nil)...))
+			unmarkedPaths = append(unmarkedPaths, schema.ValueMarks(newVal, nil)...)
 
 			if len(unmarkedPaths) > 0 {
 				newVal = newVal.MarkWithPaths(unmarkedPaths)
@@ -2491,9 +2489,12 @@ func (n *NodeAbstractResourceInstance) apply(
 	// incomplete.
 	newVal := resp.NewState
 
-	// If we have paths to mark, mark those on this new value
-	if len(afterPaths) > 0 {
-		newVal = newVal.MarkWithPaths(afterPaths)
+	// If we have paths to mark, mark those on this new value we need to
+	// re-check the value against the schema, because nested computed values
+	// won't be included in afterPaths, which are only what was read from the
+	// After plan value.
+	if marks := append(afterPaths, schema.ValueMarks(newVal, nil)...); len(marks) > 0 {
+		newVal = newVal.MarkWithPaths(marks)
 	}
 
 	if newVal == cty.NilVal {

internal/terraform/node_resource_plan_partialexp.go

@@ -275,7 +275,7 @@ func (n *nodePlannablePartialExpandedResource) managedResourceExecute(ctx EvalCo
 
 	// We need to combine the dynamic marks with the static marks implied by
 	// the provider's schema.
-	unmarkedPaths = dedupePathValueMarks(append(unmarkedPaths, schema.ValueMarks(plannedNewVal, nil)...))
+	unmarkedPaths = append(unmarkedPaths, schema.ValueMarks(plannedNewVal, nil)...)
 	if len(unmarkedPaths) > 0 {
 		plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths)
 	}