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

Netplan status --diff refactoring #444

Merged
merged 7 commits into from Feb 29, 2024

Conversation

daniloegea
Copy link
Collaborator

@daniloegea daniloegea commented Feb 27, 2024

A few improvements for netplan status --diff:

  • route tables name to number (and vice-versa) mapping: take into account that the files from /etc/iproute2/ were moved to /usr/share in a recent release of iproute2. Fix unit tests to not open the real file.
  • when comparing MAC addresses, take into account the new options added to netplan recently. For now, we will not analyze the MAC address if an option such as random is used in the YAML.
  • expose the link-local property via a new API.
  • improve how link local addresses and routes are analyzed and take into account the link-local property. Note that currently it will not check if the system interface should have a link local address (in other words, it will not show a diff if link-local is enabled but the address is not present in the system). It will only check if the address is present and check the link-local property to see if the address is expected to be there.
  • Update the netplan-status manpage and autocomplete files.

Example of link-local changes:
In this case, link-local has the default configuration, which enables it only for IPv6, but the IPv4 link local address is also present so it will show up as a diff.

  ● 16: dummy123 dummy-device UNKNOWN/UP (NetworkManager: dummy123)
          MAC Address: 06:e7:f4:6e:c9:17
+           Addresses: 169.254.65.85/16 (link)
                       fe80::4e7:f4ff:fe6e:c917/64 (link)
+              Routes: 169.254.0.0/16 from 169.254.65.85 metric 550 (link)
                       224.0.0.0/4 metric 550 (static, link)
                       fe80::/64 metric 256

Description

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.

@daniloegea daniloegea force-pushed the netplan_status_diff_fixes2 branch 2 times, most recently from 41b8a47 to 48280b0 Compare February 28, 2024 11:35
@daniloegea daniloegea marked this pull request as ready for review February 28, 2024 12:21
@daniloegea
Copy link
Collaborator Author

Hi @rkratky, may I ask you to review the last commit of this PR, please? It's updating the netplan-status manual page. Thanks!

utils.route_table_lookup:
 - use the new path of the configuration files. It was changed recently
   in iproute2. It will fallback to the old path if the iproute2 version
   in the system is older.
 - change the dictionary to map names to numbers as well
 - if it fails to open the file, return the standard content found in
   rt_tables.
 - mock this method all over the tests so they will not try to open the
   real file.

state_diff._default_route_tables_name_to_number:
 - use utils.route_table_lookup instead of its own mapping
If the MAC address in Netplan is not a valid MAC (it can also be an
option now, such as 'random' and 'permanent'), don't try to diff it
against the system's MAC address.
The low level API has two new functions that will return a boolean
representing the internal state of the property. In the netdef, this
property is stored in an internal struct that contains two gboolean
variables. To avoid having to expose this struct I decided to separate
it in two functions.

In the Python code, the new propserty 'link_local' will return a list
containing ipv4 or ipv6 or an empty list to mimic the link-local
property in the YAML.
Instead of ignoring link local IPs and routes, check the netdef
'link-local' property to see if the they should be part of the diff.
It implemented a PoC of the tabular user interface that will not be used
anymore.
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.

I like the changes overall. Left some minor inline comments.

When testing the changes, it seems like IP-address diffing is broken after the link-local changes, though?
-> Sorry, configuration error on my end.

doc/netplan-status.md Outdated Show resolved Hide resolved
doc/netplan-status.md Outdated Show resolved Hide resolved
python-cffi/netplan/netdef.py Show resolved Hide resolved
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.

LGTM with the configuration error resolved.

Update the manpage and autocomplete files.
@slyon slyon merged commit 5fb1a81 into canonical:main Feb 29, 2024
14 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants