fix(egress): prevent DNS TTL expiry from killing long-lived TCP connections#763
fix(egress): prevent DNS TTL expiry from killing long-lived TCP connections#763kinwin-ustc wants to merge 1 commit into
Conversation
| // DNS-learned allow_out_v2 entries are now written with expires_at_ns=0 | ||
| // (never expire) so that long-lived TCP connections survive DNS TTL | ||
| // rotation. Cleanup of stale IPs happens on policy replace | ||
| // (replaceNetPolicy flushes the inner map) or sandbox teardown. |
There was a problem hiding this comment.
The comment on line 12-16 explains why reapDNSLearnedPolicies is no longer called here, but the function (and its helper reapDNSLearnedPoliciesForInnerMap at lines 31-89) remain defined with no callers. This is dead code that will bitrot: it compiles fine today even if the eBPF map iteration logic inside it breaks, because Go has no dead-code compilation errors.
Consider deleting both functions in this same PR, now that the sole call site has been removed. The comment at lines 11-16 already documents the design intent, so future readers won't wonder about the deletion.
…ctions DNS-learned allow_out_v2 entries were expiring after DNS TTL, causing the eBPF datapath to RST established TCP connections when the TTL elapsed — even though the domain was still allowed by L7 policy. Changes: - Set expires_at_ns=0 for DNS-learned IPs so they persist until policy change or sandbox teardown (replaceNetPolicy flushes the map). - Increase CubeEgress proxy_read_timeout/proxy_send_timeout from 60s to 3600s to support long-lived HTTP connections (streaming, SSE, gRPC). The periodic reapDNSLearnedPolicies reaper is intentionally retained so that future changes can re-enable TTL-based expiration if needed. Since all entries now have expires_at_ns=0, the reaper is effectively a no-op but remains as infrastructure for future use. Stale IPs are cleaned up on policy replacement and sandbox deletion. New DNS responses continue to add/update entries normally. Signed-off-by: Feng King <kinwin2008@gmail.com> Assisted-by: Cursor:claude-opus-4-8-thinking-high Co-authored-by: Cursor <cursoragent@cursor.com>
d98d50f to
094c647
Compare
| proxy_send_timeout 60s; | ||
| proxy_read_timeout 60s; | ||
| proxy_send_timeout 3600s; | ||
| proxy_read_timeout 3600s; |
There was a problem hiding this comment.
Consider adding a comment explaining the rationale for 3600s (e.g., "DNS-learned entries no longer expire on TTL, so connections may survive for the full hour"). Without context, a future maintainer might revert these to a lower default, reintroducing the connection-killing issue this PR fixes.
| proxy_send_timeout 60s; | ||
| proxy_read_timeout 60s; | ||
| proxy_send_timeout 3600s; | ||
| proxy_read_timeout 3600s; |
There was a problem hiding this comment.
The proxy_buffering on (line 86) with proxy_request_buffering on (line 87) means nginx buffers the full client request body before forwarding upstream, and buffers the full upstream response before sending to the client. Combined with the 3600s timeout, this can affect streaming/gRPC/SSE workloads where low-latency forwarding of partial data is expected. Consider whether proxy_request_buffering off; is needed for streaming use cases.
Review: PR #763 — fix(egress): prevent DNS TTL expiry from killing long-lived TCP connectionsOverall assessmentThe PR correctly addresses the core bug: DNS-learned Documentation stale after this change1.
DNS-learned entries now also use 2.
These are now persistent entries, not temporary. Suggest replacing "temporary" with "persistent (cleaned up on policy change or sandbox teardown)." 3. The 60s→3600s timeout increase has no explanatory comment. A future maintainer has no context and may revert the values. Consider adding: 4. BPF map exhaustion risk (Medium)
Consider:
Stale DNS-learned entries after policy updates (Medium)
Test coverage gapsThe following areas lack test coverage directly relevant to this PR:
PR test plan note: The "policy removal mid-connection" scenario should explicitly verify the connection is terminated via policy-change-triggered flush ( Minor observations
Summary
|
| struct lpm_key key = { .prefixlen = 32, .ip = ip }; | ||
| struct net_policy_value_v2 value = { | ||
| .expires_at_ns = bpf_ktime_get_ns() + ((__u64)ttl * NSEC_PER_SEC), | ||
| .expires_at_ns = 0, |
There was a problem hiding this comment.
This will block DNS reaper to do its job and the allow_out_v2 map will get full.
Summary
allow_out_v2entries now persist (expires_at_ns=0) instead of expiring after DNS TTL, preventing the eBPF datapath from RST'ing established TCP connections when TTL elapses.proxy_read_timeout/proxy_send_timeoutfrom 60s to 3600s to support long-lived HTTP connections (streaming, SSE, gRPC).reapDNSLearnedPoliciesreaper is intentionally retained (currently a no-op since all entries haveexpires_at_ns=0) to preserve infrastructure for future TTL-based expiration if needed.Background
When a sandbox has a long-lived TCP connection to a domain allowed via L7 network policy, the eBPF datapath (
check_net_policyinfrom_cube) was killing the connection after DNS TTL expired — even though the domain was still in the allow list. This happened because:dns_learn_response_ip()setexpires_at_ns = now + DNS_TTLcheck_net_policy()treated the entry as absentallow_internet_access=false, traffic fell todeny_out(0.0.0.0/0) → TCP RSTChanges
CubeNet/src/dns_response.hexpires_at_ns=0indns_learn_response_ip()— entries never expireCubeEgress/nginx.confproxy_read_timeout/proxy_send_timeout: 60s → 3600sTest plan