[WIP] Add support for gretap/ip6gretap OVS tunnels

[Caligatio]

Description

This adds support for gretap and ip6gretap Open vSwitch (OVS) tunnels to netplan. I'm hoping to get help/feedback on a few questions:

• OVS calls GRE tunnels between two bridges just gre tunnels. However, in the semantics of netplan, it is actually a gretap tunnel (i.e. it's putting layer 2 packets through a GRE tunnel). I went with calling these gretap/ip6gretap tunnels as I feel it's technically correct but I also think this is going to cause confusion. Thoughts?
• For all tunnel types, the local_ip is a required YAML key but it's really optional for most tunnels. Is it OK to remove the required check on it? I'm happy to tweak the affected tests.
• I opted for OVS-flavored bridges to make any attached gretap/ip6gretap tunnels inherit being OVS controlled. I don't have a firm understanding on the order in which interfaces are parsed so am unsure if this method works every time or is dependent on interface order.
  • This raises a secondary question about OVS bonds: if a bond detects one of its members being OVS, it then switches its back-end to OVS. However, I didn't see any code to make a bridge containing a now OVS bond to then become OVS.

Once/if people are happy with my changes, I'll tack on another commit with tests.

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.

[slyon]

Thank you Brian for this PR! I'm overall happy with the decisions you made and left a few comments inline that should answer most of your questions. The biggest concern is probably the loosening of the local-ip validation, which should be restricted to the relevant cases.

We will have a final/full review of the PR once testing and documentation is in place, at which time we might also consult Steve about the GRE vs GRETAP wording.

This raises a secondary question about OVS bonds: if a bond detects one of its members being OVS, it then switches its back-end to OVS. However, I didn't see any code to make a bridge containing a now OVS bond to then become OVS.
Well, we might not actually have thought about that case before... I think this is a similar sorting problem to what I described in my parse.c comment and we probably need to set the parent bridge to use the OVS backend once the child bond changes. We should probably fix this (could even be part of this PR).

Thank you and keep up the good work!

[slyon]

I agree we want to use the GRETAP / IP6GRETAP modes here, if those are the technically correct terms for what is put on the (virtual) wire! (Although OVS calls it just GRE/IP6GRE)

But we probably want to have Steve's opinion on that before we merge this PR.

[slyon]

Let's add an additional comment here, to avoid an GRE VS GRETAP confusion, ideally referencing some kind of OVS documentation where it is stated that GRE in OVS is actually GRETAP.

[slyon]

Thanks for adding the comment!

nitpick: we have a tiny formatting issue here, as the comment is using tabs instead of spaces. That should be fixed in a later revision.

[slyon]

Loosening this check is probably fine for most tunnel configurations. But we should not allow people to generate invalid configuration in some cases, so could we please keep this check, but restrict it to the relevant cases?

[slyon]

The netplan net definitions are parsed in the order they are listed in the YAML file.

So if a tunnel is defined before the bridge this code should work. If the bridge is defined first a new netdef will be created via add_missing_node(), but that will not necessarily have all the tunnel mode settings ready.

So I think we need an additional check in handle_tunnel_mode() to check if this tunnel is an interface to an OVS bridge and then set the NETPLAN_BACKEND_OVS accordingly.

We should add test for both cases (bridge first, tunnel first) to make sure this works as expected.

[Caligatio]

I added an extra comment about the gre->gretap mapping; unfortunately there is no official documentation that references gretap so I attempted to explain.

Unfortunately I'm not an expert on all the tunnel types so do you know which actually need the local IP? These are the supported tunnel types and my understanding of whether local IP is needed:

• sit - ???
• gre - not needed
• ip6gre - not needed
• ipip - not needed*
• ipip6- not needed*
• ip6ip6- not needed*
• vti- ???
• vti6- ???
• wireguard - not needed
• gretap - not needed
• ip6gretap - not needed

* I'm going off solely https://developers.redhat.com/blog/2019/05/17/an-introduction-to-linux-virtual-interfaces-tunnels#ipip_tunnel which says, for ipip, local IP is defaults to any and thus not needed

Finally, I'm having second thoughts about my last question. There was already code present to make a bridge OVS-flavored if a member interface was explicitly OVS and I added code to make member interfaces OVS-flavored if the bridge is OVS. It's very possible I'm missing something but what case would hooking handle_tunnel_mode() catch?

[slyon]

I added an extra comment about the gre->gretap mapping; unfortunately there is no official documentation that references gretap so I attempted to explain.

Thank you! I added a small nitpick inline about that (formatting issue).

Unfortunately I'm not an expert on all the tunnel types so do you know which actually need the local IP? These are the supported tunnel types and my understanding of whether local IP is needed:

• sit - ???

• gre - not needed

• ip6gre - not needed

• ipip - not needed*

• ipip6- not needed*

• ip6ip6- not needed*

• vti- ???

• vti6- ???

• wireguard - not needed

• gretap - not needed

• ip6gretap - not needed

• I'm going off solely https://developers.redhat.com/blog/2019/05/17/an-introduction-to-linux-virtual-interfaces-tunnels#ipip_tunnel which says, for ipip, local IP is defaults to any and thus not needed

What's relevant for netplan is the support in the underlying rendering backends (networkd / NetworkManager), so I've checked the following documentation:

• https://systemd.network/systemd.netdev.html#%5BTunnel%5D%20Section%20Options

Local=
A static local address for tunneled packets. It must be an address on another interface of this host, or the special value "any".

• https://developer-old.gnome.org/NetworkManager/stable/settings-ip-tunnel.html

local (string)
The local endpoint of the tunnel; the value can be empty, otherwise it must contain an IPv4 or IPv6 address.

This looks like it is indeed not needed, as we can fallback to any on networkd and to an empty value on NetworkManager. So we just need to make sure to handle those cases in the respective renderers (networkd.c and nm.c)

Finally, I'm having second thoughts about my last question. There was already code present to make a bridge OVS-flavored if a member interface was explicitly OVS and I added code to make member interfaces OVS-flavored if the bridge is OVS. It's very possible I'm missing something but what case would hooking handle_tunnel_mode() catch?

I was thinking about a case where the bridge is defined first, like this:

network:
  bridges:
    some-bridge:
      interfaces: ["my-ovs-tunnel"]
      [...]
  tunnels:
    my-ovs-tunnel:
      openvswitch: {} # explicitly OVS
      [...]

IIRC this would parse the bridge and create a dummy netdef for my-ovs-tunnel. The dummy would then be filled with the actual data afterwards, when all the other NETPLAN_BACKEND_OVS handlers already ran. Thinking about that again, this case might actually by caught already my the multi-pass parsing... But I'm not 100% sure. We should be able to check this easily by having two test cases:

• One that defines the bridges first in YAML, tunnels afterwards
• One that defines the tunnels first in YAML, bridges afterwards.

[codecov-commenter]

Codecov Report

Merging #261 (a825bd6) into main (889c7be) will decrease coverage by 0.14%.
The diff coverage is 51.51%.

@@            Coverage Diff             @@
##             main     #261      +/-   ##
==========================================
- Coverage   99.07%   98.93%   -0.15%     
==========================================
  Files          61       61              
  Lines       10860    10881      +21     
==========================================
+ Hits        10760    10765       +5     
- Misses        100      116      +16     

Impacted Files	Coverage Δ
src/openvswitch.c	94.81% <6.66%> (-5.19%)	⬇️
src/parse.c	99.65% <33.33%> (-0.18%)	⬇️
src/networkd.c	100.00% <100.00%> (ø)
src/nm.c	100.00% <100.00%> (ø)
src/validation.c	99.60% <100.00%> (-0.01%)	⬇️
tests/generator/test_tunnels.py	100.00% <100.00%> (ø)

---

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 889c7be...a825bd6. Read the comment docs.

[Caligatio]

I've hit an issue with the following config:

network:
  version: 2
  renderer: networkd
  ethernets:
    enp0s3:
      dhcp4: yes
    enp0s8:
      dhcp4: no
      addresses:
        - 172.16.0.1/24
  tunnels:
    gre0:
      mode: gretap
      local: 172.16.0.1
      remote: 172.16.0.2
      key: 1000
  bridges:
    br0:
      interfaces: ["gre0"]
      addresses:
        - 10.0.0.1/24
      openvswitch: {}

Stepping through the code, it looks like the tunnel is defined with backend NETPLAN_BACKEND_NETWORKD first, the bridge gets defined, the interfaces handler (handle_bridge_interfaces) fires while the bridge's backend is still NETPLAN_BACKEND_NONE so the logic I added to handle_bridge_interfaces fails. As an end result, the tunnel is still NETPLAN_BACKEND_NETWORKD but the bridge is NETPLAN_BACKEND_OVS.

I thought about trying to call handle_bridge_interfaces from handle_ovs_backend when the node is a bridge but I don't have any reference to the interfaces node to use.

Given that the tunnel has no linkage to the bridge at the time the tunnel is defined and the bridge has no stateful reference to its members, I'm a bit stuck on how to proceed. Should I add a field to the netplan_net_definition struct that represents an element's members? If I do this, I can then add some logic to handle_ovs_backend to do what I need to do.

[slyon]

Interestingly, that test case seems to recognize gre0 as an OVS interface if the openvswitch: {} stanza is moved above the interfaces stanza of br0. So something bigger is going on here, probably with having an empty mapping ({}) at the end of the definition. I need to find some time to investigate this..

[Caligatio]

It makes me feel slightly better that you're also perplexed by this issue. Let me know if you need me to do anything!

[slyon]

Hey @Caligatio, I finally found some time to investigate the issue and understand what is going on now:

• tunnels.gre0 is parsed, gre0 backend is NONE
• bridges.br0 is parsed, backend is NONE initially
  • .interfaces are parsed, backend still NONE (therefore it cannot set the gre0 interface to OVS in handle_bridge_interfaces)
  • .addresses is parsed, backend still NONE
  • .openvswitch is parsed, br0 backend is changed to OVS, but it's too late to change the gre0 interface (we don't have a reference anymore)
• tunnels.gre0 backend falls back to NETWORKD

This also explains why it works if we move the openvswitch: {} above the definition of interfaces: [...] (the OVS backend is then already known when handling the child interfaces).

So instead of trying to get a reference during the parsing of individual interfaces, we could make use of the finish_iterator that is run after all interfaces have been parsed (c.f. slyon@fb80469):

diff --git a/src/parse.c b/src/parse.c
index d281a7f..b0d570c 100644
--- a/src/parse.c
+++ b/src/parse.c
@@ -1215,12 +1215,6 @@ handle_bridge_interfaces(NetplanParser* npp, yaml_node_t* node, const void* data
                 g_debug("%s: Bridge contains openvswitch interface, choosing OVS backend", npp->current.netdef->id);
                 npp->current.netdef->backend = NETPLAN_BACKEND_OVS;
             }
-            /* If the bridge has a OVS backend then any gretap or ip6gretap tunnels attached to the bridge must also be OVS */
-            if (component->type == NETPLAN_DEF_TYPE_TUNNEL &&
-                    (component->tunnel.mode == NETPLAN_TUNNEL_MODE_GRETAP || component->tunnel.mode == NETPLAN_TUNNEL_MODE_IP6GRETAP) &&
-                    npp->current.netdef->backend == NETPLAN_BACKEND_OVS) {
-                component->backend = NETPLAN_BACKEND_OVS;
-            }
         }
     }
 
@@ -2758,6 +2752,17 @@ netplan_parser_load_yaml(NetplanParser* npp, const char* filename, GError** erro
 static gboolean
 finish_iterator(const NetplanParser* npp, NetplanNetDefinition* nd, GError **error)
 {
+    /* If a bridge has a OVS backend then any gretap or ip6gretap tunnels attached to the bridge must also be OVS */
+    if (nd->type == NETPLAN_DEF_TYPE_TUNNEL && nd->bridge &&
+            (nd->tunnel.mode == NETPLAN_TUNNEL_MODE_GRETAP || nd->tunnel.mode == NETPLAN_TUNNEL_MODE_IP6GRETAP)) {
+        char *parent = nd->bridge;
+        NetplanNetDefinition *parent_nd = g_hash_table_lookup(npp->parsed_defs, parent);
+        if (parent_nd && parent_nd->backend == NETPLAN_BACKEND_OVS) {
+            g_debug("%s: setting backend to OVS implicitly (interface of %s)", nd->id, parent);
+            nd->backend = NETPLAN_BACKEND_OVS;
+        }
+    }
+
     /* Take more steps to make sure we always have a backend set for netdefs */
     if (nd->backend == NETPLAN_BACKEND_NONE) {
         nd->backend = get_default_backend_for_type(npp->global_backend, nd->type);

[gaby]

@slyon Any progress on getting this working? Thanks!

[slyon]

@slyon Any progress on getting this working? Thanks!

I'm not sure if @Caligatio is still interested in driving this forward? If not it would probably up for grabs, like for anybody else to continue working on this. I gave some review comments a while ago above.

[Caligatio]

Sorry for the delay in responding but I'm in the midst of an international move :)

I unfortunately am no longer in a position to work on this so it would be great if someone could pick this up where I left off.