Repository navigation
fix(lib): escape object keys in expressions and HCL maps - #569
eduardomourar wants to merge 1 commit into
Conversation
jsteinich
left a comment
There was a problem hiding this comment.
Thanks for pulling all three call sites through one helper, and for catching the \b/\f detail. This review combines two independent passes.
I ran the change against real Terraform 1.14.3 and OpenTofu 1.12.6, on JSON and HCL output, using validate, apply and output -json. It fixes keys containing quotes, backslashes, newlines, Windows paths, trailing backslashes and \b/\f. Every one of those failed validate on main, and now they apply with exactly the intended key. Existing snapshots are untouched, and keys that were already valid (spaces, dots, a leading digit, unicode, plain references) render byte-identical.
Blocking: wrapIdentifierInQuotesIfNeeded (packages/cdktn/src/hcl/render.ts:22-24)
Both problems below have the same cause. By the time renderMap / renderFuzzyJsonObject see a key, a ${…} that came from a resolved token looks exactly like one the user typed into an ordinary object key. quotedStringKey(key, true) treats every quoted key the same way, so it gets each kind wrong in one direction.
1. Regression: map keys built from a function token with a string literal no longer parse. The body of ${…} is expression syntax, but JSON.stringify escapes the quotes inside it:
# main: valid, key = "abc"
"${lower("ABC")}" = "v"
# this PR: Error: Invalid character (Terraform and OpenTofu)
"${lower(\"ABC\")}" = "v"This happens with TerraformLocal(stack, "m", { [Fn.lower("ABC")]: "v" }), and also with Fn.format("%s-x", [res.id]) and Fn.join("-", ["k", res.id]), in locals and in typed map attributes. All of these validated and applied on main. JSON output is unaffected.
2. Literal keys containing template syntax stay executable. An ordinary key such as ${1+1} renders as "${1+1}" = "x", and Terraform evaluates it to "2" instead of keeping the text. %{…} stays a directive. The test at packages/cdktn/test/quoted-string-key.test.ts:52-58 expects raw "${reference}" for a plain JavaScript key that never came from a token, so the test encodes this ambiguity.
One caveat on 2: this predates the PR, and HCL output currently agrees with JSON output. The same stack synthesized to cdk.tf.json also evaluates the key to "2" (checked on both CLIs), because Terraform JSON reads object property names as templates. Hand-written interpolation keys such as "pre-${var.p}" work today in both formats. So escaping ${/%{ in the HCL renderer alone would make synth and synth --hcl disagree, and it would break that pattern in HCL only. If 2 is fixed, it needs provenance: decide literal vs token before resolution, for example with Token.isUnresolved(key) on the original object, and apply that to both output formats. Since that changes JSON output for existing users, it probably wants its own change behind a feature flag.
For this PR, the smallest fix for 1 is to leave keys that already contain ${ on the old path:
function wrapIdentifierInQuotesIfNeeded(key: string): string {
if (!/(^\d)|[^A-Za-z0-9_-]/.test(key)) return key;
if (key.includes("${")) return `"${key}"`;
return quotedStringKey(key, true);
}Escaping only the text outside ${…} spans would be more complete, and #468 needs that same escaper for values. Please replace the ${reference} test case with real token keys: Fn.lower("ABC") (quotes inside the reference) and res.id, through Testing.synthHcl.
Behaviour change: raw ${…} keys inside Fn.* object arguments (packages/cdktn/src/tfExpression.ts:75)
In expressions the PR escapes literal ${, which matches how values are already escaped there and the Token.asAny branch from #296. That is the right direction, but it silently changes configs that apply today:
Fn.keys({ "${terraform_data.src.id}": "v" })
// main: ["<the id>"] this PR: ["${terraform_data.src.id}"]That raw-string form is currently the only way to get a dynamic key into an object passed to a Fn.* call. Real token keys there are broken on main and on this branch: Fn.merge([{ [res.id]: "v" }]) renders merge({"terraform_data.src.id" = null}). The key resolves to bare expression text, and arg[key] misses, so the value becomes null. Could you fix token keys in the same change? Iterate Object.keys(arg) and emit (${this.resolveArg(context, k)}) = … for unresolved keys. HCL accepts parenthesised key expressions, and {(terraform_data.src.id) = "v", ("pre-${terraform_data.src.id}") = "w"} applies correctly on both CLIs. If you'd rather keep this PR narrow, please call the change out in the description, and we'll track token keys separately.
Smaller points
- Tests: the HCL tests call
renderMapdirectly, so the locals path (renderFuzzyJsonObject), which this PR also changes, isn't covered, and nothing goes throughTesting.synthHcl. The expression tests use the internalcall("lookup", …); a publicFn.keys/Fn.mergerepro would be better. - Naming:
quotedStringKeyis really a general HCL quoted-string literal, and values need it too (#468). A name and location outsidehcl/would invite reuse, sincetfExpression.tsuses it for JSON output as well.
Related PRs (for maintainers)
- #520:
escapeDefaultLiteralsalready escapes keys. With both PRs merged, three of itstype-expr.test.tscases double-escape ({"a\"b" = …}becomes{"a\\\"b" = …}, and$${k}becomes$$${k}). Whichever merges second should drop that key branch. There is no textual conflict. - #501: its value escaping disagrees with this helper. In the same object literal, the key
a\"bkeeps its backslash while the value loses it, and%{is escaped in keys but not in values. The value side should reuse this helper. - #541: different hunk of
render.ts, no interaction.
What was checked
Tested shapes: quote, backslash, Windows path, trailing backslash, newline, tab, \b, \f, \u0001, unicode, literal ${…} / %{…} / $${…}, space, dotted, leading digit, token reference, function tokens with quotes and backslashes, mixed literal + token.
Each key was placed in locals, a terraform_data.input map, a variable default, Fn.keys({...}) and Fn.keys(Token.asAny({...})). Each stack was synthesized to JSON and HCL on main and on this branch, then run through init / validate / apply / output -json on Terraform 1.14.3 and OpenTofu 1.12.6. The two CLIs agreed on every case.
I also ran the full cdktn unit suite on this branch (all 302 snapshots unchanged), merged the branch with #520 and #501 to check how they interact, and hand-checked the parenthesised token-key form on both CLIs.
Not checked: older Terraform versions, the integration suite, or packaged builds.
4f81e1c to
11495d4
Compare
|
All addressed, except for:
|
jsteinich
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround on 11495d4. Most of the first review is resolved:
- HCL map keys built from function tokens (
Fn.lower("ABC"),Fn.format,Fn.join, iterator keys such aseach.value["my-tag"]) render byte-identical tomainagain and apply with the right key. - Token keys inside
Fn.*object arguments now work through the parenthesised key form.Fn.merge([{ [res.id]: "v" }])returns{"<id>": "v"}, wheremainreturned{"terraform_data.src.id": null}. - The tests now go through
Testing.synthHcl. All 302 existing snapshots are unchanged, and keys that were already valid render byte-identical. - Deferring the naming nit and the
${1+1}question is fine.
A second pass found two more problems in the new code, so I'm keeping this at request changes. Everything below was checked with validate and apply on Terraform 1.14.3 and OpenTofu 1.12.6, in JSON and HCL output. The two CLIs agreed on every case.
Blocking
1. A value that resolves to undefined now renders as null instead of being omitted (packages/cdktn/src/tfExpression.ts:78-82).
.filter((key) => arg[key] !== undefined) only catches a literal undefined. A Lazy or other resolvable that resolves to undefined passes the filter, and resolveArg() then renders it as null:
Fn.merge([{ key: Lazy.anyValue({ produce: () => undefined }), b: "x" }])
// main: {"b": "x"} this PR: {"b": "x", "key": null}This is a regression from main in both output formats. Filtering on the resolved value while keeping the original key for token detection fixes it, for example .filter((key) => context.resolve(arg[key]) !== undefined). Please add a test with a property whose Lazy value resolves to undefined.
2. Mixed literal + token keys containing a quote or backslash produce invalid HCL (packages/cdktn/src/tfExpression.ts:121-129).
resolveObjectKey() sends any key that contains a token through resolveArg() → resolveString(). That path escapes newlines and ${ in the literal fragments, but not quotes or backslashes:
| key | this PR | result (both CLIs) |
|---|---|---|
`prefix"-${res.id}` |
("prefix"-${terraform_data.src.id}") |
Invalid character |
`a\\b-${res.id}` |
("a\b-${terraform_data.src.id}") |
Invalid escape sequence |
`C:\\temp\\${res.id}` |
("C:\temp\${terraform_data.src.id}") |
Invalid escape sequence |
On main the first two were also invalid, and the last applied with a wrong, tab-mangled literal key. So these keys still don't work, and the Windows-path shape goes from silently wrong to a hard error. Since this PR introduces the mixed-key path, please build the key from its fragments: escape each literal fragment with the same rules as quotedStringKey (without the surrounding quotes), and emit each token as ${…}. Tests should cover a quote and a backslash around a token.
Non-blocking
3. Literal %{…} in HCL map keys stays an active directive (packages/cdktn/src/hcl/render.ts:32). quotedStringKey(key, true) skips %{ escaping, so the map key %{if true}x%{endif} evaluates to x instead of staying literal. That's accurate, and %{ isn't needed to keep token references working. But it behaves the same on main, and the JSON output of the same stack evaluates the key to x as well, because Terraform JSON reads object property names as templates. Escaping %{ only in the HCL renderer would make synth and synth --hcl disagree. It belongs with the deferred ${1+1} change, which would escape literal template syntax in keys in both formats behind a flag. I'm happy to take that as a follow-up rather than block on it here. If you do want to escape it in HCL now, please make the JSON side match in the same change.
Also, please add a line to the PR description saying that literal ${…} keys in Fn.* objects are now escaped. Fn.keys({ "${terraform_data.src.id}": "v" }) used to evaluate the reference, and existing configs will see a plan diff.
11495d4 to
0c3a850
Compare
|
All addressed now. |
Related issue
Fixes #350
Description
Serialize keys through one shared quoted-string helper. Use Unicode escapes for backspace and form feed. HCL map keys keep ${...} so token references stay interpolations.
A key built from a token now renders as a parenthesised key expression, e.g.
(resource.id) = "v". Afragment with a tokenescapes the literal fragment with the same rules as a plain string key, then emits the token as `${..A property whose value io
undefinedis nowomitted, matching the existing behavior for a literalundefinedvalue.Behavior change: a literal
${...}key in anFn.*object argument is now escaped and kept as ated as an interpolation.For example,Fn.keys({ "${resource.id}": "v" })used to resolve the reference; it now keeps configs that relied on the old evaluation will see a plan diff.Checklist