[release-4.18] OCPBUGS-57108: Fix hybrid overlay node subnets collision with cluster subnets

go-controller/pkg/clustermanager/node/node_allocator.go

@@ -47,6 +47,9 @@ type NodeAllocator struct {
 	networkID int
 
 	netInfo util.NetInfo
+
+	// nodeSubnets is a list of node subnets that are managed by the cluster subnet allocator
+	nodeSubnets []*net.IPNet
 }
 
 func NewNodeAllocator(networkID int, netInfo util.NetInfo, nodeLister listers.NodeLister, kube kube.Interface, tunnelIDAllocator id.Allocator) *NodeAllocator {
@@ -123,6 +126,10 @@ func (na *NodeAllocator) hasHybridOverlayAllocation() bool {
 	return config.HybridOverlay.Enabled && !na.netInfo.IsSecondary() && len(config.HybridOverlay.ClusterSubnets) > 0
 }
 
+func (na *NodeAllocator) hasHybridOverlayAllocationUnmanaged() bool {
+	return config.HybridOverlay.Enabled && !na.netInfo.IsSecondary() && len(config.HybridOverlay.ClusterSubnets) == 0
+}
+
 func (na *NodeAllocator) recordSubnetCount() {
 	// only for L3 networks
 	if na.hasNodeSubnetAllocation() {
@@ -230,6 +237,12 @@ func (na *NodeAllocator) HandleAddUpdateNodeEvent(node *corev1.Node) error {
 				}
 				return fmt.Errorf("failed to set hybrid overlay annotations for node %s: %v", node.Name, err)
 			}
+		} else if na.hasHybridOverlayAllocationUnmanaged() {
+			// this is a hybrid overlay node but not managed by the hybrid overlay subnet allocator
+			// Hybrid overlay is only available for IPv4 for now, so we only need to check for IPv4 subnets
+			if err := na.markAllocatedNetworksForUnmanagedHONode(node); err != nil {
+				return fmt.Errorf("failed to mark the subnet %v as allocated in the cluster subnet allocator for node %s: %v", na.nodeSubnets, node.Name, err)
+			}
 		}
 		return nil
 	}
@@ -380,7 +393,7 @@ func (na *NodeAllocator) HandleDeleteNode(node *corev1.Node) error {
 		return nil
 	}
 
-	if na.hasNodeSubnetAllocation() {
+	if na.hasNodeSubnetAllocation() || na.hasHybridOverlayAllocationUnmanaged() {
 		na.clusterSubnetAllocator.ReleaseAllNetworks(node.Name)
 		na.recordSubnetUsage()
 	}
@@ -411,10 +424,17 @@ func (na *NodeAllocator) Sync(nodes []interface{}) error {
 					klog.Errorf("Failed to parse hybrid overlay for node %s: %v", node.Name, err)
 				} else if hostSubnet != nil {
 					klog.V(5).Infof("Node %s contains subnets: %v", node.Name, hostSubnet)
-					if err := na.hybridOverlaySubnetAllocator.ReleaseNetworks(node.Name, hostSubnet); err != nil {
+					if err := na.hybridOverlaySubnetAllocator.MarkAllocatedNetworks(node.Name, hostSubnet); err != nil {
 						klog.Errorf("Failed to mark the subnet %v as allocated in the hybrid subnet allocator for node %s: %v", hostSubnet, node.Name, err)
 					}
 				}
+			} else if na.hasHybridOverlayAllocationUnmanaged() {
+				// this is a hybrid overlay node but not managed by the hybrid overlay subnet allocator
+				// Hybrid overlay is only available for IPv4 for now, so we only need to check for IPv4 subnets
+				if err := na.markAllocatedNetworksForUnmanagedHONode(node); err != nil {
+					klog.Errorf("Failed to mark the subnet as allocated in the cluster subnet allocator for hybrid overlay node %s: %v", node.Name, err)
+				}
+
 			}
 		} else {
 			hostSubnets, _ := util.ParseNodeHostSubnetAnnotation(node, networkName)
@@ -634,3 +654,37 @@ func (na *NodeAllocator) hasJoinSubnetAllocation() bool {
 	// we allocate join subnets for L3/L2 primary user defined networks or default network
 	return na.netInfo.IsDefault() || (util.IsNetworkSegmentationSupportEnabled() && na.netInfo.IsPrimaryNetwork())
 }
+
+func (na *NodeAllocator) markAllocatedNetworksForUnmanagedHONode(node *corev1.Node) error {
+	hostSubnet, err := houtil.ParseHybridOverlayHostSubnet(node)
+	if err != nil {
+		return fmt.Errorf("failed to parse hybrid overlay for node %s: %v", node.Name, err)
+	}
+
+	var overlaps []*net.IPNet
+	for _, clusterSubnet := range na.netInfo.Subnets() {
+		if overlaps = util.IPNetOverlaps(hostSubnet, clusterSubnet.CIDR); len(overlaps) != 0 {
+			break
+		}
+	}
+	if len(overlaps) == 0 {
+		// if the host subnet does not overlap with any cluster subnet, return
+		return nil
+	}
+
+	if hostSubnet != nil {
+		if na.nodeSubnets == nil {
+			// initialize the nodeSubnets variable at the first called
+			na.nodeSubnets = na.clusterSubnetAllocator.ListAllIPv4Networks()
+		}
+		// check if the host subnet overlaps with any node subnet that is managed by the cluster subnet allocator
+		// if it does, mark it as allocated in the cluster subnet allocator
+		if overlaps := util.IPNetOverlaps(hostSubnet, na.nodeSubnets...); overlaps != nil {
+			klog.Infof("Hybrid overlay node %s overlaps with subnets: %v", node.Name, overlaps)
+			if err := na.clusterSubnetAllocator.MarkAllocatedNetworks(node.Name, overlaps...); err != nil {
+				return fmt.Errorf("failed to mark the subnet %v as allocated: %v", hostSubnet, err)
+			}
+		}
+	}
+	return nil
+}

go-controller/pkg/clustermanager/node/node_allocator_test.go

@@ -8,6 +8,11 @@ import (
 
 	cnitypes "github.com/containernetworking/cni/pkg/types"
 
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/listers/core/v1"
+	"k8s.io/client-go/tools/cache"
+
 	ovncnitypes "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/cni/types"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config"
 	ovntest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing"
@@ -45,9 +50,10 @@ func TestController_allocateNodeSubnets(t *testing.T) {
 		existingNets  []*net.IPNet
 		alreadyOwned  *existingAllocation
 		// to be converted during the test to []*net.IPNet
-		wantStr   []string
-		allocated int
-		wantErr   bool
+		wantStr       []string
+		allocated     int
+		wantErr       bool
+		existingNodes []*corev1.Node
 	}{
 		{
 			name:          "new node, IPv4 only cluster",
@@ -60,6 +66,56 @@ func TestController_allocateNodeSubnets(t *testing.T) {
 			allocated:     1,
 			wantErr:       false,
 		},
+		{
+			name:          "new node, IPv4 only cluster, the test node is added when a hybrid overlay node with overlapped node subnet exists",
+			networkRanges: []string{"172.16.0.0/16"},
+			networkLens:   []int{24},
+			configIPv4:    true,
+			configIPv6:    false,
+			existingNets:  nil,
+			wantStr:       []string{"172.16.1.0/24"},
+			allocated:     1,
+			wantErr:       false,
+			existingNodes: []*corev1.Node{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "ho_node1",
+						Annotations: map[string]string{
+							"k8s.ovn.org/hybrid-overlay-node-subnet": "172.16.0.0/24",
+						},
+						Labels: map[string]string{
+							"hybrid-overlay-node": "true",
+						},
+					},
+					Spec: corev1.NodeSpec{},
+				},
+			},
+		},
+		{
+			name:          "new node, IPv4 only cluster, the test node is added when a hybrid overlay node with overlapped and differernt mask length node subnet exists",
+			networkRanges: []string{"172.16.0.0/16"},
+			networkLens:   []int{24},
+			configIPv4:    true,
+			configIPv6:    false,
+			existingNets:  nil,
+			wantStr:       []string{"172.16.2.0/24"},
+			allocated:     1,
+			wantErr:       false,
+			existingNodes: []*corev1.Node{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "ho_node1",
+						Annotations: map[string]string{
+							"k8s.ovn.org/hybrid-overlay-node-subnet": "172.16.0.0/23",
+						},
+						Labels: map[string]string{
+							"hybrid-overlay-node": "true",
+						},
+					},
+					Spec: corev1.NodeSpec{},
+				},
+			},
+		},
 		{
 			name:          "new node, IPv6 only cluster",
 			networkRanges: []string{"2001:db2::/56"},
@@ -208,6 +264,12 @@ func TestController_allocateNodeSubnets(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			config.HybridOverlay.Enabled = true
+			config.Kubernetes.NoHostSubnetNodes, _ = metav1.LabelSelectorAsSelector(&metav1.LabelSelector{
+				MatchLabels: map[string]string{"hybrid-overlay-node": "true"},
+			})
+			config.HybridOverlay.ClusterSubnets = nil
+
 			ranges, err := rangesFromStrings(tt.networkRanges, tt.networkLens)
 			if err != nil {
 				t.Fatal(err)
@@ -231,6 +293,15 @@ func TestController_allocateNodeSubnets(t *testing.T) {
 			if err := na.Init(); err != nil {
 				t.Fatalf("Failed to initialize node allocator: %v", err)
 			}
+			nodeInterfaces := make([]interface{}, len(tt.existingNodes))
+			for i, node := range tt.existingNodes {
+				nodeInterfaces[i] = node
+			}
+			// Sync existing nodes before allocating subnets
+			err = na.Sync(nodeInterfaces)
+			if err != nil {
+				t.Fatal(err)
+			}
 
 			if tt.alreadyOwned != nil {
 				err := na.clusterSubnetAllocator.MarkAllocatedNetworks(tt.alreadyOwned.owner, ovntest.MustParseIPNets(tt.alreadyOwned.subnet)...)
@@ -291,6 +362,7 @@ func TestController_allocateNodeSubnets_ReleaseOnError(t *testing.T) {
 	na := &NodeAllocator{
 		netInfo:                netInfo,
 		clusterSubnetAllocator: NewSubnetAllocator(),
+		nodeLister:             newFakeNodeLister([]*corev1.Node{}),
 	}
 
 	if err := na.Init(); err != nil {
@@ -323,3 +395,11 @@ func TestController_allocateNodeSubnets_ReleaseOnError(t *testing.T) {
 		t.Fatalf("Expected %d v6 allocated subnets, but got %d", v6usedBefore, v6usedAfter)
 	}
 }
+
+func newFakeNodeLister(nodes []*corev1.Node) v1.NodeLister {
+	indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
+	for _, node := range nodes {
+		_ = indexer.Add(node)
+	}
+	return v1.NewNodeLister(indexer)
+}

go-controller/pkg/clustermanager/node/subnet_allocator.go

@@ -23,6 +23,8 @@ type SubnetAllocator interface {
 	AllocateNetworks(string) ([]*net.IPNet, error)
 	AllocateIPv4Network(string) (*net.IPNet, error)
 	AllocateIPv6Network(string) (*net.IPNet, error)
+	ListAllIPv4Networks() []*net.IPNet
+	ListAllIPv6Networks() []*net.IPNet
 	// ReleaseNetworks releases the given networks if they are owned by the
 	// given owner
 	ReleaseNetworks(string, ...*net.IPNet) error
@@ -192,6 +194,22 @@ func (sna *BaseSubnetAllocator) AllocateIPv6Network(owner string) (*net.IPNet, e
 	return nil, ErrSubnetAllocatorFull
 }
 
+func (sna *BaseSubnetAllocator) ListAllIPv4Networks() []*net.IPNet {
+	var subnets []*net.IPNet
+	for _, snr := range sna.v4ranges {
+		subnets = append(subnets, snr.listAllNetworks()...)
+	}
+	return subnets
+}
+
+func (sna *BaseSubnetAllocator) ListAllIPv6Networks() []*net.IPNet {
+	var subnets []*net.IPNet
+	for _, snr := range sna.v6ranges {
+		subnets = append(subnets, snr.listAllNetworks()...)
+	}
+	return subnets
+}
+
 func (sna *BaseSubnetAllocator) ReleaseNetworks(owner string, subnets ...*net.IPNet) error {
 	sna.Lock()
 	defer sna.Unlock()
@@ -379,48 +397,20 @@ func (snr *subnetAllocatorRange) allocateNetwork(owner string) *net.IPNet {
 			return subnet
 		}
 	}
-	netMaskSize, addrLen := snr.network.Mask.Size()
-	numSubnets := uint32(1) << snr.subnetBits
-	if snr.subnetBits > 24 {
-		// We need to make sure that the uint32 math below won't overflow. If
-		// snr.subnetBits > 32 then numSubnets has already overflowed, but also if
-		// numSubnets is between 1<<24 and 1<<32 then "base << (snr.hostBits % 8)"
-		// below could overflow if snr.hostBits%8 is non-0. So we cap numSubnets
-		// at 1<<24. "16M subnets ought to be enough for anybody."
-		numSubnets = 1 << 24
-	}
-
-	var i uint32
-	for i = 0; i < numSubnets; i++ {
-		n := (i + snr.next) % numSubnets
-		base := n
-		if snr.leftShift != 0 {
-			base = ((base << snr.leftShift) & snr.leftMask) | ((base >> snr.rightShift) & snr.rightMask)
-		} else if addrLen == 128 && snr.subnetBits >= 16 {
-			// Skip the 0 subnet (and other subnets with all 0s in the low word)
-			// since the extra 0 word will get compressed out and make the address
-			// look different from addresses on other subnets.
-			if (base & 0xFFFF) == 0 {
-				continue
-			}
-		}
 
-		genIP := append([]byte{}, []byte(snr.network.IP)...)
-		subnetBits := base << (snr.hostBits % 8)
-		b := (uint32(addrLen) - snr.hostBits - 1) / 8
-		for subnetBits != 0 {
-			genIP[b] |= byte(subnetBits)
-			subnetBits >>= 8
-			b--
-		}
-
-		genSubnet := &net.IPNet{IP: genIP, Mask: net.CIDRMask(int(snr.subnetBits)+netMaskSize, addrLen)}
+	var subnet *net.IPNet
+	snr.foreach(snr.next, func(n uint32, genSubnet *net.IPNet) bool {
 		if _, ok := snr.allocMap[genSubnet.String()]; !ok {
 			snr.allocMap[genSubnet.String()] = owner
 			snr.next = n + 1
 			snr.used++
-			return genSubnet
+			subnet = genSubnet
+			return false
 		}
+		return true
+	})
+	if subnet != nil {
+		return subnet
 	}
 
 	snr.next = 0
@@ -456,3 +446,65 @@ func (snr *subnetAllocatorRange) releaseAllNetworks(owner string) {
 		}
 	}
 }
+
+// generateSubnet generates a subnet for the given base number using the allocator's parameters
+func (snr *subnetAllocatorRange) generateSubnet(base uint32) *net.IPNet {
+	netMaskSize, addrLen := snr.network.Mask.Size()
+
+	if snr.leftShift != 0 {
+		base = ((base << snr.leftShift) & snr.leftMask) | ((base >> snr.rightShift) & snr.rightMask)
+	} else if addrLen == 128 && snr.subnetBits >= 16 {
+		// Skip the 0 subnet (and other subnets with all 0s in the low word)
+		// since the extra 0 word will get compressed out and make the address
+		// look different from addresses on other subnets.
+		if (base & 0xFFFF) == 0 {
+			return nil
+		}
+	}
+
+	genIP := append([]byte{}, []byte(snr.network.IP)...)
+	subnetBits := base << (snr.hostBits % 8)
+	b := (uint32(addrLen) - snr.hostBits - 1) / 8
+	for subnetBits != 0 {
+		genIP[b] |= byte(subnetBits)
+		subnetBits >>= 8
+		b--
+	}
+
+	return &net.IPNet{
+		IP:   genIP,
+		Mask: net.CIDRMask(int(snr.subnetBits)+netMaskSize, addrLen),
+	}
+}
+
+func (snr *subnetAllocatorRange) foreach(next uint32, do func(uint32, *net.IPNet) bool) {
+	numSubnets := uint32(1) << snr.subnetBits
+	if snr.subnetBits > 24 {
+		// We need to make sure that the uint32 math below won't overflow. If
+		// snr.subnetBits > 32 then numSubnets has already overflowed, but also if
+		// numSubnets is between 1<<24 and 1<<32 then "base << (snr.hostBits % 8)"
+		// below could overflow if snr.hostBits%8 is non-0. So we cap numSubnets
+		// at 1<<24. "16M subnets ought to be enough for anybody."
+		numSubnets = 1 << 24
+	}
+
+	var i uint32
+	for i = 0; i < numSubnets; i++ {
+		n := (i + next) % numSubnets
+		base := n
+		genSubnet := snr.generateSubnet(base)
+		if genSubnet != nil && !do(n, genSubnet) {
+			return
+		}
+	}
+}
+
+// listAllNetworks returns all networks in the range
+func (snr *subnetAllocatorRange) listAllNetworks() []*net.IPNet {
+	var subnets []*net.IPNet
+	snr.foreach(0, func(_ uint32, genSubnet *net.IPNet) bool {
+		subnets = append(subnets, genSubnet)
+		return true
+	})
+	return subnets
+}