fix(gapic): mock os.path.exists in mTLS tests to support newer google auth#17807
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the unit test templates and generated golden files to mock os.path.exists alongside builtins.open when testing mTLS configurations with a mock certificate config file. It also cleans up trailing whitespaces in the template. The reviewer notes that unconditionally mocking os.path.exists to return True is risky as it affects all path checks in the context block. They recommend using side_effect with a lambda function to target only the specific configuration file.
881f77f to
926a96f
Compare
| config_filename = "mock_certificate_config.json" | ||
| config_file_content = json.dumps(config_data) | ||
| m = mock.mock_open(read_data=config_file_content) | ||
| with mock.patch("builtins.open", m): |
There was a problem hiding this comment.
How do things fail if we don't add this?
Can we instead add a new test case that fails on the newer auth? Because looking at this, it feels like we're looking to cover a gap in test cases which doesn't exist right now. Unless I'm missing something.
There was a problem hiding this comment.
Can you link the failure here?
There was a problem hiding this comment.
Hi Omair,
Thanks for the thorough review! These are excellent questions.
Here is the failure we are seeing in pre-release dependency testing:
> assert cert_source is expected_cert_source
E AssertionError: assert None is <Mock id='140387160599120'
How it fails:
In test_iam_credentials_client_get_mtls_endpoint_and_cert_source, we test mTLS auto-enablement when:
GOOGLE_API_USE_CLIENT_CERTIFICATEis unset- but
GOOGLE_API_CERTIFICATE_CONFIGpoints to a valid config file
In the older version of google-auth we only mocked the builtins.open to serve up the mock configuration JSON. However, newer versions of google-auth are a bit stricter and call os.path.exists() on that config path before attempting to open it. Since the file mock_certificate_config.json is a figment of our test's imagination (it doesn't exist on disk), the call to os.path.exists() returns False. google-auth then assumes the config is missing, disables mTLS, and returns None for cert_source, causing our assertion to fail.
Regarding the "Gap":
You are right to check, but we aren't actually looking to cover a new gap in test cases here. The test case for this specific auto-enablement path already exists in the parametrized loop.
The gap was in our mocking strategy for this existing test. It worked fine before because older google-auth wasn't checking file existence as strictly (or was relying on open directly). We aren't adding a new scenario; we are just beefing up the mocks in the existing test so it continues to pass valid scenarios with newer dependencies.
For context, here is the full traceback for one of the failing tests.
_ test_iam_credentials_client_get_mtls_endpoint_and_cert_source[IAMCredentialsAsyncClient] _
client_class = <class 'google.iam.credentials_v1.services.iam_credentials.async_client.IAMCredentialsAsyncClient'>
@pytest.mark.parametrize("client_class", [
IAMCredentialsClient, IAMCredentialsAsyncClient
])
@mock.patch.object(IAMCredentialsClient, "DEFAULT_ENDPOINT", modify_default_endpoint(IAMCredentialsClient))
@mock.patch.object(IAMCredentialsAsyncClient, "DEFAULT_ENDPOINT", modify_default_endpoint(IAMCredentialsAsyncClient))
def test_iam_credentials_client_get_mtls_endpoint_and_cert_source(client_class):
mock_client_cert_source = mock.Mock()
# Test the case GOOGLE_API_USE_CLIENT_CERTIFICATE is "true".
with mock.patch.dict(os.environ, {"GOOGLE_API_USE_CLIENT_CERTIFICATE": "true"}):
mock_api_endpoint = "foo"
options = client_options.ClientOptions(client_cert_source=mock_client_cert_source, api_endpoint=mock_api_endpoint)
api_endpoint, cert_source = client_class.get_mtls_endpoint_and_cert_source(options)
assert api_endpoint == mock_api_endpoint
assert cert_source == mock_client_cert_source
# Test the case GOOGLE_API_USE_CLIENT_CERTIFICATE is "false".
with mock.patch.dict(os.environ, {"GOOGLE_API_USE_CLIENT_CERTIFICATE": "false"}):
mock_client_cert_source = mock.Mock()
mock_api_endpoint = "foo"
options = client_options.ClientOptions(client_cert_source=mock_client_cert_source, api_endpoint=mock_api_endpoint)
api_endpoint, cert_source = client_class.get_mtls_endpoint_and_cert_source(options)
assert api_endpoint == mock_api_endpoint
assert cert_source is None
# Test the case GOOGLE_API_USE_CLIENT_CERTIFICATE is "Unsupported".
with mock.patch.dict(os.environ, {"GOOGLE_API_USE_CLIENT_CERTIFICATE": "Unsupported"}):
if hasattr(google.auth.transport.mtls, "should_use_client_cert"):
mock_client_cert_source = mock.Mock()
mock_api_endpoint = "foo"
options = client_options.ClientOptions(
client_cert_source=mock_client_cert_source, api_endpoint=mock_api_endpoint
)
api_endpoint, cert_source = client_class.get_mtls_endpoint_and_cert_source(
options
)
assert api_endpoint == mock_api_endpoint
assert cert_source is None
# Test cases for mTLS enablement when GOOGLE_API_USE_CLIENT_CERTIFICATE is unset.
test_cases = [
(
# With workloads present in config, mTLS is enabled.
{
"version": 1,
"cert_configs": {
"workload": {
"cert_path": "path/to/cert/file",
"key_path": "path/to/key/file",
}
},
},
mock_client_cert_source,
),
(
# With workloads not present in config, mTLS is disabled.
{
"version": 1,
"cert_configs": {},
},
None,
),
]
if hasattr(google.auth.transport.mtls, "should_use_client_cert"):
for config_data, expected_cert_source in test_cases:
env = os.environ.copy()
env.pop("GOOGLE_API_USE_CLIENT_CERTIFICATE", None)
with mock.patch.dict(os.environ, env, clear=True):
config_filename = "mock_certificate_config.json"
config_file_content = json.dumps(config_data)
m = mock.mock_open(read_data=config_file_content)
with mock.patch("builtins.open", m):
with mock.patch.dict(
os.environ, {"GOOGLE_API_CERTIFICATE_CONFIG": config_filename}
):
mock_api_endpoint = "foo"
options = client_options.ClientOptions(
client_cert_source=mock_client_cert_source,
api_endpoint=mock_api_endpoint,
)
api_endpoint, cert_source = (
client_class.get_mtls_endpoint_and_cert_source(options)
)
assert api_endpoint == mock_api_endpoint
> assert cert_source is expected_cert_source
E AssertionError: assert None is <Mock id='140387160599120'>
tests/unit/gapic/credentials_v1/test_iam_credentials.py:706: AssertionError
This PR fixes a test failure in the generated goldens related to mTLS (mutual Transport Layer Security) configuration.
Problem
A recent update to the
google-authlibrary (specifically how it discovers certificates) introduced stricter checks usingos.path.exists(). Our generated tests were only mockingbuiltins.open()to simulate configuration files. As a result, the new existence checks failed during tests because the mocked files didn't actually exist on disk, causing assertions to fail.Solution
packages/gapic-generator/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2) to mock bothbuiltins.open()andos.path.exists()in the relevant test blocks.Notes to Reviewers
Fixes # (No specific issue reported, but addresses failures seen in CI/CD)