feat(gapic): support mTLS certificates when available #717
Conversation
…prod/images-prod/python-librarian-generator@sha256:b8058df4c45e9a6e07f6b4d65b458d0d059241dd34c814f151c8bf6b89211209
product-auto-label
bot
added
the
api: clouderrorreporting
Summary of ChangesHello @parthea, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on a routine update to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
parthea
changed the title
![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 appears to be an automated update from the librarian tool, primarily updating the generator image and applying the resulting changes. The changes include refactoring the mTLS client certificate logic for better maintainability, which is a positive improvement. I've identified a couple of areas for improvement in the newly generated code. Specifically, there's a bug in a new version parsing function in __init__.py that could lead to incorrect behavior, for which I've suggested a more robust fix. Additionally, a new test function in the unit tests is quite repetitive, and I've proposed a refactoring to make it more concise and maintainable using pytest's parameterization features.
| def parse_version_to_tuple(version_string: str): | ||
| """Safely converts a semantic version string to a comparable tuple of integers. | ||
| Example: "4.25.8" -> (4, 25, 8) | ||
| Ignores non-numeric parts and handles common version formats. | ||
| Args: | ||
| version_string: Version string in the format "x.y.z" or "x.y.z<suffix>" | ||
| Returns: | ||
| Tuple of integers for the parsed version string. | ||
| """ | ||
| parts = [] | ||
| for part in version_string.split("."): | ||
| try: | ||
| parts.append(int(part)) | ||
| except ValueError: | ||
| # If it's a non-numeric part (e.g., '1.0.0b1' -> 'b1'), stop here. | ||
| # This is a simplification compared to 'packaging.parse_version', but sufficient | ||
| # for comparing strictly numeric semantic versions. | ||
| break | ||
| return tuple(parts) |
There was a problem hiding this comment.
The custom parse_version_to_tuple implementation is buggy for versions with pre-release identifiers that are not separated by a dot (e.g., 1.2.3a1 would be incorrectly parsed as (1, 2) instead of (1, 2, 3)). This can lead to incorrect version comparisons.
Since google-api-core (a dependency of this library) already depends on the packaging library, you can use packaging.version.parse for a more robust and correct implementation. This avoids maintaining a custom, bug-prone parsing function.
def parse_version_to_tuple(version_string: str):
"""Safely converts a semantic version string to a comparable tuple of integers.
Example: "4.25.8" -> (4, 25, 8)
Handles pre-release identifiers correctly.
Args:
version_string: Version string in semantic versioning format.
Returns:
Tuple of integers for the parsed version's release segment.
"""
from packaging.version import parse as parse_version
return parse_version(version_string).release| def test_use_client_cert_effective(): | ||
| # Test case 1: Test when `should_use_client_cert` returns True. | ||
| # We mock the `should_use_client_cert` function to simulate a scenario where | ||
| # the google-auth library supports automatic mTLS and determines that a | ||
| # client certificate should be used. | ||
| if hasattr(google.auth.transport.mtls, "should_use_client_cert"): | ||
| with mock.patch( | ||
| "google.auth.transport.mtls.should_use_client_cert", return_value=True | ||
| ): | ||
| assert ErrorGroupServiceClient._use_client_cert_effective() is True | ||
|
|
||
| # Test case 2: Test when `should_use_client_cert` returns False. | ||
| # We mock the `should_use_client_cert` function to simulate a scenario where | ||
| # the google-auth library supports automatic mTLS and determines that a | ||
| # client certificate should NOT be used. | ||
| if hasattr(google.auth.transport.mtls, "should_use_client_cert"): | ||
| with mock.patch( | ||
| "google.auth.transport.mtls.should_use_client_cert", return_value=False | ||
| ): | ||
| assert ErrorGroupServiceClient._use_client_cert_effective() is False | ||
|
|
||
| # Test case 3: Test when `should_use_client_cert` is unavailable and the | ||
| # `GOOGLE_API_USE_CLIENT_CERTIFICATE` environment variable is set to "true". | ||
| if not hasattr(google.auth.transport.mtls, "should_use_client_cert"): | ||
| with mock.patch.dict(os.environ, {"GOOGLE_API_USE_CLIENT_CERTIFICATE": "true"}): | ||
| assert ErrorGroupServiceClient._use_client_cert_effective() is True | ||
|
|
||
| # Test case 4: Test when `should_use_client_cert` is unavailable and the | ||
| # `GOOGLE_API_USE_CLIENT_CERTIFICATE` environment variable is set to "false". | ||
| if not hasattr(google.auth.transport.mtls, "should_use_client_cert"): | ||
| with mock.patch.dict( | ||
| os.environ, {"GOOGLE_API_USE_CLIENT_CERTIFICATE": "false"} | ||
| ): | ||
| assert ErrorGroupServiceClient._use_client_cert_effective() is False | ||
|
|
||
| # Test case 5: Test when `should_use_client_cert` is unavailable and the | ||
| # `GOOGLE_API_USE_CLIENT_CERTIFICATE` environment variable is set to "True". | ||
| if not hasattr(google.auth.transport.mtls, "should_use_client_cert"): | ||
| with mock.patch.dict(os.environ, {"GOOGLE_API_USE_CLIENT_CERTIFICATE": "True"}): | ||
| assert ErrorGroupServiceClient._use_client_cert_effective() is True | ||
|
|
||
| # Test case 6: Test when `should_use_client_cert` is unavailable and the | ||
| # `GOOGLE_API_USE_CLIENT_CERTIFICATE` environment variable is set to "False". | ||
| if not hasattr(google.auth.transport.mtls, "should_use_client_cert"): | ||
| with mock.patch.dict( | ||
| os.environ, {"GOOGLE_API_USE_CLIENT_CERTIFICATE": "False"} | ||
| ): | ||
| assert ErrorGroupServiceClient._use_client_cert_effective() is False | ||
|
|
||
| # Test case 7: Test when `should_use_client_cert` is unavailable and the | ||
| # `GOOGLE_API_USE_CLIENT_CERTIFICATE` environment variable is set to "TRUE". | ||
| if not hasattr(google.auth.transport.mtls, "should_use_client_cert"): | ||
| with mock.patch.dict(os.environ, {"GOOGLE_API_USE_CLIENT_CERTIFICATE": "TRUE"}): | ||
| assert ErrorGroupServiceClient._use_client_cert_effective() is True | ||
|
|
||
| # Test case 8: Test when `should_use_client_cert` is unavailable and the | ||
| # `GOOGLE_API_USE_CLIENT_CERTIFICATE` environment variable is set to "FALSE". | ||
| if not hasattr(google.auth.transport.mtls, "should_use_client_cert"): | ||
| with mock.patch.dict( | ||
| os.environ, {"GOOGLE_API_USE_CLIENT_CERTIFICATE": "FALSE"} | ||
| ): | ||
| assert ErrorGroupServiceClient._use_client_cert_effective() is False | ||
|
|
||
| # Test case 9: Test when `should_use_client_cert` is unavailable and the | ||
| # `GOOGLE_API_USE_CLIENT_CERTIFICATE` environment variable is not set. | ||
| # In this case, the method should return False, which is the default value. | ||
| if not hasattr(google.auth.transport.mtls, "should_use_client_cert"): | ||
| with mock.patch.dict(os.environ, clear=True): | ||
| assert ErrorGroupServiceClient._use_client_cert_effective() is False | ||
|
|
||
| # Test case 10: Test when `should_use_client_cert` is unavailable and the | ||
| # `GOOGLE_API_USE_CLIENT_CERTIFICATE` environment variable is set to an invalid value. | ||
| # The method should raise a ValueError as the environment variable must be either | ||
| # "true" or "false". | ||
| if not hasattr(google.auth.transport.mtls, "should_use_client_cert"): | ||
| with mock.patch.dict( | ||
| os.environ, {"GOOGLE_API_USE_CLIENT_CERTIFICATE": "unsupported"} | ||
| ): | ||
| with pytest.raises(ValueError): | ||
| ErrorGroupServiceClient._use_client_cert_effective() | ||
|
|
||
| # Test case 11: Test when `should_use_client_cert` is available and the | ||
| # `GOOGLE_API_USE_CLIENT_CERTIFICATE` environment variable is set to an invalid value. | ||
| # The method should return False as the environment variable is set to an invalid value. | ||
| if hasattr(google.auth.transport.mtls, "should_use_client_cert"): | ||
| with mock.patch.dict( | ||
| os.environ, {"GOOGLE_API_USE_CLIENT_CERTIFICATE": "unsupported"} | ||
| ): | ||
| assert ErrorGroupServiceClient._use_client_cert_effective() is False | ||
|
|
||
| # Test case 12: Test when `should_use_client_cert` is available and the | ||
| # `GOOGLE_API_USE_CLIENT_CERTIFICATE` environment variable is unset. Also, | ||
| # the GOOGLE_API_CONFIG environment variable is unset. | ||
| if hasattr(google.auth.transport.mtls, "should_use_client_cert"): | ||
| with mock.patch.dict(os.environ, {"GOOGLE_API_USE_CLIENT_CERTIFICATE": ""}): | ||
| with mock.patch.dict(os.environ, {"GOOGLE_API_CERTIFICATE_CONFIG": ""}): | ||
| assert ErrorGroupServiceClient._use_client_cert_effective() is False | ||
|
|
There was a problem hiding this comment.
The test test_use_client_cert_effective is quite long and contains a lot of repetitive code, especially for testing different casings of 'true' and 'false' for the environment variable. You can significantly shorten and simplify this test by using pytest.mark.parametrize and splitting the test logic based on the availability of should_use_client_cert.
@pytest.mark.skipif(not hasattr(google.auth.transport.mtls, "should_use_client_cert"), reason="google-auth does not have should_use_client_cert")
def test_use_client_cert_effective_with_should_use_client_cert():
# Test that `should_use_client_cert` is used and the env var is ignored.
with mock.patch.dict(os.environ, {"GOOGLE_API_USE_CLIENT_CERTIFICATE": "true"}):
with mock.patch("google.auth.transport.mtls.should_use_client_cert", return_value=True):
assert ErrorGroupServiceClient._use_client_cert_effective() is True
with mock.patch("google.auth.transport.mtls.should_use_client_cert", return_value=False):
assert ErrorGroupServiceClient._use_client_cert_effective() is False
@pytest.mark.skipif(hasattr(google.auth.transport.mtls, "should_use_client_cert"), reason="google-auth has should_use_client_cert")
class TestUseClientCertEffectiveEnvVar:
@pytest.mark.parametrize("true_val", ["true", "True", "TRUE"])
def test_env_var_true(self, true_val):
with mock.patch.dict(os.environ, {"GOOGLE_API_USE_CLIENT_CERTIFICATE": true_val}):
assert ErrorGroupServiceClient._use_client_cert_effective() is True
@pytest.mark.parametrize("false_val", ["false", "False", "FALSE"])
def test_env_var_false(self, false_val):
with mock.patch.dict(os.environ, {"GOOGLE_API_USE_CLIENT_CERTIFICATE": false_val}):
assert ErrorGroupServiceClient._use_client_cert_effective() is False
def test_env_var_unset(self):
with mock.patch.dict(os.environ, clear=True):
assert ErrorGroupServiceClient._use_client_cert_effective() is False
def test_env_var_invalid(self):
with mock.patch.dict(os.environ, {"GOOGLE_API_USE_CLIENT_CERTIFICATE": "unsupported"}):
with pytest.raises(ValueError):
ErrorGroupServiceClient._use_client_cert_effective()
PR created by the Librarian CLI to initialize a release. Merging this PR will auto trigger a release. Librarian Version: v0.7.0 Language Image: us-central1-docker.pkg.dev/cloud-sdk-librarian-prod/images-prod/python-librarian-generator@sha256:b8058df4c45e9a6e07f6b4d65b458d0d059241dd34c814f151c8bf6b89211209 <details><summary>google-cloud-error-reporting: 1.14.0</summary> ## [1.14.0](v1.13.0...v1.14.0) (2025-12-16) ### Features * support mTLS certificates when available (#717) ([7e99258](7e99258b)) </details>
2 participants
chore: librarian update image pull request: 20251216T195732Z