configure DNS even when router.Set fails (#20488)

* wgengine: configure DNS even when router.Set fails Reconfig configured the router first and returned on any router.Set error, before the DNS block ran. On a host where router config fails on every reconfig -- e.g. a tun MTU below 1280 that breaks IPv6, or a kernel missing netfilter features -- the OS resolver was never told about MagicDNS or the tailnet search domain, so tailnet names failed to resolve with no DNS error in the logs. Record the router error and continue instead of returning on it, still attempt dns.Set, and join the router, DNS, and VPN-reconfigure errors into the return value. DNS stays after router config (still needed: some DNS managers refuse to apply settings before the device has an address); only the error coupling is broken. Fixes a regression from 84430cdfa (v1.8.0). Updates #20447 Signed-off-by: Brendan Creane <bcreane@gmail.com> * wgengine/router/osrouter: gate IPv6 on per-interface support, not just global getV6Available reported IPv6 usable whenever the netfilter runner reported global IPv6 support, missing the case where the kernel has IPv6 but has not enabled it on tailscale0 specifically -- e.g. when the tun MTU is below the 1280-byte IPv6 minimum, so /proc/sys/net/ipv6/conf/tailscale0 never exists and the v6 address and route adds fail, aborting the whole Set. See #20447. AND a per-interface check into getV6Available, evaluated per call so a later Set picks up v6 if the interface gains it. All v6-gated operations funnel through getV6Available, so Set now skips v6 gracefully instead of erroring. Also remove the dead r.v6Available field that masked this with its global name. Updates #20447 Signed-off-by: Brendan Creane <bcreane@gmail.com> --------- Signed-off-by: Brendan Creane <bcreane@gmail.com>
2026-07-16 14:53:30 -07:00
parent b2de420e3d
commit cfd101f9d7
4 changed files with 234 additions and 20 deletions
@@ -13,6 +13,7 @@ import (
 	"net/netip"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"strconv"
 	"strings"
 	"sync"
@@ -74,9 +75,21 @@ type linuxRouter struct {

 	// Various feature checks for the network stack.
 	ipRuleAvailable bool     // whether kernel was built with IP_MULTIPLE_TABLES
-	v6Available     bool     // whether the kernel supports IPv6
 	fwmaskWorksLazy opt.Bool // whether we can use 'ip rule...fwmark <mark>/<mask>'; set lazily

+	// interfaceV6Usable reports whether the kernel has IPv6 enabled on the
+	// tunnel interface specifically (distinct from global IPv6 support,
+	// which the netfilter runner tracks). Always set: the constructor wires
+	// it to interfaceV6UsableForTun; tests override it. See #20447.
+	interfaceV6Usable func() bool
+
+	// interfaceV6UsableMemo memoizes interfaceV6Usable for the duration of a
+	// single Set, so its many getV6Available calls don't each hit /proc. It's
+	// an atomic tri-state (see the memoV6 constants) because getV6Available is
+	// also reached, without holding mu, from the onIPRuleDeleted timer's
+	// justAddIPRules; the unset value there means "read live". See #20447.
+	interfaceV6UsableMemo atomic.Int32
+
 	// ipPolicyPrefBase is the base priority at which ip rules are installed.
 	ipPolicyPrefBase int

@@ -124,6 +137,7 @@ func newUserspaceRouterAdvanced(logf logger.Logf, tunname string, netMon *netmon
 		ipRuleFixLimiter: rate.NewLimiter(rate.Every(5*time.Second), 10),
 		ipPolicyPrefBase: 5200,
 	}
+	r.interfaceV6Usable = func() bool { return interfaceV6UsableForTun(r.tunname) }
 	ec := bus.Client("router-linux")
 	r.rulesAddedPub = eventbus.Publish[AddIPRules](ec)
 	eventbus.SubscribeFunc(ec, func(rs netmon.RuleDeleted) {
@@ -172,8 +186,6 @@ func newUserspaceRouterAdvanced(logf logger.Logf, tunname string, netMon *netmon
 		r.logf("mwan3 on openWRT detected, switching policy base priority to 1300")
 	}

-	r.v6Available = linuxfw.CheckIPv6(r.logf) == nil
-
 	r.fixupWSLMTU()

 	return r, nil
@@ -421,6 +433,11 @@ func (r *linuxRouter) setupNetfilterLocked(kind string) error {
 func (r *linuxRouter) Set(cfg *router.Config) error {
 	r.mu.Lock()
 	defer r.mu.Unlock()
+	// Memoize the tun's IPv6 usability for the duration of this Set so the
+	// per-address/route getV6Available calls don't each re-read /proc.
+	// snapshotV6Usable runs now; the closure it returns (trailing ()) is
+	// deferred to clear the memo on return.
+	defer r.snapshotV6Usable()()
 	var errs []error
 	if cfg == nil {
 		cfg = &shutdownConfig
@@ -893,11 +910,72 @@ func (r *linuxRouter) getV6FilteringAvailable() bool {
 // getV6Available reports whether the router can manage IPv6. r.nfr can be nil if
 // setupNetfilterLocked failed earlier in Set (which continues on error), so
 // treat a nil runner as no IPv6 rather than dereferencing it.
+//
+// It requires both global IPv6 support (the netfilter runner) and IPv6 on the
+// tun interface itself, which can differ: the kernel may refuse IPv6 on the tun
+// while global IPv6 is fine. The per-interface check consults /proc, so within
+// a single Set (which calls this once per address and route) the result is
+// snapshotted by snapshotV6Usable rather than re-read each time. See #20447.
 func (r *linuxRouter) getV6Available() bool {
 	if r.nfr == nil {
 		return false
 	}
-	return r.nfr.HasIPV6()
+	switch memoV6(r.interfaceV6UsableMemo.Load()) {
+	case memoV6Usable:
+		return r.nfr.HasIPV6()
+	case memoV6Unusable:
+		return false
+	default: // memoV6Unset: no snapshot active, read live.
+		return r.nfr.HasIPV6() && r.interfaceV6Usable()
+	}
+}
+
+// memoV6 is the state of a linuxRouter.interfaceV6UsableMemo snapshot.
+type memoV6 int32
+
+const (
+	memoV6Unset    memoV6 = iota // no snapshot active; read live
+	memoV6Usable                 // snapshot: IPv6 usable on the tun interface
+	memoV6Unusable               // snapshot: IPv6 not usable on the tun interface
+)
+
+// snapshotV6Usable memoizes interfaceV6Usable for the duration of a Set, so
+// its many getV6Available calls don't each hit /proc. It returns a function
+// that clears the snapshot, intended to be deferred. The snapshot is taken
+// once: an IPv6-enabled flip concurrent with a single Set is rare and, since a
+// stray v6 operation is no longer fatal, harmless until the next Set. See
+// #20447.
+func (r *linuxRouter) snapshotV6Usable() func() {
+	m := memoV6Unusable
+	if r.interfaceV6Usable() {
+		m = memoV6Usable
+	}
+	r.interfaceV6UsableMemo.Store(int32(m))
+	return func() { r.interfaceV6UsableMemo.Store(int32(memoV6Unset)) }
+}
+
+// interfaceV6UsableForTun reports whether the kernel has IPv6 enabled on the
+// named interface. The kernel creates /proc/sys/net/ipv6/conf/<iface>/ only
+// once IPv6 is up on the interface (e.g. an MTU below the 1280-byte IPv6
+// minimum removes it entirely), and disable_ipv6 within it reflects whether
+// IPv6 has since been turned off explicitly.
+func interfaceV6UsableForTun(tunname string) bool {
+	if tunname == "" {
+		return false
+	}
+	bs, err := os.ReadFile(filepath.Join("/proc/sys/net/ipv6/conf", tunname, "disable_ipv6"))
+	if err != nil {
+		// A missing directory/knob means IPv6 isn't up on the interface, so
+		// it's unavailable. Any other error (e.g. EACCES) means the knob
+		// exists but we couldn't read it; assume IPv6 is usable rather than
+		// skipping it on a transient error.
+		return !os.IsNotExist(err)
+	}
+	disabled, err := strconv.ParseBool(strings.TrimSpace(string(bs)))
+	if err != nil {
+		return true // unparseable; assume usable
+	}
+	return !disabled
 }

 // addAddress adds an IP/mask to the tunnel interface. Fails if the
@@ -525,6 +525,10 @@ v6/nat/ts-postrouting -m mark --mark 0x40000/0xff0000 -j MASQUERADE
 	ht := health.NewTracker(bus)
 	router, err := newUserspaceRouterAdvanced(t.Logf, "tailscale0", mon, fake, ht, bus)
 	router.(*linuxRouter).nfr = fake.nfr
+	// Don't consult the live /proc for the tun's IPv6 state in tests; the
+	// fake netfilter runner's HasIPV6 (noV6) is the authoritative v6 signal
+	// here. See #20447.
+	router.(*linuxRouter).interfaceV6Usable = func() bool { return true }
 	if err != nil {
 		t.Fatalf("failed to create router: %v", err)
 	}
@@ -1614,6 +1618,9 @@ func TestSetSkipsNetfilterAddonsWhenSetupFails(t *testing.T) {
 	}
 	lr := r.(*linuxRouter)
 	lr.nfr = nfr
+	// Keep the fake netfilter runner's HasIPV6 the sole v6 signal; don't
+	// consult the live /proc for the tun's IPv6 state. See #20447.
+	lr.interfaceV6Usable = func() bool { return true }
 	if err := lr.Up(); err != nil {
 		t.Fatalf("Up: %v", err)
 	}
@@ -1661,6 +1668,9 @@ func newTestLinuxRouter(t *testing.T) (*linuxRouter, *fakeOS) {
 	}
 	lr := r.(*linuxRouter)
 	lr.nfr = fake.nfr
+	// Keep the fake netfilter runner's HasIPV6 (noV6) the sole v6 signal;
+	// don't consult the live /proc for the tun's IPv6 state. See #20447.
+	lr.interfaceV6Usable = func() bool { return true }
 	if err := lr.Up(); err != nil {
 		t.Fatalf("Up: %v", err)
 	}
@@ -1933,3 +1943,57 @@ func TestSetSkipsV6OrphansWhenV6Unavailable(t *testing.T) {
 		t.Errorf("v6 orphan should be left alone when v6 is unavailable; ips=%q", fake.ips)
 	}
 }
+
+// TestSetSkipsV6WhenInterfaceV6Unusable verifies that when global IPv6 is
+// available (the netfilter runner reports HasIPV6) but the kernel has not
+// enabled IPv6 on the tunnel interface itself -- e.g. the tun MTU is below the
+// 1280-byte IPv6 minimum -- Set skips the v6 address rather than failing. This
+// is the #20447 scenario: historically the v6 addAddress errored, aborting the
+// whole router pass, which in turn suppressed DNS configuration.
+func TestSetSkipsV6WhenInterfaceV6Unusable(t *testing.T) {
+	lr, fake := newTestLinuxRouter(t)
+	// Global v6 is fine (fake nfr HasIPV6 defaults true), but the tun
+	// interface has no usable v6.
+	lr.interfaceV6Usable = func() bool { return false }
+
+	cfg := &Config{
+		LocalAddrs:    mustCIDRs("100.64.0.1/32", "fd7a:115c:a1e0::1/128"),
+		NetfilterMode: netfilterOff,
+	}
+	if err := lr.Set(cfg); err != nil {
+		t.Fatalf("Set: %v", err)
+	}
+
+	// The v4 address is programmed; the v6 address is skipped rather than
+	// attempted (which would have errored on a v6-less interface).
+	if !slices.Contains(fake.ips, "100.64.0.1/32 dev tailscale0") {
+		t.Errorf("v4 address was not programmed; ips=%q", fake.ips)
+	}
+	if slices.Contains(fake.ips, "fd7a:115c:a1e0::1/128 dev tailscale0") {
+		t.Errorf("v6 address should be skipped when the interface has no usable v6; ips=%q", fake.ips)
+	}
+}
+
+// TestSetSnapshotsV6Usable verifies that a single Set consults the
+// per-interface IPv6 check (which reads /proc) once, rather than once per
+// address and route, even though many getV6Available calls happen within it.
+func TestSetSnapshotsV6Usable(t *testing.T) {
+	lr, _ := newTestLinuxRouter(t)
+	var calls int
+	lr.interfaceV6Usable = func() bool {
+		calls++
+		return true
+	}
+
+	cfg := &Config{
+		LocalAddrs:    mustCIDRs("100.64.0.1/32", "fd7a:115c:a1e0::1/128"),
+		Routes:        mustCIDRs("10.0.0.0/8", "fd00::/8"),
+		NetfilterMode: netfilterOff,
+	}
+	if err := lr.Set(cfg); err != nil {
+		t.Fatalf("Set: %v", err)
+	}
+	if calls != 1 {
+		t.Errorf("interfaceV6Usable called %d times during one Set; want 1 (snapshotted)", calls)
+	}
+}
@@ -884,12 +884,15 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config,
 	// per peer by [Engine.SyncDevicePeer]), and its private key is set
 	// above when it changes.

+	// A router.Set error is recorded but must not abort the reconfig: DNS
+	// configuration below must be attempted independently. See #20447.
+	var routerErr error
 	if routerChanged {
 		e.logf("wgengine: Reconfig: configuring router")
-		err := e.router.Set(routerCfg)
-		e.health.SetRouterHealth(err)
-		if err != nil {
-			return err
+		routerErr = e.router.Set(routerCfg)
+		e.health.SetRouterHealth(routerErr)
+		if routerErr != nil {
+			e.logf("wgengine: Reconfig: router config failed (%v); continuing to DNS config so name resolution still works", routerErr)
 		}
 	}

@@ -902,6 +905,7 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config,
 	// TODO(bradfitz): try to do the "configuring DNS" part below only if
 	// dnsChanged, not routerChanged. The "resolver.ShouldUseRoutes" part
 	// probably needs to keep happening for both.
+	var dnsErr, vpnErr error
 	if buildfeatures.HasDNS && (routerChanged || dnsChanged) {
 		if resolver.ShouldUseRoutes(e.controlKnobs) {
 			e.logf("wgengine: Reconfig: user dialer")
@@ -914,31 +918,32 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config,
 		// DNS managers refuse to apply settings if the device has no
 		// assigned address.
 		e.logf("wgengine: Reconfig: configuring DNS")
-		err := e.dns.Set(*dnsCfg)
-		e.health.SetDNSHealth(err)
-		if err != nil {
-			return err
-		}
-		if err := e.reconfigureVPNIfNecessary(); err != nil {
-			return err
+		dnsErr = e.dns.Set(*dnsCfg)
+		e.health.SetDNSHealth(dnsErr)
+		if dnsErr == nil {
+			vpnErr = e.reconfigureVPNIfNecessary()
 		}
 	}

-	// Let the network flow logger finish reacting after the router is
-	// configured, so that a stopping logger captures final packets.
+	// Let the network flow logger finish reacting, pairing the Reconfig
+	// call near the top of this function. This runs regardless of router
+	// or DNS errors above: skipping it would leave a stopping logger
+	// running until the next successful reconfig.
 	// This may block to flush pending log messages.
 	if e.netlogger != nil {
 		e.netlogger.ReconfigDone()
 	}

-	// Let the BIRD integration apply any protocol state change now,
-	// after the router is configured.
+	// Let the BIRD integration apply any protocol state change computed by
+	// its Reconfig call above. As with the netlogger, this runs even if
+	// router/DNS config failed, so BIRD's protocol state still tracks the
+	// primary-subnet-router transition.
 	if e.bird != nil {
 		e.bird.ReconfigDone()
 	}

 	e.logf("[v1] wgengine: Reconfig done")
-	return nil
+	return errors.Join(routerErr, dnsErr, vpnErr)
 }

 func (e *userspaceEngine) GetFilter() *filter.Filter {
@@ -4,6 +4,7 @@
 package wgengine

 import (
+	"errors"
 	"fmt"
 	"math/rand"
 	"net/netip"
@@ -116,6 +117,72 @@ func TestUserspaceEngineReconfig(t *testing.T) {
 	}
 }

+// failingRouter is a router.Router whose Set always fails, used to verify that
+// DNS configuration is still attempted when router configuration fails.
+type failingRouter struct {
+	err error
+}
+
+func (failingRouter) Up() error                  { return nil }
+func (r failingRouter) Set(*router.Config) error { return r.err }
+func (failingRouter) Close() error               { return nil }
+
+// recordingOSConfigurator is a dns.OSConfigurator that records whether SetDNS
+// was called.
+type recordingOSConfigurator struct {
+	setDNSCalled bool
+}
+
+func (c *recordingOSConfigurator) SetDNS(dns.OSConfig) error { c.setDNSCalled = true; return nil }
+func (c *recordingOSConfigurator) SupportsSplitDNS() bool    { return false }
+func (c *recordingOSConfigurator) Close() error              { return nil }
+func (c *recordingOSConfigurator) GetBaseConfig() (dns.OSConfig, error) {
+	return dns.OSConfig{}, dns.ErrGetBaseConfigNotSupported
+}
+
+// TestUserspaceEngineReconfigDNSAfterRouterError verifies that a router.Set
+// failure does not prevent DNS from being configured. Historically Reconfig
+// returned on router error before dns.Set ran, so MagicDNS was never
+// configured on hosts where router config failed on every reconfig. See
+// tailscale/tailscale#20447.
+func TestUserspaceEngineReconfigDNSAfterRouterError(t *testing.T) {
+	bus := eventbustest.NewBus(t)
+
+	ht := health.NewTracker(bus)
+	reg := new(usermetric.Registry)
+	e, err := NewFakeUserspaceEngine(t.Logf, 0, ht, reg, bus)
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(e.Close)
+	ue := e.(*userspaceEngine)
+
+	routerErr := fmt.Errorf("router boom")
+	ue.router = failingRouter{err: routerErr}
+
+	osCfg := &recordingOSConfigurator{}
+	ue.dns = dns.NewManager(t.Logf, osCfg, ht, ue.dialer, nil, nil, runtime.GOOS, bus)
+
+	nm := &netmap.NetworkMap{
+		Peers: nodeViews([]*tailcfg.Node{{ID: 1, Key: nkFromHex("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")}}),
+	}
+	cfg := &wgcfg.Config{
+		Addresses: []netip.Prefix{netip.PrefixFrom(netaddr.IPv4(100, 100, 99, 1), 32)},
+	}
+	e.SetSelfNode(nm.SelfNode)
+
+	err = e.Reconfig(cfg, &router.Config{}, &dns.Config{})
+
+	if !osCfg.setDNSCalled {
+		t.Error("SetDNS was not called after router.Set failed; DNS config must be independent of router success")
+	}
+	if err == nil {
+		t.Error("Reconfig returned nil; want the router error to be surfaced")
+	} else if !errors.Is(err, routerErr) {
+		t.Errorf("Reconfig error = %v; want it to wrap the router error %v", err, routerErr)
+	}
+}
+
 func TestUserspaceEnginePortReconfig(t *testing.T) {
 	flakytest.Mark(t, "https://github.com/tailscale/tailscale/issues/2855")
 	const defaultPort = 49983