Pod filtering for Services based on Namespaces in NodePortLocal #1927

chauhanshubham · 2021-03-01T13:21:20Z

Also includes UT to check for namespace based pod filtering via Services.
Fixes #1926

codecov-io · 2021-03-01T14:02:53Z

Codecov Report

Merging #1927 (7d9dc3b) into main (cdf357f) will decrease coverage by 0.38%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1927      +/-   ##
==========================================
- Coverage   41.56%   41.17%   -0.39%     
==========================================
  Files         201      116      -85     
  Lines       17374    14693    -2681     
==========================================
- Hits         7221     6050    -1171     
+ Misses       9123     8124     -999     
+ Partials     1030      519     -511

Flag	Coverage Δ
kind-e2e-tests	`?`
unit-tests	`41.17% <0.00%> (?)`

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	`0.00% <0.00%> (ø)`
pkg/agent/agent_linux.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/ovs/openflow/default.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/agent/config/node_config.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/agent/cniserver/pod_configuration_linux.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/apis/controlplane/v1beta1/register.go	`0.00% <0.00%> (-92.86%)`	⬇️
pkg/ovs/openflow/logs.go	`9.52% <0.00%> (-90.48%)`	⬇️
pkg/apis/controlplane/register.go	`0.00% <0.00%> (-88.24%)`	⬇️
pkg/agent/client.go	`0.00% <0.00%> (-77.42%)`	⬇️
...agent/controller/traceflow/traceflow_controller.go	`0.00% <0.00%> (-75.10%)`	⬇️
... and 179 more

antoninbas

Please link this PR to the appropriate issue by adding Fixes #1926 to the commit message and PR description. You can also remove (#1926) from the commit title. The idea is that the issue should be closed automatically when the PR is merged, not just to reference the issue in the commit message.

antoninbas · 2021-03-01T19:59:21Z

pkg/agent/nodeportlocal/k8s/npl_controller.go

@@ -255,7 +255,9 @@ func (c *NPLController) getPodsFromService(svc *corev1.Service) []string {
 		return pods
 	}
 	for _, pod := range podList {
-		pods = append(pods, podKeyFunc(pod))
+		if pod.GetNamespace() == svc.GetNamespace() {


Instead of c.podLister.List(labels.SelectorFromSet(labels.Set(svc.Spec.Selector))) above, you should get a PodNamespaceLister object first and list only Pods from that Namespace. The following should work and will be more efficient if many Pods across different Namespaces share the same labels:

podList, err := c.podLister.Pods(svc.Namespace).List(labels.SelectorFromSet(labels.Set(svc.Spec.Selector)))

antoninbas · 2021-03-01T20:01:02Z

pkg/agent/nodeportlocal/k8s/npl_controller.go

@@ -272,7 +274,8 @@ func (c *NPLController) isNPLEnabledForServiceOfPod(obj interface{}) bool {
 		svc, isSvc := service.(*corev1.Service)
 		// Selecting Services NOT of type NodePort, with Service selector matching Pod labels.
 		if isSvc && svc.Spec.Type != corev1.ServiceTypeNodePort {
-			if matchSvcSelectorPodLabels(svc.Spec.Selector, pod.GetLabels()) {
+			if matchSvcSelectorPodLabels(svc.Spec.Selector, pod.GetLabels()) &&
+				pod.GetNamespace() == svc.GetNamespace() {


not really specific to this PR, but is there a difference between using the getters (e.g. pod.GetNamespace()) or accessing the fields directly (e.g. pod.Namespace)? @tnqn do you know?

I think no real difference, but I woud just use pod.Namespace.

I agree, it would be good to make it uniform and stop using the getters in the NPL controller. We don't use them anywhere else.

The GetXXX methods are normally used when a function needs to be generic to handle different kinds of objects. The object is then converted to or passed in as metav1.Object

pkg/agent/nodeportlocal/npl_agent_test.go

…n NodePortLocal Also includes UT to check for namespace based pod filtering via Services.

jianjuns · 2021-03-02T01:04:08Z

pkg/agent/nodeportlocal/k8s/npl_controller.go

@@ -272,7 +274,8 @@ func (c *NPLController) isNPLEnabledForServiceOfPod(obj interface{}) bool {
 		svc, isSvc := service.(*corev1.Service)
 		// Selecting Services NOT of type NodePort, with Service selector matching Pod labels.
 		if isSvc && svc.Spec.Type != corev1.ServiceTypeNodePort {
-			if matchSvcSelectorPodLabels(svc.Spec.Selector, pod.GetLabels()) {
+			if matchSvcSelectorPodLabels(svc.Spec.Selector, pod.GetLabels()) &&
+				pod.GetNamespace() == svc.GetNamespace() {


I think no real difference, but I woud just use pod.Namespace.

jianjuns · 2021-03-02T01:05:34Z

pkg/agent/nodeportlocal/k8s/npl_controller.go

@@ -272,7 +272,8 @@ func (c *NPLController) isNPLEnabledForServiceOfPod(obj interface{}) bool {
 		svc, isSvc := service.(*corev1.Service)
 		// Selecting Services NOT of type NodePort, with Service selector matching Pod labels.
 		if isSvc && svc.Spec.Type != corev1.ServiceTypeNodePort {
-			if matchSvcSelectorPodLabels(svc.Spec.Selector, pod.GetLabels()) {
+			if matchSvcSelectorPodLabels(svc.Spec.Selector, pod.GetLabels()) &&
+				pod.GetNamespace() == svc.GetNamespace() {


Why not compare Namespace first before labels, which can drop many Pods faster?

tnqn

LGTM

antoninbas

LGTM

antoninbas · 2021-03-03T21:15:49Z

/test-e2e

chauhanshubham · 2021-03-04T13:58:19Z

/test-networkpolicy

chauhanshubham · 2021-03-04T13:58:58Z

/test-conformance

…ea-io#1927) Fixes antrea-io#1926

Fixes #1926

vmwclabot added the cla-not-required label Mar 1, 2021

chauhanshubham requested review from antoninbas, tnqn, jianjuns and monotosh-avi March 1, 2021 13:22

monotosh-avi previously approved these changes Mar 1, 2021

View reviewed changes

antoninbas reviewed Mar 1, 2021

View reviewed changes

chauhanshubham changed the title ~~Fixes Pod filtering for Services based on Namespaces in NodePortLocal (#1926)~~ Fixes Pod filtering for Services based on Namespaces in NodePortLocal Mar 1, 2021

Fixes antrea-io#1926 Pod filtering for Services based on Namespaces i…

dcbdd5f

…n NodePortLocal Also includes UT to check for namespace based pod filtering via Services.

chauhanshubham changed the title ~~Fixes Pod filtering for Services based on Namespaces in NodePortLocal~~ Pod filtering for Services based on Namespaces in NodePortLocal Mar 1, 2021

chauhanshubham dismissed monotosh-avi’s stale review via dcbdd5f March 1, 2021 20:18

chauhanshubham force-pushed the podfilter-namespace-npl branch from e449cb1 to dcbdd5f Compare March 1, 2021 20:18

jianjuns reviewed Mar 2, 2021

View reviewed changes

addressing review comments

7d9dc3b

tnqn previously approved these changes Mar 2, 2021

View reviewed changes

avoid using getter functions for k8s namespace

50bdb37

chauhanshubham dismissed tnqn’s stale review via 50bdb37 March 3, 2021 20:40

antoninbas approved these changes Mar 3, 2021

View reviewed changes

tnqn approved these changes Mar 4, 2021

View reviewed changes

antoninbas merged commit 77e497b into antrea-io:main Mar 6, 2021

tnqn pushed a commit to tnqn/antrea that referenced this pull request Mar 17, 2021

Pod filtering for Services based on Namespaces in NodePortLocal (antr…

684ab23

…ea-io#1927) Fixes antrea-io#1926

antoninbas pushed a commit to antoninbas/antrea that referenced this pull request Apr 30, 2021

Pod filtering for Services based on Namespaces in NodePortLocal (antr…

1630db9

…ea-io#1927) Fixes antrea-io#1926

antoninbas pushed a commit to antoninbas/antrea that referenced this pull request Apr 30, 2021

Pod filtering for Services based on Namespaces in NodePortLocal (antr…

18a721e

…ea-io#1927) Fixes antrea-io#1926

antoninbas pushed a commit that referenced this pull request Apr 30, 2021

Pod filtering for Services based on Namespaces in NodePortLocal (#1927)

a39bfd2

Fixes #1926

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pod filtering for Services based on Namespaces in NodePortLocal #1927

Pod filtering for Services based on Namespaces in NodePortLocal #1927

chauhanshubham commented Mar 1, 2021 •

edited

Loading

codecov-io commented Mar 1, 2021 •

edited

Loading

antoninbas left a comment

antoninbas Mar 1, 2021

antoninbas Mar 1, 2021

jianjuns Mar 2, 2021

antoninbas Mar 3, 2021

tnqn Mar 4, 2021

jianjuns Mar 2, 2021

jianjuns Mar 2, 2021

tnqn left a comment

antoninbas left a comment

antoninbas commented Mar 3, 2021

chauhanshubham commented Mar 4, 2021

chauhanshubham commented Mar 4, 2021

Pod filtering for Services based on Namespaces in NodePortLocal #1927

Pod filtering for Services based on Namespaces in NodePortLocal #1927

Conversation

chauhanshubham commented Mar 1, 2021 • edited Loading

codecov-io commented Mar 1, 2021 • edited Loading

Codecov Report

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas Mar 1, 2021

Choose a reason for hiding this comment

antoninbas Mar 1, 2021

Choose a reason for hiding this comment

jianjuns Mar 2, 2021

Choose a reason for hiding this comment

antoninbas Mar 3, 2021

Choose a reason for hiding this comment

tnqn Mar 4, 2021

Choose a reason for hiding this comment

jianjuns Mar 2, 2021

Choose a reason for hiding this comment

jianjuns Mar 2, 2021

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented Mar 3, 2021

chauhanshubham commented Mar 4, 2021

chauhanshubham commented Mar 4, 2021

chauhanshubham commented Mar 1, 2021 •

edited

Loading

codecov-io commented Mar 1, 2021 •

edited

Loading