diff --git a/cmd/dhcptest/main.go b/cmd/dhcptest/main.go index b774a18ce..6ac423700 100644 --- a/cmd/dhcptest/main.go +++ b/cmd/dhcptest/main.go @@ -89,7 +89,7 @@ func main() { } } - // Build nclient4 options — identical to spinifex/services/vpcd/dhcp/nclient4.go. + // Build nclient4 options — standalone DHCP probe for upstream-router debugging. opts := []nclient4.ClientOpt{ nclient4.WithHWAddr(hwAddr), nclient4.WithTimeout(*timeout), @@ -105,7 +105,7 @@ func main() { } defer client.Close() - // Identity modifiers — identical to identityModifiers() in vpcd dhcp package. + // Identity modifiers — client-id / hostname / vendor-class for upstream lease tagging. var mods []dhcpv4.Modifier if cid != "" { mods = append(mods, dhcpv4.WithOption(dhcpv4.OptClientIdentifier([]byte(cid)))) diff --git a/cmd/spinifex/cmd/service.go b/cmd/spinifex/cmd/service.go index b07f44bdc..ad5adf37e 100644 --- a/cmd/spinifex/cmd/service.go +++ b/cmd/spinifex/cmd/service.go @@ -728,7 +728,6 @@ var vpcdStartCmd = &cobra.Command{ for _, p := range clusterConfig.Network.ExternalPools { extPools = append(extPools, vpcd.ExternalPoolConfig{ Name: p.Name, - Source: p.Source, RangeStart: p.RangeStart, RangeEnd: p.RangeEnd, Gateway: p.Gateway, diff --git a/spinifex/daemon/daemon.go b/spinifex/daemon/daemon.go index 546eb4a61..87145e598 100644 --- a/spinifex/daemon/daemon.go +++ b/spinifex/daemon/daemon.go @@ -1097,14 +1097,9 @@ func (d *Daemon) startCluster() error { slog.Warn("Failed to get JetStream for external IPAM", "err", jsErr) } else { var pools []handlers_ec2_vpc.ExternalPoolConfig - gwMAC := "" - if d.clusterConfig.Bootstrap.VpcId != "" { - gwMAC = utils.HashMAC("gw-" + d.clusterConfig.Bootstrap.VpcId) - } 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, @@ -1112,12 +1107,11 @@ func (d *Daemon) startCluster() error { PrefixLen: p.PrefixLen, Region: p.Region, AZ: p.AZ, - GatewayMAC: gwMAC, GwLrpRangeStart: p.GwLrpRangeStart, GwLrpRangeEnd: p.GwLrpRangeEnd, }) } - d.externalIPAM, err = handlers_ec2_vpc.NewExternalIPAM(d.natsConn, js, pools) + d.externalIPAM, err = handlers_ec2_vpc.NewExternalIPAM(js, pools) if err != nil { slog.Warn("Failed to initialize external IPAM", "err", err) } else { diff --git a/spinifex/daemon/daemon_handlers_test.go b/spinifex/daemon/daemon_handlers_test.go index 0a5c4b9ac..4554e0fd1 100644 --- a/spinifex/daemon/daemon_handlers_test.go +++ b/spinifex/daemon/daemon_handlers_test.go @@ -3548,7 +3548,7 @@ func TestDelegateHandlers_EIP(t *testing.T) { js, err := nc.JetStream() require.NoError(t, err) - ipam, err := handlers_ec2_vpc.NewExternalIPAM(nc, js, []handlers_ec2_vpc.ExternalPoolConfig{ + ipam, err := handlers_ec2_vpc.NewExternalIPAM(js, []handlers_ec2_vpc.ExternalPoolConfig{ {Name: "test-pool", RangeStart: "192.168.100.2", RangeEnd: "192.168.100.254", Gateway: "192.168.100.1", PrefixLen: 24}, }) require.NoError(t, err) diff --git a/spinifex/handlers/ec2/eip/service_impl_test.go b/spinifex/handlers/ec2/eip/service_impl_test.go index 48e687977..5372d1a9e 100644 --- a/spinifex/handlers/ec2/eip/service_impl_test.go +++ b/spinifex/handlers/ec2/eip/service_impl_test.go @@ -30,7 +30,7 @@ func setupTestEIP(t *testing.T) (*EIPServiceImpl, *handlers_ec2_vpc.ExternalIPAM _, nc, js := testutil.StartTestJetStream(t) pool := testPool() - ipam, err := handlers_ec2_vpc.NewExternalIPAM(nc, js, []handlers_ec2_vpc.ExternalPoolConfig{pool}) + ipam, err := handlers_ec2_vpc.NewExternalIPAM(js, []handlers_ec2_vpc.ExternalPoolConfig{pool}) require.NoError(t, err) svc, err := NewEIPServiceImpl(nc, ipam, nil) diff --git a/spinifex/handlers/ec2/vpc/external_ipam.go b/spinifex/handlers/ec2/vpc/external_ipam.go index e8f9d19ef..96cb51094 100644 --- a/spinifex/handlers/ec2/vpc/external_ipam.go +++ b/spinifex/handlers/ec2/vpc/external_ipam.go @@ -9,7 +9,6 @@ import ( "net" "github.com/mulgadc/spinifex/spinifex/migrate" - "github.com/mulgadc/spinifex/spinifex/services/vpcd/dhcp" "github.com/mulgadc/spinifex/spinifex/utils" "github.com/nats-io/nats.go" ) @@ -27,20 +26,11 @@ type ExternalIPAllocation struct { ENIId string `json:"eni_id,omitempty"` // ENI owning this IP InstanceId string `json:"instance_id,omitempty"` // Instance owning this IP Note string `json:"note,omitempty"` // Human-readable note - - // DHCP-sourced metadata — set only for allocations whose pool has - // source="dhcp" (see dhcp.RequestAcquire). These fields let describe - // surfaces and operators trace a given IP back to the upstream lease - // without re-querying vpcd. - LeaseExpiresUnix int64 `json:"lease_expires_unix,omitempty"` // Absolute expiry from the server's ACK - DHCPServerID string `json:"dhcp_server_id,omitempty"` // Option 54 of the ACK - HWAddr string `json:"hw_addr,omitempty"` // chaddr placed in the DHCP payload } // 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"` @@ -58,23 +48,14 @@ type ExternalIPAMRecord struct { // ExternalPoolConfig is the admin-defined pool from spinifex.toml. type ExternalPoolConfig struct { - Name string - Source string // "static" (default) or "dhcp" - RangeStart string - RangeEnd string - Gateway string - GatewayIP string - PrefixLen int - 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". - // GatewayMAC, when set, is used as the chaddr in per-VM DHCP DORAs so - // that the upstream DHCP binding matches the MAC that OVN GARPs use (the - // gateway LRP MAC). Without this, the DHCP server binds the public IP to - // the generated ENI-derived MAC while OVN announces a different MAC via - // GARP, causing home routers that use their DHCP lease table as ARP to - // forward traffic to a non-existent interface. - GatewayMAC string + Name string + RangeStart string + RangeEnd string + Gateway string + GatewayIP string + PrefixLen int + Region string + AZ string // 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. @@ -82,25 +63,14 @@ type ExternalPoolConfig struct { GwLrpRangeEnd string } -// IsDHCP returns true if this pool obtains IPs from router DHCP. -func (p *ExternalPoolConfig) IsDHCP() bool { - return p.Source == "dhcp" -} - // ExternalIPAM manages external IP allocation from admin-defined pools using NATS KV with CAS. type ExternalIPAM struct { kv nats.KeyValue pools []ExternalPoolConfig - // nc is used to talk to spinifex-vpcd for DHCP-sourced pools via - // vpc.dhcp.acquire / vpc.dhcp.release. May be nil when no pool has - // Source="dhcp" (callers should still pass it if available). - nc *nats.Conn } // NewExternalIPAM creates a new ExternalIPAM backed by NATS JetStream KV. -// nc is required for DHCP-sourced pools and may be nil for static-only -// deployments. -func NewExternalIPAM(nc *nats.Conn, js nats.JetStreamContext, pools []ExternalPoolConfig) (*ExternalIPAM, error) { +func NewExternalIPAM(js nats.JetStreamContext, pools []ExternalPoolConfig) (*ExternalIPAM, error) { kv, err := utils.GetOrCreateKVBucket(js, KVBucketExternalIPAM, 5) if err != nil { return nil, fmt.Errorf("create external IPAM KV bucket: %w", err) @@ -108,17 +78,16 @@ func NewExternalIPAM(nc *nats.Conn, js nats.JetStreamContext, pools []ExternalPo if err := migrate.DefaultRegistry.RunKV(KVBucketExternalIPAM, kv, KVBucketExternalIPAMVersion); err != nil { return nil, fmt.Errorf("migrate %s: %w", KVBucketExternalIPAM, err) } - ipam := &ExternalIPAM{kv: kv, pools: pools, nc: nc} + ipam := &ExternalIPAM{kv: kv, pools: pools} if err := ipam.initPools(); err != nil { return nil, fmt.Errorf("init external IPAM pools: %w", err) } return ipam, nil } -// NewExternalIPAMWithKV creates an ExternalIPAM with an existing KV bucket -// (for testing). nc may be nil when the test exercises only static pools. -func NewExternalIPAMWithKV(nc *nats.Conn, kv nats.KeyValue, pools []ExternalPoolConfig) *ExternalIPAM { - return &ExternalIPAM{kv: kv, pools: pools, nc: nc} +// NewExternalIPAMWithKV creates an ExternalIPAM with an existing KV bucket (for testing). +func NewExternalIPAMWithKV(kv nats.KeyValue, pools []ExternalPoolConfig) *ExternalIPAM { + return &ExternalIPAM{kv: kv, pools: pools} } // initPools ensures each configured pool has a KV record. Idempotent — safe to call @@ -140,18 +109,16 @@ 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.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_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 @@ -172,39 +139,11 @@ func (m *ExternalIPAM) initPool(pool ExternalPoolConfig) error { slog.Info("external IPAM pool not found, creating", "pool", pool.Name) - // TODO: Confirm gateway IP defined, should error, given we route traffic to the upstream DHCP gateway gwIP := pool.GatewayIP - if gwIP == "" && !pool.IsDHCP() { + if gwIP == "" { gwIP = pool.RangeStart } - // For DHCP pools, obtain the gateway IP from router DHCP if not set. - // The gateway lease is long-lived: vpcd's renewal goroutine keeps it - // refreshed until the pool is torn down. - var gatewayLease *dhcp.LeaseResult - if gwIP == "" && pool.IsDHCP() { - lease, dhcpErr := dhcp.RequestAcquire(m.nc, pool.DhcpBindBridge, - "gateway-"+pool.Name, - "gateway-"+pool.Name, - "mulga-spinifex-gw", - pool.Name, - "", - ) - if dhcpErr != nil { - return fmt.Errorf("DHCP gateway lease for pool %q: %w", pool.Name, dhcpErr) - } - gatewayLease = &lease - gwIP = lease.IP - slog.Info("external IPAM obtained gateway IP via DHCP", "pool", pool.Name, "gateway_ip", gwIP) - } - - gatewayAlloc := ExternalIPAllocation{Type: "gateway", Note: "OVN router SNAT address"} - if gatewayLease != nil { - gatewayAlloc.LeaseExpiresUnix = gatewayLease.ExpiresUnix - gatewayAlloc.DHCPServerID = gatewayLease.ServerID - gatewayAlloc.HWAddr = gatewayLease.HWAddr - } - record := &ExternalIPAMRecord{ PoolName: pool.Name, RangeStart: pool.RangeStart, @@ -217,7 +156,7 @@ func (m *ExternalIPAM) initPool(pool ExternalPoolConfig) error { GwLrpRangeStart: pool.GwLrpRangeStart, GwLrpRangeEnd: pool.GwLrpRangeEnd, Allocated: map[string]ExternalIPAllocation{ - gwIP: gatewayAlloc, + gwIP: {Type: "gateway", Note: "OVN router SNAT address"}, }, } @@ -234,7 +173,7 @@ func (m *ExternalIPAM) initPool(pool ExternalPoolConfig) error { return fmt.Errorf("create pool KV entry: %w", err) } - slog.Info("external IPAM pool initialized", "pool", pool.Name, "gateway_ip", gwIP, "source", pool.Source) + slog.Info("external IPAM pool initialized", "pool", pool.Name, "gateway_ip", gwIP) return nil } @@ -259,71 +198,26 @@ func (m *ExternalIPAM) AllocateFromPool(poolName, allocType, allocID, eniID, ins } func (m *ExternalIPAM) allocateFromPool(poolName, allocType, allocID, eniID, instanceID string) (string, error) { - pool := m.findPoolByName(poolName) - - // DHCP client-id: prefer allocID (EIPs), then eniID (ENIs), then instanceID. - // At least one must be non-empty for DHCP pools — the upstream DHCP server - // uses this to keep per-client leases distinct. - clientID := allocID - if clientID == "" { - clientID = eniID - } - if clientID == "" { - clientID = instanceID - } - - // For DHCP-sourced pools: acquire the lease once, up front. vpcd's - // Manager is idempotent on ClientID, so CAS retries below will not - // trigger a second DORA — we only release at the end if no CAS - // attempt succeeded. - var dhcpLease *dhcp.LeaseResult - if pool != nil && pool.IsDHCP() { - hostname, vendorClass := dhcpIdentityOptions(eniID, instanceID, poolName) - lease, err := dhcp.RequestAcquire(m.nc, pool.DhcpBindBridge, clientID, hostname, vendorClass, poolName, pool.GatewayMAC) - if err != nil { - return "", fmt.Errorf("DHCP lease for %s: %w", clientID, err) - } - dhcpLease = &lease - } - for attempt := range 5 { record, revision, err := m.getRecord(poolName) if err != nil { - if dhcpLease != nil { - _ = dhcp.RequestRelease(m.nc, clientID) - } return "", fmt.Errorf("get external IPAM record: %w", err) } - var ip string - if dhcpLease != nil { - ip = dhcpLease.IP - } else { - // Static source: pick next IP from range - ip, err = nextAvailableExternalIP(record) - if err != nil { - return "", err - } + ip, err := nextAvailableExternalIP(record) + if err != nil { + return "", err } - alloc := ExternalIPAllocation{ + record.Allocated[ip] = ExternalIPAllocation{ Type: allocType, AllocationID: allocID, ENIId: eniID, InstanceId: instanceID, } - if dhcpLease != nil { - alloc.LeaseExpiresUnix = dhcpLease.ExpiresUnix - alloc.DHCPServerID = dhcpLease.ServerID - alloc.HWAddr = dhcpLease.HWAddr - } - record.Allocated[ip] = alloc data, err := json.Marshal(record) if err != nil { - if dhcpLease != nil { - _ = dhcp.RequestRelease(m.nc, clientID) - } return "", fmt.Errorf("marshal external IPAM record: %w", err) } @@ -332,47 +226,15 @@ func (m *ExternalIPAM) allocateFromPool(poolName, allocType, allocID, eniID, ins continue } - source := "static" - if pool != nil { - source = pool.Source - } - slog.Info("external IPAM allocated IP", "pool", poolName, "ip", ip, "type", allocType, "source", source) + slog.Info("external IPAM allocated IP", "pool", poolName, "ip", ip, "type", allocType) return ip, nil } - // All CAS attempts exhausted — release any DHCP lease we acquired so - // vpcd doesn't hold a binding for an IP we won't use. - if dhcpLease != nil { - _ = dhcp.RequestRelease(m.nc, clientID) - } return "", fmt.Errorf("external IPAM allocation failed after CAS retries for pool %s", poolName) } -// dhcpIdentityOptions composes the option 12 (hostname) and option 60 -// (vendor class) values for a per-allocation lease. Empty eniID / -// instanceID fall through to identifiers that still group usefully on -// the upstream server's lease table. Instance and ENI identifiers are -// used as-is (they already carry their "i-" / "eni-" prefixes). -func dhcpIdentityOptions(eniID, instanceID, poolName string) (hostname, vendorClass string) { - hostname = eniID - if hostname == "" { - if instanceID != "" { - hostname = "spinifex-" + instanceID - } else { - hostname = "spinifex-" + poolName - } - } - vendorClass = "mulga-spinifex" - if instanceID != "" { - vendorClass = instanceID - } - return hostname, vendorClass -} - // ReleaseIP releases a previously allocated external IP back to its pool. func (m *ExternalIPAM) ReleaseIP(poolName, ip string) error { - pool := m.findPoolByName(poolName) - for attempt := range 5 { record, revision, err := m.getRecord(poolName) if err != nil { @@ -387,22 +249,6 @@ func (m *ExternalIPAM) ReleaseIP(poolName, ip string) error { return fmt.Errorf("cannot release gateway IP %s in pool %s", ip, poolName) } - // Release DHCP lease if this is a DHCP-sourced pool. clientID - // selection mirrors allocateFromPool so vpcd's Manager can find the - // lease it originally acquired. - if pool != nil && pool.IsDHCP() { - clientID := alloc.AllocationID - if clientID == "" { - clientID = alloc.ENIId - } - if clientID == "" { - clientID = alloc.InstanceId - } - if releaseErr := dhcp.RequestRelease(m.nc, clientID); releaseErr != nil { - slog.Warn("Failed to release DHCP lease", "pool", poolName, "ip", ip, "err", releaseErr) - } - } - delete(record.Allocated, ip) data, err := json.Marshal(record) @@ -455,16 +301,6 @@ func (m *ExternalIPAM) findPool(region, az string) *ExternalPoolConfig { return nil } -// findPoolByName returns the pool config by name. -func (m *ExternalIPAM) findPoolByName(name string) *ExternalPoolConfig { - for i := range m.pools { - if m.pools[i].Name == name { - return &m.pools[i] - } - } - return nil -} - func (m *ExternalIPAM) getRecord(poolName string) (*ExternalIPAMRecord, uint64, error) { entry, err := m.kv.Get(poolName) if err != nil { @@ -529,44 +365,41 @@ func ValidatePoolConfig(pool ExternalPoolConfig) error { if pool.GatewayIP != "" && net.ParseIP(pool.GatewayIP) == nil { return fmt.Errorf("invalid gateway_ip: %q", pool.GatewayIP) } - // DHCP pools don't need range_start/range_end - if !pool.IsDHCP() { - startIP := net.ParseIP(pool.RangeStart) - if startIP == nil { - return fmt.Errorf("invalid range_start: %q", pool.RangeStart) + startIP := net.ParseIP(pool.RangeStart) + if startIP == nil { + return fmt.Errorf("invalid range_start: %q", pool.RangeStart) + } + endIP := net.ParseIP(pool.RangeEnd) + if endIP == nil { + return fmt.Errorf("invalid range_end: %q", pool.RangeEnd) + } + 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) } - endIP := net.ParseIP(pool.RangeEnd) - if endIP == nil { - return fmt.Errorf("invalid range_end: %q", pool.RangeEnd) + if gwE == nil { + return fmt.Errorf("invalid gw_lrp_range_end: %q", pool.GwLrpRangeEnd) } - 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) + 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) } - // 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) - } + 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/handlers/ec2/vpc/external_ipam_test.go b/spinifex/handlers/ec2/vpc/external_ipam_test.go index e3220124d..71dad2eaf 100644 --- a/spinifex/handlers/ec2/vpc/external_ipam_test.go +++ b/spinifex/handlers/ec2/vpc/external_ipam_test.go @@ -1,24 +1,19 @@ package handlers_ec2_vpc import ( - "encoding/json" "sync" - "sync/atomic" "testing" - "time" - "github.com/mulgadc/spinifex/spinifex/services/vpcd/dhcp" "github.com/mulgadc/spinifex/spinifex/testutil" - "github.com/nats-io/nats.go" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func setupTestExternalIPAM(t *testing.T, pools []ExternalPoolConfig) *ExternalIPAM { t.Helper() - _, nc, js := testutil.StartTestJetStream(t) + _, _, js := testutil.StartTestJetStream(t) - ipam, err := NewExternalIPAM(nc, js, pools) + ipam, err := NewExternalIPAM(js, pools) require.NoError(t, err) return ipam } @@ -339,10 +334,10 @@ func TestExternalIPAM_RangeValidation(t *testing.T) { func TestExternalIPAM_InitFromConfig(t *testing.T) { pool := testPool() // Create IPAM twice — second init should be idempotent - _, nc, js := testutil.StartTestJetStream(t) + _, _, js := testutil.StartTestJetStream(t) // First init - ipam1, err := NewExternalIPAM(nc, js, []ExternalPoolConfig{pool}) + ipam1, err := NewExternalIPAM(js, []ExternalPoolConfig{pool}) require.NoError(t, err) // Allocate an IP @@ -351,7 +346,7 @@ func TestExternalIPAM_InitFromConfig(t *testing.T) { assert.Equal(t, "192.168.1.151", ip) // Second init (simulating restart) — should not lose allocation - ipam2, err := NewExternalIPAM(nc, js, []ExternalPoolConfig{pool}) + ipam2, err := NewExternalIPAM(js, []ExternalPoolConfig{pool}) require.NoError(t, err) record, err := ipam2.GetPoolRecord("wan") @@ -389,247 +384,14 @@ func TestExternalIPAM_FindPoolByName_NotFound(t *testing.T) { pool := testPool() ipam := setupTestExternalIPAM(t, []ExternalPoolConfig{pool}) - // findPoolByName is private, but exercised via AllocateFromPool with unknown pool. - // The function returns nil when no pool matches, which means static allocation path. - // We verify by using NewExternalIPAMWithKV to directly check findPoolByName returns nil. + // AllocateFromPool with a non-existent pool name: the pool "nonexistent" + // has no KV record, so getRecord fails. kv := ipam.kv - ipam2 := NewExternalIPAMWithKV(nil, kv, []ExternalPoolConfig{pool}) - // AllocateFromPool with a non-existent pool name: findPoolByName returns nil, - // so pool.IsDHCP() is skipped and static allocation is used. - // The pool "nonexistent" has no KV record, so getRecord fails. + ipam2 := NewExternalIPAMWithKV(kv, []ExternalPoolConfig{pool}) _, err := ipam2.AllocateFromPool("nonexistent", "auto_assign", "", "eni-1", "i-1") assert.Error(t, err) } -// dhcpStub captures acquire/release requests and replies with a -// deterministic lease so the handler-side IPAM code can be exercised -// without running vpcd. -type dhcpStub struct { - mu sync.Mutex - acquired []dhcp.AcquireRequestMsg - released []dhcp.ReleaseRequestMsg - nextIP string - acquireErr string - acquireCalls atomic.Int32 - releaseCalls atomic.Int32 - serverID string - leaseSeconds int64 - hwAddrOverride string - releaseErrOnce atomic.Bool -} - -func newDHCPStub(t *testing.T, nc *nats.Conn) *dhcpStub { - t.Helper() - s := &dhcpStub{ - nextIP: "192.168.1.151", - serverID: "192.168.1.1", - leaseSeconds: 3600, - } - acquire, err := nc.Subscribe(dhcp.TopicAcquire, func(msg *nats.Msg) { - s.acquireCalls.Add(1) - var req dhcp.AcquireRequestMsg - if err := json.Unmarshal(msg.Data, &req); err != nil { - respondTestJSON(t, msg, dhcp.AcquireReplyMsg{Error: "malformed request"}) - return - } - s.mu.Lock() - s.acquired = append(s.acquired, req) - ip := s.nextIP - errMsg := s.acquireErr - serverID := s.serverID - expires := time.Now().Add(time.Duration(s.leaseSeconds) * time.Second).Unix() - hw := s.hwAddrOverride - s.mu.Unlock() - - if errMsg != "" { - respondTestJSON(t, msg, dhcp.AcquireReplyMsg{Error: errMsg}) - return - } - if hw == "" { - hw = "02:00:00:aa:bb:cc" - } - respondTestJSON(t, msg, dhcp.AcquireReplyMsg{ - IP: ip, - SubnetMask: "255.255.255.0", - Routers: []string{"192.168.1.1"}, - DNS: []string{"192.168.1.1"}, - ServerID: serverID, - HWAddr: hw, - ExpiresUnix: expires, - }) - }) - require.NoError(t, err) - t.Cleanup(func() { _ = acquire.Unsubscribe() }) - - release, err := nc.Subscribe(dhcp.TopicRelease, func(msg *nats.Msg) { - s.releaseCalls.Add(1) - var req dhcp.ReleaseRequestMsg - _ = json.Unmarshal(msg.Data, &req) - s.mu.Lock() - s.released = append(s.released, req) - s.mu.Unlock() - reply := dhcp.ReleaseReplyMsg{} - if s.releaseErrOnce.CompareAndSwap(true, false) { - reply.Error = "first release fails" - } - respondTestJSON(t, msg, reply) - }) - require.NoError(t, err) - t.Cleanup(func() { _ = release.Unsubscribe() }) - return s -} - -func respondTestJSON(t *testing.T, msg *nats.Msg, v any) { - t.Helper() - data, err := json.Marshal(v) - require.NoError(t, err) - require.NoError(t, msg.Respond(data)) -} - -func TestExternalIPAM_DHCPAllocateAndRelease(t *testing.T) { - _, nc, js := testutil.StartTestJetStream(t) - stub := newDHCPStub(t, nc) - stub.nextIP = "192.168.1.200" - - pool := ExternalPoolConfig{ - Name: "wan-dhcp", - Source: "dhcp", - Gateway: "192.168.1.1", - GatewayIP: "192.168.1.1", // pre-set so initPool skips gateway DORA - PrefixLen: 24, - DhcpBindBridge: "br-wan", - } - ipam, err := NewExternalIPAM(nc, js, []ExternalPoolConfig{pool}) - require.NoError(t, err) - - ip, err := ipam.AllocateFromPool("wan-dhcp", "elastic_ip", "eipalloc-123", "eni-abc", "i-xyz") - require.NoError(t, err) - assert.Equal(t, "192.168.1.200", ip) - require.EqualValues(t, 1, stub.acquireCalls.Load()) - - stub.mu.Lock() - got := stub.acquired[0] - stub.mu.Unlock() - assert.Equal(t, "eipalloc-123", got.ClientID, "clientID precedence: allocID first") - assert.Equal(t, "eni-abc", got.Hostname, "hostname defaults to eniID") - assert.Equal(t, "i-xyz", got.VendorClass, "vendor class i-") - assert.Equal(t, "wan-dhcp", got.PoolName) - assert.Equal(t, "br-wan", got.Bridge) - - record, err := ipam.GetPoolRecord("wan-dhcp") - require.NoError(t, err) - alloc, ok := record.Allocated["192.168.1.200"] - require.True(t, ok) - assert.Equal(t, "192.168.1.1", alloc.DHCPServerID) - assert.NotZero(t, alloc.LeaseExpiresUnix) - assert.Equal(t, "02:00:00:aa:bb:cc", alloc.HWAddr) - - require.NoError(t, ipam.ReleaseIP("wan-dhcp", "192.168.1.200")) - require.EqualValues(t, 1, stub.releaseCalls.Load()) - stub.mu.Lock() - assert.Equal(t, "eipalloc-123", stub.released[0].ClientID) - stub.mu.Unlock() -} - -func TestExternalIPAM_DHCPAcquireErrorPropagates(t *testing.T) { - _, nc, js := testutil.StartTestJetStream(t) - stub := newDHCPStub(t, nc) - stub.acquireErr = "server unreachable" - - pool := ExternalPoolConfig{ - Name: "wan-dhcp-err", Source: "dhcp", Gateway: "192.168.1.1", - GatewayIP: "192.168.1.1", PrefixLen: 24, DhcpBindBridge: "br-wan", - } - ipam, err := NewExternalIPAM(nc, js, []ExternalPoolConfig{pool}) - require.NoError(t, err) - - _, err = ipam.AllocateFromPool("wan-dhcp-err", "elastic_ip", "eipalloc-err", "", "") - require.Error(t, err) - assert.Contains(t, err.Error(), "server unreachable") -} - -func TestExternalIPAM_DHCPGatewayFromLease(t *testing.T) { - _, nc, js := testutil.StartTestJetStream(t) - stub := newDHCPStub(t, nc) - stub.nextIP = "192.168.3.247" - - pool := ExternalPoolConfig{ - Name: "wan-dhcp-gw", Source: "dhcp", Gateway: "192.168.3.1", - PrefixLen: 24, DhcpBindBridge: "br-wan", - // GatewayIP intentionally empty — initPool must pull it from DHCP. - } - _, err := NewExternalIPAM(nc, js, []ExternalPoolConfig{pool}) - require.NoError(t, err) - assert.EqualValues(t, 1, stub.acquireCalls.Load()) - - stub.mu.Lock() - got := stub.acquired[0] - stub.mu.Unlock() - assert.Equal(t, "gateway-wan-dhcp-gw", got.ClientID) - assert.Equal(t, "mulga-spinifex-gw", got.VendorClass) -} - -func TestExternalIPAM_DHCPGatewayErrorPropagates(t *testing.T) { - _, nc, js := testutil.StartTestJetStream(t) - stub := newDHCPStub(t, nc) - stub.acquireErr = "no response from upstream" - - pool := ExternalPoolConfig{ - Name: "wan-gw-err", Source: "dhcp", Gateway: "192.168.1.1", - PrefixLen: 24, DhcpBindBridge: "br-wan", - } - _, err := NewExternalIPAM(nc, js, []ExternalPoolConfig{pool}) - require.Error(t, err) - assert.Contains(t, err.Error(), "no response from upstream") -} - -func TestExternalIPAM_DHCPReleaseErrorIsNonFatal(t *testing.T) { - _, nc, js := testutil.StartTestJetStream(t) - stub := newDHCPStub(t, nc) - stub.nextIP = "192.168.1.155" - stub.releaseErrOnce.Store(true) - - pool := ExternalPoolConfig{ - Name: "wan-rel-err", Source: "dhcp", Gateway: "192.168.1.1", - GatewayIP: "192.168.1.1", PrefixLen: 24, DhcpBindBridge: "br-wan", - } - ipam, err := NewExternalIPAM(nc, js, []ExternalPoolConfig{pool}) - require.NoError(t, err) - - ip, err := ipam.AllocateFromPool("wan-rel-err", "elastic_ip", "eipalloc-rel", "", "") - require.NoError(t, err) - - // Release reports vpcd failure via slog.Warn but ReleaseIP itself - // still succeeds — the allocation is removed from KV. - require.NoError(t, ipam.ReleaseIP("wan-rel-err", ip)) - rec, err := ipam.GetPoolRecord("wan-rel-err") - require.NoError(t, err) - _, stillThere := rec.Allocated[ip] - assert.False(t, stillThere) -} - -func TestDHCPIdentityOptions(t *testing.T) { - tests := []struct { - name string - eniID string - instanceID string - pool string - wantHost string - wantVendor string - }{ - {"eni+instance", "eni-1", "i-abcd", "wan", "eni-1", "i-abcd"}, - {"instance only", "", "i-xyz", "wan", "spinifex-i-xyz", "i-xyz"}, - {"neither", "", "", "wan", "spinifex-wan", "mulga-spinifex"}, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - host, vendor := dhcpIdentityOptions(tc.eniID, tc.instanceID, tc.pool) - assert.Equal(t, tc.wantHost, host) - assert.Equal(t, tc.wantVendor, vendor) - }) - } -} - // 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) { diff --git a/spinifex/network/external/external.go b/spinifex/network/external/external.go index 6b8f61d67..757edc44c 100644 --- a/spinifex/network/external/external.go +++ b/spinifex/network/external/external.go @@ -11,12 +11,10 @@ // grows those methods, IGWManager calls L1 directly for those L2 objects. // This is documented and tracked for cleanup in a follow-on bead. // -// - DHCP-sourced pools (source="dhcp") use upstream router DHCP to acquire -// the gateway LRP IP. The DHCP client and JetStream lease store live in -// services/vpcd/dhcp; this package keeps the dependency out by accepting -// a GatewayIPAllocator interface. StaticRangeAllocator is provided here; -// the DHCP-backed allocator stays in vpcd until bead mulga-siv-125.3.3 -// (DHCP removal) is unblocked. +// - Gateway LRP IP allocation goes through a GatewayIPAllocator interface. +// StaticRangeAllocator (this package) picks from pool.gw_lrp_range; +// LinkLocalAllocator returns ok=false for distributed-NAT deployments +// where the gateway LRP never goes on the wire. // // See docs/development/feature/spinifex-network-redesign.md §10 and // docs/development/feature/spinifex-network-redesign-phase2.md §2.4. diff --git a/spinifex/network/external/pool.go b/spinifex/network/external/pool.go index 7dde734c2..c1fdc2c0b 100644 --- a/spinifex/network/external/pool.go +++ b/spinifex/network/external/pool.go @@ -64,10 +64,6 @@ func FindPool(pools []ExternalPoolConfig, region, az string) *ExternalPoolConfig // pool.gw_lrp_range, persisting the assignment via the LRP's // external_ids so reconciles see it on retry. // -// - DHCP-backed allocator (lives in services/vpcd until bead -// mulga-siv-125.3.3 lands): requests an upstream DHCP lease and reuses -// it on idempotent re-attach. -// // - LinkLocalAllocator (this package): always returns ok=false; callers // fall back to linkLocalGatewayNetwork. Useful for distributed-NAT // deployments where the gateway LRP never goes on the wire. diff --git a/spinifex/network/external/types.go b/spinifex/network/external/types.go index ca98d1a1a..80b8fddd5 100644 --- a/spinifex/network/external/types.go +++ b/spinifex/network/external/types.go @@ -1,23 +1,16 @@ package external // ExternalPoolConfig describes one external IP pool wired to a VPC. Mirrors -// the [external_pools] entry in spinifex.toml. Two pool kinds are supported: -// -// - Source="static": IPs come from RangeStart..RangeEnd, gateway from -// Gateway, and the gateway LRP IP from GwLrpRangeStart..GwLrpRangeEnd -// (auto-derived from the WAN subnet when unset). -// -// - Source="dhcp": IPs come from upstream router DHCP. The gateway LRP IP -// is acquired via upstream DHCP using the pool's DhcpBindBridge. This -// package does not include the DHCP client; callers inject a -// GatewayIPAllocator that knows how to talk to the DHCP store. +// the [external_pools] entry in spinifex.toml. IPs come from +// RangeStart..RangeEnd, the gateway from Gateway, and the gateway LRP IP +// from GwLrpRangeStart..GwLrpRangeEnd (auto-derived from the WAN subnet +// when unset). The upstream-DHCP source model is gone (mulga-siv-125.3.3). // // This type duplicates services/vpcd/vpcd.go's ExternalPoolConfig. Dedup is // out of scope for this bead; the two will merge when L2 (topology) grows // the L5-driving methods and vpcd's local copy disappears. type ExternalPoolConfig struct { Name string - Source string RangeStart string RangeEnd string Gateway string @@ -26,16 +19,10 @@ type ExternalPoolConfig struct { DNSServers []string Region string AZ string - DhcpBindBridge string GwLrpRangeStart string GwLrpRangeEnd string } -// IsDHCP reports whether the pool acquires IPs from an upstream DHCP server. -func (p *ExternalPoolConfig) IsDHCP() bool { - return p.Source == "dhcp" -} - // IGWSpec is the L5 input for IGWManager.AttachIGW / DetachIGW. VPCID is // the only required field; InternetGatewayID is propagated into OVN // external_ids so reconcile paths can correlate state. diff --git a/spinifex/services/vpcd/dhcp/client.go b/spinifex/services/vpcd/dhcp/client.go deleted file mode 100644 index f0d3559ee..000000000 --- a/spinifex/services/vpcd/dhcp/client.go +++ /dev/null @@ -1,88 +0,0 @@ -// Package dhcp provides an in-process DHCPv4 client used by spinifex-vpcd -// to obtain, renew and release real server-bound leases on behalf of the -// external IPAM pool. It replaces the historical "shell out to sudo dhcpcd" -// path which had no renewal, phantom leases under -T, and a release channel -// that collided with a host-side dhcpcd daemon managing the same bridge. -// -// The real implementation (NewNClient4) opens an AF_PACKET socket per DORA -// on the target bridge via github.com/insomniacslk/dhcp/dhcpv4/nclient4. -// Tests inject Fake instead. -package dhcp - -import ( - "context" - "net" - "time" -) - -// Client is the per-handshake DHCP surface. Backed by NewNClient4 in -// production; Fake satisfies this for unit tests. -type Client interface { - // Acquire performs a full DORA on req.Bridge and returns the - // server-bound lease. Blocks until ACK or ctx cancellation. - Acquire(ctx context.Context, req AcquireRequest) (*Lease, error) - // Renew refreshes an existing lease with its recorded server-id. - // Returns a new Lease reflecting the refreshed timers. - Renew(ctx context.Context, lease *Lease) (*Lease, error) - // Release sends DHCPRELEASE to the server-id in the lease. - Release(ctx context.Context, lease *Lease) error -} - -// AcquireRequest identifies a new DHCP lease. Fields map 1:1 to DHCP -// options and the payload chaddr. -type AcquireRequest struct { - Bridge string // OVS bridge carrying the AF_PACKET socket - ClientID string // option 61 — client-identifier - Hostname string // option 12 — host-name - VendorClass string // option 60 — vendor-class-identifier - HWAddr net.HardwareAddr // chaddr in the DHCP payload -} - -// Lease is a server-bound DHCPv4 lease. Carries enough state for Renew -// and Release, plus the raw Offer/ACK packets so that the Manager can -// resume after a daemon restart. -type Lease struct { - // Request context — echoed for audit; also used to rebuild the - // underlying nclient4 client on Renew/Release. - Bridge string - ClientID string - Hostname string - VendorClass string - HWAddr net.HardwareAddr - - // Wire state extracted from the server's ACK. - IP net.IP - SubnetMask net.IPMask - Routers []net.IP - DNS []net.IP - ServerID net.IP - - AcquiredAt time.Time - LeaseDuration time.Duration - T1 time.Duration // renew timer (option 58, or lease/2) - T2 time.Duration // rebind timer (option 59, or lease*7/8) - - // Raw wire packets. Opaque to callers; nclient4 needs them to build - // RENEW / RELEASE messages that hit the same server binding. Persisted - // in KV for crash recovery. - RawOffer []byte - RawACK []byte -} - -// ExpiresAt returns the absolute deadline at which the upstream server is -// free to reassign this IP. -func (l *Lease) ExpiresAt() time.Time { - return l.AcquiredAt.Add(l.LeaseDuration) -} - -// RenewAt returns the absolute time the Manager should attempt a RENEW -// (option 58 / T1). -func (l *Lease) RenewAt() time.Time { - return l.AcquiredAt.Add(l.T1) -} - -// RebindAt returns the absolute time the Manager should attempt a REBIND -// (option 59 / T2). -func (l *Lease) RebindAt() time.Time { - return l.AcquiredAt.Add(l.T2) -} diff --git a/spinifex/services/vpcd/dhcp/client_test.go b/spinifex/services/vpcd/dhcp/client_test.go deleted file mode 100644 index 7097c7c3d..000000000 --- a/spinifex/services/vpcd/dhcp/client_test.go +++ /dev/null @@ -1,174 +0,0 @@ -package dhcp_test - -import ( - "context" - "errors" - "net" - "testing" - "time" - - "github.com/mulgadc/spinifex/spinifex/services/vpcd/dhcp" -) - -func TestLeaseTimers(t *testing.T) { - acq := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) - l := &dhcp.Lease{ - AcquiredAt: acq, - LeaseDuration: time.Hour, - T1: 30 * time.Minute, - T2: 52*time.Minute + 30*time.Second, - } - - if got := l.RenewAt(); !got.Equal(acq.Add(30 * time.Minute)) { - t.Errorf("RenewAt: got %v, want %v", got, acq.Add(30*time.Minute)) - } - if got := l.RebindAt(); !got.Equal(acq.Add(52*time.Minute + 30*time.Second)) { - t.Errorf("RebindAt: got %v, want %v", got, acq.Add(52*time.Minute+30*time.Second)) - } - if got := l.ExpiresAt(); !got.Equal(acq.Add(time.Hour)) { - t.Errorf("ExpiresAt: got %v, want %v", got, acq.Add(time.Hour)) - } -} - -func TestFakeAcquireReturnsLeaseAndTracksCount(t *testing.T) { - f := dhcp.NewFake() - mac, _ := net.ParseMAC("02:00:00:aa:bb:cc") - - lease, err := f.Acquire(context.Background(), dhcp.AcquireRequest{ - Bridge: "br-wan", ClientID: "eni-1", HWAddr: mac, - }) - if err != nil { - t.Fatalf("Acquire: %v", err) - } - if lease == nil || lease.IP == nil { - t.Fatalf("expected lease with IP, got %+v", lease) - } - if f.AcquireCount() != 1 { - t.Errorf("acquire count = %d, want 1", f.AcquireCount()) - } - if held, ok := f.HeldLease("eni-1"); !ok || held.IP.String() != lease.IP.String() { - t.Errorf("held lease = %v/%v, want match", held, ok) - } -} - -func TestFakeAcquireHookOverridesDefault(t *testing.T) { - f := dhcp.NewFake() - f.AcquireHook = func(req dhcp.AcquireRequest) (*dhcp.Lease, error) { - return nil, errors.New("injected") - } - _, err := f.Acquire(context.Background(), dhcp.AcquireRequest{Bridge: "br-wan", ClientID: "x"}) - if err == nil || err.Error() != "injected" { - t.Fatalf("expected injected error, got %v", err) - } -} - -func TestFakeRenewRefreshesAcquiredAt(t *testing.T) { - f := dhcp.NewFake() - lease, err := f.Acquire(context.Background(), dhcp.AcquireRequest{ - Bridge: "br-wan", ClientID: "eni-2", - }) - if err != nil { - t.Fatalf("Acquire: %v", err) - } - firstAcq := lease.AcquiredAt - - time.Sleep(10 * time.Millisecond) - renewed, err := f.Renew(context.Background(), lease) - if err != nil { - t.Fatalf("Renew: %v", err) - } - if !renewed.AcquiredAt.After(firstAcq) { - t.Errorf("renewed AcquiredAt %v not after original %v", renewed.AcquiredAt, firstAcq) - } - if f.RenewCount() != 1 { - t.Errorf("renew count = %d, want 1", f.RenewCount()) - } -} - -func TestFakeReleaseClearsTrackedLease(t *testing.T) { - f := dhcp.NewFake() - lease, _ := f.Acquire(context.Background(), dhcp.AcquireRequest{ - Bridge: "br-wan", ClientID: "eni-3", - }) - if err := f.Release(context.Background(), lease); err != nil { - t.Fatalf("Release: %v", err) - } - if _, ok := f.HeldLease("eni-3"); ok { - t.Errorf("expected lease removed after Release") - } - if f.ReleaseCount() != 1 { - t.Errorf("release count = %d, want 1", f.ReleaseCount()) - } -} - -func TestFakeRenewHookSurfacesError(t *testing.T) { - f := dhcp.NewFake() - f.RenewHook = func(l *dhcp.Lease) (*dhcp.Lease, error) { - return nil, errors.New("server NAK") - } - lease, _ := f.Acquire(context.Background(), dhcp.AcquireRequest{Bridge: "br-wan", ClientID: "eni-4"}) - if _, err := f.Renew(context.Background(), lease); err == nil { - t.Fatal("expected hook error") - } -} - -func TestFakeReleaseHookSurfacesError(t *testing.T) { - f := dhcp.NewFake() - f.ReleaseHook = func(*dhcp.Lease) error { return errors.New("server unreachable") } - lease, _ := f.Acquire(context.Background(), dhcp.AcquireRequest{Bridge: "br-wan", ClientID: "eni-rel"}) - if err := f.Release(context.Background(), lease); err == nil { - t.Fatal("expected hook error") - } -} - -func TestFakeRenewNilLease(t *testing.T) { - f := dhcp.NewFake() - if _, err := f.Renew(context.Background(), nil); err == nil { - t.Fatal("expected error for nil lease") - } -} - -func TestFakeReleaseNilIsNoop(t *testing.T) { - f := dhcp.NewFake() - if err := f.Release(context.Background(), nil); err != nil { - t.Fatalf("expected nil, got %v", err) - } -} - -func TestNClient4ValidatesInputs(t *testing.T) { - c := dhcp.NewNClient4(0, 0) // defaults applied - if _, err := c.Acquire(context.Background(), dhcp.AcquireRequest{}); err == nil { - t.Fatal("expected bridge-required error") - } - mac, _ := net.ParseMAC("02:00:00:aa:bb:cc") - if _, err := c.Acquire(context.Background(), dhcp.AcquireRequest{Bridge: "br-wan"}); err == nil { - t.Fatal("expected hw_addr-required error") - } - _ = mac -} - -func TestNClient4RenewRejectsNilAndMissingRaw(t *testing.T) { - c := dhcp.NewNClient4(0, 0) - if _, err := c.Renew(context.Background(), nil); err == nil { - t.Fatal("expected nil-lease error") - } - lease := &dhcp.Lease{Bridge: "br-wan", ClientID: "x"} // no Raw bytes - if _, err := c.Renew(context.Background(), lease); err == nil { - t.Fatal("expected missing-raw-bytes error") - } -} - -func TestNClient4ReleaseNilIsNoop(t *testing.T) { - c := dhcp.NewNClient4(0, 0) - if err := c.Release(context.Background(), nil); err != nil { - t.Fatalf("expected nil, got %v", err) - } -} - -func TestNClient4ReleaseRejectsMissingRaw(t *testing.T) { - c := dhcp.NewNClient4(0, 0) - lease := &dhcp.Lease{Bridge: "br-wan", ClientID: "x"} - if err := c.Release(context.Background(), lease); err == nil { - t.Fatal("expected missing-raw-bytes error") - } -} diff --git a/spinifex/services/vpcd/dhcp/fake.go b/spinifex/services/vpcd/dhcp/fake.go deleted file mode 100644 index c75304c4b..000000000 --- a/spinifex/services/vpcd/dhcp/fake.go +++ /dev/null @@ -1,179 +0,0 @@ -package dhcp - -import ( - "context" - "fmt" - "net" - "sync" - "time" -) - -// Fake is a configurable in-memory Client for unit tests. It mimics the -// DORA / RENEW / RELEASE lifecycle well enough to exercise the Manager -// without opening real sockets. Hooks let tests inject errors or custom -// responses at each step. -type Fake struct { - mu sync.Mutex - - // Default template for synthesised leases. Tests can override via - // SetDefaultLease. - defaultLease LeaseTemplate - - // Optional hooks — if non-nil the fake calls the hook instead of - // synthesising a response. Useful for fault injection. - AcquireHook func(AcquireRequest) (*Lease, error) - RenewHook func(*Lease) (*Lease, error) - ReleaseHook func(*Lease) error - - // Tracked state. - leases map[string]*Lease // keyed by ClientID - acquireCount int - renewCount int - releaseCount int -} - -var _ Client = (*Fake)(nil) - -// LeaseTemplate configures the synthetic lease Fake returns from Acquire -// when no AcquireHook is set. -type LeaseTemplate struct { - IP net.IP - SubnetMask net.IPMask - Routers []net.IP - DNS []net.IP - ServerID net.IP - LeaseDuration time.Duration -} - -// DefaultLeaseTemplate returns a reasonable template for tests (RFC 5737 -// TEST-NET-1, 1 h lease). -func DefaultLeaseTemplate() LeaseTemplate { - return LeaseTemplate{ - IP: net.IPv4(192, 0, 2, 100), - SubnetMask: net.CIDRMask(24, 32), - Routers: []net.IP{net.IPv4(192, 0, 2, 1)}, - DNS: []net.IP{net.IPv4(192, 0, 2, 1)}, - ServerID: net.IPv4(192, 0, 2, 1), - LeaseDuration: time.Hour, - } -} - -// NewFake creates a Fake pre-loaded with DefaultLeaseTemplate. -func NewFake() *Fake { - return &Fake{ - defaultLease: DefaultLeaseTemplate(), - leases: map[string]*Lease{}, - } -} - -// SetDefaultLease overrides the template used for subsequent Acquire calls. -func (f *Fake) SetDefaultLease(tmpl LeaseTemplate) { - f.mu.Lock() - defer f.mu.Unlock() - f.defaultLease = tmpl -} - -// AcquireCount returns the number of Acquire calls observed. -func (f *Fake) AcquireCount() int { f.mu.Lock(); defer f.mu.Unlock(); return f.acquireCount } - -// RenewCount returns the number of Renew calls observed. -func (f *Fake) RenewCount() int { f.mu.Lock(); defer f.mu.Unlock(); return f.renewCount } - -// ReleaseCount returns the number of Release calls observed. -func (f *Fake) ReleaseCount() int { f.mu.Lock(); defer f.mu.Unlock(); return f.releaseCount } - -// HeldLease returns the last lease Fake synthesised for the given ClientID. -func (f *Fake) HeldLease(clientID string) (*Lease, bool) { - f.mu.Lock() - defer f.mu.Unlock() - l, ok := f.leases[clientID] - return l, ok -} - -func (f *Fake) Acquire(_ context.Context, req AcquireRequest) (*Lease, error) { - f.mu.Lock() - f.acquireCount++ - hook := f.AcquireHook - tmpl := f.defaultLease - f.mu.Unlock() - - if hook != nil { - lease, err := hook(req) - if err == nil && lease != nil { - f.mu.Lock() - f.leases[req.ClientID] = lease - f.mu.Unlock() - } - return lease, err - } - - lease := &Lease{ - Bridge: req.Bridge, - ClientID: req.ClientID, - Hostname: req.Hostname, - VendorClass: req.VendorClass, - HWAddr: append(net.HardwareAddr(nil), req.HWAddr...), - IP: tmpl.IP, - SubnetMask: tmpl.SubnetMask, - Routers: tmpl.Routers, - DNS: tmpl.DNS, - ServerID: tmpl.ServerID, - AcquiredAt: time.Now(), - LeaseDuration: tmpl.LeaseDuration, - T1: tmpl.LeaseDuration / 2, - T2: tmpl.LeaseDuration * 7 / 8, - // Synthetic raw packets — opaque marker bytes so Renew/Release - // distinguish a fake lease from a nil-bytes one. - RawOffer: []byte("fake-offer-" + req.ClientID), - RawACK: []byte("fake-ack-" + req.ClientID), - } - - f.mu.Lock() - f.leases[req.ClientID] = lease - f.mu.Unlock() - return lease, nil -} - -func (f *Fake) Renew(_ context.Context, lease *Lease) (*Lease, error) { - if lease == nil { - return nil, fmt.Errorf("fake renew: lease is nil") - } - f.mu.Lock() - f.renewCount++ - hook := f.RenewHook - f.mu.Unlock() - - if hook != nil { - renewed, err := hook(lease) - if err == nil && renewed != nil { - f.mu.Lock() - f.leases[lease.ClientID] = renewed - f.mu.Unlock() - } - return renewed, err - } - - renewed := *lease - renewed.AcquiredAt = time.Now() - f.mu.Lock() - f.leases[lease.ClientID] = &renewed - f.mu.Unlock() - return &renewed, nil -} - -func (f *Fake) Release(_ context.Context, lease *Lease) error { - if lease == nil { - return nil - } - f.mu.Lock() - f.releaseCount++ - hook := f.ReleaseHook - f.mu.Unlock() - if hook != nil { - return hook(lease) - } - f.mu.Lock() - delete(f.leases, lease.ClientID) - f.mu.Unlock() - return nil -} diff --git a/spinifex/services/vpcd/dhcp/nats.go b/spinifex/services/vpcd/dhcp/nats.go deleted file mode 100644 index 758a1d39e..000000000 --- a/spinifex/services/vpcd/dhcp/nats.go +++ /dev/null @@ -1,73 +0,0 @@ -package dhcp - -// NATS subjects that spinifex-vpcd's DHCP manager subscribes to and -// publishes on. The daemon-side handlers in handlers/ec2/vpc send -// requests against the request/reply subjects. -const ( - // TopicAcquire is the request/reply subject for obtaining a lease. - // Request: AcquireRequestMsg. Reply: AcquireReplyMsg. - TopicAcquire = "vpc.dhcp.acquire" - // TopicRelease is the request/reply subject for releasing a lease. - // Request: ReleaseRequestMsg. Reply: ReleaseReplyMsg. - TopicRelease = "vpc.dhcp.release" - // TopicLeaseExpired is a fan-out event published by vpcd when a - // renewal permanently fails and the lease has passed its expiry. - // Consumers (EIP / instance / VPC reconcilers) decide whether to - // re-request a lease, reschedule, or fail the pending API call. - TopicLeaseExpired = "vpc.dhcp.lease-expired" -) - -// KV bucket for persisted lease state. Keyed by ClientID. Separate from -// the external IPAM allocation ledger by design: IPAM records who owns -// which IP, this bucket records when a lease expires and how to renew it. -const ( - KVBucketDHCPLeases = "spinifex-dhcp-leases" - KVBucketDHCPLeasesVersion = 1 -) - -// AcquireRequestMsg is the NATS payload for TopicAcquire. -type AcquireRequestMsg struct { - Bridge string `json:"bridge"` - ClientID string `json:"client_id"` - Hostname string `json:"hostname"` - VendorClass string `json:"vendor_class"` - // HWAddr is the MAC to put in the DHCP payload chaddr, formatted as - // 02:aa:bb:cc:dd:ee. Empty means the vpcd Manager derives a synthetic - // MAC from ClientID. - HWAddr string `json:"hw_addr,omitempty"` - PoolName string `json:"pool_name,omitempty"` -} - -// AcquireReplyMsg is the NATS reply for TopicAcquire. Error is the only -// field set on failure. -type AcquireReplyMsg struct { - IP string `json:"ip,omitempty"` - SubnetMask string `json:"subnet_mask,omitempty"` - Routers []string `json:"routers,omitempty"` - DNS []string `json:"dns,omitempty"` - ServerID string `json:"server_id,omitempty"` - HWAddr string `json:"hw_addr,omitempty"` - ExpiresUnix int64 `json:"expires_unix,omitempty"` - Error string `json:"error,omitempty"` -} - -// ReleaseRequestMsg is the NATS payload for TopicRelease. ClientID must -// match the identifier the lease was acquired with. -type ReleaseRequestMsg struct { - ClientID string `json:"client_id"` -} - -// ReleaseReplyMsg is the NATS reply for TopicRelease. -type ReleaseReplyMsg struct { - Error string `json:"error,omitempty"` -} - -// LeaseExpiredEvent is the payload fan-out on TopicLeaseExpired. -type LeaseExpiredEvent struct { - ClientID string `json:"client_id"` - PoolName string `json:"pool_name,omitempty"` - IP string `json:"ip,omitempty"` - // Reason is a short machine-readable code (e.g. "renew_failed", - // "nak", "server_unreachable"). - Reason string `json:"reason"` -} diff --git a/spinifex/services/vpcd/dhcp/nats_client.go b/spinifex/services/vpcd/dhcp/nats_client.go deleted file mode 100644 index 5d24d5652..000000000 --- a/spinifex/services/vpcd/dhcp/nats_client.go +++ /dev/null @@ -1,151 +0,0 @@ -package dhcp - -import ( - "encoding/json" - "fmt" - "log/slog" - "time" - - "github.com/nats-io/nats.go" -) - -// NATSTimeout bounds the request/reply round-trip to vpcd's DHCPManager. -// Picked to comfortably exceed the DORA handshake (15 s default + server -// jitter + ~10 s of slack for NAK + fresh DISCOVER fallbacks). A var rather -// than a const so tests can drive the timeout error path without waiting -// 20 s per table entry. -var NATSTimeout = 30 * time.Second - -// AcquireMaxAttempts caps retry of vpc.dhcp.acquire. 60 attempts × 1 s sleep -// covers a cold-boot vpcd start window where NATS is up but the dhcp.Manager -// hasn't yet subscribed (typical: 5–30 s, pathological: 60 s during full -// reconcile). Tests override to keep timeout-path coverage fast. -var AcquireMaxAttempts = 60 - -// AcquireRetryDelay is the wait between retries. Var for the same reason as -// AcquireMaxAttempts. -var AcquireRetryDelay = 1 * time.Second - -// LeaseResult is the data callers need from a successful acquire. Mirrors -// AcquireReplyMsg but exposed as a typed result so callers don't depend on -// the wire struct. -type LeaseResult struct { - IP string - SubnetMask string - Routers []string - DNS []string - ServerID string - HWAddr string - ExpiresUnix int64 -} - -// RequestAcquire asks vpcd's DHCPManager (over NATS) to acquire a DHCP lease -// on the given bridge, identifying the caller with option 61 (client-id), -// option 12 (hostname) and option 60 (vendor class). Blocks until vpcd -// replies or NATSTimeout expires. The Manager-side handler is idempotent on -// clientID: a second call with the same clientID while a live lease exists -// returns the same lease without a fresh DORA, so CAS retry loops on the -// caller are safe. -func RequestAcquire(nc *nats.Conn, bridge, clientID, hostname, vendorClass, poolName, hwAddr string) (LeaseResult, error) { - if nc == nil { - return LeaseResult{}, fmt.Errorf("DHCP lease: NATS connection is required") - } - if bridge == "" { - return LeaseResult{}, fmt.Errorf("DHCP lease: bridge name is required") - } - if clientID == "" { - return LeaseResult{}, fmt.Errorf("DHCP lease: client ID is required") - } - - req := AcquireRequestMsg{ - Bridge: bridge, - ClientID: clientID, - Hostname: hostname, - VendorClass: vendorClass, - PoolName: poolName, - HWAddr: hwAddr, - } - data, err := json.Marshal(req) - if err != nil { - return LeaseResult{}, fmt.Errorf("marshal dhcp acquire request: %w", err) - } - - var reply AcquireReplyMsg - maxAttempts := AcquireMaxAttempts - - for attempt := 1; attempt <= maxAttempts; attempt++ { - msg, err := nc.Request(TopicAcquire, data, NATSTimeout) - if err != nil { - if attempt == maxAttempts { - return LeaseResult{}, fmt.Errorf( - "dhcp acquire NATS request failed after %d attempts (client %s): %w", - maxAttempts, clientID, err, - ) - } - slog.Warn("dhcp acquire NATS request failed, retrying", - "clientID", clientID, - "attempt", attempt, - "maxAttempts", maxAttempts, - "err", err, - ) - time.Sleep(AcquireRetryDelay) - continue - } - - if err := json.Unmarshal(msg.Data, &reply); err != nil { - return LeaseResult{}, fmt.Errorf("unmarshal dhcp acquire reply: %w", err) - } - if reply.Error != "" { - return LeaseResult{}, fmt.Errorf("dhcp acquire client %s: %s", clientID, reply.Error) - } - break - } - - slog.Info("DHCP lease obtained", - "bridge", bridge, - "client_id", clientID, - "ip", reply.IP, - "server_id", reply.ServerID, - "expires_unix", reply.ExpiresUnix, - ) - - return LeaseResult{ - IP: reply.IP, - SubnetMask: reply.SubnetMask, - Routers: reply.Routers, - DNS: reply.DNS, - ServerID: reply.ServerID, - HWAddr: reply.HWAddr, - ExpiresUnix: reply.ExpiresUnix, - }, nil -} - -// RequestRelease asks vpcd to release the lease identified by clientID. -// Returns nil silently when nc or clientID is empty so callers in clean-up -// paths can invoke it unconditionally. -func RequestRelease(nc *nats.Conn, clientID string) error { - if nc == nil || clientID == "" { - return nil - } - - data, err := json.Marshal(ReleaseRequestMsg{ClientID: clientID}) - if err != nil { - return fmt.Errorf("marshal dhcp release request: %w", err) - } - - msg, err := nc.Request(TopicRelease, data, NATSTimeout) - if err != nil { - return fmt.Errorf("dhcp release NATS request (client %s): %w", clientID, err) - } - - var reply ReleaseReplyMsg - if err := json.Unmarshal(msg.Data, &reply); err != nil { - return fmt.Errorf("unmarshal dhcp release reply: %w", err) - } - if reply.Error != "" { - return fmt.Errorf("dhcp release (client %s): %s", clientID, reply.Error) - } - - slog.Info("DHCP lease released", "client_id", clientID) - return nil -} diff --git a/spinifex/services/vpcd/dhcp/nats_client_test.go b/spinifex/services/vpcd/dhcp/nats_client_test.go deleted file mode 100644 index 7c7c1ff49..000000000 --- a/spinifex/services/vpcd/dhcp/nats_client_test.go +++ /dev/null @@ -1,104 +0,0 @@ -package dhcp - -import ( - "encoding/json" - "testing" - "time" - - "github.com/mulgadc/spinifex/spinifex/testutil" - "github.com/nats-io/nats.go" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestRequestAcquire_RequestTimeout(t *testing.T) { - _, nc := testutil.StartTestNATS(t) - sub, err := nc.Subscribe(TopicAcquire, func(*nats.Msg) {}) - require.NoError(t, err) - t.Cleanup(func() { _ = sub.Unsubscribe() }) - - prev := NATSTimeout - NATSTimeout = 150 * time.Millisecond - t.Cleanup(func() { NATSTimeout = prev }) - - prevMax := AcquireMaxAttempts - prevDelay := AcquireRetryDelay - AcquireMaxAttempts = 2 - AcquireRetryDelay = 10 * time.Millisecond - t.Cleanup(func() { - AcquireMaxAttempts = prevMax - AcquireRetryDelay = prevDelay - }) - - _, err = RequestAcquire(nc, "br-wan", "eni-timeout", "eni-timeout", "mulga-spinifex", "wan", "") - assert.ErrorContains(t, err, "dhcp acquire NATS request") -} - -func TestRequestRelease_RequestTimeout(t *testing.T) { - _, nc := testutil.StartTestNATS(t) - sub, err := nc.Subscribe(TopicRelease, func(*nats.Msg) {}) - require.NoError(t, err) - t.Cleanup(func() { _ = sub.Unsubscribe() }) - - prev := NATSTimeout - NATSTimeout = 150 * time.Millisecond - t.Cleanup(func() { NATSTimeout = prev }) - - err = RequestRelease(nc, "eni-timeout") - assert.ErrorContains(t, err, "dhcp release NATS request") -} - -func TestRequestAcquire_MalformedReply(t *testing.T) { - _, nc := testutil.StartTestNATS(t) - sub, err := nc.Subscribe(TopicAcquire, func(msg *nats.Msg) { - _ = msg.Respond([]byte("not json")) - }) - require.NoError(t, err) - t.Cleanup(func() { _ = sub.Unsubscribe() }) - - _, err = RequestAcquire(nc, "br-wan", "eni-malformed", "eni-malformed", "mulga-spinifex", "wan", "") - assert.ErrorContains(t, err, "unmarshal dhcp acquire reply") -} - -func TestRequestRelease_MalformedReply(t *testing.T) { - _, nc := testutil.StartTestNATS(t) - sub, err := nc.Subscribe(TopicRelease, func(msg *nats.Msg) { - _ = msg.Respond([]byte("not json")) - }) - require.NoError(t, err) - t.Cleanup(func() { _ = sub.Unsubscribe() }) - - err = RequestRelease(nc, "eni-malformed") - assert.ErrorContains(t, err, "unmarshal dhcp release reply") -} - -func TestRequestRelease_ReplyError(t *testing.T) { - _, nc := testutil.StartTestNATS(t) - sub, err := nc.Subscribe(TopicRelease, func(msg *nats.Msg) { - data, _ := json.Marshal(ReleaseReplyMsg{Error: "vpcd rejected release"}) - _ = msg.Respond(data) - }) - require.NoError(t, err) - t.Cleanup(func() { _ = sub.Unsubscribe() }) - - err = RequestRelease(nc, "eni-rejected") - assert.ErrorContains(t, err, "vpcd rejected release") -} - -func TestRequestAcquire_Guards(t *testing.T) { - _, err := RequestAcquire(nil, "br-wan", "eni-1", "eni-1", "mulga-spinifex", "wan", "") - assert.ErrorContains(t, err, "NATS connection is required") - - _, nc := testutil.StartTestNATS(t) - _, err = RequestAcquire(nc, "", "eni-1", "eni-1", "mulga-spinifex", "wan", "") - assert.ErrorContains(t, err, "bridge name is required") - - _, err = RequestAcquire(nc, "br-wan", "", "eni-1", "mulga-spinifex", "wan", "") - assert.ErrorContains(t, err, "client ID is required") -} - -func TestRequestRelease_NoopWhenNilOrEmpty(t *testing.T) { - assert.NoError(t, RequestRelease(nil, "eni-1")) - _, nc := testutil.StartTestNATS(t) - assert.NoError(t, RequestRelease(nc, "")) -} diff --git a/spinifex/services/vpcd/dhcp/nclient4.go b/spinifex/services/vpcd/dhcp/nclient4.go deleted file mode 100644 index 3f5e9ff5c..000000000 --- a/spinifex/services/vpcd/dhcp/nclient4.go +++ /dev/null @@ -1,188 +0,0 @@ -package dhcp - -import ( - "context" - "fmt" - "time" - - "github.com/insomniacslk/dhcp/dhcpv4" - "github.com/insomniacslk/dhcp/dhcpv4/nclient4" -) - -// 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 -// handshake and closes it when done — no long-lived per-lease process. -// -// Retransmission lives one layer up in DHCPManager.acquireWithBackoff -// (RFC 2131 §4.1). This client always does exactly one DISCOVER attempt; -// the per-attempt window is whatever the caller's context allows. -type NClient4Client struct { - timeout time.Duration -} - -var _ Client = (*NClient4Client)(nil) - -// NewNClient4 creates an NClient4Client. timeout is the nclient4-internal -// socket read deadline used as a safety net when the caller supplies no -// context deadline; in production DHCPManager always wraps each call in a -// per-attempt ctx so this is rarely hit. The retry argument is accepted -// for backwards compat and ignored — DHCPManager.acquireWithBackoff owns -// retransmission. -func NewNClient4(timeout time.Duration, _ int) *NClient4Client { - if timeout <= 0 { - timeout = 60 * time.Second - } - return &NClient4Client{timeout: timeout} -} - -func (c *NClient4Client) Acquire(ctx context.Context, req AcquireRequest) (*Lease, error) { - if req.Bridge == "" { - return nil, fmt.Errorf("dhcp acquire: bridge is required") - } - if len(req.HWAddr) == 0 { - return nil, fmt.Errorf("dhcp acquire: hw_addr is required") - } - - client, err := nclient4.New(req.Bridge, - nclient4.WithHWAddr(req.HWAddr), - nclient4.WithTimeout(c.timeout), - // Single attempt per call — DHCPManager.acquireWithBackoff drives - // retransmission per RFC 2131 §4.1. - nclient4.WithRetry(1), - ) - if err != nil { - return nil, fmt.Errorf("open nclient4 on %s: %w", req.Bridge, err) - } - defer func() { _ = client.Close() }() - - // Without the broadcast flag, the server sends a unicast OFFER to the - // generated chaddr MAC. The physical NIC drops it in hardware (not its MAC), - // so the AF_PACKET socket on the bridge never sees the frame. Setting the - // broadcast flag forces the server to respond with ff:ff:ff:ff:ff:ff, which - // all NICs accept unconditionally. - mods := append(identityModifiers(req.ClientID, req.Hostname, req.VendorClass), dhcpv4.WithBroadcast(true)) - lease, err := client.Request(ctx, mods...) - if err != nil { - return nil, fmt.Errorf("dhcp DORA on %s (client=%s): %w", req.Bridge, req.ClientID, err) - } - return leaseFromNClient4(req, lease), nil -} - -func (c *NClient4Client) Renew(ctx context.Context, lease *Lease) (*Lease, error) { - if lease == nil { - return nil, fmt.Errorf("dhcp renew: lease is nil") - } - nclient4Lease, err := reconstructNClient4Lease(lease) - if err != nil { - return nil, fmt.Errorf("dhcp renew: %w", err) - } - - client, err := nclient4.New(lease.Bridge, - nclient4.WithHWAddr(lease.HWAddr), - nclient4.WithTimeout(c.timeout), - nclient4.WithRetry(1), - ) - if err != nil { - return nil, fmt.Errorf("open nclient4 on %s for renew: %w", lease.Bridge, err) - } - defer func() { _ = client.Close() }() - - renewed, err := client.Renew(ctx, nclient4Lease, - identityModifiers(lease.ClientID, lease.Hostname, lease.VendorClass)...) - if err != nil { - return nil, fmt.Errorf("dhcp renew on %s (client=%s): %w", lease.Bridge, lease.ClientID, err) - } - - return leaseFromNClient4(AcquireRequest{ - Bridge: lease.Bridge, - ClientID: lease.ClientID, - Hostname: lease.Hostname, - VendorClass: lease.VendorClass, - HWAddr: lease.HWAddr, - }, renewed), nil -} - -func (c *NClient4Client) Release(ctx context.Context, lease *Lease) error { - if lease == nil { - return nil - } - nclient4Lease, err := reconstructNClient4Lease(lease) - if err != nil { - return fmt.Errorf("dhcp release: %w", err) - } - - client, err := nclient4.New(lease.Bridge, - nclient4.WithHWAddr(lease.HWAddr), - nclient4.WithTimeout(c.timeout), - ) - if err != nil { - return fmt.Errorf("open nclient4 on %s for release: %w", lease.Bridge, err) - } - defer func() { _ = client.Close() }() - - if err := client.Release(nclient4Lease, - dhcpv4.WithOption(dhcpv4.OptClientIdentifier([]byte(lease.ClientID)))); err != nil { - return fmt.Errorf("dhcp release on %s (client=%s): %w", lease.Bridge, lease.ClientID, err) - } - return nil -} - -// identityModifiers builds the three identifying DHCP options we set on -// every outbound message: option 61 (client-id), option 12 (hostname), -// option 60 (vendor class). -func identityModifiers(clientID, hostname, vendorClass string) []dhcpv4.Modifier { - var mods []dhcpv4.Modifier - if clientID != "" { - mods = append(mods, dhcpv4.WithOption(dhcpv4.OptClientIdentifier([]byte(clientID)))) - } - if hostname != "" { - mods = append(mods, dhcpv4.WithOption(dhcpv4.OptHostName(hostname))) - } - if vendorClass != "" { - mods = append(mods, dhcpv4.WithOption(dhcpv4.OptClassIdentifier(vendorClass))) - } - return mods -} - -func leaseFromNClient4(req AcquireRequest, in *nclient4.Lease) *Lease { - ack := in.ACK - leaseTime := ack.IPAddressLeaseTime(24 * time.Hour) - return &Lease{ - Bridge: req.Bridge, - ClientID: req.ClientID, - Hostname: req.Hostname, - VendorClass: req.VendorClass, - HWAddr: req.HWAddr, - IP: ack.YourIPAddr, - SubnetMask: ack.SubnetMask(), - Routers: ack.Router(), - DNS: ack.DNS(), - ServerID: ack.ServerIdentifier(), - AcquiredAt: in.CreationTime, - LeaseDuration: leaseTime, - T1: ack.IPAddressRenewalTime(leaseTime / 2), - T2: ack.IPAddressRebindingTime(leaseTime * 7 / 8), - RawOffer: in.Offer.ToBytes(), - RawACK: ack.ToBytes(), - } -} - -func reconstructNClient4Lease(l *Lease) (*nclient4.Lease, error) { - if len(l.RawOffer) == 0 || len(l.RawACK) == 0 { - return nil, fmt.Errorf("lease is missing raw offer/ack bytes; renewal/release not possible") - } - offer, err := dhcpv4.FromBytes(l.RawOffer) - if err != nil { - return nil, fmt.Errorf("parse stored offer: %w", err) - } - ack, err := dhcpv4.FromBytes(l.RawACK) - if err != nil { - return nil, fmt.Errorf("parse stored ack: %w", err) - } - return &nclient4.Lease{ - Offer: offer, - ACK: ack, - CreationTime: l.AcquiredAt, - }, nil -} diff --git a/spinifex/services/vpcd/dhcp_manager.go b/spinifex/services/vpcd/dhcp_manager.go deleted file mode 100644 index e41121106..000000000 --- a/spinifex/services/vpcd/dhcp_manager.go +++ /dev/null @@ -1,865 +0,0 @@ -package vpcd - -import ( - "context" - "encoding/json" - "errors" - "fmt" - "log/slog" - "math/rand/v2" - "net" - "strings" - "sync" - "time" - - "github.com/mulgadc/spinifex/spinifex/migrate" - "github.com/mulgadc/spinifex/spinifex/services/vpcd/dhcp" - "github.com/mulgadc/spinifex/spinifex/utils" - "github.com/nats-io/nats.go" -) - -// DHCPManager owns the active DHCP leases held by spinifex-vpcd. It -// services vpc.dhcp.acquire / vpc.dhcp.release NATS requests from the -// daemon-side ExternalIPAM handlers, runs a per-lease renewal goroutine, -// and persists each lease in the spinifex-dhcp-leases KV bucket so that -// a vpcd restart does not forget what has already been allocated upstream. -type DHCPManager struct { - client dhcp.Client - kv nats.KeyValue - nc *nats.Conn - - // macForClientID produces the chaddr for a request that came in with - // HWAddr unset. Defaults to generateMAC; tests override. - macForClientID func(clientID string) net.HardwareAddr - - // acquireTimeout is the wallclock budget for a full Acquire — sum of - // every retransmit attempt in acquireBackoff plus slack. Renew/Release - // reuse this as their single-attempt deadline. - acquireTimeout time.Duration - - // acquireBackoff drives the per-DISCOVER timeouts used by - // acquireWithBackoff (RFC 2131 §4.1 retransmission). Each entry is - // the wait-for-OFFER window for one attempt; the next entry doubles. - // Default: 4s, 8s, 16s, 32s. ±1s jitter applied per attempt. - acquireBackoff []time.Duration - - // jitterFraction is the ± range applied to renewal sleeps, expressed - // as a fraction of T1 (0.1 = ±10%). - jitterFraction float64 - rng *rand.Rand - - // promiscOn/Off are called to toggle IFF_PROMISC on the DHCP bridge - // around each Acquire. nil = no-op (non-veth modes, or tests). - // promiscCount tracks in-flight acquires so the bridge is not cleared - // while a concurrent DORA is still waiting for a reply. - promiscOn func() - promiscOff func() - promiscMu sync.Mutex - promiscCount int - - mu sync.Mutex - leases map[string]*managedLease // keyed by ClientID - - // inFlightMu guards inFlight. Tracks concurrent Acquire calls per - // bridge so handleAcquire can surface contention as a diagnostic - // signal (mulga-siv-32). - inFlightMu sync.Mutex - inFlight map[string]int - - stopCtx context.Context - stopCancel context.CancelFunc - wg sync.WaitGroup -} - -type managedLease struct { - lease *dhcp.Lease - renewStop context.CancelFunc - renewDone chan struct{} -} - -// DHCPManagerOption configures DHCPManager at construction. -type DHCPManagerOption func(*DHCPManager) - -// WithDHCPMACFunc overrides the default MAC derivation (generateMAC). -func WithDHCPMACFunc(fn func(clientID string) net.HardwareAddr) DHCPManagerOption { - return func(m *DHCPManager) { m.macForClientID = fn } -} - -// WithDHCPAcquireTimeout overrides the wallclock budget for a full Acquire -// (sum of every retransmit attempt + slack). Renew/Release reuse it. -func WithDHCPAcquireTimeout(d time.Duration) DHCPManagerOption { - return func(m *DHCPManager) { m.acquireTimeout = d } -} - -// WithDHCPAcquireBackoff overrides the per-DISCOVER timeout schedule used -// by acquireWithBackoff. Tests use a tight schedule; production keeps the -// RFC 2131 §4.1 default (4s, 8s, 16s, 32s with ±1s jitter). -func WithDHCPAcquireBackoff(b []time.Duration) DHCPManagerOption { - return func(m *DHCPManager) { m.acquireBackoff = b } -} - -// WithDHCPJitterFraction overrides the ±fraction applied to renewal sleeps. -func WithDHCPJitterFraction(f float64) DHCPManagerOption { - return func(m *DHCPManager) { m.jitterFraction = f } -} - -// WithDHCPRand overrides the random source used for jitter (tests only). -func WithDHCPRand(r *rand.Rand) DHCPManagerOption { - return func(m *DHCPManager) { m.rng = r } -} - -// WithDHCPPromiscFuncs wires IFF_PROMISC toggling around each Acquire. on is -// called before the DORA and off is called after (even on error). Both are -// reference-counted so concurrent acquires keep the bridge promisc until the -// last one completes. Pass nil to disable (default). -func WithDHCPPromiscFuncs(on, off func()) DHCPManagerOption { - return func(m *DHCPManager) { - m.promiscOn = on - m.promiscOff = off - } -} - -// NewDHCPManager creates a Manager wired against js for KV persistence and -// nc for NATS request/reply. The caller still needs to call Bootstrap (to -// rehydrate persisted leases) and Subscribe (to start servicing requests). -func NewDHCPManager(nc *nats.Conn, js nats.JetStreamContext, client dhcp.Client, opts ...DHCPManagerOption) (*DHCPManager, error) { - if nc == nil { - return nil, fmt.Errorf("DHCPManager: nats.Conn is required") - } - if js == nil { - return nil, fmt.Errorf("DHCPManager: JetStreamContext is required") - } - if client == nil { - return nil, fmt.Errorf("DHCPManager: dhcp.Client is required") - } - - kv, err := utils.GetOrCreateKVBucket(js, dhcp.KVBucketDHCPLeases, 3) - if err != nil { - return nil, fmt.Errorf("create DHCP lease KV bucket: %w", err) - } - if err := migrate.DefaultRegistry.RunKV(dhcp.KVBucketDHCPLeases, kv, dhcp.KVBucketDHCPLeasesVersion); err != nil { - return nil, fmt.Errorf("migrate %s: %w", dhcp.KVBucketDHCPLeases, err) - } - - ctx, cancel := context.WithCancel(context.Background()) - m := &DHCPManager{ - client: client, - kv: kv, - nc: nc, - // 90s = sum of default acquireBackoff (4+8+16+32 = 60s) + slack for - // socket open/close on each attempt and the final ACK round-trip. - acquireTimeout: 90 * time.Second, - acquireBackoff: []time.Duration{ - 4 * time.Second, - 8 * time.Second, - 16 * time.Second, - 32 * time.Second, - }, - jitterFraction: 0.1, - rng: rand.New(rand.NewPCG(uint64(time.Now().UnixNano()), 0xdeadbeef)), //nolint:gosec // non-cryptographic jitter - leases: map[string]*managedLease{}, - inFlight: map[string]int{}, - stopCtx: ctx, - stopCancel: cancel, - macForClientID: func(id string) net.HardwareAddr { - mac, err := net.ParseMAC(generateMAC(id)) - if err != nil { - return nil - } - return mac - }, - } - for _, opt := range opts { - opt(m) - } - return m, nil -} - -// Bootstrap reads every persisted lease from KV and schedules the -// renewal goroutine. Leases already past their expiry are removed and -// a TopicLeaseExpired event is published so the reconcilers upstream -// know they need to re-request. -func (m *DHCPManager) Bootstrap(ctx context.Context) error { - keys, err := m.kv.Keys() - if err != nil { - if errors.Is(err, nats.ErrNoKeysFound) { - slog.Info("DHCP Manager bootstrapped", "leases", 0) - return nil - } - return fmt.Errorf("list DHCP lease keys: %w", err) - } - - now := time.Now() - resumed, expired := 0, 0 - for _, key := range keys { - if key == "_version" { - continue - } - entry, err := m.kv.Get(key) - if err != nil { - slog.Warn("DHCP Manager: failed to load lease", "client_id", key, "err", err) - continue - } - lease, err := decodeLease(entry.Value()) - if err != nil { - slog.Warn("DHCP Manager: failed to decode lease", "client_id", key, "err", err) - continue - } - - if now.After(lease.ExpiresAt()) { - _ = m.kv.Delete(key) - m.publishExpired(lease, "", "bootstrap_past_expiry") - expired++ - continue - } - - m.trackAndRenew(lease) - resumed++ - } - slog.Info("DHCP Manager bootstrapped", "leases", resumed, "expired", expired) - return nil -} - -// Subscribe registers queue-subscribed handlers on vpc.dhcp.acquire and -// vpc.dhcp.release. Queue group "vpcd-dhcp-workers" matches the existing -// vpcd-workers convention. -func (m *DHCPManager) Subscribe(nc *nats.Conn) ([]*nats.Subscription, error) { - var subs []*nats.Subscription - - acquire, err := nc.QueueSubscribe(dhcp.TopicAcquire, "vpcd-dhcp-workers", m.handleAcquire) - if err != nil { - return nil, fmt.Errorf("subscribe %s: %w", dhcp.TopicAcquire, err) - } - subs = append(subs, acquire) - slog.Info("DHCP Manager subscribed", "topic", dhcp.TopicAcquire) - - release, err := nc.QueueSubscribe(dhcp.TopicRelease, "vpcd-dhcp-workers", m.handleRelease) - if err != nil { - _ = acquire.Unsubscribe() - return nil, fmt.Errorf("subscribe %s: %w", dhcp.TopicRelease, err) - } - subs = append(subs, release) - slog.Info("DHCP Manager subscribed", "topic", dhcp.TopicRelease) - - return subs, nil -} - -// Close stops all renewal goroutines and waits for them to exit. -func (m *DHCPManager) Close() { - m.stopCancel() - m.wg.Wait() -} - -// handleAcquire processes a vpc.dhcp.acquire request. Idempotent: if we -// already hold a valid lease for the requested ClientID we return it -// without a fresh DORA. Lets the daemon-side CAS retry loop be safe. -func (m *DHCPManager) handleAcquire(msg *nats.Msg) { - var req dhcp.AcquireRequestMsg - if err := json.Unmarshal(msg.Data, &req); err != nil { - m.replyAcquireError(msg, fmt.Sprintf("invalid acquire request: %v", err)) - return - } - if req.Bridge == "" || req.ClientID == "" { - m.replyAcquireError(msg, "acquire: bridge and client_id are required") - return - } - - if existing, ok := m.get(req.ClientID); ok { - if time.Now().Before(existing.ExpiresAt()) { - m.replyAcquire(msg, existing) - return - } - m.forget(req.ClientID, "stale_existing") - } - - hwAddr, err := resolveHWAddr(req.HWAddr, req.ClientID, m.macForClientID) - if err != nil { - m.replyAcquireError(msg, err.Error()) - return - } - - m.enablePromisc() - defer m.disablePromisc() - - ctx, cancel := context.WithTimeout(m.stopCtx, m.acquireTimeout) - defer cancel() - - // Diagnostic for mulga-siv-32: track concurrent Acquire calls per - // bridge and the wallclock cost of each DORA. Concurrent DISCOVERs on - // the same bridge are a suspected contributor to nightly DORA timeouts. - inFlight := m.beginAcquire(req.Bridge) - defer m.endAcquire(req.Bridge) - start := time.Now() - - lease, err := m.acquireWithBackoff(ctx, dhcp.AcquireRequest{ - Bridge: req.Bridge, - ClientID: req.ClientID, - Hostname: req.Hostname, - VendorClass: req.VendorClass, - HWAddr: hwAddr, - }) - if err != nil { - slog.Warn("DHCP Manager: acquire failed", - "client_id", req.ClientID, "bridge", req.Bridge, - "in_flight_at_start", inFlight, "duration", time.Since(start), - "err", err) - m.replyAcquireError(msg, err.Error()) - return - } - slog.Debug("DHCP Manager: acquire ok", - "client_id", req.ClientID, "bridge", req.Bridge, - "in_flight_at_start", inFlight, "duration", time.Since(start)) - - if err := m.persistLease(lease); err != nil { - slog.Warn("DHCP Manager: persist lease failed; lease will not survive restart", "client_id", req.ClientID, "err", err) - } - m.trackAndRenew(lease) - - slog.Info("DHCP Manager acquired lease", - "client_id", lease.ClientID, "ip", lease.IP.String(), - "bridge", lease.Bridge, "server_id", lease.ServerID.String(), - "expires_in", time.Until(lease.ExpiresAt())) - - m.replyAcquire(msg, lease) -} - -// handleRelease processes a vpc.dhcp.release request. -func (m *DHCPManager) handleRelease(msg *nats.Msg) { - var req dhcp.ReleaseRequestMsg - if err := json.Unmarshal(msg.Data, &req); err != nil { - m.replyRelease(msg, fmt.Sprintf("invalid release request: %v", err)) - return - } - if req.ClientID == "" { - m.replyRelease(msg, "release: client_id is required") - return - } - - lease, ok := m.get(req.ClientID) - if !ok { - // No active lease; treat as a no-op so the daemon can call release - // in clean-up paths without worrying about order. - m.replyRelease(msg, "") - return - } - - ctx, cancel := context.WithTimeout(m.stopCtx, m.acquireTimeout) - defer cancel() - if err := m.client.Release(ctx, lease); err != nil { - slog.Warn("DHCP Manager: release failed", "client_id", req.ClientID, "err", err) - // Still forget locally — upstream server may or may not have seen - // the RELEASE, but holding on to a lease we've been told to drop - // serves no purpose. - } - m.forget(req.ClientID, "released") - slog.Info("DHCP Manager released lease", "client_id", req.ClientID) - m.replyRelease(msg, "") -} - -// enablePromisc increments the in-flight acquire count and enables IFF_PROMISC -// on the first call. -func (m *DHCPManager) enablePromisc() { - if m.promiscOn == nil { - return - } - m.promiscMu.Lock() - defer m.promiscMu.Unlock() - if m.promiscCount == 0 { - m.promiscOn() - } - m.promiscCount++ -} - -// disablePromisc decrements the count and disables IFF_PROMISC when it reaches -// zero (i.e. no more in-flight acquires need it). -func (m *DHCPManager) disablePromisc() { - if m.promiscOff == nil { - return - } - m.promiscMu.Lock() - defer m.promiscMu.Unlock() - m.promiscCount-- - if m.promiscCount == 0 { - m.promiscOff() - } -} - -// trackAndRenew registers the lease in the in-memory map and starts the -// per-lease renewal goroutine. Replaces any prior tracker for the same -// ClientID. -func (m *DHCPManager) trackAndRenew(lease *dhcp.Lease) { - m.mu.Lock() - if prev, ok := m.leases[lease.ClientID]; ok && prev.renewStop != nil { - prev.renewStop() - m.mu.Unlock() - <-prev.renewDone - m.mu.Lock() - } - - // cancel is stored in tracker.renewStop and fired from forget() or Close(). - ctx, cancel := context.WithCancel(m.stopCtx) - tracker := &managedLease{ - lease: lease, - renewStop: cancel, - renewDone: make(chan struct{}), - } - m.leases[lease.ClientID] = tracker - m.mu.Unlock() - - m.wg.Add(1) - go m.renewLoop(ctx, tracker) -} - -// renewLoop drives one lease's renewal lifecycle. Sleeps until T1 (with -// ±jitter), attempts Renew; on failure retries at T2; on terminal failure -// past the expiry, publishes TopicLeaseExpired and forgets the lease. -func (m *DHCPManager) renewLoop(ctx context.Context, tracker *managedLease) { - defer m.wg.Done() - defer close(tracker.renewDone) - - for { - lease := m.snapshotLease(tracker) - if lease == nil { - return - } - - // Phase 1 — wait for T1 (± jitter) and try RENEW. - wait := m.jitter(time.Until(lease.RenewAt())) - if !sleep(ctx, wait) { - return - } - - renewed, err := m.tryRenew(ctx, lease) - if err == nil { - tracker.lease = renewed - if perr := m.persistLease(renewed); perr != nil { - slog.Warn("DHCP Manager: persist renewed lease failed", "client_id", renewed.ClientID, "err", perr) - } - continue - } - slog.Warn("DHCP Manager: renew failed, will retry at T2", "client_id", lease.ClientID, "err", err) - - // Phase 2 — retry until T2. - wait = m.jitter(time.Until(lease.RebindAt())) - if !sleep(ctx, wait) { - return - } - renewed, err = m.tryRenew(ctx, lease) - if err == nil { - tracker.lease = renewed - if perr := m.persistLease(renewed); perr != nil { - slog.Warn("DHCP Manager: persist rebound lease failed", "client_id", renewed.ClientID, "err", perr) - } - continue - } - - // Phase 3 — wait out the rest of the lease; on expiry, emit - // lease-expired and drop. - wait = time.Until(lease.ExpiresAt()) - if !sleep(ctx, wait) { - return - } - slog.Warn("DHCP Manager: lease expired without renewal", "client_id", lease.ClientID, "ip", lease.IP.String(), "err", err) - m.publishExpired(lease, "", classifyRenewErr(err)) - m.forget(lease.ClientID, "expired") - return - } -} - -// acquireWithBackoff drives RFC 2131 §4.1 DISCOVER retransmission. Each -// entry in acquireBackoff sets one attempt's per-OFFER wait window with -// ±1s jitter; on timeout the loop tries the next entry. Returns on first -// success, on parent ctx cancellation, or after the schedule is exhausted. -// -// Real DHCP servers (consumer routers, ISC dhcpd, dnsmasq) silently drop -// OFFERs under load — without retransmission a single lost packet kills -// the whole DORA. mulga-siv-39. -func (m *DHCPManager) acquireWithBackoff(parent context.Context, req dhcp.AcquireRequest) (*dhcp.Lease, error) { - schedule := m.acquireBackoff - if len(schedule) == 0 { - // Defensive: caller passed an empty schedule via WithDHCPAcquireBackoff. - // Fall through to a single attempt bounded by parent ctx. - schedule = []time.Duration{0} - } - - var lastErr error - for i, base := range schedule { - if err := parent.Err(); err != nil { - if lastErr != nil { - return nil, fmt.Errorf("dhcp acquire on %s (client=%s): %w (last attempt err: %v)", req.Bridge, req.ClientID, err, lastErr) - } - return nil, fmt.Errorf("dhcp acquire on %s (client=%s): %w", req.Bridge, req.ClientID, err) - } - - attemptTimeout := m.dhcpAttemptTimeout(base) - var ( - ctx context.Context - cancel context.CancelFunc - ) - if attemptTimeout > 0 { - ctx, cancel = context.WithTimeout(parent, attemptTimeout) - } else { - ctx, cancel = context.WithCancel(parent) - } - - lease, err := m.client.Acquire(ctx, req) - cancel() - if err == nil { - return lease, nil - } - lastErr = err - - if i < len(schedule)-1 { - slog.Debug("DHCP Manager: DORA retransmit", - "client_id", req.ClientID, "bridge", req.Bridge, - "attempt", i+1, "next_timeout", m.dhcpAttemptTimeout(schedule[i+1]), - "prev_err", err) - } - } - return nil, lastErr -} - -// dhcpAttemptTimeout applies ±1s jitter to a base DISCOVER timeout. -// Pass 0 to disable timeout (used when schedule is empty fallback). -func (m *DHCPManager) dhcpAttemptTimeout(base time.Duration) time.Duration { - if base <= 0 { - return 0 - } - jitter := time.Duration((m.rng.Float64()*2 - 1) * float64(time.Second)) - out := base + jitter - if out <= 0 { - return base - } - return out -} - -// tryRenew calls the backend Renew under a bounded context. -func (m *DHCPManager) tryRenew(parent context.Context, lease *dhcp.Lease) (*dhcp.Lease, error) { - ctx, cancel := context.WithTimeout(parent, m.acquireTimeout) - defer cancel() - return m.client.Renew(ctx, lease) -} - -// beginAcquire increments the in-flight Acquire counter for bridge and -// returns the count after increment (>= 1). Diagnostic for mulga-siv-32. -func (m *DHCPManager) beginAcquire(bridge string) int { - m.inFlightMu.Lock() - defer m.inFlightMu.Unlock() - m.inFlight[bridge]++ - return m.inFlight[bridge] -} - -// endAcquire decrements the in-flight Acquire counter for bridge. -func (m *DHCPManager) endAcquire(bridge string) { - m.inFlightMu.Lock() - defer m.inFlightMu.Unlock() - if m.inFlight[bridge] > 0 { - m.inFlight[bridge]-- - } -} - -// snapshotLease returns the tracker's current lease under the lock. -func (m *DHCPManager) snapshotLease(tracker *managedLease) *dhcp.Lease { - m.mu.Lock() - defer m.mu.Unlock() - return tracker.lease -} - -// get returns the current lease for a ClientID, if tracked. -func (m *DHCPManager) get(clientID string) (*dhcp.Lease, bool) { - m.mu.Lock() - defer m.mu.Unlock() - t, ok := m.leases[clientID] - if !ok { - return nil, false - } - return t.lease, true -} - -// forget removes a lease from the tracking map, KV and stops its renewal -// goroutine. -func (m *DHCPManager) forget(clientID, reason string) { - m.mu.Lock() - t, ok := m.leases[clientID] - if ok { - delete(m.leases, clientID) - } - m.mu.Unlock() - - if ok && t.renewStop != nil { - t.renewStop() - } - if err := m.kv.Delete(clientID); err != nil && !errors.Is(err, nats.ErrKeyNotFound) { - slog.Debug("DHCP Manager: delete KV key failed", "client_id", clientID, "reason", reason, "err", err) - } -} - -// persistLease writes the lease state to the KV bucket under ClientID. -func (m *DHCPManager) persistLease(lease *dhcp.Lease) error { - data, err := encodeLease(lease) - if err != nil { - return err - } - if _, err := m.kv.Put(lease.ClientID, data); err != nil { - return fmt.Errorf("put lease %s: %w", lease.ClientID, err) - } - return nil -} - -func (m *DHCPManager) publishExpired(lease *dhcp.Lease, poolName, reason string) { - evt := dhcp.LeaseExpiredEvent{ - ClientID: lease.ClientID, - PoolName: poolName, - IP: lease.IP.String(), - Reason: reason, - } - data, err := json.Marshal(evt) - if err != nil { - slog.Warn("DHCP Manager: marshal lease-expired event failed", "client_id", lease.ClientID, "err", err) - return - } - if err := m.nc.Publish(dhcp.TopicLeaseExpired, data); err != nil { - slog.Warn("DHCP Manager: publish lease-expired failed", "client_id", lease.ClientID, "err", err) - } -} - -func (m *DHCPManager) replyAcquire(msg *nats.Msg, lease *dhcp.Lease) { - reply := dhcp.AcquireReplyMsg{ - IP: lease.IP.String(), - SubnetMask: net.IP(lease.SubnetMask).String(), - Routers: ipsToStrings(lease.Routers), - DNS: ipsToStrings(lease.DNS), - ServerID: lease.ServerID.String(), - HWAddr: lease.HWAddr.String(), - ExpiresUnix: lease.ExpiresAt().Unix(), - } - respondJSON(msg, reply) -} - -func (m *DHCPManager) replyAcquireError(msg *nats.Msg, errMsg string) { - respondJSON(msg, dhcp.AcquireReplyMsg{Error: errMsg}) -} - -func (m *DHCPManager) replyRelease(msg *nats.Msg, errMsg string) { - respondJSON(msg, dhcp.ReleaseReplyMsg{Error: errMsg}) -} - -// jitter applies ±jitterFraction of d to produce an actual sleep duration. -// Negative d is clamped to 0. -func (m *DHCPManager) jitter(d time.Duration) time.Duration { - if d <= 0 { - return 0 - } - if m.jitterFraction <= 0 { - return d - } - spread := float64(d) * m.jitterFraction - offset := (m.rng.Float64()*2 - 1) * spread - out := time.Duration(float64(d) + offset) - if out < 0 { - return 0 - } - return out -} - -// resolveHWAddr parses the hw_addr string supplied in the NATS request, -// falling back to a derived MAC when the request carries no value. -func resolveHWAddr(raw, clientID string, derive func(string) net.HardwareAddr) (net.HardwareAddr, error) { - if raw != "" { - mac, err := net.ParseMAC(raw) - if err != nil { - return nil, fmt.Errorf("parse hw_addr %q: %w", raw, err) - } - return mac, nil - } - if derive == nil { - return nil, fmt.Errorf("hw_addr is empty and no derivation function configured") - } - mac := derive(clientID) - if len(mac) == 0 { - return nil, fmt.Errorf("derive hw_addr for client %q: empty result", clientID) - } - return mac, nil -} - -// classifyRenewErr maps a renew error to a short reason code on the -// lease-expired event. Kept coarse; callers that care about specifics -// can re-log from their side. -func classifyRenewErr(err error) string { - if err == nil { - return "unknown" - } - // nclient4 returns *nclient4.ErrNak on NAK. Rather than taking a - // dependency on its private type here, match the known message. - msg := err.Error() - if strings.Contains(msg, "NAK") || strings.Contains(msg, "Nak") { - return "nak" - } - if strings.Contains(msg, "timeout") || strings.Contains(msg, "deadline exceeded") || strings.Contains(msg, "i/o timeout") { - return "server_unreachable" - } - return "renew_failed" -} - -func ipsToStrings(ips []net.IP) []string { - if len(ips) == 0 { - return nil - } - out := make([]string, 0, len(ips)) - for _, ip := range ips { - out = append(out, ip.String()) - } - return out -} - -// respondJSON marshals v and sends it as the reply. If the message has -// no Reply subject (fire-and-forget), drops the reply silently. -func respondJSON(msg *nats.Msg, v any) { - if msg.Reply == "" { - return - } - data, err := json.Marshal(v) - if err != nil { - slog.Warn("DHCP Manager: marshal reply failed", "err", err) - return - } - if err := msg.Respond(data); err != nil { - slog.Warn("DHCP Manager: respond failed", "err", err) - } -} - -// sleep blocks until d elapses or ctx is done. Returns false if ctx was -// cancelled (caller should exit). -func sleep(ctx context.Context, d time.Duration) bool { - if d <= 0 { - select { - case <-ctx.Done(): - return false - default: - return true - } - } - t := time.NewTimer(d) - defer t.Stop() - select { - case <-ctx.Done(): - return false - case <-t.C: - return true - } -} - -// --- KV encoding --- - -// leaseRecord is the JSON representation persisted in KV and used to -// rehydrate on vpcd startup. Explicit format rather than gob-encoding -// *dhcp.Lease so a future Go type change doesn't invalidate every -// in-flight lease. -type leaseRecord struct { - Version int `json:"version"` - Bridge string `json:"bridge"` - ClientID string `json:"client_id"` - Hostname string `json:"hostname,omitempty"` - VendorClass string `json:"vendor_class,omitempty"` - HWAddr string `json:"hw_addr"` - IP string `json:"ip"` - SubnetMask string `json:"subnet_mask,omitempty"` - Routers []string `json:"routers,omitempty"` - DNS []string `json:"dns,omitempty"` - ServerID string `json:"server_id,omitempty"` - AcquiredUnix int64 `json:"acquired_unix"` - LeaseSeconds int64 `json:"lease_seconds"` - T1Seconds int64 `json:"t1_seconds"` - T2Seconds int64 `json:"t2_seconds"` - RawOfferB64 []byte `json:"raw_offer,omitempty"` - RawACKB64 []byte `json:"raw_ack,omitempty"` -} - -const leaseRecordVersion = 1 - -func encodeLease(l *dhcp.Lease) ([]byte, error) { - rec := leaseRecord{ - Version: leaseRecordVersion, - Bridge: l.Bridge, - ClientID: l.ClientID, - Hostname: l.Hostname, - VendorClass: l.VendorClass, - HWAddr: l.HWAddr.String(), - IP: l.IP.String(), - SubnetMask: maskString(l.SubnetMask), - Routers: ipsToStrings(l.Routers), - DNS: ipsToStrings(l.DNS), - ServerID: l.ServerID.String(), - AcquiredUnix: l.AcquiredAt.Unix(), - LeaseSeconds: int64(l.LeaseDuration / time.Second), - T1Seconds: int64(l.T1 / time.Second), - T2Seconds: int64(l.T2 / time.Second), - RawOfferB64: l.RawOffer, - RawACKB64: l.RawACK, - } - return json.Marshal(rec) -} - -func decodeLease(data []byte) (*dhcp.Lease, error) { - var rec leaseRecord - if err := json.Unmarshal(data, &rec); err != nil { - return nil, fmt.Errorf("unmarshal lease: %w", err) - } - if rec.Version != leaseRecordVersion { - return nil, fmt.Errorf("unsupported lease record version %d", rec.Version) - } - mac, err := net.ParseMAC(rec.HWAddr) - if err != nil { - return nil, fmt.Errorf("parse hw_addr %q: %w", rec.HWAddr, err) - } - return &dhcp.Lease{ - Bridge: rec.Bridge, - ClientID: rec.ClientID, - Hostname: rec.Hostname, - VendorClass: rec.VendorClass, - HWAddr: mac, - IP: net.ParseIP(rec.IP), - SubnetMask: parseMask(rec.SubnetMask), - Routers: parseIPs(rec.Routers), - DNS: parseIPs(rec.DNS), - ServerID: net.ParseIP(rec.ServerID), - AcquiredAt: time.Unix(rec.AcquiredUnix, 0), - LeaseDuration: time.Duration(rec.LeaseSeconds) * time.Second, - T1: time.Duration(rec.T1Seconds) * time.Second, - T2: time.Duration(rec.T2Seconds) * time.Second, - RawOffer: rec.RawOfferB64, - RawACK: rec.RawACKB64, - }, nil -} - -func maskString(m net.IPMask) string { - if len(m) == 0 { - return "" - } - return net.IP(m).String() -} - -func parseMask(s string) net.IPMask { - ip := net.ParseIP(s) - if ip == nil { - return nil - } - ip4 := ip.To4() - if ip4 == nil { - return nil - } - return net.IPv4Mask(ip4[0], ip4[1], ip4[2], ip4[3]) -} - -func parseIPs(ss []string) []net.IP { - if len(ss) == 0 { - return nil - } - out := make([]net.IP, 0, len(ss)) - for _, s := range ss { - if ip := net.ParseIP(s); ip != nil { - out = append(out, ip) - } - } - return out -} diff --git a/spinifex/services/vpcd/dhcp_manager_test.go b/spinifex/services/vpcd/dhcp_manager_test.go deleted file mode 100644 index 3b724da1c..000000000 --- a/spinifex/services/vpcd/dhcp_manager_test.go +++ /dev/null @@ -1,555 +0,0 @@ -package vpcd - -import ( - "context" - "encoding/json" - "errors" - "net" - "sync" - "sync/atomic" - "testing" - "time" - - "github.com/mulgadc/spinifex/spinifex/services/vpcd/dhcp" - "github.com/mulgadc/spinifex/spinifex/testutil" - "github.com/nats-io/nats.go" - "github.com/stretchr/testify/require" -) - -// newTestDHCPManager spins up an embedded JetStream, wires a Fake client, -// and returns the Manager along with the NATS connection. Cleanup of NATS -// is handled by testutil; the Manager is closed via t.Cleanup. -func newTestDHCPManager(t *testing.T, opts ...DHCPManagerOption) (*DHCPManager, *nats.Conn, *dhcp.Fake) { - t.Helper() - _, nc, js := testutil.StartTestJetStream(t) - fake := dhcp.NewFake() - m, err := NewDHCPManager(nc, js, fake, opts...) - require.NoError(t, err) - t.Cleanup(m.Close) - subs, err := m.Subscribe(nc) - require.NoError(t, err) - t.Cleanup(func() { - for _, s := range subs { - _ = s.Unsubscribe() - } - }) - return m, nc, fake -} - -func requestAcquire(t *testing.T, nc *nats.Conn, req dhcp.AcquireRequestMsg) dhcp.AcquireReplyMsg { - t.Helper() - data, err := json.Marshal(req) - require.NoError(t, err) - msg, err := nc.Request(dhcp.TopicAcquire, data, 5*time.Second) - require.NoError(t, err) - var reply dhcp.AcquireReplyMsg - require.NoError(t, json.Unmarshal(msg.Data, &reply)) - return reply -} - -func requestRelease(t *testing.T, nc *nats.Conn, clientID string) dhcp.ReleaseReplyMsg { - t.Helper() - data, err := json.Marshal(dhcp.ReleaseRequestMsg{ClientID: clientID}) - require.NoError(t, err) - msg, err := nc.Request(dhcp.TopicRelease, data, 5*time.Second) - require.NoError(t, err) - var reply dhcp.ReleaseReplyMsg - require.NoError(t, json.Unmarshal(msg.Data, &reply)) - return reply -} - -func TestDHCPManager_AcquireReturnsLease(t *testing.T) { - _, nc, fake := newTestDHCPManager(t) - reply := requestAcquire(t, nc, dhcp.AcquireRequestMsg{ - Bridge: "br-wan", ClientID: "eni-1", Hostname: "eni-1", VendorClass: "i-abc", - }) - require.Empty(t, reply.Error, "unexpected error: %s", reply.Error) - require.Equal(t, "192.0.2.100", reply.IP) - require.NotEmpty(t, reply.ServerID) - require.NotZero(t, reply.ExpiresUnix) - require.NotEmpty(t, reply.HWAddr) - require.Equal(t, 1, fake.AcquireCount()) -} - -func TestDHCPManager_AcquireIdempotentWhileLeaseActive(t *testing.T) { - _, nc, fake := newTestDHCPManager(t) - req := dhcp.AcquireRequestMsg{Bridge: "br-wan", ClientID: "eni-idem"} - - first := requestAcquire(t, nc, req) - require.Empty(t, first.Error) - second := requestAcquire(t, nc, req) - require.Empty(t, second.Error) - - require.Equal(t, first.IP, second.IP, "idempotent acquire should return the same IP") - require.Equal(t, 1, fake.AcquireCount(), "second request must not trigger a fresh DORA") -} - -func TestDHCPManager_AcquireFailureSurfacesError(t *testing.T) { - _, nc, fake := newTestDHCPManager(t) - fake.AcquireHook = func(dhcp.AcquireRequest) (*dhcp.Lease, error) { - return nil, errors.New("server unreachable") - } - reply := requestAcquire(t, nc, dhcp.AcquireRequestMsg{Bridge: "br-wan", ClientID: "eni-err"}) - require.Contains(t, reply.Error, "server unreachable") - require.Empty(t, reply.IP) -} - -func TestDHCPManager_ReleaseRemovesLease(t *testing.T) { - m, nc, fake := newTestDHCPManager(t) - req := dhcp.AcquireRequestMsg{Bridge: "br-wan", ClientID: "eni-release"} - require.Empty(t, requestAcquire(t, nc, req).Error) - - reply := requestRelease(t, nc, req.ClientID) - require.Empty(t, reply.Error) - require.Equal(t, 1, fake.ReleaseCount()) - - _, ok := m.get("eni-release") - require.False(t, ok, "lease should be forgotten after Release") -} - -func TestDHCPManager_ReleaseUnknownIsNoop(t *testing.T) { - _, nc, fake := newTestDHCPManager(t) - reply := requestRelease(t, nc, "nonexistent") - require.Empty(t, reply.Error) - require.Equal(t, 0, fake.ReleaseCount()) -} - -func TestDHCPManager_AcquireRejectsMissingFields(t *testing.T) { - _, nc, _ := newTestDHCPManager(t) - reply := requestAcquire(t, nc, dhcp.AcquireRequestMsg{Bridge: "", ClientID: "eni-x"}) - require.Contains(t, reply.Error, "bridge and client_id are required") -} - -func TestDHCPManager_HWAddrDerivedFromClientID(t *testing.T) { - _, nc, fake := newTestDHCPManager(t) - reply := requestAcquire(t, nc, dhcp.AcquireRequestMsg{Bridge: "br-wan", ClientID: "eni-mac"}) - require.Empty(t, reply.Error) - lease, ok := fake.HeldLease("eni-mac") - require.True(t, ok) - // generateMAC produces a locally-administered unicast MAC. First octet - // is hash-derived; assert the IEEE 802 reserved bits. - require.Equal(t, byte(0x02), lease.HWAddr[0]&0x03) -} - -func TestDHCPManager_HWAddrExplicitlySuppliedIsUsed(t *testing.T) { - _, nc, fake := newTestDHCPManager(t) - reply := requestAcquire(t, nc, dhcp.AcquireRequestMsg{ - Bridge: "br-wan", ClientID: "eni-mac-explicit", HWAddr: "02:aa:bb:cc:dd:ee", - }) - require.Empty(t, reply.Error) - lease, _ := fake.HeldLease("eni-mac-explicit") - require.Equal(t, "02:aa:bb:cc:dd:ee", lease.HWAddr.String()) -} - -func TestDHCPManager_RenewalFiresAndUpdatesLease(t *testing.T) { - tmpl := dhcp.DefaultLeaseTemplate() - tmpl.LeaseDuration = 40 * time.Millisecond - fake := dhcp.NewFake() - fake.SetDefaultLease(tmpl) - - _, nc, js := testutil.StartTestJetStream(t) - m, err := NewDHCPManager(nc, js, fake, WithDHCPJitterFraction(0)) - require.NoError(t, err) - t.Cleanup(m.Close) - subs, err := m.Subscribe(nc) - require.NoError(t, err) - t.Cleanup(func() { - for _, s := range subs { - _ = s.Unsubscribe() - } - }) - - reply := requestAcquire(t, nc, dhcp.AcquireRequestMsg{Bridge: "br-wan", ClientID: "eni-renew"}) - require.Empty(t, reply.Error) - - // T1 = lease/2 = 20 ms. Wait a few multiples to catch at least one renew. - require.Eventually(t, func() bool { return fake.RenewCount() >= 1 }, 2*time.Second, 10*time.Millisecond) -} - -func TestDHCPManager_RenewFailureEmitsLeaseExpired(t *testing.T) { - tmpl := dhcp.DefaultLeaseTemplate() - tmpl.LeaseDuration = 60 * time.Millisecond - fake := dhcp.NewFake() - fake.SetDefaultLease(tmpl) - fake.RenewHook = func(*dhcp.Lease) (*dhcp.Lease, error) { - return nil, errors.New("server NAK") - } - - _, nc, js := testutil.StartTestJetStream(t) - m, err := NewDHCPManager(nc, js, fake, WithDHCPJitterFraction(0)) - require.NoError(t, err) - t.Cleanup(m.Close) - - var events atomic.Int32 - expiredMsg := make(chan dhcp.LeaseExpiredEvent, 1) - sub, err := nc.Subscribe(dhcp.TopicLeaseExpired, func(msg *nats.Msg) { - events.Add(1) - var evt dhcp.LeaseExpiredEvent - if err := json.Unmarshal(msg.Data, &evt); err == nil { - select { - case expiredMsg <- evt: - default: - } - } - }) - require.NoError(t, err) - t.Cleanup(func() { _ = sub.Unsubscribe() }) - - subs, err := m.Subscribe(nc) - require.NoError(t, err) - t.Cleanup(func() { - for _, s := range subs { - _ = s.Unsubscribe() - } - }) - - reply := requestAcquire(t, nc, dhcp.AcquireRequestMsg{Bridge: "br-wan", ClientID: "eni-expire"}) - require.Empty(t, reply.Error) - - // T1=30ms → try, fail. T2=~52ms → try, fail. Expires at 60ms. - select { - case evt := <-expiredMsg: - require.Equal(t, "eni-expire", evt.ClientID) - require.Equal(t, "nak", evt.Reason) - case <-time.After(2 * time.Second): - t.Fatalf("expected lease-expired event (events=%d)", events.Load()) - } -} - -func TestDHCPManager_BootstrapRehydratesLease(t *testing.T) { - _, nc, js := testutil.StartTestJetStream(t) - fake := dhcp.NewFake() - - m1, err := NewDHCPManager(nc, js, fake) - require.NoError(t, err) - subs, err := m1.Subscribe(nc) - require.NoError(t, err) - - reply := requestAcquire(t, nc, dhcp.AcquireRequestMsg{Bridge: "br-wan", ClientID: "eni-boot"}) - require.Empty(t, reply.Error) - - // Close first manager; KV entry should survive. - for _, s := range subs { - _ = s.Unsubscribe() - } - m1.Close() - - // Second manager picks up the lease on Bootstrap. - fake2 := dhcp.NewFake() - m2, err := NewDHCPManager(nc, js, fake2) - require.NoError(t, err) - t.Cleanup(m2.Close) - require.NoError(t, m2.Bootstrap(context.Background())) - - lease, ok := m2.get("eni-boot") - require.True(t, ok, "manager should rehydrate persisted lease") - require.Equal(t, "192.0.2.100", lease.IP.String()) - require.Equal(t, 0, fake2.AcquireCount(), "no fresh DORA on bootstrap") -} - -func TestDHCPManager_BootstrapDropsExpiredLeases(t *testing.T) { - _, nc, js := testutil.StartTestJetStream(t) - fake := dhcp.NewFake() - - m1, err := NewDHCPManager(nc, js, fake) - require.NoError(t, err) - subs, err := m1.Subscribe(nc) - require.NoError(t, err) - - reply := requestAcquire(t, nc, dhcp.AcquireRequestMsg{Bridge: "br-wan", ClientID: "eni-stale"}) - require.Empty(t, reply.Error) - - for _, s := range subs { - _ = s.Unsubscribe() - } - m1.Close() - - // Rewrite the persisted record so AcquiredAt + LeaseDuration is in the past. - kv, err := js.KeyValue(dhcp.KVBucketDHCPLeases) - require.NoError(t, err) - entry, err := kv.Get("eni-stale") - require.NoError(t, err) - var rec map[string]any - require.NoError(t, json.Unmarshal(entry.Value(), &rec)) - rec["acquired_unix"] = time.Now().Add(-2 * time.Hour).Unix() - rec["lease_seconds"] = int64(60) - raw, err := json.Marshal(rec) - require.NoError(t, err) - _, err = kv.Put("eni-stale", raw) - require.NoError(t, err) - - fake2 := dhcp.NewFake() - m2, err := NewDHCPManager(nc, js, fake2) - require.NoError(t, err) - t.Cleanup(m2.Close) - - var expired sync.WaitGroup - expired.Add(1) - sub, err := nc.Subscribe(dhcp.TopicLeaseExpired, func(msg *nats.Msg) { - var evt dhcp.LeaseExpiredEvent - if err := json.Unmarshal(msg.Data, &evt); err == nil && evt.ClientID == "eni-stale" { - expired.Done() - } - }) - require.NoError(t, err) - t.Cleanup(func() { _ = sub.Unsubscribe() }) - - require.NoError(t, m2.Bootstrap(context.Background())) - - done := make(chan struct{}) - go func() { expired.Wait(); close(done) }() - select { - case <-done: - case <-time.After(2 * time.Second): - t.Fatal("expected lease-expired event for stale lease") - } - _, err = kv.Get("eni-stale") - require.ErrorIs(t, err, nats.ErrKeyNotFound) -} - -func TestDHCPManager_JitterWithinBounds(t *testing.T) { - _, nc, js := testutil.StartTestJetStream(t) - m, err := NewDHCPManager(nc, js, dhcp.NewFake(), WithDHCPJitterFraction(0.1)) - require.NoError(t, err) - t.Cleanup(m.Close) - - base := 100 * time.Millisecond - minOK := time.Duration(float64(base) * 0.9) - maxOK := time.Duration(float64(base) * 1.1) - for i := range 100 { - got := m.jitter(base) - if got < minOK || got > maxOK { - t.Fatalf("iteration %d: jitter produced %v, outside [%v,%v]", i, got, minOK, maxOK) - } - } -} - -func TestNewDHCPManager_RejectsNilDeps(t *testing.T) { - _, nc, js := testutil.StartTestJetStream(t) - - _, err := NewDHCPManager(nil, js, dhcp.NewFake()) - require.ErrorContains(t, err, "nats.Conn is required") - - _, err = NewDHCPManager(nc, nil, dhcp.NewFake()) - require.ErrorContains(t, err, "JetStreamContext is required") - - _, err = NewDHCPManager(nc, js, nil) - require.ErrorContains(t, err, "dhcp.Client is required") -} - -func TestDHCPManager_OptionsApply(t *testing.T) { - _, nc, js := testutil.StartTestJetStream(t) - - customMAC := func(string) net.HardwareAddr { - mac, _ := net.ParseMAC("02:de:ad:be:ef:01") - return mac - } - m, err := NewDHCPManager(nc, js, dhcp.NewFake(), - WithDHCPMACFunc(customMAC), - WithDHCPAcquireTimeout(1*time.Second), - WithDHCPJitterFraction(0.05), - ) - require.NoError(t, err) - t.Cleanup(m.Close) - require.Equal(t, time.Second, m.acquireTimeout) - require.InDelta(t, 0.05, m.jitterFraction, 0.0001) - require.Equal(t, "02:de:ad:be:ef:01", m.macForClientID("anything").String()) -} - -func TestDHCPManager_BootstrapEmptyBucket(t *testing.T) { - _, nc, js := testutil.StartTestJetStream(t) - m, err := NewDHCPManager(nc, js, dhcp.NewFake()) - require.NoError(t, err) - t.Cleanup(m.Close) - - // No leases in KV → Bootstrap is a clean no-op. - require.NoError(t, m.Bootstrap(context.Background())) -} - -func TestDHCPManager_JitterWithZeroFractionReturnsInput(t *testing.T) { - _, nc, js := testutil.StartTestJetStream(t) - m, err := NewDHCPManager(nc, js, dhcp.NewFake(), WithDHCPJitterFraction(0)) - require.NoError(t, err) - t.Cleanup(m.Close) - require.Equal(t, 100*time.Millisecond, m.jitter(100*time.Millisecond)) - require.Equal(t, time.Duration(0), m.jitter(0)) -} - -func TestDHCPManager_ClassifyRenewErr(t *testing.T) { - require.Equal(t, "unknown", classifyRenewErr(nil)) - require.Equal(t, "nak", classifyRenewErr(errors.New("got NAK from server"))) - require.Equal(t, "server_unreachable", classifyRenewErr(errors.New("i/o timeout"))) - require.Equal(t, "renew_failed", classifyRenewErr(errors.New("something else"))) -} - -func TestDHCPManager_EncodeDecodeLeaseRoundTrip(t *testing.T) { - mac, _ := net.ParseMAC("02:00:00:de:ad:be") - in := &dhcp.Lease{ - Bridge: "br-wan", ClientID: "eni-rt", Hostname: "eni-rt", VendorClass: "i-xyz", - HWAddr: mac, - IP: net.ParseIP("192.0.2.50"), - SubnetMask: net.CIDRMask(24, 32), - Routers: []net.IP{net.ParseIP("192.0.2.1")}, - DNS: []net.IP{net.ParseIP("1.1.1.1")}, - ServerID: net.ParseIP("192.0.2.1"), - AcquiredAt: time.Unix(1_700_000_000, 0).UTC(), - LeaseDuration: time.Hour, - T1: 30 * time.Minute, - T2: 52 * time.Minute, - RawOffer: []byte{0x01, 0x02}, - RawACK: []byte{0x03, 0x04}, - } - data, err := encodeLease(in) - require.NoError(t, err) - out, err := decodeLease(data) - require.NoError(t, err) - - require.Equal(t, in.ClientID, out.ClientID) - require.Equal(t, in.IP.String(), out.IP.String()) - require.Equal(t, in.LeaseDuration, out.LeaseDuration) - require.Equal(t, in.HWAddr.String(), out.HWAddr.String()) - require.Equal(t, in.RawACK, out.RawACK) - require.Equal(t, in.RawOffer, out.RawOffer) -} - -// --- acquireWithBackoff (mulga-siv-39, RFC 2131 §4.1) --- - -func newBackoffManager(t *testing.T, schedule []time.Duration, fake *dhcp.Fake) *DHCPManager { - t.Helper() - _, nc, js := testutil.StartTestJetStream(t) - m, err := NewDHCPManager(nc, js, fake, WithDHCPAcquireBackoff(schedule)) - require.NoError(t, err) - t.Cleanup(m.Close) - return m -} - -func TestAcquireWithBackoff_FirstAttemptSucceeds(t *testing.T) { - fake := dhcp.NewFake() - m := newBackoffManager(t, []time.Duration{10 * time.Millisecond, 10 * time.Millisecond}, fake) - - lease, err := m.acquireWithBackoff(context.Background(), dhcp.AcquireRequest{ - Bridge: "br-wan", ClientID: "eni-fast", HWAddr: net.HardwareAddr{0x02, 0, 0, 0, 0, 1}, - }) - require.NoError(t, err) - require.NotNil(t, lease) - require.Equal(t, 1, fake.AcquireCount(), "first-attempt success must not retransmit") -} - -func TestAcquireWithBackoff_RetriesThenSucceeds(t *testing.T) { - fake := dhcp.NewFake() - var calls atomic.Int32 - tmpl := dhcp.DefaultLeaseTemplate() - fake.AcquireHook = func(req dhcp.AcquireRequest) (*dhcp.Lease, error) { - n := calls.Add(1) - if n < 3 { - return nil, errors.New("simulated OFFER timeout") - } - return &dhcp.Lease{ - Bridge: req.Bridge, ClientID: req.ClientID, HWAddr: req.HWAddr, - IP: tmpl.IP, ServerID: tmpl.ServerID, LeaseDuration: tmpl.LeaseDuration, - AcquiredAt: time.Now(), - }, nil - } - m := newBackoffManager(t, []time.Duration{ - 5 * time.Millisecond, 5 * time.Millisecond, 5 * time.Millisecond, 5 * time.Millisecond, - }, fake) - - lease, err := m.acquireWithBackoff(context.Background(), dhcp.AcquireRequest{ - Bridge: "br-wan", ClientID: "eni-retry", HWAddr: net.HardwareAddr{0x02, 0, 0, 0, 0, 2}, - }) - require.NoError(t, err) - require.NotNil(t, lease) - require.Equal(t, int32(3), calls.Load()) -} - -func TestAcquireWithBackoff_AllAttemptsFail(t *testing.T) { - fake := dhcp.NewFake() - fake.AcquireHook = func(dhcp.AcquireRequest) (*dhcp.Lease, error) { - return nil, errors.New("server silent") - } - schedule := []time.Duration{ - 2 * time.Millisecond, 2 * time.Millisecond, 2 * time.Millisecond, - } - m := newBackoffManager(t, schedule, fake) - - lease, err := m.acquireWithBackoff(context.Background(), dhcp.AcquireRequest{ - Bridge: "br-wan", ClientID: "eni-fail", HWAddr: net.HardwareAddr{0x02, 0, 0, 0, 0, 3}, - }) - require.Error(t, err) - require.Nil(t, lease) - require.ErrorContains(t, err, "server silent") - require.Equal(t, len(schedule), fake.AcquireCount(), - "every entry in the schedule must run before giving up") -} - -func TestAcquireWithBackoff_ParentCtxCancel(t *testing.T) { - fake := dhcp.NewFake() - fake.AcquireHook = func(dhcp.AcquireRequest) (*dhcp.Lease, error) { - return nil, errors.New("should not be called") - } - m := newBackoffManager(t, []time.Duration{ - time.Second, time.Second, time.Second, - }, fake) - - ctx, cancel := context.WithCancel(context.Background()) - cancel() - - lease, err := m.acquireWithBackoff(ctx, dhcp.AcquireRequest{ - Bridge: "br-wan", ClientID: "eni-cancel", HWAddr: net.HardwareAddr{0x02, 0, 0, 0, 0, 4}, - }) - require.Error(t, err) - require.Nil(t, lease) - require.ErrorIs(t, err, context.Canceled) - require.Equal(t, 0, fake.AcquireCount(), "cancelled parent must short-circuit before any attempt") -} - -func TestDHCPAttemptTimeoutJitterBounds(t *testing.T) { - _, nc, js := testutil.StartTestJetStream(t) - m, err := NewDHCPManager(nc, js, dhcp.NewFake()) - require.NoError(t, err) - t.Cleanup(m.Close) - - base := 4 * time.Second - for i := range 200 { - got := m.dhcpAttemptTimeout(base) - if got < base-time.Second || got > base+time.Second { - t.Fatalf("iter %d: dhcpAttemptTimeout(%v) = %v, outside ±1s", i, base, got) - } - } - require.Zero(t, m.dhcpAttemptTimeout(0)) -} - -func TestNewDHCPManager_DefaultBackoffSchedule(t *testing.T) { - _, nc, js := testutil.StartTestJetStream(t) - m, err := NewDHCPManager(nc, js, dhcp.NewFake()) - require.NoError(t, err) - t.Cleanup(m.Close) - - require.Equal(t, []time.Duration{ - 4 * time.Second, 8 * time.Second, 16 * time.Second, 32 * time.Second, - }, m.acquireBackoff) - require.Equal(t, 90*time.Second, m.acquireTimeout) -} - -func TestResolveHWAddr(t *testing.T) { - t.Run("InvalidRawMACReturnsParseError", func(t *testing.T) { - mac, err := resolveHWAddr("not-a-mac", "client-1", nil) - require.Error(t, err) - require.Nil(t, mac) - require.Contains(t, err.Error(), "parse hw_addr") - }) - - t.Run("EmptyRawWithNilDeriveErrors", func(t *testing.T) { - mac, err := resolveHWAddr("", "client-1", nil) - require.Error(t, err) - require.Nil(t, mac) - require.Contains(t, err.Error(), "no derivation function") - }) - - t.Run("EmptyRawWithDeriveReturningEmptyErrors", func(t *testing.T) { - mac, err := resolveHWAddr("", "client-1", func(string) net.HardwareAddr { return nil }) - require.Error(t, err) - require.Nil(t, mac) - require.Contains(t, err.Error(), "empty result") - }) -} diff --git a/spinifex/services/vpcd/topology.go b/spinifex/services/vpcd/topology.go index 5e6669c19..48c899dc3 100644 --- a/spinifex/services/vpcd/topology.go +++ b/spinifex/services/vpcd/topology.go @@ -14,7 +14,6 @@ import ( "github.com/mulgadc/spinifex/spinifex/network/external" "github.com/mulgadc/spinifex/spinifex/network/policy" "github.com/mulgadc/spinifex/spinifex/network/topology" - "github.com/mulgadc/spinifex/spinifex/services/vpcd/dhcp" "github.com/mulgadc/spinifex/spinifex/services/vpcd/nbdb" "github.com/mulgadc/spinifex/spinifex/types" "github.com/mulgadc/spinifex/spinifex/utils" @@ -160,7 +159,7 @@ const ( BridgeModeVeth = "veth" // OvnExternalBridge is the OVS bridge that ovn-bridge-mappings targets // for the "external" localnet. Owned by setup-ovn.sh's ovn-bridge-mappings - // setup and independent of dhcp_bind_bridge (which is Linux-side in veth + // setup and independent of the WAN bridge (which is Linux-side in veth // mode). Named as a constant so the vpcd sanity check refers to the // contract, not a hardcoded string (D18). OvnExternalBridge = "br-ext" @@ -173,11 +172,6 @@ type TopologyHandler struct { externalPools []ExternalPoolConfig chassisNames []string // OVN chassis names for gateway HA scheduling bridgeMode string // "direct" or "veth" — controls NAT mode and localnet options - // nc is used to talk to vpcd's DHCPManager via vpc.dhcp.acquire / - // vpc.dhcp.release for gateway LRP IPs in centralized NAT on - // source="dhcp" pools (mulga-siv-38). nil when no DHCP pool is wired or - // the test stack supplies a static-only mock. - nc *nats.Conn // lm + lmOnce cache the topology.Manager produced from h.ovn so each // adapter call doesn't re-construct (Phase 2.6, mulga-siv-129). @@ -197,9 +191,8 @@ type TopologyHandler struct { natmErr error // igwm + igwmOnce cache the external.IGWManager. Handler routes the - // static-pool / distributed branches through it; the DHCP-coupled - // centralised path keeps using attachIGWLegacy until bead - // mulga-siv-125.3.3 lands and the DHCP manager goes away. + // static-pool / distributed branches through it; attachIGWLegacy is a + // dead branch retained for Slice D's deletion (mulga-siv-125.3.3). igwmOnce sync.Once igwm external.IGWManager igwmErr error @@ -240,15 +233,6 @@ func WithBridgeMode(mode string) TopologyOption { } } -// WithNATSConn wires the NATS connection used to talk to vpcd's DHCPManager -// for gateway-LRP DHCP-acquire (mulga-siv-38). Tests with mock OVN clients -// can omit this — the static / auto-derive paths still work. -func WithNATSConn(nc *nats.Conn) TopologyOption { - return func(h *TopologyHandler) { - h.nc = nc - } -} - // useCentralizedNAT returns true if the bridge mode requires centralized NAT. // Veth mode needs centralized NAT because the Linux bridge intermediary breaks // distributed NAT hairpin routing. Only direct bridge mode supports distributed NAT. @@ -301,11 +285,10 @@ func (h *TopologyHandler) ensureLocalnetOptions(ctx context.Context, extPortName // expectedGatewayPortNetwork returns the Networks CIDR a gateway LRP for // vpcId should carry. In direct mode, or in centralized mode without a pool, -// it is the link-local fallback. In centralized mode with a DHCP pool the -// gateway IP is acquired via vpc.dhcp.acquire (mulga-siv-38). With a static -// pool the IP comes from gw_lrp_range or the auto-derived top-of-subnet -// block (mulga-siv-36). Returns the gw IP (empty when link-local) so -// callers can persist it to external_ids. +// it is the link-local fallback. In centralized mode with a static pool the +// IP comes from gw_lrp_range or the auto-derived top-of-subnet block +// (mulga-siv-36). Returns the 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 @@ -314,13 +297,6 @@ func (h *TopologyHandler) expectedGatewayPortNetwork(ctx context.Context, vpcId if pool == nil { return gatewayPortNetwork, "", nil } - if pool.IsDHCP() { - ip, prefix, dhcpErr := h.allocateGatewayLRPIPViaDHCP(ctx, vpcId, pool) - if dhcpErr != nil { - return "", "", dhcpErr - } - return fmt.Sprintf("%s/%d", ip, prefix), ip, nil - } ip, prefix, ok, allocErr := h.allocateGatewayLRPIP(ctx, vpcId, pool) if allocErr != nil { return "", "", allocErr @@ -704,18 +680,10 @@ func (h *TopologyHandler) handleUpdatePortSGs(msg *nats.Msg) { // --- Internet Gateway (external connectivity + NAT) --- -// handleIGWAttach decodes the event and chooses a path: -// -// - Centralised NAT + DHCP-sourced pool: stays on attachIGWLegacy -// because the gateway LRP IP is allocated via vpc.dhcp.acquire, and -// that DHCP client lives in services/vpcd. Bead mulga-siv-125.3.3 -// removes the DHCP manager; the legacy branch goes away then. -// -// - All other cases (distributed NAT, static-range pool, no pool): -// delegate to external.IGWManager.AttachIGW. Behaviour is identical -// end-to-end — same external switch, same localnet port options, -// same gateway LRP networks, same default route, same chassis HA, -// same waitForFlowsHV tail barrier. +// handleIGWAttach decodes the event and delegates to +// external.IGWManager.AttachIGW. The legacy DHCP-coupled path is dead +// (pool.IsDHCP() now always returns false) and is removed in Slice D +// (mulga-siv-125.3.3). func (h *TopologyHandler) handleIGWAttach(msg *nats.Msg) { if h.ovn == nil { respond(msg, fmt.Errorf("OVN client not connected")) @@ -753,9 +721,9 @@ func (h *TopologyHandler) handleIGWAttach(msg *nats.Msg) { respond(msg, nil) } -// attachIGWLegacy is the DHCP-coupled centralised-NAT IGW attach path. -// Kept inline until the vpcd-local DHCP manager goes away in bead -// mulga-siv-125.3.3. The caller pre-resolved pool. +// attachIGWLegacy is the dead legacy centralised-NAT IGW attach path. +// Unreachable since pool.IsDHCP() always returns false; retained only +// for Slice D's deletion (mulga-siv-125.3.3). func (h *TopologyHandler) attachIGWLegacy(ctx context.Context, msg *nats.Msg, evt types.IGWEvent, pool *ExternalPoolConfig) { routerName := "vpc-" + evt.VpcId extSwitchName := "ext-" + evt.VpcId @@ -848,7 +816,6 @@ func (h *TopologyHandler) attachIGWLegacy(ctx context.Context, msg *nats.Msg, ev } slog.Info("vpcd: using external pool for IGW", "pool", pool.Name, - "source", pool.Source, "lrp_network", gatewayNetwork, "wan_gateway", wanGateway, ) @@ -962,12 +929,9 @@ func (h *TopologyHandler) attachIGWLegacy(ctx context.Context, msg *nats.Msg, ev respond(msg, nil) } -// handleIGWDetach decodes the event, delegates the OVN teardown to -// external.IGWManager.DetachIGW, and unconditionally calls -// releaseGatewayLRPLease afterwards. The DHCP lease release stays here -// because the upstream client lives in services/vpcd; both attach paths -// (legacy + manager-driven) tear down through the same exit so we don't -// need to branch. +// handleIGWDetach decodes the event and delegates the OVN teardown to +// external.IGWManager.DetachIGW. Static gateway LRP IPs live in OVN +// external_ids and are cleared when the LRP itself is deleted. func (h *TopologyHandler) handleIGWDetach(msg *nats.Msg) { if h.ovn == nil { respond(msg, fmt.Errorf("OVN client not connected")) @@ -992,7 +956,6 @@ func (h *TopologyHandler) handleIGWDetach(msg *nats.Msg) { respond(msg, err) return } - h.releaseGatewayLRPLease(evt.VpcId) slog.Info("vpcd: detached internet gateway from VPC", "igw_id", evt.InternetGatewayId, "vpc_id", evt.VpcId) respond(msg, nil) @@ -1327,69 +1290,6 @@ func gwLrpRange(pool *ExternalPoolConfig) (start, end net.IP, prefix int, ok boo return uint32ToIPv4(autoStartU), uint32ToIPv4(autoEndU), prefix, true } -// gwLrpClientID is the DHCP client-id used for a VPC's gateway LRP lease. -// Stable across reboots — DHCPManager reuses the same lease on idempotent -// re-attach so the LRP IP doesn't change unless the upstream server -// reassigns it. -func gwLrpClientID(vpcId string) string { return "gw-lrp-" + vpcId } - -// allocateGatewayLRPIPViaDHCP requests a DHCP lease from vpcd's DHCPManager -// for the gateway LRP of vpcId. The handler-side acquire is idempotent on -// client-id so retries return the same IP without a fresh DORA. Prefix -// comes from the lease's SubnetMask, falling back to pool.PrefixLen when -// the server omits option 1 (rare; should not happen in practice). -func (h *TopologyHandler) allocateGatewayLRPIPViaDHCP(ctx context.Context, vpcId string, pool *ExternalPoolConfig) (ip string, prefix int, err error) { - _ = ctx - if h.nc == nil { - return "", 0, fmt.Errorf("vpcd: no NATS conn for DHCP gw LRP allocation (vpc %s, pool %s)", vpcId, pool.Name) - } - clientID := gwLrpClientID(vpcId) - hostname := "spinifex-gw-" + vpcId - vendorClass := "mulga-spinifex-gw-lrp" - lease, dhcpErr := dhcp.RequestAcquire(h.nc, pool.DhcpBindBridge, clientID, hostname, vendorClass, pool.Name, "") - if dhcpErr != nil { - return "", 0, fmt.Errorf("dhcp acquire gw LRP IP for vpc %s: %w", vpcId, dhcpErr) - } - prefix = prefixFromMask(lease.SubnetMask) - if prefix == 0 { - prefix = pool.PrefixLen - } - if prefix <= 0 || prefix > 32 { - return "", 0, fmt.Errorf("dhcp gw LRP for vpc %s: cannot determine prefix (mask=%q pool prefix=%d)", vpcId, lease.SubnetMask, pool.PrefixLen) - } - return lease.IP, prefix, nil -} - -// releaseGatewayLRPLease asks vpcd's DHCPManager to drop the lease held for -// vpcId's gateway LRP. Best-effort — log on failure but never block IGW -// detach, since the upstream server will eventually expire the lease on -// its own. -func (h *TopologyHandler) releaseGatewayLRPLease(vpcId string) { - if h.nc == nil { - return - } - if err := dhcp.RequestRelease(h.nc, gwLrpClientID(vpcId)); err != nil { - slog.Warn("vpcd: dhcp release for gw LRP failed", "vpc_id", vpcId, "err", err) - } -} - -// prefixFromMask converts a dotted-decimal mask ("255.255.255.0") to a -// prefix length. Returns 0 when the mask is empty or unparseable. -func prefixFromMask(mask string) int { - if mask == "" { - return 0 - } - ip := net.ParseIP(mask).To4() - if ip == nil { - return 0 - } - ones, bits := net.IPMask(ip).Size() - if bits != 32 { - return 0 - } - return ones -} - // 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 diff --git a/spinifex/services/vpcd/topology_manager.go b/spinifex/services/vpcd/topology_manager.go index 60fa6f461..819778008 100644 --- a/spinifex/services/vpcd/topology_manager.go +++ b/spinifex/services/vpcd/topology_manager.go @@ -120,10 +120,9 @@ func (h *TopologyHandler) natManager() (policy.NATManager, error) { return h.natm, h.natmErr } -// igwManager returns the lazily-constructed external.IGWManager. Only the -// static-pool / distributed-NAT IGW subscriber path goes through it; the -// DHCP-coupled centralised path is held in attachIGWLegacy until bead -// mulga-siv-125.3.3 removes the vpcd-local DHCP manager. +// igwManager returns the lazily-constructed external.IGWManager. The +// static-pool / distributed-NAT IGW subscriber path goes through it; +// attachIGWLegacy in topology.go is dead code awaiting Slice D deletion. func (h *TopologyHandler) igwManager() (external.IGWManager, error) { h.igwmOnce.Do(func() { nm, err := h.natManager() @@ -139,7 +138,6 @@ func (h *TopologyHandler) igwManager() (external.IGWManager, error) { if p := h.findExternalPool("", ""); p != nil { shared := external.ExternalPoolConfig{ Name: p.Name, - Source: p.Source, RangeStart: p.RangeStart, RangeEnd: p.RangeEnd, Gateway: p.Gateway, @@ -148,7 +146,6 @@ func (h *TopologyHandler) igwManager() (external.IGWManager, error) { DNSServers: p.DNSServers, Region: p.Region, AZ: p.AZ, - DhcpBindBridge: p.DhcpBindBridge, GwLrpRangeStart: p.GwLrpRangeStart, GwLrpRangeEnd: p.GwLrpRangeEnd, } diff --git a/spinifex/services/vpcd/topology_test.go b/spinifex/services/vpcd/topology_test.go index 7b649cd39..acdae44a3 100644 --- a/spinifex/services/vpcd/topology_test.go +++ b/spinifex/services/vpcd/topology_test.go @@ -11,7 +11,6 @@ import ( "time" "github.com/mulgadc/spinifex/spinifex/network/topology" - "github.com/mulgadc/spinifex/spinifex/services/vpcd/dhcp" "github.com/mulgadc/spinifex/spinifex/services/vpcd/nbdb" "github.com/mulgadc/spinifex/spinifex/testutil" "github.com/mulgadc/spinifex/spinifex/types" @@ -1373,267 +1372,6 @@ func TestTopologyHandler_IGWAttach_CentralizedNATAllocatesGwLrpIP(t *testing.T) } } -// dhcpStub is a minimal in-test responder for vpc.dhcp.acquire / release. -// Each acquire returns a distinct lease IP keyed off ClientID so multi-VPC -// tests get unique IPs without standing up a full DHCPManager. -type dhcpStub struct { - t *testing.T - leases map[string]string // client_id -> ip - released map[string]bool - nextOct int -} - -func newDHCPStub(t *testing.T, nc *nats.Conn) *dhcpStub { - t.Helper() - s := &dhcpStub{ - t: t, - leases: map[string]string{}, - released: map[string]bool{}, - nextOct: 101, - } - subA, err := nc.Subscribe(dhcp.TopicAcquire, func(msg *nats.Msg) { - var req dhcp.AcquireRequestMsg - if err := json.Unmarshal(msg.Data, &req); err != nil { - data, _ := json.Marshal(dhcp.AcquireReplyMsg{Error: err.Error()}) - _ = msg.Respond(data) - return - } - ip, ok := s.leases[req.ClientID] - if !ok { - ip = fmt.Sprintf("192.168.3.%d", s.nextOct) - s.nextOct++ - s.leases[req.ClientID] = ip - } - reply := dhcp.AcquireReplyMsg{ - IP: ip, - SubnetMask: "255.255.255.0", - ServerID: "192.168.3.1", - Routers: []string{"192.168.3.1"}, - HWAddr: "02:00:00:00:00:" + fmt.Sprintf("%02x", s.nextOct%256), - ExpiresUnix: 0, - } - data, _ := json.Marshal(reply) - _ = msg.Respond(data) - }) - if err != nil { - t.Fatalf("subscribe acquire: %v", err) - } - subR, err := nc.Subscribe(dhcp.TopicRelease, func(msg *nats.Msg) { - var req dhcp.ReleaseRequestMsg - _ = json.Unmarshal(msg.Data, &req) - s.released[req.ClientID] = true - delete(s.leases, req.ClientID) - data, _ := json.Marshal(dhcp.ReleaseReplyMsg{}) - _ = msg.Respond(data) - }) - if err != nil { - t.Fatalf("subscribe release: %v", err) - } - t.Cleanup(func() { - _ = subA.Unsubscribe() - _ = subR.Unsubscribe() - }) - return s -} - -// TestTopologyHandler_IGWAttach_DHCPGwLrpIP guards mulga-siv-38: with -// pool.Source="dhcp" the gateway LRP IP must come from a DHCP lease -// (vpc.dhcp.acquire), not the static gw_lrp_range allocator. Three VPCs -// sharing one DHCP pool must each get a distinct lease IP and persist it -// on external_ids:spinifex:gateway_ip. IGW detach must release the lease. -func TestTopologyHandler_IGWAttach_DHCPGwLrpIP(t *testing.T) { - t.Skip("DHCP source no longer supported; Slice C deletes attachIGWLegacy + DHCP gateway-LRP path") - _, nc := startTestNATS(t) - stub := newDHCPStub(t, nc) - mock := NewMockOVNClient() - _ = mock.Connect(context.Background()) - ctx := context.Background() - - pools := []ExternalPoolConfig{ - { - Name: "lan-dhcp", - Source: "dhcp", - Gateway: "192.168.3.1", - PrefixLen: 24, - DhcpBindBridge: "br-wan", - }, - } - topo := NewTopologyHandler(mock, - WithExternalNetwork("pool", pools), - WithBridgeMode(BridgeModeVeth), - WithNATSConn(nc), - ) - subs, err := topo.Subscribe(nc) - if err != nil { - t.Fatalf("subscribe: %v", err) - } - defer func() { - for _, s := range subs { - _ = s.Unsubscribe() - } - }() - - vpcIds := []string{"vpc-dh-a", "vpc-dh-b", "vpc-dh-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) - } - ip, cidr, _ := strings.Cut(gwPort.Networks[0], "/") - if cidr != "24" { - t.Errorf("%s: expected /24 from DHCP lease mask, got /%s", gwPortName, cidr) - } - if !strings.HasPrefix(ip, "192.168.3.") { - t.Errorf("%s: expected 192.168.3.X from stub, 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 := seen[gwIP]; dup { - t.Errorf("gw IP collision: %s and %s share %s", gwPortName, other, gwIP) - } - seen[gwIP] = gwPortName - - // stub recorded the lease keyed by gw-lrp- - clientID := "gw-lrp-" + vpcId - if stub.leases[clientID] != ip { - t.Errorf("%s: stub lease %q != lrp IP %q", clientID, stub.leases[clientID], ip) - } - } - - // Detach one VPC, assert release reaches the stub. - detachVpc := vpcIds[0] - devt := types.IGWEvent{InternetGatewayId: "igw-" + detachVpc, VpcId: detachVpc} - ddata, _ := json.Marshal(devt) - resp, err := nc.Request(TopicIGWDetach, ddata, 5_000_000_000) - if err != nil { - t.Fatalf("request vpc.igw-detach: %v", err) - } - assertSuccess(t, resp, "detach IGW "+detachVpc) - if !stub.released["gw-lrp-"+detachVpc] { - t.Errorf("expected DHCP release for gw-lrp-%s, stub released=%v", detachVpc, stub.released) - } -} - -// TestTopologyHandler_IGWAttach_DHCPAcquireFailureAborts ensures that when -// vpcd's DHCPManager replies with an error, IGW attach surfaces the failure -// loud instead of falling back to link-local. Otherwise we silently restore -// the broken pre-siv-36 behavior. -func TestTopologyHandler_IGWAttach_DHCPAcquireFailureAborts(t *testing.T) { - t.Skip("DHCP source no longer supported; Slice C deletes attachIGWLegacy + DHCP gateway-LRP path") - _, nc := startTestNATS(t) - sub, err := nc.Subscribe(dhcp.TopicAcquire, func(msg *nats.Msg) { - reply := dhcp.AcquireReplyMsg{Error: "no leases available"} - data, _ := json.Marshal(reply) - _ = msg.Respond(data) - }) - if err != nil { - t.Fatalf("subscribe: %v", err) - } - defer func() { _ = sub.Unsubscribe() }() - - mock := NewMockOVNClient() - _ = mock.Connect(context.Background()) - ctx := context.Background() - - pools := []ExternalPoolConfig{ - { - Name: "lan-dhcp", - Source: "dhcp", - Gateway: "192.168.3.1", - PrefixLen: 24, - DhcpBindBridge: "br-wan", - }, - } - topo := NewTopologyHandler(mock, - WithExternalNetwork("pool", pools), - WithBridgeMode(BridgeModeVeth), - WithNATSConn(nc), - ) - subs, err := topo.Subscribe(nc) - if err != nil { - t.Fatalf("subscribe: %v", err) - } - defer func() { - for _, s := range subs { - _ = s.Unsubscribe() - } - }() - - _ = mock.CreateLogicalRouter(ctx, &nbdb.LogicalRouter{ - Name: "vpc-vpc-dh-fail", - ExternalIDs: map[string]string{ - "spinifex:vpc_id": "vpc-dh-fail", - "spinifex:cidr": "10.0.0.0/16", - }, - }) - evt := types.IGWEvent{InternetGatewayId: "igw-fail", VpcId: "vpc-dh-fail"} - data, _ := json.Marshal(evt) - resp, _ := nc.Request(TopicIGWAttach, data, 5_000_000_000) - var reply struct { - Error string `json:"error"` - } - _ = json.Unmarshal(resp.Data, &reply) - if reply.Error == "" { - t.Fatal("expected attach to fail when DHCP acquire errors, got success") - } - if !strings.Contains(reply.Error, "dhcp acquire") && !strings.Contains(reply.Error, "no leases available") { - t.Errorf("expected dhcp-related error, got %q", reply.Error) - } - // External switch must be rolled back. - if _, err := mock.GetLogicalSwitch(ctx, "ext-vpc-dh-fail"); err == nil { - t.Error("expected external switch rolled back after DHCP failure") - } -} - -// TestPrefixFromMask checks dotted-mask → prefix conversion across the -// boundary cases gw LRP DHCP relies on (option 1 missing, /32, /0, -// non-contiguous masks, IPv6). -func TestPrefixFromMask(t *testing.T) { - cases := []struct { - mask string - want int - }{ - {"", 0}, - {"255.255.255.0", 24}, - {"255.255.255.255", 32}, - {"0.0.0.0", 0}, - {"255.255.0.0", 16}, - {"not-an-ip", 0}, - } - for _, tc := range cases { - t.Run(tc.mask, func(t *testing.T) { - got := prefixFromMask(tc.mask) - if got != tc.want { - t.Errorf("prefixFromMask(%q)=%d, want %d", tc.mask, got, tc.want) - } - }) - } -} - 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 cdabb73f4..cd954e447 100644 --- a/spinifex/services/vpcd/vpcd.go +++ b/spinifex/services/vpcd/vpcd.go @@ -19,9 +19,7 @@ import ( "github.com/mulgadc/spinifex/spinifex/network/host" "github.com/mulgadc/spinifex/spinifex/network/policy" "github.com/mulgadc/spinifex/spinifex/network/reconcile" - "github.com/mulgadc/spinifex/spinifex/services/vpcd/dhcp" "github.com/mulgadc/spinifex/spinifex/utils" - "github.com/nats-io/nats.go" ) // sudoCommand wraps exec.Command with sudo when running as non-root. @@ -86,7 +84,6 @@ type Config struct { // ExternalPoolConfig mirrors config.ExternalPool for vpcd's internal use. type ExternalPoolConfig struct { Name string - Source string // "static" (default) or "dhcp" RangeStart string RangeEnd string Gateway string @@ -95,13 +92,13 @@ type ExternalPoolConfig struct { DNSServers []string Region string AZ string - DhcpBindBridge string // Bridge where the DHCP AF_PACKET socket binds (e.g. "br-wan"). Required for source="dhcp". GwLrpRangeStart string // Sub-range for OVN gateway LRP IPs in centralized NAT mode (mulga-siv-36). GwLrpRangeEnd string } -// IsDHCP is retained until Slice C removes the DHCP-source code paths; loader -// rejects source="dhcp" so this always returns false in steady state. +// IsDHCP is a dead stub retained for the unreachable attachIGWLegacy +// dispatcher branch in topology.go; Slice D deletes both together +// (mulga-siv-125.3.3). func (p *ExternalPoolConfig) IsDHCP() bool { return false } @@ -413,16 +410,16 @@ func launchService(cfg *Config) error { defer liveClient.Close() slog.Info("Connected to OVN NB DB", "endpoint", cfg.OVNNBAddr) - bridgeMode, dhcpBindBridge := resolveBridgeConfig(cfg.BridgeMode, cfg.ExternalInterface) - slog.Info("External bridge mode", "mode", bridgeMode, "dhcp_bind_bridge", dhcpBindBridge) - if err := verifyBridgeMode(bridgeMode, cfg.ExternalInterface, dhcpBindBridge); err != nil { + bridgeMode, wanBridge := resolveBridgeConfig(cfg.BridgeMode, cfg.ExternalInterface) + slog.Info("External bridge mode", "mode", bridgeMode, "wan_bridge", wanBridge) + if err := verifyBridgeMode(bridgeMode, cfg.ExternalInterface, wanBridge); err != nil { slog.Error("vpcd: bridge mode sanity check failed", "err", err) return err } // Resolve external CIDR from the WAN bridge before any reconcile. Skipped // when external networking is disabled (overlay-only deployments). - if err := ensureExternalCIDRReady(ctx, cfg.ExternalMode, dhcpBindBridge); err != nil { + if err := ensureExternalCIDRReady(ctx, cfg.ExternalMode, wanBridge); err != nil { return err } @@ -449,7 +446,6 @@ func launchService(cfg *Config) error { topoOpts = append(topoOpts, WithChassisNames(chassisNames)) slog.Info("vpcd: gateway chassis discovered", "chassis", chassisNames) topoOpts = append(topoOpts, WithBridgeMode(bridgeMode)) - topoOpts = append(topoOpts, WithNATSConn(nc)) topo := NewTopologyHandler(liveClient, topoOpts...) // Elect a single vpcd to run startup reconcile. Without this, N vpcds in a @@ -472,51 +468,11 @@ func launchService(cfg *Config) error { } }() - // DHCP manager: services vpc.dhcp.acquire/release requests from the - // daemon-side ExternalIPAM handlers and (mulga-siv-38) topology's gw-LRP - // allocator. MUST subscribe before any reconcile pass — bootstrap - // reconcile, ReconcileFromKV, and retrofit all call into reconcileIGW / - // expectedGatewayPortNetwork which fan out vpc.dhcp.acquire on - // source="dhcp" pools. Without a live subscription those requests hit - // "no responders" and burn the 60-attempt cold-boot retry budget per - // existing IGW. Started unconditionally — pools with source="static" - // never issue acquire requests, so the Manager sits idle until - // something actually wants DHCP. js, err := nc.JetStream() if err != nil { - slog.Error("Failed to get JetStream context for DHCP manager", "err", err) + slog.Error("Failed to get JetStream context", "err", err) return err } - // timeout/retry args are legacy no-ops (mulga-siv-39): DHCPManager - // owns DORA retransmission via acquireWithBackoff. - // - // Retry on JetStream-not-ready: NewDHCPManager creates the DHCP-leases - // KV bucket, which fails immediately if the JetStream cluster has not - // reached quorum. On multi-node bring-up vpcd's launchService runs - // before all peers are reachable, so we mirror the daemon's - // initJetStream backoff (500ms→10s, 5 min cap) instead of crashing the - // service and aborting the rest of the node bring-up. - dhcpManager, err := newDHCPManagerWithRetry(ctx, nc, js) - if err != nil { - slog.Error("Failed to create DHCP manager", "err", err) - return err - } - defer dhcpManager.Close() - - if err := dhcpManager.Bootstrap(ctx); err != nil { - slog.Error("DHCP manager bootstrap failed", "err", err) - return err - } - dhcpSubs, err := dhcpManager.Subscribe(nc) - if err != nil { - slog.Error("DHCP manager subscribe failed", "err", err) - return err - } - defer func() { - for _, s := range dhcpSubs { - _ = s.Unsubscribe() - } - }() // Construct the network/reconcile manager stack. The reconciler replaces // the five-pass startup sequence (Reconcile + ReconcileFromKV + @@ -598,7 +554,7 @@ func launchService(cfg *Config) error { }() slog.Info("vpcd service started, waiting for VPC lifecycle events", - "subscriptions", len(subs), "dhcp_subscriptions", len(dhcpSubs)) + "subscriptions", len(subs)) // Wait for shutdown signal sigChan := make(chan os.Signal, 1) @@ -611,48 +567,6 @@ func launchService(cfg *Config) error { return nil } -// newDHCPManagerWithRetry constructs a DHCPManager, retrying on transient -// JetStream-not-ready errors with exponential backoff (500ms→10s, capped -// at 5 min). NewDHCPManager creates the spinifex-dhcp-leases KV bucket; -// on multi-node bring-up the JetStream cluster may not yet have quorum -// when vpcd starts, and a single-shot CreateKeyValue would otherwise crash -// the service before the rest of the node finishes coming up. -// -// Mirrors daemon.initJetStream's backoff so vpcd waits as patiently as the -// daemon does for the cluster to form. -func newDHCPManagerWithRetry(ctx context.Context, nc *nats.Conn, js nats.JetStreamContext) (*DHCPManager, error) { - const maxWait = 5 * time.Minute - retryDelay := 500 * time.Millisecond - start := time.Now() - attempt := 0 - - for { - attempt++ - mgr, err := NewDHCPManager(nc, js, dhcp.NewNClient4(0, 0)) - if err == nil { - if attempt > 1 { - slog.Info("vpcd: DHCP manager ready", "attempts", attempt, "elapsed", time.Since(start).Round(time.Second)) - } - return mgr, nil - } - - elapsed := time.Since(start) - if elapsed >= maxWait { - return nil, fmt.Errorf("DHCP manager init timed out after %s (%d attempts): %w", elapsed.Round(time.Second), attempt, err) - } - - slog.Warn("vpcd: DHCP manager not ready (waiting for JetStream cluster quorum)", - "err", err, "attempt", attempt, "elapsed", elapsed.Round(time.Second), "retryIn", retryDelay) - - select { - case <-ctx.Done(): - return nil, fmt.Errorf("DHCP manager init cancelled: %w", ctx.Err()) - case <-time.After(retryDelay): - } - retryDelay = min(retryDelay*2, 10*time.Second) - } -} - // externalPoolConfigToShared translates the vpcd-local ExternalPoolConfig // into the network/external package's shared shape. The two will merge // once topology.go is split (bead mulga-siv-125.3.4 — see external.go's @@ -660,7 +574,6 @@ func newDHCPManagerWithRetry(ctx context.Context, nc *nats.Conn, js nats.JetStre func externalPoolConfigToShared(p ExternalPoolConfig) external.ExternalPoolConfig { return external.ExternalPoolConfig{ Name: p.Name, - Source: p.Source, RangeStart: p.RangeStart, RangeEnd: p.RangeEnd, Gateway: p.Gateway, @@ -669,7 +582,6 @@ func externalPoolConfigToShared(p ExternalPoolConfig) external.ExternalPoolConfi DNSServers: p.DNSServers, Region: p.Region, AZ: p.AZ, - DhcpBindBridge: p.DhcpBindBridge, GwLrpRangeStart: p.GwLrpRangeStart, GwLrpRangeEnd: p.GwLrpRangeEnd, } @@ -744,37 +656,36 @@ var readLinkMaster = func(iface string) (string, error) { // verifyBridgeMode is the post-detect sanity check. It refuses to start vpcd // when the chosen bridge mode does not match the host plumbing (D4+D18): // -// - direct: ExternalInterface must be an OVS port on DhcpBindBridge. That is -// the whole contract of direct mode. +// - direct: ExternalInterface must be an OVS port on the WAN bridge. That +// is the whole contract of direct mode. // - veth: (a) veth-wan-ovs must be an OVS port on OvnExternalBridge — the // OVN side, owned by setup-ovn.sh's ovn-bridge-mappings. (b) veth-wan-br -// must be enslaved to DhcpBindBridge — the Linux side, where the DHCP -// client sees LAN frames. +// must be enslaved to the WAN bridge — the Linux side. // - empty / unknown: fail with the list of supported values. // // Fail-start, not soft-degrade — the distributed-NAT-on-veth-host footgun is // exactly what this plan set out to kill. -func verifyBridgeMode(mode, externalIface, dhcpBindBridge string) error { +func verifyBridgeMode(mode, externalIface, wanBridge string) error { switch mode { case BridgeModeDirect: if externalIface == "" { return fmt.Errorf("vpcd: direct bridge mode requires external_interface (the WAN NIC name)") } - if dhcpBindBridge == "" { - return fmt.Errorf("vpcd: direct bridge mode requires dhcp_bind_bridge (the OVS bridge holding the WAN NIC)") + if wanBridge == "" { + return fmt.Errorf("vpcd: direct bridge mode requires the WAN bridge (the OVS bridge holding the WAN NIC)") } br, err := portToBr(externalIface) if err != nil { return fmt.Errorf("vpcd: direct bridge mode: %w", err) } - if br != dhcpBindBridge { - return fmt.Errorf("vpcd: direct bridge mode: %q is on OVS bridge %q, expected %q (dhcp_bind_bridge)", - externalIface, br, dhcpBindBridge) + if br != wanBridge { + return fmt.Errorf("vpcd: direct bridge mode: %q is on OVS bridge %q, expected %q", + externalIface, br, wanBridge) } return nil case BridgeModeVeth: - if dhcpBindBridge == "" { - return fmt.Errorf("vpcd: veth bridge mode requires dhcp_bind_bridge (the Linux bridge holding the WAN NIC)") + if wanBridge == "" { + return fmt.Errorf("vpcd: veth bridge mode requires wan_bridge (the Linux bridge holding the WAN NIC)") } br, err := portToBr("veth-wan-ovs") if err != nil { @@ -788,9 +699,9 @@ func verifyBridgeMode(mode, externalIface, dhcpBindBridge string) error { if err != nil { return fmt.Errorf("vpcd: veth bridge mode: veth-wan-br missing or has no master — is spinifex-veth-wan.service running? %w", err) } - if master != dhcpBindBridge { - return fmt.Errorf("vpcd: veth bridge mode: veth-wan-br master is %q, expected %q (dhcp_bind_bridge)", - master, dhcpBindBridge) + if master != wanBridge { + return fmt.Errorf("vpcd: veth bridge mode: veth-wan-br master is %q, expected %q (wan_bridge)", + master, wanBridge) } return nil default: diff --git a/spinifex/services/vpcd/vpcd_test.go b/spinifex/services/vpcd/vpcd_test.go index 4bf974e84..986209594 100644 --- a/spinifex/services/vpcd/vpcd_test.go +++ b/spinifex/services/vpcd/vpcd_test.go @@ -258,15 +258,15 @@ func TestVerifyBridgeMode_DirectMissingExternalIface(t *testing.T) { func TestVerifyBridgeMode_DirectMissingBindBridge(t *testing.T) { err := verifyBridgeMode(BridgeModeDirect, "enp0s3", "") - if err == nil || !strings.Contains(err.Error(), "dhcp_bind_bridge") { - t.Fatalf("expected dhcp_bind_bridge error, got: %v", err) + if err == nil || !strings.Contains(err.Error(), "WAN bridge") { + t.Fatalf("expected WAN bridge error, got: %v", err) } } func TestVerifyBridgeMode_VethMissingBindBridge(t *testing.T) { err := verifyBridgeMode(BridgeModeVeth, "", "") - if err == nil || !strings.Contains(err.Error(), "dhcp_bind_bridge") { - t.Fatalf("expected dhcp_bind_bridge error, got: %v", err) + if err == nil || !strings.Contains(err.Error(), "wan_bridge") { + t.Fatalf("expected wan_bridge error, got: %v", err) } }