Merge pull request #13055 from hashicorp/backport/fix_locals_eval_ord…

…er/hugely-welcome-rodent

This pull request was automerged via backport-assistant

hcl2template/parser.go

@@ -190,6 +190,8 @@ func (p *Parser) Parse(filename string, varFiles []string, argVars map[string]st
 			diags = append(diags, morediags...)
 			cfg.LocalBlocks = append(cfg.LocalBlocks, moreLocals...)
 		}
+
+		diags = diags.Extend(cfg.checkForDuplicateLocalDefinition())
 	}
 
 	// parse var files
@@ -296,7 +298,6 @@ func filterVarsFromLogs(inputOrLocal Variables) {
 func (cfg *PackerConfig) Initialize(opts packer.InitializeOptions) hcl.Diagnostics {
 	diags := cfg.InputVariables.ValidateValues()
 	diags = append(diags, cfg.evaluateDatasources(opts.SkipDatasourcesExecution)...)
-	diags = append(diags, checkForDuplicateLocalDefinition(cfg.LocalBlocks)...)
 	diags = append(diags, cfg.evaluateLocalVariables(cfg.LocalBlocks)...)
 
 	filterVarsFromLogs(cfg.InputVariables)

hcl2template/types.packer_config.go

@@ -217,14 +217,16 @@ func parseLocalVariableBlocks(f *hcl.File) ([]*LocalBlock, hcl.Diagnostics) {
 	return locals, diags
 }
 
-func (c *PackerConfig) evaluateAllLocalVariables(locals []*LocalBlock) hcl.Diagnostics {
-	var diags hcl.Diagnostics
+func (c *PackerConfig) localByName(local string) (*LocalBlock, error) {
+	for _, loc := range c.LocalBlocks {
+		if loc.Name != local {
+			continue
+		}
 
-	for _, local := range locals {
-		diags = append(diags, c.evaluateLocalVariable(local)...)
+		return loc, nil
 	}
 
-	return diags
+	return nil, fmt.Errorf("local %s not found", local)
 }
 
 func (c *PackerConfig) evaluateLocalVariables(locals []*LocalBlock) hcl.Diagnostics {
@@ -238,53 +240,99 @@ func (c *PackerConfig) evaluateLocalVariables(locals []*LocalBlock) hcl.Diagnost
 		c.LocalVariables = Variables{}
 	}
 
-	for foundSomething := true; foundSomething; {
-		foundSomething = false
-		for i := 0; i < len(locals); {
-			local := locals[i]
-			moreDiags := c.evaluateLocalVariable(local)
-			if moreDiags.HasErrors() {
-				i++
+	for _, local := range c.LocalBlocks {
+		// Note: when looking at the expressions, we only need to care about
+		// attributes, as HCL2 expressions are not allowed in a block's labels.
+		vars := FilterTraversalsByType(local.Expr.Variables(), "local")
+
+		var localDeps []*LocalBlock
+		for _, v := range vars {
+			// Some local variables may be locally aliased as
+			// `local`, which
+			if len(v) < 2 {
 				continue
 			}
-			foundSomething = true
-			locals = append(locals[:i], locals[i+1:]...)
+			varName := v[1].(hcl.TraverseAttr).Name
+			block, err := c.localByName(varName)
+			if err != nil {
+				diags = diags.Append(&hcl.Diagnostic{
+					Severity: hcl.DiagError,
+					Summary:  "Missing variable dependency",
+					Detail: fmt.Sprintf("The expression for variable %q depends on local.%s, which is not defined.",
+						local.Name, varName),
+				})
+				continue
+			}
+			localDeps = append(localDeps, block)
 		}
+		local.dependencies = localDeps
 	}
 
-	if len(locals) != 0 {
-		// get errors from remaining variables
-		return c.evaluateAllLocalVariables(locals)
+	// Immediately return in case the dependencies couldn't be figured out.
+	if diags.HasErrors() {
+		return diags
+	}
+
+	for _, local := range c.LocalBlocks {
+		diags = diags.Extend(c.evaluateLocalVariable(local, 0))
 	}
 
 	return diags
 }
 
-func checkForDuplicateLocalDefinition(locals []*LocalBlock) hcl.Diagnostics {
+// checkForDuplicateLocalDefinition walks through the list of defined variables
+// in order to detect duplicate locals definitions.
+func (c *PackerConfig) checkForDuplicateLocalDefinition() hcl.Diagnostics {
 	var diags hcl.Diagnostics
 
-	// we could sort by name and then check contiguous names to use less memory,
-	// but using a map sounds good enough.
-	names := map[string]struct{}{}
-	for _, local := range locals {
-		if _, found := names[local.Name]; found {
-			diags = append(diags, &hcl.Diagnostic{
+	localNames := map[string]*LocalBlock{}
+
+	for _, block := range c.LocalBlocks {
+		loc, ok := localNames[block.Name]
+		if ok {
+			diags = diags.Append(&hcl.Diagnostic{
 				Severity: hcl.DiagError,
 				Summary:  "Duplicate local definition",
-				Detail:   "Duplicate " + local.Name + " definition found.",
-				Subject:  local.Expr.Range().Ptr(),
+				Detail: fmt.Sprintf("Local variable %q is defined twice in your templates. Other definition found at %q",
+					block.Name, loc.Expr.Range()),
+				Subject: block.Expr.Range().Ptr(),
 			})
 			continue
 		}
-		names[local.Name] = struct{}{}
+
+		localNames[block.Name] = block
 	}
+
 	return diags
 }
 
-func (c *PackerConfig) evaluateLocalVariable(local *LocalBlock) hcl.Diagnostics {
+func (c *PackerConfig) evaluateLocalVariable(local *LocalBlock, depth int) hcl.Diagnostics {
+	// If the variable already was evaluated, we can return immediately
+	if local.evaluated {
+		return nil
+	}
+
+	if depth >= 10 {
+		return hcl.Diagnostics{&hcl.Diagnostic{
+			Severity: hcl.DiagError,
+			Summary:  "Max local recursion depth exceeded.",
+			Detail: "An error occured while recursively evaluating locals." +
+				"Your local variables likely have a cyclic dependency. " +
+				"Please simplify your config to continue. ",
+		}}
+	}
+
 	var diags hcl.Diagnostics
 
+	for _, dep := range local.dependencies {
+		localDiags := c.evaluateLocalVariable(dep, depth+1)
+		diags = diags.Extend(localDiags)
+	}
+
 	value, moreDiags := local.Expr.Value(c.EvalContext(LocalContext, nil))
+
+	local.evaluated = true
+
 	diags = append(diags, moreDiags...)
 	if moreDiags.HasErrors() {
 		return diags

hcl2template/types.variables.go

@@ -30,6 +30,17 @@ type LocalBlock struct {
 	// When Sensitive is set to true Packer will try its best to hide/obfuscate
 	// the variable from the output stream. By replacing the text.
 	Sensitive bool
+
+	// dependsOn lists the dependencies for being able to evaluate this local
+	//
+	// Only `local`/`locals` will be referenced here as we execute all the
+	// same component types at once.
+	dependencies []*LocalBlock
+	// evaluated toggles to true if it has been evaluated.
+	//
+	// We use this to determine if we're ready to get the value of the
+	// expression.
+	evaluated bool
 }
 
 // VariableAssignment represents a way a variable was set: the expression

hcl2template/utils.go

@@ -205,6 +205,13 @@ func GetVarsByType(block *hcl.Block, topLevelLabels ...string) []hcl.Traversal {
 		}
 	}
 
+	return FilterTraversalsByType(travs, topLevelLabels...)
+}
+
+// FilterTraversalsByType lets the caller filter the traversals per top-level type.
+//
+// This can then be used to detect dependencies between block types.
+func FilterTraversalsByType(travs []hcl.Traversal, topLevelLabels ...string) []hcl.Traversal {
 	var rets []hcl.Traversal
 	for _, t := range travs {
 		varRootname := t.RootName()

packer_test/commands_test.go

@@ -5,15 +5,15 @@ import (
 	"os"
 	"os/exec"
 	"strings"
-	"sync"
 	"testing"
 )
 
 type packerCommand struct {
-	once       sync.Once
+	runs       int
 	packerPath string
 	args       []string
 	env        map[string]string
+	stdin      string
 	stderr     *strings.Builder
 	stdout     *strings.Builder
 	workdir    string
@@ -23,11 +23,9 @@ type packerCommand struct {
 
 // PackerCommand creates a skeleton of packer command with the ability to execute gadgets on the outputs of the command.
 func (ts *PackerTestSuite) PackerCommand() *packerCommand {
-	stderr := &strings.Builder{}
-	stdout := &strings.Builder{}
-
 	return &packerCommand{
 		packerPath: ts.packerPath,
+		runs:       1,
 		env: map[string]string{
 			"PACKER_LOG": "1",
 			// Required for Windows, otherwise since we overwrite all
@@ -40,10 +38,12 @@ func (ts *PackerTestSuite) PackerCommand() *packerCommand {
 			// case of Panic will fail to be created (unless tests
 			// are running as Administrator, but please don't).
 			"TMP": os.TempDir(),
+			// Since those commands are used to run tests, we want to
+			// make them as self-contained and quick as possible.
+			// Removing telemetry here is probably for the best.
+			"CHECKPOINT_DISABLE": "1",
 		},
-		stderr: stderr,
-		stdout: stdout,
-		t:      ts.T(),
+		t: ts.T(),
 	}
 }
 
@@ -77,42 +77,97 @@ func (pc *packerCommand) AddEnv(key, val string) *packerCommand {
 	return pc
 }
 
+// Runs changes the number of times the command is run.
+//
+// This is useful for testing non-deterministic bugs, which we can reasonably
+// execute multiple times and expose a dysfunctional run.
+//
+// This is not necessarily a guarantee that the code is sound, but so long as
+// we run the test enough times, we can be decently confident the problem has
+// been solved.
+func (pc *packerCommand) Runs(runs int) *packerCommand {
+	if runs <= 0 {
+		panic(fmt.Sprintf("cannot set command runs to %d", runs))
+	}
+
+	pc.runs = runs
+	return pc
+}
+
+// Stdin changes the contents of the stdin for the command.
+//
+// Each run will be populated with a copy of this string, and wait for the
+// command to terminate.
+//
+// Note: this could lead to a deadlock if the command doesn't support stdin
+// closing after it's finished feeding the inputs.
+func (pc *packerCommand) Stdin(in string) *packerCommand {
+	pc.stdin = in
+	return pc
+}
+
 // Run executes the packer command with the args/env requested and returns the
 // output streams (stdout, stderr)
 //
-// Note: "Run" will only execute the command once, and return the streams and
-// error from the only execution for every subsequent call
+// Note: while originally "Run" was designed to be idempotent, with the
+// introduction of multiple runs for a command, this is not the case anymore
+// and the function should not be considered thread-safe anymore.
 func (pc *packerCommand) Run() (string, string, error) {
-	pc.once.Do(pc.doRun)
+	if pc.runs <= 0 {
+		return pc.stdout.String(), pc.stderr.String(), pc.err
+	}
+	pc.runs--
 
-	return pc.stdout.String(), pc.stderr.String(), pc.err
-}
+	pc.stdout = &strings.Builder{}
+	pc.stderr = &strings.Builder{}
 
-func (pc *packerCommand) doRun() {
 	cmd := exec.Command(pc.packerPath, pc.args...)
 	for key, val := range pc.env {
 		cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", key, val))
 	}
 	cmd.Stdout = pc.stdout
 	cmd.Stderr = pc.stderr
 
+	if pc.stdin != "" {
+		cmd.Stdin = strings.NewReader(pc.stdin)
+	}
+
 	if pc.workdir != "" {
 		cmd.Dir = pc.workdir
 	}
 
 	pc.err = cmd.Run()
+
+	// Check that the command didn't panic, and if it did, we can immediately error
+	panicErr := PanicCheck{}.Check(pc.stdout.String(), pc.stderr.String(), pc.err)
+	if panicErr != nil {
+		pc.t.Fatalf("Packer panicked during execution: %s", panicErr)
+	}
+
+	return pc.stdout.String(), pc.stderr.String(), pc.err
 }
 
 func (pc *packerCommand) Assert(checks ...Checker) {
-	stdout, stderr, err := pc.Run()
+	attempt := 0
+	for pc.runs > 0 {
+		attempt++
+		stdout, stderr, err := pc.Run()
+
+		for _, check := range checks {
+			checkErr := check.Check(stdout, stderr, err)
+			if checkErr != nil {
+				checkerName := InferName(check)
+				pc.t.Errorf("check %q failed: %s", checkerName, checkErr)
+			}
+		}
+
+		if pc.t.Failed() {
+			pc.t.Errorf("attempt %d failed validation", attempt)
 
-	checks = append(checks, PanicCheck{})
+			pc.t.Logf("dumping stdout: %s", stdout)
+			pc.t.Logf("dumping stdout: %s", stderr)
 
-	for _, check := range checks {
-		checkErr := check.Check(stdout, stderr, err)
-		if checkErr != nil {
-			checkerName := InferName(check)
-			pc.t.Errorf("check %q failed: %s", checkerName, checkErr)
+			break
 		}
 	}
 }

packer_test/init_test.go

@@ -1,6 +1,8 @@
 package packer_test
 
 func (ts *PackerTestSuite) TestPackerInitForce() {
+	ts.SkipNoAcc()
+
 	pluginPath, cleanup := ts.MakePluginDir()
 	defer cleanup()
 
@@ -18,6 +20,8 @@ func (ts *PackerTestSuite) TestPackerInitForce() {
 }
 
 func (ts *PackerTestSuite) TestPackerInitUpgrade() {
+	ts.SkipNoAcc()
+
 	pluginPath, cleanup := ts.MakePluginDir()
 	defer cleanup()
 
@@ -62,6 +66,8 @@ func (ts *PackerTestSuite) TestPackerInitWithNonGithubSource() {
 }
 
 func (ts *PackerTestSuite) TestPackerInitWithMixedVersions() {
+	ts.SkipNoAcc()
+
 	pluginPath, cleanup := ts.MakePluginDir()
 	defer cleanup()
 

packer_test/install_test.go

@@ -69,6 +69,8 @@ func (ts *PackerTestSuite) TestInstallPluginPrerelease() {
 }
 
 func (ts *PackerTestSuite) TestRemoteInstallWithPluginsInstall() {
+	ts.SkipNoAcc()
+
 	pluginPath, cleanup := ts.MakePluginDir()
 	defer cleanup()
 
@@ -80,6 +82,8 @@ func (ts *PackerTestSuite) TestRemoteInstallWithPluginsInstall() {
 }
 
 func (ts *PackerTestSuite) TestRemoteInstallOfPreReleasePlugin() {
+	ts.SkipNoAcc()
+
 	pluginPath, cleanup := ts.MakePluginDir()
 	defer cleanup()