From b4f1aa61436ba0ab09be586f706b39dcec1066cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=8D=C3=B1igo=20Huguet?= Date: Mon, 4 Nov 2024 14:34:11 +0100 Subject: [PATCH] route: correctly compare the route's next-hop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In nmstate routes without next-hop are represented with "0.0.0.0" or "::". This is because we need to differentiate the case "I don't care about the next-hop for routes matching" (next-hop=None) and "This route doesn't have next-hop" (next-hop=0.0.0.0). However, NM doesn't do the same because it doesn't need that differentiation. If a NM profile contains a direct route, without next-hop, it will just leave it empty, thus we get next-hop=None. This causes that comparing an NmIpRoute created from a nmstate route with an NmIpRoute created from an existing NM profile will fail because of this difference. Fix it by converting "0.0.0.0" / "::" to None when creating a NmIpRoute from an nmstate route. This is the right representation for it at `nm` layer, anyway. Signed-off-by: Íñigo Huguet --- rust/src/lib/nm/settings/route.rs | 13 +++++++- tests/integration/route_test.py | 48 ++++++++++++++++++++++++++++++ tests/integration/testlib/dummy.py | 28 +++++++++++++++++ 3 files changed, 88 insertions(+), 1 deletion(-) diff --git a/rust/src/lib/nm/settings/route.rs b/rust/src/lib/nm/settings/route.rs index a520f5e3aa..1563079e98 100644 --- a/rust/src/lib/nm/settings/route.rs +++ b/rust/src/lib/nm/settings/route.rs @@ -6,6 +6,9 @@ use crate::{ ip::is_ipv6_addr, InterfaceIpAddr, NmstateError, RouteEntry, RouteType, }; +const IPV4_EMPTY_NEXT_HOP: &str = "0.0.0.0"; +const IPV6_EMPTY_NEXT_HOP: &str = "::"; + pub(crate) fn gen_nm_ip_routes( routes: &[RouteEntry], is_ipv6: bool, @@ -31,7 +34,15 @@ pub(crate) fn gen_nm_ip_routes( Some(i) => Some(i), None => None, }; - nm_route.next_hop = route.next_hop_addr.as_ref().cloned(); + // Empty next-hop is represented by 0.0.0.0 or :: in nmstate, but NM and + // the kernel just leave it undefined. + nm_route.next_hop = route + .next_hop_addr + .as_ref() + .filter(|&nh| { + nh != IPV4_EMPTY_NEXT_HOP && nh != IPV6_EMPTY_NEXT_HOP + }) + .cloned(); nm_route.src = route.source.as_ref().cloned(); if let Some(weight) = route.weight { nm_route.weight = Some(weight as u32); diff --git a/tests/integration/route_test.py b/tests/integration/route_test.py index 1f3b676df2..69c793b7fa 100644 --- a/tests/integration/route_test.py +++ b/tests/integration/route_test.py @@ -19,8 +19,10 @@ from .testlib import cmdlib from .testlib import iprule from .testlib.bridgelib import linux_bridge +from .testlib.dummy import dummy_interface from .testlib.env import nm_minor_version from .testlib.genconf import gen_conf_apply +from .testlib.iproutelib import ip_monitor_assert_stable_link_up from .testlib.route import assert_routes from .testlib.route import assert_routes_missing from .testlib.servicelib import disable_service @@ -112,6 +114,12 @@ def clean_up_route_rule(): ) +@pytest.fixture(scope="function") +def dummy0_up(test_env_setup): + with dummy_interface("dummy0") as ifstate: + yield ifstate + + @pytest.mark.tier1 def test_add_static_routes(static_eth1_with_routes): routes = _get_ipv4_test_routes() + _get_ipv6_test_routes() @@ -2189,3 +2197,43 @@ def test_kernel_mode_static_route_and_remove(cleanup_veth1_kernel_mode): assert_routes_missing( desired_state[Route.KEY][Route.CONFIG], cur_state, nic="veth1" ) + + +# https://issues.redhat.com/browse/RHEL-64707 +# ip_monitor_assert_stable_link_up doesn't work with ethernet, use dummy +@ip_monitor_assert_stable_link_up("dummy0") +def test_apply_routes_twice_only_reapplies(dummy0_up): + dummy0_state = dummy0_up[Interface.KEY][0] + dummy0_state[Interface.IPV4] = ETH1_INTERFACE_STATE[Interface.IPV4] + dummy0_state[Interface.IPV6] = ETH1_INTERFACE_STATE[Interface.IPV6] + routes = [ + { + Route.NEXT_HOP_INTERFACE: "dummy0", + Route.DESTINATION: IPV4_TEST_NET1, + Route.NEXT_HOP_ADDRESS: IPV4_EMPTY_ADDRESS, + Route.METRIC: 100, + Route.TABLE_ID: 254, + }, + { + Route.NEXT_HOP_INTERFACE: "dummy0", + Route.DESTINATION: IPV6_TEST_NET1, + Route.NEXT_HOP_ADDRESS: IPV6_EMPTY_ADDRESS, + Route.METRIC: 100, + Route.TABLE_ID: 254, + }, + ] + state = { + Interface.KEY: [dummy0_state], + Route.KEY: {Route.CONFIG: routes}, + } + libnmstate.apply(state) + + # Apply a second time to test that empty next-hop is treated + # correctly. Per issue RHEL-64707, the next-hop didn't match between the + # nmstate generated routes and those retrieved from NM, causing nmstate to + # incorrectly think that there were routes to remove. + # This caused deactivate & activate instead of reapply. + libnmstate.apply(state) + + cur_state = libnmstate.show() + assert_routes(routes, cur_state) diff --git a/tests/integration/testlib/dummy.py b/tests/integration/testlib/dummy.py index 8f56ab7756..4d20767381 100644 --- a/tests/integration/testlib/dummy.py +++ b/tests/integration/testlib/dummy.py @@ -5,6 +5,7 @@ import libnmstate from libnmstate.schema import Interface from libnmstate.schema import InterfaceState +from libnmstate.schema import InterfaceType from . import cmdlib @@ -32,3 +33,30 @@ def nm_unmanaged_dummy(name): except Exception: # dummy1 might not became managed by NM, hence removal might fail cmdlib.exec_cmd(f"ip link del {name}".split()) + + +@contextmanager +def dummy_interface(ifname): + desired_state = { + Interface.KEY: [ + { + Interface.NAME: ifname, + Interface.TYPE: InterfaceType.DUMMY, + Interface.STATE: InterfaceState.UP, + } + ] + } + libnmstate.apply(desired_state) + try: + yield desired_state + finally: + libnmstate.apply( + { + Interface.KEY: [ + { + Interface.NAME: ifname, + Interface.STATE: InterfaceState.ABSENT, + } + ] + } + )