Skip to content
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

feat: Change to managed IAM policy for ecs tasks with path attrribute support over inline role policy #268

Open
wants to merge 2 commits into
base: wip/v6
Choose a base branch
from

Conversation

partlythomas
Copy link

Description

Note: New PR to base off from wip/v6 branch instead.

The following changes are proposed:

  • Change the ECS tasks role policy to be managed as a separate aws_iam_policy resource rather than the inline type aws_iam_role_policy
  • Add explicit role policy attachment to attach the managed ECS tasks policy to the role.
  • Add support for specifying path of the ECS tasks policy.

Motivation and Context

The change opens for more flexibility in the ECS tasks role for specifying the path of the policy.
This also aligns the ECS tasks role and policy with the same pattern that the existing role and policy for the ECS task execution role in the module.

Breaking Changes

  • If ECS task role is not used (i.e. conditions for creating task role is not met), no changes will be picked up by Terraform.
  • If ECS task role is used (i.e. conditions for creating task role is not met), the inline iam role policy will be replaced by the iam policy and role policy attachment.

Both cases applies both with or without the path configuration of the policy being set with the new variable tasks_iam_policy_path.

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using the examples/complete project deployed cleanly first, with later addition of ecs service variable input of tasks_iam_policy_path prompting to take the changes into effect. The changes was deployed sucessfully.
  • I have executed pre-commit run -a on my pull request

Sorry, something went wrong.

resource "aws_iam_role_policy" "tasks" {
count = local.create_tasks_iam_role && (var.tasks_iam_role_statements != null || var.enable_execute_command) ? 1 : 0
resource "aws_iam_policy" "tasks" {
count = local.create_tasks_iam_role && (length(var.tasks_iam_role_statements) > 0 || var.enable_execute_command) ? 1 : 0
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is changed from how the logic was used in the master branch. The > 0 logic only creates the policy if there are statements defined. The change of '!= null' (which is not found in master branch) can attempt to create the policy if just an empty list is passed.

I followed the logic from the master branch, but have no preference if the != null logic is to be used instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should not change the logic, it should be checking != null

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected to != null now, as is case in the wip/v6 branch currently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants