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.

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.

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).

def test_get_default_mtls_endpoint():
    # Test valid API endpoints
    assert 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 case-insensitivity
    assert (
        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 converted
    assert (
        get_default_mtls_endpoint("foo.mtls.googleapis.com")
        == "foo.mtls.googleapis.com"
    )
    assert get_default_mtls_endpoint("foo.com") == "foo.com"

    # Test empty/None endpoints
    assert get_default_mtls_endpoint("") == ""
    assert get_default_mtls_endpoint(None) is None

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.