From 5d17d7fe65f9e26fbf7cce96449161e91bb37fb7 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Fri, 28 Jan 2022 19:21:53 +0100 Subject: [PATCH] prevent duplicate local block creation (#11534) * prevent duplicate local block creation * remove duplicate locals block bug * local variables: first get block, then decode it + simplify retry loop * Update types.packer_config.go * revert go get of hcl lib --- hcl2template/parser.go | 3 +- hcl2template/types.packer_config.go | 142 +++++++++++++++------------- hcl2template/types.variables.go | 12 +-- 3 files changed, 78 insertions(+), 79 deletions(-) diff --git a/hcl2template/parser.go b/hcl2template/parser.go index c84934c53..1dc518c05 100644 --- a/hcl2template/parser.go +++ b/hcl2template/parser.go @@ -193,7 +193,7 @@ func (p *Parser) Parse(filename string, varFiles []string, argVars map[string]st } for _, file := range files { - moreLocals, morediags := cfg.parseLocalVariables(file) + moreLocals, morediags := parseLocalVariableBlocks(file) diags = append(diags, morediags...) cfg.LocalBlocks = append(cfg.LocalBlocks, moreLocals...) } @@ -315,6 +315,7 @@ func (cfg *PackerConfig) Initialize(opts packer.InitializeOptions) hcl.Diagnosti moreDiags = cfg.LocalVariables.ValidateValues() diags = append(diags, moreDiags...) diags = append(diags, cfg.evaluateDatasources(opts.SkipDatasourcesExecution)...) + diags = append(diags, checkForDuplicateLocalDefinition(cfg.LocalBlocks)...) diags = append(diags, cfg.evaluateLocalVariables(cfg.LocalBlocks)...) filterVarsFromLogs(cfg.InputVariables) diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index 41c0c03f2..2ceabda71 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -177,42 +177,29 @@ func (c *PackerConfig) decodeInputVariables(f *hcl.File) hcl.Diagnostics { return diags } -// parseLocalVariables looks in the found blocks for 'locals' blocks. It -// should be called after parsing input variables so that they can be -// referenced. -func (c *PackerConfig) parseLocalVariables(f *hcl.File) ([]*LocalBlock, hcl.Diagnostics) { +// parseLocalVariableBlocks looks in the AST for 'local' and 'locals' blocks and +// returns them all. +func parseLocalVariableBlocks(f *hcl.File) ([]*LocalBlock, hcl.Diagnostics) { var diags hcl.Diagnostics content, moreDiags := f.Body.Content(configSchema) diags = append(diags, moreDiags...) - locals := c.LocalBlocks + var locals []*LocalBlock for _, block := range content.Blocks { switch block.Type { case localLabel: - l, moreDiags := decodeLocalBlock(block, locals) + block, moreDiags := decodeLocalBlock(block) diags = append(diags, moreDiags...) - if l != nil { - locals = append(locals, l) - } if moreDiags.HasErrors() { return locals, diags } + locals = append(locals, block) case localsLabel: attrs, moreDiags := block.Body.JustAttributes() diags = append(diags, moreDiags...) for name, attr := range attrs { - if _, found := c.LocalVariables[name]; found { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Duplicate value in " + localsLabel, - Detail: "Duplicate " + name + " definition found.", - Subject: attr.NameRange.Ptr(), - Context: block.DefRange.Ptr(), - }) - return locals, diags - } locals = append(locals, &LocalBlock{ Name: name, Expr: attr.Expr, @@ -221,67 +208,88 @@ func (c *PackerConfig) parseLocalVariables(f *hcl.File) ([]*LocalBlock, hcl.Diag } } - c.LocalBlocks = locals return locals, diags } func (c *PackerConfig) evaluateAllLocalVariables(locals []*LocalBlock) hcl.Diagnostics { - var local_diags hcl.Diagnostics - - // divide by 2 so that you don't get duplicate locals - // appear to have double locals in LocalBlock, not sure if intentional - for i := 0; i < len(locals)/2; i++ { - diags := c.evaluateLocalVariable(locals[i]) - if diags.HasErrors() { - local_diags = append(local_diags, diags...) - } - } - - return local_diags -} - -func (c *PackerConfig) evaluateLocalVariables(locals []*LocalBlock) hcl.Diagnostics { var diags hcl.Diagnostics - if len(locals) > 0 && c.LocalVariables == nil { - c.LocalVariables = Variables{} - } - - var retry, previousL int - 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 c.evaluateAllLocalVariables(locals) - } - 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:]...) + for _, local := range locals { + diags = append(diags, c.evaluateLocalVariable(local)...) } return diags } +func (c *PackerConfig) evaluateLocalVariables(locals []*LocalBlock) hcl.Diagnostics { + var diags hcl.Diagnostics + + if len(locals) == 0 { + return diags + } + + if c.LocalVariables == nil { + c.LocalVariables = Variables{} + } + + llen := len(locals) + var retry int + // avoid to retrying more than the number of items we have in the slice + for i := 0; llen > 0 && retry < llen+1; i++ { + i := i % llen + local := locals[i] + moreDiags := c.evaluateLocalVariable(local) + if moreDiags.HasErrors() { + if llen == 1 { + // If this is the only local left there's no need + // to try evaluating again + return append(diags, moreDiags...) + } + retry++ + continue + } + + // could evaluate + retry = 0 + diags = append(diags, moreDiags...) + + // Remove local from slice + locals = append(locals[:i], locals[i+1:]...) + llen-- + } + + if len(locals) != 0 { + // get errors from remaining variables + return c.evaluateAllLocalVariables(locals) + } + + return diags +} + +func checkForDuplicateLocalDefinition(locals []*LocalBlock) 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{ + Severity: hcl.DiagError, + Summary: "Duplicate local definition", + Detail: "Duplicate " + local.Name + " definition found.", + Subject: local.Expr.Range().Ptr(), + }) + continue + } + names[local.Name] = struct{}{} + } + return diags +} + func (c *PackerConfig) evaluateLocalVariable(local *LocalBlock) hcl.Diagnostics { var diags hcl.Diagnostics + value, moreDiags := local.Expr.Value(c.EvalContext(LocalContext, nil)) diags = append(diags, moreDiags...) if moreDiags.HasErrors() { diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go index 61bb54345..6ace7abf2 100644 --- a/hcl2template/types.variables.go +++ b/hcl2template/types.variables.go @@ -275,18 +275,8 @@ var localBlockSchema = &hcl.BodySchema{ }, } -func decodeLocalBlock(block *hcl.Block, locals []*LocalBlock) (*LocalBlock, hcl.Diagnostics) { +func decodeLocalBlock(block *hcl.Block) (*LocalBlock, hcl.Diagnostics) { name := block.Labels[0] - for _, loc := range locals { - if loc.Name == name { - return nil, []*hcl.Diagnostic{{ - Severity: hcl.DiagError, - Summary: "Duplicate variable", - Detail: "Duplicate " + block.Labels[0] + " variable definition found.", - Context: block.DefRange.Ptr(), - }} - } - } content, diags := block.Body.Content(localBlockSchema) if !hclsyntax.ValidIdentifier(name) {