[pve-devel] [PATCH installer v2 06/15] common: implement support for `network_interface_pin_map` config
Michael Köppl
m.koeppl at proxmox.com
Tue Nov 4 14:55:02 CET 2025
left 2 comments inline
On Thu Oct 30, 2025 at 12:06 PM CET, Christoph Heiss wrote:
> +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 mapping for '{mac}' cannot be empty");
> + }
> +
> + if name.len() > MAX_IFNAME_LEN {
> + bail!(
> + "interface name mapping '{name}' for '{mac}' cannot be longer than {} characters",
> + MAX_IFNAME_LEN
> + );
> + }
> +
> + // Mimicking the `pve-iface` schema verification
> + if !name
> + .chars()
> + .nth(0)
> + .map(|c| c.is_ascii_alphabetic())
> + .unwrap_or_default()
> + {
> + bail!(
> + "interface name '{name}' for '{mac}' is invalid: name must not start with a number"
> + );
> + }
The ordering of this is problematic, no? If this check comes before the
check for fully numeric interface names, then you can never really get
to the check for fully numeric interface names because it will always
fail here.
Also, as an alternative suggestion:
```
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"
);
}
```
I think it makes the intent a bit more clear IMO.
> +
> + 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 name.chars().all(char::is_numeric) {
> + bail!("interface name '{name}' for '{mac}' is invalid: must not be fully numeric");
> + }
> +
> + if let Some(duplicate_mac) = reverse_mapping.get(name) {
> + if 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 +557,7 @@ pub struct NetworkOptions {
> pub address: CidrAddress,
> pub gateway: IpAddr,
> pub dns_server: IpAddr,
> + pub pinning_opts: Option<NetworkInterfacePinningOptions>,
> }
>
> impl NetworkOptions {
> @@ -492,6 +567,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,6 +583,7 @@ 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() {
> @@ -517,7 +594,11 @@ impl NetworkOptions {
> let mut filled = false;
> if let Some(gw) = &routes.gateway4 {
> if let Some(iface) = network.interfaces.get(&gw.dev) {
> - 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(addr) = iface.addresses.iter().find(|addr| addr.is_ipv4()) {
> this.gateway = gw.gateway;
> this.address = addr.clone();
> @@ -529,7 +610,11 @@ impl NetworkOptions {
> if let Some(gw) = &routes.gateway6 {
> if let Some(iface) = network.interfaces.get(&gw.dev) {
> if let Some(addr) = iface.addresses.iter().find(|addr| addr.is_ipv6()) {
> - 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();
> }
> @@ -544,7 +629,20 @@ impl NetworkOptions {
> // earlier in that case, so use the first one enumerated.
> if this.ifname.is_empty() {
> if 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);
> }
> }
>
> @@ -672,6 +770,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(),
> @@ -703,49 +802,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,
> }
> );
> }
> @@ -755,37 +858,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,
> }
> );
> }
> @@ -798,6 +904,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(),
> @@ -818,14 +925,67 @@ 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 mapping 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 mapping 'waytoolonginterfacename' for 'ab:cd:ef:12:34:56' cannot be longer than 15 characters"
> + )
> + }
> +
IMO it would be nice to also have tests for the new checks:
```
#[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_invalid_characters() {
let mut options = NetworkInterfacePinningOptions::default();
options
.mapping
.insert("ab:cd:ef:12:34:56".to_owned(), "nic-0".to_owned());
let res = options.verify();
assert!(res.is_err());
assert_eq!(
res.unwrap_err().to_string(),
"interface name 'nic-0' 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_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: must not be fully numeric"
)
}
```
> + #[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());
> +
More information about the pve-devel
mailing list