Adding VRF and VXLAN support for systemd-networkd (LP: #1764716, LP: #1773522) #272

netdever · 2022-04-15T22:15:14Z

Description

Adding VRF and VXLAN support for systemd-networkd.

Checklist

Runs make check successfully.
Retains 100% code coverage (make check-coverage).
New/changed keys in YAML format are documented.
(Optional) Adds example YAML for new feature.
(Optional) Closes an open bug in Launchpad. LP#1764716, LP#1773522

codecov-commenter · 2022-04-15T22:25:07Z

Codecov Report

Merging #272 (c8ed0cf) into main (6b9a3b7) will decrease coverage by 0.82%.
The diff coverage is 66.54%.

@@            Coverage Diff             @@
##             main     #272      +/-   ##
==========================================
- Coverage   99.06%   98.24%   -0.83%     
==========================================
  Files          61       63       +2     
  Lines       11160    11471     +311     
==========================================
+ Hits        11056    11270     +214     
- Misses        104      201      +97

Impacted Files	Coverage Δ
src/parse-nm.c	`100.00% <ø> (ø)`
src/parse.c	`97.97% <38.23%> (-1.87%)`	⬇️
src/netplan.c	`90.97% <45.71%> (-9.03%)`	⬇️
src/networkd.c	`95.62% <57.53%> (-4.38%)`	⬇️
src/validation.c	`98.51% <75.00%> (-1.10%)`	⬇️
netplan/configmanager.py	`100.00% <100.00%> (ø)`
src/types.c	`100.00% <100.00%> (ø)`
tests/generator/base.py	`100.00% <100.00%> (ø)`
tests/generator/test_bridges.py	`100.00% <100.00%> (ø)`
tests/generator/test_ethernets.py	`100.00% <100.00%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b9a3b7...c8ed0cf. Read the comment docs.

src/parse-nm.c

slyon

Hi @netdever
Thank you very much for your contribution to netplan, this is highly appreciated, especially as the two LP bugs that you're addressing have been long-standing feature requests!

I did an initial review and mentioned some smaller issues and questions in inline comments.

I am now also thinking about bigger changes that might be needed:

configmanager.py to go away (conflict with YAML consolidation: final patch set for configmanager.py (FR-702) #255)
NetplanTristate type should be added (conflict with Add tristate type for offload options (LP: #1956264) #270)
Linking of network interfaces: I wonder if we should rather define a VRF's child interfaces inside the vrf definition itself, using an interfaces: [...] setting (as suggested in the LP#1773522) instead of splitting the definition across parent and child, using the vrf: ... link:

network:
  version: 2
  renderer: networkd
  vrfs:
    vrf-test:
      interfaces:
        - bond0
        - br0
        - br-bond0
        - tun0
      table: auto # or a specific integer

Similarly, I need to thinkg about the linking of VXLAN devices, as the ethX.vxlans.names = [...] seems to be a bit cumbersome. Is there a specific reason why you went for this nestring structure?
Overall, I want to review all the new YAML settings in detail, to make sure we implement a simple and easily comprehensible wording. There is nothing to change about this right now... for the current stage we can just go with systemd-networkd's naming scheme and we can easily change/adopt that, once agreed upon (but before we merge the PR :))
NetworkManager also supports (most) VRF/VXLAN settings that are implemented in this PR for the systemd-networkd backend renderer. With all the parsing code already in place, it should be relatively easy to implement those in src/nm.c. Would you be interested in doing that? But it could also be done as part of a separate PR, in which case we should at least log a warning from the NetworkManager backend renderer, that those settings are not currently supported on that backend.
Finally, I'd really love to see some actuall integration tests (in tests/integration/...) that make use of those new VRF and VXLAN features and check the fully configured system at runtime, to make sure the settings are actually applied. Is that something you could add do your PR? The tests are being run as autopkgtests, as can be seen (and reproduced locally) in ".github/workflows/autopkgtest.yml`.

slyon · 2022-05-04T14:54:40Z

doc/netplan.md

+     ``udp-checksum`` (scalar)
+     :    Takes a boolean. When true, transmitting UDP checksums when doing VXLAN/IPv4 is turned on.
+
+     ``udp6-zero-checksum-tx`` (scalar)
+     :    Takes a boolean. When true, sending zero checksums in VXLAN/IPv6 is turned on.
+
+     ``udp6-zero-checksum-rx`` (scalar)
+     :    Takes a boolean. When true, receiving zero checksums in VXLAN/IPv6 is turned on.
+
+     ``remote-checksum-tx`` (scalar)
+     :    Takes a boolean. When true, remote transmit checksum offload of VXLAN is turned on.
+
+     ``remote-checksum-rx`` (scalar)
+     :    Takes a boolean. When true, remote receive checksum offload in VXLAN is turned on.


I wonder if we should bundle those up as individual Flags somehow, e.g. checksums: [udp6-zero-rx, udp6-zero-tx, remote-rx, remote-tx, udp]

Similar to how it is done for optional-addresses

I gave this a shot but hit some snags. I'm not sure optional-addresses behaves the same way, but I could be missing something.

doc/netplan.md

slyon · 2022-05-04T15:00:23Z

netplan/configmanager.py

+    @property
+    def vxlans(self):
+        return self.network['vxlans']
+
+    @property
+    def vrfs(self):
+        return self.network['vrfs']
+


configmanager.py will go away once we land PR #255 So this might need to be re-worked to make it play nice with the new and consolidated YAML parser from that PR (depending on which PR will be merged first...)

This is just a FYI for you right now. The vrf/vxlan configmanager code is good as-is for now. We shouldn't put too much effort into it, though as it will go away.

Fingers crossed this gets merged first :)

slyon · 2022-05-04T15:03:02Z

src/abi.h

+        gboolean udp_checksum;
+        gboolean udp6_zero_checksum_tx;
+        gboolean udp6_zero_checksum_rx;
+        gboolean remote_checksum_tx;
+        gboolean remote_checksum_rx;


maybe those could be bundled as flags/enum, similar to NetplanOptionalAddressFlag

Had some trouble trying to bundle these myself, so leaving as is for now. Any help would be appreciated if we feel it's necessary to make this change.

src/abi.h

slyon · 2022-05-04T15:11:54Z

src/netplan.c

+        YAML_BOOL_FALSE(def, event, emitter, "group-policy-extension", def->vxlan_params.group_policy_extension);
+        YAML_BOOL_FALSE(def, event, emitter, "generic-protocol-extension", def->vxlan_params.generic_protocol_extension);
+        YAML_UINT_0(def, event, emitter, "destination-port", def->vxlan_params.destination_port);
+        YAML_STRING(def, event, emitter, "port-range", def->vxlan_params.port_range);


In which way would we specify a "port-range" value? Can you give an example for this?

It seems like NetworkManager uses two separate settings for this:

source-port-max

source-port-min

Combining those into a single "port-range" sounds nice, but I'm wondering about the syntax?

https://networkmanager.dev/docs/api/latest/nm-settings-nmcli.html#id-1.2.9.4.43

In my testing, I could only specify an integer for PortRange= using systemd-networkd. Obviously that's not a "range." Documentation is lacking for this parameter in systemd-networkd.

I found and answer in systemd's source code: https://github.com/systemd/systemd/blob/main/src/basic/parse-util.c#L259

the parse_range() util allows giving a range like this: "1-2892" from LOWER to UPPER bound, separated by dash.

I think we should potentially use a source-port-range: [1, 2892] syntax in netplan, i.e. an explicit list with exactly two elements (LOWER, UPPER).

OK, I was able to get this to work as described. There isn't validation for it yet, though.

slyon · 2022-05-04T15:13:45Z

src/netplan.c

+        YAML_STRING(def, event, emitter, "remote", def->vxlan_params.remote);
+        YAML_STRING(def, event, emitter, "local", def->vxlan_params.local);
+        YAML_STRING(def, event, emitter, "group", def->vxlan_params.group);


IIUC Networkmanager uses a single setting to specify "remote" and "group", I wonder if we could do the same? Would the "remote" and "group" settings ever be used at the same time? Or can we deduct from the setting's value which one was meant by the user?

https://networkmanager.dev/docs/api/latest/nm-settings-nmcli.html#id-1.2.9.4.43

Ineed, Group= will always be a multicast IP address (i.e. from a reserved IPv4/6 range), so we can implicitly derive this information, therefore I'd like to reduce those two fields to just a single remote field, that is than translated to Remote= or Group= for systemd-networkd depending on if it is a multicast IP or not.

slyon · 2022-05-04T15:20:17Z

src/abi.h

+        guint destination_port;
+        char* port_range;
+        guint flow_label;
+        gboolean ip_do_not_fragment;


Beware that the gboolean variable cannot hold a TRUE, FALSE or UNSET (i.e. kernel's default) value, as described in the netplan.md documentation. I think we need a NetplanTristate type here, as is currently being drafted in #270 (src/types.h). We should probably copy and use that type definition into this PR, too, so it can be merged later on (depending on which PR lands first).

The same problem applies to neigh-suppress and potentially other boolean values, that are also supposed to have an "unset" state.

src/parse-nm.c

src/abi.h

doc/netplan.md

netdever · 2022-05-06T02:19:50Z

Linking of VRFs now looks more like the example in the linked LP bug.
Linking of VXLANs to source interfaces no longer requires the names subvar.
VXLAN source-port-range now works as suggested (lacks validation)

slyon · 2022-05-09T08:44:22Z

src/abi.h

+    /* netplan-feature: vxlan */
+    guint vxlan_vni;
+    GArray* vxlans;
+    gboolean neigh_suppress;


This seems to be a rather generic [Bridge] setting, not necessarily related to VXLANs.. Is it required for VXLAN? If so, we should implement it as a generic bridge feature (and probably use a tristate type, too).

Otherwise, I'd like to just drop it from this PR, as it seems unrelated. Especially, as there is currently no implementation in any backend (neither networkd.c nor nm.c for this neigh_suppress variable).

Neighbor suppression (arp-nd-suppression) is required for VXLAN configuration. It is disabled by default, but many implementations, specifically those utilizing EVPN, will need it to be enabled. In systemd-networkd, it looks like this:

root@host1:~# cat /run/systemd/network/10-netplan-vxlan10.network [Match] Name=vxlan10 [Link] MTUBytes=8950 [Network] LinkLocalAddressing=no IPv6AcceptRA=no ConfigureWithoutCarrier=yes Bridge=bridge10 [Bridge] NeighborSuppression=True

The command to enable this feature is bridge link set dev vxlan10 neigh_suppress on

Thank you for your comments about this. Being a [Bridge] setting in sd-networkd, I still wonder if this is a setting that should be set on the VXLAN device itself, or rather on a bridge device that is a parent interface to the vxlan?

It is a VXLAN setting (ie. you cannot enable it on the master Bridge interface). It may be confusing for netplan to have it as a bridge setting, especially since enslaving a VXLAN to a Bridge is not technically a requirement.

root@host1:~# bridge link set dev bridge10 neigh_suppress on RTNETLINK answers: Operation not supported

slyon · 2022-07-04T12:28:28Z

This PR got closed by accident after rebasing on top of the current main branch and then pushing the wrong branch to netdever/main. I do not have write access to netdever's branch anymore. So I created a copy of this in #285

A backup of netdever's original branch is still available here: https://github.com/slyon/netplan/tree/vrf-vxlan-netdever-BACKUP

netdever force-pushed the main branch from 2204e9b to bd936ef Compare April 19, 2022 15:17

Conan-Kudo suggested changes Apr 19, 2022

View reviewed changes

src/parse-nm.c Outdated Show resolved Hide resolved

schopin-pro added the schema change This PR includes a change to netplan's YAML schema, which needs sign-off label Apr 19, 2022

netdever force-pushed the main branch 3 times, most recently from d5e03cc to 9890f2c Compare April 27, 2022 15:29

slyon self-requested a review May 4, 2022 08:04

slyon changed the title ~~Adding VRF and VXLAN support for systemd-networkd~~ Adding VRF and VXLAN support for systemd-networkd (LP: #1764716, LP: #1773522) May 4, 2022

slyon reviewed May 4, 2022

View reviewed changes

slyon reviewed May 5, 2022

View reviewed changes

src/abi.h Outdated Show resolved Hide resolved

slyon reviewed May 5, 2022

View reviewed changes

doc/netplan.md Outdated Show resolved Hide resolved

slyon reviewed May 9, 2022

View reviewed changes

slyon closed this Jul 4, 2022

slyon force-pushed the main branch from c8ed0cf to e748158 Compare July 4, 2022 12:22

slyon mentioned this pull request Jul 4, 2022

Add support for VRF devices (LP: #1773522) #285

Merged

5 tasks

slyon mentioned this pull request Jul 14, 2022

Add support for VXLAN tunnels (LP: #1764716) #288

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding VRF and VXLAN support for systemd-networkd (LP: #1764716, LP: #1773522) #272

Adding VRF and VXLAN support for systemd-networkd (LP: #1764716, LP: #1773522) #272

netdever commented Apr 15, 2022 •

edited by slyon

Loading

codecov-commenter commented Apr 15, 2022 •

edited

Loading

slyon left a comment

slyon May 4, 2022

netdever May 5, 2022

slyon May 4, 2022

netdever May 5, 2022

slyon May 4, 2022

netdever May 5, 2022

slyon May 4, 2022

netdever May 5, 2022

slyon May 5, 2022

netdever May 6, 2022

slyon May 4, 2022

slyon May 5, 2022

slyon May 4, 2022

netdever commented May 6, 2022

slyon May 9, 2022

netdever May 9, 2022

netdever May 9, 2022

slyon May 9, 2022

netdever May 9, 2022

slyon commented Jul 4, 2022 •

edited

Loading

Adding VRF and VXLAN support for systemd-networkd (LP: #1764716, LP: #1773522) #272

Adding VRF and VXLAN support for systemd-networkd (LP: #1764716, LP: #1773522) #272

Conversation

netdever commented Apr 15, 2022 • edited by slyon Loading

Description

Checklist

codecov-commenter commented Apr 15, 2022 • edited Loading

Codecov Report

slyon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netdever commented May 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slyon commented Jul 4, 2022 • edited Loading

netdever commented Apr 15, 2022 •

edited by slyon

Loading

codecov-commenter commented Apr 15, 2022 •

edited

Loading

slyon commented Jul 4, 2022 •

edited

Loading