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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 12 commits into from
Jun 21, 2023
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ac1341d
feat: allows to use YAML instead of JSON for IAM policy
gberenice May 24, 2023
1332ead
Merge branch 'main' into feature/lambda_improvements
gberenice May 24, 2023
ffe20b2
chore: bumps Terraform version
gberenice May 25, 2023
c01a181
Merge branch 'cloudposse:main' into feature/lambda_improvements
gberenice May 29, 2023
e58b512
chore: returns policy JSON back
gberenice May 29, 2023
a3aa15f
chore: utilize 'terraform-aws-iam-policy' for IAM policy statements
gberenice May 31, 2023
72aad7c
Merge branch 'main' into feature/lambda_improvements
gberenice Jun 5, 2023
b6245bc
Merge branch 'cloudposse:main' into feature/lambda_improvements
gberenice Jun 19, 2023
159e269
feat: bump 'cloudposse/iam-policy/aws' module version to 1.0.1
gberenice Jun 13, 2023
61ede2a
chore: update example in README.md
gberenice Jun 19, 2023
c762aee
Merge branch 'main' into feature/lambda_improvements
gberenice Jun 21, 2023
c7945d5
chore: apply reviewer's suggestions
gberenice Jun 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 12 additions & 4 deletions modules/lambda/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ components:
# https://docs.aws.amazon.com/lambda/latest/dg/lambda-runtimes.html
runtime: python3.9
package_type: Zip # `Zip` or `Image`
policy_json: null

# Filename example
filename: lambdas/hello-world-python/output.zip # generated by zip variable.
Expand All @@ -54,6 +53,14 @@ components:
# s3_bucket_name: lambda-source # lambda main.tf calculates the rest of the bucket_name
# s3_key: hello-world-go.zip

custom_iam_policy_statements:
- sid: AllowSQSWorkerWriteAccess
effect: Allow
actions:
- sqs:SendMessage
- sqs:SendMessageBatch
resources:
- arn:aws:sqs:*:111111111111:worker-queue
```


Expand All @@ -62,7 +69,7 @@ components:

| Name | Version |
|------|---------|
| <a name="requirement_terraform"></a> [terraform](#requirement\_terraform) | >= 1.0 |
| <a name="requirement_terraform"></a> [terraform](#requirement\_terraform) | >= 1.3.0 |
| <a name="requirement_archive"></a> [archive](#requirement\_archive) | >= 2.3.0 |
| <a name="requirement_aws"></a> [aws](#requirement\_aws) | >= 4.9.0 |

Expand All @@ -89,6 +96,7 @@ components:
| [aws_iam_policy.default](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_policy) | resource |
| [aws_iam_role_policy_attachment.default](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy_attachment) | resource |
| [archive_file.lambdazip](https://registry.terraform.io/providers/hashicorp/archive/latest/docs/data-sources/file) | data source |
| [aws_iam_policy_document.default](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |

## Inputs

Expand All @@ -104,6 +112,7 @@ components:
| <a name="input_cloudwatch_logs_retention_in_days"></a> [cloudwatch\_logs\_retention\_in\_days](#input\_cloudwatch\_logs\_retention\_in\_days) | Specifies the number of days you want to retain log events in the specified log group. Possible values are:<br> 1, 3, 5, 7, 14, 30, 60, 90, 120, 150, 180, 365, 400, 545, 731, 1827, 3653, and 0. If you select 0, the events in the<br> log group are always retained and never expire. | `number` | `null` | no |
| <a name="input_context"></a> [context](#input\_context) | Single object for setting entire context at once.<br>See description of individual variables for details.<br>Leave string and numeric variables as `null` to use default value.<br>Individual variable settings (non-null) override settings in context object,<br>except for attributes, tags, and additional\_tag\_map, which are merged. | `any` | <pre>{<br> "additional_tag_map": {},<br> "attributes": [],<br> "delimiter": null,<br> "descriptor_formats": {},<br> "enabled": true,<br> "environment": null,<br> "id_length_limit": null,<br> "label_key_case": null,<br> "label_order": [],<br> "label_value_case": null,<br> "labels_as_tags": [<br> "unset"<br> ],<br> "name": null,<br> "namespace": null,<br> "regex_replace_chars": null,<br> "stage": null,<br> "tags": {},<br> "tenant": null<br>}</pre> | no |
| <a name="input_custom_iam_policy_arns"></a> [custom\_iam\_policy\_arns](#input\_custom\_iam\_policy\_arns) | ARNs of custom policies to be attached to the lambda role | `set(string)` | `[]` | no |
| <a name="input_custom_iam_policy_statements"></a> [custom\_iam\_policy\_statements](#input\_custom\_iam\_policy\_statements) | IAM policy statements to add to the Lambda IAM role. | <pre>list(object({<br> sid = optional(string, "")<br> effect = optional(string, "")<br> actions = optional(list(string), [])<br> resources = optional(list(string), [])<br> conditions = optional(list(object({<br> test = string<br> variable = string<br> values = list(string)<br> })), [])<br> }))</pre> | `[]` | no |
| <a name="input_dead_letter_config_target_arn"></a> [dead\_letter\_config\_target\_arn](#input\_dead\_letter\_config\_target\_arn) | ARN of an SNS topic or SQS queue to notify when an invocation fails. If this option is used, the function's IAM role<br> must be granted suitable access to write to the target object, which means allowing either the sns:Publish or<br> sqs:SendMessage action on this ARN, depending on which service is targeted." | `string` | `null` | no |
| <a name="input_delimiter"></a> [delimiter](#input\_delimiter) | Delimiter to be used between ID elements.<br>Defaults to `-` (hyphen). Set to `""` to use no delimiter at all. | `string` | `null` | no |
| <a name="input_description"></a> [description](#input\_description) | Description of what the Lambda Function does. | `string` | `null` | no |
Expand All @@ -112,7 +121,7 @@ components:
| <a name="input_environment"></a> [environment](#input\_environment) | ID element. Usually used for region e.g. 'uw2', 'us-west-2', OR role 'prod', 'staging', 'dev', 'UAT' | `string` | `null` | no |
| <a name="input_event_source_mappings"></a> [event\_source\_mappings](#input\_event\_source\_mappings) | Creates event source mappings to allow the Lambda function to get events from Kinesis, DynamoDB and SQS. The IAM role<br> of this Lambda function will be enhanced with necessary minimum permissions to get those events. | `any` | `{}` | no |
| <a name="input_filename"></a> [filename](#input\_filename) | The path to the function's deployment package within the local filesystem. If defined, The s3\_-prefixed options and image\_uri cannot be used. | `string` | `null` | no |
| <a name="input_function_name"></a> [function\_name](#input\_function\_name) | Unique name for the Lambda Function. | `string` | n/a | yes |
| <a name="input_function_name"></a> [function\_name](#input\_function\_name) | Unique name for the Lambda Function. | `string` | `null` | no |
| <a name="input_handler"></a> [handler](#input\_handler) | The function entrypoint in your code. | `string` | `null` | no |
| <a name="input_iam_policy_description"></a> [iam\_policy\_description](#input\_iam\_policy\_description) | Description of the IAM policy for the Lambda IAM role | `string` | `"Minimum SSM read permissions for Lambda IAM Role"` | no |
| <a name="input_id_length_limit"></a> [id\_length\_limit](#input\_id\_length\_limit) | Limit `id` to this many characters (minimum 6).<br>Set to `0` for unlimited length.<br>Set to `null` for keep the existing setting, which defaults to `0`.<br>Does not affect `id_full`. | `number` | `null` | no |
Expand All @@ -134,7 +143,6 @@ components:
| <a name="input_namespace"></a> [namespace](#input\_namespace) | ID element. Usually an abbreviation of your organization name, e.g. 'eg' or 'cp', to help ensure generated IDs are globally unique | `string` | `null` | no |
| <a name="input_package_type"></a> [package\_type](#input\_package\_type) | The Lambda deployment package type. Valid values are `Zip` and `Image`. | `string` | `"Zip"` | no |
| <a name="input_permissions_boundary"></a> [permissions\_boundary](#input\_permissions\_boundary) | ARN of the policy that is used to set the permissions boundary for the role | `string` | `""` | no |
| <a name="input_policy_json"></a> [policy\_json](#input\_policy\_json) | IAM policy to attach to the Lambda IAM role | `string` | `null` | no |
| <a name="input_publish"></a> [publish](#input\_publish) | Whether to publish creation/change as new Lambda Function Version. | `bool` | `false` | no |
| <a name="input_regex_replace_chars"></a> [regex\_replace\_chars](#input\_regex\_replace\_chars) | Terraform regular expression (regex) string.<br>Characters matching the regex will be removed from the ID elements.<br>If not set, `"/[^a-zA-Z0-9-]/"` is used to remove all characters other than hyphens, letters and digits. | `string` | `null` | no |
| <a name="input_region"></a> [region](#input\_region) | AWS Region | `string` | n/a | yes |
Expand Down
33 changes: 27 additions & 6 deletions modules/lambda/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
locals {
enabled = module.this.enabled
iam_policy_enabled = local.enabled && var.policy_json != null
iam_policy_enabled = local.enabled && length(var.custom_iam_policy_statements) > 0
s3_bucket_full_name = var.s3_bucket_name != null ? format("%s-%s-%s-%s-%s", module.this.namespace, module.this.tenant, module.this.environment, module.this.stage, var.s3_bucket_name) : null
}



module "label" {
source = "cloudposse/label/null"
version = "0.25.0"
Expand All @@ -15,13 +13,37 @@ module "label" {
context = module.this.context
}

data "aws_iam_policy_document" "default" {
gberenice marked this conversation as resolved.
Show resolved Hide resolved
count = local.iam_policy_enabled ? 1 : 0
dynamic "statement" {
for_each = var.custom_iam_policy_statements

content {
sid = statement.value.sid
effect = statement.value.effect
actions = statement.value.actions
resources = statement.value.resources

dynamic "condition" {
for_each = statement.value.conditions

content {
test = condition.value.test
variable = condition.value.variable
values = condition.value.values
}
}
}
}
}

resource "aws_iam_policy" "default" {
count = local.iam_policy_enabled ? 1 : 0

name = module.label.id
path = "/"
description = format("%s Lambda policy", module.label.id)
policy = var.policy_json
policy = data.aws_iam_policy_document.default[0].json

tags = module.this.tags
}
Expand All @@ -45,7 +67,7 @@ module "lambda" {
source = "cloudposse/lambda-function/aws"
version = "0.4.1"

function_name = module.label.id
function_name = coalesce(var.function_name, module.label.id)
gberenice marked this conversation as resolved.
Show resolved Hide resolved
description = var.description
handler = var.handler
lambda_environment = var.lambda_environment
Expand Down Expand Up @@ -86,4 +108,3 @@ module "lambda" {

context = module.this.context
}

23 changes: 17 additions & 6 deletions modules/lambda/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ variable "region" {
variable "function_name" {
type = string
description = "Unique name for the Lambda Function."
default = null
}

variable "architectures" {
Expand Down Expand Up @@ -266,12 +267,6 @@ variable "iam_policy_description" {
default = "Minimum SSM read permissions for Lambda IAM Role"
}

variable "policy_json" {
type = string
description = "IAM policy to attach to the Lambda IAM role"
default = null
}

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Sponsor Contributor

@Nuru Nuru May 25, 2023 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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!

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

variable "zip" {
type = object({
enabled = optional(bool, false)
Expand All @@ -281,3 +276,19 @@ variable "zip" {
description = "Zip Configuration for local file deployments"
default = {}
}

variable "custom_iam_policy_statements" {
description = "IAM policy statements to add to the Lambda IAM role."
default = []
type = list(object({
sid = optional(string, "")
gberenice marked this conversation as resolved.
Show resolved Hide resolved
effect = optional(string, "")
actions = optional(list(string), [])
resources = optional(list(string), [])
conditions = optional(list(object({
test = string
variable = string
values = list(string)
})), [])
}))
}
2 changes: 1 addition & 1 deletion modules/lambda/versions.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
terraform {
required_version = ">= 1.0"
required_version = ">= 1.3.0"

required_providers {
aws = {
Expand Down