Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

Manouchehri
Copy link
Collaborator

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. :)

image
model_list:
  - model_name: command-r
    litellm_params:
      model: bedrock/cohere.command-r-v1:0
      aws_region_name: us-east-1
      max_tokens: 4000
      seed: 1337
      aws_session_name: "test"
      aws_role_name: "arn:aws:iam::REMOVED:role/litellm-demo-example-run"
      aws_web_identity_token: "oidc/google/https://example.com"

Copy link

vercel bot commented May 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
litellm ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 16, 2024 7:27pm

) = 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)
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

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.

Copy link
Collaborator Author

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:

  1. Getting the OIDC token (which uses HTTPHandler atm)
  2. Using the OIDC token (which uses boto3 for Bedrock) <- this PR only modifies this code

@krrishdholakia
Copy link
Contributor

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.

@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 -

async def test_send_token_budget_crossed_alerts(alerting_type):

Place to add the test - https://github.com/BerriAI/litellm/blob/main/litellm/tests/test_bedrock_completion.py

@Manouchehri
Copy link
Collaborator Author

The proper unit test is in #3688.

@krrishdholakia
Copy link
Contributor

should these just be 1 pr then? @Manouchehri

@Manouchehri
Copy link
Collaborator Author

They could be. #3688 will fail unit tests until #3578 (comment) is addressed.

@krrishdholakia
Copy link
Contributor

@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?

@Manouchehri
Copy link
Collaborator Author

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.

@Manouchehri
Copy link
Collaborator Author

Closing in favour of #3688.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Support upstream OIDC in bedrock_httpx.py
2 participants