From b3f80b9469d30a43698378fe2bbf323d7a5628bb Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Mon, 14 Nov 2016 18:33:29 -0800
Subject: [PATCH] config: maintain slice index accounting for computed keys

Fixes #10075
Fixes #10013

When interpolating, we were only maintaining the last known slice index.
If you had sibling slices then you could lose your slice index when
exiting the slice. The resulting behavior was that no some runs the
computed key would be: "slice.0.attr" and on others would be
"slice.attr", the latter being incorrect.

We now maintain a list of slice indexes so that as we unnest, we
properly restore the old value.

Surprisingly unrelated to the graph but the shadow graph caught this
which is great. :)
---
 config/interpolate_walk.go | 13 +++++-------
 config/raw_config_test.go  | 43 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/config/interpolate_walk.go b/config/interpolate_walk.go
index 828045ec3..81fa81208 100644
--- a/config/interpolate_walk.go
+++ b/config/interpolate_walk.go
@@ -32,7 +32,7 @@ type interpolationWalker struct {
 	cs          []reflect.Value
 	csKey       []reflect.Value
 	csData      interface{}
-	sliceIndex  int
+	sliceIndex  []int
 	unknownKeys []string
 }
 
@@ -54,9 +54,6 @@ type interpolationWalkerContextFunc func(reflectwalk.Location, ast.Node)
 
 func (w *interpolationWalker) Enter(loc reflectwalk.Location) error {
 	w.loc = loc
-	if loc == reflectwalk.WalkLoc {
-		w.sliceIndex = -1
-	}
 	return nil
 }
 
@@ -75,7 +72,7 @@ func (w *interpolationWalker) Exit(loc reflectwalk.Location) error {
 		w.cs = w.cs[:len(w.cs)-1]
 	case reflectwalk.SliceElem:
 		w.csKey = w.csKey[:len(w.csKey)-1]
-		w.sliceIndex = -1
+		w.sliceIndex = w.sliceIndex[:len(w.sliceIndex)-1]
 	}
 
 	return nil
@@ -90,8 +87,8 @@ func (w *interpolationWalker) MapElem(m, k, v reflect.Value) error {
 	w.csData = k
 	w.csKey = append(w.csKey, k)
 
-	if w.sliceIndex != -1 {
-		w.key = append(w.key, fmt.Sprintf("%d.%s", w.sliceIndex, k.String()))
+	if l := len(w.sliceIndex); l > 0 {
+		w.key = append(w.key, fmt.Sprintf("%d.%s", w.sliceIndex[l-1], k.String()))
 	} else {
 		w.key = append(w.key, k.String())
 	}
@@ -107,7 +104,7 @@ func (w *interpolationWalker) Slice(s reflect.Value) error {
 
 func (w *interpolationWalker) SliceElem(i int, elem reflect.Value) error {
 	w.csKey = append(w.csKey, reflect.ValueOf(i))
-	w.sliceIndex = i
+	w.sliceIndex = append(w.sliceIndex, i)
 	return nil
 }
 
diff --git a/config/raw_config_test.go b/config/raw_config_test.go
index 70ae968e6..4def6a77a 100644
--- a/config/raw_config_test.go
+++ b/config/raw_config_test.go
@@ -339,6 +339,49 @@ func TestRawConfig_unknownPartialList(t *testing.T) {
 	}
 }
 
+// This tests a race found where we were not maintaining the "slice index"
+// accounting properly. The result would be that some computed keys would
+// look like they had no slice index when they in fact do. This test is not
+// very reliable but it did fail before the fix and passed after.
+func TestRawConfig_sliceIndexLoss(t *testing.T) {
+	raw := map[string]interface{}{
+		"slice": []map[string]interface{}{
+			map[string]interface{}{
+				"foo": []interface{}{"foo/${var.unknown}"},
+				"bar": []interface{}{"bar"},
+			},
+		},
+	}
+
+	vars := map[string]ast.Variable{
+		"var.unknown": ast.Variable{
+			Value: UnknownVariableValue,
+			Type:  ast.TypeUnknown,
+		},
+		"var.known": ast.Variable{
+			Value: "123456",
+			Type:  ast.TypeString,
+		},
+	}
+
+	// We run it a lot because its fast and we try to get a race out
+	for i := 0; i < 50; i++ {
+		rc, err := NewRawConfig(raw)
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		if err := rc.Interpolate(vars); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		expectedKeys := []string{"slice.0.foo"}
+		if !reflect.DeepEqual(rc.UnknownKeys(), expectedKeys) {
+			t.Fatalf("bad: %#v", rc.UnknownKeys())
+		}
+	}
+}
+
 func TestRawConfigValue(t *testing.T) {
 	raw := map[string]interface{}{
 		"foo": "${var.bar}",