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

twelho · 2022-07-12T12:16:37Z

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.

Fixes #4536.

Acceptance

Please use the following checklist:

you linked an issue (if applicable)
you included tests (if applicable)
you ran conformance (make conformance)
you formatted your code (make fmt)
you linted your code (make lint)
you generated documentation (make docs)
you ran unit-tests (make unit-tests)

smira · 2022-07-12T12:23:00Z

/ok-to-test

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

smira

thanks for your work, and this looks really great!

smira · 2022-07-12T17:57:19Z

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


I would really love to avoid the need to use the fork unless of course we can't get any feedback from the maintainer.

let's see if we can try to ping them on that PR

Alright, I've pinged the people I believe to be the maintainers in insomniacslk/dhcp#469 now. Let's see if they answer.

smira · 2022-07-12T17:58:09Z

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

+func (d *DHCP4) setupHostnameWatch(ctx context.Context) (<-chan state.Event, error) {
+	hostnameWatchCh := make(chan state.Event)
+
+	return hostnameWatchCh, d.state.WatchKind(ctx, hostnameStatusMetadata, hostnameWatchCh)


Suggested change

return hostnameWatchCh, d.state.WatchKind(ctx, hostnameStatusMetadata, hostnameWatchCh)

return hostnameWatchCh, d.state.Watch(ctx, hostnameStatusMetadata, hostnameWatchCh)

This should be Watch to watch a specific resource instance here

Updated. Is there only ever a single instance of HostnameStatus, or why is Watch with resource.VersionUndefined preferred to using WatchKind here? Just curious.

Metadata consists of namespace, type, id and version.
WatchKind allows you watching items passing only namespace and type what makes it return the list of items of this kind.
There is always a single HostnameStatus resource in Talos and the id for it is known, so you can use Watch which always watches a single resource.

smira · 2022-07-12T18:00:10Z

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

+				return
+			case <-time.After(renewInterval):
+			case <-hostnameWatchCh:
+				err := d.updateHostname(ctx, &hostname)


in fact the event itself carries the new hostname, so we don't need to get it once again.

we should check the even type, but basically something like:

if event.Type == state.Created || event.Type == state.Updated { hostname = event.Resource.(*network.HostnameStatus).TypedSpec().Hostname }

I've now removed updateHostname altogether and use the hostname directly from the event. Thanks for the pointer. The check needs to cover all event types, since hostname deletion should also indicate to the DHCP server that the node has lost its previous hostname. I'm doing a checked cast to network.HostnameStatus, which fails intentionally if it gets fed a Tombstone generated by a deletion event. I've also added a comment about this.

smira · 2022-07-12T18:04:39Z

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

+}
+
+// knownHostname checks if the given hostname has been defined by this operator.
+func (d *DHCP4) knownHostname(hostname *string) bool {


I wonder if I should do a little bit of refactoring and also put the hostname source into the HostnameStatus resource, this way we can avoid this comparison in favor of checking if hostname source is the operator

This is a good idea. knownHostname really is a hack, and it would be useful to know which of the hostname candidates was elected as the node hostname for transparency and debugging as well. It would be best if the HostnameStatus source be used to identify an individual HostnameSpec, as just knowing the configuration layer of the source is not enough (e.g. ambiguity ensues when both the dhcp4 and dhcp6 operators have HostnameSpecs defined in the operator layer).

The current functionality with knownHostname works as-is if you prefer to merge this PR first, but I can also rebase on top of the changes to HostnameStatus if that's better for you.

thanks, I think I can get in some changes early next week for that, as we're still waiting for upstream dhcp library fixes?

Yes, go for it. I can then rebase on top of that work. There's one more issue with the hostname "parroting" prevention as well (not directly related to knownHostname), the approach needs to be changed a bit to work even when the only hostname (apart from the default hostname) is defined by DHCP. I'll look at fixing that early next week.

The new approach is now in place and confirmed working even in the new edge cases I found. When the locally defined hostname and the hostname served by DHCP are the same, both now register correctly.

twelho · 2022-07-13T11:29:38Z

The CI build job seems to have failed due to a temporary issue in cloning the repo, it needs a restart.

twelho · 2022-07-13T11:30:45Z

The original (pre-review) version of the sources can be found here: https://github.com/twelho/talos/tree/dhcpv4-send-hostname-v1

twelho · 2022-07-13T12:12:12Z

The functionality of e85233f has now been tested against dnsmasq, including edge cases. No issues found.

twelho · 2022-07-19T14:44:24Z

I've split the hostname sending and requesting operations into two separate invocations to completely sidestep the DHCP server "parroting" issue, where the DHCP server just automatically replies with the hostname sent by the client without verifying that it is actually registered on the server.

The current hostname is always sent when acquiring a new lease (which happens also when the local hostname changes, e.g. due to a machine configuration edit), and the DHCP-served hostname is requested right after with an INFORM request. This avoids needing to wait for the first RENEW, which may also never happen depending on the server configuration. RENEWs will still always request the hostname to detect and apply changes done on the DHCP server side. The INFORM support additionally requires some upstream improvements, which I've submitted at insomniacslk/dhcp#470.

twelho · 2022-07-27T13:39:29Z

I've identified a race condition when testing the INFORM request build, which has now been fixed in the latest commit. Any hostname request (or lease renewal for that matter) is performed using unicast, which requires the address sourced from DHCP to be available on the host (i.e. bound to an interface). This does not happen immediately after notifying the controller about the new address, and thus any immediate INFORM hostname requests will fail to bind to an address. After trying out a couple different approaches, I settled on a context-cancellable wait for both the address assignment and network upon lease negotiation to safely continue with unicast operations. This resolves the random failures I've been seeing when testing this at a large-ish scale.

Another issue I've spotted and subsequently fixed is the machine hostnames disappearing from DNS after a renewal. Turns out that at least Windows Server notices if the hostname is only sent during the initial negotiation and removes it from DNS. The most robust solution I came up with is always sending the hostname during request/renew operations, and extending the use of the INFORM hostname requests to lease renewals as well. So far so good.

smira · 2022-08-08T13:31:28Z

It looks like things fail with our "test" DHCP server, but we can fix that of course. This test DHCP server was never supposed to be complete implementation.

smira · 2022-08-08T13:32:06Z

It looks like upstreaming changes didn't quite work yet? Anything we can do to help?

twelho · 2022-08-08T15:11:09Z

It looks like things fail with our "test" DHCP server, but we can fix that of course. This test DHCP server was never supposed to be complete implementation.

It should keep working even if the server doesn't support or can't process DHCPINFORM requests, hostname request errors are non-fatal on purpose. There are some intermittent CI failures, but I haven't been able to pin-point then to this DHCP rewrite. Of course if you have found some specific failure cases I would be happy to try to fix them.

It looks like upstreaming changes didn't quite work yet? Anything we can do to help?

I will ping the maintainers once more with a message in both insomniacslk/dhcp#469 and insomniacslk/dhcp#470. Of course if you can voice the importance of these changes there might be higher chance of people finally reviewing and merging them, as I think I have now addressed all of the feedback (at least in the former PR) and I don't plan on doing any more changes in either.

smira · 2022-08-15T13:01:18Z

As we're going to branch off v1.2.0 stable branch today, I moved it to the next iteration - v1.3.0

dimatha · 2023-02-26T18:39:29Z

Great job ! Any plans to finish this one? Thanks

smira · 2023-03-23T12:56:21Z

@twelho thanks for your work, we plan to cut Talos beta release next week, and I want this PR to be merged before that.

If the upstream changes doesn't get forward in this time, I'm looking towards forking the DHCP library with your change (or using your fork) until your PR gets merged.

twelho · 2023-03-23T13:12:13Z

Alright, I feel like the DHCP library work is very close. I've think I've satisfied all of the maintainer's constraints from my side, so it's essentially down to them to just decide how INFORM requests should be constructed. I believe it would help if you could also notify them about the urgency one more time in this thread: insomniacslk/dhcp#470 (comment).

If they can still make up their mind this week and get the work merged, I can rebase/update this PR right away. However, if it looks like it won't happen before next week, I can also finish up this work using my fork for the time being, but avoiding keeping the fork would be my preference as well.

twelho · 2023-03-27T20:15:08Z

With insomniacslk/dhcp#470 (and insomniacslk/dhcp#498 as a minor fix) finally merged (yay 🎉), this work does not need to rely on my own fork anymore, as all of my changes have now now landed upstream.

With that, I've gone through this PR one more time to remove the replace directive and clean up both the comments and logic. One more mistake was spotted and fixed, namely avoiding sending the server identifier together with INFORM requests as that is against the spec.

Note, however, that I've not yet tested this in its final form. As it's been 8 months since starting this work, I don't have access to the original test setup anymore (which also covered Windows environments), so I will need to create a new test setup for this, but I will only be able to test against dnsmasq. I would appreciate if others would also be able to confirm that this works as intended using their infrastructure.

smira · 2023-03-29T13:38:27Z

I did some (small) changes:

reworked our test dhcpd to be compliant with the new renewal protocol
did some changes around the hostname change detection logic, there were some changes to Talos since the time you started your work @twelho (default hostnames), in my testing it seems to work for all cases reasonably well
some cosmetic changes

The question of detecting "is it DHCP hostname", "should I send the hostname" is still not 100% reliable, but seems to be better now.

frezbo · 2023-03-29T13:46:57Z

peer review in bullpen?

smira · 2023-03-29T14:01:37Z

/promote integration-misc

smira · 2023-03-29T14:08:11Z

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

+)
+
+// ToUDPAddr combines the given net.IP and port to form a net.UDPAddr.
+func ToUDPAddr(ip net.IP, port uint16) (*net.UDPAddr, error) {


move to internal/ under operator/ ?

move resources/network?

ended up removing this file and doing a straightforward &net.UDPAddr{} initialization

smira · 2023-03-29T14:45:32Z

/promote e2e

smira · 2023-03-29T15:28:14Z

/promote e2e-aws

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 issues and missing features in the upstream `insomniacslk/dhcp` library, the fixes and improvements for which are now finally merged. 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]> Signed-off-by: Andrey Smirnov <[email protected]>

smira · 2023-03-29T17:30:57Z

/promote integration-qemu

smira · 2023-03-29T17:35:29Z

/promote e2e-aws

smira · 2023-03-29T18:16:49Z

/promote e2e-nothing

smira · 2023-03-29T18:17:17Z

/m

twelho force-pushed the dhcpv4-send-hostname branch from 23bd03e to 0b4440d Compare July 12, 2022 12:22

smira added this to the v1.2 milestone Jul 12, 2022

talos-bot added the status/ok-to-test label Jul 12, 2022

Unix4ever reviewed Jul 12, 2022

View reviewed changes

internal/app/machined/pkg/controllers/network/operator/dhcp4.go Outdated Show resolved Hide resolved

internal/app/machined/pkg/controllers/network/operator/dhcp4.go Outdated Show resolved Hide resolved

smira reviewed Jul 12, 2022

View reviewed changes

twelho force-pushed the dhcpv4-send-hostname branch from 0b4440d to 68f214f Compare July 13, 2022 10:55

twelho mentioned this pull request Jul 13, 2022

dhcpv4: support RENEW requests and option deletion insomniacslk/dhcp#469

Merged

twelho force-pushed the dhcpv4-send-hostname branch from 68f214f to e85233f Compare July 13, 2022 11:18

twelho force-pushed the dhcpv4-send-hostname branch from e85233f to 56a757c Compare July 19, 2022 14:33

twelho force-pushed the dhcpv4-send-hostname branch 4 times, most recently from e5624bc to 40101cf Compare July 27, 2022 13:27

twelho force-pushed the dhcpv4-send-hostname branch from 40101cf to dfa0a54 Compare August 8, 2022 13:12

twelho force-pushed the dhcpv4-send-hostname branch from dfa0a54 to fa731c8 Compare August 8, 2022 14:58

twelho mentioned this pull request Aug 8, 2022

dhcpv4: implement interface for sending INFORM requests insomniacslk/dhcp#470

Merged

smira modified the milestones: v1.2, v1.3 Aug 15, 2022

twelho force-pushed the dhcpv4-send-hostname branch 2 times, most recently from 649bcfe to ebff506 Compare September 10, 2022 21:39

twelho force-pushed the dhcpv4-send-hostname branch from 91b640c to cb93448 Compare February 28, 2023 15:48

twelho force-pushed the dhcpv4-send-hostname branch 2 times, most recently from 0c6ffeb to a9e257b Compare March 27, 2023 19:59

twelho force-pushed the dhcpv4-send-hostname branch 2 times, most recently from 4b1e63a to 2a49d53 Compare March 27, 2023 21:04

smira force-pushed the dhcpv4-send-hostname branch from 2a49d53 to 3ddaf4d Compare March 29, 2023 13:36

smira approved these changes Mar 29, 2023

View reviewed changes

smira reviewed Mar 29, 2023

View reviewed changes

smira force-pushed the dhcpv4-send-hostname branch from 3ddaf4d to 45c5b47 Compare March 29, 2023 17:07

talos-bot added the status/ok-to-merge label Mar 29, 2023

talos-bot merged commit 45c5b47 into siderolabs:main Mar 29, 2023

talos-bot removed status/ok-to-test status/ok-to-merge labels Mar 29, 2023

twelho deleted the dhcpv4-send-hostname branch March 30, 2023 12:51

uhthomas mentioned this pull request Mar 30, 2023

DHCP renewal failed #6905

Closed

twelho mentioned this pull request Apr 3, 2023

v1.4.0-alpha.4 is not respecting hostname from DHCP #7041

Closed

	return hostnameWatchCh, d.state.WatchKind(ctx, hostnameStatusMetadata, hostnameWatchCh)
	return hostnameWatchCh, d.state.Watch(ctx, hostnameStatusMetadata, hostnameWatchCh)

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

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

Conversation

twelho commented Jul 12, 2022

Acceptance

smira commented Jul 12, 2022

smira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twelho commented Jul 13, 2022

twelho commented Jul 13, 2022

twelho commented Jul 13, 2022

twelho commented Jul 19, 2022

twelho commented Jul 27, 2022

smira commented Aug 8, 2022

smira commented Aug 8, 2022

twelho commented Aug 8, 2022

smira commented Aug 15, 2022

dimatha commented Feb 26, 2023

smira commented Mar 23, 2023

twelho commented Mar 23, 2023

twelho commented Mar 27, 2023

smira commented Mar 29, 2023

frezbo commented Mar 29, 2023

smira commented Mar 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smira commented Mar 29, 2023

smira commented Mar 29, 2023

smira commented Mar 29, 2023

smira commented Mar 29, 2023

smira commented Mar 29, 2023

smira commented Mar 29, 2023