fixup! Implement multi forwardingRule in interanal case

Author: justinsb

providers/gce/gce_loadbalancer_internal.go

@@ -29,7 +29,6 @@ import (
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
-	"github.com/google/go-cmp/cmp"
 	compute "google.golang.org/api/compute/v1"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -188,121 +187,104 @@ func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v
 
 	// Get all existing forwarding rules for this service.
 	// A service can have a forwarding rule with the base name, or with a protocol suffix.
-	existingFwdRules := make(map[string]*compute.ForwardingRule)
-	// The `existingFwdRule` is the one with the base name, passed into this function.
-	if existingFwdRule != nil {
-		existingFwdRules[existingFwdRule.Name] = existingFwdRule
-	}
+	// TODO: Remove existingFwdRules and use the one from the function.
 	// Check for forwarding rules with protocol suffixes.
-	if len(groupedPorts) > 1 {
-		for protocol := range groupedPorts {
-			frName := fmt.Sprintf("%s-%s", loadBalancerName, strings.ToLower(string(protocol)))
-			if _, ok := existingFwdRules[frName]; ok {
-				continue
-			}
-			fr, err := g.GetRegionForwardingRule(frName, g.region)
-			if err != nil && !isNotFound(err) {
-				return nil, err
-			}
-			if fr != nil {
-				existingFwdRules[fr.Name] = fr
-			}
-		}
-	}
-
-	desiredFwdRuleNames := sets.NewString()
-	var desiredFwdRuleProtocols []v1.Protocol
-	for protocol := range groupedPorts {
-		desiredFwdRuleProtocols = append(desiredFwdRuleProtocols, protocol)
+	existingForwardingRules, err := g.getExistingForwardingRules(loadBalancerName)
+	if err != nil {
+		return nil, err
 	}
-	// Sort protocols to have a stable order for naming and processing.
-	sort.Slice(desiredFwdRuleProtocols, func(i, j int) bool {
-		return desiredFwdRuleProtocols[i] < desiredFwdRuleProtocols[j]
-	})
 
 	var createdFwdRules []*compute.ForwardingRule
 	var desiredBackendServices = make(map[string]bool)
 
-	for _, protocol := range desiredFwdRuleProtocols {
-		portStruct := groupedPorts[protocol]
-		ports := portStruct.portRanges
-
-		// Each protocol gets its own backend service.
-		// The backend service name must be unique per protocol.
-		// Pass a single-protocol map to makeBackendServiceName.
-		singleProtocolGroupedPorts := map[v1.Protocol]ProtocolPorts{protocol: portStruct}
-		backendServiceName := makeBackendServiceName(loadBalancerName, clusterID, sharedBackend, scheme, singleProtocolGroupedPorts, svc.Spec.SessionAffinity)
-		desiredBackendServices[backendServiceName] = true
-		backendServiceLink := g.getBackendServiceLink(backendServiceName)
-
-		bsDescription := makeBackendServiceDescription(nm, sharedBackend)
-		err = g.ensureInternalBackendService(backendServiceName, bsDescription, svc.Spec.SessionAffinity, scheme, protocol, igLinks, hc.SelfLink)
-		if err != nil {
-			return nil, err
-		}
-
+	var desiredForwardingRules map[v1.Protocol]*compute.ForwardingRule
+	for protocol, protocolPorts := range groupedPorts {
 		// Each protocol gets its own forwarding rule.
 		// If there's only one protocol, the forwarding rule name is the load balancer name.
 		// Otherwise, it's load-balancer-name-<protocol>.
 		frName := loadBalancerName
 		if len(groupedPorts) > 1 {
 			frName = fmt.Sprintf("%s-%s", loadBalancerName, strings.ToLower(string(protocol)))
 		}
-		desiredFwdRuleNames.Insert(frName)
 
-		newFwdRule := &compute.ForwardingRule{
+		forwardingRule := &compute.ForwardingRule{
 			Name:                frName,
 			Description:         fwdRuleDescriptionString,
 			IPAddress:           ipToUse,
 			BackendService:      backendServiceLink,
-			Ports:               ports,
 			IPProtocol:          string(protocol),
 			LoadBalancingScheme: string(scheme),
 			Subnetwork:          subnetworkURL,
 			Network:             g.networkURL,
 		}
 		if options.AllowGlobalAccess {
-			newFwdRule.AllowGlobalAccess = options.AllowGlobalAccess
+			forwardingRule.AllowGlobalAccess = options.AllowGlobalAccess
 		}
-		if len(ports) > maxL4ILBPorts {
-			newFwdRule.Ports = nil
-			newFwdRule.AllPorts = true
+		if len(protocolPorts.ports) > maxL4ILBPorts {
+			forwardingRule.Ports = nil
+			forwardingRule.AllPorts = true
+		} else {
+			for _, port := range protocolPorts.ports {
+				forwardingRule.Ports = append(forwardingRule.Ports, strconv.Itoa(port))
+			}
 		}
 
 		// Check if a forwarding rule for this protocol already exists.
+		for _, existingFwdRule := range existingForwardingRules {
+			if existingFwdRule.IPProtocol == forwardingRule.IPProtocol {
+				forwardingRule.Name = existingFwdRule.Name
+				break
+			}
+		}
+
+		desiredForwardingRules = append(desiredForwardingRules, forwardingRule)
+	}
+
+	for _, desiredForwardingRule := range desiredForwardingRules {
 		var existingFwdRuleForProtocol *compute.ForwardingRule
-		if fr, ok := existingFwdRules[frName]; ok {
-			existingFwdRuleForProtocol = fr
+		for _, existingFwdRule := range existingForwardingRules {
+			if existingFwdRule.IPProtocol == desiredForwardingRule.IPProtocol {
+				existingFwdRuleForProtocol = existingFwdRule
+				break
+			}
 		}
 
-		if err := g.ensureInternalForwardingRule(existingFwdRuleForProtocol, newFwdRule); err != nil {
+		if err := g.ensureInternalForwardingRule(existingFwdRuleForProtocol, desiredForwardingRule); err != nil {
 			return nil, err
 		}
-		createdFwdRules = append(createdFwdRules, newFwdRule)
+		createdFwdRules = append(createdFwdRules, desiredForwardingRule)
 	}
 
 	// Delete any forwarding rules that are no longer needed.
-	for frName, fr := range existingFwdRules {
-		if desiredFwdRuleNames.Has(frName) {
+	for frName, fr := range existingForwardingRules {
+		var matching *compute.ForwardingRule
+		for _, desiredForwardingRule := range desiredForwardingRules {
+			if desiredForwardingRule.Name == fr.Name {
+				matching = desiredForwardingRule
+				continue
+			}
+		}
+		if matching != nil {
 			continue
 		}
+
 		klog.V(2).Infof("ensureInternalLoadBalancer(%v): deleting stale forwarding rule %s", loadBalancerName, frName)
 		if err := ignoreNotFound(g.DeleteRegionForwardingRule(frName, g.region)); err != nil {
 			return nil, err
 		}
-		// Also delete the associated backend service if it's not used by other forwarding rules.
-		if fr.BackendService != "" {
-			bsName := getNameFromLink(fr.BackendService)
-			if !desiredBackendServices[bsName] {
-				klog.V(2).Infof("ensureInternalLoadBalancer(%v): deleting stale backend service %s", loadBalancerName, bsName)
-				if err := g.teardownInternalBackendService(bsName); err != nil {
-					klog.Warningf("ensureInternalLoadBalancer: could not delete old backend service %s: %v", bsName, err)
-				}
-			}
-		}
-	}
-
-	if len(createdFwdRules) == 0 {
+		// // Also delete the associated backend service if it's not used by other forwarding rules.
+		// if fr.BackendService != "" {
+		// 	bsName := getNameFromLink(fr.BackendService)
+		// 	if !desiredBackendServices[bsName] {
+		// 		klog.V(2).Infof("ensureInternalLoadBalancer(%v): deleting stale backend service %s", loadBalancerName, bsName)
+		// 		if err := g.teardownInternalBackendService(bsName); err != nil {
+		// 			klog.Warningf("ensureInternalLoadBalancer: could not delete old backend service %s: %v", bsName, err)
+		// 		}
+		// 	}
+		// }
+	}
+
+	if len(desiredForwardingRules) == 0 {
 		klog.V(2).Infof("ensureInternalLoadBalancer(%v): no forwarding rules needed, all deleted.", loadBalancerName)
 		return &v1.LoadBalancerStatus{}, nil
 	}