Merge pull request #28687 from hashicorp/jbardin/sensitive-changes

Decode change values with marks

command/format/diff.go

@@ -129,14 +129,6 @@ func ResourceChange(
 	changeV.Change.Before = objchange.NormalizeObjectFromLegacySDK(changeV.Change.Before, schema)
 	changeV.Change.After = objchange.NormalizeObjectFromLegacySDK(changeV.Change.After, schema)
 
-	// Now that the change is decoded, add back the marks at the defined paths
-	if len(change.BeforeValMarks) > 0 {
-		changeV.Change.Before = changeV.Change.Before.MarkWithPaths(change.BeforeValMarks)
-	}
-	if len(change.AfterValMarks) > 0 {
-		changeV.Change.After = changeV.Change.After.MarkWithPaths(change.AfterValMarks)
-	}
-
 	result := p.writeBlockBodyDiff(schema, changeV.Before, changeV.After, 6, path)
 	if result.bodyWritten {
 		buf.WriteString("\n")

command/jsonplan/plan.go

@@ -349,10 +349,15 @@ func (p *plan) marshalResourceChanges(changes *plans.Changes, schemas *terraform
 		if err != nil {
 			return err
 		}
+		// We drop the marks from the change, as decoding is only an
+		// intermediate step to re-encode the values as json
+		changeV.Before, _ = changeV.Before.UnmarkDeep()
+		changeV.After, _ = changeV.After.UnmarkDeep()
 
 		var before, after []byte
 		var beforeSensitive, afterSensitive []byte
 		var afterUnknown cty.Value
+
 		if changeV.Before != cty.NilVal {
 			before, err = ctyjson.Marshal(changeV.Before, changeV.Before.Type())
 			if err != nil {
@@ -475,6 +480,10 @@ func (p *plan) marshalOutputChanges(changes *plans.Changes) error {
 		if err != nil {
 			return err
 		}
+		// We drop the marks from the change, as decoding is only an
+		// intermediate step to re-encode the values as json
+		changeV.Before, _ = changeV.Before.UnmarkDeep()
+		changeV.After, _ = changeV.After.UnmarkDeep()
 
 		var before, after []byte
 		afterUnknown := cty.False

command/jsonplan/values.go

@@ -60,13 +60,15 @@ func marshalPlannedOutputs(changes *plans.Changes) (map[string]output, error) {
 		if err != nil {
 			return ret, err
 		}
+		// The values may be marked, but we must rely on the Sensitive flag
+		// as the decoded value is only an intermediate step in transcoding
+		// this to a json format.
+		changeV.After, _ = changeV.After.UnmarkDeep()
 
-		if changeV.After != cty.NilVal {
-			if changeV.After.IsWhollyKnown() {
-				after, err = ctyjson.Marshal(changeV.After, changeV.After.Type())
-				if err != nil {
-					return ret, err
-				}
+		if changeV.After != cty.NilVal && changeV.After.IsWhollyKnown() {
+			after, err = ctyjson.Marshal(changeV.After, changeV.After.Type())
+			if err != nil {
+				return ret, err
 			}
 		}
 
@@ -193,6 +195,11 @@ func marshalPlanResources(changes *plans.Changes, ris []addrs.AbsResourceInstanc
 		if err != nil {
 			return nil, err
 		}
+		// The values may be marked, but we must rely on the Sensitive flag
+		// as the decoded value is only an intermediate step in transcoding
+		// this to a json format.
+		changeV.Before, _ = changeV.Before.UnmarkDeep()
+		changeV.After, _ = changeV.After.UnmarkDeep()
 
 		if changeV.After != cty.NilVal {
 			if changeV.After.IsWhollyKnown() {

plans/changes_src.go

@@ -200,9 +200,10 @@ func (cs *ChangeSrc) Decode(ty cty.Type) (*Change, error) {
 			return nil, fmt.Errorf("error decoding 'after' value: %s", err)
 		}
 	}
+
 	return &Change{
 		Action: cs.Action,
-		Before: before,
-		After:  after,
+		Before: before.MarkWithPaths(cs.BeforeValMarks),
+		After:  after.MarkWithPaths(cs.AfterValMarks),
 	}, nil
 }

plans/changes_test.go

@@ -0,0 +1,41 @@
+package plans
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/zclconf/go-cty/cty"
+)
+
+func TestChangeEncodeSensitive(t *testing.T) {
+	testVals := []cty.Value{
+		cty.ObjectVal(map[string]cty.Value{
+			"ding": cty.StringVal("dong").Mark("sensitive"),
+		}),
+		cty.StringVal("bleep").Mark("bloop"),
+		cty.ListVal([]cty.Value{cty.UnknownVal(cty.String).Mark("sup?")}),
+	}
+
+	for _, v := range testVals {
+		t.Run(fmt.Sprintf("%#v", v), func(t *testing.T) {
+			change := Change{
+				Before: cty.NullVal(v.Type()),
+				After:  v,
+			}
+
+			encoded, err := change.Encode(v.Type())
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			decoded, err := encoded.Decode(v.Type())
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			if !v.RawEquals(decoded.After) {
+				t.Fatalf("%#v != %#v\n", decoded.After, v)
+			}
+		})
+	}
+}

plans/objchange/normalize_obj.go

@@ -19,6 +19,12 @@ import (
 // okay because type consistency is enforced when deserializing the value
 // returned from the provider over the RPC wire protocol anyway.
 func NormalizeObjectFromLegacySDK(val cty.Value, schema *configschema.Block) cty.Value {
+	val, valMarks := val.UnmarkDeepWithPaths()
+	val = normalizeObjectFromLegacySDK(val, schema)
+	return val.MarkWithPaths(valMarks)
+}
+
+func normalizeObjectFromLegacySDK(val cty.Value, schema *configschema.Block) cty.Value {
 	if val == cty.NilVal || val.IsNull() {
 		// This should never happen in reasonable use, but we'll allow it
 		// and normalize to a null of the expected type rather than panicking
@@ -50,7 +56,7 @@ func NormalizeObjectFromLegacySDK(val cty.Value, schema *configschema.Block) cty
 				if lv.IsNull() && blockS.Nesting == configschema.NestingGroup {
 					vals[name] = blockS.EmptyValue()
 				} else {
-					vals[name] = NormalizeObjectFromLegacySDK(lv, &blockS.Block)
+					vals[name] = normalizeObjectFromLegacySDK(lv, &blockS.Block)
 				}
 			} else {
 				vals[name] = unknownBlockStub(&blockS.Block)
@@ -65,7 +71,7 @@ func NormalizeObjectFromLegacySDK(val cty.Value, schema *configschema.Block) cty
 				subVals := make([]cty.Value, 0, lv.LengthInt())
 				for it := lv.ElementIterator(); it.Next(); {
 					_, subVal := it.Element()
-					subVals = append(subVals, NormalizeObjectFromLegacySDK(subVal, &blockS.Block))
+					subVals = append(subVals, normalizeObjectFromLegacySDK(subVal, &blockS.Block))
 				}
 				vals[name] = cty.ListVal(subVals)
 			}
@@ -79,7 +85,7 @@ func NormalizeObjectFromLegacySDK(val cty.Value, schema *configschema.Block) cty
 				subVals := make([]cty.Value, 0, lv.LengthInt())
 				for it := lv.ElementIterator(); it.Next(); {
 					_, subVal := it.Element()
-					subVals = append(subVals, NormalizeObjectFromLegacySDK(subVal, &blockS.Block))
+					subVals = append(subVals, normalizeObjectFromLegacySDK(subVal, &blockS.Block))
 				}
 				vals[name] = cty.SetVal(subVals)
 			}

terraform/context_apply2_test.go

@@ -446,3 +446,64 @@ resource "test_resource" "b" {
 		t.Fatal(diags.ErrWithWarnings())
 	}
 }
+
+func TestContext2Apply_sensitiveOutputPassthrough(t *testing.T) {
+	// Ensure we're not trying to double-mark values decoded from state
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+module "mod" {
+  source = "./mod"
+}
+
+resource "test_object" "a" {
+  test_string = module.mod.out
+}
+`,
+
+		"mod/main.tf": `
+variable "in" {
+  sensitive = true
+  default = "foo"
+}
+output "out" {
+  value = var.in
+}
+`,
+	})
+
+	p := simpleMockProvider()
+
+	ctx := testContext2(t, &ContextOpts{
+		Config: m,
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+	})
+
+	_, diags := ctx.Plan()
+	if diags.HasErrors() {
+		t.Fatal(diags.ErrWithWarnings())
+	}
+
+	state, diags := ctx.Apply()
+	if diags.HasErrors() {
+		t.Fatal(diags.ErrWithWarnings())
+	}
+
+	obj := state.ResourceInstance(mustResourceInstanceAddr("test_object.a"))
+	if len(obj.Current.AttrSensitivePaths) != 1 {
+		t.Fatalf("Expected 1 sensitive mark for test_object.a, got %#v\n", obj.Current.AttrSensitivePaths)
+	}
+
+	plan, diags := ctx.Plan()
+	if diags.HasErrors() {
+		t.Fatal(diags.ErrWithWarnings())
+	}
+
+	// make sure the same marks are compared in the next plan as well
+	for _, c := range plan.Changes.Resources {
+		if c.Action != plans.NoOp {
+			t.Errorf("Unexpcetd %s change for %s", c.Action, c.Addr)
+		}
+	}
+}

terraform/context_plan_test.go

@@ -4793,7 +4793,7 @@ func TestContext2Plan_ignoreChangesSensitive(t *testing.T) {
 
 	checkVals(t, objectVal(t, schema, map[string]cty.Value{
 		"id":   cty.StringVal("bar"),
-		"ami":  cty.StringVal("ami-abcd1234"),
+		"ami":  cty.StringVal("ami-abcd1234").Mark("sensitive"),
 		"type": cty.StringVal("aws_instance"),
 	}), ric.After)
 }
@@ -5627,7 +5627,7 @@ func TestContext2Plan_variableSensitivity(t *testing.T) {
 		switch i := ric.Addr.String(); i {
 		case "aws_instance.foo":
 			checkVals(t, objectVal(t, schema, map[string]cty.Value{
-				"foo": cty.StringVal("foo"),
+				"foo": cty.StringVal("foo").Mark("sensitive"),
 			}), ric.After)
 			if len(res.ChangeSrc.BeforeValMarks) != 0 {
 				t.Errorf("unexpected BeforeValMarks: %#v", res.ChangeSrc.BeforeValMarks)
@@ -5694,8 +5694,8 @@ func TestContext2Plan_variableSensitivityModule(t *testing.T) {
 		switch i := ric.Addr.String(); i {
 		case "module.child.aws_instance.foo":
 			checkVals(t, objectVal(t, schema, map[string]cty.Value{
-				"foo":   cty.StringVal("foo"),
-				"value": cty.StringVal("boop"),
+				"foo":   cty.StringVal("foo").Mark("sensitive"),
+				"value": cty.StringVal("boop").Mark("sensitive"),
 			}), ric.After)
 			if len(res.ChangeSrc.BeforeValMarks) != 0 {
 				t.Errorf("unexpected BeforeValMarks: %#v", res.ChangeSrc.BeforeValMarks)
@@ -5729,6 +5729,13 @@ func TestContext2Plan_variableSensitivityModule(t *testing.T) {
 
 func checkVals(t *testing.T, expected, got cty.Value) {
 	t.Helper()
+	// The GoStringer format seems to result in the closest thing to a useful
+	// diff for values with marks.
+	// TODO: if we want to continue using cmp.Diff on cty.Values, we should
+	// make a transformer that creates a more comparable structure.
+	valueTrans := cmp.Transformer("gostring", func(v cty.Value) string {
+		return fmt.Sprintf("%#v\n", v)
+	})
 	if !cmp.Equal(expected, got, valueComparer, typeComparer, equateEmpty) {
 		t.Fatal(cmp.Diff(expected, got, valueTrans, equateEmpty))
 	}

terraform/evaluate.go

@@ -459,7 +459,7 @@ func (d *evaluationStateData) GetModule(addr addrs.ModuleCall, rng tfdiags.Sourc
 				continue
 			}
 
-			instance[cfg.Name] = change.After.MarkWithPaths(changeSrc.AfterValMarks)
+			instance[cfg.Name] = change.After
 
 			if change.Sensitive && !change.After.HasMark("sensitive") {
 				instance[cfg.Name] = change.After.Mark("sensitive")

terraform/node_output.go

@@ -259,9 +259,11 @@ func (n *NodeApplyableOutput) Execute(ctx EvalContext, op walkOperation) (diags
 	// we we have a change recorded, we don't need to re-evaluate if the value
 	// was known
 	if changeRecorded {
-		var err error
-		val, err = n.Change.After.Decode(cty.DynamicPseudoType)
+		change, err := n.Change.Decode()
 		diags = diags.Append(err)
+		if err == nil {
+			val = change.After
+		}
 	}
 
 	// If there was no change recorded, or the recorded change was not wholly