From d7048cb640c53294df36283e297316751e113fcd Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Sat, 7 Apr 2018 13:06:47 -0700 Subject: [PATCH] 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. --- helper/schema/resource_diff.go | 38 +++++++- helper/schema/resource_diff_test.go | 134 ++++++++++++++++++++++++++++ helper/schema/schema_test.go | 42 +++++++++ 3 files changed, 210 insertions(+), 4 deletions(-) diff --git a/helper/schema/resource_diff.go b/helper/schema/resource_diff.go index 773c24cf7..92b891fc5 100644 --- a/helper/schema/resource_diff.go +++ b/helper/schema/resource_diff.go @@ -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 { diff --git a/helper/schema/resource_diff_test.go b/helper/schema/resource_diff_test.go index 4345fac1e..fdb8937e5 100644 --- a/helper/schema/resource_diff_test.go +++ b/helper/schema/resource_diff_test.go @@ -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) { diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 4b0d5afb8..820352aea 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -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{