fix(clients/python): add TLS/HTTPS support to dynamic connection strategies#1071
Open
chw120 wants to merge 2 commits into
Open
fix(clients/python): add TLS/HTTPS support to dynamic connection strategies#1071chw120 wants to merge 2 commits into
chw120 wants to merge 2 commits into
Conversation
The connector built every dynamic base URL with a literal http:// scheme, so Gateway-fronted TLS termination and in-cluster HTTPS sandboxes were unreachable. Direct connections already let callers pass https:// via api_url, but the three dynamic strategies (Gateway, InCluster, LocalTunnel) had no way to opt into TLS. This change: - Adds a scheme: Literal["http","https"] field to Gateway, InCluster, and LocalTunnel configs (defaults to "http" for backward compatibility) and a TLSConfig (ca_cert / insecure_skip_verify / server_name_override) on all four configs. Pydantic validators reject inconsistent combinations (scheme=http + tls, api_url=http://... + tls, ca_cert + insecure_skip_verify). - Centralizes URL construction in utils.build_base_url with IPv6 bracket handling and optional port omission for Gateway URLs (which use scheme defaults 80/443; server_port is forwarded as the X-Sandbox-Port router header instead). - Implements ca_cert as either inline PEM content or a file path (auto-detected via -----BEGIN header). For requests we materialize PEM to a temp file with lifecycle tied to SandboxConnector.close(); for httpx we build an SSLContext directly. - Wires server_name_override end-to-end: sync uses a custom HTTPAdapter that sets server_hostname/assert_hostname on the urllib3 pool; async uses the per-request sni_hostname extension. This is the missing piece for LocalTunnel + https (127.0.0.1 never matches the cert CN/SAN). - Emits a warning when Gateway or InCluster configs use plaintext http, pointing users at the new scheme/tls fields. LocalTunnel is exempt (loopback-only) and DirectConnection is exempt (scheme comes from the user-supplied URL). Sync (requests) and async (httpx) share build_ssl_context so the same TLSConfig produces equivalent verify posture in both. Previously the two clients diverged on whether ca_cert and insecure_skip_verify could combine. 31 new unit tests cover URL construction per scheme, TLSConfig validators, config-scheme consistency, ca_cert PEM-vs-path detection, strategy URL output per scheme (regression guard against the original bug), and the plaintext warning behavior. Existing 281 unit tests pass unchanged.
✅ Deploy Preview for agent-sandbox canceled.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chw120 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
the
cncf-cla: yes
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds TLS configuration support across the Python sandbox client. It introduces TLSConfig validation, shared URL/SSL helpers, sync and async connector wiring for HTTPS, custom CA, and SNI override handling, plus plaintext-HTTP warnings and new unit tests. ChangesTLS Configuration and Connector Wiring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
kubernetes-prow
Bot
added
the
size/XL
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@clients/python/agentic-sandbox-client/k8s_agent_sandbox/async_connector.py`:
- Around line 236-239: The async connector’s SNI override path only updates the
TLS extension and leaves the HTTP Host header unchanged, unlike the sync
sibling. In the async connector’s request-building logic around the existing
sni_hostname handling, also apply server_name_override to the Host header when
one was not already provided by the caller, keeping behavior aligned with the
sync connector’s parity.
In `@clients/python/agentic-sandbox-client/k8s_agent_sandbox/connector.py`:
- Around line 372-381: The sync TLS setup in connector.py only applies
server_name_override to SNI/verification, not the HTTP Host header. Update the
HTTPS mount path in the connector’s sync client setup to pass the override
through the same _TLSAdapter logic used by the async sibling, so requests send
the overridden Host value as well as SNI. Use the existing
TLSConfig.server_name_override, build_ssl_context, and _TLSAdapter wiring to
keep sync and async behavior aligned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d933e29-2790-4de4-8436-57b1b5cd9ed7
📒 Files selected for processing (5)
clients/python/agentic-sandbox-client/k8s_agent_sandbox/async_connector.pyclients/python/agentic-sandbox-client/k8s_agent_sandbox/connector.pyclients/python/agentic-sandbox-client/k8s_agent_sandbox/models.pyclients/python/agentic-sandbox-client/k8s_agent_sandbox/test/unit/test_tls_support.pyclients/python/agentic-sandbox-client/k8s_agent_sandbox/utils.py
There was a problem hiding this comment.
Pull request overview
This PR enhances the Python SDK’s connection layer by adding configurable http/https schemes and TLS configuration across dynamic connection strategies, removing the historical hardcoded http:// URL construction and aligning sync (requests) and async (httpx) trust behavior.
Changes:
- Introduces
TLSConfigand strategy-levelschemesupport (Gateway / InCluster / LocalTunnel), with Pydantic validation to prevent inconsistent TLS + scheme combinations. - Centralizes base URL construction via
build_base_url()(including IPv6 bracket handling) and adds shared TLS verification plumbing viabuild_ssl_context(). - Adds a new unit test suite to cover scheme/TLS validation and URL formatting behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| clients/python/agentic-sandbox-client/k8s_agent_sandbox/utils.py | Adds shared URL builder and TLS verification helpers (incl. requests session TLS helper). |
| clients/python/agentic-sandbox-client/k8s_agent_sandbox/models.py | Adds TLSConfig, adds scheme/tls fields to connection configs, and enforces consistency via validators. |
| clients/python/agentic-sandbox-client/k8s_agent_sandbox/connector.py | Updates sync connector/strategies to use centralized URL building and adds TLS adapter + plaintext warnings + temp CA cleanup. |
| clients/python/agentic-sandbox-client/k8s_agent_sandbox/async_connector.py | Updates async connector to use centralized URL building, shared TLS verify construction, and SNI override propagation. |
| clients/python/agentic-sandbox-client/k8s_agent_sandbox/test/unit/test_tls_support.py | Adds unit coverage for scheme/TLS validation, URL construction (incl. IPv6), and plaintext warning behavior. |
janetkuo
added
ready-for-review
and removed
action-required: resolve-copilot-comments
labels
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Historically, the Python SDK connector constructed all dynamic base URLs using a hardcoded
http://scheme, making Gateway-fronted TLS termination and in-cluster HTTPS sandboxes unreachable. While direct connections allowed callers to pass anhttps://scheme viaapi_url, the other three dynamic strategies (Gateway,InCluster, andLocalTunnel) had no capability to opt into TLS or configure secure connection contexts.To resolve Finding 10, this PR implements the following changes:
Configurable Schemes and TLS Validation:
scheme: Literal["http", "https"]field toGateway,InCluster, andLocalTunnelconfigs, defaulting to"http"for backwards compatibility.TLSConfig(supporting customca_cert,insecure_skip_verify, andserver_name_override) to all four configurations.scheme="http"with a TLS configuration, usinghttpson direct endpoints without a matching protocol, or passing mutually exclusive parameters like combining aca_certwithinsecure_skip_verify.Centralized URL Construction:
utils.build_base_urlto correctly handle IPv6 bracket wrapping and port rules.server_portis forwarded via theX-Sandbox-Portrouter header instead.Secure Certificate Materialization:
ca_certinput as either an absolute file path or inline PEM contents, auto-detected using a-----BEGINheader check.SandboxConnectorlifecycle, while async clients build anssl.SSLContextdirectly with the PEM string in memory.E2E Server Name Indication (SNI) Overrides:
HTTPAdapterto injectserver_hostnameandassert_hostnameoverrides into the underlyingurllib3pool.httpxvia the per-requestsni_hostnameextension. This is essential for setups likeLocalTunnelover HTTPS where loopback (127.0.0.1) does not match the server certificate common name or SAN.Plaintext Warnings:
http://configurations withGatewayorInClusterconnection schemes.Shared Trust Posture:
build_ssl_contextlogic, ensuring consistent certificate validation posture across both clients.Verification / Test Plan
A comprehensive suite of 31 new unit tests has been implemented inside
test_tls_support.py. These tests guard against:http://schemes.All 281 pre-existing tests pass cleanly and without regression.
Release Note
Summary by CodeRabbit
New Features
Bug Fixes
Tests