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.

Sign up for GitHub

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

Jump to bottom

Check if the interface name is too long (LP: #1988749) #313

Merged
merged 7 commits into from
Jan 19, 2023
Merged

Check if the interface name is too long (LP: #1988749) #313

merged 7 commits into from
Jan 19, 2023

Conversation

daniloegea
Copy link
Collaborator

@daniloegea daniloegea commented Jan 17, 2023 •
edited by slyon
Loading

Description

Netplan will allow the user to enter an interface name that's too long without issues any warning message. The backends will fail to process such an interface with errors like:

Jan 17 08:37:31 ubuntu2204 systemd-udevd[176]: eth0: Invalid network interface name, ignoring: anotherverylongname0

Jan 17 14:17:53 ubuntu2204 systemd-networkd[1019]: /run/systemd/network/10-netplan-notagoodnameforabridge0.netdev:2: Interface name is not valid or too long, ignoring assignment: notagoodnameforabridge0
Jan 17 14:17:53 ubuntu2204 systemd-networkd[1019]: NetDev without Name= configured in /run/systemd/network/10-netplan-notagoodnameforabridge0.netdev. Ignoring

Also, netplan apply will crash when trying to rename the interface:

oot@ubuntu2204:~# netplan apply
Traceback (most recent call last):
  File "/usr/sbin/netplan", line 23, in <module>
    netplan.main()
  File "/usr/share/netplan/netplan/cli/core.py", line 50, in main
    self.run_command()
  File "/usr/share/netplan/netplan/cli/utils.py", line 231, in run_command
    self.func()
  File "/usr/share/netplan/netplan/cli/commands/apply.py", line 61, in run
    self.run_command()
  File "/usr/share/netplan/netplan/cli/utils.py", line 231, in run_command
    self.func()
  File "/usr/share/netplan/netplan/cli/commands/apply.py", line 245, in command_apply
    subprocess.check_call(['ip', 'link', 'set',
  File "/usr/lib/python3.10/subprocess.py", line 369, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['ip', 'link', 'set', 'dev', 'eth0', 'name', 'anotherverylongname0']' returned non-zero exit status 255

Ideally we should fail on such cases, but at this point some users might have invalid names in their configuration so it would cause problems.

It addresses LP#1988749

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 slyon changed the title Check if the interface name is too long Check if the interface name is too long (LP: #1988749) Jan 18, 2023
daniloegea and others added 7 commits January 19, 2023 12:57
@daniloegea @slyon
tests: fix a keyfile test logic for bridges
db57b90 
For bridges, the emitter will use the interface name as ID and not the
NetworkManager UUID.
@daniloegea @slyon
parser: warning if the interface name is too long
cf3a1aa 
Interfaces names are limited to 15 characters (see IF_NAMESIZE in
net/if.h). Currently Netplan will allow the user to use invalid names
without a warning. In this case the operation will be ignored by the
backends.

With this change, Netplan will show a warning but still complete
successfully. As there might be users with invalid interface names in
their configuration without knowing it, changing the behavior to fail
would probably cause problems.
@daniloegea @slyon
validation: check if netdef is NULL
333fae0 
Also, fix apply.py code style to make the linter happy.
@daniloegea @slyon
ctests: add tests for the new validation function
31f3c7a 
Also add a utility to load the yaml content from strings.
@slyon
validation: add a comment about IF_NAMSIZE warning
f26dff4
@slyon
nm: replace undocumented constant with IF_NAMESIZE
892ac81
@slyon
ctest:utils: adopt to netplan_error_clear rename
f6374fb
slyon
slyon approved these changes Jan 19, 2023
View reviewed changes
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

I agree we should only print a warning for now. As a hard failure could break the netplan generator at boot time and leave people without any network connectivity.

I've added a comment about this in the code and adopted nm.c to make use of the same validation logic, instead of using an undocumented constant there.

I'll also rebase and adopt to the netplan_error_clear() rename from #317

@slyon slyon force-pushed the max_iface_name_len branch from 9b4c5f7 to f6374fb Compare January 19, 2023 12:01
@slyon slyon merged commit d53d96c into canonical:main Jan 19, 2023
@slyon
Copy link
Collaborator

slyon commented Jan 24, 2023

Afterthought: Actually, there might be use cases for longer interface names, using "alternative names", once those are properly supported by netplan, cf.: #292

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slyon slyon slyon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@daniloegea @slyon