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

[Robert Fairley <rfairley@xxxxxxxxxx>]

On Mon, Jun 8, 2020 at 10:54 AM Miroslav Lichvar <mlichvar@xxxxxxxxxx> wrote:
> Would it make sense to add a suffix to the non-DHCP dispatcher script
> (e.g .online -> 20-chrony-online) to avoid confusion with the two
> scripts?

Done - I gave it `onoffline` as a suffix, just to help suggest it
applies for offline situations too (for dropping in on.d and off.d
for networkd-dispatcher). "online" would be fine too though.

> Also, could you please rename the default_options variables in the
> helper and dispatcher scripts to something like default_source_options
> or default_server_options to make it more clear where the options are
> used?

Done.

Updated the patches and attached an interdiff below for the changes.

We'll want to change the .dhcp dispatcher or the helper soon to use
the new directive, once implemented. Good with holding this
pending resolution of the new directive, and updating to use the
directive, but also good with merging as-is and iterating.

From 9cd346a6478193e9606de5ecebe33633744ae37a Mon Sep 17 00:00:00 2001
From: Robert Fairley <rfairley@xxxxxxxxxx>
Date: Thu, 4 Jun 2020 14:48:24 -0400
Subject: [PATCH 1/3] examples: upstream chrony.helper script

Add an example chrony.helper file, which was initially copied from
the downstream Fedora downstream package, with distro-specific parts
taken out. Overall the following changes are made from the downstream
version:

* Remove use of `/etc/sysconfig`, which is specific to
  Fedora/RHEL.
* Make the paths in variables such as chrony_conf and
  helper_dir configurable through macros @CHRONY_CONF@ and
  @CHRONY_HELPER_DIR@. Values used here may vary across
  distributions, and should be set by downstream packages.
* Introduce the server_dir variable, where `chrony.servers.*` files
  are placed, so that a common macro @CHRONY_SERVER_DIR@ between the
  helper script and the NetworkManager dispatch script can be used.
* Remove dhclient identifiers and commands.
---
 examples/chrony.helper.in | 264 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 264 insertions(+)
 create mode 100644 examples/chrony.helper.in

diff --git a/examples/chrony.helper.in b/examples/chrony.helper.in
new file mode 100644
index 0000000..6dbab81
--- /dev/null
+++ b/examples/chrony.helper.in
@@ -0,0 +1,264 @@
+#!/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 $?
-- 
2.26.2

From 481c62a9bb71686add8a5e972226f4ef0281b536 Mon Sep 17 00:00:00 2001
From: Robert Fairley <rfairley@xxxxxxxxxx>
Date: Thu, 4 Jun 2020 14:48:35 -0400
Subject: [PATCH 2/3] 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..d5d3f93
--- /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
+
+chrony_helper=@CHRONY_HELPER@
+default_server_options=@CHRONY_DEFAULT_SERVER_OPTIONS@
+server_dir=@CHRONY_SERVER_DIR@
+
+dhcp_server_file=$server_dir/chrony.servers.$interface
+# 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"
+    done
+    $chrony_helper update-daemon || :
+}
+
+clear_servers_from_dhcp() {
+    if [ -f "$dhcp_server_file" ]; then
+        rm -f "$dhcp_server_file"
+        $chrony_helper update-daemon || :
+    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 56da2df915ca1997fc70c958d5a139aeb4c76776 Mon Sep 17 00:00:00 2001
From: Robert Fairley <rfairley@xxxxxxxxxx>
Date: Thu, 4 Jun 2020 14:48:40 -0400
Subject: [PATCH 3/3] 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
index d50ccf7..6dbab81 100644
--- a/examples/chrony.helper.in
+++ b/examples/chrony.helper.in
@@ -8,7 +8,7 @@
 chronyc=@CHRONYC@
 chrony_conf=@CHRONY_CONF@
 chrony_service=@CHRONY_SERVICE@
-default_options=@CHRONY_DEFAULT_OPTIONS@
+default_server_options=@CHRONY_DEFAULT_SERVER_OPTIONS@
 helper_dir=@CHRONY_HELPER_DIR@
 server_dir=@CHRONY_SERVER_DIR@
 
@@ -88,7 +88,7 @@ get_dnssrv_servers() {
     echo "$output" | while read -r _ _ port target; do
         server=${target%.}
         [ -z "$server" ] && continue
-        echo "$server port $port $default_options"
+        echo "$server port $port $default_server_options"
     done
 }
 
diff --git a/examples/chrony.nm-dispatcher.dhcp.in b/examples/chrony.nm-dispatcher.dhcp.in
index 5835e05..d5d3f93 100644
--- a/examples/chrony.nm-dispatcher.dhcp.in
+++ b/examples/chrony.nm-dispatcher.dhcp.in
@@ -11,7 +11,7 @@ interface=$1
 action=$2
 
 chrony_helper=@CHRONY_HELPER@
-default_options=@CHRONY_DEFAULT_OPTIONS@
+default_server_options=@CHRONY_DEFAULT_SERVER_OPTIONS@
 server_dir=@CHRONY_SERVER_DIR@
 
 dhcp_server_file=$server_dir/chrony.servers.$interface
@@ -21,7 +21,7 @@ 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_options" >> "$dhcp_server_file"
+        echo "$server $default_server_options" >> "$dhcp_server_file"
     done
     $chrony_helper update-daemon || :
 }
diff --git a/examples/chrony.nm-dispatcher.in b/examples/chrony.nm-dispatcher.onoffline.in
similarity index 100%
rename from examples/chrony.nm-dispatcher.in
rename to examples/chrony.nm-dispatcher.onoffline.in