From d90c7532867be9d9d405b918c9cab50f8999c470 Mon Sep 17 00:00:00 2001 From: Aaron U'Ren Date: Sun, 23 May 2021 18:19:43 -0500 Subject: [PATCH] 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. --- pkg/controllers/netpol/pod.go | 22 +++++++++++----- pkg/controllers/netpol/utils.go | 15 +++++++++++ pkg/controllers/netpol/utils_test.go | 38 ++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 7 deletions(-) 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