Use target port of Service in NodePortLocal to configure Pod reachability #2222

monotosh-avi · 2021-05-28T19:27:01Z

Currently we use container ports of a Pod to program iptables rules to make the Pod
reachable through a port in the Node. But container ports are not mandatory and multiple
services can use different target ports for the same pod. Hence adding a change in
NodePortLocal implementation, where target ports of all services would be used to
program iptables rules and annotate pods.

To implement this, target ports are being obtained from all the Services selecting a Pod,
in the function handleAddUpdatePod().
Necessary changes in the tests have been added.

Fixes #1912

codecov-commenter · 2021-05-28T20:17:55Z

Codecov Report

Merging #2222 (686b2d9) into main (ef14ad1) will increase coverage by 0.00%.
The diff coverage is 62.50%.

@@           Coverage Diff           @@
##             main    #2222   +/-   ##
=======================================
  Coverage   62.05%   62.05%           
=======================================
  Files         277      277           
  Lines       21588    21615   +27     
=======================================
+ Hits        13396    13414   +18     
- Misses       6792     6800    +8     
- Partials     1400     1401    +1

Flag	Coverage Δ
e2e-tests	`∅ <ø> (?)`
kind-e2e-tests	`52.37% <62.50%> (+0.02%)`	⬆️
unit-tests	`41.79% <0.00%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/agent/nodeportlocal/k8s/npl_controller.go	`59.57% <62.50%> (+0.35%)`	⬆️
pkg/apiserver/handlers/endpoint/handler.go	`58.82% <0.00%> (-11.77%)`	⬇️
pkg/apis/controlplane/v1beta2/sets.go	`66.66% <0.00%> (-2.57%)`	⬇️
pkg/controller/networkpolicy/status_controller.go	`86.45% <0.00%> (-0.65%)`	⬇️
pkg/agent/openflow/client.go	`58.39% <0.00%> (-0.26%)`	⬇️
pkg/agent/flowexporter/exporter/exporter.go	`78.89% <0.00%> (+0.22%)`	⬆️
pkg/agent/stats/collector.go	`98.46% <0.00%> (+3.84%)`	⬆️

antoninbas · 2021-06-08T20:24:43Z

pkg/agent/nodeportlocal/k8s/npl_controller.go

@@ -282,11 +289,13 @@ func (c *NPLController) isNPLEnabledForServiceOfPod(obj interface{}) bool {
 		if isSvc && svc.Spec.Type != corev1.ServiceTypeNodePort {
 			if pod.Namespace == svc.Namespace &&
 				matchSvcSelectorPodLabels(svc.Spec.Selector, pod.GetLabels()) {
-				return true
+				for _, port := range svc.Spec.Ports {
+					targetPorts.Insert(int(port.TargetPort.IntVal))


TargetPort can be a string (named port in the target Pod's container). If we don't want to support this case yet, it's fine, but we should check for that case and log an error.

but it seems that we should be able to support named ports here, by returning both a list of numerical ports and a list of named ports (strings), and letting handleAddUpdatePod do the resolution from port name to port number.

Added support for named target port

antoninbas · 2021-06-08T20:29:17Z

pkg/agent/nodeportlocal/k8s/npl_controller.go

-				}
+			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[int(cport.ContainerPort)] = int(cport.HostPort)


it feels like using ContainerPort as the key is not sufficient, we need ContainerPort + Protocol?

it could be interesting to have a test case with 2 container ports, one for UDP and one for TCP but using the same container port number, and have them both exposed as host ports

Added protocol type in the key too.
I didn't understand the test case with two container ports.
I have added a few for test cases. Let me know if those covers that case.

I meant a test with a Pod like this one:

testPod.Spec.Containers[0].Ports = append( testPod.Spec.Containers[0].Ports, corev1.ContainerPort{ContainerPort: int32(defaultPort), HostPort: int32(hostPort), Protocol: corev1.ProtocolTCP}, ) testPod.Spec.Containers[0].Ports = append( testPod.Spec.Containers[0].Ports, corev1.ContainerPort{ContainerPort: int32(defaultPort), HostPort: int32(hostPort), Protocol: corev1.ProtocolUDP}, )

with a Service using both of these as target ports

@monotosh-avi ping on this, what do you think of adding this test?

@antoninbas I had missed this. Added a test case now.

antoninbas · 2021-06-08T20:41:07Z

pkg/agent/nodeportlocal/npl_agent_test.go

 // TestPodAddMultiPort creates a Pod with multiple ports and verifies that the Pod's NPL annotation
-// and the local port table are updated correctly.
+// and the local port table are updated with only one port used as target port of the Service.


that kind of defeats the original purpose of the test, now that we install forwarding rules for a single port...

maybe we should have a different test case to validate that only ports used as Service target ports are exposed with NPL

Yes. The original test case has been modified.
Now we are validating that only ports used as Service target ports are exposed with NPL. For annotation, this was already being done. Added couple of more checks to ensure no NPL rules are added for pod container ports.

antoninbas · 2021-06-08T20:42:37Z

test/e2e/nodeportlocal_test.go

+		_, _, err := data.runCommandFromPod(antreaNamespace, antreaPod, agentContainerName, cmd)
+		if err != nil {
+			t.Logf("Error while deleting rule from iptables: %v", err)
+			// Retry, as sometimes error can occur due to concurrent operations on iptables.


I don't think we need this, I added -w 10 to the iptables delete command already to accommodate for this issue

antoninbas · 2021-06-09T19:23:34Z

pkg/agent/nodeportlocal/k8s/npl_controller.go

+	if len(targetPortsInt) == 0 {
+		if err := c.deleteAllPortRulesIfAny(podIP); err != nil {
+			return err
+		}
+		if _, exists := pod.Annotations[NPLAnnotationKey]; exists {
+			return c.cleanupNPLAnnotationForPod(pod)
+		}
+		return nil
+	}


I think it would be better to keep this part early in the function and use if len(targetPortsInt) == 0 && len(targetsPortStr) as the if check

I had thought about this, but in that case we would miss the condition where targetsPortStr is not empty but no matching port name is found in the container ports.

sounds good, but maybe a comment to that effect would avoid confusion in the future

do we have a unit test for that edge case?

Sure. Added a comment.
In the test case TestTargetPortWithName, I have added a check to make sure updating Service with wrong target port ensures deletion of rules and annotations from the Pod.

antoninbas · 2021-06-09T19:59:30Z

pkg/agent/nodeportlocal/k8s/npl_controller.go

+			portProtoStr := cport.Name + delim + string(cport.Protocol)
+			portProtoInt := fmt.Sprint(cport.ContainerPort) + delim + 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 targetPortsStr.Has(portProtoStr) {
+				targetPortsInt.Insert(portProtoInt)
+			}


can't we re-organize the code like this, I feel like it would make more sense since the port name is not always set:

portProtoInt := fmt.Sprint(cport.ContainerPort) + delim + 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 := cport.Name + delim + string(cport.Protocol) if targetPortsStr.Has(portProtoStr) { targetPortsInt.Insert(portProtoInt) }

antoninbas · 2021-06-09T20:02:29Z

pkg/agent/nodeportlocal/k8s/npl_controller.go

+		portProto := strings.Split(targetPort, delim)
+		port, err := strconv.Atoi(portProto[0])
+		if err != nil {
+			return fmt.Errorf("failed to parse port number from %s for pod %s: %v", portProto[0], key, err)
+		}


can we have a helper function for building the port:proto string and one for parsing it?

antoninbas · 2021-06-09T20:05:24Z

pkg/agent/nodeportlocal/npl_agent_test.go

+	newPort1 := 91
+	newPort2 := 92
 	testPod.Spec.Containers[0].Ports = append(
 		testPod.Spec.Containers[0].Ports,
-		corev1.ContainerPort{ContainerPort: int32(newPort)},
+		corev1.ContainerPort{ContainerPort: int32(newPort1)},
+	)
+	testPod.Spec.Containers[0].Ports = append(
+		testPod.Spec.Containers[0].Ports,
+		corev1.ContainerPort{ContainerPort: int32(newPort2)},


I'm still trying to understand why we need to add 2 new ports here (91 and 91). Neither of these is a target port, so neither of these is used for NPL. We only use the default port (90), yet the test is called multi-port. It feels like 90 and 91 should both be NPL ports, but not 92.

Actually default port is 80. Updated the pod container ports to 80 and 81 and then the test ensures that we are adding rule for only 80.

antoninbas · 2021-06-09T20:09:03Z

pkg/agent/nodeportlocal/npl_agent_test.go

@@ -453,6 +524,22 @@ func TestMultiplePods(t *testing.T) {
 	assert.NotEqual(t, nplData1[0].NodePort, nplData2[0].NodePort)
 }

+func TestMultipleService(t *testing.T) {


s/TestMultipleService/TestMultipleServices

or maybe TestMultipleServicesSameBackendPod for clarity

antoninbas · 2021-06-09T20:11:40Z

test/e2e/framework.go

+// 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)
+}


this function should be moved to nodeportlocal_test.go IMO. But I am not sure why we even need it. You always use the same port number (90) and I see no reference to the port name ("http").

Moved to nodeportlocal_test.go. We needed this function to have option to provide port number for the container port of nginx pod.

antoninbas · 2021-06-10T20:03:19Z

pkg/agent/nodeportlocal/k8s/npl_controller.go

+	return name + delim + prototcol
+}
+
+func parsePortProto(targetPort string) (int, error) {


shouldn't this return both port and protocol?

The caller function is only using port for now. Later if required we can return both.

antoninbas · 2021-06-10T20:04:35Z

pkg/agent/nodeportlocal/k8s/npl_controller.go

+	if len(targetPortsInt) == 0 {
+		if err := c.deleteAllPortRulesIfAny(podIP); err != nil {
+			return err
+		}
+		if _, exists := pod.Annotations[NPLAnnotationKey]; exists {
+			return c.cleanupNPLAnnotationForPod(pod)
+		}
+		return nil
+	}


sounds good, but maybe a comment to that effect would avoid confusion in the future

do we have a unit test for that edge case?

antoninbas · 2021-06-10T20:10:08Z

pkg/agent/nodeportlocal/npl_agent_test.go

@@ -389,24 +412,29 @@ func TestPodDelete(t *testing.T) {
 }

 // TestPodAddMultiPort creates a Pod with multiple ports and verifies that the Pod's NPL annotation
-// and the local port table are updated correctly.
+// and the local port table are updated with only one port used as target port of the Service.
 func TestPodAddMultiPort(t *testing.T) {


I think we're back to my original comment. Why are we modifying this test like this, instead of having 2 separate tests

TestServiceMultipleTargetPorts: Service has 2 target ports pointing to 2 different container ports, both are exposed with NPL

TestPodContainerPortWithoutService: a Pod has 2 container ports, only one of them exposed with NPL (this is your current test, but I feel like the name is confusing)

Or do we already have a test for the first case?

Got it. I have added a separate test case.

antoninbas

some small comments, otherwise LGTM

antoninbas · 2021-06-11T21:50:07Z

pkg/agent/nodeportlocal/k8s/npl_controller.go

+
+	// targetPortsInt contains list of all ports that needs to be exposed for the Pod, including container ports
+	// for named ports present in targetPortsStr. If it is empty, then all existing rules and annotations for the
+	// Pod have to be cleaned up.


I think we should add: "If a Service uses a named target port that doesn't match any named container port for the current Pod, no corresponding entry will be added to the targetPortsInt set by the code above."

antoninbas · 2021-06-11T21:51:22Z

pkg/agent/nodeportlocal/npl_agent_test.go

-	testSvc := getTestSvc()
-	testPod := getTestPod()
+// TestPodAddMultiPort creates a Pod with multiple ports and a Service with same ports as target ports.
+// It is verified that the Pod's NPL annotationthe local port table are updated correctly.


Suggested change

// It is verified that the Pod's NPL annotationthe local port table are updated correctly.

// It verifies that the Pod's NPL annotation and the local port table are updated correctly.

antoninbas · 2021-06-11T21:52:23Z

pkg/agent/nodeportlocal/npl_agent_test.go

+	testPod.Spec.Containers[0].Ports = append(
+		testPod.Spec.Containers[0].Ports,
+		corev1.ContainerPort{ContainerPort: defaultPort},
+	)
 	testPod.Spec.Containers[0].Ports = append(
 		testPod.Spec.Containers[0].Ports,
 		corev1.ContainerPort{ContainerPort: int32(newPort)},


adding Pod container ports is not required any more (now that we use the Service's target ports), unless we are dealing with a named port. Maybe for this test case we should omit the container ports.

@monotosh-avi ping on this, any thoughts?

missed this, updated now

antoninbas · 2021-06-11T21:53:31Z

pkg/agent/nodeportlocal/npl_agent_test.go

+// It is verified that the Pod's NPL annotation and the local port table are updated
+// with only one port used as target port of the Service.


It verifies that the Pod's NPL annotation and the local port table are updated correctly, with only one port corresponding to the Service's single target port.

antoninbas · 2021-06-11T21:53:52Z

pkg/agent/nodeportlocal/npl_agent_test.go

+// TestPodAddHostPort creates a Pod with multiple host ports having same value but different protocol.
+// It is verified that the Pod's NPL annotation is updated with host port, without allocating extra port
+// from NPL pool. No NPL rule is required for this case.
+func TestPodAddMultiProtocol(t *testing.T) {


The test case name should include HostPort

antoninbas · 2021-06-11T21:54:14Z

pkg/agent/nodeportlocal/npl_agent_test.go

@@ -427,6 +496,76 @@ func TestPodAddHostPort(t *testing.T) {
 	assert.False(t, testData.portTable.RuleExists(defaultPodIP, defaultPort))
 }

+// TestPodAddHostPort creates a Pod with multiple host ports having same value but different protocol.
+// It is verified that the Pod's NPL annotation is updated with host port, without allocating extra port


s/It is verified/It verifies

antoninbas · 2021-06-11T21:56:25Z

test/e2e/nodeportlocal_test.go

+}
+
+// createNginxPodWithPort creates an nginx Pod in the test namespace with configurable container port.
+func createNginxPodWithPort(name, nodeName string, port int32) error {


I think we should stick with a method on the test data object for consistency:

func (data *testData) createNginxPodWithPort(...)

antoninbas · 2021-06-17T20:08:19Z

test/e2e/nodeportlocal_test.go

+// 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)
 }


I am still unsure about this function and it's use: since NPL doesn't require the ContainerPort to be set anymore, what's the point?

Also it doesn't seem that there is any value in setting it to 90 in tests. I feel like it is actually misleading since we never use 90 as the Service targetPort. Am I missing something?

I thought that adding a different port number in the pod explicitly would show that NPL doesn't depend on that port. But yeah, we don't need that. Removed.

antoninbas

LGTM

antoninbas · 2021-06-18T03:35:00Z

/test-all

antoninbas · 2021-06-18T04:58:16Z

/test-networkpolicy

monotosh-avi · 2021-06-19T01:43:36Z

/test-e2e

antoninbas · 2021-06-21T17:57:51Z

/test-all

antoninbas · 2021-06-21T20:32:13Z

/test-e2e
/test-networkpolicy

monotosh-avi · 2021-06-23T01:22:31Z

/test-e2e
/test-networkpolicy

antoninbas · 2021-06-23T16:37:24Z

@monotosh-avi I see that the e2e tests were consistently failing before your last push. Were you able to identify the root cause?

monotosh-avi · 2021-06-24T05:43:38Z

@monotosh-avi I see that the e2e tests were consistently failing before your last push. Were you able to identify the root cause?

@antoninbas it looked like there was some problem with the test setup.
fixtures.go:341: Error when tearing down test: error when deleting 'antrea-test' Namespace: Delete "https://192.168.9.154:6443/api/v1/namespaces/antrea-test": dial tcp 192.168.9.154:6443: connect: no route to host

I do see the similar issue with other tests too., e.g. https://jenkins.antrea-ci.rocks/job/antrea-e2e-for-pull-request/2747/console.

I didn't face this issue when I ran the tests in my local set up.

antoninbas · 2021-06-24T16:58:56Z

@monotosh-avi could you rebase main. Some fixes went in to address known stability issues with the test jobs. I want to make sure that the test failures were because of them and not something else. It may be a good opportunity to squash your commits too (the squashed commit must be signed). Thanks!

…lity Currently we use container ports of a Pod to program iptables rules to make the Pod reachable through a port in the Node. But container ports are not mandatory and multiple services can use different target ports for the same pod. Hence adding a change in NodePortLocal implementation, where target ports of all services would be used to program iptables rules and annotate pods. To implement this, target ports are being obtained from all the Services selecting a Pod, in the function handleAddUpdatePod(). Necessary changes in the tests have been added. Fixes antrea-io#1912 Signed-off-by: Monotosh Das <[email protected]>

monotosh-avi · 2021-06-24T18:55:07Z

@monotosh-avi could you rebase main. Some fixes went in to address known stability issues with the test jobs. I want to make sure that the test failures were because of them and not something else. It may be a good opportunity to squash your commits too (the squashed commit must be signed). Thanks!

@antoninbas rebased and squashed commits.

antoninbas · 2021-06-24T20:35:09Z

/test-all

antoninbas · 2021-06-25T03:47:07Z

Another --- FAIL: TestFlowAggregator/IPv4/RemoteServiceAccess (13.40s) failure
/skip-e2e

monotosh-avi force-pushed the monotosh-npl-svc-port branch from 99a684e to e644083 Compare May 31, 2021 06:53

monotosh-avi force-pushed the monotosh-npl-svc-port branch from e644083 to db74963 Compare June 7, 2021 08:35

antoninbas self-requested a review June 8, 2021 03:54

antoninbas reviewed Jun 8, 2021

View reviewed changes

monotosh-avi force-pushed the monotosh-npl-svc-port branch from db74963 to 7a4b5de Compare June 9, 2021 14:43

antoninbas reviewed Jun 9, 2021

View reviewed changes

antoninbas reviewed Jun 10, 2021

View reviewed changes

monotosh-avi force-pushed the monotosh-npl-svc-port branch from be14851 to be9ec9f Compare June 11, 2021 11:10

antoninbas reviewed Jun 11, 2021

View reviewed changes

monotosh-avi force-pushed the monotosh-npl-svc-port branch 2 times, most recently from 869430a to 86174a7 Compare June 17, 2021 18:49

antoninbas reviewed Jun 17, 2021

View reviewed changes

antoninbas previously approved these changes Jun 18, 2021

View reviewed changes

monotosh-avi dismissed antoninbas’s stale review via 6d62cc1 June 22, 2021 16:59

monotosh-avi force-pushed the monotosh-npl-svc-port branch from 0577357 to 6d62cc1 Compare June 22, 2021 16:59

monotosh-avi force-pushed the monotosh-npl-svc-port branch from 6d62cc1 to 686b2d9 Compare June 24, 2021 18:54

antoninbas approved these changes Jun 25, 2021

View reviewed changes

antoninbas merged commit 95973ae into antrea-io:main Jun 25, 2021

	// It is verified that the Pod's NPL annotationthe local port table are updated correctly.
	// It verifies that the Pod's NPL annotation and the local port table are updated correctly.

		// It is verified that the Pod's NPL annotation and the local port table are updated
		// with only one port used as target port of the Service.

Use target port of Service in NodePortLocal to configure Pod reachability #2222

Use target port of Service in NodePortLocal to configure Pod reachability #2222

Conversation

monotosh-avi commented May 28, 2021 • edited Loading

codecov-commenter commented May 28, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented Jun 18, 2021

antoninbas commented Jun 18, 2021

monotosh-avi commented Jun 19, 2021

antoninbas commented Jun 21, 2021

antoninbas commented Jun 21, 2021

monotosh-avi commented Jun 23, 2021

antoninbas commented Jun 23, 2021

monotosh-avi commented Jun 24, 2021

antoninbas commented Jun 24, 2021

monotosh-avi commented Jun 24, 2021

antoninbas commented Jun 24, 2021

antoninbas commented Jun 25, 2021

monotosh-avi commented May 28, 2021 •

edited

Loading

codecov-commenter commented May 28, 2021 •

edited

Loading