Skip to content

Commit 3557c11

Browse files
coadlercoadler
committed
additional tests
1 parent 7e2d1bb commit 3557c11

File tree

2 files changed

+124
-10
lines changed

2 files changed

+124
-10
lines changed

‎tailnet/configmaps.go

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,9 @@ func (c *configMaps) netMapLocked() *netmap.NetworkMap {
217217
func (c *configMaps) peerConfigLocked() []*tailcfg.Node {
218218
out := make([]*tailcfg.Node, 0, len(c.peers))
219219
for _, p := range c.peers {
220-
if !p.readyForHandshake {
220+
// Don't add nodes that we havent received a READY_FOR_HANDSHAKE for
221+
// yet, if we want to wait for them.
222+
if !p.readyForHandshake && c.waitForHandshake {
221223
continue
222224
}
223225
n := p.node.Clone()
@@ -373,7 +375,7 @@ func (c *configMaps) updatePeerLocked(update *proto.CoordinateResponse_PeerUpdat
373375
return false
374376
}
375377
logger := c.logger.With(slog.F("peer_id", id))
376-
lc, ok := c.peers[id]
378+
lc, peerOk := c.peers[id]
377379
var node *tailcfg.Node
378380
if update.Kind == proto.CoordinateResponse_PeerUpdate_NODE {
379381
// If no preferred DERP is provided, we can't reach the node.
@@ -387,7 +389,7 @@ func (c *configMaps) updatePeerLocked(update *proto.CoordinateResponse_PeerUpdat
387389
return false
388390
}
389391
logger = logger.With(slog.F("key_id", node.Key.ShortString()), slog.F("node", node))
390-
peerStatus, ok := status.Peer[node.Key]
392+
peerStatus, statusOk := status.Peer[node.Key]
391393
// Starting KeepAlive messages at the initialization of a connection
392394
// causes a race condition. If we send the handshake before the peer has
393395
// our node, we'll have to wait for 5 seconds before trying again.
@@ -397,11 +399,10 @@ func (c *configMaps) updatePeerLocked(update *proto.CoordinateResponse_PeerUpdat
397399
// SSH connections don't send packets while idle, so we use keep alives
398400
// to avoid random hangs while we set up the connection again after
399401
// inactivity.
400-
// TODO: tie this into waitForHandshake (upstack PR)
401-
node.KeepAlive = ok && peerStatus.Active
402+
node.KeepAlive = (statusOk && peerStatus.Active) || (peerOk && lc.node.KeepAlive)
402403
}
403404
switch {
404-
case !ok && update.Kind == proto.CoordinateResponse_PeerUpdate_NODE:
405+
case !peerOk && update.Kind == proto.CoordinateResponse_PeerUpdate_NODE:
405406
// new!
406407
var lastHandshake time.Time
407408
if ps, ok := status.Peer[node.Key]; ok {
@@ -425,7 +426,7 @@ func (c *configMaps) updatePeerLocked(update *proto.CoordinateResponse_PeerUpdat
425426
// since we just got this node, we don't know if it's ready for
426427
// handshakes yet.
427428
return lc.readyForHandshake
428-
case ok && update.Kind == proto.CoordinateResponse_PeerUpdate_NODE:
429+
case peerOk && update.Kind == proto.CoordinateResponse_PeerUpdate_NODE:
429430
// update
430431
node.Created = lc.node.Created
431432
dirty = !lc.node.Equal(node) && lc.readyForHandshake
@@ -434,20 +435,22 @@ func (c *configMaps) updatePeerLocked(update *proto.CoordinateResponse_PeerUpdat
434435
lc.resetTimer()
435436
logger.Debug(context.Background(), "node update to existing peer", slog.F("dirty", dirty))
436437
return dirty
437-
case ok && update.Kind == proto.CoordinateResponse_PeerUpdate_READY_FOR_HANDSHAKE:
438+
case peerOk && update.Kind == proto.CoordinateResponse_PeerUpdate_READY_FOR_HANDSHAKE:
438439
wasReady := lc.readyForHandshake
439440
lc.readyForHandshake = true
440441
if lc.readyForHandshakeTimer != nil {
441442
lc.readyForHandshakeTimer.Stop()
443+
lc.readyForHandshakeTimer = nil
442444
}
445+
lc.node.KeepAlive = true
443446
logger.Debug(context.Background(), "peer ready for handshake")
444447
return !wasReady
445-
case !ok && update.Kind == proto.CoordinateResponse_PeerUpdate_READY_FOR_HANDSHAKE:
448+
case !peerOk && update.Kind == proto.CoordinateResponse_PeerUpdate_READY_FOR_HANDSHAKE:
446449
// TODO: should we keep track of ready for handshake messages we get
447450
// from unknown nodes?
448451
logger.Debug(context.Background(), "got peer ready for handshake for unknown peer")
449452
return false
450-
case !ok:
453+
case !peerOk:
451454
// disconnected or lost, but we don't have the node. No op
452455
logger.Debug(context.Background(), "skipping update for peer we don't recognize")
453456
return false
@@ -596,6 +599,7 @@ func (c *configMaps) peerReadyForHandshakeTimeout(peerID uuid.UUID) {
596599
lc.readyForHandshakeTimer = nil
597600
lc.readyForHandshake = true
598601
if !wasReady {
602+
c.netmapDirty = true
599603
c.Broadcast()
600604
}
601605
}

‎tailnet/configmaps_internal_test.go

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,52 @@ func TestConfigMaps_updatePeers_new(t *testing.T) {
185185
_ = testutil.RequireRecvCtx(ctx, t, done)
186186
}
187187

188+
func TestConfigMaps_updatePeers_new_waitForHandshake_neverConfigures(t *testing.T) {
189+
t.Parallel()
190+
ctx := testutil.Context(t, testutil.WaitShort)
191+
logger := slogtest.Make(t, nil).Leveled(slog.LevelDebug)
192+
fEng := newFakeEngineConfigurable()
193+
nodePrivateKey := key.NewNode()
194+
nodeID := tailcfg.NodeID(5)
195+
discoKey := key.NewDisco()
196+
uut := newConfigMaps(logger, fEng, nodeID, nodePrivateKey, discoKey.Public())
197+
defer uut.close()
198+
uut.setWaitForHandshake(true)
199+
start := time.Date(2024, time.January, 1, 8, 0, 0, 0, time.UTC)
200+
mClock := clock.NewMock()
201+
mClock.Set(start)
202+
uut.clock = mClock
203+
204+
p1ID := uuid.UUID{1}
205+
p1Node := newTestNode(1)
206+
p1n, err := NodeToProto(p1Node)
207+
require.NoError(t, err)
208+
209+
// it should not send the peer to the netmap
210+
requireNeverConfigures(ctx, t, &uut.phased)
211+
212+
go func() {
213+
<-fEng.status
214+
fEng.statusDone <- struct{}{}
215+
}()
216+
217+
u1 := []*proto.CoordinateResponse_PeerUpdate{
218+
{
219+
Id: p1ID[:],
220+
Kind: proto.CoordinateResponse_PeerUpdate_NODE,
221+
Node: p1n,
222+
},
223+
}
224+
uut.updatePeers(u1)
225+
226+
done := make(chan struct{})
227+
go func() {
228+
defer close(done)
229+
uut.close()
230+
}()
231+
_ = testutil.RequireRecvCtx(ctx, t, done)
232+
}
233+
188234
func TestConfigMaps_updatePeers_new_waitForHandshake(t *testing.T) {
189235
t.Parallel()
190236
ctx := testutil.Context(t, testutil.WaitShort)
@@ -196,6 +242,10 @@ func TestConfigMaps_updatePeers_new_waitForHandshake(t *testing.T) {
196242
uut := newConfigMaps(logger, fEng, nodeID, nodePrivateKey, discoKey.Public())
197243
defer uut.close()
198244
uut.setWaitForHandshake(true)
245+
start := time.Date(2024, time.January, 1, 8, 0, 0, 0, time.UTC)
246+
mClock := clock.NewMock()
247+
mClock.Set(start)
248+
uut.clock = mClock
199249

200250
p1ID := uuid.UUID{1}
201251
p1Node := newTestNode(1)
@@ -254,6 +304,66 @@ func TestConfigMaps_updatePeers_new_waitForHandshake(t *testing.T) {
254304
_ = testutil.RequireRecvCtx(ctx, t, done)
255305
}
256306

307+
func TestConfigMaps_updatePeers_new_waitForHandshake_timeout(t *testing.T) {
308+
t.Parallel()
309+
ctx := testutil.Context(t, testutil.WaitShort)
310+
logger := slogtest.Make(t, nil).Leveled(slog.LevelDebug)
311+
fEng := newFakeEngineConfigurable()
312+
nodePrivateKey := key.NewNode()
313+
nodeID := tailcfg.NodeID(5)
314+
discoKey := key.NewDisco()
315+
uut := newConfigMaps(logger, fEng, nodeID, nodePrivateKey, discoKey.Public())
316+
defer uut.close()
317+
uut.setWaitForHandshake(true)
318+
start := time.Date(2024, time.March, 29, 8, 0, 0, 0, time.UTC)
319+
mClock := clock.NewMock()
320+
mClock.Set(start)
321+
uut.clock = mClock
322+
323+
p1ID := uuid.UUID{1}
324+
p1Node := newTestNode(1)
325+
p1n, err := NodeToProto(p1Node)
326+
require.NoError(t, err)
327+
328+
go func() {
329+
<-fEng.status
330+
fEng.statusDone <- struct{}{}
331+
}()
332+
333+
u1 := []*proto.CoordinateResponse_PeerUpdate{
334+
{
335+
Id: p1ID[:],
336+
Kind: proto.CoordinateResponse_PeerUpdate_NODE,
337+
Node: p1n,
338+
},
339+
}
340+
uut.updatePeers(u1)
341+
342+
mClock.Add(5 * time.Second)
343+
344+
// it should now send the peer to the netmap
345+
346+
nm := testutil.RequireRecvCtx(ctx, t, fEng.setNetworkMap)
347+
r := testutil.RequireRecvCtx(ctx, t, fEng.reconfig)
348+
349+
require.Len(t, nm.Peers, 1)
350+
n1 := getNodeWithID(t, nm.Peers, 1)
351+
require.Equal(t, "127.3.3.40:1", n1.DERP)
352+
require.Equal(t, p1Node.Endpoints, n1.Endpoints)
353+
require.False(t, n1.KeepAlive)
354+
355+
// we rely on nmcfg.WGCfg() to convert the netmap to wireguard config, so just
356+
// require the right number of peers.
357+
require.Len(t, r.wg.Peers, 1)
358+
359+
done := make(chan struct{})
360+
go func() {
361+
defer close(done)
362+
uut.close()
363+
}()
364+
_ = testutil.RequireRecvCtx(ctx, t, done)
365+
}
366+
257367
func TestConfigMaps_updatePeers_same(t *testing.T) {
258368
t.Parallel()
259369
ctx := testutil.Context(t, testutil.WaitShort)

0 commit comments

Comments
 (0)