feat(memory): add agentcore memory l2 construct#35757
feat(memory): add agentcore memory l2 construct#35757mergify[bot] merged 26 commits intoaws:mainfrom
Conversation
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/memory/memory-strategy.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/memory/memory-strategy.ts
Outdated
Show resolved
Hide resolved
Pull request has been modified.
|
tagging the PR as "ready for review" since the RFC was approved |
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/memory/memory.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/memory/memory.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/memory/strategies/unified-strategy.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/memory/strategies/unified-strategy.ts
Outdated
Show resolved
Hide resolved
| throw new ValidationError(errors.join('\n')); | ||
| } | ||
| return param; | ||
| } |
There was a problem hiding this comment.
Validation errors should be scoped to the resource it emits. (This should've been specified in the design guidelines, not sure why it isn't), see here.
Also, the entire validation helper file seem to be very general functions that can be used elsewhere, and is quite similar to the ones added by runtime. To avoid duplication, use what is available in core/lib. If it's not available
in core, you can add it there.
There was a problem hiding this comment.
Yes we use the same validation methods in every part of agentcore alpha package. Is this a blocker for you now ? I can create a follow up PR to harmonize these across the three primitives (memory, 1p tools, runtime)
There was a problem hiding this comment.
It's not a strong blocker, but I'd prefer the validation errors be scoped to the resource at least so the users can know where the error is coming from.
There was a problem hiding this comment.
scoped in ddae010
as discussed will open a PR later to harmonize across the different pieces of the alpha package
Pull request has been modified.
|
|
||
| export const FULL_ACCESS_PERMS = [ | ||
| ...new Set([...STM.WRITE_PERMS, ...READ_PERMS, ...DELETE_PERMS, ...ADMIN_PERMS]), | ||
| ]; |
There was a problem hiding this comment.
I might be misunderstanding something here, but shouldn't these permissions be used in the grant methods?
There was a problem hiding this comment.
they are in the grantFullAccess method:
grantFullAccess(grantee: iam.IGrantable): iam.Grant {
return this.grant(grantee, ...MemoryPerms.FULL_ACCESS_PERMS);
}
Is that what you are referring to ?
There was a problem hiding this comment.
I can't find this grant method in the PR. Is it pushed to this PR?
There was a problem hiding this comment.
Yes in memory.ts, line 326
/**
* Grant the given principal identity permissions to do every action on this memory.
*
* @param grantee - The IAM principal to grant full access permissions to
* @default - Default grant configuration:
* - actions: ['bedrock-agentcore:CreateEvent',
'bedrock-agentcore:GetEvent',
'bedrock-agentcore:DeleteEvent',
'bedrock-agentcore:GetMemoryRecord',
'bedrock-agentcore:RetrieveMemoryRecords',
'bedrock-agentcore:ListMemoryRecords',
'bedrock-agentcore:ListActors',
'bedrock-agentcore:ListSessions',
'bedrock-agentcore:CreateMemory',
'bedrock-agentcore:GetMemory',
'bedrock-agentcore:DeleteMemory',
'bedrock-agentcore:UpdateMemory'] on this.memoryArn
* @returns An IAM Grant object representing the granted permissions
*/
grantFullAccess(grantee: iam.IGrantable): iam.Grant {
return this.grant(grantee, ...MemoryPerms.FULL_ACCESS_PERMS);
}
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/memory/strategies/managed-strategy.ts
Outdated
Show resolved
Hide resolved
Pull request has been modified.
| grantAdmin(grantee: iam.IGrantable): iam.Grant { | ||
| return this.grant(grantee, ...MemoryPerms.ADMIN_PERMS); | ||
| } | ||
| /** |
|
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). |
|
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). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Related to aws/aws-cdk-rfcs#825
Reason for this change
Adding a new alpha package for Amazon Bedrock AgentCore and add support for memory.
Description of changes
Describe any new or updated permissions being added
Using permissions for agent core defined in https://docs.aws.amazon.com/service-authorization/latest/reference/list_amazonbedrockagentcore.html
Description of how you validated changes
Unit tests, integration tests, manual tests
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license