[pve-devel] [PATCH installer v3 06/15] common: implement support for `network_interface_pin_map` config

[Christoph Heiss <c.heiss@proxmox.com>]

Adds all the pieces for installer frontends to wire up pinning support,
i.e. deserializing from the runtime environment, doing verification and
serializing it out to the low-level installer.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v2 -> v3:
  * rebased
  * switched fully-numeric check before first character digit check
  * simplified first character digit check
  * added more unit tests (thanks @Michael)

Changes v1 -> v2:
  * improved validation for interface names, now according to our
    `pve-iface` json schema

 proxmox-auto-installer/src/utils.rs     |   3 +-
 proxmox-installer-common/src/lib.rs     |   5 +
 proxmox-installer-common/src/options.rs | 242 ++++++++++++++++++++++--
 proxmox-installer-common/src/setup.rs   |  51 ++++-
 proxmox-tui-installer/src/setup.rs      |   3 +-
 5 files changed, 279 insertions(+), 25 deletions(-)

diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index 2d68829..ce49baa 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -2,7 +2,7 @@ use anyhow::{Context, Result, bail};
 use glob::Pattern;
 use log::info;
 use std::{
-    collections::{BTreeMap, HashSet},
+    collections::{BTreeMap, HashMap, HashSet},
     process::Command,
 };
 
@@ -483,6 +483,7 @@ pub fn parse_answer(
         root_ssh_keys: answer.global.root_ssh_keys.clone(),
 
         mngmt_nic: network_settings.ifname,
+        network_interface_pin_map: HashMap::new(),
 
         hostname: network_settings
             .fqdn
diff --git a/proxmox-installer-common/src/lib.rs b/proxmox-installer-common/src/lib.rs
index ea907a0..a85d5f8 100644
--- a/proxmox-installer-common/src/lib.rs
+++ b/proxmox-installer-common/src/lib.rs
@@ -10,6 +10,11 @@ pub mod http;
 #[cfg(feature = "cli")]
 pub mod cli;
 
+pub mod net {
+    /// Maximum length of the (primary) name of a network interface
+    pub const MAX_IFNAME_LEN: usize = 15; // IFNAMSIZ - 1 to account for NUL byte
+}
+
 pub const RUNTIME_DIR: &str = "/run/proxmox-installer";
 
 /// Default placeholder value for the administrator email address.
diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index fa91060..0d72f54 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -1,12 +1,14 @@
 use anyhow::{Result, bail};
 use regex::Regex;
 use serde::{Deserialize, Serialize};
+use std::collections::HashMap;
 use std::net::{IpAddr, Ipv4Addr};
 use std::str::FromStr;
 use std::sync::OnceLock;
 use std::{cmp, fmt};
 
 use crate::disk_checks::check_raid_min_disks;
+use crate::net::MAX_IFNAME_LEN;
 use crate::setup::{LocaleInfo, NetworkInfo, RuntimeInfo, SetupInfo};
 use crate::utils::{CidrAddress, Fqdn};
 
@@ -476,6 +478,75 @@ impl TimezoneOptions {
     }
 }
 
+/// Options controlling the behaviour of the network interface pinning (by
+/// creating appropriate systemd.link files) during the installation.
+#[derive(Clone, Debug, Default, PartialEq, Deserialize)]
+#[serde(rename_all = "kebab-case", deny_unknown_fields)]
+pub struct NetworkInterfacePinningOptions {
+    /// Maps MAC address to custom name
+    #[serde(default)]
+    pub mapping: HashMap<String, String>,
+}
+
+impl NetworkInterfacePinningOptions {
+    /// Default prefix to prepend to the pinned interface ID as received from the low-level
+    /// installer.
+    pub const DEFAULT_PREFIX: &str = "nic";
+
+    /// Does some basic checks on the options.
+    ///
+    /// This includes checks for:
+    /// - empty interface names
+    /// - overlong interface names
+    /// - duplicate interface names
+    /// - only contains ASCII alphanumeric characters and underscore, as
+    ///   enforced by our `pve-iface` json schema.
+    pub fn verify(&self) -> Result<()> {
+        let mut reverse_mapping = HashMap::<String, String>::new();
+        for (mac, name) in self.mapping.iter() {
+            if name.is_empty() {
+                bail!("interface name for '{mac}' cannot be empty");
+            }
+
+            if name.len() > MAX_IFNAME_LEN {
+                bail!(
+                    "interface name '{name}' for '{mac}' cannot be longer than {} characters",
+                    MAX_IFNAME_LEN
+                );
+            }
+
+            if name.chars().all(char::is_numeric) {
+                bail!(
+                    "interface name '{name}' for '{mac}' is invalid: name must not be fully numeric"
+                );
+            }
+
+            // Mimicking the `pve-iface` schema verification
+            if name.starts_with(|c: char| c.is_ascii_digit()) {
+                bail!(
+                    "interface name '{name}' for '{mac}' is invalid: name must not start with a number"
+                );
+            }
+
+            if !name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') {
+                bail!(
+                    "interface name '{name}' for '{mac}' is invalid: name must only consist of alphanumeric characters and underscores"
+                );
+            }
+
+            if let Some(duplicate_mac) = reverse_mapping.get(name)
+                && mac != duplicate_mac
+            {
+                bail!("duplicate interface name mapping '{name}' for: {mac}, {duplicate_mac}");
+            }
+
+            reverse_mapping.insert(name.clone(), mac.clone());
+        }
+
+        Ok(())
+    }
+}
+
 #[derive(Clone, Debug, PartialEq)]
 pub struct NetworkOptions {
     pub ifname: String,
@@ -483,6 +554,7 @@ pub struct NetworkOptions {
     pub address: CidrAddress,
     pub gateway: IpAddr,
     pub dns_server: IpAddr,
+    pub pinning_opts: Option<NetworkInterfacePinningOptions>,
 }
 
 impl NetworkOptions {
@@ -492,6 +564,7 @@ impl NetworkOptions {
         setup: &SetupInfo,
         network: &NetworkInfo,
         default_domain: Option<&str>,
+        pinning_opts: Option<&NetworkInterfacePinningOptions>,
     ) -> Self {
         // Sets up sensible defaults as much as possible, such that even in the
         // worse case nothing breaks down *completely*.
@@ -507,28 +580,39 @@ impl NetworkOptions {
             address: CidrAddress::new(Ipv4Addr::new(192, 168, 100, 2), 24).unwrap(),
             gateway: Ipv4Addr::new(192, 168, 100, 1).into(),
             dns_server: Ipv4Addr::new(192, 168, 100, 1).into(),
+            pinning_opts: pinning_opts.cloned(),
         };
 
         if let Some(ip) = network.dns.dns.first() {
             this.dns_server = *ip;
         }
 
-        if let Some(routes) = &network.routes {
-            if let Some(gw) = &routes.gateway4
-                && let Some(iface) = network.interfaces.get(&gw.dev)
-            {
-                // we got some ipv4 connectivity, so use that
+        if let Some(routes) = &network.routes
+            && let Some(gw) = &routes.gateway4
+            && let Some(iface) = network.interfaces.get(&gw.dev)
+        {
+            // we got some ipv4 connectivity, so use that
+
+            if let Some(opts) = pinning_opts {
+                this.ifname.clone_from(&iface.to_pinned(opts).name);
+            } else {
                 this.ifname.clone_from(&iface.name);
-                if let Some(addr) = iface.addresses.iter().find(|addr| addr.is_ipv4()) {
-                    this.gateway = gw.gateway;
-                    this.address = addr.clone();
-                }
+            }
+
+            if let Some(addr) = iface.addresses.iter().find(|addr| addr.is_ipv4()) {
+                this.gateway = gw.gateway;
+                this.address = addr.clone();
             } else if let Some(gw) = &routes.gateway6
                 && let Some(iface) = network.interfaces.get(&gw.dev)
                 && let Some(addr) = iface.addresses.iter().find(|addr| addr.is_ipv6())
             {
                 // no ipv4, but ipv6 connectivity
-                this.ifname.clone_from(&iface.name);
+                if let Some(opts) = pinning_opts {
+                    this.ifname.clone_from(&iface.to_pinned(opts).name);
+                } else {
+                    this.ifname.clone_from(&iface.name);
+                }
+
                 this.gateway = gw.gateway;
                 this.address = addr.clone();
             }
@@ -541,7 +625,20 @@ impl NetworkOptions {
         if this.ifname.is_empty()
             && let Some(iface) = network.interfaces.values().min_by_key(|v| v.index)
         {
-            this.ifname.clone_from(&iface.name);
+            if let Some(opts) = pinning_opts {
+                this.ifname.clone_from(&iface.to_pinned(opts).name);
+            } else {
+                this.ifname.clone_from(&iface.name);
+            }
+        }
+
+        if let Some(ref mut opts) = this.pinning_opts {
+            // Ensure that all unique interfaces indeed have an entry in the map,
+            // as required by the low-level installer
+            for iface in network.interfaces.values() {
+                let pinned_name = iface.to_pinned(opts).name;
+                opts.mapping.entry(iface.mac.clone()).or_insert(pinned_name);
+            }
         }
 
         this
@@ -668,6 +765,7 @@ mod tests {
             Interface {
                 name: "eth0".to_owned(),
                 index: 0,
+                pinned_id: "0".to_owned(),
                 state: InterfaceState::Up,
                 driver: "dummy".to_owned(),
                 mac: "01:23:45:67:89:ab".to_owned(),
@@ -699,49 +797,53 @@ mod tests {
         let (setup, mut info) = mock_setup_network();
 
         pretty_assertions::assert_eq!(
-            NetworkOptions::defaults_from(&setup, &info, None),
+            NetworkOptions::defaults_from(&setup, &info, None, None),
             NetworkOptions {
                 ifname: "eth0".to_owned(),
                 fqdn: Fqdn::from("foo.bar.com").unwrap(),
                 address: CidrAddress::new(Ipv4Addr::new(192, 168, 0, 2), 24).unwrap(),
                 gateway: IpAddr::V4(Ipv4Addr::new(192, 168, 0, 1)),
                 dns_server: Ipv4Addr::new(192, 168, 100, 1).into(),
+                pinning_opts: None,
             }
         );
 
         info.hostname = None;
         pretty_assertions::assert_eq!(
-            NetworkOptions::defaults_from(&setup, &info, None),
+            NetworkOptions::defaults_from(&setup, &info, None, None),
             NetworkOptions {
                 ifname: "eth0".to_owned(),
                 fqdn: Fqdn::from("pve.bar.com").unwrap(),
                 address: CidrAddress::new(Ipv4Addr::new(192, 168, 0, 2), 24).unwrap(),
                 gateway: IpAddr::V4(Ipv4Addr::new(192, 168, 0, 1)),
                 dns_server: Ipv4Addr::new(192, 168, 100, 1).into(),
+                pinning_opts: None,
             }
         );
 
         info.dns.domain = None;
         pretty_assertions::assert_eq!(
-            NetworkOptions::defaults_from(&setup, &info, None),
+            NetworkOptions::defaults_from(&setup, &info, None, None),
             NetworkOptions {
                 ifname: "eth0".to_owned(),
                 fqdn: Fqdn::from("pve.example.invalid").unwrap(),
                 address: CidrAddress::new(Ipv4Addr::new(192, 168, 0, 2), 24).unwrap(),
                 gateway: IpAddr::V4(Ipv4Addr::new(192, 168, 0, 1)),
                 dns_server: Ipv4Addr::new(192, 168, 100, 1).into(),
+                pinning_opts: None,
             }
         );
 
         info.hostname = Some("foo".to_owned());
         pretty_assertions::assert_eq!(
-            NetworkOptions::defaults_from(&setup, &info, None),
+            NetworkOptions::defaults_from(&setup, &info, None, None),
             NetworkOptions {
                 ifname: "eth0".to_owned(),
                 fqdn: Fqdn::from("foo.example.invalid").unwrap(),
                 address: CidrAddress::new(Ipv4Addr::new(192, 168, 0, 2), 24).unwrap(),
                 gateway: IpAddr::V4(Ipv4Addr::new(192, 168, 0, 1)),
                 dns_server: Ipv4Addr::new(192, 168, 100, 1).into(),
+                pinning_opts: None,
             }
         );
     }
@@ -751,37 +853,40 @@ mod tests {
         let (setup, mut info) = mock_setup_network();
 
         pretty_assertions::assert_eq!(
-            NetworkOptions::defaults_from(&setup, &info, None),
+            NetworkOptions::defaults_from(&setup, &info, None, None),
             NetworkOptions {
                 ifname: "eth0".to_owned(),
                 fqdn: Fqdn::from("foo.bar.com").unwrap(),
                 address: CidrAddress::new(Ipv4Addr::new(192, 168, 0, 2), 24).unwrap(),
                 gateway: IpAddr::V4(Ipv4Addr::new(192, 168, 0, 1)),
                 dns_server: Ipv4Addr::new(192, 168, 100, 1).into(),
+                pinning_opts: None,
             }
         );
 
         info.dns.domain = None;
         pretty_assertions::assert_eq!(
-            NetworkOptions::defaults_from(&setup, &info, Some("custom.local")),
+            NetworkOptions::defaults_from(&setup, &info, Some("custom.local"), None),
             NetworkOptions {
                 ifname: "eth0".to_owned(),
                 fqdn: Fqdn::from("foo.custom.local").unwrap(),
                 address: CidrAddress::new(Ipv4Addr::new(192, 168, 0, 2), 24).unwrap(),
                 gateway: IpAddr::V4(Ipv4Addr::new(192, 168, 0, 1)),
                 dns_server: Ipv4Addr::new(192, 168, 100, 1).into(),
+                pinning_opts: None,
             }
         );
 
         info.dns.domain = Some("some.domain.local".to_owned());
         pretty_assertions::assert_eq!(
-            NetworkOptions::defaults_from(&setup, &info, Some("custom.local")),
+            NetworkOptions::defaults_from(&setup, &info, Some("custom.local"), None),
             NetworkOptions {
                 ifname: "eth0".to_owned(),
                 fqdn: Fqdn::from("foo.custom.local").unwrap(),
                 address: CidrAddress::new(Ipv4Addr::new(192, 168, 0, 2), 24).unwrap(),
                 gateway: IpAddr::V4(Ipv4Addr::new(192, 168, 0, 1)),
                 dns_server: Ipv4Addr::new(192, 168, 100, 1).into(),
+                pinning_opts: None,
             }
         );
     }
@@ -794,6 +899,7 @@ mod tests {
             Interface {
                 name: "eth0".to_owned(),
                 index: 0,
+                pinned_id: "0".to_owned(),
                 state: InterfaceState::Up,
                 driver: "dummy".to_owned(),
                 mac: "01:23:45:67:89:ab".to_owned(),
@@ -814,14 +920,112 @@ mod tests {
         let setup = SetupInfo::mocked();
 
         pretty_assertions::assert_eq!(
-            NetworkOptions::defaults_from(&setup, &info, None),
+            NetworkOptions::defaults_from(&setup, &info, None, None),
             NetworkOptions {
                 ifname: "eth0".to_owned(),
                 fqdn: Fqdn::from("pve.example.invalid").unwrap(),
                 address: CidrAddress::new(Ipv4Addr::new(192, 168, 100, 2), 24).unwrap(),
                 gateway: IpAddr::V4(Ipv4Addr::new(192, 168, 100, 1)),
                 dns_server: Ipv4Addr::new(192, 168, 100, 1).into(),
+                pinning_opts: None,
             }
         );
     }
+
+    #[test]
+    fn network_interface_pinning_options_fail_on_empty_name() {
+        let mut options = NetworkInterfacePinningOptions::default();
+        options
+            .mapping
+            .insert("ab:cd:ef:12:34:56".to_owned(), String::new());
+
+        let res = options.verify();
+        assert!(res.is_err());
+        assert_eq!(
+            res.unwrap_err().to_string(),
+            "interface name for 'ab:cd:ef:12:34:56' cannot be empty"
+        )
+    }
+
+    #[test]
+    fn network_interface_pinning_options_fail_on_overlong_name() {
+        let mut options = NetworkInterfacePinningOptions::default();
+        options.mapping.insert(
+            "ab:cd:ef:12:34:56".to_owned(),
+            "waytoolonginterfacename".to_owned(),
+        );
+
+        let res = options.verify();
+        assert!(res.is_err());
+        assert_eq!(
+            res.unwrap_err().to_string(),
+            "interface name 'waytoolonginterfacename' for 'ab:cd:ef:12:34:56' cannot be longer than 15 characters"
+        )
+    }
+
+    #[test]
+    fn network_interface_pinning_options_fail_on_duplicate_name() {
+        let mut options = NetworkInterfacePinningOptions::default();
+        options
+            .mapping
+            .insert("ab:cd:ef:12:34:56".to_owned(), "nic0".to_owned());
+        options
+            .mapping
+            .insert("12:34:56:ab:cd:ef".to_owned(), "nic0".to_owned());
+
+        let res = options.verify();
+        assert!(res.is_err());
+        let err = res.unwrap_err().to_string();
+
+        // [HashMap] does not guarantee iteration order, so just check for the substrings
+        // we expect to find
+        assert!(err.contains("duplicate interface name mapping 'nic0' for: "));
+        assert!(err.contains("12:34:56:ab:cd:ef"));
+        assert!(err.contains("ab:cd:ef:12:34:56"));
+    }
+
+    #[test]
+    fn network_interface_pinning_options_fail_on_invalid_characters() {
+        let mut options = NetworkInterfacePinningOptions::default();
+        options
+            .mapping
+            .insert("ab:cd:ef:12:34:56".to_owned(), "nic-".to_owned());
+
+        let res = options.verify();
+        assert!(res.is_err());
+        assert_eq!(
+            res.unwrap_err().to_string(),
+            "interface name 'nic-' for 'ab:cd:ef:12:34:56' is invalid: name must only consist of alphanumeric characters and underscores"
+        )
+    }
+
+    #[test]
+    fn network_interface_pinning_options_fail_on_name_starting_with_number() {
+        let mut options = NetworkInterfacePinningOptions::default();
+        options
+            .mapping
+            .insert("ab:cd:ef:12:34:56".to_owned(), "0nic".to_owned());
+
+        let res = options.verify();
+        assert!(res.is_err());
+        assert_eq!(
+            res.unwrap_err().to_string(),
+            "interface name '0nic' for 'ab:cd:ef:12:34:56' is invalid: name must not start with a number"
+        )
+    }
+
+    #[test]
+    fn network_interface_pinning_options_fail_on_fully_numeric_name() {
+        let mut options = NetworkInterfacePinningOptions::default();
+        options
+            .mapping
+            .insert("ab:cd:ef:12:34:56".to_owned(), "12345".to_owned());
+
+        let res = options.verify();
+        assert!(res.is_err());
+        assert_eq!(
+            res.unwrap_err().to_string(),
+            "interface name '12345' for 'ab:cd:ef:12:34:56' is invalid: name must not be fully numeric"
+        )
+    }
 }
diff --git a/proxmox-installer-common/src/setup.rs b/proxmox-installer-common/src/setup.rs
index 4873fff..3e99576 100644
--- a/proxmox-installer-common/src/setup.rs
+++ b/proxmox-installer-common/src/setup.rs
@@ -3,6 +3,7 @@ use std::{
     collections::{BTreeMap, HashMap},
     fmt,
     fs::File,
+    hash::Hash,
     io::{self, BufReader},
     net::IpAddr,
     path::{Path, PathBuf},
@@ -13,8 +14,8 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer, de};
 
 use crate::{
     options::{
-        BtrfsBootdiskOptions, BtrfsCompressOption, Disk, FsType, ZfsBootdiskOptions,
-        ZfsChecksumOption, ZfsCompressOption,
+        BtrfsBootdiskOptions, BtrfsCompressOption, Disk, FsType, NetworkInterfacePinningOptions,
+        ZfsBootdiskOptions, ZfsChecksumOption, ZfsCompressOption,
     },
     utils::CidrAddress,
 };
@@ -443,8 +444,9 @@ pub struct Gateway {
     pub gateway: IpAddr,
 }
 
-#[derive(Clone, Deserialize)]
+#[derive(Clone, Deserialize, Serialize)]
 #[serde(rename_all = "UPPERCASE")]
+/// The current interface state as reported by the kernel.
 pub enum InterfaceState {
     Up,
     Down,
@@ -452,6 +454,8 @@ pub enum InterfaceState {
     Unknown,
 }
 
+serde_plain::derive_display_from_serialize!(InterfaceState);
+
 impl InterfaceState {
     // avoid display trait as this is not the string representation for a serializer
     pub fn render(&self) -> String {
@@ -469,6 +473,9 @@ pub struct Interface {
 
     pub index: usize,
 
+    /// Sequential interface ID for pinning interface names.
+    pub pinned_id: String,
+
     pub mac: String,
 
     pub state: InterfaceState,
@@ -484,7 +491,22 @@ pub struct Interface {
 impl Interface {
     // avoid display trait as this is not the string representation for a serializer
     pub fn render(&self) -> String {
-        format!("{} {}", self.state.render(), self.name)
+        format!("{} {} ({})", self.state.render(), self.name, self.mac)
+    }
+
+    pub fn to_pinned(&self, options: &NetworkInterfacePinningOptions) -> Self {
+        let mut this = self.clone();
+        this.name = options
+            .mapping
+            .get(&this.mac)
+            .unwrap_or(&format!(
+                "{}{}",
+                NetworkInterfacePinningOptions::DEFAULT_PREFIX,
+                this.pinned_id
+            ))
+            .clone();
+
+        this
     }
 }
 
@@ -577,6 +599,14 @@ pub struct InstallConfig {
     pub root_ssh_keys: Vec<String>,
 
     pub mngmt_nic: String,
+    // Maps MAC addresses -> custom name. If set, enables pinning for all
+    // interfaces present.
+    #[serde(
+        default,
+        skip_serializing_if = "HashMap::is_empty",
+        deserialize_with = "deserialize_optional_map"
+    )]
+    pub network_interface_pin_map: HashMap<String, String>,
 
     pub hostname: String,
     pub domain: String,
@@ -610,3 +640,16 @@ pub enum LowLevelMessage {
         text: Option<String>,
     },
 }
+
+/// Deserializes an optional [HashMap].
+///
+/// If missing, it returns an empty map, otherwise the deserialized map.
+fn deserialize_optional_map<'de, D, K, V>(deserializer: D) -> Result<HashMap<K, V>, D::Error>
+where
+    D: Deserializer<'de>,
+    K: Eq + Hash + Deserialize<'de>,
+    V: Deserialize<'de>,
+{
+    let map: Option<HashMap<K, V>> = Deserialize::deserialize(deserializer)?;
+    Ok(map.unwrap_or_default())
+}
diff --git a/proxmox-tui-installer/src/setup.rs b/proxmox-tui-installer/src/setup.rs
index b90c7dc..d2ffb70 100644
--- a/proxmox-tui-installer/src/setup.rs
+++ b/proxmox-tui-installer/src/setup.rs
@@ -1,4 +1,4 @@
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, HashMap};
 
 use crate::options::InstallerOptions;
 use proxmox_installer_common::{
@@ -32,6 +32,7 @@ impl From<InstallerOptions> for InstallConfig {
             root_ssh_keys: vec![],
 
             mngmt_nic: options.network.ifname,
+            network_interface_pin_map: HashMap::new(),
 
             // Safety: At this point, it is know that we have a valid FQDN, as
             // this is set by the TUI network panel, which only lets the user
-- 
2.51.0



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