[Bug]: Proxy settings are not correctly set for Azure clients

anetbnd commented 1 week ago

What happened?

When the Azure client is initialized inside router.py it reads at around line 1544 the proxy settings from the environment variables HTTP_PROXY and HTTPS_PROXY but it does some mistakes by doing so:

It only sets the proxy for the httpx.AsyncClient, when both environment variables are set. If only one variable set (for example HTTPS_PROXY not not HTTP_PROXY) it does not apply any proxy settings.
It does not read the values from NO_PROXY

This leads to an error in case only https connections need a proxy, but http connection should not use an proxy, or when there are certain domains, which should not use proxies.

In our usecase, we have internal and external models. The external models use https and we have to use a cooperative proxy for connecting to them. The internal models use http and we must not use a proxy for them. So the current proxy setup does not allow to do this.

Relevant other issues are: https://github.com/BerriAI/litellm/issues/2127 and https://github.com/BerriAI/litellm/issues/3556 but both are just handling the NO_PROXY problem, but not the problem with having one proxy (https or http) None and the other set to a value.

Relevant log output

Twitter / LinkedIn details

-

ishaan-jaff commented 1 week ago

@anetbnd it's a bit hard to repro this scenario for us, can you help us out and tell us what needs to be changed for your scenario

here's the code where we ready the NO_PROX settings


# Check if the HTTP_PROXY and HTTPS_PROXY environment variables are set and use them accordingly.
http_proxy = os.getenv("HTTP_PROXY", None)
https_proxy = os.getenv("HTTPS_PROXY", None)
no_proxy = os.getenv("NO_PROXY", None)

# Create the proxies dictionary only if the environment variables are set.
sync_proxy_mounts = None
async_proxy_mounts = None
if http_proxy is not None and https_proxy is not None:
  sync_proxy_mounts = {
      "http://": httpx.HTTPTransport(proxy=httpx.Proxy(url=http_proxy)),
      "https://": httpx.HTTPTransport(proxy=httpx.Proxy(url=https_proxy)),
  }
  async_proxy_mounts = {
      "http://": httpx.AsyncHTTPTransport(
          proxy=httpx.Proxy(url=http_proxy)
      ),
      "https://": httpx.AsyncHTTPTransport(
          proxy=httpx.Proxy(url=https_proxy)
      ),
  }

  # assume no_proxy is a list of comma separated urls
  if no_proxy is not None and isinstance(no_proxy, str):
      no_proxy_urls = no_proxy.split(",")

      for url in no_proxy_urls:  # set no-proxy support for specific urls
          sync_proxy_mounts[url] = None  # type: ignore
          async_proxy_mounts[url] = None  # type: ignore

anetbnd commented 1 week ago

In our scenario the HTTPS_PROXY is set and the HTTP_PROXY is not set. This leads to the situation, that http_proxy is None and https_proxyis not None. Therefore on the line if http_proxy is not None and https_proxy is not None it does not setup any proxy related settings for httpx.

To be honest, I have not checked the latest version regarding the no_proxy issue, because I have seen that the related bug reports are still open and not solved (https://github.com/BerriAI/litellm/issues/2127). However looking at your code now, it should work. I will try this. But still the issue with HTTPS_PROXY and HTTP_PROXY is open.

Just out of curiosity: HTTPX is handling the HTTP_PROXY, HTTPS_PROXY and NO_PROXY environment variables out of the box, see: https://www.python-httpx.org/environment_variables/#http_proxy-https_proxy-all_proxy Why do you need to set those values in your code? Would it not be possible to set nothing proxy related and use the default handling from HTTPX here?

anetbnd commented 1 week ago

I debugged this issue today a little bit further and updated to the latest litellm version 1.40.26.

Here I noticed another issue: Litellm is just copying the content of the NO_PROXY variable into the mounts of the AsyncClient from httpx. However the format of the mounts is different to the format of NO_PROXY, so for example in NO_PROXY there is the value localhost and httpx expects a value all://localhost inside the mounts. So actually the values from NO_PROXY must first be translated to the format httpx expects.

This finding motivated me again, to ask the question "Why does litellm this proxy stuff at all, because actually httpx is taking care about it".

I tested it: When I create an httpx.AsyncClient() in my code, it automatically reads out the HTTP_PROXY and HTTPS_PROXY and NO_PROXY and translates this into the format it needs. So why has litellm to do it in its own code?

I partially found an answer to that: If I just configure the system in a way, that litellm is giving None to the mounts of the AsyncClient (the default value, which I also gave in my tests). The connection does not work. Because the AsyncClient objects ends up with an empty dictionary {} in its attribute _mounts, therefore no proxy settings have been applied. While the AsyncClient object, I created in my test (also with mounts being None), has a full dictionary inside the _mounts attribute, because it has translated the proxy settings from the environment variables automatically to it.

Therefore: LiteLLM has to do it, because for some mysterious reasons the automatic setting of the proxy settings is either not working or it is later overwritten by some other code. Until now, I was not able to identify why this is happening, but maybe you have an idea?

The second question, which might be worth to investigate is: "How does httpx translate the proxy settings into the mounts dictionary?". Because if this is a public available function, which does this, litellm could just reuse this function instead of doing the translation by itself (and failing in doing so).

Tomorrow, I will investigate further and keep you up to date about my findings.

anetbnd commented 1 week ago

Hello again, I found why the environment variables proxy settings are not applied. It is because of the custom transport. If there is a custom transport used for the AsyncClient the environment variable proxy settings are not taken into account anymore. Therefore the _mounts end up to be empty.

Unfortunately, I don't see an easy solution for that. Because the mounts defines for which url pattern, which transport should be used. So if you define an own transport, but also mounts, then it will use the transport from the mounts, every time a pattern from the mounts is matching and otherwise it will use the transport you defined. Therefore the current code inside litellm has many issues:

if HTTP_PROXY and HTTPS_PROXY is defined, it uses a proxy transport and not AsyncCustomHTTPTransport, which you actually wanted to use
the format translation from NO_PROXY to mounts is wrong
if only one environment variable HTTP_PROXY or HTTPS_PROXY is defined, it is not using any proxy at all (also not considering NO_PROXY)

Since you need to use a custom transport (AsyncCustomHTTPTransport), you have to do a careful definition of the transport and mounts arguments:

You have to create a proxy map from the environment variables. For this you can use this code snipped, which is used by httpx (from httpx._utils import get_environment_proxies and from httpx import Proxy):
```
{
key: None if url is None else Proxy(url=url)
for key, url in get_environment_proxies().items()
}
```
Afterwards you map every entry of the proxy map to a transport with the following rules:
- Is the value of the item is an instance of Proxy, map this key to an AsyncCustomHTTPTransport, which uses this proxy
- Is the value of the item is None, map this key to an AsyncCustomHTTPTransport, which is not using any proxy (the standard transport)
Now you can create the httpx.AsyncClient by assigning the standard transport (the one you have been used for the items, which had None values) as value for transport and your mounts dictionary from the last step

Since you are creating clients all over the code on different places, I would suggest to create an utility function, which handles this logic.

krrishdholakia commented 6 days ago

Hey @anetbnd I have a PR out to fix this - https://github.com/BerriAI/litellm/pull/4434

Can you confirm if my test covers your scenario - https://github.com/BerriAI/litellm/pull/4434/files#diff-e4391c9023696dfb1c5cc812d142ae613babbfacd4b9c6085b043d467ab3a90b

krrishdholakia commented 6 days ago

Can we do a 10min call sometime this/next week?

https://calendly.com/d/4mp-gd3-k5k/litellm-1-1-onboarding-chat

Context - want to learn how you're trying to use litellm so we can improve

anetbnd commented 6 days ago

Hi @krrishdholakia

Thanks for the fast fix and PR, I gave my comments to it. And we already have a call scheduled together for next week 😊

krrishdholakia commented 14 hours ago

Hey @anetbnd put out a dev release with fix. Can you confirm it works for you - https://github.com/BerriAI/litellm/releases/tag/v1.41.3.dev3

anetbnd commented 2 hours ago

Hi @krrishdholakia

Apart from the three issues inside the PR, which I commented today, I can confirm that the proxy and no-proxy selection works now.

Thank you very much for the fast fix. I'm looking forward to the final release of it.

BerriAI / litellm