The Windows CI machine experiences significant random execution delays.
For example, in this code from watchdog.go:
done := make(chan bool)
go func() {
start := time.Now()
mu.Lock()
There was a 500ms delay from initializing done to locking mu.
This test checks that we receive a sufficient number of events quickly enough.
In the face of random 500ms delays, unsurprisingly, the test fails.
There's not much principled we can do about it.
We could build a system of retries or attempt to detect these random delays,
but that game isn't worth the candle.
Skip the test.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
This works around the close syscall being slow.
We can revert this if we find a fix or if Apple makes close fast again.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
When a handshake race occurs, a queued data packet can get lost.
TestTwoDevicePing expected that the very first data packet would arrive.
This caused occasional flakes.
Change TestTwoDevicePing to repeatedly re-send packets
and succeed when one of them makes it through.
This is acceptable (vs making WireGuard not drop the packets)
because this only affects communication with extremely old clients.
And those extremely old clients will eventually connect,
because the kernel will retry sends on timeout.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
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>
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>
I based my estimation of the required timeout based on locally
observed behavior. But CI machines are worse than my local machine.
16s was enough to reduce flakiness but not eliminate it. Bump it up again.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
This was in place because retrieved allowed_ips was very expensive.
Upstream changed the data structure to make them cheaper to compute.
This commit is an experiment to find out whether they're now cheap enough.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
We removed the "fast retry" code from our wireguard-go fork.
As a result, pings can take longer to transit when retries are required.
Allow that.
Fixes#1277
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Upstream wireguard-go decided to use errors.Is(err, net.ErrClosed)
instead of checking the error string.
It also provided an unsafe linknamed version of net.ErrClosed
for clients running Go 1.15. Switch to that.
This reduces the time required for the wgengine/magicsock tests
on my machine from ~35s back to the ~13s it was before
456cf8a3765948d6f1992162993eaf3844371592.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Use tb.Cleanup to simplify both the API and the implementation.
One behavior change: When the number of goroutines shrinks, don't log.
I've never found these logs to be useful, and they frequently add noise.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
This is mostly code movement from the wireguard-go repo.
Most of the new wgcfg package corresponds to the wireguard-go wgcfg package.
wgengine/wgcfg/device{_test}.go was device/config{_test}.go.
There were substantive but simple changes to device_test.go to remove
internal package device references.
The API of device.Config (now wgcfg.DeviceConfig) grew an error return;
we previously logged the error and threw it away.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
We log lines like this:
c.logf("[v1] magicsock: disco: %v->%v (%v, %v) sent %v", c.discoShort, dstDisco.ShortString(), dstKey.ShortString(), derpStr(dst.String()), disco.MessageSummary(m))
The leading [v1] causes it to get unintentionally rate limited.
Until we have a proper fix, work around it.
Fixes#1216
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Rewrite log lines on the fly, based on the set of known peers.
This enables us to use upstream wireguard-go logging,
but maintain the Tailscale-style peer public key identifiers
that the rest of our systems (and people) expect.
Fixes#1183
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
DstToString is used in two places in wireguard-go: Logging and uapi.
We are switching to use uapi for wireguard-go config.
To preserve existing behavior, we need the full set of addrs.
And for logging, having the full set of addrs seems useful.
(The Addrs method itself is slated for removal. When that happens,
the implementation will move to DstToString.)
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
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
Windows has a low resolution timer.
Some of the tests assumed that unblock takes effect immediately.
Consider:
t := time.Now()
elapsed := time.Now().After(t)
It seems plausible that elapsed should always be true.
However, with a low resolution timer, that might fail.
Change time.Now().After to !time.Now().Before,
so that unblocking always takes effect immediately.
Fixes#873.
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>
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>
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>
If any goroutine continues to use the logger in TestLocalLogLines
after the test finishes, the test panics.
The culprit for this was wireguard-go; the previous commit fixed that.
This commit adds suspenders: When the test is done, make logging calls
into no-ops.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
The log lines that wireguard-go prints as it starts
and stops its worker routines are mostly noise.
They also happen after other work is completed,
which causes failures in some of the log testing packages.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
This appears to have been the intent of the previous code,
but in practice, it only returned A records.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
This is a replacement for the key-related parts
of the wireguard-go wgcfg package.
This is almost a straight copy/paste from the wgcfg package.
I have slightly changed some of the exported functions and types
to avoid stutter, added and tweaked some comments,
and removed some now-unused code.
To avoid having wireguard-go depend on this new package,
wgcfg will keep its key types.
We translate into and out of those types at the last minute.
These few remaining uses will be eliminated alongside
the rest of the wgcfg package.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Upgrading staticcheck upgraded golang.org/x/sync
(one downside of mixing our tools in with our regular go.mod),
which introduced a new dependency via
https://go-review.googlesource.com/c/sync/+/251677
That CL could and probably should be written without runtime/debug,
but it's not clear to me that that is better at this moment
than simply accepting the additional package as a dependency.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
Add content length hints to headers.
The server can use these hints to more efficiently select buffers.
Stop attempting to compress tiny requests.
The bandwidth savings are negligible (and sometimes negative!),
and it makes extra work for the server.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
The cornerstone API is a more memory-efficient Unmarshal.
The savings come from re-using a json.Decoder.
BenchmarkUnmarshal-8 4016418 288 ns/op 8 B/op 1 allocs/op
BenchmarkStdUnmarshal-8 4189261 283 ns/op 184 B/op 2 allocs/op
It also includes a Bytes type to reduce allocations
when unmarshalling a non-hex-encoded JSON string into a []byte.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
likelyHomeRouterIPDarwinSyscall iterates through the list of routes,
looking for a private gateway, returning the first one it finds.
likelyHomeRouterIPDarwinExec does the same thing,
except that it returns the last one it finds.
As a result, when there are multiple gateways,
TestLikelyHomeRouterIPSyscallExec fails.
(At least, I think that that is what is happening;
I am going inferring from observed behavior.)
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
The compiler is failing to draw the connection between
slice cap and slice len, so is missing some obvious BCE opportunities.
Give it a hint by making the cap equal to the length.
The generated code is smaller and cleaner, and a bit faster.
name old time/op new time/op delta
Decode/tcp4-8 12.2ns ± 1% 11.6ns ± 3% -5.31% (p=0.000 n=28+29)
Decode/tcp6-8 12.5ns ± 2% 11.9ns ± 2% -4.84% (p=0.000 n=30+30)
Decode/udp4-8 11.5ns ± 1% 11.1ns ± 1% -3.11% (p=0.000 n=25+24)
Decode/udp6-8 11.8ns ± 3% 11.4ns ± 1% -3.08% (p=0.000 n=30+26)
Decode/icmp4-8 11.0ns ± 3% 10.6ns ± 1% -3.38% (p=0.000 n=25+30)
Decode/icmp6-8 11.4ns ± 1% 11.1ns ± 2% -2.29% (p=0.000 n=27+30)
Decode/igmp-8 10.3ns ± 0% 10.0ns ± 1% -3.26% (p=0.000 n=19+23)
Decode/unknown-8 8.68ns ± 1% 8.38ns ± 1% -3.55% (p=0.000 n=28+29)
Part of #19.
name old time/op new time/op delta
Filter/icmp4-8 32.2ns ± 3% 32.5ns ± 2% ~ (p=0.524 n=10+8)
Filter/icmp6-8 49.7ns ± 6% 43.1ns ± 4% -13.12% (p=0.000 n=9+10)
Signed-off-by: David Anderson <danderson@tailscale.com>
This lets servers using tsweb register expvars
that will track the number of requests ending
in 200s/300s/400s/500s.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
This makes it easy to compact slices that contain duplicate elements
by sorting and then uniqing.
This is an alternative to constructing an intermediate map
and then extracting elements from it. It also provides
more control over equality than using a map key does.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
When the network link changes, existing UDP sockets fail immediately
and permanently on macOS.
The forwarder set up a single UDP conn and never changed it.
As a result, any time there was a network link change,
all forwarded DNS queries failed.
To fix this, create a new connection when send requests
fail because of network unreachability.
This change is darwin-only, although extended it to other platforms
should be straightforward.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
While we're here, parseQuery into a plain function.
This is helpful for fuzzing. (Which I did a bit of. Didn't find anything.)
And clean up a few minor things.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
I had to use
go get -u github.com/tailscale/depaware@e09ee10c18249e4bf198e66bbd47babcd502637a
to force it to the correct version; it kept selecting head~1.
Maybe because the branch is called main instead of master?
Maybe because of some delay?
Updates #654. See that issue for a discussion of why
this timeout reduces flakiness, and what next steps are.
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
