fix: Fixed regression in tags with dynamic values

[JorgeReus]

Fixes #510

This fixes the regression when using dynamic values, in a sense.

The root cause is the runner API not recognizing the values, this PR only ignores the error and continues, which was the behavior in 0.23.x

[bendrucker]

LGTM, can you try to get test coverage added here? This should work:

resource "aws_s3_bucket" "a" {
  tags = {
    NotFoo = "bar"
  }
}

resource "aws_s3_bucket" "b" {
  tags = aws_s3_bucket.a.tags
}

Where Foo is a required tag. There should be an issue on the first resource, but not the second. Even if we managed to figure out map key evaluation a la for_each, this kind of reference would always be unknown. And shouldn't run up against issues with lack of support for reading data sources.

You could probably do the same with the provider block's default_tags block, although it'll get slightly contrived. You'd probably have to have a provider alias b used with aws_s3_bucket.b and set the tags in that provider instead of on the resource.

Another option might be to use the sensitive() function, since sensitive values are handled by TFLint in a similar way to unknown/null values, as detailed in EvaluateExpr's docs.

[JorgeReus]

On it!

[JorgeReus]

So, this fails

{
  Name: "Unknown values should be ignored",
  Content: `provider "aws" {
  alias = "ignored"
  region = "us-east-1"
}

resource "aws_s3_bucket" "a" {
  tags = {
    NotFoo = "bar"
  }
}

resource "aws_s3_bucket" "b" {
  provider = aws.ignored
  tags = aws_s3_bucket.a.tags
}

`,
			Config: `
rule "aws_resource_missing_tags" {
  enabled = true
  tags = ["NotFoo"]
}`,
			Expected: helper.Issues{},
},

With

aws_resource_missing_tags_test.go:469: Unexpected error occurred in test "Unknown values should be ignored": module.tf:14,10-23: Unknown variable; There is no variable named "aws_s3_bucket"

Also, neither functions nor locals seem to be available using the test local runner.

Putting in here for visibility https://github.com/hashicorp/terraform/blob/main/internal/terraform/eval_for_each.go

[bendrucker]

Ok, thanks for trying/confirming. A variable is a bit closer to working:

https://github.com/terraform-linters/tflint-plugin-sdk/blob/d10b04fbb82b87caaf87704cd6f0a4f0220f2a62/helper/runner.go#L376-L437

But I just verified that this only works when a default is set such that TFLint can evaluate the variable. If the variable is effectively unknown (no default), it errors:

Unexpected error occurred in test "Unknown values should be ignored": element "Region": string required

However, if you change your EvaluateExpr callbacks to take cty.Value instead, this might be workable. In that case, the code gets a bit more verbose since you'd probably need to use value.AsValueMap() and then convert from a map[string]cty.Value to a map[string]string. It's not great to have to compromise the source code to accommodate the tests but it may be quicker than trying to modify the SDK to handle this in testing. Based on the above results with variable, decoding resources etc isn't actually enough to fix this, as EvaluateExpr is also going to error if it can't assign the argument type when invoked from the test runner.

[bendrucker]

See terraform-linters/tflint-plugin-sdk#64 for the tracking issue that explains why the tests behave different than a real run in which the CLI communicates with the ruleset plugin over RPC. Terraform tackled a similar issue and added a dependency on the Terraform CLI for the provider acceptance testing package, where it used to be a custom runner for testing, IIRC skipping RPC and calling the provider directly via its Go API. They switched to a more "E2E" style that writes each test step's config to a temporary directory and invokes a real Terraform CLI that targets a provider binary built to a temp directory. Even though their previous test runner was more comprehensive than TFLint's, they made the switch for similar reasons—differing behavior between tests and real CLI-driven Terraform runs.

[JorgeReus]

You were completely right @bendrucker! Using cty.Value allowed me to have "full control" of the validation and parsing of the valueMap, also now not wholly known tags maps work!

Added two testcases one for not wholly known and one for completely uknown, LMK if anything has to be changed

[bendrucker]

Nice work here!