wgengine/router: avoid unncessary routing configuration changes #4965
Conversation
wgengine/router/router.go
Outdated
| } | ||
|
|
||
| for i := range a.LocalAddrs { | ||
| if a.LocalAddrs[i] != b.LocalAddrs[i] { |
There was a problem hiding this comment.
Wasn't sure if the "can compare with ==" comment in netaddr is only true for IP, or if we can also use it for IPPrefix
There was a problem hiding this comment.
also IPPrefix and IPPort and IPRange. I guess docs could be more clear.
| @@ -72,6 +72,49 @@ type Config struct { | |||
| NetfilterMode preftype.NetfilterMode // how much to manage netfilter rules | |||
| } | |||
|
|
|||
| func (a *Config) Equal(b *Config) bool { | |||
There was a problem hiding this comment.
it's probably worth adding a test like tailcfg's TestHostinfoEqual here, so a test fails if somebody adds a new field to Config and forgets to update this method.
At least just this part of the test:
func TestHostinfoEqual(t *testing.T) {
hiHandles := []string{
"IPNVersion", "FrontendLogID", "BackendLogID",
"OS", "OSVersion", "Desktop", "Package", "DeviceModel", "Hostname",
"ShieldsUp", "ShareeNode",
"GoArch",
"RoutableIPs", "RequestTags",
"Services", "NetInfo", "SSH_HostKeys",
}
if have := fieldsOf(reflect.TypeOf(Hostinfo{})); !reflect.DeepEqual(have, hiHandles) {
t.Errorf("Hostinfo.Equal check might be out of sync\nfields: %q\nhandled: %q\n",
have, hiHandles)
}There was a problem hiding this comment.
Would it also make sense to use reflect.DeepEqual in the implementation of Equal for Config (that's what Hostinfo does)?
There was a problem hiding this comment.
it's not as fast and can allocate and nil vs empty slices are different. So if those don't matter then that's fine.
There was a problem hiding this comment.
This is not a hotspot as far as I can tell, so I switched to it. Added tests too.
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 df9ce97 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>
mihaip
force-pushed
the
mihaip/chrome-network-error-changed
branch
from
a8c488a
to
cee69a3
Compare
Especially want to get tailscale/tailscale#4965 into an Open Testing release. Signed-off-by: Denton Gentry <dgentry@tailscale.com>
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
OneCGNATRouteflag from df9ce97 isused (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