hashicorp / terraform

Terraform enables you to safely and predictably create, change, and improve infrastructure. It is a source-available tool that codifies APIs into declarative configuration files that can be shared amongst team members, treated as code, edited, reviewed, and versioned.
https://www.terraform.io/
Other
42.57k stars 9.54k forks source link

stripmarkers and indented heredoc #23710

Open bentterp opened 4 years ago

bentterp commented 4 years ago

Terraform Version

0.12.18

Terraform Configuration Files

locals {
  hosts = [
    {private_ip: "10.0.1.4", name: "server1"},
    {private_ip: "10.0.2.4", name: "server2"},
    {private_ip: "10.0.3.4", name: "server3"}
  ]
}

output "missing_strip" {
  value = <<-EOT
    %{ for server in local.hosts }
    ${server.private_ip} ${server.name}
    %{ endfor ~}
    EOT
}

output "missing_indent" {
  value = <<-EOT
    %{ for server in local.hosts ~}
    ${server.private_ip} ${server.name}
    %{ endfor }
    EOT
}

Debug Output

Crash Output

Expected Behavior

According to the documentation, it should not matter if the strip marker is placed in the for directive or the endfor directive.

Actual Behavior

When the strip marker is put in the for directive, the indentation of the heredoc is not removed as expected but the newline is removed. When the strip marker is put in the endfor directive, the indentation of the heredoc is removed as expected, but not the newline.

Steps to Reproduce

terraform apply

Additional Context

References

bentterp commented 4 years ago

Having the strip marker in both places doesn't work, either

apparentlymart commented 4 years ago

Thanks for reporting this, @bentterp.

Indeed, it does seem like the strip markers and the flush-parsing mode are interacting poorly here. This seems to be a bug upstream in HCL, and seems to be caused by the strip marker changing the resulting tokens during parsing in a way that the flush mode processing can't handle. Specifically, flush mode expects to find a newline before each "indentation", but I think the strip marker is stripping away that newline during parsing so that flush mode then doesn't understand the following indent as being an indent.

I think the root problem here is that the strip marker handling is a little flawed: it's stripping leading whitespace from the next literal token, but that's not sufficient in this case because the whitespace is actually split over two tokens, like this:

The strip marker removes the newline, but it doesn't remove the four spaces of indent because they are in a separate token.

If I'm right about this cause, I think the fix would be for the template parser to have an additional case in its handling of right strip markers where if the next token it encounters is entirely whitespace (that is, if the string is empty after removing all of the whitespace from the front) then to leave the "left strip" flag true so that it can continue stripping whitespace on subsequent tokens until it finds either a non-literal or a literal containing at least one non-whitespace character.

We'll need to fix this with some care because although the current behavior is not following the HCL spec I expect there are some folks relying on the current buggy implementation nonetheless, and so changing it will be impactful to those users. For Terraform in particular, heredoc templates are commonly used to populate arguments like user_data on aws_instance where changes would require replacement. However, there is some pressure the other way too: the longer this bug remains in the Go HCL implementation the more other applications will end up including it and thus the harder it will be to change.

pdecat commented 4 years ago

I expect there are some folks relying on the current bugging implementation nonetheless, and so changing it will be impactful to those users

What about introducing a new strict indented heredoc variant, e.g. <<=TOKEN or <<~TOKEN to avoid breaking existing configurations and let users opt-in with the new syntax?

kbolino commented 4 years ago

There are indentation-related inconsistencies even with regular heredocs. The following output different values even though as I read it they should produce the same result:

output "hello_heredoc" {
  value = <<EOF
Hello,
%{~ if "" == "" ~}
 world
%{~ endif ~}
!
EOF
}

output "hello_literal" {
  value = "Hello,\n%{~ if "" == "" ~}\n world\n%{~ endif ~}\n!"
}

Output:

$ terraform apply

Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

Outputs:

hello_heredoc = Hello, world!

hello_literal = Hello,world!
brikis98 commented 2 years ago

I'm having the same issue here: a normal heredoc and stripmarkers are behaving differently now than they used to. Here's an example from Terraform: Up & Running:

variable "names" {
  description = "Names to render"
  type        = list(string)
  default     = ["neo", "trinity", "morpheus"]
}

output "for_directive_strip_marker" {
  value = <<EOF
%{~ for name in var.names }
  ${name}
%{~ endfor }
EOF
}

With TF 0.12 or so, it used to output:

neo
trinity
morpheus

Now, with TF 1.1, the output has extra leading and trailing whitespace:


neo
trinity
morpheus

I had automated tests for this that used to pass and are failing now... And I can't find any way to fix it. It seems like HEREDOC doesn't let you put text on the same line as the HEREDOC marker, but the stripmarker no longer gets rid of those extra newlines.

src386 commented 2 years ago

Hi,

I can confirm the issue is still present on Terraform v1.1.2:

locals {
    mylist = [
        "item1",
        "item2",
        "item3"
    ]
}

output "template" {
    value = <<-EOF
        %{for item in local.mylist ~}
        ${item}
        %{endfor~}
    EOF
}

Results show that the indentation is not stripped as expected:

template = <<EOT
        item1
        item2
        item3

EOT

I tried to combine this with trimspace but it did not work.

I finally gave up and just use the following syntax:

locals {
    mylist = [
        "item1",
        "item2",
        "item3"
    ]
}

output "template" {
    value = <<EOF
%{for item in local.mylist ~}
${item}
%{endfor~}
EOF
}

Results:

template = <<EOT
item1
item2
item3

EOT

Not really a workaround because it doesn't makes the code more readable but I couldn't find a better solution.

stevehipwell commented 2 years ago

@apparentlymart is there a reason why this still isn't fixed?

apparentlymart commented 2 years ago

The current behavior is protected by the v1.x compatibility promises and so we cannot change the existing behavior without introducing new syntax to opt into it.

Therefore this requires further design work. Nobody on the team at HashiCorp is currently working on this.

stevehipwell commented 2 years ago

@apparentlymart this was reported and a known issue well before the Terraform v1 compatibility promise was even a potential deliverable. This defect does not follow the HCL language spec, does not follow the Terraform documented behaviour and the actual behaviour is not documented or easily discoverable. What this defect actually means is; 1 that it's impossible to template inside an indented heredoc string, and 2 that templating in an un-indented heredoc string will result in additional whitespace no matter what.

Could this (and the boolean operator short-circuit issue) not be fixed by adding a new opt-in field in the terraform block?

If you're not going to fix this then it needs to be documented with big warning signs as it regularly causes significant time wasting as someone tries to get the documented behaviour to happen.

danlsgiga commented 1 year ago

Facing the same issue when using <<-EOF for a terraform template to initialize configs in ECS. Everything works as expected until it enters the for loop, then the whitespaces/newlines stripping is not applied.

kbolino commented 11 months ago

I think it would be better to change some userdata scripts and cause some EC2 instances to have deltas in a plan, than to keep a confusingly broken templating system as a core "compatibility guarantee" for the language. If people have set up their IaC to auto-approve all plans and also haven't set prevent_destroy on affected resources, then they should be anticipating and ready to handle changes like this (and if not, well, how far are you going to go to hold the hand of people doing the wrong thing on multiple levels?).

It's also quite possible that, like with the Go loopvar behavior change, the vast majority of userdata scripts (and similar) that would be changed should change because they're already broken (not doing what was intended) but the faults are being silently ignored. After all, whitespace is significant in a lot of languages, including shell and YAML.

Moreover, the actual text of the compatibility promise seems to contradict leaving this unfixed:

"If the actual behavior of a feature differs from what we explicitly documented as the feature's behavior, we will usually treat that as a bug and change the feature to match the documentation, although we will avoid making such changes if they seem likely to cause broad compatibility problems. We cannot promise to always remain "bug-compatible" with previous releases, but we will consider such fixes carefully to minimize their impact."

As it stands, the only place the current behavior seems to be documented "officially" is this issue. The docs for Strings and Templates don't mention any caveats about indented heredocs and templates.