helper/schema: ResourceDiff ForceNew attribute correctness

A couple of bugs have been discovered in ResourceDiff.ForceNew:

* NewRemoved is not preserved when a diff for a key is already present.
This is because the second diff that happens after customization
performs a second getChange on not just state and config, but also on
the pre-existing diff. This results in Exists == true, meaning nil is
never returned as a new value.
* ForceNew was doing the work of adding the key to the list of changed
keys by doing a full SetNew on the existing value. This has a side
effect of fetching zero values from what were otherwise undefined values
and creating diffs for these values where there should not have been
(example: "" => "0").

This update fixes these scenarios by:

* Adding a new private function to check the existing diff for
NewRemoved keys. This is included in the check on new values in
diffChange.
* Keys that have been flagged as ForceNew (or parent keys of lists and
sets that have been flagged as ForceNew) are now maintained in a
separate map. UpdatedKeys now returns the results of both of these maps,
but otherwise these keys are ignored by ResourceDiff.
* Pursuant the above, values are no longer pushed into the newDiff
writer by ForceNew. This prevents the zero value problem, and makes for
a cleaner implementation where the provider has to "manually" SetNew to
update the appropriate values in the writer. It also prevents
non-computed keys from winding up in the diff, which ResourceDiff
normally blocks by design.

There are also a couple of tests for cases that should never come up
right now involving Optional/Computed values and NewRemoved, for which
explanations are given in annotations of each test. These are here to
guard against future regressions.
This commit is contained in:
Chris Marchesi 2018-04-07 13:06:47 -07:00
parent d9b315564e
commit d7048cb640
No known key found for this signature in database
GPG Key ID: 8D6F1589D9834498
3 changed files with 210 additions and 4 deletions

View File

@ -135,6 +135,10 @@ type ResourceDiff struct {
// diff does not get re-run on keys that were not touched, or diffs that were
// just removed (re-running on the latter would just roll back the removal).
updatedKeys map[string]bool
// Tracks which keys were flagged as forceNew. These keys are not saved in
// newWriter, but we need to track them so that they can be re-diffed later.
forcedNewKeys map[string]bool
}
// newResourceDiff creates a new ResourceDiff instance.
@ -193,17 +197,30 @@ func newResourceDiff(schema map[string]*Schema, config *terraform.ResourceConfig
}
d.updatedKeys = make(map[string]bool)
d.forcedNewKeys = make(map[string]bool)
return d
}
// UpdatedKeys returns the keys that were updated by this ResourceDiff run.
// These are the only keys that a diff should be re-calculated for.
//
// This is the combined result of both keys for which diff values were updated
// for or cleared, and also keys that were flagged to be re-diffed as a result
// of ForceNew.
func (d *ResourceDiff) UpdatedKeys() []string {
var s []string
for k := range d.updatedKeys {
s = append(s, k)
}
for k := range d.forcedNewKeys {
for _, l := range s {
if k == l {
break
}
}
s = append(s, k)
}
return s
}
@ -257,7 +274,7 @@ func (d *ResourceDiff) diffChange(key string) (interface{}, interface{}, bool, b
if !old.Exists {
old.Value = nil
}
if !new.Exists {
if !new.Exists || d.removed(key) {
new.Value = nil
}
@ -335,9 +352,12 @@ func (d *ResourceDiff) ForceNew(key string) error {
schema.ForceNew = true
// We need to set whole lists/sets/maps here
_, new := d.GetChange(keyParts[0])
return d.setDiff(keyParts[0], new, false)
// Flag this for a re-diff. Don't save any values to guarantee that existing
// diffs aren't messed with, as this gets messy when dealing with complex
// structures, zero values, etc.
d.forcedNewKeys[keyParts[0]] = true
return nil
}
// Get hands off to ResourceData.Get.
@ -449,6 +469,16 @@ func (d *ResourceDiff) getChange(key string) (getResult, getResult, bool) {
return old, new, false
}
// removed checks to see if the key is present in the existing, pre-customized
// diff and if it was marked as NewRemoved.
func (d *ResourceDiff) removed(k string) bool {
diff, ok := d.diff.Attributes[k]
if !ok {
return false
}
return diff.NewRemoved
}
// get performs the appropriate multi-level reader logic for ResourceDiff,
// starting at source. Refer to newResourceDiff for the level order.
func (d *ResourceDiff) get(addr []string, source string) getResult {

View File

@ -553,6 +553,52 @@ func testDiffCases(t *testing.T, oldPrefix string, oldOffset int, computed bool)
NewValue: "qux",
ExpectedError: true,
},
resourceDiffTestCase{
// NOTE: This case is technically impossible in the current
// implementation, because optional+computed values never show up in the
// diff, and we actually clear existing diffs when SetNew or
// SetNewComputed is run. This test is here to ensure that if either of
// these behaviors change that we don't introduce regressions.
Name: "NewRemoved in diff for Optional and Computed, should be fully overridden",
Schema: map[string]*Schema{
"foo": &Schema{
Type: TypeString,
Optional: true,
Computed: true,
},
},
State: &terraform.InstanceState{
Attributes: map[string]string{
"foo": "bar",
},
},
Config: testConfig(t, map[string]interface{}{}),
Diff: &terraform.InstanceDiff{
Attributes: map[string]*terraform.ResourceAttrDiff{
"foo": &terraform.ResourceAttrDiff{
Old: "bar",
New: "",
NewRemoved: true,
},
},
},
Key: "foo",
NewValue: "qux",
Expected: &terraform.InstanceDiff{
Attributes: map[string]*terraform.ResourceAttrDiff{
"foo": &terraform.ResourceAttrDiff{
Old: "bar",
New: func() string {
if computed {
return ""
}
return "qux"
}(),
NewComputed: computed,
},
},
},
},
}
}
@ -765,6 +811,94 @@ func TestForceNew(t *testing.T) {
},
},
},
resourceDiffTestCase{
Name: "preserve NewRemoved on existing diff",
Schema: map[string]*Schema{
"foo": &Schema{
Type: TypeString,
Optional: true,
},
},
State: &terraform.InstanceState{
Attributes: map[string]string{
"foo": "bar",
},
},
Config: testConfig(t, map[string]interface{}{}),
Diff: &terraform.InstanceDiff{
Attributes: map[string]*terraform.ResourceAttrDiff{
"foo": &terraform.ResourceAttrDiff{
Old: "bar",
New: "",
NewRemoved: true,
},
},
},
Key: "foo",
Expected: &terraform.InstanceDiff{
Attributes: map[string]*terraform.ResourceAttrDiff{
"foo": &terraform.ResourceAttrDiff{
Old: "bar",
New: "",
RequiresNew: true,
NewRemoved: true,
},
},
},
},
resourceDiffTestCase{
Name: "nested field, preserve original diff without zero values",
Schema: map[string]*Schema{
"foo": &Schema{
Type: TypeList,
Required: true,
MaxItems: 1,
Elem: &Resource{
Schema: map[string]*Schema{
"bar": {
Type: TypeString,
Optional: true,
},
"baz": {
Type: TypeInt,
Optional: true,
},
},
},
},
},
State: &terraform.InstanceState{
Attributes: map[string]string{
"foo.#": "1",
"foo.0.bar": "abc",
},
},
Config: testConfig(t, map[string]interface{}{
"foo": []map[string]interface{}{
map[string]interface{}{
"bar": "abcdefg",
},
},
}),
Diff: &terraform.InstanceDiff{
Attributes: map[string]*terraform.ResourceAttrDiff{
"foo.0.bar": &terraform.ResourceAttrDiff{
Old: "abc",
New: "abcdefg",
},
},
},
Key: "foo.0.bar",
Expected: &terraform.InstanceDiff{
Attributes: map[string]*terraform.ResourceAttrDiff{
"foo.0.bar": &terraform.ResourceAttrDiff{
Old: "abc",
New: "abcdefg",
RequiresNew: true,
},
},
},
},
}
for _, tc := range cases {
t.Run(tc.Name, func(t *testing.T) {

View File

@ -2866,6 +2866,48 @@ func TestSchemaMap_Diff(t *testing.T) {
},
{
// NOTE: This case is technically impossible in the current
// implementation, because optional+computed values never show up in the
// diff. In the event behavior changes this test should ensure that the
// intended diff still shows up.
Name: "overridden removed attribute diff with a CustomizeDiff function, ForceNew not in schema",
Schema: map[string]*Schema{
"availability_zone": &Schema{
Type: TypeString,
Optional: true,
Computed: true,
},
},
State: nil,
Config: map[string]interface{}{},
CustomizeDiff: func(d *ResourceDiff, meta interface{}) error {
if err := d.SetNew("availability_zone", "bar"); err != nil {
return err
}
if err := d.ForceNew("availability_zone"); err != nil {
return err
}
return nil
},
Diff: &terraform.InstanceDiff{
Attributes: map[string]*terraform.ResourceAttrDiff{
"availability_zone": &terraform.ResourceAttrDiff{
Old: "",
New: "bar",
RequiresNew: true,
},
},
},
Err: false,
},
{
Name: "overridden diff with a CustomizeDiff function, ForceNew in schema",
Schema: map[string]*Schema{
"availability_zone": &Schema{