fact(NPC): refactor isPodUpdateNetPolRelevant

Refactor this logic so that it can be more easily tested and expanded
without cluttering the pod.go file. Additionally, add some safe guards
around the pod cast to ensure that we're working with pods before we
pass them.

Author: aauren

pkg/controllers/netpol/pod.go

@@ -3,7 +3,6 @@ package netpol
 import (
 	"crypto/sha256"
 	"encoding/base32"
-	"reflect"
 	"strings"
 
 	api "k8s.io/api/core/v1"
@@ -23,12 +22,21 @@ func (npc *NetworkPolicyController) newPodEventHandler() cache.ResourceEventHand
 			}
 		},
 		UpdateFunc: func(oldObj, newObj interface{}) {
-			newPodObj := newObj.(*api.Pod)
-			oldPodObj := oldObj.(*api.Pod)
-			if newPodObj.Status.Phase != oldPodObj.Status.Phase ||
-				newPodObj.Status.PodIP != oldPodObj.Status.PodIP ||
-				!reflect.DeepEqual(newPodObj.Labels, oldPodObj.Labels) {
-				// for the network policies, we are only interested in pod status phase change or IP change
+			var newPodObj, oldPodObj *api.Pod
+			var ok bool
+
+			// If either of these objects are not pods, quit now
+			if newPodObj, ok = newObj.(*api.Pod); !ok {
+				return
+			}
+			if oldPodObj, ok = oldObj.(*api.Pod); !ok {
+				return
+			}
+
+			// We don't check isNetPolActionable here, because if it is transitioning in or out of the actionable state
+			// we want to run the full sync so that it can be added or removed from the existing network policy of the host
+			// For the network policies, we are only interested in some changes, most pod changes aren't relevant to network policy
+			if isPodUpdateNetPolRelevant(oldPodObj, newPodObj) {
 				npc.OnPodUpdate(newObj)
 			}
 		},

pkg/controllers/netpol/utils.go

@@ -2,6 +2,7 @@ package netpol
 
 import (
 	"fmt"
+	"reflect"
 	"regexp"
 	"strconv"
 
@@ -12,6 +13,20 @@ const (
 	PodCompleted api.PodPhase = "Completed"
 )
 
+// isPodUpdateNetPolRelevant checks the attributes that we care about for building NetworkPolicies on the host and if it
+// finds a relevant change, it returns true otherwise it returns false. The things we care about for NetworkPolicies:
+// 1) Is the phase of the pod changing? (matters for catching completed, succeeded, or failed jobs)
+// 2) Is the pod IP changing? (changes how the network policy is applied to the host)
+// 3) Is the pod's host IP changing? (should be caught in the above, with the CNI kube-router runs with but we check this as well for sanity)
+// 4) Is a pod's label changing? (potentially changes which NetworkPolicies select this pod)
+func isPodUpdateNetPolRelevant(oldPod, newPod *api.Pod) bool {
+	return newPod.Status.Phase != oldPod.Status.Phase ||
+		newPod.Status.PodIP != oldPod.Status.PodIP ||
+		!reflect.DeepEqual(newPod.Status.PodIPs, oldPod.Status.PodIPs) ||
+		newPod.Status.HostIP != oldPod.Status.HostIP ||
+		!reflect.DeepEqual(newPod.Labels, oldPod.Labels)
+}
+
 func isNetPolActionable(pod *api.Pod) bool {
 	return !isFinished(pod) && pod.Status.PodIP != "" && !pod.Spec.HostNetwork
 }

pkg/controllers/netpol/utils_test.go

@@ -38,6 +38,44 @@ var (
 	}
 )
 
+func Test_isPodUpdateNetPolRelevant(t *testing.T) {
+	t.Run("Pod phase change should be detected as NetworkPolicy relevant", func(t *testing.T) {
+		newPod := fakePod.DeepCopy()
+		newPod.Status.Phase = api.PodFailed
+		assert.True(t, isPodUpdateNetPolRelevant(&fakePod, newPod))
+	})
+	t.Run("Pod IP change should be detected as NetworkPolicy relevant", func(t *testing.T) {
+		newPod := fakePod.DeepCopy()
+		newPod.Status.PodIP = "172.16.0.2"
+		assert.True(t, isPodUpdateNetPolRelevant(&fakePod, newPod))
+	})
+	t.Run("Pod IPs change should be detected as NetworkPolicy relevant", func(t *testing.T) {
+		newPod := fakePod.DeepCopy()
+		newPod.Status.PodIPs = []api.PodIP{{IP: "172.16.0.2"}}
+		assert.True(t, isPodUpdateNetPolRelevant(&fakePod, newPod))
+	})
+	t.Run("Pod Label change should be detected as NetworkPolicy relevant", func(t *testing.T) {
+		newPod := fakePod.DeepCopy()
+		newPod.ObjectMeta.Labels = map[string]string{"bar": "foo"}
+		assert.True(t, isPodUpdateNetPolRelevant(&fakePod, newPod))
+	})
+	t.Run("Pod Host IP change should be detected as NetworkPolicy relevant", func(t *testing.T) {
+		newPod := fakePod.DeepCopy()
+		newPod.Status.HostIP = "10.0.0.2"
+		assert.True(t, isPodUpdateNetPolRelevant(&fakePod, newPod))
+	})
+	t.Run("Pod Image change should NOT be detected as NetworkPolicy relevant", func(t *testing.T) {
+		newPod := fakePod.DeepCopy()
+		newPod.Spec.Containers[0].Image = "k8s.gcr.io/otherimage"
+		assert.False(t, isPodUpdateNetPolRelevant(&fakePod, newPod))
+	})
+	t.Run("Pod Name change should NOT be detected as NetworkPolicy relevant", func(t *testing.T) {
+		newPod := fakePod.DeepCopy()
+		newPod.ObjectMeta.Name = "otherpod"
+		assert.False(t, isPodUpdateNetPolRelevant(&fakePod, newPod))
+	})
+}
+
 func Test_isPodFinished(t *testing.T) {
 	t.Run("Failed pod should be detected as finished", func(t *testing.T) {
 		fakePod.Status.Phase = api.PodFailed