diff --git a/spinifex/services/vpcd/dhcp/nclient4.go b/spinifex/services/vpcd/dhcp/nclient4.go index 70ca30f92..3c1bfa239 100644 --- a/spinifex/services/vpcd/dhcp/nclient4.go +++ b/spinifex/services/vpcd/dhcp/nclient4.go @@ -13,22 +13,27 @@ import ( // 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 - retry int } var _ Client = (*NClient4Client)(nil) -// NewNClient4 creates an NClient4Client with sensible DORA defaults. -func NewNClient4(timeout time.Duration, retry int) *NClient4Client { +// 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 = 15 * time.Second + timeout = 60 * time.Second } - if retry <= 0 { - retry = 3 - } - return &NClient4Client{timeout: timeout, retry: retry} + return &NClient4Client{timeout: timeout} } func (c *NClient4Client) Acquire(ctx context.Context, req AcquireRequest) (*Lease, error) { @@ -42,7 +47,9 @@ func (c *NClient4Client) Acquire(ctx context.Context, req AcquireRequest) (*Leas client, err := nclient4.New(req.Bridge, nclient4.WithHWAddr(req.HWAddr), nclient4.WithTimeout(c.timeout), - nclient4.WithRetry(c.retry), + // 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) @@ -68,7 +75,7 @@ func (c *NClient4Client) Renew(ctx context.Context, lease *Lease) (*Lease, error client, err := nclient4.New(lease.Bridge, nclient4.WithHWAddr(lease.HWAddr), nclient4.WithTimeout(c.timeout), - nclient4.WithRetry(c.retry), + nclient4.WithRetry(1), ) if err != nil { return nil, fmt.Errorf("open nclient4 on %s for renew: %w", lease.Bridge, err) diff --git a/spinifex/services/vpcd/dhcp_manager.go b/spinifex/services/vpcd/dhcp_manager.go index fe3772f31..3597d5ed1 100644 --- a/spinifex/services/vpcd/dhcp_manager.go +++ b/spinifex/services/vpcd/dhcp_manager.go @@ -32,10 +32,17 @@ type DHCPManager struct { // HWAddr unset. Defaults to generateMAC; tests override. macForClientID func(clientID string) net.HardwareAddr - // acquireTimeout bounds a single DORA (Acquire) / Renew / Release - // handshake. Independent of the RPC reply timeout on the handler side. + // 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 @@ -69,11 +76,19 @@ func WithDHCPMACFunc(fn func(clientID string) net.HardwareAddr) DHCPManagerOptio return func(m *DHCPManager) { m.macForClientID = fn } } -// WithDHCPAcquireTimeout overrides the per-handshake DORA timeout. +// 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 } @@ -108,10 +123,18 @@ func NewDHCPManager(nc *nats.Conn, js nats.JetStreamContext, client dhcp.Client, ctx, cancel := context.WithCancel(context.Background()) m := &DHCPManager{ - client: client, - kv: kv, - nc: nc, - acquireTimeout: 15 * time.Second, + 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{}, @@ -245,7 +268,7 @@ func (m *DHCPManager) handleAcquire(msg *nats.Msg) { defer m.endAcquire(req.Bridge) start := time.Now() - lease, err := m.client.Acquire(ctx, dhcp.AcquireRequest{ + lease, err := m.acquireWithBackoff(ctx, dhcp.AcquireRequest{ Bridge: req.Bridge, ClientID: req.ClientID, Hostname: req.Hostname, @@ -392,6 +415,73 @@ func (m *DHCPManager) renewLoop(ctx context.Context, tracker *managedLease) { } } +// 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) diff --git a/spinifex/services/vpcd/dhcp_manager_test.go b/spinifex/services/vpcd/dhcp_manager_test.go index e85e8d4e6..3b724da1c 100644 --- a/spinifex/services/vpcd/dhcp_manager_test.go +++ b/spinifex/services/vpcd/dhcp_manager_test.go @@ -412,6 +412,125 @@ func TestDHCPManager_EncodeDecodeLeaseRoundTrip(t *testing.T) { 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) diff --git a/spinifex/services/vpcd/vpcd.go b/spinifex/services/vpcd/vpcd.go index eaa33a3ba..fb75a7c6b 100644 --- a/spinifex/services/vpcd/vpcd.go +++ b/spinifex/services/vpcd/vpcd.go @@ -10,7 +10,6 @@ import ( "path/filepath" "strings" "syscall" - "time" "github.com/mulgadc/spinifex/spinifex/admin" "github.com/mulgadc/spinifex/spinifex/services/vpcd/dhcp" @@ -404,7 +403,9 @@ func launchService(cfg *Config) error { slog.Error("Failed to get JetStream context for DHCP manager", "err", err) return err } - dhcpManager, err := NewDHCPManager(nc, js, dhcp.NewNClient4(15*time.Second, 3)) + // timeout/retry args are legacy no-ops (mulga-siv-39): DHCPManager + // owns DORA retransmission via acquireWithBackoff. + dhcpManager, err := NewDHCPManager(nc, js, dhcp.NewNClient4(0, 0)) if err != nil { slog.Error("Failed to create DHCP manager", "err", err) return err