Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Referencing undefined variables is OK #7328

Closed
woky opened this issue Feb 20, 2019 · 8 comments
Closed

Referencing undefined variables is OK #7328

woky opened this issue Feb 20, 2019 · 8 comments
Labels
core Core components of Packer interpolation

Comments

@woky
Copy link

woky commented Feb 20, 2019

When referencing unset variable, Packer should fail or, to keep backward compatibility, warn by default and fail if the behavior is requested by command flag. This was requested before in #3439 and swept under the rug (closed) because it was not planned for release 1.0.

Packer validate relies only on back-end declaring some object fields as non-empty, which is very short-sighted as there are much more optional but very much used fields, and silently replacing undefined variables in them with empty strings leads to undefined behvaior and cryptic errors.

One such example is configuration of googlecompute backed with account_file. Seemingly the backed is OK if the account_file is empty, perhaps because it also works with properly configured gcloud or in GCE environment. If you have GCE image builder configured with

"type": "googlecompute",
"account_file": "{{ user `account_file` }}",

and account_file variable is not supplied, packer validate doesn't complain, and packer build produces several pages of errors, starting with:

==> googlecompute: Checking image does not exist...
==> googlecompute: Creating temporary SSH key for instance...
==> googlecompute: Error getting source image for instance creation: Could not find image, ubuntu-minimal-1810-cosmic-v20190206, in projects, [developer-129233 centos-cloud cos-cloud coreos-cloud debian-cloud rhel-cloud rhel-sap-cloud suse-cloud suse-sap-cloud ubuntu-os-cloud windows-cloud windows-sql-cloud gce-uefi-images gce-nvme google-containers opensuse-cloud]: 16 error(s) occurred:
==> googlecompute: 
==> googlecompute: * googleapi: Error 403: Insufficient Permission: Request had insufficient authentication scopes., insufficientPermisions
...

Consequences of silently replacing undefined variable with empty string in provision command line can be worse.

Since it's post-1.0, it'd be good if Packer could be (globally) configured to fail on undefined variables and warn on their usage by default, both in build and validate commands. It'd be even better if this was the default behavior, but it'd probably break existing users relying on current faulty behavior.

@rickard-von-essen rickard-von-essen added core Core components of Packer interpolation labels Feb 20, 2019
@rickard-von-essen
Copy link
Collaborator

Related to #6473

@woky
Copy link
Author

woky commented Feb 20, 2019

I think it'd be good to keep the current behavior as resolved in #6473, i.e. not needing to specify variables in file to specify them in command line. If you put this restrictions, i.e. if are forced to define every variable (meant to be set from command line) with empty value in file, you'll loose the ability of Packer checking if a variable is defined (it'll be always defined by the rule). Just failing on access on undefined variable access should catch most of the errors.

@fr34k8
Copy link

fr34k8 commented Apr 26, 2019

Any progress on this ?
I‘m unable to build inside googlecompute.
That’s anoying..

@SwampDragons
Copy link
Contributor

SwampDragons commented Apr 26, 2019

Hey all, I must have missed this when it first got opened.

We do have a behavior where if you set a variable to "null" in the file, the user is required to provide it on the command line or in a var file. Example:

{
	"variables": {
		"account_file": null
	},
	"builders": [
		{
			"type": "null",
			"communicator": "none"
		}
	],
	"provisioners": [
		{
			"type": "shell-local",
			"inline": ["echo {{ user `account_file`}}"]
		}
	]
}

This will fail validation unless you explicitly set that variable through a var-file or the command line.

My feeling is that this behavior is better than making all variables required; there are many situations where an unset variable is okay and appropriate, and we can't possibly know what those situations are -- which is why we allow you to declare the important variables as required and trust you to do what's right for the rest.

@fr34k8 I'd be interested to hear more about why this is preventing you from building altogether.

@fr34k8
Copy link

fr34k8 commented May 2, 2019

Cheers @SwampDragons !

if answer_file var is defined in googlecompute builder section aka: $HOME/.config/gcloud/application_default_credentials.json
which is the default location 4 gcloud sdk, validation fails.

if referenced in variables section and used in builder section as variable, same result.

if not defined validation works, cause packer is used defaults of gcloud SDK.
my intention is to use one pipeline and overwrite credentials inside each stage.

I used this inside gitlab pipelines.

@SwampDragons
Copy link
Contributor

I'm still not following. Can I see an example config that's failing for you?

@SwampDragons
Copy link
Contributor

Closing as I never heard back and I'm hoping that using "null" was an appropriate solution for the OP.

@ghost
Copy link

ghost commented Mar 28, 2020

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.

@ghost ghost locked and limited conversation to collaborators Mar 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
core Core components of Packer interpolation
Projects
None yet
Development

No branches or pull requests

4 participants