feat(requests-ca): respect REQUESTS_CA_BUNDLE environment variable #755
Conversation
Current implementation of `HttpClient` uses requests library to create a `PreparedRequest` and send it, but do not take into consideration the environment variables that `requests` do. This make it impossible to use custom CA certificates together with Airbyte. This PR adds the support of env settings to `HttpClient`. The implementation is doing what `requests` library recommends when working with `PreparedRequests` to properly handle self-signed certificates: https://requests.readthedocs.io/en/latest/user/advanced/#prepared-requests
📝 WalkthroughWalkthroughHttpClient.send_request now merges requests.Session environment settings into per-request kwargs before retry handling. A unit test asserts that REQUESTS_CA_BUNDLE from the environment is propagated as the verify parameter to the internal _send_with_retry call. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant HC as HttpClient
participant RS as requests.Session
participant Retry as _send_with_retry
Caller->>HC: send_request(method, url, request_kwargs)
HC->>RS: prepare_request(Request(...))
RS-->>HC: PreparedRequest
HC->>RS: merge_environment_settings(url, None, None, None, None)
RS-->>HC: env_settings {proxies, verify, cert, ...}
Note over HC: Merge env_settings into request_kwargs
HC->>Retry: _send_with_retry(prepared_req, request_kwargs+env)
Retry-->>HC: Response
HC-->>Caller: Response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Would you like to also add a test for proxies or client certificates from merge_environment_settings to cover more environment-driven cases, wdyt? Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
unit_tests/sources/streams/http/test_http_client.py (2)
4-4: Remove unused import to keep linters happy.Looks like os isn’t used elsewhere in this file (the patch.dict decorator resolves the module). Shall we drop it, wdyt?
-import os
749-767: Add override-precedence test and fix formatting with Ruff.Could we add a sibling test confirming that an explicit
request_kwargs["verify"]overridesREQUESTS_CA_BUNDLE? For example:@patch.dict("os.environ", {"REQUESTS_CA_BUNDLE": "/env/ca-bundle.crt"}) def test_send_request_explicit_verify_overrides_env(): http_client = HttpClient(name="test", logger=MagicMock()) with patch.object(http_client, "_send_with_retry") as mock_send: http_client.send_request( http_method="GET", url="https://api.example.com", request_kwargs={"timeout": 10, "verify": False}, ) passed_kwargs = mock_send.call_args[1]["request_kwargs"] assert passed_kwargs["verify"] is FalseThe CI failure is due to Ruff formatting—running
ruff --fix .should resolve it, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/streams/http/http_client.py(1 hunks)unit_tests/sources/streams/http/test_http_client.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
airbyte_cdk/sources/streams/http/http_client.py (1)
airbyte_cdk/sources/streams/http/http.py (1)
request_kwargs(242-253)
unit_tests/sources/streams/http/test_http_client.py (1)
airbyte_cdk/sources/streams/http/http_client.py (2)
name(518-519)send_request(521-558)
🪛 GitHub Actions: Linters
unit_tests/sources/streams/http/test_http_client.py
[error] 746-746: Ruff format --diff detected formatting changes. 1 file would be reformatted (unit_tests/sources/streams/http/test_http_client.py). Exit code 1. Command: 'poetry run ruff format --diff .'
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Manifest Server Docker Image Build
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (1)
airbyte_cdk/sources/streams/http/http_client.py (1)
548-550: In http_client.py, let per-request kwargs override environment settings, wdyt?- env_settings = self._session.merge_environment_settings(request.url, None, None, None, None) - request_kwargs = {**request_kwargs, **env_settings} + env_settings = self._session.merge_environment_settings( + request.url, + request_kwargs.get("proxies"), + request_kwargs.get("stream"), + request_kwargs.get("verify"), + request_kwargs.get("cert"), + ) + # Environment defaults first; explicit per-request kwargs take precedence. + request_kwargs = {**env_settings, **request_kwargs}Could you confirm no existing call sites depend on the current merge order for keys like
verifyorproxies?
|
/autofix |
|
/autofix
|
|
/test
|
| data=data, | ||
| ) | ||
|
|
||
| env_settings = self._session.merge_environment_settings(request.url, None, None, None, None) |
There was a problem hiding this comment.
|
@bosper351 - Thank you for this contribution! 🙏 Can you check on these two pytest failures? |
|
/autofix |
|
|
||
| list(stream.read_records(sync_mode=SyncMode.full_refresh)) | ||
|
|
||
| stream._http_client._session.send.assert_any_call(ANY, **request_kwargs) |
There was a problem hiding this comment.
proxies dict is converted to OrderedDict somewhere under the hood. This plain assertion fails due to type mismatch between a dict and OrderedDict. To avoid this mismatch, each key of proxies is compared separately.
|
@aaronsteers it's resolved. Please check out when you have a moment. |
|
/test
|
Current implementation of
HttpClientusesrequestslibrary to create aPreparedRequestand send it, but do not take into consideration the environment variables thatrequestsdo. This make it impossible to use custom CA certificates together with Airbyte. This PR adds the support of env settings toHttpClient.The implementation is doing what
requestslibrary recommends when working withPreparedRequeststo properly handle self-signed certificates: https://requests.readthedocs.io/en/latest/user/advanced/#prepared-requestsSummary by CodeRabbit
Bug Fixes
Tests