[PATCH v2 0/1] services: network-manager: Add extra-configuration-files field.

[PATCH v2 0/1] services: network-manager: Add extra-configuration-files field.

[45mg]

Hi Guix,

As the first revision of this patch failed to get any attention in two weeks,
I'm CC'ing guix-devel on this one, hoping it'll get reviewed this time. Let me
know if I should have done anything differently; I'm still relatively new to
contributing.

This patch allows users to specify configuration files for NetworkManager.
While perhaps it would be more Guix-y to instead have a field in
`network-manager-configuration` for every configuration option, this would be
a monumental undertaking (look at the number of options listed in
NetworkManager.conf(5)!). At any rate, I think any means of configuring
NetworkManager is better than none.

The difference from the first revision is that instead of specifying a single
directory (file-like object) containing the configuration files (which was
then symlinked to /etc/NetworkManager/conf.d), you now specify an alist
mapping file names to file-like objects, like with `etc-service-type`; and
those are then added to /etc/NetworkManager/conf.d. The rationale behind this
change is that it doesn't rule out putting our own stuff in
/etc/NetworkManager/conf.d. For example, if we wanted a default set of files
in there, we could modify the procedure
`network-manager-configuration-directory` to add the files supplied via the
field to our default set; in the first revision, this wouldn't be possible as
the user specifies the entire directory. (I don't know whether we'd ever
actually want to do this, but I thought it best to leave our options open.)

45mg (1):
  services: network-manager: Add extra-configuration-files field.

 doc/guix.texi               | 21 +++++++++++++++++++++
 gnu/services/networking.scm | 26 ++++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 2 deletions(-)


base-commit: 7f27dc47c52886b785359799b6dc67b61f638544
-- 
2.47.1

Re: [bug#75145] [PATCH v2 0/1] services: network-manager: Add extra-configuration-files field.

[Arnaud Daby-Seesaram]

[-- Attachment #1: Type: text/plain, Size: 4146 bytes --]

Hi,

Thank you for your patch.

45mg <45mg.writes@gmail.com> writes:
> As the first revision of this patch failed to get any attention in two weeks,
> I'm CC'ing guix-devel on this one, hoping it'll get reviewed this time. Let me
> know if I should have done anything differently; I'm still relatively new to
> contributing.

I am also new to contributing, so the following may not be fully
accurate.

I think that you did everything right.  Unfortunately, some patches
"fall through the cracks".  I have seen some people replying to
issues [0], so that emails would show up in people' mailboxes (similar
to what you did) and restart the discussion.

I think that it is also possible to suggest simple patches for patch
review events [1].  I do not know if proposing one's own patches is
frowned upon or not, but I would not expect it to be as long as there is
no abuse.

[0] Random example: https://issues.guix.gnu.org/64620#21
[1] Wiki page for last year:
    https://libreplanet.org/wiki/Group:Guix/PatchReviewSessions2024

> This patch allows users to specify configuration files for NetworkManager.
> While perhaps it would be more Guix-y to instead have a field in
> `network-manager-configuration` for every configuration option, this would be
> a monumental undertaking (look at the number of options listed in
> NetworkManager.conf(5)!). At any rate, I think any means of configuring
> NetworkManager is better than none.

I agree that an escape hatch enabling to configure NetworkManager would
be nice indeed.

> The difference from the first revision is that instead of specifying a single
> directory (file-like object) containing the configuration files (which was
> then symlinked to /etc/NetworkManager/conf.d), you now specify an alist
> mapping file names to file-like objects, like with `etc-service-type`; and
> those are then added to /etc/NetworkManager/conf.d. The rationale behind this
> change is that it doesn't rule out putting our own stuff in
> /etc/NetworkManager/conf.d. For example, if we wanted a default set of files
> in there, we could modify the procedure
> `network-manager-configuration-directory` to add the files supplied via the
> field to our default set; in the first revision, this wouldn't be possible as
> the user specifies the entire directory. (I don't know whether we'd ever
> actually want to do this, but I thought it best to leave our options open.)

On adding default files in .../conf.d/:
```````````````````````````````````````

Adding a default configuration could one day be interesting.  However,
I would rather (personal opinion) see the default value in an exported
variable %default-networkmanager-files (non-contractual name) if needed.
This variable could become the default value of the field.

This way, it would be more transparent to users, and would enable them
to easily opt-out.  Adding their files could be done with the following
configuration:

--8<---------------cut here---------------start------------->8---
(extra-configuration-files
  (cons* `("file1" ,(plain-file ...))
         ...
         %default-networkmanager-files))
--8<---------------cut here---------------end--------------->8---


On doing a similar job than `etc-service-type':
```````````````````````````````````````````````

As you said, you are doing something similar to `etc-service-type'.  Is
there a reason not to extend [2] it directly (e.g. like
`greetd-service-type' does in `(gnu services base)')?

You could, for example, prepend "NetworkManager/conf.d/" to file names
and pass the value to `etc-service-type'.  WDYT?

[2] more on service extension can be found in (guix)Service Reference:
    https://guix.gnu.org/manual/devel/en/html_node/Service-Reference.html


Final thought:
``````````````

Would it make sense to allow the NetworkManager service to be extended?
(E.g to allow another service to add a configuration file.)

Note: this is not a blocker for your patch and could be done in a later
      patch.  It is simply a thought that I wanted to share.


Best regards,

-- 
Arnaud

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

[bug#75145] [PATCH v2 0/1] services: network-manager: Add extra-configuration-files field.

[45mg]

Hi Arnaud!

Arnaud Daby-Seesaram <ds-ac@nanein.fr> writes:

> Adding a default configuration could one day be interesting.  However,
> I would rather (personal opinion) see the default value in an exported
> variable %default-networkmanager-files (non-contractual name) if needed.
> This variable could become the default value of the field.
>
> This way, it would be more transparent to users, and would enable them
> to easily opt-out.  Adding their files could be done with the following
> configuration:
>
> --8<---------------cut here---------------start------------->8---
> (extra-configuration-files
>   (cons* `("file1" ,(plain-file ...))
>          ...
>          %default-networkmanager-files))
> --8<---------------cut here---------------end--------------->8---

Yes, I think I agree. Of course, it's a moot point for now since we
don't have any use for such a variable yet, but it seems like a good
approach.

> As you said, you are doing something similar to `etc-service-type'.  Is
> there a reason not to extend [2] it directly (e.g. like
> `greetd-service-type' does in `(gnu services base)')?
>
> You could, for example, prepend "NetworkManager/conf.d/" to file names
> and pass the value to `etc-service-type'.  WDYT?

That's a good suggestion! I don't know why it didn't occur to me.

So I tried it; the problem is that if I pass something like this:
--8<---------------cut here---------------start------------->8---
(service-extension etc-service-type
                   (lambda (config)
                     `(("NetworkManager/conf.d"
                        ,(network-manager-configuration-directory config)))))
--8<---------------cut here---------------end--------------->8---

...then I get this error when creating a container:
--8<---------------cut here---------------start------------->8---
ERROR: In procedure primitive-load:
In procedure mkdir: Read-only file system
--8<---------------cut here---------------end--------------->8---

It looks like this is because of this line in `network-manager-activation`:
--8<---------------cut here---------------start------------->8---
        (mkdir-p "/etc/NetworkManager/system-connections")
--8<---------------cut here---------------end--------------->8---

When we use `etc-service-type`, "/etc/NetworkManager" becomes a symlink
to "/etc/static/NetworkManager"; and "/etc/static" is a symlink to the
result of building the derivation returned by `etc-entry` (see (gnu
services)). And derivations are in the store, which is read-only. So we
can't create "/etc/NetworkManager/system-connections/". And this won't
do, since NetworkManager itself needs to be able to write to that
directory to manage saved connections.

So it looks like that won't work.

> Would it make sense to allow the NetworkManager service to be extended?
> (E.g to allow another service to add a configuration file.)
>
> Note: this is not a blocker for your patch and could be done in a later
>       patch.  It is simply a thought that I wanted to share.

Yeah, this was another reason for this revision. Again, it's not
actually something we need right now; it's just worth leaving open as a
possibility.

Re: [PATCH v2 0/1] services: network-manager: Add extra-configuration-files field.

[Felix Lechner via Development of GNU Guix and the GNU System distribution.]

Hi 45mg,

On Thu, Jan 09 2025, 45mg wrote:

> this patch failed to get any attention in two weeks

Two weeks isn't a long time in free software projects.  A more common
time frame would be between one year and eighteen months.

> I'm CC'ing guix-devel on this one

Pings are generally frowned upon, but I also just did it yesterday. [1]
Pings are usually not worth it anywhere, not just in GNU Guix.  Pings
are likely to be ineffective in hastening the acceptance of your patch
but will almost certainly affect your social relationships negatively.

> This patch allows users to specify configuration files for NetworkManager.

If you require that change personally, as I suspect you do, my
recommendation would be to clone the Guix repo and pull your 'guix'
executable from your custom branch.  For example, I have in my
channels.scm a branch that I rebase periodically. [2]

I pull with

    guix pull --allow-downgrades --disable-authentication

That kind of setup totally removes the timing pressure from your
contribution.  It also allows you to test your patch in real life and
then argue with committers solely about its merits.  It's better for
everybody, I think.

It may leave everyone, most importantly yourself, in a happier place.

Kind regards
Felix

[1] https://lists.gnu.org/archive/html/guix-devel/2025-01/msg00056.html
[2] https://codeberg.org/lechner/guix-config/src/branch/history/channels.scm

[PATCH v2 1/1] services: network-manager: Add extra-configuration-files field.

[45mg]

Allow users to specify additional configuration files for
NetworkManager. These files will be added to
`/etc/NetworkManager/conf.d` (NetworkManager's default configuration
directory location).

* gnu/services/networking.scm (<network-manager-configuration>)
[extra-configuration-files]: New field.
(network-manager-configuration-directory): New procedure.
(network-manager-activation): Honor the new field.
* doc/guix.texi (Networking Setup): Document the new field.

Change-Id: I07479958e4d0aa318328c666a9630b779230b300
---
 doc/guix.texi               | 21 +++++++++++++++++++++
 gnu/services/networking.scm | 26 ++++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index caebe3b03c..279fdb838b 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -21455,6 +21455,27 @@ Networking Setup
 (VPNs).  An example of this is the @code{network-manager-openvpn}
 package, which allows NetworkManager to manage VPNs @i{via} OpenVPN.
 
+@item @code{extra-configuration-files} (default: @code{'()})
+An alist of file names to file-like objects, representing configuration
+files which will be added to @file{/etc/NetworkManager/conf.d}.
+NetworkManager will read additional configuration from this directory.
+For details on configuration file precedence and the configuration file
+format, see the @command{NetworkManager.conf(5)} man page.
+
+For example, to add two files @file{001-basic.conf} and
+@file{002-unmanaged.conf}:
+
+@lisp
+(service network-manager-service-type
+         (network-manager-configuration
+          (extra-configuration-files
+           `(("existing-file" ,(local-file "001-basic.conf"))
+             ("constructed-file" ,(plain-file "002-unmanaged.conf"
+                                              "[keyfile]
+unmanaged-devices=interface-name:wlo1_ap
+"))))))
+@end lisp
+
 @end table
 @end deftp
 
diff --git a/gnu/services/networking.scm b/gnu/services/networking.scm
index 48a86b3694..4355158225 100644
--- a/gnu/services/networking.scm
+++ b/gnu/services/networking.scm
@@ -1262,18 +1262,40 @@ (define-record-type* <network-manager-configuration>
                (default '()))
   (iwd? network-manager-configuration-iwd?  ; TODO: deprecated field, remove.
         (default #f)
-        (sanitize warn-iwd?-field-deprecation)))
+        (sanitize warn-iwd?-field-deprecation))
+  (extra-configuration-files network-manager-configuration-extra-configuration-files
+                             (default '())))  ;alist of file names to file-like objects
+
+(define (network-manager-configuration-directory extra-configuration-files)
+  "Return a directory containing EXTRA-CONFIGURATION-FILES."
+  (with-imported-modules (source-module-closure '((guix build utils)))
+    (computed-file
+     "network-manager-configuration-directory"
+     #~(begin
+         (use-modules (guix build utils))
+         (mkdir-p #$output)
+         (for-each (lambda (pair)
+                     (let* ((filename (list-ref pair 0))
+                            (file (list-ref pair 1))
+                            (dest (string-append #$output "/" filename)))
+                       (copy-file file dest)))
+                   '#$extra-configuration-files)))))
 
 (define (network-manager-activation config)
   ;; Activation gexp for NetworkManager
   (match-record config <network-manager-configuration>
-    (network-manager dns vpn-plugins)
+                (network-manager dns vpn-plugins extra-configuration-files)
     #~(begin
         (use-modules (guix build utils))
         (mkdir-p "/etc/NetworkManager/system-connections")
         #$@(if (equal? dns "dnsmasq")
                ;; create directory to store dnsmasq lease file
                '((mkdir-p "/var/lib/misc"))
+               '())
+        #$@(if extra-configuration-files
+               `((symlink
+                  ,(network-manager-configuration-directory extra-configuration-files)
+                  "/etc/NetworkManager/conf.d"))
                '()))))
 
 (define (vpn-plugin-directory plugins)
-- 
2.47.1