feat: [grpc] Add retry logic when certificate mismatch for existing credentials & Agent Identity workloads - #18019
Conversation
feat: Add passphrase handling to client cert callback
feat: Add cert rotation handling support
chore: Add passphrase in _mtls_helper for requests
…rase chore: Modify _mtls_helper call to include additional variable passphrase
Updated mock return values in test cases to include None for additional parameters.
Updated mock callback to return an additional None value.
Updated mock call_client_cert_callback to include a None value in the return tuple.
chore: Add unit tests for cert rotation handling for grpc
chore: Add tests for grpc cert roatation handling changes
![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 introduces mTLS certificate rotation and automatic retry capabilities for gRPC transport by implementing a refreshing channel wrapper and call interceptors. It also updates helper utilities to support key passphrases. The review feedback suggests handling potential CancelledError exceptions gracefully when checking futures to prevent thread crashes, and recommends removing several redundant getattr checks for attributes that are guaranteed to be initialized in their respective class constructors.
fix: Refactor gRPC call handling and state management
|
The github status checks are stuck. You could try closing and re-opening this PR to re-trigger them |
cdd91ce to
524247b
Compare
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
…mments and fix doctrings chore: Update Refactor mTLS gRPC client interceptor logic based on comments and fix doctrings
…ype hint for passphrase. chore: Refactor error handling for certificate retrieval and update type hint for passphrase.
fix: Refactor deadline error handling in grpc.py
daniel-sanche
left a comment
daniel-sanche
left a comment
There was a problem hiding this comment.
I left some comments after a first pass, but I need to spend more time reviewing some of these wrappers.
First though, are you sure this is the right layer? I don't have much context on this change, but IIRC, gapic libraries use a different channel type. So I just want to make sure this is accomplishing what you want it to
| "client_cert_callback": client_cert_callback, | ||
| "_is_recreation": True, # Hidden flag to stop recursion | ||
| **kwargs, | ||
| } |
There was a problem hiding this comment.
Instead of passing through all these arguments, it could be better to create a create_channel_fn partial here, and pass that in instead (we did something similar for bigtable)
| return self._is_mtls | ||
|
|
||
|
|
||
| class _MTLSCallInterceptor( |
There was a problem hiding this comment.
It might be worth creating a new file for these new classes? There are a bunch of them that seem pretty tightly coupled, without much integration with other classes in this file
9ee9410 to
7b3f17b
Compare
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
7b3f17b to
d8050ed
Compare
chore: Refactor gRPC call handling with base wrapper class
chore: Include Wrapper in CertRotationInterceptor initialization
4 participants
feat: [grpc] Add retry logic when certificate mismatch for existing credentials & Agent Identity workloads
This PR introduces mTLS certificate rotation and encrypted private key passphrase handling to the gRPC transport. It implements an interceptor and a refreshing channel to automatically reload client certificates upon authentication failures when certificates rotate. It also updates tests and mock return values across all transports.
See go/grpc-cert-rotation-in-pythonsdk-for-x509 for details.