From 59ca59bb37cdaedfa7efa8a3638530923449e204 Mon Sep 17 00:00:00 2001
From: Dan Lorenc <lorenc.d@gmail.com>
Date: Tue, 28 Jul 2020 08:35:18 -0500
Subject: [PATCH] Make string variable interpolation deterministic, and
 single-pass.

Right now we replace string variables one at a time, in an order governed by
iteration through a map[string]string. This has two implications:
- We may expand variables twice. If one variable expands to another variable, that one may be expanded. And so on.
- Whether or not this secondary expansion occurs (and even how it occurs!) can vary from run to run.

This commit changes to replace everything at once, using the strings.Replacer package.
It also adds a test to ensure replacements are stable and non-recursiive.
---
 pkg/substitution/substitution.go      |  8 ++++++--
 pkg/substitution/substitution_test.go | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/pkg/substitution/substitution.go b/pkg/substitution/substitution.go
index 30d027074a1..1676bd2f075 100644
--- a/pkg/substitution/substitution.go
+++ b/pkg/substitution/substitution.go
@@ -118,10 +118,14 @@ func matchGroups(matches []string, pattern *regexp.Regexp) map[string]string {
 }
 
 func ApplyReplacements(in string, replacements map[string]string) string {
+	replacementsList := []string{}
 	for k, v := range replacements {
-		in = strings.Replace(in, fmt.Sprintf("$(%s)", k), v, -1)
+		replacementsList = append(replacementsList, fmt.Sprintf("$(%s)", k), v)
 	}
-	return in
+	// strings.Replacer does all replacements in one pass, preventing multiple replacements
+	// See #2093 for an explanation on why we need to do this.
+	replacer := strings.NewReplacer(replacementsList...)
+	return replacer.Replace(in)
 }
 
 // Take an input string, and output an array of strings related to possible arrayReplacements. If there aren't any
diff --git a/pkg/substitution/substitution_test.go b/pkg/substitution/substitution_test.go
index c218fda0612..9e5f11d53b5 100644
--- a/pkg/substitution/substitution_test.go
+++ b/pkg/substitution/substitution_test.go
@@ -167,6 +167,24 @@ func TestApplyReplacements(t *testing.T) {
 	}
 }
 
+func TestNestedReplacements(t *testing.T) {
+	replacements := map[string]string{
+		// Foo should turn into barbar, which could then expand into bazbaz depending on how this is expanded
+		"foo": "$(bar)$(bar)",
+		"bar": "baz",
+	}
+	input := "$(foo) is cool"
+	expected := "$(bar)$(bar) is cool"
+
+	// Run this test a lot of times to ensure the behavior is deterministic
+	for i := 0; i <= 1000; i++ {
+		got := substitution.ApplyReplacements(input, replacements)
+		if d := cmp.Diff(expected, got); d != "" {
+			t.Errorf("ApplyReplacements() output did not match expected value %s", diff.PrintWantGot(d))
+		}
+	}
+}
+
 func TestApplyArrayReplacements(t *testing.T) {
 	type args struct {
 		input              string