diff --git a/wgengine/magicsock/endpoint.go b/wgengine/magicsock/endpoint.go index d8d1e6ee3..385c9245e 100644 --- a/wgengine/magicsock/endpoint.go +++ b/wgengine/magicsock/endpoint.go @@ -106,24 +106,27 @@ type endpoint struct { func (de *endpoint) udpRelayEndpointReady(maybeBest addrQuality) { de.mu.Lock() defer de.mu.Unlock() + now := mono.Now() + curBestAddrTrusted := now.Before(de.trustBestAddrUntil) + sameRelayServer := de.bestAddr.vni.isSet() && maybeBest.relayServerDisco.Compare(de.bestAddr.relayServerDisco) == 0 - if maybeBest.relayServerDisco.Compare(de.bestAddr.relayServerDisco) == 0 { - // TODO(jwhited): add some observability for this case, e.g. did we - // flip transports during a de.bestAddr transition from untrusted to - // trusted? + if !curBestAddrTrusted || + sameRelayServer || + betterAddr(maybeBest, de.bestAddr) { + // We must set maybeBest as de.bestAddr if: + // 1. de.bestAddr is untrusted. betterAddr does not consider + // time-based trust. + // 2. maybeBest & de.bestAddr are on the same relay. If the maybeBest + // handshake happened to use a different source address/transport, + // the relay will drop packets from the 'old' de.bestAddr's. + // 3. maybeBest is a 'betterAddr'. // - // If these are equal we must set maybeBest as bestAddr, otherwise we - // could leave a stale bestAddr if it goes over a different - // address family or src. - } else if !betterAddr(maybeBest, de.bestAddr) { - return + // TODO(jwhited): add observability around !curBestAddrTrusted and sameRelayServer + // TODO(jwhited): collapse path change logging with endpoint.handlePongConnLocked() + de.c.logf("magicsock: disco: node %v %v now using %v mtu=%v", de.publicKey.ShortString(), de.discoShort(), maybeBest.epAddr, maybeBest.wireMTU) + de.setBestAddrLocked(maybeBest) + de.trustBestAddrUntil = now.Add(trustUDPAddrDuration) } - - // Promote maybeBest to bestAddr. - // TODO(jwhited): collapse path change logging with endpoint.handlePongConnLocked() - de.c.logf("magicsock: disco: node %v %v now using %v mtu=%v", de.publicKey.ShortString(), de.discoShort(), maybeBest.epAddr, maybeBest.wireMTU) - de.setBestAddrLocked(maybeBest) - de.trustBestAddrUntil = mono.Now().Add(trustUDPAddrDuration) } func (de *endpoint) setBestAddrLocked(v addrQuality) { diff --git a/wgengine/magicsock/endpoint_test.go b/wgengine/magicsock/endpoint_test.go index 3a1e55b8b..92f4ef1d3 100644 --- a/wgengine/magicsock/endpoint_test.go +++ b/wgengine/magicsock/endpoint_test.go @@ -9,6 +9,7 @@ import ( "time" "tailscale.com/tailcfg" + "tailscale.com/tstime/mono" "tailscale.com/types/key" ) @@ -365,3 +366,91 @@ func Test_epAddr_isDirectUDP(t *testing.T) { }) } } + +func Test_endpoint_udpRelayEndpointReady(t *testing.T) { + directAddrQuality := addrQuality{epAddr: epAddr{ap: netip.MustParseAddrPort("192.0.2.1:7")}} + peerRelayAddrQuality := addrQuality{epAddr: epAddr{ap: netip.MustParseAddrPort("192.0.2.2:77")}, latency: time.Second} + peerRelayAddrQuality.vni.set(1) + peerRelayAddrQualityHigherLatencySameServer := addrQuality{ + epAddr: epAddr{ap: netip.MustParseAddrPort("192.0.2.3:77"), vni: peerRelayAddrQuality.vni}, + latency: peerRelayAddrQuality.latency * 10, + } + peerRelayAddrQualityHigherLatencyDiffServer := addrQuality{ + epAddr: epAddr{ap: netip.MustParseAddrPort("192.0.2.3:77"), vni: peerRelayAddrQuality.vni}, + latency: peerRelayAddrQuality.latency * 10, + relayServerDisco: key.NewDisco().Public(), + } + peerRelayAddrQualityLowerLatencyDiffServer := addrQuality{ + epAddr: epAddr{ap: netip.MustParseAddrPort("192.0.2.4:77"), vni: peerRelayAddrQuality.vni}, + latency: peerRelayAddrQuality.latency / 10, + relayServerDisco: key.NewDisco().Public(), + } + peerRelayAddrQualityEqualLatencyDiffServer := addrQuality{ + epAddr: epAddr{ap: netip.MustParseAddrPort("192.0.2.4:77"), vni: peerRelayAddrQuality.vni}, + latency: peerRelayAddrQuality.latency, + relayServerDisco: key.NewDisco().Public(), + } + tests := []struct { + name string + curBestAddr addrQuality + trustBestAddrUntil mono.Time + maybeBest addrQuality + wantBestAddr addrQuality + }{ + { + name: "bestAddr trusted direct", + curBestAddr: directAddrQuality, + trustBestAddrUntil: mono.Now().Add(1 * time.Hour), + maybeBest: peerRelayAddrQuality, + wantBestAddr: directAddrQuality, + }, + { + name: "bestAddr untrusted direct", + curBestAddr: directAddrQuality, + trustBestAddrUntil: mono.Now().Add(-1 * time.Hour), + maybeBest: peerRelayAddrQuality, + wantBestAddr: peerRelayAddrQuality, + }, + { + name: "maybeBest same relay server higher latency bestAddr trusted", + curBestAddr: peerRelayAddrQuality, + trustBestAddrUntil: mono.Now().Add(1 * time.Hour), + maybeBest: peerRelayAddrQualityHigherLatencySameServer, + wantBestAddr: peerRelayAddrQualityHigherLatencySameServer, + }, + { + name: "maybeBest diff relay server higher latency bestAddr trusted", + curBestAddr: peerRelayAddrQuality, + trustBestAddrUntil: mono.Now().Add(1 * time.Hour), + maybeBest: peerRelayAddrQualityHigherLatencyDiffServer, + wantBestAddr: peerRelayAddrQuality, + }, + { + name: "maybeBest diff relay server lower latency bestAddr trusted", + curBestAddr: peerRelayAddrQuality, + trustBestAddrUntil: mono.Now().Add(1 * time.Hour), + maybeBest: peerRelayAddrQualityLowerLatencyDiffServer, + wantBestAddr: peerRelayAddrQualityLowerLatencyDiffServer, + }, + { + name: "maybeBest diff relay server equal latency bestAddr trusted", + curBestAddr: peerRelayAddrQuality, + trustBestAddrUntil: mono.Now().Add(1 * time.Hour), + maybeBest: peerRelayAddrQualityEqualLatencyDiffServer, + wantBestAddr: peerRelayAddrQuality, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + de := &endpoint{ + c: &Conn{logf: func(msg string, args ...any) { return }}, + bestAddr: tt.curBestAddr, + trustBestAddrUntil: tt.trustBestAddrUntil, + } + de.udpRelayEndpointReady(tt.maybeBest) + if de.bestAddr != tt.wantBestAddr { + t.Errorf("de.bestAddr = %v, want %v", de.bestAddr, tt.wantBestAddr) + } + }) + } +}