Fix a race condition when evaluating on the root context (#2096)

wata727 · web-flow · commit 01b2a571563a · 2024-08-17T22:44:45.000+09:00
* 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.
diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml
@@ -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
diff --git a/Makefile b/Makefile
@@ -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 ./...
diff --git a/integrationtest/race/eval_locals_on_root_ctx/.tflint.hcl b/integrationtest/race/eval_locals_on_root_ctx/.tflint.hcl
@@ -0,0 +1,3 @@
+plugin "testing" {
+  enabled = true
+}
diff --git a/integrationtest/race/eval_locals_on_root_ctx/main.tf b/integrationtest/race/eval_locals_on_root_ctx/main.tf
@@ -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"
+}
diff --git a/integrationtest/race/eval_locals_on_root_ctx/module/main.tf b/integrationtest/race/eval_locals_on_root_ctx/module/main.tf
@@ -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]
+}
diff --git a/integrationtest/race/eval_locals_on_root_ctx/result.json b/integrationtest/race/eval_locals_on_root_ctx/result.json
@@ -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": []
+}
diff --git a/integrationtest/race/race_test.go b/integrationtest/race/race_test.go
@@ -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
+}
diff --git a/terraform/evaluator.go b/terraform/evaluator.go
@@ -4,7 +4,6 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"strings"
 
 	"github.com/agext/levenshtein"
 	"github.com/hashicorp/hcl/v2"
@@ -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.
@@ -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)
@@ -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.
@@ -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
 	}
@@ -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.
@@ -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
 }
 
@@ -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()
@@ -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.
@@ -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:
@@ -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":
diff --git a/terraform/evaluator_test.go b/terraform/evaluator_test.go
diff --git a/terraform/lang/scope.go b/terraform/lang/scope.go
diff --git a/tflint/runner.go b/tflint/runner.go

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+plugin "testing" {`
	`2`	`+ enabled = true`
	`3`	`+}`