Re: [chrony-dev] [PATCH] nm-dispatcher: handle NTP servers from DHCP

[Robert Fairley <rfairley@xxxxxxxxxx>]

On Mon, Jun 22, 2020 at 11:19 AM Miroslav Lichvar <mlichvar@xxxxxxxxxx> wrote:
>
> On Thu, Jun 18, 2020 at 11:21:29AM -0400, Robert Fairley wrote:
> > On Thu, Jun 18, 2020 at 6:31 AM Miroslav Lichvar <mlichvar@xxxxxxxxxx> wrote:
> > >
> > > I think it could be a fragment now, and probably everything else
> > > except the default servers. I'll see if I can add an example of
> > > a fragmented configuration, which downstreams could use.
> > >
>
> I've some questions about fragmented configuration:
>
> - Should they be in /usr/lib/chrony.d or /usr/share/chrony.d?
>

My opinion is /usr/lib/chrony.d to align with most existing
projects using overlay/dropin fragments - e.g. systemd sysusers.d
and tmpfiles.d. For /usr/share it seems like a common pattern is
serving it to a group of machines from an NFS server whereas
/usr/lib would normally be local - I guess in those situations
/usr/lib would be more efficient avoiding a network call to read
the config files? https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s11.html

> - Does it make sense to add fragments that only have commented-out
>   directives? Should they be in /etc/chrony.d? The provided files
>   shouldn't be edited by the admin, but how will they know about the
>   important directives?
>

Still thinking on this, but I wonder if something like
`reset <directive>` could be implemented to allow a directive specified
later to ignore previous settings for that directive. E.g., the
usual `/etc/chrony.conf` file could be left as it is now with
the current `pool` setting, but if a distribution wanted to "reset"
this so that no pool was given it could drop
`/usr/lib/chrony.d/90-pool.conf` containing:

```
reset pool
pool 2.fedora.pool.ntp.org iburst
```

which would have the effect of deleting the `pool` directive from
`/etc/chrony.conf` and replacing it with the fedora pool. This would
be a similar effect to passing an empty string to `ExecStart=` in
systemd unit files
(https://www.freedesktop.org/software/systemd/man/systemd.service.html#ExecStart=).
This would let the main options stay together within `/etc/chrony.conf`
but allow vendors (like Fedora CoreOS) to override them if needed by
dropping into `/usr/lib/chrony.d`.

There could still be ways to proceed without doing this, but I'm not
sure of a natural way of grouping the directives into files, without
assuming which ones are likely to be overridden by a distribution
and preserving visibility of the config (e.g. `pool`, `makestep`,
`sourcedirs` would be best in separate files within `/usr/lib/chrony.d`
to allow overriding in the case of Fedora CoreOS, but that won't apply
for other distributions). Another way is having only one directive
in each `.conf` file under `/usr/lib/chrony.d` and require admins to
copy the file into `/etc/chrony.d` before editing it, which I think
would still be reasonable, but it's more difficult for the admin to
see all the main directives that way.

> > +++ b/examples/chrony.nm-dispatcher.dhcp.in
> > @@ -0,0 +1,43 @@
>
> > +chronyc=@CHRONYC@
> > +default_server_options=@CHRONY_DEFAULT_SERVER_OPTIONS@
> > +server_dir=@CHRONY_SERVER_DIR@
>
> After removing the helper and its directory, I'm now wondering if
> those three substitution are worth the trouble. Why not just set them
> to "/usr/bin/chronyc", "iburst", and "/var/run/chrony-dhcp"? That
> should work for most distributions/users and the remaining can fix
> them, like the other provided examples. It could also work with an
> example fragment using "sourcedirs" to read the files.
>

Agreed, it makes sense now to simplify the file now the helper
isn't included - updated now.

From 09619ea5b6924aaedec391b15e2ec7c512f517c0 Mon Sep 17 00:00:00 2001
From: Robert Fairley <rfairley@xxxxxxxxxx>
Date: Thu, 4 Jun 2020 14:48:35 -0400
Subject: [PATCH 1/2] examples: add dispatcher for NTP servers from DHCP

Add new NM dispatcher script for NTP servers given by DHCP through
NetworkManager in a similar way to how distributions have done in
11-dhclient, e.g. [1]. New NTP servers are written as entries to a
file per-interface in /var/run/chrony-dhcp, which is re-read by
chronyd upon executing `chronyc reload sources`.

This provides a way for NTP server configuration to be carried over
from NetworkManager DHCP events to chrony, for DHCP clients other
than dhclient. Part of fixing integration where the NetworkManager
internal client is used, e.g [2].

Paths to the chronyc executable and sources directory are set in
variables, which may be overwritten by downstream packages, but
should work for distributions for the most part.

[1] https://src.fedoraproject.org/rpms/dhcp/blob/master/f/11-dhclient
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1800901
---
 examples/chrony.nm-dispatcher.dhcp | 43 ++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 examples/chrony.nm-dispatcher.dhcp

diff --git a/examples/chrony.nm-dispatcher.dhcp b/examples/chrony.nm-dispatcher.dhcp
new file mode 100644
index 0000000..6ea4c37
--- /dev/null
+++ b/examples/chrony.nm-dispatcher.dhcp
@@ -0,0 +1,43 @@
+#!/bin/sh
+# This is a NetworkManager dispatcher script for chronyd to update
+# its NTP sources passed from DHCP options. Note that this script is
+# specific to NetworkManager-dispatcher due to use of the
+# DHCP4_NTP_SERVERS environment variable.
+
+export LC_ALL=C
+
+interface=$1
+action=$2
+
+chronyc=/usr/bin/chronyc
+default_server_options=iburst
+server_dir=/var/run/chrony-dhcp
+
+dhcp_server_file=$server_dir/$interface.sources
+# DHCP4_NTP_SERVERS is passed from DHCP options by NetworkManager.
+nm_dhcp_servers=$DHCP4_NTP_SERVERS
+
+add_servers_from_dhcp() {
+    rm -f "$dhcp_server_file"
+    for server in $nm_dhcp_servers; do
+        echo "server $server $default_server_options" >> "$dhcp_server_file"
+    done
+    $chronyc reload sources > /dev/null 2>&1 || :
+}
+
+clear_servers_from_dhcp() {
+    if [ -f "$dhcp_server_file" ]; then
+        rm -f "$dhcp_server_file"
+        $chronyc reload sources > /dev/null 2>&1 || :
+    fi
+}
+
+mkdir -p $server_dir
+
+if [ "$action" = "up" ] || [ "$action" = "dhcp4-change" ]; then
+    add_servers_from_dhcp
+elif [ "$action" = "down" ]; then
+    clear_servers_from_dhcp
+fi
+
+exit 0
-- 
2.26.2

From 76fe58f702953e87a3be16d81527c78883b5c5ef Mon Sep 17 00:00:00 2001
From: Robert Fairley <rfairley@xxxxxxxxxx>
Date: Thu, 4 Jun 2020 14:48:40 -0400
Subject: [PATCH 2/2] examples: align onoffline with DHCP NM dispatcher

Similar to the DHCP dispatcher, add a variable for the chronyc
executable path, which can be overwritten more easily by
downstream packages if needed.

Also give an `.onoffline` suffix to more clearly differentiate
this script from `chrony.nm-dispatcher.dhcp`.
---
 .../{chrony.nm-dispatcher => chrony.nm-dispatcher.onoffline}  | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
 rename examples/{chrony.nm-dispatcher => chrony.nm-dispatcher.onoffline} (86%)

diff --git a/examples/chrony.nm-dispatcher b/examples/chrony.nm-dispatcher.onoffline
similarity index 86%
rename from examples/chrony.nm-dispatcher
rename to examples/chrony.nm-dispatcher.onoffline
index 0b0c3e7..34cfa0d 100644
--- a/examples/chrony.nm-dispatcher
+++ b/examples/chrony.nm-dispatcher.onoffline
@@ -5,11 +5,13 @@
 
 export LC_ALL=C
 
+chronyc=/usr/bin/chronyc
+
 # For NetworkManager consider only up/down events
 [ $# -ge 2 ] && [ "$2" != "up" ] && [ "$2" != "down" ] && exit 0
 
 # Note: for networkd-dispatcher routable.d ~= on and off.d ~= off
 
-chronyc onoffline > /dev/null 2>&1
+$chronyc onoffline > /dev/null 2>&1
 
 exit 0
-- 
2.26.2

diff --git a/examples/chrony.nm-dispatcher.dhcp.in b/examples/chrony.nm-dispatcher.dhcp
similarity index 90%
rename from examples/chrony.nm-dispatcher.dhcp.in
rename to examples/chrony.nm-dispatcher.dhcp
index bbd9430..6ea4c37 100644
--- a/examples/chrony.nm-dispatcher.dhcp.in
+++ b/examples/chrony.nm-dispatcher.dhcp
@@ -9,9 +9,9 @@ export LC_ALL=C
 interface=$1
 action=$2
 
-chronyc=@CHRONYC@
-default_server_options=@CHRONY_DEFAULT_SERVER_OPTIONS@
-server_dir=@CHRONY_SERVER_DIR@
+chronyc=/usr/bin/chronyc
+default_server_options=iburst
+server_dir=/var/run/chrony-dhcp
 
 dhcp_server_file=$server_dir/$interface.sources
 # DHCP4_NTP_SERVERS is passed from DHCP options by NetworkManager.
diff --git a/examples/chrony.nm-dispatcher.onoffline.in b/examples/chrony.nm-dispatcher.onoffline
similarity index 94%
rename from examples/chrony.nm-dispatcher.onoffline.in
rename to examples/chrony.nm-dispatcher.onoffline
index 7c0e2dd..34cfa0d 100644
--- a/examples/chrony.nm-dispatcher.onoffline.in
+++ b/examples/chrony.nm-dispatcher.onoffline
@@ -5,7 +5,7 @@
 
 export LC_ALL=C
 
-chronyc=@CHRONYC@
+chronyc=/usr/bin/chronyc
 
 # For NetworkManager consider only up/down events
 [ $# -ge 2 ] && [ "$2" != "up" ] && [ "$2" != "down" ] && exit 0