diff --git a/pkg/controllers/netpol/pod.go b/pkg/controllers/netpol/pod.go index 17f69de0..1f99560c 100644 --- a/pkg/controllers/netpol/pod.go +++ b/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) } }, diff --git a/pkg/controllers/netpol/utils.go b/pkg/controllers/netpol/utils.go index 9428bb67..1e685857 100644 --- a/pkg/controllers/netpol/utils.go +++ b/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 } diff --git a/pkg/controllers/netpol/utils_test.go b/pkg/controllers/netpol/utils_test.go index 1bfb0748..834c975b 100644 --- a/pkg/controllers/netpol/utils_test.go +++ b/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