Concerns with Connector Classes: Improper Use of HttpClient and the Risk of Port/Socket Exhaustion #527
Assignees
Comments
|
hi @alexandrereyes thanks for looking into this, yes we're aware of the issue and we plan on start using IHttpClientFactory shortly. The work fits into a bigger story that covers refactoring the early prototype, introducing ServiceCollection and the usual .NET DI, which will then allow to support this easily. |
adrianwyatt
pushed a commit
that referenced
this issue
May 1, 2023
### Motivation and Context My company network needs more custom set up in HttpClient to make calls, but SK OpenAI code only allows me to pass a hander delegate factory. I cannot use SK without more settings in HttpClient and I want to pass in my own instances anyway so I can manage them from my application. I think I am not the only one to need this: - #527 - #133 See https://learn.microsoft.com/en-us/dotnet/fundamentals/networking/http/httpclient-guidelines. ### Description Changes included: - The OpenAI and AzureOpenAI connectors take optional HttpClient? in the constructor methods instead of IDelegatingHandlerFactory - Note first approach was adding two new constructor methods to each type so you could choose. But two constructors could not be there together with optional HttpClient? in one and optional IDelegatingHandlerFactory? in the other because it made calls ambiguous. Then I tried making HttpClient not optional, but that made the parameter order different because in some there are also other optional parameters like openai `string? organization = null`. So the not optional HttpClient would have to come BEFORE optional openai organization in one constructor, but the optional IDelegatingHandlerFactory comes AFTER optional openai organization in another constructor. In the end it seemed better to just take HttpClient and do the custom client creation in the extension methods. I hope this is okay I know it is a bigger change than I planned. - IDelegatingHandlerFactory is not being used as a factory in these Open AI connector classes anyway. They use it one to create an HttpClient instance in the constructor method anyway and never call it again so it's just as easy to pass in the HttpClient you want. - Updated the extension methods in `KernelConfigOpenAIExtensions` so that they can optionally pass in a custom HttpClient and ILogger. If null then it uses the same defaults as before. - There was some mix use of `Logger log` and `ILogger logger` in different places so on the parts I changed I set these all to "logger" because it makes it more clear that it is a object and not a "log" method. I can undo if you don't like this. - Changed "credentials" to "credential" to match this is one `TokenCredenial` not a credential collection. I can undo if you don't like this. - Made the default HttpClientHandler static and shared for new HttpClients with `disposeHandler: false` dotnet/runtime#16255 - Updated a sample Program - I can add more tests if required ### Contribution Checklist <!-- Before submitting this PR, please make sure: --> - [X] The code builds clean without any errors or warnings - [X] The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) - [X] The code follows the .NET coding conventions (https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) verified with `dotnet format` - [X] All unit tests pass, and I have added new tests where possible - Most tests pass but it skips some because I don't have keys for all these model types so I hope your github build will run this and double check. - [ ] I didn't break anyone 😄 - I changed the constructor method args in the OpenAI code, and the will break your users that are creating these open ai classes using their constructors directly. If the users are using the extension methods (AddAzureOpenAI) then they should not have problems. - If you want I can put back all of the constructor methods so they can be created with HttpClient or IDelegatingHandlerFactory but with parameters orders different as explained - I don't think those classes need to be aware of IDelegatingHandlerFactory or have it included in the constructor methods, since the extension method or calling code can always convert to HttpClient before calling.
awharrison-28
pushed a commit
to awharrison-28/semantic-kernel
that referenced
this issue
May 1, 2023
### Motivation and Context My company network needs more custom set up in HttpClient to make calls, but SK OpenAI code only allows me to pass a hander delegate factory. I cannot use SK without more settings in HttpClient and I want to pass in my own instances anyway so I can manage them from my application. I think I am not the only one to need this: - microsoft#527 - microsoft#133 See https://learn.microsoft.com/en-us/dotnet/fundamentals/networking/http/httpclient-guidelines. ### Description Changes included: - The OpenAI and AzureOpenAI connectors take optional HttpClient? in the constructor methods instead of IDelegatingHandlerFactory - Note first approach was adding two new constructor methods to each type so you could choose. But two constructors could not be there together with optional HttpClient? in one and optional IDelegatingHandlerFactory? in the other because it made calls ambiguous. Then I tried making HttpClient not optional, but that made the parameter order different because in some there are also other optional parameters like openai `string? organization = null`. So the not optional HttpClient would have to come BEFORE optional openai organization in one constructor, but the optional IDelegatingHandlerFactory comes AFTER optional openai organization in another constructor. In the end it seemed better to just take HttpClient and do the custom client creation in the extension methods. I hope this is okay I know it is a bigger change than I planned. - IDelegatingHandlerFactory is not being used as a factory in these Open AI connector classes anyway. They use it one to create an HttpClient instance in the constructor method anyway and never call it again so it's just as easy to pass in the HttpClient you want. - Updated the extension methods in `KernelConfigOpenAIExtensions` so that they can optionally pass in a custom HttpClient and ILogger. If null then it uses the same defaults as before. - There was some mix use of `Logger log` and `ILogger logger` in different places so on the parts I changed I set these all to "logger" because it makes it more clear that it is a object and not a "log" method. I can undo if you don't like this. - Changed "credentials" to "credential" to match this is one `TokenCredenial` not a credential collection. I can undo if you don't like this. - Made the default HttpClientHandler static and shared for new HttpClients with `disposeHandler: false` dotnet/runtime#16255 - Updated a sample Program - I can add more tests if required ### Contribution Checklist <!-- Before submitting this PR, please make sure: --> - [X] The code builds clean without any errors or warnings - [X] The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) - [X] The code follows the .NET coding conventions (https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) verified with `dotnet format` - [X] All unit tests pass, and I have added new tests where possible - Most tests pass but it skips some because I don't have keys for all these model types so I hope your github build will run this and double check. - [ ] I didn't break anyone 😄 - I changed the constructor method args in the OpenAI code, and the will break your users that are creating these open ai classes using their constructors directly. If the users are using the extension methods (AddAzureOpenAI) then they should not have problems. - If you want I can put back all of the constructor methods so they can be created with HttpClient or IDelegatingHandlerFactory but with parameters orders different as explained - I don't think those classes need to be aware of IDelegatingHandlerFactory or have it included in the constructor methods, since the extension method or calling code can always convert to HttpClient before calling.
codebrain
pushed a commit
to searchpioneer/semantic-kernel
that referenced
this issue
May 16, 2023
### Motivation and Context My company network needs more custom set up in HttpClient to make calls, but SK OpenAI code only allows me to pass a hander delegate factory. I cannot use SK without more settings in HttpClient and I want to pass in my own instances anyway so I can manage them from my application. I think I am not the only one to need this: - microsoft#527 - microsoft#133 See https://learn.microsoft.com/en-us/dotnet/fundamentals/networking/http/httpclient-guidelines. ### Description Changes included: - The OpenAI and AzureOpenAI connectors take optional HttpClient? in the constructor methods instead of IDelegatingHandlerFactory - Note first approach was adding two new constructor methods to each type so you could choose. But two constructors could not be there together with optional HttpClient? in one and optional IDelegatingHandlerFactory? in the other because it made calls ambiguous. Then I tried making HttpClient not optional, but that made the parameter order different because in some there are also other optional parameters like openai `string? organization = null`. So the not optional HttpClient would have to come BEFORE optional openai organization in one constructor, but the optional IDelegatingHandlerFactory comes AFTER optional openai organization in another constructor. In the end it seemed better to just take HttpClient and do the custom client creation in the extension methods. I hope this is okay I know it is a bigger change than I planned. - IDelegatingHandlerFactory is not being used as a factory in these Open AI connector classes anyway. They use it one to create an HttpClient instance in the constructor method anyway and never call it again so it's just as easy to pass in the HttpClient you want. - Updated the extension methods in `KernelConfigOpenAIExtensions` so that they can optionally pass in a custom HttpClient and ILogger. If null then it uses the same defaults as before. - There was some mix use of `Logger log` and `ILogger logger` in different places so on the parts I changed I set these all to "logger" because it makes it more clear that it is a object and not a "log" method. I can undo if you don't like this. - Changed "credentials" to "credential" to match this is one `TokenCredenial` not a credential collection. I can undo if you don't like this. - Made the default HttpClientHandler static and shared for new HttpClients with `disposeHandler: false` dotnet/runtime#16255 - Updated a sample Program - I can add more tests if required ### Contribution Checklist <!-- Before submitting this PR, please make sure: --> - [X] The code builds clean without any errors or warnings - [X] The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) - [X] The code follows the .NET coding conventions (https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) verified with `dotnet format` - [X] All unit tests pass, and I have added new tests where possible - Most tests pass but it skips some because I don't have keys for all these model types so I hope your github build will run this and double check. - [ ] I didn't break anyone 😄 - I changed the constructor method args in the OpenAI code, and the will break your users that are creating these open ai classes using their constructors directly. If the users are using the extension methods (AddAzureOpenAI) then they should not have problems. - If you want I can put back all of the constructor methods so they can be created with HttpClient or IDelegatingHandlerFactory but with parameters orders different as explained - I don't think those classes need to be aware of IDelegatingHandlerFactory or have it included in the constructor methods, since the extension method or calling code can always convert to HttpClient before calling.
dehoward
pushed a commit
to lemillermicrosoft/semantic-kernel
that referenced
this issue
Jun 1, 2023
### Motivation and Context My company network needs more custom set up in HttpClient to make calls, but SK OpenAI code only allows me to pass a hander delegate factory. I cannot use SK without more settings in HttpClient and I want to pass in my own instances anyway so I can manage them from my application. I think I am not the only one to need this: - microsoft#527 - microsoft#133 See https://learn.microsoft.com/en-us/dotnet/fundamentals/networking/http/httpclient-guidelines. ### Description Changes included: - The OpenAI and AzureOpenAI connectors take optional HttpClient? in the constructor methods instead of IDelegatingHandlerFactory - Note first approach was adding two new constructor methods to each type so you could choose. But two constructors could not be there together with optional HttpClient? in one and optional IDelegatingHandlerFactory? in the other because it made calls ambiguous. Then I tried making HttpClient not optional, but that made the parameter order different because in some there are also other optional parameters like openai `string? organization = null`. So the not optional HttpClient would have to come BEFORE optional openai organization in one constructor, but the optional IDelegatingHandlerFactory comes AFTER optional openai organization in another constructor. In the end it seemed better to just take HttpClient and do the custom client creation in the extension methods. I hope this is okay I know it is a bigger change than I planned. - IDelegatingHandlerFactory is not being used as a factory in these Open AI connector classes anyway. They use it one to create an HttpClient instance in the constructor method anyway and never call it again so it's just as easy to pass in the HttpClient you want. - Updated the extension methods in `KernelConfigOpenAIExtensions` so that they can optionally pass in a custom HttpClient and ILogger. If null then it uses the same defaults as before. - There was some mix use of `Logger log` and `ILogger logger` in different places so on the parts I changed I set these all to "logger" because it makes it more clear that it is a object and not a "log" method. I can undo if you don't like this. - Changed "credentials" to "credential" to match this is one `TokenCredenial` not a credential collection. I can undo if you don't like this. - Made the default HttpClientHandler static and shared for new HttpClients with `disposeHandler: false` dotnet/runtime#16255 - Updated a sample Program - I can add more tests if required ### Contribution Checklist <!-- Before submitting this PR, please make sure: --> - [X] The code builds clean without any errors or warnings - [X] The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) - [X] The code follows the .NET coding conventions (https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) verified with `dotnet format` - [X] All unit tests pass, and I have added new tests where possible - Most tests pass but it skips some because I don't have keys for all these model types so I hope your github build will run this and double check. - [ ] I didn't break anyone 😄 - I changed the constructor method args in the OpenAI code, and the will break your users that are creating these open ai classes using their constructors directly. If the users are using the extension methods (AddAzureOpenAI) then they should not have problems. - If you want I can put back all of the constructor methods so they can be created with HttpClient or IDelegatingHandlerFactory but with parameters orders different as explained - I don't think those classes need to be aware of IDelegatingHandlerFactory or have it included in the constructor methods, since the extension method or calling code can always convert to HttpClient before calling.
|
The HttpClient has been updated across the kernel; please let us know if this issue still persists. |
alliscode
pushed a commit
to microsoft/chat-copilot
that referenced
this issue
Jul 19, 2023
### Motivation and Context My company network needs more custom set up in HttpClient to make calls, but SK OpenAI code only allows me to pass a hander delegate factory. I cannot use SK without more settings in HttpClient and I want to pass in my own instances anyway so I can manage them from my application. I think I am not the only one to need this: - microsoft/semantic-kernel#527 - microsoft/semantic-kernel#133 See https://learn.microsoft.com/en-us/dotnet/fundamentals/networking/http/httpclient-guidelines. ### Description Changes included: - The OpenAI and AzureOpenAI connectors take optional HttpClient? in the constructor methods instead of IDelegatingHandlerFactory - Note first approach was adding two new constructor methods to each type so you could choose. But two constructors could not be there together with optional HttpClient? in one and optional IDelegatingHandlerFactory? in the other because it made calls ambiguous. Then I tried making HttpClient not optional, but that made the parameter order different because in some there are also other optional parameters like openai `string? organization = null`. So the not optional HttpClient would have to come BEFORE optional openai organization in one constructor, but the optional IDelegatingHandlerFactory comes AFTER optional openai organization in another constructor. In the end it seemed better to just take HttpClient and do the custom client creation in the extension methods. I hope this is okay I know it is a bigger change than I planned. - IDelegatingHandlerFactory is not being used as a factory in these Open AI connector classes anyway. They use it one to create an HttpClient instance in the constructor method anyway and never call it again so it's just as easy to pass in the HttpClient you want. - Updated the extension methods in `KernelConfigOpenAIExtensions` so that they can optionally pass in a custom HttpClient and ILogger. If null then it uses the same defaults as before. - There was some mix use of `Logger log` and `ILogger logger` in different places so on the parts I changed I set these all to "logger" because it makes it more clear that it is a object and not a "log" method. I can undo if you don't like this. - Changed "credentials" to "credential" to match this is one `TokenCredenial` not a credential collection. I can undo if you don't like this. - Made the default HttpClientHandler static and shared for new HttpClients with `disposeHandler: false` dotnet/runtime#16255 - Updated a sample Program - I can add more tests if required ### Contribution Checklist <!-- Before submitting this PR, please make sure: --> - [X] The code builds clean without any errors or warnings - [X] The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) - [X] The code follows the .NET coding conventions (https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) verified with `dotnet format` - [X] All unit tests pass, and I have added new tests where possible - Most tests pass but it skips some because I don't have keys for all these model types so I hope your github build will run this and double check. - [ ] I didn't break anyone 😄 - I changed the constructor method args in the OpenAI code, and the will break your users that are creating these open ai classes using their constructors directly. If the users are using the extension methods (AddAzureOpenAI) then they should not have problems. - If you want I can put back all of the constructor methods so they can be created with HttpClient or IDelegatingHandlerFactory but with parameters orders different as explained - I don't think those classes need to be aware of IDelegatingHandlerFactory or have it included in the constructor methods, since the extension method or calling code can always convert to HttpClient before calling.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I'm analyzing the connector classes and I'm worried about the use of HttpClient. It seems that they are not following the guidelines outlined in https://learn.microsoft.com/en-us/dotnet/fundamentals/networking/http/httpclient-guidelines. This could potentially cause port/socket exhaustion problems and lead to the application shutting down.
The text was updated successfully, but these errors were encountered: