From 291e550e8ac3fa4454d779450f731f7faec92b44 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Wed, 22 Jul 2026 14:19:16 +1000 Subject: [PATCH] fix(dhcp): honour the caller's retransmission budget and encode option 61 correctly (#565) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(dhcp): derive nclient4 read deadline from the caller's context nclient4 races its own read deadline against the caller's context and the shorter one ends the attempt. Acquire and Renew passed a fixed 5s, so every window in Manager's 4s/8s/16s/32s retransmission schedule past the first was truncated to 5s: the client listened for ~19s of its 90s budget and a server that went quiet for longer was never waited out. Measured in the sink, the split was total — attempt 1 (jittered under 5s) always failed with the caller's context deadline, attempts 2 to 4 always failed with nclient4's ErrNoResponse — and only 27% of clients that lost attempt 1 were rescued by attempt 2. Track the context's remaining time instead, keeping the configured value as the fallback its doc comment already describes. * fix(dhcp): keep the socket deadline strictly behind the caller's Setting the nclient4 read deadline to exactly the context's remaining time makes the two expire together, so a timed-out attempt reports either context.DeadlineExceeded or nclient4's ErrNoResponse depending on which goroutine wins. Observed live on a settled bridge: three attempts reported the context deadline and the fourth reported ErrNoResponse, which reads as a packet-matching fault rather than the plain timeout it is. Give the socket a second of grace so ctx.Done() is always what fires. * fix(dhcp): encode option 61 with its hardware-type byte The client identifier was sent as a bare string, so the server read the identifier's first character as the hardware type — "dhcp-gw-lrp-vpc-x" arrived as type 100 ('d') with the identifier truncated. Since that is not Ethernet, the server recorded no hardware address, and the upstream lease table listed our leases with a MAC of all zeroes. Send the chaddr typed as Ethernet, falling back to a type-0 identifier when no hardware address is available. RELEASE now encodes identically, since the server matches a release to a lease by client identifier. Option 61 no longer carries the readable id, so restore it via hostname and vendor class, which were specified but never populated. dhcptest builds its options through the same helper rather than its own copy, which had drifted into reproducing the broken encoding. * fix(dhcptest): derive the probe MAC the way vpcd does The probe carried its own copy of the MAC derivation, still on the 24-bit 02:00:00:xx:xx:xx hash that vpcd moved off when widening it to a SHA-256 prefix. So --spinifex-id leased under a MAC the daemon would never use: "gateway-wan1" resolved to 02:00:00:7c:b7:b6 here and 02:54:df:71:22:5c in vpcd, which makes the probe unable to reproduce a daemon lease at all. Call DeriveMAC instead and drop the local copy. --- cmd/dhcptest/main.go | 34 ++--- spinifex/network/external/dhcp/nclient4.go | 77 +++++++++-- .../external/dhcp/nclient4_internal_test.go | 130 ++++++++++++++++++ 3 files changed, 205 insertions(+), 36 deletions(-) create mode 100644 spinifex/network/external/dhcp/nclient4_internal_test.go diff --git a/cmd/dhcptest/main.go b/cmd/dhcptest/main.go index 7e7a3ed03..941e07f3e 100644 --- a/cmd/dhcptest/main.go +++ b/cmd/dhcptest/main.go @@ -6,7 +6,7 @@ // Usage (must run as root — needs AF_PACKET): // // sudo dhcptest --iface=br-wan -// sudo dhcptest --iface=br-wan --mac=02:00:00:7c:b7:b9 --promisc +// sudo dhcptest --iface=br-wan --mac=02:54:df:71:22:5c --promisc // sudo dhcptest --iface=br-wan --spinifex-id=gateway-wan1 package main @@ -21,8 +21,8 @@ import ( "strings" "time" - "github.com/insomniacslk/dhcp/dhcpv4" "github.com/insomniacslk/dhcp/dhcpv4/nclient4" + "github.com/mulgadc/spinifex/spinifex/network/external/dhcp" ) func main() { @@ -44,9 +44,11 @@ func main() { switch { case *spinifexID != "": - hwAddr, err = net.ParseMAC(generateMAC(*spinifexID)) + // vpcd's own derivation, so the probe leases under the MAC the daemon + // would have used for this id. + hwAddr, err = dhcp.DeriveMAC(*spinifexID) if err != nil { - slog.Error("Could not parse generated MAC", "spinifex-id", *spinifexID, "error", err) + slog.Error("Could not derive MAC", "spinifex-id", *spinifexID, "error", err) os.Exit(1) } fmt.Printf("spinifex virtual MAC for %q: %s\n", *spinifexID, hwAddr) @@ -109,17 +111,9 @@ func main() { } defer client.Close() - // 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)))) - } - if *hostname != "" { - mods = append(mods, dhcpv4.WithOption(dhcpv4.OptHostName(*hostname))) - } - if *vendorClass != "" { - mods = append(mods, dhcpv4.WithOption(dhcpv4.OptClassIdentifier(*vendorClass))) - } + // Identity modifiers — client-id / hostname / vendor-class for upstream lease + // tagging. Built by the vpcd code path so the probe cannot drift from it. + mods := dhcp.IdentityModifiers(cid, *hostname, *vendorClass, hwAddr) fmt.Println("Sending DHCP Discover...") start := time.Now() @@ -153,16 +147,6 @@ func (slogPrintfer) Printf(format string, v ...any) { slog.Info(fmt.Sprintf(format, v...), "component", "nclient4") } -// generateMAC is the same deterministic hash used by vpcd's topology.go. -// It produces a locally-administered MAC (02:00:00:xx:xx:xx) from a resource ID. -func generateMAC(resourceID string) string { - h := uint32(0) - for _, c := range resourceID { - h = h*31 + uint32(c) - } - return fmt.Sprintf("02:00:00:%02x:%02x:%02x", (h>>16)&0xff, (h>>8)&0xff, h&0xff) -} - func ifaceMAC(name string) (net.HardwareAddr, error) { ifc, err := net.InterfaceByName(name) if err != nil { diff --git a/spinifex/network/external/dhcp/nclient4.go b/spinifex/network/external/dhcp/nclient4.go index 4a17d1d0f..63a5be355 100644 --- a/spinifex/network/external/dhcp/nclient4.go +++ b/spinifex/network/external/dhcp/nclient4.go @@ -33,6 +33,30 @@ func NewNClient4(timeout time.Duration) *NClient4Client { return &NClient4Client{timeout: timeout} } +// socketTimeoutGrace keeps the socket deadline strictly behind the caller's, so +// ctx.Done() is what ends a timed-out attempt. Matching the two exactly makes +// them expire together and the reported error becomes a coin toss between +// context.DeadlineExceeded and nclient4's ErrNoResponse, which reads as a +// packet-matching fault rather than the plain timeout it is. +const socketTimeoutGrace = time.Second + +// socketTimeout is the read deadline handed to nclient4 for one DORA. It must +// track the caller's deadline: nclient4 races its own deadline against ctx and +// the shorter one ends the attempt, so a fixed value below the caller's budget +// silently truncates every window in Manager's retransmission schedule. +func (c *NClient4Client) socketTimeout(ctx context.Context) time.Duration { + deadline, ok := ctx.Deadline() + if !ok { + return c.timeout + } + // A deadline already in the past is left to ctx.Done(); nclient4 rejects a + // non-positive timeout, so keep the fallback rather than pass it through. + if remaining := time.Until(deadline); remaining > 0 { + return remaining + socketTimeoutGrace + } + return c.timeout +} + func (c *NClient4Client) Acquire(ctx context.Context, req AcquireRequest) (*Lease, error) { if req.Bridge == "" { return nil, fmt.Errorf("dhcp acquire: bridge is required") @@ -73,7 +97,7 @@ 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.WithTimeout(c.socketTimeout(ctx)), nclient4.WithRetry(nclient4Retries), ) if err != nil { @@ -83,7 +107,7 @@ func (c *NClient4Client) Acquire(ctx context.Context, req AcquireRequest) (*Leas // Broadcast flag forces ff:ff:ff:ff:ff:ff destination — without it the // server unicasts to the derived chaddr MAC which the NIC drops in hw. - mods := append(identityModifiers(req.ClientID, req.Hostname, req.VendorClass), dhcpv4.WithBroadcast(true)) + mods := append(IdentityModifiers(req.ClientID, req.Hostname, req.VendorClass, req.HWAddr), 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) @@ -113,7 +137,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.WithTimeout(c.socketTimeout(ctx)), nclient4.WithRetry(nclient4Retries), ) if err != nil { @@ -122,7 +146,7 @@ func (c *NClient4Client) Renew(ctx context.Context, lease *Lease) (*Lease, error defer func() { _ = client.Close() }() renewed, err := client.Renew(ctx, nclient4Lease, - identityModifiers(lease.ClientID, lease.Hostname, lease.VendorClass)...) + IdentityModifiers(lease.ClientID, lease.Hostname, lease.VendorClass, lease.HWAddr)...) if err != nil { return nil, fmt.Errorf("dhcp renew on %s (client=%s): %w", lease.Bridge, lease.ClientID, err) } @@ -166,8 +190,10 @@ func (c *NClient4Client) Release(_ context.Context, lease *Lease) error { } defer func() { _ = client.Close() }() + // The server matches a RELEASE to a lease by client-id, so this must encode + // identically to the DISCOVER/REQUEST that took the lease out. if err := client.Release(nclient4Lease, - dhcpv4.WithOption(dhcpv4.OptClientIdentifier([]byte(lease.ClientID)))); err != nil { + dhcpv4.WithOption(clientIDOption(lease.ClientID, lease.HWAddr))); err != nil { return fmt.Errorf("dhcp release on %s (client=%s): %w", lease.Bridge, lease.ClientID, err) } return nil @@ -184,20 +210,49 @@ func DeriveMAC(clientID string) (net.HardwareAddr, error) { return hw, nil } -// identityModifiers builds the three identifying DHCP options set on every +// defaultVendorClass marks a lease as ours in the upstream lease table when the +// caller supplies nothing more specific. +const defaultVendorClass = "mulga-spinifex" + +// clientIDOption encodes option 61 as RFC 2132 §9.14 requires: a type byte +// followed by the identifier. Ethernet (type 1) paired with the chaddr lets the +// server bind the lease to a hardware address, so it lists with a real MAC. +// +// Sending a bare string instead makes the server read its first byte as the +// hardware type — "dhcp-gw-lrp-vpc-x" arrives as hardware-type 100 ('d') with +// the identifier truncated to "hcp-gw-lrp-vpc-x" — and since that is not +// Ethernet, no hardware address is recorded against the lease at all. +func clientIDOption(clientID string, hwAddr net.HardwareAddr) dhcpv4.Option { + if len(hwAddr) == 6 { + return dhcpv4.OptClientIdentifier(append([]byte{0x01}, hwAddr...)) + } + // Type 0 marks an identifier that is not a hardware address (RFC 4361 §6.1). + return dhcpv4.OptClientIdentifier(append([]byte{0x00}, clientID...)) +} + +// IdentityModifiers builds the three identifying DHCP options set on every // outbound message: option 61 (client-id), option 12 (hostname), option 60 -// (vendor class). -func identityModifiers(clientID, hostname, vendorClass string) []dhcpv4.Modifier { +// (vendor class). Hostname and vendor class carry the human-readable identity, +// since option 61 encodes the chaddr rather than the client-id string; without +// them a lease is unattributable in the upstream table. +// +// Exported so the dhcptest probe puts the same bytes on the wire as vpcd does; +// a probe that builds its own options can only confirm its own behaviour. +func IdentityModifiers(clientID, hostname, vendorClass string, hwAddr net.HardwareAddr) []dhcpv4.Modifier { var mods []dhcpv4.Modifier if clientID != "" { - mods = append(mods, dhcpv4.WithOption(dhcpv4.OptClientIdentifier([]byte(clientID)))) + mods = append(mods, dhcpv4.WithOption(clientIDOption(clientID, hwAddr))) + } + if hostname == "" { + hostname = clientID } if hostname != "" { mods = append(mods, dhcpv4.WithOption(dhcpv4.OptHostName(hostname))) } - if vendorClass != "" { - mods = append(mods, dhcpv4.WithOption(dhcpv4.OptClassIdentifier(vendorClass))) + if vendorClass == "" { + vendorClass = defaultVendorClass } + mods = append(mods, dhcpv4.WithOption(dhcpv4.OptClassIdentifier(vendorClass))) return mods } diff --git a/spinifex/network/external/dhcp/nclient4_internal_test.go b/spinifex/network/external/dhcp/nclient4_internal_test.go new file mode 100644 index 000000000..a9c3e9a1a --- /dev/null +++ b/spinifex/network/external/dhcp/nclient4_internal_test.go @@ -0,0 +1,130 @@ +package dhcp + +import ( + "bytes" + "context" + "net" + "testing" + "time" + + "github.com/insomniacslk/dhcp/dhcpv4" +) + +// socketTimeout must never cap the caller's budget, because nclient4 ends an +// attempt on whichever of the two deadlines fires first. +func TestSocketTimeoutTracksContextDeadline(t *testing.T) { + c := NewNClient4(5 * time.Second) + + t.Run("no deadline falls back to the configured timeout", func(t *testing.T) { + if got := c.socketTimeout(context.Background()); got != 5*time.Second { + t.Fatalf("socketTimeout = %v, want 5s", got) + } + }) + + t.Run("longer deadline is honoured rather than capped", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 32*time.Second) + defer cancel() + got := c.socketTimeout(ctx) + if got <= 5*time.Second { + t.Fatalf("socketTimeout = %v, want the remaining ~32s, not the 5s fallback", got) + } + // Strictly beyond the caller's deadline so ctx.Done() reports the timeout. + if got <= 32*time.Second { + t.Fatalf("socketTimeout = %v, want more than the remaining 32s", got) + } + }) + + t.Run("shorter deadline shortens the read", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + got := c.socketTimeout(ctx) + if got <= time.Second || got > 2*time.Second { + t.Fatalf("socketTimeout = %v, want just beyond the remaining 1s", got) + } + }) + + t.Run("expired deadline yields a positive timeout", func(t *testing.T) { + ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) + defer cancel() + // nclient4 rejects a non-positive read deadline; ctx.Done() ends the + // attempt immediately regardless of what is passed here. + if got := c.socketTimeout(ctx); got != 5*time.Second { + t.Fatalf("socketTimeout = %v, want the 5s fallback", got) + } + }) +} + +// Option 61 carries a leading hardware-type byte. Omitting it makes the server +// consume the identifier's first character as the type, which is how leases end +// up in the upstream table with no hardware address at all. +func TestClientIDOptionCarriesHardwareType(t *testing.T) { + hw, err := net.ParseMAC("02:0c:46:55:cf:ed") + if err != nil { + t.Fatalf("parse mac: %v", err) + } + + t.Run("ethernet address is typed 1 and sent verbatim", func(t *testing.T) { + got := clientIDOption("dhcp-gw-lrp-vpc-abc", hw).Value.ToBytes() + want := append([]byte{0x01}, hw...) + if !bytes.Equal(got, want) { + t.Fatalf("client-id = %v, want %v", got, want) + } + }) + + t.Run("without a hardware address the id is typed 0", func(t *testing.T) { + got := clientIDOption("eni-1234", nil).Value.ToBytes() + want := append([]byte{0x00}, "eni-1234"...) + if !bytes.Equal(got, want) { + t.Fatalf("client-id = %v, want %v", got, want) + } + }) + + t.Run("no leading byte is mistaken for a hardware type", func(t *testing.T) { + // 'd' is 0x64, so the unfixed encoding announced hardware type 100. + got := clientIDOption("dhcp-gw-lrp-vpc-abc", hw).Value.ToBytes() + if got[0] == 'd' { + t.Fatalf("client-id starts with the identifier text, not a type byte: %v", got) + } + }) +} + +// The readable identity moves to options 12 and 60 once option 61 carries the +// chaddr, so a lease stays attributable in the upstream table. +func TestIdentityModifiersAlwaysCarryReadableIdentity(t *testing.T) { + hw, err := net.ParseMAC("02:0c:46:55:cf:ed") + if err != nil { + t.Fatalf("parse mac: %v", err) + } + + build := func(clientID, hostname, vendorClass string) *dhcpv4.DHCPv4 { + msg, err := dhcpv4.New(IdentityModifiers(clientID, hostname, vendorClass, hw)...) + if err != nil { + t.Fatalf("build message: %v", err) + } + return msg + } + + t.Run("hostname defaults to the client-id", func(t *testing.T) { + msg := build("dhcp-gw-lrp-vpc-abc", "", "") + if got := msg.HostName(); got != "dhcp-gw-lrp-vpc-abc" { + t.Fatalf("hostname = %q, want the client-id", got) + } + }) + + t.Run("vendor class defaults so leases are identifiable as ours", func(t *testing.T) { + msg := build("eni-1234", "", "") + if got := msg.ClassIdentifier(); got != defaultVendorClass { + t.Fatalf("vendor class = %q, want %q", got, defaultVendorClass) + } + }) + + t.Run("explicit values win over the defaults", func(t *testing.T) { + msg := build("eni-1234", "host-a", "acme") + if got := msg.HostName(); got != "host-a" { + t.Fatalf("hostname = %q, want host-a", got) + } + if got := msg.ClassIdentifier(); got != "acme" { + t.Fatalf("vendor class = %q, want acme", got) + } + }) +}