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 aws_iam_role_deprecated_policy_attributes rule #833

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

Conversation

alexjfisher
Copy link

No description provided.

}
}

for _, rule := range resource.Body.Blocks {
Copy link
Member

Choose a reason for hiding this comment

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

Avoid assuming that inline_policies is the only block type and explicitly check that in this loop and skip other blocks.

Copy link
Author

Choose a reason for hiding this comment

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

I'll look into this. Is this just a safety check, or a really bad assumption I've made with how the schema passed to GetResourceContent works?? I think I was assuming the only block type I'd ever see is for inline_policy. But this is my first attempt at writing a plugin, (and I barely know any go!)

Copy link
Member

Choose a reason for hiding this comment

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

I understand this as a safety check that asserts expectations.

Copy link
Member

Choose a reason for hiding this comment

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

Just a safety check!

attribute, exists := resource.Body.Attributes["managed_policy_arns"]

if exists {
if err := runner.EmitIssue(r, "The managed_policy_arns argument is deprecated. Use the aws_iam_role_policy_attachment resource instead. If Terraform should exclusively manage all managed policy attachments (the current behavior of this argument), use the aws_iam_role_policy_attachments_exclusive resource as well.", attribute.Range); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I would flip these. Users should prefer the exclusive resources by default and fall back on the non-authoritative versions if they want to attach policies separate from the role definition.

Copy link
Author

Choose a reason for hiding this comment

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

The warning messages were taken verbatim from the iam_role documentation page. https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role

If you want exclusive behaviour, you have to use both aws_iam_role_policy_attachments_exclusive and aws_iam_role_policy_attachment resources.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, in that case quoting the provider docs is fine

}

for _, rule := range resource.Body.Blocks {
if err := runner.EmitIssue(r, "The inline_policy argument is deprecated. Use the aws_iam_role_policy resource instead. If Terraform should exclusively manage all inline policy associations (the current behavior of this argument), use the aws_iam_role_policies_exclusive resource as well.", rule.DefRange); err != nil {
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 err := runner.EmitIssue(r, "The inline_policy argument is deprecated. Use the aws_iam_role_policy resource instead. If Terraform should exclusively manage all inline policy associations (the current behavior of this argument), use the aws_iam_role_policies_exclusive resource as well.", rule.DefRange); err != nil {
if err := runner.EmitIssue(r, "The inline_policy block is deprecated. Use the aws_iam_role_policy resource instead. If Terraform should exclusively manage all inline policy associations (the current behavior of this argument), use the aws_iam_role_policies_exclusive resource as well.", rule.DefRange); err != nil {

Copy link
Author

Choose a reason for hiding this comment

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

Again, taken verbatim from upstream resource docs, but I can't disagree, block is more accurate, so I can still change it if you prefer.

The warning messages have been copied verbatim from the upstream
documentation of the `aws_iam_role` resource.
@alexjfisher alexjfisher force-pushed the aws_iam_role_deprecated_policy_attributes branch from 57cd57a to ff01475 Compare February 26, 2025 14:43
@wata727
Copy link
Member

wata727 commented Mar 6, 2025

Thanks for working on this. However, it looks like these deprecation warnings have already been addressed by the AWS provider.
https://github.com/hashicorp/terraform-provider-aws/blob/v5.89.0/internal/service/iam/role.go#L94-L98
https://github.com/hashicorp/terraform-provider-aws/blob/v5.89.0/internal/service/iam/role.go#L134-L138

Given this, it seems like there would be no point in introducing the same rule in TFLint, but what use cases do you have?

@bendrucker
Copy link
Member

terraform validate will print warnings for deprecated provider attributes. Duplicating that in TFLint is costly. It gives you control over disabling the rule or ignoring it for specific files/lines. But it does make me think twice about adding the rule.

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