fix: bypass HTTP logging for websocket tunnels - #667
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
WalkthroughAdds shared WebSocket tunnel mode to TcpForwarder's bidirectional forwarding path. The forwarder now tracks upgrade state with an AtomicBool, switches both directions to direct byte forwarding once a WebSocket upgrade is detected, and updates early-cancel tests to pass the shared state. ChangesWebSocket Tunnel Mode
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![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)
crates/kftray-portforward/src/kube/tcp_forwarder.rs (1)
623-625: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
Connectionheader token matching is too strict.Some clients send
Connection: keep-alive, Upgrade, which lowercases toconnection: keep-alive, upgradeand fails the exact"connection: upgrade"substring check. The handshake still works via the response-side detection, but the request-side fast path won’t trigger. Consider matching anupgradetoken within theConnectionheader value rather than the fullconnection: upgradeliteral.🤖 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 `@crates/kftray-portforward/src/kube/tcp_forwarder.rs` around lines 623 - 625, The request-side WebSocket handshake check in tcp_forwarder should not require the exact "connection: upgrade" literal, since headers like "Connection: keep-alive, Upgrade" will miss the fast path. Update the header matching logic near the websocket upgrade condition to look for an Upgrade token within the Connection header value instead of a full-string match, while keeping the existing checks in the same handshake branch.
🤖 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 `@crates/kftray-portforward/src/kube/tcp_forwarder.rs`:
- Line 265: The two early-cancel test call sites in tcp_forwarder.rs still use
the पुराने forward_client_to_upstream and forward_upstream_to_client signatures;
update both calls to pass the websocket_tunnel_mode Arc<AtomicBool> argument so
they match the new function signature. Use the existing websocket_tunnel_mode
variable in the same test block and ensure both helper invocations receive it
consistently.
- Around line 293-300: The websocket tunnel flag is being enabled too early in
the request path inside the `TcpForwarder` flow, which can bypass the
response-side handshake confirmation. Move the
`websocket_tunnel_mode.store(true, ...)` transition out of the
`is_websocket_upgrade_request` branch and instead set it when
`HttpResponseAnalyzer::is_websocket_upgrade` confirms the upstream returned a
`101` response. Keep the upgrade request buffered and forwarded normally until
that response check succeeds so HTTP logging and non-upgrade failures still
follow the standard path.
---
Nitpick comments:
In `@crates/kftray-portforward/src/kube/tcp_forwarder.rs`:
- Around line 623-625: The request-side WebSocket handshake check in
tcp_forwarder should not require the exact "connection: upgrade" literal, since
headers like "Connection: keep-alive, Upgrade" will miss the fast path. Update
the header matching logic near the websocket upgrade condition to look for an
Upgrade token within the Connection header value instead of a full-string match,
while keeping the existing checks in the same handshake branch.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 399f4a0c-5014-47ba-9a7a-307e3514dcda
📒 Files selected for processing (1)
crates/kftray-portforward/src/kube/tcp_forwarder.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/kftray-portforward/src/kube/tcp_forwarder.rs (1)
434-443: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAccumulate bytes before checking for a websocket upgrade
is_websocket_upgrade(&buffer[..n])only sees the current read, so a101 Switching Protocolssplit across TCP packets won’t flipwebsocket_tunnel_mode. That leaves the response buffer growing on a long-lived websocket connection. Use the accumulated response bytes here, or defer the switch until the full header block is available.🤖 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 `@crates/kftray-portforward/src/kube/tcp_forwarder.rs` around lines 434 - 443, The websocket upgrade check in tcp_forwarder::handle_response logic only inspects the current read buffer, so split HTTP 101 headers can be missed; update the response handling around is_websocket_upgrade_response and websocket_tunnel_mode to evaluate accumulated response bytes (or wait until the full header block is available) before switching modes, while keeping response_state and should_log in sync.
🤖 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.
Outside diff comments:
In `@crates/kftray-portforward/src/kube/tcp_forwarder.rs`:
- Around line 434-443: The websocket upgrade check in
tcp_forwarder::handle_response logic only inspects the current read buffer, so
split HTTP 101 headers can be missed; update the response handling around
is_websocket_upgrade_response and websocket_tunnel_mode to evaluate accumulated
response bytes (or wait until the full header block is available) before
switching modes, while keeping response_state and should_log in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c062765d-46b9-4236-bfeb-2ab2a6516d74
📒 Files selected for processing (1)
crates/kftray-portforward/src/kube/tcp_forwarder.rs
|
@hcavarsan I am not that familiar with GitLab. I an the owner of @eugene-harold-krabs. How can I get the remaining pending checks rolling? I would like to complete the task. |
|
@hcavarsan Poke... Plz review... Thank you. |
3 participants
Summary
Fixes #666
Testing