helper/schema: Schema.DiffSuppressOnRefresh (#882)

Unfortunately in the original design of DiffSuppressFunc we made it work
only during planning, and so the normalization vs. drift classification
it does isn't honored in any other situation.

Unilaterally enabling it for both refresh and apply would be too risky at
this late stage where so many existing provider behaviors rely on these
subtle details, but here we introduce a more surgical _opt-in_ mechanism
whereby specific attributes can be set to also check the Refresh result
against DiffSuppressFunc, and so avoid reporting any normalization done
by the remote system as if it were drift.

This behavior will be primarily useful for strings containing complex data
serialized into some particular microsyntax which allows expressing the
same information multiple ways. In particular, existing situations where
we classify whitespace changes and other such immaterial changes in JSON
values as normalization vs. drift would be a good candidate to enable
this flag, so that we can get the same results during refreshing and thus
avoid churning any downstream resources that refer to the
unexpectedly-updated result.

.changelog/882.txt

@@ -0,0 +1,7 @@
+```release-note:note
+helper/schema: The `Schema` type `DiffSuppressOnRefresh` field opts in to using `DiffSuppressFunc` to detect normalization changes during refresh, using the same rules as for planning. This can prevent normalization cascading downstream and producing confusing changes in other resources, and will avoid reporting "Values changed outside of Terraform" for normalization-only situations. This is a desirable behavior for most attributes that have `DiffSuppressFunc` and so would ideally be on by default, but it is opt-in for backward compatibility reasons.
+```
+
+```release-note:enhancement
+helper/schema: Added the `DiffSuppressOnRefresh` field to the `Schema` type
+```

helper/schema/resource.go

@@ -655,6 +655,7 @@ func (r *Resource) RefreshWithoutUpgrade(
 		state = nil
 	}
 
+	schemaMap(r.Schema).handleDiffSuppressOnRefresh(ctx, s, state)
 	return r.recordCurrentSchemaVersion(state), diags
 }
 

helper/schema/resource_test.go

@@ -1143,6 +1143,53 @@ func TestResourceRefresh(t *testing.T) {
 	}
 }
 
+func TestResourceRefresh_DiffSuppressOnRefresh(t *testing.T) {
+	r := &Resource{
+		SchemaVersion: 2,
+		Schema: map[string]*Schema{
+			"foo": {
+				Type:     TypeString,
+				Optional: true,
+				DiffSuppressFunc: func(key, oldV, newV string, d *ResourceData) bool {
+					return true
+				},
+				DiffSuppressOnRefresh: true,
+			},
+		},
+	}
+
+	r.Read = func(d *ResourceData, m interface{}) error {
+		return d.Set("foo", "howdy")
+	}
+
+	s := &terraform.InstanceState{
+		ID: "bar",
+		Attributes: map[string]string{
+			"foo": "hello",
+		},
+	}
+
+	expected := &terraform.InstanceState{
+		ID: "bar",
+		Attributes: map[string]string{
+			"id":  "bar",
+			"foo": "hello", // new value was suppressed
+		},
+		Meta: map[string]interface{}{
+			"schema_version": "2",
+		},
+	}
+
+	actual, diags := r.RefreshWithoutUpgrade(context.Background(), s, 42)
+	if diags.HasError() {
+		t.Fatalf("err: %s", diagutils.ErrorDiags(diags))
+	}
+
+	if !reflect.DeepEqual(actual, expected) {
+		t.Fatalf("bad: %#v", actual)
+	}
+}
+
 func TestResourceRefresh_blankId(t *testing.T) {
 	r := &Resource{
 		Schema: map[string]*Schema{

helper/schema/schema.go

@@ -23,6 +23,7 @@ import (
 	"strings"
 
 	"github.com/hashicorp/go-cty/cty"
+	"github.com/hashicorp/terraform-plugin-log/tfsdklog"
 	"github.com/mitchellh/copystructure"
 	"github.com/mitchellh/mapstructure"
 
@@ -92,8 +93,42 @@ type Schema struct {
 	// If CustomizeDiffFunc makes this field ForceNew=true, the
 	// following DiffSuppressFunc will come in with the value of old being
 	// empty, as if creating a new resource.
+	//
+	// By default, DiffSuppressFunc is considered only when deciding whether
+	// a configuration value is significantly different than the prior state
+	// value during planning. Set DiffSuppressOnRefresh to opt in to checking
+	// this also during the refresh step.
 	DiffSuppressFunc SchemaDiffSuppressFunc
 
+	// DiffSuppressOnRefresh enables using the DiffSuppressFunc to ignore
+	// normalization-classified changes returned by the resource type's
+	// "Read" or "ReadContext" function, in addition to the default behavior of
+	// doing so during planning.
+	//
+	// This is a particularly good choice for attributes which take strings
+	// containing "microsyntaxes" where various different values are packed
+	// together in some serialization where there are many ways to express the
+	// same information. For example, attributes which accept JSON data can
+	// include different whitespace characters without changing meaning, and
+	// case-insensitive identifiers may refer to the same object using different
+	// characters.
+	//
+	// This is valid only for attributes of primitive types, because
+	// DiffSuppressFunc itself is only compatible with primitive types.
+	//
+	// The key benefit of activating this flag is that the result of Read or
+	// ReadContext will be cleaned of normalization-only changes in the same
+	// way as the planning result would normaly be, which therefore prevents
+	// churn for downstream expressions deriving from this attribute and
+	// prevents incorrect "Values changed outside of Terraform" messages
+	// when the remote API returns values which have the same meaning as the
+	// prior state but in a different serialization.
+	//
+	// This is an opt-in because it was a later addition to the DiffSuppressFunc
+	// functionality which would cause some significant changes in behavior
+	// for existing providers if activated everywhere all at once.
+	DiffSuppressOnRefresh bool
+
 	// If this is non-nil, then this will be a default value that is used
 	// when this item is not set in the configuration.
 	//
@@ -767,6 +802,10 @@ func (m schemaMap) internalValidate(topSchemaMap schemaMap, attrsOnly bool) erro
 			}
 		}
 
+		if v.DiffSuppressOnRefresh && v.DiffSuppressFunc == nil {
+			return fmt.Errorf("%s: cannot set DiffSuppressOnRefresh without DiffSuppressFunc", k)
+		}
+
 		if v.Type == TypeList || v.Type == TypeSet {
 			if v.Elem == nil {
 				return fmt.Errorf("%s: Elem must be set for lists", k)
@@ -991,6 +1030,7 @@ func (m schemaMap) diff(
 					continue
 				}
 
+				log.Printf("[DEBUG] ignoring change of %q due to DiffSuppressFunc", attrK)
 				attrV = &terraform.ResourceAttrDiff{
 					Old: attrV.Old,
 					New: attrV.Old,
@@ -1437,6 +1477,60 @@ func (m schemaMap) diffString(
 	return nil
 }
 
+// handleDiffSuppressOnRefresh visits each of the attributes set in "new" and,
+// if the corresponding schema sets both DiffSuppressFunc and
+// DiffSuppressOnRefresh, checks whether the new value is materially different
+// than the old and if not it overwrites the new value with the old one,
+// in-place.
+func (m schemaMap) handleDiffSuppressOnRefresh(ctx context.Context, oldState, newState *terraform.InstanceState) {
+	if newState == nil || oldState == nil {
+		return // nothing to do, then
+	}
+
+	// We'll populate this in the loop below only if we find at least one
+	// attribute which needs this analysis.
+	var d *ResourceData
+
+	oldAttrs := oldState.Attributes
+	newAttrs := newState.Attributes
+	for k, newV := range newAttrs {
+		oldV, ok := oldAttrs[k]
+		if !ok {
+			continue // no old value to compare with
+		}
+		if newV == oldV {
+			continue // no change to test
+		}
+
+		schemaList := addrToSchema(strings.Split(k, "."), m)
+		if len(schemaList) == 0 {
+			continue // no schema? weird, but not our responsibility to handle
+		}
+		schema := schemaList[len(schemaList)-1]
+		if !schema.DiffSuppressOnRefresh || schema.DiffSuppressFunc == nil {
+			continue // not relevant
+		}
+
+		if d == nil {
+			// We populate "d" only on demand, to avoid the cost for most
+			// existing schemas where DiffSuppressOnRefresh won't be set.
+			var err error
+			d, err = m.Data(newState, nil)
+			if err != nil {
+				// Should not happen if we got far enough to be doing this
+				// analysis, but if it does then we'll bail out.
+				tfsdklog.Warn(ctx, fmt.Sprintf("schemaMap.handleDiffSuppressOnRefresh failed to construct ResourceData: %s", err))
+				return
+			}
+		}
+
+		if schema.DiffSuppressFunc(k, oldV, newV, d) {
+			tfsdklog.Debug(ctx, fmt.Sprintf("ignoring change of %q due to DiffSuppressFunc", k))
+			newState.Attributes[k] = oldV // keep the old value, then
+		}
+	}
+}
+
 func (m schemaMap) validate(
 	k string,
 	schema *Schema,