Skip to content

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

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

Closed
TheRealFalcon wants to merge 2 commits into from
+82 −71
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aeb93b6
Update parsing of ip addr show
TheRealFalcon Jan 11, 2022
4963ea6
thanks pylint
TheRealFalcon Jan 11, 2022
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
6 changes: 4 additions & 2 deletions cloudinit/netinfo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,10 @@ def _netdev_info_iproute(ipaddr_out):
devs[dev_name]["ipv6"].append(m.groupdict())
elif "inet" in line:
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+)"
r"(\smetric\s(?P<metric>\d+))?"
Copy link
Copy Markdown
Member

@holmanb holmanb Jan 11, 2022

Choose a reason for hiding this comment

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

I did a little bit of manual testing with this regex and didn't spot any issues.

Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw Jan 13, 2022

Choose a reason for hiding this comment

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

The bigger this regex gets, the more it makes me question whether we should really be extending a fragile regex parsing of human-readable output of ip. While I originally weighed in to the contrary because cloud-init is supported even more broadly than just Ubuntu, I think it would probably be preferable given that we attempt to try ip -json format first, and fallback to the legacy parsing if -json param is not present. This way most of the latest *nix distros with recent ip tooling can benefit from simple parsing of machine-readable output and any older distributions will continue to parse correctly with the older ip tooling and output formats.

r"(\sbrd\s(?P<bcast>\S+))?"
r"\sscope\s(?P<scope>\S+).*",
line,
)
if not m:
Expand Down
12 changes: 12 additions & 0 deletions tests/data/netinfo/sample-ipaddrshow-output-metric
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
inet 127.0.0.1/8 scope host lo\ valid_lft forever preferred_lft forever
Copy link
Copy Markdown
Member

@holmanb holmanb Jan 11, 2022

Choose a reason for hiding this comment

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

Are the backslashes on lines 3 and 4 a typo?

Copy link
Copy Markdown
Contributor Author

@TheRealFalcon TheRealFalcon Jan 11, 2022

Choose a reason for hiding this comment

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

I just copied the sample-ipaddrshow-output file and added the metric part

Copy link
Copy Markdown
Member

@holmanb holmanb Jan 11, 2022

Choose a reason for hiding this comment

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

I see, nevermind. It doesn't matter what it says after scope since the regex ends with .*.

inet6 ::1/128 scope host \ valid_lft forever preferred_lft forever
2: enp0s25: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP mode DEFAULT group default qlen 1000
link/ether 50:7b:9d:2c:af:91 brd ff:ff:ff:ff:ff:ff
inet 192.168.2.18/24 metric 100 brd 192.168.2.255 scope global dynamic enp0s25
valid_lft 84174sec preferred_lft 84174sec
inet6 fe80::7777:2222:1111:eeee/64 scope global
valid_lft forever preferred_lft forever
inet6 fe80::8107:2b92:867e:f8a6/64 scope link
valid_lft forever preferred_lft forever
135 changes: 66 additions & 69 deletions tests/unittests/test_netinfo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
# This file is part of cloud-init. See LICENSE file for license information.

"""Tests netinfo module functions and classes."""

from copy import copy

import pytest

from cloudinit.netinfo import netdev_info, netdev_pformat, route_pformat
from tests.unittests.helpers import CiTestCase, mock, readResource
from tests.unittests.helpers import mock, readResource

# Example ifconfig and route output
SAMPLE_OLD_IFCONFIG_OUT = readResource("netinfo/old-ifconfig-output")
SAMPLE_NEW_IFCONFIG_OUT = readResource("netinfo/new-ifconfig-output")
SAMPLE_FREEBSD_IFCONFIG_OUT = readResource("netinfo/freebsd-ifconfig-output")
SAMPLE_IPADDRSHOW_OUT = readResource("netinfo/sample-ipaddrshow-output")
SAMPLE_IPADDRSHOW_OUT_METRIC = readResource(
"netinfo/sample-ipaddrshow-output-metric"
)
SAMPLE_ROUTE_OUT_V4 = readResource("netinfo/sample-route-output-v4")
SAMPLE_ROUTE_OUT_V6 = readResource("netinfo/sample-route-output-v6")
SAMPLE_IPROUTE_OUT_V4 = readResource("netinfo/sample-iproute-output-v4")
Expand All @@ -21,19 +25,15 @@
FREEBSD_NETDEV_OUT = readResource("netinfo/freebsd-netdev-formatted-output")


class TestNetInfo(CiTestCase):

maxDiff = None
with_logs = True

class TestNetInfo:
@mock.patch("cloudinit.netinfo.subp.which")
@mock.patch("cloudinit.netinfo.subp.subp")
def test_netdev_old_nettools_pformat(self, m_subp, m_which):
"""netdev_pformat properly rendering old nettools info."""
m_subp.return_value = (SAMPLE_OLD_IFCONFIG_OUT, "")
m_which.side_effect = lambda x: x if x == "ifconfig" else None
content = netdev_pformat()
self.assertEqual(NETDEV_FORMATTED_OUT, content)
assert NETDEV_FORMATTED_OUT == content

@mock.patch("cloudinit.netinfo.subp.which")
@mock.patch("cloudinit.netinfo.subp.subp")
Expand All @@ -42,7 +42,7 @@ def test_netdev_new_nettools_pformat(self, m_subp, m_which):
m_subp.return_value = (SAMPLE_NEW_IFCONFIG_OUT, "")
m_which.side_effect = lambda x: x if x == "ifconfig" else None
content = netdev_pformat()
self.assertEqual(NETDEV_FORMATTED_OUT, content)
assert NETDEV_FORMATTED_OUT == content

@mock.patch("cloudinit.netinfo.subp.which")
@mock.patch("cloudinit.netinfo.subp.subp")
Expand All @@ -54,13 +54,16 @@ def test_netdev_freebsd_nettools_pformat(self, m_subp, m_which):
print()
print(content)
print()
self.assertEqual(FREEBSD_NETDEV_OUT, content)
assert FREEBSD_NETDEV_OUT == content

@pytest.mark.parametrize(
"resource", [SAMPLE_IPADDRSHOW_OUT, SAMPLE_IPADDRSHOW_OUT_METRIC]
)
@mock.patch("cloudinit.netinfo.subp.which")
@mock.patch("cloudinit.netinfo.subp.subp")
def test_netdev_iproute_pformat(self, m_subp, m_which):
def test_netdev_iproute_pformat(self, m_subp, m_which, resource):
"""netdev_pformat properly rendering ip route info."""
m_subp.return_value = (SAMPLE_IPADDRSHOW_OUT, "")
m_subp.return_value = (resource, "")
m_which.side_effect = lambda x: x if x == "ip" else None
content = netdev_pformat()
new_output = copy(NETDEV_FORMATTED_OUT)
Expand All @@ -70,19 +73,19 @@ def test_netdev_iproute_pformat(self, m_subp, m_which):
new_output = new_output.replace(
"255.0.0.0 | . |", "255.0.0.0 | host |"
)
self.assertEqual(new_output, content)
assert new_output == content

@mock.patch("cloudinit.netinfo.subp.which")
@mock.patch("cloudinit.netinfo.subp.subp")
def test_netdev_warn_on_missing_commands(self, m_subp, m_which):
def test_netdev_warn_on_missing_commands(self, m_subp, m_which, caplog):
"""netdev_pformat warns when missing both ip and 'netstat'."""
m_which.return_value = None # Niether ip nor netstat found
content = netdev_pformat()
self.assertEqual("\n", content)
self.assertEqual(
"WARNING: Could not print networks: missing 'ip' and 'ifconfig'"
" commands\n",
self.logs.getvalue(),
assert "\n" == content
log = caplog.records[0]
assert log.levelname == "WARNING"
assert log.msg == (
"Could not print networks: missing 'ip' and 'ifconfig' commands"
)
m_subp.assert_not_called()

Expand All @@ -95,23 +98,20 @@ def test_netdev_info_nettools_down(self, m_subp, m_which):
"",
)
m_which.side_effect = lambda x: x if x == "ifconfig" else None
self.assertEqual(
{
"eth0": {
"ipv4": [],
"ipv6": [],
"hwaddr": "00:16:3e:de:51:a6",
"up": False,
},
"lo": {
"ipv4": [{"ip": "127.0.0.1", "mask": "255.0.0.0"}],
"ipv6": [{"ip": "::1/128", "scope6": "host"}],
"hwaddr": ".",
"up": True,
},
assert netdev_info(".") == {
"eth0": {
"ipv4": [],
"ipv6": [],
"hwaddr": "00:16:3e:de:51:a6",
"up": False,
},
netdev_info("."),
)
"lo": {
"ipv4": [{"ip": "127.0.0.1", "mask": "255.0.0.0"}],
"ipv6": [{"ip": "::1/128", "scope6": "host"}],
"hwaddr": ".",
"up": True,
},
}

@mock.patch("cloudinit.netinfo.subp.which")
@mock.patch("cloudinit.netinfo.subp.subp")
Expand All @@ -122,30 +122,27 @@ def test_netdev_info_iproute_down(self, m_subp, m_which):
"",
)
m_which.side_effect = lambda x: x if x == "ip" else None
self.assertEqual(
{
"lo": {
"ipv4": [
{
"ip": "127.0.0.1",
"bcast": ".",
"mask": "255.0.0.0",
"scope": "host",
}
],
"ipv6": [{"ip": "::1/128", "scope6": "host"}],
"hwaddr": ".",
"up": True,
},
"eth0": {
"ipv4": [],
"ipv6": [],
"hwaddr": "00:16:3e:de:51:a6",
"up": False,
},
assert netdev_info(".") == {
"lo": {
"ipv4": [
{
"ip": "127.0.0.1",
"bcast": ".",
"mask": "255.0.0.0",
"scope": "host",
}
],
"ipv6": [{"ip": "::1/128", "scope6": "host"}],
"hwaddr": ".",
"up": True,
},
netdev_info("."),
)
"eth0": {
"ipv4": [],
"ipv6": [],
"hwaddr": "00:16:3e:de:51:a6",
"up": False,
},
}

@mock.patch("cloudinit.netinfo.netdev_info")
def test_netdev_pformat_with_down(self, m_netdev_info):
Expand All @@ -166,9 +163,9 @@ def test_netdev_pformat_with_down(self, m_netdev_info):
"up": False,
},
}
self.assertEqual(
readResource("netinfo/netdev-formatted-output-down"),
netdev_pformat(),
assert (
readResource("netinfo/netdev-formatted-output-down")
== netdev_pformat()
)

@mock.patch("cloudinit.netinfo.subp.which")
Expand All @@ -186,7 +183,7 @@ def subp_netstat_route_selector(*args, **kwargs):
m_subp.side_effect = subp_netstat_route_selector
m_which.side_effect = lambda x: x if x == "netstat" else None
content = route_pformat()
self.assertEqual(ROUTE_FORMATTED_OUT, content)
assert ROUTE_FORMATTED_OUT == content

@mock.patch("cloudinit.netinfo.subp.which")
@mock.patch("cloudinit.netinfo.subp.subp")
Expand All @@ -204,19 +201,19 @@ def subp_iproute_selector(*args, **kwargs):
m_subp.side_effect = subp_iproute_selector
m_which.side_effect = lambda x: x if x == "ip" else None
content = route_pformat()
self.assertEqual(ROUTE_FORMATTED_OUT, content)
assert ROUTE_FORMATTED_OUT == content

@mock.patch("cloudinit.netinfo.subp.which")
@mock.patch("cloudinit.netinfo.subp.subp")
def test_route_warn_on_missing_commands(self, m_subp, m_which):
def test_route_warn_on_missing_commands(self, m_subp, m_which, caplog):
"""route_pformat warns when missing both ip and 'netstat'."""
m_which.return_value = None # Niether ip nor netstat found
content = route_pformat()
self.assertEqual("\n", content)
self.assertEqual(
"WARNING: Could not print routes: missing 'ip' and 'netstat'"
" commands\n",
self.logs.getvalue(),
assert "\n" == content
log = caplog.records[0]
assert log.levelname == "WARNING"
assert log.msg == (
"Could not print routes: missing 'ip' and 'netstat' commands"
)
m_subp.assert_not_called()

Expand Down