Add TCP support to DNS server and client #27
Conversation
- Updated README to reflect TCP support on port 5300 - Modified docker-compose to expose TCP port 5300 - Enhanced Native adapter to handle TCP connections - Updated Swoole adapter for TCP support - Improved Client class to support TCP queries - Added unit test for TCP queries in ClientTest
WalkthroughAdds TCP DNS support across the stack: Native adapter now multiplexes UDP and TCP with per-client buffering, framing, and TCP lifecycle handling; Swoole adapter adds a TCP receive path and uses a UDP Port listener; DNS client gains optional TCP query flow, framing helpers, and response decoding; docker-compose exposes TCP port 5300 in addition to UDP; README sample Zone records replaced with descriptive prose; new end-to-end TCP client test added; minor formatting tweaks in the DNS validator. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
40-54: The code example is malformed and will not execute.The quick start example has corrupted content. Line 40 contains prose text inside what should be a PHP array of
Recordobjects, and lines 41-45 have orphaned PHP fragments. Additionally, there's now duplicate/conflicting text about port behavior (line 40 mentions "UDP and TCP port 5300" while line 54 mentions only "UDP port 5300").The intended structure should be something like:
$zone = new Zone( name: 'example.test', records: [ new Record(name: 'example.test', type: Record::TYPE_A, rdata: '192.0.2.1', ttl: 60), new Record(name: 'www.example.test', type: Record::TYPE_CNAME, rdata: 'example.test', ttl: 60), new Record( name: 'example.test', type: Record::TYPE_SOA, rdata: 'ns1.example.test hostmaster.example.test 1 7200 1800 1209600 3600', ttl: 60 ), ] );Please also update line 54 to reflect that the server now listens on both UDP and TCP.
src/DNS/Client.php (1)
53-79: Add null check to satisfy static analysis.The pipeline reports that
$this->socketmay benullwhen passed tosocket_sendtoandsocket_recvfrom. While this path is only reached whenuseTcpis false (where$socketis initialized), static analysis cannot infer this.🔎 Proposed fix
public function query(Message $message): Message { if ($this->useTcp) { return $this->queryTcp($message); } + if ($this->socket === null) { + throw new Exception('UDP socket not initialized.'); + } + $packet = $message->encode(); if (socket_sendto($this->socket, $packet, strlen($packet), 0, $this->server, $this->port) === false) {
🧹 Nitpick comments (2)
tests/e2e/DNS/ClientTest.php (1)
15-28: Consider adding more assertions for parity with UDP tests.The test validates TCP connectivity and basic response structure but is less thorough than the UDP counterpart (
testARecords) which verifiesttl,class, andrdatavalues for the same domain. Consider adding similar assertions for consistency:$this->assertCount(2, $records); $this->assertSame('dev2.appwrite.io', $records[0]->name); $this->assertSame(Record::TYPE_A, $records[0]->type); +$this->assertSame(Record::CLASS_IN, $records[0]->class); +$this->assertSame(1800, $records[0]->ttl); +$this->assertSame('142.6.0.1', $records[0]->rdata); +$this->assertSame('142.6.0.2', $records[1]->rdata);src/DNS/Adapter/Native.php (1)
198-209: Consider adding a maximum payload length check.The code trusts the 2-byte length prefix from clients. A malicious client could send a large length value (up to 65535), causing the server to buffer significant data. While DNS messages are typically small, adding a sanity check would be defensive.
🔎 Suggested improvement
while (strlen($this->tcpBuffers[$clientId]) >= 2) { $length = unpack('nlen', substr($this->tcpBuffers[$clientId], 0, 2)); $payloadLength = $length['len'] ?? 0; if ($payloadLength === 0) { $this->closeTcpClient($client); return; } + // DNS messages are limited to ~512 bytes for UDP, but TCP allows larger. + // Cap at a reasonable limit to prevent memory exhaustion. + if ($payloadLength > 65535) { + $this->closeTcpClient($client); + return; + } + if (strlen($this->tcpBuffers[$clientId]) < ($payloadLength + 2)) { return; }Note: The 2-byte length prefix already caps at 65535, so this is more about documentation and explicit handling. The Swoole adapter uses
package_max_length => 65537which provides similar protection.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.mddocker-compose.ymlsrc/DNS/Adapter/Native.phpsrc/DNS/Adapter/Swoole.phpsrc/DNS/Client.phpsrc/DNS/Validator/DNS.phptests/e2e/DNS/ClientTest.php
🧰 Additional context used
🧬 Code graph analysis (4)
tests/e2e/DNS/ClientTest.php (2)
src/DNS/Client.php (2)
Client(8-174)query(51-79)src/DNS/Message/Question.php (1)
Question(7-54)
src/DNS/Client.php (1)
src/DNS/Resolver/Proxy.php (1)
__construct(21-26)
src/DNS/Adapter/Native.php (1)
src/DNS/Adapter.php (2)
onWorkerStart(13-13)onPacket(21-21)
src/DNS/Adapter/Swoole.php (3)
src/DNS/Server.php (2)
Server(60-308)onPacket(171-283)src/DNS/Adapter.php (2)
Adapter(5-34)onPacket(21-21)src/DNS/Adapter/Native.php (1)
onPacket(74-77)
🪛 GitHub Actions: CodeQL
src/DNS/Client.php
[error] 58-58: Parameter #1 $socket of function socket_sendto expects Socket, Socket|null given.
[error] 66-66: Parameter #1 $socket of function socket_recvfrom expects Socket, Socket|null given.
[error] 144-144: Parameter #2 $length of function fread expects int<1, max>, int given.
src/DNS/Adapter/Native.php
[error] 163-163: Instanceof between Socket and Socket will always evaluate to true. 🪪 instanceof.alwaysTrue
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit & E2E
🔇 Additional comments (7)
docker-compose.yml (1)
17-18: LGTM!The TCP port mapping complements the existing UDP mapping, enabling both protocols on port 5300 as expected for DNS servers per RFC 5966.
src/DNS/Validator/DNS.php (1)
61-61: LGTM!Minor formatting adjustment adding a space after the
matchkeyword, aligning with PSR-12 style conventions.src/DNS/Adapter/Swoole.php (2)
26-36: LGTM on TCP server configuration.The length-prefixed framing configuration follows RFC 1035 DNS-over-TCP correctly:
- 2-byte network-order length prefix (
package_length_type => 'n')- Body starts after the 2-byte header (
package_body_offset => 2)- Max length accounts for the 65535-byte payload plus 2-byte prefix
74-88: LGTM on TCP receive handler.The handler correctly strips the 2-byte length prefix (already validated by Swoole's
open_length_check), processes the payload, and frames the response. The pattern matches the Native adapter's TCP handling.src/DNS/Client.php (1)
81-126: LGTM on TCP query implementation.The
queryTcpmethod correctly implements RFC 1035 DNS-over-TCP:
- Length-prefixed framing with 2-byte network-order header
- Proper connection management with
try/finallyensuring socket closure- Comprehensive error handling for partial writes and incomplete reads
- Transaction ID validation in
decodeResponsesrc/DNS/Adapter/Native.php (2)
228-244: LGTM on TCP response handling.The
sendTcpResponsemethod correctly handles partial writes with a loop and properly cleans up the client on failure.
246-253: LGTM on client cleanup.The
closeTcpClientmethod properly removes all references to the client (preventing memory leaks in long-running servers) and suppresses errors onsocket_closewhich may already be closed.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/DNS/Adapter/Native.phpsrc/DNS/Client.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/DNS/Client.php (1)
src/DNS/Resolver/Proxy.php (1)
__construct(21-26)
src/DNS/Adapter/Native.php (2)
src/DNS/Server.php (2)
onWorkerStart(122-129)onPacket(171-283)src/DNS/Adapter.php (2)
onWorkerStart(13-13)onPacket(21-21)
🔇 Additional comments (13)
src/DNS/Client.php (6)
10-11: LGTM! Socket property correctly made nullable.The nullable type is appropriate since TCP mode bypasses UDP socket initialization.
16-18: LGTM! TCP flag added with backward-compatible default.The
$useTcpparameter defaults tofalse, preserving existing behavior for callers that don't specify it.Also applies to: 28-32
53-59: LGTM! Query routing correctly delegates based on protocol.The TCP/UDP routing is clear, and the defensive null-check at lines 57-59 ensures safety even though the constructor guarantees UDP socket initialization when
useTcpis false.
85-130: LGTM! TCP query implementation follows RFC 1035 framing.The method correctly implements DNS-over-TCP with proper length-prefix framing, error handling, and resource cleanup in the finally block.
132-141: LGTM! Response decoding includes transaction ID validation.The transaction ID check at line 136 prevents accepting mismatched responses, which is a good security practice.
176-183: LGTM! IPv6 addresses correctly wrapped for stream socket URI.The bracket wrapping at line 179 is necessary for IPv6 addresses in stream socket URIs (e.g.,
tcp://[::1]:53).src/DNS/Adapter/Native.php (7)
11-19: LGTM! Property declarations properly typed.The separation of
$udpServerand$tcpServer, along with per-client state arrays, provides a clear structure for multiplexing.Also applies to: 30-30
36-36: LGTM! TCP enabled by default for the server.Note that the server enables TCP by default (
$enableTcp = true), whileClientdefaults to UDP ($useTcp = false). This is appropriate since servers typically listen on both protocols while clients choose one.Also applies to: 40-40
42-56: LGTM! Socket creation includes SO_REUSEADDR for TCP.The
SO_REUSEADDRoption at line 54 is good practice, allowing the server to restart quickly without "address already in use" errors.
84-98: LGTM! TCP server properly configured with non-blocking mode.The non-blocking mode at line 97 is essential for multiplexing with
socket_select.
106-166: LGTM! Socket multiplexing correctly handles UDP and TCP.The
socket_selectat line 119 efficiently multiplexes the UDP server, TCP listen socket, and multiple TCP client connections. The per-protocol handling is clear and correct.
179-225: LGTM! TCP client handler correctly implements length-prefixed framing.The buffering and frame extraction logic (lines 197-224) properly handles partial reads and multiple frames per recv, aligning with DNS-over-TCP requirements.
245-252: LGTM! TCP client cleanup properly manages state.The cleanup at line 249 removes both the socket and buffer entries, preventing memory leaks.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/DNS/Adapter/Native.phpsrc/DNS/Client.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/DNS/Adapter/Native.php (1)
src/DNS/Adapter.php (2)
onWorkerStart(13-13)onPacket(21-21)
src/DNS/Client.php (4)
src/DNS/Message/Header.php (3)
__construct(11-31)encode(64-84)decode(33-62)src/DNS/Message/Question.php (3)
__construct(11-17)encode(48-53)decode(22-46)src/DNS/Message.php (5)
__construct(32-68)Message(11-208)encode(186-207)response(111-139)decode(141-184)src/DNS/Resolver/Proxy.php (1)
__construct(21-26)
🔇 Additional comments (12)
src/DNS/Client.php (6)
16-32: LGTM! Clean TCP mode initialization.The
$useTcpflag and early-return pattern correctly establish TCP-only mode, skipping UDP socket initialization when TCP is enabled. The default value preserves backward compatibility.
53-59: LGTM! Proper routing with defensive guard.The TCP/UDP routing logic is clear, and the
instanceofcheck ensures the UDP socket is initialized before use, preventing potential runtime errors.
85-130: LGTM! Robust TCP query implementation.The TCP query flow correctly implements DNS-over-TCP framing (2-byte length prefix), handles timeouts, validates response completeness, and ensures socket cleanup with try-finally. Error handling is comprehensive.
132-141: LGTM! Essential transaction ID verification.Verifying that the response transaction ID matches the query is critical for security and correctness, preventing acceptance of mismatched responses.
143-174: LGTM! Defensive read implementation addresses past concerns.The method correctly handles partial reads, timeouts, and EOF conditions. The explicit guards at lines 150-152 and the EOF check at line 163 address the static analysis warnings and infinite loop risk from previous reviews.
176-183: LGTM! Correct IPv6 formatting.Properly wraps IPv6 addresses in brackets for TCP connection URIs, which is required for correct parsing of host:port combinations.
src/DNS/Adapter/Native.php (6)
11-57: LGTM! Clean dual-protocol architecture.The property structure and constructor correctly establish optional TCP support alongside UDP. Per-client buffering (
tcpBuffers) is essential for TCP frame reassembly, and theenableTcpflag provides deployment flexibility.
84-98: LGTM! Proper TCP server initialization.The TCP bind, listen (with reasonable backlog), and non-blocking mode setup for the server socket are all correct for multiplexed accept operations.
105-145: LGTM! Well-structured event loop multiplexing.The socket_select-based multiplexing correctly handles UDP, TCP server accept, and TCP client I/O in a single event loop. The UDP path is straightforward and includes appropriate error handling.
179-225: LGTM! Solid TCP frame handling with proper buffering.The method correctly handles partial reads, EOF detection (line 188), and frame reassembly. The while loop (line 197) efficiently processes multiple frames per read event, and reusing the
onPacketcallback maintains consistent semantics across UDP and TCP.
227-251: LGTM! Write loop correctly handles transient errors.The EAGAIN/EWOULDBLOCK handling (lines 239-243) properly distinguishes transient from fatal errors. The
socket_clear_errorand brief sleep before retry are good practices. This addresses the previous review concern.
253-260: LGTM! Clean resource cleanup.Properly removes client state from both tracking arrays and closes the socket. The
@suppression is acceptable since the socket may already be closed by the peer.
| if ($this->tcpServer !== null && $socket === $this->tcpServer) { | ||
| $client = @socket_accept($this->tcpServer); | ||
|
|
||
| if ($client instanceof Socket) { | ||
| socket_set_option($client, SOL_SOCKET, SO_KEEPALIVE, 1); | ||
| socket_set_option($client, SOL_SOCKET, SO_RCVTIMEO, ['sec' => 5, 'usec' => 0]); | ||
| socket_set_option($client, SOL_SOCKET, SO_SNDTIMEO, ['sec' => 5, 'usec' => 0]); | ||
|
|
||
| if (socket_sendto($this->server, $answer, strlen($answer), 0, $ip, $port) === false) { | ||
| printf('Error in socket\n'); | ||
| $id = spl_object_id($client); | ||
| $this->tcpClients[$id] = $client; | ||
| $this->tcpBuffers[$id] = ''; | ||
| } | ||
|
|
||
| continue; | ||
| } |
There was a problem hiding this comment.
Set accepted TCP clients to non-blocking mode.
The TCP server socket is non-blocking (line 97), but accepted client sockets inherit blocking mode by default. Since sendTcpResponse (lines 236-243) checks for EAGAIN/EWOULDBLOCK, it expects non-blocking sockets. Without non-blocking mode, socket_write can hang the event loop when send buffers fill, freezing the entire server.
🔎 Proposed fix
if ($client instanceof Socket) {
+ socket_set_nonblock($client);
socket_set_option($client, SOL_SOCKET, SO_KEEPALIVE, 1);
socket_set_option($client, SOL_SOCKET, SO_RCVTIMEO, ['sec' => 5, 'usec' => 0]);
socket_set_option($client, SOL_SOCKET, SO_SNDTIMEO, ['sec' => 5, 'usec' => 0]);🤖 Prompt for AI Agents
In src/DNS/Adapter/Native.php around lines 147 to 161, accepted TCP client
sockets are not set to non-blocking mode even though the server socket is
non-blocking, which can cause socket_write to block the event loop; after
accepting a client, call socket_set_nonblock($client) (or the equivalent
non-blocking socket_set_option) and ensure any error is handled so the client is
registered only if non-blocking was applied successfully; this guarantees
sendTcpResponse's EAGAIN/EWOULDBLOCK handling works without risking a blocking
write.
There was a problem hiding this comment.
@lohanidamodar this seems concerning
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
stnguyen90
left a comment
stnguyen90
left a comment
There was a problem hiding this comment.
Do we have any benchmarks we can run to compare this with the UDP only server to make sure the performance of the server is still good?
Has this been tested on staging?
| if ($this->tcpServer !== null && $socket === $this->tcpServer) { | ||
| $client = @socket_accept($this->tcpServer); | ||
|
|
||
| if ($client instanceof Socket) { | ||
| socket_set_option($client, SOL_SOCKET, SO_KEEPALIVE, 1); | ||
| socket_set_option($client, SOL_SOCKET, SO_RCVTIMEO, ['sec' => 5, 'usec' => 0]); | ||
| socket_set_option($client, SOL_SOCKET, SO_SNDTIMEO, ['sec' => 5, 'usec' => 0]); | ||
|
|
||
| if (socket_sendto($this->server, $answer, strlen($answer), 0, $ip, $port) === false) { | ||
| printf('Error in socket\n'); | ||
| $id = spl_object_id($client); | ||
| $this->tcpClients[$id] = $client; | ||
| $this->tcpBuffers[$id] = ''; | ||
| } | ||
|
|
||
| continue; | ||
| } |
There was a problem hiding this comment.
@lohanidamodar this seems concerning
| $this->udpPort->on('Packet', function ($server, $data, $clientInfo) { | ||
| $ip = $clientInfo['address'] ?? ''; | ||
| $port = (int) ($clientInfo['port'] ?? 0); | ||
| $answer = \call_user_func($this->onPacket, $data, $ip, $port); | ||
|
|
||
| // Swoole UDP sockets reject zero-length payloads; skip responding instead. | ||
| if ($answer === '') { | ||
| return; | ||
| } | ||
|
|
||
| $server->sendto($ip, $port, $answer); |
There was a problem hiding this comment.
There's a bit of duplication here between UDP and TCP listener, can we extract that do a private function handleQuery to keep things consistent?
| $this->host = $host; | ||
| $this->port = $port; | ||
| $this->server = new Server($this->host, $this->port, SWOOLE_PROCESS, SWOOLE_SOCK_UDP); | ||
| $this->server = new Server($this->host, $this->port, SWOOLE_PROCESS, SWOOLE_SOCK_TCP); |
There was a problem hiding this comment.
Small detail, but I think important to encapsulate: This seems backwards to me, UDP is the primary protocol for DNS (vast majority of queries).
Do you think UDP should be the primary Server and TCP should be tacked on as an additional listener. Probably guarded by $enableTcp flag the same way Native adapter does it
There was a problem hiding this comment.
The TCP implementation looks good, but we're missing truncation handling for UDP responses.
DNS over UDP has a 512-byte limit (or higher with EDNS0). When a response exceeds this, the server should:
- Set the
TC(truncated) flag in the header - Send a truncated response
- Client then retries over TCP to get the full response
Currently Message::encode() has no size awareness - it'll return arbitrarily large packets. The UDP send path will happily transmit these, but they may be dropped or fragmented by the network.
The adapter layer should check encoded response size before sending over UDP:
$answer = call_user_func($this->onPacket, $buf, $ip, $port);
$maxUdpSize = 512; // or from EDNS0 OPT record if present
if (strlen($answer) > $maxUdpSize) {
// Re-encode with truncation flag set, minimal records
$answer = $this->truncateResponse($answer, $maxUdpSize);
}Or push this into Message::encode($maxSize = null) so it can self-truncate and set the TC flag.
This is why TCP support matters—without it, large responses (big TXT records, DNSSEC, many A records) silently fail. With TCP but without truncation signaling, clients don't know to retry.
| $readSockets[] = $this->tcpServer; | ||
| } | ||
|
|
||
| foreach ($this->tcpClients as $client) { |
There was a problem hiding this comment.
Probably wise to have a limit here, otherwise you can easily be exhausted
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.