Allow for overriding of specific pool key params

[sigmavirus24]

This re-enables the use case of providing a custom SSLContext via a Transport Adapter as broken in #6655 and reported in #6715

Closes #6715

[nateprewitt]

If @florianlink, @luisvicenteatprima and/or @Marco-Kaulea are willing to give this patch a try, it would be nice to make sure we're not missing any other requirements.

Otherwise this seems straight forward to me, just a couple minor comments to add a date and the current PR reference.

[nateprewitt]

I think this looks good to go. We've received some early indication is solves this class of problems from what's been raised. I'll plan to cut a release with this tomorrow AM in the US.

[Marco-Kaulea]

Sorry for the late response, this is my work account. I tested the changes in this branch. I was able to successfully modify our code and send requests.

[luisvicenteatprima]

Sorry, I missed the ping @nateprewitt. I'm going to give this a try locally

[pquentin]

While this provides a new API to change the kwargs, the previous API (currently used by the Python Elasticsearch client) no longer works. The Requests docs contains an example where setting the ssl_version in init_poolmanager is enough. The snippet below is a modified version of that example to require TLS 1.3 or above and then trying to request a TLS 1.2 endpoint:

import ssl

from requests import Request
from requests.adapters import HTTPAdapter
from urllib3.poolmanager import PoolManager


class TLS13HttpAdapter(HTTPAdapter):
    def init_poolmanager(self, connections, maxsize, block=False):
        ctx = ssl.create_default_context()
        ctx.minimum_version = ssl.TLSVersion.TLSv1_3
        self.poolmanager = PoolManager(ssl_context=ctx)


if __name__ == "__main__":
    adapter = TLS13HttpAdapter()
    req = Request("GET", "https://tls-v1-2.badssl.com:1012/")
    resp = adapter.send(req.prepare())
    print(resp)

It fails correctly with requests 2.31.0, but unexpectedly works with requests 2.32.2 and with the changes in this pull request.

I also want to make it clear that this regression has nothing to do with the get_connection changes (which this pull request seems to assume?). It was caused by #6667. If I revert #6667 on top of main, the code above behaves as expected by failing the request.

[sigmavirus24]

It's a combination of the two effectively. Using the default module is done in the function used to replace get connection. That means that there's no good way to interact with specifying a context via initializing the pool manager. If instead we had created a different API here, it wouldn't be a problem and realistically, if we changed that instead of introducing this, it would be a better change but it's still a different API for folks to adapt to

[achapkowski]

@sigmavirus24 this doesn't fix the recursion issue we see in issue #6715

[nateprewitt]

@achapkowski I believe we already root caused the recursion issue for you some time ago in Esri/arcgis-python-api#1698 and provided guidance on what you need to fix in arcgis. Please fix the issue in your library and open a new issue if the problem persists.

[pquentin]

@sigmavirus24 I'm not sure I'm following the details, but I can certainly use build_connection_pool_key_attributes going forward (while keeping the old way for older versions of requests).

That does mean a lot of adapter users will silently break in 2.32, especially if the examples in docs don't change.

[nateprewitt]

@pquentin we're talking through options now to minimize friction/the amount of breakage. For everyone else in the thread, I'm waiting to release 2.32.3 until we spend some more time working through improving the init_poolmanager use case. This new API may either end up modified or removed depending on what we come up with. For the moment, leaving Requests at <2.32 is probably the best option so we're not rushing something half-baked out.

I'll update the thread at the end of today with status if we don't release before then.

[achapkowski]

@nateprewitt when I create an adapter using truststore per the guidance of that issue. This workaround is now broken.
Using the code above, it works, but it's odd.

class TruststoreAdapter(HTTPAdapter):
    def init_poolmanager(self, connections, maxsize, block=False):
        ctx = truststore.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
        return super().init_poolmanager(connections, maxsize, block, ssl_context=ctx)

if I modify the adapter from the example above:

def init_poolmanager(self, connections, maxsize, block=False):
        ctx = truststore.SSLContext(ssl.PROTOCOL_TLS_CLIENT)

        self.poolmanager = PoolManager(ssl_context=ctx)

It will works (it doesn't recursively die), but I need to verify that it's actually using the truststore SSLContext and not ssl.SSLContext.

[nateprewitt]

Yep, that's the case we're working on fixing here. The solution you have works as expected in Requsts<2.32.

[nateprewitt]

To provide that quick update, we're currently looking at disabling the SSLContext optimization in the event we have a PoolManager with any custom configuration kwargs. That should leave the default Requests implementation better off without requiring lifting from users with custom init_poolmanager implementations in their adapters.

Whether we introduce this new API for further customization is still TBD, but we'll update this PR once we have a decision.

[nateprewitt]

Alright, waiting for tests to pass and I think this should be good to go. If I can get another pair of eyes from another maintainer, I'll merge and cut a new release.

[sigmavirus24]

@nateprewitt looks good to me