Add support for the no_proxy env var mechanism in the HTTP client

[scirelli]

What do these changes do?

Prior to this patch aiohttp would ignore no_proxy settings.

Are there changes in behavior for the user?

If no_proxy or NO_PROXY is set, and the request host matches one of the hosts from the no_proxy list, no proxy will be used.

Need help in finding where the unit tests go for this.

Related issue number

#4431

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• Add a new news fragment into the CHANGES folder
  • name it <issue_id>.<type> for example (588.bugfix)
  • if you don't have an issue_id change it to the pr id after creating the pr
  • ensure type is one of the following:
    • .feature: Signifying a new feature.
    • .bugfix: Signifying a bug fix.
    • .doc: Signifying a documentation improvement.
    • .removal: Signifying a deprecation or removal of public API.
    • .misc: A ticket has been closed, but it is not of interest to users.
  • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

[codecov-io]

Codecov Report

Merging #4445 (9e4408a) into master (71f574f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4445   +/-   ##
=======================================
  Coverage   97.17%   97.17%           
=======================================
  Files          41       41           
  Lines        8854     8863    +9     
  Branches     1424     1425    +1     
=======================================
+ Hits         8604     8613    +9     
  Misses        133      133           
  Partials      117      117           

Flag	Coverage Δ
unit	97.06% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiohttp/helpers.py	96.83% <100.00%> (+0.06%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71f574f...9e4408a. Read the comment docs.

[r-or]

Interested in this too!

[scirelli]

Not sure why the linter is failing. Any pointers?

[webknjaz]

The log is here https://github.com/aio-libs/aiohttp/pull/4445/checks?check_run_id=1827835221#step:6:60

Also, you can run the pre-commit.com tool locally to see the same.

[scirelli]

The log is here #4445 (checks)

Also, you can run the pre-commit.com tool locally to see the same.

Figures, doesn't fail locally when I run pre-commit.

Github

mypy --show-error-codes aiohttp tests
aiohttp/client.py:78: error: Module 'aiohttp.helpers' has no attribute 'proxy_bypass'  [attr-defined]
Found 1 error in 1 file (checked 102 source files)
make: *** [mypy] Error 1
Makefile:77: recipe for target 'mypy' failed
Error: Process completed with exit code 2.

Local

pre-commit run --all-files
Check CHANGES................................................................Passed
Check for merge conflicts....................................................Passed
Strip unnecessary `# noqa`s..................................................Passed
isort........................................................................Passed
black........................................................................Passed
Fix End of Files.............................................................Passed
Fix requirements.txt.........................................................Passed
Trim Trailing Whitespace.....................................................Passed
File Contents Sorter.........................................................Passed
Check for case conflicts.....................................................Passed
Check JSON...................................................................Passed
Check Xml....................................................................Passed
Check that executables have shebangs.........................................Passed
Check Toml...................................................................Passed
Check Xml....................................................................Passed
Check Yaml...................................................................Passed
Debug Statements (Python)....................................................Passed
Check for added large files..................................................Passed
Check for broken symlinks................................(no files to check)Skipped
Debug Statements (Python)....................................................Passed
fix UTF-8 byte order marker..................................................Passed
Fix python encoding pragma...................................................Passed
Check that executables have shebangs.........................................Passed
Check for case conflicts.....................................................Passed
Detect AWS Credentials.......................................................Passed
Detect Private Key...........................................................Passed
pyupgrade....................................................................Passed
flake8.......................................................................Passed
Check .rst files with the same linter used by pypi.org.......................Passed

It's definitely there proxy_bypass.

[PawelPeczek]

Hi there, could anyone estimate when there will be a chance to merge and release this fix?

[webknjaz]

Figures, doesn't fail locally when I run pre-commit.

I suppose the CI pulls-in newer code from master. You should probably rebase to get the same errors locally.

Hi there, could anyone estimate when there will be a chance to merge and release this fix?

Once merged, this will need to be backported to the 3.8 branch and then we'd need to make that branch release-worthy which requires quite a bit of work to at least stabilize the CI.

[webknjaz]

I've left a few suggestions to improve the structure. And this will also need some accompanying tests and docs changes. It's important to understand how this feature is supposed to be used by the users.

Also, it looks more like a feature so we should probably rename the change fragment to .feature instead of .bugfix.

[webknjaz]

I don't understand why it's necessary to have both spellings in the same test. It could be reasonable to test them separately, though. And maybe have different spellings with different values testing which one overrides another.

WDYT?

[scirelli]

You mean the case difference? If I remember correctly (been awhile, I'll go check) but there's no spec. for the proxy info, it's just by convention. And there's more than one; one with lower case and the other upper. I think there's even all_proxy ALL_PROXY. I looked at the source of urllib.request.get_env_proxy_for_url and they just lower case it. So it doesn't matter, I guess I'm just used to seeing them together all the time.

[webknjaz]

Yes, I mean the case difference. When you write a test, you communicate the expectations of how something is supposed to be used and to work in general. Point is that when you set both http_proxy and HTTP_PROXY, this creates a false impression (for any third party reader) and in order for this feature to work, the end-user is supposed to set two env variables with the same value. The underlying implementation you linked implies that the upper-case variant is always preferred and the lower-case is used as a fall-back.
The only reasonable case I see for setting both variables in a test is when the values are different: for example, a test could demonstrate that with http_proxy=http://example.org HTTP_PROXY=http://example.com set it would choose http://example.com.

[scirelli]

Make sense. Just doing a little debugging now. Before when I ran the tests I didn't have any environment variables set for the proxies, setting them, then running the tests, the tests fail.

Looks like some other tests break:

FAILED tests/test_helpers.py::test_proxies_from_env[https] - AssertionError: assert dict_keys(['http', 'https']) == {'https'}
FAILED tests/test_helpers.py::test_proxies_from_env[ws] - AssertionError: assert dict_keys(['http', 'ws']) == {'ws'}
FAILED tests/test_helpers.py::test_proxies_from_env[wss] - AssertionError: assert dict_keys(['http', 'wss']) == {'wss'}
FAILED tests/test_helpers.py::test_proxies_from_env_skipped[https] - AssertionError: assert {'http': ProxyInfo(proxy=URL('http://www.example.com,http//proxy.com'), proxy_auth=None)} == {}
FAILED tests/test_helpers.py::test_proxies_from_env_skipped[wss] - AssertionError: assert {'http': ProxyInfo(proxy=URL('http://www.example.com,http//proxy.com'), proxy_auth=None)} == {}

To reproduce:

export http_proxy='http://www.example.com,http://proxy.com'
make test

Should I create an issue for this? I can push a PR after this one to fix it....once I figure it out.

[webknjaz]

Normally, it's best to avoid dynamism like this in tests because it makes it harder to reason about what's expected because one should first dig through several levels of indirection correctly.

But since I don't want an infinite number of reviews on this PR, I'll just accept it. I think it's rather good now.

[webknjaz]

@scirelli could you send a backport PR against the 3.8 branch?
https://docs.aiohttp.org/en/stable/contributing.html#backporting