The earlier aa5da2e5f2 made peer adds and removes through a netmap
delta path that mutates only nodeBackend, on the assumption that
PeerForIP, lookupPeerByIP, the engine's wireguard config
(e.lastCfgFull), the engine BART, wgdev's PeerLookupFunc closure, and
the engine's cached netmap (e.netMap) would all stay correct without
further updates. They don't. I'd totally forgotten that
Engine.PeerForIP has its own alternate IP-to-peer lookup codepath.
Concretely, all of these failed for a peer that arrived via
[tailcfg.MapResponse.PeersChanged] (and never via a full
[tailcfg.MapResponse.Peers] list):
- [wgengine.Engine.PeerForIP] read from e.netMap and e.lastCfgFull
(neither updated on the delta path) and so missed the new
peer. The rando non-data-plane callers (Ping, TSMP, pendopen,
debug endpoints, tsdial.Dialer.UseNetstackForIP for tsnet and
onlyNetstack tailscaled) all returned "no matching peer".
- The engine BART (built from e.lastCfgFull) missed the new peer's
subnet routes / exit-node default routes.
- wgdev's [device.PeerLookupFunc] closure (rebuilt only inside
wgcfg.ReconfigDevice) didn't have the new peer's noise key, so
outbound encryption to the new peer dropped the packet even when
SetPeerByIPPacketFunc returned the right NodePublic.
- And nothing in the delta path triggered NodeMutationRemove to
flow through to authReconfig either, so the same stale state
pointed at removed peers indefinitely.
So just (functionally) revert it for now, to have something easily
cherry-pickable to the 1.100 release branch. Proper fixes can come later
for the next release.
This also adds three new tests:
- TestPingPeerLearnedViaDelta runs disco and TSMP subtests over a
delta-added peer with only self addresses. disco exercises the
cold PeerForIP path (magicsock); TSMP exercises the full data path
through wgdev encryption. Both fail without this fix.
- TestPingSubnetRouteOfDeltaPeer exercises a subnet-router peer
arriving via delta. With s1 in --accept-routes mode, an IP
inside the advertised CIDR must resolve to s2 and a TSMP ping
must round-trip. Hits the BART + lastCfgFull + wgdev staleness
in one go.
- TestPingSelfReturnsIsLocalIP is a regression guard for the
IsSelf early-out in Engine.Ping. Passes on main today; included
here so future refactors of PeerForIP can't regress self
handling without test breakage.
Updates tailscale/corp#43394
Change-Id: I7a049271359bd73e7147ae9e2554e85614c2b8d2
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Brad FitzpatrickandBrad Fitzpatrick
ae743642d9