Split GetPortsAndProtocol function to 4 separate functions

pkg/l4lb/l4netlbcontroller.go

@@ -471,7 +471,7 @@ func (lc *L4NetLBController) ensureBackendLinking(port utils.ServicePort) error
 func (lc *L4NetLBController) ensureInstanceGroups(service *v1.Service, nodeNames []string) error {
 	// TODO(kl52752) Move instance creation and deletion logic to NodeController
 	// to avoid race condition between controllers
-	_, _, nodePorts, _ := utils.GetPortsAndProtocol(service.Spec.Ports)
+	nodePorts := utils.GetNodePorts(service.Spec.Ports)
 	_, err := lc.instancePool.EnsureInstanceGroupsAndPorts(lc.ctx.ClusterNamer.InstanceGroup(), nodePorts)
 	if err != nil {
 		return err

pkg/loadbalancers/forwarding_rules.go

@@ -242,7 +242,9 @@ func (l *L4) ensureForwardingRule(loadBalancerName, bsLink string, options gce.I
 		}()
 	}
 
-	ports, _, _, protocol := utils.GetPortsAndProtocol(l.Service.Spec.Ports)
+	servicePorts := l.Service.Spec.Ports
+	ports := utils.GetPorts(servicePorts)
+	protocol := utils.GetProtocol(servicePorts)
 	// Create the forwarding rule
 	frDesc, err := utils.MakeL4LBServiceDescription(utils.ServiceKeyFunc(l.Service.Namespace, l.Service.Name), ipToUse,
 		version, false, utils.ILB)

pkg/loadbalancers/l4.go

@@ -170,7 +170,7 @@ func (l *L4) deleteFirewall(name string) error {
 // This appends the protocol to the forwarding rule name, which will help supporting multiple protocols in the same ILB
 // service.
 func (l *L4) GetFRName() string {
-	_, _, _, protocol := utils.GetPortsAndProtocol(l.Service.Spec.Ports)
+	protocol := utils.GetProtocol(l.Service.Spec.Ports)
 	return l.getFRNameWithProtocol(string(protocol))
 }
 
@@ -213,7 +213,9 @@ func (l *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service)
 	}
 	result.Annotations[annotations.HealthcheckKey] = hcResult.HCName
 
-	_, portRanges, _, protocol := utils.GetPortsAndProtocol(l.Service.Spec.Ports)
+	servicePorts := l.Service.Spec.Ports
+	portRanges := utils.GetServicePortRanges(servicePorts)
+	protocol := utils.GetProtocol(servicePorts)
 
 	// Check if protocol has changed for this service. In this case, forwarding rule should be deleted before
 	// the backend service can be updated.

pkg/loadbalancers/l4_test.go

@@ -1286,7 +1286,7 @@ func assertInternalLbResources(t *testing.T, apiService *v1.Service, l *L4, node
 	if err != nil {
 		t.Errorf("Failed to create description for shared resources, err %v", err)
 	}
-	_, _, _, proto := utils.GetPortsAndProtocol(apiService.Spec.Ports)
+	proto := utils.GetProtocol(apiService.Spec.Ports)
 	expectedAnnotations := make(map[string]string)
 	hcName, hcFwName := l.namer.L4HealthCheck(apiService.Namespace, apiService.Name, sharedHC)
 	// hcDesc is the resource description for healthcheck and firewall rule allowing healthcheck.

pkg/loadbalancers/l4netlb.go

@@ -131,7 +131,9 @@ func (l4netlb *L4NetLB) EnsureFrontend(nodeNames []string, svc *corev1.Service)
 	result.Annotations[annotations.HealthcheckKey] = hcResult.HCName
 
 	name := l4netlb.ServicePort.BackendName()
-	_, portRanges, _, protocol := utils.GetPortsAndProtocol(l4netlb.Service.Spec.Ports)
+	servicePorts := l4netlb.Service.Spec.Ports
+	portRanges := utils.GetServicePortRanges(servicePorts)
+	protocol := utils.GetProtocol(servicePorts)
 
 	bs, err := l4netlb.backendPool.EnsureL4BackendService(name, hcResult.HCLink, string(protocol), string(l4netlb.Service.Spec.SessionAffinity), string(cloud.SchemeExternal), l4netlb.NamespacedName, meta.VersionGA)
 	if err != nil {

pkg/loadbalancers/l4netlb_test.go

@@ -5,7 +5,7 @@ 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
+	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,
@@ -265,7 +265,7 @@ func assertNetLbResources(t *testing.T, apiService *v1.Service, l4NetLb *L4NetLB
 	// Check that Firewalls are created for the LoadBalancer and the HealthCheck
 	resourceName := l4NetLb.ServicePort.BackendName()
 
-	_, _, _, proto := utils.GetPortsAndProtocol(apiService.Spec.Ports)
+	proto := utils.GetProtocol(apiService.Spec.Ports)
 
 	hcName, hcFwName := l4NetLb.namer.L4HealthCheck(apiService.Namespace, apiService.Name, true)
 

pkg/utils/utils.go

@@ -627,22 +627,39 @@ func GetPortRanges(ports []int) (ranges []string) {
 	return ranges
 }
 
-// GetPortsAndProtocol returns the list of ports, list of port ranges and the protocol given the list of k8s port info.
-func GetPortsAndProtocol(svcPorts []api_v1.ServicePort) (ports []string, portRanges []string, nodePorts []int64, protocol api_v1.Protocol) {
+func GetProtocol(svcPorts []api_v1.ServicePort) api_v1.Protocol {
 	if len(svcPorts) == 0 {
-		return []string{}, []string{}, []int64{}, api_v1.ProtocolTCP
+		return api_v1.ProtocolTCP
 	}
 
-	// GCP doesn't support multiple protocols for a single load balancer
-	protocol = svcPorts[0].Protocol
-	portInts := []int{}
+	return svcPorts[0].Protocol
+}
+
+func GetNodePorts(svcPorts []api_v1.ServicePort) []int64 {
+	nodePorts := []int64{}
+	for _, p := range svcPorts {
+		nodePorts = append(nodePorts, int64(p.NodePort))
+	}
+
+	return nodePorts
+}
+
+func GetPorts(svcPorts []api_v1.ServicePort) []string {
+	ports := []string{}
 	for _, p := range svcPorts {
 		ports = append(ports, strconv.Itoa(int(p.Port)))
+	}
+
+	return ports
+}
+
+func GetServicePortRanges(svcPorts []api_v1.ServicePort) []string {
+	portInts := []int{}
+	for _, p := range svcPorts {
 		portInts = append(portInts, int(p.Port))
-		nodePorts = append(nodePorts, int64(p.NodePort))
 	}
 
-	return ports, GetPortRanges(portInts), nodePorts, protocol
+	return GetPortRanges(portInts)
 }
 
 func minMaxPort(svcPorts []api_v1.ServicePort) (int32, int32) {

pkg/utils/utils_test.go

@@ -1326,39 +1326,198 @@ func TestIsLoadBalancerType(t *testing.T) {
 		})
 	}
 }
+func TestGetProtocol(t *testing.T) {
+	tcpPort := api_v1.ServicePort{
+		Name:     "TCP Port",
+		Protocol: api_v1.ProtocolTCP,
+	}
+	udpPort := api_v1.ServicePort{
+		Name:     "UDP Port",
+		Protocol: api_v1.ProtocolUDP,
+	}
 
-func TestGetServiceNodePort(t *testing.T) {
 	testCases := []struct {
-		ports        []api_v1.ServicePort
-		wantNodePort int64
+		ports            []api_v1.ServicePort
+		expectedProtocol api_v1.Protocol
+		desc             string
 	}{
+		{
+			ports:            []api_v1.ServicePort{},
+			expectedProtocol: api_v1.ProtocolTCP,
+			desc:             "Empty ports should resolve to TCP",
+		},
 		{
 			ports: []api_v1.ServicePort{
-				{
-					NodePort: 123,
-				},
+				udpPort,
+				tcpPort,
 			},
-			wantNodePort: 123,
+			expectedProtocol: api_v1.ProtocolUDP,
+			desc:             "Mixed protocols, first UDP",
 		},
 		{
-			ports:        []api_v1.ServicePort{},
-			wantNodePort: 0,
+			ports: []api_v1.ServicePort{
+				tcpPort,
+				udpPort,
+			},
+			expectedProtocol: api_v1.ProtocolTCP,
+			desc:             "Mixed protocols, first TCP",
 		},
 	}
 
 	for _, tc := range testCases {
-		desc := fmt.Sprintf("Get Service Port for ports %v", tc.ports)
-		t.Run(desc, func(t *testing.T) {
-			svc := &api_v1.Service{
-				Spec: api_v1.ServiceSpec{
-					Ports: tc.ports,
-				},
+		t.Run(tc.desc, func(t *testing.T) {
+			protocol := GetProtocol(tc.ports)
+
+			if protocol != tc.expectedProtocol {
+				t.Errorf("GetProtocol returned %v, not equal to expected protocol = %v", protocol, tc.expectedProtocol)
+			}
+		})
+	}
+}
+
+func TestGetNodePorts(t *testing.T) {
+	testCases := []struct {
+		ports             []api_v1.ServicePort
+		expectedNodePorts []int64
+		desc              string
+	}{
+		{
+			ports:             []api_v1.ServicePort{},
+			expectedNodePorts: []int64{},
+			desc:              "Empty ports should return empty node ports",
+		},
+		{
+			ports: []api_v1.ServicePort{
+				{NodePort: 80}, {NodePort: 81}, {NodePort: 3000},
+			},
+			expectedNodePorts: []int64{80, 81, 3000},
+			desc:              "Multiple ports",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			nodePorts := GetNodePorts(tc.ports)
+
+			if !reflect.DeepEqual(nodePorts, tc.expectedNodePorts) {
+				t.Errorf("GetNodePorts returned %v, not equal to expected node ports = %v", nodePorts, tc.expectedNodePorts)
 			}
+		})
+	}
+}
 
-			nodePort := GetServiceNodePort(svc)
+func TestGetPorts(t *testing.T) {
+	testCases := []struct {
+		ports         []api_v1.ServicePort
+		expectedPorts []string
+		desc          string
+	}{
+		{
+			ports:         []api_v1.ServicePort{},
+			expectedPorts: []string{},
+			desc:          "Empty ports should return empty slice",
+		},
+		{
+			ports: []api_v1.ServicePort{
+				{Port: 80}, {Port: 81}, {Port: 3000},
+			},
+			expectedPorts: []string{"80", "81", "3000"},
+			desc:          "Multiple ports",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			ports := GetPorts(tc.ports)
+
+			if !reflect.DeepEqual(ports, tc.expectedPorts) {
+				t.Errorf("GetPorts returned %v, not equal to expected ports = %v", ports, tc.expectedPorts)
+			}
+		})
+	}
+}
+
+func TestGetServicePortRanges(t *testing.T) {
+	testCases := []struct {
+		ports          []api_v1.ServicePort
+		expectedRanges []string
+		desc           string
+	}{
+		{
+			desc: "All Unique",
+			ports: []api_v1.ServicePort{
+				{Port: 8}, {Port: 66}, {Port: 23}, {Port: 13}, {Port: 89},
+			},
+			expectedRanges: []string{"8", "13", "23", "66", "89"},
+		},
+		{
+			desc: "All Unique Sorted",
+			ports: []api_v1.ServicePort{
+				{Port: 1}, {Port: 7}, {Port: 9}, {Port: 16}, {Port: 26},
+			},
+			expectedRanges: []string{"1", "7", "9", "16", "26"},
+		},
+		{
+			desc: "Ranges",
+			ports: []api_v1.ServicePort{
+				{Port: 56}, {Port: 78}, {Port: 67}, {Port: 79}, {Port: 21}, {Port: 80}, {Port: 12},
+			},
+			expectedRanges: []string{"12", "21", "56", "67", "78-80"},
+		},
+		{
+			desc: "Ranges Sorted",
+			ports: []api_v1.ServicePort{
+				{Port: 5}, {Port: 7}, {Port: 90}, {Port: 1002}, {Port: 1003},
+				{Port: 1004}, {Port: 1005}, {Port: 2501},
+			},
+			expectedRanges: []string{"5", "7", "90", "1002-1005", "2501"},
+		},
+		{
+			desc: "Ranges Duplicates",
+			ports: []api_v1.ServicePort{
+				{Port: 15}, {Port: 37}, {Port: 900}, {Port: 2002}, {Port: 2003},
+				{Port: 2003}, {Port: 2004}, {Port: 2004},
+			},
+			expectedRanges: []string{"15", "37", "900", "2002-2004"},
+		},
+		{
+			desc: "Duplicates", ports: []api_v1.ServicePort{
+				{Port: 10}, {Port: 10}, {Port: 10}, {Port: 10}, {Port: 10}},
+			expectedRanges: []string{"10"},
+		},
+		{
+			desc: "Only ranges",
+			ports: []api_v1.ServicePort{
+				{Port: 18}, {Port: 19}, {Port: 20}, {Port: 21}, {Port: 22}, {Port: 55},
+				{Port: 56}, {Port: 77}, {Port: 78}, {Port: 79}, {Port: 3504}, {Port: 3505}, {Port: 3506},
+			},
+			expectedRanges: []string{"18-22", "55-56", "77-79", "3504-3506"},
+		},
+		{
+			desc: "Single Range", ports: []api_v1.ServicePort{
+				{Port: 6000}, {Port: 6001}, {Port: 6002}, {Port: 6003}, {Port: 6004}, {Port: 6005},
+			},
+			expectedRanges: []string{"6000-6005"}},
+		{
+			desc: "One value",
+			ports: []api_v1.ServicePort{
+				{Port: 12},
+			},
+			expectedRanges: []string{"12"},
+		},
+		{
+			desc:           "Empty",
+			ports:          []api_v1.ServicePort{},
+			expectedRanges: nil,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			ranges := GetServicePortRanges(tc.ports)
 
-			if nodePort != tc.wantNodePort {
-				t.Errorf("GetServiceNodePort(%v) returned %v, wantNodePort = %v", svc, nodePort, tc.wantNodePort)
+			if !reflect.DeepEqual(ranges, tc.expectedRanges) {
+				t.Errorf("GetServicePortRanges returned %v, not equal to expected ranges = %v", ranges, tc.expectedRanges)
 			}
 		})
 	}