Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Resource Tags rule #639

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

marckysharky
Copy link

@marckysharky marckysharky commented May 7, 2024

Add aws_resource_tags rule to support enforcing of specific values for the given tag keys.

Subsumes the existing aws_resource_missing_tags rule.

@marckysharky marckysharky force-pushed the invalid-tags-rule branch 2 times, most recently from 526d22f to e39d0a0 Compare May 7, 2024 14:02
@marckysharky marckysharky reopened this May 7, 2024
@wata727
Copy link
Member

wata727 commented May 12, 2024

Intuitively, this is a superset of the aws_resource_missing_tags rule, so it seems to make sense to extend it. What do you think?

@marckysharky
Copy link
Author

@wata727 what are you referring to?
The code implementation, extension of the existing rule and modifying the interface, or both/something else?

@bendrucker
Copy link
Member

If nothing else aws_resource_invalid_tags is a confusing name. Tags are optional so it's reasonable clear "missing" is a user-level construct. "Invalid" suggests they would not be accepted by AWS.

I would agree that it doesn't make sense to have one rule to require tag keys and a separate one for tag values. Seems like they should be consolidated into aws_resource_tags and the existing "missing" rule deprecated:

rule "aws_resource_tags" {
  required = ["Foo"]

  values = {
    Foo = ["Bar"]
    Baz = ["Qux"] # optional
  }
}

@marckysharky
Copy link
Author

marckysharky commented May 14, 2024

If we're happy to implement a single rule to provide both features for tags I would extend upon @bendrucker's comment above and propose the following:

rule "aws_resource_tags" {
  rules = {
    Foo = { values: ["Bar"], required: true },
    Baz = { values: ["Qux"] }, # not required by default
  }
}

The AWS Docs specify some hard restrictions on the tags which could be configured with an additionally adjusted DSL:

rule "aws_resource_tags" {
  tags_max_count       = 10, # default value from doc
  key_max_length       = 32, # default value from doc
  value_max_length     = 128, # default value from doc
  character_allow_list = ["+", "-", "="], # default value from doc
  
  rules = {
    Foo = { allowed_prefixes = ["foo:"] }, # support per-tag value prefix matching
    Baz = { allowed_characters = ["+", "-"] }, # support per-tag restriction of valid characters
    Xar = { max_length = 3 }, # support per-tag character length restriction
  }
}

NOTE: I am not suggesting the future implementation in this PR, just the former to meet the existing and proposed functionality:

  • presence check
  • value restrictions

@bendrucker
Copy link
Member

required can be readily paired with forbidden in a flat configuration. Not a fan of putting a rules key in the configuration for a rule as it starts to overload "rule."

I don't expect we'll ever pursue all these knobs around allowed characters, length, etc. We don't need to create space for that at the expense of straightforward cases. It's an option, but we shouldn't be creating an awkward API with the expectation of extending it substantially.

A good practical goal here would be to support an Environment tag. It would have a few known values and should be used over equivalents like Env.

@marckysharky marckysharky changed the title Add Resource Invalid Tags rule Add Resource Tags rule Jun 11, 2024
@marckysharky
Copy link
Author

Updated MR to merge the tag concepts of required and value validation into a single aws_resource_tags rule.

@bendrucker bendrucker self-requested a review June 11, 2024 17:19
@wata727
Copy link
Member

wata727 commented Jun 16, 2024

The aws_resource_missing_tags rule cannot be removed without warning, so I recommend introducing the new rule first and allowing a transition period.

Copy link
Member

@bendrucker bendrucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The aws_resource_missing_tags rule cannot be removed without warning

Yep, this would be a breaking change which we'd want to defer for a bit.

There isn't a straightforward way to deprecate a rule, arguably that's something we should work on. Rules have no readily accessible way to write to the CLI's stdout since logs are suppressed by default without a value for TFLINT_LOG.

Logging a warning seems like your only option for now.

@@ -0,0 +1,47 @@
# aws_resource_tags

Rule for resources tag presence and value validation from prefixed list.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Rule for resources tag presence and value validation from prefixed list.
Enforce required tag keys and restrict tag values.

"Prefixed" connotes a string prefix whereas I'm pretty sure you just mean fixed/exact.

}

provider "aws" {
...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
...
# ...

Ensure that code samples are valid


## Why

Enforce standard tag values across all resources.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Enforce standard tag values across all resources.
Organizations commonly use conventional AWS tags to track resources, such as `Environment`, `Department`, or `Service`. Example AWS features that depend on having a well-known set of tags include:
* [Cost allocation](https://docs.aws.amazon.com/awsaccountbilling/latest/aboutv2/cost-alloc-tags.html)
* [ABAC](https://docs.aws.amazon.com/IAM/latest/UserGuide/introduction_attribute-based-access-control.html)

Make sure Why tells the user why they should care about enforcing this and doesn't just restate the rule description.


## How To Fix

Align the provider, resource or autoscaling group tags to the configured expectation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Align the provider, resource or autoscaling group tags to the configured expectation.
Set the required tag keys/values, either as `default_tags` on the `provider` or as `tags` on the resource itself.

return keys, true
}
return keys, !value.ForEachElement(func(key, _ cty.Value) bool {
// If any key is unknown or sensitive, return early as any missing tag could be this unknown key.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// If any key is unknown or sensitive, return early as any missing tag could be this unknown key.
// If any key is unknown or sensitive, return early as any missing keys could be this unknown key.

})
}

func stringInSlice(a string, list []string) bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return tags, true
}

return tags, !value.ForEachElement(func(key, value cty.Value) bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still has a lot of references to tags

@wata727
Copy link
Member

wata727 commented Jun 19, 2024

There isn't a straightforward way to deprecate a rule

Good point, I've opened an issue in the SDK to track this terraform-linters/tflint-plugin-sdk#331

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants