Merge pull request #8875 from hashicorp/fix_8812

Fix user var recursion with env vars

packer/core.go

@@ -2,6 +2,8 @@ package packer
 
 import (
 	"fmt"
+	"log"
+	"regexp"
 	"sort"
 	"strings"
 
@@ -346,13 +348,24 @@ func (c *Core) validate() error {
 	return err
 }
 
-func (c *Core) init() error {
+func isDoneInterpolating(v string) (bool, error) {
-	if c.variables == nil {
+	// Check for whether the var contains any more references to `user`, wrapped
-		c.variables = make(map[string]string)
+	// in interpolation syntax.
+	filter := `{{\s*user\s*\x60.*\x60\s*}}`
+	matched, err := regexp.MatchString(filter, v)
+	if err != nil {
+		return false, fmt.Errorf("Can't tell if interpolation is done: %s", err)
 	}
-	// Go through the variables and interpolate the environment and
+	if matched {
-	// user variables
+		// not done interpolating; there's still a call to "user" in a template
+		// engine
+		return false, nil
+	}
+	// No more calls to "user" as a template engine, so we're done.
+	return true, nil
+}
 
+func (c *Core) renderVarsRecursively() (*interpolate.Context, error) {
 	ctx := c.Context()
 	ctx.EnableEnv = true
 	ctx.UserVariables = make(map[string]string)
@@ -386,15 +399,30 @@ func (c *Core) init() error {
 	// We need to read the keys from both, then loop over all of them to figure
 	// out the appropriate interpolations.
 
-	allVariables := make(map[string]string)
+	repeatMap := make(map[string]string)
+	allKeys := make([]string, 0)
+
 	// load in template variables
 	for k, v := range c.Template.Variables {
-		allVariables[k] = v.Default
+		repeatMap[k] = v.Default
+		allKeys = append(allKeys, k)
 	}
 
 	// overwrite template variables with command-line-read variables
 	for k, v := range c.variables {
-		allVariables[k] = v
+		repeatMap[k] = v
+		allKeys = append(allKeys, k)
+	}
+
+	// sort map to force the following loop to be deterministic.
+	sort.Strings(allKeys)
+	type keyValue struct {
+		Key   string
+		Value string
+	}
+	sortedMap := make([]keyValue, len(repeatMap))
+	for _, k := range allKeys {
+		sortedMap = append(sortedMap, keyValue{k, repeatMap[k]})
 	}
 
 	// Regex to exclude any build function variable or template variable
@@ -404,44 +432,84 @@ func (c *Core) init() error {
 
 	for i := 0; i < 100; i++ {
 		shouldRetry = false
+		changed = false
+		deleteKeys := []string{}
 		// First, loop over the variables in the template
-		for k, v := range allVariables {
+		for _, kv := range sortedMap {
 			// Interpolate the default
-			renderedV, err := interpolate.RenderRegex(v, ctx, renderFilter)
+			renderedV, err := interpolate.RenderRegex(kv.Value, ctx, renderFilter)
 			switch err.(type) {
 			case nil:
 				// We only get here if interpolation has succeeded, so something is
 				// different in this loop than in the last one.
 				changed = true
-				c.variables[k] = renderedV
+				c.variables[kv.Key] = renderedV
 				ctx.UserVariables = c.variables
+				// Remove fully-interpolated variables from the map, and flag
+				// variables that still need interpolating for a repeat.
+				done, err := isDoneInterpolating(kv.Value)
+				if err != nil {
+					return ctx, err
+				}
+				if done {
+					deleteKeys = append(deleteKeys, kv.Key)
+				} else {
+					shouldRetry = true
+				}
 			case ttmp.ExecError:
 				castError := err.(ttmp.ExecError)
 				if strings.Contains(castError.Error(), interpolate.ErrVariableNotSetString) {
 					shouldRetry = true
-					failedInterpolation = fmt.Sprintf(`"%s": "%s"; error: %s`, k, v, err)
+					failedInterpolation = fmt.Sprintf(`"%s": "%s"; error: %s`, kv.Key, kv.Value, err)
 				} else {
-					return err
+					return ctx, err
 				}
 			default:
-				return fmt.Errorf(
+				return ctx, fmt.Errorf(
 					// unexpected interpolation error: abort the run
 					"error interpolating default value for '%s': %s",
-					k, err)
+					kv.Key, err)
 			}
 		}
 		if !shouldRetry {
 			break
 		}
+
+		// Clear completed vars from sortedMap before next loop. Do this one
+		// key at a time because the indices are gonna change ever time you
+		// delete from the map.
+		for _, k := range deleteKeys {
+			for ind, kv := range sortedMap {
+				if kv.Key == k {
+					log.Printf("Deleting kv.Value: %s", kv.Value)
+					sortedMap = append(sortedMap[:ind], sortedMap[ind+1:]...)
+					break
+				}
+			}
+		}
+		deleteKeys = []string{}
 	}
 
 	if !changed && shouldRetry {
-		return fmt.Errorf("Failed to interpolate %s: Please make sure that "+
+		return ctx, fmt.Errorf("Failed to interpolate %s: Please make sure that "+
 			"the variable you're referencing has been defined; Packer treats "+
 			"all variables used to interpolate other user varaibles as "+
 			"required.", failedInterpolation)
 	}
 
+	return ctx, nil
+}
+
+func (c *Core) init() error {
+	if c.variables == nil {
+		c.variables = make(map[string]string)
+	}
+	// Go through the variables and interpolate the environment and
+	// user variables
+	ctx, err := c.renderVarsRecursively()
+	if err != nil {
+		return err
+	}
 	for _, v := range c.Template.SensitiveVariables {
 		secret := ctx.UserVariables[v.Key]
 		c.secrets = append(c.secrets, secret)

packer/core_test.go

@@ -463,37 +463,15 @@ func TestCoreValidate(t *testing.T) {
 		Vars map[string]string
 		Err  bool
 	}{
-		{
+		{"validate-dup-builder.json", nil, true},
-			"validate-dup-builder.json",
-			nil,
-			true,
-		},
 
 		// Required variable not set
-		{
+		{"validate-req-variable.json", nil, true},
-			"validate-req-variable.json",
+		{"validate-req-variable.json", map[string]string{"foo": "bar"}, false},
-			nil,
-			true,
-		},
-
-		{
-			"validate-req-variable.json",
-			map[string]string{"foo": "bar"},
-			false,
-		},
 
 		// Min version good
-		{
+		{"validate-min-version.json", map[string]string{"foo": "bar"}, false},
-			"validate-min-version.json",
+		{"validate-min-version-high.json", map[string]string{"foo": "bar"}, true},
-			map[string]string{"foo": "bar"},
-			false,
-		},
-
-		{
-			"validate-min-version-high.json",
-			map[string]string{"foo": "bar"},
-			true,
-		},
 	}
 
 	for _, tc := range cases {
@@ -562,7 +540,12 @@ func TestCore_InterpolateUserVars(t *testing.T) {
 		})
 
 		if (err != nil) != tc.Err {
-			t.Fatalf("err: %s\n\n%s", tc.File, err)
+			if tc.Err == false {
+				t.Fatalf("Error interpolating %s: Expected no error, but got: %s", tc.File, err)
+			} else {
+				t.Fatalf("Error interpolating %s: Expected an error, but got: %s", tc.File, err)
+			}
+
 		}
 		if !tc.Err {
 			for k, v := range ccf.variables {
@@ -700,6 +683,101 @@ func TestSensitiveVars(t *testing.T) {
 	}
 }
 
+// Normally I wouldn't test a little helper function, but it's regex.
+func TestIsDoneInterpolating(t *testing.T) {
+	cases := []struct {
+		inputString  string
+		expectedBool bool
+		expectedErr  bool
+	}{
+		// Many of these tests are just exercising the regex to make sure it
+		// doesnt get confused by different kinds of whitespace
+		{"charmander-{{ user `spacesaroundticks` }}", false, false},
+		{"pidgey-{{ user `partyparrot`}}", false, false},
+		{"jigglypuff-{{ user`notickspaaces`}}", false, false},
+		{"eevee-{{user`nospaces`}}", false, false},
+		{"staryu-{{  user  `somanyspaces`  }}", false, false},
+		{"{{  user  `somanyspaces`  }}-{{isotime}}", false, false},
+		// Make sure that we only flag on "user" when it's in the right set of
+		// brackets, in a properly declared template engine format
+		{"missingno-{{ user `missingbracket` }", true, false},
+		{"missing2-{user ``missingopenbrackets }}", true, false},
+		{"wat-userjustinname", true, false},
+		// Any functions that aren't "user" should have already been properly
+		// interpolated by the time this is called, so these cases aren't
+		// realistic. That said, this makes it clear that this function doesn't
+		// care about anything but the user function
+		{"pokemon-{{ isotime }}", true, false},
+		{"squirtle-{{ env `water`}}", true, false},
+		{"bulbasaur-notinterpolated", true, false},
+		{"extra-{{thisfunc `user`}}", true, false},
+	}
+	for _, tc := range cases {
+		done, err := isDoneInterpolating(tc.inputString)
+		if (err != nil) != tc.expectedErr {
+			t.Fatalf("Test case failed. Error: %s expected error: "+
+				"%t test string: %s", err, tc.expectedErr, tc.inputString)
+		}
+		if done != tc.expectedBool {
+			t.Fatalf("Test case failed. inputString: %s. "+
+				"Expected done = %t but got done = %t", tc.inputString,
+				tc.expectedBool, done)
+		}
+	}
+}
+
+func TestEnvAndFileVars(t *testing.T) {
+	os.Setenv("INTERPOLATE_TEST_ENV_1", "bulbasaur")
+	os.Setenv("INTERPOLATE_TEST_ENV_3", "/path/to/nowhere")
+	os.Setenv("INTERPOLATE_TEST_ENV_2", "5")
+	os.Setenv("INTERPOLATE_TEST_ENV_4", "bananas")
+
+	f, err := os.Open(fixtureDir("complex-recursed-env-user-var-file.json"))
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	tpl, err := template.Parse(f)
+	f.Close()
+	if err != nil {
+		t.Fatalf("err: %s\n\n%s", "complex-recursed-env-user-var-file.json", err)
+	}
+
+	ccf, err := NewCore(&CoreConfig{
+		Template: tpl,
+		Version:  "1.0.0",
+		Variables: map[string]string{
+			"var_1":     "partyparrot",
+			"var_2":     "{{user `env_1`}}-{{user `env_2`}}{{user `env_3`}}-{{user `var_1`}}",
+			"final_var": "{{user `env_1`}}/{{user `env_2`}}/{{user `env_4`}}{{user `env_3`}}-{{user `var_1`}}/vmware/{{user `var_2`}}.vmx",
+		},
+	})
+	expected := map[string]string{
+		"var_1":     "partyparrot",
+		"var_2":     "bulbasaur-5/path/to/nowhere-partyparrot",
+		"final_var": "bulbasaur/5/bananas/path/to/nowhere-partyparrot/vmware/bulbasaur-5/path/to/nowhere-partyparrot.vmx",
+		"env_1":     "bulbasaur",
+		"env_2":     "5",
+		"env_3":     "/path/to/nowhere",
+		"env_4":     "bananas",
+	}
+	if err != nil {
+		t.Fatalf("err: %s\n\n%s", "complex-recursed-env-user-var-file.json", err)
+	}
+	for k, v := range ccf.variables {
+		if expected[k] != v {
+			t.Fatalf("Expected value %s for key %s but got %s",
+				expected[k], k, v)
+		}
+	}
+
+	// Clean up env vars
+	os.Unsetenv("INTERPOLATE_TEST_ENV_1")
+	os.Unsetenv("INTERPOLATE_TEST_ENV_3")
+	os.Unsetenv("INTERPOLATE_TEST_ENV_2")
+	os.Unsetenv("INTERPOLATE_TEST_ENV_4")
+}
+
 func testCoreTemplate(t *testing.T, c *CoreConfig, p string) {
 	tpl, err := template.ParseFile(p)
 	if err != nil {

packer/test-fixtures/complex-recursed-env-user-var-file.json

@@ -0,0 +1,11 @@
+{
+  "variables": {
+    "env_1": "{{ env `INTERPOLATE_TEST_ENV_1` }}",
+    "env_2": "{{ env `INTERPOLATE_TEST_ENV_2` }}",
+    "env_3": "{{ env `INTERPOLATE_TEST_ENV_3` }}",
+    "env_4": "{{ env `INTERPOLATE_TEST_ENV_4` }}"
+  },
+    "builders": [{
+        "type": "test"
+    }]
+}