[RDY] iproute: copy files in /etc #39536

teto · 2018-04-26T10:28:58Z

When doing source routing/multihoming, it's practical to give names to routing
tables. The absence of the rt_table file in /etc make this impossible.
This patch recreates these files on rebuild so that they can be modified
by the user

Motivation for this change

see #38638.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

GrahamcOfBorg · 2018-04-26T10:45:54Z

Success on aarch64-linux (full log)

Attempted: iproute

Partial log (click to expand)

/nix/store/wk7fql3ag8aq1n2zy5v7r25h2cjnfqr0-iproute2-4.16.0

GrahamcOfBorg · 2018-04-26T19:12:15Z

Success on x86_64-linux (full log)

Attempted: iproute

Partial log (click to expand)

/nix/store/07jzv5nwz5p1rn7nmw9fnf9gikir7ivg-iproute2-4.16.0

Mic92 · 2018-04-27T09:39:23Z

nixos/modules/system/activation/activation-script.nix

@@ -119,6 +119,12 @@ in

    system.activationScripts.stdio = ""; # obsolete

+    system.activationScripts.iproute2 = ''
+      cp -R ${pkgs.iproute}/etc/iproute2 /etc


but does this not overwrite any modification done by the user?

I that case, would it not be better to have config options for those files? This feels wrong. The files should either be managed by nix or not touched like that at all..

I agree with that.

once iproute is installed, users expect ip route list table main to work but if iproute can't find the file than it breaks this expection. While the file can be modified, it does have default. Maybe iproute should be patched look somewhere else than /etc/iproute2 (like in /var/run) but some sort of default should exist. To prevent overwriting user changes, the activationScript could write it only if it does'nt already exist.

teto · 2018-04-28T00:32:09Z

yes it is a problem but i couldn't think of any better solution. in my case restarting networkmanager should be enough to fix the rt_table file/the routing tables (i update them through hm hooks). Le ven. 27 avr. 2018 19:09, Jörg Thalheim <notifications@github.com> a écrit :

…

***@***.**** commented on this pull request. ------------------------------ In nixos/modules/system/activation/activation-script.nix <#39536 (comment)>: > @@ -119,6 +119,12 @@ in system.activationScripts.stdio = ""; # obsolete + system.activationScripts.iproute2 = '' + cp -R ${pkgs.iproute}/etc/iproute2 /etc but does this not overwrite any modification done by the user? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#39536 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA2FOrAZh1vfhjhfsRkT5GPMGfWNDNkjks5tsudPgaJpZM4Tk6yt> .

matthewbauer · 2018-05-09T06:37:34Z

Hm... Does iproute follow any environment variables to change the location of these things?

teto · 2018-05-09T06:41:33Z

nope but you can configure an alternate path to look for on compilation (like /var/run).

matthewbauer · 2018-05-09T06:53:28Z

Maybe we should just patch it to look in /run/current-system/sw/etc. That would avoid the cp call.

To have /etc/iproute2 installed in /run/current-system/sw, you will need to add it to patchsToLink here:

nixpkgs/nixos/modules/config/system-path.nix

Lines 103 to 125 in b6ec6fd

    
           environment.pathsToLink = 
        
             [ "/bin" 
        
               "/etc/xdg" 
        
               "/etc/gtk-2.0" 
        
               "/etc/gtk-3.0" 
        
               "/lib" # FIXME: remove and update debug-info.nix 
        
               "/sbin" 
        
               "/share/applications" 
        
               "/share/desktop-directories" 
        
               "/share/doc" 
        
               "/share/emacs" 
        
               "/share/icons" 
        
               "/share/menus" 
        
               "/share/mime" 
        
               "/share/nano" 
        
               "/share/org" 
        
               "/share/themes" 
        
               "/share/vim-plugins" 
        
               "/share/vulkan" 
        
               "/share/kservices5" 
        
               "/share/kservicetypes5" 
        
               "/share/kxmlgui5" 
        
             ];

teto · 2018-05-09T08:58:58Z

For my specific scenario, the file is modified dynamically depending on network conditions. If a network pops up, it will create its own routing table alias in /etc/iproute2/rt_tables. So the file needs to be writable (ideally reset on boot), I don't think I can do this with pathsToLink.

matthewbauer · 2018-05-09T09:07:04Z

Ok. This would probably make sense as a module then. It definitely should not be done by default. Or at least not until other users start complaining about the missing routing tables.

teto · 2018-05-11T08:47:34Z

I made it a module, I can merge it with the networking module if need be.

matthewbauer · 2018-05-11T08:57:31Z

pkgs/os-specific/linux/iproute/default.nix

  buildFlags = [
-    "CONFDIR=/etc/iproute2"
+    "CONFDIR=/run/iproute2"


You can share this with the module by using something like config.iproute2.confdir or "/etc/iproute2"

I was looking for a way to unify the setting too but not sure how to do it. I just tried sthg but it fails. Any suggestion ? Thanks for your patience

Yes I think you would do this like:

"CONFDIR=${config.iproute2.confdir or "/run/iproute2"}"

& then add config as an arg.

Also add to your module implementation:

nixpkgs.config.iproute2.confdir = "/run/iproute2"

just making sure:

does it have to be a toplevel config.iproute2 or is the current config.networking.iproute2 ok ?

I already have confDir as an option. isn't that enough ?

teto · 2018-05-12T15:28:10Z

@matthewbauer is that conform to what you requested, if yes feel free to squash & merge.
many packets depend on iproute so I am interested in having this small change upstream.

matthewbauer · 2018-05-12T15:36:39Z

pkgs/os-specific/linux/iproute/default.nix

  buildFlags = [
-    "CONFDIR=/etc/iproute2"
+    "CONFDIR=${config.networking.iproute2.confDir or "/run/iproute2"}"


Can you verify if this works? That is can you change confDir and get a different value passed to buildFlags? I'm not 100% on this but I think you need to directly pass it with nixpkgs.config in your module (see https://github.com/NixOS/nixpkgs/blob/release-18.03/nixos/modules/misc/nixpkgs.nix#L92-L106). Nixpkgs & NixOS have different config sets which can be confusing.

IMHO this requires a change log entry.

I am also not so sure if changing the default to /run is a good decision. If someone needs this the configuration will be set to whatever the user wants and the default behavior should not be altered if not used.

Am I missing something?

I agree this can be cool to have as a changelogentry. How would I do that ? other PR ? what document ?
I don't see a usecase either to let the user set the path (he would lose the cache) so I am tempted to remove the confDir = mkOption { but the paths between the module and the package must be in sync so I would let the nixpkgs.config.iproute2.confDir. If the user can't choose confDir, then we need to agree on the default. /etc/iproute2 is the common one but I guess /run/iproute2 matches dynamic files better. I am fine either way.

Ok yeah just using /run/iproute2 makes sense.

do you want me to hardcode it or current Pr is fine ?

It's fine now

many thanks for shepharding.

teto · 2018-05-12T17:41:17Z

I tested it and the changes took effect. I squashed all.

When doing source routing/multihoming, it's practical to give names to routing tables. The absence of the rt_table file in /etc make this impossible. This patch recreates these files on rebuild so that they can be modified by the user see NixOS#38638. iproute2 is modified to look into config.networking.iproute2.confDir instead of /etc/iproute2.

teto · 2018-06-08T03:36:56Z

That sounds better, I don't think we need to have a configurable path. But I think this should be done by nixos so the module may still make sense. While I am just interested in rt_tables the other files in etc/iproute2/ should be copied too.
Is there a way to do this

environment.etc."iproute2/rt_tables" = {
  text = (lib.fileContents "${pkgs.iproute}/etc/iproute2/rt_tables") + '' any extras '';
  mode = "0644";
};

but for the whole ${pkgs.iproute}/etc/iproute2 folder ?

andir · 2018-06-08T06:11:29Z

You could iterate through that dir using builtins.readDir "${pks.iproute}/etc/iproute2/". That will give you an attrset where the key/name is the filename and the value is the type of the dir.

…

On Fri, 8 Jun 2018, 05:37 Matthieu Coudron, ***@***.***> wrote: That sounds better, I don't think we need to have a configurable path. But I think this should be done by nixos so the module may still make sense. While I am just interested in rt_tables the other files in etc/iproute2/ should be copied too. Is there a way to do this environment.etc."iproute2/rt_tables" = { text = (lib.fileContents "${pkgs.iproute}/etc/iproute2/rt_tables") + '' any extras ''; mode = "0644"; }; but for the whole ${pkgs.iproute}/etc/iproute2 folder ? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#39536 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAm_dCncuDhWMizQbIZG8VfK-X940oNLks5t6fFlgaJpZM4Tk6yt> .

teto · 2018-06-10T14:57:40Z

doesn't seem simpler than the current cp -R.
To sum up the requested change would be to default back to /etc/iproute2 (and even remove the configurability). @volth feel free to open a PR, else I can do it.

teto · 2018-06-10T15:12:43Z

What is that multihoming module ? I see your point in having the file generated.

As for myself I use networkmanage hooks to achieve dynamic multihoming #39142 so I would need mode = "0644";

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Apr 26, 2018

Mic92 reviewed Apr 27, 2018

View reviewed changes

teto force-pushed the iproute branch 2 times, most recently from e9371da to 18cfb6a Compare May 11, 2018 08:46

matthewbauer reviewed May 11, 2018

View reviewed changes

teto force-pushed the iproute branch from 18cfb6a to 87b0cc0 Compare May 11, 2018 09:05

GrahamcOfBorg added 10.rebuild-darwin: 1-10 10.rebuild-linux: 501+ and removed 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels May 11, 2018

matthewbauer reviewed May 12, 2018

View reviewed changes

teto force-pushed the iproute branch from 5002559 to 45637ee Compare May 12, 2018 17:16

teto changed the title ~~iproute: copy files in /etc~~ [RDY] iproute: copy files in /etc May 14, 2018

teto added 2 commits May 15, 2018 21:55

release notes: mention iproute2 module

b75a959

teto force-pushed the iproute branch from 45637ee to b75a959 Compare May 15, 2018 12:55

GrahamcOfBorg added 8.has: changelog 8.has: documentation labels May 15, 2018

matthewbauer merged commit a3e4340 into NixOS:master May 15, 2018

teto deleted the iproute branch May 15, 2018 16:58

teto mentioned this pull request May 22, 2018

absence of /etc/iproute2/rt_tables #38638

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RDY] iproute: copy files in /etc #39536

[RDY] iproute: copy files in /etc #39536

teto commented Apr 26, 2018

GrahamcOfBorg commented Apr 26, 2018

GrahamcOfBorg commented Apr 26, 2018

Mic92 Apr 27, 2018

andir Apr 28, 2018

Mic92 Apr 29, 2018

teto May 1, 2018

teto commented Apr 28, 2018 via email

matthewbauer commented May 9, 2018

teto commented May 9, 2018

matthewbauer commented May 9, 2018 •

edited

teto commented May 9, 2018

matthewbauer commented May 9, 2018

teto commented May 11, 2018

matthewbauer May 11, 2018

teto May 11, 2018

matthewbauer May 11, 2018

teto May 11, 2018 •

edited

teto commented May 12, 2018

matthewbauer May 12, 2018

andir May 15, 2018

teto May 15, 2018

matthewbauer May 15, 2018

teto May 15, 2018

matthewbauer May 15, 2018

teto May 15, 2018

teto commented May 12, 2018

teto commented Jun 8, 2018

andir commented Jun 8, 2018 via email

teto commented Jun 10, 2018

teto commented Jun 10, 2018

[RDY] iproute: copy files in /etc #39536

[RDY] iproute: copy files in /etc #39536

Conversation

teto commented Apr 26, 2018

Motivation for this change

Things done

GrahamcOfBorg commented Apr 26, 2018

GrahamcOfBorg commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teto commented Apr 28, 2018 via email

matthewbauer commented May 9, 2018

teto commented May 9, 2018

matthewbauer commented May 9, 2018 • edited

teto commented May 9, 2018

matthewbauer commented May 9, 2018

teto commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teto May 11, 2018 • edited

Choose a reason for hiding this comment

teto commented May 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teto commented May 12, 2018

teto commented Jun 8, 2018

andir commented Jun 8, 2018 via email

teto commented Jun 10, 2018

teto commented Jun 10, 2018

matthewbauer commented May 9, 2018 •

edited

teto May 11, 2018 •

edited