Fix interpolation pattern escaping in map variables (fixes #3368) #4681
Conversation
…io#3368) - Added escaping logic to ParseCtyValueToMap() in internal/ctyhelper/helper.go - Escape ${...} patterns to $${...} to prevent Terraform variable interpolation - Recursively handle nested maps, arrays, and complex object structures - Added comprehensive unit tests for various interpolation scenarios Root cause: When using type = map(string) variables, CTY value processing preserved ${...} patterns from file() content, but Terraform interpreted these as variable references instead of literal strings. Solution: Double the $ in ${...} patterns (${var} -> $${var}) at the CTY-to-map conversion stage, preventing Terraform from treating them as variable interpolation while preserving original content. Fixes gruntwork-io#3368: Terragrunt doesn't escape input variables in some situations 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughParseCtyValueToMap now post-processes the unmarshaled map to recursively escape Terraform interpolation patterns ("${...}") in string values by doubling the leading dollar sign. New helpers traverse maps and slices and provide an idempotent exported string-escaping function; tests assert behavior across nested structures and edge cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ParseCtyValueToMap
participant JSON
participant Escaper
Caller->>ParseCtyValueToMap: call with cty.Value
ParseCtyValueToMap->>JSON: marshal/unmarshal to map[string]any
ParseCtyValueToMap->>Escaper: recurse maps/slices to escape "${...}"
Escaper-->>ParseCtyValueToMap: return escaped map
ParseCtyValueToMap-->>Caller: return escaped map
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/ctyhelper/helper_test.go (1)
60-133: Great coverage; add tests to enforce idempotency and slice/object recursion.Please add:
- A case where input already contains "$${...}" to ensure we do not re-escape.
- A case covering slices that contain strings and nested maps.
These will catch the double-escape bug and guard future regressions.
Apply this diff to extend the table:
@@ tests := []struct { expected map[string]any input cty.Value name string }{ @@ { name: "no interpolation patterns", input: cty.ObjectVal(map[string]cty.Value{ "key": cty.StringVal("normal string"), "num": cty.NumberIntVal(42), "bool": cty.BoolVal(true), }), expected: map[string]any{ "key": "normal string", "num": float64(42), // JSON unmarshaling converts numbers to float64 "bool": true, }, }, + { + name: "already-escaped interpolation remains unchanged", + input: cty.ObjectVal(map[string]cty.Value{ + "key": cty.StringVal("$${bar}"), + }), + expected: map[string]any{ + "key": "$${bar}", + }, + }, + { + name: "arrays and nested maps are escaped", + input: cty.ObjectVal(map[string]cty.Value{ + "arr": cty.ListVal([]cty.Value{ + cty.StringVal("a ${b} c"), + cty.ObjectVal(map[string]cty.Value{ + "inner": cty.StringVal("${z}"), + }), + }), + }), + expected: map[string]any{ + "arr": []any{ + "a $${b} c", + map[string]any{"inner": "$${z}"}, + }, + }, + }, }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/ctyhelper/helper.go(2 hunks)internal/ctyhelper/helper_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit Configuration File
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
internal/ctyhelper/helper_test.gointernal/ctyhelper/helper.go
Addresses feedback from CodeRabbit AI review to prevent double-escaping
of already escaped ${...} patterns in input variables.
Changes:
- Replace direct string replacement with idempotent escapeInterpolationInString() helper
- Check if ${...} patterns are already escaped (preceded by $) before escaping
- Add comprehensive test coverage for idempotent behavior including:
* Already escaped patterns remain unchanged ($${var} stays $${var})
* Mixed escaped and unescaped patterns in same string
* Nested maps and arrays with mixed patterns
* Recursive escaping for complex data structures
Technical improvements:
- Use strings.Builder for efficient string construction
- Pre-allocate capacity for better performance
- Maintain backward compatibility while fixing edge cases
Fixes gruntwork-io#3368: Terragrunt doesn't escape input variables in some situations
🤖 Generated with Claude Code (https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/ctyhelper/helper.go (2)
53-75: Recursive map traversal and idempotent string escaping look correct. Minor prealloc nit.Logic is clean and easy to follow, and you correctly recurse into nested maps/slices. Small micro-optimization: preallocate the result map with capacity to avoid rehashing.
Apply this diff:
func escapeInterpolationPatterns(m map[string]any) map[string]any { - result := make(map[string]any) + result := make(map[string]any, len(m))
97-129: Idempotent escaper is correct. Optional: slightly improve builder preallocation and add explicit idempotency tests.Your byte-wise scan correctly avoids double-escaping ($${...} stays unchanged) and handles multiple patterns and edge cases. Two small follow-ups:
- Preallocation: add a bit of headroom to reduce reallocs when adding extra '$'.
- Tests: include explicit cases for idempotency and non-interpolation dollars (e.g., "$$5") if not already covered.
Apply this diff for preallocation:
- result.Grow(len(s)) // Pre-allocate capacity + // Pre-allocate with headroom to minimize reallocs when adding '$' + result.Grow(len(s) + strings.Count(s, "${"))If helpful, I can add a table-driven test like:
func TestEscapeInterpolationInString_IdempotentAndNonInterp(t *testing.T) { t.Parallel() cases := []struct{ in, want string }{ {"${foo}", "$${foo}"}, {"$${foo}", "$${foo}"}, // idempotent: already escaped {"${a} and ${b}", "$${a} and $${b}"}, {"no interpolation", "no interpolation"}, {"price is $$5", "price is $$5"}, // non-interp dollars unchanged {"$${${nested}}", "$${${nested}}"}, // tricky sequences } for _, tc := range cases { got := escapeInterpolationInString(tc.in) if got != tc.want { t.Fatalf("escapeInterpolationInString(%q) = %q; want %q", tc.in, got, tc.want) } // Idempotency: running twice should not change got2 := escapeInterpolationInString(got) if got2 != tc.want { t.Fatalf("idempotency failed for %q -> %q -> %q; want %q", tc.in, got, got2, tc.want) } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/ctyhelper/helper.go(2 hunks)internal/ctyhelper/helper_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/ctyhelper/helper_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit Configuration File
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
internal/ctyhelper/helper.go
🔇 Additional comments (2)
internal/ctyhelper/helper.go (2)
46-51: Applying escaping at the ParseCtyValueToMap boundary is the right fix.Escaping at this point ensures -var JSON values mirror Terragrunt’s literal semantics and prevents Terraform from interpreting ${...}. Good placement and minimal surface-area change.
77-96: Slice handler mirrors map logic appropriately.Covers strings, nested maps, and nested slices; non-string scalars are preserved as-is. Looks good.
Addresses CodeRabbit AI feedback to improve performance and test coverage: Performance optimizations: - Preallocate result map with capacity to avoid rehashing - Add headroom to string builder to minimize reallocations when adding '$' - Export EscapeInterpolationInString for direct testing Enhanced test coverage: - Add explicit idempotency tests verifying repeated escaping produces same result - Test edge cases: non-interpolation dollars ($5), nested sequences, empty strings - Verify mixed escaped/unescaped patterns work correctly - Ensure backward compatibility with all existing functionality Technical improvements: - More efficient memory allocation patterns - Better test coverage for edge cases and idempotent behavior - Maintains all existing functionality while improving performance 🤖 Generated with Claude Code (https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/ctyhelper/helper.go (1)
97-110: Minor perf nit: avoid an extra scan in EscapeInterpolationInString.You do an initial Contains scan plus Count plus the main loop. You can drop the Contains check and use Count to determine both the fast-path and capacity to reduce one extra pass.
Apply this diff:
- if !strings.Contains(s, "${") { - return s - } - - // Use a string builder for efficient string construction - var result strings.Builder - - // Pre-allocate with headroom to minimize reallocs when adding '$' - result.Grow(len(s) + strings.Count(s, "${")) + // Count occurrences once; fast-path when absent and use count for capacity + count := strings.Count(s, "${") + if count == 0 { + return s + } + + // Use a string builder for efficient string construction + var result strings.Builder + // Pre-allocate with headroom to minimize reallocs when adding '$' + result.Grow(len(s) + count)Also applies to: 111-129
internal/ctyhelper/helper_test.go (2)
60-176: Great coverage across nested maps, arrays, and mixed escaped/unescaped cases.This validates the exact scenarios from the issue and ensures idempotency at the map level. Consider adding an array-of-objects case to exercise recursion within slices more thoroughly.
Apply this diff to add an extra case:
@@ { name: "array with mixed escaped and unescaped patterns", input: cty.ObjectVal(map[string]cty.Value{ "items": cty.ListVal([]cty.Value{ cty.StringVal("$${already_escaped}"), cty.StringVal("${needs_escaping}"), cty.StringVal("normal string"), }), }), expected: map[string]any{ "items": []any{ "$${already_escaped}", "$${needs_escaping}", "normal string", }, }, }, + { + name: "array of objects with nested strings", + input: cty.ObjectVal(map[string]cty.Value{ + "items": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{"k": cty.StringVal("${v}")}), + cty.ObjectVal(map[string]cty.Value{"k": cty.StringVal("$${v}")}), + }), + }), + expected: map[string]any{ + "items": []any{ + map[string]any{"k": "$${v}"}, + map[string]any{"k": "$${v}"}, + }, + }, + },
178-214: Nice idempotency and edge-case assertions for EscapeInterpolationInString.Consider adding one more case to capture triple-dollar before interpolation to guard against regressions.
Apply this diff:
@@ cases := []struct { name string in string want string }{ {"simple interpolation", "${foo}", "$${foo}"}, {"already escaped stays unchanged", "$${foo}", "$${foo}"}, {"multiple patterns", "${a} and ${b}", "$${a} and $${b}"}, {"no interpolation", "no interpolation", "no interpolation"}, {"non-interp dollars unchanged", "price is $5", "price is $5"}, {"nested sequences", "${${nested}}", "$${$${nested}}"}, {"mixed escaped and unescaped", "$${escaped} and ${unescaped}", "$${escaped} and $${unescaped}"}, {"dollar at end", "test$", "test$"}, {"just dollar sign", "$", "$"}, {"empty string", "", ""}, + {"triple dollar before interp", "$$${foo}", "$$${foo}"}, }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/ctyhelper/helper.go(2 hunks)internal/ctyhelper/helper_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit Configuration File
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
internal/ctyhelper/helper_test.gointernal/ctyhelper/helper.go
🧬 Code Graph Analysis (1)
internal/ctyhelper/helper_test.go (1)
internal/ctyhelper/helper.go (2)
ParseCtyValueToMap(24-51)EscapeInterpolationInString(100-130)
🔇 Additional comments (4)
internal/ctyhelper/helper.go (4)
8-8: LGTM: strings import is appropriate.Required by the new escaping helpers.
46-51: Correct place to apply escaping in ParseCtyValueToMap.Applying escaping after CTY→JSON→map conversion ensures Terraform won’t interpolate on -var JSON. This aligns with the PR objective and avoids changing upstream CTY semantics.
53-75: Recursive map escaper reads cleanly and is idempotent via EscapeInterpolationInString.Covers strings, nested maps, and delegates arrays. Retains types for non-strings. Good separation of concerns.
77-95: Slice handler complements map traversal and handles nested arrays/maps.Solid coverage for nested collections; preserves original non-string elements.
Addresses latest CodeRabbit AI feedback for micro-optimizations and edge cases:
Performance improvements:
- Eliminate redundant string scan in EscapeInterpolationInString
- Use single strings.Count() call for both fast-path check and capacity allocation
- Reduces from 3 string scans (Contains + Count + main loop) to 2 (Count + main loop)
Enhanced test coverage:
- Add array-of-objects test case to validate nested recursion within slices
- Add triple-dollar regression test ($$$${foo}) to guard against edge cases
- Comprehensive validation of complex nested data structures
Technical optimizations:
- More efficient string scanning with single-pass count check
- Better memory allocation patterns with precise capacity calculation
- Maintains 100% backward compatibility and idempotent behavior
All tests pass including original issue reproduction and new edge cases.
🤖 Generated with Claude Code (https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Description
Fixes #3368: Terragrunt doesn't escape input variables in some situations
This PR fixes a bug where Terragrunt incorrectly handled Terraform interpolation expressions (
${...}) in input variables when they appeared in map values loaded from files. The issue occurred because of how and when Terraform processes variable values with different types.When Variable Interpolation Happens:
The interpolation occurs when Terraform processes the variables that Terragrunt passes to it via
-varcommand line arguments, not during Terragrunt's own processing:file("stuff.txt")→ reads${bar}as literal string${bar}${bar}-var 'mymap={"foo":"${bar}"}'→ JSON string containing${bar}-varJSON value → THIS IS WHERE INTERPOLATION HAPPENSWhy
-varvs-var-fileMatters:This issue is specific to Terragrunt's use of
-vararguments with JSON strings. If Terragrunt used-var-filewith JSON files instead, this problem wouldn't occur because:-varwith JSON: Terraform parses{"foo":"${bar}"}as structured data where each value is an expression context that triggers interpolation-var-filewith JSON: Content like{"foo":"${bar}"}in a JSON file would be treated as literal data where${bar}remains a stringExample demonstrating the difference:
-var(fails):terraform plan -var stuff='{"simple":"${foo}"}'→ Error: "Variables not allowed"-var-file(works): Same content{"stuff": {"simple": "${foo}"}}in a JSON file → SuccessTerragrunt chose the
-varapproach for performance and consistency reasons, making the escaping fix the appropriate solution.Why Maps Behave Differently:
type = string: Terraform treats the entire value as a single string literal where${bar}is processed as-is without interpolationtype = map(string): Terraform parses the JSON structure{"foo": "${bar}"}and during JSON parsing, Terraform sees${bar}in a structured expression context where it tries to resolvevar.baras a variable interpolationRoot Cause & Solution:
The issue was in
ParseCtyValueToMap()function ininternal/ctyhelper/helper.go. When Terragrunt converted CTY values to Go maps (which then get JSON-marshaled), it wasn't escaping the${...}patterns.Why We Made the Change Here:
$${produces a literal${in the outputChanges Made:
ParseCtyValueToMap()ininternal/ctyhelper/helper.go${...}patterns to$${...}to prevent Terraform variable interpolation🤖 Partially Generated using Claude Code
TODOs
Read the Gruntwork contribution guidelines.
Note: This code is authored by myself with the use of Claude AI.
Does not apply.
Does not apply.
Release Notes (draft)
Fixed: Terragrunt now properly escapes Terraform interpolation expressions (
${...}) in map-typed input variables loaded from files, preventing "Variables not allowed" errors when using functions likefile()with content containing${...}patterns.Migration Guide
No migration required. This is a bug fix that maintains backward compatibility. The fix automatically applies to all existing configurations that previously failed with "Variables not allowed" errors when using map-typed variables containing
${...}patterns. All previously working configurations continue to work unchanged.Summary by CodeRabbit
Bug Fixes
New Features
Tests