terraform fmt removing resource name when % in value

[jacobbednarz]

Terraform Version

v1.0.11 through to v1.9.0. Reproduction below is 1.9.0.

Terraform Configuration Files

No configuration needed

Debug Output

2024-07-03T10:29:31.570+1000 [INFO]  Terraform version: 1.9.0
2024-07-03T10:29:31.570+1000 [DEBUG] using github.com/hashicorp/go-tfe v1.51.0
2024-07-03T10:29:31.570+1000 [DEBUG] using github.com/hashicorp/hcl/v2 v2.20.0
2024-07-03T10:29:31.570+1000 [DEBUG] using github.com/hashicorp/terraform-svchost v0.1.1
2024-07-03T10:29:31.570+1000 [DEBUG] using github.com/zclconf/go-cty v1.14.4
2024-07-03T10:29:31.570+1000 [INFO]  Go runtime version: go1.22.4
2024-07-03T10:29:31.570+1000 [INFO]  CLI args: []string{"terraform", "fmt", "-"}
2024-07-03T10:29:31.570+1000 [TRACE] Stdout is a terminal of width 283
2024-07-03T10:29:31.570+1000 [TRACE] Stderr is a terminal of width 283
2024-07-03T10:29:31.570+1000 [TRACE] Stdin is not a terminal
2024-07-03T10:29:31.570+1000 [DEBUG] Attempting to open CLI config file: /Users/jacob/.terraformrc
2024-07-03T10:29:31.570+1000 [DEBUG] File doesn't exist, but doesn't need to. Ignoring.
2024-07-03T10:29:31.571+1000 [DEBUG] ignoring non-existing provider search directory terraform.d/plugins
2024-07-03T10:29:31.571+1000 [DEBUG] ignoring non-existing provider search directory /Users/jacob/.terraform.d/plugins
2024-07-03T10:29:31.571+1000 [DEBUG] ignoring non-existing provider search directory /Users/jacob/Library/Application Support/io.terraform/plugins
2024-07-03T10:29:31.571+1000 [DEBUG] ignoring non-existing provider search directory /Library/Application Support/io.terraform/plugins
2024-07-03T10:29:31.571+1000 [INFO]  CLI command args: []string{"fmt", "-"}
2024-07-03T10:29:31.571+1000 [TRACE] terraform fmt: Formatting <stdin>
resource "foo" {}

Expected Behavior

Output should be resource "foo" "%[1]s" {}

Actual Behavior

resource "foo" {}

Steps to Reproduce

1. Run 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 %[...]s shouldn'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

[apparentlymart]

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:

terraform/internal/command/fmt.go

Lines 187 to 194 in c19118f

	// File must be parseable as HCL native syntax before we'll try to format
	// it. If not, the formatter is likely to make drastic changes that would
	// be hard for the user to undo.
	_, syntaxDiags := hclsyntax.ParseConfig(src, path, hcl.Pos{Line: 1, Column: 1})
	if syntaxDiags.HasErrors() {
	diags = diags.Append(syntaxDiags)
	return diags
	}

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:

terraform/internal/command/fmt.go

Lines 319 to 322 in c19118f

	// Normalize the label formatting, removing any weird stuff like
	// interleaved inline comments and using the idiomatic quoted
	// label syntax.
	block.SetLabels(block.Labels())

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:

https://github.com/hashicorp/hcl/blob/360ae579460fab69e9939599af85c8f255a59007/hclwrite/ast_block.go#L153-L171

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]

Thanks for the deep dive into this and I'm largely onboard with the line of thinking here. I'll leave the detective work up of isolating the correct boundaries to someone more knowledgeable in the interactions at play here however, I do have some thoughts on introducing some change to move this forward.

I totally agree that fixing this (whatever shape that may take) given the behaviour now is confusing. The bit that I'd like to see a bit more flexibility in is how strict we are with the validation.

The reason this came about for me is that we're moving our test configurations (the HCL used in assertions) to their own dedicated .tf files so that they are not sprinkled within Go files. This allows us to do better validation of the configurations and with some tooling, turn these into live, runnable examples that we can reference elsewhere. It also enables clearer integration with language servers from editors as you aren't trying to interpret HCL inside of a Go string and can evaluate the two languages separately.

For me, a good middle ground would be separating these two issues (strict validation, potentially missing labels/parser issues) and addressing them individually which in my view, also gets a nice in between where the example syntax like this can be used but also allow operators to be stricter with their validation if they would like to. I would put the stricter validation behind a new flag -- my very creative suggestion being -strict or -mode strict) -- which for now is not enforced but can be toggled on. The intent being that in the next major version, it becomes the default to weed out any Terraform specific formatting bugs.

[apparentlymart]

Hi @jacobbednarz,

I was reflecting on what I'd said here overnight and independently reached a conclusion that I think you will find more agreeable, if I'm understanding your comment correctly.

In my comment yesterday I was thinking back to the earliest versions of modern terraform fmt (the rewrite of it that we did for Terraform v0.12) and how initially it was just trying to make sense of whatever jumble of tokens it was given, regardless of whether those tokens were in a sensible order in the input, which caused some very drastic counterproductive formatting in the presence of ambiguous input such as missing open/closing bracketing tokens. Anyone using a "format on save" approach would find that if they saved changes while in the middle of typing something the tool would select incorrect indentation and/or alignment for everything after the cursor position, making further edits very confusing.

We resolved that with the decision that the tool would do nothing unless the input was already valid syntax, as I was describing yesterday. However, in retrospect my yesterday comment proposed to apply that rule too strongly: the Terraform-specific formatting rules cannot create HCL-level syntax ambiguity because Terraform's language is built in terms of HCL's grammar. Only HCL grammar features contribute to the indentation and alignment decisions, and so Terraform's own concerns cannot have such a broad impact on the larger document.

Therefore I think the rule should be:

• HCL syntax parsing must succeed for any formatting to be done, but successful Terraform deciding is not required. If HCL-level parsing fails then that's an error, just like today.
• Terraform's own formatting rules must tolerate the input syntax being invalid and should not make any changes in that case, but should not return any errors. This should be scoped only to the specific construct that the rule relates to, so for example an invalid block header would get preserved as-is into the result but would still be subjected to the global rules about indentation, spacing, and alignment, and it wouldn't cause block headers elsewhere in the document to be left unformatted.
• The meaning of the result must always be equivalent to the meaning of the input. terraform fmt's mission is to present the same information in a more idiomatic way, not to change how other parts of Terraform would understand the input.

Since what you shared is valid HCL syntax despite being invalid Terraform syntax, the above rules would cause the behavior you expected in this particular case. We can get there by fixing the bug I pointed out in hclwrite, so that it handles the block labels in the same way that structural decoding would handle it.

However, I do still want to caution that terraform fmt is a tool for formatting actual Terraform source files that are going to be interpreted directly by Terraform, and not for formatting a template that some other software will use to generate those. While I expect this particular case will work, I cannot promise that your general strategy will work in all cases. A more typical strategy I've seen for Terraform code generation like this is to wait until after the final valid .tf files have been generated and then format those, so that the generated result is correctly formatted even if the input template wasn't. If you want to continue formatting your template files then you will need to design your template syntax carefully so that any valid template is also valid HCL syntax.

[jacobbednarz]

That all makes total sense to me! If we get any wilder with the templating, i.e. outside of just passing in string values, we're probably best off moving to a full templating solution anyway.