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

fix(aws-ecs): downscope permissions required by instance draining hook #2761

Merged
merged 3 commits into from
Jun 12, 2019

Conversation

piradeepk
Copy link
Contributor

@piradeepk piradeepk commented Jun 5, 2019

PR to address issue #1204.


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
    • Design: For significant features, design document added to design folder
  • Title and Description
    • Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

@piradeepk piradeepk requested review from SoManyHs and a team as code owners June 5, 2019 22:28
@piradeepk piradeepk force-pushed the downscopedrainhook branch 3 times, most recently from 8abbb8b to 7fcdab5 Compare June 6, 2019 18:32
@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 7, 2019

'ecs:SubmitContainerStateChange' and 'ecs:SubmitTaskStateChange' are only actions that can be performed by ECS Agent and as such do not see a need in restricting them.

What does it mean, "can only be performed by ECS Agent"? Can I not take the SDK and make the same call? If the SDK does not have it, can I not take curl and make the same call? If I did, what would be the effect?

If you feel very secure in this reasoning, put it in the code as well please (in a comment) so the reasoning will be easily discovered later.

@piradeepk piradeepk force-pushed the downscopedrainhook branch 3 times, most recently from 5c3047a to 0fbb063 Compare June 7, 2019 23:38
@piradeepk
Copy link
Contributor Author

'ecs:SubmitContainerStateChange' and 'ecs:SubmitTaskStateChange' are only actions that can be performed by ECS Agent and as such do not see a need in restricting them.

What does it mean, "can only be performed by ECS Agent"? Can I not take the SDK and make the same call? If the SDK does not have it, can I not take curl and make the same call? If I did, what would be the effect?

If you feel very secure in this reasoning, put it in the code as well please (in a comment) so the reasoning will be easily discovered later.

After further offline discussions, 'ecs:SubmitContainerStateChange' and 'ecs:SubmitTaskStateChange' have been restricted to the cluster.

@piradeepk piradeepk force-pushed the downscopedrainhook branch 4 times, most recently from b107b28 to d064196 Compare June 11, 2019 09:38
@piradeepk piradeepk changed the title Downscope permissions required by instance draining hook fix(aws-ecs): downscope permissions required by instance draining hook Jun 11, 2019
@piradeepk piradeepk force-pushed the downscopedrainhook branch 2 times, most recently from 41e7458 to a122137 Compare June 11, 2019 10:38
@piradeepk piradeepk force-pushed the downscopedrainhook branch from 483dc0a to 32d8ac6 Compare June 11, 2019 19:00
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Thanks for the hard work Piradeep!

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