Keep static_host_map DNS results when a lookup fails

examples/config.yml

@@ -39,8 +39,9 @@ static_host_map:
   # public address. Other valid options are "ip6" and "ip" (returns both.)
   #network: ip4
 
-  # lookup_timeout is the DNS query timeout.
-  #lookup_timeout: 250ms
+  # lookup_timeout is the DNS query timeout. A failed lookup keeps the last known good addresses, only a not found
+  # answer removes them.
+  #lookup_timeout: 5s
 
 lighthouse:
   # am_lighthouse is used to enable lighthouse functionality for a node. This should ONLY be true on nodes

handshake_manager.go

@@ -403,6 +403,8 @@ func (hm *HandshakeManager) StartHandshake(vpnAddr netip.Addr, cacheCb func(*Han
 
 	hm.Unlock()
 	hm.lightHouse.QueryServer(vpnAddr)
+	// If this host came from a static_map hostname its addresses may have moved, retries will pick up anything new
+	hostinfo.remotes.ResolveHostnames()
 	return hostinfo
 }
 

lighthouse.go

@@ -452,7 +452,7 @@ func getStaticMapCadence(c *config.C) (time.Duration, error) {
 }
 
 func getStaticMapLookupTimeout(c *config.C) (time.Duration, error) {
-	lookupTimeout := c.GetString("static_map.lookup_timeout", "250ms")
+	lookupTimeout := c.GetString("static_map.lookup_timeout", "5s")
 	d, err := time.ParseDuration(lookupTimeout)
 	if err != nil {
 		return 0, err
@@ -630,7 +630,8 @@ func (lh *LightHouse) addStaticRemotes(i int, d time.Duration, network string, t
 	ctx := lh.ctx
 	lh.Unlock()
 
-	hr, err := NewHostnameResults(ctx, lh.l, d, network, timeout, toAddrs, func() {
+	// On reload am.hr is the previous resolver, its last known good addresses carry over in case DNS is down right now
+	hr, err := NewHostnameResults(ctx, lh.l, d, network, timeout, toAddrs, am.hr, func() {
 		// This callback runs whenever the DNS hostname resolver finds a different set of addr's
 		// in its resolution for hostnames.
 		am.Lock()
@@ -642,7 +643,8 @@ func (lh *LightHouse) addStaticRemotes(i int, d time.Duration, network string, t
 	}
 	am.unlockedSetHostnamesResults(hr)
 
-	for _, addrPort := range hr.GetAddrs() {
+	// Only literals belong in the cache, anything from DNS has to stay in hr where a not found answer can remove it
+	for _, addrPort := range hr.literalAddrs() {
 		if !lh.shouldAdd([]netip.Addr{vpnAddr}, addrPort.Addr()) {
 			continue
 		}

remote_list.go

@@ -2,6 +2,7 @@ package nebula
 
 import (
 	"context"
+	"errors"
 	"log/slog"
 	"net"
 	"net/netip"
@@ -58,6 +59,8 @@ type cacheV6 struct {
 type hostnamePort struct {
 	name string
 	port uint16
+	// addr is set when name is an ip literal, these never need a lookup
+	addr netip.Addr
 }
 
 type hostnamesResults struct {
@@ -65,15 +68,20 @@ type hostnamesResults struct {
 	network       string
 	lookupTimeout time.Duration
 	cancelFn      func()
+	resolveNow    chan struct{}
 	l             *slog.Logger
 	ips           atomic.Pointer[map[netip.AddrPort]struct{}]
+	// known is the last known good addresses per hostname, kept so a reload can carry them forward
+	known atomic.Pointer[map[hostnamePort][]netip.AddrPort]
 }
 
-func NewHostnameResults(ctx context.Context, l *slog.Logger, d time.Duration, network string, timeout time.Duration, hostPorts []string, onUpdate func()) (*hostnamesResults, error) {
+// NewHostnameResults starts resolving hostPorts. Any hostnames also in prev start with its last known good addresses.
+func NewHostnameResults(ctx context.Context, l *slog.Logger, d time.Duration, network string, timeout time.Duration, hostPorts []string, prev *hostnamesResults, onUpdate func()) (*hostnamesResults, error) {
 	r := &hostnamesResults{
 		hostnames:     make([]hostnamePort, len(hostPorts)),
 		network:       network,
 		lookupTimeout: timeout,
+		resolveNow:    make(chan struct{}, 1),
 		l:             l,
 	}
 
@@ -81,6 +89,13 @@ func NewHostnameResults(ctx context.Context, l *slog.Logger, d time.Duration, ne
 	// DNS lookups for hostnames that aren't hardcoded IP's will happen in a background goroutine.
 	performBackgroundLookup := false
 	ips := map[netip.AddrPort]struct{}{}
+	known := make([][]netip.AddrPort, len(hostPorts))
+	var prevKnown map[hostnamePort][]netip.AddrPort
+	if prev != nil && prev.network == network {
+		if p := prev.known.Load(); p != nil {
+			prevKnown = *p
+		}
+	}
 	for idx, hostPort := range hostPorts {
 
 		rIp, sPort, err := net.SplitHostPort(hostPort)
@@ -98,13 +113,19 @@ func NewHostnameResults(ctx context.Context, l *slog.Logger, d time.Duration, ne
 		if err != nil {
 			// This address is a hostname, not an IP address
 			performBackgroundLookup = true
+			known[idx] = prevKnown[r.hostnames[idx]]
+			for _, a := range known[idx] {
+				ips[a] = struct{}{}
+			}
 			continue
 		}
 
 		// Save the IP address immediately
+		r.hostnames[idx].addr = addr
 		ips[netip.AddrPortFrom(addr, uint16(iPort))] = struct{}{}
 	}
 	r.ips.Store(&ips)
+	r.storeKnown(known)
 
 	// Time for the DNS lookup goroutine
 	if performBackgroundLookup {
@@ -114,21 +135,12 @@ func NewHostnameResults(ctx context.Context, l *slog.Logger, d time.Duration, ne
 		go func() {
 			defer ticker.Stop()
 			for {
+				r.resolve(newCtx, known)
+				r.storeKnown(known)
 				netipAddrs := map[netip.AddrPort]struct{}{}
-				for _, hostPort := range r.hostnames {
-					timeoutCtx, timeoutCancel := context.WithTimeout(ctx, r.lookupTimeout)
-					addrs, err := net.DefaultResolver.LookupNetIP(timeoutCtx, r.network, hostPort.name)
-					timeoutCancel()
-					if err != nil {
-						l.Error("DNS resolution failed for static_map host",
-							"hostname", hostPort.name,
-							"network", r.network,
-							"error", err,
-						)
-						continue
-					}
+				for _, addrs := range known {
 					for _, a := range addrs {
-						netipAddrs[netip.AddrPortFrom(a.Unmap(), hostPort.port)] = struct{}{}
+						netipAddrs[a] = struct{}{}
 					}
 				}
 				origSet := r.ips.Load()
@@ -159,7 +171,7 @@ func NewHostnameResults(ctx context.Context, l *slog.Logger, d time.Duration, ne
 				case <-newCtx.Done():
 					return
 				case <-ticker.C:
-					continue
+				case <-r.resolveNow:
 				}
 			}
 		}()
@@ -168,6 +180,84 @@ func NewHostnameResults(ctx context.Context, l *slog.Logger, d time.Duration, ne
 	return r, nil
 }
 
+// resolve looks up every hostname in parallel and folds the results into known
+func (hr *hostnamesResults) resolve(ctx context.Context, known [][]netip.AddrPort) {
+	var wg sync.WaitGroup
+	for i, hostPort := range hr.hostnames {
+		if hostPort.addr.IsValid() {
+			known[i] = []netip.AddrPort{netip.AddrPortFrom(hostPort.addr, hostPort.port)}
+			continue
+		}
+
+		wg.Go(func() {
+			timeoutCtx, timeoutCancel := context.WithTimeout(ctx, hr.lookupTimeout)
+			addrs, err := net.DefaultResolver.LookupNetIP(timeoutCtx, hr.network, hostPort.name)
+			timeoutCancel()
+			known[i] = applyLookup(known[i], hostPort.port, addrs, err)
+			// A canceled ctx means we are reloading or shutting down, not that DNS failed
+			if err != nil && ctx.Err() == nil {
+				hr.l.Error("DNS resolution failed for static_map host",
+					"hostname", hostPort.name,
+					"network", hr.network,
+					"error", err,
+					"usingAddrs", known[i],
+				)
+			}
+		})
+	}
+	wg.Wait()
+}
+
+// applyLookup returns the addresses to use for a hostname given the previous ones and a lookup result.
+// Only a not found answer clears them, a stale lighthouse address costs a handshake but an empty set costs everything.
+// darwin also reports not found while offline, that's harmless since nothing is reachable and the next handshake
+// triggers a fresh lookup once we're back.
+func applyLookup(prev []netip.AddrPort, port uint16, addrs []netip.Addr, err error) []netip.AddrPort {
+	if err != nil {
+		var dnsErr *net.DNSError
+		if errors.As(err, &dnsErr) && dnsErr.IsNotFound {
+			return nil
+		}
+		return prev
+	}
+
+	out := make([]netip.AddrPort, 0, len(addrs))
+	for _, a := range addrs {
+		out = append(out, netip.AddrPortFrom(a.Unmap(), port))
+	}
+	return out
+}
+
+func (hr *hostnamesResults) storeKnown(known [][]netip.AddrPort) {
+	byName := make(map[hostnamePort][]netip.AddrPort, len(known))
+	for i, addrs := range known {
+		byName[hr.hostnames[i]] = addrs
+	}
+	hr.known.Store(&byName)
+}
+
+// literalAddrs returns the entries that were configured as ip addresses rather than hostnames
+func (hr *hostnamesResults) literalAddrs() []netip.AddrPort {
+	var out []netip.AddrPort
+	for _, hostPort := range hr.hostnames {
+		if hostPort.addr.IsValid() {
+			out = append(out, netip.AddrPortFrom(hostPort.addr, hostPort.port))
+		}
+	}
+	return out
+}
+
+// ResolveNow asks the background goroutine to look up the hostnames again without waiting for the next tick
+func (hr *hostnamesResults) ResolveNow() {
+	if hr == nil || hr.resolveNow == nil {
+		return
+	}
+	select {
+	case hr.resolveNow <- struct{}{}:
+	default:
+	}
+}
+
 func (hr *hostnamesResults) Cancel() {
 	if hr != nil && hr.cancelFn != nil {
 		hr.cancelFn()
@@ -256,6 +346,13 @@ func (r *RemoteList) ResetForOwner(ownerVpnAddr netip.Addr) {
 	r.shouldRebuild = true
 }
 
+// ResolveHostnames kicks a fresh DNS lookup of any static_map hostnames for this host
+func (r *RemoteList) ResolveHostnames() {
+	r.RLock()
+	defer r.RUnlock()
+	r.hr.ResolveNow()
+}
+
 // ClearHostnameResults cancels the in-flight DNS resolver goroutine (if any) and drops the resolved IP cache.
 func (r *RemoteList) ClearHostnameResults() {
 	r.Lock()

remote_list_test.go

@@ -1,10 +1,14 @@
 package nebula
 
 import (
+	"context"
 	"encoding/binary"
+	"net"
 	"net/netip"
 	"testing"
+	"time"
 
+	"github.com/slackhq/nebula/test"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -328,6 +332,55 @@ func BenchmarkSortRebuild(b *testing.B) {
 	})
 }
 
+func TestApplyLookup(t *testing.T) {
+	prev := []netip.AddrPort{netip.MustParseAddrPort("1.1.1.1:4242")}
+
+	// A successful lookup replaces the previous addresses
+	got := applyLookup(prev, 4242, []netip.Addr{netip.MustParseAddr("::ffff:2.2.2.2")}, nil)
+	assert.Equal(t, []netip.AddrPort{netip.MustParseAddrPort("2.2.2.2:4242")}, got)
+
+	// A not found answer means the record is gone
+	assert.Empty(t, applyLookup(prev, 4242, nil, &net.DNSError{Err: "no such host", IsNotFound: true}))
+
+	// Anything else is not new information, keep what we had
+	assert.Equal(t, prev, applyLookup(prev, 4242, nil, &net.DNSError{Err: "i/o timeout", IsTimeout: true}))
+	assert.Equal(t, prev, applyLookup(prev, 4242, nil, &net.DNSError{Err: "server misbehaving", IsTemporary: true}))
+	assert.Equal(t, prev, applyLookup(prev, 4242, nil, context.DeadlineExceeded))
+}
+
+func TestNewHostnameResults_CarriesKnown(t *testing.T) {
+	// A canceled context keeps the background lookup from learning anything, as if DNS were down
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+
+	known := netip.MustParseAddrPort("1.1.1.1:4242")
+	prev := &hostnamesResults{network: "ip4"}
+	prev.known.Store(&map[hostnamePort][]netip.AddrPort{{name: "lh.invalid", port: 4242}: {known}})
+
+	hostPorts := []string{"lh.invalid:4242", "10.0.0.1:4242"}
+	literal := netip.MustParseAddrPort("10.0.0.1:4242")
+
+	hr, err := NewHostnameResults(ctx, test.NewLogger(), time.Hour, "ip4", time.Second, hostPorts, prev, func() {})
+	require.NoError(t, err)
+	assert.ElementsMatch(t, []netip.AddrPort{known, literal}, hr.GetAddrs())
+	assert.Equal(t, []netip.AddrPort{literal}, hr.literalAddrs())
+
+	// And again from the new one, before it has finished a lookup of its own
+	hr2, err := NewHostnameResults(ctx, test.NewLogger(), time.Hour, "ip4", time.Second, hostPorts, hr, func() {})
+	require.NoError(t, err)
+	assert.ElementsMatch(t, []netip.AddrPort{known, literal}, hr2.GetAddrs())
+
+	// A port change is a different entry
+	hr3, err := NewHostnameResults(ctx, test.NewLogger(), time.Hour, "ip4", time.Second, []string{"lh.invalid:4243"}, prev, func() {})
+	require.NoError(t, err)
+	assert.Empty(t, hr3.GetAddrs())
+
+	// So is a network change
+	hr4, err := NewHostnameResults(ctx, test.NewLogger(), time.Hour, "ip6", time.Second, hostPorts, prev, func() {})
+	require.NoError(t, err)
+	assert.Equal(t, []netip.AddrPort{literal}, hr4.GetAddrs())
+}
+
 func newIp4AndPortFromString(s string) *V4AddrPort {
 	a := netip.MustParseAddrPort(s)
 	v4Addr := a.Addr().As4()