-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(stepfunctions-tasks): bedrock createModelCustomizationJob integration #31913
base: main
Are you sure you want to change the base?
feat(stepfunctions-tasks): bedrock createModelCustomizationJob integration #31913
Conversation
…e-model-customization-job.ts Co-authored-by: Luca Pizzini <[email protected]>
…e-model-customization-job.ts Co-authored-by: Luca Pizzini <[email protected]>
…e-model-customization-job.ts Co-authored-by: Luca Pizzini <[email protected]>
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/create-model-customization-job.ts
Show resolved
Hide resolved
} | ||
|
||
private validatePattern(name: string, pattern: RegExp, value?: string): void { | ||
if (value !== undefined && !Token.isUnresolved(value) && !pattern.test(value)) { |
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data High
regular expression
library input
083ec3a
to
c209f7f
Compare
@moelasmar It appears that the needs-maintainer-review label was attached, but it was automatically removed. Additionally, I’m seeing a CodeQL error related to a regular expression, but this expression is directly from the CloudFormation documentation. In this case, I believe it’s acceptable to ignore the error. What are your thoughts? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #31913 +/- ##
=======================================
Coverage 81.38% 81.38%
=======================================
Files 222 222
Lines 13698 13698
Branches 2413 2413
=======================================
Hits 11148 11148
Misses 2271 2271
Partials 279 279
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @badmintoncryer , thank you for working on this! I saw the previously open PR was approved (apologies that it got closed by our automation), I just have a few more comments.
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/create-model-customization-job.ts
Show resolved
Hide resolved
this.props.customModelKmsKey.addToResourcePolicy(new iam.PolicyStatement({ | ||
actions: ['kms:Decrypt', 'kms:GenerateDataKey', 'kms:DescribeKey', 'kms:CreateGrant'], | ||
resources: ['*'], | ||
principals: [new iam.ArnPrincipal(this._role.roleArn)], | ||
})); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To adhere to the best security practice stated here: https://docs.aws.amazon.com/bedrock/latest/userguide/encryption-custom-job.html#encryption-cm-statements, can we add a kms:ViaService
condition to this policy to limit key access to the Amazon Bedrock service? There's an example under the Encrypt a model
dropdown in that link
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated my code and executed integ test again!
S3Uri: this.props.outputData.bucket.s3UrlForObject(this.props.outputData.prefix), | ||
}, | ||
RoleArn: this._role.roleArn, | ||
TrainingDataConfig: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering if there's a reason we omit invocationLogsConfig
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When it was created, I remember handling all the parameters, so additional arguments might have been added later. Should I address this as well?
Pull request has been modified.
@gracelu0 Thank you for your kindness check. I've addressed all of your comments. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
This PR was previously created and passed the community review, but the maintainer review stopped midway, and it was eventually closed. There shouldn’t be any issues with the content, so I am submitting the PR again.
Issue # (if applicable)
Closes #29042
Reason for this change
AWS stepfunctions support optimized integration with AWS bedrock.
Currently, only invokeModel is supported by CDK, but I would like createModelCustomizationJob to be supported in the same manner.
Description of changes
I've added CreatemodelCustomizationJob class.
Description of how you validated changes
I've added both unit and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license