GNU bug report logs - #63403
[PATCH 1/1] services: wireguard: Implement a dynamic IP monitoring feature.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 63403 in the body.
You can then email your comments to 63403 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: guix-patches <at> gnu.org,
	maxim.cournoyer <at> gmail.com
Subject: [PATCH 1/1] services: wireguard: Implement a dynamic IP monitoring
 feature.
Date: Tue,  9 May 2023 21:09:00 -0400

* gnu/services/vpn.scm (<wireguard-configuration>)
[monitor-ips?, monitor-ips-internal]: New fields.
* gnu/services/vpn.scm (define-with-source): New syntax.
(wireguard-service-name, strip-port/maybe)
(ipv4-address?, ipv6-address?, host-name?)
(peers->endpoint-host-names)
(wireguard-monitoring-jobs): New procedures.
(wireguard-service-type): Register it.
* tests/services/vpn.scm: New file.
* Makefile.am (SCM_TESTS): Register it.
* doc/guix.texi (VPN Services): Update doc.
---
 Makefile.am            |   1 +
 doc/guix.texi          |  18 +++++-
 gnu/services/vpn.scm   | 122 +++++++++++++++++++++++++++++++++++++++--
 tests/services/vpn.scm |  80 +++++++++++++++++++++++++++
 4 files changed, 215 insertions(+), 6 deletions(-)
 create mode 100644 tests/services/vpn.scm

diff --git a/Makefile.am b/Makefile.am
index 13718e4353..fb6e4f57cd 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -553,6 +553,7 @@ SCM_TESTS =					\
   tests/services/lightdm.scm			\
   tests/services/linux.scm			\
   tests/services/telephony.scm			\
+  tests/services/vpn.scm			\
   tests/sets.scm				\
   tests/size.scm				\
   tests/status.scm				\
diff --git a/doc/guix.texi b/doc/guix.texi
index c69fde646d..fad7f32bca 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -32345,9 +32345,23 @@ VPN Services
 @item @code{dns} (default: @code{#f})
 The DNS server(s) to announce to VPN clients via DHCP.
 
+@item @code{monitor-ips?} (default: @code{#f})
+@cindex Dynamic IP, with Wireguard
+@cindex dyndns, usage with Wireguard
+Whether to monitor the resolved Internet addresses (IPs) of the
+endpoints of the configured peers, restarting the service when there is
+a mismatch between the endpoint IPs in actual use versus those freshly
+resolved from their host names.  Set this to @code{#t} if one or more
+endpoints use host names provided by a dynamic DNS service to keep
+connections working.
+
+@item @code{monitor-ips-internal} (default: @code{'(next-minute (range 0 60 5))})
+The time interval at which the IP monitoring job should run, provided as
+an mcron time specification (@pxref{Guile Syntax,,,mcron}).
+
 @item @code{private-key} (default: @code{"/etc/wireguard/private.key"})
-The private key file for the interface.  It is automatically generated if
-the file does not exist.
+The private key file for the interface.  It is automatically generated
+if the file does not exist.
 
 @item @code{peers} (default: @code{'()})
 The authorized peers on this interface.  This is a list of
diff --git a/gnu/services/vpn.scm b/gnu/services/vpn.scm
index a884d71eb2..5a56884008 100644
--- a/gnu/services/vpn.scm
+++ b/gnu/services/vpn.scm
@@ -11,6 +11,7 @@
 ;;; Copyright © 2021 Nathan Dehnel <ncdehnel <at> gmail.com>
 ;;; Copyright © 2022 Cameron V Chaparro <cameron <at> cameronchaparro.com>
 ;;; Copyright © 2022 Timo Wilken <guix <at> twilken.net>
+;;; Copyright © 2023 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -31,10 +32,12 @@ (define-module (gnu services vpn)
   #:use-module (gnu services)
   #:use-module (gnu services configuration)
   #:use-module (gnu services dbus)
+  #:use-module (gnu services mcron)
   #:use-module (gnu services shepherd)
   #:use-module (gnu system shadow)
   #:use-module (gnu packages admin)
   #:use-module (gnu packages vpn)
+  #:use-module (guix modules)
   #:use-module (guix packages)
   #:use-module (guix records)
   #:use-module (guix gexp)
@@ -73,6 +76,8 @@ (define-module (gnu services vpn)
             wireguard-configuration-addresses
             wireguard-configuration-port
             wireguard-configuration-dns
+            wireguard-configuration-monitor-ips?
+            wireguard-configuration-monitor-ips-interval
             wireguard-configuration-private-key
             wireguard-configuration-peers
             wireguard-configuration-pre-up
@@ -741,6 +746,10 @@ (define-record-type* <wireguard-configuration>
                       (default '()))
   (dns                wireguard-configuration-dns ;list of strings
                       (default #f))
+  (monitor-ips?       wireguard-configuration-monitor-ips? ;boolean
+                      (default #f))
+  (monitor-ips-interval wireguard-configuration-monitor-ips-interval
+                        (default '(next-minute (range 0 60 5)))) ;string | list
   (pre-up             wireguard-configuration-pre-up ;list of strings
                       (default '()))
   (post-up            wireguard-configuration-post-up ;list of strings
@@ -871,6 +880,49 @@ (define (wireguard-activation config)
             (chmod #$private-key #o400)
             (close-pipe pipe))))))
 
+;;; XXX: Copied from (guix scripts pack), changing define to define*.
+(define-syntax-rule (define-with-source (variable args ...) body body* ...)
+  "Bind VARIABLE to a procedure accepting ARGS defined as BODY, also setting
+its source property."
+  (begin
+    (define* (variable args ...)
+      body body* ...)
+    (eval-when (load eval)
+      (set-procedure-property! variable 'source
+                               '(define* (variable args ...) body body* ...)))))
+
+(define (wireguard-service-name interface)
+  "Return the WireGuard service name (a symbol) configured to use INTERFACE."
+  (symbol-append 'wireguard- (string->symbol interface)))
+
+(define-with-source (strip-port/maybe endpoint #:key ipv6?)
+  "Strip the colon and port, if present in ENDPOINT, a string."
+  (if ipv6?
+      (if (string-prefix? "[" endpoint)
+          (first (string-split (string-drop endpoint 1) #\])) ;ipv6
+          endpoint)
+      (first (string-split endpoint #\:)))) ;ipv4
+
+(define (ipv4-address? str)
+  "Return true if STR denotes an IPv4 address."
+  (false-if-exception
+   (->bool (inet-pton AF_INET (strip-port/maybe str)))))
+
+(define (ipv6-address? str)
+  "Return true if STR denotes an IPv6 address."
+  (false-if-exception
+   (->bool (inet-pton AF_INET6 (strip-port/maybe str #:ipv6? #t)))))
+
+(define (host-name? name)
+  "Predicate to check whether NAME is a host name, i.e. not an IP address."
+  (not (or (ipv6-address? name) (ipv4-address? name))))
+
+(define (peers->endpoint-host-names peers)
+  "Return host names used as the endpoints of PEERS, if any.  Any \":PORT\"
+suffixes are stripped."
+  (map strip-port/maybe
+       (filter host-name? (map wireguard-peer-endpoint peers))))
+
 (define (wireguard-shepherd-service config)
   (match-record config <wireguard-configuration>
     (wireguard interface)
@@ -878,9 +930,7 @@ (define (wireguard-shepherd-service config)
           (config (wireguard-configuration-file config)))
       (list (shepherd-service
              (requirement '(networking))
-             (provision (list
-                         (symbol-append 'wireguard-
-                                        (string->symbol interface))))
+             (provision (list (wireguard-service-name interface)))
              (start #~(lambda _
                        (invoke #$wg-quick "up" #$config)))
              (stop #~(lambda _
@@ -888,6 +938,68 @@ (define (wireguard-shepherd-service config)
                        #f))                       ;stopped!
              (documentation "Run the Wireguard VPN tunnel"))))))
 
+(define (wireguard-monitoring-jobs config)
+  (match-record config <wireguard-configuration>
+    (interface monitor-ips? monitor-ips-interval peers)
+    (let ((host-names (peers->endpoint-host-names peers)))
+      (if monitor-ips?
+          (if (null? host-names)
+              (begin
+                (warn "monitor-ips? is #t but no host name to monitor")
+                '())
+              ;; The mcron monitor job may be a string or a list; ungexp strips
+              ;; one quote level, which must be added back when a list is
+              ;; provided.
+              (list
+               #~(job
+                  (if (string? #$monitor-ips-interval)
+                      #$monitor-ips-interval
+                      '#$monitor-ips-interval)
+                  #$(program-file
+                     (format #f "wireguard-~a-monitoring" interface)
+                     (with-imported-modules (source-module-closure
+                                             '((gnu services herd)))
+                       #~(begin
+                           (use-modules (gnu services herd)
+                                        (ice-9 popen)
+                                        (ice-9 textual-ports)
+                                        (srfi srfi-1)
+                                        (srfi srfi-26))
+
+                           (define (host-name->ip name)
+                             "Return the IP address resolved from NAME."
+                             (let* ((ai (car (getaddrinfo name)))
+                                    (sa (addrinfo:addr ai)))
+                               (inet-ntop (sockaddr:fam sa)
+                                          (sockaddr:addr sa))))
+
+                           #$(procedure-source strip-port/maybe)
+
+                           (define service-name '#$(wireguard-service-name
+                                                    interface))
+
+                           (when (start-service service-name)
+                             (let* ((resolved-ips (map host-name->ip
+                                                       '#$host-names))
+                                    (pipe (open-pipe*
+                                           OPEN_READ
+                                           #$(file-append wireguard-tools
+                                                          "/bin/wg")
+                                           "show" #$interface "endpoints"))
+                                    (lines (string-split (get-string-all pipe)
+                                                         #\newline))
+                                    (used-ips (map (compose
+                                                    strip-port/maybe
+                                                    last
+                                                    (cut string-split <> #\tab))
+                                                   lines)))
+                               (close-pipe pipe)
+                               (unless (every (cut member <> used-ips)
+                                              resolved-ips)
+                                 (format #t "restarting ~a service due to \
+stale endpoint IPs~%" service-name)
+                                 (restart-service service-name))))))))))))))
+
 (define wireguard-service-type
   (service-type
    (name 'wireguard)
@@ -898,6 +1010,8 @@ (define wireguard-service-type
                              wireguard-activation)
           (service-extension profile-service-type
                              (compose list
-                                      wireguard-configuration-wireguard))))
+                                      wireguard-configuration-wireguard))
+          (service-extension mcron-service-type
+                             wireguard-monitoring-jobs)))
    (description "Set up Wireguard @acronym{VPN, Virtual Private Network}
 tunnels.")))
diff --git a/tests/services/vpn.scm b/tests/services/vpn.scm
new file mode 100644
index 0000000000..9c6fa65df6
--- /dev/null
+++ b/tests/services/vpn.scm
@@ -0,0 +1,80 @@
+;;; GNU Guix --- Functional package management for GNU
+;;; Copyright © 2023 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
+;;;
+;;; This file is part of GNU Guix.
+;;;
+;;; GNU Guix is free software; you can redistribute it and/or modify it
+;;; under the terms of the GNU General Public License as published by
+;;; the Free Software Foundation; either version 3 of the License, or (at
+;;; your option) any later version.
+;;;
+;;; GNU Guix is distributed in the hope that it will be useful, but
+;;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;;; GNU General Public License for more details.
+;;;
+;;; You should have received a copy of the GNU General Public License
+;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
+
+(define-module (tests services vpn)
+  #:use-module (gnu packages vpn)
+  #:use-module (gnu services vpn)
+  #:use-module (guix gexp)
+  #:use-module (ice-9 match)
+  #:use-module (srfi srfi-1)
+  #:use-module (srfi srfi-64))
+
+;;; Commentary:
+;;;
+;;; Unit tests for the (gnu services vpn) module.
+;;;
+;;; Code:
+
+;;; Access some internals for whitebox testing.
+(define ipv4-address? (@@ (gnu services vpn) ipv4-address?))
+(define ipv6-address? (@@ (gnu services vpn) ipv6-address?))
+(define host-name? (@@ (gnu services vpn) host-name?))
+(define peers->endpoint-host-names
+  (@@ (gnu services vpn) peers->endpoint-host-names))
+
+(test-begin "vpn-services")
+
+(test-assert "ipv4-address?"
+  (every ipv4-address?
+         (list "192.95.5.67:1234"
+               "10.0.0.1")))
+
+(test-assert "ipv6-address?"
+  (every ipv6-address?
+         (list "[2607:5300:60:6b0::c05f:543]:2468"
+               "2607:5300:60:6b0::c05f:543"
+               "2345:0425:2CA1:0000:0000:0567:5673:23b5"
+               "2345:0425:2CA1::0567:5673:23b5")))
+
+(define %wireguard-peers
+  (list (wireguard-peer
+         (name "dummy1")
+         (public-key "VlesLiEB5BFd//OD2ILKXviolfz+hodG6uZ+XjoalC8=")
+         (endpoint "some.dynamic-dns.service:53281")
+         (allowed-ips '()))
+        (wireguard-peer
+         (name "dummy2")
+         (public-key "AlesLiEB5BFd//OD2ILKXviolfz+hodG6uZ+XgoalC9=")
+         (endpoint "example.org")
+         (allowed-ips '()))
+        (wireguard-peer
+         (name "dummy3")
+         (public-key "BlesLiEB5BFd//OD2ILKXviolfz+hodG6uZ+XgoalC7=")
+         (endpoint "10.0.0.7:7777")
+         (allowed-ips '()))
+        (wireguard-peer
+         (name "dummy4")
+         (public-key "ClesLiEB5BFd//OD2ILKXviolfz+hodG6uZ+XgoalC6=")
+         (endpoint "[2345:0425:2CA1::0567:5673:23b5]:44444")
+         (allowed-ips '()))))
+
+(test-equal "peers->endpoint-host-names"
+  '("some.dynamic-dns.service" "example.org")
+  (peers->endpoint-host-names %wireguard-peers))
+
+(test-end "vpn-services")
-- 
2.39.2

Message #10 received at 63403 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: 63403 <at> debbugs.gnu.org
Subject: Re: bug#63403: [PATCH 1/1] services: wireguard: Implement a dynamic
 IP monitoring feature.
Date: Mon, 15 May 2023 11:57:02 -0400

Hi,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes:

> * gnu/services/vpn.scm (<wireguard-configuration>)
> [monitor-ips?, monitor-ips-internal]: New fields.
> * gnu/services/vpn.scm (define-with-source): New syntax.
> (wireguard-service-name, strip-port/maybe)
> (ipv4-address?, ipv6-address?, host-name?)
> (peers->endpoint-host-names)
> (wireguard-monitoring-jobs): New procedures.
> (wireguard-service-type): Register it.
> * tests/services/vpn.scm: New file.
> * Makefile.am (SCM_TESTS): Register it.
> * doc/guix.texi (VPN Services): Update doc.

I've found a bug when no endpoints were used.  The following changes
were needed:

--8<---------------cut here---------------start------------->8---
modified   gnu/services/vpn.scm
@@ -921,7 +921,7 @@ (define (peers->endpoint-host-names peers)
   "Return host names used as the endpoints of PEERS, if any.  Any \":PORT\"
 suffixes are stripped."
   (map strip-port/maybe
-       (filter host-name? (map wireguard-peer-endpoint peers))))
+       (filter host-name? (filter-map wireguard-peer-endpoint peers))))
 
 (define (wireguard-shepherd-service config)
   (match-record config <wireguard-configuration>
@@ -998,7 +998,8 @@ (define (wireguard-monitoring-jobs config)
                                               resolved-ips)
                                  (format #t "restarting ~a service due to \
 stale endpoint IPs~%" service-name)
-                                 (restart-service service-name))))))))))))))
+                                 (restart-service service-name))))))))))
+          '()))))                       ;monitor-ips? is #f
 
 (define wireguard-service-type
   (service-type
--8<---------------cut here---------------end--------------->8---

Will send a v2.

-- 
Thanks,
Maxim

Message #13 received at 63403 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 63402 <at> debbugs.gnu.org, 63403 <at> debbugs.gnu.org
Subject: Re: bug#63403: [PATCH 1/1] services: wireguard: Implement a dynamic
 IP monitoring feature.
Date: Mon, 22 May 2023 17:00:16 +0200

Hi,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> * gnu/services/herd.scm (current-service): New procedure, mostly reusing the
> existing current-services.
> (current-services): Implement in terms of the above procedure.

How about having (lookup-service name) that calls the ‘status’ action on
the given service and either returns a <live-service> or #f?

‘current-services’ might be implemented as (lookup-service 'root) but
this should be kept as an implementation detail.

Ludo’.

Message #16 received at 63403 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 63402 <at> debbugs.gnu.org, 63403 <at> debbugs.gnu.org
Subject: Re: bug#63403: [PATCH 1/1] services: wireguard: Implement a dynamic
 IP monitoring feature.
Date: Mon, 22 May 2023 17:03:57 +0200

Hi,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> * gnu/services/vpn.scm (<wireguard-configuration>)
> [monitor-ips?, monitor-ips-internal]: New fields.
> * gnu/services/vpn.scm (define-with-source): New syntax.
> (wireguard-service-name, strip-port/maybe)
> (ipv4-address?, ipv6-address?, host-name?)
> (endpoint-host-names): New procedure.
> (wireguard-monitoring-jobs): Likewise.
> (wireguard-service-type): Register it.
> * tests/services/vpn.scm: New file.
> * Makefile.am (SCM_TESTS): Register it.
> * doc/guix.texi (VPN Services): Update doc.

As discussed on IRC the other day, I tend to think that this is “not our
job” but rather upstream’s.  (As a rule of thumb, I think services
should merely expose what upstream implements.)

You mentioned that upstream has a shell script to do something similar.
Using that may not be as nice as what you propose here in terms of
integration, but the upside is that we wouldn’t have to maintain it
ourselves.

Would that be a viable option?  WDYT?

Thanks,
Ludo’.

Message #19 received at 63403 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 63402 <at> debbugs.gnu.org, 63403 <at> debbugs.gnu.org
Subject: Re: bug#63403: [PATCH 1/1] services: wireguard: Implement a dynamic
 IP monitoring feature.
Date: Mon, 22 May 2023 19:22:23 -0400

Hi Ludovic,

Ludovic Courtès <ludo <at> gnu.org> writes:

> Hi,
>
> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
>
>> * gnu/services/herd.scm (current-service): New procedure, mostly reusing the
>> existing current-services.
>> (current-services): Implement in terms of the above procedure.
>
> How about having (lookup-service name) that calls the ‘status’ action on
> the given service and either returns a <live-service> or #f?

I'd rather keep the name 'current-service', because 'lookup-service' is
already a public procedure exported by Shepherd's (shepherd service)
module; it'd be confusing.

> ‘current-services’ might be implemented as (lookup-service 'root) but
> this should be kept as an implementation detail.

Yeah, that's my view on current-services being implemented in terms of
(current-service 'root).  It's a bit weird, but that's because the
underlying API is not symmetrical either.

Thanks for taking a look!

-- 
Thanks,
Maxim

Message #22 received at 63403 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 63402 <at> debbugs.gnu.org, 63403 <at> debbugs.gnu.org
Subject: Re: bug#63403: [PATCH 1/1] services: wireguard: Implement a dynamic
 IP monitoring feature.
Date: Mon, 22 May 2023 19:32:08 -0400

Hi Ludovic,

Ludovic Courtès <ludo <at> gnu.org> writes:

> Hi,
>
> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
>
>> * gnu/services/vpn.scm (<wireguard-configuration>)
>> [monitor-ips?, monitor-ips-internal]: New fields.
>> * gnu/services/vpn.scm (define-with-source): New syntax.
>> (wireguard-service-name, strip-port/maybe)
>> (ipv4-address?, ipv6-address?, host-name?)
>> (endpoint-host-names): New procedure.
>> (wireguard-monitoring-jobs): Likewise.
>> (wireguard-service-type): Register it.
>> * tests/services/vpn.scm: New file.
>> * Makefile.am (SCM_TESTS): Register it.
>> * doc/guix.texi (VPN Services): Update doc.
>
> As discussed on IRC the other day, I tend to think that this is “not our
> job” but rather upstream’s.  (As a rule of thumb, I think services
> should merely expose what upstream implements.)
>
> You mentioned that upstream has a shell script to do something similar.
> Using that may not be as nice as what you propose here in terms of
> integration, but the upside is that we wouldn’t have to maintain it
> ourselves.

Yeah, upstream offers a contrib shell script called reresolve-dns.sh
[0], that works a bit differently (it's doesn't actually monitor IPs but
just keep a watch on when was the last successful handshake made).

[0]  https://github.com/WireGuard/wireguard-tools/blob/master/contrib/reresolve-dns/reresolve-dns.

> Would that be a viable option?  WDYT?

I think my Guile script is more precise in terms of what it does and
also produces useful output.  If I knew of the shell script existence
when I started I probably wouldn't have bothered re-implementing it in
Scheme, but since it's here, and better, I see no reason to not use it
:-).  I don't foresee high maintenance for the stable APIs involved
(resolving host names and setting an endpoint with 'wg set').

-- 
Thanks,
Maxim

Message #25 received at 63403 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 63402 <at> debbugs.gnu.org, 63403 <at> debbugs.gnu.org
Subject: Re: bug#63403: [PATCH 1/1] services: wireguard: Implement a dynamic
 IP monitoring feature.
Date: Wed, 24 May 2023 16:44:40 +0200

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> Ludovic Courtès <ludo <at> gnu.org> writes:
>
>> Hi,
>>
>> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
>>
>>> * gnu/services/herd.scm (current-service): New procedure, mostly reusing the
>>> existing current-services.
>>> (current-services): Implement in terms of the above procedure.
>>
>> How about having (lookup-service name) that calls the ‘status’ action on
>> the given service and either returns a <live-service> or #f?
>
> I'd rather keep the name 'current-service',

There’s no notion of a “current service” in the Shepherd; that would be
confusing to me.

> because 'lookup-service' is already a public procedure exported by
> Shepherd's (shepherd service) module; it'd be confusing.

Yeah well, I think we should clarify the client/server architecture and
the context in which (shepherd …) modules are meant to be used.  I made
a first attempt:

  https://git.savannah.gnu.org/cgit/shepherd.git/commit/?id=d3d437a34bcb11fc416bf141181d8908064aeceb

However, what matters most to me is that the procedure names really
represent what they do.  With that in mind, it’s no surprise that the
procedure to look up a service is called ‘lookup-service’ in both
contexts.

Thanks,
Ludo’.

Message #28 received at 63403 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 63402 <at> debbugs.gnu.org, 63403 <at> debbugs.gnu.org
Subject: Re: bug#63403: [PATCH 1/1] services: wireguard: Implement a dynamic
 IP monitoring feature.
Date: Wed, 24 May 2023 16:53:56 +0200

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> Yeah, upstream offers a contrib shell script called reresolve-dns.sh
> [0], that works a bit differently (it's doesn't actually monitor IPs but
> just keep a watch on when was the last successful handshake made).
>
> [0]  https://github.com/WireGuard/wireguard-tools/blob/master/contrib/reresolve-dns/reresolve-dns.
>
>> Would that be a viable option?  WDYT?
>
> I think my Guile script is more precise in terms of what it does and
> also produces useful output.  If I knew of the shell script existence
> when I started I probably wouldn't have bothered re-implementing it in
> Scheme, but since it's here, and better, I see no reason to not use it
> :-).  I don't foresee high maintenance for the stable APIs involved
> (resolving host names and setting an endpoint with 'wg set').

I don’t doubt your script is better (first because it’s in Guile ;-)).
I’m concerned about adding non-trivial “peripheral” code that we’ll all
be responsible for going forward (the Jami services pose a similar
challenge IMO: I experienced first-hand the maintenance burden recently
when investigating system test failures.)

So I’m a bit torn.  I sympathize with the need to improve those
services, but I’m also concerned what will happen if we don’t have clear
criteria to decide what to take and what to reject.

WDYT?

Ludo’.

Message #31 received at 63403 <at> debbugs.gnu.org (full text, mbox):

From: Bruno Victal <mirai <at> makinata.eu>
To: Ludovic Courtès <ludo <at> gnu.org>,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 63402 <at> debbugs.gnu.org, 63403 <at> debbugs.gnu.org
Subject: Re: [bug#63403] [PATCH 1/1] services: wireguard: Implement a dynamic
 IP monitoring feature.
Date: Wed, 24 May 2023 23:12:26 +0100

Hi Ludo’,

On 2023-05-24 15:53, Ludovic Courtès wrote:
> I don’t doubt your script is better (first because it’s in Guile ;-)).
> I’m concerned about adding non-trivial “peripheral” code that we’ll all
> be responsible for going forward (the Jami services pose a similar
> challenge IMO: I experienced first-hand the maintenance burden recently
> when investigating system test failures.)
> 
> So I’m a bit torn.  I sympathize with the need to improve those
> services, but I’m also concerned what will happen if we don’t have clear
> criteria to decide what to take and what to reject.
> 

I think having some “indigenous” guix capabilities is a good idea,
if the guix services are to be something more than a (lossy) scheme
translation of some daemon's configuration file syntax.

IMO as long the feature in question is:
* Not overly tailored to some specific setup scenario.
* Generic (or can be reasonably refactored/extended as needed)
* Improves the overall experience of a service.

It should be acceptable to have it in Guix since it brings more value
to the service subsystem. (rather than require a user to import
$MYSTERY_CHANNEL_FROM_INTERNET_USER_5554$ or reinvent the
ω+1 iteration of the same wheel)


-- 
Furthermore, I consider that nonfree software must be eradicated.

Cheers,
Bruno.

Message #34 received at 63403 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 63402 <at> debbugs.gnu.org, 63403 <at> debbugs.gnu.org
Subject: Re: bug#63403: [PATCH 1/1] services: wireguard: Implement a dynamic
 IP monitoring feature.
Date: Thu, 25 May 2023 11:13:10 -0400

Hi Ludovic,

Ludovic Courtès <ludo <at> gnu.org> writes:

> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
>
>> Yeah, upstream offers a contrib shell script called reresolve-dns.sh
>> [0], that works a bit differently (it's doesn't actually monitor IPs but
>> just keep a watch on when was the last successful handshake made).
>>
>> [0]  https://github.com/WireGuard/wireguard-tools/blob/master/contrib/reresolve-dns/reresolve-dns.
>>
>>> Would that be a viable option?  WDYT?
>>
>> I think my Guile script is more precise in terms of what it does and
>> also produces useful output.  If I knew of the shell script existence
>> when I started I probably wouldn't have bothered re-implementing it in
>> Scheme, but since it's here, and better, I see no reason to not use it
>> :-).  I don't foresee high maintenance for the stable APIs involved
>> (resolving host names and setting an endpoint with 'wg set').
>
> I don’t doubt your script is better (first because it’s in Guile ;-)).
> I’m concerned about adding non-trivial “peripheral” code that we’ll all
> be responsible for going forward (the Jami services pose a similar
> challenge IMO: I experienced first-hand the maintenance burden recently
> when investigating system test failures.)

I get that the Jami service is complex, but to be fair here the tests
being broken by a (good) change in the marionette behavior caused by
commit a09c7da, which also affected a few other tests, as demonstrated
in the follow-up commit f518882, rather than because it crumbled under
its own weight.  I personally think this service is a great test suite
for the service infrastructure in Guix :-)  I've now fixed the Jami test
suite with 99fc7e5.  Hopefully QA helps catching regressions like this
early in the future, avoiding the need to fix things after the facts.

> So I’m a bit torn.  I sympathize with the need to improve those
> services, but I’m also concerned what will happen if we don’t have clear
> criteria to decide what to take and what to reject.

I think this happens rarely enough that it can be left as an exercise of
judgement rather than policy; e.g. deemed to provide enough value to
justify the maintenance burden, keeping in mind that using some
'contrib' shell script from upstream is not guaranteed to be
maintenance-free.  In this case it's also not on any critical path: it'd
only affects users of the new feature; if it ever breaks only that
feature would be impacted.

-- 
Thanks,
Maxim

Message #37 received at 63403 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 63402 <at> debbugs.gnu.org, 63403 <at> debbugs.gnu.org
Subject: Re: bug#63403: [PATCH 1/1] services: wireguard: Implement a dynamic
 IP monitoring feature.
Date: Thu, 20 Jul 2023 22:15:20 -0400

Hi,

Ludovic Courtès <ludo <at> gnu.org> writes:

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
>
>> Ludovic Courtès <ludo <at> gnu.org> writes:
>>
>>> Hi,
>>>
>>> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
>>>
>>>> * gnu/services/herd.scm (current-service): New procedure, mostly reusing the
>>>> existing current-services.
>>>> (current-services): Implement in terms of the above procedure.
>>>
>>> How about having (lookup-service name) that calls the ‘status’ action on
>>> the given service and either returns a <live-service> or #f?
>>
>> I'd rather keep the name 'current-service',
>
> There’s no notion of a “current service” in the Shepherd; that would be
> confusing to me.

We already have current-services in the same module, documented as:

  Return the list of currently defined Shepherd services, represented as
  <live-service> objects.

It's already a public interface.  Here I was interested in having a
convenient way to retrieve a single live-service instead of the full
list, thus the singular version.

Does that make sense?

-- 
Thanks,
Maxim

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.