diff mbox series

[bug#60735,v2,2/3] system: Deprecate hosts-file.

Message ID a814b828bfbbbe13791cb5ab74bf45a1ad617ab5.1674060850.git.mirai@makinata.eu
State New
Headers show
Series [bug#60735,v2,1/3] services: Add etc-hosts-service-type. | expand

Commit Message

Bruno Victal Jan. 18, 2023, 4:54 p.m. UTC 
* gnu/system.scm (operating-system-hosts-file): Deprecate procedure.
(warn-hosts-file-field-deprecation): New procedure, helper for
deprecated variable).
(operating-system)[hosts-file]: Use helper to warn deprecated field.
(operating-system-default-essential-services)
(hurd-default-essential-services): Use hosts-service-type.
(local-host-aliases): Return a list of host-entry records.
(default-/etc/hosts): Remove procedure.
(operating-system-etc-service): Remove hosts file.
* doc/guix.texi (operating-system Reference)
(Networking Services) (Virtualization Services): Rewrite documentation
entries to use hosts-service-type.
* gnu/tests/ganeti.scm: Use hosts-service-type extension.
---

WIP, the ganeti tests fail because host-name is added as an alias of localhost.
Ideally hosts-service-type should be moved to %base-services but we lose access
to os host-name if we do so.

 doc/guix.texi        | 60 +++++++++++++++++++++++---------------------
 gnu/system.scm       | 59 ++++++++++++++++++++++++++++++++-----------
 gnu/tests/ganeti.scm | 18 ++++++-------
 3 files changed, 85 insertions(+), 52 deletions(-)

Comments

Ludovic Courtès Jan. 23, 2023, 10:37 p.m. UTC | #1 
Bruno Victal <mirai@makinata.eu> skribis:

> * gnu/system.scm (operating-system-hosts-file): Deprecate procedure.
> (warn-hosts-file-field-deprecation): New procedure, helper for
> deprecated variable).
> (operating-system)[hosts-file]: Use helper to warn deprecated field.
> (operating-system-default-essential-services)
> (hurd-default-essential-services): Use hosts-service-type.
> (local-host-aliases): Return a list of host-entry records.
> (default-/etc/hosts): Remove procedure.
> (operating-system-etc-service): Remove hosts file.
> * doc/guix.texi (operating-system Reference)
> (Networking Services) (Virtualization Services): Rewrite documentation
> entries to use hosts-service-type.
> * gnu/tests/ganeti.scm: Use hosts-service-type extension.
> ---
>
> WIP, the ganeti tests fail because host-name is added as an alias of localhost.

Before these patches, ‘host-name’ was already an alias of ‘localhost’.
Is there something else interfering?

> +    (simple-service 'block-facebook-hosts hosts-service-type
> +                    (let ((host-pairs
> +                            (filter-map
> +                              (lambda (x)
> +                                (and (not (or (string-null? x)
> +                                              (string-prefix? "#" x)))
> +	                             (remove string-null?
> +                                             (string-split
> +                                               x
> +                                               char-set:whitespace))))
> +                              (string-split %facebook-host-aliases #\newline))))
> +                      (map (match-lambda
> +                             ((addr name)
> +                              (host addr name)))
> +                           host-pairs)))

It doesn’t matter because it’s removed in the commit that follows I
think using ‘string-tokenize’ instead of ‘string-split’ may bring
simplifications.

> +++ b/gnu/system.scm
> @@ -14,6 +14,7 @@
>  ;;; Copyright © 2020, 2022 Efraim Flashner <efraim@flashner.co.il>
>  ;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
>  ;;; Copyright © 2021 raid5atemyhomework <raid5atemyhomework@protonmail.com>
> +;;; Copyright © 2023 Bruno Victal <mirai@makinata.eu>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -31,6 +32,7 @@
>  ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
>  
>  (define-module (gnu system)
> +  #:use-module (guix discovery)

Do we really need this module?

Otherwise LGTM.

Ludo’.
Bruno Victal Jan. 23, 2023, 11:19 p.m. UTC | #2 
On 2023-01-23 22:37, Ludovic Courtès wrote:
> Bruno Victal <mirai@makinata.eu> skribis:
> 
>> ---
>>
>> WIP, the ganeti tests fail because host-name is added as an alias of localhost.
> 
> Before these patches, ‘host-name’ was already an alias of ‘localhost’.
> Is there something else interfering?

In some cases, it's not desired for host-name to be an alias of localhost.
The ganeti tests did this by passing a hosts-file file-like object where
host-name wasn't an alias of localhost.

I've brainstormed a bit on this and here's what I thought:

Approach 1 (DOESN'T WORK):
* hosts-service-type in essential-services (gnu/systems.scm), default value:
	127.0.0.1  localhost
	::1  localhost
* simple-service extension on base-services (gnu/services/base.scm):
	\\FLOPS since both /etc/hosts and /etc/hostname are provisioned with activation-service-type.
	This means we can't write /etc/hosts AFTER /etc/hostname or host-name-service-type is ready.

Approach 2:
* NO /etc/hosts in essential-services (is this possible?)
	* is an absent /etc/hosts (or absent %base-services) a valid OS?
* Value set in %base-services, hosts-service-type as a ONE-SHOT shepherd service.
	* Can be changed with modify-services.
	* It's a one-shot shepherd service since we're depending on /etc/hostname which is activation-service-type. (we're depending on either etc-service-type or host-name-service-type)

Approach 3:
* Do not set our hostname as an alias of localhost by default.
	* Manpage doesn't seem to make this mandatory, in fact, our hostname can point to any IP. (it says 'often', not 'mandatory')
	* We only set localhost name.
		* Is this mandatory? If not, there might be cases where this entry is undesired.

> 
>> +    (simple-service 'block-facebook-hosts hosts-service-type
>> +                    (let ((host-pairs
>> +                            (filter-map
>> +                              (lambda (x)
>> +                                (and (not (or (string-null? x)
>> +                                              (string-prefix? "#" x)))
>> +	                             (remove string-null?
>> +                                             (string-split
>> +                                               x
>> +                                               char-set:whitespace))))
>> +                              (string-split %facebook-host-aliases #\newline))))
>> +                      (map (match-lambda
>> +                             ((addr name)
>> +                              (host addr name)))
>> +                           host-pairs)))
> 
> It doesn’t matter because it’s removed in the commit that follows I
> think using ‘string-tokenize’ instead of ‘string-split’ may bring
> simplifications.

It was added because otherwise the "split" commits would seem to be missing some context.
I can leave it as is, delete it here or try your suggestion.

>> +++ b/gnu/system.scm
>> @@ -14,6 +14,7 @@
>>  ;;; Copyright © 2020, 2022 Efraim Flashner <efraim@flashner.co.il>
>>  ;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
>>  ;;; Copyright © 2021 raid5atemyhomework <raid5atemyhomework@protonmail.com>
>> +;;; Copyright © 2023 Bruno Victal <mirai@makinata.eu>
>>  ;;;
>>  ;;; This file is part of GNU Guix.
>>  ;;;
>> @@ -31,6 +32,7 @@
>>  ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
>>  
>>  (define-module (gnu system)
>> +  #:use-module (guix discovery)
> 
> Do we really need this module?

IIRC this was for the deprecated procedures to work. Can they work without this module?


Cheers,
Bruno
Ludovic Courtès Jan. 24, 2023, 8:53 a.m. UTC | #3 
Hi Bruno!

Bruno Victal <mirai@makinata.eu> skribis:

> On 2023-01-23 22:37, Ludovic Courtès wrote:
>> Bruno Victal <mirai@makinata.eu> skribis:
>> 
>>> ---
>>>
>>> WIP, the ganeti tests fail because host-name is added as an alias of localhost.
>> 
>> Before these patches, ‘host-name’ was already an alias of ‘localhost’.
>> Is there something else interfering?
>
> In some cases, it's not desired for host-name to be an alias of localhost.
> The ganeti tests did this by passing a hosts-file file-like object where
> host-name wasn't an alias of localhost.

Oh, I see.

> I've brainstormed a bit on this and here's what I thought:
>
> Approach 1 (DOESN'T WORK):
> * hosts-service-type in essential-services (gnu/systems.scm), default value:
> 	127.0.0.1  localhost
> 	::1  localhost
> * simple-service extension on base-services (gnu/services/base.scm):
> 	\\FLOPS since both /etc/hosts and /etc/hostname are provisioned with activation-service-type.
> 	This means we can't write /etc/hosts AFTER /etc/hostname or host-name-service-type is ready.
>
> Approach 2:
> * NO /etc/hosts in essential-services (is this possible?)
> 	* is an absent /etc/hosts (or absent %base-services) a valid OS?
> * Value set in %base-services, hosts-service-type as a ONE-SHOT shepherd service.
> 	* Can be changed with modify-services.
> 	* It's a one-shot shepherd service since we're depending on /etc/hostname which is activation-service-type. (we're depending on either etc-service-type or host-name-service-type)
>
> Approach 3:
> * Do not set our hostname as an alias of localhost by default.
> 	* Manpage doesn't seem to make this mandatory, in fact, our hostname can point to any IP. (it says 'often', not 'mandatory')
> 	* We only set localhost name.
> 		* Is this mandatory? If not, there might be cases where this entry is undesired.

Wait, why don’t we keep ‘hosts-service-type’ in ‘essential-services’,
with the localhost/host-name alias, and have ‘%ganeti-os’ in (gnu tests
ganeti) modify its essential services to get what it wants?  As in:

  (operating-system
    ;; …
    (essential-services
      (modify-services (operation-system-default-essential-services this-operating-system)
        (hosts-service-type config => …))))

Granted, that’s a bit verbose :-), but it should do the job just like
setting ‘hosts-file’ currently in ‘master’, no?

>> It doesn’t matter because it’s removed in the commit that follows I
>> think using ‘string-tokenize’ instead of ‘string-split’ may bring
>> simplifications.
>
> It was added because otherwise the "split" commits would seem to be missing some context.
> I can leave it as is, delete it here or try your suggestion.

Yeah leave it as is.

>>> +  #:use-module (guix discovery)
>> 
>> Do we really need this module?
>
> IIRC this was for the deprecated procedures to work. Can they work without this module?

Yes, ‘define-deprecated’ is defined in (guix deprecation).

Thanks!

Ludo’.
diff mbox series

Patch

diff --git a/doc/guix.texi b/doc/guix.texi
index 5fb3df441c..eb4c1a2dbb 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -16479,13 +16479,6 @@  operating-system Reference
 @item @code{host-name}
 The host name.
 
-@item @code{hosts-file}
-@cindex hosts file
-A file-like object (@pxref{G-Expressions, file-like objects}) for use as
-@file{/etc/hosts} (@pxref{Host Names,,, libc, The GNU C Library
-Reference Manual}).  The default is a file with entries for
-@code{localhost} and @var{host-name}.
-
 @item @code{mapped-devices} (default: @code{'()})
 A list of mapped devices.  @xref{Mapped Devices}.
 
@@ -21010,22 +21003,33 @@  Networking Services
 on-line service---e.g., @code{www.facebook.com}---to the local
 host---@code{127.0.0.1} or its IPv6 equivalent, @code{::1}.
 
-This variable is typically used in the @code{hosts-file} field of an
-@code{operating-system} declaration (@pxref{operating-system Reference,
-@file{/etc/hosts}}):
+This variable is typically used as a @code{hosts-service-type}
+service extension (@pxref{Service Reference, @code{hosts-service-type}}):
 
 @lisp
-(use-modules (gnu) (guix))
+(use-modules (gnu) (gnu services) (guix) (srfi srfi-1) (ice-9 match))
+(use-service-modules networking)
 
 (operating-system
-  (host-name "mymachine")
-  ;; ...
-  (hosts-file
-    ;; Create a /etc/hosts file with aliases for "localhost"
-    ;; and "mymachine", as well as for Facebook servers.
-    (plain-file "hosts"
-                (string-append (local-host-aliases host-name)
-                               %facebook-host-aliases))))
+  ;; @dots{}
+
+  (service
+    (simple-service 'block-facebook-hosts hosts-service-type
+                    (let ((host-pairs
+                            (filter-map
+                              (lambda (x)
+                                (and (not (or (string-null? x)
+                                              (string-prefix? "#" x)))
+	                             (remove string-null?
+                                             (string-split
+                                               x
+                                               char-set:whitespace))))
+                              (string-split %facebook-host-aliases #\newline))))
+                      (map (match-lambda
+                             ((addr name)
+                              (host addr name)))
+                           host-pairs)))
+    ;; @dots{}
 @end lisp
 
 This mechanism can prevent programs running locally, such as Web
@@ -34310,7 +34314,7 @@  Virtualization Services
 services which are described later in this section.  In addition to the Ganeti
 service, you will need the OpenSSH service (@pxref{Networking Services,
 @code{openssh-service-type}}), and update the @file{/etc/hosts} file
-(@pxref{operating-system Reference, @code{hosts-file}}) with the cluster name
+(@pxref{Service Reference, @code{hosts-service-type}}) with the cluster name
 and address (or use a DNS server).
 
 All nodes participating in a Ganeti cluster should have the same Ganeti and
@@ -34324,14 +34328,6 @@  Virtualization Services
 (operating-system
   ;; @dots{}
   (host-name "node1")
-  (hosts-file (plain-file "hosts" (format #f "
-127.0.0.1       localhost
-::1             localhost
-
-192.168.1.200   ganeti.example.com
-192.168.1.201   node1.example.com node1
-192.168.1.202   node2.example.com node2
-")))
 
   ;; Install QEMU so we can use KVM-based instances, and LVM, DRBD and Ceph
   ;; in order to use the "plain", "drbd" and "rbd" storage backends.
@@ -34359,6 +34355,14 @@  Virtualization Services
                           (openssh-configuration
                            (permit-root-login 'prohibit-password)))
 
+                 (simple-service 'ganeti-hosts-entries hosts-service-type
+                                 (list
+                                   (host "192.168.1.200" "ganeti.example.com")
+                                   (host "192.168.1.201" "node1.example.com"
+                                         '("node1"))
+                                   (host "192.168.1.202" "node2.example.com"
+                                         '("node2"))))
+
                  (service ganeti-service-type
                           (ganeti-configuration
                            ;; This list specifies allowed file system paths
diff --git a/gnu/system.scm b/gnu/system.scm
index d67f9a615b..d80f2a3a23 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -14,6 +14,7 @@ 
 ;;; Copyright © 2020, 2022 Efraim Flashner <efraim@flashner.co.il>
 ;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
 ;;; Copyright © 2021 raid5atemyhomework <raid5atemyhomework@protonmail.com>
+;;; Copyright © 2023 Bruno Victal <mirai@makinata.eu>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -31,6 +32,7 @@ 
 ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
 (define-module (gnu system)
+  #:use-module (guix discovery)
   #:use-module (guix inferior)
   #:use-module (guix store)
   #:use-module (guix memoization)
@@ -97,7 +99,7 @@  (define-module (gnu system)
             operating-system-user-services
             operating-system-packages
             operating-system-host-name
-            operating-system-hosts-file
+            operating-system-hosts-file ;deprecated
             operating-system-hurd
             operating-system-kernel
             operating-system-kernel-file
@@ -208,6 +210,15 @@  (define* (bootable-kernel-arguments system root-device version)
                          #$system "/boot")))
 
 ;; System-wide configuration.
+
+(define-with-syntax-properties (warn-hosts-file-field-deprecation
+                                (value properties))
+  (when value
+    (warning (source-properties->location properties)
+             (G_ "the 'hosts-file' field is deprecated, please use \
+'hosts-service-type' instead~%")))
+  value)
+
 ;; TODO: Add per-field docstrings/stexi.
 (define-record-type* <operating-system> operating-system
   make-operating-system
@@ -239,8 +250,9 @@  (define-record-type* <operating-system> operating-system
             (default %base-firmware))
 
   (host-name operating-system-host-name)          ; string
-  (hosts-file operating-system-hosts-file         ; file-like | #f
-              (default #f))
+  (hosts-file %operating-system-hosts-file         ; deprecated
+              (default #f)
+              (sanitize warn-hosts-file-field-deprecation))
 
   (mapped-devices operating-system-mapped-devices ; list of <mapped-device>
                   (default '()))
@@ -296,6 +308,10 @@  (define-record-type* <operating-system> operating-system
                             source-properties->location))
             (innate)))
 
+(define-deprecated (operating-system-hosts-file os)
+  hosts-service-type
+  (%operating-system-hosts-file os))
+
 (define* (operating-system-kernel-arguments
           os root-device #:key (version %boot-parameters-version))
   "Return all the kernel arguments, including the ones not specified directly
@@ -733,7 +749,8 @@  (define (operating-system-default-essential-services os)
          (non-boot-fs  (non-boot-file-system-service os))
          (swaps        (swap-services os))
          (procs        (service user-processes-service-type))
-         (host-name    (host-name-service (operating-system-host-name os)))
+         (host-name    (operating-system-host-name os))
+         (hosts-file   (operating-system-hosts-file os))
          (entries      (operating-system-directory-base-entries os)))
     (cons* (service system-service-type entries)
            (service linux-builder-service-type
@@ -755,12 +772,19 @@  (define (operating-system-default-essential-services os)
                                     (operating-system-groups os))
                             (operating-system-skeletons os))
            (operating-system-etc-service os)
+           ;; XXX: hosts-file is deprecated
+           (if hosts-file
+               (simple-service 'deprecated-hosts-file etc-service-type
+                               (list `("hosts" ,hosts-file)))
+               (service hosts-service-type
+                        (local-host-aliases host-name)))
            (service fstab-service-type
                     (filter file-system-needed-for-boot?
                             (operating-system-file-systems os)))
            (session-environment-service
             (operating-system-environment-variables os))
-           host-name procs root-fs
+           (host-name-service host-name)
+           procs root-fs
            (service setuid-program-service-type
                     (operating-system-setuid-programs os))
            (service profile-service-type
@@ -774,7 +798,9 @@  (define (operating-system-default-essential-services os)
                                   (operating-system-firmware os)))))))
 
 (define (hurd-default-essential-services os)
-  (let ((entries (operating-system-directory-base-entries os)))
+  (let ((host-name    (operating-system-host-name os))
+        (hosts-file   (operating-system-hosts-file os))
+        (entries      (operating-system-directory-base-entries os)))
     (list (service system-service-type entries)
           %boot-service
           %hurd-startup-service
@@ -794,6 +820,12 @@  (define (hurd-default-essential-services os)
                            (operating-system-file-systems os)))
           (pam-root-service (operating-system-pam-services os))
           (operating-system-etc-service os)
+          ;; XXX: hosts-file is deprecated
+          (if hosts-file
+              (simple-service 'deprecated-hosts-file etc-service-type
+                              (list `("hosts" ,hosts-file)))
+              (service hosts-service-type
+                       (local-host-aliases host-name)))
           (service setuid-program-service-type
                    (operating-system-setuid-programs os))
           (service profile-service-type (operating-system-packages os)))))
@@ -914,12 +946,13 @@  (define %default-issue
 
 (define (local-host-aliases host-name)
   "Return aliases for HOST-NAME, to be used in /etc/hosts."
-  (string-append "127.0.0.1 localhost " host-name "\n"
-                 "::1       localhost " host-name "\n"))
-
-(define (default-/etc/hosts host-name)
-  "Return the default /etc/hosts file."
-  (plain-file "hosts" (local-host-aliases host-name)))
+  (map (lambda (address)
+         (host-entry
+          (address address)
+          (canonical-name "localhost")
+          (aliases (list host-name))))
+       '("127.0.0.1"
+         "::1")))
 
 (define (validated-sudoers-file file)
   "Return a copy of FILE, a sudoers file, after checking that it is
@@ -1068,8 +1101,6 @@  (define* (operating-system-etc-service os)
        ,@(if nsswitch `(("nsswitch.conf" ,#~#$nsswitch)) '())
        ("profile" ,#~#$profile)
        ("bashrc" ,#~#$bashrc)
-       ("hosts" ,#~#$(or (operating-system-hosts-file os)
-                         (default-/etc/hosts (operating-system-host-name os))))
        ;; Write the operating-system-host-name to /etc/hostname to prevent
        ;; NetworkManager from changing the system's hostname when connecting
        ;; to certain networks.  Some discussion at
diff --git a/gnu/tests/ganeti.scm b/gnu/tests/ganeti.scm
index f647e9554c..10ec2980ee 100644
--- a/gnu/tests/ganeti.scm
+++ b/gnu/tests/ganeti.scm
@@ -46,16 +46,6 @@  (define %ganeti-os
                         %base-file-systems))
     (firmware '())
 
-    ;; The hosts file must contain a nonlocal IP for host-name.
-    ;; In addition, the cluster name must resolve to an IP address that
-    ;; is not currently provisioned.
-    (hosts-file (plain-file "hosts" (format #f "
-127.0.0.1       localhost
-::1             localhost
-10.0.2.15       gnt1.example.com gnt1
-192.168.254.254 ganeti.example.com
-")))
-
     (packages (append (list ganeti-instance-debootstrap ganeti-instance-guix)
                       %base-packages))
     (services
@@ -65,6 +55,14 @@  (define %ganeti-os
                             (openssh-configuration
                              (permit-root-login 'prohibit-password)))
 
+                   ;; The hosts file must contain a nonlocal IP for host-name.
+                   ;; In addition, the cluster name must resolve to an IP address that
+                   ;; is not currently provisioned.
+                   (simple-service 'ganeti-host-entries hosts-service-type
+                                   (list
+                                    (host "10.0.2.15" "gnt1.example.com" '("gnt1"))
+                                    (host "192.168.254.254" "ganeti.example.com")))
+
                    (service ganeti-service-type
                             (ganeti-configuration
                              (file-storage-paths '("/srv/ganeti/file-storage"))