hcl2_upgrade: change handling of Go template escaping sequences

[lbajolet-hashicorp]

Legacy JSON templates are essentially Go templates with extra limitations.

Leveraging this fact, we wrote the hcl2_upgrade command to migrate from JSON to HCL2 templates, and doing so, we interpret the template with a couple of fallbacks for things we don't want/can't replace.

This list being explicit, it means that whenever one's missing, it gets replaced by go template's default behaviour: <no value>.

As pointed out in issue #11920, this is surprising for users. Also, this is a pretty rigid approach, as we must change the code and release a new version of Packer every time there's something else that pops up that wasn't already known and handled.
There's also the problem of nested values, due to the type of PASSTHROUGH, there can only be direct replacements from string to string. This falls through in cases like {{ .SourceAMITags.TagName }}.

This PR is an attempt at a more general fix: rather than relying on a manually populated list of strings to replace by themselves, we go for a two-pass approach, where we escape the go-template sequences {{ and }} so their contents are not interpreted afterwards, leaving out those we can handle.

Closes #11920

[nywilken]

I really like this approach. I left a nit pick and a question out of curiosity. That said, my concern about allowing unknown template variables is that users may have some invalid fields and Packer does not know they are invalid. Which can be confusing to the user when they validate or try to execute. I do wonder if there should be some sort of warning when executing with the with-annotations flag?

I changed your input template to use {{ .Else }} as the communicator type and then executed validate on it. You can see it is reporting for the type, when it is set to {{ .Else }}.

~>  go run . validate /tmp/input.json.pkr.hcl
Error: 4 error(s) occurred:

* Communicator type <no value> is invalid
* a Host must be specified, please reference your communicator documentation
* a Username must be specified, please reference your communicator documentation
* one authentication method must be specified, please reference your communicator documentation

  on /tmp/input.json.pkr.hcl line 13:
  (source code not available)


exit status 1

variable "envtest" {
  type    = string
  default = "${env("Something")}"
}

variable "test" {
  type    = string
  default = "{{ .Something }}"
}
# The "legacy_isotime" function has been provided for backwards compatability, but we recommend switching to the timestamp and formatdate functions.

source "null" "autogenerated_1" {
  communicator = "{{ .Else }}"
}

build {
  sources = ["source.null.autogenerated_1"]

  provisioner "shell-local" {
    inline = ["packer-${legacy_isotime("2006-01-02 03:04:05")}", "echo {{ .Vars }} sudo -E -S bash '{{ .Path }}'", "echo '${build.SSHPrivateKey}' > /tmp/packer-session.pem"]
  }

}

[lbajolet-hashicorp]

Hey @nywilken, thanks for the review. To your point: I agree that this approach trades correctness for flexibility.
If a user does reference something that does not exist, it will be expanded to <no value> now. We could change this behaviour in the rest of the code to produce an error in case of a non-defined variable, this would arguably make more sense than reporting <no value> when interpreting the template.
At the same time, I think the current approach of vetting every possible variable from every plugin is also doomed to be inaccurate, as we'll repeatedly end-up patching Packer to add the ones we forgot/didn't know about.
I think letting the users know that having legacy templated values may be erroneous in HCL2 would be a better idea. We could add some warning either in the file, or in stdout/err so they're aware of that, and they can choose what to do with them afterwards.

[nywilken]

Thinking about this a little more the majority of these go template variables that are being escaped are the builders specific, right?

So to actually use them in HCL should they not be build.VariableName?

thinking of https://developer.hashicorp.com/packer/docs/templates/hcl_templates/contextual-variables

[lbajolet-hashicorp]

Very true, but from inside a source/build block, we can't use the HCL2 build.VariableName syntax according to the docs at https://developer.hashicorp.com/packer/plugins/builders/amazon/ebs#build-shared-information-variables.
So I imagine until we fix this, we should probably leave the current Go-template syntax.

On another note, I can confirm this isn't accepted right now, trying to get the variable from within a source returns an error:

$ packer validate ami_template.json.pkr.hcl 
Error: Unsupported attribute

  on ami_template.json.pkr.hcl line 4:
  (source code not available)

This object does not have an attribute named "SourceAMI".

[nywilken]

Looking at the original PR where the PASSTHROUGH logic was implemented and seeing how it fails. This change makes sense to me, and offers more flexibility than the current implementation.

[github-actions]

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.