refactor(client): modularize DNS interception logic and address remapping#2751
refactor(client): modularize DNS interception logic and address remapping#2751fortuna wants to merge 21 commits into
Conversation
Co-authored-by: Copilot <copilot@github.com>
|
@greptile review |
Greptile SummaryThis PR refactors the DNS interception layer into composable, single-purpose components: a
Confidence Score: 4/5Safe to merge after addressing the missing sync.Once in singleResponseReceiver and fixing the no-op timeout test assertion. Two P1 findings: singleResponseReceiver regresses from explicit sync.Once protection to an unguarded Close() (real concurrent risk under duplicate UDP responses), and the timeout integration test asserts nothing. Both are targeted fixes. The core architecture, lazy initialization, and address-remapping logic are well-structured and well-tested. client/go/outline/dnsintercept/interceptor.go (singleResponseReceiver double-close) and client/go/outline/configregistry/outline_dns_intercept_test.go (timeout test missing assertion) Important Files Changed
Sequence DiagramsequenceDiagram
participant OS
participant DNSInterceptor
participant lazyBase as lazyPacketProxy(ppBase)
participant lazyDNS as lazyPacketProxy(ppDNSDelegate)
participant ppDNSBase as ppDNSBase (10s timeout)
participant ppDNSTrunc as ppDNSTrunc (local TC)
participant Transport
note over lazyDNS: Starts as ppDNSTrunc (safe default)
OS->>DNSInterceptor: WriteTo(p, 169.254.113.53:53) [DNS]
DNSInterceptor->>lazyDNS: WriteTo(p, 1.1.1.1:53) [remapped]
lazyDNS->>ppDNSTrunc: NewSession + WriteTo (UDP blocked path)
ppDNSTrunc-->>OS: WriteFrom(TC response, 169.254.113.53:53)
note over lazyDNS: After connectivity check passes → SetProxy(ppDNSBase)
OS->>DNSInterceptor: WriteTo(p, 169.254.113.53:53) [DNS]
DNSInterceptor->>lazyDNS: WriteTo(p, 1.1.1.1:53) [remapped]
lazyDNS->>ppDNSBase: NewSession + WriteTo
ppDNSBase->>Transport: UDP query to 1.1.1.1:53
Transport-->>ppDNSBase: UDP response from 1.1.1.1:53
ppDNSBase-->>OS: WriteFrom(response, 169.254.113.53:53) [remapped back]
OS->>DNSInterceptor: WriteTo(p, 1.2.3.4:443) [non-DNS]
DNSInterceptor->>lazyBase: WriteTo(p, 1.2.3.4:443)
lazyBase->>Transport: UDP packet to 1.2.3.4:443
Reviews (2): Last reviewed commit: "test(configregistry): fix data race in D..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func (r *singleResponseReceiver) WriteFrom(p []byte, source net.Addr) (int, error) { | ||
| n, err := r.PacketResponseReceiver.WriteFrom(p, source) | ||
| if atomic.LoadInt32(r.sentCount) == 1 { | ||
| r.PacketResponseReceiver.Close() | ||
| } | ||
| return n, err |
There was a problem hiding this comment.
singleResponseReceiver closes the shared outer resp
r.PacketResponseReceiver here is dnsResp (a natResponseReceiver), which embeds the same resp that was also passed to i.baseProxy.NewSession(resp). Calling r.PacketResponseReceiver.Close() therefore calls resp.Close(), signalling to the OS that the entire session is done. If any non-DNS traffic was sent on the same session, future responses from the base transport session would be delivered to an already-closed receiver.
In the expected VPN architecture each OS UDP socket is DNS-only, so this is unlikely to matter in practice — but the assumption isn't enforced anywhere. A comment noting that the auto-close is intentional and that it relies on DNS sessions being single-purpose would help future readers understand the invariant.
There was a problem hiding this comment.
Pull request overview
Refactors the Outline client’s DNS interception stack by splitting DNS dispatch/remapping into a dedicated DNSInterceptor, introducing lazy session creation to reduce idle resource usage, and updating the configregistry wiring (including separate DNS vs non-DNS UDP idle timeouts).
Changes:
- Replace the old
forward.go/truncate.goapproach withDNSInterceptor(DNS vs non-DNS dispatch + address remapping) pluslazyPacketProxy. - Switch truncation to the SDK’s
dnstruncate.NewPacketProxy()and update config wiring to dynamically select forwarding vs truncation for DNS only. - Add new tests and update
dnsintercept/README.mddiagrams to reflect the new architecture.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| client/go/outline/dnsintercept/truncate_test.go | Removed tests for the deleted custom truncate proxy. |
| client/go/outline/dnsintercept/truncate.go | Removed custom truncation proxy in favor of SDK truncation. |
| client/go/outline/dnsintercept/helpers.go | Moved isEquivalentAddrPort into interceptor.go. |
| client/go/outline/dnsintercept/forward_test.go | Removed tests for the deleted forwarding proxy. |
| client/go/outline/dnsintercept/forward.go | Deleted legacy DNS forwarding proxy implementation. |
| client/go/outline/dnsintercept/lazy_packet_proxy.go | Added lazy session creation wrapper to defer base session allocation until first write. |
| client/go/outline/dnsintercept/lazy_packet_proxy_test.go | Added unit tests for lazy session creation/close behavior. |
| client/go/outline/dnsintercept/interceptor.go | Added DNSInterceptor for DNS dispatch/remapping plus response receiver wrappers. |
| client/go/outline/dnsintercept/interceptor_test.go | Added tests for DNS interception/remapping and receiver auto-close behavior. |
| client/go/outline/dnsintercept/README.md | Updated architecture diagrams and descriptions for the new modular design. |
| client/go/outline/configregistry/outline_dns_intercept.go | Updated integration to use DNSInterceptor, delegate DNS proxy switching, and per-traffic idle timeouts. |
| client/go/outline/configregistry/outline_dns_intercept_test.go | Added integration tests/benchmark for DNS interception, switching, and timeout behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (r *singleResponseReceiver) WriteFrom(p []byte, source net.Addr) (int, error) { | ||
| n, err := r.PacketResponseReceiver.WriteFrom(p, source) | ||
| if atomic.LoadInt32(r.sentCount) == 1 { | ||
| r.PacketResponseReceiver.Close() | ||
| } | ||
| return n, err |
There was a problem hiding this comment.
Perhaps I should count all writes, not just the DNS ones.
|
This needs more work. The closing of the receiver needs to be cleaned up, and I realized that we are not setting read deadlines on the external socket, which is bad. That fix will go on the sdk |
|
Superseded by #2777 and the SDK changes |
Description
This PR refactors the DNS interception logic to be more modular and easier to maintain, while also addressing resource exhaustion issues.
Key changes:
DNSInterceptor(interceptor.go). This replaces the previous logic inforward.go(which has been deleted).lazyPacketProxy(lazy_packet_proxy.go) to defer session creation until the first packet is actually written, reducing idle resource usage.singleResponseReceiverininterceptor.goto automatically close the response receiver after the first response is received (assuming a single request-response pattern for DNS), preventing long-lived idle sessions.truncate.go) in favor of the Outline SDK'sdnstruncate.NewPacketProxy().outline_dns_intercept.goto use theDNSInterceptorcombined with aDelegatePacketProxyfor dynamically switching between UDP forwarding and local truncation based on connectivity checks.outline_dns_intercept.go.client/go/outline/dnsintercept/README.mdwith new Mermaid diagrams reflecting the updated architecture.These changes resolve the resource exhaustion issues while making the individual proxy components single-purpose.