Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hetzner: Set prefix length to /32 for main address #1070

Closed
wants to merge 1 commit into from

Conversation

@aszlig
Copy link
Member

@aszlig aszlig commented Jan 4, 2019

This was already suggested by @flokli in #1032 and it basically makes sure that all traffic is sent to the gateway instead of being sent directly.

The Hetzner Wiki describes configuration of Debian to include a peertopeer directive in /etc/network/interfaces, which roughly translates to the command ip address add ... peer A.B.C.D.

However, after testing with and without the peer keyword of ip address I didn't notice any difference in behavior in comparison to setting a plain /32 prefix length as we do now.

Apart from making configuration less complicated, this also gets rid of a bunch of code we now no longer need, eg. calculating subnet masks or getting the real prefix length.

Tested on a newly deployed PX61-NVMe.


@nh2, @flokli, @basvandijk: If possible, can you test this and check whether this causes any problems?

This was already suggested by @flokli in #1032 and it basically makes
sure that all traffic is sent to the gateway instead of being sent
directly.

The Hetzner Wiki at [1] describes configuration of Debian to include a
"peertopeer" directive in /etc/network/interfaces, which roughly
translates to the command "ip address add ... peer A.B.C.D".

However, after testing with and without the peer keyword of "ip address"
I didn't notice any difference in behaviour in comparison to setting a
plain /32 prefix length as we do now.

Apart from making configuration less complicated, this also gets rid of
a bunch of code we now no longer need, eg. calculating subnet masks or
getting the real prefix length.

Tested on a newly deployed PX61-NVMe.

[1]: https://wiki.hetzner.de/index.php/Netzkonfiguration_Debian/en#Dedicated_Servers

Signed-off-by: aszlig <aszlig@nix.build>
@flokli
Copy link
Contributor

@flokli flokli commented Jan 4, 2019

@aszlig how does the routing table now look like on a newly provisioned server?

@aszlig
Copy link
Member Author

@aszlig aszlig commented Jan 4, 2019

@flokli: With 1.2.3.4 being the gateway and 1.2.3.5 being the host address:

# ip r
default via 1.2.3.4 dev eth0 proto static 
1.2.3.4 dev eth0 proto static scope link
# ip a s eth0
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000
    link/ether aa:bb:cc:dd:ee:ff brd ff:ff:ff:ff:ff:ff
    inet 1.2.3.5/32 scope global eth0
       valid_lft forever preferred_lft forever
    inet6 1234::/64 scope global 
       valid_lft forever preferred_lft forever
    inet6 fe80::4321/64 scope link 
       valid_lft forever preferred_lft forever
@flokli
Copy link
Contributor

@flokli flokli commented Jan 4, 2019

So how is the 1.2.3.4 dev eth0 proto static scope link being created, if we don't specify it as peer?

Is that some magic somewhere deep in the NixOS networking scripts?

I'm asking because I use networkd, and had to configure a peer there too:

[Address]
Address=1.2.3.5/32
Peer=1.2.3.4/32
@aszlig
Copy link
Member Author

@aszlig aszlig commented Jan 4, 2019

@flokli:

Is that some magic somewhere deep in the NixOS networking scripts?

Nope, the address is simply added via ip addr add using the specified prefixLength unmodified.

I'm asking because I use networkd, and had to configure a peer there too:

[Address]
Address=1.2.3.5/32
Peer=1.2.3.4/32

Hm, I guess networkd isn't supported by this configuration, here is a small VM test:

import <nixpkgs/nixos/tests/make-test.nix> {
  name = "test-prefix32-networkd";

  nodes = let
    common = {
      networking.useDHCP = false;
      networking.firewall.enable = false;
    };
  in {
    node1 = { lib, ... }: {
      imports = [ common ];
      virtualisation.vlans = [ 1 ];
      networking.useNetworkd = true;
      networking.defaultGateway = {
        address = "1.2.0.1";
        interface = "eth1";
      };
      networking.interfaces = lib.mkOverride 0 {
        eth1.ipv4.addresses = lib.singleton {
          address = "1.2.3.4";
          prefixLength = 32;
        };
      };
    };

    node2 = { lib, ... }: {
      imports = [ common ];
      virtualisation.vlans = [ 2 ];
      networking.useNetworkd = true;
      networking.defaultGateway = {
        address = "1.2.0.2";
        interface = "eth1";
      };
      networking.interfaces = lib.mkOverride 0 {
        eth1.ipv4.addresses = lib.singleton {
          address = "1.2.3.5";
          prefixLength = 32;
        };
      };
    };

    router = { lib, ... }: {
      imports = [ common ];
      virtualisation.vlans = [ 1 2 ];
      boot.kernel.sysctl."net.ipv4.ip_forward" = true;
      networking.interfaces = lib.mkOverride 0 {
        eth1.ipv4.addresses = lib.singleton {
          address = "1.2.0.1";
          prefixLength = 16;
        };
        eth2.ipv4.addresses = lib.singleton {
          address = "1.2.0.2";
          prefixLength = 16;
        };
      };
      networking.localCommands = ''
        ip route add 1.2.0.0/16 dev eth1 src 1.2.0.1 table 10
        ip route add 1.2.0.0/16 dev eth2 src 1.2.0.2 table 20
        ip rule add table 20 to 1.2.3.5
        ip rule add table 10 to 1.2.3.4
      '';
    };
  };

  testScript = ''
    startAll;
    $router->waitForUnit('multi-user.target');
    $node1->waitForUnit('multi-user.target');
    $node2->waitForUnit('multi-user.target');

    $node1->succeed('ping -c1 1.2.3.5');
    $node2->succeed('ping -c1 1.2.3.4');
  '';
}

Output:

Failed assertions:
- networking.defaultGateway.interface is not supported by networkd.
(use '--show-trace' to show detailed location information)

Setting useNetworkd above to false lets the test pass, though... So I guess this PR needs to wait until we either have support networking.defaultGateway.interface or we add some ugly condition just for networkd... 😒

@flokli
Copy link
Contributor

@flokli flokli commented Jan 7, 2019

@flokli:

Is that some magic somewhere deep in the NixOS networking scripts?

Nope, the address is simply added via ip addr add using the specified prefixLength unmodified.

My question was more about the route to 1.2.3.4/32 being inserted, not the address - and there's indeed some magic in here that seems to lead to the desired effect with scripted networking and without explicitly specifying the gateway as peer, whoch doesn't map to networkd 1:1.

For comparison, I use that configuration for networkd:

  mkHetznerEth0Network = ipv6Primary: ipv4Address: ipv4Gateway: dnsServers: {
    address = ["${ipv6Primary}/128"];
    gateway = ["${ipv4Gateway}" "fe80::1"];
    dns = dnsServers;
    networkConfig.IPv6AcceptRA = false;
    addresses = [{
      addressConfig.Address = "${ipv4Address}/32";
      addressConfig.Peer = "${ipv4Gateway}/32";
    }];
  };

I'm not sure if we want to fix this for current setups first, and tackle "automatic" networkd support at a later point of time - there's some refactorization of the networkd support planned for NixOS anyway - cc @fpletz

@grahamc
Copy link
Member

@grahamc grahamc commented Mar 26, 2020

Hello!

Thank you for this PR.

In the past several months, some major changes have taken place in
NixOps:

  1. Backends have been removed, preferring a plugin-based architecture.
    Here are some of them:

  2. NixOps Core has been updated to be Python 3 only, and at the
    same time, MyPy type hints have been added and are now strictly
    required during CI.

This is all accumulating in to what I hope will be a NixOps 2.0
release
. There is a tracking issue for that:
#1242 . It is possible that
more core changes will be made to NixOps for this release, with a
focus on simplifying NixOps core and making it easier to use and work
on.

My hope is that by adding types and more thorough automated testing,
it will be easier for contributors to make improvements, and for
contributions like this one to merge in the future.

However, because of the major changes, it has become likely that this
PR cannot merge right now as it is. The backlog of now-unmergable PRs
makes it hard to see which ones are being kept up to date.

If you would like to see this merge, please bring it up to date with
master and reopen it
. If the or mypy type checking fails, please
correct any issues and then reopen it. I will be looking primarily at
open PRs whose tests are all green.

Thank you again for the work you've done here, I am sorry to be
closing it now.

Graham

@grahamc grahamc closed this Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.