From aca99a3a38d2eb57039b7eaa7dbbea5e0d1cdeba Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Tue, 18 Jul 2023 21:39:36 +0200 Subject: [PATCH 1/3] test-network: validate JSON where applicable --- test/test-network/systemd-networkd-tests.py | 69 ++++++++++++--------- 1 file changed, 39 insertions(+), 30 deletions(-) diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index 7f3d262ac9..c7821a6384 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -11,6 +11,7 @@ import argparse import errno import itertools +import json import os import pathlib import re @@ -124,6 +125,13 @@ def run(*command, **kwargs): command = command[0].split() + list(command[1:]) return subprocess.run(command, check=False, universal_newlines=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, **kwargs) +def check_json(string): + try: + json.loads(string) + except json.JSONDecodeError: + print(f"String is not a valid JSON: '{string}'") + raise + def is_module_available(*module_names): for module_name in module_names: lsmod_output = check_output('lsmod') @@ -2365,8 +2373,9 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): for i in range(1, 254): self.assertIn(f'inet 10.3.3.{i}/16 brd 10.3.255.255', output) - # TODO: check json string - check_output(*networkctl_cmd, '--json=short', 'status', env=env) + output = check_output(*networkctl_cmd, '--json=short', 'status', env=env) + check_json(output) + def test_address_null(self): copy_network_unit('25-address-null.network', '12-dummy.netdev') @@ -2591,8 +2600,8 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): self.assertRegex(output, 'iif test1') self.assertRegex(output, 'lookup 10') - # TODO: check json string - check_output(*networkctl_cmd, '--json=short', 'status', env=env) + output = check_output(*networkctl_cmd, '--json=short', 'status', env=env) + check_json(output) def test_routing_policy_rule_issue_11280(self): copy_network_unit('25-routing-policy-rule-test1.network', '11-dummy.netdev', @@ -2814,8 +2823,8 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): self.assertIn('via 2001:1234:5:8fff:ff:ff:ff:ff dev dummy98', output) self.assertIn('via 2001:1234:5:9fff:ff:ff:ff:ff dev dummy98', output) - # TODO: check json string - check_output(*networkctl_cmd, '--json=short', 'status', env=env) + output = check_output(*networkctl_cmd, '--json=short', 'status', env=env) + check_json(output) copy_network_unit('25-address-static.network') networkctl_reload() @@ -3082,8 +3091,8 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): self.assertRegex(output, '192.168.10.1.*00:00:5e:00:02:65.*PERMANENT') self.assertRegex(output, '2004:da8:1::1.*00:00:5e:00:02:66.*PERMANENT') - # TODO: check json string - check_output(*networkctl_cmd, '--json=short', 'status', env=env) + output = check_output(*networkctl_cmd, '--json=short', 'status', env=env) + check_json(output) def test_neighbor_reconfigure(self): copy_network_unit('25-neighbor-section.network', '12-dummy.netdev') @@ -3121,8 +3130,8 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): print(output) self.assertRegex(output, '2001:db8:0:f102::17 lladdr 2a:?00:ff:?de:45:?67:ed:?de:[0:]*:49:?88 PERMANENT') - # TODO: check json string - check_output(*networkctl_cmd, '--json=short', 'status', env=env) + output = check_output(*networkctl_cmd, '--json=short', 'status', env=env) + check_json(output) def test_link_local_addressing(self): copy_network_unit('25-link-local-addressing-yes.network', '11-dummy.netdev', @@ -3520,8 +3529,8 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): self.assertIn('nexthop via 192.168.20.1 dev dummy98 weight 1', output) self.assertIn('nexthop via 192.168.5.1 dev veth99 weight 3', output) - # TODO: check json string - check_output(*networkctl_cmd, '--json=short', 'status', env=env) + output = check_output(*networkctl_cmd, '--json=short', 'status', env=env) + check_json(output) copy_network_unit('25-nexthop.network', '25-veth.netdev', '25-veth-peer.network', '12-dummy.netdev', '25-nexthop-dummy.network') @@ -3840,8 +3849,8 @@ class NetworkdStateFileTests(unittest.TestCase, Utilities): # make link state file updated check_output(*resolvectl_cmd, 'revert', 'dummy98', env=env) - # TODO: check json string - check_output(*networkctl_cmd, '--json=short', 'status', env=env) + output = check_output(*networkctl_cmd, '--json=short', 'status', env=env) + check_json(output) output = read_link_state_file('dummy98') print(output) @@ -3869,8 +3878,8 @@ class NetworkdStateFileTests(unittest.TestCase, Utilities): check_output(*resolvectl_cmd, 'dnssec', 'dummy98', 'yes', env=env) check_output(*timedatectl_cmd, 'ntp-servers', 'dummy98', '2.fedora.pool.ntp.org', '3.fedora.pool.ntp.org', env=env) - # TODO: check json string - check_output(*networkctl_cmd, '--json=short', 'status', env=env) + output = check_output(*networkctl_cmd, '--json=short', 'status', env=env) + check_json(output) output = read_link_state_file('dummy98') print(output) @@ -3884,8 +3893,8 @@ class NetworkdStateFileTests(unittest.TestCase, Utilities): check_output(*timedatectl_cmd, 'revert', 'dummy98', env=env) - # TODO: check json string - check_output(*networkctl_cmd, '--json=short', 'status', env=env) + output = check_output(*networkctl_cmd, '--json=short', 'status', env=env) + check_json(output) output = read_link_state_file('dummy98') print(output) @@ -3899,8 +3908,8 @@ class NetworkdStateFileTests(unittest.TestCase, Utilities): check_output(*resolvectl_cmd, 'revert', 'dummy98', env=env) - # TODO: check json string - check_output(*networkctl_cmd, '--json=short', 'status', env=env) + output = check_output(*networkctl_cmd, '--json=short', 'status', env=env) + check_json(output) output = read_link_state_file('dummy98') print(output) @@ -4998,8 +5007,8 @@ class NetworkdDHCPClientTests(unittest.TestCase, Utilities): self.assertNotRegex(output, r'8.8.8.8 via 192.168.5.[0-9]* proto dhcp src 192.168.5.[0-9]* metric 1024') self.assertNotRegex(output, r'9.9.9.9 via 192.168.5.[0-9]* proto dhcp src 192.168.5.[0-9]* metric 1024') - # TODO: check json string - check_output(*networkctl_cmd, '--json=short', 'status', env=env) + output = check_output(*networkctl_cmd, '--json=short', 'status', env=env) + check_json(output) def test_dhcp_client_settings_anonymize(self): copy_network_unit('25-veth.netdev', '25-dhcp-server-veth-peer.network', '25-dhcp-client-anonymize.network') @@ -5253,8 +5262,8 @@ class NetworkdDHCPClientTests(unittest.TestCase, Utilities): else: self.assertNotIn('2600::1', output) - # TODO: check json string - check_output(*networkctl_cmd, '--json=short', 'status', env=env) + output = check_output(*networkctl_cmd, '--json=short', 'status', env=env) + check_json(output) copy_network_unit('25-veth.netdev', '25-dhcp-server-veth-peer.network', '25-dhcp-client.network', copy_dropins=False) @@ -5292,8 +5301,8 @@ class NetworkdDHCPClientTests(unittest.TestCase, Utilities): else: self.assertNotIn('Captive Portal: http://systemd.io', output) - # TODO: check json string - check_output(*networkctl_cmd, '--json=short', 'status', env=env) + output = check_output(*networkctl_cmd, '--json=short', 'status', env=env) + check_json(output) copy_network_unit('25-veth.netdev', '25-dhcp-server-veth-peer.network', '25-dhcp-client.network', copy_dropins=False) @@ -5329,8 +5338,8 @@ class NetworkdDHCPClientTests(unittest.TestCase, Utilities): self.assertNotIn('Captive Portal: ', output) self.assertNotIn('invalid/url', output) - # TODO: check json string - check_output(*networkctl_cmd, '--json=short', 'status', env=env) + output = check_output(*networkctl_cmd, '--json=short', 'status', env=env) + check_json(output) copy_network_unit('25-veth.netdev', '25-dhcp-server-veth-peer.network', '25-dhcp-client.network', copy_dropins=False) @@ -5825,8 +5834,8 @@ class NetworkdIPv6PrefixTests(unittest.TestCase, Utilities): print(output) self.assertIn('example.com', output) - # TODO: check json string - check_output(*networkctl_cmd, '--json=short', 'status', env=env) + output = check_output(*networkctl_cmd, '--json=short', 'status', env=env) + check_json(output) def test_ipv6_route_prefix_deny_list(self): copy_network_unit('25-veth.netdev', '25-ipv6ra-prefix-client-deny-list.network', '25-ipv6ra-prefix.network', From 7e107bc31f8ccd7ed279edd03d804708ae207f77 Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Tue, 18 Jul 2023 21:28:54 +0200 Subject: [PATCH 2/3] test-network: probe a couple of uncovered networkctl codepaths --- test/test-network/systemd-networkd-tests.py | 44 +++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index c7821a6384..8f4cb788d2 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -116,6 +116,11 @@ def call(*command, **kwargs): command = command[0].split() + list(command[1:]) return subprocess.run(command, check=False, universal_newlines=True, stderr=subprocess.STDOUT, **kwargs).returncode +def call_check(*command, **kwargs): + # Same as call() above, but it triggers CalledProcessError if rc != 0 + command = command[0].split() + list(command[1:]) + return subprocess.run(command, check=False, universal_newlines=True, stderr=subprocess.STDOUT, **kwargs).check_returncode() + def call_quiet(*command, **kwargs): command = command[0].split() + list(command[1:]) return subprocess.run(command, check=False, universal_newlines=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, **kwargs).returncode @@ -1015,6 +1020,42 @@ class NetworkctlTests(unittest.TestCase, Utilities): self.assertIn('inet 10.1.2.4/16 brd 10.1.255.255 scope global secondary dummy98', output) self.assertIn('inet 10.2.2.4/16 brd 10.2.255.255 scope global dummy98', output) + def test_renew(self): + def check(): + self.wait_online(['veth99:routable', 'veth-peer:routable']) + output = check_output(*networkctl_cmd, '-n', '0', 'status', 'veth99', env=env) + print(output) + self.assertRegex(output, r'Address: 192.168.5.[0-9]* \(DHCP4 via 192.168.5.1\)') + self.assertIn('Gateway: 192.168.5.3', output) + self.assertRegex(output, 'DNS: 192.168.5.1\n *192.168.5.10') + self.assertRegex(output, 'NTP: 192.168.5.1\n *192.168.5.11') + + copy_network_unit('25-veth.netdev', '25-dhcp-client.network', '25-dhcp-server.network') + start_networkd() + check() + output = check_output(*networkctl_cmd, '--lines=0', '--stats', '--all', '--full', '--json=short', 'status') + check_json(output) + + for verb in ['renew', 'forcerenew']: + call_check(*networkctl_cmd, verb, 'veth99') + check() + call_check(*networkctl_cmd, verb, 'veth99', 'veth99', 'veth99') + check() + + def test_up_down(self): + copy_network_unit('25-address-static.network', '12-dummy.netdev') + start_networkd() + self.wait_online(['dummy98:routable']) + + call_check(*networkctl_cmd, 'down', 'dummy98') + self.wait_online(['dummy98:off']) + call_check(*networkctl_cmd, 'up', 'dummy98') + self.wait_online(['dummy98:routable']) + call_check(*networkctl_cmd, 'down', 'dummy98', 'dummy98', 'dummy98') + self.wait_online(['dummy98:off']) + call_check(*networkctl_cmd, 'up', 'dummy98', 'dummy98', 'dummy98') + self.wait_online(['dummy98:routable']) + def test_reload(self): start_networkd() @@ -1117,6 +1158,9 @@ class NetworkctlTests(unittest.TestCase, Utilities): self.check_link_exists('veth99', expected=False) self.check_link_exists('veth-peer', expected=False) + def test_label(self): + call_check(*networkctl_cmd, 'label') + class NetworkdMatchTests(unittest.TestCase, Utilities): def setUp(self): From 8bdece7479cf147bc8f278f7874c03f5fc23a3a1 Mon Sep 17 00:00:00 2001 From: Frantisek Sumsal Date: Tue, 18 Jul 2023 22:47:43 +0200 Subject: [PATCH 3/3] network: fix fetching link properties This fixes regression introduced in 5a0c810462 with which all requests for link properties ended up with EINVAL as we kept hitting the signature_is_single() assert in sd_bus_get_property(). --- src/network/networkctl.c | 18 ++++++------------ test/test-network/systemd-networkd-tests.py | 3 +++ 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/network/networkctl.c b/src/network/networkctl.c index ffbda5dc0e..8c0b096e1d 100644 --- a/src/network/networkctl.c +++ b/src/network/networkctl.c @@ -708,7 +708,8 @@ static int link_get_property( sd_bus_error *error, sd_bus_message **reply, const char *iface, - const char *propname) { + const char *propname, + const char *type) { _cleanup_free_ char *path = NULL; char ifindex_str[DECIMAL_STR_MAX(int)]; @@ -721,6 +722,7 @@ static int link_get_property( assert(reply); assert(iface); assert(propname); + assert(type); xsprintf(ifindex_str, "%i", link->ifindex); @@ -728,7 +730,7 @@ static int link_get_property( if (r < 0) return r; - return sd_bus_get_property(bus, "org.freedesktop.network1", path, iface, propname, error, reply, "ss"); + return sd_bus_get_property(bus, "org.freedesktop.network1", path, iface, propname, error, reply, type); } static int acquire_link_bitrates(sd_bus *bus, LinkInfo *link) { @@ -739,7 +741,7 @@ static int acquire_link_bitrates(sd_bus *bus, LinkInfo *link) { assert(bus); assert(link); - r = link_get_property(bus, link, &error, &reply, "org.freedesktop.network1.Link", "BitRates"); + r = link_get_property(bus, link, &error, &reply, "org.freedesktop.network1.Link", "BitRates", "(tt)"); if (r < 0) { bool quiet = sd_bus_error_has_names(&error, SD_BUS_ERROR_UNKNOWN_PROPERTY, BUS_ERROR_SPEED_METER_INACTIVE); @@ -748,10 +750,6 @@ static int acquire_link_bitrates(sd_bus *bus, LinkInfo *link) { r, "Failed to query link bit rates: %s", bus_error_message(&error, r)); } - r = sd_bus_message_enter_container(reply, 'v', "(tt)"); - if (r < 0) - return bus_log_parse_error(r); - r = sd_bus_message_read(reply, "(tt)", &link->tx_bitrate, &link->rx_bitrate); if (r < 0) return bus_log_parse_error(r); @@ -1402,7 +1400,7 @@ static int dump_dhcp_leases(Table *table, const char *prefix, sd_bus *bus, const assert(bus); assert(link); - r = link_get_property(bus, link, &error, &reply, "org.freedesktop.network1.DHCPServer", "Leases"); + r = link_get_property(bus, link, &error, &reply, "org.freedesktop.network1.DHCPServer", "Leases", "a(uayayayayt)"); if (r < 0) { bool quiet = sd_bus_error_has_name(&error, SD_BUS_ERROR_UNKNOWN_PROPERTY); @@ -1411,10 +1409,6 @@ static int dump_dhcp_leases(Table *table, const char *prefix, sd_bus *bus, const return 0; } - r = sd_bus_message_enter_container(reply, 'v', "a(uayayayayt)"); - if (r < 0) - return bus_log_parse_error(r); - r = sd_bus_message_enter_container(reply, 'a', "(uayayayayt)"); if (r < 0) return bus_log_parse_error(r); diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index 8f4cb788d2..01336bc175 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -4710,6 +4710,9 @@ class NetworkdDHCPServerTests(unittest.TestCase, Utilities): self.assertRegex(output, 'DNS: 192.168.5.1\n *192.168.5.10') self.assertRegex(output, 'NTP: 192.168.5.1\n *192.168.5.11') + output = check_output(*networkctl_cmd, '-n', '0', 'status', 'veth-peer', env=env) + self.assertRegex(output, "Offered DHCP leases: 192.168.5.[0-9]*") + def test_dhcp_server_with_uplink(self): copy_network_unit('25-veth.netdev', '25-dhcp-client.network', '25-dhcp-server-downstream.network', '12-dummy.netdev', '25-dhcp-server-uplink.network')