OCPBUGS-57447,OCPBUGS-45056: Refrain from adding Egress IP to public LB backend pool

pkg/cloudprovider/aws.go

@@ -226,6 +226,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"
@@ -26,6 +24,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/klog/v2"
 	utilnet "k8s.io/utils/net"
+	"k8s.io/utils/ptr"
 )
 
 const (
@@ -48,10 +47,10 @@ type Azure struct {
 	vmClient                     *armcompute.VirtualMachinesClient
 	virtualNetworkClient         *armnetwork.VirtualNetworksClient
 	networkClient                *armnetwork.InterfacesClient
-	backendAddressPoolClient     *armnetwork.LoadBalancerBackendAddressPoolsClient
 	nodeMapLock                  sync.Mutex
 	nodeLockMap                  map[string]*sync.Mutex
 	azureWorkloadIdentityEnabled bool
+	lbBackendPoolSynced          bool
 }
 
 type azureCredentialsConfig struct {
@@ -162,11 +161,6 @@ func (a *Azure) initCredentials() error {
 		return fmt.Errorf("failed to initialize new VirtualNetworksClient: %w", err)
 	}
 
-	a.backendAddressPoolClient, err = armnetwork.NewLoadBalancerBackendAddressPoolsClient(cfg.subscriptionID, cred, options)
-	if err != nil {
-		return fmt.Errorf("failed to initialize new LoadBalancerBackendAddressPoolsClient: %w", err)
-	}
-
 	return nil
 }
 
@@ -198,65 +192,14 @@ func (a *Azure) AssignPrivateIP(ip net.IP, node *corev1.Node) error {
 	name := fmt.Sprintf("%s_%s", node.Name, ipc)
 	untrue := false
 
-	// In some Azure setups (Azure private, public ARO, private ARO) outbound connectivity is achieved through
-	// outbound rules tied to the backend address pool of the primary IP of the VM NIC. An Azure constraint
-	// forbids the creation of a secondary IP tied to such address pool and would result in
-	// OutboundRuleCannotBeUsedWithBackendAddressPoolThatIsReferencedBySecondaryIpConfigs.
-	// Work around it by not specifying the backend address pool when an outbound rule is set, even though
-	// that means preventing outbound connectivity to the egress IP, which will be able to reach the
-	// infrastructure subnet nonetheless. In public Azure clusters, outbound connectivity is achieved through
-	// UserDefinedRouting, which doesn't impose such constraints on secondary IPs.
-	loadBalancerBackendAddressPoolsArgument := networkInterface.Properties.IPConfigurations[0].Properties.LoadBalancerBackendAddressPools
-	var attachedOutboundRule *armnetwork.SubResource
-OuterLoop:
-	for _, ipconfig := range networkInterface.Properties.IPConfigurations {
-		if ipconfig.Properties.LoadBalancerBackendAddressPools != nil {
-			for _, pool := range ipconfig.Properties.LoadBalancerBackendAddressPools {
-				if pool.ID == nil {
-					continue
-				}
-				// for some reason, the struct for the pool above is not entirely filled out:
-				//     BackendAddressPoolPropertiesFormat:(*network.BackendAddressPoolPropertiesFormat)(nil)
-				// Do a separate get for this pool in order to check whether there are any outbound rules
-				// attached to it
-				realPool, err := a.getBackendAddressPool(ptr.Deref(pool.ID, ""))
-				if err != nil {
-					return fmt.Errorf("error looking up backend address pool %s with ID %s: %v", ptr.Deref(pool.Name, ""), ptr.Deref(pool.ID, ""), err)
-				}
-				if len(realPool.Properties.LoadBalancerBackendAddresses) > 0 {
-					if realPool.Properties.OutboundRule != nil {
-						loadBalancerBackendAddressPoolsArgument = nil
-						attachedOutboundRule = realPool.Properties.OutboundRule
-						break OuterLoop
-					}
-					if len(realPool.Properties.OutboundRules) > 0 {
-						loadBalancerBackendAddressPoolsArgument = nil
-						attachedOutboundRule = (realPool.Properties.OutboundRules)[0]
-						break OuterLoop
-					}
-				}
-			}
-		}
-	}
-	if loadBalancerBackendAddressPoolsArgument == nil {
-		outboundRuleStr := ""
-		if attachedOutboundRule != nil && attachedOutboundRule.ID != nil {
-			// https://issues.redhat.com/browse/OCPBUGS-33617 showed that there can be a rule without an ID...
-			outboundRuleStr = fmt.Sprintf(": %s", ptr.Deref(attachedOutboundRule.ID, ""))
-		}
-		klog.Warningf("Egress IP %s will have no outbound connectivity except for the infrastructure subnet: "+
-			"omitting backend address pool when adding secondary IP: it has an outbound rule already%s",
-			ipc, outboundRuleStr)
-	}
 	newIPConfiguration := &armnetwork.InterfaceIPConfiguration{
 		Name: &name,
 		Properties: &armnetwork.InterfaceIPConfigurationPropertiesFormat{
-			PrivateIPAddress:                &ipc,
-			PrivateIPAllocationMethod:       ptr.To(armnetwork.IPAllocationMethodStatic),
-			Subnet:                          networkInterface.Properties.IPConfigurations[0].Properties.Subnet,
-			Primary:                         &untrue,
-			LoadBalancerBackendAddressPools: loadBalancerBackendAddressPoolsArgument,
-			ApplicationSecurityGroups:       applicationSecurityGroups,
+			PrivateIPAddress:          &ipc,
+			PrivateIPAllocationMethod: ptr.To(armnetwork.IPAllocationMethodStatic),
+			Subnet:                    networkInterface.Properties.IPConfigurations[0].Properties.Subnet,
+			Primary:                   &untrue,
+			ApplicationSecurityGroups: applicationSecurityGroups,
 		},
 	}
 	for _, ipCfg := range ipConfigurations {
@@ -272,6 +215,8 @@ OuterLoop:
 	ipConfigurations = append(ipConfigurations, newIPConfiguration)
 	networkInterface.Properties.IPConfigurations = ipConfigurations
 	// Send the request
+	klog.Warningf("Egress IP %s will have no outbound connectivity except for the infrastructure subnet: "+
+		"omitting backend address pool when adding secondary IP", ipc)
 	poller, err := a.createOrUpdate(networkInterface)
 	if err != nil {
 		return err
@@ -357,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()
@@ -491,46 +490,6 @@ func (a *Azure) getNetworkInterfaces(instance *armcompute.VirtualMachine) ([]arm
 	return networkInterfaces, nil
 }
 
-func splitObjectID(azureResourceID string) (resourceGroupName, loadBalancerName, backendAddressPoolName string) {
-	// example of an azureResourceID:
-	// "/subscriptions/53b8f551-f0fc-4bea-8cba-6d1fefd54c8a/resourceGroups/huirwang-debug1-2qh9t-rg/providers/Microsoft.Network/loadBalancers/huirwang-debug1-2qh9t/backendAddressPools/huirwang-debug1-2qh9t"
-
-	// Split the Azure resource ID into parts using "/"
-	parts := strings.Split(azureResourceID, "/")
-
-	// Iterate through the parts to find the relevant subIDs
-	for i, part := range parts {
-		switch part {
-		case "resourceGroups":
-			if i+1 < len(parts) {
-				resourceGroupName = parts[i+1]
-			}
-		case "loadBalancers":
-			if i+1 < len(parts) {
-				loadBalancerName = parts[i+1]
-			}
-		case "backendAddressPools":
-			if i+1 < len(parts) {
-				backendAddressPoolName = parts[i+1]
-			}
-		}
-	}
-	return
-}
-
-func (a *Azure) getBackendAddressPool(poolID string) (*armnetwork.BackendAddressPool, error) {
-	ctx, cancel := context.WithTimeout(a.ctx, defaultAzureOperationTimeout)
-	defer cancel()
-	resourceGroupName, loadBalancerName, backendAddressPoolName := splitObjectID(poolID)
-	response, err := a.backendAddressPoolClient.Get(ctx, resourceGroupName, loadBalancerName, backendAddressPoolName, nil)
-	if err != nil {
-		return nil, fmt.Errorf("failed to retrieve backend address pool for backendAddressPoolClient=%s, loadBalancerName=%s, backendAddressPoolName=%s: %w",
-			resourceGroupName, loadBalancerName, backendAddressPoolName, err)
-	}
-	return &response.BackendAddressPool, nil
-
-}
-
 func (a *Azure) getNetworkInterface(id string) (armnetwork.Interface, error) {
 	ctx, cancel := context.WithTimeout(a.ctx, defaultAzureOperationTimeout)
 	defer cancel()

pkg/cloudprovider/cloudprovider.go

@@ -9,11 +9,10 @@ import (
 	"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"
-
-	v1 "github.com/openshift/api/cloudnetwork/v1"
 	corev1 "k8s.io/api/core/v1"
 )
 
@@ -61,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

@@ -191,6 +191,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

@@ -546,6 +546,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)