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 02A8E1FF17A for ; Fri, 4 Jul 2025 18:15:03 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9AC193B9F2; Fri, 4 Jul 2025 18:15:42 +0200 (CEST) Date: Fri, 4 Jul 2025 18:15:38 +0200 From: Gabriel Goller To: Wolfgang Bumiller Message-ID: Mail-Followup-To: Wolfgang Bumiller , pve-devel@lists.proxmox.com References: <20250702145101.894299-1-g.goller@proxmox.com> <20250702145101.894299-33-g.goller@proxmox.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20241002-35-39f9a6 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.124 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.218 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [fabrics.rs] Subject: Re: [pve-devel] [PATCH proxmox-perl-rs v4 4/5] pve-rs: sdn: fabrics: add helper to generate ifupdown2 configuration 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 Cc: pve-devel@lists.proxmox.com Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" >> [snip] >> diff --git a/pve-rs/src/bindings/sdn/fabrics.rs b/pve-rs/src/bindings/sdn/fabrics.rs >> index a7a740f5aac9..099c1a7ab515 100644 >> --- a/pve-rs/src/bindings/sdn/fabrics.rs >> +++ b/pve-rs/src/bindings/sdn/fabrics.rs >> @@ -6,6 +6,8 @@ pub mod pve_rs_sdn_fabrics { >> //! / writing the configuration, as well as for generating ifupdown2 and FRR configuration. >> >> use std::collections::{BTreeMap, HashSet}; >> + use std::fmt::Write; >> + use std::net::IpAddr; >> use std::ops::Deref; >> use std::sync::Mutex; >> >> @@ -15,6 +17,7 @@ pub mod pve_rs_sdn_fabrics { >> >> use perlmod::Value; >> use proxmox_frr::serializer::to_raw_config; >> + use proxmox_network_types::ip_address::Cidr; >> use proxmox_section_config::typed::SectionConfigData; >> use proxmox_ve_config::common::valid::Validatable; >> >> @@ -349,4 +352,105 @@ pub mod pve_rs_sdn_fabrics { >> >> to_raw_config(&frr_config) >> } >> + >> + /// Helper method to generate the default /e/n/i config for a given CIDR. > >^ Not sure we want to shorten it like that, but at least put backticks >around `/e/n/i` ;-) Agree, changed to full path and added backticks :) >> + fn render_interface(name: &str, cidr: Cidr, is_dummy: bool) -> Result { >> + let mut interface = String::new(); >> + >> + writeln!(interface)?; > >^ In our doc generator I recently removed all the leading newlines (and >the trailing ones except for the one ending the final line) because the >inconsistency across the building blocks became an unmaintainable mess. Good point. >Do we really want to start off with a newline here, rather than just say >"this creates one stanza and it's the caller's responsibility to not >merge it together with whatever comes before it"? I could remove the newline here and then add it down below where this helper is called. Although this will be probably be reworked quite soon anyway as I think for wireguard we'll implement some nicer ifupdown serialization library thingy. >Also this is IMO kind of a cryptic way (which includes error handling!) >or starting with `let mut interface = "\n".to_string();`. (Or heck even >`let interface = format!("\nauto {name}\n");`) > >> + writeln!(interface, "auto {name}")?; >> + match cidr { >> + Cidr::Ipv4(_) => writeln!(interface, "iface {name} inet static")?, >> + Cidr::Ipv6(_) => writeln!(interface, "iface {name} inet6 static")?, >> + } >> + writeln!(interface, "\taddress {cidr}")?; >> + if is_dummy { >> + writeln!(interface, "\tlink-type dummy")?; >> + } >> + writeln!(interface, "\tip-forward 1")?; >> + >> + Ok(interface) >> + } >> + >> + /// Class method: Generate the ifupdown2 configuration for a given node. >> + #[export] >> + fn get_interfaces_etc_network_config( >> + #[try_from_ref] this: &PerlFabricConfig, >> + node_id: NodeId, >> + ) -> Result { >> + let config = this.fabric_config.lock().unwrap(); >> + let mut interfaces = String::new(); >> + >> + let node_fabrics = config.values().filter_map(|entry| { >> + entry >> + .get_node(&node_id) >> + .map(|node| (entry.fabric(), node)) >> + .ok() >> + }); >> + >> + for (fabric, node) in node_fabrics { >> + // dummy interface >> + if let Some(ip) = node.ip() { >> + let interface = render_interface( >> + &format!("dummy_{}", fabric.id()), >> + Cidr::new_v4(ip, 32)?, >> + true, >> + )?; >> + write!(interfaces, "{interface}")?; s/write/writeln so that we have a newline after every interface definiton. >> + } >> + if let Some(ip6) = node.ip6() { >> + let interface = render_interface( >> + &format!("dummy_{}", fabric.id()), >> + Cidr::new_v6(ip6, 128)?, >> + true, >> + )?; >> + write!(interfaces, "{interface}")?; >> + } >> + match node { >> + ConfigNode::Openfabric(node_section) => { >> + for interface in node_section.properties().interfaces() { >> + if let Some(ip) = interface.ip() { >> + let interface = >> + render_interface(interface.name(), Cidr::from(ip), false)?; >> + write!(interfaces, "{interface}")?; >> + } >> + if let Some(ip) = interface.ip6() { >> + let interface = >> + render_interface(interface.name(), Cidr::from(ip), false)?; >> + write!(interfaces, "{interface}")?; >> + } >> + >> + // If not ip is configured, add auto and empty iface to bring interface up >> + if let (None, None) = (interface.ip(), interface.ip6()) { >> + writeln!(interfaces)?; >> + writeln!(interfaces, "auto {}", interface.name())?; >> + writeln!(interfaces, "iface {}", interface.name())?; >> + writeln!(interfaces, "\tip-forward 1")?; >> + } >> + } >> + } >> + ConfigNode::Ospf(node_section) => { >> + for interface in node_section.properties().interfaces() { >> + if let Some(ip) = interface.ip() { >> + let interface = >> + render_interface(interface.name(), Cidr::from(ip), false)?; >> + write!(interfaces, "{interface}")?; >> + } else { >> + let interface = render_interface( >> + interface.name(), >> + Cidr::from(IpAddr::from( >> + node.ip() >> + .ok_or(anyhow::anyhow!("there has to be a ipv4 address"))?, > >^ use `ok_or_else` here please, unless there's a documented guarantee >that this is cheap and does not allocate? Agree, added `ok_or_else`. >> [snip] Thanks for the review! _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel