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

Remove AmazonEC2ContainerServiceAutoscaleRole creation #33

Merged
merged 2 commits into from
Jun 18, 2018

Conversation

rbreslow
Copy link
Contributor

Overview

Repeal Amazon EC2ContainerServiceAutoscaleRole creation and replace with data reference to service-linked IAM role.

Fixes #24

Testing

I brought up a Django app using azavea/terraform-aws-ecs-cluster and azavea/terraform-aws-ecs-web-service.

Run terraform plan:

Terraform will perform the following actions:

-/+ aws_ecs_task_definition.app (new resource required)
      id:                    "maiden" => <computed> (forces new resource)
      arn:                   "arn:aws:ecs:us-east-1:279682201306:task-definition/maiden:56" => <computed>
      container_definitions: "[...]" (forces new resource)
      family:                "maiden" => "maiden"
      network_mode:          "" => <computed>
      revision:              "56" => <computed>

  ~ module.app_web_service.aws_ecs_service.main
      task_definition:       "maiden:56" => "${var.task_definition_id}"


Plan: 1 to add, 1 to change, 1 to destroy.

AWS does not replace aws_appautoscaling_target.role_arn.

@rbreslow rbreslow self-assigned this Jun 15, 2018
@rbreslow rbreslow changed the title WIP: Remove AmazonEC2ContainerServiceAutoscaleRole creation Remove AmazonEC2ContainerServiceAutoscaleRole creation Jun 15, 2018
@rbreslow rbreslow requested a review from hectcastro June 15, 2018 19:59
@hectcastro
Copy link
Contributor

I think that my concern with this change is vaguely covered by this sentence:

You don't need to manually create the service-linked roles for Application Auto Scaling. Application Auto Scaling creates the appropriate service-linked role for you when you call RegisterScalableTarget or PutScalingPolicy.

If something doesn't call RegisterScalableTarget or PutScalingPolicy within a given region before trying to use this module, the data resource lookup will return nothing, correct?

Given that hashicorp/terraform-provider-aws#2889 makes the role optional, and the main reason for emitting this role from the ECS cluster module was to be able to feed it to terraform-aws-ecs-web-service, perhaps we should remove the creation of AmazonEC2ContainerServiceAutoscaleRole and replace it with nothing (in addition to addressing azavea/terraform-aws-ecs-web-service#10).

@rbreslow
Copy link
Contributor Author

rbreslow commented Jun 18, 2018

It would make sense that the data resource lookup will return nothing.

A service-linked role is a unique type of IAM role that is linked directly to an AWS service. Service-linked roles are predefined by the service and include all the permissions that the service requires to call other AWS services on your behalf.

This vague piece of AWS documentation makes me want to test to make sure.

The only places that RegisterScalableTarget and PutScalingPolicy are called in the Terraform provider are:

Since these methods are only invoked in aws_appautoscaling_target and aws_appautoscaling_policy, there wouldn't ever be a context in which that data resource lookup could be useful.

I'm not sure what the difference between aws_autoscaling_policy and aws_appautoscaling_policy is, but PutScalingPolicy is also called here:

https://www.terraform.io/docs/providers/aws/r/autoscaling_policy.html

@rbreslow
Copy link
Contributor Author

I removed the data resource lookup from the module, refactored outputs.tf, and updated the README.

Copy link
Contributor

@hectcastro hectcastro left a comment

Choose a reason for hiding this comment

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

This seems like the best move. 👍

After merging this PR, if you end up creating a new release, please make sure to update the CHANGELOG and bump the major version number.

@rbreslow rbreslow merged commit 24d4a35 into develop Jun 18, 2018
@rbreslow rbreslow deleted the feature/jrb/remove-iam-ec2-role branch June 18, 2018 15:20
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.

2 participants