From 5cbb76b47afbe7c52689369622c69051146739f7 Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Wed, 24 Jan 2024 16:27:15 +0400 Subject: [PATCH] fix: stop spamming DERP map updates for equivalent maps (#11792) Fixes 2 related issues: 1. wsconncache had incorrect logic to test whether to send DERPMap updates, sending if the maps were equivalent, instead of if they were _not equivalent_. 2. configmaps used a bugged check to test equality between DERPMaps, since it contains a map and the map entries are serialized in random order. Instead, we avoid comparing the protobufs and instead depend on the existing function that compares `tailcfg.DERPMap`. This also has the effect of reducing the number of times we convert to and from protobuf. --- coderd/workspaceagents.go | 2 +- tailnet/configmaps.go | 16 +++------- tailnet/configmaps_internal_test.go | 48 +++++++++++++++++++++++------ tailnet/conn.go | 4 +-- tailnet/proto/compare.go | 12 -------- 5 files changed, 46 insertions(+), 36 deletions(-) diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index 568fb17f20..d438d6663d 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -904,7 +904,7 @@ func (api *API) _dialWorkspaceAgentTailnet(agentID uuid.UUID) (*codersdk.Workspa } derpMap := api.DERPMap() - if lastDERPMap == nil || tailnet.CompareDERPMaps(lastDERPMap, derpMap) { + if lastDERPMap == nil || !tailnet.CompareDERPMaps(lastDERPMap, derpMap) { conn.SetDERPMap(derpMap) lastDERPMap = derpMap } diff --git a/tailnet/configmaps.go b/tailnet/configmaps.go index 4dd307536e..c922073fa6 100644 --- a/tailnet/configmaps.go +++ b/tailnet/configmaps.go @@ -65,7 +65,7 @@ type configMaps struct { static netmap.NetworkMap peers map[uuid.UUID]*peerLifecycle addresses []netip.Prefix - derpMap *proto.DERPMap + derpMap *tailcfg.DERPMap logger slog.Logger blockEndpoints bool @@ -204,7 +204,7 @@ func (c *configMaps) netMapLocked() *netmap.NetworkMap { nm.Addresses = make([]netip.Prefix, len(c.addresses)) copy(nm.Addresses, c.addresses) - nm.DERPMap = DERPMapFromProto(c.derpMap) + nm.DERPMap = c.derpMap.Clone() nm.Peers = c.peerConfigLocked() nm.SelfNode.Addresses = nm.Addresses nm.SelfNode.AllowedIPs = nm.Addresses @@ -255,15 +255,10 @@ func (c *configMaps) setBlockEndpoints(blockEndpoints bool) { // setDERPMap sets the DERP map, triggering a configuration of the engine if it has changed. // c.L MUST NOT be held. -func (c *configMaps) setDERPMap(derpMap *proto.DERPMap) { +func (c *configMaps) setDERPMap(derpMap *tailcfg.DERPMap) { c.L.Lock() defer c.L.Unlock() - eq, err := c.derpMap.Equal(derpMap) - if err != nil { - c.logger.Critical(context.Background(), "failed to compare DERP maps", slog.Error(err)) - return - } - if eq { + if CompareDERPMaps(c.derpMap, derpMap) { return } c.derpMap = derpMap @@ -273,8 +268,7 @@ func (c *configMaps) setDERPMap(derpMap *proto.DERPMap) { // derMapLocked returns the current DERPMap. c.L must be held func (c *configMaps) derpMapLocked() *tailcfg.DERPMap { - m := DERPMapFromProto(c.derpMap) - return m + return c.derpMap.Clone() } // reconfig computes the correct wireguard config and calls the engine.Reconfig diff --git a/tailnet/configmaps_internal_test.go b/tailnet/configmaps_internal_test.go index a6921f9397..f842a710a6 100644 --- a/tailnet/configmaps_internal_test.go +++ b/tailnet/configmaps_internal_test.go @@ -674,12 +674,12 @@ func TestConfigMaps_setDERPMap_different(t *testing.T) { uut := newConfigMaps(logger, fEng, nodeID, nodePrivateKey, discoKey.Public()) defer uut.close() - derpMap := &proto.DERPMap{ - HomeParams: &proto.DERPMap_HomeParams{RegionScore: map[int64]float64{1: 0.025}}, - Regions: map[int64]*proto.DERPMap_Region{ + derpMap := &tailcfg.DERPMap{ + HomeParams: &tailcfg.DERPHomeParams{RegionScore: map[int]float64{1: 0.025}}, + Regions: map[int]*tailcfg.DERPRegion{ 1: { RegionCode: "AUH", - Nodes: []*proto.DERPMap_Region_Node{ + Nodes: []*tailcfg.DERPNode{ {Name: "AUH0"}, }, }, @@ -716,15 +716,24 @@ func TestConfigMaps_setDERPMap_same(t *testing.T) { defer uut.close() // Given: DERP Map already set - derpMap := &proto.DERPMap{ - HomeParams: &proto.DERPMap_HomeParams{RegionScore: map[int64]float64{1: 0.025}}, - Regions: map[int64]*proto.DERPMap_Region{ + derpMap := &tailcfg.DERPMap{ + HomeParams: &tailcfg.DERPHomeParams{RegionScore: map[int]float64{ + 1: 0.025, + 1001: 0.111, + }}, + Regions: map[int]*tailcfg.DERPRegion{ 1: { RegionCode: "AUH", - Nodes: []*proto.DERPMap_Region_Node{ + Nodes: []*tailcfg.DERPNode{ {Name: "AUH0"}, }, }, + 1001: { + RegionCode: "DXB", + Nodes: []*tailcfg.DERPNode{ + {Name: "DXB0"}, + }, + }, }, } uut.L.Lock() @@ -734,8 +743,27 @@ func TestConfigMaps_setDERPMap_same(t *testing.T) { // Then: we don't configure requireNeverConfigures(ctx, t, &uut.phased) - // When we set the same DERP map - uut.setDERPMap(derpMap) + // When we set the equivalent DERP map, with different ordering + uut.setDERPMap(&tailcfg.DERPMap{ + HomeParams: &tailcfg.DERPHomeParams{RegionScore: map[int]float64{ + 1001: 0.111, + 1: 0.025, + }}, + Regions: map[int]*tailcfg.DERPRegion{ + 1001: { + RegionCode: "DXB", + Nodes: []*tailcfg.DERPNode{ + {Name: "DXB0"}, + }, + }, + 1: { + RegionCode: "AUH", + Nodes: []*tailcfg.DERPNode{ + {Name: "AUH0"}, + }, + }, + }, + }) done := make(chan struct{}) go func() { diff --git a/tailnet/conn.go b/tailnet/conn.go index 0b830a7913..b574df3e10 100644 --- a/tailnet/conn.go +++ b/tailnet/conn.go @@ -218,7 +218,7 @@ func NewConn(options *Options) (conn *Conn, err error) { magicConn.DiscoPublicKey(), ) cfgMaps.setAddresses(options.Addresses) - cfgMaps.setDERPMap(DERPMapToProto(options.DERPMap)) + cfgMaps.setDERPMap(options.DERPMap) cfgMaps.setBlockEndpoints(options.BlockEndpoints) nodeUp := newNodeUpdater( @@ -326,7 +326,7 @@ func (c *Conn) SetNodeCallback(callback func(node *Node)) { // SetDERPMap updates the DERPMap of a connection. func (c *Conn) SetDERPMap(derpMap *tailcfg.DERPMap) { - c.configMaps.setDERPMap(DERPMapToProto(derpMap)) + c.configMaps.setDERPMap(derpMap) } func (c *Conn) SetDERPForceWebSockets(v bool) { diff --git a/tailnet/proto/compare.go b/tailnet/proto/compare.go index 7a2b158aa1..012ac293a0 100644 --- a/tailnet/proto/compare.go +++ b/tailnet/proto/compare.go @@ -18,15 +18,3 @@ func (s *Node) Equal(o *Node) (bool, error) { } return bytes.Equal(sBytes, oBytes), nil } - -func (s *DERPMap) Equal(o *DERPMap) (bool, error) { - sBytes, err := gProto.Marshal(s) - if err != nil { - return false, err - } - oBytes, err := gProto.Marshal(o) - if err != nil { - return false, err - } - return bytes.Equal(sBytes, oBytes), nil -}