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

Check if the name to set to an interface is already an altname (LP: #1994822) #292

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions netplan/cli/commands/apply.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import shutil
import netifaces
import time
import yaml

import netplan.cli.utils as utils
from netplan.configmanager import ConfigManager, ConfigurationError
Expand Down Expand Up @@ -60,6 +61,14 @@ def run(self): # pragma: nocover (covered in autopkgtest)
self.parse_args()
self.run_command()

@staticmethod
def get_alt_names(iface):
ret = subprocess.run(['ip', '-j', 'link', 'show', iface], capture_output=True)
data = yaml.safe_load(ret.stdout.decode('utf-8'))
if data and len(data) == 1:
return data[0].get('altnames', [])
return []

def command_apply(self, run_generate=True, sync=False, exit_on_error=True, state_dir=None): # pragma: nocover
config_manager = ConfigManager()
if state_dir:
Expand Down Expand Up @@ -237,6 +246,9 @@ def command_apply(self, run_generate=True, sync=False, exit_on_error=True, state
if iface in devices and new_name in devices_after_udev:
logging.debug('Interface rename {} -> {} already happened.'.format(iface, new_name))
continue # re-name already happened via 'udevadm test'
if new_name in NetplanApply.get_alt_names(iface):
logging.debug('Interface name {} already present as altname on {}.'.format(new_name, iface))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should at least be at "logging.warning" level, as Netplan was asked to rename the interface, but won't do it.

But I wonder if this is the correct thing to do at all (i.e. skipping the rename)?
Netplan was asked to rename the interface, so we probably shouldn't just skip it.
Maybe the better approach would be to delete the altname, then rename the interface to that name afterwards.
ip link property del altname NEW_NAME dev IFACE
ip link set dev IFACE name NEW_NAME

What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

In theory the altname works seamlessly as if it where the interface name. You can run commands using both names and should be the same. They exist to allow having names longer that 16 as the "main" name can't grow more than that without breaking compatibility. See here for the comments on the kernel patch that added them: https://lwn.net/Articles/794289/

That is why I think having the name in the altname should be essentially the same as having it in the name. Any commands that rely on the name of the interface being the string in the set-name parameter should work anyway. That's also why I treated it as the other case where the desired name was already present.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for your comment and further reference/context. After consulting some other netplan devs for a 2nd opinion, I feel like we should really go the extra mile here, i.e. drop the alt name and actually re-name the interface properly.

"working seamlessly in theory" isn't really enough, as we don't know the side-effects for edge cases. On the kernel commit (and during my testing) it showed that e.g. /sys/class/net/IFNAME/* is not yet supported/available, so we couldn't rely on the interface name anymore to query sysfs data. Also, the state of a running system vs re-booted system could diverge (netplan apply ignoring an interface rename at runtime, because an altname already exists VS the interface being actually renamed at the next reboot).

In addition to that, if netplan would add support for altnames in the future, we would have a looming conflict of real interface name vs altname here if we accept to ignore renaming interfaces if they're covered by an altname.

For those reasons I'd like to ask for changing the code to actually drop the altname and do the proper interface rename, instead.

Choose a reason for hiding this comment

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

The issue there is that the "rename the interface properly" will fail on case for which the altname was specifically created: interface names in excess of 16 characters.

The real purpose of the altname is to provide an interface name that raises the IFNAMSIZ (16) character limit of the "regular" name, because some interface names generated by the systemd persistent naming exceed 16 characters. If netplan attempts to transfer one of those names from altname to "regular" name, it will fail.

Also, what would netplan do for the case of multiple altnames?

Copy link
Author

Choose a reason for hiding this comment

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

Adding to the discussion, I tried changing this patch to delete the altname in lines 250-251 instead of just skipping the step of the loop. This works fine running netplan apply after booting but the interface does not get the altname as the "regular" name on boot. After booting the interface remains as before with the desired name as altname and a different "real" name. So this will also be a case where the could be differences between a running vs re-booted system

Copy link
Collaborator

Choose a reason for hiding this comment

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

The issue there is that the "rename the interface properly" will fail on case for which the altname was specifically created: interface names in excess of 16 characters.

The real purpose of the altname is to provide an interface name that raises the IFNAMSIZ (16) character limit of the "regular" name, because some interface names generated by the systemd persistent naming exceed 16 characters. If netplan attempts to transfer one of those names from altname to "regular" name, it will fail.

I don't think so, as we're not trying to transfer an arbitrary altname to a regular name, but we're only trying to rename an interface to the new name configured in Netplan's "set-name" setting, which is supposed to be a proper interface name and Netplan SHOULD actually validate this value fits the IFNAMESIZ (16) character limit (in validation.c); it doesn't currently validate that, though.

Also, what would netplan do for the case of multiple altnames?

We have the current interface name "OLD_NAME", the requested new interface name "NEW_NAME" and can have multiple altnames. We want to drop OLD_NAME and replace it by NEW_NAME. If NEW_NAME is already taken by an altname, we drop that specific altname and do the OLD_NAME -> NEW_NAME rename of the interface. We do not touch any other altnames, and keep them as is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding to the discussion, I tried changing this patch to delete the altname in lines 250-251 instead of just skipping the step of the loop. This works fine running netplan apply after booting but the interface does not get the altname as the "regular" name on boot. After booting the interface remains as before with the desired name as altname and a different "real" name. So this will also be a case where the could be differences between a running vs re-booted system

How are those altnames set-up in your case? Are they set by the kernel itself, i.e. are available directly at boot time?

The patch proposed in this PR handles the "rename at runtime" case only, via netplan apply cli. Renaming of interfaces at boot time happens through udev and we might need to update that code path accordingly as well (see networkd.c, write_rules_file())

Choose a reason for hiding this comment

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

I believe the altname in question is coming from systemd (udev), as /lib/systemd/network/99-default.link contains

NamePolicy=keep kernel database onboard slot path
AlternativeNamesPolicy=database onboard slot path

Maybe netplan is just a victim of the AlternativeNamesPolicy here?

Copy link
Author

Choose a reason for hiding this comment

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

As Jay said the altnames are added because of the AlternativeNamesPolicy. But systemd seems to take the same path as this patch when netplan's .link file specifies a name that is also an altname because of the policy (ie. it leaves the name as an altname). When a different name is configured in the .link file (one that is not an altname) the interface ¨regular" name gets updated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, sounds like Netplan is a victim of the AlternativeNamesPolicy. I need to read a bit more about the naming policies... I wonder if we could change those when writing a .link file for renaming the interface (but I remember vaguely those can only be applied once during bootup...)?

@drencrom Are you sure about systemd/udev behaving the same way? I think this might just be their "error case", see:
https://github.com/systemd/systemd/blob/main/src/libsystemd/sd-netlink/netlink-util.c#L33

What they're actually trying to do is to delete the AlternativeName, then renaming the interface (as suggested above). But this works only if the interface is DOWN and they are not supposed to bring down the interface themselves... So they keep the name in case the interface is UP already.

Netplan OTOH is in control of the networking stack and can cycle an interface DOWN/UP if needed, so should do the right think and clear the alternative name, to set it as a proper new interface name.

For reference, see also: systemd/systemd#25221 (Ubuntu Foundations currently working on a similar issue with upstream systemd)

continue # name already present in interface
# bring down the interface, using its current (matched) interface name
subprocess.check_call(['ip', 'link', 'set', 'dev', iface, 'down'],
stdout=subprocess.DEVNULL,
Expand Down
18 changes: 18 additions & 0 deletions tests/test_cli_units.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,24 @@ def test_is_composite_member_with_renderer(self):
res = NetplanApply.is_composite_member([{'renderer': 'networkd', 'br0': {'interfaces': ['eth0']}}], 'eth0')
self.assertTrue(res)

@patch('subprocess.run')
def test_get_alt_names(self, mock):
stdout_mock = mock.Mock()
stdout_mock.stdout = '[{"ifindex":3,"ifname":"ens4","flags":["BROADCAST","MULTICAST","UP","LOWER_UP"],'\
'"mtu":8958,"qdisc":"fq_codel","operstate":"UP","linkmode":"DEFAULT","group":"default",'\
'"txqlen":1000,"link_type":"ether","address":"xx:xx:xx:xx:xx:xx",'\
'"broadcast":"ff:ff:ff:ff:ff:ff","altnames":["enp0s4","enp0s41"]}]'.encode('utf-8')
mock.return_value = stdout_mock
res = NetplanApply.get_alt_names('ens4')
mock.assert_called_with(['ip', '-j', 'link', 'show', 'ens4'], capture_output=True)
self.assertEquals(res, ['enp0s4', 'enp0s41'])

mock.reset_mock()
stdout_mock.stdout = ''.encode('utf-8')
res = NetplanApply.get_alt_names('ens4')
mock.assert_called_with(['ip', '-j', 'link', 'show', 'ens4'], capture_output=True)
self.assertEquals(res, [])

@patch('subprocess.check_call')
def test_clear_virtual_links(self, mock):
# simulate as if 'tun3' would have already been delete another way,
Expand Down