fix(stepfunctions-tasks): missing tags & perms for emr cluster creation#28327
fix(stepfunctions-tasks): missing tags & perms for emr cluster creation#28327mergify[bot] merged 3 commits intoaws:mainfrom
Conversation
| this._clusterRole = this.props.clusterRole ?? this.createClusterRole(); | ||
|
|
||
| // Service role must be able to iam:PassRole on the cluster role | ||
| this._clusterRole.grantPassRole(this._serviceRole); |
There was a problem hiding this comment.
AmazonEMRServicePolicy_v2 only grants iam:PassRole on the default role. Need this here for the cluster role we create.
| 'elasticmapreduce:RunJobFlow', | ||
| 'elasticmapreduce:DescribeCluster', | ||
| 'elasticmapreduce:TerminateJobFlows', | ||
| 'elasticmapreduce:AddTags', |
| conditions: { | ||
| StringEquals: { 'aws:RequestTag/for-use-with-amazon-emr-managed-policies': 'true' }, | ||
| }, | ||
| }), |
There was a problem hiding this comment.
This condition isn't needed and was actually causing it to fail.
There was a problem hiding this comment.
Could you explain your reasoning for why it's not needed? Also how was it causing the integ tests to fail?
There was a problem hiding this comment.
When included, I get the following error:
EMR service role arn:aws:iam::<REDACTED>:role/aws-cdk-emr-create-cluste-EmrCreateClusterServiceRo-GrrjLUAR1a2Q is invalid
There was a problem hiding this comment.
Gotcha, I see the part where it's being added to the _base_tags as well. Thanks for clarifying.
| new sfn.StateMachine(stack, 'SM', { | ||
| definition: step, | ||
| }); | ||
|
|
There was a problem hiding this comment.
this wasn't actually deploying a state machine. I added this and ran the step function.
4aaed16 to
e02fc1d
Compare
| conditions: { | ||
| StringEquals: { 'aws:RequestTag/for-use-with-amazon-emr-managed-policies': 'true' }, | ||
| }, | ||
| }), |
There was a problem hiding this comment.
Could you explain your reasoning for why it's not needed? Also how was it causing the integ tests to fail?
paulhcsun
left a comment
There was a problem hiding this comment.
Overall looks good to me. This PR also adds additional integ tests that deploys EMR cluster from a state machine and is working. Thanks for the fix!
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Per the documentation:
for-use-with-amazon-emr-managed-policies = truewhen you provision a cluster with the CLI, SDK, or another method.Also,
AmazonEMRServicePolicy_v2only hasiam:PassRoleon the default EMR roles and needs this on the cluster role created by the CDK.Running the step function:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license