From f274a0cc0218f04238f1929bde7243465b5b9ca5 Mon Sep 17 00:00:00 2001 From: nicklesimba Date: Thu, 13 Jul 2023 14:36:29 -0500 Subject: [PATCH 1/3] Denormalize IP name before checking if pod is alive This commit solves the issue of normalized IP names being used for checking if a pod is alive, which resulted in overlappingrangeipreservations being falsely deleted by the ip-reconciler. Signed-off-by: nicklesimba --- pkg/reconciler/iploop.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/reconciler/iploop.go b/pkg/reconciler/iploop.go index 7ef8e9202..37ac9cb39 100644 --- a/pkg/reconciler/iploop.go +++ b/pkg/reconciler/iploop.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net" + "strings" "time" v1 "k8s.io/api/core/v1" @@ -164,9 +165,12 @@ func (rl *ReconcileLooper) findClusterWideIPReservations(ctx context.Context) er for _, clusterWideIPReservation := range clusterWideIPReservations { ip := clusterWideIPReservation.GetName() + // De-normalize the IP + denormalizedip := strings.ReplaceAll(ip, "-", ":") + podRef := clusterWideIPReservation.Spec.PodRef - if !rl.isPodAlive(podRef, ip) { + if !rl.isPodAlive(podRef, denormalizedip) { logging.Debugf("pod ref %s is not listed in the live pods list", podRef) rl.orphanedClusterWideIPs = append(rl.orphanedClusterWideIPs, clusterWideIPReservation) } From fafc631cc84f3e6b4c613a7cd320f3c5e8b332b6 Mon Sep 17 00:00:00 2001 From: nicklesimba Date: Fri, 21 Jul 2023 15:59:14 -0500 Subject: [PATCH 2/3] added comments and a unit test Signed-off-by: nicklesimba --- pkg/reconciler/ip_test.go | 66 +++++++++++++++++++++++++++++++++++++++ pkg/reconciler/iploop.go | 2 ++ 2 files changed, 68 insertions(+) diff --git a/pkg/reconciler/ip_test.go b/pkg/reconciler/ip_test.go index f76fb6841..19bec9fd2 100644 --- a/pkg/reconciler/ip_test.go +++ b/pkg/reconciler/ip_test.go @@ -241,6 +241,72 @@ var _ = Describe("Whereabouts IP reconciler", func() { }) }) + Context("reconciling cluster wide IPs - overlapping IPs (ipv6)", func() { + const ( + numberOfPods = 1 + ipv6FirstIPInRange = "2001:1b74:0480:60b1:0000:0000:0000:0002" + networkName = "dummyNetwork" + networkRange = "2001:1b74:480:60b1::10/64" + poolName = "dummyPool" + ) + + var ( + pods []*v1.Pod + pools []v1alpha1.IPPool + clusterWideIPs []v1alpha1.OverlappingRangeIPReservation + wbClient wbclient.Interface + ) + + wrapToRuntimeObject := func(pods ...*v1.Pod) []runtime.Object { + var runtimeObjects []runtime.Object + for _, pod := range pods { + runtimeObjects = append(runtimeObjects, pod) + } + return runtimeObjects + } + + BeforeEach(func() { + ips := []string{ipv6FirstIPInRange} + networks := []string{networkName} + for i := 0; i < numberOfPods; i++ { + pod := generatePod(namespace, fmt.Sprintf("pod%d", i+1), ipInNetwork{ + ip: ips[i], + networkName: networks[i%2], // pod1 and pod3 connected to network1; pod2 connected to network2 + }) + pods = append(pods, pod) + } + k8sClientSet = fakek8sclient.NewSimpleClientset(wrapToRuntimeObject(pods...)...) + }) + + BeforeEach(func() { + firstPool := generateIPPoolSpec(networkRange, namespace, poolName, pods[0].GetName()) + wbClient = fakewbclient.NewSimpleClientset(firstPool) + pools = append(pools, *firstPool) + }) + + BeforeEach(func() { + podIPs := []string{ipv6FirstIPInRange} + for i := 0; i < numberOfPods; i++ { + var clusterWideIP v1alpha1.OverlappingRangeIPReservation + ownerPodRef := fmt.Sprintf("%s/%s", namespace, pods[i].GetName()) + _, err := wbClient.WhereaboutsV1alpha1().OverlappingRangeIPReservations(namespace).Create(context.TODO(), generateClusterWideIPReservation(namespace, podIPs[i], ownerPodRef), metav1.CreateOptions{}) + Expect(err).NotTo(HaveOccurred()) + clusterWideIPs = append(clusterWideIPs, clusterWideIP) + } + }) + + It("will not delete an IP address that isn't orphaned after running reconciler", func() { + newReconciler, err := NewReconcileLooperWithClient(context.TODO(), kubernetes.NewKubernetesClient(wbClient, k8sClientSet, timeout), timeout) + Expect(err).NotTo(HaveOccurred()) + Expect(newReconciler.ReconcileOverlappingIPAddresses(context.TODO())).To(Succeed()) + + expectedClusterWideIPs := 1 + clusterWideIPAllocations, err := wbClient.WhereaboutsV1alpha1().OverlappingRangeIPReservations(namespace).List(context.TODO(), metav1.ListOptions{}) + Expect(err).NotTo(HaveOccurred()) + Expect(clusterWideIPAllocations.Items).To(HaveLen(expectedClusterWideIPs)) + }) + }) + Context("a pod in pending state, without an IP in its network-status", func() { const poolName = "pool1" diff --git a/pkg/reconciler/iploop.go b/pkg/reconciler/iploop.go index 37ac9cb39..c2c79cfc5 100644 --- a/pkg/reconciler/iploop.go +++ b/pkg/reconciler/iploop.go @@ -166,6 +166,8 @@ func (rl *ReconcileLooper) findClusterWideIPReservations(ctx context.Context) er for _, clusterWideIPReservation := range clusterWideIPReservations { ip := clusterWideIPReservation.GetName() // De-normalize the IP + // In the UpdateOverlappingRangeAllocation function, the IP address is created with a "normalized" name to comply with the k8s api. + // We must denormalize here in order to properly look up the IP address in the regular format, which pods use. denormalizedip := strings.ReplaceAll(ip, "-", ":") podRef := clusterWideIPReservation.Spec.PodRef From b48320ae4f0476d00e96a170fe3a929f0b89b286 Mon Sep 17 00:00:00 2001 From: nicklesimba Date: Mon, 24 Jul 2023 17:21:28 -0500 Subject: [PATCH 3/3] delete pod after unit test run Signed-off-by: nicklesimba --- pkg/reconciler/ip_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/reconciler/ip_test.go b/pkg/reconciler/ip_test.go index 19bec9fd2..27f29a567 100644 --- a/pkg/reconciler/ip_test.go +++ b/pkg/reconciler/ip_test.go @@ -304,6 +304,7 @@ var _ = Describe("Whereabouts IP reconciler", func() { clusterWideIPAllocations, err := wbClient.WhereaboutsV1alpha1().OverlappingRangeIPReservations(namespace).List(context.TODO(), metav1.ListOptions{}) Expect(err).NotTo(HaveOccurred()) Expect(clusterWideIPAllocations.Items).To(HaveLen(expectedClusterWideIPs)) + Expect(k8sClientSet.CoreV1().Pods(namespace).Delete(context.TODO(), pods[0].Name, metav1.DeleteOptions{})).NotTo(HaveOccurred()) }) })