terraform-aws-modules / terraform-aws-iam

Terraform module to create AWS IAM resources 🇺🇦
https://registry.terraform.io/modules/terraform-aws-modules/iam/aws
Apache License 2.0
779 stars 985 forks source link

feat: Add JSON documents support for inline policies #499

Closed ajax-ryzhyi-r closed 1 month ago

ajax-ryzhyi-r commented 1 month ago

Description

This PR complements #479. It allows passing inline policies as JSON documents in addition to HCL statements. It also adds inline policy support for the iam-github-oidc-role module.

Motivation and Context

We use JSON files to define policies for roles in many places because we don't want to overload Terraform modules with HCL statements that describe IAM policies. Additionally, we have some automatic validation for IAM policies that expects these policies to be defined in JSON format.

In general, I think it would be nice to have an alternative option for defining inline policies for roles 😊

Breaking Changes

All changes are backward compatible.

How Has This Been Tested?

bryantbiggs commented 1 month ago

thank you but unfortunately we will not be accepting this PR - it is better to work through the HCL syntax since it provides a number of validations during plan/apply

ajax-ryzhyi-r commented 1 month ago

thank you but unfortunately we will not be accepting this PR - it is better to work through the HCL syntax since it provides a number of validations during plan/apply

@bryantbiggs Thank you for your reply. I want to clarify some points regarding this PR:

  1. Plain JSON policy is optional, so users can always choose how to define policies.
  2. The aws_iam_role_policy resource has validation for the policy attribute during plan/apply. So, no matter how the policy is passed to the module—whether as JSON or HCL—it will be validated anyway (https://github.com/hashicorp/terraform-provider-aws/blob/9aa183f3cd991bad10877c55abbeb979afe44528/internal/service/iam/role_policy.go#L66).
  3. iam-policy and iam-group-with-policies modules already accept plain JSON for policies, so I thought it would be consistent to use it for role policies as well. Currently, we are using iam-policy module in some places to manage policies for roles, but we want to switch to the roles module's inline policies.

It would be great if you could accept these changes 👀

@antonbabenko What do you think about these changes?

bryantbiggs commented 1 month ago

there are a lot of various configurations and forms of supplying said configurations that could be applied within these modules, but that will only increase their complexity and introduce numerous avenues for bugs or issues. Terraform is used to provide structure for defining your infrastructure - it seems odd that we want to use a free form here when the structured form is more strongly preferred

ajax-ryzhyi-r commented 1 month ago

there are a lot of various configurations and forms of supplying said configurations that could be applied within these modules, but that will only increase their complexity and introduce numerous avenues for bugs or issues. Terraform is used to provide structure for defining your infrastructure - it seems odd that we want to use a free form here when the structured form is more strongly preferred

I agree that structured forms are generally better, but here we have IAM policies with their own syntax, originally written in JSON. When we use HCL to define policies, we rewrite them from JSON to HCL, then pass them as HCL to the module where aws_iam_policy_document data source transforms them back to JSON and passes them as JSON to the aws_iam_role_policy resource. This seems a bit odd as well. So, I propose adding the ability to bypass aws_iam_policy_document data source and pass JSON policies directly from inputs to aws_iam_role_policy resource.

bryantbiggs commented 1 month ago

I mean, what you just described is how nearly all of the interaction between Terraform and AWS APIs occur. We write those via HCL, that's why (I would think) you chose Terraform

ajax-ryzhyi-r commented 1 month ago

I mean, what you just described is how nearly all of the interaction between Terraform and AWS APIs occur. We write those via HCL, that's why (I would think) you chose Terraform

Yeah, in general, we declare resources in HCL, and Terraform transforms them into appropriate provider API calls. But here, we have a resource attribute that is originally a JSON document, which you can see in the AWS console, AWS docs, whitepapers, blogs, and even validate (https://docs.aws.amazon.com/IAM/latest/UserGuide/access-analyzer-policy-validation.html#access-analyzer-policy-validation-cli). I just want to optionally allow using this original JSON format for policies in addition to HCL abstraction 🙂

ajax-ryzhyi-r commented 1 month ago

Also, AWS provider documentation for the aws_iam_policy_document data source states the following:

Using this data source to generate policy documents is optional. It is also valid to use literal JSON strings in your configuration or to use the file interpolation function to read a raw JSON policy document from a file.

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document

github-actions[bot] commented 3 weeks ago

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.