feat: dhcpv4: send current hostname, fix spec compliance of renewals

This adds support for automatically registering node hostnames in DNS by
sending the current hostname to DHCP via option 12. If the current hostname is
updated, issue a new DISCOVER to propagate the update to DHCP (updating the
hostname on lease renewals is not universally supported by DHCP servers). This
addition maintains the previous functionality where the node can also request
its hostname from the DHCP server. The received hostname will be processed and
prioritized as usual by the `network.HostnameSpecController`.

This change set also contains fixes to make DHCP renewals compliant with RFC
2131, specifically avoiding sending the server identifier and requested IP
address when issuing renewals using a previous offer. This also uncovered an
issue in the upstream `insomniacslk/dhcp` library, for which a fix is now
pending at insomniacslk/dhcp#469. As upstream
contributions seem to have stalled, I have temporarily added a replacement for
the library to my own fork that carries the fix.

Sending hostname updates have been tested against `dnsmasq` and the built-in
DHCP + DNS services in Windows Server. Hostname retrieval from DHCP and edge
cases with overridden hostnames from different configuration layers have been
extensively tested against `dnsmasq`.

Signed-off-by: Dennis Marttinen <[email protected]>

.golangci.yml

@@ -132,6 +132,7 @@ linters-settings:
     replace-allow-list:
       - gopkg.in/yaml.v3
       - inet.af/tcpproxy
+      - github.com/insomniacslk/dhcp
       - github.com/vmware-tanzu/sonobuoy
     retract-allow-no-explanation: false
     exclude-forbidden: true

go.mod

@@ -3,6 +3,10 @@ module github.com/talos-systems/talos
 go 1.18
 
 replace (
+	// Forked insomniacslk/dhcp with DHCP spec compliancy fixes
+	// Upstream PR: https://github.com/insomniacslk/dhcp/pull/469
+	github.com/insomniacslk/dhcp => github.com/twelho/dhcp v0.0.0-20220708120125-da9fc6c72fb5
+
 	// Use nested module.
 	github.com/talos-systems/talos/pkg/machinery => ./pkg/machinery
 

go.sum

@@ -712,8 +712,6 @@ github.com/imdario/mergo v0.3.12 h1:b6R2BslTbIEToALKP7LxUvijTsNI9TAe80pLWN2g/HU=
 github.com/imdario/mergo v0.3.12/go.mod h1:jmQim1M+e3UYxmgPu/WyfjB3N3VflVyUjjjwH0dnCYA=
 github.com/inconshreveable/mousetrap v1.0.0 h1:Z8tu5sraLXCXIcARxBp/8cbvlwVa7Z1NHg9XEKhtSvM=
 github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8=
-github.com/insomniacslk/dhcp v0.0.0-20220504074936-1ca156eafb9f h1:l1QCwn715k8nYkj4Ql50rzEog3WnMdrd4YYMMwemxEo=
-github.com/insomniacslk/dhcp v0.0.0-20220504074936-1ca156eafb9f/go.mod h1:h+MxyHxRg9NH3terB1nfRIUaQEcI0XOVkdR9LNBlp8E=
 github.com/j-keck/arping v0.0.0-20160618110441-2cf9dc699c56/go.mod h1:ymszkNOg6tORTn+6F6j+Jc8TOr5osrynvN6ivFWZ2GA=
 github.com/jmespath/go-jmespath v0.0.0-20160202185014-0b12d6b521d8/go.mod h1:Nht3zPeWKUH0NzdCt2Blrr5ys8VGpn0CEB0cQHVjt7k=
 github.com/jmespath/go-jmespath v0.0.0-20160803190731-bd40a432e4c7/go.mod h1:Nht3zPeWKUH0NzdCt2Blrr5ys8VGpn0CEB0cQHVjt7k=
@@ -1178,6 +1176,8 @@ github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8/go.mod h1
 github.com/tmc/grpc-websocket-proxy v0.0.0-20190109142713-0ad062ec5ee5/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U=
 github.com/tmc/grpc-websocket-proxy v0.0.0-20201229170055-e5319fda7802/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U=
 github.com/tv42/httpunix v0.0.0-20191220191345-2ba4b9c3382c/go.mod h1:hzIxponao9Kjc7aWznkXaL4U4TWaDSs8zcsY4Ka08nM=
+github.com/twelho/dhcp v0.0.0-20220708120125-da9fc6c72fb5 h1:oIs75geLkhMCwm3+omA0kfqUMqMnBmEbkZPiGFG3MNE=
+github.com/twelho/dhcp v0.0.0-20220708120125-da9fc6c72fb5/go.mod h1:h+MxyHxRg9NH3terB1nfRIUaQEcI0XOVkdR9LNBlp8E=
 github.com/u-root/u-root v0.8.0 h1:jqP7uPC2+0eRszYTrmdZ6UDyO1Dbuy0rpMo+BnPZ9cY=
 github.com/u-root/u-root v0.8.0/go.mod h1:But1FHzS4Ua4ywx6kZOaRzZTucUKIDKOPOLEKOckQ68=
 github.com/u-root/uio v0.0.0-20210528114334-82958018845c h1:BFvcl34IGnw8yvJi8hlqLFo9EshRInwWBs2M5fGWzQA=

go.work.sum

@@ -9,6 +9,7 @@ github.com/godbus/dbus v0.0.0-20190422162347-ade71ed3457e h1:BWhy2j3IXJhjCbC68Fp
 github.com/grpc-ecosystem/grpc-gateway v1.16.0 h1:gmcG1KaJ57LophUzW0Hy8NmPhnMZb4M0+kPpLofRdBo=
 github.com/stretchr/testify v1.7.2/go.mod h1:R6va5+xMeoiuVRoj+gSkQ7d3FALtqAAGI1FQKckRals=
 github.com/talos-systems/siderolink v0.1.1/go.mod h1:1PLRyKRx+MAkz1vWJXIP19p5wChF0TejbIbX/CQMWuw=
+github.com/twelho/dhcp v0.0.0-20220708120125-da9fc6c72fb5/go.mod h1:h+MxyHxRg9NH3terB1nfRIUaQEcI0XOVkdR9LNBlp8E=
 github.com/vishvananda/netlink v1.2.0-beta h1:CTNzkunO9iTkRaupF540+w47mexyQgNkA/ibnuKc39w=
 github.com/vishvananda/netlink v1.2.0-beta/go.mod h1:twkDnbuQxJYemMlGd4JFIcuhgX83tXhKS2B/PRMpOho=
 go.etcd.io/etcd v0.5.0-alpha.5.0.20200910180754-dd1b699fc489 h1:1JFLBqwIgdyHN1ZtgjTBwO+blA6gVOmZurpiMEsETKo=

internal/app/machined/pkg/controllers/network/operator/dhcp4.go

@@ -13,6 +13,8 @@ import (
 	"sync"
 	"time"
 
+	"github.com/cosi-project/runtime/pkg/resource"
+	"github.com/cosi-project/runtime/pkg/state"
 	"github.com/insomniacslk/dhcp/dhcpv4"
 	"github.com/insomniacslk/dhcp/dhcpv4/nclient4"
 	"go.uber.org/zap"
@@ -27,6 +29,7 @@ import (
 // DHCP4 implements the DHCPv4 network operator.
 type DHCP4 struct {
 	logger *zap.Logger
+	state  state.State
 
 	linkName    string
 	routeMetric uint32
@@ -44,9 +47,10 @@ type DHCP4 struct {
 }
 
 // NewDHCP4 creates DHCPv4 operator.
-func NewDHCP4(logger *zap.Logger, linkName string, routeMetric uint32, platform runtime.Platform) *DHCP4 {
+func NewDHCP4(logger *zap.Logger, linkName string, routeMetric uint32, platform runtime.Platform, state state.State) *DHCP4 {
 	return &DHCP4{
 		logger:      logger,
+		state:       state,
 		linkName:    linkName,
 		routeMetric: routeMetric,
 		// <3 azure
@@ -61,6 +65,39 @@ func (d *DHCP4) Prefix() string {
 	return fmt.Sprintf("dhcp4/%s", d.linkName)
 }
 
+// hostnameStatusMetadata represents the full metadata of any version of a network.HostnameStatus.
+var hostnameStatusMetadata = resource.NewMetadata(network.NamespaceName, network.HostnameStatusType, network.HostnameID, resource.VersionUndefined)
+
+// extractHostname extracts a hostname from the given resource if it is a valid network.HostnameStatus.
+func extractHostname(res resource.Resource) string {
+	if res, ok := res.(*network.HostnameStatus); ok {
+		return res.TypedSpec().Hostname
+	}
+
+	return ""
+}
+
+// setupHostnameWatch returns the initial hostname and a channel that outputs all events related to hostname changes.
+func (d *DHCP4) setupHostnameWatch(ctx context.Context) (string, <-chan state.Event, error) {
+	hostnameWatchCh := make(chan state.Event)
+	if err := d.state.Watch(ctx, hostnameStatusMetadata, hostnameWatchCh); err != nil {
+		return "", nil, err
+	}
+
+	return extractHostname((<-hostnameWatchCh).Resource), hostnameWatchCh, nil
+}
+
+// knownHostname checks if the given hostname has been defined by this operator.
+func (d *DHCP4) knownHostname(hostname string) bool {
+	for i := range d.hostname {
+		if d.hostname[i].Hostname == hostname {
+			return true
+		}
+	}
+
+	return false
+}
+
 // Run the operator loop.
 //
 //nolint:gocyclo,dupl
@@ -69,8 +106,13 @@ func (d *DHCP4) Run(ctx context.Context, notifyCh chan<- struct{}) {
 
 	renewInterval := minRenewDuration
 
+	hostname, hostnameWatchCh, err := d.setupHostnameWatch(ctx)
+	if err != nil && !errors.Is(err, context.Canceled) {
+		d.logger.Warn("failed to watch for hostname changes", zap.Error(err))
+	}
+
 	for {
-		leaseTime, err := d.renew(ctx)
+		leaseTime, err := d.renew(ctx, hostname)
 		if err != nil && !errors.Is(err, context.Canceled) {
 			d.logger.Warn("renew failed", zap.Error(err), zap.String("link", d.linkName))
 		}
@@ -93,10 +135,36 @@ func (d *DHCP4) Run(ctx context.Context, notifyCh chan<- struct{}) {
 			renewInterval = minRenewDuration
 		}
 
-		select {
-		case <-ctx.Done():
-			return
-		case <-time.After(renewInterval):
+		for {
+			select {
+			case <-ctx.Done():
+				return
+			case <-time.After(renewInterval):
+			case event := <-hostnameWatchCh:
+				// if the hostname resource was deleted entirely, we must still
+				// inform the DHCP server that the node has no hostname anymore
+				hostname = extractHostname(event.Resource)
+
+				// If, on first invocation, the DHCP server has given a new hostname for the node, and the
+				// network.HostnameSpecController decides to apply it as a preferred hostname, this
+				// operator would unnecessarily drop the lease and restart DHCP discovery. Thus, if the
+				// selected hostname has been sourced from this operator, we don't need to do anything.
+				if d.knownHostname(hostname) {
+					continue
+				}
+
+				// While updating the hostname together with a RENEW request works for dnsmasq, it doesn't
+				// work with the Windows Server DHCP + DNS. A hostname update via an INIT-REBOOT request
+				// also gets ignored. Thus, the only reliable way to update the hostname seems to be to
+				// forget the old release and initiate a new DISCOVER flow with the new hostname. RFC 2131
+				// doesn't define any better way to do this, and since according to the spec the DISCOVER
+				// cannot be targeted at the previous lessor , the node may switch DHCP servers on hostname
+				// change. This is not that big of a concern though, since a single network should not have
+				// multiple competing DHCP servers in the first place.
+				d.offer = nil
+			}
+
+			break
 		}
 	}
 }
@@ -150,7 +218,7 @@ func (d *DHCP4) TimeServerSpecs() []network.TimeServerSpecSpec {
 }
 
 //nolint:gocyclo
-func (d *DHCP4) parseAck(ack *dhcpv4.DHCPv4) {
+func (d *DHCP4) parseAck(ack *dhcpv4.DHCPv4, hostname string) {
 	d.mu.Lock()
 	defer d.mu.Unlock()
 
@@ -263,7 +331,13 @@ func (d *DHCP4) parseAck(ack *dhcpv4.DHCPv4) {
 		d.resolvers = nil
 	}
 
-	if ack.HostName() != "" {
+	// DHCP hostname parroting protection: if e.g. `dnsmasq` receives a request that both sends a
+	// hostname and requests one, it will "parrot" the sent hostname back if no other name has been
+	// defined for the requesting host. That causes update anomalies here, since removing a
+	// hostname defined previously by e.g. the configuration layer causes a copy of that hostname
+	// to live on in a spec defined by this operator, even though it isn't sourced from DHCP. To
+	// avoid this issue, do an inequality check to determine if the hostname really came from DHCP.
+	if ack.HostName() != "" && (hostname == "" || ack.HostName() != hostname) {
 		spec := network.HostnameSpecSpec{
 			ConfigLayer: network.ConfigOperator,
 		}
@@ -301,7 +375,7 @@ func (d *DHCP4) parseAck(ack *dhcpv4.DHCPv4) {
 	}
 }
 
-func (d *DHCP4) renew(ctx context.Context) (time.Duration, error) {
+func (d *DHCP4) renew(ctx context.Context, hostname string) (time.Duration, error) {
 	opts := []dhcpv4.OptionCode{
 		dhcpv4.OptionClasslessStaticRoute,
 		dhcpv4.OptionDomainNameServer,
@@ -316,19 +390,50 @@ func (d *DHCP4) renew(ctx context.Context) (time.Duration, error) {
 	}
 
 	mods := []dhcpv4.Modifier{dhcpv4.WithRequestedOptions(opts...)}
-	clientOpts := []nclient4.ClientOpt{}
 
+	// if the system has a hostname, send it to the DHCP server with option 12
+	if len(hostname) > 0 {
+		mods = append(mods, dhcpv4.WithOption(dhcpv4.OptHostName(hostname)))
+	}
+
+	var clientOpts []nclient4.ClientOpt
+
+	// existing lease is being renewed
 	if d.offer != nil {
-		// do not use broadcast, but send the packet to DHCP server directly
-		addr, err := net.ResolveUDPAddr("udp", d.offer.ServerIPAddr.String()+":67")
+		serverAddr, err := ToUDPAddr(d.offer.ServerIPAddr, nclient4.ServerPort)
 		if err != nil {
 			return 0, err
 		}
 
-		// by default it's set to 0.0.0.0 which actually breaks lease renew
-		d.offer.ClientIPAddr = d.offer.YourIPAddr
+		clientAddr, err := ToUDPAddr(d.offer.YourIPAddr, nclient4.ClientPort)
+		if err != nil {
+			return 0, err
+		}
 
-		clientOpts = append(clientOpts, nclient4.WithServerAddr(addr))
+		// RFC 2131, section 4.3.2:
+		//     DHCPREQUEST generated during RENEWING state:
+		//     ... This message will be unicast, so no relay
+		//     agents will be involved in its transmission.
+		clientOpts = append(clientOpts,
+			nclient4.WithServerAddr(serverAddr),
+			nclient4.WithUnicast(clientAddr), // this must be specified manually, WithServerAddr is not enough
+		)
+
+		// RFC 2131, section 4.3.2:
+		//     DHCPREQUEST generated during RENEWING state:
+		//     'server identifier' MUST NOT be filled in, 'requested IP address'
+		//     option MUST NOT be filled in, 'ciaddr' MUST be filled in with
+		//     client's IP address.
+		// In the returned offer both the server identifier and requested IP address
+		// are filled in. Modifiers are used, as nclient4.RequestFromOffer uses
+		// the underlying offer data for validation and disregards some fields,
+		// so modifying it directly is not viable. The client IP address is also
+		// set to just zeros, so that needs to be updated here as well.
+		mods = append(mods,
+			dhcpv4.WithOption(dhcpv4.OptServerIdentifier(nil)),
+			dhcpv4.WithOption(dhcpv4.OptRequestedIPAddress(nil)),
+		)
+		d.offer.ClientIPAddr = d.offer.YourIPAddr
 	}
 
 	cli, err := nclient4.New(d.linkName, clientOpts...)
@@ -357,7 +462,7 @@ func (d *DHCP4) renew(ctx context.Context) (time.Duration, error) {
 	d.logger.Debug("DHCP ACK", zap.String("link", d.linkName), zap.String("dhcp", collapseSummary(lease.ACK.Summary())))
 
 	d.offer = lease.Offer
-	d.parseAck(lease.ACK)
+	d.parseAck(lease.ACK, hostname)
 
 	return lease.ACK.IPAddressLeaseTime(time.Minute * 30), nil
 }

internal/app/machined/pkg/controllers/network/operator/utils.go

@@ -0,0 +1,22 @@
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+// Package operator provides common methods for operators.
+package operator
+
+import (
+	"fmt"
+	"net"
+	"net/netip"
+)
+
+// ToUDPAddr combines the given net.IP and port to form a net.UDPAddr.
+func ToUDPAddr(ip net.IP, port uint16) (*net.UDPAddr, error) {
+	addr, ok := netip.AddrFromSlice(ip)
+	if !ok {
+		return nil, fmt.Errorf("failed to parse %q as an IP address", ip)
+	}
+
+	return net.UDPAddrFromAddrPort(netip.AddrPortFrom(addr, port)), nil
+}

internal/app/machined/pkg/controllers/network/operator_spec.go

@@ -421,7 +421,7 @@ func (ctrl *OperatorSpecController) newOperator(logger *zap.Logger, spec *networ
 	case network.OperatorDHCP4:
 		logger = logger.With(zap.String("operator", "dhcp4"))
 
-		return operator.NewDHCP4(logger, spec.LinkName, spec.DHCP4.RouteMetric, ctrl.V1alpha1Platform)
+		return operator.NewDHCP4(logger, spec.LinkName, spec.DHCP4.RouteMetric, ctrl.V1alpha1Platform, ctrl.State)
 	case network.OperatorDHCP6:
 		logger = logger.With(zap.String("operator", "dhcp6"))
 

pkg/machinery/resources/network/hostname_spec.go

@@ -22,7 +22,7 @@ type HostnameSpec = typed.Resource[HostnameSpecSpec, HostnameSpecRD]
 // HostnameID is the ID of the singleton instance.
 const HostnameID resource.ID = "hostname"
 
-// HostnameSpecSpec describes node nostname.
+// HostnameSpecSpec describes node hostname.
 type HostnameSpecSpec struct {
 	Hostname    string      `yaml:"hostname"`
 	Domainname  string      `yaml:"domainname"`