feat(generator): gapic generator centralization mtls - #17818
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request refactors mTLS configuration and client certificate source logic into a new compatibility template _compat.py.j2 to support older versions of google-api-core, updating client.py.j2 and unit tests accordingly. Feedback suggests using the imported mtls module directly in the test template's hasattr check to avoid a potential NameError, and notes that the generated golden files should be regenerated to ensure they are fully in sync with the updated templates.
Sorry, something went wrong.
|
/gemini review |
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request introduces a compatibility module (_compat.py.j2) to support older versions of google-api-core in generated GAPIC clients, refactoring several helper functions out of the main client template and adding corresponding unit tests. Feedback on these changes highlights a backwards-compatibility issue in the fallback implementation of get_client_cert_source where an exception is raised instead of returning None, as well as a minor type mismatch in a test assertion where a string is passed instead of a boolean.
Sorry, something went wrong.
| elif ( | ||
| hasattr(mtls, "has_default_client_cert_source") | ||
| and mtls.has_default_client_cert_source() | ||
| ): | ||
| client_cert_source = mtls.default_client_cert_source() | ||
| else: | ||
| raise ValueError( | ||
| "Client certificate is required for mTLS, but no client certificate source was provided or found." | ||
| ) |
There was a problem hiding this comment.
The fallback implementation of get_client_cert_source raises a ValueError when no client certificate source is found, whereas the historical implementation in client.py.j2 gracefully returned None. To prevent breaking changes for downstream users and maintain backwards compatibility, we should preserve the historical fallback behavior by returning None instead of raising an exception.
elif (
hasattr(mtls, "has_default_client_cert_source")
and mtls.has_default_client_cert_source()
):
client_cert_source = mtls.default_client_cert_source()
- Do not replace historical graceful fallback behaviors (such as returning False/falling back to standard TLS) with exceptions if doing so would introduce breaking changes for downstream users and violate backwards compatibility.
Sorry, something went wrong.
| with mock.patch('google.auth.transport.mtls.has_default_client_cert_source', return_value=True): | ||
| with mock.patch('google.auth.transport.mtls.default_client_cert_source', return_value=mock_default_cert_source): | ||
| assert {{ service.client_name }}._get_client_cert_source(None, True) is mock_default_cert_source | ||
| assert {{ service.client_name }}._get_client_cert_source(mock_provided_cert_source, "true") is mock_provided_cert_source |
There was a problem hiding this comment.
The second argument to _get_client_cert_source is annotated as a boolean (use_cert_flag: bool). Passing the string "true" instead of the boolean True is a type mismatch and can cause type-checking failures in downstream libraries. Please use the boolean literal True instead.
assert {{ service.client_name }}._get_client_cert_source(mock_provided_cert_source, True) is mock_provided_cert_source
Sorry, something went wrong.
This PR updates the generator templates to rely on the centralized MTLS fallback logic from google-api-core.
Since the MTLS helpers (use_client_cert_effective, get_client_cert_source, read_environment_variables) are being centralized into google/api_core/gapic_v1/config.py in the linked api-core PR, we no longer need the generator to produce fallback implementations for them. The generated _compat.py will now import these directly from config.py.
Depends on the api-core MTLS centralization PR.
related pr: #17817