refactor(bedrock-mantle): align SigV4 signing service name with canonical "bedrock-mantle" - #31476
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1827b04 to
d1520db
Compare
|
@greptileai review |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Re: Greptile review requirement I requested a Greptile review (see Per the repo's own triage logic, a missing Greptile score is not a merge blocker (the automation treats |
|
I think should go with @mateo-berri 's work in #30714 |
|
@greptileai review |
Greptile SummaryThis PR aligns the SigV4 signing service name for all Bedrock Mantle request paths from the legacy
Confidence Score: 5/5Safe to merge — the AWS endpoint accepts both the old and new service names, so no existing users are broken, and the new name is the one the endpoint itself declares as canonical. All four signing paths are updated in lock-step, the Literal type guard prevents typos at static-analysis time, and the tests include a real botocore SigV4 path (offline, no network) that would catch any regression in the credential-scope string. The large formatting diff in base_aws_llm.py is cosmetic and carries no logic risk. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/llms/bedrock/base_aws_llm.py | Adds "bedrock-mantle" to the Literal type for _sign_request's service_name parameter; corrects the docstring return type from Optional[str] to Optional[bytes]. Remaining diff is pure formatting with no logic changes. |
| litellm/llms/bedrock/chat/mantle/transformation.py | Adds sign_request override to AmazonMantleConfig that hardwires service_name="bedrock-mantle" when delegating to _sign_request. Change is correct and properly narrows the signing scope from the parent's default. |
| litellm/llms/bedrock/messages/mantle_transformation.py | Mirror of the chat transformation change — adds sign_request override to AmazonMantleMessagesConfig with service_name="bedrock-mantle". Symmetric and correct. |
| litellm/llms/bedrock_mantle/common_utils.py | Core signing change: BedrockMantleAuthMixin.sign_request switches service_name from "bedrock" to "bedrock-mantle". This is the shared auth path used by both the chat and responses backends. |
| litellm/llms/bedrock_mantle/chat/transformation.py | Docstring updated from service "bedrock" to "bedrock-mantle"; minor formatting cleanup. No logic changes. |
| litellm/llms/bedrock_mantle/responses/transformation.py | Docstring updated from service "bedrock" to "bedrock-mantle"; minor formatting cleanup. No logic changes. |
| tests/test_litellm/llms/bedrock/test_mantle.py | Existing credential-scope assertions updated from /bedrock/ to /bedrock-mantle/. Three new tests added including one real botocore SigV4 test using offline fake credentials. Test additions strengthen coverage without introducing real network calls. |
| tests/test_litellm/llms/bedrock_mantle/test_bedrock_mantle_responses_transformation.py | Five credential-scope assertions updated from /bedrock/ to /bedrock-mantle/. Remaining diff is formatting cleanup. Assertion updates are consistent with the production change and do not weaken coverage. |
| tests/test_litellm/llms/bedrock_mantle/test_bedrock_mantle_transformation.py | Five credential-scope assertions updated from /bedrock/ to /bedrock-mantle/. Remaining diff is formatting cleanup. Changes correctly track the production change. |
Reviews (2): Last reviewed commit: "style: apply ruff format to fix lint che..." | Re-trigger Greptile
|
@laiweihwa thanks for digging into this. Before this lands I wanted to verify the premise against the live endpoint, because I don't think the current I ran the full SigV4 service-name matrix against
The 401 in the description was produced by signing with So this reads as a safe naming cleanup rather than a fix for a broken path. Signing as One scenario these tests don't cover: a role scoped strictly to |
…ical "bedrock-mantle" The bedrock-mantle endpoint declares "bedrock-mantle" as its canonical SigV4 service name (signing with "mantle" returns HTTP 401 "Credential should be scoped to correct service: 'bedrock-mantle'"). However, the endpoint also accepts "bedrock" in the credential scope (both return HTTP 200 for real inference), so the previous signing was not broken. This change aligns the SigV4 credential scope with the canonical service name and the bedrock-mantle IAM namespace (bedrock-mantle:*, per the AWS-managed AmazonBedrockMantle*Access policies). Verified with a role scoped to only AmazonBedrockMantleInferenceAccess: both service="bedrock" and service="bedrock-mantle" produce HTTP 200 completions, confirming this is a naming alignment rather than a fix for broken auth. Changes: - Add "bedrock-mantle" to the allowed service Literal in BaseAWSLLM._sign_request. - Sign with service_name="bedrock-mantle" on all three signing surfaces: - AmazonMantleConfig.sign_request (bedrock/mantle/ chat route) - AmazonMantleMessagesConfig.sign_request (bedrock/mantle/ messages route) - BedrockMantleAuthMixin.sign_request (bedrock_mantle/ standalone provider) - Correct stale docstrings that still named the SigV4 service "bedrock" (bedrock_mantle/common_utils.py, bedrock_mantle/chat/transformation.py) and fix the _sign_request docstring return type to Tuple[dict, Optional[bytes]]. - Use the builtin tuple return annotation on the mantle sign_request overrides to satisfy the strict ruff UP006 gate. - Update existing test assertions to /bedrock-mantle/aws4_request and add a real-signature SigV4 test (no _sign_request mock) asserting both bedrock/mantle/ configs scope the credential to bedrock-mantle. Related: BerriAI#31475, BerriAI#31113, BerriAI#31196, BerriAI#30714
d1520db to
038fbd3
Compare
|
Thanks for the review, @6matt .You're right -- I ran the decisive test you proposed and it confirms your assessment. I created a role with only Both I've reframed the PR as a naming alignment (commit prefix changed from The earlier "bug fix" framing was based on a flawed probe methodology -- we tested only empty-body requests (to avoid billing) and over-interpreted the 401 error message as exclusionary rather than canonical. Your full-inference test exposed the gap. Thanks for the diligence. |
|
should be good to go for a re-review when you have a moment, @6matt |
|
@greptileai review |
Relevant issues
Related: #31475, #31113, #31196, #30714
Type
Refactor / Naming Alignment
Changes
The
bedrock-mantleendpoint (bedrock-mantle.{region}.api.aws) declaresbedrock-mantleas its canonical SigV4 signing service name. However, the endpoint's credential-scope authorizer also acceptsbedrock(the legacy name used bybedrock-runtime), so the previous signing was not broken -- both return HTTP 200 for real inference. This is a naming alignment, not a bug fix.This PR aligns all Bedrock Mantle signing paths with the canonical service name
"bedrock-mantle"to match the service's own IAM namespace and the naming convention used by the AWS-managedAmazonBedrockMantle*Accesspolicies.Evidence: this is cosmetic, not a fix
Tested with a role scoped to only
AmazonBedrockMantleInferenceAccess(bedrock-mantle:*actions) -- the narrowest possible permission set:Both
bedrockandbedrock-mantleauthenticate successfully, even with restricted IAM. The endpoint rejects only genuinely unrecognized names likemantle. The credential-scope service name affects signature verification routing, not IAM action authorization.Why align anyway?
bedrock-mantleis the canonical name declared by the endpoint itself (the 401 message names it)bedrock-mantle:*) and ARN namespace (arn:aws:bedrock-mantle:*)bedrock-agentcore(owns its IAM namespace, signs as itself)bedrock, this code is already correctCode changes
litellm/llms/bedrock/base_aws_llm.py-- add"bedrock-mantle"to theLiteralon_sign_request(and correct the docstring return type toOptional[bytes]).litellm/llms/bedrock/chat/mantle/transformation.py--AmazonMantleConfig.sign_requestoverride signing withbedrock-mantle(thebedrock/mantle/chat route).litellm/llms/bedrock/messages/mantle_transformation.py--AmazonMantleMessagesConfig.sign_requestoverride signing withbedrock-mantle(thebedrock/mantle/messages route).litellm/llms/bedrock_mantle/common_utils.py--BedrockMantleAuthMixin.sign_requestsigns withbedrock-mantle(the standalonebedrock_mantle/provider, shared by chat + responses).bedrock_mantle/module updated to saybedrock-mantle./bedrock/aws4_requestto/bedrock-mantle/aws4_request; added unit tests asserting the signing service, plus a real-signature test asserting the actual credential scope for bothbedrock/mantle/configs.Full diff
Pre-Submission checklist
Screenshots / Proof of Fix
All bedrock-mantle unit tests pass and the ruff strict gate is green: