fix(stepfunctions-tasks): state machine role is missing sagemaker:AddTags permission for SageMakerCreateTransformJob task #27264

tam0ri · 2023-09-24T15:20:34Z

If we specified RUN_JOB as IntegrationPattern prop for SageMakerCreateTransformJob construct, StepFunctions executes SageMaker batch transform job synchronously. In this case, StepFunctions add a tag (key: MANAGED_BY_AWS, value: STARTED_BY_STEP_FUNCTIONS) to the job, so state machine role needs the permission to do that. However, currently CDK does not add the permission automatically.

This PR solves the issue by adding sagemaker:AddTags permission to state machine role when RUN_JOB is specified as IntegrationPattern prop.

Closes #26012

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

…:AddTags permission for SageMakerCreateTransformJob task

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

tam0ri · 2023-09-24T23:58:49Z

Exemption Request: There is no change against existing snapshots. Also, this is a small fix, so I didn't add a new integ test at this point. Please let me know if it's needed.

sumupitchayan · 2023-10-12T17:56:32Z

Exemption Request: There is no change against existing snapshots. Also, this is a small fix, so I didn't add a new integ test at this point. Please let me know if it's needed.

@tam0ri I believe an integ test here would be useful.

aws-cdk-automation · 2023-10-16T00:08:27Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

…-job-permission

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

lpizzinidev

Thanks (and sorry for the late review) 👍
Looks good, just some minor adjustments are needed.

lpizzinidev · 2023-10-27T10:32:37Z

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/sagemaker/create-transform-job.ts

      );
+      policyStatements.push(


Suggested change

);

policyStatements.push(

A single push call can be used for both statements.

lpizzinidev · 2023-10-27T10:33:20Z

packages/aws-cdk-lib/aws-stepfunctions-tasks/test/sagemaker/create-transform-job.test.ts

@@ -306,3 +308,215 @@ test('create transform job with instance type supplied as JsonPath', () => {
    },
  });
 });
+
+test('required permmisions are granted to service role if RUN_JOB is supplied as service integration pattern', () => {


Suggested change

test('required permmisions are granted to service role if RUN_JOB is supplied as service integration pattern', () => {

test('required permissions are granted to service role if RUN_JOB is supplied as service integration pattern', () => {

lpizzinidev · 2023-10-27T10:35:28Z

packages/aws-cdk-lib/aws-stepfunctions-tasks/test/sagemaker/create-transform-job.test.ts

+  });
+});
+
+test('required permmisions are granted to service role if REQUEST_RESPONSE is supplied as service integration pattern', () => {


Suggested change

test('required permmisions are granted to service role if REQUEST_RESPONSE is supplied as service integration pattern', () => {

test('required permissions are granted to service role if REQUEST_RESPONSE is supplied as service integration pattern', () => {

lpizzinidev · 2023-10-27T10:40:14Z

...ng/framework-integ/test/aws-stepfunctions-tasks/test/sagemaker/integ.create-transform-job.ts

+    super(scope, id, props);
+    const batch_inference_job = new SageMakerCreateTransformJob(this, 'Batch Inference', {


Suggested change

super(scope, id, props);

const batch_inference_job = new SageMakerCreateTransformJob(this, 'Batch Inference', {

super(scope, id, props);

const task = new SageMakerCreateTransformJob(this, 'BatchInferenceTask', {

lpizzinidev · 2023-10-27T10:40:32Z

...ng/framework-integ/test/aws-stepfunctions-tasks/test/sagemaker/integ.create-transform-job.ts

+    });
+
+    new StateMachine(this, 'SimpleStateMachine', {
+      definitionBody: DefinitionBody.fromChainable(batch_inference_job),


Suggested change

definitionBody: DefinitionBody.fromChainable(batch_inference_job),

definitionBody: DefinitionBody.fromChainable(task),

tam0ri · 2023-10-28T14:49:38Z

@lpizzinidev Thank you for your review! I pushed changes according to your comment. Could you review when you have some time?

lpizzinidev

Looks good to me, thanks 👍

…-job-permission

Signed-off-by: Vinayak Kukreja <vinakuk@amazon.com>

vinayak-kukreja

Hey @tam0ri , apologies for the delay. Thank you for contributing to AWS CDK.

aws-cdk-automation · 2023-12-07T22:00:06Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: efe672f
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-12-07T22:00:25Z

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).

fix(aws-stepfunctions-tasks): state machine role is missing sagemaker…

70b3d23

…:AddTags permission for SageMakerCreateTransformJob task

github-actions bot added the admired-contributor [Pilot] contributed between 13-24 PRs to the CDK label Sep 24, 2023

aws-cdk-automation requested a review from a team September 24, 2023 15:20

github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p2 labels Sep 24, 2023

aws-cdk-automation previously requested changes Sep 24, 2023

View reviewed changes

tam0ri changed the title ~~fix(aws-stepfunctions-tasks): state machine role is missing sagemaker:AddTags permission for SageMakerCreateTransformJob task~~ fix(stepfunctions-tasks): state machine role is missing sagemaker:AddTags permission for SageMakerCreateTransformJob task Sep 24, 2023

removed a new integ test

6b0066d

aws-cdk-automation previously requested changes Sep 24, 2023

View reviewed changes

aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Sep 24, 2023

github-actions bot mentioned this pull request Oct 1, 2023

Monthly PRs metrics report - Sep 2023 #27368

Closed

sumupitchayan removed the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Oct 12, 2023

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 12, 2023

tam0ri added 2 commits October 18, 2023 13:07

Merge branch 'main' into fix/aws-stepfunctions-tasks-create-transform…

b414030

…-job-permission

Add integ test and d snapshot

9ea56f2

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 18, 2023

lpizzinidev suggested changes Oct 27, 2023

View reviewed changes

minor fix by PR review

683cad6

lpizzinidev approved these changes Oct 28, 2023

View reviewed changes

aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Oct 28, 2023

Merge branch 'main' into fix/aws-stepfunctions-tasks-create-transform…

77bdba1

…-job-permission

Merge branch 'main' into fix/aws-stepfunctions-tasks-create-transform…

7fd71c8

…-job-permission

vinayak-kukreja self-assigned this Dec 7, 2023

run integ test

efe672f

Signed-off-by: Vinayak Kukreja <vinakuk@amazon.com>

vinayak-kukreja approved these changes Dec 7, 2023

View reviewed changes

mergify bot merged commit 8ce0283 into aws:main Dec 7, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(stepfunctions-tasks): state machine role is missing sagemaker:AddTags permission for SageMakerCreateTransformJob task #27264

fix(stepfunctions-tasks): state machine role is missing sagemaker:AddTags permission for SageMakerCreateTransformJob task #27264

tam0ri commented Sep 24, 2023

aws-cdk-automation left a comment

aws-cdk-automation left a comment

tam0ri commented Sep 24, 2023

sumupitchayan commented Oct 12, 2023

aws-cdk-automation commented Oct 16, 2023

lpizzinidev left a comment

lpizzinidev Oct 27, 2023

lpizzinidev Oct 27, 2023

lpizzinidev Oct 27, 2023

lpizzinidev Oct 27, 2023

lpizzinidev Oct 27, 2023

tam0ri commented Oct 28, 2023

lpizzinidev left a comment

vinayak-kukreja left a comment

aws-cdk-automation commented Dec 7, 2023

mergify bot commented Dec 7, 2023

	test('required permmisions are granted to service role if RUN_JOB is supplied as service integration pattern', () => {
	test('required permissions are granted to service role if RUN_JOB is supplied as service integration pattern', () => {

		super(scope, id, props);
		const batch_inference_job = new SageMakerCreateTransformJob(this, 'Batch Inference', {

	definitionBody: DefinitionBody.fromChainable(batch_inference_job),
	definitionBody: DefinitionBody.fromChainable(task),

fix(stepfunctions-tasks): state machine role is missing sagemaker:AddTags permission for SageMakerCreateTransformJob task #27264

fix(stepfunctions-tasks): state machine role is missing sagemaker:AddTags permission for SageMakerCreateTransformJob task #27264

Conversation

tam0ri commented Sep 24, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

tam0ri commented Sep 24, 2023

sumupitchayan commented Oct 12, 2023

aws-cdk-automation commented Oct 16, 2023

lpizzinidev left a comment

Choose a reason for hiding this comment

lpizzinidev Oct 27, 2023

Choose a reason for hiding this comment

lpizzinidev Oct 27, 2023

Choose a reason for hiding this comment

lpizzinidev Oct 27, 2023

Choose a reason for hiding this comment

lpizzinidev Oct 27, 2023

Choose a reason for hiding this comment

lpizzinidev Oct 27, 2023

Choose a reason for hiding this comment

tam0ri commented Oct 28, 2023

lpizzinidev left a comment

Choose a reason for hiding this comment

vinayak-kukreja left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Dec 7, 2023

AWS CodeBuild CI Report

mergify bot commented Dec 7, 2023