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.
@@ -456,7 +456,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 +533,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 +1129,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 +1198,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)