Remove any Egress IP from Azure public LB backend

The consensus is to not add egress IP to public load balancer
backend pool regardless of the presence of an OutBoundRule.
During upgrade this PR let cobtroller removes any egress IP
added to public load balancer backend pool previously.

Signed-off-by: Arnab Ghosh <[email protected]>

Author: arghosh93

pkg/cloudprovider/aws.go

@@ -222,6 +222,11 @@ func (a *AWS) GetNodeEgressIPConfiguration(node *corev1.Node, cpicIPs sets.Set[s
 	return []*NodeEgressIPConfiguration{config}, nil
 }
 
+func (a *AWS) SyncLBBackend(_ net.IP, _ *corev1.Node) error {
+	// We dont add Egress IP to AWS public LB backend; nothing to do
+	return nil
+}
+
 // Unfortunately the AWS API (WaitUntilInstanceRunning) only handles equality
 // assertion: so on delete we can't specify and assert that the IP which is
 // being removed is completely removed, we are forced to do the inverse, i.e:

pkg/cloudprovider/azure.go

@@ -9,13 +9,11 @@ import (
 	"sync"
 	"time"
 
-	"github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime"
-	"k8s.io/utils/ptr"
-
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud"
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime"
 	"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute"
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v6"
@@ -26,6 +24,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/klog/v2"
 	utilnet "k8s.io/utils/net"
+	"k8s.io/utils/ptr"
 )
 
 const (
@@ -51,6 +50,8 @@ type Azure struct {
 	nodeMapLock                  sync.Mutex
 	nodeLockMap                  map[string]*sync.Mutex
 	azureWorkloadIdentityEnabled bool
+	lbBackendPoolSynced          map[string]bool
+	lbBackendPoolSyncedLock      sync.Mutex
 }
 
 type azureCredentialsConfig struct {
@@ -172,11 +173,11 @@ func (a *Azure) AssignPrivateIP(ip net.IP, node *corev1.Node) error {
 	defer nodeLock.Unlock()
 	instance, err := a.getInstance(node)
 	if err != nil {
-		return err
+		return fmt.Errorf("error while retrieving instance details from Azure: %w", err)
 	}
 	networkInterfaces, err := a.getNetworkInterfaces(instance)
 	if err != nil {
-		return err
+		return fmt.Errorf("error while retrieving interface details from Azure: %w", err)
 	}
 	if networkInterfaces[0].Properties == nil {
 		return fmt.Errorf("nil network interface properties")
@@ -219,9 +220,18 @@ func (a *Azure) AssignPrivateIP(ip net.IP, node *corev1.Node) error {
 		"omitting backend address pool when adding secondary IP", ipc)
 	poller, err := a.createOrUpdate(networkInterface)
 	if err != nil {
-		return err
+		return fmt.Errorf("error while updating network interface: %w", err)
+	}
+	if err = a.waitForCompletion(poller); err != nil {
+		return fmt.Errorf("error while updating network interface: %w", err)
 	}
-	return a.waitForCompletion(poller)
+	// setting lbBackendPoolSynced to true here to make sure that we dont try to
+	// sync LB backend for any new egress IP later
+	cacheKey := getIPCacheKey(ip, node)
+	a.lbBackendPoolSyncedLock.Lock()
+	a.lbBackendPoolSynced[cacheKey] = true
+	a.lbBackendPoolSyncedLock.Unlock()
+	return nil
 }
 
 func (a *Azure) ReleasePrivateIP(ip net.IP, node *corev1.Node) error {
@@ -231,11 +241,11 @@ func (a *Azure) ReleasePrivateIP(ip net.IP, node *corev1.Node) error {
 	defer nodeLock.Unlock()
 	instance, err := a.getInstance(node)
 	if err != nil {
-		return err
+		return fmt.Errorf("error while retrieving instance details from Azure: %w", err)
 	}
 	networkInterfaces, err := a.getNetworkInterfaces(instance)
 	if err != nil {
-		return err
+		return fmt.Errorf("error while retrieving interface details from Azure: %w", err)
 	}
 	// Perform the operation against the first interface listed, which will be
 	// the primary interface (if it's defined as such) or the first one returned
@@ -262,9 +272,16 @@ func (a *Azure) ReleasePrivateIP(ip net.IP, node *corev1.Node) error {
 	// Send the request
 	poller, err := a.createOrUpdate(networkInterface)
 	if err != nil {
-		return err
+		return fmt.Errorf("error while updating network interface: %w", err)
+	}
+	if err = a.waitForCompletion(poller); err != nil {
+		return fmt.Errorf("error while updating network interface: %w", err)
 	}
-	return a.waitForCompletion(poller)
+	cacheKey := getIPCacheKey(ip, node)
+	a.lbBackendPoolSyncedLock.Lock()
+	delete(a.lbBackendPoolSynced, cacheKey)
+	a.lbBackendPoolSyncedLock.Unlock()
+	return nil
 }
 
 func (a *Azure) GetNodeEgressIPConfiguration(node *corev1.Node, cpicIPs sets.Set[string]) ([]*NodeEgressIPConfiguration, error) {
@@ -302,6 +319,62 @@ func (a *Azure) GetNodeEgressIPConfiguration(node *corev1.Node, cpicIPs sets.Set
 	return []*NodeEgressIPConfiguration{config}, nil
 }
 
+// The consensus is to not add egress IP to public load balancer
+// backend pool regardless of the presence of an OutBoundRule.
+// During upgrade this function removes any egress IP added to
+// public load balancer backend pool previously.
+func (a *Azure) SyncLBBackend(ip net.IP, node *corev1.Node) error {
+	ipc := ip.String()
+	cacheKey := getIPCacheKey(ip, node)
+	a.lbBackendPoolSyncedLock.Lock()
+	defer a.lbBackendPoolSyncedLock.Unlock()
+	if synced, ok := a.lbBackendPoolSynced[cacheKey]; ok && synced {
+		// nothing to do. Return immediately if LB backend has already synced
+		return nil
+	}
+	klog.Infof("Acquiring node lock for modifying load balancer backend pool, node: %s, ip: %s", node.Name, ipc)
+	nodeLock := a.getNodeLock(node.Name)
+	nodeLock.Lock()
+	defer nodeLock.Unlock()
+	instance, err := a.getInstance(node)
+	if err != nil {
+		return fmt.Errorf("error while retrieving instance details from Azure: %w", err)
+	}
+	networkInterfaces, err := a.getNetworkInterfaces(instance)
+	if err != nil {
+		return fmt.Errorf("error while retrieving interface details from Azure: %w", err)
+	}
+	if networkInterfaces[0].Properties == nil {
+		return fmt.Errorf("nil network interface properties")
+	}
+	// Perform the operation against the first interface listed, which will be
+	// the primary interface (if it's defined as such) or the first one returned
+	// following the order Azure specifies.
+	networkInterface := networkInterfaces[0]
+	var loadBalancerBackendPoolModified bool
+	// omit Egress IP from LB backend pool
+	ipConfigurations := networkInterface.Properties.IPConfigurations
+	for _, ipCfg := range ipConfigurations {
+		if ptr.Deref(ipCfg.Properties.PrivateIPAddress, "") == ipc &&
+			ipCfg.Properties.LoadBalancerBackendAddressPools != nil {
+			ipCfg.Properties.LoadBalancerBackendAddressPools = nil
+			loadBalancerBackendPoolModified = true
+		}
+	}
+	if loadBalancerBackendPoolModified {
+		networkInterface.Properties.IPConfigurations = ipConfigurations
+		poller, err := a.createOrUpdate(networkInterface)
+		if err != nil {
+			return fmt.Errorf("error while updating network interface: %w", err)
+		}
+		if err = a.waitForCompletion(poller); err != nil {
+			return fmt.Errorf("error while updating network interface: %w", err)
+		}
+		a.lbBackendPoolSynced[cacheKey] = true
+	}
+	return nil
+}
+
 func (a *Azure) createOrUpdate(networkInterface armnetwork.Interface) (*runtime.Poller[armnetwork.InterfacesClientCreateOrUpdateResponse], error) {
 	ctx, cancel := context.WithTimeout(a.ctx, defaultAzureOperationTimeout)
 	defer cancel()
@@ -604,3 +677,7 @@ func ParseCloudEnvironment(env azureapi.Environment) cloud.Configuration {
 	}
 	return cloudConfig
 }
+
+func getIPCacheKey(ip net.IP, node *corev1.Node) string {
+	return fmt.Sprintf("%s|%s", node.Name, ip.String())
+}

pkg/cloudprovider/cloudprovider.go

@@ -67,6 +67,10 @@ type CloudProviderIntf interface {
 	// CleanupNode removes any internal state associated with the node.
 	// This should be called when a node is deleted.
 	CleanupNode(nodeName string)
+
+	// SyncLBBackend removes any egress IP which is already added to backend pool of
+	// a public load balancer. This is mostly Azure specific and may be removed later.
+	SyncLBBackend(ip net.IP, node *corev1.Node) error
 }
 
 // CloudProviderWithMoveIntf is additional interface that can be added to cloud
@@ -161,6 +165,7 @@ func NewCloudProviderClient(cfg CloudProviderConfig, platformStatus *configv1.Pl
 			platformStatus:               azurePlatformStatus,
 			nodeLockMap:                  make(map[string]*sync.Mutex),
 			azureWorkloadIdentityEnabled: featureGates.Enabled(apifeatures.FeatureGateAzureWorkloadIdentity),
+			lbBackendPoolSynced:          make(map[string]bool),
 		}
 	case PlatformTypeAWS:
 		cloudProviderIntf = &AWS{

pkg/cloudprovider/cloudprovider_fake.go

@@ -72,3 +72,7 @@ func (f *FakeCloudProvider) GetNodeEgressIPConfiguration(node *corev1.Node, cpic
 
 func (f *FakeCloudProvider) CleanupNode(nodeName string) {
 }
+
+func (f *FakeCloudProvider) SyncLBBackend(_ net.IP, _ *corev1.Node) error {
+	return nil
+}

pkg/cloudprovider/gcp.go

@@ -201,6 +201,11 @@ func (g *GCP) GetNodeEgressIPConfiguration(node *corev1.Node, cpicIPs sets.Set[s
 	return nil, nil
 }
 
+func (g *GCP) SyncLBBackend(_ net.IP, _ *corev1.Node) error {
+	// We dont add Egress IP to GCP public LB backend; nothing to do
+	return nil
+}
+
 // The GCP zone operations API call. All GCP infrastructure modifications are
 // assigned a unique operation ID and are queued in a global/zone operations
 // queue. In the case of assignments of private IP addresses to instances, the

pkg/cloudprovider/openstack.go

@@ -545,6 +545,11 @@ func (o *OpenStack) GetNodeEgressIPConfiguration(node *corev1.Node, cpicIPs sets
 	return nil, fmt.Errorf("no suitable interface configurations found")
 }
 
+func (o *OpenStack) SyncLBBackend(_ net.IP, _ *corev1.Node) error {
+	// We dont add Egress IP to OpenStack public LB backend; nothing to do
+	return nil
+}
+
 // getNeutronPortNodeEgressIPConfiguration renders the NeutronPortNodeEgressIPConfiguration for a given port.
 // The interface is keyed by a neutron UUID.
 // If multiple IPv4 repectively multiple IPv6 subnets are attached to the same port, throw an error.

pkg/controller/cloudprivateipconfig/cloudprivateipconfig_controller.go

@@ -180,8 +180,14 @@ func (c *CloudPrivateIPConfigController) SyncHandler(key string) error {
 
 	nodeNameToAdd, nodeNameToDel := c.computeOp(cloudPrivateIPConfig)
 	switch {
-	// Dequeue on NOOP, there's nothing to do
 	case nodeNameToAdd == "" && nodeNameToDel == "":
+		node, err := c.nodesLister.Get(cloudPrivateIPConfig.Spec.Node)
+		if err != nil {
+			return err
+		}
+		if err := c.cloudProviderClient.SyncLBBackend(ip, node); err != nil {
+			return fmt.Errorf("error while syncing egress IP %q added to LB backend pool: %w", key, err)
+		}
 		return nil
 	case nodeNameToAdd != "" && nodeNameToDel != "":
 		klog.Infof("CloudPrivateIPConfig: %q will be moved from node %q to node %q", key, nodeNameToDel, nodeNameToAdd)