Automatically create enhanced monitoring role #21
Conversation
|
Hi @antonbabenko, any chance this could be merged soon? 😉 |
|
Just came back from websummit and going to review it all during today/tomorrow. Sorry for the delay. |
modules/db_instance/main.tf
Outdated
| resource "aws_iam_role" "enhanced_monitoring" { | ||
| name = "${var.monitoring_role_autocreate}" | ||
| assume_role_policy = "${file("${path.module}/policy/enhancedmonitoring.json")}" | ||
| count = "${local.create_enhanced_role ? 1 : 0}" |
There was a problem hiding this comment.
Put count as the first argument, add an empty line after it.
modules/db_instance/main.tf
Outdated
| name = "${var.monitoring_role_autocreate}-enhanced-monitoring-attachment" | ||
| roles = ["${aws_iam_role.enhanced_monitoring.name}"] | ||
| policy_arn = "arn:aws:iam::aws:policy/service-role/AmazonRDSEnhancedMonitoringRole" | ||
| count = "${local.create_enhanced_role ? 1 : 0}" |
README.md
Outdated
| monitoring_interval = "30" | ||
| monitoring_role_arn = "arn:aws:iam::123456789012:role/rds-monitoring-role" | ||
| monitoring_role_autocreate = "rds-monitoring-role" |
There was a problem hiding this comment.
monitoring_role_autocreate sounds like a boolean, not string.
Let's call this as monitoring_role_name, and use create_monitoring_role as the main switch - true/false.
There was a problem hiding this comment.
So I should additional variable create_monitoring_role for the switch and set some default name for monitoring_role_name?
There was a problem hiding this comment.
yes, and the defaults are:
create_monitoring_role = false
monitoring_role_name = ""
modules/db_instance/main.tf
Outdated
| @@ -1,6 +1,24 @@ | |||
| ############## | |||
| # DB instance | |||
| ############## | |||
|
|
|||
| locals { | |||
| create_enhanced_role = "${var.monitoring_role_autocreate != "" && var.monitoring_role_arn == ""}" | |||
There was a problem hiding this comment.
Avoid such rules (and locals in general unless it is possible to use specific module versions in Terraform Registry). Use count = "${var.create_monitoring_role}" as the main switch.
There was a problem hiding this comment.
I wanted to avoid the case of misconfiguration when a user provides ARN and also configure auto-creation, which could be skipped, but sure I will remove it.
modules/db_instance/main.tf
Outdated
| } | ||
|
|
||
| resource "aws_iam_policy_attachment" "enhanced_monitoring" { | ||
| name = "${var.monitoring_role_autocreate}-enhanced-monitoring-attachment" |
There was a problem hiding this comment.
name = "${var.monitoring_role_name}" - shorter is better usually. In this case, it is clear that it is an attachment of enhanced monitoring policy, so -enhanced-monitoring-attachment can be safely removed.
| @@ -31,7 +49,7 @@ resource "aws_db_instance" "this" { | |||
| iops = "${var.iops}" | |||
| publicly_accessible = "${var.publicly_accessible}" | |||
| monitoring_interval = "${var.monitoring_interval}" | |||
| monitoring_role_arn = "${var.monitoring_role_arn}" | |||
| monitoring_role_arn = "${coalesce(var.monitoring_role_arn, join("", aws_iam_role.enhanced_monitoring.*.arn))}" | |||
There was a problem hiding this comment.
I like this hack with join 👍
variables.tf
Outdated
| @@ -114,6 +114,11 @@ variable "monitoring_role_arn" { | |||
| default = "" | |||
| } | |||
|
|
|||
| variable "monitoring_role_autocreate" { | |||
| description = "Automatically create IAM role with a defined name that permits RDS to send enhanced monitoring metrics to CloudWatch Logs. Creation in skipped, when monitoring_role_arn is provided." | |||
|
Thanks for CR, all your comments are addressed. |
|
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. |
No description provided.