Fix RBS crash, when getting NodePorts for Services without ports

pkg/l4lb/l4netlbcontroller.go

@@ -234,7 +234,7 @@ func (lc *L4NetLBController) hasForwardingRuleAnnotation(svc *v1.Service, frName
 
 // isRBSBasedService checks if service has either RBS annotation, finalizer or RBSForwardingRule
 func (lc *L4NetLBController) isRBSBasedService(svc *v1.Service) bool {
-	if svc == nil {
+	if !utils.IsLoadBalancerServiceType(svc) {
 		return false
 	}
 

pkg/l4lb/l4netlbcontroller_test.go

@@ -1203,3 +1203,58 @@ func TestPreventTargetPoolToRBSMigration(t *testing.T) {
 		})
 	}
 }
+
+func TestIsRBSBasedServiceForNonLoadBalancersType(t *testing.T) {
+	testCases := []struct {
+		desc    string
+		ports   []v1.ServicePort
+		svcType v1.ServiceType
+	}{
+		{
+			desc: "Service ClusterIP with ports should not be marked as RBS",
+			ports: []v1.ServicePort{
+				{Name: "testport", Port: 8080, Protocol: "TCP", NodePort: 32999},
+			},
+			svcType: v1.ServiceTypeClusterIP,
+		},
+		{
+			desc:    "Service ClusterIP with empty ports array should not be marked as RBS",
+			ports:   []v1.ServicePort{},
+			svcType: v1.ServiceTypeClusterIP,
+		},
+		{
+			desc:    "Service ClusterIP without ports should not be marked as RBS",
+			ports:   nil,
+			svcType: v1.ServiceTypeClusterIP,
+		},
+		{
+			desc:    "Service NodePort should not be marked as RBS",
+			svcType: v1.ServiceTypeNodePort,
+		},
+		{
+			desc:    "Service ExternalName should not be marked as RBS",
+			svcType: v1.ServiceTypeExternalName,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			// Setup
+			svc := &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:        "example-svc",
+					Annotations: make(map[string]string),
+				},
+				Spec: v1.ServiceSpec{
+					Type:  tc.svcType,
+					Ports: tc.ports,
+				},
+			}
+			controller := newL4NetLBServiceController()
+
+			if controller.isRBSBasedService(svc) {
+				t.Errorf("isRBSBasedService(%v) = true, want false", svc)
+			}
+		})
+	}
+}

pkg/loadbalancers/l4netlb.go

@@ -95,7 +95,7 @@ func NewL4NetLB(service *corev1.Service, cloud *gce.Cloud, scope meta.KeyType, n
 	l4netlb.ServicePort = utils.ServicePort{
 		ID:           portId,
 		BackendNamer: l4netlb.namer,
-		NodePort:     int64(service.Spec.Ports[0].NodePort),
+		NodePort:     utils.GetServiceNodePort(service),
 		L4RBSEnabled: true,
 	}
 	return l4netlb

pkg/utils/utils.go

@@ -242,8 +242,9 @@ func PrettyJson(data interface{}) (string, error) {
 
 // KeyName returns the name portion from a full or partial GCP resource URL.
 // Example:
-//  Input:  https://googleapis.com/v1/compute/projects/my-project/global/backendServices/my-backend
-//  Output: my-backend
+//
+//	Input:  https://googleapis.com/v1/compute/projects/my-project/global/backendServices/my-backend
+//	Output: my-backend
 func KeyName(url string) (string, error) {
 	id, err := cloud.ParseResourceURL(url)
 	if err != nil {
@@ -261,8 +262,9 @@ func KeyName(url string) (string, error) {
 // RelativeResourceName returns the project, location, resource, and name from a full/partial GCP
 // resource URL. This removes the endpoint prefix and version.
 // Example:
-//  Input:  https://googleapis.com/v1/compute/projects/my-project/global/backendServices/my-backend
-//  Output: projects/my-project/global/backendServices/my-backend
+//
+//	Input:  https://googleapis.com/v1/compute/projects/my-project/global/backendServices/my-backend
+//	Output: projects/my-project/global/backendServices/my-backend
 func RelativeResourceName(url string) (string, error) {
 	resID, err := cloud.ParseResourceURL(url)
 	if err != nil {
@@ -274,8 +276,9 @@ func RelativeResourceName(url string) (string, error) {
 // ResourcePath returns the location, resource and name portion from a
 // full or partial GCP resource URL. This removes the endpoint prefix, version, and project.
 // Example:
-//  Input:  https://googleapis.com/v1/compute/projects/my-project/global/backendServices/my-backend
-//  Output: global/backendServices/my-backend
+//
+//	Input:  https://googleapis.com/v1/compute/projects/my-project/global/backendServices/my-backend
+//	Output: global/backendServices/my-backend
 func ResourcePath(url string) (string, error) {
 	resID, err := cloud.ParseResourceURL(url)
 	if err != nil {
@@ -782,3 +785,20 @@ func GetNetworkTier(service *api_v1.Service) (cloud.NetworkTier, bool) {
 		return cloud.NetworkTierDefault, false
 	}
 }
+
+// IsLoadBalancerServiceType checks if kubernetes service is type of LoadBalancer.
+func IsLoadBalancerServiceType(service *api_v1.Service) bool {
+	if service == nil {
+		return false
+	}
+	return service.Spec.Type == api_v1.ServiceTypeLoadBalancer
+}
+
+// GetServiceNodePort safely gets service's first node port,
+// even if they are empty, which can happen for headless services
+func GetServiceNodePort(service *api_v1.Service) int64 {
+	if len(service.Spec.Ports) == 0 {
+		return 0
+	}
+	return int64(service.Spec.Ports[0].NodePort)
+}

pkg/utils/utils_test.go

@@ -20,12 +20,11 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"net/http"
 	"reflect"
 	"testing"
 	"time"
 
-	"net/http"
-
 	"github.com/google/go-cmp/cmp"
 	"google.golang.org/api/compute/v1"
 	"google.golang.org/api/googleapi"
@@ -613,7 +612,7 @@ func TestGetNodeConditionPredicate(t *testing.T) {
 			node: api_v1.Node{
 				Spec: api_v1.NodeSpec{
 					Taints: []api_v1.Taint{
-						api_v1.Taint{
+						{
 							Key:    ToBeDeletedTaint,
 							Value:  fmt.Sprint(time.Now().Unix()),
 							Effect: api_v1.TaintEffectNoSchedule,
@@ -1282,3 +1281,85 @@ func TestIsNetworkMismatchError(t *testing.T) {
 		}
 	}
 }
+
+func TestIsLoadBalancerType(t *testing.T) {
+	testCases := []struct {
+		serviceType            api_v1.ServiceType
+		wantIsLoadBalancerType bool
+	}{
+		{
+			serviceType:            api_v1.ServiceTypeClusterIP,
+			wantIsLoadBalancerType: false,
+		},
+		{
+			serviceType:            api_v1.ServiceTypeNodePort,
+			wantIsLoadBalancerType: false,
+		},
+		{
+			serviceType:            api_v1.ServiceTypeExternalName,
+			wantIsLoadBalancerType: false,
+		},
+		{
+			serviceType:            api_v1.ServiceTypeLoadBalancer,
+			wantIsLoadBalancerType: true,
+		},
+		{
+			serviceType:            "",
+			wantIsLoadBalancerType: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		desc := fmt.Sprintf("Test if is load balancer for type %v", tc.serviceType)
+		t.Run(desc, func(t *testing.T) {
+			svc := &api_v1.Service{
+				Spec: api_v1.ServiceSpec{
+					Type: tc.serviceType,
+				},
+			}
+
+			isLoadBalancer := IsLoadBalancerServiceType(svc)
+
+			if isLoadBalancer != tc.wantIsLoadBalancerType {
+				t.Errorf("IsLoadBalancerServiceType(%v) returned %t, expected %t", svc, isLoadBalancer, tc.wantIsLoadBalancerType)
+			}
+		})
+	}
+}
+
+func TestGetServiceNodePort(t *testing.T) {
+	testCases := []struct {
+		ports        []api_v1.ServicePort
+		wantNodePort int64
+	}{
+		{
+			ports: []api_v1.ServicePort{
+				{
+					NodePort: 123,
+				},
+			},
+			wantNodePort: 123,
+		},
+		{
+			ports:        []api_v1.ServicePort{},
+			wantNodePort: 0,
+		},
+	}
+
+	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,
+				},
+			}
+
+			nodePort := GetServiceNodePort(svc)
+
+			if nodePort != tc.wantNodePort {
+				t.Errorf("GetServiceNodePort(%v) returned %v, wantNodePort = %v", svc, nodePort, tc.wantNodePort)
+			}
+		})
+	}
+}