Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(terraform): do not re-expand dynamic blocks #6151

Merged
merged 3 commits into from
Feb 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 32 additions & 51 deletions pkg/iac/scanners/terraform/parser/evaluator.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package parser
import (
"context"
"errors"
"fmt"
"io/fs"
"reflect"
"time"
Expand Down Expand Up @@ -148,7 +147,8 @@ func (e *evaluator) EvaluateAll(ctx context.Context) (terraform.Modules, map[str
}
}

// expand out resources and modules via count (not a typo, we do this twice so every order is processed)
// 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)

Expand Down Expand Up @@ -204,7 +204,7 @@ func (e *evaluator) isModuleLocal() bool {
}

func (e *evaluator) expandBlocks(blocks terraform.Blocks) terraform.Blocks {
return e.expandDynamicBlocks(e.expandBlockForEaches(e.expandBlockCounts(blocks))...)
return e.expandDynamicBlocks(e.expandBlockForEaches(e.expandBlockCounts(blocks), false)...)
}

func (e *evaluator) expandDynamicBlocks(blocks ...*terraform.Block) terraform.Blocks {
Expand All @@ -219,103 +219,84 @@ func (e *evaluator) expandDynamicBlock(b *terraform.Block) {
e.expandDynamicBlock(sub)
}
for _, sub := range b.AllBlocks().OfType("dynamic") {
if sub.IsExpanded() {
continue
}
blockName := sub.TypeLabel()
expanded := e.expandBlockForEaches(terraform.Blocks{sub})
expanded := e.expandBlockForEaches(terraform.Blocks{sub}, true)
for _, ex := range expanded {
if content := ex.GetBlock("content"); content.IsNotNil() {
_ = e.expandDynamicBlocks(content)
b.InjectBlock(content, blockName)
}
}
sub.MarkExpanded()
}
}

func validateForEachArg(arg cty.Value) error {
if arg.IsNull() {
return errors.New("arg is null")
}

ty := arg.Type()

if !arg.IsKnown() || ty.Equals(cty.DynamicPseudoType) || arg.LengthInt() == 0 {
return nil
}

if !(ty.IsSetType() || ty.IsObjectType() || ty.IsMapType()) {
return fmt.Errorf("%s type is not supported: arg is not set or map", ty.FriendlyName())
}

if ty.IsSetType() {
if !ty.ElementType().Equals(cty.String) {
return errors.New("arg is not set of strings")
}

it := arg.ElementIterator()
for it.Next() {
key, _ := it.Element()
if key.IsNull() {
return errors.New("arg is set of strings, but contains null")
}

if !key.IsKnown() {
return errors.New("arg is set of strings, but contains unknown value")
}
}
}

return nil
}

func isBlockSupportsForEachMetaArgument(block *terraform.Block) bool {
return slices.Contains([]string{"module", "resource", "data", "dynamic"}, block.Type())
}

func (e *evaluator) expandBlockForEaches(blocks terraform.Blocks) terraform.Blocks {
func (e *evaluator) expandBlockForEaches(blocks terraform.Blocks, isDynamic bool) terraform.Blocks {
var forEachFiltered terraform.Blocks

for _, block := range blocks {

forEachAttr := block.GetAttribute("for_each")

if forEachAttr.IsNil() || block.IsCountExpanded() || !isBlockSupportsForEachMetaArgument(block) {
if forEachAttr.IsNil() || block.IsExpanded() || !isBlockSupportsForEachMetaArgument(block) {
forEachFiltered = append(forEachFiltered, block)
continue
}

forEachVal := forEachAttr.Value()

if err := validateForEachArg(forEachVal); err != nil {
e.debug.Log(`"for_each" argument is invalid: %s`, err.Error())
Comment on lines -287 to -288
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the other cases as checked in the validateForEachArg()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need to check that the argument is a collection so that we can iterate over it. Does it make sense to validate the elements of the collection on the Trivy side? I think the user should pass a valid config.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah fair enough. I was more concerned on what happens if the user does pass us an invalid configuration (which is common) anyway. Looks like we still handle that fine as the debug logs have the following line:

2024-02-22T17:45:05.909-0700    DEBUG   [misconf] 45:05.909212000 terraform.parser.<root>          error parsing 'main.tf': main.tf:2,12-15: Invalid expression; Expected the start of an expression, but found an invalid expression token.

if forEachVal.IsNull() || !forEachVal.IsKnown() || !forEachAttr.IsIterable() {
continue
}

clones := make(map[string]cty.Value)
_ = forEachAttr.Each(func(key cty.Value, val cty.Value) {

if !key.Type().Equals(cty.String) {
// 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 {
e.debug.Log(
`Invalid "for-each" argument: map key (or set value) is not a string, but %s`,
key.Type().FriendlyName(),
)
return
}

clone := block.Clone(key)
// if the argument is a collection but not a map, then the resource identifier
// is the value of the collection. The exception is the use of for-each inside a dynamic block,
// because in this case the collection element may not be a primitive value.
if (forEachVal.Type().IsCollectionType() || forEachVal.Type().IsTupleType()) &&
!forEachVal.Type().IsMapType() && !isDynamic {
stringVal, err := convert.Convert(val, cty.String)
if err != nil {
e.debug.Log("Failed to convert for-each arg %v to string", val)
return
}
idx = stringVal
}

clone := block.Clone(idx)

ctx := clone.Context()

e.copyVariables(block, clone)

ctx.SetByDot(key, "each.key")
ctx.SetByDot(idx, "each.key")
ctx.SetByDot(val, "each.value")

ctx.Set(key, block.TypeLabel(), "key")
ctx.Set(idx, block.TypeLabel(), "key")
ctx.Set(val, block.TypeLabel(), "value")

forEachFiltered = append(forEachFiltered, clone)

values := clone.Values()
clones[key.AsString()] = values
clones[idx.AsString()] = values
e.ctx.SetByDot(values, clone.GetMetadata().Reference())
})

Expand All @@ -341,7 +322,7 @@ func (e *evaluator) expandBlockCounts(blocks terraform.Blocks) terraform.Blocks
var countFiltered terraform.Blocks
for _, block := range blocks {
countAttr := block.GetAttribute("count")
if countAttr.IsNil() || block.IsCountExpanded() || !isBlockSupportsCountMetaArgument(block) {
if countAttr.IsNil() || block.IsExpanded() || !isBlockSupportsCountMetaArgument(block) {
countFiltered = append(countFiltered, block)
continue
}
Expand Down
94 changes: 0 additions & 94 deletions pkg/iac/scanners/terraform/parser/evaluator_test.go

This file was deleted.

Loading
Loading