Skip to content

feat: add ability to specify existing IAM role for ECS task execution #128

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

feat: add ability to specify existing IAM role for ECS task execution #128

wants to merge 1 commit into from

Conversation

druchoo
Copy link

@druchoo druchoo commented May 26, 2020

Description

Add ability to specify existing IAM role for ECS task execution.

Motivation and Context

Allow module to be used in scenarios where IAM roles cannot be created or already exist.

Breaking Changes

No.

How Has This Been Tested?

module "atlantis" {
  ...

  ecs_task_execution_iam_role_arn = "arn:aws:iam::xxxxxxxxxxxx:role/xxxxxxxxxx"
}

@druchoo
Copy link
Author

druchoo commented Jul 14, 2020

Hello @antonbabenko, can you provide feedback on this PR? Wondering if there is any chance of getting this and my other PR (#127) merged or is it not in line with your goals for this project?

@antonbabenko
Copy link
Member

Hi @druchoo !

This PR and another looks pretty good, but I will be able to process them at the end of August.

@nitrocode nitrocode mentioned this pull request Jul 27, 2020
Merged
@nitrocode
Copy link
Member

@antonbabenko completely understand that you're busy. Would it be possible to add additional maintainers for this terraform module? This way the module can be continuously maintained without putting pressure on you.

@antonbabenko
Copy link
Member

@bryantbiggs Could you please take a look at this PR?

@nitrocode I was on vacation, but since June there are several of us ( @terraform-aws-modules/triage-supporters ) who can do code reviews and close issues.

@bryantbiggs
Copy link
Member

@antonbabenko yes I can take a look this week.

@druchoo would you mind updating your branch with master please?

@druchoo
Copy link
Author

druchoo commented Aug 17, 2020

@bryantbiggs can you advise on how to fix the semantic check?

@bryantbiggs
Copy link
Member

@druchoo I think it wants the Add to be lower case, I would try feat: add ability to specify existing IAM role for ECS task execution and if that doesn't work then feat: add ability to specify existing iam role for ecs task execution

@druchoo druchoo changed the title feat: Add ability to specify existing IAM role for ECS task execution feat: add ability to specify existing IAM role for ECS task execution Aug 17, 2020
bryantbiggs
bryantbiggs reviewed Aug 17, 2020
View reviewed changes
@druchoo druchoo force-pushed the feature/ecs_task_execution_iam_role_arn branch from 53446c1 to 26b74c0 Compare August 17, 2020 18:43
@druchoo druchoo force-pushed the feature/ecs_task_execution_iam_role_arn branch from 26b74c0 to 9bba610 Compare August 17, 2020 18:46
@druchoo
Copy link
Author

druchoo commented Aug 17, 2020

@bryantbiggs, Should be good now. I was failing with trying to fix the merge and ended up force-pushing.

bryantbiggs
bryantbiggs reviewed Aug 17, 2020
View reviewed changes
main.tf

role = aws_iam_role.ecs_task_execution.id
role = aws_iam_role.ecs_task_execution.0.id
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
role = aws_iam_role.ecs_task_execution.0.id
role = aws_iam_role.ecs_task_execution[0].id

bryantbiggs
bryantbiggs reviewed Aug 17, 2020
View reviewed changes
main.tf
name = "${var.name}-ecs_task_execution"
assume_role_policy = data.aws_iam_policy_document.ecs_tasks.json
assume_role_policy = data.aws_iam_policy_document.ecs_tasks.0.json
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assume_role_policy = data.aws_iam_policy_document.ecs_tasks.0.json
assume_role_policy = data.aws_iam_policy_document.ecs_tasks[0].json

bryantbiggs
bryantbiggs reviewed Aug 17, 2020
View reviewed changes
main.tf
@@ -14,6 +14,9 @@ locals {
)}"
atlantis_url_events = "${local.atlantis_url}/events"

# IAM
ecs_task_execution_iam_role_arn = var.ecs_task_execution_iam_role_arn != "" ? var.ecs_task_execution_iam_role_arn : aws_iam_role.ecs_task_execution.0.id
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ecs_task_execution_iam_role_arn = var.ecs_task_execution_iam_role_arn != "" ? var.ecs_task_execution_iam_role_arn : aws_iam_role.ecs_task_execution.0.id
ecs_task_execution_iam_role_arn = var.ecs_task_execution_iam_role_arn != "" ? var.ecs_task_execution_iam_role_arn : aws_iam_role.ecs_task_execution[0].id

bryantbiggs
bryantbiggs reviewed Aug 17, 2020
View reviewed changes
main.tf
@@ -403,7 +412,7 @@ data "aws_iam_policy_document" "ecs_task_access_secrets" {
data "aws_iam_policy_document" "ecs_task_access_secrets_with_kms" {
count = var.ssm_kms_key_arn == "" ? 0 : 1

source_json = data.aws_iam_policy_document.ecs_task_access_secrets.json
source_json = data.aws_iam_policy_document.ecs_task_access_secrets.0.json
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
source_json = data.aws_iam_policy_document.ecs_task_access_secrets.0.json
source_json = data.aws_iam_policy_document.ecs_task_access_secrets[0].json

bryantbiggs
bryantbiggs reviewed Aug 17, 2020
View reviewed changes
main.tf

name = "ECSTaskAccessSecretsPolicy"

role = aws_iam_role.ecs_task_execution.id
role = aws_iam_role.ecs_task_execution.0.id
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
role = aws_iam_role.ecs_task_execution.0.id
role = aws_iam_role.ecs_task_execution[0].id

bryantbiggs
bryantbiggs reviewed Aug 17, 2020
View reviewed changes
outputs.tf
@@ -22,7 +22,7 @@ output "webhook_secret" {
# ECS
output "task_role_arn" {
description = "The Atlantis ECS task role arn"
value = aws_iam_role.ecs_task_execution.arn
value = local.ecs_task_execution_iam_role_arn
}

output "task_role_id" {
Copy link
Member

Choose a reason for hiding this comment

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

the role outputs will all need to be updated to follow the [0].x access pattern.

bryantbiggs
bryantbiggs reviewed Aug 17, 2020
View reviewed changes
variables.tf
@@ -365,6 +365,10 @@ variable "firelens_configuration" {
options = map(string)
})
default = null
variable "ecs_task_execution_iam_role_arn" {
Copy link
Member

@bryantbiggs bryantbiggs Aug 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

you're missing a closing } above.

lets ask the user for the existing role name (rather than the ARN) and then add the following:

data "aws_iam_role" "existing" {
	count = var.ecs_task_role_name != "" ? 1 : 0
	name =  var.ecs_task_role_name
}

then with our outputs we should be able to do something like:

output "task_role_id" {
  description = "The Atlantis ECS task role id"
  value       = var.ecs_task_role_name == "" ? aws_iam_role.ecs_task_execution[0].id : data.aws_iam_role.existing[0].id
}

while also still getting access to the ARN to use. note - you'll need to update the locals variable for this.

@bryantbiggs
Copy link
Member

hey @druchoo I left some comments - please feel free to use the github-complete under the examples directory for testing. you test out your changes using that examples project (you'll just need to update for the existing IAM role name - no need to commit this change, just use it to validate your changes to the module)

@druchoo
Copy link
Author

druchoo commented Aug 17, 2020

@bryantbiggs, ok cool. thanks for reviewing. I don't think I'll be able to update and test for a few days though unfortunately.

@bryantbiggs
Copy link
Member

@druchoo no worries - I have an idea I might put up in a PR tonight that will help solve this and some of the other issues around using existing resources. we can circle back to this when you get time

@github-actions
Copy link

github-actions bot commented Jan 8, 2022

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Jan 8, 2022
@github-actions
Copy link

This PR was automatically closed because of stale in 10 days

@github-actions github-actions bot closed this Jan 19, 2022
@github-actions
Copy link

github-actions bot commented Nov 9, 2022

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bryantbiggs bryantbiggs bryantbiggs left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@druchoo @antonbabenko @nitrocode @bryantbiggs