Update ordering for marking/unmarking and asserting plan valid

Update when we unmark objects so we can assert the plan is valid,
and process UnknownAsNull on the unmarked value

command/format/diff.go

@@ -1373,10 +1373,15 @@ func ctyCollectionValues(val cty.Value) []cty.Value {
 		return nil
 	}
 
+	// Sets with marks mark the whole set as marked,
+	// so when we unmark it, apply those marks to all members
+	// of the set
+	val, marks := val.Unmark()
+
 	ret := make([]cty.Value, 0, val.LengthInt())
 	for it := val.ElementIterator(); it.Next(); {
 		_, value := it.Element()
-		ret = append(ret, value)
+		ret = append(ret, value.WithMarks(marks))
 	}
 	return ret
 }

command/format/diff_test.go

@@ -3644,6 +3644,12 @@ func TestResourceChange_sensitiveVariable(t *testing.T) {
 					cty.StringVal("friends"),
 					cty.StringVal("!"),
 				}),
+				"nested_block": cty.ListVal([]cty.Value{
+					cty.ObjectVal(map[string]cty.Value{
+						"an_attr": cty.StringVal("secretval"),
+						"another": cty.StringVal("not secret"),
+					}),
+				}),
 			}),
 			AfterValMarks: []cty.PathValueMarks{
 				{
@@ -3662,6 +3668,11 @@ func TestResourceChange_sensitiveVariable(t *testing.T) {
 					Path:  cty.Path{cty.GetAttrStep{Name: "map_key"}, cty.IndexStep{Key: cty.StringVal("dinner")}},
 					Marks: cty.NewValueMarks("sensitive"),
 				},
+				{
+					// Nested blocks/sets will mark the whole set/block as sensitive
+					Path:  cty.Path{cty.GetAttrStep{Name: "nested_block"}},
+					Marks: cty.NewValueMarks("sensitive"),
+				},
 			},
 			RequiredReplace: cty.NewPathSet(),
 			Tainted:         false,
@@ -3673,6 +3684,17 @@ func TestResourceChange_sensitiveVariable(t *testing.T) {
 					"map_key":    {Type: cty.Map(cty.Number), Optional: true},
 					"list_field": {Type: cty.List(cty.String), Optional: true},
 				},
+				BlockTypes: map[string]*configschema.NestedBlock{
+					"nested_block": {
+						Block: configschema.Block{
+							Attributes: map[string]*configschema.Attribute{
+								"an_attr": {Type: cty.String, Optional: true},
+								"another": {Type: cty.String, Optional: true},
+							},
+						},
+						Nesting: configschema.NestingList,
+					},
+				},
 			},
 			ExpectedOutput: `  # test_instance.example will be created
   + resource "test_instance" "example" {
@@ -3688,6 +3710,11 @@ func TestResourceChange_sensitiveVariable(t *testing.T) {
           + "dinner"    = (sensitive)
         }
       + map_whole  = (sensitive)
+
+      + nested_block {
+          + an_attr = (sensitive)
+          + another = (sensitive)
+        }
     }
 `,
 		},

states/instance_object.go

@@ -87,6 +87,11 @@ const (
 // so the caller must not mutate the receiver any further once once this
 // method is called.
 func (o *ResourceInstanceObject) Encode(ty cty.Type, schemaVersion uint64) (*ResourceInstanceObjectSrc, error) {
+	// If it contains marks, remove these marks before traversing the
+	// structure with UnknownAsNull, and save the PathValueMarks
+	// so we can save them in state.
+	val, pvm := o.Value.UnmarkDeepWithPaths()
+
 	// Our state serialization can't represent unknown values, so we convert
 	// them to nulls here. This is lossy, but nobody should be writing unknown
 	// values here and expecting to get them out again later.
@@ -96,15 +101,9 @@ func (o *ResourceInstanceObject) Encode(ty cty.Type, schemaVersion uint64) (*Res
 	// for expression evaluation. The apply step should never produce unknown
 	// values, but if it does it's the responsibility of the caller to detect
 	// and raise an error about that.
-	val := cty.UnknownAsNull(o.Value)
+	val = cty.UnknownAsNull(val)
 
-	// If it contains marks, save these in state
-	unmarked := val
-	var pvm []cty.PathValueMarks
-	if val.ContainsMarked() {
-		unmarked, pvm = val.UnmarkDeepWithPaths()
-	}
-	src, err := ctyjson.Marshal(unmarked, ty)
+	src, err := ctyjson.Marshal(val, ty)
 	if err != nil {
 		return nil, err
 	}

terraform/eval_diff.go

@@ -279,11 +279,6 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) {
 		panic(fmt.Sprintf("PlanResourceChange of %s produced nil value", absAddr.String()))
 	}
 
-	// Add the marks back to the planned new value
-	if len(unmarkedPaths) > 0 {
-		plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths)
-	}
-
 	// We allow the planned new value to disagree with configuration _values_
 	// here, since that allows the provider to do special logic like a
 	// DiffSuppressFunc, but we still require that the provider produces
@@ -302,7 +297,7 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) {
 		return nil, diags.Err()
 	}
 
-	if errs := objchange.AssertPlanValid(schema, priorVal, configValIgnored, plannedNewVal); len(errs) > 0 {
+	if errs := objchange.AssertPlanValid(schema, unmarkedPriorVal, unmarkedConfigVal, plannedNewVal); len(errs) > 0 {
 		if resp.LegacyTypeSystem {
 			// The shimming of the old type system in the legacy SDK is not precise
 			// enough to pass this consistency check, so we'll give it a pass here,
@@ -333,6 +328,13 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) {
 		}
 	}
 
+	// Add the marks back to the planned new value -- this must happen after ignore changes
+	// have been processed
+	unmarkedPlannedNewVal := plannedNewVal
+	if len(unmarkedPaths) > 0 {
+		plannedNewVal = plannedNewVal.MarkWithPaths(unmarkedPaths)
+	}
+
 	// The provider produces a list of paths to attributes whose changes mean
 	// that we must replace rather than update an existing remote object.
 	// However, we only need to do that if the identified attributes _have_
@@ -395,7 +397,6 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) {
 
 	// Unmark for this test for equality. If only sensitivity has changed,
 	// this does not require an Update or Replace
-	unmarkedPlannedNewVal, _ := plannedNewVal.UnmarkDeep()
 	eqV := unmarkedPlannedNewVal.Equals(unmarkedPriorVal)
 	eq := eqV.IsKnown() && eqV.True()