New 'netplan status' CLI by slyon · Pull Request #290 · canonical/netplan

slyon · 2022-08-31T09:56:15Z

Description

Implement new, initial netplan status command, according to spec FO049, which will show a system's current networking state, querying data from multiple backends (e.g. sd-networkd, NetworkManager, iproute2, DBus/sd-resolved, ...) and connecting the interface data with its corresponding Netplan/Netdef ID.

A new depdendency python3-rich is used for pretty printing the output to a terminal, in addition to structural JSON or YAML output. Usage:

usage: netplan status [-h] [--debug] [-a] [-f FORMAT] [ifname]

Query networking state of the running system

positional arguments:
  ifname                Show only this interface

options:
  -h, --help            show this help message and exit
  --debug               Enable debug messages
  -a, --all             Show all interface data (incl. inactive)
  -f FORMAT, --format FORMAT
                        Output in machine readable `json` or `yaml` format

Example output (non-pretty, as we're missing colors/bold text in this GitHub PR view):

     Online state: online
    DNS Addresses: 127.0.0.53 (stub)
       DNS Search: search.domain

●  2: enp0s31f6 ethernet UP (networkd: enp0s31f6)
      MAC Address: 54:e1:ad:5f:24:b4 (Intel Corporation)
        Addresses: 192.168.178.62/24 (dhcp)
                   2001:9e8:a19f:1c00:56e1:adff:fe5f:24b4/64
                   fe80::56e1:adff:fe5f:24b4/64 (link)
    DNS Addresses: 192.168.178.1
                   fd00::cece:1eff:fe3d:c737
       DNS Search: search.domain
           Routes: default via 192.168.178.1 from 192.168.178.62 metric 100 (dhcp)
                   192.168.178.0/24 from 192.168.178.62 metric 100 (link)
                   192.168.178.1 from 192.168.178.62 metric 100 (dhcp, link)
                   2001:9e8:a19f:1c00::/64 metric 100 (ra)
                   2001:9e8:a19f:1c00::/56 via fe80::cece:1eff:fe3d:c737 metric 100 (ra)
                   fe80::/64 metric 256
                   default via fe80::cece:1eff:fe3d:c737 metric 100 (ra)
  Activation Mode: manual

●  5: wlan0 wifi/"MYCON" UP (NetworkManager: NM-b6b7a21d-186e-45e1-b3a6-636da1735563)
      MAC Address: 1c:4d:70:e4:e4:0e (Intel Corporation)
        Addresses: 192.168.178.142/24
                   2001:9e8:a19f:1c00:7011:2d1:951:ad03/64
                   2001:9e8:a19f:1c00:f24f:f724:5dd1:d0ad/64
                   fe80::fec1:6ced:5268:b46c/64 (link)
    DNS Addresses: 192.168.178.1
                   fd00::cece:1eff:fe3d:c737
       DNS Search: search.domain
           Routes: default via 192.168.178.1 metric 600 (dhcp)
                   192.168.178.0/24 from 192.168.178.142 metric 600 (link)
                   2001:9e8:a19f:1c00::/64 metric 600 (ra)
                   2001:9e8:a19f:1c00::/56 via fe80::cece:1eff:fe3d:c737 metric 600 (ra)
                   fe80::/64 metric 1024
                   default via fe80::cece:1eff:fe3d:c737 metric 20600 (ra)

● 41: wg0 tunnel/wireguard UNKNOWN/UP (networkd: wg0)
        Addresses: 10.10.0.2/24
           Routes: 10.10.0.0/24 from 10.10.0.2 (link)
  Activation Mode: manual

● 48: tun0 tunnel/sit UNKNOWN/UP (networkd: tun0)
        Addresses: 2001:dead:beef::2/64
           Routes: 2001:dead:beef::/64 metric 256
  Activation Mode: manual

● 42: fakedev0 other DOWN (unmanaged)
           Routes: 10.0.0.0/16 via 10.0.0.1 (local)

1 inactive interfaces hidden. Use "--all" to show all.

The code in this branch can be executed like this:

sudo apt build-dep netplan.io
sudo apt install python3-rich
meson setup build --prefix=/usr
meson compile -C build
LD_LIBRARY_PATH=build/src/ PYTHONPATH=. src/netplan.script status

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

I think the most critical issue I've found is in the Interface class' constructor. It crashes in systems where network-manager is not installed, like LXD images (at least kinetic).

daniloegea · 2022-11-03T19:33:09Z

+import subprocess
+import sys
+import yaml
+import netplan.cli.utils as utils


nitpick: We probably should follow PEP8 and group imports in this order:

standard library import

third party imports

application imports

daniloegea · 2022-11-03T19:47:06Z

+        self.adminstate: str = 'UP' if 'UP' in ip['flags'] else 'DOWN'
+        self.operstate: str = ip['operstate'].upper()
+        self.macaddress: str = None
+        if 'address' in ip and len(ip['address']) == 17:  # 6 byte MAC


suggestion: Magic numbers look better as 'constants' with meaningful names, or we could even have a little private method returning the MAC address or None.

daniloegea · 2022-11-04T09:15:01Z

+    @property
+    def netdef_id(self) -> str:
+        if self.backend == 'networkd':
+            return self.nd.get('NetworkFile', '').split(


question: If the goal here is to get the interface name, wouldn't it be better to just use the Name attribute?

No. This is not about getting the interface name, but rather the Netplan netdef_id (which COULD be equal to the interface name, but can also be different). E.g. on my system I have this, where "usbC" is the NetdefID inside my netplan YAML config, while "lan0" is the interface name.

{ "Index" : 14, "Name" : "lan0", "AlternativeNames" : [], "Type" : "ether", "Driver" : "ax88179_178a", "SetupState" : "configured", "OperationalState" : "routable", "CarrierState" : "carrier", "AddressState" : "routable", "IPv4AddressState" : "routable", "IPv6AddressState" : "routable", "OnlineState" : "online", "NetworkFile" : "/run/systemd/network/10-netplan-usbC.network", "LinkFile" : "/run/systemd/network/10-netplan-usbC.link", "Path" : "pci-0000:00:0d.0-usb-0:3.2.1:1.0", "Vendor" : "ASIX Electronics Corp.", "Model" : "AX88179 Gigabit Ethernet" }

daniloegea · 2022-11-04T10:59:39Z

+                    if 'flags' not in extra or 'link' not in extra['flags']:
+                        non_local_ips.append(ip)
+                default_routes = [x for x in itf.routes if x.get('to', None) == 'default']
+                if len(non_local_ips) > 0 and len(default_routes) > 0 and len(itf.dns_addresses) > 0:


nitpick: empty lists will evaluate to False so this could be simplified to if list1 and list2 and list3.

daniloegea · 2022-11-04T11:24:06Z

+        dns_addr = ns.get('addresses', [])
+        dns_mode = ns.get('mode')
+        dns_search = ns.get('search', [])
+        if len(dns_addr) > 0:


nitpick: empty lists evaluate to False, could be simplified to if dns_addr:. There are few more instances like this across the code.

daniloegea · 2022-11-04T12:53:19Z

+        # due to hard package dependencies
+        iproute2 = self.query_iproute2()
+        networkd = self.query_networkd()
+        if not iproute2 or not networkd:


question: Is it expected to fail on systems that don't use networkd? It's stopping here on my system (standard kinetic upgraded from jammy).

systemd[-networkd] should be installed on any relevant system, as it is a dependency of netplan.io. IIRC I designed this new CLI in a way that it would assume networkd to be available... But maybe I should double-check if networkd is actually up and running on a system which does not necessarily depend on it (like a standard Ubuntu Desktop system).

Dang. I can confirm that systemd-networkd is installed but not running on a default Ubuntu Desktop installation. Let's see what I can do about that (maybe starting the service before trying to query it).

Checking the activity status of systemd-networkd.service and starting it if needed solves this issue. We assume systemd-networkd to be installed by default (it's part of the systemd package in Ubuntu).

One drawback: netplan status would be able to run without sudo privileges, if not for this reason of starting a system service... But maybe that's something to solve at a later time, as Netplan is a system/admin tool and requires root privileges for most of it's actions anyways.

Right. So, assuming sd-networkd is a hard dependency for netplan status, and I don't have a strong opinion on that, we should check first if we can start it. If it's masked for example netplan status will crash.

It would be nice to show a nice message like "netplan status depends on networkd" if we can't start it. A good thing is that once it's running one can call netplan status as a non-privileged user.

Or maybe we could just tell the user that networkd is not running and ask them to start it instead of trying to do that inside netplan...

I'd like to have it working "out of the box" on a default Ubuntu Desktop installation, that's why I'd like to automatically have it started if needed.

But checking if systemd-networkd.service is masked is a fair point. We'll now be bailing out if networkd is masked with an error message notifying the user to start networkd. If it's just disabled OTOH we'll try to start it automatically.

Netplan's 'dbus' tests module was in conflict with the system's global 'dbus' module (i.e. D-Bus bindings), leading to failing tests. We rename the tests to 'netplan_dbus' to avoid this. Cleanup pytest-3 deprecation warnings while on it

We make use of the previously assembled structural JSON data, to make sure we always present the same data in JSON/YAML and human readable format and don't miss anything in the structual output.

https://peps.python.org/pep-0008/#imports

slyon · 2023-01-04T14:14:43Z

Thank you very much for the review @daniloegea I think I addressed all of your remarks.

PTAL.

daniloegea

The output looks really nice. Now it's working on the scenarios I've tested before.

Found only 2 things we probably should address: we should at least check if networkd is not masked before trying to start it to avoid a crash and disable the conversion of sequences like :ab: to emojis.

daniloegea · 2023-01-04T15:44:43Z

+    def command(self):
+        networkd: str = subprocess.check_output(['networkctl', '--json=short'],
+                                                universal_newlines=True)
+        networkd_data = yaml.safe_load(networkd)


thought: hmm using the yaml module to load a JSON string, not a problem just counterintuitive. Anyway, the result of json.loads(networkd) is exactly the same.

daniloegea · 2023-01-04T18:02:19Z

+            logging.debug('Cannot query resolved DNS data: {}'.format(str(e)))
+        return (addresses, search)
+
+    def pretty_print(self, data: JSON, total: int, _console_width=None) -> None:


I've found by accident that sequences like :ab: and :1234: are being converted to emojis:

● 13: br123 bridge DOWN/UP (NetworkManager: br123) MAC Address: ee:21:ad:24:f9:71 Addresses: a🆎b🆎a🔢a:abcd/128 Routes: a🆎b🆎a🔢a:abcd metric 426

daniloegea · 2023-01-04T18:27:07Z

+                    self.routes.append(elem)
+
+        self.addresses: list = None
+        if 'addr_info' in ip and ip['addr_info']:


nitpick (no need to change): since python 3.8 this pattern can be simplified a bit with the := operator

if addr_info := ip.get('addr_info'): for addr in addr_info:

daniloegea · 2023-01-04T18:28:48Z

+        # due to hard package dependencies
+        iproute2 = self.query_iproute2()
+        networkd = self.query_networkd()
+        if not iproute2 or not networkd:


Or maybe we could just tell the user that networkd is not running and ask them to start it instead of trying to do that inside netplan...

slyon · 2023-01-05T10:19:50Z

Thanks for another sanity check and spotting those issues!

daniloegea

lgtm

Add python3-dbus and python3-rich[1] to RDEPENDS. [1] canonical/netplan#290 Signed-off-by: Yi Zhao <yi.zhao@windriver.com> Signed-off-by: Khem Raj <raj.khem@gmail.com>

slyon force-pushed the slyon/status branch 3 times, most recently from 917a3f1 to a44c6b6 Compare October 26, 2022 15:00

slyon marked this pull request as ready for review October 26, 2022 15:24

slyon requested a review from schopin-pro October 26, 2022 15:47

daniloegea suggested changes Nov 4, 2022

View reviewed changes

slyon force-pushed the slyon/status branch from a44c6b6 to add5849 Compare January 3, 2023 15:53

slyon added 19 commits January 4, 2023 15:02

tests: move conflicting dbus tests module

618c8ec

Netplan's 'dbus' tests module was in conflict with the system's global 'dbus' module (i.e. D-Bus bindings), leading to failing tests. We rename the tests to 'netplan_dbus' to avoid this. Cleanup pytest-3 deprecation warnings while on it

GA: add temporary python3-rich dependency

592f012

cli:status: initial skeleton

0a08c29

cli:status: query basic data

b9f7472

test:cli:status: basic data query tests

b3590db

cli:status: structure data into Interface() class, provide JSON output

0acdd25

test:cli:status: validate Interface() class, JSON output

f7b8339

cli:status: query global data, structural output

e5fa91b

test:cli:status: validate global data query and structural output

83b677b

cli:status: pretty print output, based on JSON data

940f0c1

We make use of the previously assembled structural JSON data, to make sure we always present the same data in JSON/YAML and human readable format and don't miss anything in the structual output.

test:cli:status: validate pretty printed output

1458382

tests: bundle CLI unit-tests

dd3a548

cli:status: update comments about networkctl --json improvements

7477004

cli:status: order imports according to PEP8

bb242e9

https://peps.python.org/pep-0008/#imports

cli:status: helper to extract MAC

71cc260

cli:status: simplify empty-list checks

3ad6bf4

cli:status: don't crash if NetworkManager is not available

6987726

meson: install setup cli

f8f0e05

cli:status: start systemd-networkd to query its data

444652f

slyon force-pushed the slyon/status branch from a6a2916 to 444652f Compare January 4, 2023 14:06

slyon requested review from daniloegea and removed request for schopin-pro January 4, 2023 14:14

daniloegea suggested changes Jan 4, 2023

View reviewed changes

slyon added 5 commits January 5, 2023 09:58

cli:status: avoid printing emojis

6aad1ae

cli:status: use json module to load JSON data

8b1229b

cli:status: simplify some assignments inside expressions

40d8252

cli:utils: add systemctl_is_masked helper

8feb126

cli:status: faile gracefully if systemd-networkd.service is masked

acae580

slyon requested a review from daniloegea January 5, 2023 11:25

daniloegea approved these changes Jan 5, 2023

View reviewed changes

slyon merged commit bb86353 into main Jan 5, 2023

slyon deleted the slyon/status branch January 5, 2023 15:47

Conversation

slyon commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

daniloegea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slyon commented Jan 4, 2023

Uh oh!

daniloegea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slyon commented Jan 5, 2023

Uh oh!

daniloegea left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

slyon commented Aug 31, 2022 •

edited

Loading