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

mazyu36 · 2024-06-02T05:54:19Z

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

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

lpizzinidev

Thanks 👍 Left some comments for adjustments on the interface.

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invoke-model.ts

lpizzinidev · 2024-06-04T19:52:16Z

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invoke-model.ts

+  /**
+   * The guardrail is applied to the invocation
+   *
+   * @default - No guardrail is applied to the invocation.
+   */
+  readonly guardrailConfiguration?: GuardrailConfiguration;


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

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

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 👍

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.

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.

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.

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.

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.

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.

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

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invoke-model.ts

mazyu36 · 2024-06-07T09:32:57Z

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

lpizzinidev

Thanks 👍 Left some notes for some final adjustments

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invoke-model.ts

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

mazyu36 · 2024-06-08T06:02:45Z

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

lpizzinidev

Nice 👍

paulhcsun

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!

paulhcsun · 2024-06-20T22:38:48Z

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invoke-model.ts

+  /**
+   * The guardrail is applied to the invocation
+   *
+   * @default - No guardrail is applied to the invocation.
+   */
+  readonly guardrailConfiguration?: GuardrailConfiguration;


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.

paulhcsun · 2024-06-20T22:42:17Z

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invoke-model.ts

+    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}`);


Could we also add validation on the length constraints?

Min. 0, Max. 2048.

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

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.

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

Pull request has been modified.

mazyu36 · 2024-06-21T00:18:10Z

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

mazyu36 · 2024-06-22T16:14:53Z

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invoke-model.ts

@@ -221,15 +221,18 @@ export class BedrockInvokeModel extends sfn.TaskStateBase {
    }

    if (this.props.guardrail) {
+      const isArn = this.props.guardrail.guardrailIdentifier.startsWith('arn:');


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

paulhcsun · 2024-06-26T18:13:36Z

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invoke-model.ts

+    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]+))$/;


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

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?

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

aws-cdk/packages/aws-cdk-lib/aws-route53/README.md

Line 305 in b7f626b

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

@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

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

paulhcsun · 2024-06-27T21:33:48Z

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invoke-model.ts

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


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.

Thank you. I have implemented it as follows.

https://github.com/aws/aws-cdk/pull/30426/files#diff-254b356f1a1b98a7034dec79302bc664e46a4de89a1fe14aa5c62d8982109dc2R15

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

Pull request has been modified.

paulhcsun

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! 🙏

mergify · 2024-07-05T17:56:11Z

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 · 2024-07-05T19:20:23Z

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 · 2024-07-05T19:20:45Z

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

…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 · 2024-07-25T20:49:27Z

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.

feat: add guardrailConfiguration an trace to the BedrockInvokeModel

747827a

github-actions bot added feature-request A feature should be added or improved. p2 labels Jun 2, 2024

aws-cdk-automation requested a review from a team June 2, 2024 05:54

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

fix: update README

a54228f

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

github-actions bot mentioned this pull request Jun 3, 2024

Weekly PR metrics report #30434

Closed

lpizzinidev suggested changes Jun 4, 2024

View reviewed changes

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 added 2 commits June 6, 2024 08:52

a

094a115

incorporated review comments

9234818

lpizzinidev suggested changes Jun 7, 2024

View reviewed changes

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invoke-model.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invoke-model.ts Outdated Show resolved Hide resolved

mazyu36 and others added 3 commits June 8, 2024 12:51

Update packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invok…

aae5122

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

Update packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/bedrock/invok…

d7c850a

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

error fixed

0a63f7f

lpizzinidev approved these changes Jun 8, 2024

View reviewed changes

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

paulhcsun previously requested changes Jun 20, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 20, 2024

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 21, 2024

mazyu36 requested a review from paulhcsun June 21, 2024 00:18

mazyu36 force-pushed the sfn-bedrock-guardrail-trace branch 2 times, most recently from 2444620 to 121bb62 Compare June 21, 2024 16:10

mazyu36 added 2 commits June 23, 2024 01:12

rename guardrailConfiguration to guardrail

5781569

fix: fix policy statement when guardrailIdentifier is set to arn

ea54fba

mazyu36 force-pushed the sfn-bedrock-guardrail-trace branch from d5e5a58 to ea54fba Compare June 22, 2024 16:13

mazyu36 commented Jun 22, 2024

View reviewed changes

mazyu36 and others added 2 commits June 23, 2024 01:31

Merge branch 'main' into sfn-bedrock-guardrail-trace

d8d5721

Merge branch 'main' into sfn-bedrock-guardrail-trace

be97141

paulhcsun previously requested changes Jun 27, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 27, 2024

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 29, 2024

incorporated review comments

ad01e3b

mazyu36 force-pushed the sfn-bedrock-guardrail-trace branch from e592ec6 to ad01e3b Compare June 29, 2024 13:15

Merge branch 'main' into sfn-bedrock-guardrail-trace

4603188

mazyu36 requested a review from paulhcsun June 29, 2024 14:09

update README

17506d6

github-actions bot mentioned this pull request Jul 1, 2024

Monthly PR metrics report #30715

Closed

fix: validate arn

7f307c9

mazyu36 force-pushed the sfn-bedrock-guardrail-trace branch from c55c1df to 7f307c9 Compare July 4, 2024 14:01

paulhcsun added the pr/do-not-merge This PR should not be merged at this time. label Jul 4, 2024

paulhcsun approved these changes Jul 4, 2024

View reviewed changes

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

Merge branch 'main' into sfn-bedrock-guardrail-trace

80fc07b

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

mazyu36 deleted the sfn-bedrock-guardrail-trace branch July 9, 2024 01:12

aws locked as resolved and limited conversation to collaborators Jul 25, 2024

-  /**
-   * 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;


		}

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

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

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

Conversation

mazyu36 commented Jun 2, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

lpizzinidev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mazyu36 Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

paulhcsun Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

TheRealAmazonKendra Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mazyu36 commented Jun 7, 2024

lpizzinidev left a comment

Choose a reason for hiding this comment

mazyu36 commented Jun 8, 2024 • edited Loading

lpizzinidev left a comment

Choose a reason for hiding this comment

paulhcsun left a comment

Choose a reason for hiding this comment

paulhcsun Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mazyu36 commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulhcsun left a comment

Choose a reason for hiding this comment

mergify bot commented Jul 5, 2024

aws-cdk-automation commented Jul 5, 2024

AWS CodeBuild CI Report

mergify bot commented Jul 5, 2024

aws-cdk-automation commented Jul 25, 2024

mazyu36 Jun 7, 2024 •

edited

Loading

paulhcsun Jun 20, 2024 •

edited

Loading

TheRealAmazonKendra Jun 21, 2024 •

edited

Loading

mazyu36 commented Jun 8, 2024 •

edited

Loading

paulhcsun Jun 20, 2024 •

edited

Loading