apparentlymart
commented
4 months ago Hi @jacobbednarz! Thanks for reporting this.
I was able to reproduce the problem using files on disk too. I created a directory with a file containing the following content:
resource "foo" "%[1]s" {}After running terraform fmt in that directory, it was rewritten like this:
resource "foo" {}I agree that this is incorrect behavior, but since "%[1]s" is not a valid resource name I would have expected this to return a syntax error rather than succeeding with that invalid name preserved, because the design intent for terraform fmt is for it to leave files containing syntax errors totally unchanged to avoid potentially making the syntax error even worse (which it seems to have done in this case, for some reason).
Therefore I think the correct behavior here would be to return the following error message:
╷
│ Error: Invalid resource name
│
│ on fmt-invalid-resource-name.tf line 2, in resource "foo" "%[1]s":
│ 2: resource "foo" "%[1]s" {}
│
│ A name must start with a letter or underscore and may contain only
│ letters, digits, underscores, and dashes.
╵From reading the code, I think what we have here is a historical accident: when we originally wrote the terraform fmt implementation it was literally just a wrapper around HCL's own formatter, with no Terraform-specific rules. At that time it made sense for us to use HCL's definition of "valid syntax", rather than Terraform's more prescriptive interpretation.
In the meantime we've introduced into terraform fmt a number of Terraform-specific rules, including the one that's causing the problem here: it rewrites a block header like resource foo bar into the idiomatic style resource "foo" "bar".
All of the terraform fmt rules are written under the assumption that the input has already been checked to be valid syntax, but it seems that we forgot to also change the definition of "valid syntax" to include Terraform's own rules, and so we're letting invalid syntax reach that formatting rule and then it doesn't know how to handle the invalid tokens it found in this block header and so apparently just skipped generating anything at all for that second block label.
My first suggested fix would be to change the following code so that it calls Terraform's own logic for loading and decoding the configuration for a single file:
That would then raise exactly the error I described above. However, we should consider carefully whether this would be a breaking change, since it will make terraform fmt reject considerably more invalid input with an error than it does today. It's debatable whether it's actually valuable to apply formatting to a file that isn't valid, so we might convince ourselves that this is an acceptable pragmatic compromise, but that's something we'd need to discuss further.
With all of that said, it does seem like something is wrong in hclwrite here. As pointed out in the original writeup, HCL shouldn't understand the characters in that block label as being anything other than a literal string; the rule that it can contain only identifier-safe characters is Terraform's rule.
The rewrite that terraform fmt is doing here is relying entirely on hclwrite behavior, without doing anything Terraform-specific:
Therefore it seems like either block.Labels incorrectly returned only one label or block.SetLabels incorrectly ignored the second label it was passed.
I've not yet proven it with running code, but I suspect this is a buggy interaction between an edge case of the main hclsyntax parser and the block labels logic in hclwrite:
IIRC in some cases HCL decodes a quoted literal as a sequence of separate literal tokens just due to a quirk of how the tokenizer works, and so it might be interpreting "%[1]s" as an open quote followed by a literal "%" followed by a literal [1]s followed by a close quote.
When such an expression gets evaluated normally that quirk is invisible: the two literals just get concatenated together and the result is the single string %[1]s.
But hclwrite works with the individual tokens, and its implementation of block.Labels seems to assume that there will only ever be zero or one literal tokens between the quotes:
If I'm right about how the tokenizer is dealing with this, then we could improve it by making that logic tolerate zero or more literal strings between the quotes, and concatenate their results together in the same way that expression evaluation would.
That solution would not have any compatibility implications and would make the system behave as in "Expected Behavior" in the original issue, but does not address the more general problem that terraform fmt may be trying to apply other rules that expect valid Terraform syntax without actually checking whether the input is valid Terraform syntax first. The original idea I proposed above would make sure that terraform fmt's rules cannot possibly choke on any invalid Terraform-specific input, and this example is invalid from Terraform's perspective.
I suppose the most ideal thing would therefore to be make both of the changes I described: the first one would make the hclwrite bug irrelevant from Terraform's perspective because it would prevent us from reaching the hclwrite logic when the input isn't valid Terraform language syntax anyway, whereas the hclwrite bugfix I described would make this work correctly for hclwrite callers other than terraform fmt that might not impose the same rules as Terraform does on what's valid in a block label.
jacobbednarz
Terraform Version
Terraform Configuration Files
No configuration needed
Debug Output
Expected Behavior
Output should be
resource "foo" "%[1]s" {}Actual Behavior
resource "foo" {}Steps to Reproduce
echo 'resource "foo" "%[1]s" {}' | terraform fmt -Additional Context
I suspect this may be related to string directive expressions which are used for evaluating conditionals however, I think
%[...]sshouldn't be an catching and getting removed here as it is a valid Go template placeholder without an overlap in HCL (at least to my knowledge).References
No response