Panics when emitting issues on child blocks

[bendrucker]

Summary

When a ruleset emits an issue with the declaration range of a child block, TFLint will panic if that issue is emitted from a child module. The range itself covers just the block name, which TFLint interprets as an expression, and attempts to parse it for variables. That fails and causes a panic.

See terraform-linters/tflint-ruleset-opa#85

This issue is not directly related to the OPA ruleset plugin. Given the configuration below, the emitted issue range covers rule (no braces). TFLint should not assume that the declaration range of a block is a valid expression, because it's not. This happens to work for labeled blocks (e.g. resource "foo" "bar", but causes errors for unlabeled blocks which are just one-word identifiers and syntactically look like an expression.

Command

tflint

Terraform Configuration

# main.tf

module "m" {
  source = "./module"
}

# module/main.tf
resource "signalfx_detector" "test" {
  rule {}
}

TFLint Configuration

plugin "opa" {
  enabled = true
  version = "0.5.0"
  source  = "github.com/terraform-linters/tflint-ruleset-opa"
}

Output

panic: module/main.tf:2,3-7: Invalid reference; A reference to a resource type must be followed by at least one attribute access, specifying the resource name.

goroutine 88 [running]:
github.com/terraform-linters/tflint/tflint.listVarRefs({0x10342b058?, 0x140001349c0?})
	github.com/terraform-linters/tflint/tflint/runner.go:326 +0x150
github.com/terraform-linters/tflint/tflint.(*Runner).listModuleVars(0x1400068f7a0, {0x10342b058?, 0x140001349c0?})
	github.com/terraform-linters/tflint/tflint/runner.go:314 +0x34
github.com/terraform-linters/tflint/tflint.(*Runner).EmitIssue(0x1400068f7a0, {0x10ad35998?, 0x140001ec3c0}, {0x140001442a0, 0x2d}, {{0x1400012e600, 0x14}, {0x3, 0x3, 0x58}, ...}, ...)
	github.com/terraform-linters/tflint/tflint/runner.go:240 +0x6c
github.com/terraform-linters/tflint/plugin.(*GRPCServer).EmitIssue.func1(...)
	github.com/terraform-linters/tflint/plugin/server.go:202
github.com/terraform-linters/tflint/tflint.(*Runner).WithExpressionContext(...)
	github.com/terraform-linters/tflint/tflint/runner.go:263
github.com/terraform-linters/tflint/plugin.(*GRPCServer).EmitIssue(0x140006082a0, {0x10342fd50?, 0x140001ec3c0}, {0x140001442a0, 0x2d}, {{0x1400012e600, 0x14}, {0x3, 0x3, 0x58}, ...}, ...)
	github.com/terraform-linters/tflint/plugin/server.go:201 +0x11c
github.com/terraform-linters/tflint-plugin-sdk/plugin/internal/plugin2host.(*GRPCServer).EmitIssue(0x14000708ad0, {0x102dcb070?, 0x10?}, 0x140006517c0)
	github.com/terraform-linters/tflint-plugin-sdk@v0.18.0/plugin/internal/plugin2host/server.go:164 +0x1bc
github.com/terraform-linters/tflint-plugin-sdk/plugin/internal/proto._Runner_EmitIssue_Handler.func1({0x10342b4b8, 0x140000ddce0}, {0x103386d80?, 0x140006517c0})
	github.com/terraform-linters/tflint-plugin-sdk@v0.18.0/plugin/internal/proto/tflint_grpc.pb.go:722 +0x74
github.com/terraform-linters/tflint-plugin-sdk/plugin/internal/host2plugin.(*GRPCClient).Check.func1.RequestLogging.func1({0x10342b4b8, 0x140000ddce0}, {0x103386d80?, 0x140006517c0?}, 0x14000175f00, 0x14000800108)
	github.com/terraform-linters/tflint-plugin-sdk@v0.18.0/plugin/internal/interceptor/logging.go:16 +0x1a8
github.com/terraform-linters/tflint-plugin-sdk/plugin/internal/proto._Runner_EmitIssue_Handler({0x10339bb00?, 0x14000708ad0}, {0x10342b4b8, 0x140000ddce0}, 0x14000452500, 0x1400000f770)
	github.com/terraform-linters/tflint-plugin-sdk@v0.18.0/plugin/internal/proto/tflint_grpc.pb.go:724 +0x12c
google.golang.org/grpc.(*Server).processUnaryRPC(0x140001b8400, {0x10342b4b8, 0x140000dd650}, {0x103431088, 0x14000283860}, 0x1400061fe60, 0x14000413c20, 0x103c6d748, 0x0)
	google.golang.org/grpc@v1.60.1/server.go:1372 +0xb8c
google.golang.org/grpc.(*Server).handleStream(0x140001b8400, {0x103431088, 0x14000283860}, 0x1400061fe60)
	google.golang.org/grpc@v1.60.1/server.go:1783 +0xc4c
google.golang.org/grpc.(*Server).serveStreams.func2.1()
	google.golang.org/grpc@v1.60.1/server.go:1016 +0x5c
created by google.golang.org/grpc.(*Server).serveStreams.func2 in goroutine 74
	google.golang.org/grpc@v1.60.1/server.go:1027 +0x138

TFLint Version

0.50.2

Terraform Version

No response

Operating System

• Linux
• macOS
• Windows

[bendrucker]

Two general approaches would be:

1. Add an argument to the issues API that allows the rule to instruct on whether or not the range at issue is an expression or a declaration. This is likely to be a breaking change given that callers use EmitIssue to create issues and pass a Range as its own argument.
2. Ignore reference parsing errors entirely:

   tflint/tflint/runner.go

   Lines 324 to 327 in 5ed807d

   	if diags.HasErrors() {
   	// Maybe this is bug
   	panic(diags)
   	}

On further thought it seems like 2 is actually ok given that this is meant to limit errors to expressions that reference module variables. If the expression is invalid it can't contain any variables.

[wata727]

Agreed with 2. 1 is a better approach, but I think module call inspection should stop assuming only issues for expressions in the first place.

In the future it would be good to redesign the plugin API by terraform-linters/tflint-plugin-sdk#193.