Skip to content

Commit

Permalink
Merge pull request #443 from wata727/handle_invalid_references
Browse files Browse the repository at this point in the history 
Fix panic when the expression includes invalid references
  • Loading branch information
@wata727
wata727 authored Sep 18, 2019
2 parents e19349d + c1870d4 commit d6d47e7
Show file tree
Hide file tree
Showing 4 changed files with 114 additions and 26 deletions.
7 changes: 6 additions & 1 deletion rules/awsrules/aws_route_not_specified_target.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,12 @@ func (r *AwsRouteNotSpecifiedTargetRule) Check(runner *tflint.Runner) error {

var nullAttributes int
for _, attribute := range body.Attributes {
if runner.IsNullExpr(attribute.Expr) {
isNull, err := runner.IsNullExpr(attribute.Expr)
if err != nil {
return err
}

if isNull {
nullAttributes = nullAttributes + 1
}
}
Expand Down
7 changes: 6 additions & 1 deletion rules/awsrules/aws_route_specified_multiple_targets.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,12 @@ func (r *AwsRouteSpecifiedMultipleTargetsRule) Check(runner *tflint.Runner) erro

var nullAttributes int
for _, attribute := range body.Attributes {
if runner.IsNullExpr(attribute.Expr) {
isNull, err := runner.IsNullExpr(attribute.Expr)
if err != nil {
return err
}

if isNull {
nullAttributes = nullAttributes + 1
}
}
Expand Down
80 changes: 60 additions & 20 deletions tflint/runner.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,12 @@ func NewModuleRunners(parent *Runner) ([]*Runner, error) {
modVars := map[string]*moduleVariable{}
for varName, rawVar := range cfg.Module.Variables {
if attribute, exists := attributes[varName]; exists {
if isEvaluableExpr(attribute.Expr) {
evalauble, err := isEvaluableExpr(attribute.Expr)
if err != nil {
return runners, err
}

if evalauble {
val, diags := parent.ctx.EvaluateExpr(attribute.Expr, cty.DynamicPseudoType, nil)
if diags.HasErrors() {
err := &Error{
Expand Down Expand Up @@ -204,7 +209,23 @@ func NewModuleRunners(parent *Runner) ([]*Runner, error) {
// When it received slice as `ret`, it converts cty.Value to expected list type
// because raw cty.Value has TupleType.
func (r *Runner) EvaluateExpr(expr hcl.Expression, ret interface{}) error {
if !isEvaluableExpr(expr) {
evaluable, err := isEvaluableExpr(expr)
if err != nil {
err := &Error{
Code: EvaluationError,
Level: ErrorLevel,
Message: fmt.Sprintf(
"Failed to parse an expression in %s:%d",
expr.Range().Filename,
expr.Range().Start.Line,
),
Cause: err,
}
log.Printf("[ERROR] %s", err)
return err
}

if !evaluable {
err := &Error{
Code: UnevaluableError,
Level: WarningLevel,
Expand Down Expand Up @@ -234,7 +255,7 @@ func (r *Runner) EvaluateExpr(expr hcl.Expression, ret interface{}) error {
return err
}

err := cty.Walk(val, func(path cty.Path, v cty.Value) (bool, error) {
err = cty.Walk(val, func(path cty.Path, v cty.Value) (bool, error) {
if !v.IsKnown() {
err := &Error{
Code: UnknownValueError,
Expand Down Expand Up @@ -320,7 +341,23 @@ func (r *Runner) EvaluateExpr(expr hcl.Expression, ret interface{}) error {

// EvaluateBlock is a wrapper of terraform.BultinEvalContext.EvaluateBlock and gocty.FromCtyValue
func (r *Runner) EvaluateBlock(block *hcl.Block, schema *configschema.Block, ret interface{}) error {
if !isEvaluableBlock(block.Body, schema) {
evaluable, err := isEvaluableBlock(block.Body, schema)
if err != nil {
err := &Error{
Code: EvaluationError,
Level: ErrorLevel,
Message: fmt.Sprintf(
"Failed to parse a block in %s:%d",
block.DefRange.Filename,
block.DefRange.Start.Line,
),
Cause: err,
}
log.Printf("[ERROR] %s", err)
return err
}

if !evaluable {
err := &Error{
Code: UnevaluableError,
Level: WarningLevel,
Expand Down Expand Up @@ -350,7 +387,7 @@ func (r *Runner) EvaluateBlock(block *hcl.Block, schema *configschema.Block, ret
return err
}

err := cty.Walk(val, func(path cty.Path, v cty.Value) (bool, error) {
err = cty.Walk(val, func(path cty.Path, v cty.Value) (bool, error) {
if !v.IsKnown() {
err := &Error{
Code: UnknownValueError,
Expand Down Expand Up @@ -564,15 +601,20 @@ func (r *Runner) EnsureNoError(err error, proc func() error) error {
}

// IsNullExpr check the passed expression is null
func (r *Runner) IsNullExpr(expr hcl.Expression) bool {
if !isEvaluableExpr(expr) {
return false
func (r *Runner) IsNullExpr(expr hcl.Expression) (bool, error) {
evaluable, err := isEvaluableExpr(expr)
if err != nil {
return false, err
}

if !evaluable {
return false, nil
}
val, diags := r.ctx.EvaluateExpr(expr, cty.DynamicPseudoType, nil)
if diags.HasErrors() {
return false
return false, diags.Err()
}
return val.IsNull()
return val.IsNull(), nil
}

// LookupResourcesByType returns `configs.Resource` list according to the resource type
Expand Down Expand Up @@ -670,32 +712,30 @@ func prepareVariableValues(configVars map[string]*configs.Variable, variables ..
return variableValues
}

func isEvaluableExpr(expr hcl.Expression) bool {
func isEvaluableExpr(expr hcl.Expression) (bool, error) {
refs, diags := lang.ReferencesInExpr(expr)
if diags.HasErrors() {
// Maybe this is bug
panic(diags.Err())
return false, diags.Err()
}
for _, ref := range refs {
if !isEvaluableRef(ref) {
return false
return false, nil
}
}
return true
return true, nil
}

func isEvaluableBlock(body hcl.Body, schema *configschema.Block) bool {
func isEvaluableBlock(body hcl.Body, schema *configschema.Block) (bool, error) {
refs, diags := lang.ReferencesInBlock(body, schema)
if diags.HasErrors() {
// Maybe this is bug
panic(diags.Err())
return false, diags.Err()
}
for _, ref := range refs {
if !isEvaluableRef(ref) {
return false
return false, nil
}
}
return true
return true, nil
}

func isEvaluableRef(ref *addrs.Reference) bool {
Expand Down
46 changes: 42 additions & 4 deletions tflint/runner_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -788,6 +788,7 @@ func Test_isEvaluableExpr(t *testing.T) {
Name string
Content string
Expected bool
Error error
}{
{
Name: "literal",
Expand Down Expand Up @@ -907,13 +908,31 @@ resource "null_resource" "test" {
}`,
Expected: true,
},
{
Name: "invalid reference",
Content: `
resource "null_resource" "test" {
key = invalid
}`,
Expected: false,
Error: errors.New("Invalid reference: A reference to a resource type must be followed by at least one attribute access, specifying the resource name."),
},
}

for _, tc := range cases {
runner := TestRunner(t, map[string]string{"main.tf": tc.Content})

err := runner.WalkResourceAttributes("null_resource", "key", func(attribute *hcl.Attribute) error {
ret := isEvaluableExpr(attribute.Expr)
ret, err := isEvaluableExpr(attribute.Expr)
if err != nil && tc.Error == nil {
t.Fatalf("Failed `%s` test: unexpected error occurred: %s", tc.Name, err)
}
if err == nil && tc.Error != nil {
t.Fatalf("Failed `%s` test: expected error is %s, but no errors", tc.Name, tc.Error)
}
if err != nil && tc.Error != nil && err.Error() != tc.Error.Error() {
t.Fatalf("Failed `%s` test: expected error is %s, but got %s", tc.Name, tc.Error, err)
}
if ret != tc.Expected {
t.Fatalf("Failed `%s` test: expected value is %t, but get %t", tc.Name, tc.Expected, ret)
}
Expand Down Expand Up @@ -1531,6 +1550,7 @@ func Test_IsNullExpr(t *testing.T) {
Name string
Content string
Expected bool
Error error
}{
{
Name: "non null literal",
Expand Down Expand Up @@ -1597,15 +1617,33 @@ resource "null_resource" "test" {
key = "${null}-1"
}`,
Expected: false,
Error: errors.New("Invalid template interpolation value: The expression result is null. Cannot include a null value in a string template."),
},
{
Name: "invalid references",
Content: `
resource "null_resource" "test" {
key = invalid
}`,
Expected: false,
Error: errors.New("Invalid reference: A reference to a resource type must be followed by at least one attribute access, specifying the resource name."),
},
}

for _, tc := range cases {
runner := TestRunner(t, map[string]string{"main.tf": tc.Content})

err := runner.WalkResourceAttributes("null_resource", "test", func(attribute *hcl.Attribute) error {
ret := runner.IsNullExpr(attribute.Expr)

err := runner.WalkResourceAttributes("null_resource", "key", func(attribute *hcl.Attribute) error {
ret, err := runner.IsNullExpr(attribute.Expr)
if err != nil && tc.Error == nil {
t.Fatalf("Failed `%s` test: unexpected error occurred: %s", tc.Name, err)
}
if err == nil && tc.Error != nil {
t.Fatalf("Failed `%s` test: expected error is %s, but no errors", tc.Name, tc.Error)
}
if err != nil && tc.Error != nil && err.Error() != tc.Error.Error() {
t.Fatalf("Failed `%s` test: expected error is %s, but got %s", tc.Name, tc.Error, err)
}
if tc.Expected != ret {
t.Fatalf("Failed `%s` test: expected value is %t, but get %t", tc.Name, tc.Expected, ret)
}
Expand Down

0 comments on commit d6d47e7

Please sign in to comment.