Add json parsing of ip addr show (SC-723) by TheRealFalcon · Pull Request #1210 · canonical/cloud-init

TheRealFalcon · 2022-01-24T21:34:47Z

Proposed Commit Message

Add json parsing of ip addr show

When obtaining information from "ip addr", default to using
"ip --json addr" rather than using regex to parse "ip addr show"
as json is machine readable as less prone to error.

Deprecate but leave fallback to use "ip addr" for older iproute2
tooling which does not support --json param. 

Fix regex parsing of "ip addr" to support peer addresses and
metrics.

Additional Context

#1183

Test Steps

Checklist:

My code follows the process laid out in the documentation
I have updated or added any unit tests accordingly
I have updated or added any documentation accordingly

When obtaining information from "ip addr", default to using "ip --json addr" rather than using regex to parse "ip addr show" as json is machine readable as less prone to error.

TheRealFalcon · 2022-01-24T21:36:45Z

-    maxDiff = None
-    with_logs = True
-
+class TestNetInfo:


Removed base class from these tests. Changes in this file that are related to the changed are the newly parametrized ones.

blackboxsw

Minimally minor docstring change and maybe more defensive "flags" handling, which will trigger CI now that we have credits.

I found a bug in handle peer or point to point type devices in _netdev_info_iproute that you might want to take into account in this PR or minimally understand for our JSON parsing

blackboxsw · 2022-01-25T20:28:29Z

+    devs = {}
+
+    for dev in ipaddr_data:
+        flags = dev["flags"]


Given that we aren't catching KeyErrors, should be be a bit more defensive here given at least on my local system the JSON output seems to have a number of optional keys that don't seem to exist on all devices:

master

link_index

address

broadcast

link_netnsid

Suggested change

flags = dev["flags"]

flags = dev["flags"] if "flags" in dev else []

I perhaps took this recommendation a bit too far 🙂

blackboxsw · 2022-01-25T21:03:16Z

+            "ipv6": [],
+        }
+        for addr in dev["addr_info"]:
+            if addr["family"] == "inet":


Generally the logic here looks good and mirrors what we have in _netdev_info_iproute. I can also note that on my laptop your JSON output handling actually fixes an existing bug in properly parsing inet6 lines from ip addr show.

We have a gap though and I think it's worth adding a unittest that validatates that comparable output from ip --json addr show and ip addr show gets the same resulting dict from _netdev_info_iproute_json and _netdev_info_iproute respectively.

When I try running the following asserts on my laptop given both ip addr show and ip --json addr show output I can see a failure in `_netdev_info_iproute to parse inet6 lines leaving that value empty.

from cloudinit.netinfo import _netdev_info_iproute_json, _netdev_info_iproute json_dict = _netdev_info_iproute_json(ip_show_json) orig_dict = _netdev_info_iproute(ip_show_out) assert json_dict.keys() == orig_dict.keys() for k in orig_dict.keys(): if orig_dict[k] != json_dict[k]: assert orig_dict[k].keys() == json_dict[k].keys() for subk in orig_dict[k].keys(): assert orig_dict[k][subk] == json_dict[k][subk], f"{k}.{subk} not equal" $ python3 ./test.py AssertionError: tun0.ipv6 not equal

Unrelated to this PR, but the failures I see locally on Focal are due to this line not matching our regex
' inet6 2001:67c:1562:8007::aac:41aa peer 2001:67c:1562:8007::1/64 scope global noprefixroute '

m = re.match( r"\s+inet6\s(?P<ip>\S+)\sscope\s(?P<scope6>\S+).*", line )

I think we may need to fix our regex to also handle optional "(peer\s(?P<peer_ip>\S+)?". Not sure if you want to tackle that in this PR or not.

Works on my machine! Ship it!

But in all seriousness, do we want to maintain both implementations? The whole point of the previous PR was to fix an issue where I was seeing the metric field on the inet line. It's not actually a new field, just jammy defaults to using it. So should we be fixing that in the regex as well?

@TheRealFalcon agreed we don't want to maintain both implementations. This is seen as an existing bug on distros with older iproute2. I don't think it's worth the regex fix the older version parsing. So yeah, let's forget about that because 99% of our distributiions which carry an ip tool should support the --json flag.

Heh...I recently pushed the commit to fix it. I think at this point it's easier to just keep it and not update it anymore going forward.

blackboxsw · 2022-01-25T21:26:24Z

+                dev_info["ipv4"].append(parsed_addr)
+            elif addr["family"] == "inet6":
+                parsed_addr = {
+                    "ip": f"{addr['local']}/{addr['prefixlen']}",


A note that tun0 for PTP link_type: "none" will provide noprefixroute: true which I think means you shouldn't add the prefix suffix on the local IP in this case as it only applies to the "peer" link.
"""
If a peer address is specified, the local address cannot
have a prefix length. The network prefix is associated
with the peer rather than with the local address
"""
from https://man7.org/linux/man-pages/man8/ip-address.8.html

Here's the output from --json for my tun0 interface:

{ "ifindex": 15, "ifname": "tun0", "flags": [ "POINTOPOINT", "MULTICAST", "NOARP", "UP", "LOWER_UP" ], "mtu": 1500, "qdisc": "fq_codel", "operstate": "UNKNOWN", "group": "default", "txqlen": 100, "link_type": "none", "addr_info": [ { "family": "inet", "local": "10.172.65.170", "prefixlen": 18, "broadcast": "10.172.127.255", "scope": "global", "noprefixroute": true, "label": "tun0", "valid_life_time": 4294967295, "preferred_life_time": 4294967295 }, { "family": "inet6", "local": "2001:67c:1562:8007::aac:41aa", "address": "2001:67c:1562:8007::1", "prefixlen": 64, "scope": "global", "noprefixroute": true, "valid_life_time": 4294967295, "preferred_life_time": 4294967295 } ] }

I think what we want here is "ip": "2001:67c:1562:8007::aac:41aa" instead of "2001:67c:1562:8007::aac:41aa/64"
or better handing of link_type: "none"

I don't think that's what noprefixroute: true means. That's related to the kernel automatically creating a default route for this address. I think you're right though that if there is a separate address and local, that the local shouldn't display a mask.

I also wonder if 'link_type' is none if we should include the address at all or perhaps figure out a way to show display it differently in the net device info / route info tables we display from this info.

blackboxsw

Suggested patch to maybe fix _netdev_info_iproute for parsing inet6 lines containing "peer" type info

diff --git a/cloudinit/netinfo.py b/cloudinit/netinfo.py
index b855bc3aa..9acbdb99e 100644
--- a/cloudinit/netinfo.py
+++ b/cloudinit/netinfo.py
@@ -110,7 +110,7 @@ def _netdev_info_iproute(ipaddr_out):
             }
         elif "inet6" in line:
             m = re.match(
-                r"\s+inet6\s(?P<ip>\S+)\sscope\s(?P<scope6>\S+).*", line
+                r"\s+inet6\s(?P<ip>\S+)\s((peer\s\S+)\s)?scope\s(?P<scope6>\S+).*", line
             )
             if not m:
                 LOG.warning(

TheRealFalcon · 2022-01-26T20:28:27Z

@blackboxsw , I added a few commits based on your comments.

We have a gap though and I think it's worth adding a unittest that validatates that comparable output from ip --json addr show and ip addr show gets the same resulting dict from _netdev_info_iproute_json and _netdev_info_iproute respectively.

That's essentially what the parametrization of test_netdev_iproute_pformat is doing. I added the metric parsing code, and I think we can add your examples with the point-to-point addresses. I don't have anything like this handy on my machine, so do you mind providing an ip addr show version in addition to the json version you posted above? I don't think it makes sense to have a unit test that directly compares ip output on the machine it's on, because our unit tests should be able to run on machines that don't use iproute2 utilities and shouldn't be machine dependent.

blackboxsw

+1 thanks for the rework here and over-defensive handling of ip --json output. Let's plan to drop the deprecated non --json handling in next upstream release 22.2

blackboxsw · 2022-01-26T21:47:11Z

+                # If a peer address is specified, the local address cannot
+                # have a prefix length. The network prefix is associated
+                # with the peer rather than with the local address.
+                if ip and not addr.get("address"):


+1 thanks for this clarification here + comments

blackboxsw · 2022-01-26T21:48:30Z

            m = re.match(
-                r"\s+inet\s(?P<cidr4>\S+)(\sbrd\s(?P<bcast>\S+))?\sscope\s"
-                r"(?P<scope>\S+).*",
+                r"\s+inet\s(?P<cidr4>\S+)"


Nice, since you've added it, let's keep it. But yes no need to maintain both long-term. Ultimately, let's plan on dropping this function in 22.2 upstream release

TheRealFalcon

~~deleted comment~~

Add json parsing of ip addr show

956e9c8

When obtaining information from "ip addr", default to using "ip --json addr" rather than using regex to parse "ip addr show" as json is machine readable as less prone to error.

TheRealFalcon mentioned this pull request Jan 24, 2022

Update parsing of ip addr show (SC-723) #1183

Closed

3 tasks

TheRealFalcon commented Jan 24, 2022

View reviewed changes

blackboxsw requested changes Jan 25, 2022

View reviewed changes

blackboxsw reviewed Jan 25, 2022

View reviewed changes

be defensive with keys

a309366

blackboxsw self-assigned this Jan 25, 2022

TheRealFalcon added 4 commits January 26, 2022 12:48

ignore mask for ipv6 point-to-point

a135cf4

update regexes we know are broken (hopefully for the last time)

a135f2e

Add metric to netinfo test samples

cb28c05

regex is hard

89fd664

blackboxsw approved these changes Jan 26, 2022

View reviewed changes

TheRealFalcon commented Jan 26, 2022

View reviewed changes

additional test and fix found by test

ab6bb93

TheRealFalcon merged commit 223b23e into canonical:main Jan 27, 2022

TheRealFalcon deleted the ip-json branch January 27, 2022 01:40

ubuntu-server-builder mentioned this pull request May 12, 2023

Release 22.1 #3948

Closed

	flags = dev["flags"]
	flags = dev["flags"] if "flags" in dev else []

Conversation

TheRealFalcon commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Commit Message

Additional Context

Test Steps

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blackboxsw left a comment

Choose a reason for hiding this comment

Uh oh!

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!

blackboxsw left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealFalcon commented Jan 26, 2022

Uh oh!

blackboxsw 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!

TheRealFalcon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

TheRealFalcon commented Jan 24, 2022 •

edited

Loading

TheRealFalcon left a comment •

edited

Loading