From 01b2a571563aada76ac83e15c2a0fded4832da8c Mon Sep 17 00:00:00 2001 From: Kazuma Watanabe Date: Sat, 17 Aug 2024 22:44:45 +0900 Subject: [PATCH] Fix a race condition when evaluating on the root context (#2096) * 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. --- .github/workflows/e2e.yml | 15 +++ Makefile | 5 +- .../race/eval_locals_on_root_ctx/.tflint.hcl | 3 + .../race/eval_locals_on_root_ctx/main.tf | 17 +++ .../eval_locals_on_root_ctx/module/main.tf | 7 ++ .../race/eval_locals_on_root_ctx/result.json | 25 ++++ integrationtest/race/race_test.go | 116 ++++++++++++++++++ terraform/evaluator.go | 99 ++++----------- terraform/evaluator_test.go | 2 - terraform/lang/scope.go | 58 +++++++++ tflint/runner.go | 1 - 11 files changed, 272 insertions(+), 76 deletions(-) create mode 100644 integrationtest/race/eval_locals_on_root_ctx/.tflint.hcl create mode 100644 integrationtest/race/eval_locals_on_root_ctx/main.tf create mode 100644 integrationtest/race/eval_locals_on_root_ctx/module/main.tf create mode 100644 integrationtest/race/eval_locals_on_root_ctx/result.json create mode 100644 integrationtest/race/race_test.go diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 5a36aeba8..a0b51381a 100644 --- 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 index ee42c6b11..4ac1fdb58 100644 --- 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 new file mode 100644 index 000000000..e19f589dd --- /dev/null +++ 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 new file mode 100644 index 000000000..870a41223 --- /dev/null +++ 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 new file mode 100644 index 000000000..c67c4af28 --- /dev/null +++ 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 new file mode 100644 index 000000000..a33b69e9a --- /dev/null +++ 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 new file mode 100644 index 000000000..3578e4022 --- /dev/null +++ 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 index e46f0dafc..cc9282c35 100644 --- 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 index 860c32dfd..bcecbc04a 100644 --- a/terraform/evaluator_test.go +++ b/terraform/evaluator_test.go @@ -788,7 +788,6 @@ locals { ModulePath: config.Path.UnkeyedInstanceShim(), Config: config, VariableValues: variableValues, - CallStack: NewCallStack(), } if evaluator.Meta == nil { evaluator.Meta = &ContextMeta{Env: Workspace()} @@ -2181,7 +2180,6 @@ resource "aws_instance" "main" { ModulePath: config.Path.UnkeyedInstanceShim(), Config: config, VariableValues: variableValues, - CallStack: NewCallStack(), } expanded, diags := evaluator.ExpandBlock(file.Body, test.schema) diff --git a/terraform/lang/scope.go b/terraform/lang/scope.go index 424517572..532b05819 100644 --- a/terraform/lang/scope.go +++ b/terraform/lang/scope.go @@ -1,8 +1,10 @@ package lang import ( + "strings" "sync" + "github.com/hashicorp/hcl/v2" "github.com/zclconf/go-cty/cty/function" "github.com/terraform-linters/tflint/terraform/addrs" @@ -29,6 +31,62 @@ type Scope struct { // then differ during apply. PureOnly bool + // CallStack is a stack for recording local value references to detect + // circular references. + CallStack *CallStack + funcs map[string]function.Function funcsLock sync.Mutex } + +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) +} diff --git a/tflint/runner.go b/tflint/runner.go index d58266580..1f83909cd 100644 --- a/tflint/runner.go +++ b/tflint/runner.go @@ -57,7 +57,6 @@ func NewRunner(originalWorkingDir string, c *Config, ants map[string]Annotations ModulePath: cfg.Path.UnkeyedInstanceShim(), Config: cfg.Root, VariableValues: variableValues, - CallStack: terraform.NewCallStack(), } runner := &Runner{