feat(rest): integrate box network tunnel backend#992
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:
📝 WalkthroughWalkthroughThe PR adds a box-scoped network tunnel API, changes ChangesNetwork tunnel support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RestBox
participant ApiClient
participant BoxTunnel
participant UnixStream
RestBox->>ApiClient: describe_box_tunnel(box_id, port)
ApiClient-->>RestBox: Return public URL
RestBox->>ApiClient: connect_box_network_tunnel(box_id, port)
ApiClient->>UnixStream: Upgrade HTTP CONNECT
UnixStream-->>RestBox: Return prepared stream
RestBox->>BoxTunnel: Combine URL and stream
Possibly related PRs
🚥 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 |
5cecb35 to
6b39f16
Compare
|
|
c9ad161 to
bf8f74c
Compare
📦 BoxLite review — couldn't completepowered by BoxLite |
![boxlite-agent[bot]](https://avatars.githubusercontent.com/in/4244458?s=60&v=4){kind=small}
| fn try_clone(&self) -> BoxliteResult<Self> { | ||
| match self { | ||
| Self::Url(url) => Ok(Self::Url(url.clone())), | ||
| Self::Fd(fd) => fd | ||
| .try_clone() | ||
| .map(Self::Fd) | ||
| .map_err(|error| BoxliteError::Network(format!("clone local tunnel fd: {error}"))), | ||
| } | ||
| } |
There was a problem hiding this comment.
For a local box, BoxEndpoint::Fd wraps the actual connected tunnel socket; endpoint() dup(2)s it via try_clone while leaving the original fd in self.endpoint. A caller that calls endpoint() (e.g. to hand the fd to another language's socket layer, the documented into_fd FFI use case) and later also calls open_stream() gets two independent fds sharing one open file description — reads/writes on either steal bytes meant for the other, silently corrupting the tunnel. The Url variant has no such hazard since a String clone is inert; only Fd aliases a stateful, single-consumer resource. Prior code avoided this because endpoint() and connect() drained the same shared stream slot for local boxes rather than duplicating a live socket.
| #[async_trait] | ||
| impl BoxNetworkBackend for RestBox { | ||
| async fn tunnel(&self, target: SocketAddr) -> BoxliteResult<BoxTunnel> { | ||
| if target.ip().to_string() != crate::net::constants::GUEST_IP { | ||
| return Err(BoxliteError::Unsupported( | ||
| "REST box tunnels only support service ports on the guest IP".into(), | ||
| )); | ||
| } | ||
|
|
||
| let port = target.port(); | ||
| let box_id = self.box_id_str(); | ||
| let endpoint = self.client.describe_box_tunnel(&box_id, port).await?; | ||
| let stream = self | ||
| .client | ||
| .connect_box_network_tunnel(&box_id, port) | ||
| .await?; | ||
| Ok(BoxTunnel::new( | ||
| Some(BoxEndpoint::Url(endpoint)), | ||
| Some(crate::net::BoxInternalTunnel::from_local(stream, target)), |
There was a problem hiding this comment.
RestBox::tunnel unconditionally chains describe_box_tunnel (POST) then connect_box_network_tunnel (CONNECT). If the CONNECT leg fails (upstream box unreachable, 502 UpstreamUnavailableError) after the POST succeeded, tunnel() returns Err and the caller can no longer obtain just the public URL — a regression from the previous lazy design where fetching the endpoint and opening the stream were independent operations that could fail separately.
There was a problem hiding this comment.
Superseded by the lazy tunnel refactor in 24b72ad. tunnel() now prepares only a reusable endpoint: remote boxes retain the public URL and defer HTTP CONNECT until open_stream(), while local boxes expose a stable Unix socket whose listener opens a fresh gvproxy tunnel for each client connection. This restores independent endpoint discovery and connection failure while keeping local and remote SDK lifecycle semantics aligned.
![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
🤖 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 `@openapi/box.openapi.yaml`:
- Around line 873-874: Align the OpenAPI upstream failure responses, including
the additional occurrence, with the status and error type handled by the Rust
mapping in error.rs. Update the UpstreamUnavailableError contract and its
example so they use the same HTTP status and typed error classification
recognized by the Rust error mapping, preserving consistency in both layers.
In `@src/boxlite/src/rest/client.rs`:
- Around line 391-395: Update the rejected CONNECT branch in the REST client to
read the response body with the existing bounded-body mechanism and pass the
status and body through the shared error mapper instead of always constructing
BoxliteError::Network. Preserve the mapper’s established handling for HTTP
statuses and structured OpenAPI error responses, while retaining the current
success path for StatusCode::OK.
🪄 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: 9adf0df0-ee79-491e-b6f8-5c80f563255e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
openapi/box.openapi.yamlsrc/boxlite/Cargo.tomlsrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/mod.rssrc/boxlite/src/litebox/network.rssrc/boxlite/src/rest/client.rssrc/boxlite/src/rest/litebox.rssrc/boxlite/src/rest/runtime.rssrc/boxlite/src/runtime/backend.rs
| notify = "6.1" | ||
| proptest = "1.4" | ||
| toml = "0.8" | ||
| rustls = { version = "0.23", default-features = false, features = ["ring"] } |
There was a problem hiding this comment.
unified test build now compiles two rustls crypto providers (ring from this dev-dep, aws-lc-rs default via hyper-rustls); only the new connect_box_network_tunnel test installs a default provider, so any other TLS-using test that runs first in the same process (e.g. oci-client/reqwest tests) can hit rustls's 'no default CryptoProvider' error depending on test execution order.
| pub async fn endpoint(&self) -> BoxliteResult<BoxEndpoint> { | ||
| self.endpoint | ||
| .lock() | ||
| .await | ||
| .as_ref() | ||
| .ok_or_else(|| { | ||
| BoxliteError::InvalidState("tunnel endpoint has already been consumed".into()) | ||
| })? | ||
| .try_clone() | ||
| } | ||
|
|
||
| /// Consume the transport established by [`NetworkHandle::tunnel`]. | ||
| pub async fn open_stream(&self) -> BoxliteResult<Box<dyn BoxConnection>> { | ||
| let fd = { | ||
| let mut endpoint = self.endpoint.lock().await; | ||
| match endpoint.as_ref() { | ||
| Some(BoxEndpoint::Fd(_)) => match endpoint.take() { | ||
| Some(BoxEndpoint::Fd(fd)) => Some(fd), | ||
| _ => unreachable!("endpoint variant changed while locked"), | ||
| }, | ||
| _ => None, | ||
| } | ||
| }; | ||
| if let Some(fd) = fd { | ||
| let stream = std::os::unix::net::UnixStream::from(fd); |
There was a problem hiding this comment.
🧹 endpoint() dup's fd without consuming it
for a local (Fd) tunnel, calling endpoint() then open_stream() hands out two independent live fds to the same connected socket (dup via try_clone plus the original consumed by open_stream), unlike the Url path where stream/endpoint are cleanly separated; no in-repo caller does this today so unverified as reachable, but the API doesn't prevent it.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 329-339: Update the CONNECT authority construction near the host
extraction to bracket IPv6 literals before appending an explicit port, producing
[host]:port while preserving existing formatting for IPv4 and hostnames. Apply
this through the authority used to build the hyper::Uri.
🪄 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: 706d3c05-3269-4973-97dd-d181671ca9c1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
openapi/box.openapi.yamlsrc/boxlite/Cargo.tomlsrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/mod.rssrc/boxlite/src/litebox/network.rssrc/boxlite/src/rest/client.rssrc/boxlite/src/rest/litebox.rssrc/boxlite/src/rest/runtime.rssrc/boxlite/src/runtime/backend.rs
14531ef to
4e16d72
Compare
| 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() |
There was a problem hiding this comment.
connect_box_network_tunnel() constructs a fresh HttpsConnectorBuilder and loads native root certs (blocking filesystem reads via rustls_native_certs) inline on every tunnel() call instead of caching it once on ApiClient, adding blocking I/O on the async task and setup latency each time a box tunnel is opened; not perf-tested due to missing toolchain.
4e16d72 to
955de31
Compare
955de31 to
517cb3e
Compare
| let mut connector = HttpsConnectorBuilder::new() | ||
| .with_native_roots() |
There was a problem hiding this comment.
🛑 CONNECT tunnel panics: no default CryptoProvider installed
HttpsConnectorBuilder::with_native_roots() requires a process-wide rustls CryptoProvider; only the new unit test installs one (client.rs:690), so first real connect_box_network_tunnel call panics with 'no process-level CryptoProvider available'.
| let mut connector = HttpsConnectorBuilder::new() | |
| .with_native_roots() | |
| let mut connector = HttpsConnectorBuilder::new() | |
| .with_provider_and_native_roots(rustls::crypto::aws_lc_rs::default_provider().into()) | |
| .map_err(|e| BoxliteError::Config(format!("TLS roots unavailable: {e}")))? |
6776d87 to
24b72ad
Compare
a62f5cc to
d469ccc
Compare
d469ccc to
62ceea7
Compare
141135e to
2288521
Compare
1 participant
Integrate the REST network tunnel backend on top of the Rust core tunnel handle from PR991.
This PR also consolidates the tunnel endpoint work from PR993:
PR993 remains draft because its local endpoint work is now included here.
Test plan:
Dependency: this draft is intentionally stacked on PR991 and is not expected to compile independently before PR991 is merged.
Summary by CodeRabbit
CONNECT-based tunnel setup by port (with optional bearer auth).