Setting `services.tailscale.useRoutingFeatures = "server"` usually doesn't work

alex-robbins commented 1 year ago

Describe the bug

Setting services.tailscale.useRoutingFeatures = "server" causes an error on systems where the proxyARP option is off for all interfaces, which is the default case. This is because the tailscale option is implemented as:

    boot.kernel.sysctl = mkIf (cfg.useRoutingFeatures == "server" || cfg.useRoutingFeatures == "both") {
      "net.ipv4.conf.all.forwarding" = mkDefault true;
      "net.ipv6.conf.all.forwarding" = mkDefault true;
    };

But network-interfaces.nix:1374 sets a conflicting default:

"net.ipv4.conf.all.forwarding" = mkDefault (any (i: i.proxyARP) interfaces);

The use of mkDefault for the tailscale option was requested by @RaitoBezarius in the original PR from @Enzime, but I'm not sure that this makes sense. First of all, if you want to enable "server routing features", then enabling IP forwarding is a requirement, not a default. If I set useRoutingFeatures = "server" and something else sets ip forwarding to false, that's when I'd want to see an error, rather than the issue being silently ignored. Secondly, I don't understand the original comment's claim that "it would prevent fine-grained net.ipv4.conf.$interface.forwarding = true". I don't know how the kernel handles the per-interface setting being different than the "all" setting, but it seems moot; we're talking here about defaults in the NixOS module system, not in sysctl.

So, I think the mkDefaults should be removed in the above code from tailscale.nix.

Steps To Reproduce

Set services.tailscale.useRoutingFeatures = "server" on a system where the proxyARP option is off for all interfaces (which is the default).

Receive this error when building the system closure:

error: The option `boot.kernel.sysctl."net.ipv4.conf.all.forwarding"' is defined multiple times.

   Definition values:
   - In `/nix/store/wh4nnz2ga9lpxl406ia1cnsi891xaa4x-source/nixos/modules/services/networking/tailscale.nix': true
   - In `/nix/store/wh4nnz2ga9lpxl406ia1cnsi891xaa4x-source/nixos/modules/tasks/network-interfaces.nix': false

Notify maintainers

I feel like it probably isn't necessary to page more maintainers right away? Let's start with the above; someone will know if we need more eyes.

alex-robbins commented 1 year ago

Also, while I'm here, and since this option is so new ... Why did we decide to implement this with a single option of string type? Wouldn't two bools make more sense (one for client features, and one for server)?

That way, you could set e.g. services.tailscale.useRoutingFeatures.server = true in one module and services.tailscale.useRoutingFeatures.client = true in another module, and everything would just work. As it is, setting the option to "server" in one module and "client" in another is an error.

RaitoBezarius commented 1 year ago

The use of mkDefault for the tailscale option was requested by @RaitoBezarius https://github.com/NixOS/nixpkgs/pull/201119#discussion_r1037123185 from @Enzime, but I'm not sure that this makes sense. First of all, if you want to enable "server routing features", then enabling IP forwarding is a requirement, not a default. If I set useRoutingFeatures = "server" and something else sets ip forwarding to false, that's when I'd want to see an error, rather than the issue being silently ignored. Secondly, I don't understand the original comment's claim that "it would prevent fine-grained net.ipv4.conf.$interface.forwarding = true". I don't know how the kernel handles the per-interface setting being different than the "all" setting, but it seems moot; we're talking here about defaults in the NixOS module system, not in sysctl.

It's not mandatory to set all interfaces as forwarding, usually, the forwarding domain on a Tailscale machine is not all interfaces but some subset of those interfaces, therefore, it should be possible to override the all and set it up on each interface to have a well-defined forwarding domain.

Enabling IP forwarding is a requirement, indeed, but enabling IP forwarding on ALL interfaces is not a requirement.

A good condition should be a assert to check if there's at least one IP forwarding domain enabled.

So, I think the mkDefaults should be removed in the above code from tailscale.nix.

I am not certain this should be the case.

It is unfortunate another module introduces a conflicting definition, but it should be managed through priority rather than enforcing it, otherwise someone else (e.g. me for example) will open an issue with a use case where it does not work because of enforcement.

If @Enzime you have some time to submit a PR increasing the option priority, it would be great.

RaitoBezarius commented 1 year ago

Also, while I'm here, and since this option is so new ... Why did we decide to implement this with a single option of string type? Wouldn't two bools make more sense (one for client features, and one for server)?

That way, you could set e.g. services.tailscale.useRoutingFeatures.server = true in one module and services.tailscale.useRoutingFeatures.client = true in another module, and everything would just work. As it is, setting the option to "server" in one module and "client" in another is an error.

Good point, if you are interested into making the change, feel free to submit a PR!

NixOS / nixpkgs