[lambda] feat: allows to use YAML instead of JSON for IAM policy

[gberenice]

what

• BREAKING CHANGE: Actually use variable function_name to set the lambda function name.
• Make the variable function_name optional. When not set, the old null-lable-derived name will be use.
• Allow IAM policy to be specified in a custom terraform object as an alternative to JSON.

why

• function_name was required to set, but it wasn't actually passed to module "lambda" inputs.
• Allow callers to stop providing function_name and preserve old behavior of using automatically generated name.
• When using Atmos to generate inputs from "stack" YAML files, having the ability to pass the statements in as a custom object means specifying them via YAML, which makes the policy declaration in stack more readable compared to embedding a JSON string in the YAML.

[Gowiem]

We're already using these changes, so I'm biased. Added @Benbentwo + @milldr for a non-biased review. 👍

[aknysh]

LGTM, please see comments

[milldr]

looks good. thank you for the contribution

[Nuru]

@osterman Are you OK with opening the door to YAML IAM policies in this way?

[Nuru]

Cloud Posse has made a high-level design decision NOT to use YAML for IAM policies. While I would consider a community PR that adds support for YAML, I do not support one that removes support for JSON policies.

[Gowiem]

Ah what was the reasoning behind JSON > YAML policies? I'm surprised as sticking with YAML support for policies seems logical since JSON policies inside Stacks feels like we're mixing and matching our formats in an ugly way. It seems inconsistent in the components as there is a JSON policy variable input in the kms component, but the s3-bucket component has an policy statements input as a map.

[Nuru]

JSON is the native format of IAM policies: it is what is published in AWS documentation and it is output by Terraform AWS data sources and resources. Practically no one uses YAML for IAM policies.

We have old code that uses old policy modules that takes Terraform objects and converts them to policies, but what that entails is us developing an alternate specification language for policies when we already have to deal with 2: JSON and Terraform. So we are trying to move away from that and instead only specify policies in JSON or Terraform.

[nitrocode]

That's interesting. I also thought exposing the statements to the yaml interface was convenient and handy. I recall a client doing that and another client asking for it which led to creating a generic interface for https://github.com/cloudposse/terraform-aws-iam-policy in order to expose policy statements as a generic any map.

In ref to maybe old code (in addition to matts references), we currently have some components that expose iam policy statements as an input that can be fed via yaml.

https://github.com/cloudposse/terraform-aws-components/tree/main/modules/ecs-service#input_iam_policy_statements

There may be other components too

https://github.com/search?q=repo%3Acloudposse%2Fterraform-aws-components%20iam_policy_statements&type=code

I think the efs controller has a recent addition to its iam policy statement resource file too

[gberenice]

@Nuru I've added back JSON policy support, so it's possible to set one of them. Please review if that fits better the community best practices. Thank you!

[Gowiem]

I think @gberenice's support for both looks good, but I question if we want to do that. I would double down on wanting to use YAML > JSON in components. All good if that is not what Cloud Posse wants to support, but I'd reiterate that it just feels wrong to be using JSON blocks within our YAML when the two are so portable between one another and whenever we're including custom policies inside Atmos Stack YAMLs, we're doing so because they require customization from standard AWS IAM Policies 90% of the time (otherwise, they'd likely just be built into the component).

[osterman]

I tend to agree with @Gowiem on this. If we're already supporting IAM policies as a JSON input, I can understand wanting YAML for consistency.

[osterman]

It should be noted that as implemented (lists), it doesn't support the finer points of atmos's ability to deepmerge. I understand why - since the interface was copied from how it already works in JSON. Just point it out, as a consideration.

[Nuru]

@nitrocode Note that if someone wanted to use terraform-aws-iam-policy to take the YAML input and generate a policy, the way to do that with the current version of this module is to take the JSON output of terraform-aws-iam-policy and feed it into policy_json input.

@osterman I am opposed to creating a third parallel syntax (YAML) for IAM policies, but if we are going to do it, I would prefer that we standardize on using terraform-aws-iam-policy and make it full-featured, so that everywhere we allow it, it has the same interface, and we do not need to invest energy reinventing it.

Regarding the input being a list and not allowing deep merging, I am not as concerned, because the downside of deep merging is that it is difficult (impossible?) to delete an inherited value.

[gberenice]

@Nuru I've utilized terraform-aws-iam-policy in my recent changes. That also allows us to avoid creating the excessive (in this case) aws_iam_policy resource. Please let me know your thoughts.

[Nuru]

We should allow both policy inputs to be used.

[Nuru]

Please also change description of custom_iam_policy_arns at

terraform-aws-components/modules/lambda/variables.tf

Line 250 in 61ede2a

description = "ARNs of custom policies to be attached to the lambda role"

to

  description = "ARNs of IAM policies to be attached to the Lambda role"