tasks/network-interfaces.nix: Enable ip_forwarding for ipv4 and p… #62671

kfiz · 2019-06-04T15:53:41Z

…roxy_ndp for ipv6 for interfaces that set proxy_ARP

Motivation for this change

Fixes #62339

Things done

nixos-discourse · 2019-09-01T02:13:56Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-in-distress/3604/4

brainrake · 2020-05-14T19:05:08Z

nixos/modules/tasks/network-interfaces.nix

      "net.ipv6.conf.all.disable_ipv6" = mkDefault (!cfg.enableIPv6);
      "net.ipv6.conf.default.disable_ipv6" = mkDefault (!cfg.enableIPv6);
      "net.ipv6.conf.all.forwarding" = mkDefault (any (i: i.proxyARP) interfaces);
    } // listToAttrs (flip concatMap (filter (i: i.proxyARP) interfaces)
-        (i: flip map [ "4" "6" ] (v: nameValuePair "net.ipv${v}.conf.${i.name}.proxy_arp" true)))
+        (i: flip map [ "4" "6" ] (v: if v == "4" 


the if can be all the way inside, like ${if v == "4" then "arp" else "ndp"}

Thanks for the comment. I simplified this.

brainrake · 2020-05-14T19:10:08Z

I see this problem is still in master.

brainrake · 2020-05-14T19:16:07Z

Please fix, rebase and LGTM, seems like an easy win.

kfiz · 2020-05-16T04:34:53Z

Done.

brainrake · 2020-05-23T14:29:08Z

Hey, I meant

  } // listToAttrs (flip concatMap (filter (i: i.proxyARP) interfaces)
        (i: forEach [ "4" "6" ] (v: nameValuePair "net.ipv${v}.conf.${replaceChars ["."] ["/"] i.name}.proxy_${if i == "4" then "arp" else "ndp"}" true)))

but maybe what you wrote is more readable.

I tested the PR, works fine, sets proxy_arp and proxy_ndp sysctls to 1.

@flokli could you take a look? Thank you!

flokli · 2020-05-23T14:55:50Z

@kfiz Please make this one single commit, and format the commit message properly. Also, the commit is missing a description on the why, as per CONTRIBUTING.md:

In addition to writing properly formatted commit messages, it's important to include relevant information so other developers can later understand why a change was made. While this information usually can be found by digging code, mailing list/Discourse archives, pull request discussions or upstream changes, it may require a lot of work.

@brainrape I think the variant currently present in this PR is more readable.

kfiz · 2020-05-24T07:15:05Z

@flokli Done.

flokli

Also note the commit author seems to be different.

flokli · 2020-05-24T12:32:21Z

nixos/modules/tasks/network-interfaces.nix

      "net.ipv6.conf.all.disable_ipv6" = mkDefault (!cfg.enableIPv6);
      "net.ipv6.conf.default.disable_ipv6" = mkDefault (!cfg.enableIPv6);
      "net.ipv6.conf.all.forwarding" = mkDefault (any (i: i.proxyARP) interfaces);
    } // listToAttrs (flip concatMap (filter (i: i.proxyARP) interfaces)
-        (i: forEach [ "4" "6" ] (v: nameValuePair "net.ipv${v}.conf.${replaceChars ["."] ["/"] i.name}.proxy_arp" true)))
+        (i: [(nameValuePair "net.ipv4.conf.${replaceChars ["."] ["/"] i.name}.proxy_arp" true)
+             (nameValuePair "net.ipv6.conf.${replaceChars ["."] ["/"] i.name}.proxy_ndp" true)]))


It seems GitHub ate my comment.

This reads like we always set a wrong proxy_ndp sysctl (so it never worked), and even if we set it now, the kernel-provided ndp proxy infrastructure requires addition of each individual address to ndp proxy for (see https://github.com/DanielAdolfsson/ndppd/blob/master/README#L57)

It seems more useful to just remove the NDP-specific bits from the proxyARP documentation, and let users either use ndppd (which is packaged in NixOS), or set options themselves.

Luckily, as it never worked before, we don't break any existing configurations.

Hmm...you are right. Setting proxy_ndp alone is not enough to make this work.

The question for me here would be whether a user who sets this knows that she has to additionally set something like ip -6 neigh add proxy <ip> dev <if> or not. If she is aware of this then this PR is aligned with her expectations, if not than obviously not.

In any way, another way to go apart from your suggestion would be to generate a warning message that tells users that additional settings have to be specified for this to work and alternatively point to ndppd.

proxyARP shouldn't enable proxy NDP - these are two different concepts, and we shouldn't mix them. Given it never worked, I don't see a problem with removing the (disfunctional and dangerous) proxy NDP part.

Agreed. We shouldn't mix the two. It's better to force users to make this decision consciously.

Can you push a new commit changing this?

Changed this.

flokli

The description for networking.interfaces.<name?>.proxyARP needs to be updated - it still refers to the NDP bits.

Also, please update the commit message to something like this:

tasks/networking-interfaces.nix: remove broken NDP bits from proxyARP option

The `networking.interfaces.<name?>.proxyARP` previously mentioned it would also enable IPv6 forwarding and `proxy_ndp`.

However, the `proxy_ndp` option was never actually set (as it did set the non-existing `net.ipv6.conf.proxy_arp` sysctl instead), and `proxy_ndp` also needs individual entries
for each ip to proxy for.

Proxy ARP and Proxy NDP are two different concepts, and enabling the latter
should be a conscious decision.

This commit removes the broken NDP support, and disables explicitly
enabling IPv6 forwarding (which is the default in most cases anyways)

Fixes #62339.

flokli · 2020-05-25T18:57:36Z

nixos/modules/tasks/network-interfaces.nix

@@ -1055,11 +1055,12 @@ in
      optionalString hasBonds "options bonding max_bonds=0";

    boot.kernel.sysctl = {
+      "net.ipv4.conf.all.forwarding" = mkDefault (any (i: i.proxyARP) interfaces);
      "net.ipv6.conf.all.disable_ipv6" = mkDefault (!cfg.enableIPv6);
      "net.ipv6.conf.default.disable_ipv6" = mkDefault (!cfg.enableIPv6);
      "net.ipv6.conf.all.forwarding" = mkDefault (any (i: i.proxyARP) interfaces);


Sorry, I missed this line. With all the NDP stuff gone, we also shouldn't enable ipv6 forwarding on proxyARP anymore.

This however now should get a release note entry now.

@flokli What do you think. Are we good to go?

The `networking.interfaces.<name?>.proxyARP` option previously mentioned it would also enable IPv6 forwarding and `proxy_ndp`. However, the `proxy_ndp` option was never actually set (the non-existing `net.ipv6.conf.proxy_arp` sysctl was set instead). In addition `proxy_ndp` also needs individual entries for each ip to proxy for. Proxy ARP and Proxy NDP are two different concepts, and enabling the latter should be a conscious decision. This commit removes the broken NDP support, and disables explicitly enabling IPv6 forwarding (which is the default in most cases anyways) Fixes NixOS#62339.

flokli · 2020-05-31T20:22:54Z

Thanks!

kfiz mentioned this pull request Jun 4, 2019

Networking proxy arp fix #62640

Closed

10 tasks

ofborg bot added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jun 4, 2019

kfiz changed the title ~~tasks/networking-interfaces.nix: Enable ip_forwarding for ipv4 and p…~~ tasks/network-interfaces.nix: Enable ip_forwarding for ipv4 and p… Jun 5, 2019

brainrake suggested changes May 14, 2020

View reviewed changes

kfiz force-pushed the networking-proxy_arp-fix branch from 47582e2 to 8f4aca3 Compare May 16, 2020 04:11

ofborg bot added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 labels May 16, 2020

kfiz force-pushed the networking-proxy_arp-fix branch from 8f4aca3 to 1ab4e58 Compare May 24, 2020 07:00

flokli requested changes May 24, 2020

View reviewed changes

flokli reviewed May 24, 2020

View reviewed changes

kfiz force-pushed the networking-proxy_arp-fix branch 2 times, most recently from b6e4d59 to 8eda489 Compare May 25, 2020 13:33

flokli requested changes May 25, 2020

View reviewed changes

kfiz force-pushed the networking-proxy_arp-fix branch from 8eda489 to 5d3a72f Compare May 25, 2020 23:09

flokli merged commit 4cd605f into NixOS:master May 31, 2020

kfiz deleted the networking-proxy_arp-fix branch May 31, 2020 20:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tasks/network-interfaces.nix: Enable ip_forwarding for ipv4 and p… #62671

tasks/network-interfaces.nix: Enable ip_forwarding for ipv4 and p… #62671

kfiz commented Jun 4, 2019 •

edited

nixos-discourse commented Sep 1, 2019

brainrake May 14, 2020

kfiz May 16, 2020

brainrake commented May 14, 2020

brainrake commented May 14, 2020

kfiz commented May 16, 2020

brainrake commented May 23, 2020

flokli commented May 23, 2020

kfiz commented May 24, 2020

flokli left a comment

flokli May 24, 2020

kfiz May 24, 2020

flokli May 24, 2020

kfiz May 25, 2020

flokli May 25, 2020

kfiz May 25, 2020

flokli left a comment

flokli May 25, 2020

kfiz May 29, 2020

flokli commented May 31, 2020

Navigation Menu

tasks/network-interfaces.nix: Enable ip_forwarding for ipv4 and p… #62671

tasks/network-interfaces.nix: Enable ip_forwarding for ipv4 and p… #62671

Conversation

kfiz commented Jun 4, 2019 • edited

Motivation for this change

Things done

nixos-discourse commented Sep 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brainrake commented May 14, 2020

brainrake commented May 14, 2020

kfiz commented May 16, 2020

brainrake commented May 23, 2020

flokli commented May 23, 2020

kfiz commented May 24, 2020

flokli left a 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

Choose a reason for hiding this comment

flokli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flokli commented May 31, 2020

kfiz commented Jun 4, 2019 •

edited