Conversation
Implements RFC 9250 DNS-over-QUIC transport: - handleDoQListener: Accept incoming QUIC connections - handleDoQConnection: Handle QUIC sessions and streams - handleDoQStream: Process DNS messages on bidirectional streams - setupDoQListener: Create QUIC listener with TLS config Adds doqListener and DoQAddr fields to Server struct. Uses quic-go library with 0-RTT support and 30s timeouts.
Tests for DNS-over-QUIC implementation: - TestDoQ_E2E_BasicQuery: Valid DNS query returns A record - TestDoQ_E2E_MultipleStreams: Multiple queries on same connection - TestDoQ_E2E_InvalidQuery: Malformed message handling - TestDoQ_E2E_ConnectionClose: Connection lifecycle
Documents RFC 9250 implementation decision, configuration, and consequences.
- README.md: Add DoQ feature bullet - features.md: Add DoQ under Advanced DNS Standards - go.mod: Add quic-go v0.59.0 dependency
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds DNS-over-QUIC (DoQ, RFC 9250) support: documentation and ADR, quic-go dependency, Server fields and lifecycle wiring for DoQ, a QUIC listener/connection/stream implementation forwarding raw DNS bytes into existing packet handler, and end-to-end tests covering streams and error cases. ChangesDoQ feature (docs, server wiring, QUIC handling, tests)
Sequence Diagram(s)sequenceDiagram
actor Client as QUIC Client
participant DoQListener as DoQ Listener
participant Connection as QUIC Connection
participant StreamHandler as Stream Handler
participant DNSHandler as DNS Handler
Client->>DoQListener: Establish QUIC connection (ALPN: doq)
DoQListener->>Connection: Accept connection → spawn conn goroutine
Client->>Connection: Open bidirectional stream
Connection->>StreamHandler: Spawn stream goroutine
Client->>StreamHandler: Write raw DNS query bytes
StreamHandler->>DNSHandler: handlePacket(query, "doq")
DNSHandler-->>StreamHandler: Return DNS response bytes
StreamHandler->>Client: Write response and close stream
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 46 minutes and 28 seconds.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: 3
🧹 Nitpick comments (1)
internal/dns/server/doq_e2e_test.go (1)
57-57: ⚡ Quick winReplace the fixed sleeps with readiness polling.
These hard-coded delays will race on slower CI runners and make the suite flaky. Prefer a helper that retries
quic.DialAddruntil the listener is ready and rely on read deadlines / eventual assertions instead of sleeping after writes and closes.Also applies to: 90-91, 131-131, 204-204, 246-246, 260-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/dns/server/doq_e2e_test.go` at line 57, Replace fixed time.Sleep calls with a readiness-poll helper and rely on read deadlines/eventual assertions: implement a helper (e.g., waitForQuicDial or waitForListenerReady) that repeatedly calls quic.DialAddr with a short timeout until it succeeds or a global test deadline elapses, use that helper wherever time.Sleep is used (locations around the current time.Sleep at line 57 and the other instances at 90-91, 131, 204, 246, 260-261), and remove post-write/close sleeps by using SetReadDeadline/SetDeadline on the connection and asserting expected reads/closures with retries instead of sleeping. Ensure the helper uses context/time.After for overall timeout and small per-attempt timeouts to avoid long hangs and reference quic.DialAddr and the test connection read/deadline methods when locating where to replace sleeps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/decisions/0010-dns-over-quic-doq.md`:
- Around line 118-119: Update the incorrect RFC reference in the bullet list:
replace the second entry labeled "[RFC 7830: DNS-over-QUIC Initial Keys]" with
the correct QUIC/TLS key derivation spec by changing the label and URL to
reference RFC 9001 (e.g., "[RFC 9001: QUIC:
TLS](https://datatracker.ietf.org/doc/html/rfc9001)"), leaving the first RFC
9250 entry unchanged.
In `@internal/dns/server/doq.go`:
- Around line 59-72: The DoQ stream read uses a single stream.Read call that can
return a partial DNS message; replace the manual buffer + Read logic in the DoQ
handler (the code that reads from `stream` in internal/dns/server/doq.go) with a
read-until-EOF call such as io.ReadAll(stream) to assemble the complete message,
then check for zero-length and use the returned byte slice as the DNS payload
(i.e., remove the fixed 65535 buffer, the single Read call and the manual copy
into dnsMsg). Ensure any read errors are handled and returned consistently with
existing error handling in the handler.
- Around line 83-95: The TLS config passed into quic.ListenAddr must advertise
the "doq" ALPN; currently s.TLSConfig is used as-is and lacks NextProtos
enrichment. Fix by cloning s.TLSConfig (to avoid mutating the original), ensure
NextProtos contains "doq" (append if missing) before calling quic.ListenAddr,
and leave quic.Config.Allow0RTT at its default (do not rely on KeepAlivePeriod
for 0-RTT); also update any docs/comments that claim 0-RTT is enabled by
default. Target symbols: s.TLSConfig, TLSConfig.NextProtos, quic.ListenAddr,
quic.Config.Allow0RTT, KeepAlivePeriod.
---
Nitpick comments:
In `@internal/dns/server/doq_e2e_test.go`:
- Line 57: Replace fixed time.Sleep calls with a readiness-poll helper and rely
on read deadlines/eventual assertions: implement a helper (e.g., waitForQuicDial
or waitForListenerReady) that repeatedly calls quic.DialAddr with a short
timeout until it succeeds or a global test deadline elapses, use that helper
wherever time.Sleep is used (locations around the current time.Sleep at line 57
and the other instances at 90-91, 131, 204, 246, 260-261), and remove
post-write/close sleeps by using SetReadDeadline/SetDeadline on the connection
and asserting expected reads/closures with retries instead of sleeping. Ensure
the helper uses context/time.After for overall timeout and small per-attempt
timeouts to avoid long hangs and reference quic.DialAddr and the test connection
read/deadline methods when locating where to replace sleeps.
🪄 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
Run ID: e699b38e-da9c-4ff9-b5b2-7749d7da3d19
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
README.mddocs/decisions/0010-dns-over-quic-doq.mdfeatures.mdgo.modinternal/dns/server/doq.gointernal/dns/server/doq_e2e_test.gointernal/dns/server/server.go
| - [RFC 9250: DNS-over-QUIC](https://datatracker.ietf.org/doc/html/rfc9250) | ||
| - [RFC 7830: DNS-over-QUIC Initial Keys](https://datatracker.ietf.org/doc/html/rfc7830) |
There was a problem hiding this comment.
Fix the QUIC reference here.
RFC 7830 is the EDNS(0) Padding Option, not a DoQ / QUIC initial-keys reference. If you meant the QUIC-TLS key derivation spec, this should point to RFC 9001 instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/decisions/0010-dns-over-quic-doq.md` around lines 118 - 119, Update the
incorrect RFC reference in the bullet list: replace the second entry labeled
"[RFC 7830: DNS-over-QUIC Initial Keys]" with the correct QUIC/TLS key
derivation spec by changing the label and URL to reference RFC 9001 (e.g., "[RFC
9001: QUIC: TLS](https://datatracker.ietf.org/doc/html/rfc9001)"), leaving the
first RFC 9250 entry unchanged.
| // DNS-over-QUIC uses variable-length messages (no 2-byte length prefix like TCP) | ||
| buf := make([]byte, 65535) | ||
| n, err := stream.Read(buf) | ||
| if err != nil { | ||
| if err != io.EOF && err != io.ErrUnexpectedEOF { | ||
| return | ||
| } | ||
| } | ||
| if n == 0 { | ||
| return | ||
| } | ||
|
|
||
| dnsMsg := make([]byte, n) | ||
| copy(dnsMsg, buf[:n]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/dns/server/doq.go | head -100Repository: poyrazK/cloudDNS
Length of output: 3182
🏁 Script executed:
find . -type f -name "*doq*test*.go" -o -name "*test*doq*.go" | head -20Repository: poyrazK/cloudDNS
Length of output: 98
🏁 Script executed:
cat -n ./internal/dns/server/doq_e2e_test.goRepository: poyrazK/cloudDNS
Length of output: 9287
🏁 Script executed:
rg -n "DNSPacket|FromBuffer" internal/dns/packet/ --type go | head -30Repository: poyrazK/cloudDNS
Length of output: 2338
🏁 Script executed:
rg -n "func.*FromBuffer" internal/dns/packet/ --type go -A 10 | head -50Repository: poyrazK/cloudDNS
Length of output: 3361
🏁 Script executed:
sed -n '941,970p' internal/dns/packet/packet.goRepository: poyrazK/cloudDNS
Length of output: 1012
🏁 Script executed:
cat -n internal/dns/packet/packet.go | sed -n '1,50p'Repository: poyrazK/cloudDNS
Length of output: 2110
🏁 Script executed:
rg -n "type BytePacketBuffer" internal/dns/packet/ --type go -A 20Repository: poyrazK/cloudDNS
Length of output: 1331
🏁 Script executed:
rg -n "Load|read|Read" internal/dns/packet/buffer.go --type go | head -20Repository: poyrazK/cloudDNS
Length of output: 999
🏁 Script executed:
cat -n internal/dns/packet/buffer.go | sed -n '68,75p'Repository: poyrazK/cloudDNS
Length of output: 354
🏁 Script executed:
rg -n "length|Length|framing|Framing" internal/dns/server/doq.go --type goRepository: poyrazK/cloudDNS
Length of output: 228
🏁 Script executed:
find . -name "*.go" -path "*/dns/server/*" | grep -E "tcp|dot" | head -10Repository: poyrazK/cloudDNS
Length of output: 42
🏁 Script executed:
rg -n "handleTCP|handleDOT" internal/dns/server/ --type go -A 15 | head -50Repository: poyrazK/cloudDNS
Length of output: 2922
🏁 Script executed:
rg -n "func.*handleTCPConnection" internal/dns/server/ --type go -A 25 | head -60Repository: poyrazK/cloudDNS
Length of output: 1819
Use io.ReadAll() to read complete DoQ message before parsing.
QUIC streams are byte streams without message boundaries. A single Read() may return only a partial DNS message if the client writes are split across packets. The current handler will intermittently parse truncated queries, though E2E tests don't expose this because they write complete queries in single packets. Read until EOF/FIN, then parse the assembled message.
Suggested direction
- buf := make([]byte, 65535)
- n, err := stream.Read(buf)
+ dnsMsg, err := io.ReadAll(stream)
if err != nil {
- if err != io.EOF && err != io.ErrUnexpectedEOF {
+ if err != io.EOF && err != io.ErrUnexpectedEOF {
return
}
}
- if n == 0 {
+ if len(dnsMsg) == 0 {
return
}
-
- dnsMsg := make([]byte, n)
- copy(dnsMsg, buf[:n])Compare with TCP handler (lines 598-609 in server.go) which uses 2-byte length framing with io.ReadFull() to ensure complete message receipt before parsing.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // DNS-over-QUIC uses variable-length messages (no 2-byte length prefix like TCP) | |
| buf := make([]byte, 65535) | |
| n, err := stream.Read(buf) | |
| if err != nil { | |
| if err != io.EOF && err != io.ErrUnexpectedEOF { | |
| return | |
| } | |
| } | |
| if n == 0 { | |
| return | |
| } | |
| dnsMsg := make([]byte, n) | |
| copy(dnsMsg, buf[:n]) | |
| // DNS-over-QUIC uses variable-length messages (no 2-byte length prefix like TCP) | |
| dnsMsg, err := io.ReadAll(stream) | |
| if err != nil { | |
| if err != io.EOF && err != io.ErrUnexpectedEOF { | |
| return | |
| } | |
| } | |
| if len(dnsMsg) == 0 { | |
| return | |
| } |
🧰 Tools
🪛 GitHub Check: Lint
[failure] 63-63:
comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/dns/server/doq.go` around lines 59 - 72, The DoQ stream read uses a
single stream.Read call that can return a partial DNS message; replace the
manual buffer + Read logic in the DoQ handler (the code that reads from `stream`
in internal/dns/server/doq.go) with a read-until-EOF call such as
io.ReadAll(stream) to assemble the complete message, then check for zero-length
and use the returned byte slice as the DNS payload (i.e., remove the fixed 65535
buffer, the single Read call and the manual copy into dnsMsg). Ensure any read
errors are handled and returned consistently with existing error handling in the
handler.
- Add missing zone to TestDoQ_E2E_MultipleStreams - Add defer cancel to TestDoQ_E2E_ConnectionClose - Log unexpected read errors in handleDoQStream Note: 0-RTT warning not added as disabling early data in quic-go requires complex configuration; documented in ADR instead.
- Use errors.Is for wrapped error comparison - Check error returns from Close/CloseWithError - Add underscore to ignored handlePacket return
Pass lifecycleCtx to handleDoQConnection and AcceptStream to satisfy contextcheck linter requirement.
Propagate lifecycleCtx from Run() through handleDoQListener, handleDoQConnection, to handleDoQStream to satisfy contextcheck linter.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/dns/server/doq_e2e_test.go (1)
57-57: ⚡ Quick winReplace fixed sleeps with readiness polling to reduce flakiness.
Using fixed delays for startup/processing makes these tests timing-sensitive and slower in CI. Prefer a short retry loop with timeout (dial until success / operation ready) instead of sleeping.
Also applies to: 91-91, 134-134, 207-207, 250-250, 265-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/dns/server/doq_e2e_test.go` at line 57, Replace the hard-coded time.Sleep calls in internal/dns/server/doq_e2e_test.go with a short readiness-polling loop that retries until success or a timeout to avoid flakiness; implement a small helper (e.g., waitForReady or retryUntilTimeout) used from the test functions that repeatedly performs the real readiness check (dialing the DoQ endpoint, performing a minimal DNS query, or checking the server listener) with a small backoff and a total timeout, and replace the occurrences of time.Sleep(200 * time.Millisecond) (and the other listed sleeps) with calls to that helper so tests proceed as soon as the service is ready instead of waiting a fixed interval.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/dns/server/doq_e2e_test.go`:
- Around line 228-232: The test currently only logs unexpected stream errors so
invalid-query cases can silently pass; change the error handling after calling
stream.Read(respBuf) to fail the test when err is non-nil and not isEOFLike(err)
by replacing the t.Logf call with a test failure (e.g., t.Fatalf or t.Errorf
followed by return) so unexpected errors in the stream.Read(respBuf) path cause
the test to fail; modify the branch that references isEOFLike and t.Logf to use
t.Fatalf (or t.Errorf + t.FailNow) instead.
- Around line 147-149: The test currently ignores errors from conn.OpenStream()
and subsequent stream I/O/decoding (e.g., stream1, stream2 opens and
stream.Read() results), which can hide failures and cause nil derefs; update the
multi-stream test to fail fast by checking and handling all returned errors from
conn.OpenStream(), Close(), and Read() calls (e.g., after calling
conn.OpenStream() for stream1/stream2, assert err==nil and t.Fatalf on error),
validate the returned stream is non-nil before using it, and check Read()/decode
errors and lengths before parsing so any I/O or decode error immediately fails
the test rather than being ignored.
---
Nitpick comments:
In `@internal/dns/server/doq_e2e_test.go`:
- Line 57: Replace the hard-coded time.Sleep calls in
internal/dns/server/doq_e2e_test.go with a short readiness-polling loop that
retries until success or a timeout to avoid flakiness; implement a small helper
(e.g., waitForReady or retryUntilTimeout) used from the test functions that
repeatedly performs the real readiness check (dialing the DoQ endpoint,
performing a minimal DNS query, or checking the server listener) with a small
backoff and a total timeout, and replace the occurrences of time.Sleep(200 *
time.Millisecond) (and the other listed sleeps) with calls to that helper so
tests proceed as soon as the service is ready instead of waiting a fixed
interval.
🪄 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
Run ID: 9cdf7a97-2dac-49c4-b03e-c5ee2ce2092b
📒 Files selected for processing (3)
internal/dns/server/doq.gointernal/dns/server/doq_e2e_test.gointernal/dns/server/server.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/dns/server/doq.go
- internal/dns/server/server.go
| stream1, _ := conn.OpenStream() | ||
| defer stream1.Close() | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ignored returned errors in this test file.
rg -n -C2 ':\s*=\s*[^,\n]+,\s*_' internal/dns/server/doq_e2e_test.go
rg -n -C2 '_\s*=\s*[^;\n]+\(' internal/dns/server/doq_e2e_test.goRepository: poyrazK/cloudDNS
Length of output: 2143
🏁 Script executed:
cat -n internal/dns/server/doq_e2e_test.go | sed -n '140,195p'Repository: poyrazK/cloudDNS
Length of output: 2227
Fail fast on stream I/O and decode errors in the multi-stream test.
Lines 147, 154, 158, 161, 169, 173, 178, and 186 ignore errors, which can mask protocol regressions and may panic if stream operations fail. In particular, ignoring errors from OpenStream() can result in nil-stream operations, and ignoring Read() errors while using the byte count can lead to parsing incomplete data.
Suggested fix
- stream1, _ := conn.OpenStream()
+ stream1, err := conn.OpenStream()
+ if err != nil {
+ t.Fatalf("Failed to open first stream: %v", err)
+ }
defer stream1.Close()
@@
- _, _ = stream1.Write(qb1.Buf[:qb1.Position()])
+ if _, err := stream1.Write(qb1.Buf[:qb1.Position()]); err != nil {
+ t.Fatalf("Failed to write first query: %v", err)
+ }
@@
- n1, _ := stream1.Read(respBuf1)
+ n1, err := stream1.Read(respBuf1)
+ if err != nil && !(n1 > 0 && isEOFLike(err)) {
+ t.Fatalf("Failed to read first response: %v", err)
+ }
@@
- stream2, _ := conn.OpenStream()
+ stream2, err := conn.OpenStream()
+ if err != nil {
+ t.Fatalf("Failed to open second stream: %v", err)
+ }
defer stream2.Close()
@@
- _, _ = stream2.Write(qb2.Buf[:qb2.Position()])
+ if _, err := stream2.Write(qb2.Buf[:qb2.Position()]); err != nil {
+ t.Fatalf("Failed to write second query: %v", err)
+ }
@@
- n2, _ := stream2.Read(respBuf2)
+ n2, err := stream2.Read(respBuf2)
+ if err != nil && !(n2 > 0 && isEOFLike(err)) {
+ t.Fatalf("Failed to read second response: %v", err)
+ }
@@
- _ = resPacket.FromBuffer(pb)
+ if err := resPacket.FromBuffer(pb); err != nil {
+ t.Fatalf("Failed to parse first response: %v", err)
+ }
@@
- _ = resPacket2.FromBuffer(pb2)
+ if err := resPacket2.FromBuffer(pb2); err != nil {
+ t.Fatalf("Failed to parse second response: %v", err)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/dns/server/doq_e2e_test.go` around lines 147 - 149, The test
currently ignores errors from conn.OpenStream() and subsequent stream
I/O/decoding (e.g., stream1, stream2 opens and stream.Read() results), which can
hide failures and cause nil derefs; update the multi-stream test to fail fast by
checking and handling all returned errors from conn.OpenStream(), Close(), and
Read() calls (e.g., after calling conn.OpenStream() for stream1/stream2, assert
err==nil and t.Fatalf on error), validate the returned stream is non-nil before
using it, and check Read()/decode errors and lengths before parsing so any I/O
or decode error immediately fails the test rather than being ignored.
| _, err = stream.Read(respBuf) | ||
| // EOF or error is acceptable - server may close stream on parse failure | ||
| if err != nil && !isEOFLike(err) { | ||
| t.Logf("Stream error (may be expected): %v", err) | ||
| } |
There was a problem hiding this comment.
Invalid-query test currently passes on unexpected stream errors.
Line 230 only logs unexpected errors, so the test can succeed when behavior is wrong. This should fail the test.
Suggested fix
- if err != nil && !isEOFLike(err) {
- t.Logf("Stream error (may be expected): %v", err)
- }
+ if err != nil && !isEOFLike(err) {
+ t.Fatalf("Unexpected stream error for invalid query: %v", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, err = stream.Read(respBuf) | |
| // EOF or error is acceptable - server may close stream on parse failure | |
| if err != nil && !isEOFLike(err) { | |
| t.Logf("Stream error (may be expected): %v", err) | |
| } | |
| _, err = stream.Read(respBuf) | |
| // EOF or error is acceptable - server may close stream on parse failure | |
| if err != nil && !isEOFLike(err) { | |
| t.Fatalf("Unexpected stream error for invalid query: %v", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/dns/server/doq_e2e_test.go` around lines 228 - 232, The test
currently only logs unexpected stream errors so invalid-query cases can silently
pass; change the error handling after calling stream.Read(respBuf) to fail the
test when err is non-nil and not isEOFLike(err) by replacing the t.Logf call
with a test failure (e.g., t.Fatalf or t.Errorf followed by return) so
unexpected errors in the stream.Read(respBuf) path cause the test to fail;
modify the branch that references isEOFLike and t.Logf to use t.Fatalf (or
t.Errorf + t.FailNow) instead.
The quic.Listener.Accept call doesn't support context cancellation properly, so use context.Background. Context is still propagated to handleDoQConnection and handleDoQStream for proper lifecycle.
Use lifecycleCtx from server struct to satisfy contextcheck linter. Listener.Accept still uses context.Background as quic-go doesn't properly support context cancellation there.
Use ctx instead of context.Background for listener.Accept to satisfy contextcheck linter. This is the final fix.
Log a warning when DoQ listener starts to alert operators that 0-RTT has replay attack implications.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/dns/server/server.go`:
- Around line 513-523: The code silently skips starting DoQ when s.DoQAddr is
set but s.TLSConfig is nil or when setupDoQListener returns an error; update the
startup logic in the block that checks s.DoQAddr to treat these as failures (or
at minimum log an error) instead of ignoring them: validate that s.TLSConfig is
non-nil and if nil call s.Logger.Error with context ("DoQ configured but
TLSConfig is nil") and return/propagate the failure or mark startup as failed;
likewise, if setupDoQListener returns a non-nil errDoQ, log it at error level
with the addr and the error and stop/startup failure rather than silently
continuing, while still setting s.doqListener and spawning the goroutine only on
success.
- Around line 69-70: The shared TLSConfig (TLSConfig) may lack the "doq" ALPN
and causes QUIC handshakes to fail; in setupDoQListener (called from doq.go:91)
clone the provided TLSConfig (use tlsConfig.Clone() or shallow copy if nil),
ensure NextProtos includes "doq" (append if missing) on the cloned config, and
pass that cloned config to quic.ListenAddr so the original TLSConfig used for
DoT/DoH is not mutated and the QUIC listener always presents the "doq" ALPN.
🪄 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
Run ID: 69b25f15-8edf-4a16-87b4-1853409d7542
📒 Files selected for processing (2)
internal/dns/server/doq.gointernal/dns/server/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/dns/server/doq.go
- server.go: Log error when TLSConfig is nil or setupDoQListener fails - doq.go: Clone TLSConfig and append "doq" ALPN to avoid handshake failures Both issues were identified in post-merge review of PR #127.
1 participant
Summary
Test plan
go test -short ./...- All tests passgo test -run DoQ ./internal/dns/server/...- DoQ specific tests passCoverage
Summary by CodeRabbit
New Features
Documentation
Tests