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

ECS drain hook can't change instance state to draining #3190

Closed
ScOut3R opened this issue Jul 3, 2019 · 6 comments · Fixed by #3199 or MechanicalRock/tech-radar#14 · May be fixed by MechanicalRock/cdk-constructs#5, MechanicalRock/cdk-constructs#6 or MechanicalRock/cdk-constructs#7
Assignees
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug.

Comments

@ScOut3R
Copy link
Contributor

ScOut3R commented Jul 3, 2019

The draining function's IAM policy does not provide enough permission to change the ECS instance state.

  • What is the current behavior?
    The ecs:UpdateContainerInstancesState permission is provided for the cluster ARN but the function needs to set the state on the instance which has a different ARN.
An error occurred (AccessDeniedException) when calling the UpdateContainerInstancesState operation: User: arn:aws:sts::assumed-role/[role]/[function] is not authorized to perform: ecs:UpdateContainerInstancesState on resource: arn:aws:ecs:ap-southeast-2::container-instance/[instance]
  • What is the expected behavior (or behavior of feature suggested)?
    The function should have access to the cluster instance to set the state.

  • What is the motivation / use case for changing the behavior or adding this feature?
    The draining function can't set the instance to draining which defeats its sole purpose.

  • Please tell us about your environment:

    • CDK CLI Version: 0.36.1
    • Module Version: 0.36.1
    • OS: Linux
    • Language: TypeScript
@ScOut3R ScOut3R added the needs-triage This issue or PR still needs to be triaged. label Jul 3, 2019
@piradeepk
Copy link
Contributor

Thanks for reaching out @ScOut3R, we're looking into the issue and will get back to you with more information.

@NGL321 NGL321 added bug This issue is a bug. @aws-cdk/aws-ecs Related to Amazon Elastic Container parked and removed needs-triage This issue or PR still needs to be triaged. parked labels Jul 3, 2019
@piradeepk
Copy link
Contributor

UpdateContainerInstancesState and ListTasks need to be restricted to the containerInstance resource.

https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-supported-iam-actions-resources.html

@ScOut3R
Copy link
Contributor Author

ScOut3R commented Jul 3, 2019

Thank you for pointing out the documentation! Looks like the ARN format defined in the documentation refers to the long ARN @pkandasamy91. The legacy format does not have the cluster name.

UPDATE:
I tried with a cluster opted-in for the long ARN and the container instance does not have the cluster name in its ARN.
@pkandasamy91 could you please verify that the linked AWS documentation has the correct ARN format for the container instance?

ScOut3R added a commit to ScOut3R/aws-cdk that referenced this issue Jul 4, 2019
…#3190)

The UpdateContainerInstancesState permission was scoped to the ECS cluster
while it should be scoped to the container instance.

fixes aws#3190
kohidave pushed a commit to kohidave/aws-cdk that referenced this issue Jul 4, 2019
…ions

UpdateContainerInstanceState and ListTask APIs require permissions on
a container-instance resource, rather than a cluster resource. This
change updates the policy to:

1. remove the cluster as the resource restriction
2. add the cluster as a resource condition

More info on ECS Resource-Level permissions can be found here:
https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-supported-iam-actions-resources.html

Fixes aws#3190
kohidave pushed a commit to kohidave/aws-cdk that referenced this issue Jul 4, 2019
…ions

UpdateContainerInstanceState and ListTask APIs require permissions on
a container-instance resource, rather than a cluster resource. This
change updates the policy to:

1. remove the cluster as the resource restriction
2. add the cluster as a resource condition

More info on ECS Resource-Level permissions can be found here:
https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-supported-iam-actions-resources.html

Fixes aws#3190
kohidave added a commit to kohidave/aws-cdk that referenced this issue Jul 4, 2019
UpdateContainerInstanceState and ListTask APIs require permissions on
a container-instance resource, rather than a cluster resource. This
change updates the policy to:

1. remove the cluster as the resource restriction
2. add the cluster as a resource condition

More info on ECS Resource-Level permissions can be found here:
https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-supported-iam-actions-resources.html

Fixes aws#3190
@piradeepk
Copy link
Contributor

piradeepk commented Jul 4, 2019

@ScOut3R the fix is pending: PR:#3196

@ScOut3R
Copy link
Contributor Author

ScOut3R commented Jul 4, 2019

@pkandasamy91 beat me to it :D

piradeepk pushed a commit to piradeepk/aws-cdk that referenced this issue Jul 4, 2019
UpdateContainerInstanceState and ListTask APIs require permissions on
a container-instance resource, rather than a cluster resource. This
change updates the policy to:

1. remove the cluster as the resource restriction
2. add the cluster as a resource condition

More info on ECS Resource-Level permissions can be found here:
https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-supported-iam-actions-resources.html

Fixes aws#3190
piradeepk pushed a commit to piradeepk/aws-cdk that referenced this issue Jul 4, 2019
UpdateContainerInstanceState and ListTask APIs require permissions on
a container-instance resource, rather than a cluster resource. This
change updates the policy to:

1. remove the cluster as the resource restriction
2. add the cluster as a resource condition

More info on ECS Resource-Level permissions can be found here:
https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-supported-iam-actions-resources.html

Fixes aws#3190
rix0rrr pushed a commit that referenced this issue Jul 4, 2019
UpdateContainerInstanceState and ListTask APIs require permissions on
a container-instance resource, rather than a cluster resource. This
change updates the policy to:

1. remove the cluster as the resource restriction
2. add the cluster as a resource condition

More info on ECS Resource-Level permissions can be found here:
https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-supported-iam-actions-resources.html

Fixes #3190
@piradeepk
Copy link
Contributor

@ScOut3R thanks for the feedback! Instead of using the resource arn (which would vary depending on if you opted into using long arns or not), we used the cluster as the condition key for restricting the scope of the permissions on ListTasks and UpdateContainerInstancesState actions.

The fix has been merged in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment