Add support for VXLAN tunnels (LP: #1764716)

[slyon]

Description

Adding support for VXLAN tunnels using the sd-networkd and NetworkManager backends.
It builds upon #272 (squashed & rebased), extracted VXLAN bits. Applied on top of #285.
Schema according to (internal) spec FO042.

The first 3 commits e83ea89 6d3597e 02c4f00 are pretty much unrelated to this PR. Those are drive-by fixes that I had been tracking in a separate branch, but the diff is fairly small, so I thought it'd make sense to include them into this PR instead of doing an extra one just for this.

Example:

network:
  ethernets:
    lo:
      addresses: [ 192.168.10.10/32 ]
  tunnels:
    vx1:
      mode: vxlan
      id: 1
      link: lo
      mtu: 8950
      accept-ra: no
      neigh-suppress: true
      mac-learning: false
      port: 4789
      local: 192.168.10.10
  bridges:
    br1:
      interfaces: [ vx1 ]

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

[slyon]

I think this is shaping up nicely for review now. @netdever may I ask for your review, please, as you have been the original author of this work, especially if the neigh-suppress part is still functioning as expected?

I've mostly rebased it on top of the new origin/main branch (like using NetplanTristate types), transformed your work from introducing a new vxlans interface type to using an additional tunnels.mode = vxlan tunnel type and adopted the YAML schema a bit. The new YAML schema is pre-approved according to (internal) spec FO042.

[netdever]

Awesome work! We're very excited for this. One quick note that at least in your example yml above, the mode parameter is missing, and this will result in an error:

/etc/netplan/50-cloud-init.yaml:57:13: Error in network definition: vx1: missing 'mode' property for tunnel
            id: 1

Regarding neigh-suppress, this must be present under the Bridge heading in the .network file when using systemd-networkd, otherwise it has no effect.

[Bridge]
NeighborSuppression=True

I initially handled this with:

    if (def->neigh_suppress) {
        g_string_append_printf(network, "\n[Bridge]\n");
        g_string_append_printf(network, "NeighborSuppression=%s\n", def->neigh_suppress ? "True" : "False");
    }

[slyon]

Thanks for your feedback @netdever I think I've now fixed both comments:
https://github.com/canonical/netplan/compare/1b2916bdbbc2272ed46409b05258b33c7a6cd6f4..c29c29f9c26f366a7e1899de7b9851813e284424

[netdever]

I have verified that neigh-suppress is now working as expected. This all looks good to me!

root@lab1:/home/ubuntu/netplan# cat /run/systemd/network/10-netplan-vx1.network | grep Neighbor -B1
[Bridge]
NeighborSuppression=true
root@lab1:/home/ubuntu/netplan# ip -d link show dev vx1 | grep -Po 'neigh_suppress \w+'
neigh_suppress on

[schopin-pro]

Solid work!

I have a few nitpicks here and there, and I'd really like to see the ABI question addressed before approving this PR. ("addressed" can very well be "not a problem, moving on", of course ;-) )

[slyon]

Solid work!

I have a few nitpicks here and there, and I'd really like to see the ABI question addressed before approving this PR. ("addressed" can very well be "not a problem, moving on", of course ;-) )

Thank you for the quality review!
I rebased on top of origin/main and addressed all of your comments, could you please double-check my "V2" commits (especially 6e8bc56)?

[schopin-pro]

Sadly, there's an issue with the bitfield name macro that needs to be solved before merging :/

[slyon]

TA! I updated 055a6e0 (V3), added 72774d8 and ae582f2

My initial idea was to use link as an universal/multi-purpose field for different types, but I think it makes sense to have the link filed as part of the NetplanVxlan struct and making it exclusive to VXLAN, especially if its only allocated if needed. It's easier to grasp this way, so I implemented that in ae582f2.

Could you have another look, especially about the dirty tracking in handle_vxlan_id_ref?

[slyon]

Could you have another look, especially about the dirty tracking in handle_vxlan_id_ref?

Thinking about it... dirty tracking doesn't make too much sense for the handle_netdef/vxlan_id_ref handlers, does it? Either we have a ID reference set, or not. So the default value is no value. So I think we can probably drop the mark_data_as_dirty() call in both of those handlers.

[schopin-pro]

Considering that the point of the dirty tracking is to be able to differentiate between default value and user input that happens to be the same as the default, the mechanism wouldn't work when said value is null, since the null handling would erase the user input before it hits the parsing code. So there's no need to do dirty tracking there.

Furthermore, does it actually make sense to have a vxlan entry without upstream link?

[slyon]

Furthermore, does it actually make sense to have a vxlan entry without upstream link?

Yes, there is a concept of "Independent" VXLAN interfaces: https://systemd.network/systemd.netdev.html#Independent=

[schopin-pro]

LGTM :)

[schopin-pro]

I'd say leave the dirty tracking in place. While I think it isn't used (yet?), it doesn't hurt either and has the merits of being consistent with other data.