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.
2020-05-24 11:55:09 -04:00
parent f9ad7db9a6
commit 9fb6e2ef55
8 changed files with 217 additions and 310 deletions
--- 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)
+			}
+		})
+	}
+}