feat(model_gateway): add native Amazon Bedrock routing with SigV4#1472
feat(model_gateway): add native Amazon Bedrock routing with SigV4#1472MohanKumar21 wants to merge 2 commits into
Conversation
MohanKumar21
requested review from
CatherineSue,
key4ng and
slin1237
as code owners
github-actions
Bot
added
documentation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds AWS Bedrock support across provider detection, router config/CLI, SigV4 signing, Bedrock router (request/response mapping, streaming), event-stream decoding, factory/registry wiring, tests, and related docs. ChangesAWS Bedrock Router Integration Bedrock Provider & Docs
Configuration, CLI & Validation
Signing & Crypto Dependencies
Request/Response & Event Stream Mapping
Bedrock Router & Errors
Factory & Provider Registry Wiring
Router Manager & Workflows
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b0b635205
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/getting-started/external-providers.md (1)
14-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrerequisite text is now too narrow for Bedrock users.
“An API key for at least one external provider” is inaccurate for Bedrock-only setups that use AWS credentials.
Suggested edit
-- An API key for at least one external provider +- Credentials for at least one external provider (for example API key or AWS credentials for Bedrock)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/getting-started/external-providers.md` at line 14, Update the prerequisite line "An API key for at least one external provider" to acknowledge Bedrock-only setups that use AWS IAM credentials by rephrasing to something like "Credentials for at least one external provider (e.g., an API key) or AWS credentials for Bedrock-only setups"; locate and edit the exact string "An API key for at least one external provider" in docs/getting-started/external-providers.md to make the wording inclusive of AWS credentials and keep examples concise.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@model_gateway/Cargo.toml`:
- Around line 128-129: Update the cryptographic dependency versions in
Cargo.toml: change the hmac dependency entry from "0.12" to "0.13.0" and the hex
dependency entry from "0.4" to "0.4.3" so the hmac and hex crates use the latest
stable releases.
In `@model_gateway/src/config/types.rs`:
- Around line 220-240: The default for Bedrock runtime is incorrect: update the
Default impl for the BedrockConfig struct so the service field defaults to
"bedrock-runtime" (not "bedrock") to ensure SigV4 signs against the
runtime/data-plane endpoint; locate the BedrockConfig struct and its impl
Default and change the service initialization from "bedrock".to_string() to
"bedrock-runtime".to_string().
- Line 225: The Bedrock config currently allows pub region: Option<String> (in
model_gateway/src/config/types.rs) but runtime silently falls back to
"us-east-1", causing signature errors; update the config validation logic that
checks Bedrock routing mode to require region be present and non-empty when
bedrock routing is enabled, or alternatively change the runtime region
resolution to read AWS_REGION or AWS_DEFAULT_REGION from the environment before
using the hardcoded "us-east-1" default; locate the Bedrock-related validation
function (the config validation code that inspects the bedrock struct/field) and
either enforce presence of bedrock.region there or add environment-aware
resolution prior to defaulting.
In `@model_gateway/src/config/validation.rs`:
- Around line 373-377: In the RoutingMode::Bedrock { worker_urls } branch,
reject any worker URL that uses the "grpc" scheme before/after calling
Self::validate_urls; iterate worker_urls (if non-empty) and parse each with
url::Url::parse (or check starts_with("grpc://")) and return a validation error
(e.g., the same error type used elsewhere in this module) if url.scheme() ==
"grpc". Keep the existing allowance for an empty list, but ensure validate_urls
still runs for other checks and add the extra grpc-scheme guard to prevent
invalid Bedrock configs from passing.
In `@model_gateway/src/routers/bedrock/errors.rs`:
- Around line 40-45: The NOT_IMPLEMENTED response for unsupported endpoints is
missing a JSON content-type header; update the Response::builder call in
unsupported_endpoint (or the function that returns this Response) to include a
Content-Type: application/json header (e.g., add .header(header::CONTENT_TYPE,
"application/json") or equivalent) while keeping the
StatusCode::NOT_IMPLEMENTED, Body::from(...) and the existing unwrap_or_else
fallback intact.
In `@model_gateway/src/routers/bedrock/response_map.rs`:
- Around line 55-59: The Usage construction currently sets total_tokens to 0
when Bedrock omits it, causing inconsistent stats; update the code paths that
build Usage (the one creating variable usage from parsed.usage and the similar
block around lines 75-77) to backfill total_tokens by summing input_tokens and
output_tokens when total_tokens is None but input_tokens/output_tokens are Some,
i.e., compute total = total_tokens.unwrap_or_else(|| input_tokens.unwrap_or(0) +
output_tokens.unwrap_or(0)) and use that value when constructing the Usage
instance so total_tokens reflects the sum if present.
- Around line 49-53: The current binding for text uses find_map on
parsed.output.as_ref().and_then(|o| o.message.content.iter().find_map(|c|
c.text.clone())) which only takes the first content.text and truncates
multi-part assistant outputs; change this to collect/flatten all content.text
from o.message.content (e.g., map and filter_map over c.text), join them (with
"" or a separator) and use that combined string as the text variable so all
Bedrock content blocks are preserved (update the variable `text` and the
expression that builds it in response_map.rs).
In `@model_gateway/src/routers/bedrock/router.rs`:
- Line 157: Replace the hardcoded 120s timeout
(.timeout(Duration::from_secs(120))) with a configurable value from the router
config; read the timeout (e.g., context.router_config.request_timeout_secs or a
Bedrock-specific timeout) and convert it to a Duration, falling back to a
sensible default if unset, then pass that Duration into .timeout(...) so
deployments can control request timeouts; ensure you reference the existing
context and router_config fields used elsewhere in router.rs and handle
non-positive or missing values safely.
In `@model_gateway/src/routers/bedrock/signing.rs`:
- Line 35: The http_client field is created with reqwest::Client::new() which
has no timeouts and can block indefinitely; replace this with a configured
client using reqwest::Client::builder() and set sensible timeouts (e.g., request
timeout via timeout(Duration::from_secs(...)) and optionally connect_timeout),
then .build() and assign to the http_client field so all ECS/IMDS credential
metadata calls use the timeout; update any initialization site that sets
http_client to use the builder and make the timeout value configurable or a
constant.
- Line 11: The credentials field currently uses OnceCell<AwsCredentials> which
permanently caches credentials and discards the Expiration returned by AWS;
modify this to store credentials plus their Expiration and refresh metadata
(e.g., replace OnceCell with a lock/atomic wrapper around a struct like
CachedAwsCredentials { creds: AwsCredentials, expires_at: DateTime } and a
refresh task), update extract_credential_fields() to parse and preserve the
Expiration timestamp into that cached struct, and implement proactive refresh
logic in the signing flow (or a background task) to refresh when now >=
(expires_at - Duration::minutes(5)) so SigV4 signing always uses valid
credentials and automatically refreshes before expiry.
In `@model_gateway/src/routers/bedrock/tests.rs`:
- Around line 63-66: The test currently sets global AWS env vars
(AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_REGION,
AWS_EC2_METADATA_DISABLED) which can race with other tests; update the test that
contains these std::env::set_var calls to run serially by adding serial_test =
"3.0" to [dev-dependencies] in Cargo.toml and annotating the test function with
the serial_test::serial attribute (or use the serial_test::serial attribute
macro), or alternatively replace direct env mutation by injecting/mocking the
AWS credential provider used by the Bedrock client creation (the code paths
around the env setters and any function that builds the AWS client) so the test
does not mutate process-wide state.
---
Outside diff comments:
In `@docs/getting-started/external-providers.md`:
- Line 14: Update the prerequisite line "An API key for at least one external
provider" to acknowledge Bedrock-only setups that use AWS IAM credentials by
rephrasing to something like "Credentials for at least one external provider
(e.g., an API key) or AWS credentials for Bedrock-only setups"; locate and edit
the exact string "An API key for at least one external provider" in
docs/getting-started/external-providers.md to make the wording inclusive of AWS
credentials and keep examples concise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 40510561-df89-45e2-84bd-b16a8992cd35
📒 Files selected for processing (24)
crates/protocols/src/worker.rsdocs/getting-started/external-providers.mdmodel_gateway/Cargo.tomlmodel_gateway/src/config/builder.rsmodel_gateway/src/config/types.rsmodel_gateway/src/config/validation.rsmodel_gateway/src/main.rsmodel_gateway/src/routers/bedrock/context.rsmodel_gateway/src/routers/bedrock/errors.rsmodel_gateway/src/routers/bedrock/mod.rsmodel_gateway/src/routers/bedrock/request_map.rsmodel_gateway/src/routers/bedrock/response_map.rsmodel_gateway/src/routers/bedrock/router.rsmodel_gateway/src/routers/bedrock/signing.rsmodel_gateway/src/routers/bedrock/tests.rsmodel_gateway/src/routers/factory.rsmodel_gateway/src/routers/mod.rsmodel_gateway/src/routers/openai/provider/bedrock.rsmodel_gateway/src/routers/openai/provider/mod.rsmodel_gateway/src/routers/openai/provider/registry.rsmodel_gateway/src/routers/router_manager.rsmodel_gateway/src/server.rsmodel_gateway/src/workflow/job_queue.rsmodel_gateway/src/workflow/steps/external/discover_models.rs
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Amazon Bedrock support, adding a dedicated router, AWS SigV4 signing, and mapping logic for the Bedrock Converse API. The review feedback highlights several necessary improvements: implementing true streaming via the /converse-stream endpoint to reduce latency, replacing a hardcoded timeout with the global configuration, and correctly mapping Bedrock's stop reasons to OpenAI's finish reasons. Additionally, the reviewer noted that AWS credential resolution should include EKS web identity tokens and that model discovery logic must be adjusted to handle Bedrock's unique authentication and API structure.
| "role": "assistant", | ||
| "content": text, | ||
| }, | ||
| "finish_reason": parsed.stop_reason.unwrap_or_else(|| "stop".to_string()), |
There was a problem hiding this comment.
The Bedrock stop_reason is being passed directly to the OpenAI finish_reason field without translation. Bedrock uses values like end_turn and max_tokens, whereas OpenAI clients expect stop and length. This can break client logic that relies on standard OpenAI finish reasons. Map end_turn to stop, max_tokens to length, stop_sequence to stop, content_filtering to content_filter, and tool_use to tool_calls.
References
- For protocol data structures that mirror an external API (e.g., OpenAI), prioritize alignment with the external specification over internal consistency.
MohanKumar21
force-pushed
the
ksmkumar/add-aws-routing
branch
from
2b0b635 to
8e2ad71
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e2ad717b2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (10)
model_gateway/src/routers/bedrock/response_map.rs (2)
55-59:⚠️ Potential issue | 🟡 MinorToken backfill issue remains unresolved.
The past review comment identified that when Bedrock omits
total_tokens, the code returns0instead of computing the sum ofinput_tokensandoutput_tokens, creating inconsistent usage statistics. This issue is still present.Also applies to: 75-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model_gateway/src/routers/bedrock/response_map.rs` around lines 55 - 59, The current unwrap_or fallback for parsed.usage creates a Usage with total_tokens = 0 even when input_tokens and output_tokens exist; update the construction so that if parsed.usage is None or its total_tokens is None you compute total_tokens = input_tokens.unwrap_or(0) + output_tokens.unwrap_or(0) and set that into the Usage struct; specifically adjust the code that assigns the local usage (the let usage = ... assignment referencing parsed.usage and the Usage struct with fields input_tokens/output_tokens/total_tokens) and make the same change at the other occurrence (the similar block around the second instance referenced 75-77) so total_tokens is backfilled from input_tokens + output_tokens when missing.
49-53:⚠️ Potential issue | 🟠 MajorMulti-part content blocks are still being truncated.
The past review comment on lines 49-53 identified that using
find_maponly extracts the firsttextcontent block, truncating multi-part assistant outputs. This issue remains unresolved in the current code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model_gateway/src/routers/bedrock/response_map.rs` around lines 49 - 53, The current extraction for the assistant response uses parsed.output.as_ref().and_then(...find_map(...)).unwrap_or_default() which only returns the first content.text and truncates multi-part message blocks; update the logic that builds the variable text to iterate over all content entries in parsed.output.message.content, collect all non-empty c.text values (for the assistant role or appropriate content entries), and concatenate them (e.g., join or fold) into a single String before using unwrap_or_default; target the code around the variable text and the parsed.output -> .message.content traversal to replace find_map with a collect+join/fold to preserve multi-part outputs.model_gateway/src/routers/bedrock/signing.rs (2)
35-35:⚠️ Potential issue | 🟠 MajorHTTP client timeout configuration remains missing.
The past review comment on line 35 identified that
Client::new()has no timeouts, which can block credential resolution indefinitely when calling ECS/IMDS metadata endpoints. This issue is still unresolved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model_gateway/src/routers/bedrock/signing.rs` at line 35, The http_client field is created with reqwest::Client::new() which has no timeouts and can block credential resolution (ECS/IMDS); update the initialization of http_client (the line creating "http_client: reqwest::Client::new()") to use reqwest::Client::builder() and set sensible timeouts (e.g., connect_timeout and overall timeout or a single timeout) so metadata/credential calls fail fast; ensure the change is applied where the signing client struct/constructor that owns http_client is initialized so ECS/IMDS calls no longer hang indefinitely.
11-11:⚠️ Potential issue | 🟠 MajorCredential expiration tracking remains unimplemented.
The past review comment on line 11 identified that
OnceCell<AwsCredentials>permanently caches credentials without tracking expiration. ECS task credentials and IMDS IAM role credentials include anExpirationfield that must be honored. This issue is still unresolved, and expired credentials will cause SigV4 signature failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model_gateway/src/routers/bedrock/signing.rs` at line 11, The code currently uses OnceCell<AwsCredentials> which permanently caches credentials and ignores their Expiration; change the caching to store both credentials and their expiration and refresh them when expired (e.g. replace OnceCell<AwsCredentials> with a thread-safe container like Mutex/RwLock or tokio::sync::RwLock holding Option<CachedCredentials { creds: AwsCredentials, expires_at: Instant/DateTime }>), update the credential-fetching path (the functions that read from OnceCell/AwsCredentials in signing.rs) to check expires_at before use and asynchronously refresh/fetch new ECS/IMDS credentials when expired, and ensure the refresh logic updates the stored CachedCredentials atomically so SigV4 signing always uses current, non-expired credentials.model_gateway/src/routers/bedrock/tests.rs (1)
63-66:⚠️ Potential issue | 🟡 MinorEnvironment variable pollution risk remains unresolved.
The past review comment on lines 63-66 identified that setting global environment variables in tests can cause race conditions when Cargo runs tests in parallel. The recommendation to use
serial_test::serialattribute or mock the credential provider has not been implemented.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model_gateway/src/routers/bedrock/tests.rs` around lines 63 - 66, The test currently sets global AWS env vars (the lines setting AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_REGION, AWS_EC2_METADATA_DISABLED) which can race with other tests; fix by either marking the specific test function in tests.rs with the serial_test::serial attribute to force serial execution (add the attribute above the test function that contains these env set calls) or replace global env mutation by injecting/mocking credentials via a local/mock credential provider used by the code under test (e.g., construct a mock AWS credential provider or use a test-only config builder so you do not call std::env::set_var). Ensure you remove global env mutations if you choose mocking and keep test-scoped setup/teardown if you must mutate env.model_gateway/src/routers/bedrock/errors.rs (1)
39-45:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSet JSON
Content-Typefor unsupported endpoint response.Line 42 returns a JSON body but no
application/jsonheader, which can break strict clients.🔧 Suggested fix
-use axum::{body::Body, http::StatusCode, response::Response}; +use axum::{body::Body, http::{header::CONTENT_TYPE, StatusCode}, response::Response}; pub(crate) fn unsupported_endpoint() -> Response { Response::builder() .status(StatusCode::NOT_IMPLEMENTED) + .header(CONTENT_TYPE, "application/json") .body(Body::from( "{\"error\":{\"code\":\"not_supported\",\"message\":\"Endpoint not yet supported for bedrock router\"}}", )) .unwrap_or_else(|_| error::not_implemented("not_supported", "Unsupported endpoint")) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model_gateway/src/routers/bedrock/errors.rs` around lines 39 - 45, The unsupported_endpoint() function builds a Response with a JSON body but omits the Content-Type header; update the Response::builder() chain in unsupported_endpoint() to include a header "Content-Type" = "application/json" (while keeping the StatusCode::NOT_IMPLEMENTED and the existing Body::from payload) so strict clients correctly recognize the JSON response; keep the existing unwrap_or_else fallback to error::not_implemented("not_supported", "Unsupported endpoint").model_gateway/src/config/types.rs (1)
232-239:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDefault Bedrock SigV4 service should target runtime (
bedrock-runtime).Line 236 defaults to
"bedrock", but this router calls runtime Converse endpoints; signing with the control-plane service name can cause auth/signature failures.🔧 Suggested fix
impl Default for BedrockConfig { fn default() -> Self { Self { region: None, - service: "bedrock".to_string(), + service: "bedrock-runtime".to_string(), model_map: HashMap::new(), } } }For Amazon Bedrock Converse/Invoke runtime APIs, what SigV4 service name must be used: "bedrock" or "bedrock-runtime"?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model_gateway/src/config/types.rs` around lines 232 - 239, The Default impl for BedrockConfig sets service to "bedrock" which is incorrect for runtime Converse/Invoke endpoints; update the impl Default for BedrockConfig (the default() method) to set the service field to "bedrock-runtime" so SigV4 signs against the Bedrock runtime service (refer to BedrockConfig and its default() implementation).model_gateway/src/routers/bedrock/router.rs (2)
44-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid hardcoded
us-east-1fallback for signer region.Line 47 silently defaults region to
us-east-1; for other Bedrock regions this can generate invalid signatures unless callers override config explicitly. Prefer explicit config requirement or environment resolution (AWS_REGION/AWS_DEFAULT_REGION) before fallback.For AWS SigV4 signing to Bedrock runtime endpoints, must the signing region match the endpoint region (e.g., us-west-2 endpoint signed with us-west-2)?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model_gateway/src/routers/bedrock/router.rs` around lines 44 - 47, The code currently defaults bedrock.region to the hardcoded "us-east-1" which can produce bad SigV4 signatures; change the logic around bedrock.region (the region variable) to first use bedrock.region if provided, then try environment resolution by checking AWS_REGION and then AWS_DEFAULT_REGION, and if neither is set return an explicit configuration/error instead of silently falling back—update the region assignment in router.rs to perform that env lookup and return an error result (or propagate a configuraton error) when no region can be determined so signing uses the correct Bedrock region.
157-157: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse router-configured request timeout instead of a fixed 120s.
Line 157 hardcodes timeout and ignores
request_timeout_secs, making Bedrock behavior diverge from other routers and deployment tuning.🔧 Suggested fix
- .timeout(Duration::from_secs(120)) + .timeout(Duration::from_secs(self.retry_config.max_backoff_ms.max(1) / 1000))// Better: pass router timeout into BedrockRouter and use it directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model_gateway/src/routers/bedrock/router.rs` at line 157, The timeout is hardcoded to 120s; change the code that calls .timeout(Duration::from_secs(120)) to use the router-configured request timeout (request_timeout_secs) instead: access the timeout value from BedrockRouter (e.g., self.request_timeout_secs or a timeout field passed into the BedrockRouter constructor) and replace Duration::from_secs(120) with Duration::from_secs(request_timeout_secs as u64) (or store as Duration up-front). Ensure the router is constructed with that timeout if not already (pass request_timeout_secs into BedrockRouter) and use the router field in the place of the hardcoded 120s.model_gateway/src/config/validation.rs (1)
373-378:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject
grpc://worker URLs for Bedrock mode.Line 376 reuses
validate_urls(...), which still permitsgrpc://; that allows invalid Bedrock config to pass and fail later during request execution.🔧 Suggested fix
RoutingMode::Bedrock { worker_urls } => { // Allow empty URLs to support dynamic worker addition if !worker_urls.is_empty() { Self::validate_urls(worker_urls)?; + if worker_urls.iter().any(|u| u.starts_with("grpc://") || u.starts_with("grpcs://")) { + return Err(ConfigError::InvalidValue { + field: "worker_url".to_string(), + value: "grpc://...".to_string(), + reason: "Bedrock mode only supports http:// or https:// URLs".to_string(), + }); + } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model_gateway/src/config/validation.rs` around lines 373 - 378, The Bedrock branch currently calls Self::validate_urls(worker_urls) which allows grpc:// URLs; update the RoutingMode::Bedrock { worker_urls } handling to explicitly reject any worker URL with the "grpc" scheme (e.g. starts_with("grpc://") or parsed URL.scheme() == "grpc") before calling Self::validate_urls, returning an Err on first occurrence; reference RoutingMode::Bedrock, validate_urls, and worker_urls when making this change so invalid grpc:// endpoints are rejected at config validation time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@model_gateway/src/routers/bedrock/request_map.rs`:
- Around line 116-123: The fallback inserts a ContentBlock with empty text which
Bedrock rejects; in the empty-messages branch update the inserted
BedrockMessage/ContentBlock (the messages vector, BedrockMessage struct and
ContentBlock) to provide a non-empty default text (e.g., a meaningful
placeholder like "Hello" or a short sentinel such as "[no input]") or
alternatively short-circuit by returning an error when messages is empty; ensure
the change touches the messages push where BedrockMessage and ContentBlock are
constructed so the ContentBlock.text is never an empty string.
---
Duplicate comments:
In `@model_gateway/src/config/types.rs`:
- Around line 232-239: The Default impl for BedrockConfig sets service to
"bedrock" which is incorrect for runtime Converse/Invoke endpoints; update the
impl Default for BedrockConfig (the default() method) to set the service field
to "bedrock-runtime" so SigV4 signs against the Bedrock runtime service (refer
to BedrockConfig and its default() implementation).
In `@model_gateway/src/config/validation.rs`:
- Around line 373-378: The Bedrock branch currently calls
Self::validate_urls(worker_urls) which allows grpc:// URLs; update the
RoutingMode::Bedrock { worker_urls } handling to explicitly reject any worker
URL with the "grpc" scheme (e.g. starts_with("grpc://") or parsed URL.scheme()
== "grpc") before calling Self::validate_urls, returning an Err on first
occurrence; reference RoutingMode::Bedrock, validate_urls, and worker_urls when
making this change so invalid grpc:// endpoints are rejected at config
validation time.
In `@model_gateway/src/routers/bedrock/errors.rs`:
- Around line 39-45: The unsupported_endpoint() function builds a Response with
a JSON body but omits the Content-Type header; update the Response::builder()
chain in unsupported_endpoint() to include a header "Content-Type" =
"application/json" (while keeping the StatusCode::NOT_IMPLEMENTED and the
existing Body::from payload) so strict clients correctly recognize the JSON
response; keep the existing unwrap_or_else fallback to
error::not_implemented("not_supported", "Unsupported endpoint").
In `@model_gateway/src/routers/bedrock/response_map.rs`:
- Around line 55-59: The current unwrap_or fallback for parsed.usage creates a
Usage with total_tokens = 0 even when input_tokens and output_tokens exist;
update the construction so that if parsed.usage is None or its total_tokens is
None you compute total_tokens = input_tokens.unwrap_or(0) +
output_tokens.unwrap_or(0) and set that into the Usage struct; specifically
adjust the code that assigns the local usage (the let usage = ... assignment
referencing parsed.usage and the Usage struct with fields
input_tokens/output_tokens/total_tokens) and make the same change at the other
occurrence (the similar block around the second instance referenced 75-77) so
total_tokens is backfilled from input_tokens + output_tokens when missing.
- Around line 49-53: The current extraction for the assistant response uses
parsed.output.as_ref().and_then(...find_map(...)).unwrap_or_default() which only
returns the first content.text and truncates multi-part message blocks; update
the logic that builds the variable text to iterate over all content entries in
parsed.output.message.content, collect all non-empty c.text values (for the
assistant role or appropriate content entries), and concatenate them (e.g., join
or fold) into a single String before using unwrap_or_default; target the code
around the variable text and the parsed.output -> .message.content traversal to
replace find_map with a collect+join/fold to preserve multi-part outputs.
In `@model_gateway/src/routers/bedrock/router.rs`:
- Around line 44-47: The code currently defaults bedrock.region to the hardcoded
"us-east-1" which can produce bad SigV4 signatures; change the logic around
bedrock.region (the region variable) to first use bedrock.region if provided,
then try environment resolution by checking AWS_REGION and then
AWS_DEFAULT_REGION, and if neither is set return an explicit configuration/error
instead of silently falling back—update the region assignment in router.rs to
perform that env lookup and return an error result (or propagate a configuraton
error) when no region can be determined so signing uses the correct Bedrock
region.
- Line 157: The timeout is hardcoded to 120s; change the code that calls
.timeout(Duration::from_secs(120)) to use the router-configured request timeout
(request_timeout_secs) instead: access the timeout value from BedrockRouter
(e.g., self.request_timeout_secs or a timeout field passed into the
BedrockRouter constructor) and replace Duration::from_secs(120) with
Duration::from_secs(request_timeout_secs as u64) (or store as Duration
up-front). Ensure the router is constructed with that timeout if not already
(pass request_timeout_secs into BedrockRouter) and use the router field in the
place of the hardcoded 120s.
In `@model_gateway/src/routers/bedrock/signing.rs`:
- Line 35: The http_client field is created with reqwest::Client::new() which
has no timeouts and can block credential resolution (ECS/IMDS); update the
initialization of http_client (the line creating "http_client:
reqwest::Client::new()") to use reqwest::Client::builder() and set sensible
timeouts (e.g., connect_timeout and overall timeout or a single timeout) so
metadata/credential calls fail fast; ensure the change is applied where the
signing client struct/constructor that owns http_client is initialized so
ECS/IMDS calls no longer hang indefinitely.
- Line 11: The code currently uses OnceCell<AwsCredentials> which permanently
caches credentials and ignores their Expiration; change the caching to store
both credentials and their expiration and refresh them when expired (e.g.
replace OnceCell<AwsCredentials> with a thread-safe container like Mutex/RwLock
or tokio::sync::RwLock holding Option<CachedCredentials { creds: AwsCredentials,
expires_at: Instant/DateTime }>), update the credential-fetching path (the
functions that read from OnceCell/AwsCredentials in signing.rs) to check
expires_at before use and asynchronously refresh/fetch new ECS/IMDS credentials
when expired, and ensure the refresh logic updates the stored CachedCredentials
atomically so SigV4 signing always uses current, non-expired credentials.
In `@model_gateway/src/routers/bedrock/tests.rs`:
- Around line 63-66: The test currently sets global AWS env vars (the lines
setting AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_REGION,
AWS_EC2_METADATA_DISABLED) which can race with other tests; fix by either
marking the specific test function in tests.rs with the serial_test::serial
attribute to force serial execution (add the attribute above the test function
that contains these env set calls) or replace global env mutation by
injecting/mocking credentials via a local/mock credential provider used by the
code under test (e.g., construct a mock AWS credential provider or use a
test-only config builder so you do not call std::env::set_var). Ensure you
remove global env mutations if you choose mocking and keep test-scoped
setup/teardown if you must mutate env.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a8ca6426-d08c-49fc-bad0-7968f175d392
📒 Files selected for processing (24)
crates/protocols/src/worker.rsdocs/getting-started/external-providers.mdmodel_gateway/Cargo.tomlmodel_gateway/src/config/builder.rsmodel_gateway/src/config/types.rsmodel_gateway/src/config/validation.rsmodel_gateway/src/main.rsmodel_gateway/src/routers/bedrock/context.rsmodel_gateway/src/routers/bedrock/errors.rsmodel_gateway/src/routers/bedrock/mod.rsmodel_gateway/src/routers/bedrock/request_map.rsmodel_gateway/src/routers/bedrock/response_map.rsmodel_gateway/src/routers/bedrock/router.rsmodel_gateway/src/routers/bedrock/signing.rsmodel_gateway/src/routers/bedrock/tests.rsmodel_gateway/src/routers/factory.rsmodel_gateway/src/routers/mod.rsmodel_gateway/src/routers/openai/provider/bedrock.rsmodel_gateway/src/routers/openai/provider/mod.rsmodel_gateway/src/routers/openai/provider/registry.rsmodel_gateway/src/routers/router_manager.rsmodel_gateway/src/server.rsmodel_gateway/src/workflow/job_queue.rsmodel_gateway/src/workflow/steps/external/discover_models.rs
MohanKumar21
force-pushed
the
ksmkumar/add-aws-routing
branch
2 times, most recently
from
a045e81 to
08e33b6
Compare
|
Hi @MohanKumar21, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e023349b9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
MohanKumar21
force-pushed
the
ksmkumar/add-aws-routing
branch
from
1e02334 to
115a676
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 115a676d55
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let router_id = match w.provider_for_model(model) { | ||
| Some(ProviderType::Gemini) => &router_ids::HTTP_GEMINI, | ||
| Some(ProviderType::Anthropic) => &router_ids::HTTP_ANTHROPIC, | ||
| Some(ProviderType::Bedrock) => &router_ids::HTTP_BEDROCK, | ||
| _ => &router_ids::HTTP_OPENAI, |
There was a problem hiding this comment.
In IGW mode this selection only looks at w.provider_for_model(model), but Bedrock workers are created in wildcard mode with no discovered models (discovery is skipped) and no default provider set, so this falls through to HTTP_OPENAI. That path cannot SigV4-sign requests, so Bedrock calls fail with auth errors unless operators manually set provider metadata for every worker. Add a fallback that can still pick HTTP_BEDROCK (for example from model/URL heuristics) when provider metadata is absent.
Useful? React with 👍 / 👎.
| let resp = self | ||
| .http_client | ||
| .get(&uri) | ||
| .send() |
There was a problem hiding this comment.
The ECS credential fetch ignores AWS_CONTAINER_AUTHORIZATION_TOKEN and AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE, so environments that expose credentials via AWS_CONTAINER_CREDENTIALS_FULL_URI with required authorization (common in EKS Pod Identity-style setups) will return 401/403 and the signer will fail to obtain credentials. This breaks Bedrock routing even though valid container credentials are configured.
Useful? React with 👍 / 👎.
MohanKumar21
force-pushed
the
ksmkumar/add-aws-routing
branch
from
115a676 to
026caf7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/getting-started/external-providers.md`:
- Around line 39-47: The example router configuration currently uses service:
bedrock but the runtime expects "bedrock-runtime"; update the YAML example under
the bedrock configuration block in docs/getting-started/external-providers.md so
the service value is "bedrock-runtime" (matching the default in
model_gateway/src/config/types.rs and the Bendrock data-plane Converse API),
keeping the same region and model_map entries.
In `@model_gateway/src/routers/bedrock/request_map.rs`:
- Around line 4-13: Remove the model_id field from the BedrockConverseRequest
struct (it must not be serialized into the request body) and update the code
that constructs BedrockConverseRequest in map_chat_request to stop populating
model_id (either drop the resolved_model parameter or stop using it when
building the struct); also remove the unit-test assertion that expects model_id
to be present. Ensure InferenceConfig/system/messages fields remain unchanged
and that the HTTP path still supplies the model id elsewhere (router code).
In `@model_gateway/src/routers/bedrock/router.rs`:
- Around line 167-176: The stream branch currently calls RequestBuilder::timeout
on req (the block where stream is true), which applies a total request deadline
and will prematurely terminate long Bedrock SSE/token streams; remove the
.timeout(request_timeout) call in the stream branch (leave the non-stream branch
as-is) or replace it with a separate, much larger or optional stream timeout
value (e.g., stream_request_timeout_secs) that is used only when stream is true,
updating references to req and the stream branch around the converse-stream
handling to ensure streaming requests do not get the ordinary
router_config.request_timeout_secs total deadline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b0eed9e-d113-4403-b5e4-105eefb332a4
📒 Files selected for processing (26)
crates/protocols/src/worker.rsdocs/getting-started/external-providers.mdmodel_gateway/Cargo.tomlmodel_gateway/src/config/builder.rsmodel_gateway/src/config/types.rsmodel_gateway/src/config/validation.rsmodel_gateway/src/main.rsmodel_gateway/src/routers/bedrock/context.rsmodel_gateway/src/routers/bedrock/converse_stream.rsmodel_gateway/src/routers/bedrock/errors.rsmodel_gateway/src/routers/bedrock/event_stream.rsmodel_gateway/src/routers/bedrock/mod.rsmodel_gateway/src/routers/bedrock/request_map.rsmodel_gateway/src/routers/bedrock/response_map.rsmodel_gateway/src/routers/bedrock/router.rsmodel_gateway/src/routers/bedrock/signing.rsmodel_gateway/src/routers/bedrock/tests.rsmodel_gateway/src/routers/factory.rsmodel_gateway/src/routers/mod.rsmodel_gateway/src/routers/openai/provider/bedrock.rsmodel_gateway/src/routers/openai/provider/mod.rsmodel_gateway/src/routers/openai/provider/registry.rsmodel_gateway/src/routers/router_manager.rsmodel_gateway/src/server.rsmodel_gateway/src/workflow/job_queue.rsmodel_gateway/src/workflow/steps/external/discover_models.rs
MohanKumar21
force-pushed
the
ksmkumar/add-aws-routing
branch
from
026caf7 to
22c8585
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
model_gateway/src/config/types.rs (1)
232-240:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCRITICAL: Default service name must be "bedrock-runtime" for data plane APIs.
The
servicefield defaults to"bedrock", but AWS Bedrock has two distinct service names for SigV4 signing:
"bedrock"→ Control plane APIs (model management, provisioning)"bedrock-runtime"→ Data plane APIs (InvokeModel, Converse, ConverseStream)This PR implements Bedrock Runtime Converse API routing (per the PR description), which is a data-plane operation. Using
"bedrock"will cause AWS SigV4 signature mismatches and result in 403 Forbidden errors for all inference requests.🔧 Proposed fix
impl Default for BedrockConfig { fn default() -> Self { Self { region: None, - service: "bedrock".to_string(), + service: "bedrock-runtime".to_string(), model_map: HashMap::new(), } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model_gateway/src/config/types.rs` around lines 232 - 240, The Default impl for BedrockConfig sets service to "bedrock" which is for control-plane APIs; change the default service string in the impl Default for BedrockConfig (the service field on BedrockConfig) to "bedrock-runtime" so SigV4 signing uses the data-plane service name (used by InvokeModel/Converse/ConverseStream) and avoids 403 errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@model_gateway/Cargo.toml`:
- Line 130: Update the pinned hmac crate version from "0.12" to the latest
stable "0.13" (e.g., change the dependency declaration hmac = "0.12" to hmac =
"0.13" or hmac = "0.13.0") so the project uses the 0.13.x release with current
security patches; ensure Cargo.lock is regenerated by running cargo update -p
hmac after changing the dependency.
In `@model_gateway/src/routers/bedrock/request_map.rs`:
- Around line 50-53: The ChatMessage::User branch currently always does
messages.push(BedrockMessage { role: "user", content:
content_blocks_from_content(content) }) which can emit an empty content array;
change it to call content_blocks_from_content(content), check the resulting
Vec/array is non-empty, and only push the BedrockMessage when that content
vector has at least one ContentBlock (otherwise skip or log). This ensures the
BedrockMessage created in the ChatMessage::User match only contains non-empty
content per Bedrock Converse API validation.
---
Duplicate comments:
In `@model_gateway/src/config/types.rs`:
- Around line 232-240: The Default impl for BedrockConfig sets service to
"bedrock" which is for control-plane APIs; change the default service string in
the impl Default for BedrockConfig (the service field on BedrockConfig) to
"bedrock-runtime" so SigV4 signing uses the data-plane service name (used by
InvokeModel/Converse/ConverseStream) and avoids 403 errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 750ffc1f-6c41-485c-92d1-f0a300b83e27
📒 Files selected for processing (13)
model_gateway/Cargo.tomlmodel_gateway/src/config/types.rsmodel_gateway/src/config/validation.rsmodel_gateway/src/routers/bedrock/converse_stream.rsmodel_gateway/src/routers/bedrock/errors.rsmodel_gateway/src/routers/bedrock/event_stream.rsmodel_gateway/src/routers/bedrock/mod.rsmodel_gateway/src/routers/bedrock/request_map.rsmodel_gateway/src/routers/bedrock/response_map.rsmodel_gateway/src/routers/bedrock/router.rsmodel_gateway/src/routers/bedrock/signing.rsmodel_gateway/src/routers/bedrock/tests.rsmodel_gateway/src/workflow/steps/external/discover_models.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22c8585550
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } else { | ||
| "converse" | ||
| }; | ||
| let endpoint = format!("{}/model/{}/{}", worker.url(), bedrock_model, stream_path); |
There was a problem hiding this comment.
Build the Converse endpoint with an encoded model ID segment instead of interpolating bedrock_model directly into the URL path. Bedrock accepts modelId as IDs or ARNs, and ARN-style IDs include / (for example inference-profile or provisioned-model ARNs); with the current format!("{}/model/{}/{}", ...), those slashes are treated as path separators, so requests target the wrong resource path and signature scope, causing auth/404 failures for valid ARN-based deployments.
Useful? React with 👍 / 👎.
MohanKumar21
force-pushed
the
ksmkumar/add-aws-routing
branch
from
22c8585 to
016fb1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
model_gateway/src/routers/bedrock/tests.rs (1)
51-54:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore mutated AWS env vars after the signer test.
This test sets process-global env vars and leaves them set, which can contaminate later tests.
Suggested patch
#[tokio::test] #[serial] async fn signer_builds_authorization_header_with_env_credentials() { @@ assert!(signed.authorization.contains("SignedHeaders=")); assert!(!signed.amz_date.is_empty()); assert_eq!(signed.payload_hash.len(), 64); + + std::env::remove_var("AWS_ACCESS_KEY_ID"); + std::env::remove_var("AWS_SECRET_ACCESS_KEY"); + std::env::remove_var("AWS_REGION"); + std::env::remove_var("AWS_EC2_METADATA_DISABLED"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model_gateway/src/routers/bedrock/tests.rs` around lines 51 - 54, The test mutates process-global AWS env vars and doesn't restore them; capture each original value with std::env::var_os for "AWS_ACCESS_KEY_ID", "AWS_SECRET_ACCESS_KEY", "AWS_REGION", and "AWS_EC2_METADATA_DISABLED" before calling std::env::set_var, then restore them after the test finishes (if original was Some -> std::env::set_var, else std::env::remove_var). For a cleaner approach, create a small RAII helper (e.g., EnvRestore) local to the test that stores the originals and restores them in Drop, and use it at the start of the signer test to guarantee restoration even on panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@model_gateway/src/config/validation.rs`:
- Around line 1282-1307: The test mutates AWS environment variables then
restores them manually which can leak on panic; wrap the save/remove/restore
logic in a RAII drop guard so env vars are always restored even if the test
panics. Create a small helper (e.g., EnvRestore) that captures saved_aws and
saved_default in its constructor, removes the vars, and restores them in Drop,
then replace the manual save/remove/restore block in the test around the
RouterConfig/ConfigValidator::validate call with this guard.
In `@model_gateway/src/routers/bedrock/converse_stream.rs`:
- Around line 87-91: The closure `num` is converting negative i64 token counts
into huge u64 values; update `num` to first try `.get(k).and_then(|v|
v.as_u64()).or_else(|| v.as_i64().and_then(|i| if i >= 0 { Some(i as u64) } else
{ None }))` (or equivalent) so negative i64 values are treated as absent/zero
instead of being cast to a large unsigned value; ensure the final
`.unwrap_or(0)` remains to return 0 for negatives or missing keys and locate
this change in the `num` closure that reads from `usage`.
In `@model_gateway/src/routers/bedrock/request_map.rs`:
- Around line 149-156: The function content_blocks_from_content currently treats
whitespace-only strings as valid; change it to reject them by trimming the text
before the emptiness check and using the trimmed value when constructing the
ContentBlock. Concretely, in content_blocks_from_content, call
content.to_simple_string(), let trimmed = text.trim(), return Vec::new() if
trimmed.is_empty(), otherwise create vec![ContentBlock { text:
trimmed.to_string() }]; this ensures ContentBlock.text is not empty or
whitespace-only and avoids Bedrock ValidationException.
In `@model_gateway/src/routers/bedrock/router.rs`:
- Around line 132-136: The endpoint is built using bedrock_model unescaped which
breaks Url::parse when model IDs contain reserved chars; update the endpoint
construction (the variable named endpoint built from worker.url(),
bedrock_model, and stream_path) to percent-encode bedrock_model (e.g., use
urlencoding::encode or equivalent) before formatting so the modelId remains a
single path segment, then pass that encoded string into the existing Url::parse
call.
In `@model_gateway/src/routers/bedrock/signing.rs`:
- Around line 219-233: The ECS credential loader load_ecs_credentials currently
issues unauthenticated GETs; modify it to read AWS_CONTAINER_AUTHORIZATION_TOKEN
or AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE when
AWS_CONTAINER_CREDENTIALS_FULL_URI is present and, if a token is found, add an
Authorization header ("Authorization: <token>") to the http_client GET request
before send(), handling file read errors and propagating them as formatted Err
strings; ensure the header is only attached when a token exists and preserve
existing behavior for relative URI resolution and returning Ok(None) when no URI
is configured.
---
Duplicate comments:
In `@model_gateway/src/routers/bedrock/tests.rs`:
- Around line 51-54: The test mutates process-global AWS env vars and doesn't
restore them; capture each original value with std::env::var_os for
"AWS_ACCESS_KEY_ID", "AWS_SECRET_ACCESS_KEY", "AWS_REGION", and
"AWS_EC2_METADATA_DISABLED" before calling std::env::set_var, then restore them
after the test finishes (if original was Some -> std::env::set_var, else
std::env::remove_var). For a cleaner approach, create a small RAII helper (e.g.,
EnvRestore) local to the test that stores the originals and restores them in
Drop, and use it at the start of the signer test to guarantee restoration even
on panic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 51e95eeb-b850-4a71-aaab-059de188d727
📒 Files selected for processing (15)
docs/getting-started/external-providers.mdmodel_gateway/Cargo.tomlmodel_gateway/src/config/types.rsmodel_gateway/src/config/validation.rsmodel_gateway/src/routers/bedrock/converse_stream.rsmodel_gateway/src/routers/bedrock/errors.rsmodel_gateway/src/routers/bedrock/event_stream.rsmodel_gateway/src/routers/bedrock/mod.rsmodel_gateway/src/routers/bedrock/request_map.rsmodel_gateway/src/routers/bedrock/response_map.rsmodel_gateway/src/routers/bedrock/router.rsmodel_gateway/src/routers/bedrock/signing.rsmodel_gateway/src/routers/bedrock/tests.rsmodel_gateway/src/routers/skills/handlers.rsmodel_gateway/src/workflow/steps/external/discover_models.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 016fb1b6a4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
MohanKumar21
force-pushed
the
ksmkumar/add-aws-routing
branch
from
016fb1b to
93e9d08
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93e9d0861e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6eff1b68d4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
MohanKumar21
force-pushed
the
ksmkumar/add-aws-routing
branch
2 times, most recently
from
93e9d08 to
0fcd401
Compare
Signed-off-by: MohanKumar21! <mohanmrm20@gmail.com>
Signed-off-by: MohanKumar21! <mohanmrm20@gmail.com>
MohanKumar21
force-pushed
the
ksmkumar/add-aws-routing
branch
from
0fcd401 to
5285682
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 528568200b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let mut text = content.to_simple_string(); | ||
| if !tool_call_id.is_empty() { | ||
| text = format!("tool_result {tool_call_id}: {text}"); | ||
| } |
There was a problem hiding this comment.
ChatMessage::Tool is converted into a plain text block ("tool_result {id}: ...") instead of a Bedrock toolResult content block carrying toolUseId. In multi-turn tool-calling flows, Bedrock uses toolUseId to link the tool output to the prior toolUse; flattening it to text breaks that linkage and can cause the model to ignore or mis-handle tool outputs on follow-up turns.
Useful? React with 👍 / 👎.
| let text = content.to_simple_string(); | ||
| let trimmed = text.trim(); | ||
| if trimmed.is_empty() { | ||
| Vec::new() | ||
| } else { | ||
| vec
Description
Problem
SMG had no first-class path to Amazon Bedrock Runtime. Operators who wanted Bedrock-backed models had to work around the gateway instead of using a dedicated routing mode with correct AWS SigV4 auth and OpenAI-compatible chat mapping.
Solution
This PR adds Bedrock as an HTTP-only routing mode (RoutingMode::Bedrock, --backend bedrock), a dedicated Bedrock router that calls Bedrock’s Converse API, maps OpenAI-style chat requests/responses (including basic tool message handling), and signs outbound requests with a minimal SigV4 implementation plus a credential chain (environment variables, shared credentials file, ECS container credentials, EC2 IMDS) so the gateway does not depend on the full AWS SDK (avoiding rustc/toolchain friction). ProviderType::Bedrock and registry/IGW wiring ensure model-based routing and discovery heuristics stay consistent with other external providers.
Changes
BedrockConfigonRouterConfig(region, service,model_map);bedrock_modeon builder; validation (non-empty service/region when set; no service discovery in Bedrock mode).Backend::Bedrock, worker URL aggregation, startup banner.model_gateway::routers::bedrockmodule (request map, response map, errors, signing, router, tests); factory +HTTP_BEDROCKrouter id; gRPC mode rejected for Bedrock.ProviderType::Bedrockinprotocols;BedrockProviderin OpenAI provider registry;job_queue/serverreadiness /discover_modelsBedrock branches.hmac,hexonmodel_gatewayfor SigV4.docs/getting-started/external-providers.md.Test Plan
cargo test -p smg bedrock -- --nocaptureCovers request JSON shape, non-stream and pseudo-stream SSE mapping, canonical URI encoding for model IDs with :, and signer producing a non-empty Authorization / X-Amz-Date / payload hash when env credentials are set (test sets dummy keys and disables IMDS).
cargo run -p smg --bin smg -- launch \ --backend bedrock \ --worker-urls "https://bedrock-runtime.us-east-1.amazonaws.com" \ --port 30000Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
New Features
Documentation
Tests