Match by PermanentMACAddress

[rlaager]

Description

Using bond, bridge, and VLAN devices results in sharing MAC addresses across interfaces. Matching by MACAddress fails in two ways. It will overmatch by matching all the interfaces (physical and virtual) that share that MAC address. It will undermatch by no longer matching the physical interface once its MAC address has been changed.

Matching by PermanentMACAddress fixes both of those issues.

This replaces the old "workaround" of setting Type=!vlan bond bridge.

That code had a comment saying, "something outputs netplan config that includes using the MAC of the first phy member of a bond as default value for the MAC of the bond device itself." I think this was a misunderstanding of what was going on there. In both examples, the configuration was attempting to match a physical interface by MAC address, not provide a MAC address to set on the virtual device.

These are the two Launchpad bugs referenced by the code:
https://bugs.launchpad.net/netplan/+bug/1804861
https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1888726

The comment continued, "This is evil, it's an optional field and networkd knows what to do if the MAC isn't specified". This configuration is perfectly reasonable. It is in fact required, as explained in the netplan documentation, if one wants to use settings that apply in the .link file (wakeonlan or the offload settings). This is because networkd refuses to apply a .link file that matches by name if the interface has already been renamed. At a minimum, this means that matching by MAC address is required if one wants to be able to change the settings after boot (on interfaces that are renamed). If the interfaces are renamed at early boot, before the netplan configuration applies, matching by MAC address is required for those settings to apply at all.

For my example, see:
#270 (comment)
#270 (comment)

Arguably, netplan should always use PermanentMACAddress, at least for physical interfaces. But that change might have unintended consequences, so I opted for the more minimal change of only using it in these complex scenarios (which are already special-cased).

Checklist

• Runs make check successfully.
• Retains 100% existing 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.

[codecov-commenter]

Codecov Report

Merging #278 (7d5c59d) into main (6a47c70) will decrease coverage by 0.12%.
The diff coverage is 100.00%.

❗ Current head 7d5c59d differs from pull request most recent head 5209e82. Consider uploading reports for the commit 5209e82 to get more accurate results

@@            Coverage Diff             @@
##             main     #278      +/-   ##
==========================================
- Coverage   99.19%   99.07%   -0.12%     
==========================================
  Files          61       61              
  Lines       11206    11193      -13     
==========================================
- Hits        11116    11090      -26     
- Misses         90      103      +13     

Impacted Files	Coverage Δ
tests/generator/test_common.py	100.00% <ø> (ø)
tests/generator/test_vlans.py	100.00% <ø> (ø)
src/networkd.c	100.00% <100.00%> (ø)
src/abi_compat.c	79.03% <0.00%> (-20.97%)	⬇️
src/nm.c	100.00% <0.00%> (ø)
src/generate.c	100.00% <0.00%> (ø)
tests/generator/base.py	100.00% <0.00%> (ø)
tests/generator/test_auth.py	100.00% <0.00%> (ø)
tests/parser/test_keyfile.py	100.00% <0.00%> (ø)
netplan/cli/commands/apply.py	100.00% <0.00%> (ø)
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[daniloegea]

I'm inclined to agree that we should use PermanentMACAddress by default for systemd-networkd. That's the default NetworkManager's behavior. But as was mentioned in the comments it might be dangerous as someone might be taking advantage of the current behavior in some scenario.

Including a new option such as permanentmacaddress seems to be the safest option. For NetworkManager it would emit the same configuration as macaddress.

@slyon what do you think?

[nicolasbock]

The other obvious use case for a new PermanentMacAddress option is to match on devices that have their macaddress changed, i.e.

network:
    version: 2
    ethernets:
        ens4:
            set-name: ens4
            dhcp4: true
            match:
                permanentmacaddress: 52:54:00:c4:51:a2
            macaddress: 52:54:00:c4:51:a3

The above also works without a new option but for more complex configurations it might become necessary to use the permanent address here.

[slyon]

I do not like the idea of adding a new permanentmacaddress stanza, too much, as that will basically put the complexity of this decision onto the user. But we rather want to reduce complexity.

I'm inclined to use PermanentMACAddress= by default, especially if this is the way it is handled for the NetworkManager backend, too [1]. We want to keep the behavior between our backends as similar as possible. Only in rare cases could this impact a user's network connectivity on update, if they have been relying on this "bug".

We could also go with @rlaager 's current solution, replacing our current workaround, and go full "PermanentMACAddress=" in the future.

[1] https://developer-old.gnome.org/NetworkManager/unstable/settings-802-3-ethernet.html :

mac-address (Alias: mac)
If specified, this connection will only apply to the Ethernet device whose permanent MAC address matches.

PS: I rebased this branch to make it re-run the autopkgtests, so we're able to check if the failure is still there (logs had been outdated).

[slyon]

Ok, talking to some other Netplan folks, I feel like we should go full PermanentMACAddress= right now and mention it boldly in the release notes.

Also we need to check and fix this integration test failure:

======================================================================
FAIL: test_bridge_mac (__main__.TestNetworkd)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/autopkgtest.soH2fh/build.7vl/real-tree/tests/integration/bridges.py", line 274, in test_bridge_mac
    self.generate_and_settle([self.dev_e_client, self.state_dhcp4('br0')])
  File "/tmp/autopkgtest.soH2fh/build.7vl/real-tree/tests/integration/base.py", line 321, in generate_and_settle
    self.networkd_wait_connected(iface, 60)
  File "/tmp/autopkgtest.soH2fh/build.7vl/real-tree/tests/integration/base.py", line 379, in networkd_wait_connected
    self.wait_output(['networkctl', 'status', iface], '(configured', timeout)
  File "/tmp/autopkgtest.soH2fh/build.7vl/real-tree/tests/integration/base.py", line 372, in wait_output
    self.fail('timed out waiting for "{}" to appear in {}'.format(expected_output, cmd))
AssertionError: timed out waiting for "(configured" to appear in ['networkctl', 'status', 'eth42']

----------------------------------------------------------------------
Ran 21 tests in 195.019s

FAILED (failures=1, skipped=1)
autopkgtest [16:29:35]: test bridges: -----------------------]
autopkgtest [16:29:35]: test bridges:  - - - - - - - - - - results - - - - - - - - - -
bridges              FAIL non-zero exit status 1

[slyon]

Well... one issue that came up during testing is that PermanentMACAddress= does not apply to veth type devices. Netplan does not support veths, though (LP#1876730), but we're still using them in our integration tests. This is why the test_bridge_mac (and potentially test_rename_interfaces) test is failing.

It's trying to match its ethbr parent interface by (permanent) MAC. But ethbr is our eth42 test device, which is of veth type. We can possibly work around this in the test suite, but need to be aware of this limitation (and update the docs). It has always been this way on NetworkManager and one should have other means to match a manually created veth device (e.g. by name).

network:
  renderer: %(r)s
  ethernets:
    ethbr:
      match:
        name: %(ec)s
        macaddress: %(ec_mac)s
  bridges:
    br0:
      interfaces: [ethbr]
      macaddress: "00:01:02:03:04:05"
      dhcp4: yes

[slyon]

After my recent changes, this lgtm.
Let's see if the integration tests are passing now, too.

[daniloegea]

It looks good.

PermanentMACAddress also accepts the 20 bytes Infiniband MAC address apparently (but I couldn't really test if the match works for infiniband).

[rlaager]

I confirmed this worked as expected on my real world system. Specifically, I applied this PR's diff on top of the Ubuntu package (netplan.io-0.104-0ubuntu2~20.04.2), built it, and installed it. With the configuration in the desired state, with the stock netplan.io package, networkd did not apply the offload option on boot. With the patched netplan.io package, it did apply the offload option (both immediately and on boot).

[slyon]

Thanks for the confirmation @rlaager! Also, thanks for your +1 @daniloegea. Wrt. IPoIB (Infiniband): I spotted that as well (and had no way to test it), but at least for NetworkManager the behavior seems to be the same as for other physical devices (which are the only ones that Netplan's match stanza applies to). Let's get this merged.

infiniband.mac-address [1]:
If specified, this connection will only apply to the IPoIB device whose permanent MAC address matches. This property does not change the MAC address of the device (i.e. MAC spoofing).

[1] https://developer-old.gnome.org/NetworkManager/stable/settings-infiniband.html

[slyon]

permanentMAC support is missing in python3-netifaces, which we use for CLI al45tair/netifaces#66 but this shouldn't affect this implementation on the generator level.

[CpServiceSpb]

I need to change as interface name as MAC address.
And potential permanentmacaddress in Match section of Netplan is the single good way without put it to .link files.

[mklassen]

This patch causes issue in LXD containers that use netplan. Because most of the interfaces provided to the container are veths matching on MAC address within the containers no longer works.

[rlaager]

@mklassen Perhaps the answer is to conditionalize on veth and use MACAddress for those?

[slyon]

interfaces provided to the container are veths matching on MAC address within the containers no longer works.

Yes. Netplan's match stanza was never supposed to be used on virtual interfaces (like veth's) in the first place. On the NetworkManager backend the behavior was consistent with the documentation. On systemd-networkd it was a bit more lax, which got fixed by this PR. It's now functioning as it is supposed to.

See also:
https://bugs.launchpad.net/netplan/+bug/2022947
https://netplan.readthedocs.io/en/stable/netplan-yaml/#properties-for-physical-device-types