Potential data race with localNode in NodeDiscovery

[gandro]

As pointed out by Louis, there is a potential data race with the NodeDiscovery module. It protects local node updates via the following mutex:

cilium/pkg/nodediscovery/nodediscovery.go

Lines 650 to 658 in 47e936e

	n.localNodeLock.Lock()
	defer n.localNodeLock.Unlock()
	if option.Config.EnableIPv4 && len(ipv4AllocCIDRs) > 0 {
	ipv4PrimaryCIDR, ipv4SecondaryCIDRs := splitAllocCIDRs(ipv4AllocCIDRs)
	validatePrimaryCIDR(n.localNode.IPv4AllocCIDR, ipv4PrimaryCIDR, ipam.IPv4)
	n.localNode.IPv4AllocCIDR = ipv4PrimaryCIDR
	n.localNode.IPv4SecondaryAllocCIDRs = ipv4SecondaryCIDRs
	}

But the getters of the localNode object are not aware of the lock, and therefore could concurrently read the fields while they are getting updated.

cilium/pkg/node/types/node.go

Lines 439 to 448 in b4b7500

	func (n *Node) GetIPv4AllocCIDRs() []*cidr.CIDR {
	result := make([]*cidr.CIDR, 0, len(n.IPv4SecondaryAllocCIDRs)+1)
	if n.IPv4AllocCIDR != nil {
	result = append(result, n.IPv4AllocCIDR)
	}
	if len(n.IPv4SecondaryAllocCIDRs) > 0 {
	result = append(result, n.IPv4SecondaryAllocCIDRs...)
	}
	return result
	}

To my knowledge, the n.localNode reference is not stored by any consumer at the moment, so I don't think the race can happen in practise at the moment. This is because the lock is held during the call to n.Manager.NodeUpdated(n.localNode) where all the data access happens via getters happens.

But regardless, we should still fix this, for example by creating a copy of the local node object, to ensure that any future consumers will not introduce a data race by accident.

[gandro]

LocalAllocCIDRsUpdated has been removed from the tree. This particular data race here is no longer present. #21085 looks like it remains though.