-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat (provider/azure): add support for azure managed identity. #3911
base: main
Are you sure you want to change the base?
Conversation
0294700
to
4804e03
Compare
Thanks for the PR. I would prefer a different design for passing in auth that's more extensible. cc @shaper ideally this would align with the design we end up with for google vertex so it's similar across providers |
Thanks for the feedback, I didn't realize there were plans to implement token based auth for google vertex as well. One idea that comes to mind is setting up a type-based approach for providers that have multiple auth methods, something like type AuthenticationMethod =
| { type: 'apiKey'; value: string }
| { type: 'bearer'; tokenProvider: () => Promise<string> }
| { type: 'custom'; getAuth: () => Promise<Record<string, string>> }; You could still have |
Hi, thanks for the contribution, I will take a closer look at it! A question: all of the above seem effectively implementable if we expose a way to optionally add to request headers. We have one today via That has a nice feel from a simplicity of API surface perspective as it's one thing to think about, though it's also admittedly a little raw, as not all devs or docs want to think about or deal with talking about headers vs. the auth primitives/methods. A concern could be any performance impact/overhead from calling the dev-provided headers population (do we do it once and cache it, or on each request, or catch auth errors and try refreshing by calling it again, ...). I expect we already have this auth-lifetime issue in some cases, and I wonder whether folks are having to catch auth timeout/failure and re-create the provider with updated auth in some cases. Stepping back -- today I theorize you actually can already do what you want by calling your identity token provider before you create the Azure provider, getting your token, and then using the existing It would be great to hear your thoughts on the above. |
@shaper Those are great points and you nailed the main challenges here. I actually thought about using Initially, I had token auth working by setting it up in the You can achieve this with what's available in the SDK right now, but it's not as intuitive as other libraries, and like you said, "a little raw". I was hoping to help bridge that gap, but I agree that the docs could address this just as well. I’ve actually seen a few comments from people saying they can’t use the SDK because of its missing native token-based auth support for their cloud provider. I’ve also heard similar feedback in enterprise settings. So, even if this PR doesn’t move forward, adding an example in the docs for token-based auth would go a long way toward helping folks out. I’m happy to tweak this further or help with the docs if that’s the way to go |
Hi @shaper I saw the PRs for the Resolvable utility type and Vertex AI auth changed get merged. I'll get to work on changing this PR to use those standards. I saw that the Vertex auth implementation used the I can support both implementations, let me know which sounds better. |
The easier approach and keeping the provider lighter weight sounds better to me. If you put together a draft PR we'd be happy to look at it and discuss, that would be great, thanks! There were two reasons we kept the google-auth-library for vertex:
The last point is a down side of the simpler approach. I think you'll end up with something that's like at least 4 lines or so wherever someone creates a provider instance. Let's see what it looks like when you try it? My only other idea, if we want to pursue the goal of lighter-weight providers, is to have a separate contrib-type package where small hooks like this can live and folks could leverage that way, and we can push heavyweight deps to those other more specific packages that depend on the provider. I am not saying we should do this yet, it's just the only thing that comes to mind. Maybe you'll find something else syntactic-sugar-wise etc. |
Fixes: #3124
Similar to my last PR, #3463 but updated to v4 and cleaned up the code. Had to close the previous PR as it was too far behind and easier to start over than rebase.
Made it a fetch wrapper to grab a new token on new requests, similar to how OpenAI package does it.
Please let me know what else I can add or change.