feat(api-core): centralize MTLS fallback functions in gapic_v1 config - #17817
feat(api-core): centralize MTLS fallback functions in gapic_v1 config#17817hebaalazzeh wants to merge 2 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request introduces helper functions to manage mutual TLS (mTLS) configuration and environment variables, specifically use_client_cert_effective, get_client_cert_source, and read_environment_variables. It also adds comprehensive unit tests to verify their behavior under various conditions. There are no review comments, and I have no feedback to provide.
Sorry, something went wrong.
|
/gemini review |
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request introduces helper functions (use_client_cert_effective, get_client_cert_source, and read_environment_variables) in config.py to handle mutual TLS (mTLS) configuration and environment variable parsing, along with corresponding unit tests in test_config.py. I have no feedback to provide as there are no review comments to evaluate.
Sorry, something went wrong.
daniel-sanche
left a comment
There was a problem hiding this comment.
I left a couple comments with more details, but I don't actually think any of these methods belong in api_core. They all seem to be essentially just delegating to the true implementation in google-auth, which is the more natural place for this kind of logic
Sorry, something went wrong.
| "Environment variable `GOOGLE_API_USE_MTLS_ENDPOINT` " | ||
| "must be `never`, `auto` or `always`" | ||
| ) | ||
| return use_client_cert, use_mtls_endpoint, universe_domain_env |
There was a problem hiding this comment.
My feedback from #17750 (comment) stands: can we defer to google.auth.transport.mtls.should_use_mtls_endpoint to parse this variable? That would be a real centralization improvement, since that's the source of truth for this kind of thing
I don't actually think read_environment_variables is worthwile in api-core, since it's just reading back a couple variables. We probably don't need a helper for that
Sorry, something went wrong.
| raise ValueError( | ||
| "Client certificate is required for mTLS, but no client certificate source was provided or found." | ||
| ) | ||
| return client_cert_source |
There was a problem hiding this comment.
I don't think use_client_cert_effective or get_client_cert_source need to be in api-core either. These are basically just aliases to google-auth, and it creates an extra dependency link for no reason.
In the clients, we can just reach out to mtls.default_client_cert_source(), and provide a fallback in _compat
Sorry, something went wrong.
This PR centralizes the MTLS configuration fallback functions (use_client_cert_effective, get_client_cert_source, and read_environment_variables) directly into google.api_core.gapic_v1.config.
PR #17799 successfully centralized the endpoint routing functions into universe.py, but it did not include these MTLS functions. By moving these into api-core, we can remove the fallback implementations that are currently being generated into _compat.py by the GAPIC generator.
This provides the actual implementations that were missing from the earlier attempt in PR #17750.