Fix crash when an unset variable is used by azr · Pull Request #8837 · hashicorp/packer

azr · 2020-03-04T12:05:52Z

This was due to a misunderstanding on my side. When a variable is not set one needs to set it to cty.NullVal(cty.DynamicPseudoType) , like so:

packer/hcl2template/types.variables.go

Lines 57 to 93 in 23b940d

    
           func (v *Variable) Value() (cty.Value, *hcl.Diagnostic) { 
        
           	for _, value := range []cty.Value{ 
        
           		v.CmdValue, 
        
           		v.VarfileValue, 
        
           		v.EnvValue, 
        
           		v.DefaultValue, 
        
           	} { 
        
           		if !value.IsNull() { 
        
           			return value, nil 
        
           		} 
        
           	} 
        
           	value := cty.NullVal(cty.DynamicPseudoType) 
        
           	return value, &hcl.Diagnostic{ 
        
           		Severity: hcl.DiagError, 
        
           		Summary:  fmt.Sprintf("Unset variable %q", v.Name), 
        
           		Detail: "A used variable must be set or have a default value; see " + 
        
           			"https://packer.io/docs/configuration/from-1.5/syntax.html for details.", 
        
           		Context: v.block.DefRange.Ptr(), 
        
           	} 
        
           } 
        
           type Variables map[string]*Variable 
        
           func (variables Variables) Values() map[string]cty.Value { 
        
           	res := map[string]cty.Value{} 
        
           	for k, v := range variables { 
        
           		res[k], _ = v.Value() 
        
           		// here the value might not be used and in that case we don't want to 
        
           		// force users to set it. So this error is ignored. we still set it as 
        
           		// it can be a `cty.NullVal(cty.DynamicPseudoType)`, which is the go 
        
           		// cty value for 'unknown value' and should error when used. 
        
           	} 
        
           	return res 
        
           }

I choose not to error when a variable is not set and not used. But when someone does something like this:

variable "proxmox_username" {
  type = string
}

build {
  sources = [
    "source.null.null-builder${var.proxmox_username}",
  ]
}


source "null" "null-builder" {
}

The error is the following:


  on .vscode/hcl2/crash/var_with_no_default.pkr.hcl line 8, in build:
   8:     "source.null.null-builder${var.proxmox_username}",

Variables may not be used here.

Error: Unsuitable value type

  on .vscode/hcl2/crash/var_with_no_default.pkr.hcl line 7, in build:
   7:   sources = [
   8:     "source.null.null-builder${var.proxmox_username}",
   9:   ]

Unsuitable value: value must be known

fix #8730

…ding things to make sure tests are working similarly as real life version

hcl2template/types.packer_config.go

because a variable can be defined in the Locals block

sylviamoss

LGTM! 🎉

hcl2template/types.build.go

hcl2template/types.packer_config.go

azr · 2020-03-04T15:26:28Z

Question for the team. With this PR a variable can be defined and have no value.But this would be different that for Terraform; so quoting this message:

By default, a Terraform variable is required and so Terraform would emit an error if it isn't set. An author can make a variable optional by providing a default value.
variable "example" {
  type    = string
  default = "Hello"
}
In some cases there isn't any reasonable default value, and the module simply wants to detect whether or not the value is set. In that case, the author can specify the default as null to say that the variable is optional but it has no default:
variable "example" {
  type    = string
  default = null
}

Should we do the same ?

sylviamoss · 2020-03-04T15:38:16Z

Should we do the same ?

I think as a terraform user the one might expect to be the same. In terraform if the variable without a default is used, is it going to return an error as your implementation does?

I like the way you did and I think both solutions are good. If it's not too difficult, I would do it as terraform just for consistency. Maybe just add the option of setting default = null without removing what you did.

nywilken · 2020-03-04T23:24:22Z

hcl2template/types.variables.go

 	}

-	(*variables)[block.Labels[0]] = res
+	(*variables)[name] = res


nywilken · 2020-03-05T11:53:09Z

hcl2template/types.variables.go

 	Sensitive bool

-	block *hcl.Block
+	Range hcl.Range


Is this so that we can keep track of the block when generating diagnostics?

azr · 2020-03-05T13:48:10Z

go.mod


 replace github.com/gofrs/flock => github.com/azr/flock v0.0.0-20190823144736-958d66434653

-replace github.com/zclconf/go-cty => github.com/azr/go-cty v1.1.1-0.20200203143058-28fcda2fe0cc


My pr was merged; so this can be removed.

A variable's default value can be set to null to force user to set it.

azr · 2020-03-10T14:19:59Z

Hey folks ! I think this PR is good to review. I think the behaviour are super close to what we want in the RFC. But since this also fixes a crash I think we should merge it.

I'll gladly update the behaviours if/when the RFC changes later on 🙂.

SwampDragons · 2020-03-10T17:35:04Z

hcl2template/testdata/variables/basic.pkr.hcl

@@ -29,6 +29,7 @@ variable "super_secret_password" {
 description = <<IMSENSIBLE


Did you mean "sensitive" here instead of "sensible"?

nywilken · 2020-03-10T18:22:23Z

website/source/docs/configuration/from-1.5/variables.html.md

 ~> **Important:** Variables with map and object values behave the same way as
 other variables: the last value found overrides the previous values.
+
+### A variable value must be known :


💯 👍 🏆 Nicely done on adding the variables table.

Thanks to the following issue hashicorp/packer#8846, and the fact that they’ll be removing abiity to use a simple ‘go get’ (hashicorp/packer#8837 (comment)), it makes no sense to try to build it locally anymore.

…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-10T01:50:23Z

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.

azr added 15 commits February 14, 2020 11:54

try to reproduce #8730 in tests

4b10c5a

refactor prepare into MockConfig

1ec68ca

insert "github.com/zclconf/go-cty/cty/json" encoding beforce hcl deco…

00c812c

…ding things to make sure tests are working similarly as real life version

add test for unset variable

79867ca

give correct error when variable is unset

cebfb1c

continue and realise and unused undefaulted value triggers an error

3c213e6

Update mock.go

52e5b70

WIP

50896d4

Merge remote-tracking branch 'origin/master' into fix_8730

59cc246

Update types.packer_config.go

382f209

don't force to set unused variables too

e851050

fix this !

23b940d

parseLocalVariables: simplify loop

bd3112f

evaluateLocalVariable: also pass the variable name

8482c6c

update tests

9b64959

azr requested a review from a team as a code owner March 4, 2020 12:05

azr commented Mar 4, 2020

View reviewed changes

hcl2template/types.packer_config.go Show resolved Hide resolved

azr added 3 commits March 4, 2020 13:19

variables: use Range instead of Block

e9d3826

because a variable can be defined in the Locals block

bup modules

f013164

vendor vendors

e195c62

sylviamoss approved these changes Mar 4, 2020

View reviewed changes

hcl2template/types.build.go Show resolved Hide resolved

hcl2template/types.packer_config.go Show resolved Hide resolved

sylviamoss added hcl2 bug labels Mar 4, 2020

azr mentioned this pull request Mar 4, 2020

Packer with HCL crashes on missing variable default. #8730

Closed

nywilken reviewed Mar 4, 2020

View reviewed changes

hcl2template/types.variables.go

}

(*variables)[block.Labels[0]] = res

(*variables)[name] = res

Copy link
Copy Markdown

Contributor

nywilken Mar 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

nywilken reviewed Mar 5, 2020

View reviewed changes

azr commented Mar 5, 2020

View reviewed changes

nywilken mentioned this pull request Mar 5, 2020

packer 1.5.4 build error #8846

Closed

azr changed the title ~~Fix crash when an unset variable is used~~ [WIP] Fix crash when an unset variable is used Mar 6, 2020

azr added 3 commits March 9, 2020 16:16

tweak validation & add tests

6d8cce5

all variables must have a value.

0ccff0d

A variable's default value can be set to null to force user to set it.

Merge branch 'master' into fix_8730

631f402

azr changed the title ~~[WIP] Fix crash when an unset variable is used~~ Fix crash when an unset variable is used Mar 10, 2020

azr added 2 commits March 10, 2020 14:51

document variables behaviours with tables

c4349f5

Update variables.html.md

d4d3012

SwampDragons reviewed Mar 10, 2020

View reviewed changes

SwampDragons approved these changes Mar 10, 2020

View reviewed changes

SwampDragons merged commit 8a1caaa into master Mar 10, 2020

nywilken reviewed Mar 10, 2020

View reviewed changes

azr deleted the fix_8730 branch March 11, 2020 09:19

SwampDragons added this to the 1.5.5 milestone Mar 12, 2020

ghost locked and limited conversation to collaborators Apr 10, 2020


	func (v Variable) Value() (cty.Value, hcl.Diagnostic) {
	for _, value := range []cty.Value{
	v.CmdValue,
	v.VarfileValue,
	v.EnvValue,
	v.DefaultValue,
	} {
	if !value.IsNull() {
	return value, nil
	}
	}

	value := cty.NullVal(cty.DynamicPseudoType)

	return value, &hcl.Diagnostic{
	Severity: hcl.DiagError,
	Summary: fmt.Sprintf("Unset variable %q", v.Name),
	Detail: "A used variable must be set or have a default value; see " +
	"https://packer.io/docs/configuration/from-1.5/syntax.html for details.",
	Context: v.block.DefRange.Ptr(),
	}
	}

	type Variables map[string]*Variable

	func (variables Variables) Values() map[string]cty.Value {
	res := map[string]cty.Value{}
	for k, v := range variables {
	res[k], _ = v.Value()
	// here the value might not be used and in that case we don't want to
	// force users to set it. So this error is ignored. we still set it as
	// it can be a `cty.NullVal(cty.DynamicPseudoType)`, which is the go
	// cty value for 'unknown value' and should error when used.
	}
	return res
	}


		replace github.com/gofrs/flock => github.com/azr/flock v0.0.0-20190823144736-958d66434653

		replace github.com/zclconf/go-cty => github.com/azr/go-cty v1.1.1-0.20200203143058-28fcda2fe0cc

		@@ -29,6 +29,7 @@ variable "super_secret_password" {
		description = <<IMSENSIBLE

Conversation

azr commented Mar 4, 2020

Uh oh!

Uh oh!

sylviamoss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

azr commented Mar 4, 2020

Uh oh!

sylviamoss commented Mar 4, 2020

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!

azr commented Mar 10, 2020

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!

ghost commented Apr 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants