libnetwork/d/overlay: properly model peer db

The overlay driver assumes that the peer table in NetworkDB will always
converge to a 1:1:1 mapping from peer endpoint IP address to MAC address
to VTEP. While this currently holds true in practice most of the time,
it is not an invariant and there are ways that users can violate this
assumption.

The driver detects whether peer entries conflict with each other by
matching up (IP, MAC) tuples. In the common case this works out fine as
the MAC address for an endpoint is generally derived from the assigned
IP address. If an IP address gets reassigned to a container on another
node the MAC address will follow, so the driver's conflict resolution
logic will behave as intended. However users may explicitly configure
the MAC address for a container's network endpoints. If an IP address
gets reassigned from a container with an auto-generated MAC address to a
container with a manually-configured MAC, or vice versa, the driver
would not detect the conflict as the (IP, MAC) tuples won't match up. It
would attempt to program the kernel's neighbor table with two
conflicting MAC addresses for one IP, which will fail. And since it
does not realize that there is a conflict, the driver won't reprogram
the kernel from the remaining entry when the other entry is deleted.

The assumption that only one IP address may resolve to a given MAC
address is violated if multiple IP addresses are assigned to an
endpoint. This rarely comes up in practice today as the overlay driver
only supports IPv4 single-stack connectivity for endpoints. If multiple
distinct peer entries exist with the same MAC address, the driver will
delete the MAC->VTEP mapping from the kernel's forwarding database when
any entry is deleted, even if other entries remain active. This
limitation is one of the biggest obstacles in the way of supporting IPv6
and dual-stack connectivity for endpoints attached to overlay networks.

Modify the peer db logic to correctly handle the cases where peer
entries have non-unique MAC or VTEP values. Treat any set of entries
with non-unique IP addresses as a conflict, irrespective of the entries'
MAC addresses. Maintain a reference count of forwarding database entries
and only delete the MAC->VTEP mapping from the kernel when there are no
longer any neighbor entries which resolve to that MAC.

Signed-off-by: Cory Snider <csnider@mirantis.com>

libnetwork/drivers/overlay/ov_network.go

@@ -1,4 +1,5 @@
-//go:build linux
+// FIXME(thaJeztah): remove once we are a module; the go:build directive prevents go from downgrading language version to go1.16:
+//go:build go1.23 && linux
 
 package overlay
 
@@ -18,6 +19,7 @@ import (
 	"github.com/docker/docker/internal/nlwrap"
 	"github.com/docker/docker/libnetwork/driverapi"
 	"github.com/docker/docker/libnetwork/drivers/overlay/overlayutils"
+	"github.com/docker/docker/libnetwork/internal/countmap"
 	"github.com/docker/docker/libnetwork/internal/netiputil"
 	"github.com/docker/docker/libnetwork/netlabel"
 	"github.com/docker/docker/libnetwork/ns"
@@ -53,6 +55,8 @@ type network struct {
 	endpoints endpointTable
 	driver    *driver
 	joinCnt   int
+	// Ref count of VXLAN Forwarding Database entries programmed into the kernel
+	fdbCnt    countmap.Map[ipmac]
 	sboxInit  bool
 	initEpoch int
 	initErr   error
@@ -99,6 +103,7 @@ func (d *driver) CreateNetwork(ctx context.Context, id string, option map[string
 		driver:    d,
 		endpoints: endpointTable{},
 		subnets:   []*subnet{},
+		fdbCnt:    countmap.Map[ipmac]{},
 	}
 
 	vnis := make([]uint32, 0, len(ipV4Data))
@@ -586,6 +591,7 @@ func (n *network) initSandbox() error {
 
 	// this is needed to let the peerAdd configure the sandbox
 	n.sbox = sbox
+	n.fdbCnt = countmap.Map[ipmac]{}
 
 	return nil
 }

libnetwork/drivers/overlay/peerdb.go

@@ -20,9 +20,9 @@ import (
 const ovPeerTable = "overlay_peer_table"
 
 type peerEntry struct {
-	eid        string
-	vtep       netip.Addr // Virtual Tunnel End Point for non-local peers
-	prefixBits int        // number of 1-bits in network mask of peerIP
+	eid  string
+	mac  macAddr
+	vtep netip.Addr
 }
 
 func (p *peerEntry) isLocal() bool {
@@ -30,8 +30,7 @@ func (p *peerEntry) isLocal() bool {
 }
 
 type peerMap struct {
-	// set of peerEntry, note the values have to be objects and not pointers to maintain the proper equality checks
-	mp setmatrix.SetMatrix[ipmac, peerEntry]
+	mp setmatrix.SetMatrix[netip.Prefix, peerEntry]
 	sync.Mutex
 }
 
@@ -41,16 +40,16 @@ type peerNetworkMap struct {
 	sync.Mutex
 }
 
-func (d *driver) peerDbNetworkWalk(nid string, f func(netip.Addr, net.HardwareAddr, *peerEntry) bool) error {
+func (d *driver) peerDbNetworkWalk(nid string, f func(netip.Prefix, peerEntry) bool) {
 	d.peerDb.Lock()
 	pMap, ok := d.peerDb.mp[nid]
 	d.peerDb.Unlock()
 
 	if !ok {
-		return nil
+		return
 	}
 
-	mp := map[ipmac]peerEntry{}
+	mp := map[netip.Prefix]peerEntry{}
 	pMap.Lock()
 	for _, pKey := range pMap.mp.Keys() {
 		entryDBList, ok := pMap.mp.Get(pKey)
@@ -60,38 +59,28 @@ func (d *driver) peerDbNetworkWalk(nid string, f func(netip.Addr, net.HardwareAd
 	}
 	pMap.Unlock()
 
-	for pKey, pEntry := range mp {
-		if f(pKey.ip, pKey.mac.HardwareAddr(), &pEntry) {
-			return nil
+	for k, v := range mp {
+		if f(k, v) {
+			return
 		}
 	}
-
-	return nil
 }
 
-func (d *driver) peerDbSearch(nid string, peerIP netip.Addr) (netip.Addr, net.HardwareAddr, *peerEntry, error) {
-	var peerIPMatched netip.Addr
-	var peerMacMatched net.HardwareAddr
-	var pEntryMatched *peerEntry
-	err := d.peerDbNetworkWalk(nid, func(ip netip.Addr, mac net.HardwareAddr, pEntry *peerEntry) bool {
-		if ip == peerIP {
-			peerIPMatched = ip
-			peerMacMatched = mac
-			pEntryMatched = pEntry
-			return true
-		}
-
-		return false
-	})
-	if err != nil {
-		return netip.Addr{}, nil, nil, fmt.Errorf("peerdb search for peer ip %q failed: %v", peerIP, err)
+func (d *driver) peerDbGet(nid string, peerIP netip.Prefix) (peerEntry, bool) {
+	d.peerDb.Lock()
+	pMap, ok := d.peerDb.mp[nid]
+	d.peerDb.Unlock()
+	if !ok {
+		return peerEntry{}, false
 	}
 
-	if !peerIPMatched.IsValid() || pEntryMatched == nil {
-		return netip.Addr{}, nil, nil, fmt.Errorf("peer ip %q not found in peerdb", peerIP)
+	pMap.Lock()
+	defer pMap.Unlock()
+	c, _ := pMap.mp.Get(peerIP)
+	if len(c) == 0 {
+		return peerEntry{}, false
 	}
-
-	return peerIPMatched, peerMacMatched, pEntryMatched, nil
+	return c[0], true
 }
 
 func (d *driver) peerDbAdd(nid, eid string, peerIP netip.Prefix, peerMac net.HardwareAddr, vtep netip.Addr) (bool, int) {
@@ -103,22 +92,21 @@ func (d *driver) peerDbAdd(nid, eid string, peerIP netip.Prefix, peerMac net.Har
 	}
 	d.peerDb.Unlock()
 
-	pKey := ipmacOf(peerIP.Addr(), peerMac)
-
 	pEntry := peerEntry{
-		eid:        eid,
-		vtep:       vtep,
-		prefixBits: peerIP.Bits(),
+		eid:  eid,
+		mac:  macAddrOf(peerMac),
+		vtep: vtep,
 	}
 
 	pMap.Lock()
 	defer pMap.Unlock()
-	b, i := pMap.mp.Insert(pKey, pEntry)
+	b, i := pMap.mp.Insert(peerIP, pEntry)
 	if i != 1 {
-		// Transient case, there is more than one endpoint that is using the same IP,MAC pair
-		s, _ := pMap.mp.String(pKey)
-		log.G(context.TODO()).Warnf("peerDbAdd transient condition - Key:%s cardinality:%d db state:%s", pKey.String(), i, s)
+		// Transient case, there is more than one endpoint that is using the same IP
+		s, _ := pMap.mp.String(peerIP)
+		log.G(context.TODO()).Warnf("peerDbAdd transient condition - Key:%s cardinality:%d db state:%s", peerIP, i, s)
 	}
+
 	return b, i
 }
 
@@ -131,21 +119,19 @@ func (d *driver) peerDbDelete(nid, eid string, peerIP netip.Prefix, peerMac net.
 	}
 	d.peerDb.Unlock()
 
-	pKey := ipmacOf(peerIP.Addr(), peerMac)
-
 	pEntry := peerEntry{
-		eid:        eid,
-		vtep:       vtep,
-		prefixBits: peerIP.Bits(),
+		eid:  eid,
+		mac:  macAddrOf(peerMac),
+		vtep: vtep,
 	}
 
 	pMap.Lock()
 	defer pMap.Unlock()
-	b, i := pMap.mp.Remove(pKey, pEntry)
+	b, i := pMap.mp.Remove(peerIP, pEntry)
 	if i != 0 {
-		// Transient case, there is more than one endpoint that is using the same IP,MAC pair
-		s, _ := pMap.mp.String(pKey)
-		log.G(context.TODO()).Warnf("peerDbDelete transient condition - Key:%s cardinality:%d db state:%s", pKey, i, s)
+		// Transient case, there is more than one endpoint that is using the same IP
+		s, _ := pMap.mp.String(peerIP)
+		log.G(context.TODO()).Warnf("peerDbDelete transient condition - Key:%s cardinality:%d db state:%s", peerIP, i, s)
 	}
 	return b, i
 }
@@ -164,16 +150,12 @@ func (d *driver) peerDbDelete(nid, eid string, peerIP netip.Prefix, peerMac net.
 func (d *driver) initSandboxPeerDB(nid string) {
 	d.peerOpMu.Lock()
 	defer d.peerOpMu.Unlock()
-	err := d.peerDbNetworkWalk(nid, func(peerIP netip.Addr, peerMac net.HardwareAddr, pEntry *peerEntry) bool {
+	d.peerDbNetworkWalk(nid, func(peerIP netip.Prefix, pEntry peerEntry) bool {
 		if !pEntry.isLocal() {
-			d.addNeighbor(nid, netip.PrefixFrom(peerIP, pEntry.prefixBits), peerMac, pEntry.vtep)
+			d.addNeighbor(nid, peerIP, pEntry.mac.HardwareAddr(), pEntry.vtep)
 		}
 		return false // walk all entries
 	})
-
-	if err != nil {
-		log.G(context.TODO()).WithError(err).Warn("Peer init operation failed")
-	}
 }
 
 // peerAdd adds a new entry to the peer database.
@@ -242,8 +224,10 @@ func (d *driver) addNeighbor(nid string, peerIP netip.Prefix, peerMac net.Hardwa
 	}
 
 	// Add fdb entry to the bridge for the peer mac
-	if err := sbox.AddNeighbor(vtep.AsSlice(), peerMac, osl.WithLinkName(s.vxlanName), osl.WithFamily(syscall.AF_BRIDGE)); err != nil {
-		return fmt.Errorf("could not add fdb entry into the sandbox: %w", err)
+	if n.fdbCnt.Add(ipmacOf(vtep, peerMac), 1) == 1 {
+		if err := sbox.AddNeighbor(vtep.AsSlice(), peerMac, osl.WithLinkName(s.vxlanName), osl.WithFamily(syscall.AF_BRIDGE)); err != nil {
+			return fmt.Errorf("could not add fdb entry into the sandbox: %w", err)
+		}
 	}
 
 	return nil
@@ -287,23 +271,22 @@ func (d *driver) peerDelete(nid, eid string, peerIP netip.Prefix, peerMac net.Ha
 		if dbEntries > 0 {
 			// If there is still an entry into the database and the deletion went through without errors means that there is now no
 			// configuration active in the kernel.
-			// Restore one configuration for the <ip,mac> directly from the database, note that is guaranteed that there is one
-			peerIPAddr, peerMac, peerEntry, err := d.peerDbSearch(nid, peerIP.Addr())
-			if err != nil {
+			// Restore one configuration for the ip directly from the database, note that is guaranteed that there is one
+			peerEntry, ok := d.peerDbGet(nid, peerIP)
+			if !ok {
 				log.G(context.TODO()).WithFields(log.Fields{
 					"nid": nid,
 					"ip":  peerIP,
-				}).WithError(err).Error("peerDelete unable to restore a configuration")
+				}).Error("peerDelete unable to restore a configuration: no entry found in the database")
 				return
 			}
-			peerIP = netip.PrefixFrom(peerIPAddr, peerEntry.prefixBits)
-			err = d.addNeighbor(nid, peerIP, peerMac, peerEntry.vtep)
+			err = d.addNeighbor(nid, peerIP, peerEntry.mac.HardwareAddr(), peerEntry.vtep)
 			if err != nil {
 				log.G(context.TODO()).WithFields(log.Fields{
 					"nid":  nid,
 					"eid":  eid,
 					"ip":   peerIP,
-					"mac":  peerMac,
+					"mac":  peerEntry.mac,
 					"vtep": peerEntry.vtep,
 				}).WithError(err).Error("Peer delete operation failed")
 			}
@@ -335,8 +318,10 @@ func (d *driver) deleteNeighbor(nid string, peerIP netip.Prefix, peerMac net.Har
 		return fmt.Errorf("could not find the subnet %q in network %q", peerIP.String(), n.id)
 	}
 	// Remove fdb entry to the bridge for the peer mac
-	if err := sbox.DeleteNeighbor(vtep.AsSlice(), peerMac, osl.WithLinkName(s.vxlanName), osl.WithFamily(syscall.AF_BRIDGE)); err != nil {
-		return fmt.Errorf("could not delete fdb entry in the sandbox: %w", err)
+	if n.fdbCnt.Add(ipmacOf(vtep, peerMac), -1) == 0 {
+		if err := sbox.DeleteNeighbor(vtep.AsSlice(), peerMac, osl.WithLinkName(s.vxlanName), osl.WithFamily(syscall.AF_BRIDGE)); err != nil {
+			return fmt.Errorf("could not delete fdb entry in the sandbox: %w", err)
+		}
 	}
 
 	// Delete neighbor entry for the peer IP

libnetwork/internal/countmap/countmap.go

@@ -0,0 +1,19 @@
+// FIXME(thaJeztah): remove once we are a module; the go:build directive prevents go from downgrading language version to go1.16:
+//go:build go1.23
+
+package countmap
+
+// Map is a map of counters.
+type Map[T comparable] map[T]int
+
+// Add adds delta to the counter for v and returns the new value.
+//
+// If the new value is 0, the entry is removed from the map.
+func (m Map[T]) Add(v T, delta int) int {
+	m[v] += delta
+	c := m[v]
+	if c == 0 {
+		delete(m, v)
+	}
+	return c
+}

libnetwork/internal/countmap/countmap_test.go

@@ -0,0 +1,27 @@
+package countmap_test
+
+import (
+	"testing"
+
+	"github.com/docker/docker/libnetwork/internal/countmap"
+	"gotest.tools/v3/assert"
+	is "gotest.tools/v3/assert/cmp"
+)
+
+func TestMap(t *testing.T) {
+	m := countmap.Map[string]{}
+	m["foo"] = 7
+	m["bar"] = 2
+	m["zeroed"] = -2
+
+	m.Add("bar", -3)
+	m.Add("foo", -8)
+	m.Add("baz", 1)
+	m.Add("zeroed", 2)
+	assert.Check(t, is.DeepEqual(m, countmap.Map[string]{"foo": -1, "bar": -1, "baz": 1}))
+
+	m.Add("foo", 1)
+	m.Add("bar", 1)
+	m.Add("baz", -1)
+	assert.Check(t, is.DeepEqual(m, countmap.Map[string]{}))
+}