-
Notifications
You must be signed in to change notification settings - Fork 74
feat: auto-enable mTLS when supported certificates are detected #2472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: auto-enable mTLS when supported certificates are detected #2472
Conversation
Summary of ChangesHello @agrawalradhika-cell, 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 refines the mTLS enablement logic within the Python SDK, introducing a more intelligent and adaptive mechanism. It ensures that mTLS is explicitly controlled when 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the mTLS enablement logic to use a new _mtls_helper which automatically enables mTLS for MWID/X.509 certificate sources when the GOOGLE_API_USE_CLIENT_CERTIFICATE environment variable is not set. The changes are applied across numerous generated client files and templates, and new unit tests are added.
My review has found a couple of critical syntax errors in the test templates that will prevent the code from running. I've also pointed out that the new tests are confusing and contain redundant code. Additionally, there are some medium-severity concerns regarding the use of a private module and an inconsistency in type-hinting ignores. Please see the detailed comments for suggestions on how to address these issues.
gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2
Outdated
Show resolved
Hide resolved
gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2
Outdated
Show resolved
Hide resolved
tests/integration/goldens/asset/tests/unit/gapic/asset_v1/test_asset_service.py
Outdated
Show resolved
Hide resolved
gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2
Outdated
Show resolved
Hide resolved
gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new hybrid approach for mTLS enablement by using a helper function _mtls_helper.check_use_client_cert(). The changes are consistently applied across numerous client templates and generated "golden" files, and new unit tests are added to cover the new logic. My review focuses on improving code style, maintainability, and readability. I've pointed out some trailing whitespace issues, suggested refactoring duplicated test code, and recommended a minor code reordering for better readability.
gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2
Outdated
Show resolved
Hide resolved
gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2
Show resolved
Hide resolved
gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2
Outdated
Show resolved
Hide resolved
...ration/goldens/logging_internal/google/cloud/logging_v2/services/config_service_v2/client.py
Outdated
Show resolved
Hide resolved
...ation/goldens/logging_internal/google/cloud/logging_v2/services/metrics_service_v2/client.py
Outdated
Show resolved
Hide resolved
tests/integration/goldens/redis_selective/google/cloud/redis_v1/services/cloud_redis/client.py
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the logic for mTLS enablement to automatically activate it when MWID or X.509 certificate sources are detected and the GOOGLE_API_USE_CLIENT_CERTIFICATE environment variable is not set. This logic is encapsulated in a new _mtls_helper.check_use_client_cert() function. The changes are propagated across numerous generated client and test files.
My review focuses on the template files to ensure the consistency and maintainability of the generated code. I've identified a few areas for improvement:
- Inconsistent code formatting (tabs vs. spaces) in some import statements.
- Duplicated code in the newly added unit tests, which could be refactored for better readability and maintenance.
- A slightly misleading comment in the new tests.
Overall, the changes are sound and the new tests cover the intended logic. Addressing the feedback will enhance the quality and consistency of the codebase.
gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2
Outdated
Show resolved
Hide resolved
gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2
Outdated
Show resolved
Hide resolved
gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2
Outdated
Show resolved
Hide resolved
...ration/goldens/logging_internal/google/cloud/logging_v2/services/config_service_v2/client.py
Outdated
Show resolved
Hide resolved
|
/gemini review |
f21a72b to
eeb2ef1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a hybrid approach for mTLS enablement, using a new _mtls_helper to automatically enable mTLS when MWID or X.509 certificate sources are detected and the GOOGLE_API_USE_CLIENT_CERTIFICATE environment variable is not set. The changes are applied across numerous generated client libraries and their tests. While the overall approach is sound, I've identified a critical logic bug in one of the templates that would prevent mTLS from being enabled, a critical syntax error in a test template, and opportunities to reduce code duplication in the updated test files. Please see my detailed comments.
gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2
Outdated
Show resolved
Hide resolved
tests/integration/goldens/asset/tests/unit/gapic/asset_v1/test_asset_service.py
Outdated
Show resolved
Hide resolved
gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2
Outdated
Show resolved
Hide resolved
gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the logic for mTLS enablement to use a hybrid approach, automatically enabling it when certain certificate sources are detected but still allowing manual override via an environment variable. The changes are applied across numerous generated client files and templates, and corresponding unit tests are added or updated. My review focuses on improving the robustness of the new logic, reducing code duplication in tests, and removing redundant checks. I've identified a potential bug in the template files' handling of the environment variable when set to an empty string and suggested a fix. I've also recommended refactoring in the test files to improve maintainability.
gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2
Outdated
Show resolved
Hide resolved
gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2
Outdated
Show resolved
Hide resolved
gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2
Outdated
Show resolved
Hide resolved
gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2
Outdated
Show resolved
Hide resolved
tests/integration/goldens/asset/google/cloud/asset_v1/services/asset_service/client.py
Outdated
Show resolved
Hide resolved
tests/integration/goldens/asset/tests/unit/gapic/asset_v1/test_asset_service.py
Outdated
Show resolved
Hide resolved
gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a hybrid approach for mTLS enablement, which is a valuable improvement. The changes in the templates correctly use feature detection to decide between the new automatic mTLS logic and the old environment variable-based approach. However, there are several critical issues in the generated 'golden' files. They import a private module _mtls_helper from google-auth, creating a brittle dependency. More critically, the code incorrectly handles the return value from this helper, treating a boolean as a string, which will cause ValueError exceptions at runtime. I've also pointed out an opportunity to reduce code duplication in the templates and a minor style issue. Please address the critical issues in the golden files.
tests/integration/goldens/asset/google/cloud/asset_v1/services/asset_service/client.py
Outdated
Show resolved
Hide resolved
tests/integration/goldens/asset/google/cloud/asset_v1/services/asset_service/client.py
Outdated
Show resolved
Hide resolved
tests/integration/goldens/eventarc/google/cloud/eventarc_v1/services/eventarc/client.py
Outdated
Show resolved
Hide resolved
tests/integration/goldens/asset/google/cloud/asset_v1/services/asset_service/client.py
Outdated
Show resolved
Hide resolved
...integration/goldens/credentials/google/iam/credentials_v1/services/iam_credentials/client.py
Outdated
Show resolved
Hide resolved
...ation/goldens/logging_internal/google/cloud/logging_v2/services/metrics_service_v2/client.py
Outdated
Show resolved
Hide resolved
tests/integration/goldens/redis/google/cloud/redis_v1/services/cloud_redis/client.py
Outdated
Show resolved
Hide resolved
tests/integration/goldens/redis_selective/google/cloud/redis_v1/services/cloud_redis/client.py
Outdated
Show resolved
Hide resolved
gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2
Outdated
Show resolved
Hide resolved
gapic/ads-templates/%namespace/%name/%version/%sub/services/%service/client.py.j2
Outdated
Show resolved
Hide resolved
gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2
Outdated
Show resolved
Hide resolved
gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2
Outdated
Show resolved
Hide resolved
…_CERTIFICATE is not set, if the MWID/X.509 cert sources detected Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
…ert and handling the unsupported value for GOOGLE_API_USE_CLIENT_CERTIFICATE on the client library side Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new, more flexible approach for mTLS enablement, which is a valuable enhancement. The code refactoring into a helper method is a good step towards cleaner code. However, I've identified several issues that need attention. There are copy-paste errors in multiple test files that cause them to use the wrong client, invalidating the tests. Additionally, there's significant code duplication in the new tests, with one of the duplicated blocks containing a logical flaw. I also found some formatting issues in docstrings and potentially buggy logic in some of the generated internal client files where a refactoring seems incomplete. Addressing these points will improve the correctness and maintainability of the codebase.
tests/integration/goldens/asset/tests/unit/gapic/asset_v1/test_asset_service.py
Outdated
Show resolved
Hide resolved
...ration/goldens/logging_internal/google/cloud/logging_v2/services/config_service_v2/client.py
Outdated
Show resolved
Hide resolved
gapic/templates/%namespace/%name_%version/%sub/services/%service/client.py.j2
Show resolved
Hide resolved
gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2
Show resolved
Hide resolved
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
…ll covered Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
daniel-sanche
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM. It's not clear to me if the CI failures are related to this change or an infrastructure issue though
| client = client_class(transport=transport_name) | ||
| if not hasattr(google.auth.transport.mtls, "should_use_client_cert"): | ||
| with pytest.raises(ValueError): | ||
| client = client_class(transport=transport_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test (and others) branch on hasattr(google.auth.transport.mtls, "should_use_client_cert"). I think it's important both sides of the branch are covered.
IIUC, I think it will be, because of the testing/constraints files. We run tests against both the most recent supported version of dependencies, and the oldest version, so that will cover both branches.
I'd probably prefer to mention this detail in a comment above this kind of test, something like:
# expect error in google-auth < x.y.z
# these tests will be triggered with oldest and newest versions of the google-auth library
But I'm not sure if this is worth re-generating all the files to add. Just wanted to note it here at least
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for ads-templates, don't think they are related to goldens
As plan is to move it off, but I'll add all the testcases here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked with Daniel, this was for adding comments not testcases, can skip
daniel-sanche
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM. It's not clear to me if the CI failures are related to this change or an infrastructure issue though
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
andyrzhao
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| _DEFAULT_UNIVERSE = "googleapis.com" | ||
|
|
||
| @staticmethod | ||
| def _use_client_cert_effective(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should file a bug to follow up on moving this to a shared file. Each service will have this function repeated which can cause bloat in the generated code when there are many services.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, opened at #2485
#2483 was opened for CL failures |
The Python SDK will use a hybrid approach for mTLS enablement:
If the GOOGLE_API_USE_CLIENT_CERTIFICATE environment variable is set (either true or false or any value), the SDK will respect that setting. This is necessary for test scenarios and users who need to explicitly control mTLS behavior.
If the GOOGLE_API_USE_CLIENT_CERTIFICATE environment variable is not set, the SDK will automatically enable mTLS only if it detects Managed Workload Identity (MWID) or X.509 Workforce Identity Federation (WIF) certificate sources. In other cases where the variable is not set, mTLS will remain disabled.
** This change also adds unit test with respect to this change.
** This change is only for Client-Library use-cases