fix(NPC): check if pod is actionable

Check if the Pod is actionable before taking NetworkPolicy actions which
includes both adding KUBE-POD-FW and KUBE-NWPLCY chains for it.

Checks have now been consolidated to a single isNetPolActionable()
function which checks for pod phases that we don't want NetworkPolicy
for like: Failed, Completed, and Succeeded, missing pod IP addresses,
and pods with HostNetwork enabled.

fixes cloudnativelabs#1056

Author: aauren

pkg/controllers/netpol/pod.go

@@ -226,8 +226,8 @@ func (npc *NetworkPolicyController) getIngressNetworkPolicyEnabledPods(networkPo
 	for _, obj := range npc.podLister.List() {
 		pod := obj.(*api.Pod)
 
-		// ignore the pods running on the different node or running in host network
-		if strings.Compare(pod.Status.HostIP, nodeIP) != 0 || pod.Spec.HostNetwork {
+		// ignore the pods running on the different node and pods that are not actionable
+		if strings.Compare(pod.Status.HostIP, nodeIP) != 0 || !isNetPolActionable(pod) {
 			continue
 		}
 
@@ -246,8 +246,8 @@ func (npc *NetworkPolicyController) getIngressNetworkPolicyEnabledPods(networkPo
 			}
 		}
 	}
-	return &nodePods, nil
 
+	return &nodePods, nil
 }
 
 func (npc *NetworkPolicyController) getEgressNetworkPolicyEnabledPods(networkPoliciesInfo []networkPolicyInfo, nodeIP string) (*map[string]podInfo, error) {
@@ -257,10 +257,11 @@ func (npc *NetworkPolicyController) getEgressNetworkPolicyEnabledPods(networkPol
 	for _, obj := range npc.podLister.List() {
 		pod := obj.(*api.Pod)
 
-		// ignore the pods running on the different node or running in host network
-		if strings.Compare(pod.Status.HostIP, nodeIP) != 0 || pod.Spec.HostNetwork {
+		// ignore the pods running on the different node and pods that are not actionable
+		if strings.Compare(pod.Status.HostIP, nodeIP) != 0 || !isNetPolActionable(pod) {
 			continue
 		}
+
 		for _, policy := range networkPoliciesInfo {
 			if policy.namespace != pod.ObjectMeta.Namespace {
 				continue
@@ -276,6 +277,7 @@ func (npc *NetworkPolicyController) getEgressNetworkPolicyEnabledPods(networkPol
 			}
 		}
 	}
+
 	return &nodePods, nil
 }
 

pkg/controllers/netpol/policy.go

@@ -483,7 +483,7 @@ func (npc *NetworkPolicyController) buildNetworkPoliciesInfo() ([]networkPolicyI
 		namedPort2IngressEps := make(namedPort2eps)
 		if err == nil {
 			for _, matchingPod := range matchingPods {
-				if matchingPod.Status.PodIP == "" {
+				if !isNetPolActionable(matchingPod) {
 					continue
 				}
 				newPolicy.targetPods[matchingPod.Status.PodIP] = podInfo{ip: matchingPod.Status.PodIP,
@@ -519,7 +519,7 @@ func (npc *NetworkPolicyController) buildNetworkPoliciesInfo() ([]networkPolicyI
 				for _, peer := range specIngressRule.From {
 					if peerPods, err := npc.evalPodPeer(policy, peer); err == nil {
 						for _, peerPod := range peerPods {
-							if peerPod.Status.PodIP == "" {
+							if !isNetPolActionable(peerPod) {
 								continue
 							}
 							ingressRule.srcPods = append(ingressRule.srcPods,
@@ -560,7 +560,7 @@ func (npc *NetworkPolicyController) buildNetworkPoliciesInfo() ([]networkPolicyI
 				if policyRulePortsHasNamedPort(specEgressRule.Ports) {
 					matchingPeerPods, _ := npc.ListPodsByNamespaceAndLabels(policy.Namespace, labels.Everything())
 					for _, peerPod := range matchingPeerPods {
-						if peerPod.Status.PodIP == "" {
+						if !isNetPolActionable(peerPod) {
 							continue
 						}
 						npc.grabNamedPortFromPod(peerPod, &namedPort2EgressEps)
@@ -571,7 +571,7 @@ func (npc *NetworkPolicyController) buildNetworkPoliciesInfo() ([]networkPolicyI
 				for _, peer := range specEgressRule.To {
 					if peerPods, err := npc.evalPodPeer(policy, peer); err == nil {
 						for _, peerPod := range peerPods {
-							if peerPod.Status.PodIP == "" {
+							if !isNetPolActionable(peerPod) {
 								continue
 							}
 							egressRule.dstPods = append(egressRule.dstPods,

pkg/controllers/netpol/utils.go

@@ -4,8 +4,26 @@ import (
 	"fmt"
 	"regexp"
 	"strconv"
+
+	api "k8s.io/api/core/v1"
+)
+
+const (
+	PodCompleted api.PodPhase = "Completed"
 )
 
+func isNetPolActionable(pod *api.Pod) bool {
+	return !isFinished(pod) && pod.Status.PodIP != "" && !pod.Spec.HostNetwork
+}
+
+func isFinished(pod *api.Pod) bool {
+	switch pod.Status.Phase {
+	case api.PodFailed, api.PodSucceeded, PodCompleted:
+		return true
+	}
+	return false
+}
+
 func validateNodePortRange(nodePortOption string) (string, error) {
 	nodePortValidator := regexp.MustCompile(`^([0-9]+)[:-]([0-9]+)$`)
 	if matched := nodePortValidator.MatchString(nodePortOption); !matched {

pkg/controllers/netpol/utils_test.go

@@ -4,8 +4,89 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	api "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+var (
+	fakePod = api.Pod{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "testpod",
+			Namespace: "testnamespace",
+			Labels:    map[string]string{"foo": "bar"}},
+		Spec: api.PodSpec{
+			Containers: []api.Container{
+				{
+					Image: "k8s.gcr.io/busybox",
+				},
+			},
+		},
+		Status: api.PodStatus{
+			PodIP: "172.16.0.1",
+			PodIPs: []api.PodIP{
+				{
+					IP: "172.16.0.1",
+				},
+			},
+			HostIP: "10.0.0.1",
+			Phase:  api.PodRunning,
+		},
+	}
+)
+
+func Test_isPodFinished(t *testing.T) {
+	t.Run("Failed pod should be detected as finished", func(t *testing.T) {
+		fakePod.Status.Phase = api.PodFailed
+		assert.True(t, isFinished(&fakePod))
+	})
+	t.Run("Succeeded pod should be detected as finished", func(t *testing.T) {
+		fakePod.Status.Phase = api.PodSucceeded
+		assert.True(t, isFinished(&fakePod))
+	})
+	t.Run("Completed pod should be detected as finished", func(t *testing.T) {
+		fakePod.Status.Phase = PodCompleted
+		assert.True(t, isFinished(&fakePod))
+	})
+	t.Run("Running pod should NOT be detected as finished", func(t *testing.T) {
+		fakePod.Status.Phase = api.PodRunning
+		assert.False(t, isFinished(&fakePod))
+	})
+	t.Run("Pending pod should NOT be detected as finished", func(t *testing.T) {
+		fakePod.Status.Phase = api.PodPending
+		assert.False(t, isFinished(&fakePod))
+	})
+	t.Run("Unknown pod should NOT be detected as finished", func(t *testing.T) {
+		fakePod.Status.Phase = api.PodUnknown
+		assert.False(t, isFinished(&fakePod))
+	})
+}
+
+func Test_isNetPolActionable(t *testing.T) {
+	t.Run("Normal pod should be actionable", func(t *testing.T) {
+		assert.True(t, isNetPolActionable(&fakePod))
+	})
+	t.Run("Pod without Pod IP should not be actionable", func(t *testing.T) {
+		fakePod.Status.PodIP = ""
+		assert.False(t, isNetPolActionable(&fakePod))
+	})
+	t.Run("Finished Pod should not be actionable", func(t *testing.T) {
+		fakePod.Status.Phase = api.PodFailed
+		assert.False(t, isNetPolActionable(&fakePod))
+		fakePod.Status.Phase = api.PodSucceeded
+		assert.False(t, isNetPolActionable(&fakePod))
+		fakePod.Status.Phase = PodCompleted
+		assert.False(t, isNetPolActionable(&fakePod))
+	})
+	t.Run("Host Networked Pod should not be actionable", func(t *testing.T) {
+		fakePod.Spec.HostNetwork = true
+		assert.False(t, isNetPolActionable(&fakePod))
+	})
+}
+
 func Test_NewNetworkPolicyController(t *testing.T) {
 	t.Run("Node Port range specified with a hyphen should pass validation", func(t *testing.T) {
 		portRange, err := validateNodePortRange("1000-2000")