[pbs-devel] [PATCH proxmox{, -network-interface-pinning} v2 0/3] make ip link deserializing more robust

[pbs-devel] [PATCH proxmox{, -network-interface-pinning} v2 0/3] make ip link deserializing more robust

[Stefan Hanreich]

As reported in the forum by [1], there are types of interfaces that do not have
a MAC address, which tripped up the previous deserializing logic. Make the ip
link parsing more robust in general, to prevent tripping up on unexpected
interface types / network configuration.

Changes from v1:
* abstracted link_type specific properties into an enum (Thanks @Christian)
* added tests for all links that proved problematic for the old parser (Thanks
  @Christian for providing me with sample output)
* added a small housekeeping patch that adds some documentation to the public
  API

[1] https://forum.proxmox.com/threads/proxmox-backup-proxy-seemingly-crashes-after-being-accessed-through-reverse-proxy-after-update-to-4-0.169313/

proxmox:

Stefan Hanreich (2):
  network-api: properly parse ip link output based on link_type
  config: helper: document public API

 proxmox-network-api/src/config/helper.rs | 277 ++++++++++++++++++++++-
 1 file changed, 266 insertions(+), 11 deletions(-)


proxmox-network-interface-pinning:

Stefan Hanreich (1):
  network-interface-pinning: adapt to optional mac address

 src/main.rs | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)


Summary over all repositories:
  2 files changed, 283 insertions(+), 21 deletions(-)

-- 
Generated by git-murpp 0.8.0

_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox v2 1/2] network-api: properly parse ip link output based on link_type

[Stefan Hanreich]

The structure of the JSON schema can change, depending on the
link_type field returned. By introducing a new enum that uses
link_type as tag and has variants that hold the respective
type-specific fields we can handle differing link types much more
gracefully.

This also introduces a unknown variant, that acts as a catch_all for
all unknown interface types. This makes parsing much more robust,
since we should be able to deal with any link_type returned by ip
link.

The respective functions relying on the fields that were there
unconditionally previously have been adapted. The detection of
physical interfaces currently only works for ethernet interfaces
properly, in other cases we fall back to the regex which should catch
some non-ethernet interfaces (e.g. Infiniband).

Also add tests for the different kinds of links that were problematic
before, so we can be sure those are caught every time.

Suggested-by: Christian Ebner <c.ebner@proxmox.com>
Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 proxmox-network-api/src/config/helper.rs | 266 ++++++++++++++++++++++-
 1 file changed, 255 insertions(+), 11 deletions(-)

diff --git a/proxmox-network-api/src/config/helper.rs b/proxmox-network-api/src/config/helper.rs
index 4f17b9ee..6d372b26 100644
--- a/proxmox-network-api/src/config/helper.rs
+++ b/proxmox-network-api/src/config/helper.rs
@@ -130,14 +130,49 @@ pub struct LinkInfo {
     info_kind: Option<String>,
 }
 
+/// The fields specific to an interface of type `ether`.
+#[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, serde::Deserialize)]
+pub struct EtherLink {
+    address: MacAddress,
+}
+
+/// Catch all variant for all unknown link types.
+#[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, serde::Deserialize)]
+pub struct UnknownLink {
+    // required, since otherwise the tagged enum will fail to parse, due to the tag of [`Link`]
+    // being link_type.
+    link_type: String,
+}
+
+/// Enum abstracting the fields that are specific to a given link type.
+///
+/// The JSON returned by `ip -details -json link show` returns different fields for different link
+/// types. Depending on the type, fields with the same name can contain different types. This enum
+/// is used to handle the fields that vary between the different link types.
+///
+/// The last variant is a catch all variant, that should capture everything else, so we do not get
+/// deserialization errors in any case.
+#[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, serde::Deserialize)]
+#[serde(tag = "link_type", rename_all = "lowercase")]
+pub enum Link {
+    Ether(EtherLink),
+    #[serde(untagged)]
+    Unknown(UnknownLink),
+}
+
+/// An IpLink entry, as returned by `ip -details -json link show`.
+///
+/// For now this parses only the fields that are used throughout our stack, the fields of this
+/// struct are incomplete. To abstract fields that are specific to a link type, this struct uses
+/// [`Link`].
 #[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, serde::Deserialize)]
 pub struct IpLink {
     ifname: String,
     #[serde(default)]
     altnames: Vec<String>,
     ifindex: i64,
-    link_type: String,
-    address: MacAddress,
+    #[serde(flatten)]
+    link_type: Link,
     linkinfo: Option<LinkInfo>,
     operstate: String,
 }
@@ -147,26 +182,49 @@ impl IpLink {
         self.ifindex
     }
 
+    /// Whether this is a physical or virtual interface.
+    ///
+    /// For ethernet interfaces, this checks whether info_kind is set in link_info,
+    /// since virtual 'physical' interfaces (e.g. bridges) have link type ether as well.
+    ///
+    /// Otherwise, we fall back to [`PHYSICAL_NIC_REGEX`], which was the sole method for
+    /// determining whether an interface is physical or not before. This should cover other types of
+    /// non-ethernet physical interfaces (e.g. Infiniband), that have not been manually renamed.
     pub fn is_physical(&self) -> bool {
-        (self.link_type == "ether"
-            && (self.linkinfo.is_none() || self.linkinfo.as_ref().unwrap().info_kind.is_none()))
-            || PHYSICAL_NIC_REGEX.is_match(&self.ifname)
+        if let Link::Ether(_) = self.link_type {
+            if let Some(linkinfo) = &self.linkinfo {
+                if linkinfo.info_kind.is_none() {
+                    return true;
+                }
+            } else {
+                return true;
+            }
+        }
+
+        PHYSICAL_NIC_REGEX.is_match(&self.ifname)
     }
 
     pub fn name(&self) -> &str {
         &self.ifname
     }
 
-    pub fn permanent_mac(&self) -> MacAddress {
-        if let Some(link_info) = &self.linkinfo {
-            if let Some(info_slave_data) = &link_info.info_slave_data {
-                if let Some(perm_hw_addr) = info_slave_data.perm_hw_addr {
-                    return perm_hw_addr;
+    /// Returns the MAC address of the physical device, even if the interface is enslaved.
+    ///
+    /// Some interfaces can change their MAC address if they are enslaved to bonds or bridges. This
+    /// method returns the permanent MAC address of a link, independent of whether they are
+    /// enslaved or not.
+    pub fn permanent_mac(&self) -> Option<MacAddress> {
+        if let Link::Ether(ether) = &self.link_type {
+            if let Some(link_info) = &self.linkinfo {
+                if let Some(info_slave_data) = &link_info.info_slave_data {
+                    return info_slave_data.perm_hw_addr;
                 }
             }
+
+            return Some(ether.address);
         }
 
-        self.address
+        None
     }
 
     pub fn altnames(&self) -> impl Iterator<Item = &String> {
@@ -260,3 +318,189 @@ pub fn network_reload() -> Result<(), Error> {
 
     Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_deserialize_ipv6_to_ipv4_tunnel() {
+        let interface = r#"{
+  "ifindex": 7,
+  "link": null,
+  "ifname": "sit0",
+  "flags": [
+    "NOARP"
+  ],
+  "mtu": 1480,
+  "qdisc": "noop",
+  "operstate": "DOWN",
+  "linkmode": "DEFAULT",
+  "group": "default",
+  "txqlen": 1000,
+  "link_type": "sit",
+  "address": "0.0.0.0",
+  "broadcast": "0.0.0.0",
+  "promiscuity": 0,
+  "allmulti": 0,
+  "min_mtu": 1280,
+  "max_mtu": 65555,
+  "linkinfo": {
+    "info_kind": "sit",
+    "info_data": {
+      "proto": "ip6ip",
+      "remote": "any",
+      "local": "any",
+      "ttl": 64,
+      "pmtudisc": false,
+      "prefix": "2002::",
+      "prefixlen": 16
+    }
+  },
+  "inet6_addr_gen_mode": "eui64",
+  "num_tx_queues": 1,
+  "num_rx_queues": 1,
+  "gso_max_size": 65536,
+  "gso_max_segs": 65535,
+  "tso_max_size": 65536,
+  "tso_max_segs": 65535,
+  "gro_max_size": 65536,
+  "gso_ipv4_max_size": 65536,
+  "gro_ipv4_max_size": 65536
+}"#;
+
+        serde_json::from_str::<IpLink>(interface).unwrap();
+    }
+
+    #[test]
+    fn test_deserialize_ethernet_interface() {
+        let interface = r#"{
+    "ifindex": 2,
+    "ifname": "eth1",
+    "flags": [
+      "BROADCAST",
+      "MULTICAST",
+      "UP",
+      "LOWER_UP"
+    ],
+    "mtu": 1500,
+    "qdisc": "fq_codel",
+    "master": "vmbr0",
+    "operstate": "UP",
+    "linkmode": "DEFAULT",
+    "group": "default",
+    "txqlen": 1000,
+    "link_type": "ether",
+    "address": "bc:24:11:ca:ff:ee",
+    "broadcast": "ff:ff:ff:ff:ff:ff",
+    "promiscuity": 1,
+    "allmulti": 1,
+    "min_mtu": 68,
+    "max_mtu": 9194,
+    "linkinfo": {
+      "info_slave_kind": "bridge",
+      "info_slave_data": {
+        "state": "forwarding",
+        "priority": 32,
+        "cost": 5,
+        "hairpin": false,
+        "guard": false,
+        "root_block": false,
+        "fastleave": false,
+        "learning": true,
+        "flood": true,
+        "id": "0x8001",
+        "no": "0x1",
+        "designated_port": 32769,
+        "designated_cost": 0,
+        "bridge_id": "8000.bc:24:11:00:00:00",
+        "root_id": "8000.bc:24:11:00:00:00",
+        "hold_timer": 0.00,
+        "message_age_timer": 0.00,
+        "forward_delay_timer": 0.00,
+        "topology_change_ack": 0,
+        "config_pending": 0,
+        "proxy_arp": false,
+        "proxy_arp_wifi": false,
+        "multicast_router": 1,
+        "mcast_flood": true,
+        "bcast_flood": true,
+        "mcast_to_unicast": false,
+        "neigh_suppress": false,
+        "neigh_vlan_suppress": false,
+        "group_fwd_mask": "0",
+        "group_fwd_mask_str": "0x0",
+        "vlan_tunnel": false,
+        "isolated": false,
+        "locked": false,
+        "mab": false
+      }
+    },
+    "inet6_addr_gen_mode": "eui64",
+    "num_tx_queues": 1,
+    "num_rx_queues": 1,
+    "gso_max_size": 64000,
+    "gso_max_segs": 64,
+    "tso_max_size": 64000,
+    "tso_max_segs": 64,
+    "gro_max_size": 65536,
+    "gso_ipv4_max_size": 64000,
+    "gro_ipv4_max_size": 65536,
+    "parentbus": "pci",
+    "parentdev": "0000:01:00.0",
+    "altnames": [
+      "enxbc2411aabbcc"
+    ]
+}"#;
+
+        serde_json::from_str::<IpLink>(interface).unwrap();
+    }
+
+    #[test]
+    fn test_deserialize_tailscale_interface() {
+        let interface = r#"{
+    "ifindex": 3,
+    "ifname": "tailscale0",
+    "flags": [
+      "POINTOPOINT",
+      "MULTICAST",
+      "NOARP",
+      "UP",
+      "LOWER_UP"
+    ],
+    "mtu": 1280,
+    "qdisc": "fq_codel",
+    "operstate": "UNKNOWN",
+    "linkmode": "DEFAULT",
+    "group": "default",
+    "txqlen": 500,
+    "link_type": "none",
+    "promiscuity": 0,
+    "allmulti": 0,
+    "min_mtu": 68,
+    "max_mtu": 65535,
+    "linkinfo": {
+      "info_kind": "tun",
+      "info_data": {
+        "type": "tun",
+        "pi": false,
+        "vnet_hdr": true,
+        "multi_queue": false,
+        "persist": false
+      }
+    },
+    "inet6_addr_gen_mode": "random",
+    "num_tx_queues": 1,
+    "num_rx_queues": 1,
+    "gso_max_size": 65536,
+    "gso_max_segs": 65535,
+    "tso_max_size": 65536,
+    "tso_max_segs": 65535,
+    "gro_max_size": 65536,
+    "gso_ipv4_max_size": 65536,
+    "gro_ipv4_max_size": 65536
+}"#;
+
+        serde_json::from_str::<IpLink>(interface).unwrap();
+    }
+}
-- 
2.47.2


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox v2 2/2] config: helper: document public API

[Stefan Hanreich]

Add docstrings to the public API members for network interface
parsing, where they were missing before.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 proxmox-network-api/src/config/helper.rs | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/proxmox-network-api/src/config/helper.rs b/proxmox-network-api/src/config/helper.rs
index 6d372b26..cc2f3dd0 100644
--- a/proxmox-network-api/src/config/helper.rs
+++ b/proxmox-network-api/src/config/helper.rs
@@ -119,11 +119,13 @@ pub(crate) fn parse_address_or_cidr(cidr: &str) -> Result<(String, Option<u8>, b
     }
 }
 
+/// Struct representing the info_slave_data field inside link_info, as returned by `ip -details -json link show`.
 #[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, serde::Deserialize)]
 pub struct SlaveData {
     perm_hw_addr: Option<MacAddress>,
 }
 
+/// Struct representing the link_info field, as returned by `ip -details -json link show`.
 #[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, serde::Deserialize)]
 pub struct LinkInfo {
     info_slave_data: Option<SlaveData>,
@@ -178,6 +180,7 @@ pub struct IpLink {
 }
 
 impl IpLink {
+    /// The index of the interface.
     pub fn index(&self) -> i64 {
         self.ifindex
     }
@@ -204,6 +207,7 @@ impl IpLink {
         PHYSICAL_NIC_REGEX.is_match(&self.ifname)
     }
 
+    /// The name of the interface (ifname / IFLA_IFNAME).
     pub fn name(&self) -> &str {
         &self.ifname
     }
@@ -227,15 +231,18 @@ impl IpLink {
         None
     }
 
+    /// Returns an iterator over the altnames of an interface.
     pub fn altnames(&self) -> impl Iterator<Item = &String> {
         self.altnames.iter()
     }
 
+    /// Returns whether the interface is currently in an UP state.
     pub fn active(&self) -> bool {
         self.operstate == "UP"
     }
 }
 
+/// A mapping of altnames to the interfaces' ifname.
 #[derive(Debug, Clone, serde::Deserialize)]
 pub struct AltnameMapping {
     mapping: HashMap<String, String>,
@@ -263,6 +270,10 @@ impl FromIterator<IpLink> for AltnameMapping {
     }
 }
 
+/// Returns a list of all network interfaces currently available on the host.
+///
+/// This parses the output of `ip -details -json link show` and returns a map of the ifname to the
+/// [`IpLink`] for that interface.
 pub fn get_network_interfaces() -> Result<HashMap<String, IpLink>, Error> {
     let output = std::process::Command::new("ip")
         .arg("-details")
-- 
2.47.2


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-network-interface-pinning v2 1/1] network-interface-pinning: adapt to optional mac address

[Stefan Hanreich]

MAC addresses can be optional, so the return value of permanent_mac()
has changed to Option<MacAddress>. Adapt all call sites to gracefully
handle the case where a link has no permanent MAC address.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/main.rs | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/src/main.rs b/src/main.rs
index 4e1fa4f..bff0660 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -63,7 +63,12 @@ impl InterfaceMapping {
 
         for ip_link in sorted_links {
             if let Some(new_name) = self.mapping.get(ip_link.name()) {
-                let link_file = LinkFile::new_ether(ip_link.permanent_mac(), new_name.to_string());
+                let link_file = LinkFile::new_ether(
+                    ip_link
+                        .permanent_mac()
+                        .ok_or_else(|| anyhow!("trying to pin interface without a MAC address!"))?,
+                    new_name.to_string(),
+                );
 
                 std::fs::write(
                     format!("{}/{}", SYSTEMD_LINK_FILE_PATH, link_file.file_name()),
@@ -407,10 +412,11 @@ impl PinningTool {
             .get(interface_name)
             .ok_or_else(|| anyhow!("cannot find interface with name {interface_name}"))?;
 
-        if self
-            .pinned_interfaces
-            .contains_key(&ip_link.permanent_mac())
-        {
+        let Some(mac_address) = ip_link.permanent_mac() else {
+            bail!("Interface does not have a MAC address, so it cannot be pinned!");
+        };
+
+        if self.pinned_interfaces.contains_key(&mac_address) {
             bail!("pin already exists for interface {interface_name}");
         }
 
@@ -483,11 +489,12 @@ impl PinningTool {
             .ip_links
             .values()
             .filter(|ip_link| {
-                ip_link.is_physical()
-                    && self
-                        .pinned_interfaces
-                        .get(&ip_link.permanent_mac())
-                        .is_none()
+                if let Some(mac_address) = ip_link.permanent_mac() {
+                    return ip_link.is_physical()
+                        && self.pinned_interfaces.get(&mac_address).is_none();
+                }
+
+                false
             })
             .cloned()
             .map(IpLink::from)
-- 
2.47.2


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox{, -network-interface-pinning} v2 0/3] make ip link deserializing more robust

[Christian Ebner]

Thanks for the new patches, with these also the `link/sit` interface 
type is parsed.

Consider these patches:

Tested-by: Christian Ebner <c.ebner@proxmox.com>


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] applied-series: [PATCH proxmox{, -network-interface-pinning} v2 0/3] make ip link deserializing more robust

[Fabian Grünbichler]

On August 8, 2025 12:23 pm, Stefan Hanreich wrote:
> As reported in the forum by [1], there are types of interfaces that do not have
> a MAC address, which tripped up the previous deserializing logic. Make the ip
> link parsing more robust in general, to prevent tripping up on unexpected
> interface types / network configuration.
> 
> Changes from v1:
> * abstracted link_type specific properties into an enum (Thanks @Christian)
> * added tests for all links that proved problematic for the old parser (Thanks
>   @Christian for providing me with sample output)
> * added a small housekeeping patch that adds some documentation to the public
>   API
> 
> [1] https://forum.proxmox.com/threads/proxmox-backup-proxy-seemingly-crashes-after-being-accessed-through-reverse-proxy-after-update-to-4-0.169313/
> 
> proxmox:
> 
> Stefan Hanreich (2):
>   network-api: properly parse ip link output based on link_type
>   config: helper: document public API
> 
>  proxmox-network-api/src/config/helper.rs | 277 ++++++++++++++++++++++-
>  1 file changed, 266 insertions(+), 11 deletions(-)
> 
> 
> proxmox-network-interface-pinning:
> 
> Stefan Hanreich (1):
>   network-interface-pinning: adapt to optional mac address
> 
>  src/main.rs | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> 
> Summary over all repositories:
>   2 files changed, 283 insertions(+), 21 deletions(-)
> 
> -- 
> Generated by git-murpp 0.8.0
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel