-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(agentcore): add agentcore runtime L2 construct #35623
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/perms.ts
Outdated
Show resolved
Hide resolved
| /** | ||
| * Agent runtime artifact configuration for Bedrock Agent Core Runtime | ||
| */ | ||
| export interface AgentRuntimeArtifactConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mirrors the cloudformation structure of agentruntimeartifact . IMHO, 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.
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/runtime.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/runtime.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/types.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/runtime.ts
Outdated
Show resolved
Hide resolved
| * @param clientId Cognito App Client ID | ||
| * @param region Optional AWS region (defaults to stack region) | ||
| */ | ||
| public configureCognitoAuth( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/runtime-endpoint-base.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/runtime-endpoint.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/runtime-endpoint.ts
Show resolved
Hide resolved
| * | ||
| * @returns RuntimeAuthorizerConfiguration for IAM authentication | ||
| */ | ||
| public static configureIAM(): RuntimeAuthorizerConfiguration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to: usingIAM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
| * @param allowedAudience Optional array of allowed audiences | ||
| * @returns RuntimeAuthorizerConfiguration for JWT authentication | ||
| */ | ||
| public static configureJWT( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to usingJWT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
| * @param region Optional AWS region where the User Pool is located (defaults to stack region) | ||
| * @returns RuntimeAuthorizerConfiguration for Cognito authentication | ||
| */ | ||
| public static configureCognito( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to usingCognito
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
| * @param scopes Optional array of OAuth scopes | ||
| * @returns RuntimeAuthorizerConfiguration for OAuth authentication | ||
| */ | ||
| public static configureOAuth( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to usingOAuth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
| provider: string, | ||
| discoveryUrl: string, | ||
| clientId: string, | ||
| scopes?: string[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
provider and scopes parameters are ignored in the constructor if the OAuthAuthorizerConfiguration class. Why do we need them here then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently OAuthAuthorizerConfiguration and JwtAuthorizerConfiguration are behaving same way under the hood . In my view usingOAuth provides a better user experience (having a dedicated usingOAuth method makes it clear that developers are configuring OAuth providers like GitHub or Google, even though the underlying AWS service treats it the same as JWT authentication). I kept provider and scopes for potential future enhancements where we might add OAuth-specific features or some metadata for observability.
I'm happy to remove these unused parameters to keep the code cleaner. Would you prefer I remove them, or keep them documented as reserved for future use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets remove them for now, since they are not used. If we add features that need then, we can introduce them later. In the future, if more specific configuration is required for each OAuth providers, we can introduce special methods, like usingGoogleOAuth or something like that, and in there we request the additional data. From a user perspective, if the name of the method already gives you the provider, is very clear what is the intention behind it, and with the current approach we can manage to introduce this later when needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/runtime-endpoint.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/runtime-endpoint.ts
Show resolved
Hide resolved
| value: this.description, | ||
| fieldName: 'Description', | ||
| minLength: 1, | ||
| maxLength: 1200, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In CFN doc, the max length is 256
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh they changed it. Will update thanks.
| const account = Stack.of(this).account; | ||
|
|
||
| // ECR Image Access - scoped to account repositories | ||
| role.addToPolicy(new iam.PolicyStatement({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This policy is needed if the runtime artifact is fromEcrRepository right? Could we check if the agentRuntimeArtifact is of type EcrImage to add this policy? And maybe even restrict the resource to that specific one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was following these permission as per this . However , I realized that ECR permissions are redundant for L2 construct because these are already handled with AgentRuntimeArtifact bind function with grantPull. The ecr grantPull take care of permissions in best possible way. We need these permission during bind so that it can pull the image from ecr repository before deploying it on runtime. I have removed these permission from createExecutionRole and have tested the changes.
| })); | ||
|
|
||
| // ECR Token Access - must be * for authorization token | ||
| role.addToPolicy(new iam.PolicyStatement({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the previous policy, if only needed when artifact is fromEcrRepository, we should try and restrict the creation only in that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above , removed the permission as they were redundant and using ecr grantPull to grant the permission.
…ws-cdk into agentcore-runtime
| public readonly endpointName = attrs.endpointName; | ||
| public readonly agentRuntimeArn = attrs.agentRuntimeArn; | ||
| public readonly description = attrs.description; | ||
| public readonly status: string | undefined = undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets just mark this (and the ones below) as undefined without the string | undefined. The types were already defined in the Base class, no need to add them here again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
| readonly agentRuntimeName: string; | ||
|
|
||
| /** | ||
| * The IAM role ARN that provides permissions for the agent runtime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not an ARN but an actual role here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo. Fixed.
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/runtime-base.ts
Show resolved
Hide resolved
| /** | ||
| * The IAM role ARN | ||
| */ | ||
| readonly role: iam.IRole; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This, as the JSDoc states, should be a ARN, not the role object itself. And with the ARN, when creating the object in the from..Attributes we construct the actual role using the Role.fromRoleArn() method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
|
|
||
| // Skipping ECR Image Access (RUNTIME_ECR_IMAGE_ACTIONS) | ||
| // and ECR Token Access (RUNTIME_ECR_TOKEN_ACTIONS) permission | ||
| // because these are set by runtime-artifact (grantPull) with the bind function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets remove this comments
Pull request has been modified.
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
1 similar comment
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
packages/@aws-cdk/aws-bedrock-agentcore-alpha/agentcore/runtime/runtime-base.ts
Show resolved
Hide resolved
| | `runtimeName` | `string` | Yes | The name of the agent runtime. Valid characters are a-z, A-Z, 0-9, _ (underscore). Must start with a letter and can be up to 48 characters long | | ||
| | `agentRuntimeArtifact` | `AgentRuntimeArtifact` | Yes | The artifact configuration for the agent runtime containing the container configuration with ECR URI | | ||
| | `executionRole` | `iam.IRole` | No | The IAM role that provides permissions for the agent runtime. If not provided, a role will be created automatically | | ||
| | `networkConfiguration` | `NetworkConfiguration` | No | Network configuration for the agent runtime. Defaults to `{ networkMode: NetworkMode.PUBLIC }` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets update this to the new default value RuntimeNetworkConfiguration.usingPublicNetwork()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Pull request has been modified.
|
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#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