Repository navigation
fix(auth): preserve aiohttp session settings when configuring mTLS in AsyncAuthorizedSession - #18692
agrawalradhika-cell wants to merge 1 commit into
Conversation
… AsyncAuthorizedSession configure_mtls_channel() replaced the transport with a new aiohttp.ClientSession/TCPConnector built with default arguments, so the caller's session settings were dropped on the first mTLS setup and on every certificate rotation triggered by a 401. When the current transport already has a session, the new mTLS session now copies the same settings that aiohttp.Request._clone() copies: connector limit, limit_per_host, force_close and local_addr, and session headers, cookie_jar, auth, timeout, trust_env, auto_decompress, trace_configs and json_serialize. The ssl context is always the new mTLS context. Behavior is unchanged when there is no session yet. Fixes googleapis#18314
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the mTLS channel configuration to preserve connector and session settings from an existing aiohttp.ClientSession during initial configuration and certificate rotation, and adds corresponding unit tests. The review feedback highlights two important improvements: first, the old session and its connector should be explicitly closed to prevent resource leaks; second, the code should avoid accessing private attributes of aiohttp classes and instead use public properties to reduce maintenance risk.
| old_auth_request = self._auth_request | ||
| self._auth_request = AiohttpRequest(session=new_session) |
There was a problem hiding this comment.
The old session and its underlying connector/connections are not closed when a new session is created during initial configuration or certificate rotation. This will cause resource leaks (unclosed client sessions and connectors) and trigger warnings in aiohttp. Awaiting close() on the old AiohttpRequest resolves this leak.
old_auth_request = self._auth_request
self._auth_request = AiohttpRequest(session=new_session)
if old_auth_request is not None:
await old_auth_request.close()| "limit": getattr( | ||
| orig_connector, "_limit", 100 | ||
| ), | ||
| "limit_per_host": getattr( | ||
| orig_connector, "_limit_per_host", 0 | ||
| ), | ||
| "force_close": getattr( | ||
| orig_connector, "_force_close", False | ||
| ), | ||
| "local_addr": _helpers_async._get_local_addr( | ||
| orig_connector | ||
| ), | ||
| } | ||
| ) | ||
| session_kwargs["auto_decompress"] = getattr( | ||
| orig_session, "_auto_decompress", True | ||
| ) | ||
| session_kwargs["trust_env"] = getattr( | ||
| orig_session, "_trust_env", False | ||
| ) | ||
| trace_configs = getattr( | ||
| orig_session, "_trace_configs", None | ||
| ) | ||
| if trace_configs: | ||
| session_kwargs["trace_configs"] = list( | ||
| trace_configs | ||
| ) | ||
| for attr_name, kwarg_name in [ | ||
| ("_default_headers", "headers"), | ||
| ("_cookie_jar", "cookie_jar"), | ||
| ("_default_auth", "auth"), | ||
| ("_timeout", "timeout"), | ||
| ("_json_serialize", "json_serialize"), | ||
| ]: | ||
| val = getattr(orig_session, attr_name, None) | ||
| if val is not None: | ||
| session_kwargs[kwarg_name] = val |
There was a problem hiding this comment.
Relying on private attributes (prefixed with _) of aiohttp.ClientSession and aiohttp.TCPConnector is risky because these are internal implementation details that can change across versions without notice. Instead, use the public properties where available. Accessing private fields is bad practice and creates maintenance risk.
"limit": getattr(orig_connector, "limit", 100),
"limit_per_host": getattr(orig_connector, "limit_per_host", 0),
"force_close": getattr(orig_connector, "force_close", False),
"local_addr": _helpers_async._get_local_addr(orig_connector),
}
)
session_kwargs["auto_decompress"] = getattr(orig_session, "auto_decompress", True)
session_kwargs["trust_env"] = getattr(orig_session, "trust_env", False)
trace_configs = getattr(orig_session, "trace_configs", None)
if trace_configs:
session_kwargs["trace_configs"] = list(trace_configs)
for attr_name, kwarg_name in [
("headers", "headers"),
("cookie_jar", "cookie_jar"),
("timeout", "timeout"),
]:
val = getattr(orig_session, attr_name, None)
if val is not None:
session_kwargs[kwarg_name] = valReferences
- Avoid accessing private attributes to bypass public API methods, as accessing private fields is bad practice and the minor performance gains do not justify the maintenance risk.
This branch has not been deployed
1 participant
Summary
AsyncAuthorizedSession.configure_mtls_channel()replaces the transport with a newaiohttp.ClientSession/TCPConnectorbuilt with default arguments. As a result, any settings on the caller's session are silently dropped: pool limits, headers, cookies, timeout,trust_env, tracing and so on. This happens on the first mTLS setup and again on every cert rotation triggered by a 401.If the current transport already has a session, the new mTLS session now copies the same settings that
aiohttp.Request._clone()copies:limit,limit_per_host,force_close,local_addrheaders,cookie_jar,auth,timeout,trust_env,auto_decompress,trace_configs,json_serializesslis always set to the new mTLS context. If there is no session yet (the default transport), behavior is unchanged.Pre-review checklist
configure_mtls_channel()builds the new aiohttp session, plus its unit tests. Not part of a stack.go/author-standard).test_configure_mtls_channel_preserves_existing_session_config: a realClientSessionwith every setting customized. All settings survive the first mTLS setup and a second reconfigure (rotation), and each new connector gets the new SSL context.test_configure_mtls_channel_with_minimal_existing_session: a session without aTCPConnectoror optional settings. Onlyssl,auto_decompressandtrust_envare passed.test_configure_mtls_channelstill asserts the exact calls), 401-triggered cert rotation (settings now survive each rotation), custom non-aiohttp transports (unchanged warning and TLS fallback). The aio tests pass on aiohttp 3.8.0 (Python 3.10) and 3.14.3 (Python 3.13).requests.AuthorizedSession.configure_mtls_channel()already keeps the adapter settings (max_retries, pool sizes,pool_block).urllib3.AuthorizedHttp.configure_mtls_channel()builds a freshPoolManager(ssl_context=ctx), so the settings of a caller-suppliedPoolManagerare dropped there too. This could be tracked separately (no issue filed yet).