fix(auth): allow mTLS retry when credentials raise NotImplementedError on refresh - #18349
fix(auth): allow mTLS retry when credentials raise NotImplementedError on refresh#18349mmckenzie-confl wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request removes an early return statement in sessions.py when credentials do not implement refresh(), allowing the handler to fall through and trigger a retry after mTLS reconfiguration. A corresponding unit test was added to verify this behavior. The review feedback points out a violation of the Repository Style Guide (Rule 20: Fallback Verification) regarding the use of generic assert_called_once assertions instead of explicit argument verification with assert_called_once_with in the new test case.
…entedError retry test
|
Hi mmckenzie-confl, thank you so much for putting up this PR. Unfortunately, the previous reviewer is OOO, and after reviewing and discussing this issue with other maintainers, we feel like we need to revisit #18224 which introduced some separate concurrency issues that needs to be patched - this is outside the scope of your current PR. As such, in the short term, we are going to recommend that for all Agent Identity 401 issues related async aiohttp, that the user opts out of "mTLS enforcement" by setting the flag documented here to false: https://docs.cloud.google.com/gemini-enterprise-agent-platform/scale/runtime/agent-identity#opt-out-caa I will keep you posted on the outcome of the internal PR reviews over the next several days. Thanks for your patience! Update: Sent out #18355 for maintainer review. Thanks! |
Summary
AsyncAuthorizedSession.request()'s inner_recover_auth_state()function(
google/auth/aio/transport/sessions.py), theexcept NotImplementedErrorbranchincorrectly did
return response, returning the original 401 to the caller instead offalling through to
return Noneto trigger a retry.credential types that do not implement
refresh()(e.g., Agent Identity workloads), eventhough the mTLS channel was successfully reconfigured.
RefreshErrorbranch correctly returns the 401 (a real refresh failure meansrecovery failed), but
NotImplementedErroris not an error — it means the credentialsimply doesn't support refresh. The mTLS reconfiguration was still successful, and the
retry should proceed.
This recovery path was introduced in #18224.
Extension of the fix for #18227
Test plan
test_cert_rotation_credential_refresh_not_implemented_retriesthat verifies credentials raisingNotImplementedErroron refresh still get a retry after mTLS reconfiguration