Fix bug with gathering ports for NEG.

kubernetes · Sep 13, 2021 · 302fdc0 · 302fdc0
1 parent c4fd082
commit 302fdc0
Show file tree

Hide file tree

Showing 8 changed files with 292 additions and 135 deletions.
diff --git a/cmd/glbc/app/init.go b/cmd/glbc/app/init.go
@@ -99,8 +99,9 @@ func servicePortForDefaultService(svc *v1.Service, svcPortName string, name type
 					Service: name,
 					Port:    networkingv1.ServiceBackendPort{Name: svcPortName},
 				},
-				TargetPort: port.TargetPort.StrVal,
+				TargetPort: port.TargetPort,
 				Port:       port.Port,
+				PortName:   port.Name,
 			}
 		}
 	}

diff --git a/pkg/backends/neg_linker_test.go b/pkg/backends/neg_linker_test.go
@@ -15,10 +15,12 @@ package backends
 
 import (
 	"fmt"
-	"k8s.io/ingress-gce/pkg/apis/svcneg/v1beta1"
 	"strings"
 	"testing"
 
+	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/ingress-gce/pkg/apis/svcneg/v1beta1"
+
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/mock"
 	"github.com/google/go-cmp/cmp"
@@ -82,7 +84,7 @@ func TestLinkBackendServiceToNEG(t *testing.T) {
 					Port:         80,
 					NodePort:     30001,
 					Protocol:     annotations.ProtocolHTTP,
-					TargetPort:   port,
+					TargetPort:   intstr.FromString(port),
 					NEGEnabled:   true,
 					BackendNamer: defaultNamer},
 			} {

diff --git a/pkg/controller/translator/translator.go b/pkg/controller/translator/translator.go
@@ -194,8 +194,9 @@ func (t *Translator) getServicePort(id utils.ServicePortID, params *getServicePo
 	svcPort := &utils.ServicePort{
 		ID:           id,
 		NodePort:     int64(port.NodePort),
-		Port:         int32(port.Port),
-		TargetPort:   port.TargetPort.String(),
+		Port:         port.Port,
+		PortName:     port.Name,
+		TargetPort:   port.TargetPort,
 		L7ILBEnabled: params.isL7ILB,
 		BackendNamer: namer,
 	}
@@ -464,10 +465,15 @@ func (t *Translator) GatherEndpointPorts(svcPorts []utils.ServicePort) []string
 			// TODO(mixia): refactor firewall syncing into a separate go routine with different trigger.
 			// With NEG, endpoint changes may cause firewall ports to be different if user specifies inconsistent backends.
 			var endpointPorts []int
-			if t.ctx.UseEndpointSlices {
-				endpointPorts = listEndpointTargetPortsFromEndpointSlices(t.ctx.EndpointSliceInformer.GetIndexer(), p.ID.Service.Namespace, p.ID.Service.Name, p.TargetPort)
+			// if targetPort is integer, no need to look for ports from endpoints
+			if p.TargetPort.Type == intstr.Int {
+				endpointPorts = []int{p.TargetPort.IntValue()}
 			} else {
-				endpointPorts = listEndpointTargetPortsFromEndpoints(t.ctx.EndpointInformer.GetIndexer(), p.ID.Service.Namespace, p.ID.Service.Name, p.TargetPort)
+				if t.ctx.UseEndpointSlices {
+					endpointPorts = listEndpointTargetPortsFromEndpointSlices(t.ctx.EndpointSliceInformer.GetIndexer(), p.ID.Service.Namespace, p.ID.Service.Name, p.PortName)
+				} else {
+					endpointPorts = listEndpointTargetPortsFromEndpoints(t.ctx.EndpointInformer.GetIndexer(), p.ID.Service.Namespace, p.ID.Service.Name, p.PortName)
+				}
 			}
 			for _, ep := range endpointPorts {
 				portMap[int64(ep)] = true
@@ -556,12 +562,8 @@ func listAll(store cache.Store, selector labels.Selector, appendFn cache.AppendF
 	return nil
 }
 
-// returns target port if port number is specified, finds the actual target port behind the named target port
-func listEndpointTargetPortsFromEndpointSlices(indexer cache.Indexer, namespace, name, targetPort string) []int {
-	// if targetPort is integer, no need to translate to endpoint slices ports
-	if i, err := strconv.Atoi(targetPort); err == nil {
-		return []int{i}
-	}
+// finds the actual target port behind named target port, the name of the target port is the same as service port name
+func listEndpointTargetPortsFromEndpointSlices(indexer cache.Indexer, namespace, name, svcPortName string) []int {
 	slices, err := indexer.ByIndex(endpointslices.EndpointSlicesByServiceIndex, endpointslices.FormatEndpointSlicesServiceKey(namespace, name))
 	if len(slices) == 0 {
 		klog.Errorf("No Endpoint Slices found for service %s/%s.", namespace, name)
@@ -576,21 +578,16 @@ func listEndpointTargetPortsFromEndpointSlices(indexer cache.Indexer, namespace,
 	for _, sliceObj := range slices {
 		slice := sliceObj.(*discoveryapi.EndpointSlice)
 		for _, port := range slice.Ports {
-			if port.Protocol != nil && *port.Protocol == api_v1.ProtocolTCP && port.Name != nil && *port.Name == targetPort && port.Port != nil {
+			if port.Protocol != nil && *port.Protocol == api_v1.ProtocolTCP && port.Name != nil && *port.Name == svcPortName && port.Port != nil {
 				ret = append(ret, int(*port.Port))
 			}
 		}
 	}
 	return ret
 }
 
-// returns target port if port number is specified, finds the actual target port behind the named target port
-func listEndpointTargetPortsFromEndpoints(indexer cache.Indexer, namespace, name, targetPort string) []int {
-	// if targetPort is integer, no need to translate to endpoint ports
-	if i, err := strconv.Atoi(targetPort); err == nil {
-		return []int{i}
-	}
-
+// finds the actual target port behind named target port, the name of the target port is the same as service port name
+func listEndpointTargetPortsFromEndpoints(indexer cache.Indexer, namespace, name, svcPortName string) []int {
 	ep, exists, err := indexer.Get(
 		&api_v1.Endpoints{
 			ObjectMeta: meta_v1.ObjectMeta{
@@ -612,7 +609,7 @@ func listEndpointTargetPortsFromEndpoints(indexer cache.Indexer, namespace, name
 	ret := []int{}
 	for _, subset := range ep.(*api_v1.Endpoints).Subsets {
 		for _, port := range subset.Ports {
-			if port.Protocol == api_v1.ProtocolTCP && port.Name == targetPort {
+			if port.Protocol == api_v1.ProtocolTCP && port.Name == svcPortName {
 				ret = append(ret, int(port.Port))
 			}
 		}