Serialize AttrSensitivePaths in a consistent order

.changes/unreleased/BUG FIXES-20250210-163038.yaml

@@ -0,0 +1,5 @@
+kind: BUG FIXES
+body: Changes to the order of sensitive attributes in the state format would erroneously indicate a plan contained changes when there were none.
+time: 2025-02-10T16:30:38.78853-05:00
+custom:
+    Issue: "36465"

internal/lang/format/format.go

@@ -0,0 +1,68 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: BUSL-1.1
+
+package format
+
+import (
+	"fmt"
+	"strconv"
+	"strings"
+
+	"github.com/zclconf/go-cty/cty"
+)
+
+// CtyPath is a helper function to produce a user-friendly string
+// representation of a cty.Path. The result uses a syntax similar to the
+// HCL expression language in the hope of it being familiar to users.
+func CtyPath(path cty.Path) string {
+	var buf strings.Builder
+	for _, step := range path {
+		switch ts := step.(type) {
+		case cty.GetAttrStep:
+			fmt.Fprintf(&buf, ".%s", ts.Name)
+		case cty.IndexStep:
+			buf.WriteByte('[')
+			key := ts.Key
+			keyTy := key.Type()
+			switch {
+			case key.IsNull():
+				buf.WriteString("null")
+			case !key.IsKnown():
+				buf.WriteString("(not yet known)")
+			case keyTy == cty.Number:
+				bf := key.AsBigFloat()
+				buf.WriteString(bf.Text('g', -1))
+			case keyTy == cty.String:
+				buf.WriteString(strconv.Quote(key.AsString()))
+			default:
+				buf.WriteString("...")
+			}
+			buf.WriteByte(']')
+		}
+	}
+	return buf.String()
+}
+
+// ErrorDiag is a helper function to produce a user-friendly string
+// representation of certain special error types that we might want to
+// include in diagnostic messages.
+func ErrorDiag(err error) string {
+	perr, ok := err.(cty.PathError)
+	if !ok || len(perr.Path) == 0 {
+		return err.Error()
+	}
+
+	return fmt.Sprintf("%s: %s", CtyPath(perr.Path), perr.Error())
+}
+
+// ErrorDiagPrefixed is like Error except that it presents any path
+// information after the given prefix string, which is assumed to contain
+// an HCL syntax representation of the value that errors are relative to.
+func ErrorDiagPrefixed(err error, prefix string) string {
+	perr, ok := err.(cty.PathError)
+	if !ok || len(perr.Path) == 0 {
+		return fmt.Sprintf("%s: %s", prefix, err.Error())
+	}
+
+	return fmt.Sprintf("%s%s: %s", prefix, CtyPath(perr.Path), perr.Error())
+}

internal/lang/marks/paths.go

@@ -4,9 +4,10 @@
 package marks
 
 import (
-	"fmt"
 	"sort"
+	"strings"
 
+	"github.com/hashicorp/terraform/internal/lang/format"
 	"github.com/zclconf/go-cty/cty"
 )
 
@@ -77,9 +78,17 @@ func MarksEqual(a, b []cty.PathValueMarks) bool {
 
 	less := func(s []cty.PathValueMarks) func(i, j int) bool {
 		return func(i, j int) bool {
+			cmp := strings.Compare(format.CtyPath(s[i].Path), format.CtyPath(s[j].Path))
+
+			switch {
+			case cmp < 0:
+				return true
+			case cmp > 0:
+				return false
+			}
 			// the sort only needs to be consistent, so use the GoString format
 			// to get a comparable value
-			return fmt.Sprintf("%#v", s[i]) < fmt.Sprintf("%#v", s[j])
+			return s[i].Marks.GoString() < s[j].Marks.GoString()
 		}
 	}
 

internal/moduletest/mocking/values.go

@@ -220,7 +220,7 @@ func (data MockedData) makeKnown(attribute *configschema.Attribute, with cty.Val
 			diags = diags.Append(tfdiags.AttributeValue(
 				tfdiags.Error,
 				"Failed to compute attribute",
-				fmt.Sprintf("Terraform could not compute a value for the target type %s with the mocked data defined at %s with the attribute %q: %s.", attribute.ImpliedType().FriendlyName(), data.Range, fmtPath(append(path, relPath...)), err),
+				fmt.Sprintf("Terraform could not compute a value for the target type %s with the mocked data defined at %s with the attribute %q: %s.", attribute.ImpliedType().FriendlyName(), data.Range, tfdiags.FormatCtyPath(append(path, relPath...)), err),
 				path))
 
 			// We still want to return a valid value here. If the conversion did
@@ -285,7 +285,7 @@ func (data MockedData) getMockedDataForPath(path cty.Path) (cty.Value, tfdiags.D
 				diags = diags.Append(tfdiags.AttributeValue(
 					tfdiags.Error,
 					"Failed to compute attribute",
-					fmt.Sprintf("Terraform expected an object type for attribute %q defined within the mocked data at %s, but found %s.", fmtPath(currentPath), data.Range, current.Type().FriendlyName()),
+					fmt.Sprintf("Terraform expected an object type for attribute %q defined within the mocked data at %s, but found %s.", tfdiags.FormatCtyPath(currentPath), data.Range, current.Type().FriendlyName()),
 					currentPath))
 
 				return cty.NilVal, diags
@@ -304,23 +304,3 @@ func (data MockedData) getMockedDataForPath(path cty.Path) (cty.Value, tfdiags.D
 
 	return current, diags
 }
-
-func fmtPath(path cty.Path) string {
-	var current string
-
-	first := true
-	for _, step := range path {
-		// Since we only ever parse the attribute steps when finding replacement
-		// values, we can do the same again here.
-		switch step := step.(type) {
-		case cty.GetAttrStep:
-			if first {
-				first = false
-				current = step.Name
-				continue
-			}
-			current = fmt.Sprintf("%s.%s", current, step.Name)
-		}
-	}
-	return current
-}

internal/moduletest/mocking/values_test.go

@@ -809,7 +809,7 @@ func TestComputedValuesForDataSource(t *testing.T) {
 				}),
 			}),
 			expectedFailures: []string{
-				"Terraform expected an object type for attribute \"nested_object\" defined within the mocked data at :0,0-0, but found string.",
+				"Terraform expected an object type for attribute \".nested_object[...]\" defined within the mocked data at :0,0-0, but found string.",
 			},
 		},
 		"invalid_replacement_path_nested_block": {
@@ -846,7 +846,7 @@ func TestComputedValuesForDataSource(t *testing.T) {
 				}),
 			}),
 			expectedFailures: []string{
-				"Terraform expected an object type for attribute \"nested_object\" defined within the mocked data at :0,0-0, but found string.",
+				"Terraform expected an object type for attribute \".nested_object[...]\" defined within the mocked data at :0,0-0, but found string.",
 			},
 		},
 		"invalid_replacement_type": {
@@ -863,7 +863,7 @@ func TestComputedValuesForDataSource(t *testing.T) {
 				"value": cty.StringVal("Hello, world!"),
 			}),
 			expectedFailures: []string{
-				"Terraform could not compute a value for the target type string with the mocked data defined at :0,0-0 with the attribute \"id\": string required.",
+				"Terraform could not compute a value for the target type string with the mocked data defined at :0,0-0 with the attribute \".id\": string required.",
 			},
 		},
 		"invalid_replacement_type_nested": {
@@ -899,7 +899,7 @@ func TestComputedValuesForDataSource(t *testing.T) {
 				}),
 			}),
 			expectedFailures: []string{
-				"Terraform could not compute a value for the target type string with the mocked data defined at :0,0-0 with the attribute \"nested.id\": string required.",
+				`Terraform could not compute a value for the target type string with the mocked data defined at :0,0-0 with the attribute ".nested[\"one\"].id": string required.`,
 			},
 		},
 		"invalid_replacement_type_nested_block": {
@@ -933,7 +933,7 @@ func TestComputedValuesForDataSource(t *testing.T) {
 				}),
 			}),
 			expectedFailures: []string{
-				"Terraform could not compute a value for the target type string with the mocked data defined at :0,0-0 with the attribute \"block.id\": string required.",
+				"Terraform could not compute a value for the target type string with the mocked data defined at :0,0-0 with the attribute \".block[0].id\": string required.",
 			},
 		},
 		"dynamic_attribute_unset": {

internal/states/instance_object.go

@@ -11,8 +11,8 @@ import (
 	ctyjson "github.com/zclconf/go-cty/cty/json"
 
 	"github.com/hashicorp/terraform/internal/addrs"
+	"github.com/hashicorp/terraform/internal/lang/format"
 	"github.com/hashicorp/terraform/internal/lang/marks"
-	"github.com/hashicorp/terraform/internal/tfdiags"
 )
 
 // ResourceInstanceObject is the local representation of a specific remote
@@ -173,8 +173,15 @@ func unmarkValueForStorage(v cty.Value) (unmarkedV cty.Value, sensitivePaths []c
 	if len(withOtherMarks) != 0 {
 		return cty.NilVal, nil, fmt.Errorf(
 			"%s: cannot serialize value marked as %#v for inclusion in a state snapshot (this is a bug in Terraform)",
-			tfdiags.FormatCtyPath(withOtherMarks[0].Path), withOtherMarks[0].Marks,
+			format.CtyPath(withOtherMarks[0].Path), withOtherMarks[0].Marks,
 		)
 	}
+
+	// sort the sensitive paths for consistency in comparison and serialization
+	sort.Slice(sensitivePaths, func(i, j int) bool {
+		// use our human-readable format of paths for comparison
+		return format.CtyPath(sensitivePaths[i]) < format.CtyPath(sensitivePaths[j])
+	})
+
 	return val, sensitivePaths, nil
 }

internal/states/instance_object_test.go

@@ -4,17 +4,24 @@
 package states
 
 import (
+	"fmt"
 	"sync"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/hashicorp/terraform/internal/addrs"
+	"github.com/hashicorp/terraform/internal/lang/marks"
 	"github.com/zclconf/go-cty/cty"
 )
 
 func TestResourceInstanceObject_encode(t *testing.T) {
 	value := cty.ObjectVal(map[string]cty.Value{
 		"foo": cty.True,
+		"obj": cty.ObjectVal(map[string]cty.Value{
+			"sensitive": cty.StringVal("secret").Mark(marks.Sensitive),
+		}),
+		"sensitive_a": cty.StringVal("secret").Mark(marks.Sensitive),
+		"sensitive_b": cty.StringVal("secret").Mark(marks.Sensitive),
 	})
 	// The in-memory order of resource dependencies is random, since they're an
 	// unordered set.
@@ -83,6 +90,14 @@ func TestResourceInstanceObject_encode(t *testing.T) {
 			t.Errorf("identical dependencies got encoded in different orders:\n%s", diff)
 		}
 	}
+
+	// sensitive paths must also be consistent got comparison
+	for i := 0; i < len(encoded)-1; i++ {
+		a, b := fmt.Sprintf("%#v", encoded[i].AttrSensitivePaths), fmt.Sprintf("%#v", encoded[i+1].AttrSensitivePaths)
+		if diff := cmp.Diff(a, b); diff != "" {
+			t.Errorf("sensitive paths got encoded in different orders:\n%s", diff)
+		}
+	}
 }
 
 func TestResourceInstanceObject_encodeInvalidMarks(t *testing.T) {

internal/terraform/node_resource_abstract_instance.go

@@ -18,6 +18,7 @@ import (
 	"github.com/hashicorp/terraform/internal/configs/configschema"
 	"github.com/hashicorp/terraform/internal/instances"
 	"github.com/hashicorp/terraform/internal/lang/ephemeral"
+	"github.com/hashicorp/terraform/internal/lang/format"
 	"github.com/hashicorp/terraform/internal/lang/marks"
 	"github.com/hashicorp/terraform/internal/moduletest/mocking"
 	"github.com/hashicorp/terraform/internal/plans"
@@ -2483,7 +2484,7 @@ func (n *NodeAbstractResourceInstance) apply(
 		var unknownPaths []string
 		cty.Transform(configVal, func(p cty.Path, v cty.Value) (cty.Value, error) {
 			if !v.IsKnown() {
-				unknownPaths = append(unknownPaths, fmt.Sprintf("%#v", p))
+				unknownPaths = append(unknownPaths, format.CtyPath(p))
 			}
 			return v, nil
 		})

internal/tfdiags/config_traversals.go

@@ -4,68 +4,23 @@
 package tfdiags
 
 import (
-	"bytes"
-	"fmt"
-	"strconv"
-
-	"github.com/zclconf/go-cty/cty"
+	"github.com/hashicorp/terraform/internal/lang/format"
 )
 
+// These functions have been moved to the format package to allow for imports
+// which would otherwise cause cycles.
+
 // FormatCtyPath is a helper function to produce a user-friendly string
 // representation of a cty.Path. The result uses a syntax similar to the
 // HCL expression language in the hope of it being familiar to users.
-func FormatCtyPath(path cty.Path) string {
-	var buf bytes.Buffer
-	for _, step := range path {
-		switch ts := step.(type) {
-		case cty.GetAttrStep:
-			fmt.Fprintf(&buf, ".%s", ts.Name)
-		case cty.IndexStep:
-			buf.WriteByte('[')
-			key := ts.Key
-			keyTy := key.Type()
-			switch {
-			case key.IsNull():
-				buf.WriteString("null")
-			case !key.IsKnown():
-				buf.WriteString("(not yet known)")
-			case keyTy == cty.Number:
-				bf := key.AsBigFloat()
-				buf.WriteString(bf.Text('g', -1))
-			case keyTy == cty.String:
-				buf.WriteString(strconv.Quote(key.AsString()))
-			default:
-				buf.WriteString("...")
-			}
-			buf.WriteByte(']')
-		}
-	}
-	return buf.String()
-}
+var FormatCtyPath = format.CtyPath
 
 // FormatError is a helper function to produce a user-friendly string
 // representation of certain special error types that we might want to
 // include in diagnostic messages.
-//
-// This currently has special behavior only for cty.PathError, where a
-// non-empty path is rendered in a HCL-like syntax as context.
-func FormatError(err error) string {
-	perr, ok := err.(cty.PathError)
-	if !ok || len(perr.Path) == 0 {
-		return err.Error()
-	}
-
-	return fmt.Sprintf("%s: %s", FormatCtyPath(perr.Path), perr.Error())
-}
+var FormatError = format.ErrorDiag
 
 // FormatErrorPrefixed is like FormatError except that it presents any path
 // information after the given prefix string, which is assumed to contain
 // an HCL syntax representation of the value that errors are relative to.
-func FormatErrorPrefixed(err error, prefix string) string {
-	perr, ok := err.(cty.PathError)
-	if !ok || len(perr.Path) == 0 {
-		return fmt.Sprintf("%s: %s", prefix, err.Error())
-	}
-
-	return fmt.Sprintf("%s%s: %s", prefix, FormatCtyPath(perr.Path), perr.Error())
-}
+var FormatErrorPrefixed = format.ErrorDiagPrefixed