From 3e5fd21e686718ffbbe4d494f9952ba9dca53db7 Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Wed, 3 Jul 2019 07:01:57 +0100 Subject: [PATCH] file: close correctlty after AXFR (#2943) * file: close correctlty after AXFR Don't hijack, but wait for the writes to be done and then savely close the connection. Fixes: #2929 Signed-off-by: Miek Gieben * Update comment Signed-off-by: Miek Gieben * file: close correctlty after AXFR (#2943) apply Signed-off-by: Miek Gieben --- plugin/file/xfr.go | 13 +++++++++---- test/file_xfr_test.go | 38 ++++++++++++++++++++------------------ 2 files changed, 29 insertions(+), 22 deletions(-) diff --git a/plugin/file/xfr.go b/plugin/file/xfr.go index 18b6bb117..08f71030a 100644 --- a/plugin/file/xfr.go +++ b/plugin/file/xfr.go @@ -3,6 +3,7 @@ package file import ( "context" "fmt" + "sync" "github.com/coredns/coredns/plugin" "github.com/coredns/coredns/request" @@ -31,9 +32,13 @@ func (x Xfr) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (in } ch := make(chan *dns.Envelope) - defer close(ch) tr := new(dns.Transfer) - go tr.Out(w, r, ch) + wg := new(sync.WaitGroup) + go func() { + wg.Add(1) + tr.Out(w, r, ch) + wg.Done() + }() j, l := 0, 0 records = append(records, records[0]) // add closing SOA to the end @@ -49,9 +54,9 @@ func (x Xfr) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (in if j < len(records) { ch <- &dns.Envelope{RR: records[j:]} } + close(ch) // Even though we close the channel here, we still have + wg.Wait() // to wait before we can return and close the connection. - w.Hijack() - // w.Close() // Client closes connection return dns.RcodeSuccess, nil } diff --git a/test/file_xfr_test.go b/test/file_xfr_test.go index ccea22ca6..5e4df3a67 100644 --- a/test/file_xfr_test.go +++ b/test/file_xfr_test.go @@ -2,7 +2,6 @@ package test import ( "fmt" - "io" "strings" "testing" "time" @@ -35,8 +34,7 @@ func TestLargeAXFR(t *testing.T) { } } ` - - // Start server, and send an AXFR query to the TCP port. We set the deadline to prevent the test from hanging. + // Start server, and send an AXFR query to the TCP port. We set the deadline to prevent the test from hanging. i, _, tcp, err := CoreDNSServerAndPorts(corefile) if err != nil { t.Fatalf("Could not get CoreDNS serving instance: %s", err) @@ -53,11 +51,18 @@ func TestLargeAXFR(t *testing.T) { co.SetWriteDeadline(time.Now().Add(5 * time.Second)) err = co.WriteMsg(m) if err != nil { - t.Fatalf("Unable to send TCP query: %s", err) + t.Fatalf("Unable to send AXFR/TCP query: %s", err) } - nrr := 0 // total number of transferred RRs - co.SetReadDeadline(time.Now().Add(60 * time.Second)) // use a longer timeout as it involves transferring a non-trivial amount of data. + // Then send another query on the same connection. We use this to confirm that multiple outstanding queries won't cause a race. + m.SetQuestion("0.example.com.", dns.TypeAAAA) + err = co.WriteMsg(m) + if err != nil { + t.Fatalf("Unable to send AAAA/TCP query: %s", err) + } + + // The AXFR query should be responded first. + nrr := 0 // total number of transferred RRs for { resp, err := co.ReadMsg() if err != nil { @@ -82,18 +87,15 @@ func TestLargeAXFR(t *testing.T) { t.Fatalf("Got an unexpected number of RRs: %d", nrr) } - // At the time of the initial implementation of this test, the remaining check would fail due to the problem described in PR #2866, so we return here. - // Once the issue is resolved this workaround should be removed to enable the check. - return - - // Once xfr is completed the server should close the connection, so a further read should result in an EOF error. - // This time we use a short timeout to make it faster in case the server doesn't behave as expected. - co.SetReadDeadline(time.Now().Add(time.Second)) - _, err = co.ReadMsg() - if err == nil { - t.Fatalf("Expected failure on further read, but it succeeded") + // The file plugin shouldn't hijack or (yet) close the connection, so the second query should also be responded. + resp, err := co.ReadMsg() + if err != nil { + t.Fatalf("Expected to receive reply, but didn't: %s", err) } - if err != io.EOF { - t.Fatalf("Expected EOF on further read, but got a different error: %v", err) + if len(resp.Answer) < 1 { + t.Fatalf("Expected a non-empty answer, but it was empty") + } + if resp.Answer[len(resp.Answer)-1].Header().Rrtype != dns.TypeAAAA { + t.Fatalf("Expected a AAAA answer, but it wasn't: type %d", resp.Answer[len(resp.Answer)-1].Header().Rrtype) } }