feat: basic functionality - #1
Conversation
3320508 to
4d3cbdd
Compare
skomski
left a comment
There was a problem hiding this comment.
Reviewed the WebSocket↔DNS translation. The in-place approach is sound: both sides are message-oriented, and the write cursor always trails the read cursor — pong output lands at least 4 bytes behind its source, and the reserved prefix area in dns_to_ws works out exactly (512 messages × 2 bytes = the 1024-byte offset; tight, but correct). Everything below is framing arithmetic and buffer sizing around that approach, not the approach itself.
Two are confirmed against the built binary rather than inferred:
- DNS messages larger than 65531 bytes are silently dropped (
src/main.rsline 189) - A query pipelined ahead of a Close frame never reaches upstream (
src/translate.rsline 143)
Two more are latent and share a root cause with the first: "buffer full" and "peer closed" are currently the same signal (src/main.rs line 197), so a frame that cannot fit turns into a clean-looking close instead of an error.
I have both bugs reproduced as integration tests and all four fixes working locally — all 34 tests green, clippy and fmt clean. Happy to push that if useful.
Smaller things, not worth their own threads:
- The proxy always answers with code 1000 regardless of the client's close code; RFC 6455 §5.5.1 suggests echoing it.
- No idle or read timeout anywhere. Each WebSocket connection pins an upstream TCP socket for its lifetime, and there is no cap on concurrent connections. RFC 7766 §6.2.3 expects timeouts on DNS-over-TCP. This gets more relevant if the half-close drain described on
src/translate.rsline 143 lands, since that drain waits on upstream EOF. - A zero-length binary message is forwarded upstream as a 0-length DNS message rather than rejected.
- Non-minimal length encodings are accepted (a 5-byte payload sent with the 126 extended form, say). Harmless here, just noting the leniency.
| let mut dns_pos = 0; | ||
|
|
||
| let code = loop { | ||
| let size @ 1.. = sock_r.read(&mut ws_buf[ws_fill..]).await? else { |
There was a problem hiding this comment.
A full buffer makes read return Ok(0), which is indistinguishable from EOF
When ws_fill == BUF_SIZE, &mut ws_buf[ws_fill..] is a zero-length slice, so read returns Ok(0) without touching the socket. The 1.. pattern fails and the loop breaks with 1000 — the same branch that handles a real EOF.
Progress depends entirely on ws_to_dns consuming bytes. When it returns ws_pos == 0 because a frame is incomplete, lines 216-217 shift nothing and ws_fill only grows. A frame that can never fit therefore becomes a silent close rather than an error. That is the mechanism behind both the >65531-byte drop (line 189) and the unreachable 1009 (src/translate.rs line 164).
Worth separating the two conditions even after the buffer is resized, so a future sizing mistake surfaces as a protocol error instead of a silent drop:
| let size @ 1.. = sock_r.read(&mut ws_buf[ws_fill..]).await? else { | |
| // A full buffer would make `read` return `Ok(0)` without touching the | |
| // socket, which is indistinguishable from EOF. `ws_buf` holds the | |
| // largest legal frame, so this only triggers on input that can never | |
| // be framed. | |
| let Some(space @ [_, ..]) = ws_buf.get_mut(ws_fill..) else { | |
| break 1009; | |
| }; | |
| let size @ 1.. = sock_r.read(space).await? else { |
forward_responses on line 157 has the same shape but cannot reach it: the largest unprocessed trailer is 65536 bytes copied back to offset 1024, so dns_fill peaks at 66560 against a BUF_SIZE of 66561. One byte of slack — correct, but with no margin.
There was a problem hiding this comment.
This condition (0 bytes free buffer space) should never happen. If it unexpectedly happens due to a bug, treating it as EOF is not catastrophic (as looping forever would be, for example). Therefore, I don't see any reasons to improve this.
| let old_ws_pos = ws_pos; | ||
| ws_pos = data_range.end; | ||
| if minimal_header[0] == MASKED | CLOSE { | ||
| return Err(1000); |
There was a problem hiding this comment.
Err(1000) discards queries already translated in the same call
Err throws away the dns_data_complete and pong_pos accumulated earlier in this call, and forward_requests breaks before reaching its up_w.write_all. A client that pipelines a query and a Close into one TCP segment gets no answer — the query never reaches upstream.
Verified end-to-end: a 12-byte query frame followed by a Close frame in a single write_all comes back as only a Close frame, no DNS response.
Nothing makes that client behaviour illegal. It has stopped sending, but the proxy may keep sending until it emits its own Close, so the answer is still deliverable.
Two pieces are needed:
1. Report the close code alongside the state instead of through Err, so the caller can flush what was already translated:
close_code = Some(1000);
break;with the signature becoming Result<(usize, usize, usize, usize, Option<u16>), u16> and the tail returning Ok((dns_data_complete, dns_data_pos, pong_pos, ws_pos, close_code)).
2. That alone is not sufficient — I tried it, and the query does reach upstream but the answer still never arrives. forward_requests takes sock_w, writes Close, and returns Err, which makes the try_join! in handle_connection drop forward_responses before the response comes back. It needs half-close semantics instead:
if close_code.is_some() {
// The client is done sending, but queries forwarded just above may
// still be unanswered. Half-close upstream so it sees the end of the
// query stream, and leave the closing handshake to `forward_responses`.
up_w.shutdown().await?;
return Ok(());
}With both pieces plus the src/main.rs line 189 fix, a regression test for this passes.
One caveat: the drain then waits on upstream EOF, and there are no timeouts anywhere in the proxy. Not a regression — an idle client can already hold a connection open indefinitely — but it does make the missing timeouts more visible.
There was a problem hiding this comment.
"Nothing makes that client behaviour illegal."
Yes, but there is also nothing that prevents the server from cancelling outstanding requests when the client closes the connection. The client should simply keep the connection open until it receives all responses, if the client is still interested in them.
One caveat
This "one caveat" is also against RFC 6455 (The WebSocket Protocol), which says that about sending the Close response: "It SHOULD do so as soon as practical." This whole problem can be solved by cancelling outstanding requests on close (as the code currently does). By the way, timeouts can be configured in the reverse proxy if necessary.
140b707 to
76b3203
Compare
No description provided.