-
Notifications
You must be signed in to change notification settings - Fork 8
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.
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
Modify service module to support external ECS execution and task roles #106
Conversation
017d623
to
94a462d
Compare
94a462d
to
a30b406
Compare
.modules/service/variables.tf
Outdated
@@ -65,3 +65,21 @@ variable "rolling_updates" { | |||
default = false | |||
type = bool | |||
} | |||
|
|||
variable "create_policies" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be calculated, there should be no need to pass this and the ARN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it (described in the PR description), and it did not work. I'll check if it works if we pass the role name and use it a data source as you suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it works if a name is passed and used for the data source instead of passing the ARN directly. Thanks for the tip!
.modules/service/variables.tf
Outdated
type = bool | ||
} | ||
|
||
variable "external_ecs_execution_role_arn" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass the role name and use the data source to fetch the arn, that makes the rest of the code more readable as that can determine which role to get (local/external) based on if this is passed or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should rename the local role as well? E.g., add -local
?
As policies are global, why keeping them into the module directory and try to find a workaround to get it working? |
This PR modifies the
service
module to support external ECS execution and task roles to be defined for the service and prevent module policy creation. This is useful for services likestun_server
, which runs in multiple regions, but IAM roles need to be created only in one.I tried defining policy creation in theservice
module (count
value) based on the role variable values passed to the module; however, because roles have to be created first, Terraform could not determine the actual variable values during the run, and I had to implement a separatecreate_policies
variable instead.