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

nixos/networking: add ipvlan option #48753

Closed
wants to merge 1 commit into from
Closed

Conversation

XA21X
Copy link

@XA21X XA21X commented Oct 20, 2018

Largely adapted from the closely related macvlan implementation by @wkennington and @montag451.

Eventually got DHCP working for the tests, thanks to @dcbw and @gleber:

Motivation for this change

This change makes ipvlans as easy to use as macvlans.

Ipvlans can be considered instead of macvlans when there are restrictions on the usage of additional MAC addresses i.e. switch port security enforced by VM hosts.

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)
    • networking.nix
    • containers-macvlans.nix
    • containers-ipvlans.nix (NEW!)
  • 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.

@Mic92
Copy link
Member

Mic92 commented Oct 23, 2018

You have a sandbox if you are using 18.09 or newer btw.

nixos/tests/networking.nix Outdated Show resolved Hide resolved
Largely adapted from the closely related macvlan implementation. Ipvlans
are useful when there are restrictions on the usage of additional MAC
addresses (i.e. switch port security enforced by VM hosts) which are
required for macvlans.

* Use mkIf to switch between DHCP configurations.
* Use duid for dhcpcd (default for networkd).
@XA21X
Copy link
Author

XA21X commented Oct 24, 2018

You have a sandbox if you are using 18.09 or newer btw.

Missed that from the release notes, although I was using 18.03 for the tests instead of the 18.09 server where I'm using these changes. Thanks for reminding me to upgrade! :)

I think dhcpcd does use mac addresses while networkd uses duid by default for dhcp.

I've looked through the documentation for dhcpcd and networkd which confirms that only the latter defaults to DUID, but I'm still not sure how networkd's implementation works. If I assume it's similar to dhcpcd, the DUID is generated from some machine UUID which should remain constant for a given VM, and the IAID should also be the same because the ipvlan has the same MAC as eth1, which should result in a conflict, but it doesn't.

@mmahut
Copy link
Member

mmahut commented Aug 10, 2019

Are there any updates on this pull request, please?

@duairc
Copy link
Contributor

duairc commented Dec 17, 2019

i'm very interested in this too.

@stale
Copy link

stale bot commented Jun 14, 2020

Thank you for your contributions.

This has been automatically marked as stale because it has had no activity for 180 days.

If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.

Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse.
  3. Ask on the #nixos channel on irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 14, 2020
@duairc
Copy link
Contributor

duairc commented Jun 14, 2020

This is still important to me. @XA21X, @Mic92, do you think there's any chance that this will be merged?

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 14, 2020
@deliciouslytyped
Copy link
Contributor

I'm using ipvlans a lot due to "bridging" from wifi currently, and I discovered this issue through the xlink to containernetworking/cni#17 by accident. It would be nice to get better upstream ipvlan support. Currently I just have a small patch copying the nixos-containers macvlan option to provide the ipvlan argument to nspawn containers.

@dcbw Thanks for your explanation in the other thread, it helped me find an issue with a linux ipvlan setup.

This section is wrong because NixOS actually uses DHCPCD

  • TODO:I couldn't get the patch to apply.

The ISC DHCP client (dhclient, which seems to be the default dhcp client on most linuxes) has semi-hardcoded constants for whether the broadcast fag is set in dhcp packets (which seems like an odd thing to do...). A user at https://archive.vn/?url=https://forum.netgate.com/topic/141362/dhcp-client-unable-to-get-lease-from-cable-provider-solved explains as much. As can be seen at https://github.com/isc-projects/dhcp/search?q=can_receive_unicast_unconfigured , these values are still hardcoded.

It seems the easiest way to tell which API dhclient is using is to look at it's -v output; you will see something like the following:

Listening on LPF/<interface>/<mac address>
Sending on   LPF/<interface>/<mac address>
Sending on   Socket/fallback

So apparently on NixOS (and most other linuxes), LPF is the default API (or something? It's not entirely obvious from the source https://github.com/isc-projects/dhcp/blob/31e68e5e3b863a4859562e0bb808888d74af7497/includes/osdep.h ). As we can see in the log, packets are sent over LPF and/or Socket (I'm not sure what's going on here, but seemingly only one packet leaves the interface?). In this case we can see the value is truly hardcoded: https://github.com/isc-projects/dhcp/blob/31e68e5e3b863a4859562e0bb808888d74af7497/common/lpf.c#L497 .

The following demonstrates a patch that makes this work: (be careful about tabs in the patch)

        nixpkgs.overlays = [ (self: super: {
          dhcp = super.dhcp.overrideAttrs (old: {
            patches = [ (pkgs.writeText "broadcast.patch" ''
              --- a/common/lpf.c
              +++ b/common/lpf.c
              @@ -494,5 +494,5 @@ int can_unicast_without_arp (ip)
               int can_receive_unicast_unconfigured (ip)
                      struct interface_info *ip;
               {
              -       return 1;
              +       return 0;
               }
              --- a/common/socket.c
              +++ b/common/socket.c
              @@ -1196,7 +1196,7 @@ int can_receive_unicast_unconfigured (ip)
                      struct interface_info *ip;
               {
               #if defined (SOCKET_CAN_RECEIVE_UNICAST_UNCONFIGURED)
              -       return 1;
              +       return 0;
               #else
                      return 0;
               #endif
              '') ];
            });          
          }) ];

dhcpcd / This section is correct

The following is sufficient when dhcpcd is used (if you look at the dhcpcd man page you will also see flags -J and for clientid);

{
#...
        networking.interfaces."iv-${netdev}".useDHCP = true;

        networking.dhcpcd.extraConfig = ''
          broadcast
          clientid ${my_id}
          '';
}

@stale
Copy link

stale bot commented Oct 22, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 22, 2021
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 12, 2023
@Artturin
Copy link
Member

Superseded by #268180, I think.

@Artturin Artturin closed this Nov 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants