From 2b4140ee4609916562f22e020849ec224a00ce35 Mon Sep 17 00:00:00 2001 From: Mihai Parparita Date: Tue, 28 Jun 2022 15:32:09 -0700 Subject: [PATCH] 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 df9ce972c79023e0b0535bffee6afb3d88e61dc3 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 (cherry picked from commit 06aa1416325424ab2881a9e2442697f9707f363b) --- wgengine/router/callback.go | 8 +- wgengine/router/router.go | 12 +++ wgengine/router/router_test.go | 135 ++++++++++++++++++++++++++++++++- 3 files changed, 150 insertions(+), 5 deletions(-) diff --git a/wgengine/router/callback.go b/wgengine/router/callback.go index b66bec0c8..dd5676c80 100644 --- a/wgengine/router/callback.go +++ b/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) } diff --git a/wgengine/router/router.go b/wgengine/router/router.go index 0a162dc28..b71ae839e 100644 --- a/wgengine/router/router.go +++ b/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. diff --git a/wgengine/router/router_test.go b/wgengine/router/router_test.go index e493f1c48..fe068602c 100644 --- a/wgengine/router/router_test.go +++ b/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) + } + } +}