wgengine/router: avoid unncessary routing configuration changes

The iOS and macOS networking extension API only exposes a single setter
for the entire routing and DNS configuration, and does not appear to
do any kind of diffing or deltas when applying changes. This results
in spurious "network changed" errors in Chrome, even when the
`OneCGNATRoute` flag from df9ce972c7 is
used (because we're setting the same configuration repeatedly).

Since we already keep track of the current routing and DNS configuration
in CallbackRouter, use that to detect if they're actually changing, and
only invoke the platform setter if it's actually necessary.

Updates #3102

Signed-off-by: Mihai Parparita <mihai@tailscale.com>

wgengine/router/callback.go

@@ -13,7 +13,7 @@
 // CallbackRouter is an implementation of both Router and dns.OSConfigurator.
 // When either network or DNS settings are changed, SetBoth is called with both configs.
 // Mainly used as a shim for OSes that want to set both network and
-// DNS configuration simultaneously (iOS, android).
+// DNS configuration simultaneously (Mac, iOS, Android).
 type CallbackRouter struct {
 	SetBoth  func(rcfg *Config, dcfg *dns.OSConfig) error
 	SplitDNS bool
@@ -39,6 +39,9 @@ func (r *CallbackRouter) Up() error {
 func (r *CallbackRouter) Set(rcfg *Config) error {
 	r.mu.Lock()
 	defer r.mu.Unlock()
+	if r.rcfg.Equal(rcfg) {
+		return nil
+	}
 	r.rcfg = rcfg
 	return r.SetBoth(r.rcfg, r.dcfg)
 }
@@ -47,6 +50,9 @@ func (r *CallbackRouter) Set(rcfg *Config) error {
 func (r *CallbackRouter) SetDNS(dcfg dns.OSConfig) error {
 	r.mu.Lock()
 	defer r.mu.Unlock()
+	if r.dcfg != nil && r.dcfg.Equal(dcfg) {
+		return nil
+	}
 	r.dcfg = &dcfg
 	return r.SetBoth(r.rcfg, r.dcfg)
 }

wgengine/router/router.go

@@ -7,6 +7,8 @@
 package router
 
 import (
+	"reflect"
+
 	"golang.zx2c4.com/wireguard/tun"
 	"inet.af/netaddr"
 	"tailscale.com/types/logger"
@@ -72,6 +74,16 @@ type Config struct {
 	NetfilterMode    preftype.NetfilterMode // how much to manage netfilter rules
 }
 
+func (a *Config) Equal(b *Config) bool {
+	if a == nil && b == nil {
+		return true
+	}
+	if (a == nil) != (b == nil) {
+		return false
+	}
+	return reflect.DeepEqual(a, b)
+}
+
 // shutdownConfig is a routing configuration that removes all router
 // state from the OS. It's the config used when callers pass in a nil
 // Config.

wgengine/router/router_test.go

@@ -2,12 +2,15 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-//go:build linux || windows
-// +build linux windows
-
 package router
 
-import "inet.af/netaddr"
+import (
+	"reflect"
+	"testing"
+
+	"inet.af/netaddr"
+	"tailscale.com/types/preftype"
+)
 
 func mustCIDRs(ss ...string) []netaddr.IPPrefix {
 	var ret []netaddr.IPPrefix
@@ -16,3 +19,127 @@ func mustCIDRs(ss ...string) []netaddr.IPPrefix {
 	}
 	return ret
 }
+
+func TestConfigEqual(t *testing.T) {
+	testedFields := []string{
+		"LocalAddrs", "Routes", "LocalRoutes", "SubnetRoutes",
+		"SNATSubnetRoutes", "NetfilterMode",
+	}
+	configType := reflect.TypeOf(Config{})
+	configFields := []string{}
+	for i := 0; i < configType.NumField(); i++ {
+		configFields = append(configFields, configType.Field(i).Name)
+	}
+	if !reflect.DeepEqual(configFields, testedFields) {
+		t.Errorf("Config.Equal check might be out of sync\nfields: %q\nhandled: %q\n",
+			configFields, testedFields)
+	}
+
+	nets := func(strs ...string) (ns []netaddr.IPPrefix) {
+		for _, s := range strs {
+			n, err := netaddr.ParseIPPrefix(s)
+			if err != nil {
+				panic(err)
+			}
+			ns = append(ns, n)
+		}
+		return ns
+	}
+	tests := []struct {
+		a, b *Config
+		want bool
+	}{
+		{
+			nil,
+			nil,
+			true,
+		},
+		{
+			&Config{},
+			nil,
+			false,
+		},
+		{
+			nil,
+			&Config{},
+			false,
+		},
+		{
+			&Config{},
+			&Config{},
+			true,
+		},
+
+		{
+			&Config{LocalAddrs: nets("100.1.27.82/32")},
+			&Config{LocalAddrs: nets("100.2.19.82/32")},
+			false,
+		},
+		{
+			&Config{LocalAddrs: nets("100.1.27.82/32")},
+			&Config{LocalAddrs: nets("100.1.27.82/32")},
+			true,
+		},
+
+		{
+			&Config{Routes: nets("100.1.27.0/24")},
+			&Config{Routes: nets("100.2.19.0/24")},
+			false,
+		},
+		{
+			&Config{Routes: nets("100.2.19.0/24")},
+			&Config{Routes: nets("100.2.19.0/24")},
+			true,
+		},
+
+		{
+			&Config{LocalRoutes: nets("100.1.27.0/24")},
+			&Config{LocalRoutes: nets("100.2.19.0/24")},
+			false,
+		},
+		{
+			&Config{LocalRoutes: nets("100.1.27.0/24")},
+			&Config{LocalRoutes: nets("100.1.27.0/24")},
+			true,
+		},
+
+		{
+			&Config{SubnetRoutes: nets("100.1.27.0/24")},
+			&Config{SubnetRoutes: nets("100.2.19.0/24")},
+			false,
+		},
+		{
+			&Config{SubnetRoutes: nets("100.1.27.0/24")},
+			&Config{SubnetRoutes: nets("100.1.27.0/24")},
+			true,
+		},
+
+		{
+			&Config{SNATSubnetRoutes: false},
+			&Config{SNATSubnetRoutes: true},
+			false,
+		},
+		{
+			&Config{SNATSubnetRoutes: false},
+			&Config{SNATSubnetRoutes: false},
+			true,
+		},
+
+		{
+			&Config{NetfilterMode: preftype.NetfilterOff},
+			&Config{NetfilterMode: preftype.NetfilterNoDivert},
+			false,
+		},
+		{
+			&Config{NetfilterMode: preftype.NetfilterNoDivert},
+			&Config{NetfilterMode: preftype.NetfilterNoDivert},
+			true,
+		},
+	}
+	for i, tt := range tests {
+		got := tt.a.Equal(tt.b)
+		if got != tt.want {
+			t.Errorf("%d. Equal = %v; want %v", i, got, tt.want)
+		}
+	}
+}