diff --git a/plugin/clouddns/clouddns_test.go b/plugin/clouddns/clouddns_test.go index 829aa71b0..269be2b04 100644 --- a/plugin/clouddns/clouddns_test.go +++ b/plugin/clouddns/clouddns_test.go @@ -4,8 +4,10 @@ import ( "context" "errors" "reflect" + "sync" "testing" + "github.com/coredns/coredns/plugin/file" "github.com/coredns/coredns/plugin/pkg/dnstest" "github.com/coredns/coredns/plugin/pkg/fall" "github.com/coredns/coredns/plugin/pkg/upstream" @@ -325,3 +327,132 @@ func TestCloudDNS(t *testing.T) { } } } + +// TestCloudDNSConcurrentServeDNS stresses r.ServeDNS directly to trigger +// concurrent Elem.Name() initialization from the file plugin. +func TestCloudDNSConcurrentServeDNS(t *testing.T) { + ctx := context.Background() + + r, err := New(ctx, + fakeGCPClient{}, + map[string][]string{ + "org.": {"sample-project-1:sample-zone-2", "sample-project-1:sample-zone-1"}, + }, + &upstream.Upstream{}, + ) + if err != nil { + t.Fatalf("Failed to create Cloud DNS: %v", err) + } + if err := r.Run(ctx); err != nil { + t.Fatalf("Failed to initialize Cloud DNS: %v", err) + } + + queries := []struct { + qname string + qtype uint16 + }{ + {"example.org", dns.TypeA}, + {"example.org", dns.TypeAAAA}, + {"sample.example.org", dns.TypeA}, + {"a.www.example.org", dns.TypeA}, + {"split-example.org", dns.TypeA}, + {"other-example.org", dns.TypeA}, + {"_dummy._tcp.example.org.", dns.TypeSRV}, + } + + var wg sync.WaitGroup + + // Concurrently refresh zones to race with Lookup reads. + wg.Add(1) + go func() { + defer wg.Done() + for range 50 { + _ = r.updateZones(ctx) + } + }() + + const workers = 32 + const iterations = 200 + for w := range workers { + wg.Add(1) + go func(id int) { + defer wg.Done() + for i := range iterations { + tc := queries[(id+i)%len(queries)] + req := new(dns.Msg) + req.SetQuestion(dns.Fqdn(tc.qname), tc.qtype) + rec := dnstest.NewRecorder(&test.ResponseWriter{}) + code, err := r.ServeDNS(ctx, rec, req) + if err != nil { + t.Errorf("ServeDNS error: %v", err) + return + } + if code != dns.RcodeSuccess { + t.Errorf("unexpected return code: %v", code) + return + } + } + }(w) + } + + wg.Wait() +} + +// TestCloudDNSConcurrentLookupNameCache stresses hostedZone.z.Lookup +// directly to trigger concurrent Elem.Name() initialization from the file plugin. +func TestCloudDNSConcurrentLookupNameCache(t *testing.T) { + ctx := context.Background() + + r, err := New(ctx, + fakeGCPClient{}, + map[string][]string{ + "org.": {"sample-project-1:sample-zone-2", "sample-project-1:sample-zone-1"}, + }, + &upstream.Upstream{}) + if err != nil { + t.Fatalf("Failed to create Cloud DNS: %v", err) + } + if err := r.Run(ctx); err != nil { + t.Fatalf("Failed to initialize Cloud DNS: %v", err) + } + + // Use a fixed set of qnames that exist in the zones to maximize reuse of the same Elems. + queries := []struct { + qname string + qtype uint16 + }{ + {"example.org.", dns.TypeA}, + {"example.org.", dns.TypeAAAA}, + {"sample.example.org.", dns.TypeA}, + {"a.www.example.org.", dns.TypeA}, + {"split-example.org.", dns.TypeA}, + {"other-example.org.", dns.TypeA}, + {"_dummy._tcp.example.org.", dns.TypeSRV}, + } + + // Fan-out goroutines that repeatedly call Lookup on the same Zone pointer. + const workers = 32 + const iterations = 300 + + var wg sync.WaitGroup + for _, hostedZones := range r.zones { + for _, hz := range hostedZones { + z := hz.z + wg.Add(workers) + for w := range workers { + go func(id int, zptr *file.Zone) { + defer wg.Done() + for i := range iterations { + tc := queries[(id+i)%len(queries)] + req := new(dns.Msg) + req.SetQuestion(tc.qname, tc.qtype) + rec := dnstest.NewRecorder(&test.ResponseWriter{}) + state := request.Request{W: rec, Req: req} + _, _, _, _ = zptr.Lookup(ctx, state, tc.qname) + } + }(w, z) + } + } + } + wg.Wait() +} diff --git a/plugin/file/tree/elem.go b/plugin/file/tree/elem.go index c1909649d..195e35a3e 100644 --- a/plugin/file/tree/elem.go +++ b/plugin/file/tree/elem.go @@ -12,6 +12,8 @@ type Elem struct { func newElem(rr dns.RR) *Elem { e := Elem{m: make(map[uint16][]dns.RR)} e.m[rr.Header().Rrtype] = []dns.RR{rr} + // Eagerly set the cached owner name to avoid racy lazy writes later. + e.name = rr.Header().Name return &e } @@ -56,12 +58,12 @@ func (e *Elem) All() []dns.RR { // Name returns the name for this node. func (e *Elem) Name() string { + // Read-only: name is eagerly set in newElem and should not be mutated here. if e.name != "" { return e.name } for _, rrs := range e.m { - e.name = rrs[0].Header().Name - return e.name + return rrs[0].Header().Name } return "" } diff --git a/plugin/file/tree/elem_test.go b/plugin/file/tree/elem_test.go new file mode 100644 index 000000000..d1956f5d0 --- /dev/null +++ b/plugin/file/tree/elem_test.go @@ -0,0 +1,41 @@ +package tree + +import ( + "testing" + + "github.com/miekg/dns" +) + +// Test that Name() falls back to reading from the stored RRs when the cached name is empty. +func TestElemName_FallbackWhenCachedEmpty(t *testing.T) { + rr, err := dns.NewRR("a.example. 3600 IN A 1.2.3.4") + if err != nil { + t.Fatalf("failed to create RR: %v", err) + } + + // Build via newElem to ensure m is populated + e := newElem(rr) + got := e.Name() + want := "a.example." + if got != want { + t.Fatalf("unexpected name; want %q, got %q", want, got) + } + + // clear the cached name + e.name = "" + + got = e.Name() + want = "a.example." + if got != want { + t.Fatalf("unexpected name; want %q, got %q", want, got) + } + + // clear the map + e.m = make(map[uint16][]dns.RR, 0) + + got = e.Name() + want = "" + if got != want { + t.Fatalf("unexpected name after clearing RR map; want %q, got %q", want, got) + } +} diff --git a/plugin/file/tree/less_test.go b/plugin/file/tree/less_test.go index bfc36ea34..a1af23a38 100644 --- a/plugin/file/tree/less_test.go +++ b/plugin/file/tree/less_test.go @@ -3,7 +3,10 @@ package tree import ( "sort" "strings" + "sync" "testing" + + "github.com/miekg/dns" ) type set []string @@ -78,3 +81,41 @@ Tests: } } } + +func TestLess_EmptyVsName(t *testing.T) { + if d := less("", "a."); d >= 0 { + t.Fatalf("expected < 0, got %d", d) + } + if d := less("a.", ""); d <= 0 { + t.Fatalf("expected > 0, got %d", d) + } +} + +func TestLess_EmptyVsEmpty(t *testing.T) { + if d := less("", ""); d != 0 { + t.Fatalf("expected 0, got %d", d) + } +} + +// Test that concurrent calls to Less (which calls Elem.Name) do not race or panic. +// See issue #7561 for reference. +func TestLess_ConcurrentNameAccess(t *testing.T) { + rr, err := dns.NewRR("a.example. 3600 IN A 1.2.3.4") + if err != nil { + t.Fatalf("failed to create RR: %v", err) + } + e := newElem(rr) + + const n = 200 + var wg sync.WaitGroup + wg.Add(n) + for range n { + go func() { + defer wg.Done() + // Compare the same name repeatedly; previously this could race due to lazy Name() writes. + _ = Less(e, "a.example.") + _ = e.Name() + }() + } + wg.Wait() +}