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

feat(stepfunctions-tasks): add guardrailConfiguration and trace property to the BedrockInvokeModel #30426

Merged
merged 17 commits into from
Jul 5, 2024

Conversation

mazyu36
Copy link
Contributor

@mazyu36 mazyu36 commented Jun 2, 2024

Issue # (if applicable)

Closes #30425

Reason for this change

In the current BedrockInvokeModel, guardrail configuration and trace for the invocation are not supported.

Description of changes

Add gurdrailConfiguration and trace property to the BedrockInvokeModel

Description of how you validated changes

Add unit tests 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

@github-actions github-actions bot added feature-request A feature should be added or improved. p2 labels Jun 2, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team June 2, 2024 05:54
@github-actions github-actions bot added the admired-contributor [Pilot] contributed between 13-24 PRs to the CDK label Jun 2, 2024
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 2, 2024
@mazyu36 mazyu36 changed the title feat(stepfunctions-tasks): add guardrailConfiguration an trace to the BedrockInvokeModel feat(stepfunctions-tasks): add guardrailConfiguration and trace to the BedrockInvokeModel Jun 2, 2024
@mazyu36 mazyu36 changed the title feat(stepfunctions-tasks): add guardrailConfiguration and trace to the BedrockInvokeModel feat(stepfunctions-tasks): add guardrailConfiguration and trace property to the BedrockInvokeModel Jun 2, 2024
Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Thanks 👍 Left some comments for adjustments on the interface.

Comment on lines 139 to 144
/**
* The guardrail is applied to the invocation
*
* @default - No guardrail is applied to the invocation.
*/
readonly guardrailConfiguration?: GuardrailConfiguration;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/**
* The guardrail is applied to the invocation
*
* @default - No guardrail is applied to the invocation.
*/
readonly guardrailConfiguration?: GuardrailConfiguration;
/**
* The unique identifier of the guardrail that you want to use.
* If you don't provide a value, no guardrail is applied to the invocation.
*/
readonly guardrailIdentifier?: string;
/**
* The version number for the guardrail.
* The value can also be `DRAFT`.
*/
readonly guardrailVersion?: string;

To provide a simpler interface, I'd rather expose the two attributes directly and apply the guardrailVersion must be specified if guardrailIdentifier is specified validation.
What do you think?
Also, we should apply validation for the accepted values according to the docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lpizzinidev
Thank you.

Since guardrailIdentifier and guardrailVersion need to be used as a set, I thought it would be better to have them as required fields in the current standalone interface. What do you think?

For your reference, there was a similar discussion here.

I also like the flat approach, but it could lead to an increase in validation patterns, and since bedrock is a growing service, there's a possibility of more guardrail fields being added in the future.

Adding accepted values validation is agree.

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember having similar discussions in the past in favor of the plain interface approach for other scenarios (as suggested by the design guidelines).
Keeping the nested structure saves us from implementing explicit validation.
However, providing a clean interface for other programming languages is also a concern.

since bedrock is a growing service, there's a possibility of more guardrail fields being added in the future

That's a good point. Let's keep it like this and see what the core team thinks about it 👍

Copy link
Contributor Author

@mazyu36 mazyu36 Jun 7, 2024

Choose a reason for hiding this comment

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

Thank you. For now, I've decided to keep the property design approach as is, without any modifications.

I'd like to wait for the maintainer's review.

I have added the validation.​​​​​​​​​​​​​​​​

Copy link
Contributor

@paulhcsun paulhcsun Jun 20, 2024

Choose a reason for hiding this comment

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

Hi both, the teams current stance is that we would prefer to have this interface containing the two required properties rather than exposing two separate attributes as it allows us to avoid adding more error handling and just enforce that both attributes are specified when defining the resource.

While the section linked from the Design Guidelines does state that flat property structure is preferred over a nested structure, that is only the case when the two properties are not required to be specified together. I can update that to clarify the distinction.

EDIT: @lpizzinidev apologies you are right. I was getting nested objects mixed up with enum-like classes.

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra Jun 21, 2024

Choose a reason for hiding this comment

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

Totally fair concern, in that case, I would create an enum like class here for guardrail

public class Guardrail {
  public static enable(string: id, version: number);

  public static enableDraft(string: id);
}

This way they have to choose a completely different function where it's explicit that they're creating a draft.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I apologize for the repeated requests, but I still can't agree that the proposed method is better.

The guardrail version is a string in the L1 Construct, so using a method to make it a number would require casting.
Also, there's validation for 'contentType' which is not included in the guardrail, and generation of policy documents. Because of these factors, I think the current approach of not separating classes is more understandable.
The proposed approach seems over-optimized to me.

If you still think the proposed method is better despite these points, could you provide an example of where a similar approach is used elsewhere?
This would help me understand the implementation concept better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @mazyu36, one example where a similar approach is used is in the Schedule class in aws-events.

I think that casting from number to string is not a major concern here, and avoids us having to validate the version if a string is passed in.

As for generating the policy documents, I think those can be left where it is, but I agree there's not a super clean way to validate for contentType without passing in the contentType to the method which is not a straightforward DX. I will discuss with the team and get back to you on that tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @mazyu36, I believe the only allowed value we currently accept for contentType is application/json. I assume that when this module was first written it was made to be an optional property to reserve the right to support different contentTypes in the future but for the time being I think that we should just deprecate the property and remove this check. If the day comes that we need to support additional contentTypes then we can just undeprecate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paulhcsun
Thank you. I've added the GuardRail Class and made contentType deprecated.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 4, 2024
@mazyu36
Copy link
Contributor Author

mazyu36 commented Jun 7, 2024

@lpizzinidev
Thank you for the review!
I’v addressed all your comments.

Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Thanks 👍 Left some notes for some final adjustments

mazyu36 and others added 3 commits June 8, 2024 12:51
…e-model.ts

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
…e-model.ts

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
@mazyu36
Copy link
Contributor Author

mazyu36 commented Jun 8, 2024

@lpizzinidev
Thanks. I've addressed all your comments again.

Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Nice 👍

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 8, 2024
Copy link
Contributor

@paulhcsun paulhcsun left a comment

Choose a reason for hiding this comment

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

Hi @mazyu36, thanks for the contribution! Just left one comment about an additional piece of validation to add but otherwise this looks good!

Thanks for the review @lpizzinidev!

Comment on lines 139 to 144
/**
* The guardrail is applied to the invocation
*
* @default - No guardrail is applied to the invocation.
*/
readonly guardrailConfiguration?: GuardrailConfiguration;
Copy link
Contributor

@paulhcsun paulhcsun Jun 20, 2024

Choose a reason for hiding this comment

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

Hi both, the teams current stance is that we would prefer to have this interface containing the two required properties rather than exposing two separate attributes as it allows us to avoid adding more error handling and just enforce that both attributes are specified when defining the resource.

While the section linked from the Design Guidelines does state that flat property structure is preferred over a nested structure, that is only the case when the two properties are not required to be specified together. I can update that to clarify the distinction.

EDIT: @lpizzinidev apologies you are right. I was getting nested objects mixed up with enum-like classes.

Comment on lines 276 to 279
if (!Token.isUnresolved(guardrailIdentifier)) {
const guardrailConfigurationPattern = /^(([a-z0-9]+)|(arn:aws(-[^:]+)?:bedrock:[a-z0-9-]{1,20}:[0-9]{12}:guardrail\/[a-z0-9]+))$/;
if (!guardrailConfigurationPattern.test(guardrailIdentifier)) {
throw new Error(`guardrailIdentifier must match the ^(([a-z0-9]+)|(arn:aws(-[^:]+)?:bedrock:[a-z0-9-]{1,20}:[0-9]{12}:guardrail\/[a-z0-9]+))$ pattern, got ${guardrailIdentifier}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also add validation on the length constraints?

Min. 0, Max. 2048.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paulhcsun
Thanks.
I've added a validation and a unit test.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you are going to provide this kind of error checking could you please make the error message descriptive of what the expected input is instead of regex? It's better readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TheRealAmazonKendra
Thanks. I've changed the error message.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 20, 2024
@mergify mergify bot dismissed paulhcsun’s stale review June 20, 2024 23:42

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 21, 2024
@mazyu36
Copy link
Contributor Author

mazyu36 commented Jun 21, 2024

@paulhcsun
Thank you for the review.
I've addressed your comment.

@mazyu36 mazyu36 requested a review from paulhcsun June 21, 2024 00:18
@mazyu36 mazyu36 force-pushed the sfn-bedrock-guardrail-trace branch 2 times, most recently from 2444620 to 121bb62 Compare June 21, 2024 16:10
@mazyu36 mazyu36 force-pushed the sfn-bedrock-guardrail-trace branch from d5e5a58 to ea54fba Compare June 22, 2024 16:13
@@ -221,15 +221,18 @@ export class BedrockInvokeModel extends sfn.TaskStateBase {
}

if (this.props.guardrail) {
const isArn = this.props.guardrail.guardrailIdentifier.startsWith('arn:');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The policy document was incorrect for the case when guardrailIdentifier is an ARN, so I added a condition to address this.

const { guardrailIdentifier, guardrailVersion } = props.guardrail;

if (!Token.isUnresolved(guardrailIdentifier)) {
const guardrailPattern = /^(([a-z0-9]+)|(arn:aws(-[^:]+)?:bedrock:[a-z0-9-]{1,20}:[0-9]{12}:guardrail\/[a-z0-9]+))$/;
Copy link
Contributor

Choose a reason for hiding this comment

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

We have an ARN class that can be used instead of checking the pattern with regex: https://github.com/aws/aws-cdk/blob/e09126f6d97a52ee39811f8cbf874b722928debc/packages/aws-cdk-lib/core/lib/arn.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. However, after trying various approaches and searching for other implementation examples within the package, I couldn't figure out how to check the ARN. If you have any implementation examples, could you please share them with me?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes for sure, here's one example of using the formatArn() method:

const delegationRoleArn = Stack.of(this).formatArn({

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paulhcsun
Thank you.
I apologize for my poorly phrased question. While I was aware of the existence of the formatArn method, I didn't understand how to use it for validation.
After further investigation, I've implemented validation using ArnFormat. Is this approach on the right track?

7f307c9

Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies, I realize now that .formatArn() wouldn't have helped here. Your implementation looks good to me and follows a similar example.


}

const guardrailVersionPattern = /^(([1-9][0-9]{0,7})|(DRAFT))$/;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would prefer a stricter check that just ensures the version is between 10 and 99999999 or is "DRAFT" instead of using regex here. Just to improve the readability of this code and make it easier to maintain in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! The implementation looks good, I'd be happy to approve after the ARN class change is updated.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 27, 2024
@mergify mergify bot dismissed paulhcsun’s stale review June 29, 2024 10:33

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 29, 2024
@mazyu36 mazyu36 force-pushed the sfn-bedrock-guardrail-trace branch from e592ec6 to ad01e3b Compare June 29, 2024 13:15
@mazyu36 mazyu36 requested a review from paulhcsun June 29, 2024 14:09
@mazyu36 mazyu36 force-pushed the sfn-bedrock-guardrail-trace branch from c55c1df to 7f307c9 Compare July 4, 2024 14:01
@paulhcsun paulhcsun added the pr/do-not-merge This PR should not be merged at this time. label Jul 4, 2024
Copy link
Contributor

@paulhcsun paulhcsun left a comment

Choose a reason for hiding this comment

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

Nice work @mazyu36! I've approved the PR but added a do-not-merge label as we're currently holding off on merging anything while fixing a custom resources related issue which touches a lot of integ tests. This change probably would not affect that but we're pausing all merges temporarily to be safe. We should hopefully be able to get this merged in tomorrow if not sometime next week. Thanks for your patience and understanding! 🙏

@Leo10Gama Leo10Gama removed the pr/do-not-merge This PR should not be merged at this time. label Jul 5, 2024
Copy link
Contributor

mergify bot commented Jul 5, 2024

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit 772b1d1 into aws:main Jul 5, 2024
9 checks passed
Copy link
Contributor

mergify bot commented Jul 5, 2024

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

@mazyu36 mazyu36 deleted the sfn-bedrock-guardrail-trace branch July 9, 2024 01:12
GavinZZ pushed a commit that referenced this pull request Jul 10, 2024
…rty to the BedrockInvokeModel (#30426)

### Issue # (if applicable)

Closes #30425 

### Reason for this change
In the current [BedrockInvokeModel](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_stepfunctions_tasks.BedrockInvokeModel.html), guardrail configuration and trace for the invocation are not supported.



### Description of changes
Add `gurdrailConfiguration` and `trace` property to the `BedrockInvokeModel`



### Description of how you validated changes
Add unit tests and integ tests.



### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@aws-cdk-automation
Copy link
Collaborator

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

@aws aws locked as resolved and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
admired-contributor [Pilot] contributed between 13-24 PRs to the CDK feature-request A feature should be added or improved. p2 pr/needs-maintainer-review This PR needs a review from a Core Team Member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stepfunctions-tasks: enable guardrail and trace settings for BedrockInvokeModel
6 participants