Commit Graph

264 Commits (e2b3d9aa5fe6e73f12796e7af46deb4d3e13131c)

Author SHA1 Message Date
Brad Fitzpatrick 9643d8b34d wgengine/magicsock: add an addrLatency type to combine an IPPort+time.Duration
Updates #1566 (but no behavior changes as of this change)

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick 0994a9f7c4 wgengine{,/magicsock}: fix, improve "tailscale ping" to default routes and subnets
e.g.

$ tailscale ping 1.1.1.1
exit node found but not enabled

$ tailscale ping 10.2.200.2
node "tsbfvlan2" found, but not using its 10.2.200.0/24 route

$ sudo tailscale  up --accept-routes
$ tailscale ping 10.2.200.2
pong from tsbfvlan2 (100.124.196.94) via 10.2.200.34:41641 in 1ms

$ tailscale ping mon.ts.tailscale.com
pong from monitoring (100.88.178.64) via DERP(sfo) in 83ms
pong from monitoring (100.88.178.64) via DERP(sfo) in 21ms
pong from monitoring (100.88.178.64) via [2604:a880:4:d1::37:d001]:41641 in 22ms

This necessarily moves code up from magicsock to wgengine, so we can
look at the actual wireguard config.

Fixes #1564

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick 7e0d12e7cc wgengine/magicsock: don't update control if only endpoint order changes
Updates #1559

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick 32562a82a9 wgengine/magicsock: annotate a few more disco logs as verbose
Fixes #1540

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick ba8c6d0775 health, controlclient, ipn, magicsock: tell health package state of things
Not yet checking anything. Just plumbing states into the health package.

Updates #1505

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick 44ab0acbdb net/portmapper, wgengine/monitor: cache gateway IP info until link changes
Cuts down allocs & CPU in steady state (on regular STUN probes) when network
is unchanging.

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick c81814e4f8 derp{,/derphttp},magicsock: tell DERP server when ping acks can be expected
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick c576fea60e wgengine/magicsock: delete unused WhoIs method that was moved elsewhere
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick ef7bac2895 tailcfg, net/portmapper, wgengine/magicsock: add NetInfo.HavePortMap
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick 79d8288f0a wgengine/magicsock, derp, derp/derphttp: respond to DERP server->client pings
No server support yet, but we want Tailscale 1.6 clients to be able to respond
to them when the server can do it.

Updates #1310

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick 387e83c8fe wgengine/magicsock: fix Conn.Rebind race that let ErrClosed errors be read
There was a logical race where Conn.Rebind could acquire the
RebindingUDPConn mutex, close the connection, fail to rebind, release
the mutex, and then because the mutex was no longer held, ReceiveIPv4
wouldn't retry reads that failed with net.ErrClosed, letting that
error back to wireguard-go, which would then stop running that receive
IP goroutine.

Instead, keep the RebindingUDPConn mutex held for the entirety of the
replacement in all cases.

Updates tailscale/corp#1289

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick c445e3d327 wgengine/magicsock: fix typo in comment 4 years ago
Brad Fitzpatrick a6d098c750 wgengine/magicsock: log when DERP connection succeeds
Updates #1310
4 years ago
Brad Fitzpatrick 829eb8363a net/interfaces: sort returned addresses from LocalAddresses
Also change the type to netaddr.IP while here, because it made sorting
easier.

Updates tailscale/corp#1397

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick c3e5903b91 wgengine/magicsock: remove leftover portmapper debug logging
It's already logged at the right time in logEndpointChange.

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick ea3715e3ce wgengine/magicsock: remove TODO about endpoints-over-DERP
It was done in Tailscale 1.4 with CallMeMaybe disco messages
containing endpoints.

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick e9e4f1063d wgengine/magicsock: fix discoEndpoint caching bug when a node key changes
Fixes #1391

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick c64bd587ae net/portmapper: add NAT-PMP client, move port mapping service probing
* move probing out of netcheck into new net/portmapper package
* use PCP ANNOUNCE op codes for PCP discovery, rather than causing
  short-lived (sub-second) side effects with a 1-second-expiring map +
  delete.
* track when we heard things from the router so we can be less wasteful
  in querying the router's port mapping services in the future
* use portmapper from magicsock to map a public port

Fixes #1298
Fixes #1080
Fixes #1001
Updates #864

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Josh Bleecher Snyder 1632f9fd6b wgengine/magicsock: reduce log spam during tests
Only do the type assertion to *net.UDPAddr when addr is non-nil.
This prevents a bunch of log spam during tests.
4 years ago
Josh Bleecher Snyder 88586ec4a4 wgengine/magicsock: remove an alloc from ReceiveIPvN
We modified the standard net package to not allocate a *net.UDPAddr
during a call to (*net.UDPConn).ReadFromUDP if the caller's use
of the *net.UDPAddr does not cause it to escape.
That is https://golang.org/cl/291390.

This is the companion change to magicsock.
There are two changes required.
First, call ReadFromUDP instead of ReadFrom, if possible.
ReadFrom returns a net.Addr, which is an interface, which always allocates.
Second, reduce the lifetime of the returned *net.UDPAddr.
We do this by immediately converting it into a netaddr.IPPort.

We left the existing RebindingUDPConn.ReadFrom method in place,
as it is required to satisfy the net.PacketConn interface.

With the upstream change and both of these fixes in place,
we have removed one large allocation per packet received.

name           old time/op    new time/op    delta
ReceiveFrom-8    16.7µs ± 5%    16.4µs ± 8%     ~     (p=0.310 n=5+5)

name           old alloc/op   new alloc/op   delta
ReceiveFrom-8      112B ± 0%       64B ± 0%  -42.86%  (p=0.008 n=5+5)

name           old allocs/op  new allocs/op  delta
ReceiveFrom-8      3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)

Co-authored-by: Sonia Appasamy <sonia@tailscale.com>
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder 0c673c1344 wgengine/magicsock: unify on netaddr types in addrSet
addrSet maintained duplicate lists of netaddr.IPPorts and net.UDPAddrs.
Unify to use the netaddr type only.

This makes (*Conn).ReceiveIPvN a bit uglier,
but that'll be cleaned up in a subsequent commit.

This is preparatory work to remove an allocation from ReceiveIPv4.

Co-authored-by: Sonia Appasamy <sonia@tailscale.com>
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Brad Fitzpatrick 7e201806b1 wgengine/magicsock: reconnect to DERP home after network comes back up
Updates #1310
4 years ago
Brad Fitzpatrick 9b4e50cec0 wgengine/magicsock: fix typo in comment 4 years ago
Brad Fitzpatrick 6b365b0239 wgengine/magicsock: fix DERP reader hang regression during concurrent reads
Fixes #1282

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick 6d2b8df06d wgengine/magicsock: add disabled failing (deadlocking) test for #1282
The fix can make this test run unconditionally.

This moves code from 5c619882bc for
testability but doesn't fix it yet. The #1282 problem remains (when I
wrote its wake-up mechanism, I forgot there were N DERP readers
funneling into 1 UDP reader, and the code just isn't correct at all
for that case).

Also factor out some test helper code from BenchmarkReceiveFrom.

The refactoring in magicsock.go for testability should have no
behavior change.
4 years ago
Brad Fitzpatrick 1e7a35b225 types/netmap: split controlclient.NetworkMap off into its own leaf package
Updates #1278

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick 6064b6ff47 wgengine/wgcfg/nmcfg: split control/controlclient/netmap.go into own package
It couldn't move to ipnlocal due to test dependency cycles.

Updates #1278

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick f7eed25bb9 wgengine/magicsock: filter disco packets and packets when stopped from wireguard
Fixes #1167
Fixes tailscale/corp#219

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Josh Bleecher Snyder dd10babaed wgenginer/magicsock: remove Addrs methods
They are now unused.

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Brad Fitzpatrick 9541886856 wgengine/magicsock: disable regular STUNs for all platforms by default
Reduces background CPU & network.

Updates #1034

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick c55d26967b wgengine/magicsock: log more details of endpoints learned over disco
Also, don't try to use IPv6 LinkLocalUnicast addresses for now. Like endpoints
exchanged with control, we share them but don't yet use them.

Updates #1172
4 years ago
Brad Fitzpatrick 359055d3fa wgengine/magicsock: fix logging regression
c8c493f3d9 made it always say
`created=false` which scared me when I saw it, as that would've implied
things were broken much worse. Fortunately the logging was just wrong.
4 years ago
Brad Fitzpatrick edf64e0901 wgengine/magicsock: send, use endpoints in CallMeMaybe messages
Fixes #1172

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick b5b4992eff disco: support parsing/encoding endpoints in call-me-maybe frames
Updates #1172

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick 187e22a756 wgengine/magicsock: don't run the DERP cleanup so often
To save CPU and wakeups, don't run the DERP cleanup timer regularly
unless there is a non-home DERP connection open.

Also eliminates the goroutine, moving to a time.AfterFunc.

Updates #1034

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Josh Bleecher Snyder 5fe5402fcd Revert "wgengine/magicsock: shortcircuit discoEndpoint.heartbeat when its connection is closed"
This reverts commit 08baa17d9a.
It caused deadlocks due to lock ordering violations.
It was not the right fix, and thus should simply be reverted
while we look for the right fix (if we haven't already found it
in the interim; we've fixed other logging-after-test issues).

Fixes #1161
4 years ago
Brad Fitzpatrick edce91a8a6 wgengine/magicsock: fix a naked return bug/crash where we returned (nil, true)
The 'ok' from 'ipp, ok :=' above was the result parameter ok. Whoops.
4 years ago
Brad Fitzpatrick 51bd1feae4 wgengine/magicsock: add single element IPPort->endpoint cache in receive path
name           old time/op  new time/op  delta
ReceiveFrom-4  21.8µs ± 2%  20.9µs ± 2%  -4.27%  (p=0.000 n=10+10)

Updates #414

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick 5c619882bc wgengine/magicsock: simplify ReceiveIPv4+DERP path
name           old time/op  new time/op  delta
ReceiveFrom-4  35.8µs ± 3%  21.9µs ± 5%  -38.92%  (p=0.008 n=5+5)

Fixes #1145
Updates #414

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Brad Fitzpatrick 3fa86a8b23 wgengine/magicsock: use relatively new netaddr.IPPort.IsZero method 4 years ago
Brad Fitzpatrick 4811236189 wgengine/magicsock: speed up BenchmarkReceiveFrom, store context.Done chan
context.cancelCtx.Done involves a mutex and isn't as cheap as I
previously assumed. Convert the donec method into a struct field and
store the channel value once. Our one magicsock.Conn gets one pointer
larger, but it cuts ~1% of the CPU time of the ReceiveFrom benchmark
and removes a bubble from the --svg output :)
4 years ago
Josh Bleecher Snyder 63af950d8c wgengine/magicsock: adapt to wireguard-go without UpdateDst
22507adf54 stopped relying on
our fork of wireguard-go's UpdateDst callback.
As a result, we can unwind that code,
and the extra return value of ReceiveIPv{4,6}.

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
David Anderson 57d95dd005 wgengine/magicsock: default legacy networking to off for some tests.
Signed-off-by: David Anderson <danderson@tailscale.com>
4 years ago
David Anderson a2463e8948 wgengine/magicsock: add an option to disable legacy peer handling.
Used in tests to ensure we're not relying on behavior we're going
to remove eventually.

Signed-off-by: David Anderson <danderson@tailscale.com>
4 years ago
David Anderson d456bfdc6d wgengine/magicsock: fix BenchmarkReceiveFrom.
Previously, this benchmark relied on behavior of the legacy
receive codepath, which I changed in 22507adf. With this
change, the benchmark instead relies on the new active discovery
path.

Signed-off-by: David Anderson <danderson@tailscale.com>
4 years ago
Josh Bleecher Snyder 08baa17d9a wgengine/magicsock: shortcircuit discoEndpoint.heartbeat when its connection is closed
This prevents us from continuing to do unnecessary work
(including logging) after the connection has closed.

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder 7c76435bf7 wgengine/magicsock: simplify
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder 654b5f1570 all: convert from []wgcfg.Endpoint to string
This eliminates a dependency on wgcfg.Endpoint,
as part of the effort to eliminate our wireguard-go fork.

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
David Anderson 22507adf54 wgengine/magicsock: stop depending on UpdateDst in legacy codepaths.
This makes connectivity between ancient and new tailscale nodes slightly
worse in some cases, but only in cases where the ancient version would
likely have failed to get connectivity anyway.

Signed-off-by: David Anderson <danderson@tailscale.com>
4 years ago
Denton Gentry 0aa55bffce magicsock: test error case in derpWriteChanOfAddr
In derpWriteChanOfAddr when we call derphttp.NewRegionClient(),
there is a check of whether the connection is already errored and
if so it returns before grabbing the lock. The lock might already
be held and would be a deadlock.

This corner case is not being reliably exercised by other tests.
This shows up in code coverage reports, the lines of code in
derpWriteChanOfAddr are alternately added and subtracted from
code coverage.

Add a test to specifically exercise this code path, and verify that
it doesn't deadlock.

This is the best tradeoff I could come up with:
+ the moment code calls Err() to check if there is an error, we
  grab the lock to make sure it would deadlock if it tries to grab
  the lock itself.
+ if a new call to Err() is added in this code path, only the
  first one will be covered and the rest will not be tested.
+ this test doesn't verify whether code is checking for Err() in
  the right place, which ideally I guess it would.

Signed-off-by: Denton Gentry <dgentry@tailscale.com>
4 years ago