refactor: introduce loadBalancer sync operation

This clarifies the flow, laying the groundwork for multiple protocols.

Author: justinsb

providers/gce/BUILD

@@ -26,6 +26,7 @@ go_library(
         "gce_loadbalancer_internal.go",
         "gce_loadbalancer_metrics.go",
         "gce_loadbalancer_naming.go",
+        "gce_loadbalancer_sync.go",
         "gce_networkendpointgroup.go",
         "gce_networks.go",
         "gce_routes.go",

providers/gce/gce_loadbalancer.go

@@ -169,13 +169,15 @@ func (g *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, svc
 
 	klog.V(4).Infof("EnsureLoadBalancer(%v, %v, %v, %v, %v): ensure %v loadbalancer", clusterName, svc.Namespace, svc.Name, loadBalancerName, g.region, desiredScheme)
 
-	existingFwdRule, err := g.GetRegionForwardingRule(loadBalancerName, g.region)
+	actualForwardingRule, err := g.GetRegionForwardingRule(loadBalancerName, g.region)
 	if err != nil && !isNotFound(err) {
 		return nil, err
 	}
+	op := &loadBalancerSync{}
+	op.actualForwardingRule = actualForwardingRule
 
-	if existingFwdRule != nil {
-		existingScheme := cloud.LbScheme(strings.ToUpper(existingFwdRule.LoadBalancingScheme))
+	if op.actualForwardingRule != nil {
+		existingScheme := cloud.LbScheme(strings.ToUpper(op.actualForwardingRule.LoadBalancingScheme))
 
 		// If the loadbalancer type changes between INTERNAL and EXTERNAL, the old load balancer should be deleted.
 		if existingScheme != desiredScheme {
@@ -192,16 +194,16 @@ func (g *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, svc
 			}
 
 			// Assume the ensureDeleted function successfully deleted the forwarding rule.
-			existingFwdRule = nil
+			op.actualForwardingRule = nil
 		}
 	}
 
 	var status *v1.LoadBalancerStatus
 	switch desiredScheme {
 	case cloud.SchemeInternal:
-		status, err = g.ensureInternalLoadBalancer(clusterName, clusterID, svc, existingFwdRule, nodes)
+		status, err = g.ensureInternalLoadBalancer(clusterName, clusterID, svc, op, nodes)
 	default:
-		status, err = g.ensureExternalLoadBalancer(clusterName, clusterID, svc, existingFwdRule, nodes)
+		status, err = g.ensureExternalLoadBalancer(clusterName, clusterID, svc, op, nodes)
 	}
 	if err != nil {
 		klog.Errorf("Failed to EnsureLoadBalancer(%s, %s, %s, %s, %s), err: %v", clusterName, svc.Namespace, svc.Name, loadBalancerName, g.region, err)

providers/gce/gce_loadbalancer_external.go

@@ -55,11 +55,11 @@ const (
 // Due to an interesting series of design decisions, this handles both creating
 // new load balancers and updating existing load balancers, recognizing when
 // each is needed.
-func (g *Cloud) ensureExternalLoadBalancer(clusterName string, clusterID string, apiService *v1.Service, existingFwdRule *compute.ForwardingRule, nodes []*v1.Node) (*v1.LoadBalancerStatus, error) {
+func (g *Cloud) ensureExternalLoadBalancer(clusterName string, clusterID string, apiService *v1.Service, op *loadBalancerSync, nodes []*v1.Node) (*v1.LoadBalancerStatus, error) {
 	// Process services with LoadBalancerClass "networking.gke.io/l4-regional-external-legacy" used for this controller.
 	// LoadBalancerClass can't be updated so we know this controller should process the NetLB.
 	// Skip service handling if it uses Regional Backend Services and handled by other controllers
-	if !shouldProcessNetLB(apiService, existingFwdRule) {
+	if !shouldProcessNetLB(apiService, op.actualForwardingRule) {
 		return nil, cloudprovider.ImplementedElsewhere
 	}
 

providers/gce/gce_loadbalancer_external_test.go

@@ -1055,7 +1055,10 @@ func TestEnsureExternalLoadBalancerExistingFwdRule(t *testing.T) {
 			svc, err = gce.client.CoreV1().Services(svc.Namespace).Create(context.TODO(), svc, metav1.CreateOptions{})
 			require.NoError(t, err)
 
-			_, err = gce.ensureExternalLoadBalancer(vals.ClusterName, vals.ClusterID, svc, tc.existingForwardingRule, nodes)
+			op := &loadBalancerSync{}
+			op.actualForwardingRule = tc.existingForwardingRule
+
+			_, err = gce.ensureExternalLoadBalancer(vals.ClusterName, vals.ClusterID, svc, op, nodes)
 			if tc.wantError != nil {
 				assert.EqualError(t, err, (*tc.wantError).Error())
 			} else {
@@ -2121,11 +2124,15 @@ func TestEnsureExternalLoadBalancerErrors(t *testing.T) {
 			if tc.injectMock != nil {
 				tc.injectMock(gce.c.(*cloud.MockGCE))
 			}
+
+			op := &loadBalancerSync{}
+			op.actualForwardingRule = params.existingFwdRule
+
 			status, err := gce.ensureExternalLoadBalancer(
 				params.clusterName,
 				params.clusterID,
 				params.service,
-				params.existingFwdRule,
+				op,
 				params.nodes,
 			)
 			assert.Error(t, err, "Should return an error when "+desc)

providers/gce/gce_loadbalancer_internal.go

@@ -54,10 +54,10 @@ const (
 	labelGKESubnetworkName = "cloud.google.com/gke-node-pool-subnet"
 )
 
-func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v1.Service, existingFwdRule *compute.ForwardingRule, nodes []*v1.Node) (*v1.LoadBalancerStatus, error) {
+func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v1.Service, op *loadBalancerSync, nodes []*v1.Node) (*v1.LoadBalancerStatus, error) {
 	// Process services with LoadBalancerClass "networking.gke.io/l4-regional-internal-legacy" used for this controller.
 	// LoadBalancerClass can't be updated so we know this controller should process the ILB.
-	if existingFwdRule == nil && !hasFinalizer(svc, ILBFinalizerV1) && !hasLoadBalancerClass(svc, LegacyRegionalInternalLoadBalancerClass) {
+	if op.actualForwardingRule == nil && !hasFinalizer(svc, ILBFinalizerV1) && !hasLoadBalancerClass(svc, LegacyRegionalInternalLoadBalancerClass) {
 		// Neither the forwarding rule nor the V1 finalizer exists. This is most likely a new service.
 		if svc.Spec.LoadBalancerClass != nil && !hasLoadBalancerClass(svc, LegacyRegionalInternalLoadBalancerClass) {
 			klog.V(2).Infof("Skipped ensureInternalLoadBalancer for service %s/%s, as service contains %q loadBalancerClass.", svc.Namespace, svc.Name, *svc.Spec.LoadBalancerClass)
@@ -118,8 +118,8 @@ func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v
 
 	// Get existing backend service (if exists)
 	var existingBackendService *compute.BackendService
-	if existingFwdRule != nil && existingFwdRule.BackendService != "" {
-		existingBSName := getNameFromLink(existingFwdRule.BackendService)
+	if op.actualForwardingRule != nil && op.actualForwardingRule.BackendService != "" {
+		existingBSName := getNameFromLink(op.actualForwardingRule.BackendService)
 		if existingBackendService, err = g.GetRegionBackendService(existingBSName, g.region); err != nil && !isNotFound(err) {
 			return nil, err
 		}
@@ -153,7 +153,7 @@ func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v
 	}
 	// Determine IP which will be used for this LB. If no forwarding rule has been established
 	// or specified in the Service spec, then requestedIP = "".
-	ipToUse := ilbIPToUse(svc, existingFwdRule, subnetworkURL)
+	ipToUse := ilbIPToUse(svc, op.actualForwardingRule, subnetworkURL)
 
 	klog.V(2).Infof("ensureInternalLoadBalancer(%v): Using subnet %s for LoadBalancer IP %s", loadBalancerName, options.SubnetName, ipToUse)
 
@@ -199,20 +199,20 @@ func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v
 		newFwdRule.Ports = nil
 		newFwdRule.AllPorts = true
 	}
+	op.desiredForwardingRule = newFwdRule
 
-	fwdRuleDeleted := false
-	if existingFwdRule != nil && !forwardingRulesEqual(existingFwdRule, newFwdRule) {
+	if op.actualForwardingRule != nil && !forwardingRulesEqual(op.actualForwardingRule, op.desiredForwardingRule) {
 		// Delete existing forwarding rule before making changes to the backend service. For example - changing protocol
 		// of backend service without first deleting forwarding rule will throw an error since the linked forwarding
 		// rule would show the old protocol.
 		if klogV := klog.V(2); klogV.Enabled() {
-			frDiff := cmp.Diff(existingFwdRule, newFwdRule)
-			klogV.Infof("ensureInternalLoadBalancer(%v): forwarding rule changed - Existing - %+v\n, New - %+v\n, Diff(-existing, +new) - %s\n. Deleting existing forwarding rule.", loadBalancerName, existingFwdRule, newFwdRule, frDiff)
+			frDiff := cmp.Diff(op.actualForwardingRule, op.desiredForwardingRule)
+			klogV.Infof("ensureInternalLoadBalancer(%v): forwarding rule changed - Existing - %+v\n, New - %+v\n, Diff(-existing, +new) - %s\n. Deleting existing forwarding rule.", loadBalancerName, op.actualForwardingRule, op.desiredForwardingRule, frDiff)
 		}
 		if err = ignoreNotFound(g.DeleteRegionForwardingRule(loadBalancerName, g.region)); err != nil {
 			return nil, err
 		}
-		fwdRuleDeleted = true
+		op.actualForwardingRule = nil
 	}
 
 	bsDescription := makeBackendServiceDescription(nm, sharedBackend)
@@ -221,9 +221,9 @@ func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v
 		return nil, err
 	}
 
-	if fwdRuleDeleted || existingFwdRule == nil {
-		// existing rule has been deleted, pass in nil
-		if err := g.ensureInternalForwardingRule(nil, newFwdRule); err != nil {
+	if op.actualForwardingRule == nil {
+		// existing rule has been deleted
+		if err := g.ensureInternalForwardingRule(op); err != nil {
 			return nil, err
 		}
 	}
@@ -1084,24 +1084,24 @@ func getFwdRuleAPIVersion(rule *compute.ForwardingRule) (meta.Version, error) {
 	return d.APIVersion, nil
 }
 
-func (g *Cloud) ensureInternalForwardingRule(existingFwdRule, newFwdRule *compute.ForwardingRule) (err error) {
-	if existingFwdRule != nil {
-		if forwardingRulesEqual(existingFwdRule, newFwdRule) {
-			klog.V(4).Infof("existingFwdRule == newFwdRule, no updates needed (existingFwdRule == %+v)", existingFwdRule)
+func (g *Cloud) ensureInternalForwardingRule(op *loadBalancerSync) (err error) {
+	if op.actualForwardingRule != nil {
+		if forwardingRulesEqual(op.actualForwardingRule, op.desiredForwardingRule) {
+			klog.V(4).Infof("actualForwardingRule == desiredForwardingRule, no updates needed (actualForwardingRule == %+v)", op.actualForwardingRule)
 			return nil
 		}
-		klog.V(2).Infof("ensureInternalLoadBalancer(%v): deleting existing forwarding rule with IP address %v", existingFwdRule.Name, existingFwdRule.IPAddress)
-		if err = ignoreNotFound(g.DeleteRegionForwardingRule(existingFwdRule.Name, g.region)); err != nil {
+		klog.V(2).Infof("ensureInternalLoadBalancer(%v): deleting existing forwarding rule with IP address %v", op.actualForwardingRule.Name, op.actualForwardingRule.IPAddress)
+		if err = ignoreNotFound(g.DeleteRegionForwardingRule(op.actualForwardingRule.Name, g.region)); err != nil {
 			return err
 		}
 	}
 	// At this point, the existing rule has been deleted if required.
 	// Create the rule based on the api version determined
-	klog.V(2).Infof("ensureInternalLoadBalancer(%v): creating forwarding rule", newFwdRule.Name)
-	if err = g.CreateRegionForwardingRule(newFwdRule, g.region); err != nil {
+	klog.V(2).Infof("ensureInternalLoadBalancer(%v): creating forwarding rule", op.desiredForwardingRule.Name)
+	if err = g.CreateRegionForwardingRule(op.desiredForwardingRule, g.region); err != nil {
 		return err
 	}
-	klog.V(2).Infof("ensureInternalLoadBalancer(%v): created forwarding rule", newFwdRule.Name)
+	klog.V(2).Infof("ensureInternalLoadBalancer(%v): created forwarding rule", op.desiredForwardingRule.Name)
 	return nil
 }
 

providers/gce/gce_loadbalancer_internal_test.go

@@ -48,11 +48,14 @@ func createInternalLoadBalancer(gce *Cloud, svc *v1.Service, existingFwdRule *co
 		return nil, err
 	}
 
+	op := &loadBalancerSync{}
+	op.actualForwardingRule = existingFwdRule
+
 	return gce.ensureInternalLoadBalancer(
 		clusterName,
 		clusterID,
 		svc,
-		existingFwdRule,
+		op,
 		nodes,
 	)
 }
@@ -660,7 +663,10 @@ func TestUpdateInternalLoadBalancerNodes(t *testing.T) {
 	nodes, err := createAndInsertNodes(gce, node1Name, vals.ZoneName)
 	require.NoError(t, err)
 
-	_, err = gce.ensureInternalLoadBalancer(vals.ClusterName, vals.ClusterID, svc, nil, nodes)
+	op := &loadBalancerSync{}
+	op.actualForwardingRule = nil
+
+	_, err = gce.ensureInternalLoadBalancer(vals.ClusterName, vals.ClusterID, svc, op, nodes)
 	assert.NoError(t, err)
 
 	// Replace the node in initial zone; add new node in a new zone.
@@ -729,7 +735,10 @@ func TestUpdateInternalLoadBalancerNodesWithEmptyZone(t *testing.T) {
 	nodes, err := createAndInsertNodes(gce, node1Name, vals.ZoneName)
 	require.NoError(t, err)
 
-	_, err = gce.ensureInternalLoadBalancer(vals.ClusterName, vals.ClusterID, svc, nil, nodes)
+	op := &loadBalancerSync{}
+	op.actualForwardingRule = nil
+
+	_, err = gce.ensureInternalLoadBalancer(vals.ClusterName, vals.ClusterID, svc, op, nodes)
 	assert.NoError(t, err)
 
 	// Ensure Node has been added to instance group
@@ -747,7 +756,7 @@ func TestUpdateInternalLoadBalancerNodesWithEmptyZone(t *testing.T) {
 	nodes[0].Labels[v1.LabelTopologyZone] = "" // empty zone
 
 	lbName := gce.GetLoadBalancerName(context.TODO(), "", svc)
-	existingFwdRule := &compute.ForwardingRule{
+	op.actualForwardingRule = &compute.ForwardingRule{
 		Name:                lbName,
 		IPAddress:           "",
 		Ports:               []string{"123"},
@@ -756,7 +765,7 @@ func TestUpdateInternalLoadBalancerNodesWithEmptyZone(t *testing.T) {
 		Description:         fmt.Sprintf(`{"kubernetes.io/service-name":"%s"}`, types.NamespacedName{Name: svc.Name, Namespace: svc.Namespace}.String()),
 	}
 
-	_, err = gce.ensureInternalLoadBalancer(vals.ClusterName, vals.ClusterID, svc, existingFwdRule, nodes)
+	_, err = gce.ensureInternalLoadBalancer(vals.ClusterName, vals.ClusterID, svc, op, nodes)
 	assert.NoError(t, err)
 
 	// Expect load balancer to not have deleted node test-node-1
@@ -1232,11 +1241,15 @@ func TestEnsureInternalLoadBalancerErrors(t *testing.T) {
 			}
 			_, err = gce.client.CoreV1().Services(params.service.Namespace).Create(context.TODO(), params.service, metav1.CreateOptions{})
 			require.NoError(t, err)
+
+			op := &loadBalancerSync{}
+			op.actualForwardingRule = params.existingFwdRule
+
 			status, err := gce.ensureInternalLoadBalancer(
 				params.clusterName,
 				params.clusterID,
 				params.service,
-				params.existingFwdRule,
+				op,
 				params.nodes,
 			)
 			assert.Error(t, err, "Should return an error when "+desc)

providers/gce/gce_loadbalancer_sync.go

@@ -0,0 +1,29 @@
+//go:build !providerless
+// +build !providerless
+
+/*
+Copyright 2025 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package gce
+
+import compute "google.golang.org/api/compute/v1"
+
+// loadBalancerSync is a struct that tracks the desired and actual state of the GCP resources
+// that back the load balancer.  It is used to drive the actual state to the desired state.
+type loadBalancerSync struct {
+	desiredForwardingRule *compute.ForwardingRule
+	actualForwardingRule  *compute.ForwardingRule
+}