Miniupnpd and bittorrent improvements #46443

bobvanderlinden · 2018-09-09T20:34:28Z

Motivation for this change

The bittorrent tests was sometimes failing. It seemed to be related to upnp. There was no test yet that checked whether miniupnpd worked correctly. Lastly the miniupnpd service did not setup iptables correctly.

This PR fixes the miniupnpd iptables setup by using the related scripts from Miniupnpd.

It adds a test which checks whether the Miniupnpd service allows port mapping in a client <> router <> client structure.

It fixes and simplifies the bittorrent test by using the miniupnpd service, the transmission service and by reusing the network structure used in thhe miniupnpd test.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option 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/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

bobvanderlinden · 2018-09-09T20:37:37Z

@jgillich You seem to have more experience with miniupnpd than I do. What I'm running into is the following:

client1: must succeed: upnpc -l
router# [   18.270599] miniupnpd[578]: chain MINIUPNPD not found

miniupnpd does not seem to be able to find the iptables chain MINIUPNPD as defined here:

nixpkgs/nixos/modules/services/networking/miniupnpd.nix

Lines 61 to 70 in 0f1de2e

    
               networking.firewall.extraCommands = '' 
        
                 iptables -t nat -N MINIUPNPD 
        
                 iptables -t nat -A PREROUTING -i ${cfg.externalInterface} -j MINIUPNPD 
        
                 iptables -t mangle -N MINIUPNPD 
        
                 iptables -t mangle -A PREROUTING -i ${cfg.externalInterface} -j MINIUPNPD 
        
                 iptables -t filter -N MINIUPNPD 
        
                 iptables -t filter -A FORWARD -i ${cfg.externalInterface} ! -o ${cfg.externalInterface} -j MINIUPNPD 
        
                 iptables -t nat -N MINIUPNPD-PCP-PEER 
        
                 iptables -t nat -A POSTROUTING -o ${cfg.externalInterface} -j MINIUPNPD-PCP-PEER 
        
               '';

Do you have any idea what could be the problem here?

jgillich · 2018-09-09T20:58:01Z

Not sure, but I've also never gotten it to work right and found it impossible to debug, so I just gave up. It probably should have been marked as broken.

bobvanderlinden · 2018-09-13T18:38:09Z

This is related to getting all tests passing for 18.09: #45960

bobvanderlinden · 2018-09-13T21:03:35Z

@miniupnp Can you shed some light on what miniupnpcd does? And maybe good ways to debug problems? I'm trying to write a test using a few vms to make sure the core functionality of miniupnpcd always works in NixOS.

The chain MINIUPNPCD is created in iptables.

The following also succeeds:

upnpc -a ${...} 9000 9000 TCP

But then I don't see the any output from:

upnpc -l

I don't see much happening in miniupnpcd and I don't see iptables changing using:

iptables --list-rules

I have very little understanding of iptables, but I guessed that would change by miniupnpd.

Do you have some suggestions on ways to debug this problem?

miniupnp · 2018-09-14T09:30:21Z

what are the output log of miniupnpd while you run theupnpc -a [...]command ?
you can run miniupnpd with -d option to get more detailed output

bobvanderlinden · 2018-09-15T11:36:47Z

@miniupnp Thank you for pushing me in the right direction. I wasn't aware of the -d option and found out the problem (amongst others) was that it couldn't determine its external IP address (which is likely because it's inside of a sandbox environment). I set the external address in the configuration and now the upnp test is working correctly!

Next up is the bittorrent test, where I was initially working on ;).

bobvanderlinden · 2018-09-15T13:02:09Z

@jgillich the miniupnpd service should now be working correctly. It now uses the shell scripts that Miniupnpd supplies.

@miniupnp during the tests it occurred to me that upnpc -a ... exits with exit code 0 when creating a port mapping has failed. Is this intentional? Should I create an issue for this?

@samueldr If/when this gets merged, this probably also needs backporting to 18.09.

miniupnp · 2018-09-15T20:51:01Z

@bobvanderlinden how did he fail ? Have you the console output ?

jgillich · 2018-09-15T20:55:38Z

nixos/modules/services/networking/miniupnpd.nix

@@ -59,30 +59,12 @@ in
  config = mkIf cfg.enable {
    # from miniupnpd/netfilter/iptables_init.sh


You can probably remove this comment (and the other one below).

Great work btw! 👍

This attempts to improve stability of the test by using existing services for miniupnpd and transmission. It also uses explicit addresses for the network interfaces so that the external IP addresses are valid internet addresses (thus fixing validation problems from upnpc). Also disable eth0 from being used to transfer torrents over without that being the intention.

bobvanderlinden · 2018-09-15T22:10:48Z

@miniupnp I'm terribly sorry, but I can't reproduce the errors anymore (reflog is gone and I recreated the commits to adhere to nixpkgs standards :().

The biggest confusion was the exit code always being 0. Initially I didn't realize the test framework did not output any stdout/err of the upnpc comand when the exit code is 0, because succeeding commands do not ouput anything in this framework. So it was not showing up output by default. Based on your comment I added -d and realized there still was no output, so I made some changes in the test whatever the exit code is. Errors like not adding required parameters also resulted in exit code 0.

While figuring the above out I ended up using -d as the first argument. When I figured out how to get output I got a NoSuchEntryInArray error. It took me a while to realize -d is also a command when using it as the first argument (oops).

I also struggled a bit with the ext_ip and the interfaces having internal addresses by default. By chance I picked 240.x.x.x for the internet addresses, which apparently was also a reserved range (I was not aware of this). Looking at the code of miniupnpd I found a non-reserved range (80.x.x.x).

@jgillich I removed the comments in the new commit set 👍 thanks for noticing.

bobvanderlinden · 2018-09-22T19:52:28Z

@samueldr Could you trigger nixos.tests.upnp and nixos.tests.bittorrent on @grahamc?

samueldr · 2018-09-23T03:09:46Z

@GrahamcOfBorg test upnp bittorrent

GrahamcOfBorg · 2018-09-23T03:14:09Z

Success on x86_64-linux (full log)

Attempted: tests.upnp, tests.bittorrent

Partial log (click to expand)

killing client2 (pid 598)
killing router (pid 610)
killing tracker (pid 622)
killing client1 (pid 634)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde2.ctl': Directory not empty
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/4x0dnz9r0a5mpadga0q0zkaa0811n70l-vm-test-run-upnp
/nix/store/l7bil5j2f8vkcp9jq8zldcpjprim9yy9-vm-test-run-bittorrent

GrahamcOfBorg · 2018-09-23T03:20:46Z

Success on aarch64-linux (full log)

Attempted: tests.upnp, tests.bittorrent

Partial log (click to expand)

killing client1 (pid 632)
killing client2 (pid 644)
killing router (pid 657)
killing tracker (pid 671)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not emptyCould not remove ctl dir '/build/vde2.ctl': Directory not empty

/nix/store/w69zsfyxms8caag9qfpkx2yzd3g5aihp-vm-test-run-upnp
/nix/store/7cyikibrncln0nyni9s0p3xwicsf7y2v-vm-test-run-bittorrent

bobvanderlinden · 2018-09-23T09:04:06Z

🎉 I think this one is ready to merge 👍

[backport] Miniupnpd and bittorrent improvements (#46443)

GrahamcOfBorg added 6.topic: nixos 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Sep 9, 2018

GrahamcOfBorg added 8.has: module (update) 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 labels Sep 11, 2018

bobvanderlinden changed the title ~~[WIP] add test for upnp using miniupnpd and miniupnpc~~ Miniupnpd and bittorrent fixes Sep 15, 2018

miniupnpd: wrap iptables scripts to use correct PATH

2039327

bobvanderlinden force-pushed the pr-test-upnp branch from b4d375a to 6df2d01 Compare September 15, 2018 13:16

bobvanderlinden changed the title ~~Miniupnpd and bittorrent fixes~~ Miniupnpd and bittorrent improvements Sep 15, 2018

jgillich reviewed Sep 15, 2018

View reviewed changes

bobvanderlinden added 4 commits September 15, 2018 23:10

nixos: miniupnpd: use iptables scripts

d3eff01

tests: upnp: init test for upnp using miniupnpd / miniupnpc

32c63c6

tests: bittorrent: add bobvanderlinden as maintainer

5fbc521

bobvanderlinden force-pushed the pr-test-upnp branch from 6df2d01 to 5fbc521 Compare September 15, 2018 21:10

matthewbauer merged commit 33d2404 into NixOS:master Oct 6, 2018

samueldr mentioned this pull request Oct 6, 2018

[backport] Miniupnpd and bittorrent improvements (#46443) #47956

Merged

samueldr added a commit that referenced this pull request Oct 6, 2018

Merge pull request #47956 from samueldr/18.09/backport-46443

f4bb9c4

[backport] Miniupnpd and bittorrent improvements (#46443)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miniupnpd and bittorrent improvements #46443

Miniupnpd and bittorrent improvements #46443

bobvanderlinden commented Sep 9, 2018 •

edited

bobvanderlinden commented Sep 9, 2018

jgillich commented Sep 9, 2018

bobvanderlinden commented Sep 13, 2018

bobvanderlinden commented Sep 13, 2018 •

edited

miniupnp commented Sep 14, 2018

bobvanderlinden commented Sep 15, 2018

bobvanderlinden commented Sep 15, 2018

miniupnp commented Sep 15, 2018

jgillich Sep 15, 2018

bobvanderlinden commented Sep 15, 2018

bobvanderlinden commented Sep 22, 2018

samueldr commented Sep 23, 2018 •

edited

GrahamcOfBorg commented Sep 23, 2018

GrahamcOfBorg commented Sep 23, 2018

bobvanderlinden commented Sep 23, 2018

		@@ -59,30 +59,12 @@ in
		config = mkIf cfg.enable {
		# from miniupnpd/netfilter/iptables_init.sh

Miniupnpd and bittorrent improvements #46443

Miniupnpd and bittorrent improvements #46443

Conversation

bobvanderlinden commented Sep 9, 2018 • edited

Motivation for this change

Things done

bobvanderlinden commented Sep 9, 2018

jgillich commented Sep 9, 2018

bobvanderlinden commented Sep 13, 2018

bobvanderlinden commented Sep 13, 2018 • edited

miniupnp commented Sep 14, 2018

bobvanderlinden commented Sep 15, 2018

bobvanderlinden commented Sep 15, 2018

miniupnp commented Sep 15, 2018

jgillich Sep 15, 2018

Choose a reason for hiding this comment

bobvanderlinden commented Sep 15, 2018

bobvanderlinden commented Sep 22, 2018

samueldr commented Sep 23, 2018 • edited

GrahamcOfBorg commented Sep 23, 2018

GrahamcOfBorg commented Sep 23, 2018

bobvanderlinden commented Sep 23, 2018

bobvanderlinden commented Sep 9, 2018 •

edited

bobvanderlinden commented Sep 13, 2018 •

edited

samueldr commented Sep 23, 2018 •

edited