fix(google-auth): keep PoolManager settings in AuthorizedHttp.configure_mtls_channel - #18427
Shubham-Padkonde wants to merge 1 commit into
Conversation
…re_mtls_channel configure_mtls_channel replaced the underlying urllib3.PoolManager with a new one built from scratch, discarding the caller's retries, timeout, maxsize, block, headers and num_pools. Carry those non-TLS settings over to the new PoolManager on both the mTLS and the non-mTLS path, matching how AuthorizedSession.configure_mtls_channel keeps the adapter's retry and pool settings. Fixes googleapis#18366
There was a problem hiding this comment.
Code Review
This pull request updates the urllib3 transport in google-auth to ensure that connection pool settings (such as retries, timeout, maxsize, block, headers, and num_pools) are preserved when configure_mtls_channel replaces the urllib3.PoolManager. However, the implementation incorrectly accesses the private _maxsize attribute of RecentlyUsedContainer to retrieve the pool limit, which will evaluate to None at runtime and cause the num_pools setting to be silently ignored. Both the source code and the corresponding test assertions should be updated to use the public maxsize attribute instead.
| num_pools = getattr(getattr(http, "pools", None), "_maxsize", None) | ||
| if isinstance(num_pools, int) and num_pools != _DEFAULT_NUM_POOLS: | ||
| settings["num_pools"] = num_pools |
There was a problem hiding this comment.
In urllib3, the RecentlyUsedContainer class (which is the type of http.pools) stores its maximum size in the public attribute maxsize, not _maxsize. Using _maxsize will return None at runtime, causing the num_pools setting to be silently ignored and not carried over to the new PoolManager instance.
| num_pools = getattr(getattr(http, "pools", None), "_maxsize", None) | |
| if isinstance(num_pools, int) and num_pools != _DEFAULT_NUM_POOLS: | |
| settings["num_pools"] = num_pools | |
| num_pools = getattr(getattr(http, "pools", None), "maxsize", None) | |
| if isinstance(num_pools, int) and num_pools != _DEFAULT_NUM_POOLS: | |
| settings["num_pools"] = num_pools |
| for name in ("retries", "timeout", "maxsize", "block"): | ||
| assert new_http.connection_pool_kw[name] is old_http.connection_pool_kw[name] | ||
| assert new_http.headers == {"x-custom": "value"} | ||
| assert new_http.pools._maxsize == 3 |
Fixes #18366 🦕
AuthorizedHttp.configure_mtls_channel()replacedself.httpwith aPoolManagerbuilt from scratch, so a caller's pool configuration was dropped. It now carries the non-TLS settings of the currentPoolManagerover to the new one:retries,timeout,maxsizeandblock(fromconnection_pool_kw);headers;num_pools.This matches how
AuthorizedSession.configure_mtls_channel()keeps the adapter's retry and pool settings.How it works:
_pool_manager_settings(http)returns only the settings that were set on a realurllib3.PoolManager, and{}for anything else (mocks, custom HTTP objects)._make_mutual_tls_http(cert, key, **pool_kwargs)and_make_default_http(**pool_kwargs)pass them toPoolManager. With no extra settings, both behave exactly as before.GOOGLE_API_USE_CLIENT_CERTIFICATE=truebut no cert found), because both replace the caller'sPoolManager.configure_mtls_channel(), so the settings also survive rotation.ssl_context,cert_reqs,ca_certs, …) and proxy settings are not carried over.A related behaviour I noticed but didn't change here:
AuthorizedHttp.__init__callsRequestMethods.__init__(), which resetsself.headers. Becauseheadersis proxied toself.http, headers passed to a user-providedPoolManager's constructor are already cleared whenAuthorizedHttpis created. The tests therefore set headers onAuthorizedHttpafter construction, which is the case this PR fixes.configure_mtls_channeldocstring)Testing
I ran these locally in
packages/google-authwith Python 3.12:tests/transport/test_urllib3.py:_make_mutual_tls_httpwith pool kwargs;configure_mtls_channelpreserving the settings on the mTLS path;mainand pass with this change.pytest tests/transport: 452 passed with urllib3 2.8.0.tests/transport/test_urllib3.pyalso passes (41 tests) with urllib3 1.26.20.ruff check --select Iandruff format --check(ruff 0.14.14, same flags asnox -s lint) andflake8are clean on the changed files.This change was written with help from an AI coding assistant (Claude Code). I reviewed and tested it as described above.
🤖 Generated with Claude Code