feat(terraform): prune resources outside the target block closure - #74
feat(terraform): prune resources outside the target block closure#74PushTheLimit wants to merge 7 commits into
Conversation
EvaluateAll evaluates every resource and module in a module on each call. Callers that only need a subset of a module's output (for example coder/preview, which computes a workspace's input parameters) pay to evaluate resources whose values can never affect that output. OptionWithResourceClosure(targetTypes) restricts root-module evaluation to the resource blocks reachable, via references, from the given target block types (matched on a block's type label, e.g. "coder_parameter"). Every non-resource block is retained and submodules are evaluated in full, so the computed values of the target blocks are unchanged; only resources that nothing in the target closure references are dropped. The closure is conservative: a reference that cannot be resolved to a concrete block keeps the matching blocks, so a resource is excluded only when nothing a target block reads can reference it. Empty targetTypes disables the behavior (default).
fa2a879 to
3916002
Compare
Preview evaluates the entire Terraform module graph on every call, even though rendering a workspace form only needs coder_parameter, coder_workspace_preset and coder_workspace_tags (and what they reference). The resources a workspace would create cannot feed those blocks, so evaluating them is wasted work that dominates request latency on large templates. Pass OptionWithResourceClosure with the three target block types so the parser drops root-module resources that nothing in that closure references. On a real template this cuts EvaluateAll from ~2s to ~0.16s (~12x) with byte-identical parameters, presets and tags. Depends on the OptionWithResourceClosure addition in the trivy fork (coder/trivy#74). The trivy replace is temporarily pinned to that PR's commit; it will be moved to the merged coder/trivy commit before this merges.
| // References that cannot be resolved to a concrete block simply fail to match | ||
| // and prune nothing extra, so ambiguity always errs toward keeping resources. |
A resource referenced only as my_resource.x[0] or my_resource.x["k"] is currently pruned because Reference.RefersTo treats the reference key as significant while the unexpanded resource block has none. This test fails on the PR head and documents the expected behavior.
Pruning runs before count/for_each expansion, so resource blocks carry no key while references like my_resource.x[0] do. Reference.RefersTo treats that mismatch as a different block, pruning resources the target closure depends on. Compare block type and labels only.
…arg references in closure pruning Extends the resource-closure test set with cases that reach a resource through reference shapes the pruner must not treat as a different block: - splat (my_resource.web[*].name) - variable index (my_resource.pool[var.idx].name) - a parameter's nested option block - a module input argument Each asserts the referenced resource survives pruning (the parameter default still resolves) while an unrelated orphan is pruned. All pass on the current pruner, guarding reference matching against regressions.
|
@Emyrk pulled your indexed-ref fix in, thanks 🙏 Added a few more closure tests on top to lock down the other reference shapes: splat (x[*]), variable index (x[var.i]), a nested block ref, and a module input arg. All green, so the indexed-key case looks like the only spot that was over-pruning. |
|
One more reference shape where the closure is not conservative:
Repro (parser package): {
"data": {"coder_parameter": {"p": {"name": "p", "default": "${local.v}"}}},
"locals": {"v": "${my_resource.a.name}-${my_resource.b.name}"},
"resource": {"my_resource": {"a": {"name": "A"}, "b": {"name": "B"}}}
}Without the option: 2 resources kept, default The merging in the JSON path predates this PR, but pruning is what turns it from a quirk into wrong output. Iterating
|
…runer The closure pruner repeated the "resource" string literal, which tips the file over goconst's threshold on new lines. Use a local constant instead.
JSON templates (.tf.json / .tofu.json) can merge multiple references in a single expression into one reference during extraction (see Attribute.referencesFromExpression), which would let the closure drop a resource a target depends on. Reference resolution there is not reliable enough to prune safely, so keep everything when any source file is JSON. Adds a regression test (parameter default referencing two resources in one expression must resolve identically to an unpruned run).
The PR's lint (new-vs-main) surfaces a pre-existing revive unused-parameter on this eval-hook callback. The parameter is part of the fixed OptionWithEvalHook signature, so keep the descriptive name and annotate with //nolint:revive rather than renaming to _.
|
Handled the .tf.json case 👍 Went the conservative route (your option B): the pruner now skips entirely when any source file is .tf.json/.tofu.json, with a regression test using your repro (two resources in one expression, default resolves to "A-B"). That keeps us out of the shared referencesFromExpression path. Option A (making the JSON path iterate Variables like hclsyntax does) is a real general fix, but it's your call on shared scanner code, and since our templates are all .tf we did not want to take on that blast radius for an opt-in optimization. Glad to help with A separately if you want the .tf.json speedup. Two lint things the PR's new-vs-main lint surfaced that I also cleaned up:
Local run is clean: full parser package tests pass and golangci-lint new-vs-main reports 0 issues. |
2 participants
What
Add
OptionWithResourceClosure(targetTypes []string)to the Terraform parser. When set, root-moduleresourceblocks that are not reachable (via references) from the given target block types are dropped before evaluation. Default off, so existing callers are unaffected.Why
EvaluateAllevaluates every resource and module in a module on each call. Callers that only need a subset of a module's output pay to evaluate resources whose values can never affect that output.The concrete driver is
coder/previewrendering a workspace form: it only needscoder_parameter/coder_workspace_preset/coder_workspace_tagsand what they reference, but today the whole module graph is evaluated on every keystroke over the dynamic-parameters websocket. On a real Altana template this is ~2s ofEvaluateAllper request; with this option it drops to ~0.16s locally (~12x), and the computed parameters, presets and tags are byte-identical.How
resourcenor anoutput(variables, locals, data sources, providers, module arguments, and the target blocks themselves).resourcereachable from that frontier, following references through resources that are themselves retained.Safety
The retained set is the transitive reference closure of everything whose value can flow into a target block, so a resource is excluded only when nothing a target block reads can reference it: the evaluated values of the target blocks are unchanged. The closure is conservative, a reference that cannot be resolved to a concrete block keeps the matching blocks. If no target block is present, nothing is pruned.
Testing
resource_closure_test.go: an orphan resource is pruned, a resource referenced by a parameter transitively through alocalis retained, and the parameter's computed default is unchanged. Also covers default-off and no-target-present.pkg/iac/scanners/terraform/parsersuite passes unchanged.Base branch
Targets
coder/preview_v0_69(the linecoder/previewcurrently pins) so it can flow into the deployed preview build; happy to also land it onmain.