feat(l4): Add support for enabling L4 NetLB Regional Backend Services by default

cmd/glbc/main.go

@@ -365,6 +365,7 @@ func main() {
 		EnableL4ILBZonalAffinity:                  flags.F.EnableL4ILBZonalAffinity,
 		EnableL4NetLBForwardingRulesOptimizations: flags.F.EnableL4NetLBForwardingRulesOptimizations,
 		ReadOnlyMode:                              flags.F.ReadOnlyMode,
+		EnableL4NetLBRBSByDefault:                 flags.F.EnableL4NetLBRBSByDefault,
 	}
 	ctx, err := ingctx.NewControllerContext(kubeClient, backendConfigClient, frontendConfigClient, firewallCRClient, svcNegClient, svcAttachmentClient, networkClient, nodeTopologyClient, eventRecorderKubeClient, cloud, namer, kubeSystemUID, ctxConfig, rootLogger)
 	if err != nil {

pkg/annotations/service_test.go

@@ -715,3 +715,94 @@ func TestHasStrongSessionAffinityAnnotation(t *testing.T) {
 		})
 	}
 }
+
+func TestWantsL4NetLB(t *testing.T) {
+	// sPtr is a helper to return a pointer to a string,
+	// useful for setting LoadBalancerClass.
+	sPtr := func(s string) *string { return &s }
+
+	for _, tc := range []struct {
+		desc string
+		svc  *v1.Service
+		want bool
+	}{
+		{
+			desc: "Nil service",
+			svc:  nil,
+			want: false,
+		},
+		{
+			desc: "ClusterIP service should not want L4 NetLB",
+			svc: &v1.Service{
+				Spec: v1.ServiceSpec{
+					Type: v1.ServiceTypeClusterIP,
+				},
+			},
+			want: false,
+		},
+		{
+			desc: "Standard LoadBalancer service defaults to External (NetLB)",
+			svc: &v1.Service{
+				Spec: v1.ServiceSpec{
+					Type: v1.ServiceTypeLoadBalancer,
+				},
+			},
+			want: true,
+		},
+		{
+			desc: "LoadBalancer with Internal annotation should not want NetLB",
+			svc: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						"cloud.google.com/load-balancer-type": string(LBTypeInternal),
+					},
+				},
+				Spec: v1.ServiceSpec{
+					Type: v1.ServiceTypeLoadBalancer,
+				},
+			},
+			want: false,
+		},
+		{
+			desc: "LoadBalancer with explicit External annotation wants NetLB",
+			svc: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						"cloud.google.com/load-balancer-type": "External",
+					},
+				},
+				Spec: v1.ServiceSpec{
+					Type: v1.ServiceTypeLoadBalancer,
+				},
+			},
+			want: true,
+		},
+		{
+			desc: "LoadBalancer with matching Regional External Class wants NetLB",
+			svc: &v1.Service{
+				Spec: v1.ServiceSpec{
+					Type:              v1.ServiceTypeLoadBalancer,
+					LoadBalancerClass: sPtr(RegionalExternalLoadBalancerClass),
+				},
+			},
+			want: true,
+		},
+		{
+			desc: "LoadBalancer with mismatching Class does not want NetLB",
+			svc: &v1.Service{
+				Spec: v1.ServiceSpec{
+					Type:              v1.ServiceTypeLoadBalancer,
+					LoadBalancerClass: sPtr("some-other-custom-class"),
+				},
+			},
+			want: false,
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			got, _ := WantsL4NetLB(tc.svc)
+			if got != tc.want {
+				t.Errorf("WantsL4NetLB() = %v, want %v", got, tc.want)
+			}
+		})
+	}
+}

pkg/context/context.go

@@ -138,6 +138,7 @@ type ControllerContextConfig struct {
 	EnableL4ILBMixedProtocol                  bool
 	EnableL4NetLBMixedProtocol                bool
 	EnableL4NetLBForwardingRulesOptimizations bool
+	EnableL4NetLBRBSByDefault                 bool
 }
 
 // NewControllerContext returns a new shared set of informers.

pkg/flags/flags.go

@@ -149,6 +149,8 @@ var F = struct {
 	EnableNEGsForIngress                      bool
 	L4ILBLegacyHeadStartTime                  time.Duration
 	EnableIPv6NodeNEGEndpoints                bool
+	EnableL4NetLBRBSByDefault                 bool
+	L4NetLBLegacyHeadStartTime                time.Duration
 
 	// ===============================
 	// DEPRECATED FLAGS
@@ -362,6 +364,8 @@ L7 load balancing. CSV values accepted. Example: -node-port-ranges=80,8080,400-5
 	flag.BoolVar(&F.EnableNEGsForIngress, "enable-negs-for-ingress", true, "Allow the NEG controller to create NEGs for Ingress services.")
 	flag.DurationVar(&F.L4ILBLegacyHeadStartTime, "prevent-legacy-race-l4-ilb", 0*time.Second, "Delay before processing new L4 ILB services without existing finalizers. This gives the legacy controller a head start to claim the service, preventing a race condition upon service creation.")
 	flag.BoolVar(&F.EnableIPv6NodeNEGEndpoints, "enable-ipv6-node-neg-endpoints", false, "Enable populating IPv6 addresses for Node IPs in GCE_VM_IP NEGs.")
+	flag.BoolVar(&F.EnableL4NetLBRBSByDefault, "enable-l4-netlb-rbs-by-default", false, "Enable L4 NetLB Regional Backend Services by default for new L4 NetLB services.")
+	flag.DurationVar(&F.L4NetLBLegacyHeadStartTime, "prevent-legacy-race-l4-netlb", 0*time.Second, "Delay before processing new L4 NetLB services without existing finalizers. This gives the legacy controller a head start to claim the service, preventing a race condition upon service creation.")
 }
 
 func Validate() {

pkg/l4lb/l4netlbcontroller.go

@@ -36,6 +36,7 @@ import (
 	"k8s.io/ingress-gce/pkg/backends"
 	"k8s.io/ingress-gce/pkg/common/operator"
 	"k8s.io/ingress-gce/pkg/context"
+
 	"k8s.io/ingress-gce/pkg/flags"
 	"k8s.io/ingress-gce/pkg/forwardingrules"
 	"k8s.io/ingress-gce/pkg/instancegroups"
@@ -58,6 +59,8 @@ const (
 
 	instanceGroupLink backendLinkType = 0
 	negLink           backendLinkType = 1
+
+	delayDurationForLegacyOwnershipPriority = 5 * time.Second // Configurable via flag if needed
 )
 
 type backendLinkType int64
@@ -86,6 +89,7 @@ type L4NetLBController struct {
 	serviceVersions                    *serviceVersionsTracker
 	enableNEGSupport                   bool
 	enableNEGAsDefault                 bool
+	enableRBSDefault                   bool
 
 	hasSynced func() bool
 
@@ -122,6 +126,7 @@ func NewL4NetLBController(
 		serviceVersions:                    NewServiceVersionsTracker(),
 		logger:                             logger,
 		hasSynced:                          ctx.HasSynced,
+		enableRBSDefault:                   ctx.EnableL4NetLBRBSByDefault,
 	}
 	var networkLister cache.Indexer
 	if ctx.NetworkInformer != nil {
@@ -359,6 +364,7 @@ func (lc *L4NetLBController) shouldProcessService(newSvc, oldSvc *v1.Service, sv
 	warnL4FinalizerRemoved(lc.ctx, oldSvc, newSvc)
 
 	if !lc.isRBSBasedService(newSvc, svcLogger) && !lc.isRBSBasedService(oldSvc, svcLogger) {
+		svcLogger.V(4).Info("Ignoring non RBS based NetLB service")
 		return false, false
 	}
 	if lc.needsAddition(newSvc, oldSvc) || lc.needsUpdate(newSvc, oldSvc) || lc.needsDeletion(newSvc, svcLogger) {
@@ -384,34 +390,95 @@ func (lc *L4NetLBController) isRBSBasedService(svc *v1.Service, svcLogger klog.L
 	// Check if the type=LoadBalancer, so we don't execute API calls o non-LB services
 	// this call is nil-safe
 	if !utils.IsLoadBalancerServiceType(svc) {
+		svcLogger.V(4).Info("Service is not of type LoadBalancer")
 		return false
 	}
 	if svc.Spec.LoadBalancerClass != nil {
+		svcLogger.V(4).Info("Service has LoadBalancerClass annotation", "loadBalancerClass", *svc.Spec.LoadBalancerClass)
 		return annotations.HasLoadBalancerClass(svc, annotations.RegionalExternalLoadBalancerClass)
 	}
-	return annotations.HasRBSAnnotation(svc) || utils.HasL4NetLBFinalizerV2(svc) || utils.HasL4NetLBFinalizerV3(svc) || lc.hasRBSForwardingRule(svc, svcLogger)
+	if lc.enableRBSDefault {
+		svcLogger.V(4).Info("RBS is enabled by default, treating service as RBS based")
+		return true
+	}
+	if utils.HasL4NetLBFinalizerV2(svc) || utils.HasL4NetLBFinalizerV3(svc) {
+		svcLogger.V(4).Info("Service has L4 NetLB RBS finalizer")
+		return true
+	}
+	if annotations.HasRBSAnnotation(svc) {
+		svcLogger.V(4).Info("Service has RBS annotation")
+		return true
+	}
+	if utils.HasL4NetLBFinalizerV1(svc) {
+		svcLogger.V(4).Info("Service has Legacy L4 NetLB finalizer")
+		return false
+	}
+	if lc.hasRBSForwardingRule(svc, svcLogger) {
+		svcLogger.V(4).Info("Service has RBS forwarding rule")
+		return true
+	}
+	return false
+}
+
+func (lc *L4NetLBController) hasLegacyControllerOwnership(svc *v1.Service, svcLogger klog.Logger) bool {
+	// Check for legacy finalizer
+	if utils.HasL4NetLBFinalizerV1(svc) {
+		return true
+	}
+
+	// Check if forwarding rule points to a target pool
+	return lc.hasTargetPoolForwardingRule(svc, svcLogger)
+}
+
+// refreshServiceFromK8s fetches the latest version of the service from Kubernetes
+// This is needed after adding a delay to ensure we have the most up-to-date version
+// which may include finalizers added by other controllers
+func (lc *L4NetLBController) refreshServiceFromK8s(service *v1.Service, svcLogger klog.Logger) *v1.Service {
+	svcLogger.V(3).Info("Refreshing service from Kubernetes",
+		"currentResourceVersion", service.ResourceVersion)
+
+	// Get the latest service from Kubernetes
+	svcKey := utils.ServiceKeyFunc(service.Namespace, service.Name)
+	refreshedService, exists, err := lc.ctx.Services().GetByKey(svcKey)
+	if err != nil {
+		svcLogger.Info("Could not get service from store, using existing one, error: ", err)
+	} else if exists {
+		service = refreshedService
+		svcLogger.Info("finalizer Found service in informer store after wait, using it.")
+	} else {
+		// Service might have been deleted during the wait, return to avoid processing a non-existing service.
+		service = nil
+		svcLogger.Info("Service not found in informer store after wait, ignoring processing.")
+	}
+	return service
 }
 
 func (lc *L4NetLBController) preventLegacyServiceHandling(service *v1.Service, key string, svcLogger klog.Logger) (bool, error) {
-	if (annotations.HasRBSAnnotation(service) || annotations.HasLoadBalancerClass(service, annotations.RegionalExternalLoadBalancerClass)) && lc.hasTargetPoolForwardingRule(service, svcLogger) {
-		if utils.HasL4NetLBFinalizerV2(service) || utils.HasL4NetLBFinalizerV3(service) {
+	svcLogger.Info("Checking for legacy target pool service with RBS annotation or finalizers", "finalizers", service.ObjectMeta.Finalizers)
+	if lc.hasLegacyControllerOwnership(service, svcLogger) {
+		if utils.HasL4NetLBRBSFinalizers(service) {
 			// If we found that RBS finalizer was attached to service, it means that RBS controller
 			// had a race condition on Service creation with Legacy Controller.
 			// It should only happen during service creation, and we should clean up RBS resources
+			svcLogger.Info("Detected Target Pool on RBS service with RBS finalizer. Cleaning up RBS resources to prevent race condition.", "finalizers", service.ObjectMeta.Finalizers)
 			return true, lc.preventTargetPoolRaceWithRBSOnCreation(service, key, svcLogger)
-		} else {
+		} else if annotations.HasRBSAnnotation(service) {
 			// Target Pool to RBS migration is NOT yet supported and causes service to break (for now).
 			// If we detect RBS annotation on legacy service, we remove RBS annotation,
 			// so service stays with Legacy Target Pool implementation
+			svcLogger.Info("Detected Target Pool on service with RBS annotation. Removing RBS annotation to prevent unsupported migration.", "finalizers", service.ObjectMeta.Finalizers)
 			return true, lc.preventExistingTargetPoolToRBSMigration(service, svcLogger)
 		}
+		svcLogger.Info("Service is legacy Target Pool based service. No action needed.", "finalizers", service.ObjectMeta.Finalizers)
+		return true, nil
 	}
 	return false, nil
 }
 
 func (lc *L4NetLBController) hasTargetPoolForwardingRule(service *v1.Service, svcLogger klog.Logger) bool {
 	frName := utils.LegacyForwardingRuleName(service)
 	if lc.hasForwardingRuleAnnotation(service, frName) {
+		svcLogger.V(4).Info("Service does not have Target Pool forwarding rule annotation", "forwardingRule", frName)
 		return false
 	}
 
@@ -421,8 +488,10 @@ func (lc *L4NetLBController) hasTargetPoolForwardingRule(service *v1.Service, sv
 		return false
 	}
 	if existingFR != nil && existingFR.Target != "" {
+		svcLogger.V(4).Info("Service has Target Pool forwarding rule", "forwardingRule", frName, "targetPool", strings.Split(existingFR.Target, "/")[len(strings.Split(existingFR.Target, "/"))-1])
 		return true
 	}
+	svcLogger.V(4).Info("Service does not have Target Pool forwarding rule", "forwardingRule", frName)
 	return false
 }
 
@@ -611,6 +680,12 @@ func (lc *L4NetLBController) syncInternal(service *v1.Service, svcLogger klog.Lo
 		svcLogger.Info("Finished syncing L4 NetLB RBS service", "timeTaken", time.Since(startTime))
 	}()
 
+	// Prevent race condition with legacy controller
+	service = lc.handleCreationRace(service, svcLogger)
+	if service == nil {
+		return nil
+	}
+
 	usesNegBackends := lc.shouldUseNEGBackends(service, svcLogger)
 
 	l4NetLBParams := &l4resources.L4NetLBParams{
@@ -976,3 +1051,36 @@ func (lc *L4NetLBController) publishSyncMetrics(result *l4resources.L4NetLBSyncR
 
 	metrics.PublishNetLBSyncMetrics(result.Error == nil, result.SyncType, result.GCEResourceInError, utils.GetErrorType(result.Error), result.StartTime, isResync, isWeightedLB, result.MetricsState.Protocol, backendType)
 }
+
+// handleCreationRace prevents a race condition between the legacy and new L4 NetLB controllers
+// when a service is created, ensuring the L4 controller processes the most recent service
+// state and avoids conflicting operations..
+func (lc *L4NetLBController) handleCreationRace(service *v1.Service, svcLogger klog.Logger) *v1.Service {
+	l4NetLBLegacyHeadStartTime := flags.F.L4NetLBLegacyHeadStartTime
+	hasL4NetLBRBSFinalizers := utils.HasL4NetLBRBSFinalizers(service)
+	hasLegacyL4NetLBFinalizerV1 := utils.HasL4NetLBFinalizerV1(service)
+
+	if !hasLegacyL4NetLBFinalizerV1 && !hasL4NetLBRBSFinalizers && l4NetLBLegacyHeadStartTime > 0*time.Second {
+		// Add a delay to allow legacy controller to potentially claim the service first
+		svcLogger.Info("Service does not have RBS finalizer yet and RBS is default. Adding delay to allow legacy controller to act first",
+			"delay", delayDurationForLegacyOwnershipPriority)
+		time.Sleep(delayDurationForLegacyOwnershipPriority)
+
+		// Refresh the service from Kubernetes to get the latest version
+		// This is critical because the legacy controller may have added its finalizer (NetLBFinalizerV1)
+		// during the delay period
+		refreshedService := lc.refreshServiceFromK8s(service, svcLogger)
+
+		// Use the refreshed service for the rest of the sync
+		service = refreshedService
+
+		// Also check explicitly for legacy ownership to be extra safe
+		if lc.hasLegacyControllerOwnership(service, svcLogger) {
+			svcLogger.Info("After delay and refresh, detected legacy controller ownership. Skipping RBS sync.")
+			return nil
+		}
+
+		svcLogger.Info("After delay and refresh, proceeding with RBS sync", "resourceVersion", service.ResourceVersion)
+	}
+	return service
+}