[pbs-devel] [PATCH widget-toolkit/proxmox-backup 00/10] Fix #3115: VLAN Network Interface Configuration

[pbs-devel] [PATCH widget-toolkit/proxmox-backup 00/10] Fix #3115: VLAN Network Interface Configuration

[Stefan Lendl]

This patch series allows configuration of VLAN network interfaces in the PBS GUI

* Adds reading and writing of vlan network interface configs from network/interfaces
* Creating and updating of VLAN network interfaces via the API
* Copy the VlanId widget from PVE and enable the Linux VLAN interface type in the GUI
* Unit tests
* Some cleanup to leave touched functions cleaner than before

backup:

Stefan Lendl (9):
  tests: move network tests to parser.rs
  tests: rudimentary NetworkConfig.write_config tests
  config: write vlan network interface
  config: parse vlan interface from config
  config: remove unnecessary pub in various methods in NetworkConfig
  fmt: fix intendation in api macro
  api: create and update vlan interfaces
  refactor(api): simplify setting interface properties
  ui: enable vlan widget

 pbs-api-types/src/network.rs     |  17 ++
 pbs-config/src/network/lexer.rs  |   6 +
 pbs-config/src/network/mod.rs    | 300 +++++++++++++++++--------------
 pbs-config/src/network/parser.rs | 247 ++++++++++++++++++++++++-
 src/api2/node/network.rs         | 124 ++++++++-----
 www/SystemConfiguration.js       |   2 +-
 6 files changed, 515 insertions(+), 181 deletions(-)


widget-toolkit:

Stefan Lendl (1):
  form: include VlanField from PVE

 src/Makefile            |  1 +
 src/form/VlanField.js   | 40 ++++++++++++++++++++++++++++++++++++++++
 src/node/NetworkEdit.js |  6 +++---
 3 files changed, 44 insertions(+), 3 deletions(-)
 create mode 100644 src/form/VlanField.js


Summary over all repositories:
  9 files changed, 559 insertions(+), 0 deletions(-)

-- 
Generated by git-murpp 0.5.0

[pbs-devel] [PATCH proxmox-backup 01/10] tests: move network tests to parser.rs

[Stefan Lendl]

All current network/mod.rs tests only test parser functionality

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 pbs-config/src/network/mod.rs    | 147 -------------------------------
 pbs-config/src/network/parser.rs | 147 +++++++++++++++++++++++++++++++
 2 files changed, 147 insertions(+), 147 deletions(-)

diff --git a/pbs-config/src/network/mod.rs b/pbs-config/src/network/mod.rs
index e2008b7c..21187ec2 100644
--- a/pbs-config/src/network/mod.rs
+++ b/pbs-config/src/network/mod.rs
@@ -503,150 +503,3 @@ pub fn complete_port_list(arg: &str, _param: &HashMap<String, String>) -> Vec<St
         .map(|port| format!("{}{}", prefix, port))
         .collect()
 }
-
-#[cfg(test)]
-mod test {
-
-    use anyhow::Error;
-
-    use super::*;
-
-    #[test]
-    fn test_network_config_create_lo_1() -> Result<(), Error> {
-        let input = "";
-
-        let mut parser = NetworkParser::new(input.as_bytes());
-
-        let config = parser.parse_interfaces(None)?;
-
-        let output = String::try_from(config)?;
-
-        let expected = "auto lo\niface lo inet loopback\n\n";
-        assert_eq!(output, expected);
-
-        // run again using output as input
-        let mut parser = NetworkParser::new(output.as_bytes());
-
-        let config = parser.parse_interfaces(None)?;
-
-        let output = String::try_from(config)?;
-
-        assert_eq!(output, expected);
-
-        Ok(())
-    }
-
-    #[test]
-    fn test_network_config_create_lo_2() -> Result<(), Error> {
-        let input = "#c1\n\n#c2\n\niface test inet manual\n";
-
-        let mut parser = NetworkParser::new(input.as_bytes());
-
-        let config = parser.parse_interfaces(None)?;
-
-        let output = String::try_from(config)?;
-
-        // Note: loopback should be added in front of other interfaces
-        let expected = "#c1\n#c2\n\nauto lo\niface lo inet loopback\n\niface test inet manual\n\n";
-        assert_eq!(output, expected);
-
-        Ok(())
-    }
-
-    #[test]
-    fn test_network_config_parser_no_blank_1() -> Result<(), Error> {
-        let input = "auto lo\n\
-                     iface lo inet loopback\n\
-                     iface lo inet6 loopback\n\
-                     auto ens18\n\
-                     iface ens18 inet static\n\
-                     \taddress 192.168.20.144/20\n\
-                     \tgateway 192.168.16.1\n\
-                     # comment\n\
-                     iface ens20 inet static\n\
-                     \taddress 192.168.20.145/20\n\
-                     iface ens21 inet manual\n\
-                     iface ens22 inet manual\n";
-
-        let mut parser = NetworkParser::new(input.as_bytes());
-
-        let config = parser.parse_interfaces(None)?;
-
-        let output = String::try_from(config)?;
-
-        let expected = "auto lo\n\
-                        iface lo inet loopback\n\
-                        \n\
-                        iface lo inet6 loopback\n\
-                        \n\
-                        auto ens18\n\
-                        iface ens18 inet static\n\
-                        \taddress 192.168.20.144/20\n\
-                        \tgateway 192.168.16.1\n\
-                        #comment\n\
-                        \n\
-                        iface ens20 inet static\n\
-                        \taddress 192.168.20.145/20\n\
-                        \n\
-                        iface ens21 inet manual\n\
-                        \n\
-                        iface ens22 inet manual\n\
-                        \n";
-        assert_eq!(output, expected);
-
-        Ok(())
-    }
-
-    #[test]
-    fn test_network_config_parser_no_blank_2() -> Result<(), Error> {
-        // Adapted from bug 2926
-        let input = "### Hetzner Online GmbH installimage\n\
-                     \n\
-                     source /etc/network/interfaces.d/*\n\
-                     \n\
-                     auto lo\n\
-                     iface lo inet loopback\n\
-                     iface lo inet6 loopback\n\
-                     \n\
-                     auto enp4s0\n\
-                     iface enp4s0 inet static\n\
-                     \taddress 10.10.10.10/24\n\
-                     \tgateway 10.10.10.1\n\
-                     \t# route 10.10.20.10/24 via 10.10.20.1\n\
-                     \tup route add -net 10.10.20.10 netmask 255.255.255.0 gw 10.10.20.1 dev enp4s0\n\
-                     \n\
-                     iface enp4s0 inet6 static\n\
-                     \taddress fe80::5496:35ff:fe99:5a6a/64\n\
-                     \tgateway fe80::1\n";
-
-        let mut parser = NetworkParser::new(input.as_bytes());
-
-        let config = parser.parse_interfaces(None)?;
-
-        let output = String::try_from(config)?;
-
-        let expected = "### Hetzner Online GmbH installimage\n\
-                        \n\
-                        source /etc/network/interfaces.d/*\n\
-                        \n\
-                        auto lo\n\
-                        iface lo inet loopback\n\
-                        \n\
-                        iface lo inet6 loopback\n\
-                        \n\
-                        auto enp4s0\n\
-                        iface enp4s0 inet static\n\
-                        \taddress 10.10.10.10/24\n\
-                        \tgateway 10.10.10.1\n\
-                        \t# route 10.10.20.10/24 via 10.10.20.1\n\
-                        \tup route add -net 10.10.20.10 netmask 255.255.255.0 gw 10.10.20.1 dev enp4s0\n\
-                        \n\
-                        iface enp4s0 inet6 static\n\
-                        \taddress fe80::5496:35ff:fe99:5a6a/64\n\
-                        \tgateway fe80::1\n\
-                        \n";
-        assert_eq!(output, expected);
-
-        Ok(())
-    }
-}
diff --git a/pbs-config/src/network/parser.rs b/pbs-config/src/network/parser.rs
index 2cff6587..d31e5c2e 100644
--- a/pbs-config/src/network/parser.rs
+++ b/pbs-config/src/network/parser.rs
@@ -602,3 +602,150 @@ impl<R: BufRead> NetworkParser<R> {
         Ok(config)
     }
 }
+
+#[cfg(test)]
+mod test {
+
+    use anyhow::Error;
+
+    use super::*;
+
+    #[test]
+    fn test_network_config_create_lo_1() -> Result<(), Error> {
+        let input = "";
+
+        let mut parser = NetworkParser::new(input.as_bytes());
+
+        let config = parser.parse_interfaces(None)?;
+
+        let output = String::try_from(config)?;
+
+        let expected = "auto lo\niface lo inet loopback\n\n";
+        assert_eq!(output, expected);
+
+        // run again using output as input
+        let mut parser = NetworkParser::new(output.as_bytes());
+
+        let config = parser.parse_interfaces(None)?;
+
+        let output = String::try_from(config)?;
+
+        assert_eq!(output, expected);
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_network_config_create_lo_2() -> Result<(), Error> {
+        let input = "#c1\n\n#c2\n\niface test inet manual\n";
+
+        let mut parser = NetworkParser::new(input.as_bytes());
+
+        let config = parser.parse_interfaces(None)?;
+
+        let output = String::try_from(config)?;
+
+        // Note: loopback should be added in front of other interfaces
+        let expected = "#c1\n#c2\n\nauto lo\niface lo inet loopback\n\niface test inet manual\n\n";
+        assert_eq!(output, expected);
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_network_config_parser_no_blank_1() -> Result<(), Error> {
+        let input = "auto lo\n\
+                     iface lo inet loopback\n\
+                     iface lo inet6 loopback\n\
+                     auto ens18\n\
+                     iface ens18 inet static\n\
+                     \taddress 192.168.20.144/20\n\
+                     \tgateway 192.168.16.1\n\
+                     # comment\n\
+                     iface ens20 inet static\n\
+                     \taddress 192.168.20.145/20\n\
+                     iface ens21 inet manual\n\
+                     iface ens22 inet manual\n";
+
+        let mut parser = NetworkParser::new(input.as_bytes());
+
+        let config = parser.parse_interfaces(None)?;
+
+        let output = String::try_from(config)?;
+
+        let expected = "auto lo\n\
+                        iface lo inet loopback\n\
+                        \n\
+                        iface lo inet6 loopback\n\
+                        \n\
+                        auto ens18\n\
+                        iface ens18 inet static\n\
+                        \taddress 192.168.20.144/20\n\
+                        \tgateway 192.168.16.1\n\
+                        #comment\n\
+                        \n\
+                        iface ens20 inet static\n\
+                        \taddress 192.168.20.145/20\n\
+                        \n\
+                        iface ens21 inet manual\n\
+                        \n\
+                        iface ens22 inet manual\n\
+                        \n";
+        assert_eq!(output, expected);
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_network_config_parser_no_blank_2() -> Result<(), Error> {
+        // Adapted from bug 2926
+        let input = "### Hetzner Online GmbH installimage\n\
+                     \n\
+                     source /etc/network/interfaces.d/*\n\
+                     \n\
+                     auto lo\n\
+                     iface lo inet loopback\n\
+                     iface lo inet6 loopback\n\
+                     \n\
+                     auto enp4s0\n\
+                     iface enp4s0 inet static\n\
+                     \taddress 10.10.10.10/24\n\
+                     \tgateway 10.10.10.1\n\
+                     \t# route 10.10.20.10/24 via 10.10.20.1\n\
+                     \tup route add -net 10.10.20.10 netmask 255.255.255.0 gw 10.10.20.1 dev enp4s0\n\
+                     \n\
+                     iface enp4s0 inet6 static\n\
+                     \taddress fe80::5496:35ff:fe99:5a6a/64\n\
+                     \tgateway fe80::1\n";
+
+        let mut parser = NetworkParser::new(input.as_bytes());
+
+        let config = parser.parse_interfaces(None)?;
+
+        let output = String::try_from(config)?;
+
+        let expected = "### Hetzner Online GmbH installimage\n\
+                        \n\
+                        source /etc/network/interfaces.d/*\n\
+                        \n\
+                        auto lo\n\
+                        iface lo inet loopback\n\
+                        \n\
+                        iface lo inet6 loopback\n\
+                        \n\
+                        auto enp4s0\n\
+                        iface enp4s0 inet static\n\
+                        \taddress 10.10.10.10/24\n\
+                        \tgateway 10.10.10.1\n\
+                        \t# route 10.10.20.10/24 via 10.10.20.1\n\
+                        \tup route add -net 10.10.20.10 netmask 255.255.255.0 gw 10.10.20.1 dev enp4s0\n\
+                        \n\
+                        iface enp4s0 inet6 static\n\
+                        \taddress fe80::5496:35ff:fe99:5a6a/64\n\
+                        \tgateway fe80::1\n\
+                        \n";
+        assert_eq!(output, expected);
+
+        Ok(())
+    }
+}
-- 
2.42.0

[pbs-devel] [PATCH proxmox-backup 02/10] tests: rudimentary NetworkConfig.write_config tests

[Stefan Lendl]

test simple manual and static configurations

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 pbs-config/src/network/mod.rs | 79 +++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/pbs-config/src/network/mod.rs b/pbs-config/src/network/mod.rs
index 21187ec2..7fec7e29 100644
--- a/pbs-config/src/network/mod.rs
+++ b/pbs-config/src/network/mod.rs
@@ -503,3 +503,82 @@ pub fn complete_port_list(arg: &str, _param: &HashMap<String, String>) -> Vec<St
         .map(|port| format!("{}{}", prefix, port))
         .collect()
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    use NetworkConfigMethod::*;
+    use NetworkInterfaceType::*;
+    use NetworkOrderEntry::*;
+
+    #[test]
+    fn test_write_network_config_manual() {
+        let iface_name = String::from("enp3s0");
+        let mut iface = Interface::new(iface_name.clone());
+        iface.interface_type = Eth;
+        iface.method = Some(Manual);
+        iface.active = true;
+
+        let nw_config = NetworkConfig {
+            interfaces: BTreeMap::from([(iface_name.clone(), iface)]),
+            order: vec![Iface(iface_name.clone())],
+        };
+
+        assert_eq!(
+            String::try_from(nw_config).unwrap().trim(),
+            r#"iface enp3s0 inet manual"#
+        );
+    }
+
+    #[test]
+    fn test_write_network_config_static() {
+        let iface_name = String::from("enp3s0");
+        let mut iface = Interface::new(iface_name.clone());
+        iface.interface_type = Eth;
+        iface.method = Some(Static);
+        iface.cidr = Some(String::from("10.0.0.100/16"));
+        iface.active = true;
+
+        let nw_config = NetworkConfig {
+            interfaces: BTreeMap::from([(iface_name.clone(), iface)]),
+            order: vec![Iface(iface_name.clone())],
+        };
+        assert_eq!(
+            String::try_from(nw_config).unwrap().trim(),
+            format!(
+                r#"
+iface enp3s0 inet static
+	address 10.0.0.100/16"#
+            )
+            .trim()
+        );
+    }
+
+    #[test]
+    fn test_write_network_config_static_with_gateway() {
+        let iface_name = String::from("enp3s0");
+        let mut iface = Interface::new(iface_name.clone());
+        iface.interface_type = Eth;
+        iface.method = Some(Static);
+        iface.cidr = Some(String::from("10.0.0.100/16"));
+        iface.gateway = Some(String::from("10.0.0.1"));
+        iface.active = true;
+
+        let nw_config = NetworkConfig {
+            interfaces: BTreeMap::from([(iface_name.clone(), iface)]),
+            order: vec![Iface(iface_name.clone())],
+        };
+        assert_eq!(
+            String::try_from(nw_config).unwrap().trim(),
+            format!(
+                r#"
+iface enp3s0 inet static
+	address 10.0.0.100/16
+	gateway 10.0.0.1"#
+            )
+            .trim()
+        );
+    }
+}
+
-- 
2.42.0

[pbs-devel] [PATCH proxmox-backup 03/10] config: write vlan network interface

[Stefan Lendl]

add vlan_id and vlan_raw_device to Interface struct
write them out if the interface type is Vlan
add several tests

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 pbs-api-types/src/network.rs  | 17 ++++++++
 pbs-config/src/network/mod.rs | 73 ++++++++++++++++++++++++++++++++++-
 2 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/pbs-api-types/src/network.rs b/pbs-api-types/src/network.rs
index e3a5e481..fe083dc6 100644
--- a/pbs-api-types/src/network.rs
+++ b/pbs-api-types/src/network.rs
@@ -224,6 +224,15 @@ pub const NETWORK_INTERFACE_LIST_SCHEMA: Schema =
             schema: NETWORK_INTERFACE_ARRAY_SCHEMA,
             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,
@@ -287,6 +296,12 @@ pub struct Interface {
     /// Enable bridge vlan support.
     #[serde(skip_serializing_if = "Option::is_none")]
     pub bridge_vlan_aware: Option<bool>,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    #[serde(rename = "vlan-id")]
+    pub vlan_id: Option<u16>,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    #[serde(rename = "vlan-raw-device")]
+    pub vlan_raw_device: Option<String>,
 
     #[serde(skip_serializing_if = "Option::is_none")]
     pub slaves: Option<Vec<String>>,
@@ -319,6 +334,8 @@ impl Interface {
             mtu: None,
             bridge_ports: None,
             bridge_vlan_aware: None,
+            vlan_id: None,
+            vlan_raw_device: None,
             slaves: None,
             bond_mode: None,
             bond_primary: None,
diff --git a/pbs-config/src/network/mod.rs b/pbs-config/src/network/mod.rs
index 7fec7e29..971c1ebe 100644
--- a/pbs-config/src/network/mod.rs
+++ b/pbs-config/src/network/mod.rs
@@ -79,6 +79,14 @@ fn write_iface_attributes(iface: &Interface, w: &mut dyn Write) -> Result<(), Er
                 writeln!(w, "\tbond-slaves {}", slaves.join(" "))?;
             }
         }
+        NetworkInterfaceType::Vlan => {
+            if let Some(vlan_id) = iface.vlan_id {
+                writeln!(w, "\tvlan-id {}", vlan_id)?;
+            }
+            if let Some(vlan_raw_device) = &iface.vlan_raw_device {
+                writeln!(w, "\tvlan-raw-device {}", vlan_raw_device)?;
+            }
+        }
         _ => {}
     }
 
@@ -580,5 +588,68 @@ iface enp3s0 inet static
             .trim()
         );
     }
-}
 
+    #[test]
+    fn test_write_network_config_vlan_id_in_name() {
+        let iface_name = String::from("vmbr0.100");
+        let mut iface = Interface::new(iface_name.clone());
+        iface.interface_type = Vlan;
+        iface.method = Some(Manual);
+        iface.active = true;
+
+        let nw_config = NetworkConfig {
+            interfaces: BTreeMap::from([(iface_name.clone(), iface)]),
+            order: vec![Iface(iface_name.clone())],
+        };
+        assert_eq!(
+            String::try_from(nw_config).unwrap().trim(),
+            "iface vmbr0.100 inet manual"
+        );
+    }
+
+    #[test]
+    fn test_write_network_config_vlan_with_raw_device() {
+        let iface_name = String::from("vlan100");
+        let mut iface = Interface::new(iface_name.clone());
+        iface.interface_type = Vlan;
+        iface.vlan_raw_device = Some(String::from("vmbr0"));
+        iface.method = Some(Manual);
+        iface.active = true;
+
+        let nw_config = NetworkConfig {
+            interfaces: BTreeMap::from([(iface_name.clone(), iface)]),
+            order: vec![Iface(iface_name.clone())],
+        };
+        assert_eq!(
+            String::try_from(nw_config).unwrap().trim(),
+            r#"
+iface vlan100 inet manual
+	vlan-raw-device vmbr0"#
+                .trim()
+        );
+    }
+
+    #[test]
+    fn test_write_network_config_vlan_with_individual_name() {
+        let iface_name = String::from("individual_name");
+        let mut iface = Interface::new(iface_name.clone());
+        iface.interface_type = Vlan;
+        iface.vlan_raw_device = Some(String::from("vmbr0"));
+        iface.vlan_id = Some(100);
+        iface.method = Some(Manual);
+        iface.active = true;
+
+        let nw_config = NetworkConfig {
+            interfaces: BTreeMap::from([(iface_name.clone(), iface)]),
+            order: vec![Iface(iface_name.clone())],
+        };
+        assert_eq!(
+            String::try_from(nw_config).unwrap().trim(),
+            r#"
+iface individual_name inet manual
+	vlan-id 100
+	vlan-raw-device vmbr0"#
+                .trim()
+        );
+    }
+}
-- 
2.42.0

[pbs-devel] [PATCH proxmox-backup 04/10] config: parse vlan interface from config

[Stefan Lendl]

Support three types of vlan configurations defined in interfaces
conforming to the PVE configurations:

iface nic.<vlan-id> inet

iface vlan<vlan-id> inet
	vlan-raw-device <nic>

iface <arbitraty-name> inet
	vlan-id <vlan-id>
	vlan-raw-device <nic>

* Add lexer Token enum variants for vlan-id and vlan-raw-device and parse
  them in parse_iface_attributes.
* Add tests to verify this works in the above scenarios

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 pbs-config/src/network/lexer.rs  |  6 ++
 pbs-config/src/network/parser.rs | 97 +++++++++++++++++++++++++++++++-
 2 files changed, 102 insertions(+), 1 deletion(-)

diff --git a/pbs-config/src/network/lexer.rs b/pbs-config/src/network/lexer.rs
index fd23e3d8..d0b7d8cd 100644
--- a/pbs-config/src/network/lexer.rs
+++ b/pbs-config/src/network/lexer.rs
@@ -24,6 +24,8 @@ pub enum Token {
     MTU,
     BridgePorts,
     BridgeVlanAware,
+    VlanId,
+    VlanRawDevice,
     BondSlaves,
     BondMode,
     BondPrimary,
@@ -50,6 +52,10 @@ lazy_static! {
         map.insert("bridge_ports", Token::BridgePorts);
         map.insert("bridge-vlan-aware", Token::BridgeVlanAware);
         map.insert("bridge_vlan_aware", Token::BridgeVlanAware);
+        map.insert("vlan-id", Token::VlanId);
+        map.insert("vlan_id", Token::VlanId);
+        map.insert("vlan-raw-device", Token::VlanRawDevice);
+        map.insert("vlan_raw_device", Token::VlanRawDevice);
         map.insert("bond-slaves", Token::BondSlaves);
         map.insert("bond_slaves", Token::BondSlaves);
         map.insert("bond-mode", Token::BondMode);
diff --git a/pbs-config/src/network/parser.rs b/pbs-config/src/network/parser.rs
index d31e5c2e..5a83e192 100644
--- a/pbs-config/src/network/parser.rs
+++ b/pbs-config/src/network/parser.rs
@@ -361,6 +361,20 @@ impl<R: BufRead> NetworkParser<R> {
                     interface.bond_xmit_hash_policy = Some(policy);
                     self.eat(Token::Newline)?;
                 }
+                Token::VlanId => {
+                    self.eat(Token::VlanId)?;
+                    let vlan_id = self.next_text()?.parse()?;
+                    interface.vlan_id = Some(vlan_id);
+                    set_interface_type(interface, NetworkInterfaceType::Vlan)?;
+                    self.eat(Token::Newline)?;
+                }
+                Token::VlanRawDevice => {
+                    self.eat(Token::VlanRawDevice)?;
+                    let vlan_raw_device = self.next_text()?;
+                    interface.vlan_raw_device = Some(vlan_raw_device);
+                    set_interface_type(interface, NetworkInterfaceType::Vlan)?;
+                    self.eat(Token::Newline)?;
+                }
                 _ => {
                     // parse addon attributes
                     let option = self.parse_to_eol()?;
@@ -522,7 +536,7 @@ 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+$").unwrap();
+            static ref VLAN_INTERFACE_REGEX: Regex = Regex::new(r"^\S+\.\d+|vlan\d+$").unwrap();
         }
 
         if let Some(existing_interfaces) = existing_interfaces {
@@ -748,4 +762,85 @@ mod test {
 
         Ok(())
     }
+
+    #[test]
+    fn test_network_config_parser_vlan_id_in_name() {
+        let input = "iface vmbr0.100 inet static manual";
+        let mut parser = NetworkParser::new(input.as_bytes());
+        let config = parser.parse_interfaces(None).unwrap();
+
+        let iface = config.interfaces.get("vmbr0.100").unwrap();
+        assert_eq!(iface.interface_type, NetworkInterfaceType::Vlan);
+        assert_eq!(iface.vlan_raw_device, None);
+        assert_eq!(iface.vlan_id, None);
+    }
+
+    #[test]
+    fn test_network_config_parser_vlan_with_raw_device() {
+        let input = r#"
+iface vlan100 inet manual
+	vlan-raw-device vmbr0"#;
+
+        let mut parser = NetworkParser::new(input.as_bytes());
+        let config = parser.parse_interfaces(None).unwrap();
+
+        let iface = config.interfaces.get("vlan100").unwrap();
+        assert_eq!(iface.interface_type, NetworkInterfaceType::Vlan);
+        assert_eq!(iface.vlan_raw_device, Some(String::from("vmbr0")));
+        assert_eq!(iface.vlan_id, None);
+    }
+
+    #[test]
+    fn test_network_config_parser_vlan_with_raw_device_static() {
+        let input = r#"
+iface vlan100 inet static
+	vlan-raw-device vmbr0
+	address 10.0.0.100/16"#;
+
+        let mut parser = NetworkParser::new(input.as_bytes());
+        let config = parser.parse_interfaces(None).unwrap();
+
+        let iface = config.interfaces.get("vlan100").unwrap();
+        assert_eq!(iface.interface_type, NetworkInterfaceType::Vlan);
+        assert_eq!(iface.vlan_raw_device, Some(String::from("vmbr0")));
+        assert_eq!(iface.vlan_id, None);
+        assert_eq!(iface.method, Some(NetworkConfigMethod::Static));
+        assert_eq!(iface.cidr, Some(String::from("10.0.0.100/16")));
+    }
+
+    #[test]
+    fn test_network_config_parser_vlan_individual_name() {
+        let input = r#"
+iface individual_name inet manual
+	vlan-id 100
+	vlan-raw-device vmbr0"#;
+
+        let mut parser = NetworkParser::new(input.as_bytes());
+        let config = parser.parse_interfaces(None).unwrap();
+
+        let iface = config.interfaces.get("individual_name").unwrap();
+        assert_eq!(iface.interface_type, NetworkInterfaceType::Vlan);
+        assert_eq!(iface.vlan_raw_device, Some(String::from("vmbr0")));
+        assert_eq!(iface.vlan_id, Some(100));
+    }
+
+    #[test]
+    fn test_network_config_parser_vlan_individual_name_static() {
+        let input = r#"
+iface individual_name inet static
+	vlan-id 100
+	vlan-raw-device vmbr0
+	address 10.0.0.100/16
+"#;
+
+        let mut parser = NetworkParser::new(input.as_bytes());
+        let config = parser.parse_interfaces(None).unwrap();
+
+        let iface = config.interfaces.get("individual_name").unwrap();
+        assert_eq!(iface.interface_type, NetworkInterfaceType::Vlan);
+        assert_eq!(iface.vlan_raw_device, Some(String::from("vmbr0")));
+        assert_eq!(iface.vlan_id, Some(100));
+        assert_eq!(iface.method, Some(NetworkConfigMethod::Static));
+        assert_eq!(iface.cidr, Some(String::from("10.0.0.100/16")));
+    }
 }
-- 
2.42.0

[pbs-devel] [PATCH proxmox-backup 05/10] config: remove unnecessary pub in various methods in NetworkConfig

[Stefan Lendl]

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 pbs-config/src/network/mod.rs    | 10 +++++-----
 pbs-config/src/network/parser.rs |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/pbs-config/src/network/mod.rs b/pbs-config/src/network/mod.rs
index 971c1ebe..9ecc66b8 100644
--- a/pbs-config/src/network/mod.rs
+++ b/pbs-config/src/network/mod.rs
@@ -251,7 +251,7 @@ impl NetworkConfig {
     }
 
     /// Check if ports are used only once
-    pub fn check_port_usage(&self) -> Result<(), Error> {
+    fn check_port_usage(&self) -> Result<(), Error> {
         let mut used_ports = HashMap::new();
         let mut check_port_usage = |iface, ports: &Vec<String>| {
             for port in ports.iter() {
@@ -280,7 +280,7 @@ impl NetworkConfig {
     }
 
     /// Check if child mtu is less or equal than parent mtu
-    pub fn check_mtu(&self, parent_name: &str, child_name: &str) -> Result<(), Error> {
+    fn check_mtu(&self, parent_name: &str, child_name: &str) -> Result<(), Error> {
         let parent = self
             .interfaces
             .get(parent_name)
@@ -320,7 +320,7 @@ impl NetworkConfig {
     }
 
     /// Check if bond slaves exists
-    pub fn check_bond_slaves(&self) -> Result<(), Error> {
+    fn check_bond_slaves(&self) -> Result<(), Error> {
         for (iface, interface) in self.interfaces.iter() {
             if let Some(slaves) = &interface.slaves {
                 for slave in slaves.iter() {
@@ -348,7 +348,7 @@ impl NetworkConfig {
     }
 
     /// Check if bridge ports exists
-    pub fn check_bridge_ports(&self) -> Result<(), Error> {
+    fn check_bridge_ports(&self) -> Result<(), Error> {
         lazy_static! {
             static ref VLAN_INTERFACE_REGEX: Regex = Regex::new(r"^(\S+)\.(\d+)$").unwrap();
         }
@@ -372,7 +372,7 @@ impl NetworkConfig {
         Ok(())
     }
 
-    pub fn write_config(&self, w: &mut dyn Write) -> Result<(), Error> {
+    fn write_config(&self, w: &mut dyn Write) -> Result<(), Error> {
         self.check_port_usage()?;
         self.check_bond_slaves()?;
         self.check_bridge_ports()?;
diff --git a/pbs-config/src/network/parser.rs b/pbs-config/src/network/parser.rs
index 5a83e192..0c178d9b 100644
--- a/pbs-config/src/network/parser.rs
+++ b/pbs-config/src/network/parser.rs
@@ -491,7 +491,7 @@ impl<R: BufRead> NetworkParser<R> {
             .map_err(|err| format_err!("line {}: {}", self.line_nr, err))
     }
 
-    pub fn _parse_interfaces(
+    fn _parse_interfaces(
         &mut self,
         existing_interfaces: Option<&HashMap<String, bool>>,
     ) -> Result<NetworkConfig, Error> {
-- 
2.42.0

[pbs-devel] [PATCH proxmox-backup 06/10] fmt: fix intendation in api macro

[Stefan Lendl]

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 src/api2/node/network.rs | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/api2/node/network.rs b/src/api2/node/network.rs
index ade6fe40..187b27a0 100644
--- a/src/api2/node/network.rs
+++ b/src/api2/node/network.rs
@@ -227,9 +227,9 @@ pub fn read_interface(iface: String) -> Result<Value, Error> {
                 optional: true,
             },
             bridge_vlan_aware: {
-	        description: "Enable bridge vlan support.",
-	        type: bool,
-	        optional: true,
+                description: "Enable bridge vlan support.",
+                type: bool,
+                optional: true,
             },
             bond_mode: {
                 type: LinuxBondMode,
@@ -503,9 +503,9 @@ pub enum DeletableProperty {
                 optional: true,
             },
             bridge_vlan_aware: {
-	        description: "Enable bridge vlan support.",
-	        type: bool,
-	        optional: true,
+                description: "Enable bridge vlan support.",
+                type: bool,
+                optional: true,
             },
             bond_mode: {
                 type: LinuxBondMode,
-- 
2.42.0

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

[Stefan Lendl]

* 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");
+    }
+
     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(())
-- 
2.42.0

[pbs-devel] [PATCH proxmox-backup 08/10] refactor(api): simplify setting interface properties

[Stefan Lendl]

Instead of using `if ..is_some()` directly assign the Option properties of
Interface from Option parameters of update_ and create_interface.
Code is shorter and cleaner to read.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 src/api2/node/network.rs | 54 +++++++++++-----------------------------
 1 file changed, 14 insertions(+), 40 deletions(-)

diff --git a/src/api2/node/network.rs b/src/api2/node/network.rs
index d1393103..84a017e9 100644
--- a/src/api2/node/network.rs
+++ b/src/api2/node/network.rs
@@ -301,25 +301,12 @@ pub fn create_interface(
 
     let mut interface = Interface::new(iface.clone());
     interface.interface_type = interface_type;
-
-    if let Some(autostart) = autostart {
-        interface.autostart = autostart;
-    }
-    if method.is_some() {
-        interface.method = method;
-    }
-    if method6.is_some() {
-        interface.method6 = method6;
-    }
-    if mtu.is_some() {
-        interface.mtu = mtu;
-    }
-    if comments.is_some() {
-        interface.comments = comments;
-    }
-    if comments6.is_some() {
-        interface.comments6 = comments6;
-    }
+    interface.autostart = autostart.unwrap_or(false);
+    interface.method = method;
+    interface.method6 = method6;
+    interface.mtu = mtu;
+    interface.comments = comments;
+    interface.comments6 = comments6;
 
     if let Some(cidr) = cidr {
         let (_, _, is_v6) = network::parse_cidr(&cidr)?;
@@ -697,25 +684,16 @@ pub fn update_interface(
         }
     }
 
-    if let Some(autostart) = autostart {
-        interface.autostart = autostart;
-    }
-    if method.is_some() {
-        interface.method = method;
-    }
-    if method6.is_some() {
-        interface.method6 = method6;
-    }
-    if mtu.is_some() {
-        interface.mtu = mtu;
-    }
+    interface.autostart = autostart.unwrap_or(false);
+    interface.method = method;
+    interface.method6 = method6;
+    interface.mtu = mtu;
+    interface.bridge_vlan_aware = bridge_vlan_aware;
+
     if let Some(ports) = bridge_ports {
         let ports = split_interface_list(&ports)?;
         set_bridge_ports(interface, ports)?;
     }
-    if bridge_vlan_aware.is_some() {
-        interface.bridge_vlan_aware = bridge_vlan_aware;
-    }
     if let Some(slaves) = slaves {
         let slaves = split_interface_list(&slaves)?;
         set_bond_slaves(interface, slaves)?;
@@ -768,12 +746,8 @@ pub fn update_interface(
         interface.gateway6 = Some(gateway6);
     }
 
-    if comments.is_some() {
-        interface.comments = comments;
-    }
-    if comments6.is_some() {
-        interface.comments6 = comments6;
-    }
+    interface.comments = comments;
+    interface.comments6 = comments6;
 
     if interface.cidr.is_some() || interface.gateway.is_some() {
         interface.method = Some(NetworkConfigMethod::Static);
-- 
2.42.0

[pbs-devel] [PATCH proxmox-backup 09/10] ui: enable vlan widget

[Stefan Lendl]

requires update the widget-toolkit to include proxmoxvlanfield.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 www/SystemConfiguration.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/www/SystemConfiguration.js b/www/SystemConfiguration.js
index 860d85c0..33a4ed2e 100644
--- a/www/SystemConfiguration.js
+++ b/www/SystemConfiguration.js
@@ -40,7 +40,7 @@ Ext.define('PBS.SystemConfiguration', {
 		    flex: 1,
 		    minHeight: 200,
 		    showApplyBtn: true,
-		    types: ['bond', 'bridge'],
+		    types: ['bond', 'bridge', 'vlan'],
 		    nodename: 'localhost',
 		},
 	    ],
-- 
2.42.0

[pbs-devel] [PATCH widget-toolkit 10/10] form: include VlanField from PVE

[Stefan Lendl]

copied from PVE

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 src/Makefile            |  1 +
 src/form/VlanField.js   | 40 ++++++++++++++++++++++++++++++++++++++++
 src/node/NetworkEdit.js |  6 +++---
 3 files changed, 44 insertions(+), 3 deletions(-)
 create mode 100644 src/form/VlanField.js

diff --git a/src/Makefile b/src/Makefile
index 01145b1..6c65763 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -31,6 +31,7 @@ JSSRC=					\
 	form/ExpireDate.js		\
 	form/IntegerField.js		\
 	form/TextField.js		\
+	form/VlanField.js		\
 	form/DateTimeField.js		\
 	form/Checkbox.js		\
 	form/KVComboBox.js		\
diff --git a/src/form/VlanField.js b/src/form/VlanField.js
new file mode 100644
index 0000000..c30fa72
--- /dev/null
+++ b/src/form/VlanField.js
@@ -0,0 +1,40 @@
+Ext.define('Proxmox.form.field.VlanField', {
+    extend: 'Ext.form.field.Number',
+    alias: ['widget.proxmoxvlanfield'],
+
+    deleteEmpty: false,
+
+    emptyText: gettext('no VLAN'),
+
+    fieldLabel: gettext('VLAN Tag'),
+
+    allowBlank: true,
+
+    getSubmitData: function() {
+        var me = this,
+            data = null,
+            val;
+        if (!me.disabled && me.submitValue) {
+            val = me.getSubmitValue();
+            if (val) {
+                data = {};
+                data[me.getName()] = val;
+            } else if (me.deleteEmpty) {
+		data = {};
+                data.delete = me.getName();
+	    }
+        }
+        return data;
+    },
+
+    initComponent: function() {
+	var me = this;
+
+	Ext.apply(me, {
+	    minValue: 1,
+	    maxValue: 4094,
+	});
+
+	me.callParent();
+    },
+});
diff --git a/src/node/NetworkEdit.js b/src/node/NetworkEdit.js
index bb9add3..b81a21d 100644
--- a/src/node/NetworkEdit.js
+++ b/src/node/NetworkEdit.js
@@ -97,7 +97,7 @@ Ext.define('Proxmox.node.NetworkEdit', {
 		name: 'ovs_bridge',
 	    });
 	    column2.push({
-		xtype: 'pveVlanField',
+		xtype: 'proxmoxvlanfield',
 		deleteEmpty: !me.isCreate,
 		name: 'ovs_tag',
 		value: '',
@@ -140,7 +140,7 @@ Ext.define('Proxmox.node.NetworkEdit', {
 	    });
 
 	    column2.push({
-		xtype: 'pveVlanField',
+		xtype: 'proxmoxvlanfield',
 		name: 'vlan-id',
 		value: me.vlanidvalue,
 		disabled: me.disablevlanid,
@@ -211,7 +211,7 @@ Ext.define('Proxmox.node.NetworkEdit', {
 		name: 'ovs_bridge',
 	    });
 	    column2.push({
-		xtype: 'pveVlanField',
+		xtype: 'proxmoxvlanfield',
 		deleteEmpty: !me.isCreate,
 		name: 'ovs_tag',
 		value: '',
-- 
2.42.0

Re: [pbs-devel] [PATCH widget-toolkit 10/10] form: include VlanField from PVE

[Lukas Wagner]

Not a full review, just something that I spotted ;)

See comment inline.

On 1/11/24 16:53, Stefan Lendl wrote:
> copied from PVE
> 
> Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
> ---
>   src/Makefile            |  1 +
>   src/form/VlanField.js   | 40 ++++++++++++++++++++++++++++++++++++++++
>   src/node/NetworkEdit.js |  6 +++---
>   3 files changed, 44 insertions(+), 3 deletions(-)
>   create mode 100644 src/form/VlanField.js
> 
> diff --git a/src/Makefile b/src/Makefile
> index 01145b1..6c65763 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -31,6 +31,7 @@ JSSRC=					\
>   	form/ExpireDate.js		\
>   	form/IntegerField.js		\
>   	form/TextField.js		\
> +	form/VlanField.js		\
>   	form/DateTimeField.js		\
>   	form/Checkbox.js		\
>   	form/KVComboBox.js		\
> diff --git a/src/form/VlanField.js b/src/form/VlanField.js
> new file mode 100644
> index 0000000..c30fa72
> --- /dev/null
> +++ b/src/form/VlanField.js
> @@ -0,0 +1,40 @@
> +Ext.define('Proxmox.form.field.VlanField', {
> +    extend: 'Ext.form.field.Number',
> +    alias: ['widget.proxmoxvlanfield'],
> +
> +    deleteEmpty: false,
> +
> +    emptyText: gettext('no VLAN'),
> +
> +    fieldLabel: gettext('VLAN Tag'),
> +
> +    allowBlank: true,
> +
> +    getSubmitData: function() {
> +        var me = this,
> +            data = null,
> +            val;
> +        if (!me.disabled && me.submitValue) {
> +            val = me.getSubmitValue();
> +            if (val) {
> +                data = {};
> +                data[me.getName()] = val;
> +            } else if (me.deleteEmpty) {
> +		data = {};
> +                data.delete = me.getName();
> +	    }
> +        }
> +        return data;
> +    },

Indentation seems to be off in this hunk (spaces instead of tabs+spaces)

> +
> +    initComponent: function() {
> +	var me = this;
> +
> +	Ext.apply(me, {
> +	    minValue: 1,
> +	    maxValue: 4094,
> +	});
> +
> +	me.callParent();
> +    },
> +});
> diff --git a/src/node/NetworkEdit.js b/src/node/NetworkEdit.js
> index bb9add3..b81a21d 100644
> --- a/src/node/NetworkEdit.js
> +++ b/src/node/NetworkEdit.js
> @@ -97,7 +97,7 @@ Ext.define('Proxmox.node.NetworkEdit', {
>   		name: 'ovs_bridge',
>   	    });
>   	    column2.push({
> -		xtype: 'pveVlanField',
> +		xtype: 'proxmoxvlanfield',
>   		deleteEmpty: !me.isCreate,
>   		name: 'ovs_tag',
>   		value: '',
> @@ -140,7 +140,7 @@ Ext.define('Proxmox.node.NetworkEdit', {
>   	    });
>   
>   	    column2.push({
> -		xtype: 'pveVlanField',
> +		xtype: 'proxmoxvlanfield',
>   		name: 'vlan-id',
>   		value: me.vlanidvalue,
>   		disabled: me.disablevlanid,
> @@ -211,7 +211,7 @@ Ext.define('Proxmox.node.NetworkEdit', {
>   		name: 'ovs_bridge',
>   	    });
>   	    column2.push({
> -		xtype: 'pveVlanField',
> +		xtype: 'proxmoxvlanfield',
>   		deleteEmpty: !me.isCreate,
>   		name: 'ovs_tag',
>   		value: '',

-- 
- Lukas

Re: [pbs-devel] [PATCH widget-toolkit/proxmox-backup 00/10] Fix #3115: VLAN Network Interface Configuration

[Lukas Wagner]

On 1/11/24 16:52, Stefan Lendl wrote:
> This patch series allows configuration of VLAN network interfaces in the PBS GUI
> 
> * Adds reading and writing of vlan network interface configs from network/interfaces
> * Creating and updating of VLAN network interfaces via the API
> * Copy the VlanId widget from PVE and enable the Linux VLAN interface type in the GUI
> * Unit tests
> * Some cleanup to leave touched functions cleaner than before
> 

Gave these changes a try. Mostly works fine, however there is a small 
bug that prohibits updating existing non-VLAN devices - see other 
replies for details.

I liked how approached testing for these changes, e.g. by adding tests 
for the existing code first to make sure that your following changes do 
not break anything. That's always a good idea!

-- 
- Lukas

Re: [pbs-devel] [PATCH proxmox-backup 03/10] config: write vlan network interface

[Lukas Wagner]

On 1/11/24 16:52, Stefan Lendl wrote:
> add vlan_id and vlan_raw_device to Interface struct
> write them out if the interface type is Vlan
> add several tests
> 
> Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
> ---
>   pbs-api-types/src/network.rs  | 17 ++++++++
>   pbs-config/src/network/mod.rs | 73 ++++++++++++++++++++++++++++++++++-
>   2 files changed, 89 insertions(+), 1 deletion(-)
> 
> diff --git a/pbs-api-types/src/network.rs b/pbs-api-types/src/network.rs
> index e3a5e481..fe083dc6 100644
> --- a/pbs-api-types/src/network.rs
> +++ b/pbs-api-types/src/network.rs
> @@ -224,6 +224,15 @@ pub const NETWORK_INTERFACE_LIST_SCHEMA: Schema =
>               schema: NETWORK_INTERFACE_ARRAY_SCHEMA,
>               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,
> @@ -287,6 +296,12 @@ pub struct Interface {
>       /// Enable bridge vlan support.
>       #[serde(skip_serializing_if = "Option::is_none")]
>       pub bridge_vlan_aware: Option<bool>,
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    #[serde(rename = "vlan-id")]
> +    pub vlan_id: Option<u16>,
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    #[serde(rename = "vlan-raw-device")]
> +    pub vlan_raw_device: Option<String>,
>   
>       #[serde(skip_serializing_if = "Option::is_none")]
>       pub slaves: Option<Vec<String>>,
> @@ -319,6 +334,8 @@ impl Interface {
>               mtu: None,
>               bridge_ports: None,
>               bridge_vlan_aware: None,
> +            vlan_id: None,
> +            vlan_raw_device: None,
>               slaves: None,
>               bond_mode: None,
>               bond_primary: None,
> diff --git a/pbs-config/src/network/mod.rs b/pbs-config/src/network/mod.rs
> index 7fec7e29..971c1ebe 100644
> --- a/pbs-config/src/network/mod.rs
> +++ b/pbs-config/src/network/mod.rs
> @@ -79,6 +79,14 @@ fn write_iface_attributes(iface: &Interface, w: &mut dyn Write) -> Result<(), Er
>                   writeln!(w, "\tbond-slaves {}", slaves.join(" "))?;
>               }
>           }
> +        NetworkInterfaceType::Vlan => {
> +            if let Some(vlan_id) = iface.vlan_id {
> +                writeln!(w, "\tvlan-id {}", vlan_id)?;
> +            }
> +            if let Some(vlan_raw_device) = &iface.vlan_raw_device {
> +                writeln!(w, "\tvlan-raw-device {}", vlan_raw_device)?;

Mini-nit: You can inline the string formatting: {vlan_raw_device}

> +            }
> +        }
>           _ => {}
>       }
>   
> @@ -580,5 +588,68 @@ iface enp3s0 inet static
>               .trim()
>           );
>       }
> -}
>   
> +    #[test]
> +    fn test_write_network_config_vlan_id_in_name() {
> +        let iface_name = String::from("vmbr0.100");
> +        let mut iface = Interface::new(iface_name.clone());
> +        iface.interface_type = Vlan;
> +        iface.method = Some(Manual);
> +        iface.active = true;
> +
> +        let nw_config = NetworkConfig {
> +            interfaces: BTreeMap::from([(iface_name.clone(), iface)]),
> +            order: vec![Iface(iface_name.clone())],
> +        };
> +        assert_eq!(
> +            String::try_from(nw_config).unwrap().trim(),
> +            "iface vmbr0.100 inet manual"
> +        );
> +    }
> +
> +    #[test]
> +    fn test_write_network_config_vlan_with_raw_device() {
> +        let iface_name = String::from("vlan100");
> +        let mut iface = Interface::new(iface_name.clone());
> +        iface.interface_type = Vlan;
> +        iface.vlan_raw_device = Some(String::from("vmbr0"));
> +        iface.method = Some(Manual);
> +        iface.active = true;
> +
> +        let nw_config = NetworkConfig {
> +            interfaces: BTreeMap::from([(iface_name.clone(), iface)]),
> +            order: vec![Iface(iface_name.clone())],
> +        };
> +        assert_eq!(
> +            String::try_from(nw_config).unwrap().trim(),
> +            r#"
> +iface vlan100 inet manual
> +	vlan-raw-device vmbr0"#
> +                .trim()
> +        );
> +    }
> +
> +    #[test]
> +    fn test_write_network_config_vlan_with_individual_name() {
> +        let iface_name = String::from("individual_name");
> +        let mut iface = Interface::new(iface_name.clone());
> +        iface.interface_type = Vlan;
> +        iface.vlan_raw_device = Some(String::from("vmbr0"));
> +        iface.vlan_id = Some(100);
> +        iface.method = Some(Manual);
> +        iface.active = true;
> +
> +        let nw_config = NetworkConfig {
> +            interfaces: BTreeMap::from([(iface_name.clone(), iface)]),
> +            order: vec![Iface(iface_name.clone())],
> +        };
> +        assert_eq!(
> +            String::try_from(nw_config).unwrap().trim(),
> +            r#"
> +iface individual_name inet manual
> +	vlan-id 100
> +	vlan-raw-device vmbr0"#
> +                .trim()
> +        );
> +    }
> +}

-- 
- Lukas

Re: [pbs-devel] [PATCH proxmox-backup 05/10] config: remove unnecessary pub in various methods in NetworkConfig

[Lukas Wagner]

On 1/11/24 16:53, Stefan Lendl wrote:
> Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
> ---
>   pbs-config/src/network/mod.rs    | 10 +++++-----
>   pbs-config/src/network/parser.rs |  2 +-
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/pbs-config/src/network/mod.rs b/pbs-config/src/network/mod.rs
> index 971c1ebe..9ecc66b8 100644
> --- a/pbs-config/src/network/mod.rs
> +++ b/pbs-config/src/network/mod.rs
> @@ -251,7 +251,7 @@ impl NetworkConfig {
>       }
>   
>       /// Check if ports are used only once
> -    pub fn check_port_usage(&self) -> Result<(), Error> {
> +    fn check_port_usage(&self) -> Result<(), Error> {
>           let mut used_ports = HashMap::new();
>           let mut check_port_usage = |iface, ports: &Vec<String>| {
>               for port in ports.iter() {
> @@ -280,7 +280,7 @@ impl NetworkConfig {
>       }
>   
>       /// Check if child mtu is less or equal than parent mtu
> -    pub fn check_mtu(&self, parent_name: &str, child_name: &str) -> Result<(), Error> {
> +    fn check_mtu(&self, parent_name: &str, child_name: &str) -> Result<(), Error> {
>           let parent = self
>               .interfaces
>               .get(parent_name)
> @@ -320,7 +320,7 @@ impl NetworkConfig {
>       }
>   
>       /// Check if bond slaves exists
> -    pub fn check_bond_slaves(&self) -> Result<(), Error> {
> +    fn check_bond_slaves(&self) -> Result<(), Error> {
>           for (iface, interface) in self.interfaces.iter() {
>               if let Some(slaves) = &interface.slaves {
>                   for slave in slaves.iter() {
> @@ -348,7 +348,7 @@ impl NetworkConfig {
>       }
>   
>       /// Check if bridge ports exists
> -    pub fn check_bridge_ports(&self) -> Result<(), Error> {
> +    fn check_bridge_ports(&self) -> Result<(), Error> {
>           lazy_static! {
>               static ref VLAN_INTERFACE_REGEX: Regex = Regex::new(r"^(\S+)\.(\d+)$").unwrap();
>           }
> @@ -372,7 +372,7 @@ impl NetworkConfig {
>           Ok(())
>       }
>   
> -    pub fn write_config(&self, w: &mut dyn Write) -> Result<(), Error> {
> +    fn write_config(&self, w: &mut dyn Write) -> Result<(), Error> { >           self.check_port_usage()?;
>           self.check_bond_slaves()?;
>           self.check_bridge_ports()?;
> diff --git a/pbs-config/src/network/parser.rs b/pbs-config/src/network/parser.rs
> index 5a83e192..0c178d9b 100644
> --- a/pbs-config/src/network/parser.rs
> +++ b/pbs-config/src/network/parser.rs
> @@ -491,7 +491,7 @@ impl<R: BufRead> NetworkParser<R> {
>               .map_err(|err| format_err!("line {}: {}", self.line_nr, err))
>       }
>   
> -    pub fn _parse_interfaces(
> +    fn _parse_interfaces(

Maybe use this opportunity to name this method in a more idiomatic way?
The _ prefix is not really common in Rust ;)

>           &mut self,
>           existing_interfaces: Option<&HashMap<String, bool>>,
>       ) -> Result<NetworkConfig, Error> {

-- 
- Lukas

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

[Lukas Wagner]

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

Re: [pbs-devel] [PATCH proxmox-backup 08/10] refactor(api): simplify setting interface properties

[Lukas Wagner]

On 1/11/24 16:53, Stefan Lendl wrote:
> Instead of using `if ..is_some()` directly assign the Option properties of
> Interface from Option parameters of update_ and create_interface.
> Code is shorter and cleaner to read.
> 
> Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
> ---
>   src/api2/node/network.rs | 54 +++++++++++-----------------------------
>   1 file changed, 14 insertions(+), 40 deletions(-)
> 
> diff --git a/src/api2/node/network.rs b/src/api2/node/network.rs
> index d1393103..84a017e9 100644
> --- a/src/api2/node/network.rs
> +++ b/src/api2/node/network.rs
> @@ -301,25 +301,12 @@ pub fn create_interface(
>   
>       let mut interface = Interface::new(iface.clone());
>       interface.interface_type = interface_type;
> -
> -    if let Some(autostart) = autostart {
> -        interface.autostart = autostart;
> -    }
> -    if method.is_some() {
> -        interface.method = method;
> -    }
> -    if method6.is_some() {
> -        interface.method6 = method6;
> -    }
> -    if mtu.is_some() {
> -        interface.mtu = mtu;
> -    }
> -    if comments.is_some() {
> -        interface.comments = comments;
> -    }
> -    if comments6.is_some() {
> -        interface.comments6 = comments6;
> -    }
> +    interface.autostart = autostart.unwrap_or(false);
> +    interface.method = method;
> +    interface.method6 = method6;
> +    interface.mtu = mtu;
> +    interface.comments = comments;
> +    interface.comments6 = comments6;
>   
>       if let Some(cidr) = cidr {
>           let (_, _, is_v6) = network::parse_cidr(&cidr)?;
> @@ -697,25 +684,16 @@ pub fn update_interface(
>           }
>       }
>   
> -    if let Some(autostart) = autostart {
> -        interface.autostart = autostart;
> -    }
> -    if method.is_some() {
> -        interface.method = method;
> -    }
> -    if method6.is_some() {
> -        interface.method6 = method6;
> -    }
> -    if mtu.is_some() {
> -        interface.mtu = mtu;
> -    }
> +    interface.autostart = autostart.unwrap_or(false);
> +    interface.method = method;
> +    interface.method6 = method6;
> +    interface.mtu = mtu;
> +    interface.bridge_vlan_aware = bridge_vlan_aware;
> +

In general our PUT handlers do not require the caller to send at fields, 
but only those that have changed. Fields that should be cleared are 
represented by the 'deleted' field.

This means the old code is actually necessary: If a field is not 
submitted, it should *not* be changed. Only if a new value is sent, the 
field should be updated.

Your change happens to work from the UI, since the network dialog 
happens to always send all fields - which is not that AFAIK not that 
common for our UI.
Your change however breaks the CLI tool:
For example, set a comment for an interface and then do a

   proxmox-backup-manager network update enp6s18.10 --autostart false

You will notice that comment is cleared after that command :)


>       if let Some(ports) = bridge_ports {
>           let ports = split_interface_list(&ports)?;
>           set_bridge_ports(interface, ports)?;
>       }
> -    if bridge_vlan_aware.is_some() {
> -        interface.bridge_vlan_aware = bridge_vlan_aware;
> -    }
>       if let Some(slaves) = slaves {
>           let slaves = split_interface_list(&slaves)?;
>           set_bond_slaves(interface, slaves)?;
> @@ -768,12 +746,8 @@ pub fn update_interface(
>           interface.gateway6 = Some(gateway6);
>       }
>   
> -    if comments.is_some() {
> -        interface.comments = comments;
> -    }
> -    if comments6.is_some() {
> -        interface.comments6 = comments6;
> -    }
> +    interface.comments = comments;
> +    interface.comments6 = comments6;
>   
>       if interface.cidr.is_some() || interface.gateway.is_some() {
>           interface.method = Some(NetworkConfigMethod::Static);

-- 
- Lukas

Re: [pbs-devel] [PATCH widget-toolkit/proxmox-backup 00/10] Fix #3115: VLAN Network Interface Configuration

[Stefan Lendl]

sent v2