Implement migration from L4 Legacy Target Pool based services to L4 RBS

- Switch target of existing forwarding rule from target pool to regional
  backend service
- Clean up GCP resources used only by legacy service: target pool,
  health checks, firewall rules for health checks

pkg/composite/composite.go

@@ -185,7 +185,7 @@ func SetUrlMapForTargetHttpProxy(gceCloud *gce.Cloud, key *meta.Key, targetHttpP
 	}
 }
 
-// SetProxyForForwardingRule() sets the target proxy for a forwarding rule
+// SetProxyForForwardingRule sets the target proxy for a forwarding rule
 func SetProxyForForwardingRule(gceCloud *gce.Cloud, key *meta.Key, forwardingRule *ForwardingRule, targetProxyLink string) error {
 	ctx, cancel := cloud.ContextWithCallTimeout()
 	defer cancel()

pkg/l4lb/l4netlbcontroller.go

@@ -251,10 +251,7 @@ func (lc *L4NetLBController) isRBSBasedService(svc *v1.Service) bool {
 	if svc == nil {
 		return false
 	}
-	if val, ok := svc.Annotations[annotations.RBSAnnotationKey]; ok && val == annotations.RBSEnabled {
-		return true
-	}
-	return utils.HasL4NetLBFinalizerV2(svc) || lc.hasRBSForwardingRule(svc)
+	return utils.HasRBSAnnotation(svc) || utils.HasL4NetLBFinalizerV2(svc) || lc.hasRBSForwardingRule(svc)
 }
 
 func (lc *L4NetLBController) checkHealth() error {
@@ -333,6 +330,16 @@ func (lc *L4NetLBController) syncInternal(service *v1.Service) *loadbalancers.L4
 		return &loadbalancers.L4NetLBSyncResult{Error: fmt.Errorf("Failed to attach L4 External LoadBalancer finalizer to service %s/%s, err %w", service.Namespace, service.Name, err)}
 	}
 
+	needsLegacyL4Cleanup := utils.HasNeedsLegacyL4CleanupFinalizer(service)
+	frName := l4netlb.GetFRName()
+	existingForwardingRule := l4netlb.GetForwardingRule(frName, meta.VersionGA)
+	if existingForwardingRule != nil && existingForwardingRule.Target != "" {
+		if err := common.EnsureServiceFinalizer(service, common.NeedsLegacyL4CleanupFinalizer, lc.ctx.KubeClient); err != nil {
+			return &loadbalancers.L4NetLBSyncResult{Error: fmt.Errorf("Failed to attach NeedsLegacyL4CleanupFinalizer to service %s/%s, err %w", service.Namespace, service.Name, err)}
+		}
+		needsLegacyL4Cleanup = true
+	}
+
 	nodeNames, err := utils.GetReadyNodeNames(lc.nodeLister)
 	if err != nil {
 		return &loadbalancers.L4NetLBSyncResult{Error: err}
@@ -346,7 +353,7 @@ func (lc *L4NetLBController) syncInternal(service *v1.Service) *loadbalancers.L4
 
 	// Use the same function for both create and updates. If controller crashes and restarts,
 	// all existing services will show up as Service Adds.
-	syncResult := l4netlb.EnsureFrontend(nodeNames, service)
+	syncResult := l4netlb.EnsureFrontendWithExistingRule(nodeNames, service, existingForwardingRule)
 	if syncResult.Error != nil {
 		lc.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeWarning, "SyncExternalLoadBalancerFailed",
 			"Error ensuring Resource for L4 External LoadBalancer, err: %v", syncResult.Error)
@@ -375,6 +382,19 @@ func (lc *L4NetLBController) syncInternal(service *v1.Service) *loadbalancers.L4
 		syncResult.Error = fmt.Errorf("failed to set resource annotations, err: %w", err)
 		return syncResult
 	}
+
+	if needsLegacyL4Cleanup {
+		err = l4netlb.CleanLegacyServiceResources()
+		if err != nil {
+			lc.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeWarning, "CleanLegacyServiceResourcesFailed",
+				"Error deleting Legacy L4 External LoadBalancer resources, err: %v", err)
+		} else {
+			if err = common.EnsureDeleteServiceFinalizer(service, common.NeedsLegacyL4CleanupFinalizer, lc.ctx.KubeClient); err != nil {
+				return &loadbalancers.L4NetLBSyncResult{Error: fmt.Errorf("Failed to delete NeedsLegacyL4CleanupFinalizer from service %s/%s, err %w", service.Namespace, service.Name, err)}
+			}
+		}
+	}
+
 	syncResult.MetricsState.InSuccess = true
 	return syncResult
 }

pkg/loadbalancers/forwarding_rules.go

@@ -323,15 +323,14 @@ func (l *L4) deleteForwardingRule(name string, version meta.Version) {
 
 // ensureExternalForwardingRule creates a forwarding rule with the given name for L4NetLB,
 // if it does not exist. It updates the existing forwarding rule if needed.
-func (l4netlb *L4NetLB) ensureExternalForwardingRule(bsLink string) (*composite.ForwardingRule, IPAddressType, error) {
+func (l4netlb *L4NetLB) ensureExternalForwardingRule(bsLink string, existingFwdRule *composite.ForwardingRule) (*composite.ForwardingRule, IPAddressType, error) {
 	frName := l4netlb.GetFRName()
 	key, err := l4netlb.createKey(frName)
 	if err != nil {
 		return nil, IPAddrUndefined, err
 	}
 	// version used for creating the existing forwarding rule.
 	version := meta.VersionGA
-	existingFwdRule := l4netlb.GetForwardingRule(frName, version)
 
 	// Determine IP which will be used for this LB. If no forwarding rule has been established
 	// or specified in the Service spec, then requestedIP = "".
@@ -393,6 +392,21 @@ func (l4netlb *L4NetLB) ensureExternalForwardingRule(bsLink string) (*composite.
 			networkTierMismatchError := utils.NewNetworkTierErr(resource, existingFwdRule.NetworkTier, fr.NetworkTier)
 			return nil, IPAddrUndefined, networkTierMismatchError
 		}
+
+		// Check if we transition from Target Pool Legacy Service to RBS based one.
+		// If configs are not equal, we should delete forwarding rule and recreate.
+		if existingFwdRule.Target != "" && ConfigsEqual(fr, existingFwdRule) {
+			// If transitioning from Legacy L4 Net LB Service to RBS,
+			// instead of forwarding rule recreation, call set-target
+			// on forwarding rule, to minimize down-time
+			err = composite.SetProxyForForwardingRule(l4netlb.cloud, key, existingFwdRule, bsLink)
+			if err != nil {
+				return nil, IPAddrUndefined, err
+			}
+			createdFr, err := composite.GetForwardingRule(l4netlb.cloud, key, fr.Version)
+			return createdFr, isIPManaged, err
+		}
+
 		equal, err := Equal(existingFwdRule, fr)
 		if err != nil {
 			return existingFwdRule, IPAddrUndefined, err
@@ -452,6 +466,19 @@ func (l4netlb *L4NetLB) deleteForwardingRule(name string, version meta.Version)
 	}
 }
 
+// ConfigsEqual check if two forwarding rules are equal in configurations, except for backend targets
+func ConfigsEqual(fr1, fr2 *composite.ForwardingRule) bool {
+	return fr1.IPAddress == fr2.IPAddress &&
+		fr1.IPProtocol == fr2.IPProtocol &&
+		fr1.LoadBalancingScheme == fr2.LoadBalancingScheme &&
+		utils.EqualStringSets(fr1.Ports, fr2.Ports) &&
+		fr1.AllowGlobalAccess == fr2.AllowGlobalAccess &&
+		fr1.AllPorts == fr2.AllPorts &&
+		fr1.Subnetwork == fr2.Subnetwork &&
+		fr1.NetworkTier == fr2.NetworkTier
+}
+
+// Equal checks if two forwarding rules are equal both in configs and in backend targets
 func Equal(fr1, fr2 *composite.ForwardingRule) (bool, error) {
 	id1, err := cloud.ParseResourceURL(fr1.BackendService)
 	if err != nil {
@@ -461,15 +488,7 @@ func Equal(fr1, fr2 *composite.ForwardingRule) (bool, error) {
 	if err != nil {
 		return false, fmt.Errorf("forwardingRulesEqual(): failed to parse resource URL from FR, err - %w", err)
 	}
-	return fr1.IPAddress == fr2.IPAddress &&
-		fr1.IPProtocol == fr2.IPProtocol &&
-		fr1.LoadBalancingScheme == fr2.LoadBalancingScheme &&
-		utils.EqualStringSets(fr1.Ports, fr2.Ports) &&
-		id1.Equal(id2) &&
-		fr1.AllowGlobalAccess == fr2.AllowGlobalAccess &&
-		fr1.AllPorts == fr2.AllPorts &&
-		fr1.Subnetwork == fr2.Subnetwork &&
-		fr1.NetworkTier == fr2.NetworkTier, nil
+	return ConfigsEqual(fr1, fr2) && id1.Equal(id2), nil
 }
 
 // l4lbIPToUse determines which IP address needs to be used in the ForwardingRule. If an IP has been

pkg/loadbalancers/l4netlb.go

@@ -26,7 +26,9 @@ import (
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/client-go/tools/record"
+	cloudprovider "k8s.io/cloud-provider"
 	"k8s.io/cloud-provider/service/helpers"
 	"k8s.io/ingress-gce/pkg/annotations"
 	"k8s.io/ingress-gce/pkg/backends"
@@ -96,7 +98,7 @@ func (l4netlb *L4NetLB) createKey(name string) (*meta.Key, error) {
 // been created. It is health check, firewall rules, backend service and forwarding rule.
 // It returns a LoadBalancerStatus with the updated ForwardingRule IP address.
 // This function does not link instances to Backend Service.
-func (l4netlb *L4NetLB) EnsureFrontend(nodeNames []string, svc *corev1.Service) *L4NetLBSyncResult {
+func (l4netlb *L4NetLB) EnsureFrontendWithExistingRule(nodeNames []string, svc *corev1.Service, existingForwardingRule *composite.ForwardingRule) *L4NetLBSyncResult {
 	result := &L4NetLBSyncResult{
 		Annotations: make(map[string]string),
 		StartTime:   time.Now(),
@@ -140,7 +142,7 @@ func (l4netlb *L4NetLB) EnsureFrontend(nodeNames []string, svc *corev1.Service)
 		return result
 	}
 	result.Annotations[annotations.BackendServiceKey] = name
-	fr, ipAddrType, err := l4netlb.ensureExternalForwardingRule(bs.SelfLink)
+	fr, ipAddrType, err := l4netlb.ensureExternalForwardingRule(bs.SelfLink, existingForwardingRule)
 	if err != nil {
 		// User can misconfigure the forwarding rule if Network Tier will not match service level Network Tier.
 		result.MetricsState.IsUserError = utils.IsUserError(err)
@@ -159,6 +161,10 @@ func (l4netlb *L4NetLB) EnsureFrontend(nodeNames []string, svc *corev1.Service)
 	return result
 }
 
+func (l4netlb *L4NetLB) EnsureFrontend(nodeNames []string, svc *corev1.Service) *L4NetLBSyncResult {
+	return l4netlb.EnsureFrontendWithExistingRule(nodeNames, svc, nil)
+}
+
 // EnsureLoadBalancerDeleted performs a cleanup of all GCE resources for the given loadbalancer service.
 // It is health check, firewall rules and backend service
 func (l4netlb *L4NetLB) EnsureLoadBalancerDeleted(svc *corev1.Service) *L4NetLBSyncResult {
@@ -295,3 +301,62 @@ func (l4netlb *L4NetLB) createFirewalls(name, hcLink, hcFwName string, hcPort in
 	}
 	return string(protocol), result
 }
+
+// CleanLegacyServiceResources deletes resources, used only by legacy L4 NetLB, based on Target Pools and implemented in service controller
+func (l4netlb *L4NetLB) CleanLegacyServiceResources() error {
+	loadBalancerName := cloudprovider.DefaultLoadBalancerName(l4netlb.Service)
+	serviceName := types.NamespacedName{Namespace: l4netlb.Service.Namespace, Name: l4netlb.Service.Name}
+	lbRefStr := fmt.Sprintf("%v(%v)", loadBalancerName, serviceName)
+
+	errs := utilerrors.AggregateGoroutines(
+		func() error {
+			klog.Infof("cleanLegacyServiceResources(%s): Deleting firewall rule.", lbRefStr)
+			fwName := gce.MakeFirewallName(loadBalancerName)
+			err := utils.IgnoreHTTPNotFound(l4netlb.cloud.DeleteFirewall(fwName))
+			return err
+		},
+		func() error {
+			klog.Infof("cleanLegacyServiceResources(%s): Deleting target pool", lbRefStr)
+
+			err := utils.IgnoreHTTPNotFound(l4netlb.cloud.DeleteTargetPool(loadBalancerName, l4netlb.cloud.Region()))
+			if err != nil {
+				klog.Errorf("cleanLegacyServiceResources(%v): Failed to delete target pool, got error %s", lbRefStr, err.Error())
+				return err
+			}
+
+			// Deletion of health checks is allowed only after the TargetPool reference is deleted
+			// Try to delete 2 types of health checks -- used for local traffic policy, and external
+			// They both use legacy naming scheme, so they are not useful for RBS service, and it is safe to delete them
+			healthChecksToDelete := []string{loadBalancerName, gce.MakeNodesHealthCheckName(l4netlb.namer.ClusterID())}
+
+			for _, hcToDelete := range healthChecksToDelete {
+				klog.Infof("cleanLegacyServiceResources(%v): Deleting health check %v", lbRefStr, hcToDelete)
+				err := utils.IgnoreHTTPNotFound(l4netlb.cloud.DeleteHTTPHealthCheck(hcToDelete))
+				if err != nil {
+					// Delete nodes health checks will fail if any other target pool is using it.
+					if utils.IsInUsedByError(err) {
+						klog.V(4).Infof("cleanLegacyServiceResources(%v): Health check %v is in use: %v", lbRefStr, hcToDelete, err)
+						return nil
+					}
+					return err
+				}
+
+				// If health check is deleted without error, it means no load-balancer is using it.
+				// So we should delete the health check firewall as well.
+				fwName := gce.MakeHealthCheckFirewallName(l4netlb.namer.ClusterID(), hcToDelete, hcToDelete != loadBalancerName)
+
+				klog.Infof("cleanLegacyServiceResources(%v): Deleting health check firewall rule %v", lbRefStr, fwName)
+				err = utils.IgnoreHTTPNotFound(l4netlb.cloud.DeleteFirewall(fwName))
+				if err != nil {
+					klog.Errorf("cleanLegacyServiceResources(%v): Failed to delete health check %v firewall rule %v: %v", lbRefStr, hcToDelete, fwName, err)
+					return err
+				}
+			}
+			return nil
+		},
+	)
+	if errs != nil {
+		return utilerrors.Flatten(errs)
+	}
+	return nil
+}

pkg/utils/common/finalizer.go

@@ -43,6 +43,9 @@ const (
 	LegacyNetLBFinalizer = "gke.networking.io/l4-netlb-v1"
 	// NetLBFinalizerV2 is the finalizer used by newer controllers that manage L4 External LoadBalancer services.
 	NetLBFinalizerV2 = "gke.networking.io/l4-netlb-v2"
+	// NeedsLegacyL4CleanupFinalizer is the finalizer used to indicate,
+	// that Ingress-GCE should clean up GCE resources used for Legacy L4 Target-Pool based Service
+	NeedsLegacyL4CleanupFinalizer = "gke.networking.io/needs-legacy-l4-cleanup"
 )
 
 // IsDeletionCandidate is true if the passed in meta contains an ingress finalizer.

pkg/utils/namer/interfaces.go

@@ -91,6 +91,8 @@ type L4ResourcesNamer interface {
 	L4HealthCheck(namespace, name string, shared bool) (string, string)
 	// IsNEG returns if the given name is a VM_IP_NEG name.
 	IsNEG(name string) bool
+	// ClusterID returns cluster's id
+	ClusterID() string
 }
 
 type ServiceAttachmentNamer interface {

pkg/utils/namer/l4_namer.go

@@ -101,3 +101,8 @@ func (n *L4Namer) hcFirewallName(hcName string) string {
 	}
 	return hcName + firewallHcSuffix
 }
+
+// ClusterID returns cluster's id
+func (n *L4Namer) ClusterID() string {
+	return n.v2ClusterUID
+}

pkg/utils/utils.go

@@ -693,6 +693,18 @@ func HasL4NetLBFinalizerV2(svc *api_v1.Service) bool {
 	return slice.ContainsString(svc.ObjectMeta.Finalizers, common.NetLBFinalizerV2, nil)
 }
 
+// HasNeedsLegacyL4CleanupFinalizer returns true if the given Service has NeedsLegacyL4CleanupFinalizer
+func HasNeedsLegacyL4CleanupFinalizer(svc *api_v1.Service) bool {
+	return slice.ContainsString(svc.ObjectMeta.Finalizers, common.NeedsLegacyL4CleanupFinalizer, nil)
+}
+
+func HasRBSAnnotation(svc *api_v1.Service) bool {
+	if val, ok := svc.Annotations[annotations.RBSAnnotationKey]; ok && val == annotations.RBSEnabled {
+		return true
+	}
+	return false
+}
+
 func LegacyForwardingRuleName(svc *api_v1.Service) string {
 	return cloudprovider.DefaultLoadBalancerName(svc)
 }