From f4ccf92ef6b02039fafc2eb86ccc3f7e6c7af76d Mon Sep 17 00:00:00 2001 From: Slavik Panasovets Date: Mon, 8 Aug 2022 11:01:01 +0000 Subject: [PATCH] Fix RBS crash, when getting NodePorts for Services without ports --- pkg/l4lb/l4netlbcontroller.go | 2 +- pkg/l4lb/l4netlbcontroller_test.go | 55 +++++++++++++++++++ pkg/loadbalancers/l4netlb.go | 2 +- pkg/utils/utils.go | 32 ++++++++--- pkg/utils/utils_test.go | 87 ++++++++++++++++++++++++++++-- 5 files changed, 167 insertions(+), 11 deletions(-) diff --git a/pkg/l4lb/l4netlbcontroller.go b/pkg/l4lb/l4netlbcontroller.go index f4fa9a8270..8de4ba05a4 100644 --- a/pkg/l4lb/l4netlbcontroller.go +++ b/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 } diff --git a/pkg/l4lb/l4netlbcontroller_test.go b/pkg/l4lb/l4netlbcontroller_test.go index 1f6edf7021..31acee4f8f 100644 --- a/pkg/l4lb/l4netlbcontroller_test.go +++ b/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) + } + }) + } +} diff --git a/pkg/loadbalancers/l4netlb.go b/pkg/loadbalancers/l4netlb.go index 0e03a8af06..712f8bef13 100644 --- a/pkg/loadbalancers/l4netlb.go +++ b/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 diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index 5aed1291e8..e5509bb8d0 100644 --- a/pkg/utils/utils.go +++ b/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) +} diff --git a/pkg/utils/utils_test.go b/pkg/utils/utils_test.go index 0627c2f734..5c1d4c4c07 100644 --- a/pkg/utils/utils_test.go +++ b/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) + } + }) + } +}