feat(rest): integrate box network tunnel backend#992
Conversation
|
Caution Review failedFailed to post review comments. GitHub was unavailable or timed out while CodeRabbit was posting the review. Please request a new review later if the pull request still needs one. This happened while posting 2 inline comments. Use ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
⏰ Context from checks skipped due to timeout. (1)
📝 WalkthroughWalkthroughChangesThe PR adds box-scoped network tunnel API contracts, introduces prepared URL and file-descriptor tunnel endpoints, and enables REST-backed authenticated HTTP CONNECT streams for REST-created boxes. Network tunnel support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ApiClient
participant RestBox
participant BoxTunnel
participant UnixStream
RestBox->>ApiClient: Describe box tunnel
ApiClient-->>RestBox: Return public URL
RestBox->>ApiClient: Open HTTP CONNECT tunnel
ApiClient->>UnixStream: Upgrade connection
UnixStream-->>RestBox: Return prepared stream
RestBox->>BoxTunnel: Combine URL and stream
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 oasdiff (1.23.0)openapi/box.openapi.yamlError: failed to load base spec from "/tmp/coderabbit-oasdiff-base.cYqtcY": map key "ValidationError" not found 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 |
G4614
force-pushed
the
codex/rust-network-rest-standalone
branch
from
5cecb35 to
6b39f16
Compare
|
Gamnaam Song seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
G4614
force-pushed
the
codex/rust-network-rest-standalone
branch
from
c9ad161 to
bf8f74c
Compare
📦 BoxLite review — 2 issues ·
|
![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.
![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
| "502": | ||
| $ref: "#/components/responses/UpstreamUnavailableError" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the upstream failure contract with the Rust error mapping.
The OpenAPI contract returns HTTP 502 with example type NetworkError, while src/boxlite/src/rest/error.rs Lines 148-153 recognizes this code only as HTTP 503/UpstreamUnavailableError. A contract-compliant 502 response will bypass the typed mapping. Make the status and type consistent across both layers.
Also applies to: 1211-1222
🤖 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 `@openapi/box.openapi.yaml` around lines 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.
| if response.status() != hyper::StatusCode::OK { | ||
| return Err(BoxliteError::Network(format!( | ||
| "CONNECT request rejected with status {}", | ||
| response.status() | ||
| ))); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve REST error semantics for rejected CONNECT requests.
Every rejection becomes BoxliteError::Network, so responses such as 401/403/404 and structured OpenAPI errors lose their established mappings. Read the bounded response body and route the status/body through the shared error mapper before returning.
🤖 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/rest/client.rs` around lines 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.
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
New Features
Improvements