diff --git a/wgengine/magicsock/endpoint.go b/wgengine/magicsock/endpoint.go index 5e9a28583..10c58494a 100644 --- a/wgengine/magicsock/endpoint.go +++ b/wgengine/magicsock/endpoint.go @@ -71,8 +71,6 @@ type endpoint struct { endpointState map[netip.AddrPort]*endpointState isCallMeMaybeEP map[netip.AddrPort]bool - pendingCLIPings []pendingCLIPing // any outstanding "tailscale ping" commands running - // The following fields are related to the new "silent disco" // implementation that's a WIP as of 2022-10-20. // See #540 for background. @@ -82,11 +80,6 @@ type endpoint struct { isWireguardOnly bool // whether the endpoint is WireGuard only } -type pendingCLIPing struct { - res *ipnstate.PingResult - cb func(*ipnstate.PingResult) -} - // endpointDisco is the current disco key and short string for an endpoint. This // structure is immutable. type endpointDisco struct { @@ -99,6 +92,8 @@ type sentPing struct { at mono.Time timer *time.Timer // timeout timer purpose discoPingPurpose + res *ipnstate.PingResult // nil unless CLI ping + cb func(*ipnstate.PingResult) // nil unless CLI ping } // endpointState is some state and history for a specific endpoint of @@ -363,7 +358,7 @@ func (de *endpoint) heartbeat() { udpAddr, _, _ := de.addrForSendLocked(now) if udpAddr.IsValid() { // We have a preferred path. Ping that every 2 seconds. - de.startDiscoPingLocked(udpAddr, now, pingHeartbeat, 0) + de.startDiscoPingLocked(udpAddr, now, pingHeartbeat, 0, nil, nil) } if de.wantFullPingLocked(now) { @@ -415,22 +410,21 @@ func (de *endpoint) cliPing(res *ipnstate.PingResult, size int, cb func(*ipnstat return } - de.pendingCLIPings = append(de.pendingCLIPings, pendingCLIPing{res, cb}) - now := mono.Now() udpAddr, derpAddr, _ := de.addrForSendLocked(now) + if derpAddr.IsValid() { - de.startDiscoPingLocked(derpAddr, now, pingCLI, size) + de.startDiscoPingLocked(derpAddr, now, pingCLI, size, res, cb) } if udpAddr.IsValid() && now.Before(de.trustBestAddrUntil) { // Already have an active session, so just ping the address we're using. // Otherwise "tailscale ping" results to a node on the local network // can look like they're bouncing between, say 10.0.0.0/9 and the peer's // IPv6 address, both 1ms away, and it's random who replies first. - de.startDiscoPingLocked(udpAddr, now, pingCLI, size) + de.startDiscoPingLocked(udpAddr, now, pingCLI, size, res, cb) } else { for ep := range de.endpointState { - de.startDiscoPingLocked(ep, now, pingCLI, size) + de.startDiscoPingLocked(ep, now, pingCLI, size, res, cb) } } de.noteActiveLocked() @@ -573,8 +567,10 @@ const ( pingCLI ) -// startDiscoPingLocked sends a disco ping to ep in a separate goroutine. -func (de *endpoint) startDiscoPingLocked(ep netip.AddrPort, now mono.Time, purpose discoPingPurpose, size int) { +// startDiscoPingLocked sends a disco ping to ep in a separate +// goroutine. res and cb are for returning the results of CLI pings, +// otherwise they are nil. +func (de *endpoint) startDiscoPingLocked(ep netip.AddrPort, now mono.Time, purpose discoPingPurpose, size int, res *ipnstate.PingResult, cb func(*ipnstate.PingResult)) { if runtime.GOOS == "js" { return } @@ -599,7 +595,10 @@ func (de *endpoint) startDiscoPingLocked(ep netip.AddrPort, now mono.Time, purpo at: now, timer: time.AfterFunc(pingTimeoutDuration, func() { de.discoPingTimeout(txid) }), purpose: purpose, + res: res, + cb: cb, } + logLevel := discoLog if purpose == pingHeartbeat { logLevel = discoVerboseLog @@ -630,7 +629,7 @@ func (de *endpoint) sendDiscoPingsLocked(now mono.Time, sendCallMeMaybe bool) { de.c.dlogf("[v1] magicsock: disco: send, starting discovery for %v (%v)", de.publicKey.ShortString(), de.discoShort()) } - de.startDiscoPingLocked(ep, now, pingDiscovery, 0) + de.startDiscoPingLocked(ep, now, pingDiscovery, 0, nil, nil) } derpAddr := de.derpAddr if sentAny && sendCallMeMaybe && derpAddr.IsValid() { @@ -915,11 +914,13 @@ func (de *endpoint) handlePongConnLocked(m *disco.Pong, di *discoInfo, src netip })) } - for _, pp := range de.pendingCLIPings { - de.c.populateCLIPingResponseLocked(pp.res, latency, sp.to) - go pp.cb(pp.res) + // Currently only CLI ping uses this callback. + if sp.cb != nil { + if sp.purpose == pingCLI { + de.c.populateCLIPingResponseLocked(sp.res, latency, sp.to) + } + go sp.cb(sp.res) } - de.pendingCLIPings = nil // Promote this pong response to our current best address if it's lower latency. // TODO(bradfitz): decide how latency vs. preference order affects decision @@ -1134,7 +1135,6 @@ func (de *endpoint) stopAndReset() { de.heartBeatTimer.Stop() de.heartBeatTimer = nil } - de.pendingCLIPings = nil } // resetLocked clears all the endpoint's p2p state, reverting it to a diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 2e550ed23..f8d93ec8f 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -1270,7 +1270,7 @@ func (c *Conn) sendDiscoMessage(dst netip.AddrPort, dstKey key.NodePublic, dstDi // Can't send. (e.g. no IPv6 locally) } else { if !c.networkDown() { - c.logf("magicsock: disco: failed to send %T to %v: %v", m, dst, err) + c.logf("magicsock: disco: failed to send %v to %v: %v", disco.MessageSummary(m), dst, err) } } return sent, err