Merge pull request #1723 from MaxRink/fix/fix-dhcp-robustness

fix(vip): harden DHCP client lifecycle and reply parsing

pkg/vip/dhcpv4.go

@@ -132,7 +132,10 @@ func (c *DHCPv4Client) Start(ctx context.Context) error {
 	dhcpCtx, cancel := context.WithCancel(ctx)
 	defer cancel()
 
-	lease := c.requestWithBackoff(dhcpCtx)
+	lease, err := c.requestWithBackoff(dhcpCtx)
+	if err != nil {
+		return fmt.Errorf("DHCPv4 client failed: %w", err)
+	}
 
 	c.initRebootFlag = false
 	c.lease = lease
@@ -173,7 +176,12 @@ func (c *DHCPv4Client) Start(ctx context.Context) error {
 				}
 				log.Warn("[DHCPv4] ip may have changed", "ip", c.lease.ACK.YourIPAddr, "err", err)
 				c.initRebootFlag = false
-				c.lease = c.requestWithBackoff(dhcpCtx)
+				lease, backoffErr := c.requestWithBackoff(dhcpCtx)
+				if backoffErr != nil {
+					log.Error("[DHCPv4] failed to reacquire lease", "err", backoffErr)
+					continue
+				}
+				c.lease = lease
 			}
 			t1.Reset(t1Timeout)
 			t2.Reset(t2Timeout)
@@ -204,7 +212,7 @@ func (c *DHCPv4Client) Start(ctx context.Context) error {
 // |ciaddr        |zero         | IP address   |IP address|
 // --------------------------------------------------------
 
-func (c *DHCPv4Client) requestWithBackoff(ctx context.Context) *nclient4.Lease {
+func (c *DHCPv4Client) requestWithBackoff(ctx context.Context) (*nclient4.Lease, error) {
 	backoff := backoff.Backoff{
 		Factor: 2,
 		Jitter: true,
@@ -226,8 +234,7 @@ func (c *DHCPv4Client) requestWithBackoff(ctx context.Context) *nclient4.Lease {
 				errMsg := fmt.Errorf("failed to get an IPv4 address after %d attempt(s), giving up, error: %s", c.backoffAttempts, err.Error())
 				log.Error(fmt.Sprintf("[DHCPv4] %s", errMsg.Error()))
 				c.errorChan <- errMsg
-				c.Stop()
-				return nil
+				return nil, errMsg
 			}
 			log.Error("[DHCPv4] request failed", "attempt", backoff.Attempt(), "err", err.Error(), "waiting", dur)
 			time.Sleep(dur)
@@ -242,7 +249,7 @@ func (c *DHCPv4Client) requestWithBackoff(ctx context.Context) *nclient4.Lease {
 		c.ipChan <- lease.ACK.YourIPAddr.String()
 	}
 
-	return lease
+	return lease, nil
 }
 
 func (c *DHCPv4Client) request(ctx context.Context, rebind bool) (*nclient4.Lease, error) {

pkg/vip/dhcpv4_backoff_test.go

@@ -0,0 +1,40 @@
+package vip
+
+import (
+	"context"
+	"net"
+	"testing"
+	"time"
+)
+
+func TestDHCPv4BackoffExhaustionDoesNotDeadlock(t *testing.T) {
+	// DEFECT: requestWithBackoff calls Stop from the Start/request goroutine before Start can close releasedChan, so exhausted DHCPv4 retries deadlock (pkg/vip/dhcpv4.go:229).
+	client := NewDHCPv4Client(
+		&net.Interface{Name: "definitely-not-a-kube-vip-interface"},
+		false,
+		"",
+		1,
+		false,
+	)
+
+	done := make(chan struct{})
+	go func() {
+		_, _ = client.requestWithBackoff(context.Background())
+		close(done)
+	}()
+
+	select {
+	case err := <-client.ErrorChannel():
+		if err == nil {
+			t.Fatal("expected DHCPv4 request error")
+		}
+	case <-time.After(time.Second):
+		t.Fatal("DHCPv4 did not report exhausted backoff")
+	}
+
+	select {
+	case <-done:
+	case <-time.After(100 * time.Millisecond):
+		t.Fatal("DHCPv4 backoff exhaustion deadlocked")
+	}
+}

pkg/vip/dhcpv6.go

@@ -90,6 +90,7 @@ func NewDHCPv6InternalClient(iface string) (*DHCPv6InternalClient, error) {
 
 type DHCPv6Client struct {
 	iface           *net.Interface
+	managerKey      string
 	ddnsHostName    string
 	initRebootFlag  bool
 	requestedIP     net.IP
@@ -117,6 +118,7 @@ func NewDHCPv6Client(iface *net.Interface, parent netlink.Link, initRebootFlag b
 
 	return &DHCPv6Client{
 		iface:           iface,
+		managerKey:      name,
 		stopChan:        make(chan struct{}),
 		releasedChan:    make(chan struct{}),
 		errorChan:       make(chan error),
@@ -140,7 +142,7 @@ func (c *DHCPv6Client) Stop() {
 		close(c.stopChan)
 	})
 	<-c.releasedChan
-	dhcpv6ClientManager.Delete(c.iface.Name)
+	dhcpv6ClientManager.Delete(c.managerKey)
 }
 
 // Gets the IPChannel for consumption
@@ -247,7 +249,6 @@ func (c *DHCPv6Client) requestWithBackoff(ctx context.Context) (*dhcpv6.OptIAAdd
 				errMsg := fmt.Errorf("failed to get an IPv4 address after %d attempt(s), giving up, error: %s", c.backoffAttempts, err.Error())
 				log.Error(fmt.Sprintf("[DHCPv6] %s", errMsg.Error()))
 				c.errorChan <- errMsg
-				c.Stop()
 				return nil, fmt.Errorf("failed to get IPv6 address: %w", err)
 			}
 			log.Error("[DHCPv6] request failed", "attempt", backoff.Attempt(), "err", err.Error(), "waiting", dur)
@@ -385,9 +386,10 @@ func getAddress(iana []*dhcpv6.OptIANA) (*dhcpv6.OptIAAddress, error) {
 		return nil, fmt.Errorf("failed to get IANA")
 	}
 
-	if len(iana) < 1 {
-		return nil, fmt.Errorf("failed to get addresses data")
+	addrs := iana[0].Options.Addresses()
+	if len(addrs) == 0 {
+		return nil, fmt.Errorf("IANA contained no addresses")
 	}
 
-	return iana[0].Options.Addresses()[0], nil
+	return addrs[0], nil
 }

pkg/vip/dhcpv6_robustness_test.go

@@ -0,0 +1,51 @@
+package vip
+
+import (
+	"net"
+	"sync/atomic"
+	"testing"
+
+	"github.com/insomniacslk/dhcp/dhcpv6"
+)
+
+func TestDHCPv6StopReleasesManagerReferenceForParentInterface(t *testing.T) {
+	// DEFECT: Stop deletes the manager entry using the VLAN child name even though NewDHCPv6Client keyed the shared client by its parent name (pkg/vip/dhcpv6.go:143).
+	previousManager := dhcpv6ClientManager
+	t.Cleanup(func() { dhcpv6ClientManager = previousManager })
+
+	references := &atomic.Int32{}
+	references.Store(2)
+	shared := &DHCPv6InternalClient{references: references}
+	dhcpv6ClientManager = &DHCPv6ClientManager{
+		clients: map[string]*DHCPv6InternalClient{"parent0": shared},
+	}
+
+	client := &DHCPv6Client{
+		iface:        &net.Interface{Name: "vlan-child"},
+		managerKey:   "parent0",
+		ipChan:       make(chan string),
+		stopChan:     make(chan struct{}),
+		releasedChan: make(chan struct{}),
+		ic:           shared,
+	}
+	close(client.releasedChan)
+
+	client.Stop()
+
+	if got := references.Load(); got != 1 {
+		t.Fatalf("manager reference count = %d, want 1 after stopping one VLAN client", got)
+	}
+}
+
+func TestGetAddressRejectsIANAWithoutAddresses(t *testing.T) {
+	// DEFECT: getAddress indexes the first IAADDR without checking whether the IANA contains one, so a malformed/expired reply panics (pkg/vip/dhcpv6.go:392).
+	defer func() {
+		if recovered := recover(); recovered != nil {
+			t.Fatalf("getAddress panicked on an IANA without IAADDR: %v", recovered)
+		}
+	}()
+
+	if _, err := getAddress([]*dhcpv6.OptIANA{{}}); err == nil {
+		t.Fatal("getAddress accepted an IANA without an IAADDR")
+	}
+}