Re: [pve-devel] [PATCH proxmox-perl-rs v4 4/5] pve-rs: sdn: fabrics: add helper to generate ifupdown2 configuration

[Gabriel Goller <g.goller@proxmox.com>]

>> [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<String, Error> {
>> +        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<String, Error> {
>> +        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