-
Notifications
You must be signed in to change notification settings - Fork 69
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
Allow dynamic authorization headers for OpenAI + Azure OpenAI #646
Allow dynamic authorization headers for OpenAI + Azure OpenAI #646
Conversation
@@ -526,6 +545,11 @@ public Builder azureAdToken(String azureAdToken) { | |||
return this; | |||
} | |||
|
|||
public Builder configName(String modelName) { |
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.
We should probably stick with either configName
or modelName
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.
nice catch. I would prefer sticking with configName since modelName is taken by something else during the setup of the ImageModel.
authorizer = beanInstance | ||
.handlesStream() | ||
.findFirst() | ||
.map(Instance.Handle::get) | ||
.orElse(null); |
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.
I would prefer this be done without streams
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.
I like this a lot!
I made some small comments.
We should also add some documentation about the feature
@csotiriou By the way, is #634 relevant ? It you don't use any of the quarkus extensions related to oidc or jwt then it won't be relevant, otherwise it should be... In #634 I'd like to verify that OIDC authorization code flow token can be used, but if the OIDC bearer tokens are already coming in then #634 should take care of them, as long as |
FYI, I'm not suggesting in anyway that #634 might be a better solution, as I can imagine these tokens may be coming via different channels altogether, so having a dedicated dynamic authorization support is nice on its own. |
Let's figure it out |
@sberyozkin @geoand I just saw #634, I hadn't noticed it! My apologies. My PR relies in also setting up filters for the OpenAI client. However, it allows for more customization options (more generic implementation) which for some people with very strict security rules and diverse ecosystems is necessary. My PR also allows customization on a per-config-name basis, which is also a requirement for me (and I suspect others in the future combining models and installations). My suggestion would be to have the mechanism in my PR as the basis for the flexibility of customization it offers. #634 could then use this mechanism to provide an additional module based on the mechanism of this PR. Further explanation(examples of use cases) My use case is even more complex, relying on connecting IAM worlds from two cloud providers to actually retrieve a short lived token - falling well outside of the OIDC libraries scope. Depending on the enterprise security guidelines of some major companies, mechanisms such as this are not unheard of. I believe the above applies to all major cloud providers, it's just that I am able to test (and am familiar with) the Azure internals nowadays... |
@csotiriou Thanks for the nice explanation, makes total sense to have a generic dynamic mechanism in place to support various dynamic token inputs, beyond the typical standard ones. I was thinking a bit if such filters should also work for other providers, like As far as #634 is concerned, I hope, if that is proven to work and agreed it makes sense, that it will be free for users, i.e, they won't have to write and register custom dynamic filter. I'll CC you there as well to discuss things related to that PR further Thanks |
I made a rename there, and added some documentation in the OpenAI section. I'm currently contemplating adding more power to the developer by changing the interface AuthProvider {
String getAuthorization();
} to interface AuthProvider {
String getAuthorization(String method, URI url, MultivaluedMap<String, String> headers);
} Haven't decided yet, just wanted to ask. The thought is that in the scope of the named model one may want to differentiate the authentication behaviour per deployment or by things passed as headers. The deployment name is not passed in this context but it is inferrable via the request URI. Not sure about this, yet, though, what do you think? |
75b7d86
to
de43ea5
Compare
I am +1 for adding the more "powerfull" version, however I would write it like this: interface AuthProvider {
String getAuthorization(Input input);
interface Input {
String getMethod();
...
}
} This way we can add stuff in the future without breaking the method signature |
eeb7812
to
af06a08
Compare
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.
Awesome!
I just added one last comment
...nai/openai-common/runtime/src/main/java/io/quarkiverse/langchain4j/openai/OpenAiRestApi.java
Outdated
Show resolved
Hide resolved
af06a08
to
78cd849
Compare
fixing PR comments added documentation and renamed Interface to AuthProvider changed signature of of the "getAuthorization" method changed signature of the getAuthorization method made the Authorizer beans unremovable updated documentation used ModelName instead of Named annotation made Input an interface and separated instantiation in a private class fixed formatting
78cd849
to
30715fc
Compare
This allows configuring the Azure OpenAI authorization headers on the fly, by instantiating named beans and implementing an interface.
The concept is that the developer can create a bean like this:
One can create beans on a per-configuration scenario using the
@Named
annotation.Use case
This will allow using Azure OpenAI integration in complex environments where IAM policies and short-lived Azure tokens are used. In such scenarios, the tokens need to be refreshed periodically using custom security policies, thereby needing such solutions.