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

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

Merged
merged 5 commits into from
Jul 28, 2022
Merged

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

merged 5 commits into from
Jul 28, 2022

Conversation

slyon
Copy link
Collaborator

@slyon slyon commented Jul 4, 2022

Description

Adding support for Virtual Routing and Forwarding devices using the sd-networkd and NetworkManager backends.
Any VRF device is linked to a (new) routing table, specified via the table stanza and can define its child interfaces via interfaces: [...]. If any routes: or routing-policy: settings are given, those are implicitly connected to the VRF's routing table.

It builds upon #272 (squashed & rebased), extracted VRF bits.

Example:

network:
  version: 2
  [...]
  vrfs:
    vrf1005:
      table: 1005
      interfaces:
        - br1
        - br1005
      routes:  # uses "table: 1005" implicitly
        - to: default
          via: 10.10.10.4
      routing-policy:
        - from: 10.10.10.42
  bridges:
    br1:
      interfaces: [...]
    br1005:
      interfaces: [...]

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#1773522

@Conan-Kudo
Copy link
Contributor

Can we have NetworkManager supported in this PR (or shortly afterward)?

@slyon
Copy link
Collaborator Author

slyon commented Jul 4, 2022

Can we have NetworkManager supported in this PR (or shortly afterward)?

Yes, I'm working on adding NM support to this PR, too.

@Conan-Kudo
Copy link
Contributor

Awesome!

@slyon slyon changed the title Adding VRF and VXLAN support for systemd-networkd (LP: #1764716, LP: #1773522) Add support for VRF devices (LP: #1773522) Jul 6, 2022
@slyon slyon force-pushed the slyon/vrf-vxlan branch 6 times, most recently from a92759b to f77f422 Compare July 7, 2022 10:17
@slyon slyon marked this pull request as ready for review July 7, 2022 10:45
@slyon
Copy link
Collaborator Author

slyon commented Jul 7, 2022

The new integration tests (routing.test_vrf_basic) are finally passing in CI and the commit history should now be rather clean.
The new YAML schema was pre-approved via an (internal) specification document.

Thank you very much @netdever for the initial implementation.
May I ask you to do a review of the VRF code that I added/adopted on top of yours?

Also, @Conan-Kudo seems to be interested, especially in the NetworkManager part.
May I ask you to do a code-review on this PR as well?

@slyon slyon added the schema ok label Jul 7, 2022
@netdever
Copy link

netdever commented Jul 7, 2022

Looks good to me. Thanks!

@slyon slyon requested a review from schopin-pro July 7, 2022 12:41
Copy link
Contributor

@schopin-pro schopin-pro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I'd need confirmation on the revertability issue.

src/netplan.c Show resolved Hide resolved
src/parse.c Show resolved Hide resolved
src/validation.c Outdated Show resolved Hide resolved
src/validation.c Show resolved Hide resolved
doc/netplan.md Outdated
@@ -72,7 +72,7 @@ Physical devices

Virtual devices

: (Examples: veth, bridge, bond) These are fully under the control of the
: (Examples: veth, bridge, bond, vrf) These are fully under the control of the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question (blocking): Seeing vrf in this list, how is revertability handled in netplan try?

Does networkd handle it on its own?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very good question! And I needed to do some investigation in order to answer this. But this is working as expected (and similar to vlans): vrfs are cleaned up on netplan apply (if using the --state parameter properly) or on netplan try (using the --config-file parameter). netplan try --state ... still behaves a bit odd, but that's unrelated to this PR.

VRFs are different to bonds/bridges, in that the backend (networkd/NM) does not need to initialize certain parameters, which cannot be changed afterwards and is therefore much more similar to a VLAN interface, which can just be deleted.
So I think this is all fine!

$ cat /etc/netplan/main_test.yaml 
network:
  version: 2
  renderer: networkd
  ethernets:
    enp0s31f6:
      dhcp4: true
      dhcp6: true
$ cat ./test.yaml
network:
  vrfs:
    vrf0:
      table: 1004
      interfaces: [enp0s31f6]
$ sudo LD_LIBRARY_PATH=. PYTHONPATH=. src/netplan.script --config-file=test.yaml try
Do you want to keep these settings?


Press ENTER before the timeout to accept the new configuration


Changes will revert in 111 seconds

# === meanwhile in another terminal === 
$ ip l show vrf0
110: vrf0: <NOARP,MASTER,UP,LOWER_UP> mtu 65575 qdisc noqueue state UP mode DEFAULT group default qlen 1000
    link/ether a2:f7:52:d9:7c:5c brd ff:ff:ff:ff:ff:ff
# === terminal switch ===
^C
Reverting.
$ ip l show vrf0
Device "vrf0" does not exist.

(I actually found a bug/typo while testing this, which I fixed in b39dbea)

@slyon slyon force-pushed the slyon/vrf-vxlan branch 2 times, most recently from 5328f9d to f94dedc Compare July 13, 2022 14:25
@slyon slyon requested a review from schopin-pro July 13, 2022 14:27
@slyon
Copy link
Collaborator Author

slyon commented Jul 13, 2022

Thanks for the quality review @schopin-pro! I think I addressed or replied to all of your remarks.
Additionally, I've added a missing handler for the renderer stanza for VRFs in V2 of 11f9867

Also, I found a bug/typo while re-testing this, which I fixed in b39dbea

Copy link
Contributor

@schopin-pro schopin-pro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, the netplan get question is not blocking for me.

src/validation.c Show resolved Hide resolved
Anthony Timmins and others added 2 commits July 28, 2022 10:11
V2:
+ reduce VRF table mismatch if-clause
+ handle 'renderer' stanza for VRF devices

V3:
+ Print a debug log when ignoring a redundant route table value on VRF devices

Co-authored-by: Lukas Märdian <[email protected]>
Co-authored-by: Lukas Märdian <[email protected]>
@slyon
Copy link
Collaborator Author

slyon commented Jul 28, 2022

Thanks for your final review. I've added a debug log about ignoring the redundant table value in V3 of 362b85a to account for that for now.

Merging.

@slyon slyon merged commit 30ab7fc into main Jul 28, 2022
@slyon slyon deleted the slyon/vrf-vxlan branch July 28, 2022 09:41
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.

4 participants