From 36af0f8580a056597b2172bcc47f62afe335fb3b Mon Sep 17 00:00:00 2001 From: Gris Ge Date: Wed, 25 Sep 2024 16:39:37 +0800 Subject: [PATCH] nm route rule: Only search desired interface for storing route rule When finding interface to store the route rule, we should only desired interfaces (use loopback as fallback), so we do not touch unmentioned interfaces which might lead to unexpected behaviour. Integration test case included. Unit test case updated for this change. Resolves: https://issues.redhat.com/browse/RHEL-59965 Signed-off-by: Gris Ge --- rust/src/lib/nm/route_rule.rs | 33 +------- rust/src/lib/unit_tests/nm/route_rule.rs | 55 ++++++------- tests/integration/nm/route_test.py | 99 ++++++++++++++++++++++++ 3 files changed, 130 insertions(+), 57 deletions(-) diff --git a/rust/src/lib/nm/route_rule.rs b/rust/src/lib/nm/route_rule.rs index d4e74957e0..da12f03169 100644 --- a/rust/src/lib/nm/route_rule.rs +++ b/rust/src/lib/nm/route_rule.rs @@ -229,8 +229,9 @@ fn append_route_rule( Ok(()) } -// * If rule has `iif`, we use that -// * If rule has table id, we find a interface configured for that route table +// * If rule has `iif`, we use that interface. +// * If rule has table id, we find a interface in desire state configured for +// that route table // * fallback to first desired interface with ip stack enabled. // * fallback to use loop interface. fn find_interface_for_rule<'a>( @@ -293,34 +294,6 @@ fn find_interface_for_rule<'a>( } } - let mut cur_iface_names: Vec<&str> = merged_state - .interfaces - .kernel_ifaces - .iter() - .filter_map(|(n, i)| { - if !i.is_changed() { - Some(n.as_str()) - } else { - None - } - }) - .collect(); - - // we should be persistent on choice, hence sort the iface names. - cur_iface_names.sort_unstable(); - - // Try interfaces in current state - for iface_name in cur_iface_names { - if iface_has_route_for_table_id( - iface_name, - merged_state, - rule.is_ipv6(), - table_id, - ) { - return Ok(iface_name); - } - } - // Fallback to first interface in desire state with IP stack enabled. for (iface_name, iface_type) in merged_state.interfaces.insert_order.as_slice().iter() diff --git a/rust/src/lib/unit_tests/nm/route_rule.rs b/rust/src/lib/unit_tests/nm/route_rule.rs index bf530cd678..e63d985f59 100644 --- a/rust/src/lib/unit_tests/nm/route_rule.rs +++ b/rust/src/lib/unit_tests/nm/route_rule.rs @@ -200,6 +200,9 @@ route-rules: - route-table: 500 priority: 3200 ip-from: 192.0.3.0/24 +interfaces: + - name: br0 + type: ovs-interface ", ) .unwrap(); @@ -258,15 +261,15 @@ interfaces: .unwrap(); let desired: NetworkState = serde_yaml::from_str( - r" ---- -route-rules: - config: - - priority: 3200 - ip-to: 192.0.3.0/24 - - priority: 3200 - ip-from: 192.0.3.0/24 -", + r"--- + route-rules: + config: + - priority: 3200 + ip-to: 192.0.3.0/24 + - priority: 3200 + ip-from: 192.0.3.0/24 + interfaces: + - name: eth1", ) .unwrap(); @@ -304,24 +307,22 @@ route-rules: #[test] fn test_route_rule_use_loopback() { let current: NetworkState = serde_yaml::from_str( - r" ---- -interfaces: - - name: lo - type: loopback - state: up - mtu: 65536 - ipv4: - enabled: true - address: - - ip: 127.0.0.1 - prefix-length: 8 - ipv6: - enabled: true - address: - - ip: ::1 - prefix-length: 128 -", + r"--- + interfaces: + - name: lo + type: loopback + state: up + mtu: 65536 + ipv4: + enabled: true + address: + - ip: 127.0.0.1 + prefix-length: 8 + ipv6: + enabled: true + address: + - ip: ::1 + prefix-length: 128", ) .unwrap(); diff --git a/tests/integration/nm/route_test.py b/tests/integration/nm/route_test.py index bd10c56f02..6e2322fa9d 100644 --- a/tests/integration/nm/route_test.py +++ b/tests/integration/nm/route_test.py @@ -107,3 +107,102 @@ def test_route_delayed_by_nm_fails(eth1_up): Loader=yaml.SafeLoader, ) ) + + +@pytest.fixture +def eth1_with_static_routes_table_id_200(eth1_up): + libnmstate.apply( + yaml.load( + """--- + routes: + config: + - destination: 192.168.2.0/24 + metric: 108 + next-hop-address: 192.168.1.3 + next-hop-interface: eth1 + table-id: 200 + - destination: 2001:db8:a::/64 + metric: 108 + next-hop-address: 2001:db8:1::2 + next-hop-interface: eth1 + table-id: 200 + interfaces: + - name: eth1 + type: ethernet + state: up + mtu: 1500 + ipv4: + enabled: true + dhcp: false + address: + - ip: 192.168.1.1 + prefix-length: 24 + ipv6: + enabled: true + dhcp: false + autoconf: false + address: + - ip: 2001:db8:1::1 + prefix-length: 64 + """, + Loader=yaml.SafeLoader, + ) + ) + yield + libnmstate.apply( + yaml.load( + """--- + route-rules: + config: + - state: absent + routes: + config: + - state: absent + next-hop-interface: eth1 + interfaces: + - name: eth1 + state: absent + """, + Loader=yaml.SafeLoader, + ) + ) + + +# https://issues.redhat.com/browse/RHEL-59965 +@pytest.mark.tier1 +def test_route_rule_use_loopback_for_no_desired_iface( + eth1_with_static_routes_table_id_200, +): + # Even the eth1 has routes on route table 200, since it is not mentioned + # in desired state, we should not use eth1 but fallback to loopback + # interface. + libnmstate.apply( + yaml.load( + """--- + route-rules: + config: + - ip-from: 192.168.3.2/32 + route-table: 200 + priority: 1000 + - ip-from: 2001:db8:b::/64 + route-table: 200 + priority: 1001 + """, + Loader=yaml.SafeLoader, + ) + ) + + assert ( + exec_cmd( + "nmcli -g ipv4.routing-rules c show lo".split(), + check=True, + )[1].strip() + == "priority 1000 from 192.168.3.2 table 200" + ) + assert ( + exec_cmd( + "nmcli -g ipv6.routing-rules c show lo".split(), + check=True, + )[1].strip() + == r"priority 1001 from 2001\:db8\:b\:\:/64 table 200" + )