From: "Michael Köppl" <m.koeppl@proxmox.com>
To: "Proxmox VE development discussion" <pve-devel@lists.proxmox.com>
Cc: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH installer v2 06/15] common: implement support for `network_interface_pin_map` config
Date: Tue, 04 Nov 2025 14:55:02 +0100 [thread overview]
Message-ID: <DDZYMO6C9828.XUZMHJ4YQY0K@proxmox.com> (raw)
In-Reply-To: <20251030110627.812398-7-c.heiss@proxmox.com>
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());
> +
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
next prev parent reply other threads:[~2025-11-04 13:55 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-30 11:06 [pve-devel] [PATCH installer v2 00/15] support network interface name pinning Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 01/15] test: parse-kernel-cmdline: fix module import statement Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 02/15] install: add support for network interface name pinning Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 03/15] run env: network: add kernel driver name to network interface info Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 04/15] common: utils: fix clippy warnings Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 05/15] common: setup: simplify network address list serialization Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 06/15] common: implement support for `network_interface_pin_map` config Christoph Heiss
2025-11-04 13:55 ` Michael Köppl [this message]
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 07/15] auto: add support for pinning network interface names Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 08/15] assistant: verify network settings in `validate-answer` subcommand Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 09/15] post-hook: avoid redundant Option<bool> for (de-)serialization Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 10/15] post-hook: add network interface name and pinning status Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 11/15] tui: views: move network options view to own module Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 12/15] tui: views: form: allow attaching user-defined data to children Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 13/15] tui: add support for pinning network interface names Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 14/15] ui: gtk3: allow passing of dialog parent window Christoph Heiss
2025-10-30 11:06 ` [pve-devel] [PATCH installer v2 15/15] gui: add support for pinning network interface names Christoph Heiss
2025-11-04 15:38 ` [pve-devel] [PATCH installer v2 00/15] support network interface name pinning Michael Köppl
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DDZYMO6C9828.XUZMHJ4YQY0K@proxmox.com \
--to=m.koeppl@proxmox.com \
--cc=pve-devel-bounces@lists.proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox