Top-level for_each instances share mutable HCL AST and corrupt nested block values
- Dominant language
- Go
- Stars
- 3
- Forks
- 13
- PR merge metrics
- No merged PRs in 30d
Description
## Summary
Top-level `for_each` expansion creates distinct Golden block wrappers but reuses the same mutable `*hclsyntax.Block` tree. Planning one expanded instance rewrites nested-block expressions in that shared AST, so other instances can decode the wrong `each.value`.
This reproduces on Azure/golden `main` at commit [`6e9a3fc2760e6f8440e4dfe8d0b1360886588bdd`](https://github.com/Azure/golden/commit/6e9a3fc2760e6f8440e4dfe8d0b1360886588bdd). It reproduces with the serial planner; concurrency is not required.
## Minimal reproduction
This test uses Golden's existing `DummyResource` and `SecondNestedBlock` fixtures and can be added to `config_test.go`:
```go
func TestForEachStaticNestedBlockUsesInstanceContext(t *testing.T) {
testBase := newTestBase()
defer testBase.teardown()
testBase.dummyFsWithFiles(map[string]string{
"test.hcl": `
resource "dummy" "expanded" {
for_each = {
one = "first"
two = "second"
three = "third"
}
nested_block {
id = 1
name = each.value
}
}
`,
})
config, err := BuildDummyConfig("", "", nil, nil)
require.NoError(t, err)
plan, err := RunDummyPlan(config)
require.NoError(t, err)
require.Len(t, plan.Resources, 3)
got := make(map[string]string, 3)
for _, resource := range plan.Resources {
block := resource.(*DummyResource)
require.Len(t, block.NestedBlocks, 1)
got[block.Address()] = block.NestedBlocks[0].Name
}
assert.Equal(t, map[string]string{
"resource.dummy.expanded[one]": "first",
"resource.dummy.expanded[two]": "second",
"resource.dummy.expanded[three]": "third",
}, got)
}
```
Run:
```console
go test . -run '^TestForEachStaticNestedBlockUsesInstanceContext$' -count=1 -v
```
## Actual behavior
All three instances decoded the value from the instance that mutated the shared AST first in this run:
```text
expected:
resource.dummy.expanded[one]: first
resource.dummy.expanded[two]: second
resource.dummy.expanded[three]: third
actual:
resource.dummy.expanded[one]: second
resource.dummy.expanded[two]: second
resource.dummy.expanded[three]: second
```
The exact leaked value depends on planning order. A planner that evaluates ready blocks concurrently would additionally make the result nondeterministic and introduce concurrent writes to the shared AST.
## Expected behavior
Each expanded top-level block must evaluate all top-level and nested expressions using its own `each` object:
```text
expanded[one] -> first
expanded[two] -> second
expanded[three] -> third
```
Planning one instance must not mutate the source AST observed by another instance.
## Root cause
`BaseConfig.expandBlock` passes the same syntax block pointer to every expanded instance:
```go
newBlock := NewHclBlock(hclBlock.Block, hclBlock.wb, NewForEach(key, value))
```
https://github.com/Azure/golden/blob/6e9a3fc2760e6f8440e4dfe8d0b1360886588bdd/base_config.go#L314-L343
`NewHclBlock` creates a new wrapper, but its embedded `*hclsyntax.Block`, body, and nested syntax blocks still alias the original tree.
`ExpandDynamicBlocks` then creates another wrapper with the same block pointer and eventually replaces the shared parent's nested-block slice:
```go
newHb := &HclBlock{
Block: hb.Block,
// ...
}
// ...
newHb.Body.Blocks = newNestedBlocks
```
https://github.com/Azure/golden/blob/6e9a3fc2760e6f8440e4dfe8d0b1360886588bdd/hcl_block.go#L67-L135
Even a static nested block passes through this method. `evaluateAttributes` evaluates its attributes and replaces their expressions with literal expressions in place:
```go
hb.Body.Attributes[attributeName] = &hclsyntax.Attribute{
Name: attributeName,
Expr: &hclsyntax.LiteralValueExpr{Val: v},
// ...
}
```
https://github.com/Azure/golden/blob/6e9a3fc2760e6f8440e4dfe8d0b1360886588bdd/hcl_block.go#L264-L277
After the first instance turns `name = each.value` into a literal such as `name = "second"`, later instances evaluate that literal rather than their own `each.value`.
The existing tests cover top-level `for_each` expressions and nested/dynamic blocks independently, but not their intersection: top-level `for_each` plus an instance-dependent expression in a nested block.
## Proposed fix
### 1. Give every top-level `for_each` instance an independent AST
Golden already has `CloneHclBlock`, which recursively clones the `hclsyntax.Block`, body, attributes, and nested blocks. Use it when expanding a top-level block instead of wrapping the original syntax pointer. Conceptually:
```go
newBlock := CloneHclBlock(hclBlock)
newBlock.ForEach = NewForEach(key, value)
```
Then pass `newBlock` to `wrapBlock`. Please also verify that any `hclwrite.Block` state that may be mutated is independently owned; sharing is acceptable only for data treated as immutable.
A shallow copy of only the outer `hclsyntax.Block` is insufficient because `Body`, the attributes map, the nested-block slice, and nested blocks are reference-bearing fields.
### 2. Make dynamic expansion non-mutating with respect to its input
`ExpandDynamicBlocks` should construct and return a fresh block/body tree and evaluate attributes only on that tree. It should not assign through an input-owned `Body` such as:
```go
newHb.Body.Blocks = newNestedBlocks
```
This makes the ownership contract explicit and prevents similar aliasing bugs outside top-level `for_each`.
## Suggested regression coverage
1. Add the serial behavioral test above. It fails reliably without depending on goroutine timing.
2. Add a case with multiple nested levels and `each.value` in a nested attribute.
3. Add a case combining top-level `for_each` with a `dynamic` nested block.
4. Assert that expanding/planning one instance does not mutate the source HCL AST used by another instance.
5. If/when parallel planning is supported upstream, run the same cases with parallelism greater than one under `go test -race`.
## Impact
Any Golden consumer using top-level `for_each` with instance-dependent expressions inside static nested blocks can silently receive another instance's value. This is data corruption rather than only a diagnostic-quality issue, and it can redirect paths, policies, nested resource settings, or other per-instance configuration.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in base_config.go at expandBlock and in hcl_block.go at CloneHclBlock, ExpandDynamicBlocks, and evaluateAttributes. Run the provided config_test.go regression command to reproduce the shared-AST behavior. Done means each top-level for_each instance has independent nested values and expansion does not mutate another instance's source AST.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100