From 497010ce42a2827e9aed42fa56b258369e730f3e Mon Sep 17 00:00:00 2001 From: Andrew Garrett Date: Fri, 23 Dec 2016 22:59:18 +0000 Subject: [PATCH 1/4] Fix string representation of sets during interpolation The change in #10787 used flatmap.Expand to fix interpolation of nested maps, but it broke interpolation of sets such that their elements were not represented. For example, the expected string representation of a splatted aws_network_interface.whatever.*.private_ips should be: ``` [{Variable (TypeList): [{Variable (TypeString): 10.41.17.25}]} {Variable (TypeList): [{Variable (TypeString): 10.41.22.236}]}] ``` But instead it became: ``` [{Variable (TypeList): [{Variable (TypeString): }]} {Variable (TypeList): [{Variable (TypeString): }]}] ``` This is because the expandArray function of expand.go treated arrays to exclusively be lists, e.g. not sets. The old code used to match for numeric keys, so it would work for sets, whereas expandArray just assumed keys started at 0 and ascended incrementally. Remember that sets' keys are numeric, but since they are hashes, they can be any integer. The result of assuming that the keys start at 0 led to the recursive call to flatmap.Expand not matching any keys of the set, and returning nil, which is why the above example has nothing where the IP addresses used to be. So we bring back that matching behavior, but we move it to expandArray instead. We've modified it to not reconstruct the data structures like it used to when it was in the Interpolator, and to use the standard int sorter rather than implementing a custom sorter since a custom one is no longer necessary thanks to the use of flatmap.Expand. Fixes #10908, and restores the viability of the workaround I posted in #8696. Big thanks to @jszwedko for helping me with this fix. I was able to diagnose the problem along, but couldn't fix it without his help. --- flatmap/expand.go | 24 ++++++++++++++++++++-- terraform/interpolate_test.go | 38 +++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/flatmap/expand.go b/flatmap/expand.go index b2072a62f..0e3ebe003 100644 --- a/flatmap/expand.go +++ b/flatmap/expand.go @@ -2,6 +2,8 @@ package flatmap import ( "fmt" + "regexp" + "sort" "strconv" "strings" ) @@ -42,9 +44,27 @@ func expandArray(m map[string]string, prefix string) []interface{} { panic(err) } + keySet := make(map[int]struct{}) + listElementKey := regexp.MustCompile("^" + prefix + "\\.([0-9]+)(?:\\..*)?$") + for key := range m { + if matches := listElementKey.FindStringSubmatch(key); matches != nil { + k, err := strconv.ParseInt(matches[1], 0, 0) + if err != nil { + panic(err) + } + keySet[int(k)] = struct{}{} + } + } + + keysList := make([]int, 0, num) + for key := range keySet { + keysList = append(keysList, key) + } + sort.Ints(keysList) + result := make([]interface{}, num) - for i := 0; i < int(num); i++ { - result[i] = Expand(m, fmt.Sprintf("%s.%d", prefix, i)) + for i, key := range keysList { + result[i] = Expand(m, fmt.Sprintf("%s.%d", prefix, key)) } return result diff --git a/terraform/interpolate_test.go b/terraform/interpolate_test.go index 9c6cdc40e..54f762fe7 100644 --- a/terraform/interpolate_test.go +++ b/terraform/interpolate_test.go @@ -838,6 +838,44 @@ func TestInterpolator_nestedMapsAndLists(t *testing.T) { interfaceToVariableSwallowError(mapOfList)) } +func TestInterpolator_sets(t *testing.T) { + state := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Resources: map[string]*ResourceState{ + "aws_network_interface.set": &ResourceState{ + Type: "aws_network_interface", + Dependencies: []string{}, + Primary: &InstanceState{ + ID: "null", + Attributes: map[string]string{ + "private_ips.#": "1", + "private_ips.3977356764": "10.42.16.179", + }, + }, + }, + }, + }, + }, + } + + i := &Interpolater{ + Module: testModule(t, "interpolate-multi-vars"), + StateLock: new(sync.RWMutex), + State: state, + } + + scope := &InterpolationScope{ + Path: rootModulePath, + } + + set := []interface{}{"10.42.16.179"} + + testInterpolate(t, i, scope, "aws_network_interface.set.private_ips", + interfaceToVariableSwallowError(set)) +} + func testInterpolate( t *testing.T, i *Interpolater, scope *InterpolationScope, From 85d8fba3bdca20c06ebe95c06690c0170a6c8114 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 4 Jan 2017 16:03:24 -0500 Subject: [PATCH 2/4] Minor fixups to expandArray Find the index keys by comparing the strings directly, so we don't need to worry about the prefix value altering the regex. --- flatmap/expand.go | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/flatmap/expand.go b/flatmap/expand.go index 0e3ebe003..dfaf74e7b 100644 --- a/flatmap/expand.go +++ b/flatmap/expand.go @@ -2,7 +2,6 @@ package flatmap import ( "fmt" - "regexp" "sort" "strconv" "strings" @@ -44,16 +43,28 @@ func expandArray(m map[string]string, prefix string) []interface{} { panic(err) } - keySet := make(map[int]struct{}) - listElementKey := regexp.MustCompile("^" + prefix + "\\.([0-9]+)(?:\\..*)?$") - for key := range m { - if matches := listElementKey.FindStringSubmatch(key); matches != nil { - k, err := strconv.ParseInt(matches[1], 0, 0) - if err != nil { - panic(err) - } - keySet[int(k)] = struct{}{} + keySet := map[int]bool{} + for k := range m { + if !strings.HasPrefix(k, prefix+".") { + continue } + + key := k[len(prefix)+1:] + idx := strings.Index(key, ".") + if idx != -1 { + key = key[:idx] + } + + // skip the count value + if key == "#" { + continue + } + + k, err := strconv.Atoi(key) + if err != nil { + panic(err) + } + keySet[int(k)] = true } keysList := make([]int, 0, num) From 1cec04b8a715c9601cdc0ddf8291f441f49aa024 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 4 Jan 2017 16:11:46 -0500 Subject: [PATCH 3/4] Add test for set expansion in flatmap.Expand --- flatmap/expand_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/flatmap/expand_test.go b/flatmap/expand_test.go index b480e2ca9..b5da3197b 100644 --- a/flatmap/expand_test.go +++ b/flatmap/expand_test.go @@ -106,6 +106,17 @@ func TestExpand(t *testing.T) { "list2": []interface{}{"c"}, }, }, + + { + Map: map[string]string{ + "set.#": "3", + "set.1234": "a", + "set.1235": "b", + "set.1236": "c", + }, + Key: "set", + Output: []interface{}{"a", "b", "c"}, + }, } for _, tc := range cases { From ba60fd12ae90e14f0df71e815ae1e1ea3908c56f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 9 Jan 2017 09:43:45 -0500 Subject: [PATCH 4/4] Add another comment for reference --- flatmap/expand.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/flatmap/expand.go b/flatmap/expand.go index dfaf74e7b..331357ff3 100644 --- a/flatmap/expand.go +++ b/flatmap/expand.go @@ -43,6 +43,10 @@ func expandArray(m map[string]string, prefix string) []interface{} { panic(err) } + // The Schema "Set" type stores its values in an array format, but using + // numeric hash values instead of ordinal keys. Take the set of keys + // regardless of value, and expand them in numeric order. + // See GH-11042 for more details. keySet := map[int]bool{} for k := range m { if !strings.HasPrefix(k, prefix+".") {