terraform-linters / tflint-ruleset-aws

TFLint ruleset for terraform-provider-aws
Mozilla Public License 2.0
327 stars 71 forks source link

Failed to check ruleset; unevaluable expression found #326

Closed suzuki-shunsuke closed 2 years ago

suzuki-shunsuke commented 2 years ago

Overview

tflint failed due to the error Failed to check ruleset; unevaluable expression found.

Failed to check ruleset; unevaluable expression found in terraform.tf:12

How to reproduce

terraform.tf

terraform {
  required_version = ">= 0.15"
  required_providers {
    aws = {
      source  = "hashicorp/aws"
      version = "4.1.0"
    }
  }
}

provider "aws" {
  region      = local.region # Use local value
  max_retries = 3
}

locals {
  region = "ap-northeast-1"
}

.tflint.hcl

plugin "aws" {
  enabled = true
  version = "0.13.1"
  source  = "github.com/terraform-linters/tflint-ruleset-aws"

  deep_check = true # When deep_check is false, this issue can't be reproduced
}

It fails to run tflint as the following.

$ tflint
Failed to check ruleset; unevaluable expression found in terraform.tf:12

If deep_check is false, the error doesn't occur.

And when tflint and aws plugin are downgraded, the error doesn't occur too.

$ tflint -v
TFLint version 0.34.1
+ ruleset.aws (0.12.0)
$ tflint # the error doesn't occur

Version

$ terraform version
Terraform v1.1.7
on darwin_amd64
+ provider registry.terraform.io/hashicorp/aws v4.1.0
$ tflint -v
TFLint version 0.35.0
+ ruleset.aws (0.13.1)

Reference

bendrucker commented 2 years ago

Tracked in terraform-linters/tflint#571—that this worked before is unexpected/incidental

suzuki-shunsuke commented 2 years ago

I think terraform-linters/tflint#571 and this issue is a little different. terraform-linters/tflint#571 is an improvement but this is a bug. tflint should be able to parse valid Terraform Configuration.

suzuki-shunsuke commented 2 years ago

Skipping is different from returning an error.

wata727 commented 2 years ago

Yes, I think this is the same kind of bug as https://github.com/terraform-linters/tflint-ruleset-aws/issues/323.

suzuki-shunsuke commented 2 years ago

Thank you for your quick fix!

wata727 commented 2 years ago

Fixed in https://github.com/terraform-linters/tflint-ruleset-aws/releases/tag/v0.13.2

suzuki-shunsuke commented 2 years ago

I've confirmed this issue has been solved in v0.13.2.

$ tflint -v
TFLint version 0.35.0
+ ruleset.aws (0.13.2)

$ tflint # the error doesn't occur
bendrucker commented 2 years ago

This has similar mechanics to https://github.com/terraform-linters/tflint-ruleset-aws/issues/323 but it's not really the same situation. When evaluating rules, it makes sense for a static evaluator to skip over dynamic expressions.

In this case configuration is being evaluated to configure the linter itself. While there is partial support for explicit or env variable config, only a provider block supports the full set of configuration options, including assuming roles.

Parsing means correctly interpreting the declaration, not just not erroring. The configuration remains unparseable, the error is just suppressed. For most users, that represents a silent failure. Preferably that should be a returned error unless alternate config is provided. Problem there is that it's valid to have no config, e.g. a default AWS profile.

My inclination here is that you should have to add explicit plugin config if you want to skip over loading config from the provider block, e.g.:

plugin "aws" {
  deep_check         = true
  use_provider_block = false
}
wata727 commented 2 years ago

Parsing means correctly interpreting the declaration, not just not erroring. The configuration remains unparseable, the error is just suppressed. For most users, that represents a silent failure.

Makes sense. There may be room for improvement if the precondition fails.

Originally, the reason for referring to the attributes of the provider block is that I thought that the credentials used by deep checking should be the same as the credentials at running terraform apply.

I'm not sure if it's better to add a config like use_provider_block or just make an error. Perhaps it might be a good idea to simply not refer to the provider block.

bendrucker commented 2 years ago

Originally, the reason for referring to the attributes of the provider block is that I thought that the credentials used by deep checking should be the same as the credentials at running terraform apply.

Yes, I agree with this. For most users this is the most straightforward.

Some users could be referencing provider config that is only available at plan time, e.g. a role ARN from remote state. In those cases, I think it makes the most sense to refuse to parse the provider block entirely and require the user to pass credentials/config. What seems off to me is the idea that some attributes would be parsed but others would be ignored.