fix(terraform): eval submodules (#6411)

Co-authored-by: William Reade <william@stacklet.io>

pkg/iac/scanners/terraform/parser/evaluator.go

@@ -8,6 +8,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
@@ -126,48 +128,100 @@ func (e *evaluator) EvaluateAll(ctx context.Context) (terraform.Modules, map[str
 	fsMap := make(map[string]fs.FS)
 	fsMap[fsKey] = e.filesystem
 
-	var lastContext hcl.EvalContext
 	e.debug.Log("Starting module evaluation...")
-	for i := 0; i < maxContextIterations; i++ {
+	e.evaluateSteps()
 
-		e.evaluateStep()
+	// expand out resources and modules via count, for-each and dynamic
+	// (not a typo, we do this twice so every order is processed)
+	e.blocks = e.expandBlocks(e.blocks)
+	e.blocks = e.expandBlocks(e.blocks)
 
-		// 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
-		}
+	e.debug.Log("Starting submodule evaluation...")
+	submodules := e.loadSubmodules(ctx)
 
-		if len(e.ctx.Inner().Variables) != len(lastContext.Variables) {
-			lastContext.Variables = make(map[string]cty.Value, len(e.ctx.Inner().Variables))
+	for i := 0; i < maxContextIterations; i++ {
+		changed := false
+		for _, sm := range submodules {
+			changed = changed || e.evaluateSubmodule(ctx, sm)
 		}
-		for k, v := range e.ctx.Inner().Variables {
-			lastContext.Variables[k] = v
+		if !changed {
+			e.debug.Log("All submodules are evaluated at i=%d", i)
+			break
 		}
 	}
 
-	// expand out resources and modules via count, for-each and dynamic
-	// (not a typo, we do this twice so every order is processed)
-	e.blocks = e.expandBlocks(e.blocks)
-	e.blocks = e.expandBlocks(e.blocks)
+	e.debug.Log("Starting post-submodule evaluation...")
+	e.evaluateSteps()
 
-	e.debug.Log("Starting submodule evaluation...")
 	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("Module evaluation complete.")
+	rootModule := terraform.NewModule(e.projectRootPath, e.modulePath, e.blocks, e.ignores)
+	return append(terraform.Modules{rootModule}, modules...), fsMap
+}
+
+type submodule struct {
+	definition *ModuleDefinition
+	eval       *evaluator
+	modules    terraform.Modules
+	lastState  map[string]cty.Value
+	fsMap      map[string]fs.FS
+}
+
+func (e *evaluator) loadSubmodules(ctx context.Context) []*submodule {
+	var submodules []*submodule
+
 	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)
+		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
 		}
-		// export module outputs
-		e.ctx.Set(outputs, "module", definition.Name)
-		modules = append(modules, submodules...)
-		for key, val := range definition.Parser.GetFilesystemMap() {
-			fsMap[key] = val
+
+		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 {
+	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("Finished processing %d submodule(s).", len(modules))
 
-	e.debug.Log("Starting post-submodule evaluation...")
+	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()
+
+	// 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 = sm.definition.inputVars()
+	e.evaluateSteps()
+	return true
+}
+
+func (e *evaluator) evaluateSteps() {
+	var lastContext hcl.EvalContext
 	for i := 0; i < maxContextIterations; i++ {
 
 		e.evaluateStep()
@@ -176,18 +230,13 @@ 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))
 		}
 		for k, v := range e.ctx.Inner().Variables {
 			lastContext.Variables[k] = v
 		}
 	}
-
-	e.debug.Log("Module evaluation complete.")
-	rootModule := terraform.NewModule(e.projectRootPath, e.modulePath, e.blocks, e.ignores)
-	return append(terraform.Modules{rootModule}, modules...), fsMap
 }
 
 func (e *evaluator) expandBlocks(blocks terraform.Blocks) terraform.Blocks {
@@ -217,7 +266,9 @@ func (e *evaluator) expandDynamicBlock(b *terraform.Block) {
 				b.InjectBlock(content, blockName)
 			}
 		}
-		sub.MarkExpanded()
+		if len(expanded) > 0 {
+			sub.MarkExpanded()
+		}
 	}
 }
 
@@ -246,6 +297,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 {

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

pkg/iac/scanners/terraform/parser/parser.go

@@ -2,6 +2,7 @@ package parser
 
 import (
 	"context"
+	"errors"
 	"io"
 	"io/fs"
 	"os"
@@ -224,18 +225,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))
 
@@ -246,7 +248,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))
 	}
@@ -260,10 +262,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,
@@ -278,11 +280,19 @@ func (p *Parser) EvaluateAll(ctx context.Context) (terraform.Modules, cty.Value,
 		p.debug.Extend("evaluator"),
 		p.allowDownloads,
 		p.skipCachedModules,
-	)
-	modules, fsMap := 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 := e.EvaluateAll(ctx)
 	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 {

pkg/iac/scanners/terraform/parser/parser_test.go

@@ -1522,3 +1522,112 @@ 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 = 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 = passthru.handover.from_2
+}
+`,
+		"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())
+	}
+}