From cf507b1314a3864ad74cb949529088044f0dfdca Mon Sep 17 00:00:00 2001 From: Qian Sun Date: Sun, 7 Apr 2024 11:02:33 +0000 Subject: [PATCH] Fix the issue when there's no pod without the specific named port About the named port feature, when there's not any pod with the specific port in the policy (e.g. the pod not running, or even there's not any pod), the NSX operator will reports the error and the reconcile will fail. It's not expected and the policy's deletion is also blocked by the same cause. This patch is to fix this issue: the security policy will still be created even though its named port doesn't match any pod. Testings done: A. 1. create the security policy CR without any pod matching its named port 2. check the NSX security policy can be created 3. create the pod matching the named port and wait until it's running 4. confirmed that the new rule is created in the existing NSX security policy B. 1. create the pod with named port but keep it in pending phase 2. create the security policy CR 3. check the NSX security policy can be created 4. make the pod be running 5. confirmed that the new rule is created in the existing NSX security policy C. 1. create the security policy CR without any pod matching its named port 2. delete the security policy CR 3. confirmed that the NSX security policy can be removed --- pkg/controllers/securitypolicy/pod_handler.go | 5 +- pkg/nsx/services/securitypolicy/expand.go | 21 +++----- test/e2e/framework.go | 25 ++++++++++ .../named-port-without-pod.yaml | 50 +++++++++++++++++++ test/e2e/nsx_security_policy_test.go | 41 +++++++++++++++ 5 files changed, 127 insertions(+), 15 deletions(-) create mode 100644 test/e2e/manifest/testSecurityPolicy/named-port-without-pod.yaml diff --git a/pkg/controllers/securitypolicy/pod_handler.go b/pkg/controllers/securitypolicy/pod_handler.go index 48d95ae30..bab18b7fc 100644 --- a/pkg/controllers/securitypolicy/pod_handler.go +++ b/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") { diff --git a/pkg/nsx/services/securitypolicy/expand.go b/pkg/nsx/services/securitypolicy/expand.go index 18e3ad15f..f6af9e159 100644 --- a/pkg/nsx/services/securitypolicy/expand.go +++ b/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 { diff --git a/test/e2e/framework.go b/test/e2e/framework.go index fc7d07f50..a321d135b 100644 --- a/test/e2e/framework.go +++ b/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 diff --git a/test/e2e/manifest/testSecurityPolicy/named-port-without-pod.yaml b/test/e2e/manifest/testSecurityPolicy/named-port-without-pod.yaml new file mode 100644 index 000000000..6d5a18d66 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/test/e2e/nsx_security_policy_test.go b/test/e2e/nsx_security_policy_test.go index a338edc15..5bef4affc 100644 --- a/test/e2e/nsx_security_policy_test.go +++ b/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.