Skip to content

Commit

Permalink
Handle eval errors on the plugin side (#1700)
Browse files Browse the repository at this point in the history
  • Loading branch information
@wata727
wata727 authored Feb 25, 2023
1 parent e6f2cce commit 79c6e09
Show file tree
Hide file tree
Showing 6 changed files with 110 additions and 73 deletions.
14 changes: 12 additions & 2 deletions cmd/inspect.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,9 +188,19 @@ func (cli *CLI) inspectModule(opts Options, dir string, filterFiles []string) (t
}

// Run inspection
for _, ruleset := range rulesetPlugin.RuleSets {
for name, ruleset := range rulesetPlugin.RuleSets {
sdkVersion, err := ruleset.SDKVersion()
if err != nil {
if st, ok := status.FromError(err); ok && st.Code() == codes.Unimplemented {
// SDKVersion endpoint is available in tflint-plugin-sdk v0.14+.
// Use nil if not available.
} else {
return tflint.Issues{}, fmt.Errorf("Failed to get TFLint version constraints to `%s` plugin; %w", name, err)
}
}

for _, runner := range runners {
err = ruleset.Check(plugin.NewGRPCServer(runner, rootRunner, cli.loader.Files()))
err = ruleset.Check(plugin.NewGRPCServer(runner, rootRunner, cli.loader.Files(), sdkVersion))
if err != nil {
return tflint.Issues{}, fmt.Errorf("Failed to check ruleset; %w", err)
}
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ require (
github.com/sourcegraph/go-lsp v0.0.0-20200429204803-219e11d77f5d
github.com/sourcegraph/jsonrpc2 v0.1.0
github.com/spf13/afero v1.9.3
github.com/terraform-linters/tflint-plugin-sdk v0.15.0
github.com/terraform-linters/tflint-plugin-sdk v0.15.1-0.20230225141907-dd804b3671af
github.com/terraform-linters/tflint-ruleset-terraform v0.2.2
github.com/xeipuuv/gojsonschema v1.2.0
github.com/zclconf/go-cty v1.12.1
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -457,8 +457,8 @@ github.com/stretchr/testify v1.7.2/go.mod h1:R6va5+xMeoiuVRoj+gSkQ7d3FALtqAAGI1F
github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
github.com/stretchr/testify v1.8.1 h1:w7B6lhMri9wdJUVmEZPGGhZzrYTPvgJArz7wNPgYKsk=
github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4=
github.com/terraform-linters/tflint-plugin-sdk v0.15.0 h1:bUJ9OskzT/I98XaJ5+rs7ymVPHiGT8oI4bG86LkopVY=
github.com/terraform-linters/tflint-plugin-sdk v0.15.0/go.mod h1:enH5i7SHelcvC2AGZavEJzcrRF7nhAaOwTdaBjr/Zjo=
github.com/terraform-linters/tflint-plugin-sdk v0.15.1-0.20230225141907-dd804b3671af h1:TAsqOUKu3DXg6ZmV3igB8ksKkHkaQrdSdZfCE3Ff7nc=
github.com/terraform-linters/tflint-plugin-sdk v0.15.1-0.20230225141907-dd804b3671af/go.mod h1:g5UIXcskejxp38JWqvYqEb/HkvIX6X6luEdS60yimTw=
github.com/terraform-linters/tflint-ruleset-terraform v0.2.2 h1:iTE09KkaZ0DE29xvp6IIM1/gmas9V0h8CER28SyBmQ8=
github.com/terraform-linters/tflint-ruleset-terraform v0.2.2/go.mod h1:bCkvH8Vqzr16bWEE3e6Q3hvdZlmSAOR8i6G3M5y+M+k=
github.com/ulikunitz/xz v0.5.10 h1:t92gobL9l3HE202wg3rlk19F6X+JOxl9BBrCCMYEYd8=
Expand Down
45 changes: 30 additions & 15 deletions langserver/handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"runtime"
"strings"

"github.com/hashicorp/go-version"
"github.com/hashicorp/hcl/v2"
lsp "github.com/sourcegraph/go-lsp"
"github.com/sourcegraph/jsonrpc2"
Expand Down Expand Up @@ -43,6 +44,7 @@ func NewHandler(configPath string, cliConfig *tflint.Config) (jsonrpc2.Handler,
}

rulesets := []tflint.RuleSet{}
clientSDKVersions := map[string]*version.Version{}
for name, ruleset := range rulsetPlugin.RuleSets {
constraints, err := ruleset.VersionConstraints()
if err != nil {
Expand All @@ -56,31 +58,44 @@ func NewHandler(configPath string, cliConfig *tflint.Config) (jsonrpc2.Handler,
if !constraints.Check(tflint.Version) {
return nil, nil, fmt.Errorf("Failed to satisfy version constraints; tflint-ruleset-%s requires %s, but TFLint version is %s", name, constraints, tflint.Version)
}

clientSDKVersions[name], err = ruleset.SDKVersion()
if err != nil {
if st, ok := status.FromError(err); ok && st.Code() == codes.Unimplemented {
// SDKVersion endpoint is available in tflint-plugin-sdk v0.14+.
// Use nil if not available.
} else {
return nil, nil, fmt.Errorf("Failed to get SDK version of `%s` plugin; %w", name, err)
}
}

rulesets = append(rulesets, ruleset)
}
if err := cliConfig.ValidateRules(rulesets...); err != nil {
return nil, nil, err
}

return jsonrpc2.HandlerWithError((&handler{
configPath: configPath,
cliConfig: cliConfig,
config: cfg,
fs: afero.NewCopyOnWriteFs(afero.NewOsFs(), afero.NewMemMapFs()),
plugin: rulsetPlugin,
diagsPaths: []string{},
configPath: configPath,
cliConfig: cliConfig,
config: cfg,
fs: afero.NewCopyOnWriteFs(afero.NewOsFs(), afero.NewMemMapFs()),
plugin: rulsetPlugin,
clientSDKVersions: clientSDKVersions,
diagsPaths: []string{},
}).handle), rulsetPlugin, nil
}

type handler struct {
configPath string
cliConfig *tflint.Config
config *tflint.Config
fs afero.Fs
rootDir string
plugin *plugin.Plugin
shutdown bool
diagsPaths []string
configPath string
cliConfig *tflint.Config
config *tflint.Config
fs afero.Fs
rootDir string
plugin *plugin.Plugin
clientSDKVersions map[string]*version.Version
shutdown bool
diagsPaths []string
}

func (h *handler) handle(ctx context.Context, conn *jsonrpc2.Conn, req *jsonrpc2.Request) (result interface{}, err error) {
Expand Down Expand Up @@ -210,7 +225,7 @@ func (h *handler) inspect() (map[string][]lsp.Diagnostic, error) {
return ret, fmt.Errorf("Failed to apply config to `%s` plugin", name)
}
for _, runner := range runners {
err = ruleset.Check(plugin.NewGRPCServer(runner, runners[len(runners)-1], loader.Files()))
err = ruleset.Check(plugin.NewGRPCServer(runner, runners[len(runners)-1], loader.Files(), h.clientSDKVersions[name]))
if err != nil {
return ret, fmt.Errorf("Failed to check ruleset: %w", err)
}
Expand Down
17 changes: 12 additions & 5 deletions plugin/server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"log"

"github.com/hashicorp/go-version"
hcl "github.com/hashicorp/hcl/v2"
"github.com/terraform-linters/tflint-plugin-sdk/hclext"
sdk "github.com/terraform-linters/tflint-plugin-sdk/tflint"
Expand All @@ -15,14 +16,15 @@ import (

// GRPCServer is a gRPC server for responding to requests from plugins.
type GRPCServer struct {
runner *tflint.Runner
rootRunner *tflint.Runner
files map[string]*hcl.File
runner *tflint.Runner
rootRunner *tflint.Runner
files map[string]*hcl.File
clientSDKVersion *version.Version
}

// NewGRPCServer initializes a gRPC server for plugins.
func NewGRPCServer(runner *tflint.Runner, rootRunner *tflint.Runner, files map[string]*hcl.File) *GRPCServer {
return &GRPCServer{runner: runner, rootRunner: rootRunner, files: files}
func NewGRPCServer(runner *tflint.Runner, rootRunner *tflint.Runner, files map[string]*hcl.File, sdkVersion *version.Version) *GRPCServer {
return &GRPCServer{runner: runner, rootRunner: rootRunner, files: files, clientSDKVersion: sdkVersion}
}

// GetOriginalwd returns the original working directory.
Expand Down Expand Up @@ -136,6 +138,11 @@ func (s *GRPCServer) EvaluateExpr(expr hcl.Expression, opts sdk.EvaluateExprOpti
return cty.NullVal(cty.NilType), err
}

// SDK v0.16+ introduces client-side handling of unknown and NULL values.
if s.clientSDKVersion != nil && s.clientSDKVersion.GreaterThanOrEqual(version.Must(version.NewVersion("0.16.0"))) {
return val, nil
}

if *opts.WantType == cty.DynamicPseudoType {
return val, nil
}
Expand Down
101 changes: 53 additions & 48 deletions plugin/server_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,19 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/hashicorp/go-version"
hcl "github.com/hashicorp/hcl/v2"
"github.com/hashicorp/hcl/v2/hclsyntax"
"github.com/spf13/afero"
"github.com/terraform-linters/tflint-plugin-sdk/hclext"
"github.com/terraform-linters/tflint-plugin-sdk/plugin/host2plugin"
sdk "github.com/terraform-linters/tflint-plugin-sdk/tflint"
"github.com/terraform-linters/tflint/tflint"
"github.com/zclconf/go-cty/cty"
)

var SDKVersion = version.Must(version.NewVersion(host2plugin.SDKVersion))

func TestGetModuleContent(t *testing.T) {
runner := tflint.TestRunner(t, map[string]string{"main.tf": `
resource "aws_instance" "foo" {
Expand All @@ -43,7 +47,7 @@ resource "aws_instance" "bar" {
instance_type = "m5.2xlarge"
}`})

server := NewGRPCServer(runner, rootRunner, runner.Files())
server := NewGRPCServer(runner, rootRunner, runner.Files(), SDKVersion)

tests := []struct {
Name string
Expand Down Expand Up @@ -257,7 +261,7 @@ resource "aws_instance" "foo" {
files[name] = file
}

server := NewGRPCServer(runner, rootRunner, files)
server := NewGRPCServer(runner, rootRunner, files, SDKVersion)

tests := []struct {
Name string
Expand Down Expand Up @@ -324,7 +328,7 @@ resource "aws_instance" "bar" {
instance_type = "m5.2xlarge"
}`})

server := NewGRPCServer(runner, rootRunner, runner.Files())
server := NewGRPCServer(runner, rootRunner, runner.Files(), SDKVersion)

tests := []struct {
Name string
Expand Down Expand Up @@ -388,7 +392,7 @@ rule "test_in_file" {
fileConfig.Merge(cliConfig)
runner := tflint.TestRunnerWithConfig(t, map[string]string{}, fileConfig)

server := NewGRPCServer(runner, nil, runner.Files())
server := NewGRPCServer(runner, nil, runner.Files(), SDKVersion)

// default error check helper
neverHappend := func(err error) bool { return err != nil }
Expand Down Expand Up @@ -492,7 +496,9 @@ variable "foo" {
default = "baz"
}`})

server := NewGRPCServer(runner, rootRunner, runner.Files())
server := NewGRPCServer(runner, rootRunner, runner.Files(), SDKVersion)

sdkv15 := version.Must(version.NewVersion("0.15.0"))

// test util functions
hclExpr := func(expr string) hcl.Expression {
Expand All @@ -510,10 +516,11 @@ variable "foo" {
neverHappend := func(err error) bool { return err != nil }

tests := []struct {
Name string
Args func() (hcl.Expression, sdk.EvaluateExprOption)
Want cty.Value
ErrCheck func(error) bool
Name string
Args func() (hcl.Expression, sdk.EvaluateExprOption)
SDKVersion *version.Version
Want cty.Value
ErrCheck func(error) bool
}{
{
Name: "self module context",
Expand Down Expand Up @@ -557,26 +564,37 @@ variable "foo" {
Args: func() (hcl.Expression, sdk.EvaluateExprOption) {
return hclExpr(`var.no_default`), sdk.EvaluateExprOption{WantType: &cty.String, ModuleCtx: sdk.SelfModuleCtxType}
},
Want: cty.NullVal(cty.NilType),
Want: cty.UnknownVal(cty.String),
ErrCheck: neverHappend,
},
{
Name: "no default (SDK v0.15)",
Args: func() (hcl.Expression, sdk.EvaluateExprOption) {
return hclExpr(`var.no_default`), sdk.EvaluateExprOption{WantType: &cty.String, ModuleCtx: sdk.SelfModuleCtxType}
},
SDKVersion: sdkv15,
Want: cty.NullVal(cty.NilType),
ErrCheck: func(err error) bool {
return err == nil || !errors.Is(err, sdk.ErrUnknownValue)
},
},
{
Name: "no default as cty.Value",
Name: "no default as cty.Value (SDK v0.15)",
Args: func() (hcl.Expression, sdk.EvaluateExprOption) {
return hclExpr(`var.no_default`), sdk.EvaluateExprOption{WantType: &cty.DynamicPseudoType, ModuleCtx: sdk.SelfModuleCtxType}
},
Want: cty.DynamicVal,
ErrCheck: neverHappend,
SDKVersion: sdkv15,
Want: cty.DynamicVal,
ErrCheck: neverHappend,
},
{
Name: "no default value in object",
Name: "no default value in object (SDK v0.15)",
Args: func() (hcl.Expression, sdk.EvaluateExprOption) {
ty := cty.Object(map[string]cty.Type{"value": cty.String})
return hclExpr(`{ value = var.no_default }`), sdk.EvaluateExprOption{WantType: &ty, ModuleCtx: sdk.SelfModuleCtxType}
},
Want: cty.NullVal(cty.NilType),
SDKVersion: sdkv15,
Want: cty.NullVal(cty.NilType),
ErrCheck: func(err error) bool {
return err == nil || !errors.Is(err, sdk.ErrUnknownValue)
},
Expand All @@ -586,63 +604,50 @@ variable "foo" {
Args: func() (hcl.Expression, sdk.EvaluateExprOption) {
return hclExpr(`var.null`), sdk.EvaluateExprOption{WantType: &cty.String, ModuleCtx: sdk.SelfModuleCtxType}
},
Want: cty.NullVal(cty.NilType),
ErrCheck: func(err error) bool {
return err == nil || !errors.Is(err, sdk.ErrNullValue)
},
},
{
Name: "null as cty.Value",
Args: func() (hcl.Expression, sdk.EvaluateExprOption) {
return hclExpr(`var.null`), sdk.EvaluateExprOption{WantType: &cty.DynamicPseudoType, ModuleCtx: sdk.SelfModuleCtxType}
},
Want: cty.NullVal(cty.String),
ErrCheck: neverHappend,
},
{
Name: "null value in object",
Name: "null (SDK v0.15)",
Args: func() (hcl.Expression, sdk.EvaluateExprOption) {
ty := cty.Object(map[string]cty.Type{"value": cty.String})
return hclExpr(`{ value = var.null }`), sdk.EvaluateExprOption{WantType: &ty, ModuleCtx: sdk.SelfModuleCtxType}
return hclExpr(`var.null`), sdk.EvaluateExprOption{WantType: &cty.String, ModuleCtx: sdk.SelfModuleCtxType}
},
Want: cty.NullVal(cty.NilType),
SDKVersion: sdkv15,
Want: cty.NullVal(cty.NilType),
ErrCheck: func(err error) bool {
return err == nil || !errors.Is(err, sdk.ErrNullValue)
},
},
{
Name: "unevaluable",
Name: "null as cty.Value (SDK v0.15)",
Args: func() (hcl.Expression, sdk.EvaluateExprOption) {
return hclExpr(`module.instance.output`), sdk.EvaluateExprOption{WantType: &cty.String, ModuleCtx: sdk.SelfModuleCtxType}
},
Want: cty.NullVal(cty.NilType),
ErrCheck: func(err error) bool {
return err == nil || !errors.Is(err, sdk.ErrUnknownValue)
},
},
{
Name: "unevaluable as cty.Value",
Args: func() (hcl.Expression, sdk.EvaluateExprOption) {
return hclExpr(`module.instance.output`), sdk.EvaluateExprOption{WantType: &cty.DynamicPseudoType, ModuleCtx: sdk.SelfModuleCtxType}
return hclExpr(`var.null`), sdk.EvaluateExprOption{WantType: &cty.DynamicPseudoType, ModuleCtx: sdk.SelfModuleCtxType}
},
Want: cty.DynamicVal,
ErrCheck: neverHappend,
SDKVersion: sdkv15,
Want: cty.NullVal(cty.String),
ErrCheck: neverHappend,
},
{
Name: "unevaluable value in object",
Name: "null value in object (SDK v0.15)",
Args: func() (hcl.Expression, sdk.EvaluateExprOption) {
ty := cty.Object(map[string]cty.Type{"value": cty.String})
return hclExpr(`{ value = module.instance.output }`), sdk.EvaluateExprOption{WantType: &ty, ModuleCtx: sdk.SelfModuleCtxType}
return hclExpr(`{ value = var.null }`), sdk.EvaluateExprOption{WantType: &ty, ModuleCtx: sdk.SelfModuleCtxType}
},
Want: cty.NullVal(cty.NilType),
Want: cty.NullVal(cty.NilType),
SDKVersion: sdkv15,
ErrCheck: func(err error) bool {
return err == nil || !errors.Is(err, sdk.ErrUnknownValue)
return err == nil || !errors.Is(err, sdk.ErrNullValue)
},
},
}

for _, test := range tests {
t.Run(test.Name, func(t *testing.T) {
if test.SDKVersion == nil {
test.SDKVersion = SDKVersion
}
server.clientSDKVersion = test.SDKVersion

got, err := server.EvaluateExpr(test.Args())
if test.ErrCheck(err) {
t.Fatalf("failed to call EvaluateExpr: %s", err)
Expand Down Expand Up @@ -721,7 +726,7 @@ resource "aws_instance" "foo" {
t.Run(test.Name, func(t *testing.T) {
runner := tflint.TestRunner(t, map[string]string{"main.tf": config})

server := NewGRPCServer(runner, nil, runner.Files())
server := NewGRPCServer(runner, nil, runner.Files(), SDKVersion)

err := server.EmitIssue(test.Args())
if err != nil {
Expand Down

0 comments on commit 79c6e09

Please sign in to comment.