Skip to content

Commit

Permalink
Fix a race condition when evaluating on the root context (#2096)
Browse files Browse the repository at this point in the history
* Add an E2E test for race conditions

* Fix a race condition when evaluating on the root context

The root runner's Evaluator is shared across goroutines,
so side effects on the CallStack are not goroutine safe.

We solve this problem by moving the CallStack into a Scope
for each evaluation.
  • Loading branch information
wata727 authored Aug 17, 2024
1 parent 5833005 commit 01b2a57
Show file tree
Hide file tree
Showing 11 changed files with 272 additions and 76 deletions.
15 changes: 15 additions & 0 deletions .github/workflows/e2e.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,18 @@ jobs:
run: make e2e
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
race:
name: race
# go test -race is slow and only runs on ubuntu
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v4
with:
submodules: true
- name: Set up Go
uses: actions/setup-go@v5
with:
go-version-file: 'go.mod'
- name: Run e2e-race
run: make e2e-race
5 changes: 4 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ install:
go install

e2e: prepare install
go test -timeout 5m ./integrationtest/...
go test -timeout 5m $$(go list ./integrationtest/... | grep -v race)

e2e-race: prepare
go test --race --timeout 5m ./integrationtest/race

lint:
golangci-lint run ./...
Expand Down
3 changes: 3 additions & 0 deletions integrationtest/race/eval_locals_on_root_ctx/.tflint.hcl
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
plugin "testing" {
enabled = true
}
17 changes: 17 additions & 0 deletions integrationtest/race/eval_locals_on_root_ctx/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
locals {
dns_name = "www.example.com"
}

resource "aws_route53_record" "www" {
zone_id = aws_route53_zone.primary.zone_id
name = local.dns_name
type = "A"
ttl = 300
records = [aws_eip.lb.public_ip]
}

module "route53_records" {
count = 10

source = "./module"
}
7 changes: 7 additions & 0 deletions integrationtest/race/eval_locals_on_root_ctx/module/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
resource "aws_route53_record" "help" {
zone_id = aws_route53_zone.primary.zone_id
name = "help.example.com"
type = "A"
ttl = 300
records = [aws_eip.lb.public_ip]
}
25 changes: 25 additions & 0 deletions integrationtest/race/eval_locals_on_root_ctx/result.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
{
"issues": [
{
"rule": {
"name": "aws_route53_record_eval_on_root_ctx_example",
"severity": "error",
"link": ""
},
"message": "record name (root): \"www.example.com\"",
"range": {
"filename": "main.tf",
"start": {
"line": 7,
"column": 13
},
"end": {
"line": 7,
"column": 27
}
},
"callers": []
}
],
"errors": []
}
116 changes: 116 additions & 0 deletions integrationtest/race/race_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
package main

import (
"bytes"
"encoding/json"
"fmt"
"io"
"log"
"os"
"path/filepath"
"runtime"
"strings"
"testing"
"text/template"

"github.com/google/go-cmp/cmp"
"github.com/terraform-linters/tflint/cmd"
"github.com/terraform-linters/tflint/formatter"
"github.com/terraform-linters/tflint/tflint"
)

func TestMain(m *testing.M) {
log.SetOutput(io.Discard)
os.Exit(m.Run())
}

type meta struct {
Version string
}

func TestIntegration(t *testing.T) {
cases := []struct {
Name string
Command string
Dir string
}{
{
// @see https://github.com/terraform-linters/tflint/issues/2094
Name: "eval locals on the root context in parallel runners",
Command: "tflint --format json",
Dir: "eval_locals_on_root_ctx",
},
}

// Disable the bundled plugin because the `os.Executable()` is go(1) in the tests
tflint.DisableBundledPlugin = true
defer func() {
tflint.DisableBundledPlugin = false
}()

dir, _ := os.Getwd()
for _, tc := range cases {
t.Run(tc.Name, func(t *testing.T) {
testDir := filepath.Join(dir, tc.Dir)

defer func() {
if err := os.Chdir(dir); err != nil {
t.Fatal(err)
}
}()
if err := os.Chdir(testDir); err != nil {
t.Fatal(err)
}

outStream, errStream := new(bytes.Buffer), new(bytes.Buffer)
cli, err := cmd.NewCLI(outStream, errStream)
if err != nil {
t.Fatal(err)
}
args := strings.Split(tc.Command, " ")

cli.Run(args)

rawWant, err := readResultFile(testDir)
if err != nil {
t.Fatal(err)
}
var want *formatter.JSONOutput
if err := json.Unmarshal(rawWant, &want); err != nil {
t.Fatal(err)
}

var got *formatter.JSONOutput
if err := json.Unmarshal(outStream.Bytes(), &got); err != nil {
t.Fatal(err)
}

if diff := cmp.Diff(got, want); diff != "" {
t.Fatal(diff)
}
})
}
}

func readResultFile(dir string) ([]byte, error) {
resultFile := "result.json"
if runtime.GOOS == "windows" {
if _, err := os.Stat(filepath.Join(dir, "result_windows.json")); !os.IsNotExist(err) {
resultFile = "result_windows.json"
}
}
if _, err := os.Stat(fmt.Sprintf("%s.tmpl", resultFile)); !os.IsNotExist(err) {
resultFile = fmt.Sprintf("%s.tmpl", resultFile)
}

if !strings.HasSuffix(resultFile, ".tmpl") {
return os.ReadFile(filepath.Join(dir, resultFile))
}

want := new(bytes.Buffer)
tmpl := template.Must(template.ParseFiles(filepath.Join(dir, resultFile)))
if err := tmpl.Execute(want, meta{Version: tflint.Version.String()}); err != nil {
return nil, err
}
return want.Bytes(), nil
}
99 changes: 27 additions & 72 deletions terraform/evaluator.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"fmt"
"os"
"path/filepath"
"strings"

"github.com/agext/levenshtein"
"github.com/hashicorp/hcl/v2"
Expand All @@ -21,64 +20,11 @@ type ContextMeta struct {
OriginalWorkingDir string
}

type CallStack struct {
addrs map[string]addrs.Reference
stack []string
}

func NewCallStack() *CallStack {
return &CallStack{
addrs: make(map[string]addrs.Reference),
stack: make([]string, 0),
}
}

func (g *CallStack) Push(addr addrs.Reference) hcl.Diagnostics {
g.stack = append(g.stack, addr.Subject.String())

if _, exists := g.addrs[addr.Subject.String()]; exists {
return hcl.Diagnostics{
{
Severity: hcl.DiagError,
Summary: "circular reference found",
Detail: g.String(),
Subject: addr.SourceRange.Ptr(),
},
}
}
g.addrs[addr.Subject.String()] = addr
return hcl.Diagnostics{}
}

func (g *CallStack) Pop() {
if g.Empty() {
panic("cannot pop from empty stack")
}

addr := g.stack[len(g.stack)-1]
g.stack = g.stack[:len(g.stack)-1]
delete(g.addrs, addr)
}

func (g *CallStack) String() string {
return strings.Join(g.stack, " -> ")
}

func (g *CallStack) Empty() bool {
return len(g.stack) == 0
}

func (g *CallStack) Clear() {
g.addrs = make(map[string]addrs.Reference)
g.stack = make([]string, 0)
}

type Evaluator struct {
Meta *ContextMeta
ModulePath addrs.ModuleInstance
Config *Config
VariableValues map[string]map[string]cty.Value
CallStack *CallStack
}

// EvaluateExpr takes the given HCL expression and evaluates it to produce a value.
Expand All @@ -101,18 +47,27 @@ func (e *Evaluator) ExpandBlock(body hcl.Body, schema *hclext.BodySchema) (hcl.B
return e.scope().ExpandBlock(body, schema)
}

// scope creates a new evaluation scope.
// The difference with Evaluator is that each evaluation is independent
// and is not shared between goroutines.
func (e *Evaluator) scope() *lang.Scope {
return &lang.Scope{
Data: &evaluationData{
Evaluator: e,
ModulePath: e.ModulePath,
},
scope := &lang.Scope{CallStack: lang.NewCallStack()}
scope.Data = &evaluationData{
Scope: scope,
Meta: e.Meta,
ModulePath: e.ModulePath,
Config: e.Config,
VariableValues: e.VariableValues,
}
return scope
}

type evaluationData struct {
Evaluator *Evaluator
ModulePath addrs.ModuleInstance
Scope *lang.Scope
Meta *ContextMeta
ModulePath addrs.ModuleInstance
Config *Config
VariableValues map[string]map[string]cty.Value
}

var _ lang.Data = (*evaluationData)(nil)
Expand Down Expand Up @@ -142,7 +97,7 @@ func (d *evaluationData) GetForEachAttr(addr addrs.ForEachAttr, rng hcl.Range) (
func (d *evaluationData) GetInputVariable(addr addrs.InputVariable, rng hcl.Range) (cty.Value, hcl.Diagnostics) {
var diags hcl.Diagnostics

moduleConfig := d.Evaluator.Config.DescendentForInstance(d.ModulePath)
moduleConfig := d.Config.DescendentForInstance(d.ModulePath)
if moduleConfig == nil {
// should never happen, since we can't be evaluating in a module
// that wasn't mentioned in configuration.
Expand Down Expand Up @@ -172,7 +127,7 @@ func (d *evaluationData) GetInputVariable(addr addrs.InputVariable, rng hcl.Rang
}

moduleAddrStr := d.ModulePath.String()
vals := d.Evaluator.VariableValues[moduleAddrStr]
vals := d.VariableValues[moduleAddrStr]
if vals == nil {
return cty.UnknownVal(config.Type), diags
}
Expand Down Expand Up @@ -229,7 +184,7 @@ func (d *evaluationData) GetLocalValue(addr addrs.LocalValue, rng hcl.Range) (ct

// First we'll make sure the requested value is declared in configuration,
// so we can produce a nice message if not.
moduleConfig := d.Evaluator.Config.DescendentForInstance(d.ModulePath)
moduleConfig := d.Config.DescendentForInstance(d.ModulePath)
if moduleConfig == nil {
// should never happen, since we can't be evaluating in a module
// that wasn't mentioned in configuration.
Expand Down Expand Up @@ -257,13 +212,13 @@ func (d *evaluationData) GetLocalValue(addr addrs.LocalValue, rng hcl.Range) (ct
}

// Build a call stack for circular reference detection only when getting a local value.
if diags := d.Evaluator.CallStack.Push(addrs.Reference{Subject: addr, SourceRange: rng}); diags.HasErrors() {
if diags := d.Scope.CallStack.Push(addrs.Reference{Subject: addr, SourceRange: rng}); diags.HasErrors() {
return cty.UnknownVal(cty.DynamicPseudoType), diags
}

val, diags := d.Evaluator.EvaluateExpr(config.Expr, cty.DynamicPseudoType)
val, diags := d.Scope.EvalExpr(config.Expr, cty.DynamicPseudoType)

d.Evaluator.CallStack.Pop()
d.Scope.CallStack.Pop()
return val, diags
}

Expand All @@ -274,10 +229,10 @@ func (d *evaluationData) GetPathAttr(addr addrs.PathAttr, rng hcl.Range) (cty.Va
case "cwd":
var err error
var wd string
if d.Evaluator.Meta != nil {
if d.Meta != nil {
// Meta is always non-nil in the normal case, but some test cases
// are not so realistic.
wd = d.Evaluator.Meta.OriginalWorkingDir
wd = d.Meta.OriginalWorkingDir
}
if wd == "" {
wd, err = os.Getwd()
Expand Down Expand Up @@ -308,7 +263,7 @@ func (d *evaluationData) GetPathAttr(addr addrs.PathAttr, rng hcl.Range) (cty.Va
return cty.StringVal(filepath.ToSlash(wd)), diags

case "module":
moduleConfig := d.Evaluator.Config.DescendentForInstance(d.ModulePath)
moduleConfig := d.Config.DescendentForInstance(d.ModulePath)
if moduleConfig == nil {
// should never happen, since we can't be evaluating in a module
// that wasn't mentioned in configuration.
Expand All @@ -318,7 +273,7 @@ func (d *evaluationData) GetPathAttr(addr addrs.PathAttr, rng hcl.Range) (cty.Va
return cty.StringVal(filepath.ToSlash(sourceDir)), diags

case "root":
sourceDir := d.Evaluator.Config.Module.SourceDir
sourceDir := d.Config.Module.SourceDir
return cty.StringVal(filepath.ToSlash(sourceDir)), diags

default:
Expand All @@ -341,7 +296,7 @@ func (d *evaluationData) GetTerraformAttr(addr addrs.TerraformAttr, rng hcl.Rang
switch addr.Name {

case "workspace":
workspaceName := d.Evaluator.Meta.Env
workspaceName := d.Meta.Env
return cty.StringVal(workspaceName), diags

case "env":
Expand Down
Loading

0 comments on commit 01b2a57

Please sign in to comment.