diff --git a/cmd/whereabouts.go b/cmd/whereabouts.go index 50ca5812..c64854b2 100644 --- a/cmd/whereabouts.go +++ b/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 } diff --git a/pkg/allocate/allocate.go b/pkg/allocate/allocate.go index 88dead47..c7d59634 100644 --- a/pkg/allocate/allocate.go +++ b/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 { diff --git a/pkg/reconciler/ip_test.go b/pkg/reconciler/ip_test.go index 7e54d4e2..650ae15a 100644 --- a/pkg/reconciler/ip_test.go +++ b/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()) - }) - }) }) }) diff --git a/pkg/reconciler/iploop.go b/pkg/reconciler/iploop.go index cc2b8fd6..adb22ec2 100644 --- a/pkg/reconciler/iploop.go +++ b/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 -} diff --git a/pkg/storage/kubernetes/ipam.go b/pkg/storage/kubernetes/ipam.go index 299c3ee5..32f043dc 100644 --- a/pkg/storage/kubernetes/ipam.go +++ b/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 } }