mirror of https://github.com/coder/coder.git
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.
This commit is contained in:
parent
f5dbc718a7
commit
5cbb76b47a
|
@ -904,7 +904,7 @@ func (api *API) _dialWorkspaceAgentTailnet(agentID uuid.UUID) (*codersdk.Workspa
|
||||||
}
|
}
|
||||||
|
|
||||||
derpMap := api.DERPMap()
|
derpMap := api.DERPMap()
|
||||||
if lastDERPMap == nil || tailnet.CompareDERPMaps(lastDERPMap, derpMap) {
|
if lastDERPMap == nil || !tailnet.CompareDERPMaps(lastDERPMap, derpMap) {
|
||||||
conn.SetDERPMap(derpMap)
|
conn.SetDERPMap(derpMap)
|
||||||
lastDERPMap = derpMap
|
lastDERPMap = derpMap
|
||||||
}
|
}
|
||||||
|
|
|
@ -65,7 +65,7 @@ type configMaps struct {
|
||||||
static netmap.NetworkMap
|
static netmap.NetworkMap
|
||||||
peers map[uuid.UUID]*peerLifecycle
|
peers map[uuid.UUID]*peerLifecycle
|
||||||
addresses []netip.Prefix
|
addresses []netip.Prefix
|
||||||
derpMap *proto.DERPMap
|
derpMap *tailcfg.DERPMap
|
||||||
logger slog.Logger
|
logger slog.Logger
|
||||||
blockEndpoints bool
|
blockEndpoints bool
|
||||||
|
|
||||||
|
@ -204,7 +204,7 @@ func (c *configMaps) netMapLocked() *netmap.NetworkMap {
|
||||||
nm.Addresses = make([]netip.Prefix, len(c.addresses))
|
nm.Addresses = make([]netip.Prefix, len(c.addresses))
|
||||||
copy(nm.Addresses, c.addresses)
|
copy(nm.Addresses, c.addresses)
|
||||||
|
|
||||||
nm.DERPMap = DERPMapFromProto(c.derpMap)
|
nm.DERPMap = c.derpMap.Clone()
|
||||||
nm.Peers = c.peerConfigLocked()
|
nm.Peers = c.peerConfigLocked()
|
||||||
nm.SelfNode.Addresses = nm.Addresses
|
nm.SelfNode.Addresses = nm.Addresses
|
||||||
nm.SelfNode.AllowedIPs = 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.
|
// setDERPMap sets the DERP map, triggering a configuration of the engine if it has changed.
|
||||||
// c.L MUST NOT be held.
|
// c.L MUST NOT be held.
|
||||||
func (c *configMaps) setDERPMap(derpMap *proto.DERPMap) {
|
func (c *configMaps) setDERPMap(derpMap *tailcfg.DERPMap) {
|
||||||
c.L.Lock()
|
c.L.Lock()
|
||||||
defer c.L.Unlock()
|
defer c.L.Unlock()
|
||||||
eq, err := c.derpMap.Equal(derpMap)
|
if CompareDERPMaps(c.derpMap, derpMap) {
|
||||||
if err != nil {
|
|
||||||
c.logger.Critical(context.Background(), "failed to compare DERP maps", slog.Error(err))
|
|
||||||
return
|
|
||||||
}
|
|
||||||
if eq {
|
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
c.derpMap = derpMap
|
c.derpMap = derpMap
|
||||||
|
@ -273,8 +268,7 @@ func (c *configMaps) setDERPMap(derpMap *proto.DERPMap) {
|
||||||
|
|
||||||
// derMapLocked returns the current DERPMap. c.L must be held
|
// derMapLocked returns the current DERPMap. c.L must be held
|
||||||
func (c *configMaps) derpMapLocked() *tailcfg.DERPMap {
|
func (c *configMaps) derpMapLocked() *tailcfg.DERPMap {
|
||||||
m := DERPMapFromProto(c.derpMap)
|
return c.derpMap.Clone()
|
||||||
return m
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// reconfig computes the correct wireguard config and calls the engine.Reconfig
|
// reconfig computes the correct wireguard config and calls the engine.Reconfig
|
||||||
|
|
|
@ -674,12 +674,12 @@ func TestConfigMaps_setDERPMap_different(t *testing.T) {
|
||||||
uut := newConfigMaps(logger, fEng, nodeID, nodePrivateKey, discoKey.Public())
|
uut := newConfigMaps(logger, fEng, nodeID, nodePrivateKey, discoKey.Public())
|
||||||
defer uut.close()
|
defer uut.close()
|
||||||
|
|
||||||
derpMap := &proto.DERPMap{
|
derpMap := &tailcfg.DERPMap{
|
||||||
HomeParams: &proto.DERPMap_HomeParams{RegionScore: map[int64]float64{1: 0.025}},
|
HomeParams: &tailcfg.DERPHomeParams{RegionScore: map[int]float64{1: 0.025}},
|
||||||
Regions: map[int64]*proto.DERPMap_Region{
|
Regions: map[int]*tailcfg.DERPRegion{
|
||||||
1: {
|
1: {
|
||||||
RegionCode: "AUH",
|
RegionCode: "AUH",
|
||||||
Nodes: []*proto.DERPMap_Region_Node{
|
Nodes: []*tailcfg.DERPNode{
|
||||||
{Name: "AUH0"},
|
{Name: "AUH0"},
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
|
@ -716,15 +716,24 @@ func TestConfigMaps_setDERPMap_same(t *testing.T) {
|
||||||
defer uut.close()
|
defer uut.close()
|
||||||
|
|
||||||
// Given: DERP Map already set
|
// Given: DERP Map already set
|
||||||
derpMap := &proto.DERPMap{
|
derpMap := &tailcfg.DERPMap{
|
||||||
HomeParams: &proto.DERPMap_HomeParams{RegionScore: map[int64]float64{1: 0.025}},
|
HomeParams: &tailcfg.DERPHomeParams{RegionScore: map[int]float64{
|
||||||
Regions: map[int64]*proto.DERPMap_Region{
|
1: 0.025,
|
||||||
|
1001: 0.111,
|
||||||
|
}},
|
||||||
|
Regions: map[int]*tailcfg.DERPRegion{
|
||||||
1: {
|
1: {
|
||||||
RegionCode: "AUH",
|
RegionCode: "AUH",
|
||||||
Nodes: []*proto.DERPMap_Region_Node{
|
Nodes: []*tailcfg.DERPNode{
|
||||||
{Name: "AUH0"},
|
{Name: "AUH0"},
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
|
1001: {
|
||||||
|
RegionCode: "DXB",
|
||||||
|
Nodes: []*tailcfg.DERPNode{
|
||||||
|
{Name: "DXB0"},
|
||||||
|
},
|
||||||
|
},
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
uut.L.Lock()
|
uut.L.Lock()
|
||||||
|
@ -734,8 +743,27 @@ func TestConfigMaps_setDERPMap_same(t *testing.T) {
|
||||||
// Then: we don't configure
|
// Then: we don't configure
|
||||||
requireNeverConfigures(ctx, t, &uut.phased)
|
requireNeverConfigures(ctx, t, &uut.phased)
|
||||||
|
|
||||||
// When we set the same DERP map
|
// When we set the equivalent DERP map, with different ordering
|
||||||
uut.setDERPMap(derpMap)
|
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{})
|
done := make(chan struct{})
|
||||||
go func() {
|
go func() {
|
||||||
|
|
|
@ -218,7 +218,7 @@ func NewConn(options *Options) (conn *Conn, err error) {
|
||||||
magicConn.DiscoPublicKey(),
|
magicConn.DiscoPublicKey(),
|
||||||
)
|
)
|
||||||
cfgMaps.setAddresses(options.Addresses)
|
cfgMaps.setAddresses(options.Addresses)
|
||||||
cfgMaps.setDERPMap(DERPMapToProto(options.DERPMap))
|
cfgMaps.setDERPMap(options.DERPMap)
|
||||||
cfgMaps.setBlockEndpoints(options.BlockEndpoints)
|
cfgMaps.setBlockEndpoints(options.BlockEndpoints)
|
||||||
|
|
||||||
nodeUp := newNodeUpdater(
|
nodeUp := newNodeUpdater(
|
||||||
|
@ -326,7 +326,7 @@ func (c *Conn) SetNodeCallback(callback func(node *Node)) {
|
||||||
|
|
||||||
// SetDERPMap updates the DERPMap of a connection.
|
// SetDERPMap updates the DERPMap of a connection.
|
||||||
func (c *Conn) SetDERPMap(derpMap *tailcfg.DERPMap) {
|
func (c *Conn) SetDERPMap(derpMap *tailcfg.DERPMap) {
|
||||||
c.configMaps.setDERPMap(DERPMapToProto(derpMap))
|
c.configMaps.setDERPMap(derpMap)
|
||||||
}
|
}
|
||||||
|
|
||||||
func (c *Conn) SetDERPForceWebSockets(v bool) {
|
func (c *Conn) SetDERPForceWebSockets(v bool) {
|
||||||
|
|
|
@ -18,15 +18,3 @@ func (s *Node) Equal(o *Node) (bool, error) {
|
||||||
}
|
}
|
||||||
return bytes.Equal(sBytes, oBytes), nil
|
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
|
|
||||||
}
|
|
||||||
|
|
Loading…
Reference in New Issue