From 15a0504fb1bf8046db96da8970437b057fc59a2d Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Sat, 20 Jun 2026 00:32:49 +0900 Subject: [PATCH] [client] Treat answering upstreams as reachable and widen DNS health grace window (#6453) --- client/internal/dns/server.go | 33 +++++++++++-- client/internal/dns/server_test.go | 26 ++++++++++ client/internal/dns/upstream.go | 9 ++-- client/internal/dns/upstream_test.go | 72 ++++++++++++++++++++++++++++ 4 files changed, 133 insertions(+), 7 deletions(-) diff --git a/client/internal/dns/server.go b/client/internal/dns/server.go index dcd4cb9d0..77446b330 100644 --- a/client/internal/dns/server.go +++ b/client/internal/dns/server.go @@ -6,6 +6,7 @@ import ( "fmt" "net/netip" "net/url" + "os" "slices" "strings" "sync" @@ -38,11 +39,15 @@ const ( // defaultWarningDelayBase is the starting grace window before a // "Nameserver group unreachable" event fires for a group that's // never been healthy and only has overlay upstreams with no - // Connected peer. Per-server and overridable; see warningDelayFor. - defaultWarningDelayBase = 30 * time.Second + // Connected peer. Per-server and overridable via envWarningDelay; + // see warningDelay. + defaultWarningDelayBase = 60 * time.Second // warningDelayBonusCap caps the route-count bonus added to the - // base grace window. See warningDelayFor. + // base grace window. See warningDelay. warningDelayBonusCap = 30 * time.Second + // envWarningDelay overrides defaultWarningDelayBase with a Go duration + // string (e.g. "90s", "2m"). Invalid or non-positive values are ignored. + envWarningDelay = "NB_DNS_HEALTH_WARNING_DELAY" ) // errNoUsableNameservers signals that a merged-domain group has no usable @@ -298,7 +303,7 @@ func newDefaultServer( hostManager: &noopHostConfigurator{}, mgmtCacheResolver: mgmtCacheResolver, currentConfigHash: ^uint64(0), // Initialize to max uint64 to ensure first config is always applied - warningDelayBase: defaultWarningDelayBase, + warningDelayBase: warningDelayBaseFromEnv(), healthRefresh: make(chan struct{}, 1), } // Wire the local resolver against the peer status recorder so it can @@ -1154,6 +1159,26 @@ func (s *DefaultServer) projectUnhealthy(p *nsGroupProj, servers []netip.AddrPor return false } +// warningDelayBaseFromEnv returns the base grace window, honoring +// envWarningDelay when it holds a valid positive Go duration. Invalid or +// non-positive values fall back to defaultWarningDelayBase. +func warningDelayBaseFromEnv() time.Duration { + val := os.Getenv(envWarningDelay) + if val == "" { + return defaultWarningDelayBase + } + d, err := time.ParseDuration(val) + if err != nil { + log.Warnf("invalid %s value %q, using default %v: %v", envWarningDelay, val, defaultWarningDelayBase, err) + return defaultWarningDelayBase + } + if d <= 0 { + log.Warnf("%s must be positive, got %v, using default %v", envWarningDelay, d, defaultWarningDelayBase) + return defaultWarningDelayBase + } + return d +} + // warningDelay returns the grace window for the given selected-route // count. Scales gently: +1s per 100 routes, capped by // warningDelayBonusCap. Parallel handshakes mean handshake time grows diff --git a/client/internal/dns/server_test.go b/client/internal/dns/server_test.go index 722c2abd7..53d864115 100644 --- a/client/internal/dns/server_test.go +++ b/client/internal/dns/server_test.go @@ -2484,6 +2484,32 @@ func TestProjection_StopClearsHealthState(t *testing.T) { // rule 3: startup failures while the peer is handshaking, then the peer // comes up and a query succeeds before the grace window elapses. No // warning should ever have fired, and no recovery either. +func TestWarningDelayBaseFromEnv(t *testing.T) { + tests := []struct { + name string + set bool + val string + want time.Duration + }{ + {name: "unset uses default", set: false, want: defaultWarningDelayBase}, + {name: "valid override", set: true, val: "90s", want: 90 * time.Second}, + {name: "valid minutes", set: true, val: "2m", want: 2 * time.Minute}, + {name: "invalid falls back", set: true, val: "notaduration", want: defaultWarningDelayBase}, + {name: "zero falls back", set: true, val: "0s", want: defaultWarningDelayBase}, + {name: "negative falls back", set: true, val: "-30s", want: defaultWarningDelayBase}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Setenv(envWarningDelay, tc.val) + if !tc.set { + os.Unsetenv(envWarningDelay) + } + assert.Equal(t, tc.want, warningDelayBaseFromEnv(), "grace window base") + }) + } +} + func TestProjection_OverlayRecoversDuringGrace(t *testing.T) { fx := newProjTestFixture(t) fx.server.warningDelayBase = 200 * time.Millisecond diff --git a/client/internal/dns/upstream.go b/client/internal/dns/upstream.go index a4f713d68..9c0d00212 100644 --- a/client/internal/dns/upstream.go +++ b/client/internal/dns/upstream.go @@ -443,21 +443,25 @@ func (u *upstreamResolverBase) queryUpstream(parentCtx context.Context, r *dns.M return raceResult{}, &upstreamFailure{upstream: upstream, reason: "no response"} } + // A valid response means the upstream is reachable, whatever the Rcode. + u.markUpstreamOk(upstream) + proto := "" if upstreamProto != nil { proto = upstreamProto.protocol } if rm.Rcode == dns.RcodeServerFailure || rm.Rcode == dns.RcodeRefused { + // SERVFAIL and REFUSED are per-question outcomes (DNSSEC-bogus names, + // refused zones, transient recursion errors), not reachability + // problems: fail over for a better answer but keep the upstream healthy. if code, ok := nonRetryableEDE(rm); ok { if !hadEdns { stripOPT(rm) } - u.markUpstreamOk(upstream) return raceResult{msg: rm, upstream: upstream, protocol: proto, ede: edeName(code)}, nil } reason := dns.RcodeToString[rm.Rcode] - u.markUpstreamFail(upstream, reason) return raceResult{}, &upstreamFailure{upstream: upstream, reason: reason} } @@ -465,7 +469,6 @@ func (u *upstreamResolverBase) queryUpstream(parentCtx context.Context, r *dns.M stripOPT(rm) } - u.markUpstreamOk(upstream) return raceResult{msg: rm, upstream: upstream, protocol: proto}, nil } diff --git a/client/internal/dns/upstream_test.go b/client/internal/dns/upstream_test.go index 8b3c589f1..afd2053cc 100644 --- a/client/internal/dns/upstream_test.go +++ b/client/internal/dns/upstream_test.go @@ -517,6 +517,78 @@ func TestUpstreamResolver_HealthTracking(t *testing.T) { assert.NotContains(t, health, bad, "sibling upstream should not be queried when primary answers") } +// TestUpstreamResolver_HealthTracking_ResponseMeansReachable verifies that an +// upstream which answers with SERVFAIL or REFUSED is recorded as healthy: +// those are per-question outcomes from a reachable server and must not mark +// the upstream unhealthy. Only transport failures (timeouts) do. +func TestUpstreamResolver_HealthTracking_ResponseMeansReachable(t *testing.T) { + a := netip.MustParseAddrPort("192.0.2.10:53") + b := netip.MustParseAddrPort("192.0.2.11:53") + timeoutErr := &net.OpError{Op: "read", Err: fmt.Errorf("i/o timeout")} + + tests := []struct { + name string + respA mockUpstreamResponse + respB mockUpstreamResponse + wantHealthy bool + }{ + { + name: "both SERVFAIL are reachable", + respA: mockUpstreamResponse{msg: buildMockResponse(dns.RcodeServerFailure, "")}, + respB: mockUpstreamResponse{msg: buildMockResponse(dns.RcodeServerFailure, "")}, + wantHealthy: true, + }, + { + name: "both REFUSED are reachable", + respA: mockUpstreamResponse{msg: buildMockResponse(dns.RcodeRefused, "")}, + respB: mockUpstreamResponse{msg: buildMockResponse(dns.RcodeRefused, "")}, + wantHealthy: true, + }, + { + name: "timeout marks unhealthy", + respA: mockUpstreamResponse{err: timeoutErr}, + respB: mockUpstreamResponse{err: timeoutErr}, + wantHealthy: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + mockClient := &mockUpstreamResolverPerServer{ + responses: map[string]mockUpstreamResponse{ + a.String(): tc.respA, + b.String(): tc.respB, + }, + rtt: time.Millisecond, + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + resolver := &upstreamResolverBase{ + ctx: ctx, + upstreamClient: mockClient, + upstreamTimeout: UpstreamTimeout, + } + resolver.addRace([]netip.AddrPort{a, b}) + + responseWriter := &test.MockResponseWriter{WriteMsgFunc: func(m *dns.Msg) error { return nil }} + resolver.ServeDNS(responseWriter, new(dns.Msg).SetQuestion("example.com.", dns.TypeA)) + + health := resolver.UpstreamHealth() + require.Contains(t, health, a, "primary upstream should have a health record") + if tc.wantHealthy { + assert.False(t, health[a].LastOk.IsZero(), "responding upstream should have LastOk set") + assert.True(t, health[a].LastFail.IsZero(), "responding upstream should not be marked failed") + assert.Empty(t, health[a].LastErr, "responding upstream should have no error") + } else { + assert.False(t, health[a].LastFail.IsZero(), "timed-out upstream should be marked failed") + assert.NotEmpty(t, health[a].LastErr, "timed-out upstream should record an error") + } + }) + } +} + func TestFormatFailures(t *testing.T) { testCases := []struct { name string