Skip to content

Respect no_proxy environment variable - #2171

Merged
micafer merged 10 commits into
apache:trunkfrom
Sanjays2402:fix/2077-respect-no-proxy
Sep 10, 2026
Merged

Respect no_proxy environment variable#2171
micafer merged 10 commits into
apache:trunkfrom
Sanjays2402:fix/2077-respect-no-proxy

Conversation

@Sanjays2402

Copy link
Copy Markdown
Contributor

Respect no_proxy environment variable

Description

Fixes #2077.

When a proxy is configured explicitly (via set_http_proxy() or the http_proxy /
https_proxy environment variables), libcloud used it for every request, including
hosts listed in no_proxy / NO_PROXY. Other HTTP clients (curl, requests via its
own env handling, urllib) all honour no_proxy, so this was surprising and made it
impossible to talk to an internal endpoint directly while a proxy was configured.

This adds a _proxies_for_url(url) helper on LibcloudBaseConnection that returns
an empty proxy mapping ({}) when the target host matches no_proxy, and None
otherwise so the session default applies. It is passed as proxies= in
LibcloudConnection.request(). Matching is delegated to requests.utils.should_bypass_proxies,
so libcloud inherits the exact same no_proxy semantics as requests rather than
reimplementing them.

Status

done, ready for review

Checklist (tick everything that applies)

  • Code linting (required, can be done after the PR checks)
  • Documentation
  • Tests
  • ICLA (required for bigger changes)

Sanjays2402 and others added 2 commits July 24, 2026 19:53
An explicitly configured proxy was used for every request, even when the
target host matched the no_proxy / NO_PROXY environment variable. Add a
_proxies_for_url helper that returns an empty proxy mapping for bypassed
hosts so libcloud behaves consistently with other HTTP clients.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new regression test’s environment cleanup can leak no_proxy state (and doesn’t isolate NO_PROXY), potentially causing flaky tests or side effects across the test suite.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates Libcloud’s Requests integration so that explicitly configured proxies (via set_http_proxy() / proxy_url) are bypassed for targets matching no_proxy / NO_PROXY, aligning behavior with Requests/curl/urllib semantics.

Changes:

  • Add LibcloudBaseConnection._proxies_for_url(url) to selectively disable proxies when should_bypass_proxies() indicates no_proxy should apply.
  • Pass proxies= per-request in LibcloudConnection.request() to allow bypass behavior for matching hosts.
  • Add a regression test and a CHANGES entry documenting the behavior change.
File summaries
File Description
libcloud/http.py Adds per-URL proxy selection and wires it into requests made by LibcloudConnection.
libcloud/test/test_connection.py Adds a regression test validating proxy bypass behavior for no_proxy hosts.
CHANGES.rst Documents the new no_proxy/NO_PROXY proxy-bypass behavior.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread libcloud/test/test_connection.py Outdated
Comment on lines +167 to +168
os.environ["no_proxy"] = "internal.example.com"
self.addCleanup(os.environ.pop, "no_proxy", None)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Sanjays2402, could you take a look?

@Sanjays2402

Copy link
Copy Markdown
Contributor Author

Thanks @micafer — good catch. The test now snapshots both no_proxy and NO_PROXY up front, pins NO_PROXY to unset for the test's duration, and the addCleanup restores the original values instead of unconditionally popping no_proxy after tearDown. The externally-set NO_PROXY case you (and Copilot) flagged can't leak in or out anymore.

@micafer micafer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked and it seems that returning an empty proxy mapping is not sufficient to bypass a proxy configured on Session.proxies.

Requests merges the per-request proxies argument with the session proxy configuration. When {} is passed, the session-level http / https proxies can therefore be merged back in, so the request may still use the explicitly configured proxy even though _proxies_for_url() returned {}.

The current regression test only asserts the return value of _proxies_for_url(), so it doesn't exercise this merge behavior.

Could you add an end-to-end-ish test around LibcloudConnection.request() which verifies the effective proxy configuration after Requests processing? You may need to explicitly override the configured schemes with None (e.g. {"http": None, "https": None}) when should_bypass_proxies() returns true, rather than returning {}.

Returning {} from _proxies_for_url() was not sufficient: requests merges
per-request proxies with the session proxies (merge_setting), so the
session-level http/https proxy was merged back in and still used for
no_proxy hosts. Now returns {'http': None, 'https': None}; requests
strips None values during the merge, leaving an empty effective mapping.

Also adds an end-to-end-ish test around LibcloudConnection.request()
(with HTTPAdapter.send mocked) asserting the proxy mapping that actually
reaches the adapter after requests processing, plus a control case
showing non-bypassed hosts still use the configured proxy.
@Sanjays2402

Copy link
Copy Markdown
Contributor Author

You're right — I traced it through requests' source and confirmed: Session.request() runs merge_setting(proxies, self.proxies), so passing {} merged the session-level proxy straight back in. I verified this locally: merge_setting({}, session_proxies) returns the session proxies, while merge_setting({'http': None, 'https': None}, session_proxies) strips the None values and yields an empty mapping. Fixed in 34fd511 — _proxies_for_url() now returns {'http': None, 'https': None} when bypassing, and I added the end-to-end-ish test you asked for: it mocks HTTPAdapter.send, drives LibcloudConnection.request(), and asserts the proxy mapping that actually reaches the adapter (no session proxy leaked, select_proxy() returns None), plus a control case proving non-bypassed hosts still use the configured proxy. The new test fails on the old code and passes on the new.

@codecov-commenter

codecov-commenter commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.60%. Comparing base (dc57f81) to head (3c6135a).

Files with missing lines Patch % Lines
libcloud/test/test_connection.py 88.89% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            trunk    #2171   +/-   ##
=======================================
  Coverage   83.59%   83.60%           
=======================================
  Files         352      352           
  Lines       81866    81926   +60     
  Branches     8773     8783   +10     
=======================================
+ Hits        68433    68487   +54     
- Misses      10561    10564    +3     
- Partials     2872     2875    +3     
Files with missing lines Coverage Δ
libcloud/http.py 92.41% <100.00%> (+0.33%) ⬆️
libcloud/test/test_connection.py 96.65% <88.89%> (-0.99%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@micafer

micafer commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Hi @Sanjays2402,
There are some minor lint, and black style errors. Could you take a look?

@Sanjays2402

Sanjays2402 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Done @micafer

@micafer micafer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@micafer
micafer merged commit ac11dc7 into apache:trunk Sep 10, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Respect no_proxy environment variable

4 participants