From 14cf4b40d450ce5e2641792de4f5500893ad2a6c Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Tue, 11 Jun 2024 15:42:32 -0400 Subject: [PATCH] hcl2template: recursively evaluate local variables The logic for evaluating local variables used to rely on their definition order, with some edge cases. Typically `locals` blocks define multiple local variables, which don't necessarily appear in the same order at evaluation as within the template, leading to inconsistent behaviour, as the order in which those are added to the list of local variables is non-deterministic. To avoid this problem, we change how local variables are evaluated, and we're adopting a workflow similar to datasources, where the local variables first build a list of direct dependencies. Then when evaluation happens, we evaluate all the dependencies recursively for each local variable, which takes care of this issue. As with Datasources, we add a cap to the recursion: 10. I.e. if the evaluation of a single variable provokes an infinite recursion, we stop at that point and return an error to the user, telling them to fix their template. --- hcl2template/types.packer_config.go | 79 ++++++++++++++----- hcl2template/types.variables.go | 11 +++ packer_test/local_eval_test.go | 14 ++++ packer_test/templates/locals_no_order.pkr.hcl | 7 ++ 4 files changed, 93 insertions(+), 18 deletions(-) create mode 100644 packer_test/local_eval_test.go create mode 100644 packer_test/templates/locals_no_order.pkr.hcl diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index d21d10ede..4c69c4467 100644 --- a/hcl2template/types.packer_config.go +++ b/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,23 +240,41 @@ 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 + } + + // Immediately return in case the dependencies couldn't be figured out. + if diags.HasErrors() { + return diags } - if len(locals) != 0 { - // get errors from remaining variables - return c.evaluateAllLocalVariables(locals) + for _, local := range c.LocalBlocks { + diags = diags.Extend(c.evaluateLocalVariable(local, 0)) } return diags @@ -281,10 +301,33 @@ func checkForDuplicateLocalDefinition(locals []*LocalBlock) hcl.Diagnostics { 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 diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go index b3754e8f1..3e7f9e1cb 100644 --- a/hcl2template/types.variables.go +++ b/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 diff --git a/packer_test/local_eval_test.go b/packer_test/local_eval_test.go new file mode 100644 index 000000000..97cecf0ef --- /dev/null +++ b/packer_test/local_eval_test.go @@ -0,0 +1,14 @@ +package packer_test + +func (ts *PackerTestSuite) TestEvalLocalsOrder() { + ts.SkipNoAcc() + + pluginDir, cleanup := ts.MakePluginDir() + defer cleanup() + + ts.PackerCommand().UsePluginDir(pluginDir). + Runs(10). + Stdin("local.test_local\n"). + SetArgs("console", "./templates/locals_no_order.pkr.hcl"). + Assert(MustSucceed(), Grep("\\[\\]", grepStdout, grepInvert)) +} diff --git a/packer_test/templates/locals_no_order.pkr.hcl b/packer_test/templates/locals_no_order.pkr.hcl new file mode 100644 index 000000000..dbfb4a7fb --- /dev/null +++ b/packer_test/templates/locals_no_order.pkr.hcl @@ -0,0 +1,7 @@ +locals { + test_local = can(local.test_data) ? local.test_data : [] + + test_data = [ + { key = "value" } + ] +}