Merge pull request #467 from mlguerrero12/fixwrongdeleletionduplicate

Use IP to identify orphaned allocation to be deleted

cmd/whereabouts.go

@@ -19,14 +19,14 @@ import (
 
 func cmdAddFunc(args *skel.CmdArgs) error {
 	ipamConf, confVersion, err := config.LoadIPAMConfig(args.StdinData, args.Args)
-    if err != nil {
+	if err != nil {
 		logging.Errorf("IPAM configuration load failed: %s", err)
 		return err
 	}
 	logging.Debugf("ADD - IPAM configuration successfully read: %+v", *ipamConf)
 	ipam, err := kubernetes.NewKubernetesIPAM(args.ContainerID, *ipamConf)
 	if err != nil {
-			return logging.Errorf("failed to create Kubernetes IPAM manager: %v", err)
+		return logging.Errorf("failed to create Kubernetes IPAM manager: %v", err)
 	}
 	defer func() { safeCloseKubernetesBackendConnection(ipam) }()
 	return cmdAdd(args, ipam, confVersion)
@@ -48,15 +48,14 @@ func cmdDelFunc(args *skel.CmdArgs) error {
 	return cmdDel(args, ipam)
 }
 
-
 func main() {
 	skel.PluginMainFuncs(skel.CNIFuncs{
-		Add:    cmdAddFunc,
-		Check:  cmdCheck,
-		Del:    cmdDelFunc,
-	}, 
-	cniversion.All, 
-	fmt.Sprintf("whereabouts %s", version.GetFullVersionWithRuntimeInfo()))
+		Add:   cmdAddFunc,
+		Check: cmdCheck,
+		Del:   cmdDelFunc,
+	},
+		cniversion.All,
+		fmt.Sprintf("whereabouts %s", version.GetFullVersionWithRuntimeInfo()))
 }
 
 func safeCloseKubernetesBackendConnection(ipam *kubernetes.KubernetesIPAM) {
@@ -110,10 +109,7 @@ func cmdDel(args *skel.CmdArgs, client *kubernetes.KubernetesIPAM) error {
 	ctx, cancel := context.WithTimeout(context.Background(), types.DelTimeLimit)
 	defer cancel()
 
-	_, err := kubernetes.IPManagement(ctx, types.Deallocate, client.Config, client)
-	if err != nil {
-		logging.Verbosef("WARNING: Problem deallocating IP: %s", err)
-	}
+	_, _ = kubernetes.IPManagement(ctx, types.Deallocate, client.Config, client)
 
 	return nil
 }

pkg/allocate/allocate.go

@@ -36,46 +36,27 @@ func AssignIP(ipamConf types.RangeConfiguration, reservelist []types.IPReservati
 	return net.IPNet{IP: newip, Mask: ipnet.Mask}, updatedreservelist, nil
 }
 
-// DeallocateIP assigns an IP using a range and a reserve list.
-func DeallocateIP(reservelist []types.IPReservation, containerID string) ([]types.IPReservation, net.IP, error) {
-
-	updatedreservelist, hadip, err := IterateForDeallocation(reservelist, containerID, getMatchingIPReservationIndex)
-	if err != nil {
-		return nil, nil, err
-	}
-
-	logging.Debugf("Deallocating given previously used IP: %v", hadip)
-
-	return updatedreservelist, hadip, nil
-}
-
-// 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)
+// DeallocateIP removes allocation from reserve list. Returns the updated reserve list and the deallocated IP.
+func DeallocateIP(reservelist []types.IPReservation, containerID string) ([]types.IPReservation, net.IP) {
+	index := getMatchingIPReservationIndex(reservelist, containerID)
+	if index < 0 {
+		// Allocation not found. Return the original reserve list and nil IP.
+		return reservelist, nil
 	}
 
-	returnip := reservelist[foundidx].IP
+	ip := reservelist[index].IP
+	logging.Debugf("Deallocating given previously used IP: %v", ip.String())
 
-	updatedreservelist := removeIdxFromSlice(reservelist, foundidx)
-	return updatedreservelist, returnip, nil
+	return removeIdxFromSlice(reservelist, index), ip
 }
 
 func getMatchingIPReservationIndex(reservelist []types.IPReservation, id string) int {
-	foundidx := -1
 	for idx, v := range reservelist {
 		if v.ContainerID == id {
-			foundidx = idx
-			break
+			return idx
 		}
 	}
-	return foundidx
+	return -1
 }
 
 func removeIdxFromSlice(s []types.IPReservation, i int) []types.IPReservation {

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"
@@ -87,7 +86,7 @@ func (rl *ReconcileLooper) findOrphanedIPsPerPool(ipPools []storage.IPPool) erro
 				_ = logging.Errorf("pod ref missing for Allocations: %s", ipReservation)
 				continue
 			}
-			if !rl.isPodAlive(ipReservation.PodRef, ipReservation.IP.String()) {
+			if !rl.isOrphanedIP(ipReservation.PodRef, ipReservation.IP.String()) {
 				logging.Debugf("pod ref %s is not listed in the live pods list", ipReservation.PodRef)
 				orphanIP.Allocations = append(orphanIP.Allocations, ipReservation)
 			}
@@ -100,7 +99,7 @@ func (rl *ReconcileLooper) findOrphanedIPsPerPool(ipPools []storage.IPPool) erro
 	return nil
 }
 
-func (rl ReconcileLooper) isPodAlive(podRef string, ip string) bool {
+func (rl ReconcileLooper) isOrphanedIP(podRef string, ip string) bool {
 	for livePodRef, livePod := range rl.liveWhereaboutsPods {
 		if podRef == livePodRef {
 			isFound := isIpOnPod(&livePod, podRef, ip)
@@ -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
@@ -226,7 +234,7 @@ func (rl *ReconcileLooper) findClusterWideIPReservations(ctx context.Context) er
 
 		podRef := clusterWideIPReservation.Spec.PodRef
 
-		if !rl.isPodAlive(podRef, denormalizedip) {
+		if !rl.isOrphanedIP(podRef, denormalizedip) {
 			logging.Debugf("pod ref %s is not listed in the live pods list", podRef)
 			rl.orphanedClusterWideIPs = append(rl.orphanedClusterWideIPs, clusterWideIPReservation)
 		}
@@ -255,11 +263,3 @@ func (rl ReconcileLooper) ReconcileOverlappingIPAddresses(ctx context.Context) e
 	}
 	return nil
 }
-
-func findOutPodRefsToDeallocateIPsFrom(orphanedIP OrphanedIPReservations) []string {
-	var podRefsToDeallocate []string
-	for _, orphanedAllocation := range orphanedIP.Allocations {
-		podRefsToDeallocate = append(podRefsToDeallocate, orphanedAllocation.PodRef)
-	}
-	return podRefsToDeallocate
-}

pkg/storage/kubernetes/ipam.go

@@ -542,10 +542,11 @@ func IPManagementKubernetesUpdate(ctx context.Context, mode int, ipam *Kubernete
 				}
 
 			case whereaboutstypes.Deallocate:
-				updatedreservelist, ipforoverlappingrangeupdate, err = allocate.DeallocateIP(reservelist, containerID)
-				if err != nil {
-					logging.Errorf("Error deallocating IP: %v", err)
-					return newips, err
+				updatedreservelist, ipforoverlappingrangeupdate = allocate.DeallocateIP(reservelist, containerID)
+				if ipforoverlappingrangeupdate == nil {
+					// Do not fail if allocation was not found.
+					logging.Debugf("Failed to find allocation for container ID: %s", containerID)
+					return nil, nil
 				}
 			}