set enable_ecs_service_role to true only if there is a load balancer
#118
Conversation
When creating service, and there is no LB defined "You cannot specify an IAM role for services that require a service linked role." Set enable_ecs_service_role to false when there is 0 LB or more than 1.
verbalius
requested review from
Makeshift and
3h4x
and removed request for
a team
|
This pull request is now in conflict. Could you fix it @verbalius? 🙏 |
main.tf
Outdated
| @@ -1,6 +1,6 @@ | |||
| locals { | |||
| enabled = module.this.enabled | |||
| enable_ecs_service_role = module.this.enabled && var.network_mode != "awsvpc" && length(var.ecs_load_balancers) <= 1 | |||
| enable_ecs_service_role = module.this.enabled && var.network_mode != "awsvpc" && length(var.ecs_load_balancers) == 1 | |||
There was a problem hiding this comment.
@verbalius the original code enables the enable_ecs_service_role if the length(var.ecs_load_balancers) <= 1 meaning if there aren't any load balancers.
Your change sets enables the enable_ecs_service_role if the number of load balancers is equivalent to 1 which conflicts with the PR title. Is this intended ? Could you explain ?
There was a problem hiding this comment.
Hi :)
I tried doing all the steps manually in the console and see
- Task with bridge mode network
2.a. Service without LB (means length(var.ecs_load_balancers))
Result: You can't specify IAM role
2.b Service with application LB for example
Result: Lets you choose a role
So if we transfer it to expression:
if LB count = 0 this should be null:
iam_role = local.enable_ecs_service_role ? coalesce(var.service_role_arn, join("", aws_iam_role.ecs_service.*.arn)) : null
But how it is now length(var.ecs_load_balancers)=0 <= 1 -> true
There was a problem hiding this comment.
or if you want to try it yourself via terraform just try to create service with bridge network mode without load balancer.
There was a problem hiding this comment.
Why your commit contains: length(var.ecs_load_balancers) == 1 ?
In our case we can define more than one load balancer for service..
There was a problem hiding this comment.
@verbalius can you update this PR please? Change it to >= 1
nitrocode
changed the title
enable_ecs_service_role to true only if there is a load balancer
|
/test all |
There was a problem hiding this comment.
We have to wait for the security group module to be complete before this can go through.
See https://github.com/cloudposse/terraform-aws-ecs-alb-service-task/releases
Sorry!
|
Blocked by cloudposse/terraform-aws-security-group#17 |
|
@nitrocode this PR (#118) should be closed and this one re-opened: The code change in this PR (#118) is wrong. |
mergify
bot
dismissed
nitrocode’s stale review
This Pull Request has been updated, so we're dismissing all reviews.
|
Sorry for inactivity, added a fix commit as is in pull request #137. Thank you for help |
|
@nitrocode / @joe-niland mind reviewing and merging? |
|
/test all |
|
@Nuru Can you remove the 'do not merge' label? |
|
@n0rig just FYI, we are still waiting on some related fixes to happen before merging this one |
When creating service, and there is no LB defined "You cannot specify an IAM role for services that require a service linked role."
Set enable_ecs_service_role to false when there is 0 LB or more than 1.
what
When creating service with network mode != awsvpc and there is no LB defined it throws error: "You cannot specify an IAM role for services that require a service linked.
why
When setting value enable_ecs_service_role we check if there is no more than 1 LB, but less than 1 is 0 and we can't assign IAM role when it is no LB and network mode is bridge, host or none.
solution
Set
enable_ecs_service_roletofalsewhen there is 0 LB or more than 1 when network mode is notawsvpc