From 9ed047f0c44cc5b7b233b706dc1dc5e4bd8698db Mon Sep 17 00:00:00 2001 From: Brandon Casey Date: Sat, 2 Nov 2024 16:21:53 -0500 Subject: [PATCH 1/3] ATTN: networkmanager: add support for "routing-policy" NetworkManager supports ip rules via the "routing-ruleX" field. Let's teach the networkmanager backend to emit "routing-ruleX" entries for each "routing-policy" entry in the netplan yaml. Update the tests for the new behavior and add additional tests to exercise the new code paths. --- doc/netplan-yaml.md | 3 + src/nm.c | 65 ++++++++++++++++++ tests/generator/test_routing.py | 116 ++++++++++++++++++++++++++++++++ tests/generator/test_vrfs.py | 15 ++++- tests/integration/routing.py | 7 +- 5 files changed, 201 insertions(+), 5 deletions(-) diff --git a/doc/netplan-yaml.md b/doc/netplan-yaml.md index 2857e7eab..f4e0e94ec 100644 --- a/doc/netplan-yaml.md +++ b/doc/netplan-yaml.md @@ -879,6 +879,9 @@ network: > Specify a priority for the routing policy rule, to influence the order > in which routing rules are processed. A higher number means lower > priority: rules are processed in order by increasing priority number. + > Specifying an explicit, unique, priority for each routing policy rule + > is strongly recommended and is mandatory on the `NetworkManager` + > back-end. - **`mark`** (scalar) diff --git a/src/nm.c b/src/nm.c index 3dbb85bfc..6c4e33480 100644 --- a/src/nm.c +++ b/src/nm.c @@ -254,6 +254,64 @@ write_routes_nm(const NetplanNetDefinition* def, GKeyFile *kf, gint family, GErr j++; } } + + return TRUE; +} + +STATIC gboolean +write_ip_rules_nm(const NetplanNetDefinition* def, GKeyFile *kf, gint family, GError** error) +{ + const gchar* group = NULL; + gchar* tmp_key = NULL; + GString* tmp_val = NULL; + + if (family == AF_INET) + group = "ipv4"; + else if (family == AF_INET6) + group = "ipv6"; + g_assert(group != NULL); + + if (def->ip_rules != NULL) { + for (unsigned i = 0, j = 1; i < def->ip_rules->len; ++i) { + const NetplanIPRule *cur_rule = g_array_index(def->ip_rules, NetplanIPRule*, i); + + if (cur_rule->family != family) + continue; + + /* NetworkManager requires that priority be specified. This is + * also in-line with the iproute2 guidance that "Each rule should + * have an explicitly set unique priority value"[1]. + * [1]http://www.policyrouting.org/iproute2.doc.html#ss9.6.1 */ + if (cur_rule->priority == NETPLAN_IP_RULE_PRIO_UNSPEC) { + g_set_error(error, NETPLAN_BACKEND_ERROR, NETPLAN_ERROR_UNSUPPORTED, + "ERROR: %s: The priority setting is mandatory for NetworkManager routing-policy\n", def->id); + return FALSE; + } + + tmp_key = g_strdup_printf("routing-rule%u", j); + tmp_val = g_string_sized_new(200); + + g_string_printf(tmp_val, "priority %u", cur_rule->priority); + + if (cur_rule->from) + g_string_append_printf(tmp_val, " from %s", cur_rule->from); + if (cur_rule->to) + g_string_append_printf(tmp_val, " to %s", cur_rule->to); + if (cur_rule->tos != NETPLAN_IP_RULE_TOS_UNSPEC) + g_string_append_printf(tmp_val, " tos %u", cur_rule->tos); + if (cur_rule->fwmark != NETPLAN_IP_RULE_FW_MARK_UNSPEC) + g_string_append_printf(tmp_val, " fwmark %u", cur_rule->fwmark); + if (cur_rule->table != NETPLAN_ROUTE_TABLE_UNSPEC) + g_string_append_printf(tmp_val, " table %u", cur_rule->table); + + g_key_file_set_string(kf, group, tmp_key, tmp_val->str); + g_free(tmp_key); + g_string_free(tmp_val, TRUE); + + j++; + } + } + return TRUE; } @@ -716,6 +774,8 @@ write_nm_conf_access_point(const NetplanNetDefinition* def, const char* rootdir, g_key_file_set_uint64(kf, "vrf", "table", def->vrf_table); if (!write_routes_nm(def, kf, AF_INET, error) || !write_routes_nm(def, kf, AF_INET6, error)) return FALSE; + if (!write_ip_rules_nm(def, kf, AF_INET, error) || !write_ip_rules_nm(def, kf, AF_INET6, error)) + return FALSE; } if (def->type == NETPLAN_DEF_TYPE_VETH && def->veth_peer_link) { @@ -871,6 +931,8 @@ write_nm_conf_access_point(const NetplanNetDefinition* def, const char* rootdir, write_search_domains(def, "ipv4", kf); if (!write_routes_nm(def, kf, AF_INET, error)) return FALSE; + if (!write_ip_rules_nm(def, kf, AF_INET, error)) + return FALSE; } if (!def->dhcp4_overrides.use_routes) { @@ -917,6 +979,9 @@ write_nm_conf_access_point(const NetplanNetDefinition* def, const char* rootdir, if (!write_routes_nm(def, kf, AF_INET6, error)) return FALSE; + if (!write_ip_rules_nm(def, kf, AF_INET6, error)) + return FALSE; + if (!def->dhcp6_overrides.use_routes) { g_key_file_set_boolean(kf, "ipv6", "ignore-auto-routes", TRUE); g_key_file_set_boolean(kf, "ipv6", "never-default", TRUE); diff --git a/tests/generator/test_routing.py b/tests/generator/test_routing.py index 704a59a26..2612d2e96 100644 --- a/tests/generator/test_routing.py +++ b/tests/generator/test_routing.py @@ -1071,6 +1071,122 @@ def test_route_v6_default(self): address1=2001:dead:beef::2/64 ip6-privacy=0 route1=::/0,2001:beef:beef::1 +'''}) + + def test_ip_rule_missing_priority_fails_ipv4(self): + err = self.generate('''network: + version: 2 + renderer: NetworkManager + ethernets: + engreen: + addresses: ["192.168.14.2/24"] + routing-policy: + - to: 10.10.10.0/24 + table: 100 + ''', expect_fail=True) + self.assertIn("ERROR: engreen: The priority setting is mandatory for NetworkManager routing-policy", err) + + def test_ip_rule_missing_priority_fails_ipv6(self): + err = self.generate('''network: + version: 2 + renderer: NetworkManager + ethernets: + engreen: + addresses: ["2001:FFfe::1/64"] + routing-policy: + - to: 2001:FFfe::1/64 + table: 100 + ''', expect_fail=True) + self.assertIn("ERROR: engreen: The priority setting is mandatory for NetworkManager routing-policy", err) + + def test_ip_rule_table(self): + self.generate('''network: + version: 2 + renderer: NetworkManager + ethernets: + engreen: + addresses: ["192.168.14.2/24"] + routing-policy: + - to: 10.10.10.0/24 + table: 100 + priority: 99 + ''') + + self.assert_nm({'engreen': '''[connection] +id=netplan-engreen +type=ethernet +interface-name=engreen + +[ethernet] +wake-on-lan=0 + +[ipv4] +method=manual +address1=192.168.14.2/24 +routing-rule1=priority 99 to 10.10.10.0/24 table 100 + +[ipv6] +method=ignore +'''}) + + def test_ip_rule_fwmark(self): + self.generate('''network: + version: 2 + renderer: NetworkManager + ethernets: + engreen: + addresses: ["192.168.14.2/24"] + routing-policy: + - from: 10.10.10.0/24 + mark: 50 + priority: 99 + ''') + + self.assert_nm({'engreen': '''[connection] +id=netplan-engreen +type=ethernet +interface-name=engreen + +[ethernet] +wake-on-lan=0 + +[ipv4] +method=manual +address1=192.168.14.2/24 +routing-rule1=priority 99 from 10.10.10.0/24 fwmark 50 + +[ipv6] +method=ignore +'''}) + + def test_ip_rule_tos(self): + self.generate('''network: + version: 2 + renderer: NetworkManager + ethernets: + engreen: + addresses: ["192.168.14.2/24"] + routing-policy: + - to: 10.10.10.0/24 + type-of-service: 250 + priority: 99 + ''') + + self.assert_nm({'engreen': '''[connection] +id=netplan-engreen +type=ethernet +interface-name=engreen + +[ethernet] +wake-on-lan=0 + +[ipv4] +method=manual +address1=192.168.14.2/24 +routing-rule1=priority 99 to 10.10.10.0/24 tos 250 + +[ipv6] +method=ignore '''}) def test_routes_mixed(self): diff --git a/tests/generator/test_vrfs.py b/tests/generator/test_vrfs.py index 23d0966e6..529551334 100644 --- a/tests/generator/test_vrfs.py +++ b/tests/generator/test_vrfs.py @@ -37,7 +37,8 @@ def test_vrf_set_table(self): - to: default via: 1.2.3.4 routing-policy: - - from: 2.3.4.5''') + - from: 2.3.4.5 + priority: 99''') self.assert_nm({'eth0': '''[connection] id=netplan-eth0 @@ -66,6 +67,7 @@ def test_vrf_set_table(self): [ipv4] route1=0.0.0.0/0,1.2.3.4 route1_options=table=1005 +routing-rule1=priority 99 from 2.3.4.5 table 1005 method=link-local [ipv6] @@ -224,3 +226,14 @@ def test_vrf_without_routes(self): Table=1005 ''', 'vrf1005.netdev': ND_VRF % ('vrf1005', 1005)}) + + def test_vrf_routing_policy_missing_priority_nm(self): + err = self.generate('''network: + version: 2 + renderer: NetworkManager + vrfs: + vrf1005: + table: 1005 + routing-policy: + - from: 2.3.4.5''', expect_fail=True) + self.assertIn("ERROR: vrf1005: The priority setting is mandatory for NetworkManager routing-policy", err) diff --git a/tests/integration/routing.py b/tests/integration/routing.py index 6afd75c6d..2f8841eb0 100644 --- a/tests/integration/routing.py +++ b/tests/integration/routing.py @@ -317,6 +317,7 @@ def test_vrf_basic(self): via: 10.10.10.1 routing-policy: - from: 10.10.10.42 + priority: 99 ''' % {'r': self.backend, 'ec': self.dev_e_client}) self.generate_and_settle([self.dev_e_client]) self.assert_iface_up(self.dev_e_client, ['inet 10.10.10.22', 'master vrf0']) # wokeignore:rule=master @@ -332,10 +333,8 @@ def test_vrf_basic(self): self.assertIn('11.11.11.0/24 via 10.10.10.2 dev {}'.format(self.dev_e_client), out) # verify routing policy was setup correctly to the VRF's table - # 'routing-policy' is not supported on NetworkManager - if self.backend == 'networkd': - out = subprocess.check_output(['ip', 'rule', 'show'], text=True) - self.assertIn('from 10.10.10.42 lookup 1000', out) + out = subprocess.check_output(['ip', 'rule', 'show'], text=True) + self.assertIn('from 10.10.10.42 lookup 1000', out) def test_route_advmss_v6(self): self.setup_eth(None) From e9eddceda547b0b2253491dea140d98e7c9f8d4e Mon Sep 17 00:00:00 2001 From: Brandon Casey Date: Tue, 12 Nov 2024 22:42:02 -0600 Subject: [PATCH 2/3] validation: remove superfluous comparison Since the check of whether r->table == nd->vrf_table has already failed in the initial 'if' conditional, r->table is necessarily != nd->vrf_table. It is not necessary to explicitly check for this state in the 'else if' conditional. Let's not. --- src/validation.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/validation.c b/src/validation.c index a0371a8fe..8ed95137d 100644 --- a/src/validation.c +++ b/src/validation.c @@ -526,7 +526,7 @@ adopt_and_validate_vrf_routes(__unused const NetplanParser *npp, GHashTable *net if (r->table == nd->vrf_table) { g_debug("%s: Ignoring redundant routing-policy table %d (matches VRF table)", nd->id, r->table); continue; - } else if (r->table != NETPLAN_ROUTE_TABLE_UNSPEC && r->table != nd->vrf_table) { + } else if (r->table != NETPLAN_ROUTE_TABLE_UNSPEC) { g_set_error(error, NETPLAN_VALIDATION_ERROR, NETPLAN_ERROR_CONFIG_GENERIC, "%s: VRF routing-policy table mismatch (%d != %d)", nd->id, nd->vrf_table, r->table); return FALSE; From e0c8801ba367114e84c98e51a9934a44f5eefb35 Mon Sep 17 00:00:00 2001 From: Brandon Casey Date: Tue, 12 Nov 2024 22:46:35 -0600 Subject: [PATCH 3/3] ATTN: validation: warn if a priority is not set for a routing-policy The iproute2 guidance says "Each rule should have an explicitly set unique priority value", and further warns: For historical reasons ip rule add does not require any priority value and allows the priority value to be non-unique. If the user had not supplied a priority value then one was assigned by the kernel. If the user requested creating a rule with a priority value which already existed then the kernel did not reject the request and added the new rule before all old rules of the same priority. This is a mistake in the current design, nothing more. It should be fixed by the time you read this so please do not rely on this feature. You should always use explicit priorities when creating rules. So let's inspect each routing-policy entry and emit a warning if a priority has not been specified. ref. http://www.policyrouting.org/iproute2.doc.html#ss9.6.1 --- src/validation.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/validation.c b/src/validation.c index 8ed95137d..bbd45023e 100644 --- a/src/validation.c +++ b/src/validation.c @@ -523,6 +523,9 @@ adopt_and_validate_vrf_routes(__unused const NetplanParser *npp, GHashTable *net if (nd->ip_rules) { for (size_t i = 0; i < nd->ip_rules->len; i++) { NetplanIPRule* r = g_array_index(nd->ip_rules, NetplanIPRule*, i); + if (r->priority == NETPLAN_IP_RULE_PRIO_UNSPEC) { + g_warning("%s: No priority specified for routing-policy %zu", nd->id, i); + } if (r->table == nd->vrf_table) { g_debug("%s: Ignoring redundant routing-policy table %d (matches VRF table)", nd->id, r->table); continue;