From 063dd5c133ed2d665670aadaaf3640beb380a58f Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Wed, 11 Mar 2020 09:55:40 -0700 Subject: [PATCH 1/6] fix interpolation when env vars interpolate before user vars --- packer/core.go | 60 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 48 insertions(+), 12 deletions(-) diff --git a/packer/core.go b/packer/core.go index a3f33a816..16780d086 100644 --- a/packer/core.go +++ b/packer/core.go @@ -2,6 +2,7 @@ package packer import ( "fmt" + "regexp" "sort" "strings" @@ -346,13 +347,24 @@ func (c *Core) validate() error { return err } -func (c *Core) init() error { - if c.variables == nil { - c.variables = make(map[string]string) +func isDoneInterpolating(v string) (bool, error) { + // Check for whether the var contains any more references to `user`, wrapped + // in interpolation syntax. + filter := "{{\\s*|user\\s*|\\x60?.*\\x60?}}" + 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 - // user variables + if matched { + // 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 +398,15 @@ 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) // load in template variables for k, v := range c.Template.Variables { - allVariables[k] = v.Default + repeatMap[k] = v.Default } // overwrite template variables with command-line-read variables for k, v := range c.variables { - allVariables[k] = v + repeatMap[k] = v } // Regex to exclude any build function variable or template variable @@ -405,7 +417,7 @@ func (c *Core) init() error { for i := 0; i < 100; i++ { shouldRetry = false // First, loop over the variables in the template - for k, v := range allVariables { + for k, v := range repeatMap { // Interpolate the default renderedV, err := interpolate.RenderRegex(v, ctx, renderFilter) switch err.(type) { @@ -415,16 +427,27 @@ func (c *Core) init() error { changed = true c.variables[k] = 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(v) + if err != nil { + return ctx, err + } + if done { + delete(repeatMap, k) + } 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) } 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) @@ -436,12 +459,25 @@ func (c *Core) init() error { } 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) From 52f03de0f0d68d0ed6f0c5b7f17921d6082ed2b4 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Wed, 11 Mar 2020 10:49:45 -0700 Subject: [PATCH 2/6] adding tests --- packer/core_test.go | 94 ++++++++++++++++++++++++++++++++------------- 1 file changed, 67 insertions(+), 27 deletions(-) diff --git a/packer/core_test.go b/packer/core_test.go index 90f59de1d..70d7f9072 100644 --- a/packer/core_test.go +++ b/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", - map[string]string{"foo": "bar"}, - false, - }, + {"validate-req-variable.json", 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-high.json", - map[string]string{"foo": "bar"}, - true, - }, + {"validate-min-version.json", map[string]string{"foo": "bar"}, false}, + {"validate-min-version-high.json", map[string]string{"foo": "bar"}, true}, } for _, tc := range cases { @@ -700,6 +678,68 @@ 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 testCoreBuildWithInterpolatedEnvVars(t *testing.T) { +// // Not a fan of using env vars in tests but in this case we need to make +// // sure they get interpolated properly. +// os.Setenv("CI_COMMIT_REF_NAME", "working") +// os.Setenv("OUTPUT_DIR", "/u01/artifacts") +// os.Setenv("OS_VERSION", "6") +// os.Setenv("OS", "rhel") +// os.Setenv("BASE_POSTFIX", "base") +// os.Setenv("RULE", "vbox") +// os.Setenv("POSTFIX", "base") + +// packer build \ +// -var-file=common/vsphere.variables.json \ +// -var-file=common/variables.json \ +// -only=vbox-rhel-base \ +// common/linux/redhat/template.json + +// } + func testCoreTemplate(t *testing.T, c *CoreConfig, p string) { tpl, err := template.ParseFile(p) if err != nil { From 051155270f34a551f97b896f33fa1fad23f1dd2b Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Wed, 11 Mar 2020 12:48:35 -0700 Subject: [PATCH 3/6] add test for fixed behavior --- packer/core_test.go | 71 ++++++++++++++----- .../complex-recursed-env-user-var-file.json | 11 +++ 2 files changed, 66 insertions(+), 16 deletions(-) create mode 100644 packer/test-fixtures/complex-recursed-env-user-var-file.json diff --git a/packer/core_test.go b/packer/core_test.go index 70d7f9072..e2bfb86d7 100644 --- a/packer/core_test.go +++ b/packer/core_test.go @@ -721,24 +721,63 @@ func testisDoneInterpolating(t *testing.T) { } } -// func testCoreBuildWithInterpolatedEnvVars(t *testing.T) { -// // Not a fan of using env vars in tests but in this case we need to make -// // sure they get interpolated properly. -// os.Setenv("CI_COMMIT_REF_NAME", "working") -// os.Setenv("OUTPUT_DIR", "/u01/artifacts") -// os.Setenv("OS_VERSION", "6") -// os.Setenv("OS", "rhel") -// os.Setenv("BASE_POSTFIX", "base") -// os.Setenv("RULE", "vbox") -// os.Setenv("POSTFIX", "base") +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") -// packer build \ -// -var-file=common/vsphere.variables.json \ -// -var-file=common/variables.json \ -// -only=vbox-rhel-base \ -// common/linux/redhat/template.json + // run the interpolations a bunch of times, because we have had issues + // where the behavior is different based on the order in which the + // vars are interpolated + for i := 0; i < 100; i++ { + 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) diff --git a/packer/test-fixtures/complex-recursed-env-user-var-file.json b/packer/test-fixtures/complex-recursed-env-user-var-file.json new file mode 100644 index 000000000..bffb4b1ca --- /dev/null +++ b/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" + }] +} \ No newline at end of file From ad5495ac8f7f625b91e7d7d0cef5c0ae12c20507 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Thu, 12 Mar 2020 09:50:02 -0700 Subject: [PATCH 4/6] Update packer/core.go Co-Authored-By: Adrien Delorme --- packer/core.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packer/core.go b/packer/core.go index 16780d086..f52becab3 100644 --- a/packer/core.go +++ b/packer/core.go @@ -350,7 +350,7 @@ func (c *Core) validate() error { func isDoneInterpolating(v string) (bool, error) { // Check for whether the var contains any more references to `user`, wrapped // in interpolation syntax. - filter := "{{\\s*|user\\s*|\\x60?.*\\x60?}}" + filter := `{{\s*|user\s*|\x60?.*\x60?}}` matched, err := regexp.MatchString(filter, v) if err != nil { return false, fmt.Errorf("Can't tell if interpolation is done: %s", err) From 17f59c4ddce4ecc8331667997652e4f0bd55dcd4 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Thu, 12 Mar 2020 10:00:56 -0700 Subject: [PATCH 5/6] fix regex and actually test the check function --- packer/core.go | 2 +- packer/core_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packer/core.go b/packer/core.go index f52becab3..b8682d159 100644 --- a/packer/core.go +++ b/packer/core.go @@ -350,7 +350,7 @@ func (c *Core) validate() error { func isDoneInterpolating(v string) (bool, error) { // Check for whether the var contains any more references to `user`, wrapped // in interpolation syntax. - filter := `{{\s*|user\s*|\x60?.*\x60?}}` + 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) diff --git a/packer/core_test.go b/packer/core_test.go index e2bfb86d7..8982afbb2 100644 --- a/packer/core_test.go +++ b/packer/core_test.go @@ -679,7 +679,7 @@ func TestSensitiveVars(t *testing.T) { } // Normally I wouldn't test a little helper function, but it's regex. -func testisDoneInterpolating(t *testing.T) { +func TestIsDoneInterpolating(t *testing.T) { cases := []struct { inputString string expectedBool bool From 32595f71cfb2d989521c44d6a335bc70b32719de Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Thu, 12 Mar 2020 13:40:56 -0700 Subject: [PATCH 6/6] sort interpolation input to make the interpolation deterministic --- packer/core.go | 46 ++++++++++++++++++++++---- packer/core_test.go | 79 ++++++++++++++++++++++----------------------- 2 files changed, 78 insertions(+), 47 deletions(-) diff --git a/packer/core.go b/packer/core.go index b8682d159..5d7337e61 100644 --- a/packer/core.go +++ b/packer/core.go @@ -2,6 +2,7 @@ package packer import ( "fmt" + "log" "regexp" "sort" "strings" @@ -399,14 +400,29 @@ func (c *Core) renderVarsRecursively() (*interpolate.Context, error) { // out the appropriate interpolations. repeatMap := make(map[string]string) + allKeys := make([]string, 0) + // load in template variables for k, v := range c.Template.Variables { repeatMap[k] = v.Default + allKeys = append(allKeys, k) } // overwrite template variables with command-line-read variables for k, v := range c.variables { 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 @@ -416,25 +432,27 @@ func (c *Core) renderVarsRecursively() (*interpolate.Context, 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 repeatMap { + 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(v) + done, err := isDoneInterpolating(kv.Value) if err != nil { return ctx, err } if done { - delete(repeatMap, k) + deleteKeys = append(deleteKeys, kv.Key) } else { shouldRetry = true } @@ -442,7 +460,7 @@ func (c *Core) renderVarsRecursively() (*interpolate.Context, error) { 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 ctx, err } @@ -450,12 +468,26 @@ func (c *Core) renderVarsRecursively() (*interpolate.Context, error) { 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 { diff --git a/packer/core_test.go b/packer/core_test.go index 8982afbb2..17d76eb0c 100644 --- a/packer/core_test.go +++ b/packer/core_test.go @@ -540,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 { @@ -727,49 +732,43 @@ func TestEnvAndFileVars(t *testing.T) { os.Setenv("INTERPOLATE_TEST_ENV_2", "5") os.Setenv("INTERPOLATE_TEST_ENV_4", "bananas") - // run the interpolations a bunch of times, because we have had issues - // where the behavior is different based on the order in which the - // vars are interpolated - for i := 0; i < 100; i++ { - f, err := os.Open(fixtureDir("complex-recursed-env-user-var-file.json")) - if err != nil { - t.Fatalf("err: %s", err) - } + 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) - } + 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{ + ccf, err := NewCore(&CoreConfig{ + Template: tpl, + Version: "1.0.0", + Variables: 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", + "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) } - 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