diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 197c47219..0d7742873 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -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: diff --git a/spinifex/handlers/ec2/eip/service_impl_test.go b/spinifex/handlers/ec2/eip/service_impl_test.go index f0c35df94..44df84e15 100644 --- a/spinifex/handlers/ec2/eip/service_impl_test.go +++ b/spinifex/handlers/ec2/eip/service_impl_test.go @@ -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) { diff --git a/spinifex/handlers/ec2/vpc/external_ipam_test.go b/spinifex/handlers/ec2/vpc/external_ipam_test.go index eb625c37c..0408f995c 100644 --- a/spinifex/handlers/ec2/vpc/external_ipam_test.go +++ b/spinifex/handlers/ec2/vpc/external_ipam_test.go @@ -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) { diff --git a/spinifex/network/external/static_pool.go b/spinifex/network/external/static_pool.go index cdcf2a513..bc00d13c8 100644 --- a/spinifex/network/external/static_pool.go +++ b/spinifex/network/external/static_pool.go @@ -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 } } diff --git a/spinifex/network/external/static_pool_test.go b/spinifex/network/external/static_pool_test.go index 2c5196c94..fd85992b8 100644 --- a/spinifex/network/external/static_pool_test.go +++ b/spinifex/network/external/static_pool_test.go @@ -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) { diff --git a/spinifex/network/policy/nat.go b/spinifex/network/policy/nat.go index c7d2d050d..40e8541a5 100644 --- a/spinifex/network/policy/nat.go +++ b/spinifex/network/policy/nat.go @@ -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 { diff --git a/spinifex/network/policy/nat_test.go b/spinifex/network/policy/nat_test.go index 3630628b3..9720899d5 100644 --- a/spinifex/network/policy/nat_test.go +++ b/spinifex/network/policy/nat_test.go @@ -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) { diff --git a/spinifex/vpcd/vpcd.go b/spinifex/vpcd/vpcd.go index 33f259485..c18d5a376 100644 --- a/spinifex/vpcd/vpcd.go +++ b/spinifex/vpcd/vpcd.go @@ -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 == "" {