Merge pull request vmware-tanzu#542 from heypnus/fix/named-port-witho…

…ut-pod

Fix the issue when there's no pod without the specific named port

pkg/controllers/securitypolicy/pod_handler.go

@@ -102,8 +102,9 @@ var PredicateFuncsPod = predicate.Funcs{
 		oldObj := e.ObjectOld.(*v1.Pod)
 		newObj := e.ObjectNew.(*v1.Pod)
 		log.V(1).Info("receive pod update event", "namespace", oldObj.Namespace, "name", oldObj.Name)
-		if reflect.DeepEqual(oldObj.ObjectMeta.Labels, newObj.ObjectMeta.Labels) {
-			log.V(1).Info("label of pod is not changed, ignore it", "name", oldObj.Name)
+		// The NSX operator should handle the case when the pod phase is changed from Pending to Running.
+		if reflect.DeepEqual(oldObj.ObjectMeta.Labels, newObj.ObjectMeta.Labels) && oldObj.Status.Phase == newObj.Status.Phase {
+			log.V(1).Info("pod label and phase are not changed, ignore it", "name", oldObj.Name)
 			return false
 		}
 		if util.CheckPodHasNamedPort(*newObj, "update") {

pkg/nsx/services/securitypolicy/expand.go

@@ -169,24 +169,19 @@ func (service *SecurityPolicyService) resolveNamedPort(obj *v1alpha1.SecurityPol
 			return nil, err
 		}
 		for _, pod := range podsList.Items {
-			addr, err := service.resolvePodPort(pod, &spPort)
-			if errors.As(err, &nsxutil.PodIPNotFound{}) || errors.As(err, &nsxutil.PodNotRunning{}) {
-				return nil, err
-			}
+			addr := service.resolvePodPort(pod, &spPort)
 			portAddress = append(portAddress, addr...)
 		}
 	}
 
 	if len(portAddress) == 0 {
-		return nil, nsxutil.NoEffectiveOption{
-			Desc: "no pod has the corresponding named port",
-		}
+		log.Info("no pod has the corresponding named port", "port", spPort)
 	}
 	return nsxutil.MergeAddressByPort(portAddress), nil
 }
 
 // Check port name and protocol, only when the pod is really running, and it does have effective ip.
-func (service *SecurityPolicyService) resolvePodPort(pod v1.Pod, spPort *v1alpha1.SecurityPolicyPort) ([]nsxutil.PortAddress, error) {
+func (service *SecurityPolicyService) resolvePodPort(pod v1.Pod, spPort *v1alpha1.SecurityPolicyPort) []nsxutil.PortAddress {
 	var addr []nsxutil.PortAddress
 	for _, c := range pod.Spec.Containers {
 		container := c
@@ -196,12 +191,12 @@ func (service *SecurityPolicyService) resolvePodPort(pod v1.Pod, spPort *v1alpha
 				"protocol", port.Protocol, "podIP", pod.Status.PodIP)
 			if port.Name == spPort.Port.String() && port.Protocol == spPort.Protocol {
 				if pod.Status.Phase != "Running" {
-					errMsg := fmt.Sprintf("pod %s/%s is not running", pod.Namespace, pod.Name)
-					return nil, nsxutil.PodNotRunning{Desc: errMsg}
+					log.Info("pod with named port is not running", "pod.Namespace", pod.Namespace, "pod.Name", pod.Name)
+					return addr
 				}
 				if pod.Status.PodIP == "" {
-					errMsg := fmt.Sprintf("pod %s/%s ip not initialized", pod.Namespace, pod.Name)
-					return nil, nsxutil.PodIPNotFound{Desc: errMsg}
+					log.Info("pod with named port doesn't have initialized IP", "pod.Namespace", pod.Namespace, "pod.Name", pod.Name)
+					return addr
 				}
 				addr = append(
 					addr,
@@ -210,7 +205,7 @@ func (service *SecurityPolicyService) resolvePodPort(pod v1.Pod, spPort *v1alpha
 			}
 		}
 	}
-	return addr, nil
+	return addr
 }
 
 func (service *SecurityPolicyService) buildRuleIPSetGroupID(ruleModel *model.Rule) string {

test/e2e/framework.go

@@ -404,6 +404,31 @@ func (data *TestData) getCRResource(timeout time.Duration, cr string, namespace
 	return crs, nil
 }
 
+// deploymentWaitForNames polls the K8s apiServer once the specific pods are created, no matter they are running or not.
+func (data *TestData) deploymentWaitForNames(timeout time.Duration, namespace, deployment string) ([]string, error) {
+	var podNames []string
+	opt := metav1.ListOptions{
+		LabelSelector: "deployment=" + deployment,
+	}
+	err := wait.PollUntilContextTimeout(context.TODO(), 1*time.Second, timeout, false, func(ctx context.Context) (bool, error) {
+		if pods, err := data.clientset.CoreV1().Pods(namespace).List(context.TODO(), opt); err != nil {
+			if errors.IsNotFound(err) {
+				return false, nil
+			}
+			return false, fmt.Errorf("error when getting Pod  %v", err)
+		} else {
+			for _, p := range pods.Items {
+				podNames = append(podNames, p.Name)
+			}
+			return true, nil
+		}
+	})
+	if err != nil {
+		return nil, err
+	}
+	return podNames, nil
+}
+
 //Temporarily disable traffic check
 /*
 // podWaitFor polls the K8s apiServer until the specified Pod is found (in the test Namespace) and

test/e2e/manifest/testSecurityPolicy/named-port-without-pod.yaml

@@ -0,0 +1,50 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: tcp-deployment
+  namespace: web
+spec:
+  replicas: 2
+  selector:
+    matchLabels:
+      role: web
+  template:
+    metadata:
+      labels:
+        deployment: tcp-deployment
+        role: web
+    spec:
+      hostname: web-deployment
+      containers:
+        - name: web
+          image: "harbor-repo.vmware.com/dockerhub-proxy-cache/humanux/http_https_echo:latest"
+          imagePullPolicy: IfNotPresent
+          ports:
+            - containerPort: 80
+              name: web-port
+          resources:
+            requests: # make sure that the pod cannot be running
+              cpu: "10000m"
+              memory: "10000Gi"
+---
+apiVersion: nsx.vmware.com/v1alpha1
+kind: SecurityPolicy
+metadata:
+  name: named-port-policy-without-pod
+  namespace: web
+spec:
+  priority: 10
+  appliedTo:
+    - podSelector:
+        matchLabels:
+          role: web
+  rules:
+    - direction: in
+      action: allow
+      ports:
+        - protocol: TCP
+          port: web-port
+    - direction: in
+      action: drop
+    - direction: out
+      action: drop

test/e2e/nsx_security_policy_test.go

@@ -15,6 +15,7 @@
 package e2e
 
 import (
+	"fmt"
 	"path/filepath"
 	"testing"
 
@@ -233,6 +234,46 @@ func TestSecurityPolicyMatchExpression(t *testing.T) {
 	*/
 }
 
+// TestSecurityPolicyNamedPortWithoutPod verifies that the traffic of security policy when named port applied.
+// This test is to verify the named port feature of security policy.
+// When appliedTo is in policy level and there's no pod holding the related named ports.
+func TestSecurityPolicyNamedPortWithoutPod(t *testing.T) {
+	nsClient := "client"
+	nsWeb := "web"
+	securityPolicyCRName := "named-port-policy-without-pod"
+	securityPolicyNSXDisplayName := fmt.Sprintf("sp-%s-%s", nsWeb, securityPolicyCRName)
+	webA := "web"
+	labelWeb := "tcp-deployment"
+	ruleName0 := "all-ingress-isolation"
+	ruleName1 := "all-egress-isolation"
+
+	testData.deleteNamespace(nsClient, defaultTimeout)
+	testData.deleteNamespace(nsWeb, defaultTimeout)
+	_ = testData.createNamespace(nsClient)
+	_ = testData.createNamespace(nsWeb)
+	defer testData.deleteNamespace(nsClient, defaultTimeout)
+	defer testData.deleteNamespace(nsWeb, defaultTimeout)
+
+	// Create all
+	yamlPath, _ := filepath.Abs("./manifest/testSecurityPolicy/named-port-without-pod.yaml")
+	_ = applyYAML(yamlPath, "")
+	defer deleteYAML(yamlPath, "")
+
+	psb, err := testData.deploymentWaitForNames(defaultTimeout, nsWeb, labelWeb)
+	t.Logf("Pods are %v", psb)
+	assertNil(t, err, "Error when waiting for IP for Pod %s", webA)
+	err = testData.waitForCRReadyOrDeleted(defaultTimeout, SP, nsWeb, securityPolicyCRName, Ready)
+	assertNil(t, err, "Error when waiting for Security Policy %s", securityPolicyCRName)
+
+	// Check NSX resource existing
+	err = testData.waitForResourceExistOrNot(nsWeb, common.ResourceTypeSecurityPolicy, securityPolicyNSXDisplayName, true)
+	assertNil(t, err)
+	err = testData.waitForResourceExistOrNot(nsWeb, common.ResourceTypeRule, ruleName0, true)
+	assertNil(t, err)
+	err = testData.waitForResourceExistOrNot(nsWeb, common.ResourceTypeRule, ruleName1, true)
+	assertNil(t, err)
+}
+
 /*
 // TestSecurityPolicyNamedPort0 verifies that the traffic of security policy when named port applied.
 // This test is to verify the named port feature of security policy.