nixos/timesyncd: allow NTP servers advertised by DHCP to be used

[datafoo]

Description of changes

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[datafoo]

In man:timesyncd.conf(5), NTP=:

When the empty string is assigned, the list of NTP servers is reset, and all prior assignments will have no effect.

So, should we write NTP= in the file:

# cat /etc/systemd/timesyncd.conf 
[Time]
NTP=
FallbackNTP=0.nixos.pool.ntp.org 1.nixos.pool.ntp.org 2.nixos.pool.ntp.org 3.nixos.pool.ntp.org

or remove the empty NTP= altogether when servers is the empty list:

# cat /etc/systemd/timesyncd.conf 
[Time]
FallbackNTP=0.nixos.pool.ntp.org 1.nixos.pool.ntp.org 2.nixos.pool.ntp.org 3.nixos.pool.ntp.org

What do you suggest?

[flokli]

Wouldn't setting this to an empty string imply the defaults are always used, and it's not possible to receive config over DHCP?

I feel like we need a VM test that sends config over DHCP and peeks at what's getting set.

[SuperSandro2000]

When the empty string is assigned, the list of NTP servers is reset, and all prior assignments will have no effect.

This probably refers to dropins like for service files. eg. if you want to overwrite ExecStart= you first need to set it to empty otherwise systemd thinks it is a list which doesn't work.

[datafoo]

Setting NTP= to an empty string will allows the NTP servers advertised by DHCP to be used:

# cat /etc/systemd/timesyncd.conf 
[Time]
NTP=

I tested it on a local system and I confirm it works.

[SuperSandro2000]

FallbackNTP=0.nixos.pool.ntp.org 1.nixos.pool.ntp.org 2.nixos.pool.ntp.org 3.nixos.pool.ntp.org

This is already an implied default because of

nixpkgs/pkgs/os-specific/linux/systemd/default.nix

Line 664 in 3526bfa

mesonFlagsArray+=(-Dntp-servers="0.nixos.pool.ntp.org 1.nixos.pool.ntp.org 2.nixos.pool.ntp.org 3.nixos.pool.ntp.org")

https://github.com/systemd/systemd/blob/0197fb599ac4f29871e8ea1923be7b14bbd7bbf0/src/timesync/timesyncd.conf.in#L21

or remove the empty NTP= altogether when servers is the empty list:

I think we should remove it because we do not intent do explicitly remove any previously set value.

[datafoo]

I just do not want to break compatibility for users who are setting networking.timeServers to something different than the compiled-in values (i.e. {0..4}.nixos.pool.ntp.org). Hence the necessity to add the fallbackServers option.

[datafoo]

I updated the PR to let the module user choose, and by default we do not write NTP= (with empty string) so there is no risk of resetting anything.

[datafoo]

I would like the advice from systemd experts.

It seems that my worries about drop-ins are not justified after all.

When the empty string is assigned, the list of NTP servers is reset, and all prior assignments will have no effect.

I believe there will be no differences between writing NTP= or not writing NTP= at all despite the systemd documentation. That is because we are configuring the "main configuration file" which has lower precedence and therefore, there are no prior assignments. See CONFIGURATION DIRECTORIES AND PRECEDENCE in man:timesyncd.conf(5).

Do you agree, should I remove the possibility to set null?

[datafoo]

Despite my wishes, this PR breaks compatibility in few cases.

Example:

services.timesyncd = {
  servers = [];
  extraConfig = ''
    FallbackNTP=0.it.pool.ntp.org 1.it.pool.ntp.org 2.it.pool.ntp.org 3.it.pool.ntp.org
  '';
};

Therefore, I have added some release notes.

[flokli]

This needs another rebase, due to conflicts in the release notes.

[datafoo]

Rebase done.

[flokli]

Thanks!