Re: [pbs-devel] [PATCH proxmox-backup 07/10] api: create and update vlan interfaces

[Lukas Wagner <l.wagner@proxmox.com>]

On 1/11/24 16:53, Stefan Lendl wrote:
> * Implement setting vlan-id and vlan-raw-device in create_ and update_interface.
> * Checking if vlan-raw-device exists
> * Moved VLAN_INTERFACE_REGEX to top level network module to use in
>    checking functions there. Changed to match with named capture groups.
> * Unit tests to verify parsing vlan_id and vlan_raw_device from name
> 
> Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
> ---
>   pbs-config/src/network/mod.rs    | 35 +++++++++++++++++++
>   pbs-config/src/network/parser.rs |  3 +-
>   src/api2/node/network.rs         | 58 +++++++++++++++++++++++++++++++-
>   3 files changed, 94 insertions(+), 2 deletions(-)
> 
> diff --git a/pbs-config/src/network/mod.rs b/pbs-config/src/network/mod.rs
> index 9ecc66b8..bc49aded 100644
> --- a/pbs-config/src/network/mod.rs
> +++ b/pbs-config/src/network/mod.rs
> @@ -25,6 +25,8 @@ use crate::{open_backup_lockfile, BackupLockGuard};
>   
>   lazy_static! {
>       static ref PHYSICAL_NIC_REGEX: Regex = Regex::new(r"^(?:eth\d+|en[^:.]+|ib\d+)$").unwrap();
> +    static ref VLAN_INTERFACE_REGEX: Regex =
> +        Regex::new(r"^(?P<vlan_raw_device>\S+)\.(?P<vlan_id>\d+)|vlan(?P<vlan_id2>\d+)$").unwrap();
>   }
>   
>   pub fn is_physical_nic(iface: &str) -> bool {
> @@ -41,6 +43,21 @@ pub fn bond_xmit_hash_policy_from_str(s: &str) -> Result<BondXmitHashPolicy, Err
>           .map_err(|_: value::Error| format_err!("invalid bond_xmit_hash_policy '{}'", s))
>   }
>   
> +pub fn parse_vlan_id_from_name(iface_name: &str) -> Option<u16> {
> +    VLAN_INTERFACE_REGEX.captures(iface_name).and_then(|cap| {
> +        cap.name("vlan_id")
> +            .or(cap.name("vlan_id2"))
> +            .and_then(|id| id.as_str().parse::<u16>().ok())
> +    })
> +}
> +
> +pub fn parse_vlan_raw_device_from_name(iface_name: &str) -> Option<&str> {
> +    VLAN_INTERFACE_REGEX
> +        .captures(iface_name)
> +        .and_then(|cap| cap.name("vlan_raw_device"))
> +        .map(Into::into)
> +}
> +
>   // Write attributes not depending on address family
>   fn write_iface_attributes(iface: &Interface, w: &mut dyn Write) -> Result<(), Error> {
>       static EMPTY_LIST: Vec<String> = Vec::new();
> @@ -652,4 +669,22 @@ iface individual_name inet manual
>                   .trim()
>           );
>       }
> +
> +    #[test]
> +    fn test_vlan_parse_vlan_id_from_name() {
> +        assert_eq!(parse_vlan_id_from_name("vlan100"), Some(100));
> +        assert_eq!(parse_vlan_id_from_name("vlan"), None);
> +        assert_eq!(parse_vlan_id_from_name("arbitrary"), None);
> +        assert_eq!(parse_vlan_id_from_name("vmbr0.100"), Some(100));
> +        assert_eq!(parse_vlan_id_from_name("vmbr0"), None);
> +        // assert_eq!(parse_vlan_id_from_name("vmbr0.1.400"), Some(400));   // NOTE ifupdown2 does actually support this
> +    }
> +
> +    #[test]
> +    fn test_vlan_parse_vlan_raw_device_from_name() {
> +        assert_eq!(parse_vlan_raw_device_from_name("vlan100"), None);
> +        assert_eq!(parse_vlan_raw_device_from_name("arbitrary"), None);
> +        assert_eq!(parse_vlan_raw_device_from_name("vmbr0"), None);
> +        assert_eq!(parse_vlan_raw_device_from_name("vmbr0.200"), Some("vmbr0"));
> +    }
>   }
> diff --git a/pbs-config/src/network/parser.rs b/pbs-config/src/network/parser.rs
> index 0c178d9b..d66267b3 100644
> --- a/pbs-config/src/network/parser.rs
> +++ b/pbs-config/src/network/parser.rs
> @@ -1,3 +1,5 @@
> +use crate::network::VLAN_INTERFACE_REGEX;
> +
>   use std::collections::{HashMap, HashSet};
>   use std::io::BufRead;
>   use std::iter::{Iterator, Peekable};
> @@ -536,7 +538,6 @@ impl<R: BufRead> NetworkParser<R> {
>   
>           lazy_static! {
>               static ref INTERFACE_ALIAS_REGEX: Regex = Regex::new(r"^\S+:\d+$").unwrap();
> -            static ref VLAN_INTERFACE_REGEX: Regex = Regex::new(r"^\S+\.\d+|vlan\d+$").unwrap();
>           }
>   
>           if let Some(existing_interfaces) = existing_interfaces {
> diff --git a/src/api2/node/network.rs b/src/api2/node/network.rs
> index 187b27a0..d1393103 100644
> --- a/src/api2/node/network.rs
> +++ b/src/api2/node/network.rs
> @@ -12,7 +12,9 @@ use pbs_api_types::{
>       NETWORK_INTERFACE_ARRAY_SCHEMA, NETWORK_INTERFACE_LIST_SCHEMA, NETWORK_INTERFACE_NAME_SCHEMA,
>       NODE_SCHEMA, PRIV_SYS_AUDIT, PRIV_SYS_MODIFY, PROXMOX_CONFIG_DIGEST_SCHEMA,
>   };
> -use pbs_config::network::{self, NetworkConfig};
> +use pbs_config::network::{
> +    self, parse_vlan_id_from_name, parse_vlan_raw_device_from_name, NetworkConfig,
> +};
>   
>   use proxmox_rest_server::WorkerTask;
>   
> @@ -231,6 +233,15 @@ pub fn read_interface(iface: String) -> Result<Value, Error> {
>                   type: bool,
>                   optional: true,
>               },
> +            "vlan-id": {
> +                description: "VLAN ID.",
> +                type: u16,
> +                optional: true,
> +            },
> +            "vlan-raw-device": {
> +                schema: NETWORK_INTERFACE_NAME_SCHEMA,
> +                optional: true,
> +            },
>               bond_mode: {
>                   type: LinuxBondMode,
>                   optional: true,
> @@ -269,6 +280,8 @@ pub fn create_interface(
>       mtu: Option<u64>,
>       bridge_ports: Option<String>,
>       bridge_vlan_aware: Option<bool>,
> +    vlan_id: Option<u16>,
> +    vlan_raw_device: Option<String>,
>       bond_mode: Option<LinuxBondMode>,
>       bond_primary: Option<String>,
>       bond_xmit_hash_policy: Option<BondXmitHashPolicy>,
> @@ -373,6 +386,22 @@ pub fn create_interface(
>                   set_bond_slaves(&mut interface, slaves)?;
>               }
>           }
> +        NetworkInterfaceType::Vlan => {
> +            if vlan_id.is_none() && parse_vlan_id_from_name(&iface).is_none() {
> +                bail!("vlan-id must be set");
> +            }
> +            interface.vlan_id = vlan_id;
> +
> +            if let Some(dev) = vlan_raw_device
> +                .as_deref()
> +                .or_else(|| parse_vlan_raw_device_from_name(&iface))
> +            {
> +                if !config.interfaces.contains_key(dev) {
> +                    bail!("vlan-raw-device {dev} does not exist");
> +                }
> +            }
> +            interface.vlan_raw_device = vlan_raw_device;
> +        }
>           _ => bail!(
>               "creating network interface type '{:?}' is not supported",
>               interface_type
> @@ -507,6 +536,15 @@ pub enum DeletableProperty {
>                   type: bool,
>                   optional: true,
>               },
> +            "vlan-id": {
> +                description: "VLAN ID.",
> +                type: u16,
> +                optional: true,
> +            },
> +            "vlan-raw-device": {
> +                schema: NETWORK_INTERFACE_NAME_SCHEMA,
> +                optional: true,
> +            },
>               bond_mode: {
>                   type: LinuxBondMode,
>                   optional: true,
> @@ -557,6 +595,8 @@ pub fn update_interface(
>       mtu: Option<u64>,
>       bridge_ports: Option<String>,
>       bridge_vlan_aware: Option<bool>,
> +    vlan_id: Option<u16>,
> +    vlan_raw_device: Option<String>,
>       bond_mode: Option<LinuxBondMode>,
>       bond_primary: Option<String>,
>       bond_xmit_hash_policy: Option<BondXmitHashPolicy>,
> @@ -581,6 +621,19 @@ pub fn update_interface(
>           check_duplicate_gateway_v6(&config, &iface)?;
>       }
>   
> +    if let Some(dev) = vlan_raw_device
> +        .as_deref()
> +        .or_else(|| parse_vlan_raw_device_from_name(&iface))
> +    {
> +        if !config.interfaces.contains_key(dev) {
> +            bail!("vlan-raw-device {dev} does not exist");
> +        }
> +    }
> +
> +    if vlan_id.is_none() && parse_vlan_id_from_name(&iface).is_none() {
> +        bail!("vlan-id must be set");
> +    }

This seems to break updating existing interfaces/bridge.
E.g. when updating 'comment' for a bridge/interface, this seems to be 
triggered. I guess you would need to check first if this is actually a 
VLAN device?


> +
>       let interface = config.lookup_mut(&iface)?;
>   
>       if let Some(interface_type) = param.get("type") {
> @@ -734,6 +787,9 @@ pub fn update_interface(
>           interface.method6 = Some(NetworkConfigMethod::Manual);
>       }
>   
> +    interface.vlan_id = vlan_id;
> +    interface.vlan_raw_device = vlan_raw_device;
> +
>       network::save_config(&config)?;
>   
>       Ok(())

-- 
- Lukas