Fix IPAM IP release racing condition

[ upstream commit 7fbf7c6 ]

Racing condition when IP reassigned back to ENI,
during IPAM release process.

Signed-off-by: Victor Chen <victor.chenq@gmail.com>
Signed-off-by: Tobias Klauser <tobias@cilium.io>

Author: vcatlassian

pkg/ipam/node.go

@@ -758,9 +758,19 @@ func (n *Node) abortNoLongerExcessIPs(excessMap map[string]bool) {
 		}
 		// Handshake can be aborted from every state except 'released'
 		// 'released' state is removed by the agent once the IP has been removed from ciliumnode's IPAM pool as well.
+		// But if the IP is back in the pool, we need to remove it from the release status map.
 		if status == ipamOption.IPAMReleased {
+			// Check if the IP is back in the pool despite being marked as released
+			if _, ok := n.resource.Spec.IPAM.Pool[ip]; ok {
+				delete(n.resource.Status.IPAM.ReleaseIPs, ip)
+				delete(n.ipv4Alloc.ipsMarkedForRelease, ip)
+				delete(n.ipv4Alloc.ipReleaseStatus, ip)
+			}
+
+			// If it's still released and not in the pool, we don't need to do anything
 			continue
 		}
+
 		if status, ok := n.ipv4Alloc.ipReleaseStatus[ip]; ok && status != ipamOption.IPAMReleased {
 			delete(n.ipv4Alloc.ipsMarkedForRelease, ip)
 			delete(n.ipv4Alloc.ipReleaseStatus, ip)

pkg/ipam/node_manager_test.go

@@ -17,6 +17,7 @@ import (
 	operatorOption "github.com/cilium/cilium/operator/option"
 	"github.com/cilium/cilium/pkg/defaults"
 	metricsmock "github.com/cilium/cilium/pkg/ipam/metrics/mock"
+	ipamOption "github.com/cilium/cilium/pkg/ipam/option"
 	ipamStats "github.com/cilium/cilium/pkg/ipam/stats"
 	ipamTypes "github.com/cilium/cilium/pkg/ipam/types"
 	v2 "github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2"
@@ -588,6 +589,139 @@ func TestNodeManagerAbortRelease(t *testing.T) {
 	require.Equal(t, 3, node.Stats().IPv4.UsedIPs)
 }
 
+// TestNodeManagerAbortReleaseIPReassignment tests a scenario where:
+// 1. An IP is released and marked as released in status.ipam.release-ips
+// 2. The IP is removed from spec.ipam.pool
+// 3. Before being removed from status.ipam.release-ips, the same IP is reassigned to the pool (ie. AWS ENI in the case of AWS ENI IPAM)
+// 4. The IP is no longer considered excess, so the release handshake is aborted and the IP is deleted from status.ipam.release-ips
+func TestNodeManagerAbortReleaseIPReassignment(t *testing.T) {
+	operatorOption.Config.ExcessIPReleaseDelay = 2
+	am := newAllocationImplementationMock()
+	require.NotNil(t, am)
+	mngr, err := NewNodeManager(am, k8sapi, metricsmock.NewMockMetrics(), 10, true, false)
+	require.NoError(t, err)
+	require.NotNil(t, mngr)
+
+	// Announce node, wait for IPs to become available
+	cn := newCiliumNode("node4", 1, 3, 0)
+	mngr.Upsert(cn)
+	require.NoError(t, testutils.WaitUntil(func() bool { return reachedAddressesNeeded(mngr, "node4", 0) }, 5*time.Second))
+
+	node := mngr.Get("node4")
+	require.NotNil(t, node)
+	require.Equal(t, 3, node.Stats().IPv4.AvailableIPs)
+	require.Equal(t, 0, node.Stats().IPv4.UsedIPs)
+
+	// Use 3 out of 4 IPs
+	mngr.Upsert(updateCiliumNode(cn, 3))
+	require.NoError(t, testutils.WaitUntil(func() bool { return reachedAddressesNeeded(mngr, "node4", 0) }, 5*time.Second))
+
+	node = mngr.Get("node4")
+	require.NotNil(t, node)
+	require.Equal(t, 4, node.Stats().IPv4.AvailableIPs)
+	require.Equal(t, 3, node.Stats().IPv4.UsedIPs)
+
+	// Release one IP
+	mngr.Upsert(updateCiliumNode(node.resource, 2))
+
+	node = mngr.Get("node4")
+	require.NotNil(t, node)
+	require.Equal(t, 4, node.Stats().IPv4.AvailableIPs)
+	require.Equal(t, 2, node.Stats().IPv4.UsedIPs)
+
+	node.instanceSync.Trigger()
+
+	// This is the key IP that will be released and later reassigned
+	var releasedIP string
+
+	// Wait for the IP to be marked as excess for longer than the ExcessIPReleaseDelay
+	require.Eventually(t, func() bool {
+		node.mutex.Lock()
+		defer node.mutex.Unlock()
+
+		if len(node.ipv4Alloc.ipsMarkedForRelease) == 0 {
+			return false
+		}
+
+		for _, ts := range node.ipv4Alloc.ipsMarkedForRelease {
+			if ts.Add(time.Duration(operatorOption.Config.ExcessIPReleaseDelay) * time.Second).Before(time.Now()) {
+				return true
+			}
+		}
+
+		return false
+	}, 10*time.Second, time.Second)
+
+	// Mark IP as excess
+	node.instanceSync.Trigger()
+
+	// Wait for maintenance action to identify the IP to release
+	require.Eventually(t, func() bool {
+		a, err := node.determineMaintenanceAction()
+		if err != nil || a == nil || a.release == nil || len(a.release.IPsToRelease) == 0 {
+			return false
+		}
+
+		// Get the IP being released
+		releasedIP = a.release.IPsToRelease[0]
+		return releasedIP != ""
+	}, 10*time.Second, time.Second)
+
+	node.PopulateIPReleaseStatus(node.resource)
+
+	// Verify it's marked for release in the CiliumNode resource
+	require.Contains(t, node.resource.Status.IPAM.ReleaseIPs, releasedIP)
+	require.Equal(t, ipamOption.IPAMMarkForRelease, string(node.resource.Status.IPAM.ReleaseIPs[releasedIP]))
+
+	// Fake acknowledge IP for release like agent would
+	testipam.FakeAcknowledgeReleaseIps(node.resource)
+
+	// Resync one more time to process acknowledgements.
+	node.instanceSync.Trigger()
+
+	require.Eventually(t, func() bool {
+		status, exists := node.resource.Status.IPAM.ReleaseIPs[releasedIP]
+		return exists && string(status) == ipamOption.IPAMReadyForRelease
+	}, 10*time.Second, time.Second)
+
+	// Now simulate the operator releasing the IP and marking it as released
+	node.mutex.Lock()
+	delete(node.resource.Spec.IPAM.Pool, releasedIP)
+	node.resource.Status.IPAM.ReleaseIPs[releasedIP] = ipamOption.IPAMReleased
+	node.mutex.Unlock()
+	// Also mark it as released in the internal ipReleaseStatus map
+	node.ipv4Alloc.ipReleaseStatus[releasedIP] = ipamOption.IPAMReleased
+
+	// Normally at this point, the agent would see the IP is released and remove it from Status.IPAM.ReleaseIPs
+	// But before that happens, simulate the IP being reassigned back to the pool
+	if node.resource.Spec.IPAM.Pool == nil {
+		node.resource.Spec.IPAM.Pool = ipamTypes.AllocationMap{}
+	}
+	node.resource.Spec.IPAM.Pool[releasedIP] = ipamTypes.AllocationIP{Resource: "eni-test"}
+	node.ops.AllocateIPs(context.Background(), &AllocationAction{
+		IPv4: IPAllocationAction{
+			AvailableForAllocation: 1,
+		},
+	})
+
+	node.poolMaintainer.Trigger()
+	node.instanceSync.Trigger()
+
+	require.Eventually(t, func() bool {
+		node.PopulateIPReleaseStatus(node.resource)
+		_, inReleaseStatus := node.ipv4Alloc.ipReleaseStatus[releasedIP]
+		_, inMarkedForRelease := node.ipv4Alloc.ipsMarkedForRelease[releasedIP]
+		_, inReleaseIPs := node.resource.Status.IPAM.ReleaseIPs[releasedIP]
+
+		return !inReleaseStatus && !inMarkedForRelease && !inReleaseIPs
+	}, 10*time.Second, time.Second)
+
+	node = mngr.Get("node4")
+	require.NotNil(t, node)
+	require.Equal(t, 4, node.Stats().IPv4.AvailableIPs)
+	require.Equal(t, 2, node.Stats().IPv4.UsedIPs)
+}
+
 type nodeState struct {
 	cn           *v2.CiliumNode
 	name         string