From e64d93548c0e7526cb38de5d16d459692de84c17 Mon Sep 17 00:00:00 2001 From: nikpivkin Date: Thu, 28 Mar 2024 18:34:26 +0700 Subject: [PATCH 1/3] fix(terraform): eval submodules --- .../scanners/terraform/parser/evaluator.go | 118 ++++++++++++------ .../scanners/terraform/parser/load_module.go | 8 ++ pkg/iac/scanners/terraform/parser/parser.go | 28 +++-- .../scanners/terraform/parser/parser_test.go | 103 +++++++++++++++ 4 files changed, 213 insertions(+), 44 deletions(-) diff --git a/pkg/iac/scanners/terraform/parser/evaluator.go b/pkg/iac/scanners/terraform/parser/evaluator.go index 1fe9a72fdcac..0c675b8a8945 100644 --- a/pkg/iac/scanners/terraform/parser/evaluator.go +++ b/pkg/iac/scanners/terraform/parser/evaluator.go @@ -9,6 +9,7 @@ import ( "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/ext/typeexpr" + "github.com/samber/lo" "github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty/convert" "golang.org/x/exp/slices" @@ -102,6 +103,7 @@ func (e *evaluator) evaluateStep() { e.ctx.Set(e.getValuesByBlockType("data"), "data") e.ctx.Set(e.getValuesByBlockType("output"), "output") + e.ctx.Set(e.getValuesByBlockType("module"), "module") } // exportOutputs is used to export module outputs to the parent module @@ -128,25 +130,9 @@ func (e *evaluator) EvaluateAll(ctx context.Context) (terraform.Modules, map[str var parseDuration time.Duration - var lastContext hcl.EvalContext start := time.Now() e.debug.Log("Starting module evaluation...") - for i := 0; i < maxContextIterations; i++ { - - e.evaluateStep() - - // if ctx matches the last evaluation, we can bail, nothing left to resolve - if i > 0 && reflect.DeepEqual(lastContext.Variables, e.ctx.Inner().Variables) { - break - } - - if len(e.ctx.Inner().Variables) != len(lastContext.Variables) { - lastContext.Variables = make(map[string]cty.Value, len(e.ctx.Inner().Variables)) - } - for k, v := range e.ctx.Inner().Variables { - lastContext.Variables[k] = v - } - } + e.evaluateSteps() // expand out resources and modules via count, for-each and dynamic // (not a typo, we do this twice so every order is processed) @@ -156,23 +142,85 @@ func (e *evaluator) EvaluateAll(ctx context.Context) (terraform.Modules, map[str parseDuration += time.Since(start) e.debug.Log("Starting submodule evaluation...") - var modules terraform.Modules - for _, definition := range e.loadModules(ctx) { - submodules, outputs, err := definition.Parser.EvaluateAll(ctx) - if err != nil { - e.debug.Log("Failed to evaluate submodule '%s': %s.", definition.Name, err) - continue + submodules := e.loadSubmodules(ctx) + + for i := 0; i < maxContextIterations; i++ { + changed := false + for _, sm := range submodules { + changed = changed || e.evaluateSubmodule(ctx, sm) } - // export module outputs - e.ctx.Set(outputs, "module", definition.Name) - modules = append(modules, submodules...) - for key, val := range definition.Parser.GetFilesystemMap() { - fsMap[key] = val + if !changed { + e.debug.Log("All submodules are evaluated at i=%d", i) + break } } + + var modules terraform.Modules + for _, sm := range submodules { + modules = append(modules, sm.modules...) + fsMap = lo.Assign(fsMap, sm.fsMap) + } + e.debug.Log("Finished processing %d submodule(s).", len(modules)) e.debug.Log("Starting post-submodule evaluation...") + e.evaluateSteps() + + e.debug.Log("Module evaluation complete.") + parseDuration += time.Since(start) + rootModule := terraform.NewModule(e.projectRootPath, e.modulePath, e.blocks, e.ignores) + return append(terraform.Modules{rootModule}, modules...), fsMap, parseDuration +} + +type submodule struct { + definition *ModuleDefinition + eval *evaluator + modules terraform.Modules + lastState cty.Value + fsMap map[string]fs.FS +} + +func (e *evaluator) loadSubmodules(ctx context.Context) []*submodule { + var submodules []*submodule + + for _, definition := range e.loadModules(ctx) { + eval, err := definition.Parser.Load(ctx) + if errors.Is(err, ErrNoFiles) { + continue + } else if err != nil { + e.debug.Log("Failed to load submodule '%s': %s.", definition.Name, err) + continue + } + + submodules = append(submodules, &submodule{ + definition: definition, + eval: eval, + fsMap: make(map[string]fs.FS), + }) + } + + return submodules +} + +func (e *evaluator) evaluateSubmodule(ctx context.Context, sm *submodule) bool { + sm.eval.inputVars = sm.definition.inputVars() + sm.modules, sm.fsMap, _ = sm.eval.EvaluateAll(ctx) + outputs := sm.eval.exportOutputs() + + if reflect.DeepEqual(outputs, sm.lastState) { + e.debug.Log("Submodule %s outputs unchanged", sm.definition.Name) + return false + } + e.debug.Log("Submodule %s outputs changed", sm.definition.Name) + + e.ctx.Set(outputs, "module", sm.definition.Name) + sm.lastState = outputs + + return true +} + +func (e *evaluator) evaluateSteps() { + var lastContext hcl.EvalContext for i := 0; i < maxContextIterations; i++ { e.evaluateStep() @@ -181,7 +229,6 @@ func (e *evaluator) EvaluateAll(ctx context.Context) (terraform.Modules, map[str if i > 0 && reflect.DeepEqual(lastContext.Variables, e.ctx.Inner().Variables) { break } - if len(e.ctx.Inner().Variables) != len(lastContext.Variables) { lastContext.Variables = make(map[string]cty.Value, len(e.ctx.Inner().Variables)) } @@ -189,11 +236,6 @@ func (e *evaluator) EvaluateAll(ctx context.Context) (terraform.Modules, map[str lastContext.Variables[k] = v } } - - e.debug.Log("Module evaluation complete.") - parseDuration += time.Since(start) - rootModule := terraform.NewModule(e.projectRootPath, e.modulePath, e.blocks, e.ignores) - return append(terraform.Modules{rootModule}, modules...), fsMap, parseDuration } func (e *evaluator) expandBlocks(blocks terraform.Blocks) terraform.Blocks { @@ -223,7 +265,9 @@ func (e *evaluator) expandDynamicBlock(b *terraform.Block) { b.InjectBlock(content, blockName) } } - sub.MarkExpanded() + if len(expanded) > 0 { + sub.MarkExpanded() + } } } @@ -252,6 +296,10 @@ func (e *evaluator) expandBlockForEaches(blocks terraform.Blocks, isDynamic bool clones := make(map[string]cty.Value) _ = forEachAttr.Each(func(key cty.Value, val cty.Value) { + if val.IsNull() { + return + } + // instances are identified by a map key (or set member) from the value provided to for_each idx, err := convert.Convert(key, cty.String) if err != nil { diff --git a/pkg/iac/scanners/terraform/parser/load_module.go b/pkg/iac/scanners/terraform/parser/load_module.go index 461d7a7a1a56..0bd6a6395936 100644 --- a/pkg/iac/scanners/terraform/parser/load_module.go +++ b/pkg/iac/scanners/terraform/parser/load_module.go @@ -22,6 +22,14 @@ type ModuleDefinition struct { External bool } +func (d *ModuleDefinition) inputVars() map[string]cty.Value { + inputs := d.Definition.Values().AsValueMap() + if inputs == nil { + return make(map[string]cty.Value) + } + return inputs +} + // loadModules reads all module blocks and loads them func (e *evaluator) loadModules(ctx context.Context) []*ModuleDefinition { var moduleDefinitions []*ModuleDefinition diff --git a/pkg/iac/scanners/terraform/parser/parser.go b/pkg/iac/scanners/terraform/parser/parser.go index e09e9e621ef4..3f38e7734ae9 100644 --- a/pkg/iac/scanners/terraform/parser/parser.go +++ b/pkg/iac/scanners/terraform/parser/parser.go @@ -2,6 +2,7 @@ package parser import ( "context" + "errors" "io" "io/fs" "os" @@ -254,18 +255,19 @@ func (p *Parser) ParseFS(ctx context.Context, dir string) error { return nil } -func (p *Parser) EvaluateAll(ctx context.Context) (terraform.Modules, cty.Value, error) { +var ErrNoFiles = errors.New("no files found") +func (p *Parser) Load(ctx context.Context) (*evaluator, error) { p.debug.Log("Evaluating module...") if len(p.files) == 0 { p.debug.Log("No files found, nothing to do.") - return nil, cty.NilVal, nil + return nil, ErrNoFiles } blocks, ignores, err := p.readBlocks(p.files) if err != nil { - return nil, cty.NilVal, err + return nil, err } p.debug.Log("Read %d block(s) and %d ignore(s) for module '%s' (%d file[s])...", len(blocks), len(ignores), p.moduleName, len(p.files)) @@ -278,7 +280,7 @@ func (p *Parser) EvaluateAll(ctx context.Context) (terraform.Modules, cty.Value, } else { inputVars, err = loadTFVars(p.configsFS, p.tfvarsPaths) if err != nil { - return nil, cty.NilVal, err + return nil, err } p.debug.Log("Added %d variables from tfvars.", len(inputVars)) } @@ -292,10 +294,10 @@ func (p *Parser) EvaluateAll(ctx context.Context) (terraform.Modules, cty.Value, workingDir, err := os.Getwd() if err != nil { - return nil, cty.NilVal, err + return nil, err } p.debug.Log("Working directory for module evaluation is '%s'", workingDir) - evaluator := newEvaluator( + return newEvaluator( p.moduleFS, p, p.projectRoot, @@ -310,13 +312,21 @@ func (p *Parser) EvaluateAll(ctx context.Context) (terraform.Modules, cty.Value, p.debug.Extend("evaluator"), p.allowDownloads, p.skipCachedModules, - ) - modules, fsMap, parseDuration := evaluator.EvaluateAll(ctx) + ), nil +} + +func (p *Parser) EvaluateAll(ctx context.Context) (terraform.Modules, cty.Value, error) { + + e, err := p.Load(ctx) + if errors.Is(err, ErrNoFiles) { + return nil, cty.NilVal, nil + } + modules, fsMap, parseDuration := e.EvaluateAll(ctx) p.metrics.Counts.Modules = len(modules) p.metrics.Timings.ParseDuration = parseDuration p.debug.Log("Finished parsing module '%s'.", p.moduleName) p.fsMap = fsMap - return modules, evaluator.exportOutputs(), nil + return modules, e.exportOutputs(), nil } func (p *Parser) GetFilesystemMap() map[string]fs.FS { diff --git a/pkg/iac/scanners/terraform/parser/parser_test.go b/pkg/iac/scanners/terraform/parser/parser_test.go index 12594841251b..350aed9549ff 100644 --- a/pkg/iac/scanners/terraform/parser/parser_test.go +++ b/pkg/iac/scanners/terraform/parser/parser_test.go @@ -1522,3 +1522,106 @@ func compareSets(a []int, b []int) bool { return true } + +func TestModuleRefersToOutputOfAnotherModule(t *testing.T) { + files := map[string]string{ + "main.tf": ` +module "module2" { + source = "./modules/foo" +} + +module "module1" { + source = "./modules/bar" + test_var = module.module2.test_out +} +`, + "modules/foo/main.tf": ` +output "test_out" { + value = "test_value" +} +`, + "modules/bar/main.tf": ` +variable "test_var" {} + +resource "test_resource" "this" { + dynamic "dynamic_block" { + for_each = [var.test_var] + content { + some_attr = dynamic_block.value + } + } +} +`, + } + + modules := parse(t, files) + require.Len(t, modules, 3) + + resources := modules.GetResourcesByType("test_resource") + require.Len(t, resources, 1) + + attr, _ := resources[0].GetNestedAttribute("dynamic_block.some_attr") + require.NotNil(t, attr) + + assert.Equal(t, "test_value", attr.GetRawValue()) +} + +func TestCyclicModules(t *testing.T) { + files := map[string]string{ + "main.tf": ` +module "module2" { + source = "./modules/foo" + test_var = module.module1.test_out +} + +module "module1" { + source = "./modules/bar" + test_var = module.module2.test_out +} +`, + "modules/foo/main.tf": ` +variable "test_var" {} + +resource "test_resource" "this" { + dynamic "dynamic_block" { + for_each = [var.test_var] + content { + some_attr = dynamic_block.value + } + } +} + +output "test_out" { + value = "test_value" +} +`, + "modules/bar/main.tf": ` +variable "test_var" {} + +resource "test_resource" "this" { + dynamic "dynamic_block" { + for_each = [var.test_var] + content { + some_attr = dynamic_block.value + } + } +} + +output "test_out" { + value = test_resource.this.dynamic_block.some_attr +} +`, + } + + modules := parse(t, files) + require.Len(t, modules, 3) + + resources := modules.GetResourcesByType("test_resource") + require.Len(t, resources, 2) + + for _, res := range resources { + attr, _ := res.GetNestedAttribute("dynamic_block.some_attr") + require.NotNil(t, attr, res.FullName()) + assert.Equal(t, "test_value", attr.GetRawValue()) + } +} From 2e439e3b5d08a3ab39aff34b83d50f710f3683e2 Mon Sep 17 00:00:00 2001 From: William Reade Date: Tue, 2 Apr 2024 00:22:28 +0100 Subject: [PATCH 2/3] fix: evaluateSteps after each --- .../scanners/terraform/parser/evaluator.go | 32 ++++++++++--------- .../scanners/terraform/parser/parser_test.go | 10 ++++-- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/pkg/iac/scanners/terraform/parser/evaluator.go b/pkg/iac/scanners/terraform/parser/evaluator.go index 0c675b8a8945..a123ea336352 100644 --- a/pkg/iac/scanners/terraform/parser/evaluator.go +++ b/pkg/iac/scanners/terraform/parser/evaluator.go @@ -139,8 +139,6 @@ func (e *evaluator) EvaluateAll(ctx context.Context) (terraform.Modules, map[str e.blocks = e.expandBlocks(e.blocks) e.blocks = e.expandBlocks(e.blocks) - parseDuration += time.Since(start) - e.debug.Log("Starting submodule evaluation...") submodules := e.loadSubmodules(ctx) @@ -163,9 +161,6 @@ func (e *evaluator) EvaluateAll(ctx context.Context) (terraform.Modules, map[str e.debug.Log("Finished processing %d submodule(s).", len(modules)) - e.debug.Log("Starting post-submodule evaluation...") - e.evaluateSteps() - e.debug.Log("Module evaluation complete.") parseDuration += time.Since(start) rootModule := terraform.NewModule(e.projectRootPath, e.modulePath, e.blocks, e.ignores) @@ -176,7 +171,7 @@ type submodule struct { definition *ModuleDefinition eval *evaluator modules terraform.Modules - lastState cty.Value + lastState map[string]cty.Value fsMap map[string]fs.FS } @@ -203,19 +198,26 @@ func (e *evaluator) loadSubmodules(ctx context.Context) []*submodule { } func (e *evaluator) evaluateSubmodule(ctx context.Context, sm *submodule) bool { - sm.eval.inputVars = sm.definition.inputVars() + inputVars := sm.definition.inputVars() + if len(sm.modules) > 0 { + if reflect.DeepEqual(inputVars, sm.lastState) { + e.debug.Log("Submodule %s inputs unchanged", sm.definition.Name) + return false + } + } + + e.debug.Log("Evaluating submodule %s", sm.definition.Name) + sm.eval.inputVars = inputVars sm.modules, sm.fsMap, _ = sm.eval.EvaluateAll(ctx) outputs := sm.eval.exportOutputs() - if reflect.DeepEqual(outputs, sm.lastState) { - e.debug.Log("Submodule %s outputs unchanged", sm.definition.Name) - return false - } - e.debug.Log("Submodule %s outputs changed", sm.definition.Name) - + // lastState needs to be captured after applying outputs – so that they + // don't get treated as changes – but before running post-submodule + // evaluation, so that changes from that can trigger re-evaluations of + // the submodule if/when they feed back into inputs. e.ctx.Set(outputs, "module", sm.definition.Name) - sm.lastState = outputs - + sm.lastState = sm.definition.inputVars() + e.evaluateSteps() return true } diff --git a/pkg/iac/scanners/terraform/parser/parser_test.go b/pkg/iac/scanners/terraform/parser/parser_test.go index 350aed9549ff..a20bb2a84b58 100644 --- a/pkg/iac/scanners/terraform/parser/parser_test.go +++ b/pkg/iac/scanners/terraform/parser/parser_test.go @@ -1571,12 +1571,18 @@ func TestCyclicModules(t *testing.T) { "main.tf": ` module "module2" { source = "./modules/foo" - test_var = module.module1.test_out + test_var = passthru.handover.from_1 +} + +// Demonstrates need for evaluateSteps between submodule evaluations. +resource "passthru" "handover" { + from_1 = module.module1.test_out + from_2 = module.module2.test_out } module "module1" { source = "./modules/bar" - test_var = module.module2.test_out + test_var = passthru.handover.from_2 } `, "modules/foo/main.tf": ` From 6ad7e54ce792415f9c6d0314357824a48ed2e790 Mon Sep 17 00:00:00 2001 From: William Reade Date: Tue, 2 Apr 2024 09:26:23 +0100 Subject: [PATCH 3/3] restore needed post-eval --- pkg/iac/scanners/terraform/parser/evaluator.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/iac/scanners/terraform/parser/evaluator.go b/pkg/iac/scanners/terraform/parser/evaluator.go index a123ea336352..885533c0ca4b 100644 --- a/pkg/iac/scanners/terraform/parser/evaluator.go +++ b/pkg/iac/scanners/terraform/parser/evaluator.go @@ -153,6 +153,9 @@ func (e *evaluator) EvaluateAll(ctx context.Context) (terraform.Modules, map[str } } + e.debug.Log("Starting post-submodule evaluation...") + e.evaluateSteps() + var modules terraform.Modules for _, sm := range submodules { modules = append(modules, sm.modules...)