[v1.14] ipam: Fix bug where IP lease did not expire

pkg/ipam/allocator.go

@@ -247,7 +247,7 @@ func (ipam *IPAM) AllocateNextWithExpiration(family, owner string, pool Pool, ti
 	if timeout != time.Duration(0) {
 		for _, result := range []*AllocationResult{ipv4Result, ipv6Result} {
 			if result != nil {
-				result.ExpirationUUID, err = ipam.StartExpirationTimer(result.IP, pool, timeout)
+				result.ExpirationUUID, err = ipam.StartExpirationTimer(result.IP, result.IPPoolName, timeout)
 				if err != nil {
 					if ipv4Result != nil {
 						ipam.ReleaseIP(ipv4Result.IP, ipv4Result.IPPoolName)
@@ -290,7 +290,12 @@ func (ipam *IPAM) releaseIPLocked(ip net.IP, pool Pool) error {
 		"ip":    ip.String(),
 		"owner": owner,
 	}).Debugf("Released IP")
-	delete(ipam.expirationTimers, ip.String())
+
+	key := timerKey{ip: ip.String(), pool: pool}
+	if t, ok := ipam.expirationTimers[key]; ok {
+		close(t.stop)
+		delete(ipam.expirationTimers, key)
+	}
 
 	metrics.IpamEvent.WithLabelValues(metricRelease, string(family)).Inc()
 	return nil
@@ -364,24 +369,35 @@ func (ipam *IPAM) StartExpirationTimer(ip net.IP, pool Pool, timeout time.Durati
 	ipam.allocatorMutex.Lock()
 	defer ipam.allocatorMutex.Unlock()
 
-	ipString := ip.String()
-	if _, ok := ipam.expirationTimers[ipString]; ok {
+	key := timerKey{ip: ip.String(), pool: pool}
+	if _, ok := ipam.expirationTimers[key]; ok {
 		return "", fmt.Errorf("expiration timer already registered")
 	}
 
 	allocationUUID := uuid.New().String()
-	ipam.expirationTimers[ipString] = allocationUUID
+	stop := make(chan struct{})
+	ipam.expirationTimers[key] = expirationTimer{
+		uuid: allocationUUID,
+		stop: stop,
+	}
 
-	go func(ip net.IP, allocationUUID string, timeout time.Duration) {
-		ipString := ip.String()
-		time.Sleep(timeout)
+	go func(key timerKey, ip net.IP, pool Pool, allocationUUID string, timeout time.Duration, stop <-chan struct{}) {
+		timer := time.NewTimer(timeout)
+		select {
+		case <-stop:
+			// Expiration timer was explicitly stopped before timeout.
+			// Ensure time.Timer can be garbage collected and exit
+			timer.Stop()
+			return
+		case <-timer.C:
+		}
 
 		ipam.allocatorMutex.Lock()
 		defer ipam.allocatorMutex.Unlock()
 
-		if currentUUID, ok := ipam.expirationTimers[ipString]; ok {
-			if currentUUID == allocationUUID {
-				scopedLog := log.WithFields(logrus.Fields{"ip": ipString, "uuid": allocationUUID})
+		if t, ok := ipam.expirationTimers[key]; ok {
+			if t.uuid == allocationUUID {
+				scopedLog := log.WithFields(logrus.Fields{"ip": ip, "pool": pool, "uuid": allocationUUID})
 				if err := ipam.releaseIPLocked(ip, pool); err != nil {
 					scopedLog.WithError(err).Warning("Unable to release IP after expiration")
 				} else {
@@ -395,7 +411,7 @@ func (ipam *IPAM) StartExpirationTimer(ip net.IP, pool Pool, timeout time.Durati
 		} else {
 			// Expiration timer was removed. No action is required
 		}
-	}(ip, allocationUUID, timeout)
+	}(key, ip, pool, allocationUUID, timeout, stop)
 
 	return allocationUUID, nil
 }
@@ -408,14 +424,16 @@ func (ipam *IPAM) StopExpirationTimer(ip net.IP, pool Pool, allocationUUID strin
 	ipam.allocatorMutex.Lock()
 	defer ipam.allocatorMutex.Unlock()
 
-	ipString := ip.String()
-	if currentUUID, ok := ipam.expirationTimers[ipString]; !ok {
+	key := timerKey{ip: ip.String(), pool: pool}
+	t, ok := ipam.expirationTimers[key]
+	if !ok {
 		return fmt.Errorf("no expiration timer registered")
-	} else if currentUUID != allocationUUID {
+	} else if t.uuid != allocationUUID {
 		return fmt.Errorf("UUID mismatch, not stopping expiration timer")
 	}
 
-	delete(ipam.expirationTimers, ipString)
+	close(t.stop)
+	delete(ipam.expirationTimers, key)
 
 	return nil
 }

pkg/ipam/allocator_test.go

@@ -115,7 +115,9 @@ func (s *IPAMSuite) TestAllocateNextWithExpiration(c *C) {
 	fakeAddressing := fake.NewNodeAddressing()
 	ipam := NewIPAM(fakeAddressing, &testConfiguration{}, &ownerMock{}, &ownerMock{}, &mtuMock, nil)
 
-	ipv4, ipv6, err := ipam.AllocateNextWithExpiration("", "foo", PoolDefault, timeout)
+	// Allocate IPs and test expiration timer. 'pool' is empty in order to test
+	// that the allocated pool is passed to StartExpirationTimer
+	ipv4, ipv6, err := ipam.AllocateNextWithExpiration("", "foo", "", timeout)
 	c.Assert(err, IsNil)
 
 	// IPv4 address must be in use

pkg/ipam/ipam.go

@@ -112,7 +112,7 @@ func NewIPAM(nodeAddressing types.NodeAddressing, c Configuration, owner Owner,
 		nodeAddressing:   nodeAddressing,
 		config:           c,
 		owner:            map[Pool]map[string]string{},
-		expirationTimers: map[string]string{},
+		expirationTimers: map[timerKey]expirationTimer{},
 		excludedIPs:      map[string]string{},
 	}
 

pkg/ipam/types.go

@@ -96,7 +96,7 @@ type IPAM struct {
 	// expirationTimers is a map of all expiration timers. Each entry
 	// represents a IP allocation which is protected by an expiration
 	// timer.
-	expirationTimers map[string]string
+	expirationTimers map[timerKey]expirationTimer
 
 	// mutex covers access to all members of this struct
 	allocatorMutex lock.RWMutex
@@ -145,3 +145,13 @@ func (p Pool) String() string {
 const (
 	PoolDefault Pool = ipamOption.PoolDefault
 )
+
+type timerKey struct {
+	ip   string
+	pool Pool
+}
+
+type expirationTimer struct {
+	uuid string
+	stop chan<- struct{}
+}