From 1c2b744ca249fac01383f1b7abbf267d70df3e39 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Wed, 21 May 2025 14:48:41 -0400 Subject: [PATCH] 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 --- libnetwork/drivers/overlay/ov_network.go | 8 +- libnetwork/drivers/overlay/peerdb.go | 121 ++++++++---------- libnetwork/internal/countmap/countmap.go | 19 +++ libnetwork/internal/countmap/countmap_test.go | 27 ++++ 4 files changed, 106 insertions(+), 69 deletions(-) create mode 100644 libnetwork/internal/countmap/countmap.go create mode 100644 libnetwork/internal/countmap/countmap_test.go diff --git a/libnetwork/drivers/overlay/ov_network.go b/libnetwork/drivers/overlay/ov_network.go index 5b533f481e..929713c330 100644 --- a/libnetwork/drivers/overlay/ov_network.go +++ b/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 } diff --git a/libnetwork/drivers/overlay/peerdb.go b/libnetwork/drivers/overlay/peerdb.go index 5c103ad8e5..d655b0c4d7 100644 --- a/libnetwork/drivers/overlay/peerdb.go +++ b/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 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 diff --git a/libnetwork/internal/countmap/countmap.go b/libnetwork/internal/countmap/countmap.go new file mode 100644 index 0000000000..a028ed9684 --- /dev/null +++ b/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 +} diff --git a/libnetwork/internal/countmap/countmap_test.go b/libnetwork/internal/countmap/countmap_test.go new file mode 100644 index 0000000000..ab33a3b0c7 --- /dev/null +++ b/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]{})) +}