tailscale / tailscale/tailscale

an upstream REFUSED/SERVFAIL is dropped entirely after a 10s stall when that upstream doesn't answer over TCP

Open
#20,826 0 comments 0 reactions 1 assignee Claimed by @bcreane View on GitHub
bug pod/network-features
Dominant language
Go
Stars
36.5k
Forks
3.2k
Avg merge
2d 3h
Merged PRs (30d)
123

Description

**Title:**

### What is the issue?

The forwarder treats a REFUSED or SERVFAIL rcode as a soft error so that other resolvers in the race can still answer. Routing it through the error path has two side effects that weren't intended:

1. It trips `util/race`'s "start the fallback on first error" trigger, so the forwarder immediately opens a TCP connection to the *same server that just answered*. An rcode is a complete answer; the retry can only ever get the same rcode back. Cost is one TCP connect + query per affected query, forever, with no caching in between.
2. The response bytes are only released to the client once **every** resolver has finished erroring (`forwarder.go:1335`, `numErr == len(resolvers)`), so a hung or still-delayed resolver withholds an answer we already have.

Two bounds that should contain this don't work:

3. **`tcpQueryTimeout` (5s) is not enforced.** `sendTCP` puts it on the ctx (`forwarder.go:984`), but nothing interrupts the blocking `binary.Read` (`:1016`) / `io.ReadFull` (`:1023`). The conn is only closed by `fq.closeOnCtxDone`, which fires when the *caller's* ctx ends, so an affected query burns the full `dnsQueryTimeout` (10s) instead.
4. **`skipTCP` waits instead of returning.** With TCP retries disabled by envknob or control knob, `thenTCP` blocks on `<-ctx.Done()` (`forwarder.go:728-731`), so *any* UDP error - rcode or transport - holds that resolver open until the query deadline. Measured: a UDP txid mismatch with retries disabled takes the caller's full timeout instead of failing immediately.

Net effect, forwarding to a single upstream:

| upstream behavior | client sees |
|---|---|
| REFUSED/SERVFAIL over UDP, RST on TCP/53 | rcode delivered, ~0.3ms - fine |
| REFUSED/SERVFAIL over UDP, TCP/53 accepts and never answers | **nothing at all, after 10s** |
| REFUSED/SERVFAIL over UDP, TCP/53 filtered (SYN dropped) | not measured; expect ≥1s of added latency per query from the initial SYN retransmit |

A `dig` sent straight at the same upstream gets its REFUSED in single-digit milliseconds in all three cases.

This bites any deployment where an upstream answers REFUSED or SERVFAIL for some or all names - an internal resolver that refuses recursion, or one whose ACL refuses the address the forwarder's socket egresses from - combined with a TCP/53 that is filtered or doesn't serve DNS. Both halves are common.

### Steps to reproduce

1. Bind a UDP DNS server on `127.0.0.1:P` that answers every query with REFUSED (rcode 5), echoing the question.
2. Bind a TCP listener on the same `127.0.0.1:P` that accepts connections and never writes anything.
3. Point a `resolver.Resolver`'s `.` route at `127.0.0.1:P` and call `Query(ctx, q, "udp", from)`.

Result on `main` (`d200b3f18`): 10.004s, empty response, `waiting for response or error from [127.0.0.1:P]: context deadline exceeded`. Expected: the upstream's REFUSED, immediately.

Substituting SERVFAIL for REFUSED reproduces identically. Dropping step 2, so the TCP connect is refused rather than hung, returns the rcode promptly - the hang needs an upstream that completes the handshake.

For the REFUSED half of the issue, bisected to 0b4c0f208049358391b66756e4decb4b0f72177f ("net/dns/resolver: treat DNS REFUSED responses as soft errors in forwarder race", #19053), first released in **v1.98.0**:

| | latency | response |
|---|---|---|
| `0b4c0f208^` | 240 µs | REFUSED delivered (37 bytes) |
| `main` (d200b3f18) | 10.004 s | none, `context deadline exceeded` |

Before that commit a REFUSED was returned to the client as a normal answer and never reached the TCP retry.

SERVFAIL has behaved this way for much longer - it was already a soft error, and the unenforced `tcpQueryTimeout` dates to 530aaa52f ("net/dns: retry forwarder requests over TCP", v1.52.0). Defect 3 above is what turns both into a 10s total failure rather than a 5s one.

### OS

Linux (not OS-specific; the code path is platform-independent)

### Tailscale version

Reproduced from source at `main` d200b3f18; REFUSED regression present since v1.98.0.

### Bug report

N/A - reproduced from source.

### Also noticed while diagnosing

Two counters mislead in exactly this situation and cost real time to rule out:

- `dns_query_fwd_udp_error_read` counts our own teardown. When one resolver in a race wins, `forwardWithDestChan`'s deferred `fq.closeOnCtxDone.Close()` runs *before* the deferred `cancel()` (LIFO), so the loser's blocked read returns `net.ErrClosed` while `ctx.Err()` is still nil, skips the cancellation check, and is recorded as an upstream read failure. 20 queries against one good and one silent upstream produce 20 phantom `udp_error_read`.
- `dns_query_fwd_tcp` is incremented on entry to `sendTCP`, before the dial, so a metrics dump can show dozens of "TCP queries" with `dns_query_fwd_tcp_wrote 0`.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.