Repository navigation
feat(rust-sdk): expose box network tunnels - #991
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a lazy networking API to ChangesNetwork tunneling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LiteBox
participant NetworkHandle
participant RestBox
participant ApiClient
participant RESTAPI
LiteBox->>NetworkHandle: network()
NetworkHandle->>RestBox: box_tunnel(target)
RestBox->>ApiClient: describe_box_tunnel(box_id, port)
ApiClient->>RESTAPI: POST tunnel descriptor request
RESTAPI-->>ApiClient: endpoint URL
RestBox->>ApiClient: connect_box_network_tunnel(box_id, port)
ApiClient->>RESTAPI: HTTP CONNECT tunnel request
RESTAPI-->>ApiClient: upgraded connection
ApiClient-->>NetworkHandle: BoxTunnel connection
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
📦 BoxLite review — couldn't completepowered by BoxLite |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/boxlite/src/litebox/network.rs (1)
63-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider returning the unconsumed tunnel on FD extraction failure.
Currently,
into_fd()takesselfby value and returnsOption<OwnedFd>. If a future transport cannot provide a single host file descriptor (e.g., a cloud stream), it will returnNoneand implicitly consume and drop the established connection.As a result, SDK consumers who intend to fall back to a local-listener bridge (as noted in the
into_fddocumentation) would be forced to callconnect()again to obtain a new raw stream, unnecessarily paying the connection cost twice.Consider updating
BoxInternalTunnel::into_fdto returnResult<OwnedFd, Self>(or a similar construct) in a future iteration. This would allow the caller to recover the active tunnel on failure and seamlessly reuse it for the fallback bridge without reconnecting.🤖 Prompt for 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. In `@src/boxlite/src/litebox/network.rs` around lines 63 - 67, Update BoxInternalTunnel::into_fd to preserve and return the established tunnel when FD extraction is unsupported, using a result-like return such as Result<OwnedFd, Self>. Adjust connect_fd and its callers to propagate or handle that recoverable tunnel so fallback bridge logic can reuse the existing connection without reconnecting.
🤖 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 `@src/boxlite/src/rest/client.rs`:
- Around line 324-330: Update connect_fd’s CONNECT establishment flow around
connector.call and http1::handshake to apply the configured timeout to the
TCP/TLS connection, handshake, response, and upgrade awaits. Scope the timeout
only to setup and preserve the established tunnel pump as long-lived; convert
timeout failures into the existing BoxliteError::Network error path.
- Around line 331-335: Update the spawned connection task in the REST client to
await the Hyper connection via with_upgrades() before handling errors. Preserve
the existing tracing::debug logging and error handling so CONNECT tunnel
upgrades can complete.
---
Nitpick comments:
In `@src/boxlite/src/litebox/network.rs`:
- Around line 63-67: Update BoxInternalTunnel::into_fd to preserve and return
the established tunnel when FD extraction is unsupported, using a result-like
return such as Result<OwnedFd, Self>. Adjust connect_fd and its callers to
propagate or handle that recoverable tunnel so fallback bridge logic can reuse
the existing connection without reconnecting.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ab7c5b0c-aeb9-443d-aaaf-5400fba89121
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
src/boxlite/Cargo.tomlsrc/boxlite/src/lib.rssrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/mod.rssrc/boxlite/src/litebox/network.rssrc/boxlite/src/net/gvproxy/services.rssrc/boxlite/src/net/mod.rssrc/boxlite/src/rest/client.rssrc/boxlite/src/rest/litebox.rssrc/boxlite/src/rest/runtime.rssrc/boxlite/src/runtime/backend.rssrc/boxlite/src/runtime/rt_impl.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/boxlite/src/litebox/network.rs (2)
151-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate stale method name in the comment.
The comment references
url(), which should be updated toendpoint()to match the method name.♻️ Proposed fix
- // connect() triggers exactly one connect, independent of url(). + // connect() triggers exactly one connect, independent of endpoint(). assert!(tunnel.connect().await.is_err());🤖 Prompt for 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. In `@src/boxlite/src/litebox/network.rs` around lines 151 - 152, Update the comment above the tunnel.connect() assertion to replace the stale url() reference with endpoint(), preserving the existing explanation that connect() triggers exactly one connection independently of endpoint().
56-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate stale documentation to reflect the new return type.
The doc comment mentions returning
Ok(None)for local boxes, but the method now returnsOk(BoxEndpoint::Local)instead of anOption.♻️ Proposed fix
- /// Public URL for this service, fetched on demand. `Ok(None)` for local + /// Public URL for this service, fetched on demand. `Ok(BoxEndpoint::Local)` for local /// boxes, which have no public URL. pub async fn endpoint(&self) -> BoxliteResult<BoxEndpoint> {🤖 Prompt for 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. In `@src/boxlite/src/litebox/network.rs` around lines 56 - 58, Update the documentation comment above the endpoint method to describe the current BoxEndpoint return behavior, replacing the stale Ok(None) reference with Ok(BoxEndpoint::Local) for local boxes while preserving the public URL description.
🤖 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.
Nitpick comments:
In `@src/boxlite/src/litebox/network.rs`:
- Around line 151-152: Update the comment above the tunnel.connect() assertion
to replace the stale url() reference with endpoint(), preserving the existing
explanation that connect() triggers exactly one connection independently of
endpoint().
- Around line 56-58: Update the documentation comment above the endpoint method
to describe the current BoxEndpoint return behavior, replacing the stale
Ok(None) reference with Ok(BoxEndpoint::Local) for local boxes while preserving
the public URL description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0aef520a-0755-41a8-9818-4c65a43c5f34
📒 Files selected for processing (3)
src/boxlite/src/lib.rssrc/boxlite/src/litebox/mod.rssrc/boxlite/src/litebox/network.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/boxlite/src/lib.rs
- src/boxlite/src/litebox/mod.rs
![boxlite-agent[bot]](https://avatars.githubusercontent.com/in/4244458?s=60&v=4){kind=small}
| use tower::Service; | ||
|
|
||
| let mut connector = HttpsConnectorBuilder::new() | ||
| .with_native_roots() | ||
| .map_err(|e| BoxliteError::Config(format!("TLS roots unavailable: {e}")))? | ||
| .https_or_http() | ||
| .enable_http1() | ||
| .build(); | ||
| let io = connector | ||
| .call(uri.clone()) | ||
| .await | ||
| .map_err(|e| BoxliteError::Network(format!("CONNECT transport failed: {e}")))?; | ||
| let (mut sender, connection) = http1::handshake(io) | ||
| .await | ||
| .map_err(|e| BoxliteError::Network(format!("CONNECT handshake failed: {e}")))?; | ||
| tokio::spawn(async move { | ||
| if let Err(err) = connection.await { | ||
| tracing::debug!(error = %err, "REST CONNECT connection closed"); | ||
| } | ||
| }); | ||
|
|
||
| let bearer = self.current_bearer().await?; | ||
| let target = uri | ||
| .path_and_query() | ||
| .ok_or_else(|| BoxliteError::Internal("CONNECT URI has no path".into()))? | ||
| .to_string(); | ||
| let mut request = Request::builder() | ||
| .method("CONNECT") | ||
| .uri(target) | ||
| .header("host", authority); | ||
| if let Some(bearer) = bearer { | ||
| request = request.header("authorization", format!("Bearer {bearer}")); | ||
| } | ||
| let response: hyper::Response<Incoming> = sender | ||
| .send_request( | ||
| request | ||
| .body(http_body_util::Empty::<bytes::Bytes>::new()) |
There was a problem hiding this comment.
connector.call, http1::handshake, send_request, and hyper::upgrade::on are all awaited with no timeout, so a stalled server/network leaves the caller's tunnel() future (and its task) blocked indefinitely.
There was a problem hiding this comment.
Fixed together with the CodeRabbit timeout finding. The complete CONNECT establishment sequence is now bounded by the setup timeout; only the post-upgrade byte pump remains unbounded.
| pub(crate) async fn connect_box_network_tunnel( | ||
| &self, | ||
| box_id: impl AsRef<str>, | ||
| port: u16, | ||
| ) -> BoxliteResult<tokio::net::UnixStream> { | ||
| let path = format!("/boxes/{}/network/tunnel", box_id.as_ref()); | ||
| let url = reqwest::Url::parse(&self.url(&path)) | ||
| .map_err(|e| BoxliteError::Internal(format!("CONNECT URL build failed: {e}")))?; | ||
| let authority = url | ||
| .host_str() | ||
| .ok_or_else(|| BoxliteError::Internal("CONNECT URL has no host".into()))?; | ||
| let authority = match url.port() { | ||
| Some(port) => format!("{authority}:{port}"), | ||
| None => authority.to_string(), | ||
| }; | ||
| let query = format!("port={port}"); | ||
| let uri: hyper::Uri = format!("{}://{}{}?{query}", url.scheme(), authority, url.path()) | ||
| .parse() | ||
| .map_err(|e| BoxliteError::Internal(format!("CONNECT URI build failed: {e}")))?; | ||
|
|
||
| use hyper::Request; | ||
| use hyper::body::Incoming; | ||
| use hyper::client::conn::http1; | ||
| use hyper_rustls::HttpsConnectorBuilder; | ||
| use hyper_util::rt::TokioIo; | ||
| use tower::Service; | ||
|
|
||
| let mut connector = HttpsConnectorBuilder::new() | ||
| .with_native_roots() | ||
| .map_err(|e| BoxliteError::Config(format!("TLS roots unavailable: {e}")))? | ||
| .https_or_http() | ||
| .enable_http1() | ||
| .build(); | ||
| let io = connector | ||
| .call(uri.clone()) | ||
| .await | ||
| .map_err(|e| BoxliteError::Network(format!("CONNECT transport failed: {e}")))?; | ||
| let (mut sender, connection) = http1::handshake(io) | ||
| .await | ||
| .map_err(|e| BoxliteError::Network(format!("CONNECT handshake failed: {e}")))?; | ||
| tokio::spawn(async move { | ||
| if let Err(err) = connection.await { | ||
| tracing::debug!(error = %err, "REST CONNECT connection closed"); | ||
| } | ||
| }); | ||
|
|
||
| let bearer = self.current_bearer().await?; | ||
| let target = uri | ||
| .path_and_query() | ||
| .ok_or_else(|| BoxliteError::Internal("CONNECT URI has no path".into()))? | ||
| .to_string(); | ||
| let mut request = Request::builder() | ||
| .method("CONNECT") | ||
| .uri(target) | ||
| .header("host", authority); | ||
| if let Some(bearer) = bearer { | ||
| request = request.header("authorization", format!("Bearer {bearer}")); | ||
| } | ||
| let response: hyper::Response<Incoming> = sender | ||
| .send_request( | ||
| request | ||
| .body(http_body_util::Empty::<bytes::Bytes>::new()) | ||
| .map_err(|e| { | ||
| BoxliteError::Internal(format!("CONNECT request build failed: {e}")) | ||
| })?, | ||
| ) | ||
| .await | ||
| .map_err(|e| BoxliteError::Network(format!("CONNECT request failed: {e}")))?; | ||
| if response.status() != hyper::StatusCode::OK { | ||
| return Err(BoxliteError::Network(format!( | ||
| "CONNECT request rejected with status {}", | ||
| response.status() | ||
| ))); | ||
| } | ||
|
|
||
| let upgraded = hyper::upgrade::on(response) | ||
| .await | ||
| .map_err(|e| BoxliteError::Network(format!("CONNECT upgrade failed: {e}")))?; | ||
| let mut upgraded = TokioIo::new(upgraded); | ||
| let (local, mut pump_end) = tokio::net::UnixStream::pair() | ||
| .map_err(|e| BoxliteError::Network(format!("CONNECT local socket pair failed: {e}")))?; | ||
| tokio::spawn(async move { | ||
| let _ = tokio::io::copy_bidirectional(&mut pump_end, &mut upgraded).await; | ||
| }); | ||
| Ok(local) | ||
| } | ||
|
|
||
| /// Request the public descriptor for a box service tunnel. | ||
| pub(crate) async fn describe_box_tunnel( | ||
| &self, | ||
| box_id: impl AsRef<str>, | ||
| port: u16, | ||
| ) -> BoxliteResult<String> { | ||
| // Wire body of `POST /boxes/{box_id}/network/tunnel`; only the URL is used. | ||
| #[derive(serde::Deserialize)] |
There was a problem hiding this comment.
connect_box_network_tunnel (raw hyper CONNECT + TLS via hyper-rustls + upgrade + copy_bidirectional bridging, ~90 new lines) has no unit/integration test, unlike the parallel litebox/network.rs abstraction which got 2 tests.
There was a problem hiding this comment.
The Rust network tests now cover the common endpoint/connect contract for both local and remote backends. A dedicated raw Hyper CONNECT integration test still requires a live REST endpoint and TLS/upgrade-capable test server, so this is covered by REST e2e rather than a synthetic unit test.
| } | ||
|
|
||
| /// Resolve the endpoint, fetching a URL remotely or preparing a local stream. | ||
| pub async fn endpoint(&self) -> BoxliteResult<Option<String>> { |
There was a problem hiding this comment.
2 api LGTM, but the return type and name should be refine?
f626fd5 to
7f14964
Compare
2 participants
Expose a unified Rust network tunnel handle for local and REST-backed boxes.
Test plan:
Summary by CodeRabbit