-
Notifications
You must be signed in to change notification settings - Fork 616
bring back bed rock auth #13089
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
bring back bed rock auth #13089
Conversation
Signed-off-by: Yuval Kohavi <[email protected]>
ccb0a61 to
3177084
Compare
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.
Pull request overview
This PR restores AWS Bedrock authentication functionality that was previously removed in PR #12830. The implementation adds authentication support for AWS Bedrock backends in the agent gateway, allowing users to configure either implicit AWS SDK authentication or explicit credentials via Kubernetes secrets.
Key changes:
- Added
AwsAuthAPI type with support for secret-based AWS credentials - Implemented
buildBedrockAuthPolicyto translate auth configuration into backend policies - Updated test coverage with a new test case for Bedrock with secret reference
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
api/v1alpha1/agentgateway/agentgateway_backend_types.go |
Added AwsAuth struct and Auth field to BedrockConfig for configuring AWS authentication |
pkg/kgateway/agentgatewaysyncer/backend/translate.go |
Implemented buildBedrockAuthPolicy function and updated translateLLMProvider to handle Bedrock auth for both single and multi-provider configurations |
pkg/kgateway/agentgatewaysyncer/backend/translate_test.go |
Added test case "Bedrock backend with new secret ref" to verify secret-based authentication |
pkg/kgateway/agentgatewaysyncer/backend/testdata/Valid_Bedrock_backend_with_custom_region_and_guardrail.yaml |
Updated test snapshot to include implicit AWS auth policy |
pkg/kgateway/agentgatewaysyncer/backend/testdata/Bedrock_backend_with_new_secret_ref.yaml |
New test snapshot showing explicit AWS credentials from secret |
pkg/kgateway/agentgatewaysyncer/backend/testdata/Bedrock_backend_with_new_route_types_(responses_and_anthropic_token_count).yaml |
Updated test snapshot to include implicit AWS auth policy |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Kind: &api.BackendAuthPolicy_Aws{ | ||
| Aws: &api.Aws{ | ||
| Kind: &api.Aws_ExplicitConfig{ | ||
| ExplicitConfig: &api.AwsExplicitConfig{ | ||
| AccessKeyId: accessKeyId, | ||
| SecretAccessKey: secretAccessKey, | ||
| SessionToken: sessionToken, | ||
| Region: region, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, errors.Join(errs...) | ||
|
|
Copilot
AI
Dec 10, 2025
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.
The function returns a BackendAuthPolicy even when there are validation errors (errs slice is not empty). This could result in an auth policy being returned with empty accessKeyId or secretAccessKey fields when the corresponding secret values are missing. Consider returning nil for the auth policy when errors are present, or ensure that the error check happens before the policy is used.
| errs = append(errs, errors.New("accessKey is missing or not a valid string")) | ||
| } else { | ||
| accessKeyId = value | ||
| } | ||
|
|
||
| // Extract secret key | ||
| if value, exists := kubeutils.GetSecretValue(secret, wellknown.SecretKey); !exists { | ||
| errs = append(errs, errors.New("secretKey is missing or not a valid string")) | ||
| } else { | ||
| secretAccessKey = value | ||
| } | ||
|
|
Copilot
AI
Dec 10, 2025
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.
The error messages for missing secret keys don't include context about which secret is being accessed. Consider including the secret name and namespace in the error messages to help users identify which secret needs to be fixed. For example: "accessKey is missing in secret test-ns/bedrock-secret".
| // SecretRef references a Kubernetes Secret containing the AWS credentials. | ||
| // The Secret must have keys "accessKey", "secretKey", and optionally "sessionToken". | ||
| // +required | ||
| SecretRef *corev1.LocalObjectReference `json:"secretRef,omitempty"` |
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.
If required, let's remove omitempty and change it to corev1.LocalObjectReference
| }, | ||
| }, | ||
| { | ||
| name: "Bedrock backend with new secret ref", |
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.
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
| type AwsAuth struct { | ||
| // SecretRef references a Kubernetes Secret containing the AWS credentials. | ||
| // The Secret must have keys "accessKey", "secretKey", and optionally "sessionToken". | ||
| // +required |
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.
Should not be required. If not set, we use the implciit workload identity
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.
nvm, missed the 'if not set, use implicit' at the higher level. But if wer move it, we will need to make it optional
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.
moved it to BackendAuth and made optional. the secret is still required - if in the future we will add more options, we can make it optional then - it is not a breaking change
| // When omitted, we will try to use the default AWS SDK authentication methods. | ||
| // | ||
| // +optional | ||
| Auth *AwsAuth `json:"auth,omitempty"` |
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.
hmmm not sure this belongs here vs policies.backend.auth.aws. That would be consistent. On the other hand, this is currently only usable for bedrock. But in the future probably we would support lambda, etc, which would necessitate putting it in a more broad area.
I think for consistency and future proofing we need to move this.
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'll try but it will be slightly annoying to do - as if there is no policy, i'll want to implicitly attach the implicit one inline
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.
nvm - agentgw will default to the implicit one, so doing this is not annoying
there's no region in the policy, so this depends on agentgateway/agentgateway#718 Signed-off-by: Yuval Kohavi <[email protected]>
| type AwsAuth struct { | ||
| // SecretRef references a Kubernetes Secret containing the AWS credentials. | ||
| // The Secret must have keys "accessKey", "secretKey", and optionally "sessionToken". | ||
| // +required |
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 going to add workload credentials in the future?
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.
you mean from the env of the pod? i think that is already supported - it's the default if you don't specify auth. cc @howardjohn
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
50fb348 to
065d8d8
Compare
|
weird flake: note for #12981 maybe we can better cache docker images |
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
|
flakes on docker gateway timeout |
Signed-off-by: Yuval Kohavi <[email protected]>
Description
Add auth for bedrock backend
Change Type
Changelog
Additional Notes
While this looks like a feature is added, this actually just brings back something that was removed in #12830. as that PR didn't made it to a release yet, there shouldn't be a change in the release notes.