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

Author: 08volt

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/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
+}

pkg/l4lb/l4netlbcontroller_test.go

@@ -311,7 +311,7 @@ func getFakeGCECloud(vals gce.TestClusterValues) *gce.Cloud {
 	return fakeGCE
 }
 
-func buildContext(vals gce.TestClusterValues, readOnlyMode bool) (*ingctx.ControllerContext, error) {
+func buildContext(vals gce.TestClusterValues, readOnlyMode bool, isRBSdefault bool) (*ingctx.ControllerContext, error) {
 	fakeGCE := getFakeGCECloud(vals)
 	kubeClient := fake.NewSimpleClientset()
 	networkClient := netfake.NewSimpleClientset()
@@ -332,12 +332,20 @@ func buildContext(vals gce.TestClusterValues, readOnlyMode bool) (*ingctx.Contro
 }
 
 func newL4NetLBServiceController() *L4NetLBController {
-	return createL4NetLBServiceController(test.DefaultTestClusterValues(), false)
+	return createL4NetLBServiceController(test.DefaultTestClusterValues(), false, false)
 }
 
-func createL4NetLBServiceController(vals gce.TestClusterValues, readOnlyMode bool) *L4NetLBController {
+func newL4NetLBServiceControllerReadOnlyMode(readOnlyMode bool) *L4NetLBController {
+	return createL4NetLBServiceController(test.DefaultTestClusterValues(), readOnlyMode, false)
+}
+
+func newL4NetLBServiceControllerRBSDefault(isRBSdefault bool) *L4NetLBController {
+	return createL4NetLBServiceController(test.DefaultTestClusterValues(), false, isRBSdefault)
+}
+
+func createL4NetLBServiceController(vals gce.TestClusterValues, readOnlyMode bool, isRBSdefault bool) *L4NetLBController {
 	stopCh := make(chan struct{})
-	ctx, err := buildContext(vals, readOnlyMode)
+	ctx, err := buildContext(vals, readOnlyMode, isRBSdefault)
 	if err != nil {
 		klog.Fatalf("Failed to build context: %v", err)
 	}
@@ -1667,6 +1675,7 @@ func TestIsRBSBasedService(t *testing.T) {
 		finalizers       []string
 		annotations      map[string]string
 		frHook           getForwardingRuleHook
+		isRBSDefault     bool
 		expectRBSService bool
 	}{
 		{
@@ -1703,13 +1712,30 @@ func TestIsRBSBasedService(t *testing.T) {
 			frHook:           test.GetLegacyForwardingRule,
 			expectRBSService: false,
 		},
+		{
+			desc:             "Should detect RBS by default",
+			isRBSDefault:     true,
+			expectRBSService: true,
+		},
+		{
+			desc:             "Should not detect RBS by forwarding rule pointed to target pool, even if RBS is default",
+			frHook:           test.GetLegacyForwardingRule,
+			isRBSDefault:     true,
+			expectRBSService: false,
+		},
+		{
+			desc:             "Legacy service should not be marked as RBS, even if RBS is default",
+			finalizers:       []string{common.NetLBFinalizerV1},
+			isRBSDefault:     true,
+			expectRBSService: false,
+		},
 	}
 
 	for _, testCase := range testCases {
 		t.Run(testCase.desc, func(t *testing.T) {
 			// Setup
 			svc := test.NewL4LegacyNetLBService(8080, 30234)
-			controller := newL4NetLBServiceController()
+			controller := newL4NetLBServiceControllerRBSDefault(testCase.isRBSDefault)
 			svc.Annotations = testCase.annotations
 			svc.ObjectMeta.Finalizers = testCase.finalizers
 			controller.ctx.Cloud.Compute().(*cloud.MockGCE).MockForwardingRules.GetHook = testCase.frHook
@@ -2318,7 +2344,7 @@ func TestEnsureExternalLoadBalancerClass(t *testing.T) {
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
-			lc := createL4NetLBServiceController(test.DefaultTestClusterValues(), false)
+			lc := newL4NetLBServiceController()
 
 			svc := test.NewL4LBServiceWithLoadBalancerClass(tc.loadBalancerClass)
 			if tc.loadBalancerClass == "" {
@@ -2421,7 +2447,7 @@ func TestEnsureReadOnlyModeDoesNotProvision(t *testing.T) {
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
-			lc := createL4NetLBServiceController(test.DefaultTestClusterValues(), tc.readOnlyModeEnabled)
+			lc := newL4NetLBServiceControllerReadOnlyMode(tc.readOnlyModeEnabled)
 
 			svc := test.NewL4LBServiceWithLoadBalancerClass(tc.loadBalancerClass)
 			if tc.loadBalancerClass == "" {

pkg/utils/common/finalizer.go

@@ -40,6 +40,8 @@ const (
 	ILBFinalizerV2 = "gke.networking.io/l4-ilb-v2"
 	// NegFinalizerKey is the finalizer used by neg controller to ensure NEG CRs are deleted after corresponding negs are deleted
 	NegFinalizerKey = "networking.gke.io/neg-finalizer"
+	// NetLBFinalizerV1 is the finalizer used by legacy cloud controller manager that manage L4 External LoadBalancer services.
+	NetLBFinalizerV1 = "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"
 	// NetLBFinalizerV3 is the finalizer used by the NEG backed variant of the L4 External LoadBalancer services.

pkg/utils/utils.go

@@ -761,6 +761,16 @@ func HasL4ILBFinalizerV2(svc *api_v1.Service) bool {
 	return slice.ContainsString(svc.ObjectMeta.Finalizers, common.ILBFinalizerV2, nil)
 }
 
+// HasL4NetLBFinalizerV1 returns true if the given Service has NetLBFinalizerV1
+func HasL4NetLBFinalizerV1(svc *api_v1.Service) bool {
+	return slice.ContainsString(svc.ObjectMeta.Finalizers, common.NetLBFinalizerV1, nil)
+}
+
+// HasL4NetLBRBSFinalizers returns true if the given Service has any of the NetLBRBS finalizers v2 or v3
+func HasL4NetLBRBSFinalizers(svc *api_v1.Service) bool {
+	return HasL4NetLBFinalizerV2(svc) || HasL4NetLBFinalizerV3(svc)
+}
+
 // HasL4NetLBFinalizerV2 returns true if the given Service has NetLBFinalizerV2
 func HasL4NetLBFinalizerV2(svc *api_v1.Service) bool {
 	return slice.ContainsString(svc.ObjectMeta.Finalizers, common.NetLBFinalizerV2, nil)