nixos-container: support bridge and port forwarding for imperative nixos-container #20869

wavewave · 2016-12-02T21:28:38Z

Motivation for this change

systemd-nspawn supports network bridge and port forwarding via --network-bridge and --port option. Declarative container already supports network bridge but does not support port yet. Imperative container does not support both.

This PR introduces two options --bridge and --port to nixos-container commands and implements corresponding --port option in nixos/modules/virtualisation/containers.nix.

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
- Linux
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.

…rk ports to systemd-nspawn container)

mention-bot · 2016-12-02T21:28:41Z

@wavewave, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ericbmerritt, @edolstra and @kampfschlaefer to be potential reviewers.

wavewave · 2016-12-03T00:21:13Z

I made a corresponding change for nixops in the this branch.

aszlig

Have you run the containers-* tests?

aszlig · 2016-12-03T05:02:10Z

nixos/modules/virtualisation/containers.nix

@@ -318,6 +319,16 @@ let
      '';
    };

+    hostPort = mkOption {
+      type = types.nullOr types.string;


First of all, types.string is deprecated. And second, this seems to be accepting only an integer, am I correct? If yes, why not use types.int instead?

--port of systemd-nspawn can take a more general form such astcp:443:80 (see the following excerpt from systemd-nspawn man page) so for the time being, it's good to be a string than an integer. As for types.string, what is the most modern way now?

-p, --port= If private networking is enabled, maps an IP port on the host onto an IP port on the container. Takes a protocol specifier (either "tcp" or "udp"), separated by a colon from a host port number in the range 1 to 65535, separated by a colon from a container port number in the range from 1 to 65535. The protocol specifier and its separating colon may be omitted, in which case "tcp" is assumed. The container port number and its colon may be omitted, in which case the same port as the host port is implied. This option is only supported if private networking is used, such as with --network-veth, --network-zone= --network-bridge=.

Okay, then I think the example and description attributes are clearly misleading, because Allow port forwarding... sounds like if it is a boolean option. I'd flesh this out as a submodule to avoid future need of deprecation once we want to remove the stringly typed one.

Also, doesn't this make more sense to be a list? What if you want to forward several ports (which I think is not uncommon)?

First, now I found that types.str is the correct way, thanks for pointing this out.
The description is misleading. I will change it.
I have to check whether systemd-nspawn can take multiple --port parameters. If so, it's much desirable to be a list. It would be better to be a list of key/values where key = protocol,hostPort,containerPort. Thanks for suggestion.

I'd make it a submodule, like this (with missing example/description attrs):

{ type = types.listOf (types.submodule { options = { protocol = mkOption { type = types.str; default = "tcp"; }; hostPort = mkOption { type = types.int; }; containerPort = mkOption { type = types.nullOr types.int; default = null; }; }; }); }

kampfschlaefer

No test -> no security this feature doesn't get broken next week. (And also no prove your feature is working.)

Please not only execute the existing tests to check that you didn't break anything, but also add tests for your new cool features.

…tPort from 'string' to 'listOf str'

…t of (protocol,hostPort,containerPort).

wavewave · 2016-12-04T05:08:18Z

@aszlig As you suggested, I change the configuration set to a list of submodule of (protocol,hostPort,containerPort). Now I changed the config name from hostPort to forwardPorts, which is well matched with the option with the same name in services/networking/nat.nix module.

I've tested this with my local machine:
With the following declarative container definition:

{
  containers = {
    foo = {
      privateNetwork = true;
      hostAddress = "192.168.100.10";
      localAddress = "192.168.100.11";
      forwardPorts = [ { hostPort = 10011; containerPort = 30011;}
                       { protocol = "tcp"; hostPort = 10012; }
                       { protocol = "udp"; hostPort = 10013; containerPort = 50013; }
                     ];
      autoStart = true;

      config = { config, pkgs, nodes, ... }: {
        networking.firewall.enable = false;
        environment.systemPackages = [ pkgs.traceroute pkgs.telnet ];
      };
    };
    bar = {
      privateNetwork = true;
      hostAddress = "192.168.200.10";
      localAddress = "192.168.200.11";
      forwardPorts = [ { hostPort = 20011; containerPort = 40011; }
                       { hostPort = 20012; containerPort = 40012; } ];
      autoStart = true;

      config = { config, pkgs, nodes, ... }: {
        networking.firewall.enable = false;
        environment.systemPackages = [ pkgs.traceroute pkgs.telnet ];
      };
    };

  };
}

I obtain the following port forwarding automatically done with systemd-nspawn.

$ sudo iptables -vnL -t nat 
Chain PREROUTING (policy ACCEPT 4 packets, 1327 bytes)
 pkts bytes target     prot opt in     out     source               destination         
    0     0 DNAT       tcp  --  *      *       0.0.0.0/0            0.0.0.0/0            tcp dpt:10011 ADDRTYPE match dst-type LOCAL to:192.168.100.11:30011
    0     0 DNAT       tcp  --  *      *       0.0.0.0/0            0.0.0.0/0            tcp dpt:10012 ADDRTYPE match dst-type LOCAL to:192.168.100.11:10012
    0     0 DNAT       udp  --  *      *       0.0.0.0/0            0.0.0.0/0            udp dpt:10013 ADDRTYPE match dst-type LOCAL to:192.168.100.11:50013
    0     0 DNAT       tcp  --  *      *       0.0.0.0/0            0.0.0.0/0            tcp dpt:20011 ADDRTYPE match dst-type LOCAL to:192.168.200.11:40011
    0     0 DNAT       tcp  --  *      *       0.0.0.0/0            0.0.0.0/0            tcp dpt:20012 ADDRTYPE match dst-type LOCAL to:192.168.200.11:40012
 3199  894K nixos-nat-pre  all  --  *      *       0.0.0.0/0            0.0.0.0/0           

Chain INPUT (policy ACCEPT 4 packets, 1327 bytes)
 pkts bytes target     prot opt in     out     source               destination         

Chain OUTPUT (policy ACCEPT 127 packets, 11825 bytes)
 pkts bytes target     prot opt in     out     source               destination         
    0     0 DNAT       tcp  --  *      *       0.0.0.0/0           !127.0.0.0/8          tcp dpt:10011 ADDRTYPE match dst-type LOCAL to:192.168.100.11:30011
    0     0 DNAT       tcp  --  *      *       0.0.0.0/0           !127.0.0.0/8          tcp dpt:10012 ADDRTYPE match dst-type LOCAL to:192.168.100.11:10012
    0     0 DNAT       udp  --  *      *       0.0.0.0/0           !127.0.0.0/8          udp dpt:10013 ADDRTYPE match dst-type LOCAL to:192.168.100.11:50013
    0     0 DNAT       tcp  --  *      *       0.0.0.0/0           !127.0.0.0/8          tcp dpt:20011 ADDRTYPE match dst-type LOCAL to:192.168.200.11:40011
    0     0 DNAT       tcp  --  *      *       0.0.0.0/0           !127.0.0.0/8          tcp dpt:20012 ADDRTYPE match dst-type LOCAL to:192.168.200.11:40012
...

(I am showing only relevant parts)

For imperative container, to give multiple port forwarding options, one should give port forwarding option encoded as systemd-nspawn convention separated by commas. For example,

nixos-container create foo --port 10000,10001,tcp:10002:20002

I am going to run other pre-existing tests and make a test dedicated to this new feature.

wavewave · 2016-12-06T04:41:27Z

@aszlig I've checked that all containers-*.nix test passed!
@kampfschlaefer I will make a test for this new feature,

aszlig · 2016-12-17T16:08:34Z

nixos/modules/virtualisation/containers.nix

      description = ''
-        Allow port forwarding from the host to the container. 
+        List of forwarded ports from host to container. Each forwarded port is specified by protocol, hostPort and containerPort. By default, protocol is tcp and hostPort and containerPort are assumed to be the same if containerPort is not explicitly given. 


Nitpick: I'd break this long line as it's a bit tedious to read, especially on larger resolutions.

@aszlig I broke the long line now. :-)

…ers-portforward.nix )

wavewave · 2017-02-03T20:54:07Z

@kampfschlaefer I've created a port forwarding test (nixos/tests/containers-portforward.nix).

kampfschlaefer

I like the test.

(Although there is still the need for the "sleep 2" to wait on the networking of the host side…)

basvandijk · 2017-11-20T15:58:11Z

I made a corresponding change for nixops in the this branch.

@wavewave would you mind turning your nixops commits in a PR so that we can get bridge support in nixops? I would also be happy to create a PR based on your commits if you don't have time.

basvandijk · 2017-11-21T00:07:31Z

Also see: NixOS/nixops#744 (comment)

wavewave added 2 commits November 27, 2016 02:37

nixos-container: support bridge.

21d14f4

nixos-container: add --port option for nixos-container (forward netwo…

3c27525

…rk ports to systemd-nspawn container)

nixos-container: introduce hostPort in declarative container options.

174bfec

wavewave mentioned this pull request Dec 3, 2016

Bidirectional container access #16754

Open

aszlig requested changes Dec 3, 2016

View reviewed changes

kampfschlaefer suggested changes Dec 3, 2016

View reviewed changes

fpletz approved these changes Dec 3, 2016

View reviewed changes

wavewave added 2 commits December 3, 2016 20:18

nixos-container: support multiple port forwarding. change type of hos…

989a93c

…tPort from 'string' to 'listOf str'

nixos-container: hostPort -> forwardPort and forwardPort is now a lis…

fe88da1

…t of (protocol,hostPort,containerPort).

aszlig approved these changes Dec 17, 2016

View reviewed changes

wavewave added 2 commits December 17, 2016 17:48

nixos-container: break lines in description of forwardPorts.

ae00ed3

nixos-container: added test for port forwarding ( nixos/tests/contain…

a14afc5

…ers-portforward.nix )

kampfschlaefer approved these changes Feb 11, 2017

View reviewed changes

globin merged commit b7a24e0 into NixOS:master Feb 15, 2017

wavewave deleted the nixos-container branch March 25, 2017 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos-container: support bridge and port forwarding for imperative nixos-container #20869

nixos-container: support bridge and port forwarding for imperative nixos-container #20869

wavewave commented Dec 2, 2016

mention-bot commented Dec 2, 2016

wavewave commented Dec 3, 2016

aszlig left a comment

aszlig Dec 3, 2016

wavewave Dec 3, 2016

aszlig Dec 3, 2016

wavewave Dec 3, 2016

aszlig Dec 3, 2016

kampfschlaefer left a comment

wavewave commented Dec 4, 2016

wavewave commented Dec 6, 2016

aszlig Dec 17, 2016

wavewave Dec 18, 2016

wavewave commented Feb 3, 2017

kampfschlaefer left a comment

basvandijk commented Nov 20, 2017

basvandijk commented Nov 21, 2017

nixos-container: support bridge and port forwarding for imperative nixos-container #20869

nixos-container: support bridge and port forwarding for imperative nixos-container #20869

Conversation

wavewave commented Dec 2, 2016

Motivation for this change

Things done

mention-bot commented Dec 2, 2016

wavewave commented Dec 3, 2016

aszlig left a comment

Choose a reason for hiding this comment

aszlig Dec 3, 2016

Choose a reason for hiding this comment

wavewave Dec 3, 2016

Choose a reason for hiding this comment

aszlig Dec 3, 2016

Choose a reason for hiding this comment

wavewave Dec 3, 2016

Choose a reason for hiding this comment

aszlig Dec 3, 2016

Choose a reason for hiding this comment

kampfschlaefer left a comment

Choose a reason for hiding this comment

wavewave commented Dec 4, 2016

wavewave commented Dec 6, 2016

aszlig Dec 17, 2016

Choose a reason for hiding this comment

wavewave Dec 18, 2016

Choose a reason for hiding this comment

wavewave commented Feb 3, 2017

kampfschlaefer left a comment

Choose a reason for hiding this comment

basvandijk commented Nov 20, 2017

basvandijk commented Nov 21, 2017