plugin/file: simplify locking (#3024)

* plugin/file: simplify locking

Simplify the locking, remove the reloadMu and just piggyback on the
other lock for accessing content, which assumes things can be move
underneath.

Copy the Apex and Zone to new vars to make sure the pointer isn't
updated from under us.

The releadMu isn't need at all, the time.Ticker firing while we're
reading means we will just miss that tick and get it on the next go.

Add rrutil subpackage and put some more generic functions in there, that
are now used from file and the tree package. This removes some
duplication.

Rename additionalProcessing that didn't actually do that to
externalLookup, because that's what being done at some point.
Signed-off-by: Miek Gieben <miek@miek.nl>

* Update plugin/file/lookup.go
Co-Authored-By: Michael Grosser <development@stp-ip.net>

plugin/file/reload.go

@@ -13,10 +13,8 @@ func (z *Zone) Reload() error {
 	tick := time.NewTicker(z.ReloadInterval)
 
 	go func() {
-
 		for {
 			select {
-
 			case <-tick.C:
 				zFile := z.File()
 				reader, err := os.Open(zFile)
@@ -35,10 +33,10 @@ func (z *Zone) Reload() error {
 				}
 
 				// copy elements we need
-				z.reloadMu.Lock()
+				z.Lock()
 				z.Apex = zone.Apex
 				z.Tree = zone.Tree
-				z.reloadMu.Unlock()
+				z.Unlock()
 
 				log.Infof("Successfully reloaded zone %q in %q with serial %d", z.origin, zFile, z.Apex.SOA.Serial)
 				z.Notify()
@@ -52,11 +50,10 @@ func (z *Zone) Reload() error {
 	return nil
 }
 
-// SOASerialIfDefined returns the SOA's serial if the zone has a SOA record in the Apex, or
-// -1 otherwise.
+// SOASerialIfDefined returns the SOA's serial if the zone has a SOA record in the Apex, or -1 otherwise.
 func (z *Zone) SOASerialIfDefined() int64 {
-	z.reloadMu.Lock()
-	defer z.reloadMu.Unlock()
+	z.RLock()
+	defer z.RUnlock()
 	if z.Apex.SOA != nil {
 		return int64(z.Apex.SOA.Serial)
 	}

plugin/file/rrutil/util.go

+// Package rrutil provides function to find certain RRs in slices.
+package rrutil
+
+import "github.com/miekg/dns"
+
+// SubTypeSignature returns the RRSIG for the subtype.
+func SubTypeSignature(rrs []dns.RR, subtype uint16) []dns.RR {
+	sigs := []dns.RR{}
+	// there may be multiple keys that have signed this subtype
+	for _, sig := range rrs {
+		if s, ok := sig.(*dns.RRSIG); ok {
+			if s.TypeCovered == subtype {
+				sigs = append(sigs, s)
+			}
+		}
+	}
+	return sigs
+}
+
+// CNAMEForType returns the RR that have the qtype from targets.
+func CNAMEForType(rrs []dns.RR, qtype uint16) []dns.RR {
+	ret := []dns.RR{}
+	for _, target := range rrs {
+		if target.Header().Rrtype == qtype {
+			ret = append(ret, target)
+		}
+	}
+	return ret
+}

plugin/file/secondary.go

@@ -51,11 +51,11 @@ Transfer:
 		return Err
 	}
 
-	z.apexMu.Lock()
+	z.Lock()
 	z.Tree = z1.Tree
 	z.Apex = z1.Apex
 	*z.Expired = false
-	z.apexMu.Unlock()
+	z.Unlock()
 	log.Infof("Transferred: %s from %s", z.origin, tr)
 	return nil
 }

plugin/file/tree/glue.go

+package tree
+
+import (
+	"github.com/coredns/coredns/plugin/file/rrutil"
+
+	"github.com/miekg/dns"
+)
+
+// Glue returns any potential glue records for nsrrs.
+func (t *Tree) Glue(nsrrs []dns.RR, do bool) []dns.RR {
+	glue := []dns.RR{}
+	for _, rr := range nsrrs {
+		if ns, ok := rr.(*dns.NS); ok && dns.IsSubDomain(ns.Header().Name, ns.Ns) {
+			glue = append(glue, t.searchGlue(ns.Ns, do)...)
+		}
+	}
+	return glue
+}
+
+// searchGlue looks up A and AAAA for name.
+func (t *Tree) searchGlue(name string, do bool) []dns.RR {
+	glue := []dns.RR{}
+
+	// A
+	if elem, found := t.Search(name); found {
+		glue = append(glue, elem.Type(dns.TypeA)...)
+		if do {
+			sigs := elem.Type(dns.TypeRRSIG)
+			sigs = rrutil.SubTypeSignature(sigs, dns.TypeA)
+			glue = append(glue, sigs...)
+		}
+	}
+
+	// AAAA
+	if elem, found := t.Search(name); found {
+		glue = append(glue, elem.Type(dns.TypeAAAA)...)
+		if do {
+			sigs := elem.Type(dns.TypeRRSIG)
+			sigs = rrutil.SubTypeSignature(sigs, dns.TypeAAAA)
+			glue = append(glue, sigs...)
+		}
+	}
+	return glue
+}

plugin/file/zone.go

@@ -15,14 +15,15 @@ import (
 	"github.com/miekg/dns"
 )
 
-// Zone defines a structure that contains all data related to a DNS zone.
+// Zone is a structure that contains all data related to a DNS zone.
 type Zone struct {
 	origin  string
 	origLen int
 	file    string
 	*tree.Tree
 	Apex
-	apexMu sync.RWMutex
+
+	sync.RWMutex
 
 	TransferTo   []string
 	StartupOnce  sync.Once
@@ -30,9 +31,9 @@ type Zone struct {
 	Expired      *bool
 
 	ReloadInterval time.Duration
-	reloadMu       sync.RWMutex
 	reloadShutdown chan bool
-	Upstream       *upstream.Upstream // Upstream for looking up external names during the resolution process.
+
+	Upstream *upstream.Upstream // Upstream for looking up external names during the resolution process.
 }
 
 // Apex contains the apex records of a zone: SOA, NS and their potential signatures.
@@ -122,21 +123,18 @@ func (z *Zone) Insert(r dns.RR) error {
 	return nil
 }
 
-// Delete deletes r from z.
-func (z *Zone) Delete(r dns.RR) { z.Tree.Delete(r) }
-
-// File retrieves the file path in a safe way
+// File retrieves the file path in a safe way.
 func (z *Zone) File() string {
-	z.reloadMu.Lock()
-	defer z.reloadMu.Unlock()
+	z.RLock()
+	defer z.RUnlock()
 	return z.file
 }
 
-// SetFile updates the file path in a safe way
+// SetFile updates the file path in a safe way.
 func (z *Zone) SetFile(path string) {
-	z.reloadMu.Lock()
+	z.Lock()
 	z.file = path
-	z.reloadMu.Unlock()
+	z.Unlock()
 }
 
 // TransferAllowed checks if incoming request for transferring the zone is allowed according to the ACLs.
@@ -162,18 +160,16 @@ func (z *Zone) TransferAllowed(state request.Request) bool {
 // All returns all records from the zone, the first record will be the SOA record,
 // otionally followed by all RRSIG(SOA)s.
 func (z *Zone) All() []dns.RR {
-	if z.ReloadInterval > 0 {
-		z.reloadMu.RLock()
-		defer z.reloadMu.RUnlock()
-	}
-
 	records := []dns.RR{}
+	z.RLock()
 	allNodes := z.Tree.All()
+	z.RUnlock()
+
 	for _, a := range allNodes {
 		records = append(records, a.All()...)
 	}
 
-	// Either the entire Apex is filled or none it, this isn't enforced here though.
+	z.RLock()
 	if len(z.Apex.SIGNS) > 0 {
 		records = append(z.Apex.SIGNS, records...)
 	}
@@ -184,9 +180,12 @@ func (z *Zone) All() []dns.RR {
 	if len(z.Apex.SIGSOA) > 0 {
 		records = append(z.Apex.SIGSOA, records...)
 	}
+
 	if z.Apex.SOA != nil {
+		z.RUnlock()
 		return append([]dns.RR{z.Apex.SOA}, records...)
 	}
+	z.RUnlock()
 	return records
 }