Adding policy actions for additional metrics #26
Conversation
|
Hi @mbolek! Thanks for the PR. Instead of extending IAM policy for everyone I would recommend to change it so that users can attach their custom IAM policies if necessary. I am thinking about something like this (pseudo code): module "ecs" {
source = "git:...tf_aws_ecs"
}
resource "aws_iam_policy" "policy" {
name = "test-policy"
description = "A test policy"
policy = # omitted
}
resource "aws_iam_policy_attachment" "custom" {
roles = ["${module.ecs.ecs_iam_role}"]
policy_arn = "${aws_iam_policy.policy.arn}"
} |
|
@antonbabenko great idea... why haven't I thought of it :) I'll try to do it this way |
|
It is sometimes hard to figure out whether something will be needed by everyone (extend the module) or will be used just by a few (extend outside of the module). I often question this myself during a day :) |
iam.tf
Outdated
| @@ -35,6 +35,7 @@ resource "aws_iam_policy" "ecs-policy" { | |||
| name_prefix = "${replace(format("%.102s", replace("tf-ECSInPol-${var.name}-", "_", "-")), "/\\s/", "-")}" | |||
| description = "A terraform created policy for ECS" | |||
| path = "${var.iam_path}" | |||
| count = "${1 - var.use_custom_iam_policy}" | |||
There was a problem hiding this comment.
this is correct, but IMO counterintuitive. a more common idiom is the ternary operator, like this:
count = "${var.use_custom_iam_policy ? 1 : 0}"in this case, however, you don't even need the boolean; try something like this:
count = "${length(var.custom_iam_policy) > 0 ? 1 : 0}"and then you make the default value of custom_iam_policy the empty string.
does that make sense?
There was a problem hiding this comment.
glad you like the idea. I've tried to use ternary operator at first but haven't figured out the length function, which will probably make all the difference :)
I'm trying the changes now and will update the PR
variables.tf
Outdated
| @@ -59,6 +59,16 @@ variable "iam_path" { | |||
| description = "IAM path, this is useful when creating resources with the same name across multiple regions. Defaults to /" | |||
| } | |||
|
|
|||
| variable "use_custom_iam_policy" { | |||
There was a problem hiding this comment.
see comment above, i think this variable is unnecessary
variables.tf
Outdated
| } | ||
|
|
||
| variable "custom_iam_policy" { | ||
| default = "{}" |
There was a problem hiding this comment.
see comment above, perhaps default value should be ""
|
@hakamadare I've implemented your ideas. It works for me so I hope it's ok :) |
|
@mbolek LGTM :) thank you for your contribution! i’ll merge it and put out a release ASAP (though i’m on vacation right now) |
|
@mbolek i've released https://github.com/terraform-community-modules/tf_aws_ecs/tree/v4.2.0 with these changes. |
|
@hakamadare awesome, thx |
Hi, I've added some actions so that the Instance could report additional metrics (http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/mon-scripts.html). I need memory info so this is handy, and since an Instance can have only one IAM role it's hopefuly a good place to add to :)
All is needed now is a cron job that pushes them, so this is my ugly additional_user_data:
"yum install -y unzip perl-Switch perl-DateTime perl-Sys-Syslog perl-LWP-Protocol-https perl-Digest-SHA && curl http://aws-cloudwatch.s3.amazonaws.com/downloads/CloudWatchMonitoringScripts-1.2.1.zip -O && unzip CloudWatchMonitoringScripts-1.2.1.zip -d /opt/ && echo '*/5 * * * * ec2-user /opt/aws-scripts-mon/mon-put-instance-data.pl --mem-util --mem-used --mem-used-incl-cache-buff --mem-avail --from-cron --verbose' >> /etc/crontab"