Fix capacity calculation

The previous capacity calculation incorrectly only counted CloudPrivateIPConfigs
that had the status set. This is racy and can lead to artificially reduced capacity.
Fix it by accounting for all CloudPrivateIPConfigs that are or were assigned to
the matching node.

Signed-off-by: Patryk Diak <[email protected]>

Author: kyrtapz

pkg/cloudprovider/aws.go

@@ -14,8 +14,6 @@ import (
 	"github.com/aws/aws-sdk-go/aws/endpoints"
 	"github.com/aws/aws-sdk-go/aws/session"
 	"github.com/aws/aws-sdk-go/service/ec2"
-	v1 "github.com/openshift/api/cloudnetwork/v1"
-	"github.com/openshift/cloud-network-config-controller/pkg/cloudprivateipconfig"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -181,7 +179,7 @@ func (a *AWS) ReleasePrivateIP(ip net.IP, node *corev1.Node) error {
 	}
 }
 
-func (a *AWS) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPConfigs []*v1.CloudPrivateIPConfig) ([]*NodeEgressIPConfiguration, error) {
+func (a *AWS) GetNodeEgressIPConfiguration(node *corev1.Node, cpicIPs sets.Set[string]) ([]*NodeEgressIPConfiguration, error) {
 	instance, err := a.getInstance(node)
 	if err != nil {
 		return nil, err
@@ -214,10 +212,8 @@ func (a *AWS) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPConf
 	if v6Subnet != nil {
 		config.IFAddr.IPv6 = v6Subnet.String()
 	}
-	capV4, capV6, err := a.getCapacity(instanceV4Capacity, instanceV6Capacity, networkInterface, cloudPrivateIPConfigs)
-	if err != nil {
-		return nil, err
-	}
+
+	capV4, capV6 := a.getCapacity(instanceV4Capacity, instanceV6Capacity, networkInterface, cpicIPs)
 	config.Capacity = capacity{
 		IPv4: ptr.To(capV4),
 		IPv6: ptr.To(capV6),
@@ -304,32 +300,28 @@ func (a *AWS) getSubnet(networkInterface *ec2.InstanceNetworkInterface) (*net.IP
 
 // AWS uses a variable capacity per instance type, see:
 // https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/using-eni.html#AvailableIpPerENI
-// Hence we need to retrieve that and then subtract the amount already assigned
-// by default.
-func (a *AWS) getCapacity(instanceV4Capacity, instanceV6Capacity int, networkInterface *ec2.InstanceNetworkInterface, cloudPrivateIPConfigs []*v1.CloudPrivateIPConfig) (int, int, error) {
+// Capacity represents the number of IPs available for consumption.
+// We calculate this as: instance_limit - IPs_consumed_by_non_CPIC_sources
+// This way, CNCC managed IPs (regardless of their status) don't reduce capacity.
+func (a *AWS) getCapacity(instanceV4Capacity, instanceV6Capacity int, networkInterface *ec2.InstanceNetworkInterface, cpicIPs sets.Set[string]) (int, int) {
 	currentIPv4Usage, currentIPv6Usage := 0, 0
 	for _, assignedIPv6 := range networkInterface.Ipv6Addresses {
 		if assignedIP := net.ParseIP(*assignedIPv6.Ipv6Address); assignedIP != nil {
-			currentIPv6Usage++
+			if !cpicIPs.Has(assignedIP.String()) {
+				currentIPv6Usage++
+			}
 		}
 	}
+
 	for _, assignedIPv4 := range networkInterface.PrivateIpAddresses {
 		if assignedIP := net.ParseIP(*assignedIPv4.PrivateIpAddress); assignedIP != nil {
-			currentIPv4Usage++
-		}
-	}
-	for _, cloudPrivateIPConfig := range cloudPrivateIPConfigs {
-		_, ipFamily, err := cloudprivateipconfig.NameToIP(cloudPrivateIPConfig.Name)
-		if err != nil {
-			return 0, 0, err
-		}
-		if ipFamily == cloudprivateipconfig.IPv4 {
-			instanceV4Capacity++
-		} else if ipFamily == cloudprivateipconfig.IPv6 {
-			instanceV6Capacity++
+			if !cpicIPs.Has(assignedIP.String()) {
+				currentIPv4Usage++
+			}
 		}
 	}
-	return instanceV4Capacity - currentIPv4Usage, instanceV6Capacity - currentIPv6Usage, nil
+
+	return instanceV4Capacity - currentIPv4Usage, instanceV6Capacity - currentIPv6Usage
 }
 
 func (a *AWS) getNetworkInterfaces(instance *ec2.Instance) ([]*ec2.InstanceNetworkInterface, error) {

pkg/cloudprovider/azure.go

@@ -21,9 +21,9 @@ import (
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v6"
 	azureapi "github.com/Azure/go-autorest/autorest/azure"
 	"github.com/Azure/msi-dataplane/pkg/dataplane"
-	v1 "github.com/openshift/api/cloudnetwork/v1"
 	configv1 "github.com/openshift/api/config/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/klog/v2"
 	utilnet "k8s.io/utils/net"
 )
@@ -322,7 +322,7 @@ func (a *Azure) ReleasePrivateIP(ip net.IP, node *corev1.Node) error {
 	return a.waitForCompletion(poller)
 }
 
-func (a *Azure) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPConfigs []*v1.CloudPrivateIPConfig) ([]*NodeEgressIPConfiguration, error) {
+func (a *Azure) GetNodeEgressIPConfiguration(node *corev1.Node, cpicIPs sets.Set[string]) ([]*NodeEgressIPConfiguration, error) {
 	instance, err := a.getInstance(node)
 	if err != nil {
 		return nil, err
@@ -352,7 +352,7 @@ func (a *Azure) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPCo
 	}
 	config.Capacity = capacity{
 		// IPv4 and IPv6 fields not used by Azure (uses IP-family-agnostic capacity)
-		IP: ptr.To(a.getCapacity(networkInterface, len(cloudPrivateIPConfigs))),
+		IP: ptr.To(a.getCapacity(networkInterface, cpicIPs)),
 	}
 	return []*NodeEgressIPConfiguration{config}, nil
 }
@@ -408,18 +408,17 @@ func (a *Azure) getSubnet(networkInterface armnetwork.Interface) (*net.IPNet, *n
 // We need to retrieve the amounts assigned to the node by default and subtract
 // that from the default 256 value. Note: there is also a "Private IP addresses
 // per virtual network" quota, but that's 65.536, so we can skip that.
-func (a *Azure) getCapacity(networkInterface armnetwork.Interface, cloudPrivateIPsCount int) int {
-	currentIPv4Usage, currentIPv6Usage := 0, 0
+func (a *Azure) getCapacity(networkInterface armnetwork.Interface, cpicIPs sets.Set[string]) int {
+	currentIPUsage := 0
 	for _, ipConfiguration := range networkInterface.Properties.IPConfigurations {
 		if assignedIP := net.ParseIP(ptr.Deref(ipConfiguration.Properties.PrivateIPAddress, "")); assignedIP != nil {
-			if utilnet.IsIPv4(assignedIP) {
-				currentIPv4Usage++
-			} else {
-				currentIPv6Usage++
+			if !cpicIPs.Has(assignedIP.String()) {
+				currentIPUsage++
 			}
 		}
 	}
-	return defaultAzurePrivateIPCapacity + cloudPrivateIPsCount - currentIPv4Usage - currentIPv6Usage
+
+	return defaultAzurePrivateIPCapacity - currentIPUsage
 }
 
 // This is what the node's providerID looks like on Azure

pkg/cloudprovider/cloudprovider.go

@@ -13,8 +13,8 @@ import (
 	apifeatures "github.com/openshift/api/features"
 	"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
 
-	v1 "github.com/openshift/api/cloudnetwork/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 var (
@@ -60,7 +60,9 @@ type CloudProviderIntf interface {
 	// for all instance types and IP families (GCP, Azure) or variable per
 	// instance and IP family (AWS), also: the interface is either keyed by name
 	// (GCP) or ID (Azure, AWS).
-	GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPConfigs []*v1.CloudPrivateIPConfig) ([]*NodeEgressIPConfiguration, error)
+	// The cpicIPs parameter is a set of IP addresses that are
+	// managed by CloudPrivateIPConfigs and should be excluded from capacity calculations.
+	GetNodeEgressIPConfiguration(node *corev1.Node, cpicIPs sets.Set[string]) ([]*NodeEgressIPConfiguration, error)
 }
 
 // CloudProviderWithMoveIntf is additional interface that can be added to cloud

pkg/cloudprovider/cloudprovider_fake.go

@@ -5,8 +5,8 @@ import (
 	"net"
 	"time"
 
-	v1 "github.com/openshift/api/cloudnetwork/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 type FakeCloudProvider struct {
@@ -63,7 +63,7 @@ func (f *FakeCloudProvider) waitForCompletion() error {
 	return nil
 }
 
-func (f *FakeCloudProvider) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPConfigs []*v1.CloudPrivateIPConfig) ([]*NodeEgressIPConfiguration, error) {
+func (f *FakeCloudProvider) GetNodeEgressIPConfiguration(node *corev1.Node, cpicIPs sets.Set[string]) ([]*NodeEgressIPConfiguration, error) {
 	if f.mockErrorOnGetNodeEgressIPConfiguration {
 		return nil, fmt.Errorf("Get node egress IP configuration failed")
 	}

pkg/cloudprovider/gcp.go

@@ -7,11 +7,10 @@ import (
 	"net/url"
 	"strings"
 
-	v1 "github.com/openshift/api/cloudnetwork/v1"
 	google "google.golang.org/api/compute/v1"
 	"google.golang.org/api/option"
 	corev1 "k8s.io/api/core/v1"
-	utilnet "k8s.io/utils/net"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/utils/ptr"
 )
 
@@ -155,7 +154,7 @@ func (g *GCP) ReleasePrivateIP(ip net.IP, node *corev1.Node) error {
 	return g.waitForCompletion(project, zone, operation.Name)
 }
 
-func (g *GCP) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPConfigs []*v1.CloudPrivateIPConfig) ([]*NodeEgressIPConfiguration, error) {
+func (g *GCP) GetNodeEgressIPConfiguration(node *corev1.Node, cpicIPs sets.Set[string]) ([]*NodeEgressIPConfiguration, error) {
 	_, _, instance, err := g.getInstance(node)
 	if err != nil {
 		return nil, fmt.Errorf("error retrieving instance associated with node, err: %v", err)
@@ -184,7 +183,7 @@ func (g *GCP) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPConf
 		}
 		config.Capacity = capacity{
 			// IPv4 and IPv6 fields not used by GCP (uses IP-family-agnostic capacity)
-			IP: ptr.To(g.getCapacity(networkInterface, len(cloudPrivateIPConfigs))),
+			IP: ptr.To(g.getCapacity(networkInterface, cpicIPs)),
 		}
 		return []*NodeEgressIPConfiguration{config}, nil //nolint:staticcheck
 	}
@@ -235,25 +234,22 @@ func (g *GCP) getSubnet(networkInterface *google.NetworkInterface) (*net.IPNet,
 
 // Note: there is also a global "alias IP per VPC quota", but OpenShift clusters on
 // GCP seem to have that value defined to 15,000. So we can skip that.
-func (g *GCP) getCapacity(networkInterface *google.NetworkInterface, cloudPrivateIPsCount int) int {
-	currentIPv4Usage := 0
-	currentIPv6Usage := 0
+func (g *GCP) getCapacity(networkInterface *google.NetworkInterface, cpicIPs sets.Set[string]) int {
+	currentIPUsage := 0
 	for _, aliasIPRange := range networkInterface.AliasIpRanges {
+		var aliasIP net.IP
 		if assignedIP := net.ParseIP(aliasIPRange.IpCidrRange); assignedIP != nil {
-			if utilnet.IsIPv4(assignedIP) {
-				currentIPv4Usage++
-			} else {
-				currentIPv6Usage++
-			}
+			aliasIP = assignedIP
 		} else if _, assignedSubnet, err := net.ParseCIDR(aliasIPRange.IpCidrRange); err == nil {
-			if utilnet.IsIPv4CIDR(assignedSubnet) {
-				currentIPv4Usage++
-			} else {
-				currentIPv6Usage++
-			}
+			aliasIP = assignedSubnet.IP
+		}
+
+		if aliasIP != nil && !cpicIPs.Has(aliasIP.String()) {
+			currentIPUsage++
 		}
 	}
-	return defaultGCPPrivateIPCapacity + cloudPrivateIPsCount - currentIPv4Usage - currentIPv6Usage
+
+	return defaultGCPPrivateIPCapacity - currentIPUsage
 }
 
 // getInstance retrieves the GCP instance referred by the Node object.

pkg/cloudprovider/openstack.go

@@ -22,12 +22,11 @@ import (
 	neutronsubnets "github.com/gophercloud/gophercloud/v2/openstack/networking/v2/subnets"
 	"github.com/gophercloud/gophercloud/v2/pagination"
 	"github.com/gophercloud/utils/v2/openstack/clientconfig"
-	v1 "github.com/openshift/api/cloudnetwork/v1"
-	"github.com/openshift/cloud-network-config-controller/pkg/cloudprivateipconfig"
 	"gopkg.in/yaml.v2"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/util/retry"
 	"k8s.io/klog/v2"
 	utilnet "k8s.io/utils/net"
@@ -473,7 +472,7 @@ func (o *OpenStack) ReleasePrivateIP(ip net.IP, node *corev1.Node) error {
 // GetNodeEgressIPConfiguration retrieves the egress IP configuration for
 // the node, following the convention the cloud uses. This means
 // specifically for OpenStack that the interface is keyed by the port's neutron UUID.
-func (o *OpenStack) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPConfigs []*v1.CloudPrivateIPConfig) ([]*NodeEgressIPConfiguration, error) {
+func (o *OpenStack) GetNodeEgressIPConfiguration(node *corev1.Node, cpicIPs sets.Set[string]) ([]*NodeEgressIPConfiguration, error) {
 	if node == nil {
 		return nil, fmt.Errorf("invalid nil pointer provided for node when trying to get node EgressIP configuration")
 	}
@@ -492,7 +491,7 @@ func (o *OpenStack) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivate
 	var configurations []*NodeEgressIPConfiguration
 	for _, p := range serverPorts {
 		// Retrieve configuration for this port.
-		config, err := o.getNeutronPortNodeEgressIPConfiguration(p, cloudPrivateIPConfigs)
+		config, err := o.getNeutronPortNodeEgressIPConfiguration(p, cpicIPs)
 		if err != nil {
 			return nil, err
 		}
@@ -553,7 +552,7 @@ func (o *OpenStack) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivate
 // neutron operates as there is no such thing as a per port quota or limit. Therefore we set a ceiling of
 // `openstackMaxCapacity`. The number of unique IP addresses in allowed_address_pair and fixed_ips is subtracted from
 // that ceiling.
-func (o *OpenStack) getNeutronPortNodeEgressIPConfiguration(p neutronports.Port, cloudPrivateIPConfigs []*v1.CloudPrivateIPConfig) (*NodeEgressIPConfiguration, error) {
+func (o *OpenStack) getNeutronPortNodeEgressIPConfiguration(p neutronports.Port, cpicIPs sets.Set[string]) (*NodeEgressIPConfiguration, error) {
 	var ipv4, ipv6 string
 	var err error
 	var ip net.IP
@@ -590,12 +589,9 @@ func (o *OpenStack) getNeutronPortNodeEgressIPConfiguration(p neutronports.Port,
 		}
 		// Loop over all cloudPrivateIPConfigs and check if they are part of this ipnet.
 		// If the IP is contained in the ipnet, increase cloudPrivateIPsCount.
-		for _, cpic := range cloudPrivateIPConfigs {
-			cip, _, err := cloudprivateipconfig.NameToIP(cpic.Name)
-			if err != nil {
-				return nil, err
-			}
-			if ipnet.Contains(cip) {
+		for ipStr := range cpicIPs {
+			cip := net.ParseIP(ipStr)
+			if cip != nil && ipnet.Contains(cip) {
 				cloudPrivateIPsCount++
 			}
 		}

pkg/cloudprovider/openstack_test.go

@@ -18,7 +18,6 @@ import (
 	neutronsubnets "github.com/gophercloud/gophercloud/v2/openstack/networking/v2/subnets"
 	th "github.com/gophercloud/gophercloud/v2/testhelper"
 	testclient "github.com/gophercloud/gophercloud/v2/testhelper/client"
-	v1 "github.com/openshift/api/cloudnetwork/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -1017,7 +1016,7 @@ func TestGetNeutronPortNodeEgressIPConfiguration(t *testing.T) {
 	tcs := []struct {
 		port                  neutronports.Port
 		nodeEgressIPConfig    NodeEgressIPConfiguration
-		cloudPrivateIPConfigs []*v1.CloudPrivateIPConfig
+		cloudPrivateIPConfigs []string
 		errString             string
 	}{
 		{
@@ -1045,11 +1044,8 @@ func TestGetNeutronPortNodeEgressIPConfiguration(t *testing.T) {
 					IP: ptr.To(openstackMaxCapacity + 3 - 2), // excluding 2 allowed_address_pairs configured on the port.
 				},
 			},
-			// Configure cloudPrivateIPConfigs with 3 ips are within neutron subnet, 1 ip outside neutron subnet.
-			cloudPrivateIPConfigs: []*v1.CloudPrivateIPConfig{{ObjectMeta: metav1.ObjectMeta{
-				Name: "192.0.2.10"}}, {ObjectMeta: metav1.ObjectMeta{Name: "2000..1"}},
-				{ObjectMeta: metav1.ObjectMeta{Name: "2000..2"}},
-				{ObjectMeta: metav1.ObjectMeta{Name: "10.10.10.1"}}},
+			// Configure IPs with 3 ips are within neutron subnet, 1 ip outside neutron subnet.
+			cloudPrivateIPConfigs: []string{"192.0.2.10", "2000::1", "2000::2", "10.10.10.1"},
 		},
 		{
 			port:      portMap["aafecceb-d986-42b6-8ea7-449c7cacb7d9"],
@@ -1062,7 +1058,8 @@ func TestGetNeutronPortNodeEgressIPConfiguration(t *testing.T) {
 	}
 
 	for i, tc := range tcs {
-		nodeEgressIPConfig, err := o.getNeutronPortNodeEgressIPConfiguration(tc.port, tc.cloudPrivateIPConfigs)
+		cpicIPs := sets.New[string](tc.cloudPrivateIPConfigs...)
+		nodeEgressIPConfig, err := o.getNeutronPortNodeEgressIPConfiguration(tc.port, cpicIPs)
 		if err != nil {
 			if !strings.Contains(err.Error(), tc.errString) {
 				t.Fatalf("TestGetNeutronPortNodeEgressIPConfiguration(%d): Received unexpected error, err: %q, expected: %q", i, err, tc.errString)

pkg/controller/node/node_controller.go

@@ -10,6 +10,7 @@ import (
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/util/sets"
 	coreinformers "k8s.io/client-go/informers/core/v1"
 	"k8s.io/client-go/kubernetes"
 	corelisters "k8s.io/client-go/listers/core/v1"
@@ -22,6 +23,7 @@ import (
 	cloudnetworkinformers "github.com/openshift/client-go/cloudnetwork/informers/externalversions/cloudnetwork/v1"
 	cloudnetworklisters "github.com/openshift/client-go/cloudnetwork/listers/cloudnetwork/v1"
 	cloudprovider "github.com/openshift/cloud-network-config-controller/pkg/cloudprovider"
+	"github.com/openshift/cloud-network-config-controller/pkg/cloudprivateipconfig"
 	controller "github.com/openshift/cloud-network-config-controller/pkg/controller"
 )
 
@@ -116,17 +118,25 @@ func (n *NodeController) SyncHandler(key string) error {
 	if err != nil {
 		return fmt.Errorf("error listing cloud private ip config, err: %v", err)
 	}
-	// Filter out cloudPrivateIPConfigs assigned to node (key) and write the entry
-	// into same slice starting from index 0, finally chop off unwanted entries
-	// when passing it into GetNodeEgressIPConfiguration.
-	index := 0
+	// Filter cloudPrivateIPConfigs that are associated with this node.
+	// Include CPICs where spec.node OR status.node matches, to handle transitions
+	// during move operations and avoid counting CPIC IPs as "consumed capacity".
+	// We need both spec.node and status.node because during a move operation:
+	// - The IP might still be on the old node (status.node) while spec.node has changed
+	// - The IP might already be on the new node (spec.node) while status.node hasn't updated
+	// Build set of CPIC-managed IP addresses for this node
+	// Include CPICs where spec.node OR status.node matches to handle transitions
+	cpicIPs := sets.New[string]()
 	for _, cloudPrivateIPConfig := range cloudPrivateIPConfigs {
-		if isAssignedCloudPrivateIPConfigOnNode(cloudPrivateIPConfig, key) {
-			cloudPrivateIPConfigs[index] = cloudPrivateIPConfig
-			index++
+		if cloudPrivateIPConfig.Spec.Node == key || cloudPrivateIPConfig.Status.Node == key {
+			ip, _, err := cloudprivateipconfig.NameToIP(cloudPrivateIPConfig.Name)
+			if err != nil {
+				return fmt.Errorf("error parsing CloudPrivateIPConfig %s: %v", cloudPrivateIPConfig.Name, err)
+			}
+			cpicIPs.Insert(ip.String())
 		}
 	}
-	nodeEgressIPConfigs, err := n.cloudProviderClient.GetNodeEgressIPConfiguration(node, cloudPrivateIPConfigs[:index])
+	nodeEgressIPConfigs, err := n.cloudProviderClient.GetNodeEgressIPConfiguration(node, cpicIPs)
 	if err != nil {
 		return fmt.Errorf("error retrieving the private IP configuration for node: %s, err: %v", node.Name, err)
 	}