-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add OIDC to bedrock httpx #3687
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
) = params_to_check | ||
|
||
### CHECK STS ### | ||
if aws_role_name is not None and aws_session_name is not None: | ||
if aws_web_identity_token is not None and aws_role_name is not None and aws_session_name is not None: | ||
oidc_token = get_secret(aws_web_identity_token) |
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.
@Manouchehri these would also be sync http requests for async calls. I view fixing this - #3858 (comment) as a precursor to this PR.
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.
How bad is having sync http requests here? Is it mitigated by #3712?
Doing the STS calls by hand is gonna be a bit of a pain, since I would have to calculate the signatures by hand.
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.
what do you mean. Your calling 'get_secret' inside of which you use HTTPHandler
The request is to use AsyncHTTPHandler instead.
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.
get_secret
does not do a HTTP request if you're accessing Bedrock via Circle CI or GitHub.
I can make get_secret
async, but that's unrelated to this PR and I should make a separate one for it. We have two OIDC parts:
- Getting the OIDC token (which uses HTTPHandler atm)
- Using the OIDC token (which uses boto3 for Bedrock) <- this PR only modifies this code
@Manouchehri appreciate the work on this but we do need some test for this so we don't introduce a regression later on. You can use an mockmagic, to mock the flows, but just assert the call / flow is working as expected. Examples with mockmagic - litellm/litellm/tests/test_alerting.py Line 440 in a6a84e5
Place to add the test - https://github.com/BerriAI/litellm/blob/main/litellm/tests/test_bedrock_completion.py |
The proper unit test is in #3688. |
should these just be 1 pr then? @Manouchehri |
They could be. #3688 will fail unit tests until #3578 (comment) is addressed. |
@Manouchehri can we not add tests that will fail ci/cd? If we can just have the response mocked, and use that, it should be sufficient - unless you expect the circleci api format to change? |
I think doing the actual API call is about as much work as faking it, and I'm worried that something in the OIDC flow, either on AWS or Circle CI, will change without us realizing it. |
Closing in favour of #3688. |
Title
OIDC Support for Bedrock HTTPX
Relevant issues
Resolves #3674
Type
🆕 New Feature
🐛 Bug Fix
Changes
This adds OIDC support to the bedrock httpx client.
[REQUIRED] Testing - Attach a screenshot of any new tests passing locally
I don't have any AWS_SECRET_ACCESS_KEY set in my deployment, so the only way this can work is if OIDC is working. :)