fix(bedrockagentcore): Memory no longer creates an unused execution role#38045
fix(bedrockagentcore): Memory no longer creates an unused execution role#38045vishwakt wants to merge 1 commit into
Memory no longer creates an unused execution role#38045Conversation
Memory unconditionally created an IAM execution role even when no execution role was provided and no memory strategies were configured. That role had a trust policy but zero permission policies, so it served no purpose, and at scale (many memories in one stack) it added needless IAM resources. The L1 CfnMemory works without MemoryExecutionRoleArn. The role is now created only when it is actually needed: when the caller provides one, when a customer managed KMS key is configured, or when one or more memory strategies are added (including via addMemoryStrategy after construction). A memory with only short-term storage and no strategies no longer creates a role, and MemoryExecutionRoleArn is omitted from the template. When there is no role, grantPrincipal resolves to an UnknownPrincipal so grants are no-ops rather than failing. Fixes aws#38021.
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
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.
✅ A exemption request has been requested. Please wait for a maintainer's review.
Memory no longer creates an unused execution role
|
Exemption Request for the integration test requirement. This is a synth-time change to the There is also no integration test for Coverage is by unit tests in
All 150 memory unit tests pass, and |
Issue # (if applicable)
Fixes #38021.
Reason for this change
The
MemoryL2 construct always created an IAM execution role, even when noexecutionRolewas provided and no memory strategies were configured. That auto-created role has a trust policy but zero permission policies, so it does nothing. In stacks with many memories this adds a large number of useless IAM roles, and the L1CfnMemorydeploys fine withoutMemoryExecutionRoleArn. pahud confirmed on the issue that making role creation conditional aligns with L1 behavior.Description of changes
The execution role is now created only when the memory actually needs one:
executionRoleis provided, orkmsKeyis configured (the service assumes the role to use the key), oraddMemoryStrategy(), which now creates the role on demand.A memory with only short-term storage and no strategies creates no role, and
MemoryExecutionRoleArnis omitted from the template.executionRoleisundefinedin that case, andgrantPrincipalresolves to anUnknownPrincipalso grant calls are no-ops rather than failing.Implementation notes:
executionRoleandgrantPrincipalare now getters over a private backing field so the role can be created lazily. This is API-compatible: jsii models areadonlyproperty and a getter identically, andyarn compatpasses.memoryExecutionRoleArnis rendered lazily so it reflects a role created after construction.On feature flags: the only behavior change is for a bare
new Memory(...)with no strategies, KMS key, or execution role. On upgrade, the empty role is removed from the template. I did not gate this behind a feature flag because (1) the removed resource is an empty role with no policies that, as noted on the issue, serves no purpose, (2)aws-bedrockagentcoreis a recently added module so the deployed footprint of bare memories is small, and (3) the maintainer endorsed making the behavior conditional in the constructor. I am happy to add aBugFixfeature flag if reviewers would prefer to gate it.Description of how you validated changes
Unit tests in
aws-bedrockagentcore/test/agentcore/memory/memory.test.ts:executionRoleundefined,MemoryExecutionRoleArnabsent,grantPrincipalis anUnknownPrincipal;MemoryExecutionRoleArn;addMemoryStrategy()on a role-less memory creates the role on demand;All 150 memory unit tests pass.
yarn build,yarn compat, and eslint are clean. No integration test exists forMemory, and this change is synth-only (no new CFN resource types or runtime behavior); anExemption Requestwill be filed if the PR linter asks for one.Checklist
Credit to @lokasandeep for the report and diagnosis, and to @pahud for confirming the approach. Happy to iterate based on review.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license