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{