Use target port of Service in NodePortLocal to configure Pod reachabi…

…lity - addressed review comments - part 2

Signed-off-by: Monotosh Das <[email protected]>

pkg/agent/nodeportlocal/k8s/npl_controller.go

@@ -278,6 +278,15 @@ func (c *NPLController) getPodsFromService(svc *corev1.Service) []string {
 	return pods
 }
 
+func buildPortProto(name, prototcol string) string {
+	return name + delim + prototcol
+}
+
+func parsePortProto(targetPort string) (int, error) {
+	portProto := strings.Split(targetPort, delim)
+	return strconv.Atoi(portProto[0])
+}
+
 func (c *NPLController) getTargetPortsForServicesOfPod(obj interface{}) (sets.String, sets.String) {
 	targetPortsInt := sets.NewString()
 	targetPortsStr := sets.NewString()
@@ -299,11 +308,13 @@ func (c *NPLController) getTargetPortsForServicesOfPod(obj interface{}) (sets.St
 					case intstr.Int:
 						// An entry of format <target-port>:<protocol> (e.g. 8080:TCP) is added for a target port in the set targetPortsInt.
 						// This is done to ensure that we can match with both port and protocol fields in container port of a Pod.
-						klog.V(4).Infof("Added target port in targetPortsInt set: %v", fmt.Sprint(port.TargetPort.IntVal)+delim+string(port.Protocol))
-						targetPortsInt.Insert(fmt.Sprint(port.TargetPort.IntVal) + delim + string(port.Protocol))
+						portProto := buildPortProto(fmt.Sprint(port.TargetPort.IntVal), string(port.Protocol))
+						klog.V(4).Infof("Added target port in targetPortsInt set: %v", portProto)
+						targetPortsInt.Insert(portProto)
 					case intstr.String:
-						klog.V(4).Infof("Added target port in targetPortsStr set: %v", fmt.Sprint(port.TargetPort.IntVal)+delim+string(port.Protocol))
-						targetPortsStr.Insert(port.TargetPort.StrVal + delim + string(port.Protocol))
+						portProto := buildPortProto(port.TargetPort.StrVal, string(port.Protocol))
+						klog.V(4).Infof("Added target port in targetPortsStr set: %v", portProto)
+						targetPortsStr.Insert(portProto)
 					}
 				}
 			}
@@ -437,12 +448,15 @@ func (c *NPLController) handleAddUpdatePod(key string, obj interface{}) error {
 	hostPorts := make(map[string]int)
 	for _, container := range podContainers {
 		for _, cport := range container.Ports {
-			portProtoStr := cport.Name + delim + string(cport.Protocol)
-			portProtoInt := fmt.Sprint(cport.ContainerPort) + delim + string(cport.Protocol)
+			portProtoInt := buildPortProto(fmt.Sprint(cport.ContainerPort), string(cport.Protocol))
 			if int(cport.HostPort) > 0 {
 				klog.V(4).Infof("Host Port is defined for Container %s in Pod %s, thus extra NPL port is not allocated", container.Name, key)
 				hostPorts[portProtoInt] = int(cport.HostPort)
 			}
+			if cport.Name == "" {
+				continue
+			}
+			portProtoStr := buildPortProto(cport.Name, string(cport.Protocol))
 			if targetPortsStr.Has(portProtoStr) {
 				targetPortsInt.Insert(portProtoInt)
 			}
@@ -463,10 +477,9 @@ func (c *NPLController) handleAddUpdatePod(key string, obj interface{}) error {
 	// (ignoring NPL annotations) and make sure they are present. As we do so, we build the expected list of
 	// NPL annotations for the Pod.
 	for _, targetPort := range targetPortsInt.List() {
-		portProto := strings.Split(targetPort, delim)
-		port, err := strconv.Atoi(portProto[0])
+		port, err := parsePortProto(targetPort)
 		if err != nil {
-			return fmt.Errorf("failed to parse port number from %s for pod %s: %v", portProto[0], key, err)
+			return fmt.Errorf("failed to parse port number from %s for pod %s: %v", targetPort, key, err)
 		}
 		podPorts[port] = struct{}{}
 		portData := c.portTable.GetEntryByPodIPPort(podIP, port)

pkg/agent/nodeportlocal/npl_agent_test.go

@@ -416,8 +416,8 @@ func TestPodDelete(t *testing.T) {
 func TestPodAddMultiPort(t *testing.T) {
 	testSvc := getTestSvc()
 	testPod := getTestPod()
-	newPort1 := 91
-	newPort2 := 92
+	newPort1 := defaultPort
+	newPort2 := 81
 	testPod.Spec.Containers[0].Ports = append(
 		testPod.Spec.Containers[0].Ports,
 		corev1.ContainerPort{ContainerPort: int32(newPort1)},
@@ -434,7 +434,6 @@ func TestPodAddMultiPort(t *testing.T) {
 	nplData := testData.checkAnnotationValue(value, defaultPort)
 	assert.Len(t, nplData, 1)
 	assert.True(t, testData.portTable.RuleExists(defaultPodIP, defaultPort))
-	assert.False(t, testData.portTable.RuleExists(defaultPodIP, newPort1))
 	assert.False(t, testData.portTable.RuleExists(defaultPodIP, newPort2))
 }
 
@@ -524,7 +523,7 @@ func TestMultiplePods(t *testing.T) {
 	assert.NotEqual(t, nplData1[0].NodePort, nplData2[0].NodePort)
 }
 
-func TestMultipleService(t *testing.T) {
+func TestMultipleServicesSameBackendPod(t *testing.T) {
 	testSvc1 := getTestSvc()
 	testPod := getTestPod()
 	testSvc2 := getTestSvc(9090)

test/e2e/framework.go

@@ -970,17 +970,6 @@ func (data *TestData) createNginxPodOnNode(name string, nodeName string) error {
 	}, false, nil)
 }
 
-// createNginxPodWithPort creates an nginx Pod in the test namespace with configurable container port.
-func (data *TestData) createNginxPodWithPort(name, nodeName string, port int32) error {
-	return data.createPodOnNode(name, nodeName, nginxImage, []string{}, nil, nil, []corev1.ContainerPort{
-		{
-			Name:          "http",
-			ContainerPort: port,
-			Protocol:      corev1.ProtocolTCP,
-		},
-	}, false, nil)
-}
-
 // createNginxPod creates a Pod in the test namespace with a single nginx container.
 func (data *TestData) createNginxPod(name, nodeName string) error {
 	return data.createNginxPodOnNode(name, nodeName)

test/e2e/nodeportlocal_test.go

@@ -184,6 +184,17 @@ func validatePortsInAnnotation(t *testing.T, r *require.Assertions, nplAnnotatio
 	r.Emptyf(targetPorts, "Target ports %v not found in Pod annotation", targetPorts)
 }
 
+// createNginxPodWithPort creates an nginx Pod in the test namespace with configurable container port.
+func createNginxPodWithPort(name, nodeName string, port int32) error {
+	return testData.createPodOnNode(name, nodeName, nginxImage, []string{}, nil, nil, []corev1.ContainerPort{
+		{
+			Name:          "http",
+			ContainerPort: port,
+			Protocol:      corev1.ProtocolTCP,
+		},
+	}, false, nil)
+}
+
 func TestNPLAddPod(t *testing.T) {
 	skipIfNotIPv4Cluster(t)
 	skipIfHasWindowsNodes(t)
@@ -218,7 +229,7 @@ func NPLTestMultiplePods(t *testing.T) {
 	for i := 0; i < 4; i++ {
 		testPodName := randName("test-pod-")
 		testPods = append(testPods, testPodName)
-		err := testData.createNginxPodWithPort(testPodName, node, 90)
+		err := createNginxPodWithPort(testPodName, node, 90)
 		r.NoError(err, "Error creating test Pod: %v", err)
 	}
 
@@ -320,7 +331,7 @@ func NPLTestLocalAccess(t *testing.T) {
 	node := nodeName(0)
 
 	testPodName := randName("test-pod-")
-	err := testData.createNginxPodWithPort(testPodName, node, 90)
+	err := createNginxPodWithPort(testPodName, node, 90)
 	r.NoError(err, "Error creating test Pod: %v", err)
 
 	clientName := randName("test-client-")
@@ -371,7 +382,7 @@ func TestNPLMultiplePodsAgentRestart(t *testing.T) {
 	for i := 0; i < 4; i++ {
 		testPodName := randName("test-pod-")
 		testPods = append(testPods, testPodName)
-		err = data.createNginxPodWithPort(testPodName, node, 90)
+		err = createNginxPodWithPort(testPodName, node, 90)
 		r.NoError(err, "Error creating test Pod: %v", err)
 	}
 
@@ -443,7 +454,7 @@ func TestNPLChangePortRangeAgentRestart(t *testing.T) {
 	for i := 0; i < 4; i++ {
 		testPodName := randName("test-pod-")
 		testPods = append(testPods, testPodName)
-		err = data.createNginxPodWithPort(testPodName, node, 90)
+		err = createNginxPodWithPort(testPodName, node, 90)
 		r.NoError(err, "Error Creating test Pod: %v", err)
 	}