From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 72B6D1FF191 for ; Tue, 4 Nov 2025 14:55:00 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 326B5E692; Tue, 4 Nov 2025 14:55:38 +0100 (CET) Mime-Version: 1.0 Date: Tue, 04 Nov 2025 14:55:02 +0100 Message-Id: Cc: "pve-devel" From: =?utf-8?q?Michael_K=C3=B6ppl?= To: "Proxmox VE development discussion" X-Mailer: aerc 0.21.0 References: <20251030110627.812398-1-c.heiss@proxmox.com> <20251030110627.812398-7-c.heiss@proxmox.com> In-Reply-To: <20251030110627.812398-7-c.heiss@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1762264485005 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.037 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH installer v2 06/15] common: implement support for `network_interface_pin_map` config X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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::::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, > } > > 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