Feature: urllib3 instead of curl

[spawn-guy]

for #737

i've migrated CurlClient to Urllib3Client and updated some unit tests.

i'd like to test this more thoroughly, though

needs a little bit more code cleanup as i am not even use this works (tests don't fail at least)

let me know what you think

[codecov]

Codecov Report

Attention: Patch coverage is 84.42623% with 19 lines in your changes missing coverage. Please review.

Project coverage is 80.80%. Comparing base (a520b1b) to head (d2f3dd2).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kombu/asynchronous/http/urllib3_client.py	83.92%	12 Missing and 6 partials ⚠️
kombu/asynchronous/aws/ext.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2134      +/-   ##
==========================================
+ Coverage   80.33%   80.80%   +0.46%     
==========================================
  Files          75       75              
  Lines        9125     9041      -84     
  Branches     1350     1333      -17     
==========================================
- Hits         7331     7306      -25     
+ Misses       1600     1551      -49     
+ Partials      194      184      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Nusnus]

Hey there 👋
Can you please the CI failures?

Thanks 🙏

[auvipy]

this would be a good move. please also check the failing tests

[spawn-guy]

a problem i see is that urllib3.PoolManager doesn't support proxy per request. to support proxies i should instantiate urllib3.ProxyManager.
an idea popped-up - i can create two pools.. one with a proxy, the other without. but still: one pool == one proxy

i think i need some guidance on how to properly test this migration: Who uses this client Outside of "kombu in vacuum"?

also the curl files are not removed yet

[Nusnus]

I gave it a quick review, nothing special popped into my eyes so far.
I'll review again if the CI fully passes.

[Nusnus]

I love this change 🙏

[Nusnus]

a problem i see is that urllib3.PoolManager doesn't support proxy per request. to support proxies i should instantiate urllib3.ProxyManager.

i think i need some guidance on how to properly test this migration: Who uses this client Outside of "kombu in vacuum"?

Good question. Maybe check Celery code?

also the curl files are not removed yet

Can you change the PR to draft meanwhile?
Let us know it's done when you set it back to "Ready for Review" 🙏

[spawn-guy]

i've run my code inside my beta environment (or at least i hope i did.. pipenv seems to checkout the correct commit id from my repo) (i even verified that the urllib3_client.py is, indeed, in the site-packages folder)

i've also tried pytest-celery and tests in celery (where there is a weird bug on Windows with some {docstring} and unicode characters - so i removed the {docstring}) and tests succeeded with Tests passed: 32,167, ignored: 160 of 32,331 tests

and all seems to behave as usual. but i don't use proxies

@auvipy @Nusnus the last questions are:

• what do we do with the proxy-per-request? a 1 urllib3.ProxyManager with 1 proxy per Pool ... or should i create some more pools per each proxy setting in requests (some lru cache, maybe)?
• should i continue to clean up the rest of the leftover curl code?

PS: the failing tests, basically, timeout during execution ;) = no more bad py3.8 code. and some code-coverage issue to fix

[auvipy]

• what do we do with the proxy-per-request? a 1 urllib3.ProxyManager with 1 proxy per Pool ... or should i create some more pools per each proxy setting in requests (some lru cache, maybe)?

may be the first approach would be fine to start. but if you can also go extra mile, it would be great as well

[spawn-guy]

Speaking about refactoring : if I remove the curl=None from request call - we can have a proper proxy pool. I just need to figure out which configure settings are used. But this is a breaking change 😬

[Nusnus]

Speaking about refactoring : if I remove the curl=None from request call - we can have a proper proxy pool. I just need to figure out which configure settings are used. But this is a breaking change 😬

Apologies for not being so helpful, I am pretty focused on the asyncio effort so I have less resources to support this PR as well at the moment 🙏

[spawn-guy]

@Nusnus no worries.

i think i'm done :)
no "max size of pools", though. but the rest of the missing functionality is back :)

i'm gonna deploy this to my testing env

[spawn-guy]

so.. apparently, for aws.getrequest - someone disabled certificate verification.

and now (with all my fancy code that enabled missing request.parameters) urllib3 complains that ssl verification is disabled :)
https://urllib3.readthedocs.io/en/latest/advanced-usage.html#tls-warnings

but then it complains that ca_cert is missing...

and all that worked with defaults :)

fixing that based on boto code + https://urllib3.readthedocs.io/en/latest/user-guide.html#ssl