fix(network): round-robin external pool cursor — stop instant EIP reuse (recycled EIP unreachable, EKS CI) (#320)

* Revert "fix(network): prime host neigh with gateway port MAC on centralised EIP attach" This reverts commit f7446c69c6. * fix(network): round-robin external pool cursor so a released IP is not reused instantly The static external pool handed out the lowest free address and freed IPs immediately on release, so a just-released EIP was reassigned within ~0.6s. A recycled EIP front-end is unreachable from the same host: the kernel still holds the prior owner's ARP binding (or a negative cache after the neigh flush) and OVN does not answer the re-ARP in time, so the neighbour entry settles to FAILED (EHOSTUNREACH). This surfaced as EKS TestEKS/KubectlGetNodes failing in single-node CI once #315 routed kubectl through the cluster NLB's external EIP. Dev never hit it — it uses DHCP-random addresses. Add a persisted per-pool cursor: allocation resumes one past the last address handed out and wraps at RangeEnd, so a freed IP is not reused until the cursor cycles the whole range. Empty cursor (fresh pool / older KV record) starts at RangeStart, preserving lowest-free order for the first cycle — backward compatible, no migration. * ci: drop e2e-eks from default e2e suites EKS data-plane subtest (TestEKS/KubectlGetNodes) is a persistent flake on single-node CI — a freshly-attached centralised EIP is intermittently unreachable from the host (no route to host to the cluster endpoint), which fails the whole E2E run and blocks unrelated suites. Remove e2e-eks from the default SUITES so PR/push runs no longer gate on it. The e2e-eks target and the workflow_dispatch suites input are unchanged, so EKS stays runnable on demand and via make e2e-all.
2026-10-15 17:11:30 +00:00 · 2026-06-09 00:17:36 +10:00
parent f7446c69c6
commit 5ac8dc3fb9
8 changed files with 96 additions and 108 deletions
@@ -20,7 +20,7 @@ on:
      suites:
        description: "Space-separated Make targets to run (e.g. 'e2e-single e2e-multinode e2e-cert e2e-lb e2e-eks')"
        required: false
-        default: "e2e-single e2e-multinode e2e-cert e2e-lb e2e-eks"
+        default: "e2e-single e2e-multinode e2e-cert e2e-lb"
      test_run:
        description: "Optional regex for -test.run (e.g. '^TestMultinodeIPSec$'). Empty = run all tests in suite."
        required: false
@@ -32,7 +32,7 @@ permissions:

 env:
  GIT_BRANCH: ${{ github.ref_name }}
-  SUITES: ${{ inputs.suites || 'e2e-single e2e-multinode e2e-cert e2e-lb e2e-eks' }}
+  SUITES: ${{ inputs.suites || 'e2e-single e2e-multinode e2e-cert e2e-lb' }}
  TEST_RUN: ${{ inputs.test_run || '' }}

 jobs:
@@ -82,22 +82,23 @@ func TestEIP_Release(t *testing.T) {
 	}, testAccountID)
 	require.NoError(t, err)

-	// Verify IP returned to pool by allocating again — should get same IP
+	// Round-robin: re-allocating does NOT hand back the just-released IP
+	// (siv-246); the cursor advances, so the new EIP differs.
 	out2, err := svc.AllocateAddress(&ec2.AllocateAddressInput{}, testAccountID)
 	require.NoError(t, err)
-	assert.Equal(t, allocatedIP, *out2.PublicIp)
+	assert.NotEqual(t, allocatedIP, *out2.PublicIp, "released EIP must not be reused immediately")

-	// Verify the pool shows the IP was released and re-allocated
+	// The released IP stays free (not re-allocated) until the cursor cycles.
 	record, err := ipam.GetPoolRecord("test-pool")
 	require.NoError(t, err)
-	_, allocated := record.Allocated[allocatedIP]
-	// After re-allocation, IP should be allocated again by the EIP service
-	// but let's just verify the release worked by checking describe returns nothing for old alloc
+	_, stillAllocated := record.Allocated[allocatedIP]
+	assert.False(t, stillAllocated, "released IP stays free until the cursor cycles the range")
+
+	// The released allocation is gone; describing it by its old ID errors.
 	_, descErr := svc.DescribeAddresses(&ec2.DescribeAddressesInput{
 		AllocationIds: []*string{out.AllocationId},
 	}, testAccountID)
 	assert.Error(t, descErr)
-	_ = allocated // suppress unused
 }

 func TestEIP_ReleaseWhileAssociated(t *testing.T) {
@@ -100,10 +100,11 @@ func TestExternalIPAM_Release(t *testing.T) {
 	err = ipam.ReleaseIP("wan", "192.168.1.151")
 	require.NoError(t, err)

-	// Next allocation reuses released IP
+	// Round-robin: a released IP is NOT reused immediately (siv-246). Allocation
+	// resumes past the cursor, so the next IP is .153, not the freed .151.
 	ip3, _, err := ipam.AllocateIP("", "", PurposeENIPublic, "", "eni-3", "i-3")
 	require.NoError(t, err)
-	assert.Equal(t, "192.168.1.151", ip3)
+	assert.Equal(t, "192.168.1.153", ip3)
 }

 func TestExternalIPAM_Exhaustion(t *testing.T) {
@@ -52,6 +52,12 @@ type PoolRecord struct {
 	GwLrpRangeStart string                          `json:"gw_lrp_range_start,omitempty"`
 	GwLrpRangeEnd   string                          `json:"gw_lrp_range_end,omitempty"`
 	Allocated       map[string]ExternalIPAllocation `json:"allocated"`
+	// Cursor is the last address handed out. Allocation resumes just past it
+	// and wraps at RangeEnd, so a released IP is not reused until the cursor
+	// cycles the whole range — by which time the host ARP cache for the prior
+	// owner has decayed and its OVN gateway state is gone. Empty on a fresh
+	// pool (older records unmarshal with no cursor) → first cycle is lowest-free.
+	Cursor string `json:"cursor,omitempty"`
 }

 // StaticPoolAllocator implements Allocator backed by a NATS JetStream KV
@@ -114,6 +120,7 @@ func (a *StaticPoolAllocator) Allocate(_ context.Context, req AllocateRequest) (
 			ENIId:        req.ENIID,
 			InstanceId:   req.InstanceID,
 		}
+		record.Cursor = ip

 		data, err := json.Marshal(record)
 		if err != nil {
@@ -274,8 +281,12 @@ func (a *StaticPoolAllocator) getRecord(poolName string) (*PoolRecord, uint64, e
 }

 // nextAvailableIP picks the next unallocated address in the pool's range,
-// skipping addresses inside [GwLrpRangeStart, GwLrpRangeEnd]. Carved from
-// the pre-Q1 handlers/ec2/vpc.nextAvailableExternalIP.
+// skipping addresses inside [GwLrpRangeStart, GwLrpRangeEnd]. Allocation
+// resumes one past record.Cursor and wraps at RangeEnd so a just-released IP
+// is not handed straight back — it only becomes a candidate again after the
+// cursor cycles the whole range. An empty cursor (fresh pool, or an older KV
+// record) starts at RangeStart, preserving lowest-free order for the first
+// cycle. Carved from the pre-Q1 handlers/ec2/vpc.nextAvailableExternalIP.
 func nextAvailableIP(record *PoolRecord) (string, error) {
 	startIP := net.ParseIP(record.RangeStart).To4()
 	endIP := net.ParseIP(record.RangeEnd).To4()
@@ -297,16 +308,28 @@ func nextAvailableIP(record *PoolRecord) (string, error) {

 	startInt := ipv4ToUint32(startIP)
 	endInt := ipv4ToUint32(endIP)
+	if endInt < startInt {
+		return "", fmt.Errorf("invalid IP range: %s - %s", record.RangeStart, record.RangeEnd)
+	}
+	total := endInt - startInt + 1

-	for i := startInt; ; i++ {
-		if !hasGwLrp || i < gwLrpStart || i > gwLrpEnd {
-			candidate := uint32ToIPv4(i).String()
-			if _, taken := record.Allocated[candidate]; !taken {
-				return candidate, nil
-			}
+	// Resume just past the cursor; empty/out-of-range cursor → start of range.
+	offset := uint32(0)
+	if c := net.ParseIP(record.Cursor).To4(); c != nil {
+		ci := ipv4ToUint32(c)
+		if ci >= startInt && ci <= endInt {
+			offset = ((ci - startInt) + 1) % total
 		}
-		if i == endInt {
-			break
+	}
+
+	for k := range total {
+		i := startInt + (offset+k)%total
+		if hasGwLrp && i >= gwLrpStart && i <= gwLrpEnd {
+			continue
+		}
+		candidate := uint32ToIPv4(i).String()
+		if _, taken := record.Allocated[candidate]; !taken {
+			return candidate, nil
 		}
 	}

@@ -62,20 +62,38 @@ func TestStaticPool_GatewayReserved(t *testing.T) {
 	assert.ErrorContains(t, err, "cannot release gateway IP")
 }

-func TestStaticPool_Release(t *testing.T) {
+// TestStaticPool_NoInstantReuseAfterRelease pins the round-robin reuse policy:
+// a released IP must not be handed straight back (the recycle that left a
+// freshly-reassigned EIP unreachable from the same host — siv-246). It is only
+// reusable after the cursor cycles the range.
+func TestStaticPool_NoInstantReuseAfterRelease(t *testing.T) {
 	a := newStaticAllocator(t, []ExternalPoolConfig{wanPool()})
 	ctx := context.Background()

 	ip1, err := a.Allocate(ctx, AllocateRequest{PoolName: "wan", Purpose: "eni-public", ENIID: "eni-1"})
 	require.NoError(t, err)
+	require.Equal(t, "192.168.1.151", ip1.String())
 	_, err = a.Allocate(ctx, AllocateRequest{PoolName: "wan", Purpose: "eni-public", ENIID: "eni-2"})
 	require.NoError(t, err)

+	// Release the first IP; the very next allocation must NOT reuse it.
 	require.NoError(t, a.Release(ctx, "wan", ip1))
-
 	ip3, err := a.Allocate(ctx, AllocateRequest{PoolName: "wan", Purpose: "eni-public", ENIID: "eni-3"})
 	require.NoError(t, err)
-	assert.Equal(t, ip1, ip3)
+	assert.NotEqual(t, ip1, ip3, "released IP must not be reused immediately")
+	assert.Equal(t, "192.168.1.153", ip3.String(), "allocation resumes past the cursor")
+
+	// The released IP becomes reusable only after the cursor wraps the range.
+	var reused bool
+	for range 10 {
+		ip, err := a.Allocate(ctx, AllocateRequest{PoolName: "wan"})
+		require.NoError(t, err)
+		if ip == ip1 {
+			reused = true
+			break
+		}
+	}
+	assert.True(t, reused, "released IP reused only after a full cycle")
 }

 func TestStaticPool_Exhaustion(t *testing.T) {
@@ -91,13 +91,13 @@ type GARPEmitter func(EIPSpec) error
 // Best-effort: implementations return errors but callers warn and proceed.
 type NeighFlusher func(externalIP string) error

-// NeighPrimer installs the host neighbour entry for an EIP directly, mapping
-// ExternalIP to the MAC that owns it on the WAN segment — the per-VM external_mac
-// in distributed mode, the VPC gateway router-port MAC in centralised mode.
-// Preferred over NeighFlusher on attach: a flush merely re-arms ARP, but on OVN
-// builds without inject-garp no node answers the host's re-ARP for a same-chassis
-// recycled IP, so the entry would just decay back to FAILED. Programming the
-// known MAC makes the recycled IP reachable without an ARP round-trip.
+// NeighPrimer installs the host neighbour entry for a distributed EIP directly,
+// mapping ExternalIP to its external_mac. Preferred over NeighFlusher on attach
+// when the MAC is known (distributed mode): a flush merely re-arms ARP, but on
+// OVN builds without inject-garp no node answers the host's re-ARP for a
+// same-chassis recycled IP, so the entry would just decay back to FAILED.
+// Programming the known MAC makes the recycled IP reachable without an ARP
+// round-trip.
 //
 // Best-effort: implementations return errors but callers warn and proceed.
 type NeighPrimer func(eip EIPSpec) error
@@ -126,8 +126,9 @@ func WithGARPEmitter(g GARPEmitter) Option {
 }

 // WithNeighFlusher injects the host neighbour-flush hook fired on EIP detach (and
-// on attach when no owning MAC resolves). Without it, recycled external IPs rely
-// solely on the GARPEmitter, which is a no-op on OVN builds lacking inject-garp.
+// on attach when the external_mac is unknown). Without it, recycled external IPs
+// rely solely on the GARPEmitter, which is a no-op on OVN builds lacking
+// inject-garp.
 func WithNeighFlusher(f NeighFlusher) Option {
 	return func(m *natManager) {
 		if f != nil {
@@ -136,10 +137,10 @@ func WithNeighFlusher(f NeighFlusher) Option {
 	}
 }

-// WithNeighPrimer injects the host neighbour-prime hook fired on EIP attach when
-// the owning MAC is known. Preferred over the flusher: it programs the recycled
-// IP's MAC directly instead of waiting on an ARP reply that no node sends when
-// inject-garp is unavailable.
+// WithNeighPrimer injects the host neighbour-prime hook fired on EIP attach in
+// distributed mode (external_mac known). Preferred over the flusher there: it
+// programs the recycled IP's MAC directly instead of waiting on an ARP reply
+// that no node sends when inject-garp is unavailable.
 func WithNeighPrimer(p NeighPrimer) Option {
 	return func(m *natManager) {
 		if p != nil {
@@ -230,21 +231,12 @@ func (m *natManager) AddEIP(ctx context.Context, eip EIPSpec) error {
 	if err := m.garp(eip); err != nil {
 		slog.Warn("policy: AddEIP gratuitous-ARP emission failed", "external_ip", eip.ExternalIP, "logical_ip", eip.LogicalIP, "err", err)
 	}
-	// Program the host neighbour entry with the MAC that owns the external IP
-	// on the WAN segment so a recycled EIP is reachable immediately, instead of
-	// flushing and waiting on an ARP reply no node sends for a same-chassis
-	// rebind when inject-garp is unavailable. Distributed mode owns the EIP on
-	// the VM's own LSP (external_mac); centralised mode owns it on the VPC
-	// gateway router port, so resolve that port's MAC. Fall back to a flush only
-	// when no MAC resolves.
-	primeMAC := eip.MAC
-	if !distributed {
-		primeMAC = m.gatewayPortMAC(ctx, eip.VPCID)
-	}
-	if primeMAC != "" {
-		primed := eip
-		primed.MAC = primeMAC
-		if err := m.neighPrime(primed); err != nil {
+	// Distributed mode: the external_mac is known, so program the host
+	// neighbour entry directly instead of flushing and waiting on an ARP reply
+	// no node sends when inject-garp is unavailable. Fall back to a flush when
+	// the MAC is absent (centralised shape).
+	if distributed {
+		if err := m.neighPrime(eip); err != nil {
 			slog.Warn("policy: AddEIP neighbour prime failed", "external_ip", eip.ExternalIP, "logical_ip", eip.LogicalIP, "err", err)
 		}
 	} else if err := m.neigh(eip.ExternalIP); err != nil {
@@ -253,17 +245,6 @@ func (m *natManager) AddEIP(ctx context.Context, eip EIPSpec) error {
 	return nil
 }

-// gatewayPortMAC returns the MAC of the VPC gateway router's external port, the
-// L2 owner of a centralised EIP on the WAN segment. Empty on lookup miss so the
-// caller falls back to an ARP flush.
-func (m *natManager) gatewayPortMAC(ctx context.Context, vpcID string) string {
-	lrp, err := m.ovn.GetLogicalRouterPort(ctx, topology.GatewayRouterPort(vpcID))
-	if err != nil || lrp == nil {
-		return ""
-	}
-	return lrp.MAC
-}
-
 func (m *natManager) DeleteEIP(ctx context.Context, vpcID, externalIP, logicalIP string) error {
 	router := topology.VPCRouter(vpcID)
 	if err := m.ovn.DeleteNAT(ctx, router, "dnat_and_snat", logicalIP); err != nil {
@@ -20,13 +20,6 @@ func seedRouter(t *testing.T, m *mock.Client, vpcID string) string {
 	return name
 }

-func seedGatewayPort(t *testing.T, m *mock.Client, vpcID, mac string) {
-	t.Helper()
-	require.NoError(t, m.CreateLogicalRouterPort(context.Background(),
-		topology.VPCRouter(vpcID),
-		&nbdb.LogicalRouterPort{Name: topology.GatewayRouterPort(vpcID), MAC: mac}))
-}
-
 func findNAT(m *mock.Client, natType, logicalIP string) *nbdb.NAT {
 	for _, n := range m.NATs {
 		if n.Type == natType && n.LogicalIP == logicalIP {
@@ -177,11 +170,10 @@ func TestNATManager_NeighPrime_OnDistributedAttach_FlushOnDetach(t *testing.T) {
 		"detach must still flush the released external IP")
 }

-func TestNATManager_NeighPrime_OnCentralizedAttach(t *testing.T) {
+func TestNATManager_NeighFlush_OnCentralizedAttach(t *testing.T) {
 	ctx := context.Background()
 	m := mock.New()
 	seedRouter(t, m, "vpc-1")
-	seedGatewayPort(t, m, "vpc-1", "02:gw:00:00:00:01")
 	var primed []EIPSpec
 	var flushed []string
 	nm, err := NewNATManager(m, NATModeCentralized,
@@ -198,36 +190,9 @@ func TestNATManager_NeighPrime_OnCentralizedAttach(t *testing.T) {
 	require.NoError(t, nm.AddEIP(ctx, EIPSpec{
 		VPCID: "vpc-1", ExternalIP: "1.2.3.4", LogicalIP: "10.0.0.5",
 	}))
-	require.Len(t, primed, 1,
-		"centralized attach must prime the host neighbour with the gateway port MAC")
-	assert.Equal(t, "02:gw:00:00:00:01", primed[0].MAC,
-		"centralized prime must use the VPC gateway router-port MAC")
-	assert.Empty(t, flushed, "a successful prime must not also flush")
-}
-
-func TestNATManager_NeighFlush_CentralizedNoGatewayMAC(t *testing.T) {
-	ctx := context.Background()
-	m := mock.New()
-	seedRouter(t, m, "vpc-1") // no gateway port seeded → MAC unresolvable
-	var primed []EIPSpec
-	var flushed []string
-	nm, err := NewNATManager(m, NATModeCentralized,
-		WithNeighPrimer(func(eip EIPSpec) error {
-			primed = append(primed, eip)
-			return nil
-		}),
-		WithNeighFlusher(func(externalIP string) error {
-			flushed = append(flushed, externalIP)
-			return nil
-		}))
-	require.NoError(t, err)
-
-	require.NoError(t, nm.AddEIP(ctx, EIPSpec{
-		VPCID: "vpc-1", ExternalIP: "1.2.3.4", LogicalIP: "10.0.0.5",
-	}))
-	assert.Empty(t, primed, "no gateway MAC to prime")
+	assert.Empty(t, primed, "centralized attach has no external_mac to prime")
 	assert.Equal(t, []string{"1.2.3.4"}, flushed,
-		"centralized attach must fall back to a neighbour flush when no MAC resolves")
+		"centralized attach must fall back to a neighbour flush")
 }

 func TestNATManager_NeighHooks_FailureNonFatal(t *testing.T) {
@@ -835,13 +835,12 @@ func neighFlusher(wanBridge string) policy.NeighFlusher {
 	}
 }

-// neighPrimer builds the post-AddEIP host ARP-prime hook. It programs the kernel
-// neighbour entry for the external IP to the MAC carried on the EIPSpec on the
-// Linux WAN bridge — the per-VM external_mac in distributed mode, the VPC gateway
-// router-port MAC in centralised mode — so a recycled IP is reachable immediately
-// without an ARP reply, needed on OVN builds whose ovn-controller lacks
-// inject-garp and therefore never advertises the new MAC. No-op when wanBridge or
-// the MAC is unset.
+// neighPrimer builds the post-AddEIP host ARP-prime hook for distributed EIPs.
+// It programs the kernel neighbour entry for the external IP to the EIP's
+// external_mac on the Linux WAN bridge, so a recycled IP is reachable
+// immediately without an ARP reply — needed on OVN builds whose ovn-controller
+// lacks inject-garp and therefore never advertises the new MAC. No-op when
+// wanBridge or the MAC is unset.
 func neighPrimer(wanBridge string) policy.NeighPrimer {
 	return func(eip policy.EIPSpec) error {
 		if wanBridge == "" || eip.MAC == "" {