fix(auth): use requests transport for GCE MDS#16480
fix(auth): use requests transport for GCE MDS#16480daniel-sanche wants to merge 4 commits intomainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request replaces the internal _http_client transport with the requests transport in the _get_gce_credentials function and includes a minor string formatting fix. Feedback suggests that the switch to requests may require a minor version bump due to changes in proxy handling and recommends adding type hints to avoid mypy errors. Additionally, it is suggested to enhance the new unit test by asserting the function's return values.
| # While this library is normally bundled with compute_engine, there are | ||
| # some cases where it's not available, so we tolerate ImportError. | ||
| # Compute Engine requires optional `requests` dependency. |
There was a problem hiding this comment.
The switch to requests introduces support for SSL and proxies, which is a change in behavior as environment proxy settings (like HTTP_PROXY) will now be respected. This might require users to update their NO_PROXY list. Since this is a change in environment requirements and behavior, a minor version bump should be preferred over a patch version bump. Additionally, when handling the optional requests dependency by assigning None in the except ImportError block, include # type: ignore[assignment] to prevent mypy errors.
# While this library is normally bundled with compute_engine, there are
# some cases where it's not available, so we tolerate ImportError.
# Compute Engine requires the optional requests dependency. We use it
# to support HTTPS/mTLS; note that requests respects environment
# proxies (e.g. HTTP_PROXY). This change warrants a minor version bump.
requests = None # type: ignore[assignment]References
- When handling an optional dependency by assigning None in an except ImportError block, type: ignore[assignment] is necessary on the assignment to prevent a mypy error.
- When a release introduces breaking changes in environment requirements or behavior, prefer a minor version bump over a patch version bump.
| def test__get_gce_credentials_default_request(mock_request_cls, ping): | ||
| _default._get_gce_credentials() | ||
| mock_request_cls.assert_called_once() | ||
| ping.assert_called_with(request=mock_request_cls.return_value) |
There was a problem hiding this comment.
The test verifies that the requests.Request object is created and passed to the GCE check, but it doesn't verify the return value of the function. Adding assertions for the return value (which should be None, None since is_on_gce is mocked to return False) would make the test more robust.
| def test__get_gce_credentials_default_request(mock_request_cls, ping): | |
| _default._get_gce_credentials() | |
| mock_request_cls.assert_called_once() | |
| ping.assert_called_with(request=mock_request_cls.return_value) | |
| def test__get_gce_credentials_default_request(mock_request_cls, ping): | |
| credentials, project_id = _default._get_gce_credentials() | |
| mock_request_cls.assert_called_once() | |
| ping.assert_called_with(request=mock_request_cls.return_value) | |
| assert credentials is None | |
| assert project_id is None |
Fixes #16090
We were seeing errors on GCE environments, because the library would use _http_client to communicate to the metadata server, even when mTLS is enabled. This resulted in failures, because _http_client doesn't support https.
This PR addresses the issue by using requests as the transport instead. This depends on the optional
requestsdependency, but requests is already required by compute engine, so there are no changes there.I was able to reproduce and verify this test on a GCE VM