mirror of
https://github.com/tailscale/tailscale.git
synced 2024-12-04 23:45:34 +00:00
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 fromdf9ce972c7
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> (cherry picked from commit06aa141632
)
This commit is contained in:
parent
029508d8be
commit
2b4140ee46
@ -13,7 +13,7 @@
|
|||||||
// CallbackRouter is an implementation of both Router and dns.OSConfigurator.
|
// CallbackRouter is an implementation of both Router and dns.OSConfigurator.
|
||||||
// When either network or DNS settings are changed, SetBoth is called with both configs.
|
// 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
|
// 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 {
|
type CallbackRouter struct {
|
||||||
SetBoth func(rcfg *Config, dcfg *dns.OSConfig) error
|
SetBoth func(rcfg *Config, dcfg *dns.OSConfig) error
|
||||||
SplitDNS bool
|
SplitDNS bool
|
||||||
@ -39,6 +39,9 @@ func (r *CallbackRouter) Up() error {
|
|||||||
func (r *CallbackRouter) Set(rcfg *Config) error {
|
func (r *CallbackRouter) Set(rcfg *Config) error {
|
||||||
r.mu.Lock()
|
r.mu.Lock()
|
||||||
defer r.mu.Unlock()
|
defer r.mu.Unlock()
|
||||||
|
if r.rcfg.Equal(rcfg) {
|
||||||
|
return nil
|
||||||
|
}
|
||||||
r.rcfg = rcfg
|
r.rcfg = rcfg
|
||||||
return r.SetBoth(r.rcfg, r.dcfg)
|
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 {
|
func (r *CallbackRouter) SetDNS(dcfg dns.OSConfig) error {
|
||||||
r.mu.Lock()
|
r.mu.Lock()
|
||||||
defer r.mu.Unlock()
|
defer r.mu.Unlock()
|
||||||
|
if r.dcfg != nil && r.dcfg.Equal(dcfg) {
|
||||||
|
return nil
|
||||||
|
}
|
||||||
r.dcfg = &dcfg
|
r.dcfg = &dcfg
|
||||||
return r.SetBoth(r.rcfg, r.dcfg)
|
return r.SetBoth(r.rcfg, r.dcfg)
|
||||||
}
|
}
|
||||||
|
@ -7,6 +7,8 @@
|
|||||||
package router
|
package router
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"reflect"
|
||||||
|
|
||||||
"golang.zx2c4.com/wireguard/tun"
|
"golang.zx2c4.com/wireguard/tun"
|
||||||
"inet.af/netaddr"
|
"inet.af/netaddr"
|
||||||
"tailscale.com/types/logger"
|
"tailscale.com/types/logger"
|
||||||
@ -72,6 +74,16 @@ type Config struct {
|
|||||||
NetfilterMode preftype.NetfilterMode // how much to manage netfilter rules
|
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
|
// shutdownConfig is a routing configuration that removes all router
|
||||||
// state from the OS. It's the config used when callers pass in a nil
|
// state from the OS. It's the config used when callers pass in a nil
|
||||||
// Config.
|
// Config.
|
||||||
|
@ -2,12 +2,15 @@
|
|||||||
// Use of this source code is governed by a BSD-style
|
// Use of this source code is governed by a BSD-style
|
||||||
// license that can be found in the LICENSE file.
|
// license that can be found in the LICENSE file.
|
||||||
|
|
||||||
//go:build linux || windows
|
|
||||||
// +build linux windows
|
|
||||||
|
|
||||||
package router
|
package router
|
||||||
|
|
||||||
import "inet.af/netaddr"
|
import (
|
||||||
|
"reflect"
|
||||||
|
"testing"
|
||||||
|
|
||||||
|
"inet.af/netaddr"
|
||||||
|
"tailscale.com/types/preftype"
|
||||||
|
)
|
||||||
|
|
||||||
func mustCIDRs(ss ...string) []netaddr.IPPrefix {
|
func mustCIDRs(ss ...string) []netaddr.IPPrefix {
|
||||||
var ret []netaddr.IPPrefix
|
var ret []netaddr.IPPrefix
|
||||||
@ -16,3 +19,127 @@ func mustCIDRs(ss ...string) []netaddr.IPPrefix {
|
|||||||
}
|
}
|
||||||
return ret
|
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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
Loading…
Reference in New Issue
Block a user