Fix user var recursion with env vars by SwampDragons · Pull Request #8875 · hashicorp/packer

SwampDragons · 2020-03-11T19:52:49Z

#8812 contains a reasonably complex recursion case that sometimes fails interpolation because we are not checking properly whether interpolation is truly complete. This PR fixes this issue by using a regex to check whether there are any more calls to the {{ user ... }} template function, and using that information to more intelligently continue interpolating variables that need it. It also removes "finished" variables from the interpolation list to make interpolation more efficient.

It also adds a test for this regex, and a test for the interpolation behavior. The added env var interpolation test fails on the master branch and passes on this branch.

Closes #8812

SwampDragons · 2020-03-11T19:53:39Z

cc @azr this is the interpolation issue I talked about with you this morning.

packer/core.go

azr · 2020-03-12T10:09:21Z

packer/core.go

 	// out the appropriate interpolations.

-	allVariables := make(map[string]string)
+	repeatMap := make(map[string]string)


I'm not sure wether this is a good idea here, but since ranging on maps is random may be it could be a good idea to put the key/values in a sorted array. The outcome is that an error case will either always or never be reproducible; which is handy, for debugging and testing and all.

// keyValue could be sorted by first checking // wether it needs to be interpolated then by key. type keyValue struct { Key string Value string } sort.Slice(keyValues, func(i, j int) bool { if isDoneInterpolating(kv[i]) && !isDoneInterpolating(kv[j]) { return true // put the not interpolated vars first. } return kv[i].Key < kv[j].Key })

Then for each pass we can sort this array

I like @azr idea.
You can also do like I did for HCL variables, ranging over a slice of keys and move the none interpolated ones to the end of the slice so they can be interpolated after all other vars. Of course, use the repeatMap to get the key -> value.
here the HCL logic: https://github.com/hashicorp/packer/blob/master/hcl2template/types.packer_config.go#L124

This is a good idea from a reproducibility perspective.

Sorting by whether it's done interpolating seems like overkill. I did a string sort so that it'll be deterministic, but I think that's sufficient.

got it 👍

sylviamoss · 2020-03-12T10:27:19Z

packer/core_test.go

 }

+// Normally I wouldn't test a little helper function, but it's regex.
+func testisDoneInterpolating(t *testing.T) {


Was this supposed to be named as TestIsDoneInterpolating to be considered a test? I test it locally and it fails

I fixed it using this regex {{\s*|user\s*|\x60.*\x60\s*|}}

oh yeah running the test is actually a good idea 🤦‍♀

azr · 2020-03-12T10:37:12Z

Hey, nice one ! I was worried we could have the same issue with HCL2 but :

because we are not checking properly whether interpolation is truly complete

But I think we should be fine with this:

packer/hcl2template/types.packer_config.go

Lines 125 to 152 in 0ccff0d

    
           for len(locals) > 0 { 
        
           	local := locals[0] 
        
           	moreDiags := c.evaluateLocalVariable(local) 
        
           	if moreDiags.HasErrors() { 
        
           		if len(locals) == 1 { 
        
           			// If this is the only local left there's no need 
        
           			// to try evaluating again 
        
           			return append(diags, moreDiags...) 
        
           		} 
        
           		if previousL == len(locals) { 
        
           			if retry == 100 { 
        
           				// To get to this point, locals must have a circle dependency 
        
           				return append(diags, moreDiags...) 
        
           			} 
        
           			retry++ 
        
           		} 
        
           		previousL = len(locals) 
        
           		// If local uses another local that has not been evaluated yet this could be the reason of errors 
        
           		// Push local to the end of slice to be evaluated later 
        
           		locals = append(locals, local) 
        
           	} else { 
        
           		retry = 0 
        
           		diags = append(diags, moreDiags...) 
        
           	} 
        
           	// Remove local from slice 
        
           	locals = append(locals[:0], locals[1:]...) 
        
           }

The interpolation always fails if we have an unknown value here and the HCL2 library is well tested.

SwampDragons · 2020-03-12T15:57:16Z

Thanks for your notes -- I'll incorporate your feedback today.

Co-Authored-By: Adrien Delorme <azr@users.noreply.github.com>

sylviamoss · 2020-03-13T09:40:45Z

packer/core.go

+					return ctx, err
+				}
+				if done {
+					deleteKeys = append(deleteKeys, kv.Key)


if you range over the keys slice you could just delete the key from it here, then the loop for deleting the key-value from the map wouldn't be necessary. If this is possible, this way you won't need sorting the repeatMap anymore because it will be used just like a database for this 🤔
Is there any other reason to range over sortedMap instead?

I tried implementing the slice ranging like you did for HCL but after 3 hours and still having a ton of broken tests I opted for a less major change. I'm always very uncomfortable adding/removing items from the same thing I'm looping over, especially when it has to do with indexing. I think it makes the code harder to conceptualize.

packer/core.go

sylviamoss

This looks really nice! 🎉

…atching Before change ``` 413e19b Merge pull request #8942 from desolatorxxl/google-fix-ssh-keys-metadata b81800d Merge pull request #8935 from zaventh/feature/start-on-boot 9486316 Merge pull request #8922 from hashicorp/f-vsphere_iso-export-ovf-options 56aebbe Merge pull request #8920 from rhencke/patch-1 d068430 make sure locals are evaluated only once variables are + test this (#8918) 3dae5df Merge pull request #8905 from hashicorp/fix_8493 811a730 Merge pull request #8907 from hashicorp/fix_8428 fa49d21 Merge pull request #8906 from hashicorp/fix_8904 23f5603 Merge pull request #8889 from hashicorp/hcl2_singular_blocks dc9259f Merge pull request #8892 from zaventh/feature/vga-adapter fc35f02 Merge pull request #8890 from hashicorp/fix_8880 7972ab7 Merge pull request #8735 from hashicorp/fix_plugin_loading 890d7b2 Merge pull request #8875 from hashicorp/fix_8812 e94ff70 Merge pull request #8883 from hashicorp/fix_8835 9075b80 Merge pull request #8891 from rhencke/patch-1 6477d8a Merge pull request #8882 from hashicorp/fix-var-file-hcl 6008f91 Merge pull request #8847 from takaishi/support-keyboard-interactive 5604561 Merge pull request #8877 from paulcichonski/remote-esxi-bastion 698f744 Merge pull request #8887 from hashicorp/untangle_ssh_docs_from_aws aeedc9a Merge pull request #8879 from mbrancato/specify_keyvault_sku 5365fda Merge pull request #8884 from hashicorp/fix_codecov_config 4bd7b14 Merge pull request #8732 from jhawk28/reorder_cdrom_drive 072a71b Merge pull request #8863 from hashicorp/update_go-cty_regex 8a1caaa Merge pull request #8837 from hashicorp/fix_8730 7873cab Merge pull request #8858 from hashicorp/fix_8791 7e382d0 Merge pull request #8828 from mvitaly/fix_8816 8832b3e Merge pull request #8787 from jhawk28/vsphere_iso_multiple_disks 5281740 Merge pull request #8831 from rjhornsby/master e35a872 Merge pull request #8830 from hashicorp/d-var-file-hcl2-not-yet ``` After change ``` ⇶ git log v1.5.4...v1.5.5 --first-parent --oneline --grep="Merge pull request #[0-9]\+" --grep="(#[0-9]\+)$" 413e19b Merge pull request #8942 from desolatorxxl/google-fix-ssh-keys-metadata c387dc2 builder/vsphere-clone: Find the vm within the folder (#8938) b17b211 Add cleanup_remote_cache config option to vmware-iso (#8917) e6368b9 Fix azure winrm_password attribution and allow to set winrm_username (#8928) fcf10e9 Replace Amazon with Outscale for OSC BSU doc (#8944) 9240fb7 Fix typo in title (#8943) 2c6f096 Allow accepting image for the members in OpenStack builder (#8931) b81800d Merge pull request #8935 from zaventh/feature/start-on-boot daffd9c CONTRIBUTING: Update documentation for linting on Travis (#8933) 3a9d356 golangci-lint: Update --new-from-rev option to check only newly added commits (#8923) 97d797d Fix small typos in osc-bsuvolume.html.md (#8926) 9486316 Merge pull request #8922 from hashicorp/f-vsphere_iso-export-ovf-options 56aebbe Merge pull request #8920 from rhencke/patch-1 99b0b98 Add ovf export capability to vsphere builders (#8764) d068430 make sure locals are evaluated only once variables are + test this (#8918) ad8dafa HCL: add tests and fixes around var-file and var args (#8914) 7979ab0 Add after_n_builds to codecov.yml (#8913) 3dae5df Merge pull request #8905 from hashicorp/fix_8493 811a730 Merge pull request #8907 from hashicorp/fix_8428 fa49d21 Merge pull request #8906 from hashicorp/fix_8904 b94937c Update provisioner_test.go (#8900) 2319521 Add iso config test for checksum from file specific case (#8897) 23f5603 Merge pull request #8889 from hashicorp/hcl2_singular_blocks dc9259f Merge pull request #8892 from zaventh/feature/vga-adapter 690bf71 Add Codecov badge and remove report style (#8896) fc35f02 Merge pull request #8890 from hashicorp/fix_8880 7972ab7 Merge pull request #8735 from hashicorp/fix_plugin_loading 890d7b2 Merge pull request #8875 from hashicorp/fix_8812 e94ff70 Merge pull request #8883 from hashicorp/fix_8835 ```

ghost · 2020-04-13T01:50:26Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

SwampDragons added 3 commits March 11, 2020 09:55

fix interpolation when env vars interpolate before user vars

063dd5c

adding tests

52f03de

add test for fixed behavior

0511552

SwampDragons requested a review from a team as a code owner March 11, 2020 19:52

SwampDragons commented Mar 11, 2020

View reviewed changes

packer/core.go Outdated Show resolved Hide resolved

azr reviewed Mar 12, 2020

View reviewed changes

sylviamoss reviewed Mar 12, 2020

View reviewed changes

sylviamoss added bug core Core components of Packer labels Mar 12, 2020

SwampDragons and others added 3 commits March 12, 2020 09:50

Update packer/core.go

ad5495a

Co-Authored-By: Adrien Delorme <azr@users.noreply.github.com>

fix regex and actually test the check function

17f59c4

sort interpolation input to make the interpolation deterministic

32595f7

sylviamoss reviewed Mar 13, 2020

View reviewed changes

azr reviewed Mar 13, 2020

View reviewed changes

packer/core.go Show resolved Hide resolved

sylviamoss approved these changes Mar 13, 2020

View reviewed changes

SwampDragons merged commit 890d7b2 into master Mar 13, 2020

SwampDragons deleted the fix_8812 branch March 13, 2020 19:52

SwampDragons added this to the 1.5.5 milestone Mar 16, 2020

ghost locked and limited conversation to collaborators Apr 13, 2020

Conversation

SwampDragons commented Mar 11, 2020

Uh oh!

SwampDragons commented Mar 11, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sylviamoss Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sylviamoss Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azr commented Mar 12, 2020

Uh oh!

SwampDragons commented Mar 12, 2020

Uh oh!

sylviamoss Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sylviamoss left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Apr 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sylviamoss Mar 12, 2020 •

edited

Loading

sylviamoss Mar 12, 2020 •

edited

Loading

sylviamoss Mar 13, 2020 •

edited

Loading