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

fix: Give SNS feedback access to CloudWatch #237

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

fix: Give SNS feedback access to CloudWatch #237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ff034ca
fix: give SNS feedback access to CloudWatch
aldenquimby Sep 20, 2024
8d8811c
fix: aws_iam_role.inline_policy -> aws_iam_role_policy
aldenquimby Sep 20, 2024
6e31020
fix: rename for clarity
aldenquimby Sep 20, 2024
29885ea
Merge branch 'master' into master
aldenquimby Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ See the [functions](https://github.com/terraform-aws-modules/terraform-aws-notif
| [aws_caller_identity.current](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/caller_identity) | data source |
| [aws_iam_policy_document.lambda](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |
| [aws_iam_policy_document.sns_feedback](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |
| [aws_iam_policy_document.sns_feedback_allow_log_creation](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |
| [aws_partition.current](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/partition) | data source |
| [aws_region.current](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/region) | data source |

Expand Down
25 changes: 25 additions & 0 deletions iam.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,23 @@ data "aws_iam_policy_document" "sns_feedback" {
}
}

// See https://repost.aws/knowledge-center/monitor-sns-texts-cloudwatch for required permissions to deliver status logs
data "aws_iam_policy_document" "sns_feedback_allow_delivery_status_logs" {
count = local.create_sns_feedback_role ? 1 : 0

statement {
effect = "Allow"
actions = [
"logs:CreateLogGroup",
Copy link
Member

Choose a reason for hiding this comment

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

Terraform creates the log group, this one can be removed

Copy link
Author

@aldenquimby aldenquimby Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Where are the 2 SNS log groups being created? I see a log group for Lambda logs, but that is different. I believe this permission is necessary for the sns/:region/:account/:topic and sns/:region/:account/:topic/Failure log groups that are auto-created by SNS (assuming permissions are wired up correctly)

"logs:CreateLogStream",
"logs:PutLogEvents",
"logs:PutMetricFilter",
"logs:PutRetentionPolicy",
]
resources = ["*"]
Copy link
Member

Choose a reason for hiding this comment

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

we know the ARN and can scope it with the aws_cloudwatch_log_group created by the module instead of using a wildcard

Copy link
Author

Choose a reason for hiding this comment

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

I believe that is the log group for Lambda logs, which is not what we want here. We instead need the SNS feedback log groups, per my other comment. I could restrict it down to sns/:region/:account/:topic and sns/:region/:account/:topic/*? (I know the /Failure group is needed, but I'm not sure about others. The reference post just uses *, so that's not helpful)

}
}

resource "aws_iam_role" "sns_feedback_role" {
count = local.create_sns_feedback_role ? 1 : 0

Expand All @@ -36,3 +53,11 @@ resource "aws_iam_role" "sns_feedback_role" {
var.sns_topic_feedback_role_tags,
)
}

resource "aws_iam_role_policy" "sns_feedback_role" {
Copy link
Member

Choose a reason for hiding this comment

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

create a policy and attach it instead of inline policy. name should not be hardcoded - you can see other modules for references

Copy link
Author

@aldenquimby aldenquimby Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, can do! Is this what you are referring to? https://github.com/terraform-aws-modules/terraform-aws-vpc/blob/573f574c922782bc658f05523d0c902a4792b0a8/vpc-flow-logs.tf#L115-L129

Would you like me to add a _policy_name and _policy_use_name_prefix variable for this?

count = local.create_sns_feedback_role ? 1 : 0

role = aws_iam_role.sns_feedback_role[0].name
name = "allow-delivery-status-logs"
policy = data.aws_iam_policy_document.sns_feedback_allow_delivery_status_logs[0].json
}