plugin/azure: don't hold zMu across zone Lookup (#8447)

plugin/azure/azure.go

@@ -319,10 +319,15 @@ func (h *Azure) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg)
 	m.Authoritative = true
 	var result file.Result
 	for _, z := range zones {
+		// Only the zone pointer itself needs to be guarded against a
+		// concurrent swap in updateZones; Lookup can run unlocked since it
+		// may block for a while resolving external names via upstream.
 		h.zMu.RLock()
-		m.Answer, m.Ns, m.Extra, result = z.z.Lookup(ctx, state, qname)
+		zz := z.z
 		h.zMu.RUnlock()
 
+		m.Answer, m.Ns, m.Extra, result = zz.Lookup(ctx, state, qname)
+
 		// record type exists for this name (NODATA).
 		if len(m.Answer) != 0 || result == file.NoData {
 			break

plugin/azure/azure_test.go

@@ -3,11 +3,16 @@ package azure
 import (
 	"context"
 	"reflect"
+	"sync"
 	"testing"
+	"time"
 
+	"github.com/coredns/coredns/core/dnsserver"
+	"github.com/coredns/coredns/plugin"
 	"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"
 	"github.com/coredns/coredns/plugin/test"
 	"github.com/coredns/coredns/request"
 
@@ -178,3 +183,106 @@ func TestAzure(t *testing.T) {
 		}
 	}
 }
+
+// TestServeDNSDoesNotHoldLockDuringUpstreamLookup guards against holding
+// zMu.RLock() for the duration of a CNAME chase to an external name, which
+// can block on a slow upstream response. Holding the lock that long starves
+// the periodic zone update in updateZones (which needs zMu.Lock()), and
+// every query behind it queues up, so a single slow upstream response can
+// stall the whole plugin.
+func TestServeDNSDoesNotHoldLockDuringUpstreamLookup(t *testing.T) {
+	const timeout = 5 * time.Second
+
+	// entered signals that the upstream handler has been reached; release
+	// gates the handler's response so the test controls exactly when the
+	// in-flight query completes. releaseOnce lets every exit path (pass or
+	// fail) unblock the handler so no goroutine is left hanging.
+	entered := make(chan struct{})
+	release := make(chan struct{})
+	var releaseOnce sync.Once
+	releaseHandler := func() { releaseOnce.Do(func() { close(release) }) }
+	defer releaseHandler()
+
+	cfg := &dnsserver.Config{
+		Zone: ".",
+		Plugin: []plugin.Plugin{
+			func(plugin.Handler) plugin.Handler {
+				return plugin.HandlerFunc(func(_ context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
+					close(entered)
+					<-release
+					m := new(dns.Msg)
+					m.SetReply(r)
+					m.Answer = []dns.RR{test.A("external.target. 300 IN A 5.6.7.8")}
+					w.WriteMsg(m)
+					return dns.RcodeSuccess, nil
+				})
+			},
+		},
+	}
+	srv, err := dnsserver.NewServer("", []*dnsserver.Config{cfg})
+	if err != nil {
+		t.Fatal(err)
+	}
+	ctx := context.WithValue(context.Background(), dnsserver.Key{}, srv)
+
+	newZ := file.NewZone("example.org.", "")
+	newZ.Upstream = upstream.New()
+	for _, rr := range []string{
+		"example.org.	300	IN	SOA	ns1-06.azure-dns.com. azuredns-hostmaster.microsoft.com. 1 3600 300 2419200 300",
+		"cname-ext.example.org. 300 IN CNAME external.target.",
+	} {
+		r, _ := dns.NewRR(rr)
+		newZ.Insert(r)
+	}
+	zs := make(map[string][]*zone)
+	zs["example.org."] = append(zs["example.org."], &zone{zone: "example.org.", z: newZ})
+
+	az := Azure{
+		Next:      testHandler(),
+		Fall:      fall.Zero,
+		zoneNames: []string{"example.org."},
+		zones:     zs,
+	}
+
+	req := new(dns.Msg)
+	req.SetQuestion("cname-ext.example.org.", dns.TypeA)
+
+	done := make(chan struct{})
+	go func() {
+		rec := dnstest.NewRecorder(&test.ResponseWriter{})
+		az.ServeDNS(ctx, rec, req)
+		close(done)
+	}()
+
+	select {
+	case <-entered:
+		// The query has reached the upstream handler and is now blocked on
+		// release, so a subsequent zMu.Lock() genuinely races the lookup.
+	case <-time.After(timeout):
+		t.Fatal("query never reached the upstream handler")
+	}
+
+	lockAcquired := make(chan struct{})
+	go func() {
+		// Stands in for the zone swap updateZones does every minute.
+		az.zMu.Lock()
+		zs["example.org."][0].z = file.NewZone("example.org.", "")
+		az.zMu.Unlock()
+		close(lockAcquired)
+	}()
+
+	select {
+	case <-lockAcquired:
+		// zMu.Lock() wasn't blocked by the in-flight upstream lookup.
+	case <-time.After(timeout):
+		t.Fatal("zMu.Lock() blocked while a query awaited a slow upstream lookup")
+	}
+
+	releaseHandler()
+
+	select {
+	case <-done:
+	case <-time.After(timeout):
+		t.Fatal("ServeDNS did not complete after the upstream handler was released")
+	}
+}