fixup! controller support for mixed protocol LB services

Author: justinsb

providers/gce/gce_loadbalancer.go

@@ -145,28 +145,28 @@ func (g *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, svc
 		return nil, err
 	}
 
-	// Services with multiples protocols are not supported by this controller, warn the users and sets
-	// the corresponding Service Status Condition.
+	// Services with multiples protocols are not supported by this controller (without AlphaFeatureMultiProtocolLB),
+	// warn the users and set the corresponding Service Status Condition.
 	// https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1435-mixed-protocol-lb
-	if !g.AlphaFeatureGate.Enabled(AlphaFeatureMultiProtocolLB) {
-		if err := checkMixedProtocol(svc.Spec.Ports); err != nil {
-			if hasLoadBalancerPortsError(svc) {
-				return nil, err
-			}
-			klog.Warningf("Ignoring service %s/%s using different ports protocols", svc.Namespace, svc.Name)
-			g.eventRecorder.Event(svc, v1.EventTypeWarning, v1.LoadBalancerPortsErrorReason, "LoadBalancers with multiple protocols are not supported.")
-			svcApplyStatus := corev1apply.ServiceStatus().WithConditions(
-				metav1apply.Condition().
-					WithType(v1.LoadBalancerPortsError).
-					WithStatus(metav1.ConditionTrue).
-					WithReason(v1.LoadBalancerPortsErrorReason).
-					WithMessage("LoadBalancer with multiple protocols are not supported"))
-			svcApply := corev1apply.Service(svc.Name, svc.Namespace).WithStatus(svcApplyStatus)
-			if _, errApply := g.client.CoreV1().Services(svc.Namespace).ApplyStatus(ctx, svcApply, metav1.ApplyOptions{FieldManager: "gce-cloud-controller", Force: true}); errApply != nil {
-				return nil, errApply
-			}
+	if g.AlphaFeatureGate.Enabled(AlphaFeatureMultiProtocolLB) {
+		klog.Infof("AlphaFeatureMultiProtocolLB feature gate is enabled")
+	} else if err := checkMixedProtocol(svc.Spec.Ports); err != nil {
+		if hasLoadBalancerPortsError(svc) {
 			return nil, err
 		}
+		klog.Warningf("Ignoring service %s/%s using different ports protocols", svc.Namespace, svc.Name)
+		g.eventRecorder.Event(svc, v1.EventTypeWarning, v1.LoadBalancerPortsErrorReason, "LoadBalancers with multiple protocols are not supported.")
+		svcApplyStatus := corev1apply.ServiceStatus().WithConditions(
+			metav1apply.Condition().
+				WithType(v1.LoadBalancerPortsError).
+				WithStatus(metav1.ConditionTrue).
+				WithReason(v1.LoadBalancerPortsErrorReason).
+				WithMessage("LoadBalancer with multiple protocols are not supported"))
+		svcApply := corev1apply.Service(svc.Name, svc.Namespace).WithStatus(svcApplyStatus)
+		if _, errApply := g.client.CoreV1().Services(svc.Namespace).ApplyStatus(ctx, svcApply, metav1.ApplyOptions{FieldManager: "gce-cloud-controller", Force: true}); errApply != nil {
+			return nil, errApply
+		}
+		return nil, err
 	}
 
 	klog.V(4).Infof("EnsureLoadBalancer(%v, %v, %v, %v, %v): ensure %v loadbalancer", clusterName, svc.Namespace, svc.Name, loadBalancerName, g.region, desiredScheme)
@@ -230,24 +230,24 @@ func (g *Cloud) UpdateLoadBalancer(ctx context.Context, clusterName string, svc
 		return err
 	}
 
-	// Services with multiples protocols are not supported by this controller, warn the users and sets
+	// Services with multiples protocols are not supported by this controller (without AlphaFeatureMultiProtocolLB), warn the users and sets
 	// the corresponding Service Status Condition, but keep processing the Update to not break upgrades.
 	// https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1435-mixed-protocol-lb
-	if !g.AlphaFeatureGate.Enabled(AlphaFeatureMultiProtocolLB) {
-		if err := checkMixedProtocol(svc.Spec.Ports); err != nil && !hasLoadBalancerPortsError(svc) {
-			klog.Warningf("Ignoring update for service %s/%s using different ports protocols", svc.Namespace, svc.Name)
-			g.eventRecorder.Event(svc, v1.EventTypeWarning, v1.LoadBalancerPortsErrorReason, "LoadBalancer with multiple protocols are not supported.")
-			svcApplyStatus := corev1apply.ServiceStatus().WithConditions(
-				metav1apply.Condition().
-					WithType(v1.LoadBalancerPortsError).
-					WithStatus(metav1.ConditionTrue).
-					WithReason(v1.LoadBalancerPortsErrorReason).
-					WithMessage("LoadBalancer with multiple protocols are not supported"))
-			svcApply := corev1apply.Service(svc.Name, svc.Namespace).WithStatus(svcApplyStatus)
-			if _, errApply := g.client.CoreV1().Services(svc.Namespace).ApplyStatus(ctx, svcApply, metav1.ApplyOptions{FieldManager: "gce-cloud-controller", Force: true}); errApply != nil {
-				// the error is retried by the controller loop
-				return errApply
-			}
+	if g.AlphaFeatureGate.Enabled(AlphaFeatureMultiProtocolLB) {
+		klog.Infof("AlphaFeatureMultiProtocolLB feature gate is enabled")
+	} else if err := checkMixedProtocol(svc.Spec.Ports); err != nil && !hasLoadBalancerPortsError(svc) {
+		klog.Warningf("Ignoring update for service %s/%s using different ports protocols", svc.Namespace, svc.Name)
+		g.eventRecorder.Event(svc, v1.EventTypeWarning, v1.LoadBalancerPortsErrorReason, "LoadBalancer with multiple protocols are not supported.")
+		svcApplyStatus := corev1apply.ServiceStatus().WithConditions(
+			metav1apply.Condition().
+				WithType(v1.LoadBalancerPortsError).
+				WithStatus(metav1.ConditionTrue).
+				WithReason(v1.LoadBalancerPortsErrorReason).
+				WithMessage("LoadBalancer with multiple protocols are not supported"))
+		svcApply := corev1apply.Service(svc.Name, svc.Namespace).WithStatus(svcApplyStatus)
+		if _, errApply := g.client.CoreV1().Services(svc.Namespace).ApplyStatus(ctx, svcApply, metav1.ApplyOptions{FieldManager: "gce-cloud-controller", Force: true}); errApply != nil {
+			// the error is retried by the controller loop
+			return errApply
 		}
 	}
 

providers/gce/gce_loadbalancer_external.go

@@ -52,9 +52,9 @@ const (
 // IP address, a firewall rule, a target pool, and a forwarding rule. This
 // function has to manage all of them.
 //
-// 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.
+// This function handles both creating new load balancers and updating existing load balancers,
+// recognizing when each is needed.
+// This approach is resilient, for example if we are interrupted part-way during creation.
 func (g *Cloud) ensureExternalLoadBalancer(clusterName string, clusterID string, apiService *v1.Service, existingFwdRule *compute.ForwardingRule, 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.
@@ -112,134 +112,23 @@ func (g *Cloud) ensureExternalLoadBalancer(clusterName string, clusterID string,
 		g.deleteWrongNetworkTieredResources(loadBalancerName, lbRefStr, netTier)
 	}
 
-	// If the feature gate is not enabled, we check for multiple protocols and error out.
-	if !g.AlphaFeatureGate.Enabled(AlphaFeatureMultiProtocolLB) {
-		fwdRuleExists, fwdRuleNeedsUpdate, fwdRuleIP, err := g.forwardingRuleNeedsUpdate(loadBalancerName, g.region, requestedIP, ports)
-		if err != nil {
-			return nil, err
-		}
-		if !fwdRuleExists {
-			klog.V(2).Infof("ensureExternalLoadBalancer(%s): Forwarding rule %v doesn't exist.", lbRefStr, loadBalancerName)
-		}
-		// Single-protocol logic
-		ipAddressToUse, isSafeToReleaseIP, err := g.ensureIPAddress(loadBalancerName, lbRefStr, apiService.Spec.LoadBalancerIP, fwdRuleIP, netTier)
-		if err != nil {
-			return nil, err
-		}
-		defer func() {
-			if isSafeToReleaseIP {
-				if err := g.DeleteRegionAddress(loadBalancerName, g.region); err != nil && !isNotFound(err) {
-					klog.Errorf("ensureExternalLoadBalancer(%s): Failed to release static IP %s in region %v: %v.", lbRefStr, ipAddressToUse, g.region, err)
-				} else if isNotFound(err) {
-					klog.V(2).Infof("ensureExternalLoadBalancer(%s): IP address %s is not reserved.", lbRefStr, ipAddressToUse)
-				} else {
-					klog.Infof("ensureExternalLoadBalancer(%s): Released static IP %s.", lbRefStr, ipAddressToUse)
-				}
-			} else {
-				klog.Warningf("ensureExternalLoadBalancer(%s): Orphaning static IP %s in region %v: %v.", lbRefStr, ipAddressToUse, g.region, err)
-			}
-		}()
-
-		sourceRanges, err := servicehelpers.GetLoadBalancerSourceRanges(apiService)
-		if err != nil {
-			return nil, err
-		}
-
-		firewallExists, firewallNeedsUpdate, err := g.firewallNeedsUpdate(loadBalancerName, serviceName.String(), ipAddressToUse, ports, sourceRanges)
-		if err != nil {
-			return nil, err
-		}
-
-		if firewallNeedsUpdate {
-			desc := makeFirewallDescription(serviceName.String(), ipAddressToUse)
-			if firewallExists {
-				klog.Infof("ensureExternalLoadBalancer(%s): Updating firewall.", lbRefStr)
-				if err := g.updateFirewall(apiService, MakeFirewallName(loadBalancerName), desc, ipAddressToUse, sourceRanges, ports, hosts); err != nil {
-					return nil, err
-				}
-				klog.Infof("ensureExternalLoadBalancer(%s): Updated firewall.", lbRefStr)
-			} else {
-				klog.Infof("ensureExternalLoadBalancer(%s): Creating firewall.", lbRefStr)
-				if err := g.createFirewall(apiService, MakeFirewallName(loadBalancerName), desc, ipAddressToUse, sourceRanges, ports, hosts); err != nil {
-					return nil, err
-				}
-				klog.Infof("ensureExternalLoadBalancer(%s): Created firewall.", lbRefStr)
-			}
-		}
-
-		tpExists, tpNeedsRecreation, err := g.targetPoolNeedsRecreation(loadBalancerName, g.region, apiService.Spec.SessionAffinity)
-		if err != nil {
-			return nil, err
-		}
-		if !tpExists {
-			klog.Infof("ensureExternalLoadBalancer(%s): Target pool for service doesn't exist.", lbRefStr)
-		}
-
-		var hcToCreate, hcToDelete *compute.HttpHealthCheck
-		hcLocalTrafficExisting, err := g.GetHTTPHealthCheck(loadBalancerName)
-		if err != nil && !isHTTPErrorCode(err, http.StatusNotFound) {
-			return nil, fmt.Errorf("error checking HTTP health check for load balancer (%s): %v", lbRefStr, err)
-		}
-		if path, healthCheckNodePort := servicehelpers.GetServiceHealthCheckPathPort(apiService); path != "" {
-			klog.V(4).Infof("ensureExternalLoadBalancer(%s): Service needs local traffic health checks on: %d%s.", lbRefStr, healthCheckNodePort, path)
-			if hcLocalTrafficExisting == nil {
-				klog.V(2).Infof("ensureExternalLoadBalancer(%s): Updating from nodes health checks to local traffic health checks.", lbRefStr)
-				hcToDelete = makeHTTPHealthCheck(MakeNodesHealthCheckName(clusterID), GetNodesHealthCheckPath(), GetNodesHealthCheckPort())
-				tpNeedsRecreation = true
-			}
-			hcToCreate = makeHTTPHealthCheck(loadBalancerName, path, healthCheckNodePort)
-		} else {
-			klog.V(4).Infof("ensureExternalLoadBalancer(%s): Service needs nodes health checks.", lbRefStr)
-			if hcLocalTrafficExisting != nil {
-				klog.V(2).Infof("ensureExternalLoadBalancer(%s): Updating from local traffic health checks to nodes health checks.", lbRefStr)
-				hcToDelete = hcLocalTrafficExisting
-				tpNeedsRecreation = true
-			}
-			hcToCreate = makeHTTPHealthCheck(MakeNodesHealthCheckName(clusterID), GetNodesHealthCheckPath(), GetNodesHealthCheckPort())
-		}
-
-		if fwdRuleExists && (fwdRuleNeedsUpdate || tpNeedsRecreation) {
-			isSafeToReleaseIP = false
-			if err := g.DeleteRegionForwardingRule(loadBalancerName, g.region); err != nil && !isNotFound(err) {
-				return nil, fmt.Errorf("failed to delete existing forwarding rule for load balancer (%s) update: %v", lbRefStr, err)
-			}
-			klog.Infof("ensureExternalLoadBalancer(%s): Deleted forwarding rule.", lbRefStr)
-		}
-
-		if err := g.ensureTargetPoolAndHealthCheck(tpExists, tpNeedsRecreation, apiService, loadBalancerName, clusterID, ipAddressToUse, hosts, hcToCreate, hcToDelete); err != nil {
-			return nil, err
-		}
-
-		if tpNeedsRecreation || fwdRuleNeedsUpdate {
-			klog.Infof("ensureExternalLoadBalancer(%s): Creating forwarding rule, IP %s (tier: %s).", lbRefStr, ipAddressToUse, netTier)
-			if err := createForwardingRule(g, loadBalancerName, serviceName.String(), g.region, ipAddressToUse, g.targetPoolURL(loadBalancerName), ports, netTier, g.enableDiscretePortForwarding); err != nil {
-				return nil, fmt.Errorf("failed to create forwarding rule for load balancer (%s): %v", lbRefStr, err)
-			}
-			isSafeToReleaseIP = true
-			klog.Infof("ensureExternalLoadBalancer(%s): Created forwarding rule, IP %s.", lbRefStr, ipAddressToUse)
-		}
-
-		status := &v1.LoadBalancerStatus{}
-		status.Ingress = []v1.LoadBalancerIngress{{IP: ipAddressToUse}}
-
-		return status, nil
-	}
-
 	// Multi-protocol logic starts here
 	groupedPorts := groupPortsByProtocol(ports)
 	var fwdRuleIP string
 
 	// We need to determine the IP address for all forwarding rules.
 	// We check if any of the forwarding rules already exist and use their IP.
-	for protocol := range groupedPorts {
-		frName := g.getProtocolForwardingRuleName(loadBalancerName, protocol)
-		fwd, err := g.GetRegionForwardingRule(frName, g.region)
-		if err != nil && !isNotFound(err) {
-			return nil, err
-		}
-		if fwd != nil {
-			fwdRuleIP = fwd.IPAddress
-			break
+	if !g.AlphaFeatureGate.Enabled(AlphaFeatureMultiProtocolLB) {
+		for protocol := range groupedPorts {
+			frName := g.getProtocolForwardingRuleName(loadBalancerName, protocol)
+			fwd, err := g.GetRegionForwardingRule(frName, g.region)
+			if err != nil && !isNotFound(err) {
+				return nil, err
+			}
+			if fwd != nil {
+				fwdRuleIP = fwd.IPAddress
+				break
+			}
 		}
 	}
 	// If no forwarding rule exists, check for the old one.
@@ -456,7 +345,6 @@ func (g *Cloud) ensureIPAddress(loadBalancerName, lbRefStr, requestedIP, fwdRule
 	return ipAddressToUse, isSafeToReleaseIP, nil
 }
 
-
 // updateExternalLoadBalancer is the external implementation of LoadBalancer.UpdateLoadBalancer.
 func (g *Cloud) updateExternalLoadBalancer(clusterName string, service *v1.Service, nodes []*v1.Node) error {
 	// Process services with LoadBalancerClass "networking.gke.io/l4-regional-external-legacy" used for this controller.
@@ -534,8 +422,13 @@ func (g *Cloud) ensureExternalLoadBalancerDeleted(clusterName, clusterID string,
 		},
 		func() error {
 			klog.Infof("ensureExternalLoadBalancerDeleted(%s): Deleting forwarding rules.", lbRefStr)
-			// The forwarding rule must be deleted before either the target pool can,
-			// unfortunately, so we have to do these two serially.
+			// The forwarding rule must be deleted before the target pool can be deleted,
+			// unfortunately, so we have to delete forwarding rules then target pools serially.
+			if err := ignoreNotFound(g.DeleteRegionForwardingRule(loadBalancerName, g.region)); err != nil {
+				return err
+			}
+
+			// TODO: Always or just with alpha feature flag?
 			frs, err := g.ListRegionForwardingRules(g.region)
 			if err != nil {
 				return err
@@ -1125,8 +1018,7 @@ func equalPorts(existingPorts, newPorts []string, existingPortRange, newPortRang
 
 func groupPortsByProtocol(ports []v1.ServicePort) map[v1.Protocol][]v1.ServicePort {
 	grouped := make(map[v1.Protocol][]v1.ServicePort)
-	for _, p := range ports {
-		port := p
+	for _, port := range ports {
 		grouped[port.Protocol] = append(grouped[port.Protocol], port)
 	}
 	return grouped
@@ -1195,7 +1087,6 @@ func (g *Cloud) firewallNeedsUpdate(name, serviceName, ipAddress string, ports [
 	return true, false, nil
 }
 
-
 func (g *Cloud) ensureHTTPHealthCheckFirewall(svc *v1.Service, serviceName, ipAddress, region, clusterID string, hosts []*gceInstance, hcName string, hcPort int32, isNodesHealthCheck bool) error {
 	// Prepare the firewall params for creating / checking.
 	desc := fmt.Sprintf(`{"kubernetes.io/cluster-id":"%s"}`, clusterID)

providers/gce/gce_loadbalancer_naming.go

@@ -26,7 +26,7 @@ import (
 	"strings"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
 )