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.

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

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
Closed

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

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
@piradeepk
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 •
edited
Loading

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
@ScOut3R
fix(ecs): ECS drain hook can't change instance state to draining (aws…
caaee9e 
…#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
@CodePipeline-Test
fix(ecs): grant instance drain-hook policy container-instance permiss…
eb126ba 
…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
@CodePipeline-Test @kohidave
fix(ecs): grant instance drain-hook policy container-instance permiss…
18e0aa2 
…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
@kohidave
fix(ecs): grant drain-hook policy container-instance permissions
9149ccb 
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 kohidave mentioned this issue Jul 4, 2019
Closed
@piradeepk
Copy link
Contributor

piradeepk commented Jul 4, 2019 •
edited
Loading

@ScOut3R the fix is pending: PR:#3196

ScOut3R added a commit to ScOut3R/aws-cdk that referenced this issue Jul 4, 2019
@ScOut3R
fix(ecs): ECS drain hook can't change instance state to draining (aws…
150e3a8 
…#3190)

update integration test data

fixes aws#3190
@ScOut3R ScOut3R mentioned this issue Jul 4, 2019
Closed
@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
@kohidave @piradeepk
fix(ecs): grant drain-hook policy container-instance permissions
98f6963 
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 piradeepk mentioned this issue Jul 4, 2019
Merged
piradeepk pushed a commit to piradeepk/aws-cdk that referenced this issue Jul 4, 2019
@kohidave @piradeepk
fix(ecs): grant drain-hook policy container-instance permissions
8c6c9ca 
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
@piradeepk @rix0rrr
fix(ecs): grant drain-hook policy container-instance permissions (#3199)
7796cd7 
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!

This was referenced Aug 22, 2019
Open
Open
Open
Open
Open
This was referenced Dec 12, 2019
Closed
Closed
Closed
Merged
Closed
Open
Open
Open
Open
Open
Open
Open
Open
This was referenced Jan 20, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@piradeepk piradeepk

Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(ecs): grant drain-hook policy container-instance permissions piradeepk/aws-cdk
[Snyk] Upgrade @aws-cdk/aws-iam from 0.22.0 to 0.39.0 MechanicalRock/tech-radar
[Snyk] Upgrade @aws-cdk/aws-codebuild from 0.24.1 to 0.39.0 MechanicalRock/cdk-constructs
[Snyk] Upgrade @aws-cdk/aws-codepipeline from 0.24.1 to 0.39.0 MechanicalRock/cdk-constructs
[Snyk] Upgrade @aws-cdk/aws-s3 from 0.24.1 to 0.39.0 MechanicalRock/cdk-constructs
[Snyk] Upgrade @aws-cdk/aws-cloudfront from 0.15.2 to 0.39.0 MechanicalRock/cdk-products
[Snyk] Upgrade @aws-cdk/aws-codebuild from 0.15.2 to 0.39.0 MechanicalRock/cdk-products
[Snyk] Upgrade @aws-cdk/aws-codecommit from 0.15.2 to 0.39.0 MechanicalRock/cdk-products
[Snyk] Upgrade @aws-cdk/aws-codepipeline from 0.15.2 to 0.39.0 MechanicalRock/cdk-products
fix(ecs): grant drain-hook policy container-instance permissions kohidave/aws-cdk
3 participants
@ScOut3R @NGL321 @piradeepk