client4: add a new DHCPv4 client.

- Able to send UDP packets before interface is configured.
- Able to use any net.PacketConn.
- RFC2131-compliant retransmission logic.
- Tests.
- Race-condition-averse.

Previous clients (both mine and the ones here) are prone to race
condition errors.

Having one and only one place that calls receive on the socket
"continuously" without having to coordinate hand-offs makes the logic
way easier to follow, and allows for multiple requests in flux at a
time.

Signed-off-by: Christopher Koch <chrisko@google.com>

2 files changed, 74 insertions, 120 deletions

diff --git a/dhcpv4/server4/server.go b/dhcpv4/server4/server.go
index 5ef4479..1ccd5f4 100644
--- a/dhcpv4/server4/server.go
+++ b/dhcpv4/server4/server.go
@@ -1,11 +1,9 @@
 package server4
 
 import (
-	"fmt"
 	"log"
 	"net"
 	"sync"
-	"time"
 
 	"github.com/insomniacslk/dhcp/dhcpv4"
 )
@@ -36,7 +34,7 @@ import (
 	"github.com/insomniacslk/dhcp/dhcpv4"
 )
 
-func handler(conn net.PacketConn, peer net.Addr, m dhcpv4.DHCPv4) {
+func handler(conn net.PacketConn, peer net.Addr, m *dhcpv4.DHCPv4) {
 	// this function will just print the received DHCPv4 message, without replying
 	log.Print(m.Summary())
 }
@@ -46,12 +44,14 @@ func main() {
 		IP:   net.ParseIP("127.0.0.1"),
 		Port: 67,
 	}
-	server := dhcpv4.NewServer(laddr, handler)
-
-	defer server.Close()
-	if err := server.ActivateAndServe(); err != nil {
-		log.Panic(err)
+	server, err := dhcpv4.NewServer(laddr, handler)
+	if err != nil {
+		log.Fatal(err)
 	}
+
+	// This never returns. If you want to do other stuff, dump it into a
+	// goroutine.
+	server.Serve()
 }
 
 */
@@ -66,98 +66,61 @@ type Server struct {
 	connMutex  sync.Mutex
 	shouldStop chan bool
 	Handler    Handler
-	localAddr  net.UDPAddr
-}
-
-// LocalAddr returns the local address of the listening socket, or nil if not
-// listening
-func (s *Server) LocalAddr() net.Addr {
-	s.connMutex.Lock()
-	defer s.connMutex.Unlock()
-	if s.conn == nil {
-		return nil
-	}
-	return s.conn.LocalAddr()
 }
 
-// ActivateAndServe starts the DHCPv4 server. The listener will run in
-// background, and can be interrupted with `Server.Close`.
-func (s *Server) ActivateAndServe() error {
-	s.connMutex.Lock()
-	if s.conn != nil {
-		// this may panic if s.conn is closed but not reset properly. For that
-		// you should use `Server.Close`.
-		s.Close()
-	}
-	conn, err := net.ListenUDP("udp4", &s.localAddr)
-	if err != nil {
-		s.connMutex.Unlock()
-		return err
-	}
-	s.conn = conn
-	s.connMutex.Unlock()
-	var (
-		pc *net.UDPConn
-		ok bool
-	)
-	if pc, ok = s.conn.(*net.UDPConn); !ok {
-		return fmt.Errorf("error: not an UDPConn")
-	}
-	if pc == nil {
-		return fmt.Errorf("ActivateAndServe: invalid nil PacketConn")
-	}
-	log.Printf("Server listening on %s", pc.LocalAddr())
+// Serve serves requests.
+func (s *Server) Serve() {
+	log.Printf("Server listening on %s", s.conn.LocalAddr())
 	log.Print("Ready to handle requests")
 	for {
-		select {
-		case <-s.shouldStop:
-			break
-		case <-time.After(time.Millisecond):
-		}
-		pc.SetReadDeadline(time.Now().Add(time.Second))
 		rbuf := make([]byte, 4096) // FIXME this is bad
-		n, peer, err := pc.ReadFrom(rbuf)
+		n, peer, err := s.conn.ReadFrom(rbuf)
 		if err != nil {
-			switch err.(type) {
-			case net.Error:
-				if !err.(net.Error).Timeout() {
-					return err
-				}
-				// if timeout, silently skip and continue
-			default:
-				// complain and continue
-				log.Printf("Error reading from packet conn: %v", err)
-			}
-			continue
+			log.Printf("Error reading from packet conn: %v", err)
+			return
 		}
 		log.Printf("Handling request from %v", peer)
+
 		m, err := dhcpv4.FromBytes(rbuf[:n])
 		if err != nil {
 			log.Printf("Error parsing DHCPv4 request: %v", err)
 			continue
 		}
-		go s.Handler(pc, peer, m)
+		go s.Handler(s.conn, peer, m)
 	}
 }
 
 // Close sends a termination request to the server, and closes the UDP listener
 func (s *Server) Close() error {
-	s.shouldStop <- true
-	s.connMutex.Lock()
-	defer s.connMutex.Unlock()
-	if s.conn != nil {
-		ret := s.conn.Close()
-		s.conn = nil
-		return ret
+	return s.conn.Close()
+}
+
+// ServerOpt adds optional configuration to a server.
+type ServerOpt func(s *Server)
+
+// WithConn configures the server with the given connection.
+func WithConn(c net.PacketConn) ServerOpt {
+	return func(s *Server) {
+		s.conn = c
 	}
-	return nil
 }
 
 // NewServer initializes and returns a new Server object
-func NewServer(addr net.UDPAddr, handler Handler) *Server {
-	return &Server{
-		localAddr:  addr,
+func NewServer(addr *net.UDPAddr, handler Handler, opt ...ServerOpt) (*Server, error) {
+	s := &Server{
 		Handler:    handler,
 		shouldStop: make(chan bool, 1),
 	}
+
+	for _, o := range opt {
+		o(s)
+	}
+	if s.conn == nil {
+		var err error
+		s.conn, err = net.ListenUDP("udp4", addr)
+		if err != nil {
+			return nil, err
+		}
+	}
+	return s, nil
 }
diff --git a/dhcpv4/server4/server_test.go b/dhcpv4/server4/server_test.go
index f6ce18e..a6895d2 100644
--- a/dhcpv4/server4/server_test.go
+++ b/dhcpv4/server4/server_test.go
@@ -1,8 +1,9 @@
-// +build integration
+// +build go1.12
 
 package server4
 
 import (
+	"context"
 	"log"
 	"math/rand"
 	"net"
@@ -10,7 +11,7 @@ import (
 	"time"
 
 	"github.com/insomniacslk/dhcp/dhcpv4"
-	"github.com/insomniacslk/dhcp/dhcpv4/client4"
+	"github.com/insomniacslk/dhcp/dhcpv4/nclient4"
 	"github.com/insomniacslk/dhcp/interfaces"
 	"github.com/stretchr/testify/require"
 )
@@ -61,68 +62,58 @@ func DORAHandler(conn net.PacketConn, peer net.Addr, m *dhcpv4.DHCPv4) {
 
 // utility function to set up a client and a server instance and run it in
 // background. The caller needs to call Server.Close() once finished.
-func setUpClientAndServer(handler Handler) (*client4.Client, *Server) {
+func setUpClientAndServer(t *testing.T, iface net.Interface, handler Handler) (*nclient4.Client, *Server) {
 	// strong assumption, I know
 	loAddr := net.ParseIP("127.0.0.1")
-	laddr := net.UDPAddr{
+	saddr := net.UDPAddr{
 		IP:   loAddr,
 		Port: randPort(),
 	}
-	s := NewServer(laddr, handler)
-	go s.ActivateAndServe()
-
-	c := client4.NewClient()
-	// FIXME this doesn't deal well with raw sockets, the actual 0 will be used
-	// in the UDP header as source port
-	c.LocalAddr = &net.UDPAddr{IP: loAddr, Port: randPort()}
-	for {
-		if s.LocalAddr() != nil {
-			break
-		}
-		time.Sleep(10 * time.Millisecond)
-		log.Printf("Waiting for server to run...")
+	caddr := net.UDPAddr{
+		IP:   loAddr,
+		Port: randPort(),
 	}
-	c.RemoteAddr = s.LocalAddr()
-	log.Printf("Client.RemoteAddr: %s", c.RemoteAddr)
-
-	return c, s
-}
-
-func TestNewServer(t *testing.T) {
-	laddr := net.UDPAddr{
-		IP:   net.ParseIP("127.0.0.1"),
-		Port: 0,
+	s, err := NewServer(&saddr, handler)
+	if err != nil {
+		t.Fatal(err)
 	}
-	s := NewServer(laddr, DORAHandler)
-	defer s.Close()
+	go s.Serve()
 
-	require.NotNil(t, s)
-	require.Nil(t, s.conn)
-	require.Equal(t, laddr, s.localAddr)
-	require.NotNil(t, s.Handler)
+	clientConn, err := nclient4.NewIPv4UDPConn("", caddr.Port)
+	if err != nil {
+		t.Fatal(err)
+	}
+	c := nclient4.NewWithConn(clientConn, iface.HardwareAddr, nclient4.WithServerAddr(&saddr))
+	return c, s
 }
 
-func TestServerActivateAndServe(t *testing.T) {
-	c, s := setUpClientAndServer(DORAHandler)
-	defer s.Close()
-
+func TestServer(t *testing.T) {
 	ifaces, err := interfaces.GetLoopbackInterfaces()
 	require.NoError(t, err)
 	require.NotEqual(t, 0, len(ifaces))
 
+	// lo has a HardwareAddr of "nil". The client will drop all packets
+	// that don't match the HWAddr of the client interface.
+	hwaddr := net.HardwareAddr{1, 2, 3, 4, 5, 6}
+	ifaces[0].HardwareAddr = hwaddr
+
+	c, s := setUpClientAndServer(t, ifaces[0], DORAHandler)
+	defer func() {
+		require.Nil(t, s.Close())
+	}()
+
 	xid := dhcpv4.TransactionID{0xaa, 0xbb, 0xcc, 0xdd}
-	hwaddr := net.HardwareAddr{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf}
 
 	modifiers := []dhcpv4.Modifier{
 		dhcpv4.WithTransactionID(xid),
-		dhcpv4.WithHwAddr(hwaddr),
+		dhcpv4.WithHwAddr(ifaces[0].HardwareAddr),
 	}
 
-	conv, err := c.Exchange(ifaces[0].Name, modifiers...)
+	offer, ack, err := c.Request(context.Background(), modifiers...)
 	require.NoError(t, err)
-	require.Equal(t, 4, len(conv))
-	for _, p := range conv {
+	require.NotNil(t, offer, ack)
+	for _, p := range []*dhcpv4.DHCPv4{offer, ack} {
 		require.Equal(t, xid, p.TransactionID)
-		require.Equal(t, hwaddr, p.ClientHWAddr)
+		require.Equal(t, ifaces[0].HardwareAddr, p.ClientHWAddr)
 	}
 }