From e03ad29ca0fc6055b10e32830f2d3eaeaf6c2169 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 14 Feb 2022 17:00:41 +0100 Subject: [PATCH] Sanitize var code (#11566) * evaluateLocalVariables: modify code for readability and some (not benchmarked) perfs * Make default input variable type the DynamicPseudoType This should be the default, and avoids a panic. This type can represent situations where a type is not yet known. Its meaning is undefined in cty. * do not take Empty types from default value * Update types.variables.go Co-authored-by: Wilken Rivera --- command/build_test.go | 30 +++++++++++++++++ .../hcl/empty_object/setting.auto.pkrvars.hcl | 4 +++ .../hcl/empty_object/var.pkr.hcl | 18 +++++++++++ .../recursive_local_with_input/locals.pkr.hcl | 25 +++++++++++++++ .../file.pkr.hcl | 12 +++++++ hcl2template/types.packer_config.go | 32 ++++++------------- hcl2template/types.variables.go | 7 ++-- .../functions/collection/lookup.mdx | 4 --- 8 files changed, 104 insertions(+), 28 deletions(-) create mode 100644 command/test-fixtures/hcl/empty_object/setting.auto.pkrvars.hcl create mode 100644 command/test-fixtures/hcl/empty_object/var.pkr.hcl create mode 100644 command/test-fixtures/hcl/recursive_local_with_input/locals.pkr.hcl create mode 100644 command/test-fixtures/hcl/recursive_local_with_unset_input/file.pkr.hcl diff --git a/command/build_test.go b/command/build_test.go index adef38f19..ef7510030 100644 --- a/command/build_test.go +++ b/command/build_test.go @@ -435,6 +435,36 @@ func TestBuild(t *testing.T) { }, }, }, + { + name: "hcl - recursive local using input var", + args: []string{ + testFixture("hcl", "recursive_local_with_input"), + }, + fileCheck: fileCheck{ + expectedContent: map[string]string{ + "hey.txt": "hello", + }, + }, + }, + { + name: "hcl - recursive local using an unset input var", + args: []string{ + testFixture("hcl", "recursive_local_with_unset_input"), + }, + fileCheck: fileCheck{}, + expectedCode: 1, + }, + { + name: "hcl - var with default value empty object/list can be set", + args: []string{ + testFixture("hcl", "empty_object"), + }, + fileCheck: fileCheck{ + expectedContent: map[string]string{ + "foo.txt": "yo", + }, + }, + }, } for _, tt := range tc { diff --git a/command/test-fixtures/hcl/empty_object/setting.auto.pkrvars.hcl b/command/test-fixtures/hcl/empty_object/setting.auto.pkrvars.hcl new file mode 100644 index 000000000..da5194a02 --- /dev/null +++ b/command/test-fixtures/hcl/empty_object/setting.auto.pkrvars.hcl @@ -0,0 +1,4 @@ +foo = ["yo"] +bar = { + "baz": "foo", +} diff --git a/command/test-fixtures/hcl/empty_object/var.pkr.hcl b/command/test-fixtures/hcl/empty_object/var.pkr.hcl new file mode 100644 index 000000000..4cd35319a --- /dev/null +++ b/command/test-fixtures/hcl/empty_object/var.pkr.hcl @@ -0,0 +1,18 @@ + +variable "foo" { + default = [] +} + +variable "bar" { + default = {} +} + +source "file" "base" { +} + +build { + source "sources.file.base" { + target = "${var.bar.baz}.txt" + content = var.foo[0] + } +} diff --git a/command/test-fixtures/hcl/recursive_local_with_input/locals.pkr.hcl b/command/test-fixtures/hcl/recursive_local_with_input/locals.pkr.hcl new file mode 100644 index 000000000..2f99b0dad --- /dev/null +++ b/command/test-fixtures/hcl/recursive_local_with_input/locals.pkr.hcl @@ -0,0 +1,25 @@ +variable "vms_to_build" { + default = { + "amgroup": "hey" + } +} + +locals { + vms_to_build = var.vms_to_build + dynamic_slice = { + for vm, val in var.vms_to_build : + vm => lookup(local.vms_to_build, vm, "VM NAME NOT FOUND") + } +} + + +source "file" "chocolate" { + content = "hello" + target = "${local.dynamic_slice.amgroup}.txt" +} + +build { + sources = [ + "sources.file.chocolate", + ] +} diff --git a/command/test-fixtures/hcl/recursive_local_with_unset_input/file.pkr.hcl b/command/test-fixtures/hcl/recursive_local_with_unset_input/file.pkr.hcl new file mode 100644 index 000000000..05fd53ce3 --- /dev/null +++ b/command/test-fixtures/hcl/recursive_local_with_unset_input/file.pkr.hcl @@ -0,0 +1,12 @@ + +variable "vms_to_build" { +} + +locals { + vms_to_build = var.vms_to_build + dynamic_map = { + for vm in local.vms_to_build : + vm => lookup(local, vm, "VM NAME NOT FOUND") + } +} + diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index 62f0f4cd5..bee8276e0 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -232,30 +232,18 @@ func (c *PackerConfig) evaluateLocalVariables(locals []*LocalBlock) hcl.Diagnost 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...) + for foundSomething := true; foundSomething; { + foundSomething = false + for i := 0; i < len(locals); { + local := locals[i] + moreDiags := c.evaluateLocalVariable(local) + if moreDiags.HasErrors() { + i++ + continue } - retry++ - continue + foundSomething = true + locals = append(locals[:i], locals[i+1:]...) } - - // could evaluate - retry = 0 - diags = append(diags, moreDiags...) - - // Remove local from slice - locals = append(locals[:i], locals[i+1:]...) - llen-- } if len(locals) != 0 { diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go index 6ace7abf2..6cb4977e0 100644 --- a/hcl2template/types.variables.go +++ b/hcl2template/types.variables.go @@ -335,6 +335,7 @@ func (variables *Variables) decodeVariableBlock(block *hcl.Block, ectx *hcl.Eval v := &Variable{ Name: name, Range: block.DefRange, + Type: cty.DynamicPseudoType, } if attr, exists := content.Attributes["description"]; exists { @@ -386,7 +387,9 @@ func (variables *Variables) decodeVariableBlock(block *hcl.Block, ectx *hcl.Eval // It's possible no type attribute was assigned so lets make sure we // have a valid type otherwise there could be issues parsing the value. - if v.Type == cty.NilType { + if v.Type == cty.DynamicPseudoType && + !defaultValue.Type().Equals(cty.EmptyObject) && + !defaultValue.Type().Equals(cty.EmptyTuple) { v.Type = defaultValue.Type() } } @@ -752,7 +755,7 @@ func (cfg *PackerConfig) collectInputVariableValues(env []string, files []*hcl.F // The specified filename is to identify the source of where value originated from in the diagnostics report, if there is an error. func expressionFromVariableDefinition(filename string, value string, variableType cty.Type) (hclsyntax.Expression, hcl.Diagnostics) { switch variableType { - case cty.String, cty.Number, cty.NilType: + case cty.String, cty.Number, cty.NilType, cty.DynamicPseudoType: // when the type is nil (not set in a variable block) we default to // interpreting everything as a string literal. return &hclsyntax.LiteralValueExpr{Val: cty.StringVal(value)}, nil diff --git a/website/content/docs/templates/hcl_templates/functions/collection/lookup.mdx b/website/content/docs/templates/hcl_templates/functions/collection/lookup.mdx index c01fddd0e..c7cdb671c 100644 --- a/website/content/docs/templates/hcl_templates/functions/collection/lookup.mdx +++ b/website/content/docs/templates/hcl_templates/functions/collection/lookup.mdx @@ -12,10 +12,6 @@ If the given key does not exist, a the given default value is returned instead. lookup(map, key, default) ``` --> For historical reasons, the `default` parameter is actually optional. However, -omitting `default` is deprecated since v0.7 because that would then be -equivalent to the native index syntax, `map[key]`. - ## Examples ```shell-session