Uh oh!
There was an error while loading. Please reload this page.
feat(api-core): centralize endpoint routing and universe domain resolution - #17745
feat(api-core): centralize endpoint routing and universe domain resolution#17745hebaalazzeh wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new routing module to handle mTLS endpoint resolution and universe domain configuration, along with corresponding unit tests and a test fixture for mTLS environment isolation. It also includes minor refactoring in noxfile.py and test_bidi.py. The reviewer identified an issue with the regex-based endpoint conversion logic in routing.py and suggested a more robust string-based implementation to prevent potential security vulnerabilities.
Uh oh!
There was an error while loading. Please reload this page.
3e2d8e2 to
0fe63b5CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
hebaalazzeh
commented
Jul 17, 2026
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new routing module in gapic_v1 to handle API endpoint and universe domain resolution, along with comprehensive unit tests. The feedback suggests improving get_default_mtls_endpoint by making domain suffix checks case-insensitive to correctly handle mixed-case or uppercase endpoints, and adding corresponding test cases to verify this behavior.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/google-api-core/google/api_core/gapic_v1/routing.py (36-47)
Domain names are case-insensitive. Performing case-sensitive checks like .endswith(".googleapis.com") can fail to recognize mixed-case or uppercase endpoints (e.g., foo.GoogleAPIs.com), resulting in a failure to resolve the correct mTLS endpoint. We should convert the endpoint to lowercase before performing suffix checks.
if not api_endpoint or ".mtls." in api_endpoint.lower():
return api_endpoint
lowered_endpoint = api_endpoint.lower()
if lowered_endpoint.endswith(".sandbox.googleapis.com"):
# len(".sandbox.googleapis.com") == 23
return api_endpoint[:-23] + ".mtls.sandbox.googleapis.com"
if lowered_endpoint.endswith(".googleapis.com"):
# len(".googleapis.com") == 15
return api_endpoint[:-15] + ".mtls.googleapis.com"
return api_endpoint
packages/google-api-core/tests/unit/gapic/test_routing.py (35-52)
Add test cases to verify that get_default_mtls_endpoint correctly handles case-insensitive domain suffixes (e.g., foo.GoogleAPIs.com).
deftest_get_default_mtls_endpoint():
# Test valid API endpointsassertget_default_mtls_endpoint("foo.googleapis.com") =="foo.mtls.googleapis.com"assert (
get_default_mtls_endpoint("foo.sandbox.googleapis.com")
=="foo.mtls.sandbox.googleapis.com"
)
# Test case-insensitivityassert (
get_default_mtls_endpoint("foo.GoogleAPIs.com")
=="foo.mtls.googleapis.com"
)
assert (
get_default_mtls_endpoint("foo.Sandbox.GoogleAPIs.com")
=="foo.mtls.sandbox.googleapis.com"
)
# Test endpoints that shouldn't be convertedassert (
get_default_mtls_endpoint("foo.mtls.googleapis.com")
=="foo.mtls.googleapis.com"
)
assertget_default_mtls_endpoint("foo.com") =="foo.com"# Test empty/None endpointsassertget_default_mtls_endpoint("") ==""assertget_default_mtls_endpoint(None) isNoneUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
nbayati
commented
Jul 17, 2026
I noticed that the logic in To maintain a single source of truth for evaluating the mTLS endpoint configuration and compliance with AIP-4114, consider refactoring This might also allow you to remove the |
daniel-sanche
commented
Jul 17, 2026
^ I gave similar feedback for read_enviornment_variables in the other PR. I didn't realize that code was copied in so many places |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Thanks for this refactoring! The mTLS logic looks solid. I have a few comments regarding some edge cases (specifically around is_proto3_optional and mock test coverage). I'll be OOO next week and I don't want to delay this PR, so there's no need to wait on my re-review. I'll defer to @daniel-sanche for the final review and sign-off!
1. packages/google-api-core/google/api_core/gapic_v1/requests.py (Line 68)
For proto-plus messages or other objects when is_proto3_optional=True, this fallback logic if not getattr(request, field_name, None): incorrectly overwrites an explicitly set empty string "" with a new UUID. The code should distinguish between missing fields and fields explicitly set to empty values, similar to how dictionaries are handled on line 55.
2. packages/google-api-core/tests/unit/gapic/test_requests.py (Line 30 and Line 47)
The __contains__ methods defined in MockRequest and MockValueErrorRequest are dead code. The setup_request_id function only uses the in operator for dict types, so these methods will never be executed.
3. packages/google-api-core/tests/unit/gapic/test_requests.py (Line 68)
There is a missing test case for is_proto3_optional=True with an explicit empty string for non-dict objects (e.g., (MockRequest(request_id=""), True, "")). Adding this would correctly expose the bug where setup_request_id overwrites explicitly set empty strings for proto-plus messages.
4. packages/google-api-core/tests/unit/gapic/test_requests.py (Line 113)
Using re.match allows trailing characters in the string because it only checks for a match at the beginning of the string. This should be changed to re.fullmatch(UUID_REGEX, value) (or the regex should be anchored with $) to guarantee strict UUID validation.
3a14360 to
3ce36bfComparecdca139 to
173b50aCompare173b50a to
85cd706Compare85cd706 to
8816dfeCompare7b7027d to
d435c6cCompared435c6c to
016d00eCompare| __all__ = [ | ||
| "get_universe_domain", | ||
| ] |
There was a problem hiding this comment.
Why does this need to be aliased here?
There was a problem hiding this comment.
Good catch, it doesn't need to be aliased here. I've removed client_utils.py entirely, along with its references in init.py and the associated tests.
| "CLOUDSDK_CONTEXT_AWARE_USE_CLIENT_CERTIFICATE": "false", | ||
| }, | ||
| ): | ||
| yield |
There was a problem hiding this comment.
It is not. I've removed the mock for CLOUDSDK_CONTEXT_AWARE_USE_CLIENT_CERTIFICATE in conftest.py as google-api-core does not rely on it.
Description
Centralizes generic routing, mTLS, and universe domain resolution helpers under
google.api_core.universe(and exposes them viagoogle.api_core.gapic_v1.client_utils).Exposes
get_api_endpoint,get_default_mtls_endpoint, andget_universe_domainas public API helpers to be used by newly generated clients.The unit tests added in
test_client_utils.pyare identical to the generator's original routing test cases intest_service.py.j2(namelytest__get_api_endpointandtest__get_universe_domain), modified only to call the public helpers directly:test__get_api_endpointmaps to the newget_api_endpointhelper.test__get_universe_domainmaps to the newget_universe_domainhelper.Generated client libraries fall back to local implementations via a
_compat.pyhelper module if run against older versions ofgoogle-api-corethat lack these centralized functions.