Repository navigation
common: honor HTTP_PROXY/HTTPS_PROXY env vars in http client - #21752
Conversation
|
Hi @texasich, thanks for your contribution! Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:
Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below. |
|
closing this to get back under the 1-PR limit for new contributors — #21630 is closer to merging so keeping that one active. will reopen this once that lands. branch is preserved on my fork. |
|
reopening — #21630 just merged so the new-contributor limit no longer applies. ready for review whenever the maintainers have a minute. |
|
Why is this still unmerged? |
|
Gentle nudge on this one — been open a while. Happy to rebase if there are conflicts, or make any changes needed. Let me know if anything's blocking review. 🙏 |
|
Please consider this PR. |
|
+1. Please consider this PR. |
|
For a modern software, not respecting to proxy is pretty bad. |
|
I'd assume most maintainers don't use proxy so no one can confidently review / test this PR (me including) Asking @ServeurpersoCom for your thoughts about this feature / this implementation |
|
Need to methodically test the HTTP client on Linux and on Windows with a small Squid as a classic enterprise forward proxy... I'm setting up the platform and will keep you updated. |
|
Tested this and the proxy path works fine for -hf, both direct and through the proxy, Linux only. Windows check in progress. A few non-blocking notes:
|
|
Windows OK, quick test with llama-installer.sh Wine + BorringSSL (default) with Zig. If OK, LGTM. |
ServeurpersoCom
left a comment
There was a problem hiding this comment.
Clarify or drop that comment, otherwise LGTM
|
Dropped the misleading fallback comment, the code only has the two scheme-specific vars, no generic one. Good catch. |
|
It's a minimalist proxy support that will unlock GGUF downloads for a lot of users, and we can easily do a follow-up PR later if we need to expand it. You could add the env var to the docs, but it's a super standard one for every web app on Linux. |
|
Thank you for the patch and it works for me. May I ask if there is any issue that caused this PR to be closed without being merged? |
|
There's quite a bit of work pending on the proxy; I'll try to make progress this weekend. |
+1 from me too |
|
@texasich Thanks for this feature. I addressed @ServeurpersoCom's review comments (no_proxy & logging) in texasich#1. On top of that, I also merged master back in |
|
Needs a rebase, there's a README conflict with master. I already tested the proxy and network side, Linux and Windows, and it behaves as expected, so once that's sorted the only thing missing is a second approval. |
| cli.set_proxy_basic_auth(proxy.user, proxy.password); | ||
| } | ||
| } catch (...) { | ||
| // ignore malformed proxy URL — don't break existing callers |
There was a problem hiding this comment.
|
@ServeurpersoCom |
Thanks @marecabo, the NO_PROXY handling and the warning look good. Tested on Linux behind Squid: with HTTP_PROXY set and no NO_PROXY, llama cli now hangs forever on "Loading model...". It talks to its own server on 127.0.0.1 through common_http_client, so the /health polling goes to the proxy and gets a 403 in a loop. Same with lowercase http_proxy, and setting NO_PROXY=localhost,127.0.0.1 makes it work. Master works fine in the same setup, so this is a regression for exactly the users this PR targets. We're not sure yet which way to fix it is best, there are a few options:
|
|
Good catch, thanks for the detailed report. Pushed a fix that makes common_http_client bypass the proxy unconditionally for loopback destinations (localhost, *.localhost, 127.0.0.0/8, ::1, 0.0.0.0): a remote proxy can never reach the machine's own loopback, so the 403 loop was unavoidable there. Same approach Go net/http uses. NO_PROXY handling and proxy support for downloads and remote --server-base endpoints stay unchanged. @marecabo GitHub shows the branch merging clean again, so nothing else needed on the rebase. |
| > [!TIP] | ||
| > If you're behind a corporate proxy, set the `HTTP_PROXY` and `HTTPS_PROXY` environment variables before running commands that download models (e.g. `export HTTPS_PROXY=http://proxy.example.com:8080`). Hosts listed in `NO_PROXY` and loopback addresses are contacted directly. | ||
|
|
There was a problem hiding this comment.
this is not a good place to put this, move it to server docs
common_http_client() was ignoring standard proxy env vars, causing downloads via -hf and similar flags to fail silently in environments behind an HTTP proxy (corporate networks, air-gapped infra, etc.). Checks HTTPS_PROXY/https_proxy for https:// URLs and HTTP_PROXY/http_proxy for http:// URLs, in that order. If a proxy URL is set and parseable, passes it to httplib's set_proxy() and set_proxy_basic_auth() as appropriate. Malformed proxy URLs are silently ignored so existing callers aren't affected. Fixes ggml-org#21694
The comment referenced a 'generic one' that doesn't exist in the code — both HTTP_PROXY and HTTPS_PROXY are scheme-specific. Reviewer noted the mismatch. Addresses review feedback from @ServeurpersoCom.
llama cli talks to its spawned server on 127.0.0.1 through common_http_client, so with HTTP_PROXY set and no NO_PROXY the health polling went to the corporate proxy, which rejects loopback targets, and the wait loop never exits ("Loading model..." hangs forever).
A remote proxy can never reach the client machine's own loopback, so these destinations are now always contacted directly, before NO_PROXY is consulted: localhost, *.localhost, 127.0.0.0/8, ::1 and 0.0.0.0. Similar to Go's net/http. Proxy handling for downloads and remote --server-base endpoints is unchanged.
Signed-off-by: texasich <101962694+texasich@users.noreply.github.com>
per review: the top level README stays lean; the proxy howto now lives with the server docs under Usage Signed-off-by: texasich <101962694+texasich@users.noreply.github.com>
55a122e to
3ba20f8
Compare
|
Moved. The tip is out of the README and now lives in tools/server/README.md under Usage (Running behind a proxy). Branch rebased onto current master. @ngxson |
…g#21752) * common: honor HTTP_PROXY/HTTPS_PROXY env vars in http client common_http_client() was ignoring standard proxy env vars, causing downloads via -hf and similar flags to fail silently in environments behind an HTTP proxy (corporate networks, air-gapped infra, etc.). Checks HTTPS_PROXY/https_proxy for https:// URLs and HTTP_PROXY/http_proxy for http:// URLs, in that order. If a proxy URL is set and parseable, passes it to httplib's set_proxy() and set_proxy_basic_auth() as appropriate. Malformed proxy URLs are silently ignored so existing callers aren't affected. Fixes ggml-org#21694 * common: drop misleading comment about generic proxy fallback The comment referenced a 'generic one' that doesn't exist in the code — both HTTP_PROXY and HTTPS_PROXY are scheme-specific. Reviewer noted the mismatch. Addresses review feedback from @ServeurpersoCom. * common : warn when the proxy URL from the environment is malformed * common : honor NO_PROXY when selecting the http proxy * ci : retrigger checks (test-thread-safety flake) * common : bypass the proxy for loopback destinations llama cli talks to its spawned server on 127.0.0.1 through common_http_client, so with HTTP_PROXY set and no NO_PROXY the health polling went to the corporate proxy, which rejects loopback targets, and the wait loop never exits ("Loading model..." hangs forever). A remote proxy can never reach the client machine's own loopback, so these destinations are now always contacted directly, before NO_PROXY is consulted: localhost, *.localhost, 127.0.0.0/8, ::1 and 0.0.0.0. Similar to Go's net/http. Proxy handling for downloads and remote --server-base endpoints is unchanged. Signed-off-by: texasich <101962694+texasich@users.noreply.github.com> * docs: move proxy tip from README to server docs per review: the top level README stays lean; the proxy howto now lives with the server docs under Usage Signed-off-by: texasich <101962694+texasich@users.noreply.github.com> --------- Signed-off-by: texasich <101962694+texasich@users.noreply.github.com> Co-authored-by: texasich <texasich@users.noreply.github.com> Co-authored-by: texasich <texasich@gmail.com> Co-authored-by: marecabo <23156476+marecabo@users.noreply.github.com>
…g#21752) * common: honor HTTP_PROXY/HTTPS_PROXY env vars in http client common_http_client() was ignoring standard proxy env vars, causing downloads via -hf and similar flags to fail silently in environments behind an HTTP proxy (corporate networks, air-gapped infra, etc.). Checks HTTPS_PROXY/https_proxy for https:// URLs and HTTP_PROXY/http_proxy for http:// URLs, in that order. If a proxy URL is set and parseable, passes it to httplib's set_proxy() and set_proxy_basic_auth() as appropriate. Malformed proxy URLs are silently ignored so existing callers aren't affected. Fixes ggml-org#21694 * common: drop misleading comment about generic proxy fallback The comment referenced a 'generic one' that doesn't exist in the code — both HTTP_PROXY and HTTPS_PROXY are scheme-specific. Reviewer noted the mismatch. Addresses review feedback from @ServeurpersoCom. * common : warn when the proxy URL from the environment is malformed * common : honor NO_PROXY when selecting the http proxy * ci : retrigger checks (test-thread-safety flake) * common : bypass the proxy for loopback destinations llama cli talks to its spawned server on 127.0.0.1 through common_http_client, so with HTTP_PROXY set and no NO_PROXY the health polling went to the corporate proxy, which rejects loopback targets, and the wait loop never exits ("Loading model..." hangs forever). A remote proxy can never reach the client machine's own loopback, so these destinations are now always contacted directly, before NO_PROXY is consulted: localhost, *.localhost, 127.0.0.0/8, ::1 and 0.0.0.0. Similar to Go's net/http. Proxy handling for downloads and remote --server-base endpoints is unchanged. Signed-off-by: texasich <101962694+texasich@users.noreply.github.com> * docs: move proxy tip from README to server docs per review: the top level README stays lean; the proxy howto now lives with the server docs under Usage Signed-off-by: texasich <101962694+texasich@users.noreply.github.com> --------- Signed-off-by: texasich <101962694+texasich@users.noreply.github.com> Co-authored-by: texasich <texasich@users.noreply.github.com> Co-authored-by: texasich <texasich@gmail.com> Co-authored-by: marecabo <23156476+marecabo@users.noreply.github.com>
Fixes #21694
What
common_http_client()builds the httplib client but never checks standard proxy environment variables, so any download that goes through this path (e.g.-hfflag, model presets) silently fails in environments that require a proxy.How
After constructing the client, read
HTTPS_PROXY/https_proxyforhttps://URLs andHTTP_PROXY/http_proxyforhttp://ones (scheme-specific takes precedence). If set and parseable, pass it tocli.set_proxy()and optionallycli.set_proxy_basic_auth(). Malformed proxy URLs are caught and silently ignored so nothing breaks for callers without a proxy configured.Reuses the existing
common_http_parse_url()to handle auth credentials embedded in the proxy URL (e.g.http://user:pass@proxy:3128).Notes
NO_PROXY/no_proxybypass list not implemented yet; can be added as follow-upALL_PROXYnot checked; HTTPS_PROXY and HTTP_PROXY cover the common casestd::getenvis available via<cstdlib>already pulled in by httplib.h