Check for tags on AWS resources

[bwhaley]

This PR adds code generated rules for checking resources for named AWS tags.

In config, you can specify a tags attribute in the config block:

config {
    tags = ["foo", "bar"]
}

All the AWS resources with tags supported will be checked for tags matching these keys.

This addresses #150.

[wata727]

Thanks for your awesome contribution! Let me ask some questions.

First of all, what do you think about integrating these tags rules together? In my understanding, these rules don't have a difference for each resource, so implementing them as a single rule is better in terms of maintainability.

Secondly, What do you think about implementing tags config as rule configurations? The attributes of the top-level config block should describe the overall behavior, and I want to write the configuration for a particular rule in a per-rule block. e.g:

rule "aws_resource_tags_aws_accessanalyzer_analyzer" {
  tags = ["foo", "bar"]
}

Unfortunately, there is currently no way to write a config for each rule, so we have to do it first, but I think it will be better.

[bwhaley]

First of all, what do you think about integrating these tags rules together? In my understanding, these rules don't have a difference for each resource, so implementing them as a single rule is better in terms of maintainability.

I actually implemented it this way first, but I wasn't able to import github.com/terraform-providers/terraform-provider-aws/aws. Unfortunately, it resulted in a bunch of errors like:

panic: gob: registering duplicate types for "*tfdiags.rpcFriendlyDiag": *tfdiags.rpcFriendlyDiag != *tfdiags.rpcFriendlyDiag

Debugging this led me to this doc, which suggested that maybe depending on the provider in tflint was a bad idea. I saw that there was no other place in tflint where it was importing terraform-provider-aws - only in the tools. We need to be able to walk the resources to find which of them have tags. Otherwise assembling that list would be manual. Hence my approach of code generated rules, although I certainly agree that it's an ugly solution with so much duplication! I'm open to ideas on how to tackle that.

Secondly, What do you think about implementing tags config as rule configurations?

I thought about this too since the top level config block doesn't seem like the right place for an AWS-specific configuration. However, it strikes me as a terrible user experience to define the tags for each type of resource individually in a rule block. What about adding another type of block? E.g.

aws_config = {
    tags = ["foo", "bar"]
}

This would allow for additional AWS-specific resource configs in the future.

Also, should this continue to allow --tag=foo --tag=bar as a CLI option?

[bwhaley]

In the most recent committed I've updated the code generation to create a list of the types of resources that have tags, rather than a duplicated rules file for each type. This seems much cleaner.

What do you think of the idea of a top-level aws_config block as suggested above?

[wata727]

I've updated the code generation to create a list of the types of resources that have tags, rather than a duplicated rules file for each type

Great. In addition, it is a good idea to make this list in NewTagsRules(). Imagine something like https://github.com/terraform-linters/tflint/blob/master/rules/awsrules/models/aws_instance_invalid_type.go

it strikes me as a terrible user experience to define the tags for each type of resource individually in a rule block.

This problem may be solved by not defining rules for each resource. I think there is no reason to split rules for each resource. What do you think?

[bwhaley]

This is a good idea! I will work on it tomorrow. 👍🏻

[wata727]

@bwhaley I'm now working on a more flexible rule configuration in #624. If this were merged, you can define the configuration of the tags into the rule block in the same way.

[bwhaley]

Oh, okay, I'll wait for that to be merged then. I have the single rule version ready to go now. Just need to know how to load the config. Will you update the docs to explain the change in #624? I don't quite follow how it will work yet.

[wata727]

Maybe this feature is as simple as no need to update the developer guide. You just need to make the struct of the configuration you want to reference in the rule and pass it to runner.DecodeRuleConfig.

The only caveat is that the struct must have tags for hcl package, but it's best to refer to the hcl documentation: https://github.com/hashicorp/hcl/blob/v2.3.0/gohcl/doc.go

[bwhaley]

I've incorporated runner.DecodeRuleConfig. Let me know what you think.

[wata727]

Looks good. I left some comments about the behavior of the rule, please check them.

[bwhaley]

I've incorporated your feedback. Given a resource like this:

resource "aws_instance" "instance" {
  instance_type = "m5.large"
  tags = {
    foo = "bar"
    Bar = "baz"
    Baz = "qux"
  }
}

and a configuration like this:

rule "aws_resource_missing_tags" {
  enabled = true
  tags = ["Foo", "Bar"]
}

it produces an error like this:

1 issue(s) found:

Notice: aws_instance.instance is missing the following tags: "Foo". (aws_resource_missing_tags)

  on test.tf line 3:
   3:   tags = {
   4:     foo = "bar"
   5:     Bar = "baz"
   6:     Baz = "qux"
   7:   }

Reference: https://github.com/terraform-linters/tflint/blob/v0.14.0/docs/rules/aws_resource_missing_tags.md

Note that in order to access resource data in the Check() func, I had to essentially duplicate WalkResourceAttributes to call walker() with the active resource. Let me know if you'd suggest another approach on that.

[wata727]

Umm... you are right. the current API has no way to access the resource details.

Temporarily creating a similar API is not a bad idea, but given the current situation, it might be a good idea to substitute an ambiguous message like "The resource". What do you think?

[bwhaley]

Well I did like your suggestion to reference which resource was missing tags. Without it, the user will have to search through all the code to find which resource is missing them. A few options come to mind:

1. as you suggest, simplify to “the resource”
2. keep the duplicated code
3. update the API to include the resource (would entail changes to all the rules)
4. It could at least mention the resource type since I am crawling all of them that support tags. Only the resource name is unknown

[wata727]

In my opinion, the location (file name, line number) is always displayed, so the resource name is not always important. So I would like to suggest 1.

[bwhaley]

Okay, let me know if you have any further comments.

[wata727]

👍