From 7a8a44399416446c40342386e71ce35c82a5b8c8 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 19 Dec 2017 15:07:37 -0500 Subject: [PATCH 1/4] add failing test case This case should be expected to fail with the current diff algorithm, but the existing behavior was widely relied upon so we need to roll this back until there is a representable nil value. --- helper/schema/schema_test.go | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 3698fe0f4..72adca4bb 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -3110,6 +3110,36 @@ func TestSchemaMap_Diff(t *testing.T) { Err: true, }, + + // A lot of resources currently depended on using the empty string as a + // nil/unset value. + { + Name: "optional, computed, empty string", + Schema: map[string]*Schema{ + "attr": &Schema{ + Type: TypeString, + Optional: true, + Computed: true, + }, + }, + + State: &terraform.InstanceState{ + Attributes: map[string]string{ + "attr": "bar", + }, + }, + + // this does necessarily depend on an interpolated value, but this + // is often how it comes about in a configuration, otherwise the + // value would be unset. + Config: map[string]interface{}{ + "attr": "${var.foo}", + }, + + ConfigVariables: map[string]ast.Variable{ + "var.foo": interfaceToVariableSwallowError(""), + }, + }, } for i, tc := range cases { From 643ef4334f8ce254cd182dbe514cd99537aef30c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 19 Dec 2017 15:15:39 -0500 Subject: [PATCH 2/4] revert the change that broke the test case This reverts one of the changes from 6a4f7b0, which broke empty strings being seen as unset for computed values. This breaks a number of other tests, and is only an intermediate change for evaluating other solutions. --- helper/schema/schema.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/helper/schema/schema.go b/helper/schema/schema.go index 6cc71df51..3eff0a380 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -337,6 +337,12 @@ func (s *Schema) finalizeDiff( return d } + if s.Computed && d.NewComputed && d.Old != "" && d.New == "" { + // old case where we pretended that a NewComputed value of "" on a + // Computed field was unset + return nil + } + if s.Computed && !d.NewComputed { if d.Old != "" && d.New == "" { // This is a computed value with an old value set already, From cb5ce1d35ea016f7ea38aa0b9fc84fa85ed0bd19 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Tue, 19 Dec 2017 15:46:58 -0800 Subject: [PATCH 3/4] helper/schema: Extend diffChange and bubble up customized values This extends the internal diffChange method so that ResourceDiff's implementation of it can report back whether or not the value came from a customized diff. This is an effort to work to preserve the pre-ResourceDiff behaviour that ignores the diff for computed keys when the old value was populated but the new value wasn't - this behaviour is actually being depended on by users that are using it to exploit using zero values in modules. This should allow both scenarios to co-exist by shifting the NewComputed exemption over to exempting values that come from diff customization. --- helper/schema/resource_data.go | 4 +- helper/schema/resource_diff.go | 15 +++-- helper/schema/schema.go | 106 +++++++++++++++++++-------------- 3 files changed, 69 insertions(+), 56 deletions(-) diff --git a/helper/schema/resource_data.go b/helper/schema/resource_data.go index 970dc7b99..9ab8bccaa 100644 --- a/helper/schema/resource_data.go +++ b/helper/schema/resource_data.go @@ -445,7 +445,7 @@ func (d *ResourceData) init() { } func (d *ResourceData) diffChange( - k string) (interface{}, interface{}, bool, bool) { + k string) (interface{}, interface{}, bool, bool, bool) { // Get the change between the state and the config. o, n := d.getChange(k, getSourceState, getSourceConfig|getSourceExact) if !o.Exists { @@ -456,7 +456,7 @@ func (d *ResourceData) diffChange( } // Return the old, new, and whether there is a change - return o.Value, n.Value, !reflect.DeepEqual(o.Value, n.Value), n.Computed + return o.Value, n.Value, !reflect.DeepEqual(o.Value, n.Value), n.Computed, false } func (d *ResourceData) getChange( diff --git a/helper/schema/resource_diff.go b/helper/schema/resource_diff.go index 4fc1dbb68..822d0dc4d 100644 --- a/helper/schema/resource_diff.go +++ b/helper/schema/resource_diff.go @@ -236,8 +236,8 @@ func (d *ResourceDiff) clear(key string) error { // diffChange helps to implement resourceDiffer and derives its change values // from ResourceDiff's own change data, in addition to existing diff, config, and state. -func (d *ResourceDiff) diffChange(key string) (interface{}, interface{}, bool, bool) { - old, new := d.getChange(key) +func (d *ResourceDiff) diffChange(key string) (interface{}, interface{}, bool, bool, bool) { + old, new, customized := d.getChange(key) if !old.Exists { old.Value = nil @@ -246,7 +246,7 @@ func (d *ResourceDiff) diffChange(key string) (interface{}, interface{}, bool, b new.Value = nil } - return old.Value, new.Value, !reflect.DeepEqual(old.Value, new.Value), new.Computed + return old.Value, new.Value, !reflect.DeepEqual(old.Value, new.Value), new.Computed, customized } // SetNew is used to set a new diff value for the mentioned key. The value must @@ -327,7 +327,7 @@ func (d *ResourceDiff) Get(key string) interface{} { // results from the exact levels for the new diff, then from state and diff as // per normal. func (d *ResourceDiff) GetChange(key string) (interface{}, interface{}) { - old, new := d.getChange(key) + old, new, _ := d.getChange(key) return old.Value, new.Value } @@ -387,18 +387,17 @@ func (d *ResourceDiff) Id() string { // This implementation differs from ResourceData's in the way that we first get // results from the exact levels for the new diff, then from state and diff as // per normal. -func (d *ResourceDiff) getChange(key string) (getResult, getResult) { +func (d *ResourceDiff) getChange(key string) (getResult, getResult, bool) { old := d.get(strings.Split(key, "."), "state") var new getResult for p := range d.updatedKeys { if childAddrOf(key, p) { new = d.getExact(strings.Split(key, "."), "newDiff") - goto done + return old, new, true } } new = d.get(strings.Split(key, "."), "newDiff") -done: - return old, new + return old, new, false } // get performs the appropriate multi-level reader logic for ResourceDiff, diff --git a/helper/schema/schema.go b/helper/schema/schema.go index 3eff0a380..edcf58e8f 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -296,8 +296,7 @@ func (s *Schema) ZeroValue() interface{} { } } -func (s *Schema) finalizeDiff( - d *terraform.ResourceAttrDiff) *terraform.ResourceAttrDiff { +func (s *Schema) finalizeDiff(d *terraform.ResourceAttrDiff, customized bool) *terraform.ResourceAttrDiff { if d == nil { return d } @@ -337,20 +336,16 @@ func (s *Schema) finalizeDiff( return d } - if s.Computed && d.NewComputed && d.Old != "" && d.New == "" { - // old case where we pretended that a NewComputed value of "" on a - // Computed field was unset - return nil - } - - if s.Computed && !d.NewComputed { - if d.Old != "" && d.New == "" { - // This is a computed value with an old value set already, - // just let it go. - return nil + if s.Computed { + if !customized { + if d.Old != "" && d.New == "" { + // This is a computed value with an old value set already, + // just let it go. + return nil + } } - if d.New == "" { + if d.New == "" && !d.NewComputed { // Computed attribute without a new value set d.NewComputed = true } @@ -750,7 +745,7 @@ func isValidFieldName(name string) bool { // This helps facilitate diff logic for both ResourceData and ResoureDiff with // minimal divergence in code. type resourceDiffer interface { - diffChange(string) (interface{}, interface{}, bool, bool) + diffChange(string) (interface{}, interface{}, bool, bool, bool) Get(string) interface{} GetChange(string) (interface{}, interface{}) GetOk(string) (interface{}, bool) @@ -803,7 +798,7 @@ func (m schemaMap) diffList( diff *terraform.InstanceDiff, d resourceDiffer, all bool) error { - o, n, _, computedList := d.diffChange(k) + o, n, _, computedList, customized := d.diffChange(k) if computedList { n = nil } @@ -870,10 +865,13 @@ func (m schemaMap) diffList( oldStr = "" } - diff.Attributes[k+".#"] = countSchema.finalizeDiff(&terraform.ResourceAttrDiff{ - Old: oldStr, - New: newStr, - }) + diff.Attributes[k+".#"] = countSchema.finalizeDiff( + &terraform.ResourceAttrDiff{ + Old: oldStr, + New: newStr, + }, + customized, + ) } // Figure out the maximum @@ -926,7 +924,7 @@ func (m schemaMap) diffMap( // First get all the values from the state var stateMap, configMap map[string]string - o, n, _, nComputed := d.diffChange(k) + o, n, _, nComputed, customized := d.diffChange(k) if err := mapstructure.WeakDecode(o, &stateMap); err != nil { return fmt.Errorf("%s: %s", k, err) } @@ -978,6 +976,7 @@ func (m schemaMap) diffMap( Old: oldStr, New: newStr, }, + customized, ) } @@ -995,16 +994,22 @@ func (m schemaMap) diffMap( continue } - diff.Attributes[prefix+k] = schema.finalizeDiff(&terraform.ResourceAttrDiff{ - Old: old, - New: v, - }) + diff.Attributes[prefix+k] = schema.finalizeDiff( + &terraform.ResourceAttrDiff{ + Old: old, + New: v, + }, + customized, + ) } for k, v := range stateMap { - diff.Attributes[prefix+k] = schema.finalizeDiff(&terraform.ResourceAttrDiff{ - Old: v, - NewRemoved: true, - }) + diff.Attributes[prefix+k] = schema.finalizeDiff( + &terraform.ResourceAttrDiff{ + Old: v, + NewRemoved: true, + }, + customized, + ) } return nil @@ -1017,7 +1022,7 @@ func (m schemaMap) diffSet( d resourceDiffer, all bool) error { - o, n, _, computedSet := d.diffChange(k) + o, n, _, computedSet, customized := d.diffChange(k) if computedSet { n = nil } @@ -1076,20 +1081,26 @@ func (m schemaMap) diffSet( countStr = "" } - diff.Attributes[k+".#"] = countSchema.finalizeDiff(&terraform.ResourceAttrDiff{ - Old: countStr, - NewComputed: true, - }) + diff.Attributes[k+".#"] = countSchema.finalizeDiff( + &terraform.ResourceAttrDiff{ + Old: countStr, + NewComputed: true, + }, + customized, + ) return nil } // If the counts are not the same, then record that diff changed := oldLen != newLen if changed || all { - diff.Attributes[k+".#"] = countSchema.finalizeDiff(&terraform.ResourceAttrDiff{ - Old: oldStr, - New: newStr, - }) + diff.Attributes[k+".#"] = countSchema.finalizeDiff( + &terraform.ResourceAttrDiff{ + Old: oldStr, + New: newStr, + }, + customized, + ) } // Build the list of codes that will make up our set. This is the @@ -1139,7 +1150,7 @@ func (m schemaMap) diffString( all bool) error { var originalN interface{} var os, ns string - o, n, _, computed := d.diffChange(k) + o, n, _, computed, customized := d.diffChange(k) if schema.StateFunc != nil && n != nil { originalN = n n = schema.StateFunc(n) @@ -1176,13 +1187,16 @@ func (m schemaMap) diffString( return nil } - diff.Attributes[k] = schema.finalizeDiff(&terraform.ResourceAttrDiff{ - Old: os, - New: ns, - NewExtra: originalN, - NewRemoved: removed, - NewComputed: computed, - }) + diff.Attributes[k] = schema.finalizeDiff( + &terraform.ResourceAttrDiff{ + Old: os, + New: ns, + NewExtra: originalN, + NewRemoved: removed, + NewComputed: computed, + }, + customized, + ) return nil } From 0df8da59f776b211960d9ce52e57a4fa397fdf0a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 20 Dec 2017 08:46:45 -0500 Subject: [PATCH 4/4] add FIXMEs This new codepath with the getDiff "customzed" return value, along with the associated test need to be removed as soon as we can support unset fields from the config, so we don't continue to carry this broken behavior forward any longer than needed. --- helper/schema/schema.go | 5 +++++ helper/schema/schema_test.go | 2 ++ 2 files changed, 7 insertions(+) diff --git a/helper/schema/schema.go b/helper/schema/schema.go index edcf58e8f..d9a7aa1a0 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -337,6 +337,11 @@ func (s *Schema) finalizeDiff(d *terraform.ResourceAttrDiff, customized bool) *t } if s.Computed { + // FIXME: This is where the customized bool from getChange finally + // comes into play. It allows the previously incorrect behavior + // of an empty string being used as "unset" when the value is + // computed. This should be removed once we can properly + // represent an unset/nil value from the configuration. if !customized { if d.Old != "" && d.New == "" { // This is a computed value with an old value set already, diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 72adca4bb..bda54ccee 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -3113,6 +3113,8 @@ func TestSchemaMap_Diff(t *testing.T) { // A lot of resources currently depended on using the empty string as a // nil/unset value. + // FIXME: We want this to eventually produce a diff, since there + // technically is a new value in the config. { Name: "optional, computed, empty string", Schema: map[string]*Schema{