Skip to content

Commit

Permalink
interface: fix handling of interface renames
Browse files Browse the repository at this point in the history 
Whenever an interface netlink message is received from the kernel,
the interface should be identified by its ifindex, and not by its
name. This is because ifindexes are immutable, whereas interface
names can change over time.

This fix resolves a regression where holod wasn't working correctly
within containerlab, which does interface renaming after their
initial creation.

Additionally, the `Interfaces` struct is updated to use two BTreeMaps
backed by an interface arena. This modification allows for efficient
lookups using both interface names and ifindexes.

Signed-off-by: Renato Westphal <[email protected]>
  • Loading branch information
@rwestphal
rwestphal committed Nov 4, 2023
1 parent 77940f9 commit 980f5af
Show file tree
Hide file tree
Showing 4 changed files with 114 additions and 55 deletions.
1 change: 1 addition & 0 deletions holo-interface/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ async-trait.workspace = true
derive-new.workspace = true
enum-as-inner.workspace = true
futures.workspace = true
generational-arena.workspace = true
ipnetwork.workspace = true
netlink-packet-route.workspace = true
netlink-packet-core.workspace = true
Expand Down
6 changes: 3 additions & 3 deletions holo-interface/src/ibus.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::Master;
pub(crate) fn process_msg(master: &mut Master, msg: IbusMsg) {
match msg {
IbusMsg::InterfaceDump => {
for iface in master.interfaces.tree.values() {
for iface in master.interfaces.iter() {
notify_interface_update(
&master.ibus_tx,
iface.name.clone(),
Expand All @@ -31,7 +31,7 @@ pub(crate) fn process_msg(master: &mut Master, msg: IbusMsg) {
}
}
IbusMsg::InterfaceQuery { ifname, af } => {
if let Some(iface) = master.interfaces.tree.get(&ifname) {
if let Some(iface) = master.interfaces.get_by_name(&ifname) {
notify_interface_update(
&master.ibus_tx,
iface.name.clone(),
Expand All @@ -58,7 +58,7 @@ pub(crate) fn process_msg(master: &mut Master, msg: IbusMsg) {
IbusMsg::RouterIdQuery => {
notify_router_id_update(
&master.ibus_tx,
master.interfaces.router_id,
master.interfaces.router_id(),
);
}
// Ignore other events.
Expand Down
145 changes: 107 additions & 38 deletions holo-interface/src/interface.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@
// SPDX-License-Identifier: MIT
//

use std::collections::{btree_map, BTreeMap};
use std::collections::{BTreeMap, HashMap};
use std::net::{IpAddr, Ipv4Addr};

use generational_arena::{Arena, Index};
use holo_utils::ibus::IbusSender;
use holo_utils::ip::Ipv4NetworkExt;
use holo_utils::southbound::{AddressFlags, InterfaceFlags};
Expand All @@ -16,10 +17,14 @@ use crate::ibus;

#[derive(Debug, Default)]
pub struct Interfaces {
// List of interfaces.
pub tree: BTreeMap<String, Interface>,
// Interface arena.
arena: Arena<Interface>,
// Interface binary tree keyed by name (1:1).
name_tree: BTreeMap<String, Index>,
// Interface hash table keyed by ifindex (1:1).
ifindex_tree: HashMap<u32, Index>,
// Auto-generated Router ID.
pub router_id: Option<Ipv4Addr>,
router_id: Option<Ipv4Addr>,
}

#[derive(Debug)]
Expand All @@ -40,6 +45,7 @@ pub struct InterfaceAddress {
// ===== impl Interfaces =====

impl Interfaces {
// Adds or updates the interface with the specified attributes.
pub(crate) fn update(
&mut self,
ifname: String,
Expand All @@ -48,33 +54,41 @@ impl Interfaces {
flags: InterfaceFlags,
ibus_tx: Option<&IbusSender>,
) {
match self.tree.entry(ifname.clone()) {
btree_map::Entry::Vacant(v) => {
// If the interface does not exist, create a new entry.
v.insert(Interface {
name: ifname.clone(),
ifindex,
mtu,
flags,
addresses: Default::default(),
});
}
btree_map::Entry::Occupied(o) => {
let iface = o.into_mut();
match self.ifindex_tree.get(&ifindex).copied() {
Some(iface_idx) => {
let iface = &mut self.arena[iface_idx];

// If nothing of interest has changed, return early.
if iface.ifindex == ifindex
if iface.name == ifname
&& iface.mtu == mtu
&& iface.flags == flags
{
return;
}

// Update the existing interface with the new information.
iface.ifindex = ifindex;
if iface.name != ifname {
self.name_tree.remove(&iface.name);
iface.name = ifname.clone();
self.name_tree.insert(ifname.clone(), iface_idx);
}
iface.mtu = mtu;
iface.flags = flags;
}
None => {
// If the interface does not exist, create a new entry.
let iface = Interface {
name: ifname.clone(),
ifindex,
mtu,
flags,
addresses: Default::default(),
};

let iface_idx = self.arena.insert(iface);
self.name_tree.insert(ifname.clone(), iface_idx);
self.ifindex_tree.insert(ifindex, iface_idx);
}
}

// Notify protocol instances about the interface update.
Expand All @@ -83,25 +97,32 @@ impl Interfaces {
}
}

// Removes the specified interface identified by its ifindex.
pub(crate) fn remove(
&mut self,
ifname: String,
ifindex: u32,
ibus_tx: Option<&IbusSender>,
) {
// Remove interface.
if self.tree.remove(&ifname).is_none() {
let Some(iface_idx) = self.ifindex_tree.get(&ifindex).copied() else {
return;
}
};

// Notify protocol instances.
let iface = &self.arena[iface_idx];
if let Some(ibus_tx) = ibus_tx {
ibus::notify_interface_del(ibus_tx, ifname);
ibus::notify_interface_del(ibus_tx, iface.name.clone());
}

// Remove interface.
self.name_tree.remove(&iface.name);
self.ifindex_tree.remove(&iface.ifindex);
self.arena.remove(iface_idx);

// Check if the Router ID needs to be updated.
self.update_router_id(ibus_tx);
}

// Adds the specified address to the interface identified by its ifindex.
pub(crate) fn addr_add(
&mut self,
ifindex: u32,
Expand All @@ -114,11 +135,7 @@ impl Interfaces {
}

// Lookup interface.
let Some(iface) = self
.tree
.values_mut()
.find(|iface| iface.ifindex == ifindex)
else {
let Some(iface) = self.get_mut_by_ifindex(ifindex) else {
return;
};

Expand All @@ -143,18 +160,15 @@ impl Interfaces {
self.update_router_id(ibus_tx);
}

// Removes the specified address from the interface identified by its ifindex.
pub(crate) fn addr_del(
&mut self,
ifindex: u32,
addr: IpNetwork,
ibus_tx: Option<&IbusSender>,
) {
// Lookup interface.
let Some(iface) = self
.tree
.values_mut()
.find(|iface| iface.ifindex == ifindex)
else {
let Some(iface) = self.get_mut_by_ifindex(ifindex) else {
return;
};

Expand All @@ -176,14 +190,18 @@ impl Interfaces {
}
}

// Returns the auto-generated Router ID.
pub(crate) fn router_id(&self) -> Option<Ipv4Addr> {
self.router_id
}

// Updates the auto-generated Router ID.
fn update_router_id(&mut self, ibus_tx: Option<&IbusSender>) {
let loopback_interfaces = self
.tree
.values()
.iter()
.filter(|iface| iface.flags.contains(InterfaceFlags::LOOPBACK));
let non_loopback_interfaces = self
.tree
.values()
.iter()
.filter(|iface| !iface.flags.contains(InterfaceFlags::LOOPBACK));

// Helper function to find the highest IPv4 address among a list of
Expand Down Expand Up @@ -222,4 +240,55 @@ impl Interfaces {
}
}
}

// Returns a reference to the interface corresponding to the given name.
pub(crate) fn get_by_name(&self, ifname: &str) -> Option<&Interface> {
self.name_tree
.get(ifname)
.copied()
.map(|iface_idx| &self.arena[iface_idx])
}

// Returns a mutable reference to the interface corresponding to the given
// name.
#[allow(dead_code)]
pub(crate) fn get_mut_by_name(
&mut self,
ifname: &str,
) -> Option<&mut Interface> {
self.name_tree
.get(ifname)
.copied()
.map(move |iface_idx| &mut self.arena[iface_idx])
}

// Returns a reference to the interface corresponding to the given ifindex.
#[allow(dead_code)]
pub(crate) fn get_by_ifindex(&self, ifindex: u32) -> Option<&Interface> {
self.ifindex_tree
.get(&ifindex)
.copied()
.map(|iface_idx| &self.arena[iface_idx])
}

// Returns a mutable reference to the interface corresponding to the given
// ifindex.
pub(crate) fn get_mut_by_ifindex(
&mut self,
ifindex: u32,
) -> Option<&mut Interface> {
self.ifindex_tree
.get(&ifindex)
.copied()
.map(move |iface_idx| &mut self.arena[iface_idx])
}

// Returns an iterator visiting all interfaces.
//
// Interfaces are ordered by their names.
pub(crate) fn iter(&self) -> impl Iterator<Item = &'_ Interface> + '_ {
self.name_tree
.values()
.map(|iface_idx| &self.arena[*iface_idx])
}
}
17 changes: 3 additions & 14 deletions holo-interface/src/netlink.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,25 +62,14 @@ fn process_newlink_msg(master: &mut Master, msg: LinkMessage, notify: bool) {
}

fn process_dellink_msg(master: &mut Master, msg: LinkMessage, notify: bool) {
use netlink_packet_route::link::nlas::Nla;

trace!(?msg, "received RTM_DELLINK message");

// Fetch interface name.
let mut ifname = None;
for nla in msg.nlas.into_iter() {
match nla {
Nla::IfName(nla_ifname) => ifname = Some(nla_ifname),
_ => (),
}
}
let Some(ifname) = ifname else {
return;
};
// Fetch interface ifindex.
let ifindex = msg.header.index;

// Remove interface.
let ibus_tx = notify.then_some(&master.ibus_tx);
master.interfaces.remove(ifname, ibus_tx);
master.interfaces.remove(ifindex, ibus_tx);
}

fn process_newaddr_msg(master: &mut Master, msg: AddressMessage, notify: bool) {
Expand Down

0 comments on commit 980f5af

Please sign in to comment.