From 9fb6e2ef55a7e42dfb5749617046dae71435eed9 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Sun, 24 May 2020 11:55:09 -0400 Subject: [PATCH] Fix Endpoint/EndpointSlice pod change detection The endpoint controllers responded to Pod changes by trying to figure out if the generated endpoint resource would change, rather than just checking if the Pod had changed, but since the set of Pod fields that need to be checked depend on the Service and Node as well, the code ended up only checking for a subset of the changes it should have. In particular, EndpointSliceController ended up only looking at IPv4 Pod IPs when processing Pod update events, so when a Pod went from having no IP to having only an IPv6 IP, EndpointSliceController would think it hadn't changed. --- pkg/controller/endpoint/BUILD | 1 - .../endpoint/endpoints_controller.go | 64 +++--- .../endpoint/endpoints_controller_test.go | 182 +----------------- .../endpointslice/endpointslice_controller.go | 2 +- pkg/controller/endpointslice/utils.go | 15 +- pkg/controller/endpointslice/utils_test.go | 63 ------ .../util/endpoint/controller_utils.go | 35 ++-- .../util/endpoint/controller_utils_test.go | 165 ++++++++++++++++ 8 files changed, 217 insertions(+), 310 deletions(-) diff --git a/pkg/controller/endpoint/BUILD b/pkg/controller/endpoint/BUILD index 159f84b1cd6..8767dc22ccd 100644 --- a/pkg/controller/endpoint/BUILD +++ b/pkg/controller/endpoint/BUILD @@ -48,7 +48,6 @@ go_test( "//pkg/api/v1/endpoints:go_default_library", "//pkg/apis/core:go_default_library", "//pkg/controller:go_default_library", - "//pkg/controller/util/endpoint:go_default_library", "//pkg/features:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", diff --git a/pkg/controller/endpoint/endpoints_controller.go b/pkg/controller/endpoint/endpoints_controller.go index 769fc773a9d..d68b01fce2b 100644 --- a/pkg/controller/endpoint/endpoints_controller.go +++ b/pkg/controller/endpoint/endpoints_controller.go @@ -19,7 +19,6 @@ package endpoint import ( "context" "fmt" - "reflect" "strconv" "time" @@ -213,39 +212,33 @@ func (e *EndpointController) addPod(obj interface{}) { } func podToEndpointAddressForService(svc *v1.Service, pod *v1.Pod) (*v1.EndpointAddress, error) { + var endpointIP string + if !utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) { - return podToEndpointAddress(pod), nil - } + endpointIP = pod.Status.PodIP + } else { + // api-server service controller ensured that the service got the correct IP Family + // according to user setup, here we only need to match EndPoint IPs' family to service + // actual IP family. as in, we don't need to check service.IPFamily - // api-server service controller ensured that the service got the correct IP Family - // according to user setup, here we only need to match EndPoint IPs' family to service - // actual IP family. as in, we don't need to check service.IPFamily - - ipv6ClusterIP := utilnet.IsIPv6String(svc.Spec.ClusterIP) - for _, podIP := range pod.Status.PodIPs { - ipv6PodIP := utilnet.IsIPv6String(podIP.IP) - // same family? - // TODO (khenidak) when we remove the max of 2 PodIP limit from pods - // we will have to return multiple endpoint addresses - if ipv6ClusterIP == ipv6PodIP { - return &v1.EndpointAddress{ - IP: podIP.IP, - NodeName: &pod.Spec.NodeName, - TargetRef: &v1.ObjectReference{ - Kind: "Pod", - Namespace: pod.ObjectMeta.Namespace, - Name: pod.ObjectMeta.Name, - UID: pod.ObjectMeta.UID, - ResourceVersion: pod.ObjectMeta.ResourceVersion, - }}, nil + ipv6ClusterIP := utilnet.IsIPv6String(svc.Spec.ClusterIP) + for _, podIP := range pod.Status.PodIPs { + ipv6PodIP := utilnet.IsIPv6String(podIP.IP) + // same family? + // TODO (khenidak) when we remove the max of 2 PodIP limit from pods + // we will have to return multiple endpoint addresses + if ipv6ClusterIP == ipv6PodIP { + endpointIP = podIP.IP + break + } + } + if endpointIP == "" { + return nil, fmt.Errorf("failed to find a matching endpoint for service %v", svc.Name) } } - return nil, fmt.Errorf("failed to find a matching endpoint for service %v", svc.Name) -} -func podToEndpointAddress(pod *v1.Pod) *v1.EndpointAddress { return &v1.EndpointAddress{ - IP: pod.Status.PodIP, + IP: endpointIP, NodeName: &pod.Spec.NodeName, TargetRef: &v1.ObjectReference{ Kind: "Pod", @@ -253,24 +246,15 @@ func podToEndpointAddress(pod *v1.Pod) *v1.EndpointAddress { Name: pod.ObjectMeta.Name, UID: pod.ObjectMeta.UID, ResourceVersion: pod.ObjectMeta.ResourceVersion, - }} -} - -func endpointChanged(pod1, pod2 *v1.Pod) bool { - endpointAddress1 := podToEndpointAddress(pod1) - endpointAddress2 := podToEndpointAddress(pod2) - - endpointAddress1.TargetRef.ResourceVersion = "" - endpointAddress2.TargetRef.ResourceVersion = "" - - return !reflect.DeepEqual(endpointAddress1, endpointAddress2) + }, + }, nil } // When a pod is updated, figure out what services it used to be a member of // and what services it will be a member of, and enqueue the union of these. // old and cur must be *v1.Pod types. func (e *EndpointController) updatePod(old, cur interface{}) { - services := endpointutil.GetServicesToUpdateOnPodChange(e.serviceLister, e.serviceSelectorCache, old, cur, endpointChanged) + services := endpointutil.GetServicesToUpdateOnPodChange(e.serviceLister, e.serviceSelectorCache, old, cur) for key := range services { e.queue.AddAfter(key, e.endpointUpdatesBatchPeriod) } diff --git a/pkg/controller/endpoint/endpoints_controller_test.go b/pkg/controller/endpoint/endpoints_controller_test.go index ce8ea794c44..3db9982cdde 100644 --- a/pkg/controller/endpoint/endpoints_controller_test.go +++ b/pkg/controller/endpoint/endpoints_controller_test.go @@ -43,7 +43,6 @@ import ( endptspkg "k8s.io/kubernetes/pkg/api/v1/endpoints" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/controller" - endpointutil "k8s.io/kubernetes/pkg/controller/util/endpoint" "k8s.io/kubernetes/pkg/features" utilpointer "k8s.io/utils/pointer" ) @@ -67,7 +66,6 @@ func testPod(namespace string, id int, nPorts int, isReady bool, makeDualstack b Containers: []v1.Container{{Ports: []v1.ContainerPort{}}}, }, Status: v1.PodStatus{ - PodIP: fmt.Sprintf("1.2.3.%d", 4+id), Conditions: []v1.PodCondition{ { Type: v1.PodReady, @@ -83,16 +81,11 @@ func testPod(namespace string, id int, nPorts int, isReady bool, makeDualstack b p.Spec.Containers[0].Ports = append(p.Spec.Containers[0].Ports, v1.ContainerPort{Name: fmt.Sprintf("port%d", j), ContainerPort: int32(8080 + j)}) } + p.Status.PodIPs = append(p.Status.PodIPs, v1.PodIP{IP: fmt.Sprintf("1.2.3.%d", 4+id)}) if makeDualstack { - p.Status.PodIPs = []v1.PodIP{ - { - IP: p.Status.PodIP, - }, - { - IP: fmt.Sprintf("2000::%d", id), - }, - } + p.Status.PodIPs = append(p.Status.PodIPs, v1.PodIP{IP: fmt.Sprintf("2000::%d", id)}) } + p.Status.PodIP = p.Status.PodIPs[0].IP return p } @@ -1238,169 +1231,6 @@ func TestPodToEndpointAddressForService(t *testing.T) { } -func TestPodToEndpointAddress(t *testing.T) { - podStore := cache.NewStore(cache.DeletionHandlingMetaNamespaceKeyFunc) - ns := "test" - addPods(podStore, ns, 1, 1, 0, false) - pods := podStore.List() - if len(pods) != 1 { - t.Errorf("podStore size: expected: %d, got: %d", 1, len(pods)) - return - } - pod := pods[0].(*v1.Pod) - epa := podToEndpointAddress(pod) - if epa.IP != pod.Status.PodIP { - t.Errorf("IP: expected: %s, got: %s", pod.Status.PodIP, epa.IP) - } - if *(epa.NodeName) != pod.Spec.NodeName { - t.Errorf("NodeName: expected: %s, got: %s", pod.Spec.NodeName, *(epa.NodeName)) - } - if epa.TargetRef.Kind != "Pod" { - t.Errorf("TargetRef.Kind: expected: %s, got: %s", "Pod", epa.TargetRef.Kind) - } - if epa.TargetRef.Namespace != pod.ObjectMeta.Namespace { - t.Errorf("TargetRef.Kind: expected: %s, got: %s", pod.ObjectMeta.Namespace, epa.TargetRef.Namespace) - } - if epa.TargetRef.Name != pod.ObjectMeta.Name { - t.Errorf("TargetRef.Kind: expected: %s, got: %s", pod.ObjectMeta.Name, epa.TargetRef.Name) - } - if epa.TargetRef.UID != pod.ObjectMeta.UID { - t.Errorf("TargetRef.Kind: expected: %s, got: %s", pod.ObjectMeta.UID, epa.TargetRef.UID) - } - if epa.TargetRef.ResourceVersion != pod.ObjectMeta.ResourceVersion { - t.Errorf("TargetRef.Kind: expected: %s, got: %s", pod.ObjectMeta.ResourceVersion, epa.TargetRef.ResourceVersion) - } -} - -func TestPodChanged(t *testing.T) { - podStore := cache.NewStore(cache.DeletionHandlingMetaNamespaceKeyFunc) - ns := "test" - addPods(podStore, ns, 1, 1, 0, false) - pods := podStore.List() - if len(pods) != 1 { - t.Errorf("podStore size: expected: %d, got: %d", 1, len(pods)) - return - } - oldPod := pods[0].(*v1.Pod) - newPod := oldPod.DeepCopy() - - if podChangedHelper(oldPod, newPod, endpointChanged) { - t.Errorf("Expected pod to be unchanged for copied pod") - } - - newPod.Spec.NodeName = "changed" - if !podChangedHelper(oldPod, newPod, endpointChanged) { - t.Errorf("Expected pod to be changed for pod with NodeName changed") - } - newPod.Spec.NodeName = oldPod.Spec.NodeName - - newPod.ObjectMeta.ResourceVersion = "changed" - if podChangedHelper(oldPod, newPod, endpointChanged) { - t.Errorf("Expected pod to be unchanged for pod with only ResourceVersion changed") - } - newPod.ObjectMeta.ResourceVersion = oldPod.ObjectMeta.ResourceVersion - - newPod.Status.PodIP = "1.2.3.1" - if !podChangedHelper(oldPod, newPod, endpointChanged) { - t.Errorf("Expected pod to be changed with pod IP address change") - } - newPod.Status.PodIP = oldPod.Status.PodIP - - /* dual stack tests */ - // primary changes, because changing IPs is done by changing sandbox - // case 1: add new secondary IP - newPod.Status.PodIP = "1.1.3.1" - newPod.Status.PodIPs = []v1.PodIP{ - { - IP: "1.1.3.1", - }, - { - IP: "2000::1", - }, - } - if !podChangedHelper(oldPod, newPod, endpointChanged) { - t.Errorf("Expected pod to be changed with adding secondary IP") - } - // reset - newPod.Status.PodIPs = nil - newPod.Status.PodIP = oldPod.Status.PodIP - - // case 2: removing a secondary IP - saved := oldPod.Status.PodIP - oldPod.Status.PodIP = "1.1.3.1" - oldPod.Status.PodIPs = []v1.PodIP{ - { - IP: "1.1.3.1", - }, - { - IP: "2000::1", - }, - } - - newPod.Status.PodIP = "1.2.3.4" - newPod.Status.PodIPs = []v1.PodIP{ - { - IP: "1.2.3.4", - }, - } - - // reset - oldPod.Status.PodIPs = nil - newPod.Status.PodIPs = nil - oldPod.Status.PodIP = saved - newPod.Status.PodIP = saved - // case 3: change secondary - // case 2: removing a secondary IP - saved = oldPod.Status.PodIP - oldPod.Status.PodIP = "1.1.3.1" - oldPod.Status.PodIPs = []v1.PodIP{ - { - IP: "1.1.3.1", - }, - { - IP: "2000::1", - }, - } - - newPod.Status.PodIP = "1.2.3.4" - newPod.Status.PodIPs = []v1.PodIP{ - { - IP: "1.2.3.4", - }, - { - IP: "2000::2", - }, - } - - // reset - oldPod.Status.PodIPs = nil - newPod.Status.PodIPs = nil - oldPod.Status.PodIP = saved - newPod.Status.PodIP = saved - - /* end dual stack testing */ - - newPod.ObjectMeta.Name = "wrong-name" - if !podChangedHelper(oldPod, newPod, endpointChanged) { - t.Errorf("Expected pod to be changed with pod name change") - } - newPod.ObjectMeta.Name = oldPod.ObjectMeta.Name - - saveConditions := oldPod.Status.Conditions - oldPod.Status.Conditions = nil - if !podChangedHelper(oldPod, newPod, endpointChanged) { - t.Errorf("Expected pod to be changed with pod readiness change") - } - oldPod.Status.Conditions = saveConditions - - now := metav1.NewTime(time.Now().UTC()) - newPod.ObjectMeta.DeletionTimestamp = &now - if !podChangedHelper(oldPod, newPod, endpointChanged) { - t.Errorf("Expected pod to be changed with DeletionTimestamp change") - } - newPod.ObjectMeta.DeletionTimestamp = oldPod.ObjectMeta.DeletionTimestamp.DeepCopy() -} - func TestLastTriggerChangeTimeAnnotation(t *testing.T) { ns := "other" testServer, endpointsHandler := makeTestServer(t, ns) @@ -1680,6 +1510,7 @@ func TestPodUpdatesBatching(t *testing.T) { oldPod := old.(*v1.Pod) newPod := oldPod.DeepCopy() newPod.Status.PodIP = update.podIP + newPod.Status.PodIPs[0].IP = update.podIP newPod.ResourceVersion = strconv.Itoa(resourceVersion) resourceVersion++ @@ -1996,8 +1827,3 @@ func stringVal(str *string) string { } return *str } - -func podChangedHelper(oldPod, newPod *v1.Pod, endpointChanged endpointutil.EndpointsMatch) bool { - podChanged, _ := endpointutil.PodChanged(oldPod, newPod, endpointChanged) - return podChanged -} diff --git a/pkg/controller/endpointslice/endpointslice_controller.go b/pkg/controller/endpointslice/endpointslice_controller.go index 63098565a49..e4b120a706a 100644 --- a/pkg/controller/endpointslice/endpointslice_controller.go +++ b/pkg/controller/endpointslice/endpointslice_controller.go @@ -457,7 +457,7 @@ func (c *Controller) addPod(obj interface{}) { } func (c *Controller) updatePod(old, cur interface{}) { - services := endpointutil.GetServicesToUpdateOnPodChange(c.serviceLister, c.serviceSelectorCache, old, cur, podEndpointChanged) + services := endpointutil.GetServicesToUpdateOnPodChange(c.serviceLister, c.serviceSelectorCache, old, cur) for key := range services { c.queue.AddAfter(key, c.endpointUpdatesBatchPeriod) } diff --git a/pkg/controller/endpointslice/utils.go b/pkg/controller/endpointslice/utils.go index 6d1a1e570ed..a3589e53907 100644 --- a/pkg/controller/endpointslice/utils.go +++ b/pkg/controller/endpointslice/utils.go @@ -18,7 +18,6 @@ package endpointslice import ( "fmt" - "reflect" "time" corev1 "k8s.io/api/core/v1" @@ -36,19 +35,7 @@ import ( utilnet "k8s.io/utils/net" ) -// podEndpointChanged returns true if the results of podToEndpoint are different -// for the pods passed to this function. -func podEndpointChanged(pod1, pod2 *corev1.Pod) bool { - endpoint1 := podToEndpoint(pod1, &corev1.Node{}, &corev1.Service{Spec: corev1.ServiceSpec{}}) - endpoint2 := podToEndpoint(pod2, &corev1.Node{}, &corev1.Service{Spec: corev1.ServiceSpec{}}) - - endpoint1.TargetRef.ResourceVersion = "" - endpoint2.TargetRef.ResourceVersion = "" - - return !reflect.DeepEqual(endpoint1, endpoint2) -} - -// podToEndpoint returns an Endpoint object generated from a Pod and Node. +// podToEndpoint returns an Endpoint object generated from pod, node, and service. func podToEndpoint(pod *corev1.Pod, node *corev1.Node, service *corev1.Service) discovery.Endpoint { // Build out topology information. This is currently limited to hostname, // zone, and region, but this will be expanded in the future. diff --git a/pkg/controller/endpointslice/utils_test.go b/pkg/controller/endpointslice/utils_test.go index f485ff02e20..d65c4d78097 100644 --- a/pkg/controller/endpointslice/utils_test.go +++ b/pkg/controller/endpointslice/utils_test.go @@ -20,7 +20,6 @@ import ( "fmt" "reflect" "testing" - "time" "github.com/stretchr/testify/assert" v1 "k8s.io/api/core/v1" @@ -32,8 +31,6 @@ import ( "k8s.io/apimachinery/pkg/util/rand" "k8s.io/client-go/kubernetes/fake" k8stesting "k8s.io/client-go/testing" - "k8s.io/client-go/tools/cache" - endpointutil "k8s.io/kubernetes/pkg/controller/util/endpoint" utilpointer "k8s.io/utils/pointer" ) @@ -255,61 +252,6 @@ func TestPodToEndpoint(t *testing.T) { } } -func TestPodChangedWithPodEndpointChanged(t *testing.T) { - podStore := cache.NewStore(cache.DeletionHandlingMetaNamespaceKeyFunc) - ns := "test" - podStore.Add(newPod(1, ns, true, 1)) - pods := podStore.List() - if len(pods) != 1 { - t.Errorf("podStore size: expected: %d, got: %d", 1, len(pods)) - return - } - oldPod := pods[0].(*v1.Pod) - newPod := oldPod.DeepCopy() - - if podChangedHelper(oldPod, newPod, podEndpointChanged) { - t.Errorf("Expected pod to be unchanged for copied pod") - } - - newPod.Spec.NodeName = "changed" - if !podChangedHelper(oldPod, newPod, podEndpointChanged) { - t.Errorf("Expected pod to be changed for pod with NodeName changed") - } - newPod.Spec.NodeName = oldPod.Spec.NodeName - - newPod.ObjectMeta.ResourceVersion = "changed" - if podChangedHelper(oldPod, newPod, podEndpointChanged) { - t.Errorf("Expected pod to be unchanged for pod with only ResourceVersion changed") - } - newPod.ObjectMeta.ResourceVersion = oldPod.ObjectMeta.ResourceVersion - - newPod.Status.PodIPs = []v1.PodIP{{IP: "1.2.3.1"}} - if !podChangedHelper(oldPod, newPod, podEndpointChanged) { - t.Errorf("Expected pod to be changed with pod IP address change") - } - newPod.Status.PodIPs = oldPod.Status.PodIPs - - newPod.ObjectMeta.Name = "wrong-name" - if !podChangedHelper(oldPod, newPod, podEndpointChanged) { - t.Errorf("Expected pod to be changed with pod name change") - } - newPod.ObjectMeta.Name = oldPod.ObjectMeta.Name - - saveConditions := oldPod.Status.Conditions - oldPod.Status.Conditions = nil - if !podChangedHelper(oldPod, newPod, podEndpointChanged) { - t.Errorf("Expected pod to be changed with pod readiness change") - } - oldPod.Status.Conditions = saveConditions - - now := metav1.NewTime(time.Now().UTC()) - newPod.ObjectMeta.DeletionTimestamp = &now - if !podChangedHelper(oldPod, newPod, podEndpointChanged) { - t.Errorf("Expected pod to be changed with DeletionTimestamp change") - } - newPod.ObjectMeta.DeletionTimestamp = oldPod.ObjectMeta.DeletionTimestamp.DeepCopy() -} - func TestServiceControllerKey(t *testing.T) { testCases := map[string]struct { endpointSlice *discovery.EndpointSlice @@ -545,8 +487,3 @@ func newEmptyEndpointSlice(n int, namespace string, endpointMeta endpointMeta, s Endpoints: []discovery.Endpoint{}, } } - -func podChangedHelper(oldPod, newPod *v1.Pod, endpointChanged endpointutil.EndpointsMatch) bool { - podChanged, _ := endpointutil.PodChanged(oldPod, newPod, podEndpointChanged) - return podChanged -} diff --git a/pkg/controller/util/endpoint/controller_utils.go b/pkg/controller/util/endpoint/controller_utils.go index e38e65668d7..d1744441bec 100644 --- a/pkg/controller/util/endpoint/controller_utils.go +++ b/pkg/controller/util/endpoint/controller_utils.go @@ -106,9 +106,6 @@ func (sc *ServiceSelectorCache) GetPodServiceMemberships(serviceLister v1listers return set, nil } -// EndpointsMatch is a type of function that returns true if pod endpoints match. -type EndpointsMatch func(*v1.Pod, *v1.Pod) bool - // PortMapKey is used to uniquely identify groups of endpoint ports. type PortMapKey string @@ -153,9 +150,10 @@ func ShouldSetHostname(pod *v1.Pod, svc *v1.Service) bool { return len(pod.Spec.Hostname) > 0 && pod.Spec.Subdomain == svc.Name && svc.Namespace == pod.Namespace } -// PodChanged returns two boolean values, the first returns true if the pod. -// has changed, the second value returns true if the pod labels have changed. -func PodChanged(oldPod, newPod *v1.Pod, endpointChanged EndpointsMatch) (bool, bool) { +// podEndpointsChanged returns two boolean values. The first is true if the pod has +// changed in a way that may change existing endpoints. The second value is true if the +// pod has changed in a way that may affect which Services it matches. +func podEndpointsChanged(oldPod, newPod *v1.Pod) (bool, bool) { // Check if the pod labels have changed, indicating a possible // change in the service membership labelsChanged := false @@ -175,16 +173,27 @@ func PodChanged(oldPod, newPod *v1.Pod, endpointChanged EndpointsMatch) (bool, b if podutil.IsPodReady(oldPod) != podutil.IsPodReady(newPod) { return true, labelsChanged } - // Convert the pod to an Endpoint, clear inert fields, - // and see if they are the same. - // TODO: Add a watcher for node changes separate from this - // We don't want to trigger multiple syncs at a pod level when a node changes - return endpointChanged(newPod, oldPod), labelsChanged + + // Check if the pod IPs have changed + if len(oldPod.Status.PodIPs) != len(newPod.Status.PodIPs) { + return true, labelsChanged + } + for i := range oldPod.Status.PodIPs { + if oldPod.Status.PodIPs[i].IP != newPod.Status.PodIPs[i].IP { + return true, labelsChanged + } + } + + // Endpoints may also reference a pod's Name, Namespace, UID, and NodeName, but + // the first three are immutable, and NodeName is immutable once initially set, + // which happens before the pod gets an IP. + + return false, labelsChanged } // GetServicesToUpdateOnPodChange returns a set of Service keys for Services // that have potentially been affected by a change to this pod. -func GetServicesToUpdateOnPodChange(serviceLister v1listers.ServiceLister, selectorCache *ServiceSelectorCache, old, cur interface{}, endpointChanged EndpointsMatch) sets.String { +func GetServicesToUpdateOnPodChange(serviceLister v1listers.ServiceLister, selectorCache *ServiceSelectorCache, old, cur interface{}) sets.String { newPod := cur.(*v1.Pod) oldPod := old.(*v1.Pod) if newPod.ResourceVersion == oldPod.ResourceVersion { @@ -193,7 +202,7 @@ func GetServicesToUpdateOnPodChange(serviceLister v1listers.ServiceLister, selec return sets.String{} } - podChanged, labelsChanged := PodChanged(oldPod, newPod, endpointChanged) + podChanged, labelsChanged := podEndpointsChanged(oldPod, newPod) // If both the pod and labels are unchanged, no update is needed if !podChanged && !labelsChanged { diff --git a/pkg/controller/util/endpoint/controller_utils_test.go b/pkg/controller/util/endpoint/controller_utils_test.go index 474842b43da..df118e0a7af 100644 --- a/pkg/controller/util/endpoint/controller_utils_test.go +++ b/pkg/controller/util/endpoint/controller_utils_test.go @@ -499,3 +499,168 @@ func BenchmarkGetPodServiceMemberships(b *testing.B) { } } } + +func Test_podChanged(t *testing.T) { + testCases := []struct { + testName string + modifier func(*v1.Pod, *v1.Pod) + podChanged bool + labelsChanged bool + }{ + { + testName: "no changes", + modifier: func(old, new *v1.Pod) {}, + podChanged: false, + labelsChanged: false, + }, { + testName: "change NodeName", + modifier: func(old, new *v1.Pod) { + new.Spec.NodeName = "changed" + }, + // NodeName can only change before the pod has an IP, and we don't care about the + // pod yet at that point so we ignore this change + podChanged: false, + labelsChanged: false, + }, { + testName: "change ResourceVersion", + modifier: func(old, new *v1.Pod) { + new.ObjectMeta.ResourceVersion = "changed" + }, + // ResourceVersion is intentionally ignored if nothing else changed + podChanged: false, + labelsChanged: false, + }, { + testName: "add primary IPv4", + modifier: func(old, new *v1.Pod) { + new.Status.PodIP = "1.2.3.4" + new.Status.PodIPs = []v1.PodIP{{IP: "1.2.3.4"}} + }, + podChanged: true, + labelsChanged: false, + }, { + testName: "modify primary IPv4", + modifier: func(old, new *v1.Pod) { + old.Status.PodIP = "1.2.3.4" + old.Status.PodIPs = []v1.PodIP{{IP: "1.2.3.4"}} + new.Status.PodIP = "2.3.4.5" + new.Status.PodIPs = []v1.PodIP{{IP: "2.3.4.5"}} + }, + podChanged: true, + labelsChanged: false, + }, { + testName: "add primary IPv6", + modifier: func(old, new *v1.Pod) { + new.Status.PodIP = "fd00:10:96::1" + new.Status.PodIPs = []v1.PodIP{{IP: "fd00:10:96::1"}} + }, + podChanged: true, + labelsChanged: false, + }, { + testName: "modify primary IPv6", + modifier: func(old, new *v1.Pod) { + old.Status.PodIP = "fd00:10:96::1" + old.Status.PodIPs = []v1.PodIP{{IP: "fd00:10:96::1"}} + new.Status.PodIP = "fd00:10:96::2" + new.Status.PodIPs = []v1.PodIP{{IP: "fd00:10:96::2"}} + }, + podChanged: true, + labelsChanged: false, + }, { + testName: "add secondary IP", + modifier: func(old, new *v1.Pod) { + old.Status.PodIP = "1.2.3.4" + old.Status.PodIPs = []v1.PodIP{{IP: "1.2.3.4"}} + new.Status.PodIP = "1.2.3.4" + new.Status.PodIPs = []v1.PodIP{{IP: "1.2.3.4"}, {IP: "fd00:10:96::1"}} + }, + podChanged: true, + labelsChanged: false, + }, { + testName: "modify secondary IP", + modifier: func(old, new *v1.Pod) { + old.Status.PodIP = "1.2.3.4" + old.Status.PodIPs = []v1.PodIP{{IP: "1.2.3.4"}, {IP: "fd00:10:96::1"}} + new.Status.PodIP = "1.2.3.4" + new.Status.PodIPs = []v1.PodIP{{IP: "1.2.3.4"}, {IP: "fd00:10:96::2"}} + }, + podChanged: true, + labelsChanged: false, + }, { + testName: "remove secondary IP", + modifier: func(old, new *v1.Pod) { + old.Status.PodIP = "1.2.3.4" + old.Status.PodIPs = []v1.PodIP{{IP: "1.2.3.4"}, {IP: "fd00:10:96::1"}} + new.Status.PodIP = "1.2.3.4" + new.Status.PodIPs = []v1.PodIP{{IP: "1.2.3.4"}} + }, + podChanged: true, + labelsChanged: false, + }, { + testName: "change readiness", + modifier: func(old, new *v1.Pod) { + new.Status.Conditions[0].Status = v1.ConditionTrue + }, + podChanged: true, + labelsChanged: false, + }, { + testName: "mark for deletion", + modifier: func(old, new *v1.Pod) { + now := metav1.NewTime(time.Now().UTC()) + new.ObjectMeta.DeletionTimestamp = &now + }, + podChanged: true, + labelsChanged: false, + }, { + testName: "add label", + modifier: func(old, new *v1.Pod) { + new.Labels["label"] = "new" + }, + podChanged: false, + labelsChanged: true, + }, { + testName: "modify label", + modifier: func(old, new *v1.Pod) { + old.Labels["label"] = "old" + new.Labels["label"] = "new" + }, + podChanged: false, + labelsChanged: true, + }, { + testName: "remove label", + modifier: func(old, new *v1.Pod) { + old.Labels["label"] = "old" + }, + podChanged: false, + labelsChanged: true, + }, + } + + orig := &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "pod", + Labels: map[string]string{"foo": "bar"}, + }, + Status: v1.PodStatus{ + Conditions: []v1.PodCondition{ + {Type: v1.PodReady, Status: v1.ConditionFalse}, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.testName, func(t *testing.T) { + old := orig.DeepCopy() + new := old.DeepCopy() + tc.modifier(old, new) + + podChanged, labelsChanged := podEndpointsChanged(old, new) + if podChanged != tc.podChanged { + t.Errorf("Expected podChanged to be %t, got %t", tc.podChanged, podChanged) + } + if labelsChanged != tc.labelsChanged { + t.Errorf("Expected labelsChanged to be %t, got %t", tc.labelsChanged, labelsChanged) + } + }) + } +}