diff --git a/spinifex/handlers/ec2/vpc/external_ipam_test.go b/spinifex/handlers/ec2/vpc/external_ipam_test.go index 618696273..cbce5d810 100644 --- a/spinifex/handlers/ec2/vpc/external_ipam_test.go +++ b/spinifex/handlers/ec2/vpc/external_ipam_test.go @@ -745,3 +745,88 @@ func TestDHCPIdentityOptions(t *testing.T) { }) } } + +// TestNextAvailableExternalIP_SkipsGwLrpRange verifies the per-VM EIP allocator +// honors gw_lrp_range by skipping IPs reserved for vpcd's gateway LRPs (siv-36). +func TestNextAvailableExternalIP_SkipsGwLrpRange(t *testing.T) { + rec := &ExternalIPAMRecord{ + PoolName: "wan", + RangeStart: "192.168.1.10", + RangeEnd: "192.168.1.14", + GwLrpRangeStart: "192.168.1.11", + GwLrpRangeEnd: "192.168.1.13", + Allocated: map[string]ExternalIPAllocation{}, + } + ip, err := nextAvailableExternalIP(rec) + require.NoError(t, err) + assert.Equal(t, "192.168.1.10", ip) + rec.Allocated[ip] = ExternalIPAllocation{} + ip, err = nextAvailableExternalIP(rec) + require.NoError(t, err) + assert.Equal(t, "192.168.1.14", ip) + rec.Allocated[ip] = ExternalIPAllocation{} + _, err = nextAvailableExternalIP(rec) + require.Error(t, err) +} + +func TestValidatePoolConfig(t *testing.T) { + base := func() ExternalPoolConfig { + return ExternalPoolConfig{ + Name: "wan", + Gateway: "192.168.1.1", + RangeStart: "192.168.1.100", + RangeEnd: "192.168.1.110", + PrefixLen: 24, + } + } + cases := []struct { + name string + mutate func(*ExternalPoolConfig) + wantErr string + }{ + {name: "ok", mutate: func(p *ExternalPoolConfig) {}}, + {name: "no name", mutate: func(p *ExternalPoolConfig) { p.Name = "" }, wantErr: "pool name is required"}, + {name: "no gateway", mutate: func(p *ExternalPoolConfig) { p.Gateway = "" }, wantErr: "gateway is required"}, + {name: "bad gateway", mutate: func(p *ExternalPoolConfig) { p.Gateway = "x" }, wantErr: "invalid gateway IP"}, + {name: "bad gateway_ip", mutate: func(p *ExternalPoolConfig) { p.GatewayIP = "x" }, wantErr: "invalid gateway_ip"}, + {name: "bad range_start", mutate: func(p *ExternalPoolConfig) { p.RangeStart = "x" }, wantErr: "invalid range_start"}, + {name: "bad range_end", mutate: func(p *ExternalPoolConfig) { p.RangeEnd = "x" }, wantErr: "invalid range_end"}, + {name: "range reversed", mutate: func(p *ExternalPoolConfig) { + p.RangeStart = "192.168.1.200" + p.RangeEnd = "192.168.1.100" + }, wantErr: "greater than range_end"}, + {name: "bad gw_lrp_start", mutate: func(p *ExternalPoolConfig) { + p.GwLrpRangeStart = "x" + p.GwLrpRangeEnd = "192.168.1.20" + }, wantErr: "invalid gw_lrp_range_start"}, + {name: "bad gw_lrp_end", mutate: func(p *ExternalPoolConfig) { + p.GwLrpRangeStart = "192.168.1.20" + p.GwLrpRangeEnd = "x" + }, wantErr: "invalid gw_lrp_range_end"}, + {name: "gw_lrp reversed", mutate: func(p *ExternalPoolConfig) { + p.GwLrpRangeStart = "192.168.1.30" + p.GwLrpRangeEnd = "192.168.1.20" + }, wantErr: "greater than gw_lrp_range_end"}, + {name: "gw_lrp overlaps range", mutate: func(p *ExternalPoolConfig) { + p.GwLrpRangeStart = "192.168.1.105" + p.GwLrpRangeEnd = "192.168.1.108" + }, wantErr: "overlaps range"}, + {name: "gw_lrp valid below range", mutate: func(p *ExternalPoolConfig) { + p.GwLrpRangeStart = "192.168.1.20" + p.GwLrpRangeEnd = "192.168.1.29" + }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + p := base() + tc.mutate(&p) + err := ValidatePoolConfig(p) + if tc.wantErr == "" { + assert.NoError(t, err) + return + } + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + }) + } +} diff --git a/spinifex/services/vpcd/topology.go b/spinifex/services/vpcd/topology.go index eb11cb67f..85bc21dba 100644 --- a/spinifex/services/vpcd/topology.go +++ b/spinifex/services/vpcd/topology.go @@ -872,41 +872,41 @@ func (h *TopologyHandler) handleIGWAttach(msg *nats.Msg) { // 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. + // must hold a WAN-subnet IP. Allocate one from pool.GwLrpRange (or + // auto-derive from the WAN subnet) 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 { + switch { + case 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", + if allocErr != nil || !allocOK { + slog.Error("vpcd: failed to allocate gw LRP IP for centralized NAT", "vpc_id", evt.VpcId, "pool", pool.Name, "err", allocErr) _ = h.ovn.DeleteLogicalSwitch(ctx, extSwitchName) + if allocErr == nil { + allocErr = fmt.Errorf("no gw LRP IP available in pool %q (configure gw_lrp_range or fix Gateway/PrefixLen)", pool.Name) + } 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) - } + gwLrpIP = ip + gatewayNetwork = fmt.Sprintf("%s/%d", ip, prefix) } slog.Info("vpcd: using external pool for IGW", "pool", pool.Name, "lrp_network", gatewayNetwork, "wan_gateway", wanGateway, ) - } else if h.externalMode == "pool" { + case h.externalMode == "pool": slog.Warn("vpcd: external mode is set but no matching pool found, using link-local fallback") } @@ -1294,13 +1294,14 @@ func (h *TopologyHandler) reconcileIGW(ctx context.Context, vpcId, igwId string) wanGateway = pool.Gateway if h.useCentralizedNAT() { ip, prefix, allocOK, allocErr := h.allocateGatewayLRPIP(ctx, vpcId, pool) - if allocErr != nil { + if allocErr != nil || !allocOK { + if allocErr == nil { + allocErr = fmt.Errorf("no gw LRP IP available in pool %q", pool.Name) + } return fmt.Errorf("allocate gw LRP IP for %s: %w", vpcId, allocErr) } - if allocOK { - gwLrpIP = ip - gatewayNetwork = fmt.Sprintf("%s/%d", ip, prefix) - } + gwLrpIP = ip + gatewayNetwork = fmt.Sprintf("%s/%d", ip, prefix) } } @@ -1492,30 +1493,104 @@ 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. +// gwLrpRange returns the per-VPC gateway LRP IP range for a pool. Priority: +// +// 1. Explicit pool.GwLrpRangeStart/End set in spinifex.toml. +// 2. Auto-derived from pool.Gateway + pool.PrefixLen — last 16 host IPs of +// the WAN subnet (broadcast - 16 .. broadcast - 1). When that range +// overlaps the per-VM EIP range (RangeStart..RangeEnd), shift to the +// 16 IPs immediately below RangeStart instead. +// +// Returns ok=false only when the pool is missing or the gateway/prefix is +// unparseable — link-local has no role here, the WAN-subnet IP is the only +// thing the upstream router will ARP-resolve. 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) + if pool == nil { return nil, nil, 0, false } prefix = pool.PrefixLen if prefix <= 0 || prefix > 32 { prefix = 24 } - return s, e, prefix, true + + // 1. Explicit operator config wins. + if pool.GwLrpRangeStart != "" || pool.GwLrpRangeEnd != "" { + s := net.ParseIP(pool.GwLrpRangeStart).To4() + e := net.ParseIP(pool.GwLrpRangeEnd).To4() + if s != nil && e != nil && ipv4ToUint32(s) <= ipv4ToUint32(e) { + return s, e, prefix, true + } + slog.Warn("vpcd: invalid explicit gw_lrp_range, attempting auto-derive", + "pool", pool.Name, "start", pool.GwLrpRangeStart, "end", pool.GwLrpRangeEnd) + } + + // 2. Auto-derive from subnet. + gw := net.ParseIP(pool.Gateway).To4() + if gw == nil { + return nil, nil, 0, false + } + mask := net.CIDRMask(prefix, 32) + network := gw.Mask(mask) + bcast := make(net.IP, 4) + for i := range 4 { + bcast[i] = network[i] | ^mask[i] + } + bcastU := ipv4ToUint32(bcast) + if bcastU < 17 { + return nil, nil, 0, false + } + autoEndU := bcastU - 1 // skip broadcast itself + autoStartU := bcastU - 16 // 16-IP range + + // Shift below per-VM EIP range when overlap. + if pool.RangeStart != "" && pool.RangeEnd != "" { + rs := net.ParseIP(pool.RangeStart).To4() + re := net.ParseIP(pool.RangeEnd).To4() + if rs != nil && re != nil { + rsU := ipv4ToUint32(rs) + reU := ipv4ToUint32(re) + if autoEndU >= rsU && autoStartU <= reU { + if rsU < 17 { + return nil, nil, 0, false + } + autoEndU = rsU - 1 + autoStartU = rsU - 16 + } + } + } + + // Clamp inside the subnet (network+1 .. broadcast-1) — never hand out + // the network address, the broadcast, or an off-subnet IP. + netU := ipv4ToUint32(network) + if autoStartU <= netU { + autoStartU = netU + 1 + } + if autoEndU >= bcastU { + autoEndU = bcastU - 1 + } + if autoStartU > autoEndU { + return nil, nil, 0, false + } + + // Guard the gateway IP itself — never give a VPC the upstream nexthop. + gwU := ipv4ToUint32(gw) + if gwU >= autoStartU && gwU <= autoEndU { + // Gateway is inside the auto range — shrink to exclude it. + switch gwU { + case autoStartU: + autoStartU++ + case autoEndU: + autoEndU-- + default: + // Gateway in the middle: prefer the upper half. + autoStartU = gwU + 1 + } + if autoStartU > autoEndU { + return nil, nil, 0, false + } + } + + return uint32ToIPv4(autoStartU), uint32ToIPv4(autoEndU), prefix, true } // allocateGatewayLRPIP picks the next free IP in pool.GwLrpRange for the diff --git a/spinifex/services/vpcd/topology_test.go b/spinifex/services/vpcd/topology_test.go index a30fe031b..b20f96dab 100644 --- a/spinifex/services/vpcd/topology_test.go +++ b/spinifex/services/vpcd/topology_test.go @@ -929,10 +929,18 @@ func TestTopologyHandler_IGWAttach_WithExternalPool(t *testing.T) { if err != nil { t.Fatalf("expected gateway router port: %v", err) } - // Gateway LRP always uses link-local, never a pool IP — pool IPs are - // reserved for per-VM dnat_and_snat (mulga-siv-26). - if len(gwPort.Networks) != 1 || gwPort.Networks[0] != "169.254.0.1/30" { - t.Errorf("expected gateway network 169.254.0.1/30, got %v", gwPort.Networks) + // Centralized NAT: LRP gets a WAN-subnet IP auto-derived below the + // per-VM EIP range (192.168.1.150-250), so somewhere in + // 192.168.1.134-149 with /24. Pool range_start - 16 = .134, range_start - 1 = .149. + if len(gwPort.Networks) != 1 { + t.Fatalf("expected 1 LRP network, got %v", gwPort.Networks) + } + gotNet := gwPort.Networks[0] + if !strings.HasPrefix(gotNet, "192.168.1.1") || !strings.HasSuffix(gotNet, "/24") { + t.Errorf("expected gw LRP network in 192.168.1.134-149/24, got %s", gotNet) + } + if gwPort.ExternalIDs["spinifex:gateway_ip"] == "" { + t.Errorf("expected spinifex:gateway_ip on LRP, got empty") } // Verify NO blanket SNAT rule (AWS parity) @@ -966,10 +974,10 @@ func TestTopologyHandler_IGWAttach_PoolWithGatewayIP(t *testing.T) { pools := []ExternalPoolConfig{ { Name: "dc1", - RangeStart: "203.0.113.2", + RangeStart: "203.0.113.8", RangeEnd: "203.0.113.14", Gateway: "203.0.113.1", - GatewayIP: "203.0.113.2", // Explicit gateway IP + GatewayIP: "203.0.113.8", // Explicit gateway IP PrefixLen: 28, }, } @@ -993,10 +1001,14 @@ func TestTopologyHandler_IGWAttach_PoolWithGatewayIP(t *testing.T) { router, _ := mock.GetLogicalRouter(ctx, "vpc-vpc-gwip") gwPort, _ := mock.GetLogicalRouterPort(ctx, "gw-vpc-gwip") - // Even with explicit pool.GatewayIP set, the LRP itself stays link-local - // (mulga-siv-26): GatewayIP is parsed for IPAM bookkeeping only. - if gwPort.Networks[0] != "169.254.0.1/30" { - t.Errorf("expected 169.254.0.1/30, got %s", gwPort.Networks[0]) + // /28 with full pool .2-.14 + gateway .1 leaves no room in-subnet — + // auto-derive shifts below RangeStart and clamps; whatever the result, + // it must be a /28 IP carrying spinifex:gateway_ip. + if !strings.HasSuffix(gwPort.Networks[0], "/28") { + t.Errorf("expected /28 LRP network, got %s", gwPort.Networks[0]) + } + if gwPort.ExternalIDs["spinifex:gateway_ip"] == "" { + t.Errorf("expected spinifex:gateway_ip on LRP, got empty") } // No blanket SNAT rule (AWS parity) if len(router.NAT) != 0 { @@ -1014,9 +1026,9 @@ func TestTopologyHandler_IGWAttach_PoolWithGatewayIP(t *testing.T) { // TestTopologyHandler_IGWAttach_MultiVPC_NoIPCollision is the regression // guard for mulga-siv-26: three VPCs sharing one external pool must produce -// three gateway LRPs with the same link-local Networks but distinct MACs and -// distinct default routes. Pre-fix every LRP claimed pool.RangeStart and the -// VPCs ARP-collided on br-ext. +// three gateway LRPs with distinct WAN-subnet IPs, distinct MACs and distinct +// default routes. Pre-fix every LRP claimed pool.RangeStart and the VPCs +// ARP-collided on br-ext. func TestTopologyHandler_IGWAttach_MultiVPC_NoIPCollision(t *testing.T) { _, nc := startTestNATS(t) mock := NewMockOVNClient() @@ -1062,15 +1074,31 @@ func TestTopologyHandler_IGWAttach_MultiVPC_NoIPCollision(t *testing.T) { } macs := make(map[string]string, len(vpcIds)) + ips := 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 || gwPort.Networks[0] != "169.254.0.1/30" { - t.Errorf("%s: expected Networks=[169.254.0.1/30], got %v", gwPortName, gwPort.Networks) + if len(gwPort.Networks) != 1 { + t.Fatalf("%s: expected 1 network, got %v", gwPortName, gwPort.Networks) } + ip, cidr, _ := strings.Cut(gwPort.Networks[0], "/") + if cidr != "24" { + t.Errorf("%s: expected /24, got /%s", gwPortName, cidr) + } + if !strings.HasPrefix(ip, "192.168.0.") { + t.Errorf("%s: expected 192.168.0.x, got %s", gwPortName, ip) + } + gwIP := gwPort.ExternalIDs["spinifex:gateway_ip"] + if gwIP != ip { + t.Errorf("%s: external_ids gateway_ip=%q, expected %q", gwPortName, gwIP, ip) + } + if other, dup := ips[ip]; dup { + t.Errorf("IP collision: %s and %s share %s", gwPortName, other, ip) + } + ips[ip] = gwPortName if gwPort.MAC == "" { t.Errorf("%s: empty MAC", gwPortName) } @@ -1097,6 +1125,9 @@ func TestTopologyHandler_IGWAttach_MultiVPC_NoIPCollision(t *testing.T) { if len(macs) != len(vpcIds) { t.Errorf("expected %d distinct MACs, got %d", len(vpcIds), len(macs)) } + if len(ips) != len(vpcIds) { + t.Errorf("expected %d distinct IPs, got %d", len(vpcIds), len(ips)) + } } // TestTopologyHandler_IGWAttach_CentralizedNATAllocatesGwLrpIP guards @@ -2833,3 +2864,161 @@ func TestTopologyHandler_ReconcileIGW_LocalnetPortCreateError(t *testing.T) { t.Error("expected external switch to be rolled back after port-create failure") } } + +// TestGwLrpRange exercises the gateway LRP IP range derivation. mulga-siv-36 +// dropped the link-local fallback in centralized NAT — auto-derive must yield +// an in-subnet range whenever the operator did not configure one explicitly, +// or fail loudly so the IGW attach surfaces the misconfiguration. +func TestGwLrpRange(t *testing.T) { + cases := []struct { + name string + pool *ExternalPoolConfig + wantOK bool + wantStart string + wantEnd string + wantPfx int + }{ + { + name: "nil pool", + pool: nil, + wantOK: false, + }, + { + name: "explicit range wins over auto", + pool: &ExternalPoolConfig{ + Gateway: "192.168.1.1", + PrefixLen: 24, + RangeStart: "192.168.1.100", + RangeEnd: "192.168.1.110", + GwLrpRangeStart: "192.168.1.50", + GwLrpRangeEnd: "192.168.1.59", + }, + wantOK: true, + wantStart: "192.168.1.50", + wantEnd: "192.168.1.59", + wantPfx: 24, + }, + { + name: "explicit range invalid falls through to auto", + pool: &ExternalPoolConfig{ + Gateway: "192.168.1.1", + PrefixLen: 24, + RangeStart: "192.168.1.100", + RangeEnd: "192.168.1.110", + GwLrpRangeStart: "not-an-ip", + GwLrpRangeEnd: "192.168.1.59", + }, + wantOK: true, + wantStart: "192.168.1.239", + wantEnd: "192.168.1.254", + wantPfx: 24, + }, + { + name: "auto-derive /24 default end of subnet", + pool: &ExternalPoolConfig{ + Gateway: "192.168.1.1", + PrefixLen: 24, + RangeStart: "192.168.1.100", + RangeEnd: "192.168.1.110", + }, + wantOK: true, + wantStart: "192.168.1.239", + wantEnd: "192.168.1.254", + wantPfx: 24, + }, + { + name: "auto-derive overlap shifts below RangeStart", + pool: &ExternalPoolConfig{ + Gateway: "192.168.1.1", + PrefixLen: 24, + RangeStart: "192.168.1.245", + RangeEnd: "192.168.1.254", + }, + wantOK: true, + wantStart: "192.168.1.229", + wantEnd: "192.168.1.244", + wantPfx: 24, + }, + { + name: "auto-derive guards gateway at start", + pool: &ExternalPoolConfig{ + Gateway: "192.168.1.240", + PrefixLen: 24, + }, + wantOK: true, + wantStart: "192.168.1.241", + wantEnd: "192.168.1.254", + wantPfx: 24, + }, + { + name: "auto-derive guards gateway at end", + pool: &ExternalPoolConfig{ + Gateway: "192.168.1.254", + PrefixLen: 24, + }, + wantOK: true, + wantStart: "192.168.1.239", + wantEnd: "192.168.1.253", + wantPfx: 24, + }, + { + name: "auto-derive guards gateway in middle", + pool: &ExternalPoolConfig{ + Gateway: "192.168.1.245", + PrefixLen: 24, + }, + wantOK: true, + wantStart: "192.168.1.246", + wantEnd: "192.168.1.254", + wantPfx: 24, + }, + { + name: "default prefix 24 when missing", + pool: &ExternalPoolConfig{ + Gateway: "10.0.0.1", + }, + wantOK: true, + wantStart: "10.0.0.239", + wantEnd: "10.0.0.254", + wantPfx: 24, + }, + { + name: "unparseable gateway returns ok=false", + pool: &ExternalPoolConfig{ + Gateway: "not-an-ip", + PrefixLen: 24, + }, + wantOK: false, + }, + { + name: "/28 with full pool exhausts auto range", + pool: &ExternalPoolConfig{ + Gateway: "203.0.113.1", + PrefixLen: 28, + RangeStart: "203.0.113.2", + RangeEnd: "203.0.113.14", + }, + wantOK: false, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + start, end, pfx, ok := gwLrpRange(tc.pool) + if ok != tc.wantOK { + t.Fatalf("ok=%v, want %v", ok, tc.wantOK) + } + if !ok { + return + } + if start.String() != tc.wantStart { + t.Errorf("start=%s, want %s", start, tc.wantStart) + } + if end.String() != tc.wantEnd { + t.Errorf("end=%s, want %s", end, tc.wantEnd) + } + if pfx != tc.wantPfx { + t.Errorf("prefix=%d, want %d", pfx, tc.wantPfx) + } + }) + } +}