pan: Add some documentation for host resolution and context to tests

cleanup host resolution
cache result from parsing hosts file for SCION addresses until file is modified

Author: FR4NK-W

pkg/pan/dns_txt.go

@@ -22,16 +22,15 @@ import (
 )
 
 type dnsResolver struct{}
+
 var _ resolver = &dnsResolver{}
 
+// Resolve the name via DNS to return one scionAddr or an error.
 func (d *dnsResolver) Resolve(ctx context.Context, name string) (saddr scionAddr, err error) {
 	addresses, err := queryTXTRecord(ctx, name)
 	if err != nil {
 		return scionAddr{}, err
 	}
-	if len(addresses) == 0 {
-		return scionAddr{}, HostNotFoundError{Host: name}
-	}
 	var perr error
 	for _, addr := range addresses {
 		saddr, perr = parseSCIONAddr(addr)
@@ -42,27 +41,29 @@ func (d *dnsResolver) Resolve(ctx context.Context, name string) (saddr scionAddr
 	return scionAddr{}, fmt.Errorf("error parsing TXT SCION address records: %w", perr)
 }
 
-func queryTXTRecord(ctx context.Context, query string) (address []string, err error) {
-	if !strings.HasSuffix(query,".") {
-		query += "."
+// queryTXTRecord queries the DNS for DNS TXT record(s) specifying the SCION address(es) for host.
+// Returns either at least one address, or else an error, of type HostNotFoundError if no matching record was found.
+func queryTXTRecord(ctx context.Context, host string) (addresses []string, err error) {
+	if !strings.HasSuffix(host, ".") {
+		host += "."
 	}
 	resolver := net.Resolver{}
-	txtRecords, err := resolver.LookupHost(ctx, query)
+	txtRecords, err := resolver.LookupHost(ctx, host)
 	if dnsError, ok := err.(*net.DNSError); ok {
 		if dnsError.IsNotFound {
-			return address, HostNotFoundError{query}
+			return addresses, HostNotFoundError{host}
 		}
 	}
 	if err != nil {
-		return address, err
+		return addresses, err
 	}
 	for _, txt := range txtRecords {
 		if strings.HasPrefix(txt, "scion=") {
-			address = append(address, strings.TrimPrefix(txt, "scion="))
+			addresses = append(addresses, strings.TrimPrefix(txt, "scion="))
 		}
 	}
-	if len(address) == 0 {
-		return address, HostNotFoundError{query}
+	if len(addresses) == 0 {
+		return addresses, HostNotFoundError{host}
 	}
-	return address, nil
+	return addresses, nil
 }

pkg/pan/hosts.go

@@ -88,14 +88,22 @@ func (resolvers resolverList) Resolve(ctx context.Context, name string) (scionAd
 	var errHostNotFound HostNotFoundError
 	var rerr error
 	for _, resolver := range resolvers {
-		if resolver != nil {
-			addr, err := resolver.Resolve(ctx, name)
-			if err == nil {
-				return addr, nil
-			} else if !errors.As(err, &errHostNotFound) {
-				// do not directly fail on first resolver error
-				rerr = err
-			}
+		if resolver == nil {
+			// skip RAINS resolver when disabled
+			continue
+		}
+		// check ctx to avoid unnecessary calls with already expired context
+		select {
+		case <-ctx.Done():
+			break
+		default:
+		}
+		addr, err := resolver.Resolve(ctx, name)
+		if err == nil {
+			return addr, nil
+		} else if !errors.As(err, &errHostNotFound) {
+			// do not directly fail on first resolver error
+			rerr = err
 		}
 	}
 	if rerr != nil {

pkg/pan/hosts_test.go

@@ -15,6 +15,7 @@
 package pan
 
 import (
+	"context"
 	"fmt"
 	"testing"
 
@@ -53,7 +54,7 @@ func TestHostsfileResolver(t *testing.T) {
 		{"foobar", assertErrHostNotFound, scionAddr{}},
 	}
 	for _, c := range cases {
-		actual, err := resolver.Resolve(c.name)
+		actual, err := resolver.Resolve(context.TODO(), c.name)
 		if !c.assertErr(t, err) {
 			continue
 		}
@@ -63,7 +64,7 @@ func TestHostsfileResolver(t *testing.T) {
 
 func TestHostsfileResolverNonexisting(t *testing.T) {
 	resolver := &hostsfileResolver{"non_existing_hosts_file"}
-	_, err := resolver.Resolve("something")
+	_, err := resolver.Resolve(context.TODO(), "something")
 	assert.Error(t, err)
 }
 
@@ -92,7 +93,7 @@ func TestResolverList(t *testing.T) {
 		{"boo", assertErrHostNotFound, scionAddr{}},
 	}
 	for _, c := range cases {
-		actual, err := resolver.Resolve(c.name)
+		actual, err := resolver.Resolve(context.TODO(), c.name)
 		c.assertErr(t, err)
 		assert.Equal(t, c.expected, actual)
 	}
@@ -107,7 +108,9 @@ type dummyResolver struct {
 	hosts map[string]scionAddr
 }
 
-func (r dummyResolver) Resolve(name string) (scionAddr, error) {
+var _ resolver = &dummyResolver{}
+
+func (r dummyResolver) Resolve(ctx context.Context, name string) (scionAddr, error) {
 	if h, ok := r.hosts[name]; ok {
 		return h, nil
 	} else {

pkg/pan/hostsfile.go

@@ -20,22 +20,29 @@ import (
 	"fmt"
 	"os"
 	"strings"
+	"sync"
+	"time"
 )
 
 type hostsTable map[string]scionAddr
 
+var cachedHostsTable struct {
+	mapping hostsTable
+	updated time.Time
+	sync.Mutex
+}
+
 // hostsfileResolver is an implementation of the resolver interface, backed
 // by an /etc/hosts-like file.
 type hostsfileResolver struct {
 	path string
 }
+
 var _ resolver = &hostsfileResolver{}
 
 func (r *hostsfileResolver) Resolve(ctx context.Context, name string) (scionAddr, error) {
-	// Note: obviously not perfectly elegant to parse the entire file for
-	// every query. However, properly caching this and still always provide
-	// fresh results after changes to the hosts file seems like a bigger task and
-	// for now that would be overkill.
+	cachedHostsTable.Lock()
+	defer cachedHostsTable.Unlock()
 	table, err := loadHostsFile(r.path)
 	if err != nil {
 		return scionAddr{}, fmt.Errorf("error loading %s: %w", r.path, err)
@@ -47,17 +54,34 @@ func (r *hostsfileResolver) Resolve(ctx context.Context, name string) (scionAddr
 	return addr, nil
 }
 
+// loadHostsFile provides a cached copy of the hostsTable or loads and parses the host file at path if it was modified
+// since the last update.
 func loadHostsFile(path string) (hostsTable, error) {
-	file, err := os.Open(path)
+	stat, err := os.Stat(path)
 	if os.IsNotExist(err) {
 		// not existing file treated like an empty file,
 		// just return an empty table
 		return hostsTable(nil), nil
-	} else if err != nil {
-		return nil, err
+	}
+	if err != nil {
+		return hostsTable(nil), err
+	}
+	if stat.ModTime().Before(cachedHostsTable.updated) {
+		return cachedHostsTable.mapping, nil
+	}
+
+	file, err := os.Open(path)
+	if err != nil {
+		return hostsTable(nil), err
 	}
 	defer file.Close()
-	return parseHostsFile(file)
+	mapping, err := parseHostsFile(file)
+	if err != nil {
+		return hostsTable(nil), err
+	}
+	cachedHostsTable.mapping = mapping
+	cachedHostsTable.updated = time.Now()
+	return cachedHostsTable.mapping, nil
 }
 
 func parseHostsFile(file *os.File) (hostsTable, error) {

pkg/pan/pan.go

@@ -108,6 +108,8 @@ import (
 	"time"
 )
 
+var defaultResolveTimeout = time.Second
+
 // ResolveUDPAddr parses the address and resolves the hostname.
 // The address can be of the form of a SCION address (i.e. of the form "ISD-AS,[IP]:port")
 // or in the form of "hostname:port".
@@ -122,13 +124,13 @@ import (
 //
 // Returns HostNotFoundError if none of the sources did resolve the hostname.
 func ResolveUDPAddr(address string) (addr UDPAddr, err error) {
-	ctx, cancel := context.WithTimeout(context.Background(), time.Second)
-	return ResolveUDPAddrContext(ctx, cancel, address)
+	ctx, cancel := context.WithTimeout(context.Background(), defaultResolveTimeout)
+	defer cancel()
+	return ResolveUDPAddrContext(ctx, address)
 }
 
 // ResolveUDPAddrContext resolves address, taking a context and a cancel function, see ResolveUDPAddr.
-func ResolveUDPAddrContext(ctx context.Context, cancel context.CancelFunc, address string) (addr UDPAddr, err error) {
-	defer cancel()
+func ResolveUDPAddrContext(ctx context.Context, address string) (addr UDPAddr, err error) {
 	done := make(chan struct{})
 	go func() {
 		defer close(done)
@@ -142,7 +144,6 @@ func ResolveUDPAddrContext(ctx context.Context, cancel context.CancelFunc, addre
 	return
 }
 
-
 // HostNotFoundError is returned by ResolveUDPAddr when the name was not found, but
 // otherwise no error occurred.
 type HostNotFoundError struct {

pkg/pan/rains.go

@@ -35,6 +35,7 @@ func init() {
 }
 
 type rainsResolver struct{}
+
 var _ resolver = &rainsResolver{}
 
 func (r *rainsResolver) Resolve(ctx context.Context, name string) (scionAddr, error) {
@@ -46,7 +47,7 @@ func (r *rainsResolver) Resolve(ctx context.Context, name string) (scionAddr, er
 		// nobody to ask, so we won't get a reply
 		return scionAddr{}, HostNotFoundError{name}
 	}
-	return rainsQuery(server, name)
+	return rainsQuery(ctx, server, name)
 }
 
 func readRainsConfig() (UDPAddr, error) {
@@ -63,12 +64,12 @@ func readRainsConfig() (UDPAddr, error) {
 	return address, nil
 }
 
-func rainsQuery(server UDPAddr, hostname string) (scionAddr, error) {
+func rainsQuery(ctx context.Context, server UDPAddr, hostname string) (scionAddr, error) {
 	const (
-		ctx     = "."                    // use global context
-		qType   = rains.OTScionAddr      // request SCION addresses
-		expire  = 5 * time.Minute        // sensible expiry date?
-		timeout = 500 * time.Millisecond // timeout for query
+		rainsCtx = "."                    // use global context
+		qType    = rains.OTScionAddr      // request SCION addresses
+		expire   = 5 * time.Minute        // sensible expiry date?
+		timeout  = 500 * time.Millisecond // timeout for query
 	)
 	qOpts := []rains.Option{} // no options
 
@@ -78,7 +79,7 @@ func rainsQuery(server UDPAddr, hostname string) (scionAddr, error) {
 	// TODO(chaehni): This call can sometimes cause a timeout even though the server is reachable (see issue #221)
 	// The timeout value has been decreased to counter this behavior until the problem is resolved.
 	srv := server.snetUDPAddr()
-	reply, err := rains.Query(hostname, ctx, []rains.Type{qType}, qOpts, expire, timeout, srv)
+	reply, err := rains.Query(hostname, rainsCtx, []rains.Type{qType}, qOpts, expire, timeout, srv)
 	if err != nil {
 		return scionAddr{}, fmt.Errorf("address for host %q not found: %w", hostname, err)
 	}