Skip to content

Commit c2fbe21

Browse files
nbrownusnbrownus
authored
Fix a test race, make dns server reload/restart safer (#1815)
1 parent 94ac6db commit c2fbe21

2 files changed

Lines changed: 222 additions & 11 deletions

File tree

‎dns_server.go‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -97,8 +97,7 @@ func (d *dnsServer) reload(c *config.C, initial bool) error {
9797
newAddr := getDnsServerAddr(c)
9898

9999
d.serverMu.Lock()
100-
running := d.server
101-
runningStarted := d.started
100+
running := d.server != nil
102101
sameAddr := d.addr == newAddr
103102
d.addr = newAddr
104103
d.enabled.Store(enabled)
@@ -112,7 +111,7 @@ func (d *dnsServer) reload(c *config.C, initial bool) error {
112111
}
113112

114113
if !enabled {
115-
if running != nil {
114+
if running {
116115
d.Stop()
117116
}
118117
// Drop any records that accumulated while enabled; a later re-enable
@@ -121,12 +120,12 @@ func (d *dnsServer) reload(c *config.C, initial bool) error {
121120
return nil
122121
}
123122

124-
if running == nil {
123+
if !running {
125124
// Was disabled (or never started); bring it up now.
126125
go d.Start()
127126
} else if !sameAddr {
128-
d.shutdownServer(running, runningStarted, "reload")
129-
// Old Start goroutine has now exited; bring up a fresh listener on the new address.
127+
// Stop clears the slot before shutting down, otherwise the Start below can find the dying server and refuse
128+
d.Stop()
130129
go d.Start()
131130
}
132131

@@ -162,7 +161,9 @@ func (d *dnsServer) Start() {
162161

163162
started := make(chan struct{})
164163
d.serverMu.Lock()
165-
if d.ctx.Err() != nil {
164+
// Re-check enabled under the lock, a disable that raced our check above snapshots the slot under it too.
165+
// Two reloads in quick succession can both spawn a Start, the loser would orphan the live listener past Stop
166+
if d.ctx.Err() != nil || d.server != nil || !d.enabled.Load() {
166167
d.serverMu.Unlock()
167168
return
168169
}
@@ -200,6 +201,14 @@ func (d *dnsServer) Start() {
200201
close(started)
201202
}
202203

204+
// Release our slot, unless a reload already replaced us, so a dead listener can't block a future Start
205+
d.serverMu.Lock()
206+
if d.server == server {
207+
d.server = nil
208+
d.started = nil
209+
}
210+
d.serverMu.Unlock()
211+
203212
if err != nil {
204213
d.l.Warn("Failed to run the DNS responder", "error", err)
205214
}

‎dns_server_test.go‎

Lines changed: 206 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -194,14 +194,51 @@ func TestDnsServer_reload_initial_serveDnsWithoutLighthouse(t *testing.T) {
194194
}
195195

196196
func TestDnsServer_reload_sameAddr_noOp(t *testing.T) {
197+
port := freeUDPPort(t)
197198
ds, c := newTestDnsServer(t)
198-
setDnsConfig(c, "127.0.0.1", "0", true, true)
199-
199+
setDnsConfig(c, "127.0.0.1", port, true, true)
200200
require.NoError(t, ds.reload(c, true))
201-
// No server running yet, no addr change. Reload should not spawn anything.
201+
202+
go ds.Start()
203+
waitForBind(t, ds)
204+
205+
ds.serverMu.Lock()
206+
before := ds.server
207+
ds.serverMu.Unlock()
208+
require.NotNil(t, before)
209+
210+
// Same address, so the running listener must be left alone rather than rebuilt under live queries
202211
require.NoError(t, ds.reload(c, false))
203212
assert.True(t, ds.enabled.Load())
204-
assert.Nil(t, ds.server)
213+
214+
ds.serverMu.Lock()
215+
after := ds.server
216+
ds.serverMu.Unlock()
217+
assert.Same(t, before, after, "a same-address reload must not restart the listener")
218+
219+
ds.Stop()
220+
}
221+
222+
// The branch the old sameAddr test was accidentally hitting: enabled with nothing running means reload starts it.
223+
func TestDnsServer_reload_whenNotRunning_starts(t *testing.T) {
224+
port := freeUDPPort(t)
225+
ds, c := newTestDnsServer(t)
226+
setDnsConfig(c, "127.0.0.1", port, true, true)
227+
228+
// initial only records config, it never starts anything
229+
require.NoError(t, ds.reload(c, true))
230+
ds.serverMu.Lock()
231+
assert.Nil(t, ds.server, "the initial reload must not start a listener")
232+
ds.serverMu.Unlock()
233+
234+
require.NoError(t, ds.reload(c, false))
235+
waitForBind(t, ds)
236+
237+
ds.serverMu.Lock()
238+
assert.NotNil(t, ds.server, "a reload with nothing running should bring DNS up")
239+
ds.serverMu.Unlock()
240+
241+
ds.Stop()
205242
}
206243

207244
func TestDnsServer_StartStop_lifecycle(t *testing.T) {
@@ -427,3 +464,168 @@ func waitFor(t *testing.T, cond func() bool) {
427464
}
428465
t.Fatal("timed out waiting for condition")
429466
}
467+
468+
// Two reloads in quick succession, or a HUP before Control.Start, can race two Starts at the same listener.
469+
func TestDnsServer_Start_isIdempotent(t *testing.T) {
470+
port := freeUDPPort(t)
471+
ds, c := newTestDnsServer(t)
472+
setDnsConfig(c, "127.0.0.1", port, true, true)
473+
require.NoError(t, ds.reload(c, true))
474+
475+
go ds.Start()
476+
waitForBind(t, ds)
477+
478+
ds.serverMu.Lock()
479+
first := ds.server
480+
ds.serverMu.Unlock()
481+
require.NotNil(t, first)
482+
483+
// If the second Start replaces the tracked server, Stop kills the wrong one and the port leaks
484+
done := make(chan struct{})
485+
go func() {
486+
ds.Start()
487+
close(done)
488+
}()
489+
select {
490+
case <-done:
491+
case <-time.After(time.Second * 5):
492+
t.Fatal("second Start never returned")
493+
}
494+
495+
ds.serverMu.Lock()
496+
second := ds.server
497+
ds.serverMu.Unlock()
498+
assert.Same(t, first, second, "a second Start must not replace the running server")
499+
500+
// The real proof, after Stop the port must actually be free
501+
ds.Stop()
502+
waitFor(t, func() bool {
503+
pc, err := net.ListenPacket("udp", "127.0.0.1:"+port)
504+
if err != nil {
505+
return false
506+
}
507+
_ = pc.Close()
508+
return true
509+
})
510+
}
511+
512+
// An address change must actually end up listening on the new port. Start's guard refuses when a server is already
513+
// installed, so reload has to clear the slot before shutting the old one down.
514+
func TestDnsServer_reload_addrChange_restarts(t *testing.T) {
515+
first := freeUDPPort(t)
516+
second := freeUDPPort(t)
517+
518+
ds, c := newTestDnsServer(t)
519+
setDnsConfig(c, "127.0.0.1", first, true, true)
520+
require.NoError(t, ds.reload(c, true))
521+
522+
go ds.Start()
523+
waitForBind(t, ds)
524+
525+
// Cycle a few times, the failure this guards against depends on which goroutine wins serverMu
526+
for i := range 8 {
527+
want := second
528+
if i%2 == 1 {
529+
want = first
530+
}
531+
setDnsConfig(c, "127.0.0.1", want, true, true)
532+
require.NoError(t, ds.reload(c, false))
533+
waitForBind(t, ds)
534+
535+
ds.serverMu.Lock()
536+
srv := ds.server
537+
ds.serverMu.Unlock()
538+
require.NotNil(t, srv, "reload left DNS down instead of restarting it")
539+
require.Equal(t, "127.0.0.1:"+want, srv.Addr, "reload should be serving the new address")
540+
}
541+
542+
// Land back on second so the port assertions below are meaningful
543+
setDnsConfig(c, "127.0.0.1", second, true, true)
544+
require.NoError(t, ds.reload(c, false))
545+
waitForBind(t, ds)
546+
547+
// The old port must be released and the new one actually held
548+
waitFor(t, func() bool {
549+
pc, err := net.ListenPacket("udp", "127.0.0.1:"+first)
550+
if err != nil {
551+
return false
552+
}
553+
_ = pc.Close()
554+
return true
555+
})
556+
_, err := net.ListenPacket("udp", "127.0.0.1:"+second)
557+
require.Error(t, err, "the new address should be bound by the DNS responder")
558+
559+
ds.Stop()
560+
}
561+
562+
// A listener that dies on its own must release the slot, or a later same-addr reload sees it as running and no-ops.
563+
func TestDnsServer_Start_bindFailure_releasesSlot(t *testing.T) {
564+
port := freeUDPPort(t)
565+
blocker, err := net.ListenPacket("udp", "127.0.0.1:"+port)
566+
require.NoError(t, err)
567+
568+
ds, c := newTestDnsServer(t)
569+
setDnsConfig(c, "127.0.0.1", port, true, true)
570+
require.NoError(t, ds.reload(c, true))
571+
572+
ds.Start() // returns once the bind fails
573+
574+
ds.serverMu.Lock()
575+
assert.Nil(t, ds.server, "a listener that failed to bind must not stay parked in the slot")
576+
ds.serverMu.Unlock()
577+
578+
// With the slot released, a reload can retry once the port frees up
579+
require.NoError(t, blocker.Close())
580+
require.NoError(t, ds.reload(c, false))
581+
waitForBind(t, ds)
582+
583+
ds.serverMu.Lock()
584+
assert.NotNil(t, ds.server, "a same-addr reload should retry after a failed bind")
585+
ds.serverMu.Unlock()
586+
587+
ds.Stop()
588+
}
589+
590+
// A disable that lands while Start is between its unlocked check and the guard must not leave a listener behind.
591+
func TestDnsServer_Start_refusesWhenDisabledUnderLock(t *testing.T) {
592+
port := freeUDPPort(t)
593+
ds, c := newTestDnsServer(t)
594+
setDnsConfig(c, "127.0.0.1", port, true, true)
595+
require.NoError(t, ds.reload(c, true))
596+
require.True(t, ds.enabled.Load())
597+
598+
// Holding serverMu parks Start on the lock, the only way to land the disable in that window on purpose
599+
ds.serverMu.Lock()
600+
601+
done := make(chan struct{})
602+
go func() {
603+
ds.Start()
604+
close(done)
605+
}()
606+
607+
select {
608+
case <-done:
609+
ds.serverMu.Unlock()
610+
t.Fatal("Start returned early, the test never exercised the window")
611+
case <-time.After(time.Millisecond * 100):
612+
}
613+
614+
// The disable reload's critical section. It sees nothing running, so it never calls Stop.
615+
ds.enabled.Store(false)
616+
ds.serverMu.Unlock()
617+
618+
select {
619+
case <-done:
620+
case <-time.After(time.Second * 5):
621+
t.Fatal("Start never returned")
622+
}
623+
624+
ds.serverMu.Lock()
625+
assert.Nil(t, ds.server, "Start must not install a listener a disable already cancelled")
626+
ds.serverMu.Unlock()
627+
628+
pc, err := net.ListenPacket("udp", "127.0.0.1:"+port)
629+
require.NoError(t, err, "an orphaned listener is still holding the port")
630+
_ = pc.Close()
631+
}

0 commit comments

Comments
 (0)