Azure / Azure/golden

Top-level for_each instances share mutable HCL AST and corrupt nested block values

Open
#92 0 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.