Skip to content

fix: Give SNS feedback access to CloudWatch #237

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

Closed
wants to merge 4 commits into from
Closed

fix: Give SNS feedback access to CloudWatch #237

Show file tree
Hide file tree
Changes from 1 commit
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
21 changes: 21 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,22 @@ data "aws_iam_policy_document" "sns_feedback" {
}
}

data "aws_iam_policy_document" "sns_feedback_allow_log_creation" {
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 @@ -31,6 +47,11 @@ resource "aws_iam_role" "sns_feedback_role" {
permissions_boundary = var.sns_topic_feedback_role_permissions_boundary
assume_role_policy = data.aws_iam_policy_document.sns_feedback[0].json

inline_policy {
name = "allow-log-creation"
policy = data.aws_iam_policy_document.sns_feedback_allow_log_creation[0].json
}

tags = merge(
var.tags,
var.sns_topic_feedback_role_tags,
Expand Down
Loading