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

[Robert Fairley <rfairley@xxxxxxxxxx>]

On Wed, Jun 10, 2020 at 2:59 AM Miroslav Lichvar <mlichvar@xxxxxxxxxx> wrote:
>
> On Tue, Jun 09, 2020 at 02:09:54PM -0400, Robert Fairley wrote:
> > That sounds a good idea! We only need to reload the NTP sources -
> > having a directive to specify where the NTP sources are managed makes
> > sense. Would the directive point to a file/directory/glob to load
> > files containing the NTP server config from?
>
> I was thinking about reusing the confdirs code with its syntax, e.g.
>
> sourcedirs /etc/chrony.d /var/run/chrony-dhcp
>
> I'm not sure how much sense it will make to override the source files
> (maybe disable servers from a specific interface?), but it's free and
> it's easier to document.
>

Using sourcedirs, with the current behavior of reading *.sources files
from all directories specified SGTM (thank you for implementing that
already!)

For overriding, we really just need a switch to disable propagating
sources from DHCP through host configuration. At first I was thinking
have direct overrides for `*.source` files, but the per-interface
filenames would be difficult to override from /etc directories (and
probably wouldn't make sense to allow this semantic). With the current
implementation using `confdirs`, the `sourcedirs /var/run/chrony-dhcp`
line could exist as its own config fragment under /usr/lib/chrony.d,
which could be overridden/disabled by writing into /etc/chrony.d. This
would fit the use case for specifying the per-platform configs in
FCOS and ignoring DHCP -
https://github.com/coreos/fedora-coreos-config/blob/testing-devel/overlay.d/20platform-chrony/usr/lib/systemd/system-generators/coreos-platform-chrony.

In https://src.fedoraproject.org/rpms/chrony/c/93dbaa93159f5d4ffc601f88d0b57b6c7c839dda
I added an example of how the fragment could look.

> As for your patchset, it would be good to rebase it to not include the
> helper. It won't be very useful and we can probably drop it downstream
> too. AFAIK nobody is really using the DNS SRV support. It was meant to
> be used by the ipa installer, but it doesn't look like they ever used
> it. For the static sources we might want to discourage editing the
> main config.
>

Done. I left it in the package in
https://src.fedoraproject.org/rpms/chrony/pull-request/3
for now - later on the helper and code using it can be removed.

> There are other tools that rely on NTP sources being specified in the
> main config (e.g. the installer). They will need to be updated before
> we can move the default servers to a fragment.
>

For now I've left https://src.fedoraproject.org/rpms/chrony/pull-request/3
with a fragment for `sourcedirs`, as the PR is pending the `sourcedirs`
being packaged. In the meantime I can take a look to see what changes
are needed in the installer and others. Otherwise, for now having
`sourcedirs` appended the main `/etc/chrony.conf` would be fine (with
respect to allowing overriding the propagation from DHCP, we'd
later on aim to make this its own fragment, if acceptable).

So far I have tested these patches with the downstream macro
substitutions and sysconfig patch applied, and using `sourcedirs`
to specify `/var/run/chrony-dhcp`, in Fedora Rawhide. On modifying the
`*.sources` files and `chronyc reload sources` (or
deactivating/activating the NM interface to re-run the dispatcher,
the sources from a DHCP server appeared in `chronyc sources`.

From 2c5875aede60b040b5613e71eb0d7884041a48fd 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 CHRONY_SERVER_DIR, which is read by
chrony.helper when updating the chrony service.

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].

Similarly to the chrony.helper script, make paths and options
configurable through macros that can be substituted at install time.

[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.in | 44 +++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 examples/chrony.nm-dispatcher.dhcp.in

diff --git a/examples/chrony.nm-dispatcher.dhcp.in b/examples/chrony.nm-dispatcher.dhcp.in
new file mode 100644
index 0000000..d1f34eb
--- /dev/null
+++ b/examples/chrony.nm-dispatcher.dhcp.in
@@ -0,0 +1,44 @@
+#!/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. For networkd-dispatcher,
+# an alternative approach is external means such as a dhclient hook.
+
+export LC_ALL=C
+
+interface=$1
+action=$2
+
+chronyc=@CHRONYC@
+default_server_options=@CHRONY_DEFAULT_SERVER_OPTIONS@
+server_dir=@CHRONY_SERVER_DIR@
+
+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 a27b3df923d9a2fbef3aaf824325b4e35dc7e460 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/nm-dispatcher: make chronyc configurable

Use the @CHRONYC@ macro, used in the chrony.helper example, to make
the chronyc command path configurable at install time.

Add the suffix `.in` to the filename to indicate this must be
substituted.

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

diff --git a/examples/chrony.nm-dispatcher b/examples/chrony.nm-dispatcher.onoffline.in
similarity index 87%
rename from examples/chrony.nm-dispatcher
rename to examples/chrony.nm-dispatcher.onoffline.in
index 0b0c3e7..7c0e2dd 100644
--- a/examples/chrony.nm-dispatcher
+++ b/examples/chrony.nm-dispatcher.onoffline.in
@@ -5,11 +5,13 @@
 
 export LC_ALL=C
 
+chronyc=@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.helper.in b/examples/chrony.helper.in
deleted file mode 100644
index 6dbab81..0000000
--- a/examples/chrony.helper.in
+++ /dev/null
@@ -1,264 +0,0 @@
-#!/bin/bash
-# This script configures running chronyd to use NTP servers obtained from
-# DHCP and _ntp._udp DNS SRV records. Files with servers from DHCP are managed
-# externally (e.g. by a dhclient script). Files with servers from DNS SRV
-# records are updated here using the dig utility. The script can also list
-# and set static sources in the chronyd configuration file.
-
-chronyc=@CHRONYC@
-chrony_conf=@CHRONY_CONF@
-chrony_service=@CHRONY_SERVICE@
-default_server_options=@CHRONY_DEFAULT_SERVER_OPTIONS@
-helper_dir=@CHRONY_HELPER_DIR@
-server_dir=@CHRONY_SERVER_DIR@
-
-dhcp_servers_files="${server_dir}/chrony.servers.*"
-added_servers_file=$helper_dir/added_servers
-dnssrv_servers_files="$helper_dir/dnssrv@*"
-dnssrv_timer_prefix=chrony-dnssrv@
-
-chrony_command() {
-    $chronyc -a -n -m "$1"
-}
-
-is_running() {
-    chrony_command "tracking" &> /dev/null
-}
-
-get_servers_files() {
-    echo "$dhcp_servers_files"
-    echo "$dnssrv_servers_files"
-}
-
-is_update_needed() {
-    for file in $(get_servers_files) $added_servers_file; do
-        [ -e "$file" ] && return 0
-    done
-    return 1
-}
-
-update_daemon() {
-    local all_servers_with_args all_servers added_servers
-
-    if ! is_running; then
-        rm -f $added_servers_file
-        return 0
-    fi
-
-    all_servers_with_args=$(cat $(get_servers_files) 2> /dev/null)
-
-    all_servers=$(
-        echo "$all_servers_with_args" |
-            while read -r server serverargs; do
-                echo "$server"
-            done | sort -u)
-    added_servers=$( (
-        cat $added_servers_file 2> /dev/null
-        echo "$all_servers_with_args" |
-            while read -r server serverargs; do
-                [ -z "$server" ] && continue
-                chrony_command "add server $server $serverargs" &> /dev/null &&
-                    echo "$server"
-            done) | sort -u)
-
-    comm -23 <(echo -n "$added_servers") <(echo -n "$all_servers") |
-        while read -r server; do
-            chrony_command "delete $server" &> /dev/null
-        done
-
-    added_servers=$(comm -12 <(echo -n "$added_servers") <(echo -n "$all_servers"))
-
-    if [ -n "$added_servers" ]; then
-        echo "$added_servers" > $added_servers_file
-    else
-        rm -f $added_servers_file
-    fi
-}
-
-get_dnssrv_servers() {
-    local name=$1 output
-
-    if ! command -v dig &> /dev/null; then
-        echo "Missing dig (DNS lookup utility)" >&2
-        return 1
-    fi
-
-    output=$(dig "$name" srv +short +ndots=2 +search 2> /dev/null) || return 0
-
-    echo "$output" | while read -r _ _ port target; do
-        server=${target%.}
-        [ -z "$server" ] && continue
-        echo "$server port $port $default_server_options"
-    done
-}
-
-check_dnssrv_name() {
-    local name=$1
-
-    if [ -z "$name" ]; then
-        echo "No DNS SRV name specified" >&2
-        return 1
-    fi
-
-    if [ "${name:0:9}" != _ntp._udp ]; then
-        echo "DNS SRV name $name doesn't start with _ntp._udp" >&2
-        return 1
-    fi
-}
-
-update_dnssrv_servers() {
-    local name=$1
-    local srv_file=$helper_dir/dnssrv@$name servers
-
-    check_dnssrv_name "$name" || return 1
-
-    servers=$(get_dnssrv_servers "$name")
-    if [ -n "$servers" ]; then
-        echo "$servers" > "$srv_file"
-    else
-        rm -f "$srv_file"
-    fi
-}
-
-set_dnssrv_timer() {
-    local state=$1 name=$2
-    local srv_file=$helper_dir/dnssrv@$name servers
-    local timer
-
-    timer=$dnssrv_timer_prefix$(systemd-escape "$name").timer || return 1
-
-    check_dnssrv_name "$name" || return 1
-
-    if [ "$state" = enable ]; then
-        systemctl enable "$timer"
-        systemctl start "$timer"
-    elif [ "$state" = disable ]; then
-        systemctl stop "$timer"
-        systemctl disable "$timer"
-        rm -f "$srv_file"
-    fi
-}
-
-list_dnssrv_timers() {
-    systemctl --all --full -t timer list-units | grep "^$dnssrv_timer_prefix" | \
-            sed "s|^$dnssrv_timer_prefix\(.*\)\.timer.*|\1|" |
-        while read -r name; do
-            systemd-escape --unescape "$name"
-        done
-}
-
-prepare_helper_dir() {
-    mkdir -p $helper_dir
-    exec 100> $helper_dir/lock
-    if ! flock -w 20 100; then
-        echo "Failed to lock $helper_dir" >&2
-        return 1
-    fi
-}
-
-is_source_line() {
-    local pattern="^[ \t]*(server|pool|peer|refclock)[ \t]+[^ \t]+"
-    [[ "$1" =~ $pattern ]]
-}
-
-list_static_sources() {
-    while read -r line; do
-        if is_source_line "$line"; then
-            echo "$line"
-        fi
-    done < $chrony_conf
-}
-
-set_static_sources() {
-    local new_config tmp_conf
-
-    new_config=$(
-        sources=$(
-            while read -r line; do
-                is_source_line "$line" && echo "$line"
-            done)
-
-        while read -r line; do
-            if ! is_source_line "$line"; then
-                echo "$line"
-                continue
-            fi
-
-            tmp_sources=$(
-                local removed=0
-
-                echo "$sources" | while read -r line2; do
-                    if [ "$removed" -ne 0 ] || [ "$line" != "$line2" ]; then
-                        echo "$line2"
-                    else
-                        removed=1
-                    fi
-                done)
-
-            [ "$sources" == "$tmp_sources" ] && continue
-            sources=$tmp_sources
-            echo "$line"
-        done < $chrony_conf
-
-        echo "$sources"
-    )
-
-    tmp_conf=${chrony_conf}.tmp
-
-    cp -a $chrony_conf $tmp_conf &&
-        echo "$new_config" > $tmp_conf &&
-        mv $tmp_conf $chrony_conf || return 1
-
-    systemctl try-restart $chrony_service
-}
-
-print_help() {
-    echo "Usage: $0 COMMAND"
-    echo
-    echo "Commands:"
-    echo "	update-daemon"
-    echo "	update-dnssrv-servers NAME"
-    echo "	enable-dnssrv NAME"
-    echo "	disable-dnssrv NAME"
-    echo "	list-dnssrv"
-    echo "	list-static-sources"
-    echo "	set-static-sources < sources.list"
-    echo "	is-running"
-    echo "	command CHRONYC-COMMAND"
-}
-
-case "$1" in
-    update-daemon)
-        is_update_needed || exit 0
-        prepare_helper_dir && update_daemon
-        ;;
-    update-dnssrv-servers)
-        prepare_helper_dir && update_dnssrv_servers "$2" && update_daemon
-        ;;
-    enable-dnssrv)
-        set_dnssrv_timer enable "$2"
-        ;;
-    disable-dnssrv)
-        set_dnssrv_timer disable "$2" && prepare_helper_dir && update_daemon
-        ;;
-    list-dnssrv)
-        list_dnssrv_timers
-        ;;
-    list-static-sources)
-        list_static_sources
-        ;;
-    set-static-sources)
-        set_static_sources
-        ;;
-    is-running)
-        is_running
-        ;;
-    command|forced-command)
-        chrony_command "$2"
-        ;;
-    *)
-        print_help
-        exit 2
-esac
-
-exit $?
diff --git a/examples/chrony.nm-dispatcher.dhcp.in b/examples/chrony.nm-dispatcher.dhcp.in
index d5d3f93..d1f34eb 100644
--- a/examples/chrony.nm-dispatcher.dhcp.in
+++ b/examples/chrony.nm-dispatcher.dhcp.in
@@ -10,26 +10,26 @@ export LC_ALL=C
 interface=$1
 action=$2
 
-chrony_helper=@CHRONY_HELPER@
+chronyc=@CHRONYC@
 default_server_options=@CHRONY_DEFAULT_SERVER_OPTIONS@
 server_dir=@CHRONY_SERVER_DIR@
 
-dhcp_server_file=$server_dir/chrony.servers.$interface
+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 $default_server_options" >> "$dhcp_server_file"
+        echo "server $server $default_server_options" >> "$dhcp_server_file"
     done
-    $chrony_helper update-daemon || :
+    $chronyc reload sources > /dev/null 2>&1 || :
 }
 
 clear_servers_from_dhcp() {
     if [ -f "$dhcp_server_file" ]; then
         rm -f "$dhcp_server_file"
-        $chrony_helper update-daemon || :
+        $chronyc reload sources > /dev/null 2>&1 || :
     fi
 }