feat(agentcore): add agentcore runtime L2 construct #35623
Conversation
github-actions
bot
added
p2
repeat-contributor
dineshSajwan
changed the title
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
| * and limitations under the License. | ||
| */ | ||
|
|
||
| export namespace RuntimePerms { |
There was a problem hiding this comment.
Due to unintended compatibility issues when transforming .ts code to the other supported cdk languages, lets avoid using namespaces. Just create the exported constants outside of the namespace declaration, and maybe attach a prefix to them, so we know they are runtime related (e.g. RUNTIME_INVOKE_PERMS)
| /** | ||
| * Agent runtime artifact configuration for Bedrock Agent Core Runtime | ||
| */ | ||
| export interface AgentRuntimeArtifactConfig { |
There was a problem hiding this comment.
Are we expecting this configuration to have more elements in the future? Do you know if there are plans for that? Seems like having this interface at the moment is not adding too much value, you could potentially return the URI directly in the bind method rather than this object (in CDK, we tend to favor flat composition over nested composition if we can)
There was a problem hiding this comment.
This mirrors the cloudformation structure of agentruntimeartifact . INHO, we should keep it as cloud formation structure because if the underlying service add any new field in this object then the flat structure of containeruri may cause breaking changes for the L2 construct.
| * Network configuration for the agent runtime | ||
| * @default - { networkMode: NetworkMode.PUBLIC } | ||
| */ | ||
| readonly networkConfiguration?: NetworkConfiguration; |
There was a problem hiding this comment.
For this specific one, we should emulate (or reuse if possible) this same behavior: https://github.com/aws/aws-cdk/pull/35577/files#diff-f2ed15a61a4ca23b4bc7c0ab451f5ccdbc60547853e3236a46d72b37c66f12de
Mostly the static implementation, it simplifies the configuration for the external user if they can just to a RuntimeNetworkConfiguration.PUBLIC_NETWORK rather than defining the nested object.
| * Optional description for the agent runtime | ||
| * @default - No description | ||
| */ | ||
| readonly description?: string; |
There was a problem hiding this comment.
If there are string size limitations, lets add them here as part of the JSDoc
| /** | ||
| * Authorizer configuration for Agent Runtime | ||
| */ | ||
| export interface AuthorizerConfigurationRuntime { |
There was a problem hiding this comment.
Will this Authorizer configuration allow more than one type of authorizer at the same time? I mean, can I add the cognito and the customJTW together? If not, if they are mutually exclusive, then we should rather have this as a preconfigured class, with static methods for each type, similar concept like AgentRuntimeArtifact.fromEcrRepository and AgentRuntimeArtifact.fromAsset, targeting each type of authorizer
| * Handles different authentication modes and converts them to the appropriate format | ||
| * CloudFormation expects PascalCase property names | ||
| */ | ||
| private formatAuthorizerConfiguration(config: AuthorizerConfigurationRuntime): any { |
There was a problem hiding this comment.
This method adds too much logic and validations and expects the external user to have a correct understanding of what combinations of parameters to pass when configuring a type of authorizer. We can greatly simplify that by granting static methods to the AuthorizerConfigurationRuntime (and having it as a class instead of a interface). That way, if the user wants to use, for example, Cognito, then they will have a specific method for it, that accepts only the Cognito configuration. I also added a note on this, in the types.ts file, where this interface is created
There was a problem hiding this comment.
Agreed. Will implement the changes
| * @param clientId Cognito App Client ID | ||
| * @param region Optional AWS region (defaults to stack region) | ||
| */ | ||
| public configureCognitoAuth( |
There was a problem hiding this comment.
This and the other methods below are exactly what I was expecting to see instead of https://github.com/aws/aws-cdk/pull/35623/files#r2398909386.
Simple and specific for each type of auth configuration. I would prefer to keep only this idea and drop the possibility for the user to set up manually the auth config with all the possible wrong combinations they could make if we let it fully configurable.
| /** | ||
| * An imported Runtime Endpoint | ||
| */ | ||
| class ImportedRuntimeEndpoint extends RuntimeEndpointBase { |
There was a problem hiding this comment.
We probably don't need this here, we normally define this methods and imports in the concrete class, not he abstract one
| public readonly endpointName: string; | ||
| public readonly agentRuntimeArn: string; | ||
| public readonly status?: string; | ||
| public readonly targetVersion?: string; |
There was a problem hiding this comment.
What is the difference between this and the agentRuntimeVersion ?
|
|
||
| private readonly endpointResource: bedrockagentcore.CfnRuntimeEndpoint; | ||
|
|
||
| constructor(scope: Construct, id: string, props: RuntimeEndpointProps) { |
There was a problem hiding this comment.
So we are allowing customers to define their stand alone RuntimeEndpoint, and not only in through the Runtime object. If that is the case, the lets make sure this is added in the RFC and in the README doc please
Issue # (if applicable)
Closes #<785>.
Reason for this change
Adding bedrock agent core runtime and runtime endpoint
Description of changes
Describe any new or updated permissions being added
The runtime creates a role with permission to ecr repo, cloudwatch , xray .
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