From 82b0bdfd56172f980b4f22395a81afae5f2c6e77 Mon Sep 17 00:00:00 2001 From: joshsiv-mulga Date: Thu, 30 Apr 2026 14:26:43 +1000 Subject: [PATCH 1/3] revert(vpcd/dhcp): drop AF_PACKET promisc wrapper, restore nclient4.New MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ecfbd4f swapped nclient4.New for openPromiscBridgeConn + NewWithConn to catch unicast OFFER replies that bypass the bridge netdev. On dev hosts where upstream honors the BOOTP BROADCAST flag, the wrapper regressed DORA — public-IP allocation failed where main works. Reverting until the promisc path is validated against both broadcast- and unicast-OFFER servers. --- spinifex/services/vpcd/dhcp/nclient4.go | 55 ++----------------------- 1 file changed, 3 insertions(+), 52 deletions(-) diff --git a/spinifex/services/vpcd/dhcp/nclient4.go b/spinifex/services/vpcd/dhcp/nclient4.go index 8ab50860d..70ca30f92 100644 --- a/spinifex/services/vpcd/dhcp/nclient4.go +++ b/spinifex/services/vpcd/dhcp/nclient4.go @@ -3,46 +3,12 @@ package dhcp import ( "context" "fmt" - "net" "time" "github.com/insomniacslk/dhcp/dhcpv4" "github.com/insomniacslk/dhcp/dhcpv4/nclient4" - "github.com/mdlayher/packet" - "golang.org/x/sys/unix" ) -// dhcpClientPort is the IANA-assigned UDP port for DHCP clients. -const dhcpClientPort = 68 - -// openPromiscBridgeConn opens an AF_PACKET DGRAM socket on iface filtered -// to IPv4 and enables PACKET_MR_PROMISC on it. Equivalent to nclient4's -// own NewRawUDPConn but with promisc on. -// -// Why promisc: vpcd uses a synthetic per-tenant chaddr (e.g. 02:f3:...) -// that never matches the bridge's MAC. Many real-world DHCP servers -// ignore the BOOTP BROADCAST flag and reply with a unicast OFFER addressed -// to that chaddr. A Linux bridge floods unknown unicast to slave ports -// but does NOT pass it up to the bridge netdev unless IFF_PROMISC is set, -// so the AF_PACKET socket misses the OFFER and DORA times out. Promisc -// flips IFF_PROMISC for the lifetime of the socket (per-DORA), and the -// kernel refcounts it so it auto-clears on Close. -func openPromiscBridgeConn(iface string) (net.PacketConn, net.HardwareAddr, error) { - ifc, err := net.InterfaceByName(iface) - if err != nil { - return nil, nil, fmt.Errorf("lookup iface %s: %w", iface, err) - } - raw, err := packet.Listen(ifc, packet.Datagram, unix.ETH_P_IP, nil) - if err != nil { - return nil, nil, fmt.Errorf("packet.Listen on %s: %w", iface, err) - } - if err := raw.SetPromiscuous(true); err != nil { - _ = raw.Close() - return nil, nil, fmt.Errorf("set promisc on %s: %w", iface, err) - } - return nclient4.NewBroadcastUDPConn(raw, &net.UDPAddr{Port: dhcpClientPort}), ifc.HardwareAddr, nil -} - // NClient4Client is the production DHCP client backed by // github.com/insomniacslk/dhcp/dhcpv4/nclient4. Each Acquire/Renew/Release // opens an AF_PACKET socket on the target bridge for the duration of the @@ -73,17 +39,12 @@ func (c *NClient4Client) Acquire(ctx context.Context, req AcquireRequest) (*Leas return nil, fmt.Errorf("dhcp acquire: hw_addr is required") } - conn, ifaceMAC, err := openPromiscBridgeConn(req.Bridge) - if err != nil { - return nil, fmt.Errorf("open promisc conn on %s: %w", req.Bridge, err) - } - client, err := nclient4.NewWithConn(conn, ifaceMAC, + client, err := nclient4.New(req.Bridge, nclient4.WithHWAddr(req.HWAddr), nclient4.WithTimeout(c.timeout), nclient4.WithRetry(c.retry), ) if err != nil { - _ = conn.Close() return nil, fmt.Errorf("open nclient4 on %s: %w", req.Bridge, err) } defer func() { _ = client.Close() }() @@ -104,17 +65,12 @@ func (c *NClient4Client) Renew(ctx context.Context, lease *Lease) (*Lease, error return nil, fmt.Errorf("dhcp renew: %w", err) } - conn, ifaceMAC, err := openPromiscBridgeConn(lease.Bridge) - if err != nil { - return nil, fmt.Errorf("open promisc conn on %s for renew: %w", lease.Bridge, err) - } - client, err := nclient4.NewWithConn(conn, ifaceMAC, + client, err := nclient4.New(lease.Bridge, nclient4.WithHWAddr(lease.HWAddr), nclient4.WithTimeout(c.timeout), nclient4.WithRetry(c.retry), ) if err != nil { - _ = conn.Close() return nil, fmt.Errorf("open nclient4 on %s for renew: %w", lease.Bridge, err) } defer func() { _ = client.Close() }() @@ -143,16 +99,11 @@ func (c *NClient4Client) Release(ctx context.Context, lease *Lease) error { return fmt.Errorf("dhcp release: %w", err) } - conn, ifaceMAC, err := openPromiscBridgeConn(lease.Bridge) - if err != nil { - return fmt.Errorf("open promisc conn on %s for release: %w", lease.Bridge, err) - } - client, err := nclient4.NewWithConn(conn, ifaceMAC, + client, err := nclient4.New(lease.Bridge, nclient4.WithHWAddr(lease.HWAddr), nclient4.WithTimeout(c.timeout), ) if err != nil { - _ = conn.Close() return fmt.Errorf("open nclient4 on %s for release: %w", lease.Bridge, err) } defer func() { _ = client.Close() }() From 87dd7d242a16922f24a6c99182820ac4e9cde5f3 Mon Sep 17 00:00:00 2001 From: joshsiv-mulga Date: Thu, 30 Apr 2026 16:03:54 +1000 Subject: [PATCH 2/3] fix(setup-ovn): mark br-int/br-ext Unmanaged for systemd-networkd (mulga-siv-37) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fresh ISO install on Trixie+systemd-networkd left the OVS internal netdevs admin-down: networkd has no matching .network so it tags them as unmanaged but never brings them up. OVS dataplane works but ovn-controller flow programming and any tooling that probes link state misbehaves. Drop /etc/systemd/network/05-spinifex-ovs-internal.network with Unmanaged=yes so OVS itself owns the link state — `ovs-vsctl add-br` already runs the netdev up at create time. --- scripts/setup-ovn.sh | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/scripts/setup-ovn.sh b/scripts/setup-ovn.sh index a8b6c8247..15d611e66 100755 --- a/scripts/setup-ovn.sh +++ b/scripts/setup-ovn.sh @@ -290,6 +290,26 @@ sudo ovs-vsctl set Bridge br-int other-config:disable-in-band=true sudo ip link set br-int up echo " br-int: created, fail-mode=secure, up" +# Mark OVS internal netdevs Unmanaged for systemd-networkd. Without this, +# Trixie's networkd takes ownership of any unconfigured iface and may bring +# br-int/br-ext admin-down after setup-ovn.sh's `ip link set up` (no Match +# rule => default management => no carrier => link down). OVS dataplane +# still forwards, but ovn-controller flow programming and any tooling that +# probes link state misbehave. Unmanaged=yes keeps OVS in sole control. +# (mulga-siv-37) +OVS_INTERNAL_NET=/etc/systemd/network/05-spinifex-ovs-internal.network +if [ ! -f "$OVS_INTERNAL_NET" ]; then + sudo tee "$OVS_INTERNAL_NET" >/dev/null <<'NETWORK' +[Match] +Name=br-int br-ext + +[Link] +Unmanaged=yes +NETWORK + sudo networkctl reload 2>/dev/null || true + echo " wrote $OVS_INTERNAL_NET (Unmanaged=yes for br-int br-ext)" +fi + # --- Step 3b: Configure WAN bridge for public subnet uplink --- if [ -n "$WAN_BRIDGE" ]; then echo "" From 71ef7236cab78f5c8df516316c165389c11a0ed4 Mon Sep 17 00:00:00 2001 From: joshsiv-mulga Date: Thu, 30 Apr 2026 16:04:10 +1000 Subject: [PATCH 3/3] fix(vpcd): allocate gateway LRP IP from pool in centralized NAT (mulga-siv-36) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In veth/macvlan modes the gateway LRP carries all egress, so it must hold a WAN-subnet IP — otherwise the upstream router silently drops ARP for the WAN nexthop (RFC 826) and the default route never resolves. The hardcoded 169.254.0.1/30 was correct for distributed NAT but regressed centralized NAT (introduced under mulga-siv-26). Add gw_lrp_range_start/end to ExternalPool. In centralized mode at IGW attach the gateway LRP gets a fresh IP from that range, persisted as external_ids:spinifex:gateway_ip on the LRP for idempotent reconcile and retrofit. Direct mode keeps link-local. Per-VM EIP allocator skips the gw range; ValidatePoolConfig rejects overlap with the EIP range. Adds TestTopologyHandler_IGWAttach_CentralizedNATAllocatesGwLrpIP and extends RetrofitAllGatewayPortNetworks to migrate stale link-local LRPs to allocated pool IPs at startup. --- cmd/spinifex/cmd/service.go | 20 +- spinifex/config/config.go | 8 + spinifex/daemon/daemon.go | 22 +- spinifex/handlers/ec2/vpc/external_ipam.go | 100 ++++++-- spinifex/services/vpcd/topology.go | 252 ++++++++++++++++++--- spinifex/services/vpcd/topology_test.go | 91 ++++++++ spinifex/services/vpcd/vpcd.go | 20 +- 7 files changed, 429 insertions(+), 84 deletions(-) diff --git a/cmd/spinifex/cmd/service.go b/cmd/spinifex/cmd/service.go index 462012e6e..cc9ddbfcc 100644 --- a/cmd/spinifex/cmd/service.go +++ b/cmd/spinifex/cmd/service.go @@ -734,15 +734,17 @@ var vpcdStartCmd = &cobra.Command{ var extPools []vpcd.ExternalPoolConfig for _, p := range clusterConfig.Network.ExternalPools { extPools = append(extPools, vpcd.ExternalPoolConfig{ - Name: p.Name, - RangeStart: p.RangeStart, - RangeEnd: p.RangeEnd, - Gateway: p.Gateway, - GatewayIP: p.GatewayIP, - PrefixLen: p.PrefixLen, - DNSServers: p.DNSServers, - Region: p.Region, - AZ: p.AZ, + Name: p.Name, + RangeStart: p.RangeStart, + RangeEnd: p.RangeEnd, + Gateway: p.Gateway, + GatewayIP: p.GatewayIP, + PrefixLen: p.PrefixLen, + DNSServers: p.DNSServers, + Region: p.Region, + AZ: p.AZ, + GwLrpRangeStart: p.GwLrpRangeStart, + GwLrpRangeEnd: p.GwLrpRangeEnd, }) } diff --git a/spinifex/config/config.go b/spinifex/config/config.go index eb9bfd115..01cfdda63 100644 --- a/spinifex/config/config.go +++ b/spinifex/config/config.go @@ -31,6 +31,14 @@ type ExternalPool struct { DNSServers []string `mapstructure:"dns_servers"` // DNS servers for VM DHCP (auto-detected from host; fallback: 8.8.8.8, 1.1.1.1) Region string `mapstructure:"region"` // Scope to region (optional — empty means any region) AZ string `mapstructure:"az"` // Scope to AZ (optional — empty means any AZ in region) + // GwLrpRangeStart/End reserve a sub-range of the LAN for OVN gateway LRP IPs + // in centralized NAT mode (veth/macvlan). Each VPC consumes one IP from this + // range so its gateway router port can ARP with a sender IP on the LAN + // subnet — link-local 169.254.0.1/30 makes upstream routers reject ARP per + // RFC 826 (mulga-siv-36). Must NOT overlap [RangeStart, RangeEnd] or IPAM + // and vpcd will fight over the same IP. + GwLrpRangeStart string `mapstructure:"gw_lrp_range_start"` + GwLrpRangeEnd string `mapstructure:"gw_lrp_range_end"` } // NetworkConfig holds cluster-wide external network settings. diff --git a/spinifex/daemon/daemon.go b/spinifex/daemon/daemon.go index 8c0077748..59b780dda 100644 --- a/spinifex/daemon/daemon.go +++ b/spinifex/daemon/daemon.go @@ -692,16 +692,18 @@ func (d *Daemon) Start() error { } for _, p := range d.clusterConfig.Network.ExternalPools { pools = append(pools, handlers_ec2_vpc.ExternalPoolConfig{ - Name: p.Name, - Source: p.Source, - RangeStart: p.RangeStart, - RangeEnd: p.RangeEnd, - Gateway: p.Gateway, - GatewayIP: p.GatewayIP, - PrefixLen: p.PrefixLen, - Region: p.Region, - AZ: p.AZ, - DhcpBindBridge: dhcpBindBridge, + Name: p.Name, + Source: p.Source, + RangeStart: p.RangeStart, + RangeEnd: p.RangeEnd, + Gateway: p.Gateway, + GatewayIP: p.GatewayIP, + PrefixLen: p.PrefixLen, + Region: p.Region, + AZ: p.AZ, + DhcpBindBridge: dhcpBindBridge, + GwLrpRangeStart: p.GwLrpRangeStart, + GwLrpRangeEnd: p.GwLrpRangeEnd, }) } d.externalIPAM, err = handlers_ec2_vpc.NewExternalIPAM(d.natsConn, js, pools) diff --git a/spinifex/handlers/ec2/vpc/external_ipam.go b/spinifex/handlers/ec2/vpc/external_ipam.go index e96a9d721..c336af7ef 100644 --- a/spinifex/handlers/ec2/vpc/external_ipam.go +++ b/spinifex/handlers/ec2/vpc/external_ipam.go @@ -38,16 +38,21 @@ type ExternalIPAllocation struct { // ExternalIPAMRecord tracks allocated external IPs for a single pool. type ExternalIPAMRecord struct { - PoolName string `json:"pool_name"` - Source string `json:"source"` // "static" (default) or "dhcp" - RangeStart string `json:"range_start"` - RangeEnd string `json:"range_end"` - Gateway string `json:"gateway"` - GatewayIP string `json:"gateway_ip"` - PrefixLen int `json:"prefix_len"` - Region string `json:"region,omitempty"` - AZ string `json:"az,omitempty"` - Allocated map[string]ExternalIPAllocation `json:"allocated"` + PoolName string `json:"pool_name"` + Source string `json:"source"` // "static" (default) or "dhcp" + RangeStart string `json:"range_start"` + RangeEnd string `json:"range_end"` + Gateway string `json:"gateway"` + GatewayIP string `json:"gateway_ip"` + PrefixLen int `json:"prefix_len"` + Region string `json:"region,omitempty"` + AZ string `json:"az,omitempty"` + // GwLrpRangeStart/End mirrors ExternalPoolConfig — IPAM skips this + // sub-range so it doesn't collide with vpcd's gateway LRP IPs in + // centralized NAT (mulga-siv-36). + GwLrpRangeStart string `json:"gw_lrp_range_start,omitempty"` + GwLrpRangeEnd string `json:"gw_lrp_range_end,omitempty"` + Allocated map[string]ExternalIPAllocation `json:"allocated"` } // ExternalPoolConfig is the admin-defined pool from spinifex.toml. @@ -62,6 +67,11 @@ type ExternalPoolConfig struct { Region string AZ string DhcpBindBridge string // Bridge where the DHCP AF_PACKET socket binds (e.g. "br-wan"). Linux bridge in veth mode; OVS bridge in direct mode. Never "br-ext". + // GwLrpRangeStart/End reserves a sub-range of the LAN for OVN gateway + // LRP IPs in centralized NAT mode (mulga-siv-36). IPAM must skip these + // addresses or the per-VM EIP allocator and vpcd will fight over them. + GwLrpRangeStart string + GwLrpRangeEnd string } // IsDHCP returns true if this pool obtains IPs from router DHCP. @@ -122,15 +132,20 @@ func (m *ExternalIPAM) initPool(pool ExternalPoolConfig) error { } if err == nil { - if chk.RangeStart != pool.RangeStart || chk.RangeEnd != pool.RangeEnd || chk.Source != pool.Source { + if chk.RangeStart != pool.RangeStart || chk.RangeEnd != pool.RangeEnd || chk.Source != pool.Source || + chk.GwLrpRangeStart != pool.GwLrpRangeStart || chk.GwLrpRangeEnd != pool.GwLrpRangeEnd { slog.Info("external IPAM pool config drift, reconciling KV", "pool", pool.Name, "old_range", chk.RangeStart+"-"+chk.RangeEnd, "new_range", pool.RangeStart+"-"+pool.RangeEnd, - "old_source", chk.Source, "new_source", pool.Source) + "old_source", chk.Source, "new_source", pool.Source, + "old_gw_lrp_range", chk.GwLrpRangeStart+"-"+chk.GwLrpRangeEnd, + "new_gw_lrp_range", pool.GwLrpRangeStart+"-"+pool.GwLrpRangeEnd) chk.RangeStart = pool.RangeStart chk.RangeEnd = pool.RangeEnd chk.Source = pool.Source + chk.GwLrpRangeStart = pool.GwLrpRangeStart + chk.GwLrpRangeEnd = pool.GwLrpRangeEnd data, err := json.Marshal(chk) if err != nil { @@ -182,14 +197,16 @@ func (m *ExternalIPAM) initPool(pool ExternalPoolConfig) error { } record := &ExternalIPAMRecord{ - PoolName: pool.Name, - RangeStart: pool.RangeStart, - RangeEnd: pool.RangeEnd, - Gateway: pool.Gateway, - GatewayIP: gwIP, - PrefixLen: pool.PrefixLen, - Region: pool.Region, - AZ: pool.AZ, + PoolName: pool.Name, + RangeStart: pool.RangeStart, + RangeEnd: pool.RangeEnd, + Gateway: pool.Gateway, + GatewayIP: gwIP, + PrefixLen: pool.PrefixLen, + Region: pool.Region, + AZ: pool.AZ, + GwLrpRangeStart: pool.GwLrpRangeStart, + GwLrpRangeEnd: pool.GwLrpRangeEnd, Allocated: map[string]ExternalIPAllocation{ gwIP: gatewayAlloc, }, @@ -453,7 +470,9 @@ func (m *ExternalIPAM) getRecord(poolName string) (*ExternalIPAMRecord, uint64, return &record, entry.Revision(), nil } -// nextAvailableExternalIP finds the next unallocated IP in the pool's range. +// nextAvailableExternalIP finds the next unallocated IP in the pool's +// range. Addresses inside [GwLrpRangeStart, GwLrpRangeEnd] are skipped — +// vpcd reserves them for OVN gateway LRPs (mulga-siv-36). func nextAvailableExternalIP(record *ExternalIPAMRecord) (string, error) { startIP := net.ParseIP(record.RangeStart).To4() endIP := net.ParseIP(record.RangeEnd).To4() @@ -461,10 +480,23 @@ func nextAvailableExternalIP(record *ExternalIPAMRecord) (string, error) { return "", fmt.Errorf("invalid IP range: %s - %s", record.RangeStart, record.RangeEnd) } + var gwLrpStart, gwLrpEnd int64 = -1, -1 + if record.GwLrpRangeStart != "" && record.GwLrpRangeEnd != "" { + s := net.ParseIP(record.GwLrpRangeStart).To4() + e := net.ParseIP(record.GwLrpRangeEnd).To4() + if s != nil && e != nil { + gwLrpStart = ipToInt(s).Int64() + gwLrpEnd = ipToInt(e).Int64() + } + } + startInt := ipToInt(startIP) endInt := ipToInt(endIP) for i := startInt.Int64(); i <= endInt.Int64(); i++ { + if gwLrpStart >= 0 && i >= gwLrpStart && i <= gwLrpEnd { + continue + } candidate := intToIP(intFromInt64(i)).String() if _, taken := record.Allocated[candidate]; !taken { return candidate, nil @@ -501,6 +533,32 @@ func ValidatePoolConfig(pool ExternalPoolConfig) error { if ipToInt(startIP.To4()).Cmp(ipToInt(endIP.To4())) > 0 { return fmt.Errorf("range_start %s is greater than range_end %s", pool.RangeStart, pool.RangeEnd) } + // gw_lrp_range must be valid IPs and must NOT overlap range_start/end + // — otherwise vpcd's gateway LRP allocator and per-VM EIP allocator + // would fight over the same address (mulga-siv-36). + if pool.GwLrpRangeStart != "" || pool.GwLrpRangeEnd != "" { + gwS := net.ParseIP(pool.GwLrpRangeStart) + gwE := net.ParseIP(pool.GwLrpRangeEnd) + if gwS == nil { + return fmt.Errorf("invalid gw_lrp_range_start: %q", pool.GwLrpRangeStart) + } + if gwE == nil { + return fmt.Errorf("invalid gw_lrp_range_end: %q", pool.GwLrpRangeEnd) + } + gwSi := ipToInt(gwS.To4()) + gwEi := ipToInt(gwE.To4()) + if gwSi.Cmp(gwEi) > 0 { + return fmt.Errorf("gw_lrp_range_start %s is greater than gw_lrp_range_end %s", + pool.GwLrpRangeStart, pool.GwLrpRangeEnd) + } + rangeSi := ipToInt(startIP.To4()) + rangeEi := ipToInt(endIP.To4()) + // Overlap test: !(gwE < rangeS || gwS > rangeE) + if gwEi.Cmp(rangeSi) >= 0 && gwSi.Cmp(rangeEi) <= 0 { + return fmt.Errorf("gw_lrp_range %s-%s overlaps range %s-%s", + pool.GwLrpRangeStart, pool.GwLrpRangeEnd, pool.RangeStart, pool.RangeEnd) + } + } } return nil } diff --git a/spinifex/services/vpcd/topology.go b/spinifex/services/vpcd/topology.go index f0a98fb26..eb11cb67f 100644 --- a/spinifex/services/vpcd/topology.go +++ b/spinifex/services/vpcd/topology.go @@ -32,13 +32,23 @@ const ( TopicDeleteNATGateway = "vpc.delete-nat-gateway" ) -// gatewayPortNetwork is the link-local CIDR every IGW gateway-LRP carries. -// Pool IPs are reserved for per-VM dnat_and_snat external addresses; reusing -// them on the LRP causes intra-cluster ARP collisions when multiple VPCs -// share a br-ext localnet (mulga-siv-26). The default route's OutputPort is -// set explicitly so the WAN nexthop need not be on this subnet. +// gatewayPortNetwork is the link-local CIDR every IGW gateway-LRP carries +// in distributed-NAT (direct) mode. Per-VM dnat_and_snat handles ARP per +// chassis so the LRP IP itself never goes on the wire. The default route's +// OutputPort is set explicitly so the WAN nexthop need not be on this subnet. +// +// Centralized-NAT (veth/macvlan) bypasses this constant: the gateway LRP is +// the on-wire egress point and must hold a WAN-subnet IP from the pool's +// gw_lrp_range, otherwise the upstream router silently drops ARP requests +// from an off-subnet sender (RFC 826) and the default route never resolves +// (mulga-siv-36). const gatewayPortNetwork = "169.254.0.1/30" +// gatewayIPExtID is the LRP external_ids key holding the gw IP allocated +// from the pool's gw_lrp_range. Persisted so reconcile/retrofit can recover +// the assignment without re-allocating, and so siblings see it as "used". +const gatewayIPExtID = "spinifex:gateway_ip" + // VPCEvent is published on vpc.create after a VPC is persisted. type VPCEvent struct { VpcId string `json:"vpc_id"` @@ -202,22 +212,73 @@ func (h *TopologyHandler) ensureLocalnetOptions(ctx context.Context, extPortName return nil } +// expectedGatewayPortNetwork returns the Networks CIDR a gateway LRP for +// vpcId should carry. In direct mode, or in centralized mode without a +// configured gw_lrp_range, it is the link-local fallback. In centralized +// mode with a range, it is the existing spinifex:gateway_ip on the LRP if +// any (idempotent), otherwise a fresh allocation. Returns the new gw IP +// (empty when link-local) so callers can persist it to external_ids. +func (h *TopologyHandler) expectedGatewayPortNetwork(ctx context.Context, vpcId string) (network, gwIP string, err error) { + if !h.useCentralizedNAT() { + return gatewayPortNetwork, "", nil + } + pool := h.findExternalPool("", "") + if pool == nil { + return gatewayPortNetwork, "", nil + } + ip, prefix, ok, allocErr := h.allocateGatewayLRPIP(ctx, vpcId, pool) + if allocErr != nil { + return "", "", allocErr + } + if !ok { + return gatewayPortNetwork, "", nil + } + return fmt.Sprintf("%s/%d", ip, prefix), ip, nil +} + // ensureGatewayPortNetworks rewrites the gateway LRP's Networks column in -// place when it drifts from the link-local CIDR. CreateLogicalRouterPort is -// a no-op when the row exists, so reconcile of an upgraded cluster otherwise -// keeps the old pool-IP networks (mulga-siv-26 D8). Idempotent — no UPDATE -// is issued when already correct. +// place when it drifts from the mode's expected value. CreateLogicalRouterPort +// is a no-op when the row exists, so reconcile of an upgraded cluster +// otherwise keeps the stale pool-IP networks (mulga-siv-26 D8) or, in +// centralized NAT, the off-subnet link-local that breaks ARP upstream +// (mulga-siv-36). Idempotent — no UPDATE is issued when already correct. func (h *TopologyHandler) ensureGatewayPortNetworks(ctx context.Context, gwPortName string) error { lrp, err := h.ovn.GetLogicalRouterPort(ctx, gwPortName) if err != nil { return fmt.Errorf("get gateway router port %s: %w", gwPortName, err) } - if len(lrp.Networks) == 1 && lrp.Networks[0] == gatewayPortNetwork { + vpcId := lrp.ExternalIDs["spinifex:vpc_id"] + if vpcId == "" { + // No VPC tag — this is from the legacy "gw-vpc-..." naming or a + // hand-edited row; fall back to link-local to keep behavior safe. + if len(lrp.Networks) == 1 && lrp.Networks[0] == gatewayPortNetwork { + return nil + } + slog.Info("vpcd: rewriting stale gateway port networks (no vpc_id tag)", + "port", gwPortName, "old", lrp.Networks, "new", gatewayPortNetwork) + lrp.Networks = []string{gatewayPortNetwork} + return h.ovn.UpdateLogicalRouterPort(ctx, lrp) + } + wantNetwork, wantGwIP, err := h.expectedGatewayPortNetwork(ctx, vpcId) + if err != nil { + return fmt.Errorf("compute expected network for %s: %w", gwPortName, err) + } + currentGwIP := lrp.ExternalIDs[gatewayIPExtID] + if len(lrp.Networks) == 1 && lrp.Networks[0] == wantNetwork && currentGwIP == wantGwIP { return nil } slog.Info("vpcd: rewriting stale gateway port networks", - "port", gwPortName, "old", lrp.Networks, "new", gatewayPortNetwork) - lrp.Networks = []string{gatewayPortNetwork} + "port", gwPortName, "old", lrp.Networks, "new", wantNetwork, + "old_gw_ip", currentGwIP, "new_gw_ip", wantGwIP) + lrp.Networks = []string{wantNetwork} + if lrp.ExternalIDs == nil { + lrp.ExternalIDs = map[string]string{} + } + if wantGwIP == "" { + delete(lrp.ExternalIDs, gatewayIPExtID) + } else { + lrp.ExternalIDs[gatewayIPExtID] = wantGwIP + } if err := h.ovn.UpdateLogicalRouterPort(ctx, lrp); err != nil { return fmt.Errorf("update gateway router port %s: %w", gwPortName, err) } @@ -225,10 +286,10 @@ func (h *TopologyHandler) ensureGatewayPortNetworks(ctx context.Context, gwPortN } // RetrofitAllGatewayPortNetworks walks every LRP tagged -// spinifex:role=gateway and ensures Networks is the link-local CIDR. Needed +// spinifex:role=gateway and ensures Networks matches the current mode. Needed // because Reconcile/ReconcileFromKV early-return when the external switch // already exists, so reconcileIGW never runs against pre-existing topologies -// shipped by the buggy pool-IP build (mulga-siv-26 D8). Idempotent — the +// shipped by older builds (mulga-siv-26 D8 / mulga-siv-36). Idempotent — the // underlying ensureGatewayPortNetworks no-ops when already correct. func (h *TopologyHandler) RetrofitAllGatewayPortNetworks(ctx context.Context) { lrps, err := h.ovn.ListLogicalRouterPorts(ctx) @@ -804,17 +865,42 @@ func (h *TopologyHandler) handleIGWAttach(msg *nats.Msg) { return } - // Resolve external pool for this VPC's WAN nexthop. The router port - // itself always takes link-local 169.254.0.1/30 — pool IPs are reserved - // for per-VM dnat_and_snat external addresses (mulga-siv-26). Sharing a - // pool IP across VPCs causes intra-cluster ARP collisions on br-ext. + // Resolve external pool for this VPC's WAN nexthop and gw LRP IP. + // + // Direct mode: link-local LRP is fine — distributed NAT puts a per-VM + // dnat_and_snat row with external_mac/logical_port on every chassis, so + // the LRP IP itself never goes on the wire. + // + // Centralized mode (veth/macvlan): the LRP is the on-wire egress, so it + // must hold a WAN-subnet IP. Allocate one from pool.GwLrpRange so each + // VPC gets a distinct sender IP (mulga-siv-36). Without this the + // upstream router silently drops ARP for the WAN nexthop (RFC 826) and + // the default route never resolves. // TODO: use VPC's region/AZ once we track it; for now use first matching pool. pool := h.findExternalPool("", "") gatewayNetwork := gatewayPortNetwork wanGateway := "169.254.0.2" + gwLrpIP := "" if pool != nil { wanGateway = pool.Gateway + if h.useCentralizedNAT() { + ip, prefix, allocOK, allocErr := h.allocateGatewayLRPIP(ctx, evt.VpcId, pool) + if allocErr != nil { + slog.Error("vpcd: failed to allocate gw LRP IP", + "vpc_id", evt.VpcId, "pool", pool.Name, "err", allocErr) + _ = h.ovn.DeleteLogicalSwitch(ctx, extSwitchName) + respond(msg, allocErr) + return + } + if allocOK { + gwLrpIP = ip + gatewayNetwork = fmt.Sprintf("%s/%d", ip, prefix) + } else { + slog.Warn("vpcd: centralized NAT requires pool.gw_lrp_range; LRP will use link-local and external traffic will not resolve ARP upstream", + "vpc_id", evt.VpcId, "pool", pool.Name) + } + } slog.Info("vpcd: using external pool for IGW", "pool", pool.Name, "lrp_network", gatewayNetwork, @@ -826,15 +912,19 @@ func (h *TopologyHandler) handleIGWAttach(msg *nats.Msg) { // 3. Create gateway router port on the VPC router connecting to external switch gwMAC := generateMAC("gw-" + evt.VpcId) + lrpExtIDs := map[string]string{ + "spinifex:vpc_id": evt.VpcId, + "spinifex:igw_id": evt.InternetGatewayId, + "spinifex:role": "gateway", + } + if gwLrpIP != "" { + lrpExtIDs[gatewayIPExtID] = gwLrpIP + } lrp := &nbdb.LogicalRouterPort{ - Name: gwPortName, - MAC: gwMAC, - Networks: []string{gatewayNetwork}, - ExternalIDs: map[string]string{ - "spinifex:vpc_id": evt.VpcId, - "spinifex:igw_id": evt.InternetGatewayId, - "spinifex:role": "gateway", - }, + Name: gwPortName, + MAC: gwMAC, + Networks: []string{gatewayNetwork}, + ExternalIDs: lrpExtIDs, } if err := h.ovn.CreateLogicalRouterPort(ctx, routerName, lrp); err != nil { slog.Error("vpcd: failed to create gateway router port", "port", gwPortName, "err", err) @@ -1194,13 +1284,24 @@ func (h *TopologyHandler) reconcileIGW(ctx context.Context, vpcId, igwId string) gwPortName := "gw-" + vpcId switchGWPortName := "gw-port-" + vpcId - // Router port always link-local; pool IPs reserved for per-VM - // dnat_and_snat external addresses (mulga-siv-26). + // Direct mode: link-local LRP. Centralized mode: allocate from + // pool.GwLrpRange so the LRP can ARP on the WAN subnet (mulga-siv-36). pool := h.findExternalPool("", "") gatewayNetwork := gatewayPortNetwork wanGateway := "169.254.0.2" + gwLrpIP := "" if pool != nil { wanGateway = pool.Gateway + if h.useCentralizedNAT() { + ip, prefix, allocOK, allocErr := h.allocateGatewayLRPIP(ctx, vpcId, pool) + if allocErr != nil { + return fmt.Errorf("allocate gw LRP IP for %s: %w", vpcId, allocErr) + } + if allocOK { + gwLrpIP = ip + gatewayNetwork = fmt.Sprintf("%s/%d", ip, prefix) + } + } } // Build external IDs with optional IGW ID @@ -1252,14 +1353,18 @@ func (h *TopologyHandler) reconcileIGW(ctx context.Context, vpcId, igwId string) // 3. Create gateway router port gwMAC := generateMAC("gw-" + vpcId) + reconcileLrpExtIDs := map[string]string{ + "spinifex:vpc_id": vpcId, + "spinifex:role": "gateway", + } + if gwLrpIP != "" { + reconcileLrpExtIDs[gatewayIPExtID] = gwLrpIP + } lrp := &nbdb.LogicalRouterPort{ - Name: gwPortName, - MAC: gwMAC, - Networks: []string{gatewayNetwork}, - ExternalIDs: map[string]string{ - "spinifex:vpc_id": vpcId, - "spinifex:role": "gateway", - }, + Name: gwPortName, + MAC: gwMAC, + Networks: []string{gatewayNetwork}, + ExternalIDs: reconcileLrpExtIDs, } if err := h.ovn.CreateLogicalRouterPort(ctx, routerName, lrp); err != nil { _ = h.ovn.DeleteLogicalSwitch(ctx, extSwitchName) @@ -1387,6 +1492,83 @@ func (h *TopologyHandler) reconcileGatewayChassis(ctx context.Context, validName // --- Helpers --- +// gwLrpRange returns the gw LRP IP range configured on the pool. +// Returns ok=false when GwLrpRangeStart/End are not configured — caller +// must fall back to link-local in that case. +func gwLrpRange(pool *ExternalPoolConfig) (start, end net.IP, prefix int, ok bool) { + if pool == nil || pool.GwLrpRangeStart == "" || pool.GwLrpRangeEnd == "" { + return nil, nil, 0, false + } + s := net.ParseIP(pool.GwLrpRangeStart).To4() + e := net.ParseIP(pool.GwLrpRangeEnd).To4() + if s == nil || e == nil { + slog.Warn("vpcd: invalid gw_lrp_range, falling back to link-local", + "pool", pool.Name, "start", pool.GwLrpRangeStart, "end", pool.GwLrpRangeEnd) + return nil, nil, 0, false + } + if ipv4ToUint32(s) > ipv4ToUint32(e) { + slog.Warn("vpcd: gw_lrp_range start > end, falling back to link-local", + "pool", pool.Name, "start", pool.GwLrpRangeStart, "end", pool.GwLrpRangeEnd) + return nil, nil, 0, false + } + prefix = pool.PrefixLen + if prefix <= 0 || prefix > 32 { + prefix = 24 + } + return s, e, prefix, true +} + +// allocateGatewayLRPIP picks the next free IP in pool.GwLrpRange for the +// given VPC, scanning every existing LRP's spinifex:gateway_ip external_id +// to compute the used set. If gw- already has an allocation the +// existing IP is returned (idempotent reconcile). Returns ok=false when +// the pool has no gw_lrp_range — caller falls back to link-local. +func (h *TopologyHandler) allocateGatewayLRPIP(ctx context.Context, vpcId string, pool *ExternalPoolConfig) (ip string, prefix int, ok bool, err error) { + start, end, prefix, ok := gwLrpRange(pool) + if !ok { + return "", 0, false, nil + } + + gwPortName := "gw-" + vpcId + lrps, err := h.ovn.ListLogicalRouterPorts(ctx) + if err != nil { + return "", 0, false, fmt.Errorf("list LRPs for gw IP allocation: %w", err) + } + used := make(map[uint32]struct{}, len(lrps)) + for _, lrp := range lrps { + existing := lrp.ExternalIDs[gatewayIPExtID] + if existing == "" { + continue + } + if lrp.Name == gwPortName { + // Idempotent: already allocated to this VPC. + return existing, prefix, true, nil + } + if v := net.ParseIP(existing).To4(); v != nil { + used[ipv4ToUint32(v)] = struct{}{} + } + } + + startU := ipv4ToUint32(start) + endU := ipv4ToUint32(end) + for n := startU; n <= endU; n++ { + if _, taken := used[n]; taken { + continue + } + return uint32ToIPv4(n).String(), prefix, true, nil + } + return "", 0, false, fmt.Errorf("gw_lrp_range exhausted for pool %q (%s-%s)", pool.Name, pool.GwLrpRangeStart, pool.GwLrpRangeEnd) +} + +func ipv4ToUint32(ip net.IP) uint32 { + v := ip.To4() + return uint32(v[0])<<24 | uint32(v[1])<<16 | uint32(v[2])<<8 | uint32(v[3]) +} + +func uint32ToIPv4(n uint32) net.IP { + return net.IPv4(byte(n>>24&0xff), byte(n>>16&0xff), byte(n>>8&0xff), byte(n&0xff)).To4() +} + // subnetGateway computes the gateway IP (.1) from a CIDR string. // Returns the gateway IP string and the prefix length. func subnetGateway(cidr string) (string, int, error) { diff --git a/spinifex/services/vpcd/topology_test.go b/spinifex/services/vpcd/topology_test.go index 927ff8f50..a30fe031b 100644 --- a/spinifex/services/vpcd/topology_test.go +++ b/spinifex/services/vpcd/topology_test.go @@ -1099,6 +1099,97 @@ func TestTopologyHandler_IGWAttach_MultiVPC_NoIPCollision(t *testing.T) { } } +// TestTopologyHandler_IGWAttach_CentralizedNATAllocatesGwLrpIP guards +// mulga-siv-36: in centralized NAT (veth/macvlan) the gateway LRP must +// hold a WAN-subnet IP from pool.GwLrpRange so it can ARP the upstream +// nexthop. Three VPCs sharing one pool must each get a distinct IP and +// persist it on external_ids:spinifex:gateway_ip. +func TestTopologyHandler_IGWAttach_CentralizedNATAllocatesGwLrpIP(t *testing.T) { + _, nc := startTestNATS(t) + mock := NewMockOVNClient() + _ = mock.Connect(context.Background()) + ctx := context.Background() + + pools := []ExternalPoolConfig{ + { + Name: "lan", + RangeStart: "192.168.3.100", + RangeEnd: "192.168.3.150", + Gateway: "192.168.3.1", + PrefixLen: 24, + GwLrpRangeStart: "192.168.3.20", + GwLrpRangeEnd: "192.168.3.29", + }, + } + // Default bridge mode is centralized (useCentralizedNAT returns true + // when bridgeMode != "direct"). Explicit veth here to be unambiguous. + topo := NewTopologyHandler(mock, + WithExternalNetwork("pool", pools), + WithBridgeMode(BridgeModeVeth), + ) + subs, err := topo.Subscribe(nc) + if err != nil { + t.Fatalf("subscribe: %v", err) + } + defer func() { + for _, s := range subs { + _ = s.Unsubscribe() + } + }() + + vpcIds := []string{"vpc-cn-a", "vpc-cn-b", "vpc-cn-c"} + for _, vpcId := range vpcIds { + _ = mock.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{ + Name: "vpc-" + vpcId, + ExternalIDs: map[string]string{ + "spinifex:vpc_id": vpcId, + "spinifex:cidr": "10.0.0.0/16", + }, + }) + evt := types.IGWEvent{InternetGatewayId: "igw-" + vpcId, VpcId: vpcId} + data, _ := json.Marshal(evt) + resp, err := nc.Request(TopicIGWAttach, data, 5_000_000_000) + if err != nil { + t.Fatalf("request vpc.igw-attach %s: %v", vpcId, err) + } + assertSuccess(t, resp, "attach IGW "+vpcId) + } + + seen := make(map[string]string, len(vpcIds)) + for _, vpcId := range vpcIds { + gwPortName := "gw-" + vpcId + gwPort, err := mock.GetLogicalRouterPort(ctx, gwPortName) + if err != nil { + t.Fatalf("get %s: %v", gwPortName, err) + } + if len(gwPort.Networks) != 1 { + t.Fatalf("%s: expected 1 Network, got %v", gwPortName, gwPort.Networks) + } + net := gwPort.Networks[0] + if !strings.HasPrefix(net, "192.168.3.") || !strings.HasSuffix(net, "/24") { + t.Errorf("%s: expected 192.168.3.X/24 from gw_lrp_range, got %s", gwPortName, net) + } + gwIP := gwPort.ExternalIDs["spinifex:gateway_ip"] + if gwIP == "" { + t.Errorf("%s: missing spinifex:gateway_ip external_id", gwPortName) + } + if other, dup := seen[gwIP]; dup { + t.Errorf("gw IP collision: %s and %s share %s", gwPortName, other, gwIP) + } + seen[gwIP] = gwPortName + + // Idempotent: re-attach must not change the assignment. + evt := types.IGWEvent{InternetGatewayId: "igw-" + vpcId, VpcId: vpcId} + data, _ := json.Marshal(evt) + _, _ = nc.Request(TopicIGWAttach, data, 5_000_000_000) + gwPort2, _ := mock.GetLogicalRouterPort(ctx, gwPortName) + if gwPort2.ExternalIDs["spinifex:gateway_ip"] != gwIP { + t.Errorf("%s: re-attach changed gateway_ip from %s to %s", + gwPortName, gwIP, gwPort2.ExternalIDs["spinifex:gateway_ip"]) + } + } +} + func TestTopologyHandler_FindExternalPool(t *testing.T) { pools := []ExternalPoolConfig{ {Name: "az-a", Region: "us-east-1", AZ: "us-east-1a", RangeStart: "1.1.1.1"}, diff --git a/spinifex/services/vpcd/vpcd.go b/spinifex/services/vpcd/vpcd.go index dd6ad5806..dcdf11b92 100644 --- a/spinifex/services/vpcd/vpcd.go +++ b/spinifex/services/vpcd/vpcd.go @@ -80,15 +80,17 @@ type Config struct { // ExternalPoolConfig mirrors config.ExternalPool for vpcd's internal use. type ExternalPoolConfig struct { - Name string - RangeStart string - RangeEnd string - Gateway string - GatewayIP string - PrefixLen int - DNSServers []string - Region string - AZ string + Name string + RangeStart string + RangeEnd string + Gateway string + GatewayIP string + PrefixLen int + DNSServers []string + Region string + AZ string + GwLrpRangeStart string // Sub-range for OVN gateway LRP IPs in centralized NAT mode (mulga-siv-36). + GwLrpRangeEnd string } // Service implements the Spinifex service interface for vpcd.