Use IP to identify orphaned allocation to be deleted

The IP uniquely identifies the correct orphaned allocation
that needs to be deleted during IP reconcilation.

Fixes #176

pkg/allocate/allocate.go

@@ -52,24 +52,6 @@ func DeallocateIP(reservelist []types.IPReservation, containerID string) ([]type
 	return removeIdxFromSlice(reservelist, index), ip
 }
 
-// IterateForDeallocation iterates overs currently reserved IPs and the deallocates given the container id.
-func IterateForDeallocation(
-	reservelist []types.IPReservation,
-	containerID string,
-	matchingFunction func(reservation []types.IPReservation, id string) int) ([]types.IPReservation, net.IP, error) {
-
-	foundidx := matchingFunction(reservelist, containerID)
-	// Check if it's a valid index
-	if foundidx < 0 {
-		return reservelist, nil, fmt.Errorf("did not find reserved IP for container %v", containerID)
-	}
-
-	returnip := reservelist[foundidx].IP
-
-	updatedreservelist := removeIdxFromSlice(reservelist, foundidx)
-	return updatedreservelist, returnip, nil
-}
-
 func getMatchingIPReservationIndex(reservelist []types.IPReservation, id string) int {
 	for idx, v := range reservelist {
 		if v.ContainerID == id {

pkg/reconciler/ip_test.go

@@ -168,6 +168,48 @@ var _ = Describe("Whereabouts IP reconciler", func() {
 		})
 	})
 
+	Context("reconciling an IP pool with entries from the same pod reference", func() {
+		var wbClient wbclient.Interface
+		var pod *v1.Pod
+
+		It("verifies that the correct entry is cleaned up", func() {
+			pod = generatePod(namespace, podName, ipInNetwork{ip: firstIPInRange, networkName: networkName})
+			k8sClientSet = fakek8sclient.NewSimpleClientset(pod)
+
+			By("creating an IP pool with 2 entries from the same pod. Second entry was initially assigned to the pod")
+			pool := generateIPPoolSpec(ipRange, namespace, podName)
+			pool.Spec.Allocations = map[string]v1alpha1.IPAllocation{
+				"1": {
+					PodRef: fmt.Sprintf("%s/%s", namespace, podName),
+				},
+				"2": {
+					PodRef: fmt.Sprintf("%s/%s", namespace, podName),
+				},
+			}
+			wbClient = fakewbclient.NewSimpleClientset(pool)
+
+			By("initializing the reconciler")
+			var err error
+			reconcileLooper, err = NewReconcileLooperWithClient(context.TODO(), kubernetes.NewKubernetesClient(wbClient, k8sClientSet, timeout), timeout)
+			Expect(err).NotTo(HaveOccurred())
+
+			By("reconciling and checking that the correct entry is deleted")
+			deletedIPAddrs, err := reconcileLooper.ReconcileIPPools(context.TODO())
+			Expect(err).NotTo(HaveOccurred())
+			Expect(deletedIPAddrs).To(Equal([]net.IP{net.ParseIP("10.10.10.2")}))
+
+			By("verifying the IP pool")
+			poolAfterCleanup, err := wbClient.WhereaboutsV1alpha1().IPPools(namespace).Get(context.TODO(), pool.GetName(), metav1.GetOptions{})
+			Expect(err).NotTo(HaveOccurred())
+			remainingAllocation := map[string]v1alpha1.IPAllocation{
+				"1": {
+					PodRef: fmt.Sprintf("%s/%s", namespace, podName),
+				},
+			}
+			Expect(poolAfterCleanup.Spec.Allocations).To(Equal(remainingAllocation))
+		})
+	})
+
 	Context("reconciling cluster wide IPs - overlapping IPs", func() {
 		const (
 			numberOfPods       = 3
@@ -421,30 +463,6 @@ var _ = Describe("IPReconciler", func() {
 				Expect(reconciledIPs).To(ConsistOf([]net.IP{net.ParseIP("192.168.14.2")}))
 			})
 		})
-
-		Context("but the IP reservation owner does not match", func() {
-			var reservationPodRef string
-			BeforeEach(func() {
-				reservationPodRef = "default/pod2"
-				podRef := "default/pod1"
-				reservations := generateIPReservation(firstIPInRange, podRef)
-				erroredReservations := generateIPReservation(firstIPInRange, reservationPodRef)
-
-				pool := generateIPPool(ipCIDR, podRef)
-				orphanedIPAddr := OrphanedIPReservations{
-					Pool:        dummyPool{orphans: reservations, pool: pool},
-					Allocations: erroredReservations,
-				}
-
-				ipReconciler = newIPReconciler(orphanedIPAddr)
-			})
-
-			It("errors when attempting to clean up the IP address", func() {
-				reconciledIPs, err := ipReconciler.ReconcileIPPools(context.TODO())
-				Expect(err).To(MatchError(fmt.Sprintf("did not find reserved IP for container %s", reservationPodRef)))
-				Expect(reconciledIPs).To(BeEmpty())
-			})
-		})
 	})
 })
 

pkg/reconciler/iploop.go

@@ -9,7 +9,6 @@ import (
 
 	v1 "k8s.io/api/core/v1"
 
-	"github.com/k8snetworkplumbingwg/whereabouts/pkg/allocate"
 	whereaboutsv1alpha1 "github.com/k8snetworkplumbingwg/whereabouts/pkg/api/whereabouts.cni.cncf.io/v1alpha1"
 	"github.com/k8snetworkplumbingwg/whereabouts/pkg/logging"
 	"github.com/k8snetworkplumbingwg/whereabouts/pkg/storage"
@@ -175,34 +174,43 @@ func composePodRef(pod v1.Pod) string {
 }
 
 func (rl ReconcileLooper) ReconcileIPPools(ctx context.Context) ([]net.IP, error) {
-	matchByPodRef := func(reservations []types.IPReservation, podRef string) int {
-		foundidx := -1
-		for idx, v := range reservations {
-			if v.PodRef == podRef {
+	findAllocationIndex := func(reservation types.IPReservation, reservations []types.IPReservation) int {
+		for idx, r := range reservations {
+			if r.PodRef == reservation.PodRef && r.IP.Equal(reservation.IP) {
 				return idx
 			}
 		}
-		return foundidx
+		return -1
 	}
 
-	var err error
 	var totalCleanedUpIps []net.IP
 	for _, orphanedIP := range rl.orphanedIPs {
 		currentIPReservations := orphanedIP.Pool.Allocations()
-		podRefsToDeallocate := findOutPodRefsToDeallocateIPsFrom(orphanedIP)
-		var deallocatedIP net.IP
-		for _, podRef := range podRefsToDeallocate {
-			currentIPReservations, deallocatedIP, err = allocate.IterateForDeallocation(currentIPReservations, podRef, matchByPodRef)
-			if err != nil {
-				return nil, err
+
+		// Process orphaned allocation peer pool
+		var cleanedUpIpsPerPool []net.IP
+		for _, allocation := range orphanedIP.Allocations {
+			idx := findAllocationIndex(allocation, currentIPReservations)
+			if idx < 0 {
+				// Should never happen
+				logging.Debugf("Failed to find allocation for pod ref: %s and IP: %s", allocation.PodRef, allocation.IP.String())
+				continue
 			}
+
+			// Delete entry
+			currentIPReservations[idx] = currentIPReservations[len(currentIPReservations)-1]
+			currentIPReservations = currentIPReservations[:len(currentIPReservations)-1]
+
+			cleanedUpIpsPerPool = append(cleanedUpIpsPerPool, allocation.IP)
 		}
 
-		logging.Debugf("Going to update the reserve list to: %+v", currentIPReservations)
-		if err := orphanedIP.Pool.Update(ctx, currentIPReservations); err != nil {
-			return nil, logging.Errorf("failed to update the reservation list: %v", err)
+		if len(cleanedUpIpsPerPool) != 0 {
+			logging.Debugf("Going to update the reserve list to: %+v", currentIPReservations)
+			if err := orphanedIP.Pool.Update(ctx, currentIPReservations); err != nil {
+				return nil, logging.Errorf("failed to update the reservation list: %v", err)
+			}
+			totalCleanedUpIps = append(totalCleanedUpIps, cleanedUpIpsPerPool...)
 		}
-		totalCleanedUpIps = append(totalCleanedUpIps, deallocatedIP)
 	}
 
 	return totalCleanedUpIps, nil