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

@@ -224,6 +224,11 @@ func (a *AWS) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPConf
 	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"
@@ -27,6 +25,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/klog/v2"
 	utilnet "k8s.io/utils/net"
+	"k8s.io/utils/ptr"
 )
 
 const (
@@ -52,6 +51,7 @@ type Azure struct {
 	nodeMapLock                  sync.Mutex
 	nodeLockMap                  map[string]*sync.Mutex
 	azureWorkloadIdentityEnabled bool
+	lbBackendPoolSynced          bool
 }
 
 type azureCredentialsConfig struct {
@@ -302,6 +302,60 @@ func (a *Azure) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPCo
 	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 {
+	if a.lbBackendPoolSynced {
+		// nothing to do. Return immediately if LB backend has already synced
+		return nil
+	}
+	ipc := ip.String()
+	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 err
+	}
+	networkInterfaces, err := a.getNetworkInterfaces(instance)
+	if err != nil {
+		return 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 loadBalanceerBackendPoolModified 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
+			loadBalanceerBackendPoolModified = true
+		}
+	}
+	if loadBalanceerBackendPoolModified {
+		networkInterface.Properties.IPConfigurations = ipConfigurations
+		poller, err := a.createOrUpdate(networkInterface)
+		if err != nil {
+			return err
+		}
+		if err = a.waitForCompletion(poller); err != nil {
+			return err
+		}
+		a.lbBackendPoolSynced = true
+		return nil
+	}
+	return nil
+}
+
 func (a *Azure) createOrUpdate(networkInterface armnetwork.Interface) (*runtime.Poller[armnetwork.InterfacesClientCreateOrUpdateResponse], error) {
 	ctx, cancel := context.WithTimeout(a.ctx, defaultAzureOperationTimeout)
 	defer cancel()

pkg/cloudprovider/cloudprovider.go

@@ -4,15 +4,15 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	apifeatures "github.com/openshift/api/features"
-	configv1 "github.com/openshift/api/config/v1"
-	"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
 	"net"
 	"os"
 	"path/filepath"
 	"sync"
 
 	v1 "github.com/openshift/api/cloudnetwork/v1"
+	configv1 "github.com/openshift/api/config/v1"
+	apifeatures "github.com/openshift/api/features"
+	"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
 	corev1 "k8s.io/api/core/v1"
 )
 
@@ -60,6 +60,10 @@ type CloudProviderIntf interface {
 	// 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)
+
+	// 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

pkg/cloudprovider/cloudprovider_fake.go

@@ -69,3 +69,7 @@ func (f *FakeCloudProvider) GetNodeEgressIPConfiguration(node *corev1.Node, clou
 	}
 	return nil, nil
 }
+
+func (a *FakeCloudProvider) SyncLBBackend(_ net.IP, _ *corev1.Node) error {
+	return nil
+}

pkg/cloudprovider/gcp.go

@@ -189,6 +189,11 @@ func (g *GCP) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPConf
 	return nil, nil
 }
 
+func (a *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, cloudPrivate
 	return nil, fmt.Errorf("no suitable interface configurations found")
 }
 
+func (a *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", key)
+		}
 		return nil
 	case nodeNameToAdd != "" && nodeNameToDel != "":
 		klog.Infof("CloudPrivateIPConfig: %q will be moved from node %q to node %q", key, nodeNameToDel, nodeNameToAdd)