provider_missing_default_tags: correctly handle unknown values

[bendrucker]

Fixes a bug where provider_missing_default_tags does not correctly handle unknown tags. Adds the missing test coverage and then fixes the failing tests by correcting the control flow for unknown values.

Cases

Unknown

The incorrect flow:

if !known {
  return nil
}

Doing this inside the EvaluateExpr callback effectively ignores the value and leaves providerTags empty before checking for unset tags. Instead, unknown values need to short circuit the comparison. So I've returned an error, borrowing tflint.UnknownError.

Null

The multiple cases where errors are silently discarded was a red flag:

if err != nil {
  return nil
}

Now, all errors are returned except evaluation errors of the tags attribute. This could be due to an explicit unknown value error or it could be a null value. As in:

provider "aws" {
  default_tags {
    tags = {
      (var.tag): "bar"
    }
  }
}

variable "tag" {
  type = string
}

This is technically not valid configuration, as var.tag is null, and null is not a valid map key. We could just return this error and insist users pass a variable value to TFLint or set a default to make the configuration valid. I initially went that path.

But for the sake of backward compatibility, any error will short circuit checking (as before). But now, the warning log happens outside of the EvalauteExpr callback, for all errors, such that they are no longer wholly silent. If the rule is skipping a tag map because of a null key error, it will print that error in a warning.

Related

Closes #850

[wata727]

Considering that feedback may be lost in case of simple evaluation errors (e.g. undefined variables), would it be better to only short circuit in the tflint.ErrUnknownValue case?

[bendrucker]

Yes, this is an option, that's what I did originally. Ignoring null variable values would be a separate case. What do you think about erroring there, versus trying to ignore it?

[wata727]

I think unknown and null values should be ignored, but other errors should not be ignored. How about the following?

if errors.Is(err, tflint.ErrUnknownValue) {
	logger.Warn("Could not evaluate tags, skipping %s.%s.%s.%s: %s", provider.Labels[0], providerAlias, providerDefaultTagsBlockName, providerTagsAttributeName, err)
	continue
}

[bendrucker]

In spirit I agree, but the null case comes in as a raw diagnostic and not a wrapped/simple error:

https://github.com/hashicorp/hcl/blob/b4e1e146a0c3b53f973064ae3fce7b69f5ef13a0/hclsyntax/expression.go#L1219-L1230