dhcp plugin: send dhcp release from leased address

plugins/ipam/dhcp/dhcp_test.go

@@ -16,6 +16,7 @@ package main
 
 import (
 	"bytes"
+	"encoding/binary"
 	"fmt"
 	"io"
 	"net"
@@ -25,6 +26,7 @@ import (
 	"sync"
 	"time"
 
+	dhcp4 "github.com/insomniacslk/dhcp/dhcpv4"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/vishvananda/netlink"
@@ -136,6 +138,57 @@ const (
 	cniDirPrefix string = "/var/run/cni"
 )
 
+type capturedRelease struct {
+	srcIP net.IP
+	msg   *dhcp4.DHCPv4
+}
+
+// snoopRelease opens a raw socket in the given netns, bound to the DHCP
+// server address, and returns a channel that receives the first DHCPRELEASE
+// observed along with the IP source address it was sent from. The channel is
+// closed without a value if no RELEASE arrives before the deadline.
+func snoopRelease(netns ns.NetNS, serverIP string) (chan capturedRelease, error) {
+	var conn net.PacketConn
+	err := netns.Do(func(ns.NetNS) error {
+		var err error
+		conn, err = net.ListenPacket("ip4:udp", serverIP)
+		return err
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	releaseCh := make(chan capturedRelease, 1)
+	go func() {
+		defer GinkgoRecover()
+		defer conn.Close()
+
+		_ = conn.SetReadDeadline(time.Now().Add(30 * time.Second))
+		buf := make([]byte, 1500)
+		for {
+			n, src, err := conn.ReadFrom(buf)
+			if err != nil {
+				close(releaseCh)
+				return
+			}
+			// buf holds a UDP datagram (the kernel strips the IPv4
+			// header); filter on destination port 67 and parse the
+			// DHCP payload past the 8-byte UDP header.
+			if n < 8 || binary.BigEndian.Uint16(buf[2:4]) != 67 {
+				continue
+			}
+			msg, err := dhcp4.FromBytes(buf[8:n])
+			if err != nil || msg.MessageType() != dhcp4.MessageTypeRelease {
+				continue
+			}
+			releaseCh <- capturedRelease{srcIP: src.(*net.IPAddr).IP, msg: msg}
+			return
+		}
+	}()
+
+	return releaseCh, nil
+}
+
 var _ = BeforeSuite(func() {
 	err := os.MkdirAll(cniDirPrefix, 0o700)
 	Expect(err).NotTo(HaveOccurred())
@@ -303,6 +356,71 @@ var _ = Describe("DHCP Operations", func() {
 			Expect(err).NotTo(HaveOccurred())
 		})
 
+		It(fmt.Sprintf("[%s] sends a unicast DHCPRELEASE sourced from the leased address on DEL", ver), func() {
+			conf := fmt.Sprintf(`{
+			    "cniVersion": "%s",
+			    "name": "mynet",
+			    "type": "ipvlan",
+			    "ipam": {
+				"type": "dhcp",
+				"daemonSocketPath": "%s"
+			    }
+			}`, ver, socketPath)
+
+			args := &skel.CmdArgs{
+				ContainerID: "dummy",
+				Netns:       targetNS.Path(),
+				IfName:      contVethName,
+				StdinData:   []byte(conf),
+			}
+
+			var addResult *types100.Result
+			err := originalNS.Do(func(ns.NetNS) error {
+				defer GinkgoRecover()
+
+				r, _, err := testutils.CmdAddWithArgs(args, func() error {
+					return cmdAdd(args)
+				})
+				Expect(err).NotTo(HaveOccurred())
+
+				addResult, err = types100.GetResult(r)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(addResult.IPs).To(HaveLen(1))
+				Expect(addResult.IPs[0].Address.String()).To(Equal("192.168.1.5/24"))
+				return nil
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			// Configure the allocated address on the container interface,
+			// as the main plugin consuming this IPAM result would; the
+			// unicast RELEASE below has to be sourced from it.
+			err = targetNS.Do(func(ns.NetNS) error {
+				defer GinkgoRecover()
+
+				link, err := netlinksafe.LinkByName(contVethName)
+				Expect(err).NotTo(HaveOccurred())
+				err = netlink.AddrAdd(link, &netlink.Addr{IPNet: &addResult.IPs[0].Address})
+				Expect(err).NotTo(HaveOccurred())
+				return nil
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			releaseCh, err := snoopRelease(originalNS, "192.168.1.1")
+			Expect(err).NotTo(HaveOccurred())
+
+			err = originalNS.Do(func(ns.NetNS) error {
+				return testutils.CmdDelWithArgs(args, func() error {
+					return cmdDel(args)
+				})
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			var release capturedRelease
+			Eventually(releaseCh, time.Second*10).Should(Receive(&release))
+			Expect(release.srcIP.String()).To(Equal("192.168.1.5"))
+			Expect(release.msg.ClientIPAddr.String()).To(Equal("192.168.1.5"))
+		})
+
 		It(fmt.Sprintf("[%s] correctly handles multiple DELs for the same container", ver), func() {
 			conf := fmt.Sprintf(`{
 			    "cniVersion": "%s",

plugins/ipam/dhcp/lease.go

@@ -428,19 +428,37 @@ func (l *DHCPLease) renew() error {
 func (l *DHCPLease) release() error {
 	log.Printf("%v: releasing lease", l.clientID)
 
-	c, err := newDHCPClient(l.link, l.timeout)
-	if err != nil {
-		return err
+	// RFC 2131 section 4.4.4 says DHCPRELEASE is unicast to the server.
+	// Send it from the leased address so the packet source matches ciaddr.
+	// A kernel UDP socket bound to the leased IP avoids the legacy raw-socket
+	// behavior where DHCPRELEASE used IP source 0.0.0.0.
+	leasedIP := l.latestLease.ACK.YourIPAddr
+	err := l.releaseWithClient(nclient4.WithUnicast(
+		&net.UDPAddr{IP: leasedIP, Port: nclient4.ClientPort}))
+	if err == nil {
+		return nil
 	}
-	defer c.Close()
 
-	if err = c.Release(l.latestLease, withClientID(l.clientID)); err != nil {
-		return fmt.Errorf("failed to send DHCPRELEASE")
+	// Fall back to the legacy raw-socket path (best effort), e.g. if the
+	// leased address or its routes were already removed from the interface.
+	log.Printf("%v: DHCPRELEASE from leased address failed (%v), falling back to legacy raw socket", l.clientID, err)
+	if err := l.releaseWithClient(); err != nil {
+		return fmt.Errorf("failed to send DHCPRELEASE: %w", err)
 	}
 
 	return nil
 }
 
+func (l *DHCPLease) releaseWithClient(clientOpts ...nclient4.ClientOpt) error {
+	c, err := newDHCPClient(l.link, l.timeout, clientOpts...)
+	if err != nil {
+		return err
+	}
+	defer c.Close()
+
+	return c.Release(l.latestLease, withClientID(l.clientID))
+}
+
 func (l *DHCPLease) IPNet() (*net.IPNet, error) {
 	ack := l.latestLease.ACK