From 3fa11e456b211066abc1aa31feaedba59857b1b1 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Sun, 5 Nov 2017 11:14:51 -0800 Subject: [PATCH] helper/schema: Clear existing map/set/list contents before overwriting There are situations where one may need to write to a set, list, or map more than once per single TF operation (apply/refresh/etc). In these cases, further writes using Set (example: d.Set("some_set", newSet)) currently create unstable results in the set writer (the name of the writer layer that holds the data set by these calls) because old keys are not being cleared out first. This bug is most visible when using sets. Example: First write to set writes elements that have been hashed at 10 and 20, and the second write writes elements that have been hashed at 30 and 40. While the set length has been correctly set at 2, since a set is basically a map (as is the entire map writer) and map results are non-deterministic, reads to this set will now deliver unstable results in a random but predictable fashion as the map results are delivered to the caller non-deterministic - sometimes you may correctly get 30 and 40, but sometimes you may get 10 and 20, or even 10 and 30, etc. This problem propagates to state which is even more damaging as unstable results are set to state where they become part of the permanent data set going forward. The problem also applies to lists and maps. This is probably more of an issue with maps as a map can contain any key/value combination and hence there is no predictable pattern where keys would be overwritten with default or zero values. This is contrary to complex lists, which has this problem as well, but since lists are deterministic and the length of a list properly gets updated during the overwrite, the problem is masked by the fact that a read will only read to the boundary of the list, skipping any bad data that may still be available due past the list boundary. This update clears the child contents of any set, list, or map before beginning a new write to address this issue. Tests are included for all three data types. --- helper/schema/field_writer_map.go | 32 +++ helper/schema/field_writer_map_test.go | 275 +++++++++++++++++++++++++ 2 files changed, 307 insertions(+) diff --git a/helper/schema/field_writer_map.go b/helper/schema/field_writer_map.go index 689ed8d1c..814c7ba8e 100644 --- a/helper/schema/field_writer_map.go +++ b/helper/schema/field_writer_map.go @@ -39,6 +39,19 @@ func (w *MapFieldWriter) unsafeWriteField(addr string, value string) { w.result[addr] = value } +// clearTree clears a field and any sub-fields of the given address out of the +// map. This should be used to reset some kind of complex structures (namely +// sets) before writing to make sure that any conflicting data is removed (for +// example, if the set was previously written to the writer's layer). +func (w *MapFieldWriter) clearTree(addr []string) { + prefix := strings.Join(addr, ".") + "." + for k := range w.result { + if strings.HasPrefix(k, prefix) { + delete(w.result, k) + } + } +} + func (w *MapFieldWriter) WriteField(addr []string, value interface{}) error { w.lock.Lock() defer w.lock.Unlock() @@ -115,6 +128,14 @@ func (w *MapFieldWriter) setList( return fmt.Errorf("%s: %s", k, err) } + // Wipe the set from the current writer prior to writing if it exists. + // Multiple writes to the same layer is a lot safer for lists than sets due + // to the fact that indexes are always deterministic and the length will + // always be updated with the current length on the last write, but making + // sure we have a clean namespace removes any chance for edge cases to pop up + // and ensures that the last write to the set is the correct value. + w.clearTree(addr) + // Set the entire list. var err error for i, elem := range vs { @@ -162,6 +183,10 @@ func (w *MapFieldWriter) setMap( vs[mk.String()] = mv.Interface() } + // Wipe this address tree. The contents of the map should always reflect the + // last write made to it. + w.clearTree(addr) + // Remove the pure key since we're setting the full map value delete(w.result, k) @@ -308,6 +333,13 @@ func (w *MapFieldWriter) setSet( value = s } + // Clear any keys that match the set address first. This is necessary because + // it's always possible and sometimes may be necessary to write to a certain + // writer layer more than once with different set data each time, which will + // lead to different keys being inserted, which can lead to determinism + // problems when the old data isn't wiped first. + w.clearTree(addr) + for code, elem := range value.(*Set).m { if err := w.set(append(addrCopy, code), elem); err != nil { return err diff --git a/helper/schema/field_writer_map_test.go b/helper/schema/field_writer_map_test.go index 5fc308aa7..42ed9c208 100644 --- a/helper/schema/field_writer_map_test.go +++ b/helper/schema/field_writer_map_test.go @@ -261,3 +261,278 @@ func TestMapFieldWriter(t *testing.T) { } } } + +func TestMapFieldWriterCleanSet(t *testing.T) { + schema := map[string]*Schema{ + "setDeep": &Schema{ + Type: TypeSet, + Elem: &Resource{ + Schema: map[string]*Schema{ + "index": &Schema{Type: TypeInt}, + "value": &Schema{Type: TypeString}, + }, + }, + Set: func(a interface{}) int { + return a.(map[string]interface{})["index"].(int) + }, + }, + } + + values := []struct { + Addr []string + Value interface{} + Out map[string]string + }{ + { + []string{"setDeep"}, + []interface{}{ + map[string]interface{}{ + "index": 10, + "value": "foo", + }, + map[string]interface{}{ + "index": 50, + "value": "bar", + }, + }, + map[string]string{ + "setDeep.#": "2", + "setDeep.10.index": "10", + "setDeep.10.value": "foo", + "setDeep.50.index": "50", + "setDeep.50.value": "bar", + }, + }, + { + []string{"setDeep"}, + []interface{}{ + map[string]interface{}{ + "index": 20, + "value": "baz", + }, + map[string]interface{}{ + "index": 60, + "value": "qux", + }, + }, + map[string]string{ + "setDeep.#": "2", + "setDeep.20.index": "20", + "setDeep.20.value": "baz", + "setDeep.60.index": "60", + "setDeep.60.value": "qux", + }, + }, + { + []string{"setDeep"}, + []interface{}{ + map[string]interface{}{ + "index": 30, + "value": "one", + }, + map[string]interface{}{ + "index": 70, + "value": "two", + }, + }, + map[string]string{ + "setDeep.#": "2", + "setDeep.30.index": "30", + "setDeep.30.value": "one", + "setDeep.70.index": "70", + "setDeep.70.value": "two", + }, + }, + } + + w := &MapFieldWriter{Schema: schema} + + for n, tc := range values { + err := w.WriteField(tc.Addr, tc.Value) + if err != nil { + t.Fatalf("%d: err: %s", n, err) + } + + actual := w.Map() + if !reflect.DeepEqual(actual, tc.Out) { + t.Fatalf("%d: bad: %#v", n, actual) + } + } +} + +func TestMapFieldWriterCleanList(t *testing.T) { + schema := map[string]*Schema{ + "listDeep": &Schema{ + Type: TypeList, + Elem: &Resource{ + Schema: map[string]*Schema{ + "thing1": &Schema{Type: TypeString}, + "thing2": &Schema{Type: TypeString}, + }, + }, + }, + } + + values := []struct { + Addr []string + Value interface{} + Out map[string]string + }{ + { + // Base list + []string{"listDeep"}, + []interface{}{ + map[string]interface{}{ + "thing1": "a", + "thing2": "b", + }, + map[string]interface{}{ + "thing1": "c", + "thing2": "d", + }, + map[string]interface{}{ + "thing1": "e", + "thing2": "f", + }, + map[string]interface{}{ + "thing1": "g", + "thing2": "h", + }, + }, + map[string]string{ + "listDeep.#": "4", + "listDeep.0.thing1": "a", + "listDeep.0.thing2": "b", + "listDeep.1.thing1": "c", + "listDeep.1.thing2": "d", + "listDeep.2.thing1": "e", + "listDeep.2.thing2": "f", + "listDeep.3.thing1": "g", + "listDeep.3.thing2": "h", + }, + }, + { + // Remove an element + []string{"listDeep"}, + []interface{}{ + map[string]interface{}{ + "thing1": "a", + "thing2": "b", + }, + map[string]interface{}{ + "thing1": "c", + "thing2": "d", + }, + map[string]interface{}{ + "thing1": "e", + "thing2": "f", + }, + }, + map[string]string{ + "listDeep.#": "3", + "listDeep.0.thing1": "a", + "listDeep.0.thing2": "b", + "listDeep.1.thing1": "c", + "listDeep.1.thing2": "d", + "listDeep.2.thing1": "e", + "listDeep.2.thing2": "f", + }, + }, + { + // Rewrite with missing keys. This should normally not be necessary, as + // hopefully the writers are writing zero values as necessary, but for + // brevity we want to make sure that what exists in the writer is exactly + // what the last write looked like coming from the provider. + []string{"listDeep"}, + []interface{}{ + map[string]interface{}{ + "thing1": "a", + }, + map[string]interface{}{ + "thing1": "c", + }, + map[string]interface{}{ + "thing1": "e", + }, + }, + map[string]string{ + "listDeep.#": "3", + "listDeep.0.thing1": "a", + "listDeep.1.thing1": "c", + "listDeep.2.thing1": "e", + }, + }, + } + + w := &MapFieldWriter{Schema: schema} + + for n, tc := range values { + err := w.WriteField(tc.Addr, tc.Value) + if err != nil { + t.Fatalf("%d: err: %s", n, err) + } + + actual := w.Map() + if !reflect.DeepEqual(actual, tc.Out) { + t.Fatalf("%d: bad: %#v", n, actual) + } + } +} + +func TestMapFieldWriterCleanMap(t *testing.T) { + schema := map[string]*Schema{ + "map": &Schema{ + Type: TypeMap, + }, + } + + values := []struct { + Value interface{} + Out map[string]string + }{ + { + // Base map + map[string]interface{}{ + "thing1": "a", + "thing2": "b", + "thing3": "c", + "thing4": "d", + }, + map[string]string{ + "map.%": "4", + "map.thing1": "a", + "map.thing2": "b", + "map.thing3": "c", + "map.thing4": "d", + }, + }, + { + // Base map + map[string]interface{}{ + "thing1": "a", + "thing2": "b", + "thing4": "d", + }, + map[string]string{ + "map.%": "3", + "map.thing1": "a", + "map.thing2": "b", + "map.thing4": "d", + }, + }, + } + + w := &MapFieldWriter{Schema: schema} + + for n, tc := range values { + err := w.WriteField([]string{"map"}, tc.Value) + if err != nil { + t.Fatalf("%d: err: %s", n, err) + } + + actual := w.Map() + if !reflect.DeepEqual(actual, tc.Out) { + t.Fatalf("%d: bad: %#v", n, actual) + } + } +}