[plugins/range] use sqlite3 instead of text file

The range plugin appends every new entry to a text file, without
deduplication, and collisions are possible.
Implementing deduplication on a text file is either inefficient or
tricky. Detecting collisions is inefficient, no matter if it's done in-memory
and then rewriting the entire text file every time, or by parsing the text
file every time.

Sqlite3 offers consistency, uniqueness without performance hit, and can
enforce more complex constraints.

However using sqlite3 requires using CGo, which has a bunch of issues:
* it complicates cross-compiling
* builds are slower
* deployments are more complicated (no more single binary, not easily at least)

I have been running CoreDHCP with the sqlite3-based range plugin in my
home network without issues so far, and no more duplication nor
collisions.

Signed-off-by: Andrea Barberio <insomniac@slackware.it>

go.mod

@@ -8,6 +8,7 @@ require (
 	github.com/fsnotify/fsnotify v1.6.0
 	github.com/google/gopacket v1.1.19
 	github.com/insomniacslk/dhcp v0.0.0-20230731140434-0f9eb93a696c
+	github.com/mattn/go-sqlite3 v2.0.3+incompatible
 	github.com/rifflock/lfshook v0.0.0-20180920164130-b9218ef580f5
 	github.com/sirupsen/logrus v1.9.3
 	github.com/spf13/cast v1.5.1
@@ -28,7 +29,7 @@ require (
 	github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d // indirect
 	github.com/mitchellh/mapstructure v1.5.0 // indirect
 	github.com/onsi/ginkgo v1.14.0 // indirect
-	github.com/onsi/gomega v1.10.1 // indirect
+	github.com/onsi/gomega v1.27.10 // indirect
 	github.com/pelletier/go-toml/v2 v2.0.9 // indirect
 	github.com/pierrec/lz4/v4 v4.1.18 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
@@ -42,6 +43,5 @@ require (
 	golang.org/x/term v0.11.0 // indirect
 	golang.org/x/text v0.12.0 // indirect
 	gopkg.in/ini.v1 v1.67.0 // indirect
-	gopkg.in/yaml.v2 v2.4.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )

go.sum

@@ -152,6 +152,8 @@ github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovk
 github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM=
 github.com/mattn/go-isatty v0.0.19 h1:JITubQf0MOLdlGRuRq+jtsDlekdYPia9ZFsB8h/APPA=
 github.com/mattn/go-isatty v0.0.19/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y=
+github.com/mattn/go-sqlite3 v2.0.3+incompatible h1:gXHsfypPkaMZrKbD5209QV9jbUTJKjyR5WD3HYQSd+U=
+github.com/mattn/go-sqlite3 v2.0.3+incompatible/go.mod h1:FPy6KqzDD04eiIsT53CuJW3U88zkxoIYsOqkbpncsNc=
 github.com/mdlayher/packet v1.1.1 h1:7Fv4OEMYqPl7//uBm04VgPpnSNi8fbBZznppgh6WMr8=
 github.com/mdlayher/socket v0.4.0 h1:280wsy40IC9M9q1uPGcLBwXpcTQDtoGwVt+BNoITxIw=
 github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d h1:5PJl274Y63IEHC+7izoQE9x6ikvDFZS2mDVS3drnohI=
@@ -165,8 +167,9 @@ github.com/onsi/ginkgo v1.12.1/go.mod h1:zj2OWP4+oCPe1qIXoGWkgMRwljMUYCdkwsT2108
 github.com/onsi/ginkgo v1.14.0 h1:2mOpI4JVVPBN+WQRa0WKH2eXR+Ey+uK4n7Zj0aYpIQA=
 github.com/onsi/ginkgo v1.14.0/go.mod h1:iSB4RoI2tjJc9BBv4NKIKWKya62Rps+oPG/Lv9klQyY=
 github.com/onsi/gomega v1.7.1/go.mod h1:XdKZgCCFLUoM/7CFJVPcG8C1xQ1AJ0vpAezJrB7JYyY=
-github.com/onsi/gomega v1.10.1 h1:o0+MgICZLuZ7xjH7Vx6zS/zcu93/BEp1VwkIW1mEXCE=
 github.com/onsi/gomega v1.10.1/go.mod h1:iN09h71vgCQne3DLsj+A5owkum+a2tYe+TOCB1ybHNo=
+github.com/onsi/gomega v1.27.10 h1:naR28SdDFlqrG6kScpT8VWpu1xWY5nJRCF3XaYyBjhI=
+github.com/onsi/gomega v1.27.10/go.mod h1:RsS8tutOdbdgzbPtzzATp12yT7kM5I5aElG3evPbQ0M=
 github.com/pelletier/go-toml/v2 v2.0.9 h1:uH2qQXheeefCCkuBBSLi7jCiSmj3VRh2+Goq2N7Xxu0=
 github.com/pelletier/go-toml/v2 v2.0.9/go.mod h1:tJU2Z3ZkXwnxa4DPO899bsyIoywizdUvyaeZurnPPDc=
 github.com/pierrec/lz4/v4 v4.1.14/go.mod h1:gZWDp/Ze/IJXGXf23ltt2EXimqmTUXEy0GFuRQyBid4=
@@ -436,7 +439,6 @@ golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8T
 golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
-golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 h1:H2TDz8ibqkAF6YGhCdN3jS9O0/s90v0rJh3X/OLHEUk=
 google.golang.org/api v0.4.0/go.mod h1:8k5glujaEP+g9n7WNsDg8QP6cUVNI86fCNMcbazEtwE=
 google.golang.org/api v0.7.0/go.mod h1:WtwebWUNSVBH/HAw79HIFXZNqEvBhG+Ra+ax0hx3E3M=
 google.golang.org/api v0.8.0/go.mod h1:o4eAsZoiT+ibD93RtjEohWalFOjRDx6CVaqeizhEnKg=
@@ -537,8 +539,6 @@ gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7/go.mod h1:dt/ZhP58zS4L8KSrWD
 gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 gopkg.in/yaml.v2 v2.3.0/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
-gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY=
-gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ=
 gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
 gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
 gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=

plugins/range/plugin.go

@@ -5,11 +5,11 @@
 package rangeplugin
 
 import (
+	"database/sql"
 	"encoding/binary"
 	"errors"
 	"fmt"
 	"net"
-	"os"
 	"sync"
 	"time"
 
@@ -32,7 +32,7 @@ var Plugin = plugins.Plugin{
 //Record holds an IP lease record
 type Record struct {
 	IP      net.IP
-	expires time.Time
+	expires int
 }
 
 // PluginState is the data held by an instance of the range plugin
@@ -42,7 +42,7 @@ type PluginState struct {
 	// Recordsv4 holds a MAC -> IP address and lease time mapping
 	Recordsv4 map[string]*Record
 	LeaseTime time.Duration
-	leasefile *os.File
+	leasedb   *sql.DB
 	allocator allocators.Allocator
 }
 
@@ -61,7 +61,7 @@ func (p *PluginState) Handler4(req, resp *dhcpv4.DHCPv4) (*dhcpv4.DHCPv4, bool)
 		}
 		rec := Record{
 			IP:      ip.IP.To4(),
-			expires: time.Now().Add(p.LeaseTime),
+			expires: int(time.Now().Add(p.LeaseTime).Unix()),
 		}
 		err = p.saveIPAddress(req.ClientHWAddr, &rec)
 		if err != nil {
@@ -71,8 +71,9 @@ func (p *PluginState) Handler4(req, resp *dhcpv4.DHCPv4) (*dhcpv4.DHCPv4, bool)
 		record = &rec
 	} else {
 		// Ensure we extend the existing lease at least past when the one we're giving expires
-		if record.expires.Before(time.Now().Add(p.LeaseTime)) {
-			record.expires = time.Now().Add(p.LeaseTime).Round(time.Second)
+		expiry := time.Unix(int64(record.expires), 0)
+		if expiry.Before(time.Now().Add(p.LeaseTime)) {
+			record.expires = int(time.Now().Add(p.LeaseTime).Round(time.Second).Unix())
 			err := p.saveIPAddress(req.ClientHWAddr, record)
 			if err != nil {
 				log.Errorf("Could not persist lease for MAC %s: %v", req.ClientHWAddr.String(), err)
@@ -120,7 +121,10 @@ func setupRange(args ...string) (handler.Handler4, error) {
 		return nil, fmt.Errorf("invalid lease duration: %v", args[3])
 	}
 
-	p.Recordsv4, err = loadRecordsFromFile(filename)
+	if err := p.registerBackingDB(filename); err != nil {
+		return nil, fmt.Errorf("could not setup lease storage: %w", err)
+	}
+	p.Recordsv4, err = loadRecords(p.leasedb)
 	if err != nil {
 		return nil, fmt.Errorf("could not load records from file: %v", err)
 	}
@@ -137,9 +141,5 @@ func setupRange(args ...string) (handler.Handler4, error) {
 		}
 	}
 
-	if err := p.registerBackingFile(filename); err != nil {
-		return nil, fmt.Errorf("could not setup lease storage: %w", err)
-	}
-
 	return p.Handler4, nil
 }

plugins/range/storage.go

@@ -5,86 +5,87 @@
 package rangeplugin
 
 import (
-	"bufio"
+	"database/sql"
 	"errors"
 	"fmt"
-	"io"
 	"net"
-	"os"
-	"strings"
-	"time"
+
+	_ "github.com/mattn/go-sqlite3"
 )
 
+func loadDB(path string) (*sql.DB, error) {
+	db, err := sql.Open("sqlite3", fmt.Sprintf("file:%s", path))
+	if err != nil {
+		return nil, fmt.Errorf("failed to open database (%T): %w", err, err)
+	}
+	if _, err := db.Exec("create table if not exists leases4 (mac string not null, ip string not null, expiry int, primary key (mac, ip))"); err != nil {
+		return nil, fmt.Errorf("table creation failed: %w", err)
+	}
+	return db, nil
+}
+
 // loadRecords loads the DHCPv6/v4 Records global map with records stored on
 // the specified file. The records have to be one per line, a mac address and an
 // IP address.
-func loadRecords(r io.Reader) (map[string]*Record, error) {
-	sc := bufio.NewScanner(r)
-	records := make(map[string]*Record)
-	for sc.Scan() {
-		line := sc.Text()
-		if len(line) == 0 {
-			continue
-		}
-		tokens := strings.Fields(line)
-		if len(tokens) != 3 {
-			return nil, fmt.Errorf("malformed line, want 3 fields, got %d: %s", len(tokens), line)
+func loadRecords(db *sql.DB) (map[string]*Record, error) {
+	rows, err := db.Query("select mac, ip, expiry from leases4")
+	if err != nil {
+		return nil, fmt.Errorf("failed to query leases database: %w", err)
+	}
+	defer rows.Close()
+	var (
+		mac, ip string
+		expiry  int
+		records = make(map[string]*Record)
+	)
+	for rows.Next() {
+		if err := rows.Scan(&mac, &ip, &expiry); err != nil {
+			return nil, fmt.Errorf("failed to scan row: %w", err)
 		}
-		hwaddr, err := net.ParseMAC(tokens[0])
+		hwaddr, err := net.ParseMAC(mac)
 		if err != nil {
-			return nil, fmt.Errorf("malformed hardware address: %s", tokens[0])
+			return nil, fmt.Errorf("malformed hardware address: %s", mac)
 		}
-		ipaddr := net.ParseIP(tokens[1])
+		ipaddr := net.ParseIP(ip)
 		if ipaddr.To4() == nil {
 			return nil, fmt.Errorf("expected an IPv4 address, got: %v", ipaddr)
 		}
-		expires, err := time.Parse(time.RFC3339, tokens[2])
-		if err != nil {
-			return nil, fmt.Errorf("expected time of exipry in RFC3339 format, got: %v", tokens[2])
-		}
-		records[hwaddr.String()] = &Record{IP: ipaddr, expires: expires}
+		records[hwaddr.String()] = &Record{IP: ipaddr, expires: expiry}
 	}
-	return records, nil
-}
-
-func loadRecordsFromFile(filename string) (map[string]*Record, error) {
-	reader, err := os.OpenFile(filename, os.O_RDWR|os.O_CREATE, 0640)
-	defer func() {
-		if err := reader.Close(); err != nil {
-			log.Warningf("Failed to close file %s: %v", filename, err)
-		}
-	}()
-	if err != nil {
-		return nil, fmt.Errorf("cannot open lease file %s: %w", filename, err)
+	if err := rows.Err(); err != nil {
+		return nil, fmt.Errorf("failed lease database row scanning: %w", err)
 	}
-	return loadRecords(reader)
+	return records, nil
 }
 
 // saveIPAddress writes out a lease to storage
 func (p *PluginState) saveIPAddress(mac net.HardwareAddr, record *Record) error {
-	_, err := p.leasefile.WriteString(mac.String() + " " + record.IP.String() + " " + record.expires.Format(time.RFC3339) + "\n")
+	stmt, err := p.leasedb.Prepare("insert into leases4(mac, ip, expiry) select ?, ?, ? where not exists (select 1 from leases4 where mac = ? and ip = ? and expiry < TIME())")
 	if err != nil {
-		return err
+		return fmt.Errorf("statement preparation failed: %w", err)
 	}
-	err = p.leasefile.Sync()
-	if err != nil {
-		return err
+	if _, err := stmt.Exec(
+		mac.String(),
+		record.IP.String(),
+		record.expires,
+		mac.String(),
+		record.IP.String(),
+	); err != nil {
+		return fmt.Errorf("record insert/update failed: %w", err)
 	}
 	return nil
 }
 
-// registerBackingFile installs a file as the backing store for leases
-func (p *PluginState) registerBackingFile(filename string) error {
-	if p.leasefile != nil {
-		// This is TODO; swapping the file out is easy
-		// but maintaining consistency with the in-memory state isn't
-		return errors.New("cannot swap out a lease storage file while running")
+// registerBackingDB installs a database connection string as the backing store for leases
+func (p *PluginState) registerBackingDB(filename string) error {
+	if p.leasedb != nil {
+		return errors.New("cannot swap out a lease database while running")
 	}
 	// We never close this, but that's ok because plugins are never stopped/unregistered
-	newLeasefile, err := os.OpenFile(filename, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
+	newLeaseDB, err := loadDB(filename)
 	if err != nil {
-		return fmt.Errorf("failed to open lease file %s: %w", filename, err)
+		return fmt.Errorf("failed to open lease database %s: %w", filename, err)
 	}
-	p.leasefile = newLeasefile
+	p.leasedb = newLeaseDB
 	return nil
 }

plugins/range/storage_test.go

@@ -5,25 +5,34 @@
 package rangeplugin
 
 import (
-	"io/ioutil"
+	"database/sql"
+	"fmt"
 	"net"
-	"os"
-	"strings"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
 )
 
-var leasefile string = `02:00:00:00:00:00 10.0.0.0 2000-01-01T00:00:00Z
-02:00:00:00:00:01 10.0.0.1 2000-01-01T00:00:00Z
-02:00:00:00:00:02 10.0.0.2 2000-01-01T00:00:00Z
-02:00:00:00:00:03 10.0.0.3 2000-01-01T00:00:00Z
-02:00:00:00:00:04 10.0.0.4 2000-01-01T00:00:00Z
-02:00:00:00:00:05 10.0.0.5 2000-01-01T00:00:00Z
-`
+func testDBSetup() (*sql.DB, error) {
+	db, err := loadDB(":memory:")
+	if err != nil {
+		return nil, err
+	}
+	for _, record := range records {
+		stmt, err := db.Prepare("insert into leases4(mac, ip, expiry) values (?, ?, ?)")
+		if err != nil {
+			return nil, fmt.Errorf("failed to prepare insert statement: %w", err)
+		}
+		defer stmt.Close()
+		if _, err := stmt.Exec(record.mac, record.ip.IP.String(), record.ip.expires); err != nil {
+			return nil, fmt.Errorf("failed to insert record into test db: %w", err)
+		}
+	}
+	return db, nil
+}
 
-var expire = time.Date(2000, 01, 01, 00, 00, 00, 00, time.UTC)
+var expire = int(time.Date(2000, 01, 01, 00, 00, 00, 00, time.UTC).Unix())
 var records = []struct {
 	mac string
 	ip  *Record
@@ -37,33 +46,38 @@ var records = []struct {
 }
 
 func TestLoadRecords(t *testing.T) {
-	parsedRec, err := loadRecords(strings.NewReader(leasefile))
+	db, err := testDBSetup()
+	if err != nil {
+		t.Fatalf("Failed to set up test DB: %v", err)
+	}
+
+	parsedRec, err := loadRecords(db)
 	if err != nil {
 		t.Fatalf("Failed to load records from file: %v", err)
 	}
 
 	mapRec := make(map[string]*Record)
 	for _, rec := range records {
-		mapRec[rec.mac] = rec.ip
+		var (
+			ip, mac string
+			expiry  int
+		)
+		if err := db.QueryRow("select mac, ip, expiry from leases4 where mac = ?", rec.mac).Scan(&mac, &ip, &expiry); err != nil {
+			t.Fatalf("record not found for mac=%s: %v", rec.mac, err)
+		}
+		mapRec[mac] = &Record{IP: net.ParseIP(ip), expires: expiry}
 	}
 
-	assert.Equal(t, mapRec, parsedRec, "Loaded records differ from what's in the file")
+	assert.Equal(t, mapRec, parsedRec, "Loaded records differ from what's in the DB")
 }
 
 func TestWriteRecords(t *testing.T) {
-	tmpfile, err := ioutil.TempFile("", "coredhcptest")
-	if err != nil {
-		t.Skipf("Could not setup file-based test: %v", err)
-	}
-	defer os.Remove(tmpfile.Name())
-	defer tmpfile.Close()
-
 	pl := PluginState{}
-	if err := pl.registerBackingFile(tmpfile.Name()); err != nil {
+	if err := pl.registerBackingDB(":memory:"); err != nil {
 		t.Fatalf("Could not setup file")
 	}
-	defer pl.leasefile.Close()
 
+	mapRec := make(map[string]*Record)
 	for _, rec := range records {
 		hwaddr, err := net.ParseMAC(rec.mac)
 		if err != nil {
@@ -73,15 +87,13 @@ func TestWriteRecords(t *testing.T) {
 		if err := pl.saveIPAddress(hwaddr, rec.ip); err != nil {
 			t.Errorf("Failed to save ip for %s: %v", hwaddr, err)
 		}
+		mapRec[hwaddr.String()] = &Record{IP: rec.ip.IP, expires: rec.ip.expires}
 	}
 
-	if _, err := tmpfile.Seek(0, 0); err != nil {
+	parsedRec, err := loadRecords(pl.leasedb)
+	if err != nil {
 		t.Fatal(err)
 	}
 
-	written, err := ioutil.ReadAll(tmpfile)
-	if err != nil {
-		t.Fatalf("Could not read back temp file")
-	}
-	assert.Equal(t, leasefile, string(written), "Data written to the file doesn't match records")
+	assert.Equal(t, mapRec, parsedRec, "Loaded records differ from what's in the DB")
 }