Adds wireguard network support #472

b-m-f · 2022-10-27T13:32:37Z

Belongs together with
containers/common#1209 and containers/podman#16291

b-m-f · 2022-11-13T09:37:23Z

This PR is now ready for review.

This feature is spread over 3 PRs:

Review support

As this is a huge PR and I have understood that you prefer 1 signed commit instead of many small ones I have written up some things that might be helpful/contentious.

Network Driver Trait

NetworkDriver setup function was extended with another tuple of Generic Netlink sockets
- both the original and generic socket tuples are now wrapped in Options to support the NamespaceOptions struct for both

Generic Netlink

Support for WireGuard payloads and family lookup is added
The wireguard_family will be cached during the process lifetime to reduce the amount of packages.

WireGuard Network driver

Parsing

The config that is passed from podman is parsed during validation

NLA generation

NLAS for the netlink packages are generated in the driver file as the data types should be private to it. They are then passed to the generic netlink socket

Setup flow

Corresponds to https://www.wireguard.com/netns/ -> Ordinary Containerization

Interface Name

The interface name is still passed from podman.
Since it defaults to ethX this will be the interface name inside the container. Contrary to the discussed wg-12_CHAR_HASH.
I could not see an easy way to create this on the podman side, as the driver needs to be checked together with the per network options.

This code could be the entrypoint

AllowedIPs routing

Right now a route is generated if the interface address(gateway) is part of the destination OR in the same supernet as the destination.
This is inside function generate_routes_for_peer in wireguard.rs

I have added tests to verify that my logic is sound but maybe I have missed something?

Tests

Tests for 1 succesful spawn using all supported configuration parameters is added.
In addition there should be a failing case for all possible invalid configurations to make sure that the parsing is doing what its supposed to.

flouthoc

My two cents but open to discussion and please feel free to correct me, I think there should be VPNDriver and VPNDriver should only implement wiregaurd we are creating top level config for Wiregaurd but it should be sub-config so this can become scalable in future.

b-m-f · 2022-11-14T09:38:13Z

@flouthoc this would work since everything is passed via the options HashMap into netvark. Both VPN types could then use a VPN type network.

My thoughts on this:

I would prefer not to have an abstraction layer over the drivers. It will make documentation more nested (difficult) and choosing the correct driver in the code will need to be made according to the options passed into netvark, as the driver is not specific enough.

Luap99 · 2022-11-14T11:43:05Z

My two cents but open to discussion and please feel free to correct me, I think there should be VPNDriver and VPNDriver should only implement wiregaurd we are creating top level config for Wiregaurd but it should be sub-config so this can become scalable in future.

I don't think we really want to add more VPN drivers, more abstractions just makes everything more complicated.

Luap99 · 2022-11-14T11:44:06Z

@b-m-f validate and integrations test are failing, please see the CI logs
I try to review it this week when I find some time.

Luap99 · 2023-02-17T13:44:30Z

There are some updates to the other netlink crates, maybe we should update them all first.

b-m-f · 2023-02-17T14:04:40Z

@Luap99 Bingo. That seems to be it.

Upgrading them all and reordering a few imports in network/netlink.rs did the trick.

The netlink-packet-route crate stopped reexporting from netlink-packet-core.

Luap99 · 2023-02-17T14:07:33Z

Yeah just did it myself: #592

Luap99

src/network/netlink_link.rs is an empty file, please remove it.

I did not look at the tests yet but code looks good so far but I want to take another look.

Luap99 · 2023-02-22T15:19:33Z

src/network/netlink.rs

-    };
+pub struct LinkSocket {
+    socket: netlink_sys::Socket,
+    sequence_number: u32,


one of the reasons I included the buffer in the struct was that rust AFAIK initializes the buffer ([0; 8192]) with zeros every time you call it. So I wanted to it only once, of course I have not measured performance just something that I should take a look. I am ok with the change if it is not noticeable.

I see your point.

Just let me know what you prefer and I can try to move let mut buffer = [0; 8192]; back into the struct.
Looking at it right now I don't recall if there was a motive behind this change other than the comfort of having a dedicated buffer each time.
But if there was it'll come up during a rework for sure :)

Luap99 · 2023-02-22T15:22:02Z

Cargo.toml

-netlink-packet-core = "0.4.2"
+netlink-packet-route = "0.15"
+netlink-packet-core = "0.5.0"
+netlink-packet-wireguard = { git = "https://github.com/rust-netlink/netlink-packet-wireguard", version = "0.2.2" }


is there a specific thing you need from the gihub version, is it not pushed to crates.io?

I noticed that the repo repo and the crate have gotten out of sync.

My thought was to track the repo so that the libraries stay compatible.
Do you think this is a problem from a security perspective?

Not really, it will require distros that use vendored sources to update the cargo.conf to include the new location. Not a big deal just annoying. We know one of the maintainers so I can ask them to publish it on crates.io, likely some oversight.

Very nice. I will update it afterwards 👍

b-m-f · 2023-03-01T09:18:52Z

@Luap99 can you see what is failing during the integration_aarch64 task in the CI frontend?
I only get limited info here on Github

Luap99 · 2023-03-01T14:26:54Z

Test failure is a flake #433, I will press rerun until it passes

openshift-ci · 2023-03-22T14:57:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: b-m-f
Once this PR has been reviewed and has the lgtm label, please assign vrothberg for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2023-03-22T14:58:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: b-m-f
Once this PR has been reviewed and has the lgtm label, please assign vrothberg for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: b-m-f <[email protected]>

b-m-f · 2023-04-29T20:40:59Z

Hi @Luap99 @baude,
first of all it was really easy to work with the plugin API and port everything to it.
Basically just a bunch of copy and pasting. Nice one.

But I had to do all the work inside of the netavark repo for now, since the newest version is not yet on crates.io.

Maybe you could push it to the registry and then I could split out the WireGuard plugin into a new repository.

Edit: Actually I can just point cargo to the repo for now. But its probably still nicer to point to a version :)

baude · 2023-05-01T17:34:01Z

thanks for the poke .. that is on me ... should be fixed up now

b-m-f · 2023-05-01T20:40:47Z

Thanks @baude ,

I've worked the code into a plugin at https://github.com/b-m-f/netavark-wireguard-plugin .

The integration worked nicely.
Not all is working perfectly yet, mainly the routing, but that should come with time.

Adding the plugin-API was a good call, for me its much easier to organize the codebase and swap out libraries this way.

Will close the PR now

openshift-merge-robot added the needs-rebase label Oct 27, 2022

b-m-f force-pushed the wireguard-support branch from a97c0b8 to 412904a Compare October 28, 2022 10:02

openshift-merge-robot removed the needs-rebase label Oct 28, 2022

b-m-f force-pushed the wireguard-support branch 3 times, most recently from 810fb42 to 57f5727 Compare October 28, 2022 13:57

b-m-f force-pushed the wireguard-support branch from 64d5d96 to 205f288 Compare November 4, 2022 12:49

openshift-merge-robot added the needs-rebase label Nov 9, 2022

b-m-f force-pushed the wireguard-support branch from 2d48fe3 to 1438c2a Compare November 9, 2022 13:01

openshift-merge-robot removed the needs-rebase label Nov 9, 2022

b-m-f force-pushed the wireguard-support branch 7 times, most recently from 9afc3d8 to 3eff38b Compare November 12, 2022 14:22

b-m-f mentioned this pull request Nov 12, 2022

WireGuard support containers/common#1209

Closed

b-m-f force-pushed the wireguard-support branch 5 times, most recently from 0cd4656 to 344d34a Compare November 13, 2022 09:36

b-m-f mentioned this pull request Nov 13, 2022

WireGuard Support containers/podman#16291

Closed

b-m-f marked this pull request as ready for review November 13, 2022 09:37

flouthoc reviewed Nov 14, 2022

View reviewed changes

b-m-f force-pushed the wireguard-support branch from eb4443f to 22fbbd3 Compare February 17, 2023 14:04

openshift-merge-robot removed the needs-rebase label Feb 17, 2023

b-m-f force-pushed the wireguard-support branch from 22fbbd3 to 8b2cbd2 Compare February 20, 2023 08:44

Luap99 reviewed Feb 22, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase label Feb 24, 2023

b-m-f force-pushed the wireguard-support branch from 8b2cbd2 to ee8914e Compare March 1, 2023 08:49

openshift-merge-robot removed the needs-rebase label Mar 1, 2023

openshift-merge-robot added the needs-rebase label Mar 6, 2023

b-m-f force-pushed the wireguard-support branch from ee8914e to 99e81e7 Compare March 22, 2023 13:16

openshift-merge-robot removed the needs-rebase label Mar 22, 2023

b-m-f mentioned this pull request Mar 22, 2023

[Proposal] Allow for ipranges when adding routes b-m-f/wg-pod#5

Open

b-m-f closed this Mar 22, 2023

b-m-f force-pushed the wireguard-support branch from 99e81e7 to 70eee07 Compare March 22, 2023 14:47

b-m-f reopened this Mar 22, 2023

b-m-f force-pushed the wireguard-support branch 2 times, most recently from 62b1c5b to b0a26f2 Compare March 22, 2023 15:00

b-m-f mentioned this pull request Mar 22, 2023

add netavark plugin support #509

Merged

Adds WireGuard driver

efac8ed

Signed-off-by: b-m-f <[email protected]>

b-m-f force-pushed the wireguard-support branch from b0a26f2 to efac8ed Compare March 28, 2023 06:05

b-m-f closed this May 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds wireguard network support #472

Adds wireguard network support #472

b-m-f commented Oct 27, 2022 •

edited

Loading

b-m-f commented Nov 13, 2022

flouthoc left a comment

b-m-f commented Nov 14, 2022

Luap99 commented Nov 14, 2022

Luap99 commented Nov 14, 2022

Luap99 commented Feb 17, 2023

b-m-f commented Feb 17, 2023 •

edited

Loading

Luap99 commented Feb 17, 2023

Luap99 left a comment

Luap99 Feb 22, 2023

b-m-f Feb 25, 2023

Luap99 Feb 22, 2023

b-m-f Feb 25, 2023

Luap99 Feb 27, 2023

b-m-f Mar 1, 2023

b-m-f commented Mar 1, 2023

Luap99 commented Mar 1, 2023

openshift-ci bot commented Mar 22, 2023

openshift-ci bot commented Mar 22, 2023

b-m-f commented Apr 29, 2023 •

edited

Loading

baude commented May 1, 2023

b-m-f commented May 1, 2023

Adds wireguard network support #472

Adds wireguard network support #472

Conversation

b-m-f commented Oct 27, 2022 • edited Loading

b-m-f commented Nov 13, 2022

Review support

Network Driver Trait

Generic Netlink

WireGuard Network driver

Parsing

NLA generation

Setup flow

Interface Name

AllowedIPs routing

Tests

flouthoc left a comment

Choose a reason for hiding this comment

b-m-f commented Nov 14, 2022

My thoughts on this:

Luap99 commented Nov 14, 2022

Luap99 commented Nov 14, 2022

Luap99 commented Feb 17, 2023

b-m-f commented Feb 17, 2023 • edited Loading

Luap99 commented Feb 17, 2023

Luap99 left a comment

Choose a reason for hiding this comment

Luap99 Feb 22, 2023

Choose a reason for hiding this comment

b-m-f Feb 25, 2023

Choose a reason for hiding this comment

Luap99 Feb 22, 2023

Choose a reason for hiding this comment

b-m-f Feb 25, 2023

Choose a reason for hiding this comment

Luap99 Feb 27, 2023

Choose a reason for hiding this comment

b-m-f Mar 1, 2023

Choose a reason for hiding this comment

b-m-f commented Mar 1, 2023

Luap99 commented Mar 1, 2023

openshift-ci bot commented Mar 22, 2023

openshift-ci bot commented Mar 22, 2023

b-m-f commented Apr 29, 2023 • edited Loading

baude commented May 1, 2023

b-m-f commented May 1, 2023

b-m-f commented Oct 27, 2022 •

edited

Loading

b-m-f commented Feb 17, 2023 •

edited

Loading

b-m-f commented Apr 29, 2023 •

edited

Loading